Re: [PATCH v4 1/3] driver core: platform: remove software node on release()
On Mon, May 11, 2026 at 5:04 PM Danilo Krummrich 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 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 > >> > 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
Re: [PATCH v4 1/3] driver core: platform: remove software node on release()
On Mon May 11, 2026 at 4:50 PM CEST, Bartosz Golaszewski wrote: > On Mon, May 11, 2026 at 4:31 PM Danilo Krummrich 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 >> > 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()
On Mon, May 11, 2026 at 4:31 PM Danilo Krummrich 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 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()
On Mon May 11, 2026 at 4:17 PM CEST, Bartosz Golaszewski wrote: > On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich 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()
On Mon, 11 May 2026 16:00:23 +0200, Danilo Krummrich 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+1os6t...@mail.gmail.com/
Re: [PATCH v4 1/3] driver core: platform: remove software node on release()
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()
On Thu, Apr 30, 2026 at 2:38 PM Bartosz Golaszewski wrote: > > On Thu, Apr 30, 2026 at 2:02 PM Andy Shevchenko > 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()
On Thu, Apr 30, 2026 at 2:02 PM Andy Shevchenko 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
Re: [PATCH v4 1/3] driver core: platform: remove software node on release()
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? 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? -- With Best Regards, Andy Shevchenko
