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