netdev - Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to query PHY in-band autoneg capability

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: <0e921aaa-6e71-ba16-faf7-70a4bac5be23@seco.com>
Date:   Fri, 18 Nov 2022 10:49:30 -0500
From:   Sean Anderson <sean.anderson@...o.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Russell King <linux@...linux.org.uk>,
        Florian Fainelli <f.fainelli@...il.com>,
        UNGLinuxDriver@...rochip.com,
        bcm-kernel-feedback-list@...adcom.com,
        Madalin Bucur <madalin.bucur@....nxp.com>,
        Camelia Groza <camelia.groza@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Maxim Kochetkov <fido_max@...ox.ru>,
        Antoine Tenart <atenart@...nel.org>,
        Michael Walle <michael@...le.cc>,
        Raag Jadav <raagjadav@...il.com>,
        Siddharth Vadapalli <s-vadapalli@...com>,
        Ong Boon Leong <boon.leong.ong@...el.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Marek Behun <marek.behun@....cz>
Subject: Re: [PATCH v4 net-next 2/8] net: phylink: introduce generic method to
 query PHY in-band autoneg capability

On 11/18/22 10:42, Vladimir Oltean wrote:
> On Fri, Nov 18, 2022 at 10:11:06AM -0500, Sean Anderson wrote:
>> > +enum phy_an_inband {
>> > +	PHY_AN_INBAND_UNKNOWN		= BIT(0),
>> 
>> Shouldn't this be something like
>> 
>> 	PHY_AN_INBAND_UNKNOWN		= 0,
>> 
>> ?
> 
> Could be 0 as well. The code explicitly tests against PHY_AN_INBAND_UNKNOWN
> everywhere, so the precise value doesn't matter too much.
> 
>> What does it mean if a phy returns e.g. 0b101?
> 
> You mean PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN. Well, it doesn't mean
> anything, it's not a valid return code. I didn't make the code too defensive
> in this regard, because I didn't see a reason for making some pieces of
> code defend themselves against other pieces of code. It's a bit mask of
> 3 bits where not all combinations are valid. Even if PHY_AN_INBAND_UNKNOWN
> was defined as 0 instead of BIT(0), it would still be just as logically
> invalid to return PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN, but this
> would be indistinguishable in machine code from just PHY_AN_INBAND_ON.
> 
> I don't know, I don't see a practical reason to make a change here.

If we have the opportunity, we should try to make invalid return codes
inexpressible. If we remove the extra bit, then all the combinations we
would like to have:

- I don't know what I support
- In-band must be enabled
- In-band must be disabled
- I can support either

are exactly the combinations supported by the underlying data.

--Sean