| Message ID | 20211130073929.376942-1-bjorn@mork.no (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,stable] phy: sfp: fix high power modules without diag mode | expand |
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@springboard-inc.jp> > Cc: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Bjørn Mork <bjorn@mork.no> Russell, any comments?
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@springboard-inc.jp> > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > Signed-off-by: Bjørn Mork <bjorn@mork.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.
On Fri, Dec 03, 2021 at 11:54:57AM +0000, Russell King (Oracle) wrote: > 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@springboard-inc.jp> > > > Cc: Russell King <rmk+kernel@armlinux.org.uk> > > > Signed-off-by: Bjørn Mork <bjorn@mork.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. Thinking a little more, how about this: drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 51a1da50c608..4c900d063b19 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) 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) { /* The module appears not to implement bus address * 0xa2, so assume that the module powers up in the * indicated mode. @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) } } + if (power_mW <= 1000) { + /* Modules below 1W do not require a power change sequence */ + return 0; + } + + if (!supports_a2) { + /* 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 * an address change sequence, warn the user that the module may * not be functional. */ - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { dev_warn(sfp->dev, "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", power_mW / 1000, (power_mW / 100) % 10);
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > 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@springboard-inc.jp> >> > Cc: Russell King <rmk+kernel@armlinux.org.uk> >> > Signed-off-by: Bjørn Mork <bjorn@mork.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) You lost me now. This is still changing sfp_module_parse_power() and not sfp_sm_mod_hpower(). > { > 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 > ... Fine with me. Looks cleaner, and should solve the reported problem AFAICS. > 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. Makes sense. Although that's not a problem here, I guess we have to expect just about any combination of random bits ;-) > 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 > } OK > 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. Do you want this included now, or is that for a later cleanup? Bjørn
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > Thinking a little more, how about this: > > drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 51a1da50c608..4c900d063b19 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) > 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) { > /* The module appears not to implement bus address > * 0xa2, so assume that the module powers up in the > * indicated mode. > @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) > } > } > > + if (power_mW <= 1000) { > + /* Modules below 1W do not require a power change sequence */ > + return 0; > + } > + > + if (!supports_a2) { > + /* 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 > * an address change sequence, warn the user that the module may > * not be functional. > */ > - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { > + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { > dev_warn(sfp->dev, > "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", > power_mW / 1000, (power_mW / 100) % 10); Looks nice to me at least. But I don't have the hardware to test it. Bjørn
On Fri, Dec 03, 2021 at 02:55:17PM +0100, Bjørn Mork wrote: > "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > > > Thinking a little more, how about this: > > > > drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > index 51a1da50c608..4c900d063b19 100644 > > --- a/drivers/net/phy/sfp.c > > +++ b/drivers/net/phy/sfp.c > > @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) > > 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) { > > /* The module appears not to implement bus address > > * 0xa2, so assume that the module powers up in the > > * indicated mode. > > @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) > > } > > } > > > > + if (power_mW <= 1000) { > > + /* Modules below 1W do not require a power change sequence */ > > + return 0; > > + } > > + > > + if (!supports_a2) { > > + /* 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 > > * an address change sequence, warn the user that the module may > > * not be functional. > > */ > > - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { > > + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { > > dev_warn(sfp->dev, > > "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", > > power_mW / 1000, (power_mW / 100) % 10); > > Looks nice to me at least. But I don't have the hardware to test it. I don't have the hardware either, so I can't test it - but it does need testing. I assume as you've reported it and sent a patch, you know someone who has run into this issue? It would be great if you could ask them to test it and let us know if it solves the problem. Thanks.
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > On Fri, Dec 03, 2021 at 02:55:17PM +0100, Bjørn Mork wrote: >> "Russell King (Oracle)" <linux@armlinux.org.uk> writes: >> >> > Thinking a little more, how about this: >> > >> > drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- >> > 1 file changed, 21 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c >> > index 51a1da50c608..4c900d063b19 100644 >> > --- a/drivers/net/phy/sfp.c >> > +++ b/drivers/net/phy/sfp.c >> > @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) >> > 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) { >> > /* The module appears not to implement bus address >> > * 0xa2, so assume that the module powers up in the >> > * indicated mode. >> > @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) >> > } >> > } >> > >> > + if (power_mW <= 1000) { >> > + /* Modules below 1W do not require a power change sequence */ >> > + return 0; >> > + } >> > + >> > + if (!supports_a2) { >> > + /* 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 >> > * an address change sequence, warn the user that the module may >> > * not be functional. >> > */ >> > - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { >> > + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { >> > dev_warn(sfp->dev, >> > "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", >> > power_mW / 1000, (power_mW / 100) % 10); >> >> Looks nice to me at least. But I don't have the hardware to test it. > > I don't have the hardware either, so I can't test it - but it does need > testing. I assume as you've reported it and sent a patch, you know > someone who has run into this issue? It would be great if you could ask > them to test it and let us know if it solves the problem. Hello Teruyama! Any chance you can test this proposed fix from Russel? I believe it should fix the issue with your NTT OCU SFP as well. Bjørn
Hello, Mork! Sorry for the late reply. Thanks to you and Russell for your great contribution. I will test Russell's patch in a few days. Please wait a while. 2021年12月6日(月) 19:35 Bjørn Mork <bjorn@mork.no>: > > "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > > On Fri, Dec 03, 2021 at 02:55:17PM +0100, Bjørn Mork wrote: > >> "Russell King (Oracle)" <linux@armlinux.org.uk> writes: > >> > >> > Thinking a little more, how about this: > >> > > >> > drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- > >> > 1 file changed, 21 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > >> > index 51a1da50c608..4c900d063b19 100644 > >> > --- a/drivers/net/phy/sfp.c > >> > +++ b/drivers/net/phy/sfp.c > >> > @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) > >> > 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) { > >> > /* The module appears not to implement bus address > >> > * 0xa2, so assume that the module powers up in the > >> > * indicated mode. > >> > @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) > >> > } > >> > } > >> > > >> > + if (power_mW <= 1000) { > >> > + /* Modules below 1W do not require a power change sequence */ > >> > + return 0; > >> > + } > >> > + > >> > + if (!supports_a2) { > >> > + /* 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 > >> > * an address change sequence, warn the user that the module may > >> > * not be functional. > >> > */ > >> > - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { > >> > + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { > >> > dev_warn(sfp->dev, > >> > "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", > >> > power_mW / 1000, (power_mW / 100) % 10); > >> > >> Looks nice to me at least. But I don't have the hardware to test it. > > > > I don't have the hardware either, so I can't test it - but it does need > > testing. I assume as you've reported it and sent a patch, you know > > someone who has run into this issue? It would be great if you could ask > > them to test it and let us know if it solves the problem. > > Hello Teruyama! > > Any chance you can test this proposed fix from Russel? I believe it > should fix the issue with your NTT OCU SFP as well. > > > Bjørn -- 株式会社スプリングボード 照山 周一郎 teruyama@springboard-inc.jp http://www.springboard-inc.jp/ 〒110-0005 東京都台東区上野3丁目2番2号 アイオス秋葉原505号室
照山周一郎 <teruyama@springboard-inc.jp> writes:
> I will test Russell's patch in a few days.
Hello!
Sorry to nag, but I didn't see any followup. Did I miss it?
Bjørn
On Thu, Dec 30, 2021 at 12:43:48PM +0100, Bjørn Mork wrote: > 照山周一郎 <teruyama@springboard-inc.jp> writes: > > > I will test Russell's patch in a few days. > > Hello! > > Sorry to nag, but I didn't see any followup. Did I miss it? If you missed it, then I missed it as well!
Hi, Sorry, I found that my mail to you was blocked because the mail type is html. I try to resend the log on booting. Thanks. 2021年12月30日(木) 20:55 Russell King (Oracle) <linux@armlinux.org.uk>: > > On Thu, Dec 30, 2021 at 12:43:48PM +0100, Bjørn Mork wrote: > > 照山周一郎 <teruyama@springboard-inc.jp> writes: > > > > > I will test Russell's patch in a few days. > > > > Hello! > > > > Sorry to nag, but I didn't see any followup. Did I miss it? > > If you missed it, then I missed it as well! > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On 03/12/2021 13:58, Russell King (Oracle) wrote: > On Fri, Dec 03, 2021 at 11:54:57AM +0000, Russell King (Oracle) wrote: > [...] > Thinking a little more, how about this: > > drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > index 51a1da50c608..4c900d063b19 100644 > --- a/drivers/net/phy/sfp.c > +++ b/drivers/net/phy/sfp.c > @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) > 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) { > /* The module appears not to implement bus address > * 0xa2, so assume that the module powers up in the > * indicated mode. > @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) > } > } > > + if (power_mW <= 1000) { > + /* Modules below 1W do not require a power change sequence */ > + return 0; > + } > + > + if (!supports_a2) { > + /* 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 > * an address change sequence, warn the user that the module may > * not be functional. > */ > - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { > + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { > dev_warn(sfp->dev, > "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", > power_mW / 1000, (power_mW / 100) % 10); > The reporter has problems reaching you. But from what I can tell in his reply to his OpenWrt Github PR: <https://github.com/openwrt/openwrt/pull/4802#issuecomment-1013439827> your approach is working perfectly. Could you spin this up as a fully-fledged patch (backports?) Thank you & Cheers, Christian
On Sat, Jan 15, 2022 at 05:58:43PM +0100, Christian Lamparter wrote: > On 03/12/2021 13:58, Russell King (Oracle) wrote: > > On Fri, Dec 03, 2021 at 11:54:57AM +0000, Russell King (Oracle) wrote: > > [...] > > Thinking a little more, how about this: > > > > drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > > index 51a1da50c608..4c900d063b19 100644 > > --- a/drivers/net/phy/sfp.c > > +++ b/drivers/net/phy/sfp.c > > @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) > > 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) { > > /* The module appears not to implement bus address > > * 0xa2, so assume that the module powers up in the > > * indicated mode. > > @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) > > } > > } > > + if (power_mW <= 1000) { > > + /* Modules below 1W do not require a power change sequence */ > > + return 0; > > + } > > + > > + if (!supports_a2) { > > + /* 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 > > * an address change sequence, warn the user that the module may > > * not be functional. > > */ > > - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { > > + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { > > dev_warn(sfp->dev, > > "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", > > power_mW / 1000, (power_mW / 100) % 10); > > > > The reporter has problems reaching you. But from what I can tell in his reply to his > OpenWrt Github PR: > <https://github.com/openwrt/openwrt/pull/4802#issuecomment-1013439827> > > your approach is working perfectly. Could you spin this up as a fully-fledged patch (backports?) There seems to be no problem - I received an email on the 30 December complete with the test logs. However, that was during the holiday period and has been buried, so thanks for the reminder. However, I'm confused about who the reporter and testers actually are, so I'm not sure who to put in the Reported-by and Tested-by fields. From what I can see, Bjørn Mork <bjorn@mork.no> reported it (at least to mainline devs), and the fix was tested by 照山周一郎 <teruyama@springboard-inc.jp>. Is that correct? Thanks.
"Russell King (Oracle)" <linux@armlinux.org.uk> writes: > However, I'm confused about who the reporter and testers actually are, > so I'm not sure who to put in the Reported-by and Tested-by fields. > From what I can see, Bjørn Mork <bjorn@mork.no> reported it (at least > to mainline devs), and the fix was tested by 照山周一郎 > <teruyama@springboard-inc.jp>. > > Is that correct? Thanks. I just forwarded the initial report. All credit should go to 照山周一郎 <teruyama@springboard-inc.jp>. Bjørn
On 17/01/2022 15:24, Russell King (Oracle) wrote: > On Sat, Jan 15, 2022 at 05:58:43PM +0100, Christian Lamparter wrote: >> On 03/12/2021 13:58, Russell King (Oracle) wrote: >>> On Fri, Dec 03, 2021 at 11:54:57AM +0000, Russell King (Oracle) wrote: >>> [...] >>> Thinking a little more, how about this: >>> >>> drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- >>> 1 file changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c >>> index 51a1da50c608..4c900d063b19 100644 >>> --- a/drivers/net/phy/sfp.c >>> +++ b/drivers/net/phy/sfp.c >>> @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) >>> 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) { >>> /* The module appears not to implement bus address >>> * 0xa2, so assume that the module powers up in the >>> * indicated mode. >>> @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) >>> } >>> } >>> + if (power_mW <= 1000) { >>> + /* Modules below 1W do not require a power change sequence */ >>> + return 0; >>> + } >>> + >>> + if (!supports_a2) { >>> + /* 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 >>> * an address change sequence, warn the user that the module may >>> * not be functional. >>> */ >>> - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { >>> + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { >>> dev_warn(sfp->dev, >>> "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", >>> power_mW / 1000, (power_mW / 100) % 10); >>> >> >> The reporter has problems reaching you. But from what I can tell in his reply to his >> OpenWrt Github PR: >> <https://github.com/openwrt/openwrt/pull/4802#issuecomment-1013439827> >> >> your approach is working perfectly. Could you spin this up as a fully-fledged patch (backports?) > > There seems to be no problem - I received an email on the 30 December > complete with the test logs. However, that was during the holiday period > and has been buried, so thanks for the reminder. > > However, I'm confused about who the reporter and testers actually are, > so I'm not sure who to put in the Reported-by and Tested-by fields. > From what I can see, Bjørn Mork <bjorn@mork.no> reported it (at least > to mainline devs), and the fix was tested by 照山周一郎 > <teruyama@springboard-inc.jp>. > > Is that correct? Thanks. > From what I know, you are correct there. 照山周一郎 posted a patch "skip hpower setting for the module which has no revs" to fix his issue to the OpenWrt-Devel Mailinglist on the 28th November 2021: <https://www.mail-archive.com/openwrt-devel@lists.openwrt.org/msg60669.html> | |@@ -0,0 +1,12 @@ |@@ -0,0 +1,11 @@ |--- a/drivers/net/phy/sfp.c |+++ b/drivers/net/phy/sfp.c |@@ -1590,6 +1590,8 @@ static int sfp_module_parse_power(struct | | 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; Bjørn Mork picked this up and noted: |This looks like a workaround for a specific buggy module. Is that |correct? Why not update sfp_module_parse_power() instead so you can |skip the HPOWER state completely? And add an appropriate warning about |this unexpected combination of options and sff8472_compliance..." and the thread went from there, with Bjørn Mork notifying you/upstream about the problem because of the language barrier. <https://www.mail-archive.com/openwrt-devel@lists.openwrt.org/msg60697.html> | 照山周一郎 <teruy...@springboard-inc.jp> writes: | |> Thank you for your quick response. |> It worked without any problems. | |Thanks for testing! I submitted this to netdev with a stable hint now. |So it should end up in Linux v5.10.x, and therefore also OpenWrt, in a |few weeks unless there are objections. So, one could argue that both reported this in a way and 照山周一郎 tested it on his hardware. Cheers, Christian (got to catch a train)
To whom it may concern Thank you for summarizing the process. I am sorry for the inconvenience caused by my poor English. My patch was incomplete, and Russell made it better. I have checked Russell's fix on real hardware and it works correctly. I was also aware that Bjørn had taken over the rest of the merging work, leaving me as the reporter. If there is anything else I should be doing, please let me know. Thanks 2022年1月17日(月) 23:49 Christian Lamparter <chunkeey@gmail.com>: > > On 17/01/2022 15:24, Russell King (Oracle) wrote: > > On Sat, Jan 15, 2022 at 05:58:43PM +0100, Christian Lamparter wrote: > >> On 03/12/2021 13:58, Russell King (Oracle) wrote: > >>> On Fri, Dec 03, 2021 at 11:54:57AM +0000, Russell King (Oracle) wrote: > >>> [...] > >>> Thinking a little more, how about this: > >>> > >>> drivers/net/phy/sfp.c | 25 +++++++++++++++++++++---- > >>> 1 file changed, 21 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c > >>> index 51a1da50c608..4c900d063b19 100644 > >>> --- a/drivers/net/phy/sfp.c > >>> +++ b/drivers/net/phy/sfp.c > >>> @@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp) > >>> 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) { > >>> /* The module appears not to implement bus address > >>> * 0xa2, so assume that the module powers up in the > >>> * indicated mode. > >>> @@ -1779,11 +1782,24 @@ static int sfp_module_parse_power(struct sfp *sfp) > >>> } > >>> } > >>> + if (power_mW <= 1000) { > >>> + /* Modules below 1W do not require a power change sequence */ > >>> + return 0; > >>> + } > >>> + > >>> + if (!supports_a2) { > >>> + /* 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 > >>> * an address change sequence, warn the user that the module may > >>> * not be functional. > >>> */ > >>> - if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE && power_mW > 1000) { > >>> + if (sfp->id.ext.diagmon & SFP_DIAGMON_ADDRMODE) { > >>> dev_warn(sfp->dev, > >>> "Address Change Sequence not supported but module requires %u.%uW, module may not be functional\n", > >>> power_mW / 1000, (power_mW / 100) % 10); > >>> > >> > >> The reporter has problems reaching you. But from what I can tell in his reply to his > >> OpenWrt Github PR: > >> <https://github.com/openwrt/openwrt/pull/4802#issuecomment-1013439827> > >> > >> your approach is working perfectly. Could you spin this up as a fully-fledged patch (backports?) > > > > There seems to be no problem - I received an email on the 30 December > > complete with the test logs. However, that was during the holiday period > > and has been buried, so thanks for the reminder. > > > > However, I'm confused about who the reporter and testers actually are, > > so I'm not sure who to put in the Reported-by and Tested-by fields. > > From what I can see, Bjørn Mork <bjorn@mork.no> reported it (at least > > to mainline devs), and the fix was tested by 照山周一郎 > > <teruyama@springboard-inc.jp>. > > > > Is that correct? Thanks. > > > > From what I know, you are correct there. 照山周一郎 posted a patch > "skip hpower setting for the module which has no revs" to fix his > issue to the OpenWrt-Devel Mailinglist on the 28th November 2021: > <https://www.mail-archive.com/openwrt-devel@lists.openwrt.org/msg60669.html> > > | > |@@ -0,0 +1,12 @@ > |@@ -0,0 +1,11 @@ > |--- a/drivers/net/phy/sfp.c > |+++ b/drivers/net/phy/sfp.c > |@@ -1590,6 +1590,8 @@ static int sfp_module_parse_power(struct > | > | 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; > > Bjørn Mork picked this up and noted: > |This looks like a workaround for a specific buggy module. Is that > |correct? Why not update sfp_module_parse_power() instead so you can > |skip the HPOWER state completely? And add an appropriate warning about > |this unexpected combination of options and sff8472_compliance..." > > and the thread went from there, with Bjørn Mork notifying you/upstream > about the problem because of the language barrier. > > <https://www.mail-archive.com/openwrt-devel@lists.openwrt.org/msg60697.html> > > | 照山周一郎 <teruy...@springboard-inc.jp> writes: > | > |> Thank you for your quick response. > |> It worked without any problems. > | > |Thanks for testing! I submitted this to netdev with a stable hint now. > |So it should end up in Linux v5.10.x, and therefore also OpenWrt, in a > |few weeks unless there are objections. > > So, one could argue that both reported this in a way and 照山周一郎 tested > it on his hardware. > > Cheers, > Christian (got to catch a train)
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index ab77a9f439ef..9742469a1e58 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -1647,27 +1647,6 @@ static int sfp_module_parse_power(struct sfp *sfp) if (sfp->id.ext.options & cpu_to_be16(SFP_OPTIONS_HIGH_POWER_LEVEL)) power_mW = 2000; - 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)) { - /* The module appears not to implement bus address - * 0xa2, so assume that the module powers up in the - * indicated mode. - */ - dev_err(sfp->dev, - "Host does not support %u.%uW modules\n", - power_mW / 1000, (power_mW / 100) % 10); - return -EINVAL; - } else { - dev_warn(sfp->dev, - "Host does not support %u.%uW modules, module left in power mode 1\n", - power_mW / 1000, (power_mW / 100) % 10); - return 0; - } - } - /* If the module requires a higher power mode, but also requires * an address change sequence, warn the user that the module may * not be functional. @@ -1679,6 +1658,27 @@ static int sfp_module_parse_power(struct sfp *sfp) return 0; } + if (sfp->id.ext.sff8472_compliance == SFP_SFF8472_COMPLIANCE_NONE && + !(sfp->id.ext.diagmon & SFP_DIAGMON_DDM)) { + /* The module appears not to implement bus address + * 0xa2, so assume that the module powers up in the + * indicated mode. + */ + if (power_mW <= sfp->max_power_mW) + return 0; + + dev_err(sfp->dev, "Host does not support %u.%uW modules\n", + power_mW / 1000, (power_mW / 100) % 10); + return -EINVAL; + } + + if (power_mW > sfp->max_power_mW) { + dev_warn(sfp->dev, + "Host does not support %u.%uW modules, module left in power mode 1\n", + power_mW / 1000, (power_mW / 100) % 10); + return 0; + } + sfp->module_power_mW = power_mW; return 0;
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@springboard-inc.jp> Cc: Russell King <rmk+kernel@armlinux.org.uk> Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/phy/sfp.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-)