Message-ID: <ZCWUq3yEn74JRW0w@kuha.fi.intel.com>
Date:   Thu, 30 Mar 2023 16:54:51 +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 Wed, Mar 29, 2023 at 03:33:48PM +0100, Russell King (Oracle) wrote:
> On Wed, Mar 29, 2023 at 05:07:26PM +0300, Heikki Krogerus wrote:
> > 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.
> 
> You forgot to explain how to free them once they're done, because
> struct swnode will contain a pointer to the struct software_node
> which can be a dangling stale reference - and there's no way for
> code outside swnode.c to know when that reference has gone.
> 
> That is another reason why I prefer my existing solution. That
> problem is taken care of already by the existing code - and as
> it's taken care of there, and properly, there's less possibilities
> for users of swnode to get it wrong.

We need an improved release mechanism, yes.

My idea with the new dynamic allocation routine was that it could be
introduced together with a release callback that we add to the struct
software_node.

The idea of adding the release callback to the structure was actually
considered already some time ago - I think it was discussed at least
shortly also on the public ACPI mailing list. The idea back then
included a default release function that simply frees the struct
software_node instance. That default release function we could then
assign to the release callback in that new software node
allocation/creation routine. That way the drivers should be able to
continue to rely on the underlying code to take care of freeing the
node instance.

Back then there was nobody who really needed that functionality, so
nobody even tried to implement it. Now we of course clearly do need
something like it.

I think the release callback together with the default release
function should work. Let me know what you guys think.

thanks,

-- 
heikki

Powered by blists - more mailing lists