[PATCH] kernal: skip hpower setting for the module which has no revs

[Bjørn Mork]

照山周一郎 <teruyama at springboard-inc.jp> writes:

> Thanks for all the helpful advice.
>
> The EEPROM dump is as follows.
>
> ーーー
>
> Offset Values
> ---— ———
> 0x0000: 03 04 01 00 00 00 80 00 00 00 00 01 0d 00 0a 64
> 0x0010: 00 00 00 00 4e 54 54 20 20 20 20 20 20 20 20 20
> 0x0020: 20 20 20 20 00 00 00 00 30 30 30 30 30 30 30 30
> 0x0030: 30 30 30 30 30 30 30 30 30 30 30 30 05 1e 00 7d
> 0x0040: 02 00 00 00 30 30 30 30 30 30 30 30 30 30 30 30

So id.ext.options is 0x0200, meaning that SFP_OPTIONS_POWER_DECL is set
and nothing else.  

> 0x0050: 30 30 30 30 31 36 30 34 30 38 20 20 00 00 00 75

And both id.ext.diagmon and id.ext.sff8472_compliance are 0x00.  Not
sure why they'd want that with such a new module, but I guess it's
fine..

Anyway, this means that your module would have matched the old

	if (sfp->id.ext.sff8472_compliance == SFP_SFF8472_COMPLIANCE_NONE &&
	    (sfp->id.ext.diagmon & (SFP_DIAGMON_DDM | SFP_DIAGMON_ADDRMODE)) !=
	    SFP_DIAGMON_DDM) {

test and therefore have been accepted by the driver.


> 0x0060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x0090: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 0x00f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> ーーー
>
> The log is as follows.
>
> ーーー
>
> [    8.583797] kmodloader: done loading kernel modules from /etc/modules.d/*
> [    8.834704] sfp sfp-eth1: module OEM              10G-SFP-PCU1M
> rev G    sn HS18100810001    dc 181008
> [    8.856051] sfp sfp-eth3: module NTT              0000000000000000
> rev 0000 sn 0000000000000000 dc 160408
> [    8.865843] mvpp2 f4000000.ethernet eth3: switched to
> inband/1000base-x link mode
> [    8.873469] sfp sfp-eth3: Failed to read EEPROM: -5
> [    8.983251] sfp sfp-eth3: Failed to read EEPROM: -5
> [    9.103250] sfp sfp-eth3: Failed to read EEPROM: -5
> ーーー
>
> "Failed to read EEPROM: -5"  output in a loop.

Thanks.  Just wanted to be sure that even read access to EXT_STATUS
failed.  That's as expected when SFP_DIAGMON_DDM is not set.

The loop makes the problem even worse.  It's not like this is going to
fix itself.  But I guess it makes sense for the temporary access
problems this was intended to catch.


> Its caused here.
>
> https://github.com/torvalds/linux/blob/master/drivers/net/phy/sfp.c#L1694
>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/sfp.c?id=7cfa9c92d0a325f97ac13f894a7b47d37bd2040
>> if (power_mW > sfp->max_power_mW) {
>
> I think the above modification is wrong too.
>
> However, I don't have the SFP module that returns the appropriate
> revision, and I don't have the environment to verify it, so I can't
> fix the above patch myself.

Well.  I have a few modules with revisions 0x00, 0x01, 0x03 but none of
them set any of the high power flags.  So I can't really verify this
either.  Still, I think we understand the issue well enough now to make
an attempt.  Review by netdev, including a CC to the original author,
should be plenty enough for a fix.  It was enough to introduce the bug
:-)

> The NTT OCU that I want to run this time is widely used in Japan, so I
> would like to apply the patch with the method I applied.

Even more reason to fix this properly in mainline + stable and not just
in OpenWrt.  I can make an attempt with you as reporter, if you don't
want to submit this yourself?

I think the bug you found here might affect many other high power
modules too.  Any high power module without diagnostics will fail
AFAICS.

It's a great catch.  Thanks!


Bjørn