linux-kernel - Re: [PATCH 1/6] software node: Provide replacement for device_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 Feb 2021 14:50:24 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Felipe Balbi <balbi@...nel.org>,
        Mathias Nyman <mathias.nyman@...el.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:" 
        <linux-usb@...r.kernel.org>
Subject: Re: [PATCH 1/6] software node: Provide replacement for device_add_properties()

On Wed, Feb 3, 2021 at 10:45 AM Heikki Krogerus
<heikki.krogerus@...ux.intel.com> wrote:
>
> On Tue, Feb 02, 2021 at 05:08:40PM +0100, Rafael J. Wysocki wrote:
> > It looks like there is a use case that cannot be addressed by using
> > device_add_properties() and that's why you need this new function.
> >
> > Can you describe that use case, please, and explain what the problem
> > with using device_add_properties() in it is?
>
> The problem with device_add_properties() is that it gives false
> impression that the device properties are somehow directly attached to
> the devices, which is not true. Now, that should not be a major issue,
> but it seems that it is. I think Lee Jones basically used that as an
> argument to refuse changes (and pretty minor changes) that would have
> allowed us to use software nodes with the MFD drivers.
>
> Nevertheless, I was not planning to provide a replacement for it
> originally. We do in any case have the real issue caused by that
> device_remove_properties() call in device_del() which has to be fixed.

What's that issue, specifically?

> I started to fix that by moving device_add_properties() under
> drivers/base/swnode.c so I can implement it like I've implemented now
> that new function, but after that when I started to tackle the second
> issue by removing the subsystem wrappers like
> platform_device_add_device_properties() and replacing them with things
> like platform_device_add_software_node() in order to give the drivers
> a chance to actually use software nodes, I realised that there isn't
> much use for device_add_properties() after that.
>
> Also, even though I'm not super happy about adding more API to this
> thing, this function - device_create_managed_software_node() - I do
> like. Not only is it implemented so that we don't have to rely on
> anything else in drivers core in order to achieve the lifetime link
> with the device, it is also much more descriptive. The function name
> alone does not leave any questions about what is going to happen if
> that function is called. You'll end up with a software node that just
> happens to be attached to the device.
>
> On top of those two things, by adding the new function it also gives
> me a nice stepping stone to do these changes in the normal stages:
>
>         1. Add feature/modification.
>         2. Convert users.
>         3. Cleanup.
>
> And finally, even though we may not have any users left for
> device_add_properties() after I have updated all the subsystems and
> drivers, this new function will still be useful. That is because, even
> though it can be used as a drop-in replacement for
> device_add_properties(), it does add that one small but important
> change. It allows the nodes created with it to be part of node
> hierarchy, and that alone is useful to me, and I'm planning on using
> that feature later.
>
> I'm sorry about the long explanation.

No need to be sorry,  now I know what this really is about. :-)

I'm not against this patch, but I IMO the "motivation" part of the
changelog needs to be improved.

If the final goal is to get rid of device_add_properties(), please
spell it out and say what problems there are with it and why the new
function will be better.