netdev - Re: [RFC] net: phy: air_en8811h: add Airoha AN8811HB support

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: <87sedq4pyk.fsf@miraculix.mork.no>
Date: Thu, 04 Dec 2025 09:21:39 +0100
From: Bjørn Mork <bjorn@...k.no>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: netdev@...r.kernel.org, "Lucien.Jheng" <lucienzx159@...il.com>,
        Daniel Golle <daniel@...rotopia.org>
Subject: Re: [RFC] net: phy: air_en8811h: add Airoha AN8811HB support

Vladimir Oltean <vladimir.oltean@....com> writes:
> On Tue, Dec 02, 2025 at 11:22:22AM +0100, Bjørn Mork wrote:
>> @@ -967,32 +1157,61 @@ static int en8811h_probe(struct phy_device *phydev)
>>         return 0;
>>  }
>> 
>> -static int en8811h_config_serdes_polarity(struct phy_device *phydev)
>> +static bool airphy_invert_rx(struct phy_device *phydev)
>>  {
>>         struct device *dev = &phydev->mdio.dev;
>> -       int pol, default_pol;
>> -       u32 pbus_value = 0;
>> +       int default_pol  = PHY_POL_NORMAL;
>> 
>> -       default_pol = PHY_POL_NORMAL;
>>         if (device_property_read_bool(dev, "airoha,pnswap-rx"))
>>                 default_pol = PHY_POL_INVERT;
>
> I think we can discuss whether a newly added piece of hardware (at least
> from the perspective of mainline Linux) should gain compatibility with
> deprecated device tree properties or not. My concern is that if I'm soft
> on grandfathered deprecated properties, their replacements are never
> going to be used.

Wasn't sure about this.  Now I am :-)

I'll drop the deprecated properties.

>> -       pol = phy_get_rx_polarity(dev_fwnode(dev), phy_modes(phydev->interface),
>> -                                 PHY_POL_NORMAL | PHY_POL_INVERT, default_pol);
>> -       if (pol < 0)
>> -               return pol;
>> -       if (pol == PHY_POL_INVERT)
>> -               pbus_value |= EN8811H_POLARITY_RX_REVERSE;
>> +       return phy_get_rx_polarity(dev_fwnode(dev), phy_modes(phydev->interface),
>> +                                  PHY_POL_NORMAL | PHY_POL_INVERT, default_pol)
>> +               == PHY_POL_INVERT;
>
> The idea in my patches was that phy_get_rx_polarity() can return a
> negative error code (memory allocation failure, unsupported device tree
> property value like PHY_POL_AUTO, etc), which was propagated as such,
> and failed the .config_init().
>
> In your interpretation, no matter which of the above error cases took
> place, for all you care, they all mean "don't invert the polarity", and
> the show must go on. The error path that I was envisioning to bubble up
> towards the topmost caller, to attract attention that something is
> wrong, is gone.

Right.  Sorry about that.  Tried too hard to factor out the common
parts.

Will drop the refactoring. and implement error handling for the new chip
as well. Will be cleaner anyway without the deprecated properties.

> It's a bit unfortunate that in C we can't just throw an exception and
> whoever handles it handles it, but since the phy_get_rx_polarity() API
> isn't yet merged, I'd like to raise the awkwardness of error handling as
> a potential concern.
>
> You could argue that phy_get_rx_polarity() is doing too much - what's
> its business in getting the supported polarities and default polarity
> from you - can't you test by yourself if you support the value that's in
> the device tree, or fall back to a default value if nothing's there?
> Maybe, but even with these things moved out of phy_get_rx_polarity(), I
> still couldn't hide the fact that there's memory allocation inside,
> which can fail and return an error code. So I decided that if there's an
> error to handle anyway, I'd rather push the handling of unsupported
> polarities to the helper too, such that the only error that you need to
> handle is in one place. But you _do_ need to handle it.

True. Much, much better to return an error from driver probe than having
to debug arbitrary polarity mismatches..


Bjørn