netdev - Re: [PATCH] net: mvpp2: add delay at the end of .mac

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

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

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

Message-ID: <87czh1yn4x.fsf@tarshish>
Date:   Thu, 28 Apr 2022 23:03:08 +0300
From:   Baruch Siach <baruch@...s.co.il>
To:     Marcin Wojtas <mw@...ihalf.com>
Cc:     Russell King <linux@...linux.org.uk>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: mvpp2: add delay at the end of .mac_prepare

Hi Marcin,

On Thu, Apr 28 2022, Marcin Wojtas wrote:
> czw., 28 kwi 2022 o 13:16 Baruch Siach <baruch@...s.co.il> napisał(a):
>> On Thu, Apr 28 2022, Marcin Wojtas wrote:
>> > śr., 27 kwi 2022 o 17:05 Baruch Siach <baruch@...s.co.il> napisał(a):
>> >>
>> >> From: Baruch Siach <baruch.siach@...lu.com>
>> >>
>> >> Without this delay PHY mode switch from XLG to SGMII fails in a weird
>> >> way. Rx side works. However, Tx appears to work as far as the MAC is
>> >> concerned, but packets don't show up on the wire.
>> >>
>> >> Tested with Marvell 10G 88X3310 PHY.
>> >>
>> >> Signed-off-by: Baruch Siach <baruch.siach@...lu.com>
>> >> ---
>> >>
>> >> Not sure this is the right fix. Let me know if you have any better
>> >> suggestion for me to test.
>> >>
>> >> The same issue and fix reproduce with both v5.18-rc4 and v5.10.110.
>> >> ---
>> >>  drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >>
>> >> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> >>n index 1a835b48791b..8823efe396b1 100644
>> >> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> >> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
>> >> @@ -6432,6 +6432,8 @@ static int mvpp2_mac_prepare(struct phylink_config *config, unsigned int mode,
>> >>                 }
>> >>         }
>> >>
>> >> +       mdelay(10);
>> >> +
>> >>         return 0;
>> >>  }
>> >
>> > Thank you for the patch and debug effort, however at first glance it
>> > seems that adding delay may be a work-around and cover an actual root
>> > cause (maybe Russell will have more input here).
>>
>> That's my suspicion as well.
>>
>> > Can you share exact reproduction steps?
>>
>> I think I covered all relevant details. Is there anything you find
>> missing?
>>
>> The hardware setup is very similar to the Macchiatobin Doubleshot. I can
>> try to reproduce on that platform next week if it helps.
>>
>> The PHY MAC type (MV_V2_33X0_PORT_CTRL_MACTYPE_MASK) is set to
>> MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER.
>>
>> I can add that DT phy-mode is set to "10gbase-kr" (equivalent to
>> "10gbase-r" in this case). The port cp0_eth0 is connected to a 1G
>> Ethernet switch. Kernel messages indicate that on interface up the MAC
>> is first configured to XLG (10G), but after Ethernet (wire)
>> auto-negotiation that MAC switches to SGMII. If I set DT phy-mode to
>> "sgmii" the issue does not show. Same if I make a down/up cycle of the
>> interface.
>>
>> Thanks for your review.
>
> I booted MacchiatoBin doubleshot with DT (phy-mode set to "10gbase-r")
> without your patch and the 3310 PHY is connected to 1G e1000 card.
> After `ifconfig eth0 up` it properly drops to SGMII without any issue
> in my setup:
>
> # ifconfig eth0 up
> [   62.006580] mvpp2 f2000000.ethernet eth0: PHY
> [f212a600.mdio-mii:00] driver [mv88x3310] (irq=POLL)
> [   62.016777] mvpp2 f2000000.ethernet eth0: configuring for phy/sgmii link mode
> # [   66.110289] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full
> - flow control rx/tx
> [   66.118270] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> # ifconfig eth0 192.168.1.1
> # ping 192.168.1.2
> PING 192.168.1.2 (192.168.1.2): 56 data bytes
> 64 bytes from 192.168.1.2: seq=0 ttl=64 time=0.511 ms
> 64 bytes from 192.168.1.2: seq=1 ttl=64 time=0.212 ms

This is what I see here:

[   46.097184] mvpp2 f2000000.ethernet eth0: PHY [f212a600.mdio-mii:02] driver [mv88x3310] (irq=POLL)
[   46.115071] mvpp2 f2000000.ethernet eth0: configuring for phy/10gbase-r link mode
[   50.249513] mvpp2 f2000000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[   50.257539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

It is almost the same except from the link mode. Why does it try sgmii
even before auto-negotiation takes place?

> Are you aware of the firmware version of the 3310 PHY in your setup?
> In my case it's:
> mv88x3310 f212a600.mdio-mii:00: Firmware version 0.3.3.0

I have a newer version here:

mv88x3310 f212a600.mdio-mii:02: Firmware version 0.3.10.0

This is a timing sensitive issue. Slight change in firmware code might
be significant.

One more detail that might be important is that the PHY firmware is
loaded at run-time using this patch (rebased):

  https://lore.kernel.org/all/13177f5abf60215fb9c5c4251e6f487e4d0d7ff0.1587967848.git.baruch@tkos.co.il/

Thanks,
baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@...s.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -