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

[Bjørn Mork]

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

> Thanks for the advice.
>
> This is a fix for a Japanese NTT OCU, which always outputs 2W, even
> though the chip revision always returns 0. This is not a bug, but a
> specification.
> https://business.ntt-east.co.jp/service/onu/pdf/interface.pdf
>
> Therefore, since the situation is too special to be handled by
> sfp_module_parse_power(), the existing code was implemented to exit
> the process in the process just before the error occurs.

It would help understanding if you included the actual error, and maybe
an eeprom dump.  Something like this (on a patched host where the module
is supported, of course...):

  ethtool -m ethX raw on | hexdump -C

> The SF-8472 does not normally return 0, so there is no effect on other devices.
> https://members.snia.org/document/dl/25916

I see.  This makes sense. I wonder if this is actually a bug in mainline
introduced by commit 7cfa9c92d0a3 ("net: sfp: avoid power switch on
address-change modules")?

See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/sfp.c?id=7cfa9c92d0a325f97ac13f894a7b47d37bd2040

If I read that correctly, then the original behaviour was the way you
want it: We assumed that a module mathcing

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

already was in the indicated power mode and skipped the EXT_STATUS
register update.  But the patch moved most of that logic inside a

 if (power_mW > sfp->max_power_mW) {}

block.  Which I believe is a bug. We're now failing in case the host
supports higher power but the module doesn't support DOM.

If you think my analysis is correct, then you could you try to revert
that patch to verify that it fixes your problem.  There is a trivial
string conflict but otherwise it reverts just fine.

Is verified then a proper fix should go upstream.

> If possible, I would appreciate some practical advice on compile-time warnings.

No warnings before patching:

bjorn at miraculix:/usr/local/src/git/linux$ make -C /lib/modules/$(uname -r)/build M=$(pwd)/drivers/net/phy sfp.ko
make: Entering directory '/usr/src/linux-headers-5.10.0-9-amd64'
  CC [M]  /usr/local/src/git/linux/drivers/net/phy/sfp.o
  MODPOST /usr/local/src/git/linux/drivers/net/phy/Module.symvers
  CC [M]  /usr/local/src/git/linux/drivers/net/phy/sfp.mod.o
  LD [M]  /usr/local/src/git/linux/drivers/net/phy/sfp.ko
make: Leaving directory '/usr/src/linux-headers-5.10.0-9-amd64'

Apply your patch:

bjorn at miraculix:/usr/local/src/git/linux$ patch -p1 < /tmp/771-net-sfp-skip-hpowr-if-no-revision.patch 
patching file drivers/net/phy/sfp.c
Hunk #1 succeeded at 1634 (offset 44 lines).


Verify it:

bjorn at miraculix:/usr/local/src/git/linux$ git diff
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 32c34c728c7a..4099752dbcd0 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1634,6 +1634,8 @@ static int sfp_module_parse_power(struct sfp *sfp)
 
 static int sfp_sm_mod_hpower(struct sfp *sfp, bool enable)
 {
+       if (sfp->id.ext.sff8472_compliance == SFP_SFF8472_COMPLIANCE_NONE)
+               return 0;
        u8 val;
        int err;


Build again and notice a new warning:

bjorn at miraculix:/usr/local/src/git/linux$ make -C /lib/modules/$(uname -r)/build M=$(pwd)/drivers/net/phy sfp.ko
make: Entering directory '/usr/src/linux-headers-5.10.0-9-amd64'
  CC [M]  /usr/local/src/git/linux/drivers/net/phy/sfp.o
/usr/local/src/git/linux/drivers/net/phy/sfp.c: In function ‘sfp_sm_mod_hpower’:
/usr/local/src/git/linux/drivers/net/phy/sfp.c:1639:2: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
 1639 |  u8 val;
      |  ^~
  MODPOST /usr/local/src/git/linux/drivers/net/phy/Module.symvers
  CC [M]  /usr/local/src/git/linux/drivers/net/phy/sfp.mod.o
  LD [M]  /usr/local/src/git/linux/drivers/net/phy/sfp.ko
make: Leaving directory '/usr/src/linux-headers-5.10.0-9-amd64'





Bjørn