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]

Date:   Wed, 22 Mar 2023 15:52:23 -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

Hi all,

On 3/6/23 11:13, 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.
> 
> --Sean

I see this is marked as "changes requested" on patchwork. However, as explained
above, I do not think that the requested changes are necessary. Please review the
patch status.

--Sean