Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Bartosz Golaszewski]

On Thu, Apr 30, 2026 at 2:38 PM Bartosz Golaszewski <brgl@kernel.org> wrote:
>
> On Thu, Apr 30, 2026 at 2:02 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> >
> > On Thu, Apr 30, 2026 at 09:46:04AM +0200, Bartosz Golaszewski wrote:
> > > If we pass a software node to a newly created device using struct
> > > platform_device_info, it will not be removed when the device is
> > > released. This may happen when a module creating the device is removed
> > > or on failure in platform_device_add().
> > >
> > > When we try to reuse that software node in a subsequent call to
> > > platform_device_register_full(), it will fails with -EBUSY. Add the
> > > missing call to device_remove_software_node() in release path.
> > >
> > > In addition to the above change, make sure that we still function
> > > correctly if a software node is used as the primary firmware node as
> > > well as disallow using two software nodes for platform devices as
> > > device_add_software_node() does not handle this case correctly (in fact
> > > a comment inside it states that only one software node per device is
> > > allowed but it will not bail out if two are used so we need to handle it
> > > here).
> >
> > ...
> >
> > > -     if (pdevinfo->swnode && pdevinfo->properties)
> > > +     /*
> > > +      * Only one software node per device is allowed. Make sure we don't
> > > +      * accept or create two.
> > > +      */
> > > +     if ((pdevinfo->swnode && pdevinfo->properties) ||
> > > +         (pdevinfo->swnode && is_software_node(pdevinfo->fwnode)) ||
> > > +         (pdevinfo->properties && is_software_node(pdevinfo->fwnode)))
> > >               return ERR_PTR(-EINVAL);
> >
> > This makes me think of why we have these many ways of doing things...
> > Perhaps we should kill pdevinfo::properties completely?
> >
>
> Good luck with that, I'm pretty sure a lot of users are still out
> there. :) It's also relatively convenient and skips one more struct
> definition.
>
> > Second thought is what about actually refusing this on the level of
> > device_add_software_node()? And looking at it, we have that check
> > there, why do we need it here then? Did we miss to check error code from
> > device_add_software_node() somewhere?
> >
>
> device_create_managed_software_node() doesn't seem to have that check
> unlike device_add_software_node() so we'd hit an issue with properties
> != NULL && swnode supplied. I think failing earlier here makes for
> more readable code and less error-prone unwinding on failure.
>
> Also: device_add_software_node() return -EBUSY while I think it makes
> more sense to return -EINVAL. I prefer to keep this check locally
> unless driver core maintainers object.
>
> Bart

Danilo: if there are no further comments, can you pick it up for v7.1?

Thanks,
Bartosz

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Danilo Krummrich]

On Mon May 11, 2026 at 9:33 AM CEST, Bartosz Golaszewski wrote:
> Danilo: if there are no further comments, can you pick it up for v7.1?

It seems that sashiko has a valid concern in [1]; can you confirm?

[1] https://sashiko.dev/#/patchset/20260430-swnode-remove-on-dev-unreg-v4-0-01574da0aed3%40oss.qualcomm.com

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Bartosz Golaszewski]

On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich <dakr@kernel.org> said:
> On Mon May 11, 2026 at 9:33 AM CEST, Bartosz Golaszewski wrote:
>> Danilo: if there are no further comments, can you pick it up for v7.1?
>
> It seems that sashiko has a valid concern in [1]; can you confirm?
>
> [1] https://sashiko.dev/#/patchset/20260430-swnode-remove-on-dev-unreg-v4-0-01574da0aed3%40oss.qualcomm.com
>

Yes, I explained it here[1]. Basically it's similar to how we need to call
platform_device_add_data() for devices created with platform_device_alloc().

We can consider adding platform_device_add_software_node() once there's
a potential user but for now I'd just leave it like this.

Bartosz

[1] https://lore.kernel.org/all/CAMRc=MdR+xKCP5e+RiF-MvHvD9-z-Vxno=0B7nmOm+1oS6TDMQ@mail.gmail.com/

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Danilo Krummrich]

