Date:   Mon, 18 Jul 2022 20:11:40 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Vladimir Oltean <olteanv@...il.com>, 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>,
        Daniel Scally <djrscally@...il.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>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Hauke Mehrtens <hauke@...ke-m.de>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Landen Chao <Landen.Chao@...iatek.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        linux-acpi@...r.kernel.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>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.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 net-next 2/6] software node: allow named software node to
 be created

On Mon, Jul 18, 2022 at 09:43:41PM +0300, Andy Shevchenko wrote:
> On Mon, Jul 18, 2022 at 02:27:02PM +0100, Russell King (Oracle) wrote:
> > On Mon, Jul 18, 2022 at 03:29:52PM +0300, Andy Shevchenko wrote:
> > > On Fri, Jul 15, 2022 at 11:48:41PM +0300, Vladimir Oltean wrote:
> > > > So won't kobject_init_and_add() fail on namespace collision? Is it the
> > > > problem that it's going to fail, or that it's not trivial to statically
> > > > determine whether it'll fail?
> > > > 
> > > > Sorry, but I don't see something actionable about this.
> > > 
> > > I'm talking about validation before a runtime. But if you think that is fine,
> > > let's fail it at runtime, okay, and consume more backtraces in the future.
> > 
> > Is there any sane way to do validation of this namespace before
> > runtime?
> 
> For statically compiled, I think we can do it (to some extent).
> Currently only three drivers, if I'm not mistaken, define software nodes with
> names. It's easy to check that their node names are unique.
> 
> When you allow such an API then we might have tracebacks (from sysfs) bout name
> collisions. Not that is something new to kernel (we have seen many of a kind),
> but I prefer, if possible, to validate this before sysfs issues a traceback.
> 
> > The problem in this instance is we need a node named "fixed-link" that
> > is attached to the parent node as that is defined in the binding doc,
> > and we're creating swnodes to provide software generated nodes for
> > this binding.
> 
> And how you guarantee that it will be only a single one with unique pathname?
> 
> For example, you have two DSA cards (or whatever it's called) in the SMP system,
> it mean that there is non-zero probability of coexisting swnodes for them.

Good point - I guess we at least need to attach the swnode parent to the
device so its path is unique, because right now that isn't the case. I'm
guessing that:

        new_port_fwnode = fwnode_create_software_node(port_props, NULL);

will create something at the root of the swnode tree, and then:

        fixed_link_fwnode = fwnode_create_named_software_node(fixed_link_props,
                                                              new_port_fwnode,
                                                              "fixed-link");

will create a node with a fixed name. I guess it in part depends what
pathname the first node gets (which we don't specify.) I'm not familiar
with the swnode code to know what happens with the naming for the first
node.

However, it seems sensible to me to attach the first node to the device
node, thus giving it a unique fwnode path. Does that solve the problem
in swnode land?

> > There could be several such nodes scattered around, but in this
> > instance they are very short-lived before they are destroyed, they
> > don't even need to be published to userspace (and its probably a waste
> > of CPU cycles for them to be published there.)
> > 
> > So, for this specific case, is this the best approach, or is there
> > some better way to achieve what we need here?
> 
> Honestly, I don't know.
> 
> The "workaround" (but it looks to me rather a hack) is to create unique swnode
> and make fixed-link as a child of it.
> 
> Or entire concept of the root swnodes (when name is provided) should be
> reconsidered, so somehow we will have a uniqueness so that the entire
> path(s) behind it will be caller-dependent. But this I also don't like.
> 
> Maybe Heikki, Sakari, Rafael can share their thoughts...
> 
> Just for my learning, why PHY uses "fixed-link" instead of relying on a
> (firmware) graph? It might be the actual solution to your problem.

That's a question for Andrew, but I've tried to solicit his comments on
several occasions concerning this "feature" of DSA but I keep getting
no reply. Honestly, I don't know the answer to your question.

The only thing that I know is that Andrew has been promoting this
feature where a switch port, whether it be connected to the CPU or
to another switch, which doesn't specify any link parameters will
automatically use the fastest "phy interface mode" and the fastest
link speed that can be supported by the DSA device.

This has caused issues over the last few years which we've bodged
around in various ways, and with updates to one of the DSA drivers
this bodging is becoming more of a wart that's spreading. So, I'm
trying to find a way to solve this.

My initial approach was to avoid fiddling with the firmware tree,
but Vladimir proposed this approach as being cleaner - and it means
the "bodge" becomes completely localised in the DSA (distributed
switch architecture) code rather than being spread into phylink.

I wish we could get rid of this "feature" but since it's been
established for many years, and we have at least one known driver
that uses it, getting rid of it breaks existing firmware trees.
I think we also have one other driver that makes use of it as
well, but I can't say for certain (because it's not really possible
to discern which drivers use this feature from reading the driver
code.) I've tried asking Andrew if he knows and got no response.

So I'm in a complete information vacuum here - all that I know is
that trying to convert the mv88e6xxx DSA driver to use phylink_pcs
will break it (as reported by Marek Behún), because phylink doesn't
get used if firmware is using this "defaulting" feature.

It's part of the DT binding, and remains so today - the properties
specifying the "phy-mode", "fixed-link" etc all remain optional.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists