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: <20251203232402.oy4pbphj4vsqp5lb@skbuf>
Date: Thu, 4 Dec 2025 01:24:02 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Bjørn Mork <bjorn@...k.no>
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

Hi Bjorn,

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.

> -       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.

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.

> +}