On Mon May 11, 2026 at 4:17 PM CEST, Bartosz Golaszewski wrote:
> On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich <dakr@kernel.org> said:
>> On Mon May 11, 2026 at 9:33 AM CEST, Bartosz Golaszewski wrote:
>>> Danilo: if there are no further comments, can you pick it up for v7.1?
>>
>> It seems that sashiko has a valid concern in [1]; can you confirm?
>>
>> [1] https://sashiko.dev/#/patchset/20260430-swnode-remove-on-dev-unreg-v4-0-01574da0aed3%40oss.qualcomm.com
>>
>
> Yes, I explained it here[1]. Basically it's similar to how we need to call
> platform_device_add_data() for devices created with platform_device_alloc().
>
> We can consider adding platform_device_add_software_node() once there's
> a potential user but for now I'd just leave it like this.

But there are users that already need this, no? There is Xe [1] and Surface GPE
[2], or am I missing something?

[1] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/gpu/drm/xe/xe_i2c.c#L99
[2] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/platform/surface/surface_gpe.c#L308

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Bartosz Golaszewski]

On Mon, May 11, 2026 at 4:31 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon May 11, 2026 at 4:17 PM CEST, Bartosz Golaszewski wrote:
> > On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich <dakr@kernel.org> said:
> >> On Mon May 11, 2026 at 9:33 AM CEST, Bartosz Golaszewski wrote:
> >>> Danilo: if there are no further comments, can you pick it up for v7.1?
> >>
> >> It seems that sashiko has a valid concern in [1]; can you confirm?
> >>
> >> [1] https://sashiko.dev/#/patchset/20260430-swnode-remove-on-dev-unreg-v4-0-01574da0aed3%40oss.qualcomm.com
> >>
> >
> > Yes, I explained it here[1]. Basically it's similar to how we need to call
> > platform_device_add_data() for devices created with platform_device_alloc().
> >
> > We can consider adding platform_device_add_software_node() once there's
> > a potential user but for now I'd just leave it like this.
>
> But there are users that already need this, no? There is Xe [1] and Surface GPE
> [2], or am I missing something?
>
> [1] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/gpu/drm/xe/xe_i2c.c#L99
> [2] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/platform/surface/surface_gpe.c#L308

Right, I was not aware of these. That could indeed cause a regression.
I'd like to fix the problem in v7.1 but also keep it minimal so adding
platform_device_add_software_node() and updating drivers to using it
may be the next step but for now: how about adding
platform_device_release_full() which would call
device_remove_software_node() and then the existing
platform_device_release()? We'd replace the .release callback of
struct device in platform_device_register_full() but if the user just
uses platform_device_alloc(), they would keep the regular .release()
that doesn't remove the software node?

That would go into v7.1 and then I'd provide
platform_device_add_software_node(), use it in all drivers that need
it, and then we'd remove platform_device_release_full() and go back to
a single, unified release callback?

Does it make sense?

Bart

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Danilo Krummrich]

On Mon May 11, 2026 at 4:50 PM CEST, Bartosz Golaszewski wrote:
> On Mon, May 11, 2026 at 4:31 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> On Mon May 11, 2026 at 4:17 PM CEST, Bartosz Golaszewski wrote:
>> > On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich <dakr@kernel.org> said:
>> >> On Mon May 11, 2026 at 9:33 AM CEST, Bartosz Golaszewski wrote:
>> >>> Danilo: if there are no further comments, can you pick it up for v7.1?
>> >>
>> >> It seems that sashiko has a valid concern in [1]; can you confirm?
>> >>
>> >> [1] https://sashiko.dev/#/patchset/20260430-swnode-remove-on-dev-unreg-v4-0-01574da0aed3%40oss.qualcomm.com
>> >>
>> >
>> > Yes, I explained it here[1]. Basically it's similar to how we need to call
>> > platform_device_add_data() for devices created with platform_device_alloc().
>> >
>> > We can consider adding platform_device_add_software_node() once there's
>> > a potential user but for now I'd just leave it like this.
>>
>> But there are users that already need this, no? There is Xe [1] and Surface GPE
>> [2], or am I missing something?
>>
>> [1] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/gpu/drm/xe/xe_i2c.c#L99
>> [2] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/platform/surface/surface_gpe.c#L308
>
> Right, I was not aware of these. That could indeed cause a regression.
> I'd like to fix the problem in v7.1 but also keep it minimal so adding
> platform_device_add_software_node() and updating drivers to using it
> may be the next step but for now: how about adding
> platform_device_release_full() which would call
> device_remove_software_node() and then the existing
> platform_device_release()? We'd replace the .release callback of
> struct device in platform_device_register_full() but if the user just
> uses platform_device_alloc(), they would keep the regular .release()
> that doesn't remove the software node?
>
> That would go into v7.1 and then I'd provide
> platform_device_add_software_node(), use it in all drivers that need
> it, and then we'd remove platform_device_release_full() and go back to
> a single, unified release callback?
>
> Does it make sense?

