[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 7 Jul 2022 18:27:27 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Alvin __ipraga <alsi@...g-olufsen.dk>,
Claudiu Manoil <claudiu.manoil@....com>,
"David S. Miller" <davem@...emloft.net>,
DENG Qingfang <dqfext@...il.com>,
Eric Dumazet <edumazet@...gle.com>,
Florian Fainelli <f.fainelli@...il.com>,
George McCollister <george.mccollister@...il.com>,
Hauke Mehrtens <hauke@...ke-m.de>,
Jakub Kicinski <kuba@...nel.org>,
Kurt Kanzenbach <kurt@...utronix.de>,
Landen Chao <Landen.Chao@...iatek.com>,
Linus Walleij <linus.walleij@...aro.org>,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org,
Matthias Brugger <matthias.bgg@...il.com>,
netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
Sean Wang <sean.wang@...iatek.com>,
UNGLinuxDriver@...rochip.com,
Vivien Didelot <vivien.didelot@...il.com>,
Woojung Huh <woojung.huh@...rochip.com>,
Marek BehĂșn <kabel@...nel.org>
Subject: Re: [PATCH RFC net-next 5/5] net: dsa: always use phylink for CPU
and DSA ports
On Thu, Jul 07, 2022 at 11:09:43AM +0100, Russell King (Oracle) wrote:
> On Wed, Jul 06, 2022 at 05:24:09PM +0100, Russell King (Oracle) wrote:
> > On Wed, Jul 06, 2022 at 01:26:21PM +0300, Vladimir Oltean wrote:
> > > Can we please limit phylink_set_max_link_speed() to just the CPU ports
> > > where a fixed-link property is also missing, not just a phy-handle?
> > > Although to be entirely correct, we can also have MLO_AN_INBAND, which
> > > wouldn't be covered by these 2 checks and would still represent a valid
> > > DT binding.
> >
> > phylink_set_max_fixed_link() already excludes itself:
> >
> > if (pl->cfg_link_an_mode != MLO_AN_PHY || pl->phydev || pl->sfp_bus)
~~~~~~~~~~
If not NULL, this is an SFP PHY, right? In other words, it's supposed to protect from
phylink_sfp_connect_phy() - code involuntarily triggered by phylink_create() ->
phylink_register_sfp() - and not from calls to phylink_{,fwnode_}connect_phy()
that were initiated by the phylink user between phylink_create() and
phylink_set_max_fixed_link(), correct? Those are specified as invalid in the
kerneldoc and that's about it - that's not what the checking is for, correct?
But if so, why even check for pl->phydev, if you check for pl->sfp_bus?
> > return -EBUSY;
> >
> > intentionally so that if there is anything specified for the port, be
> > that a fixed link or in-band, then phylink_set_max_fixed_link() errors
> > out with -EBUSY.
> >
> > The only case that it can't detect is if there is a PHY that may be
> > added to phylink at a later time, and that is what the check above
> > is for.
Here by "PHY added at a later time", you do mean calling phylink_{,fwnode_}connect_phy()
after phylink_set_max_fixed_link(), right?
So this is what I don't understand. If we've called phylink_set_max_fixed_link()
we've changed pl->cfg_link_an_mode to MLO_AN_FIXED and this will
silently break future calls to phylink_{,fwnode_}connect_phy(), so DSA
predicts if it's going to call either of those connect_phy() functions,
and calls phylink_set_max_fixed_link() only if it won't. Right?
You've structured the checks in this "distributed" way because phylink
can't really predict whether phylink_{,fwnode_}connect_phy() will be
called after phylink_set_max_fixed_link(), right? I mean, it can
probably predict the fwnode_ variant, but not phylink_connect_phy, and
this is why it is up to the caller to decide when to call and when not to.
> I've updated the function description to mention this detail:
>
> +/**
> + * phylink_set_max_fixed_link() - set a fixed link configuration for phylink
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * Set a maximum speed fixed-link configuration for the chosen interface mode
> + * and MAC capabilities for the phylink instance if the instance has not
> + * already been configured with a SFP, fixed link, or in-band AN mode. If the
> + * interface mode is PHY_INTERFACE_MODE_NA, then search the supported
> + * interfaces bitmap for the first interface that gives the fastest supported
> + * speed.
>
> Does this address your concern?
>
> Thanks.
Not really, no, sorry, it just confuses me more. It should maybe also
say that this function shouldn't be called if phylink_{,fwnode_}connect_phy()
is going to be called later.
Can phylink absorb all this logic, and automatically call phylink_set_max_fixed_link()
based on the following?
(1) struct phylink_config gets extended with a bool fallback_max_fixed_link.
(2) DSA CPU and DSA ports set this to true in dsa_port_phylink_register().
(3) phylink_set_max_fixed_link() is hooked into this -ENODEV error
condition from phylink_fwnode_phy_connect():
phy_fwnode = fwnode_get_phy_node(fwnode);
if (IS_ERR(phy_fwnode)) {
if (pl->cfg_link_an_mode == MLO_AN_PHY)
return -ENODEV; <- here
return 0;
}
Powered by blists - more mailing lists