Message-ID: <ZCRGHlERlLNuPHgE@kuha.fi.intel.com>
Date:   Wed, 29 Mar 2023 17:07:26 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Daniel Scally <djrscally@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jakub Kicinski <kuba@...nel.org>, linux-acpi@...r.kernel.org,
        netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH RFC net-next 6/7] net: dsa: mv88e6xxx: provide software
 node for default settings

On Tue, Mar 28, 2023 at 02:23:41PM +0100, Russell King (Oracle) wrote:
> On Tue, Mar 28, 2023 at 03:09:56PM +0300, Heikki Krogerus wrote:
> > The problem is that the function you are proposing will be exploited
> > silently - people will use NULL as the parent without anybody
> > noticing. Everything will work for a while, because everybody will
> > first only have a single device for that driver. But as time goes by
> > and new hardware appears, suddenly there are multiple devices for
> > those drivers, and the conflict start to appear.
> 
> So, an easy solution would be to reject a call to
> fwnode_create_named_software_node() when parent is NULL, thereby
> preventing named nodes at the root level.
> 
> > At that point the changes that added the function call will have
> > trickled down to the stable trees, so the distros are affected. Now we
> > are no longer talking about a simple cleanup that fixes the issue. In
> > the unlikely, but possible case, this will turn into ABI problem if
> 
> There is no such thing as stable APIs for internal kernel interfaces.
> 
> Documentation/process/stable-api-nonsense.rst
> 
> > As you pointed out, this kind of risks we have to live with kbojects,
> > struct device stuff and many others, but the thing is, with the
> > software node and device property APIs right now we don't. So the fact
> > that a risk exists in one place just isn't justification to accept the
> > same risk absolutely everywhere.
> 
> Meanwhile, firmware descriptions explicitly permit looking up nodes by
> their names, but here we are, with the software node maintainers
> basically stating that they don't wish to support creating software
> nodes with explicit names.

If you want to name the nodes then you just go ahead and name them,
nobody is preventing you and you can already do that, but if you do
so, then you will take full responsibility of the entire software node
- that is what you are naming here - instead of just the fwnode that
it contains. The users of the node can deal with the fwnode alone, but
you as the creator of the software node have to take proper ownership
of it.

> > Russell, if you have some good arguments for accepting your proposal,
> > I assure you I will agree with you, but so far all you have given are
> > attacks on a sketch details and statements like that "I think you're
> > making a mountain out of a mole". Those just are not good enough.
> 
> Basically, I think you are outright wrong for all the reasons I have
> given in all my emails on this subject.
> 
> Yes, I accept there is a *slight* risk of abuse, but I see it as no
> different from the risk from incorrect usage of any other kernel
> internal interface. Therefore I just do not accept your argument
> that we should not have this function, and I do not accept your
> reasoning.

I would not be so against the function if there wasn't any other way
to handle your case, but there is.

You really can not claim that the existing API is in any way inferior,
or even more complex, compared to your function before you actually
try it. You simply can not make judgement based on a sketch that is
basically just showing you the functions and structures that you need.

If there are issues with the API, then we need to of course fix those
issues, but please keep in mind that still does not mean we have any
need for the function you are proposing.

Please also note that helpers are welcome if you feel we need them. If
you want to add for example an allocation routine that duplicates also
the properties in one go, then that alone would reduce the complexity
needed in the drivers that create the nodes. I think in most cases,
possibly also in yours, that alone would allow most stuff to be
handled from stack memory.

fwnode_create_software_node() is there just to support the legacy
device properties. You really should not be using even that. If you
need to deal with software nodes then you deal with them with struct
software_node.

thanks,

-- 
heikki

Powered by blists - more mailing lists