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

[照山周一郎]

Thank you for the very clear explanation.
I now understand that this issue should be feedback to the mainline as
well as OpenWRT.

However, I am ashamed to say that my technical skills are not useful
enough to contribute to the improvement of the mainline, so it would
be very helpful if you could take over if possible.

2021年11月29日(月) 21:18 Bjørn Mork <bjorn at mork.no>:
>
> 照山周一郎 <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



-- 
株式会社スプリングボード
照山　周一郎
teruyama at springboard-inc.jp
http://www.springboard-inc.jp/
〒110-0005
東京都台東区上野3丁目2番2号
アイオス秋葉原505号室