netdev - Re: [PATCH net v2] net: dpaa2-mac: Get serdes only for backplane links

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: <d6900e52-ddd2-9334-3ed0-734b3e4a957a@seco.com>
Date:   Thu, 23 Mar 2023 12:30:34 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net v2] net: dpaa2-mac: Get serdes only for backplane
 links

On 3/23/23 11:27, Ioana Ciornei wrote:
> On Mon, Mar 06, 2023 at 11:13:17AM -0500, Sean Anderson wrote:
>> On 3/6/23 03:09, Ioana Ciornei wrote:
>> > On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
>> >> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
>> >> add backplane link mode support"), Ioana Ciornei said [1]:
>> >> 
>> >> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
>> >> > by Linux (since the firmware is not touching these). That being said,
>> >> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
>> >> > also have their PCS managed by Linux (no interraction from the
>> >> > firmware's part with the PCS, just the SerDes).
>> >> 
>> >> This implies that Linux only manages the SerDes when the link type is
>> >> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
>> >> moving the existing conditions to more appropriate places.
>> > 
>> > I am not sure I understand why are you moving the conditions to
>> > different places. Could you please explain?
>> 
>> This is not (just) a movement of conditions, but a changing of what they
>> apply to.
>> 
>> There are two things which this patch changes: whether we manage the phy
>> and whether we say we support alternate interfaces. According to your
>> comment above (and roughly in-line with my testing), Linux manages the
>> phy *exactly* when the link type is BACKPLANE. In all other cases, the
>> firmware manages the phy. Similarly, alternate interfaces are supported
>> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
>> the conditions do not match this.
>> 
>> > Why not just append the existing condition from dpaa2_mac_connect() with
>> > "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
>> > 
>> > This way, the serdes_phy is populated only if all the conditions pass
>> > and you don't have to scatter them all around the driver.
>> 
>> If we have link type BACKPLANE, Linux manages the phy, even if the
>> firmware doesn't support changing the interface. Therefore, we need to
>> grab the phy, but not fill in alternate interfaces.
>> 
>> This does not scatter the conditions, but instead moves them to exactly
>> where they are needed. Currently, they are in the wrong places.
> 
> Sorry for not making my position clear from the first time which is:
> there is no point in having a SerDes driver or a reference to the
> SerDes PHY if the firmware does not actually support changing of
> interfaces.
> 
> Why I think that is because the SerDes is configured at boot time
> anyway for the interface type defined in the RCW (Reset Configuration
> Word). If the firmware does not support any protocol change then why
> trouble the dpaa2-eth driver with anything SerDes related?

It's actually the other way around. If the firmware is managing the phy,
why try to probe it? Consider a situation where the firmware supports
protocol change, but the link type is PHY. Then we will probe the
serdes, but we may confuse the firmware (or vice versa).

> This is why I am ok with only extending the condition from
> dpaa2_mac_connect() with an additional check but not the exact patch
> that you sent.

AIUI the condition there is correct because Linux controls the PCS in
both PHY and BACKPLANE modes (although the RGMII check is a bit
strange).

--Sean