netdev - Re: [PATCH net,stable] phy: sfp: fix high power modules without diag mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YaoFkZ53m7cILdYu@shell.armlinux.org.uk>
Date:   Fri, 3 Dec 2021 11:54:57 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Bjørn Mork <bjorn@...k.no>,
        netdev@...r.kernel.org,
        照山周一郎 <teruyama@...ingboard-inc.jp>
Subject: Re: [PATCH net,stable] phy: sfp: fix high power modules without diag
 mode

On Thu, Dec 02, 2021 at 05:58:43PM -0800, Jakub Kicinski wrote:
> On Tue, 30 Nov 2021 08:39:29 +0100 Bjørn Mork wrote:
> > Commit 7cfa9c92d0a3 ("net: sfp: avoid power switch on address-change
> > modules") changed semantics for high power modules without diag mode.
> > We repeatedly try to read the current power status from the non-existing
> > 0xa2 address, in the futile hope this failure is temporary:
> > 
> > [    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
> > 
> > Eeprom dump:
> > 
> > 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
> > 0x0050: 30 30 30 30 31 36 30 34 30 38 20 20 00 00 00 75
> > 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
> > 
> > Previously we assumed such modules were powered up in the correct
> > mode, continuing without further configuration as long as the
> > required power class was supported by the host.
> > 
> > Revert to that behaviour, refactoring to keep the improved
> > diagnostic messages.
> > 
> > Fixes: 7cfa9c92d0a3 ("net: sfp: avoid power switch on address-change modules")
> > Reported-and-tested-by: 照山周一郎 <teruyama@...ingboard-inc.jp>
> > Cc: Russell King <rmk+kernel@...linux.org.uk>
> > Signed-off-by: Bjørn Mork <bjorn@...k.no>
> 
> Russell, any comments?

Sorry for the delay, I've been out over the last couple of days. I 
hink it's fine, but the code here is not easy to understand, hence
why this subtlety was missed. So, I'm not entirely happy about going
back to the original code.

Maybe instead doing a check in sfp_sm_mod_hpower() for this would
be better? Possibly something like:

static int sfp_module_parse_power(struct sfp *sfp)
{
	u32 power_mW = 1000;
+	bool supports_a2;

	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_POWER_DECL))
		power_mW = 1500;
	if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_HIGH_POWER_LEVEL))
		power_mW = 2000;

+	supports_a2 = sfp->id.ext.sff8472_compliance !=
+				SFP_SFF8472_COMPLIANCE_NONE ||
+			sfp->id.ext.diagmon & SFP_DIAGMON_DDM;

	if (power_mW > sfp->max_power_mW) {
		/* Module power specification exceeds the allowed maximum. */
-		if (sfp->id.ext.sff8472_compliance ==
-			SFP_SFF8472_COMPLIANCE_NONE &&
-		    !(sfp->id.ext.diagmon & SFP_DIAGMON_DDM)) {
+		if (!supports_a2) {
...
	}
+
+	if (!supports_a2 && power_mW > 1000) {
+		/* The module power level is below the host maximum and the
+		 * module appears not to implement bus address 0xa2, so assume
+		 * that the module powers up in the indicated mode.
+		 */
+		return 0;
+	}

	/* If the module requires a higher power mode, but also requires
...

This way, if the module reports it doesn't support 0xa2, we don't get
the "Address Change Sequence not supported" message - since if 0xa2 is
not supported, then the address change sequence is irrelevant. However,
modules shouldn't have that bit set... but "shouldn't" doesn't mean
they do not.

This also has the advantage of making the check explicit and obvious,
and I much prefer the organisation of:

	if (module_exceeds_host_power) {
		handle this case
	} else {
		do other checks
	}

I think maybe dealing with power_mW <= 1000 early on may be a good idea,
and eliminates the tests further down for power_mW > 1000.

	if (power_mW <= 1000) {
		sfp->module_power_mW = power_mW;
		return 0;
	}

since those modules do not require any special handling.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!