@@ -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,soassumethatthemodulepowersupinthe-*indicatedmode.-*/-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);-return0;-}-}-/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.
@@ -1679,6 +1658,27 @@ static int sfp_module_parse_power(struct sfp *sfp)return0;}+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,soassumethatthemodulepowersupinthe+*indicatedmode.+*/+if(power_mW<=sfp->max_power_mW)+return0;++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);+return0;+}+sfp->module_power_mW=power_mW;return0;
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-03 11:55:14
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:
quoted
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: 照山周一郎 <redacted>
Cc: Russell King <redacted>
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-03 12:58:09
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:
quoted
On Tue, 30 Nov 2021 08:39:29 +0100 Bjørn Mork wrote:
quoted
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: 照山周一郎 <redacted>
Cc: Russell King <redacted>
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(-)
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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);
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Dec 02, 2021 at 05:58:43PM -0800, Jakub Kicinski wrote:
quoted
On Tue, 30 Nov 2021 08:39:29 +0100 Bjørn Mork wrote:
quoted
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: 照山周一郎 <redacted>
Cc: Russell King <redacted>
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
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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 [off-list ref]:
"Russell King (Oracle)" [off-list ref] writes:
quoted
On Fri, Dec 03, 2021 at 02:55:17PM +0100, Bjørn Mork wrote:
quoted
"Russell King (Oracle)" [off-list ref] writes:
quoted
Thinking a little more, how about this:
drivers/net/phy/sfp.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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
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) [off-list ref]:
On Thu, Dec 30, 2021 at 12:43:48PM +0100, Bjørn Mork wrote:
quoted
照山周一郎 [off-list ref] writes:
quoted
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?
From: Christian Lamparter <hidden> Date: 2022-01-15 16:58:48
On 03/12/2021 13:58, Russell King (Oracle) wrote:
quoted hunk
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(-)
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2022-01-17 14:24:26
On Sat, Jan 15, 2022 at 05:58:43PM +0100, Christian Lamparter wrote:
On 03/12/2021 13:58, Russell King (Oracle) wrote:
quoted
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(-)
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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);
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 [off-list ref] reported it (at least
to mainline devs), and the fix was tested by 照山周一郎
[off-list ref].
Is that correct? Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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 [off-list ref] reported it (at least
to mainline devs), and the fix was tested by 照山周一郎
[off-list ref].
Is that correct? Thanks.
I just forwarded the initial report. All credit should go to 照山周一郎
[off-list ref].
Bjørn
From: Christian Lamparter <hidden> Date: 2022-01-17 14:49:44
On 17/01/2022 15:24, Russell King (Oracle) wrote:
On Sat, Jan 15, 2022 at 05:58:43PM +0100, Christian Lamparter wrote:
quoted
On 03/12/2021 13:58, Russell King (Oracle) wrote:
quoted
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(-)
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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);
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 [off-list ref] reported it (at least
to mainline devs), and the fix was tested by 照山周一郎
[off-list ref].
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>
| 照山周一郎 [off-list ref] 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 [off-list ref]:
On 17/01/2022 15:24, Russell King (Oracle) wrote:
quoted
On Sat, Jan 15, 2022 at 05:58:43PM +0100, Christian Lamparter wrote:
quoted
On 03/12/2021 13:58, Russell King (Oracle) wrote:
quoted
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(-)
@@ -1752,17 +1752,20 @@ static int sfp_sm_probe_for_phy(struct sfp *sfp)staticintsfp_module_parse_power(structsfp*sfp){u32power_mW=1000;+boolsupports_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,soassumethatthemodulepowersupinthe*indicatedmode.
@@ -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 */+return0;+}++if(!supports_a2){+/* The module power level is below the host maximum and the+*moduleappearsnottoimplementbusaddress0xa2,soassume+*thatthemodulepowersupintheindicatedmode.+*/+return0;+}+/* If the module requires a higher power mode, but also requires*anaddresschangesequence,warntheuserthatthemodulemay*notbefunctional.*/-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);
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 [off-list ref] reported it (at least
to mainline devs), and the fix was tested by 照山周一郎
[off-list ref].
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>
| 照山周一郎 [off-list ref] 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)