If it is just those two (and at least I did not find any other drivers) it might
be easier to just add platform_device_add_software_node(), use it within those
two drivers and still land it for v7.1.

I feel like this change turns out simpler and less error prone than the above
workaround to keep changes local to the driver core.

Re: [PATCH v4 1/3] driver core: platform: remove software node on release()

[Bartosz Golaszewski]

On Mon, May 11, 2026 at 5:04 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon May 11, 2026 at 4:50 PM CEST, Bartosz Golaszewski wrote:
> > On Mon, May 11, 2026 at 4:31 PM Danilo Krummrich <dakr@kernel.org> wrote:
> >>
> >> On Mon May 11, 2026 at 4:17 PM CEST, Bartosz Golaszewski wrote:
> >> > On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich <dakr@kernel.org> said:
> >> >> On Mon May 11, 2026 at 9:33 AM CEST, Bartosz Golaszewski wrote:
> >> >>> Danilo: if there are no further comments, can you pick it up for v7.1?
> >> >>
> >> >> It seems that sashiko has a valid concern in [1]; can you confirm?
> >> >>
> >> >> [1] https://sashiko.dev/#/patchset/20260430-swnode-remove-on-dev-unreg-v4-0-01574da0aed3%40oss.qualcomm.com
> >> >>
> >> >
> >> > Yes, I explained it here[1]. Basically it's similar to how we need to call
> >> > platform_device_add_data() for devices created with platform_device_alloc().
> >> >
> >> > We can consider adding platform_device_add_software_node() once there's
> >> > a potential user but for now I'd just leave it like this.
> >>
> >> But there are users that already need this, no? There is Xe [1] and Surface GPE
> >> [2], or am I missing something?
> >>
> >> [1] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/gpu/drm/xe/xe_i2c.c#L99
> >> [2] https://elixir.bootlin.com/linux/v7.1-rc2/source/drivers/platform/surface/surface_gpe.c#L308
> >
> > Right, I was not aware of these. That could indeed cause a regression.
> > I'd like to fix the problem in v7.1 but also keep it minimal so adding
> > platform_device_add_software_node() and updating drivers to using it
> > may be the next step but for now: how about adding
> > platform_device_release_full() which would call
> > device_remove_software_node() and then the existing
> > platform_device_release()? We'd replace the .release callback of
> > struct device in platform_device_register_full() but if the user just
> > uses platform_device_alloc(), they would keep the regular .release()
> > that doesn't remove the software node?
> >
> > That would go into v7.1 and then I'd provide
> > platform_device_add_software_node(), use it in all drivers that need
> > it, and then we'd remove platform_device_release_full() and go back to
> > a single, unified release callback?
> >
> > Does it make sense?
>
> If it is just those two (and at least I did not find any other drivers) it might
> be easier to just add platform_device_add_software_node(), use it within those
> two drivers and still land it for v7.1.
>
> I feel like this change turns out simpler and less error prone than the above
> workaround to keep changes local to the driver core.

Sounds good, I'll do this then.

Bart