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(-)

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 [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)

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