[PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Bartosz Golaszewski]

The software node representing the GPIO controller to consumers is
"dangling": it's not really attached to the device. The GPIO lookup
relies on matching the name of the node to the chip's label. Set it as
the secondary firmware node of the platform device to enable proper
fwnode-based GPIO lookup.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
---
 drivers/platform/x86/barco-p50-gpio.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
index 2a6d8607c402..5f4ffa584295 100644
--- a/drivers/platform/x86/barco-p50-gpio.c
+++ b/drivers/platform/x86/barco-p50-gpio.c
@@ -365,6 +365,8 @@ static int p50_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
 
+	set_secondary_fwnode(&pdev->dev, software_node_fwnode(&gpiochip_node));
+
 	led_info.fwnode = software_node_fwnode(&gpio_leds_node);
 	p50->leds_pdev = platform_device_register_full(&led_info);
 	if (IS_ERR(p50->leds_pdev)) {
-- 
2.47.3

Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Dmitry Torokhov]

On Tue, Mar 31, 2026 at 01:28:19PM +0200, Bartosz Golaszewski wrote:
> The software node representing the GPIO controller to consumers is
> "dangling": it's not really attached to the device. The GPIO lookup
> relies on matching the name of the node to the chip's label. Set it as
> the secondary firmware node of the platform device to enable proper
> fwnode-based GPIO lookup.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> ---
>  drivers/platform/x86/barco-p50-gpio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
> index 2a6d8607c402..5f4ffa584295 100644
> --- a/drivers/platform/x86/barco-p50-gpio.c
> +++ b/drivers/platform/x86/barco-p50-gpio.c
> @@ -365,6 +365,8 @@ static int p50_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
>  
> +	set_secondary_fwnode(&pdev->dev, software_node_fwnode(&gpiochip_node));
> +
>  	led_info.fwnode = software_node_fwnode(&gpio_leds_node);
>  	p50->leds_pdev = platform_device_register_full(&led_info);
>  	if (IS_ERR(p50->leds_pdev)) {

I looks like http://sashiko.dev patch critique is on point:

"
Is the software node attached too late to take effect?

In the probe function, devm_gpiochip_add_data() is called before this
set_secondary_fwnode() call. During GPIO chip registration, the gpiolib
core snapshots the parent device's fwnode.

Because the secondary fwnode is not yet set on pdev->dev when this snapshot
happens, the GPIO device is registered with a NULL fwnode, which seems to
defeat the purpose of enabling fwnode-based lookups.

If the order is reversed, would we need to tie the software node
registration to devres (e.g., via devm_add_action_or_reset)? Otherwise, a
manual software_node_unregister_node_group() in the probe error path might
free the software node while the devm-managed gpiochip still holds a pointer
to it.

Additionally, could this leave a dangling pointer on probe failure or driver
unbind?

If a subsequent step fails (like registering keys_pdev), the probe error
path calls software_node_unregister_node_group(p50_swnodes), which frees
the underlying memory.

Because set_secondary_fwnode(&pdev->dev, NULL) is never called to clear it,
pdev->dev.fwnode would point to freed memory. Any subsequent access to the
device's firmware node could trigger a use-after-free.
"

Thanks.

-- 
Dmitry

Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Bartosz Golaszewski]

On Wed, 1 Apr 2026 04:09:21 +0200, Dmitry Torokhov
<dmitry.torokhov@gmail.com> said:
> On Tue, Mar 31, 2026 at 01:28:19PM +0200, Bartosz Golaszewski wrote:
>> The software node representing the GPIO controller to consumers is
>> "dangling": it's not really attached to the device. The GPIO lookup
>> relies on matching the name of the node to the chip's label. Set it as
>> the secondary firmware node of the platform device to enable proper
>> fwnode-based GPIO lookup.
>>
>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
>> ---
>>  drivers/platform/x86/barco-p50-gpio.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
>> index 2a6d8607c402..5f4ffa584295 100644
>> --- a/drivers/platform/x86/barco-p50-gpio.c
>> +++ b/drivers/platform/x86/barco-p50-gpio.c
>> @@ -365,6 +365,8 @@ static int p50_gpio_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
>>
>> +	set_secondary_fwnode(&pdev->dev, software_node_fwnode(&gpiochip_node));
>> +
>>  	led_info.fwnode = software_node_fwnode(&gpio_leds_node);
>>  	p50->leds_pdev = platform_device_register_full(&led_info);
>>  	if (IS_ERR(p50->leds_pdev)) {
>
> I looks like http://sashiko.dev patch critique is on point:
>

Ok, let's unpack it.

> "
> Is the software node attached too late to take effect?
>
> In the probe function, devm_gpiochip_add_data() is called before this
> set_secondary_fwnode() call. During GPIO chip registration, the gpiolib
> core snapshots the parent device's fwnode.
>

What does it mean "to snapshot" the parent's fwnode?

What happens is this:

static struct fwnode_handle *gpiochip_choose_fwnode(struct gpio_chip *gc)
{
	if (gc->fwnode)
		return gc->fwnode;

	if (gc->parent)
		return dev_fwnode(gc->parent);

	return NULL;
}

gc->fwnode is NULL so we set gc->parent as the GPIO controller's fwnode.

> Because the secondary fwnode is not yet set on pdev->dev when this snapshot
> happens, the GPIO device is registered with a NULL fwnode, which seems to
> defeat the purpose of enabling fwnode-based lookups.
>

Sashiko is completely wrong here: not only is the device registered with the
parent's fwnode assigned, the secondary fwnode is a property of the *fwnode*,
not of the device. We set the secondary fwnode of the parent's real fwnode.
This is carried over to the GPIO controller's device properties even after
it's been created.

> If the order is reversed, would we need to tie the software node
> registration to devres (e.g., via devm_add_action_or_reset)? Otherwise, a
> manual software_node_unregister_node_group() in the probe error path might
> free the software node while the devm-managed gpiochip still holds a pointer
> to it.
>

This one is valid. It should be a separate fix. remove() runs before devres
release.

> Additionally, could this leave a dangling pointer on probe failure or driver
> unbind?
>
> If a subsequent step fails (like registering keys_pdev), the probe error
> path calls software_node_unregister_node_group(p50_swnodes), which frees
> the underlying memory.
>

Scheduling devres actions following the initialization (reverse) order would
help.

> Because set_secondary_fwnode(&pdev->dev, NULL) is never called to clear it,
> pdev->dev.fwnode would point to freed memory. Any subsequent access to the
> device's firmware node could trigger a use-after-free.
> "
>

Makes sense too.

Bart

Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Dmitry Torokhov]

On Thu, Apr 02, 2026 at 01:29:35AM -0700, Bartosz Golaszewski wrote:
> On Wed, 1 Apr 2026 04:09:21 +0200, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> said:
> > On Tue, Mar 31, 2026 at 01:28:19PM +0200, Bartosz Golaszewski wrote:
> >> The software node representing the GPIO controller to consumers is
> >> "dangling": it's not really attached to the device. The GPIO lookup
> >> relies on matching the name of the node to the chip's label. Set it as
> >> the secondary firmware node of the platform device to enable proper
> >> fwnode-based GPIO lookup.
> >>
> >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> >> ---
> >>  drivers/platform/x86/barco-p50-gpio.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
> >> index 2a6d8607c402..5f4ffa584295 100644
> >> --- a/drivers/platform/x86/barco-p50-gpio.c
> >> +++ b/drivers/platform/x86/barco-p50-gpio.c
> >> @@ -365,6 +365,8 @@ static int p50_gpio_probe(struct platform_device *pdev)
> >>  	if (ret)
> >>  		return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
> >>
> >> +	set_secondary_fwnode(&pdev->dev, software_node_fwnode(&gpiochip_node));
> >> +
> >>  	led_info.fwnode = software_node_fwnode(&gpio_leds_node);
> >>  	p50->leds_pdev = platform_device_register_full(&led_info);
> >>  	if (IS_ERR(p50->leds_pdev)) {
> >
> > I looks like http://sashiko.dev patch critique is on point:
> >
> 
> Ok, let's unpack it.
> 
> > "
> > Is the software node attached too late to take effect?
> >
> > In the probe function, devm_gpiochip_add_data() is called before this
> > set_secondary_fwnode() call. During GPIO chip registration, the gpiolib
> > core snapshots the parent device's fwnode.
> >
> 
> What does it mean "to snapshot" the parent's fwnode?
> 
> What happens is this:
> 
> static struct fwnode_handle *gpiochip_choose_fwnode(struct gpio_chip *gc)
> {
> 	if (gc->fwnode)
> 		return gc->fwnode;
> 
> 	if (gc->parent)
> 		return dev_fwnode(gc->parent);
> 
> 	return NULL;
> }
> 
> gc->fwnode is NULL so we set gc->parent as the GPIO controller's fwnode.

However if parent does not have fwnode assigned (dev->fwnode is NULL)
this will return NULL as well. This effectively severs the link between
the gpiochip and the parent device.

> 
> > Because the secondary fwnode is not yet set on pdev->dev when this snapshot
> > happens, the GPIO device is registered with a NULL fwnode, which seems to
> > defeat the purpose of enabling fwnode-based lookups.
> >
> 
> Sashiko is completely wrong here: not only is the device registered with the
> parent's fwnode assigned, the secondary fwnode is a property of the *fwnode*,
> not of the device. We set the secondary fwnode of the parent's real fwnode.
> This is carried over to the GPIO controller's device properties even after
> it's been created.

I do not think it is wrong. If the parent does not have fwnode assigned
(and it does not) then you do not have a fwnode object to attach a
secondary node to. The node you are attaching as a secondary becomes
primary, but it is *too late*. The link has been severed, gpiochip's
fwnode is a NULL pointer and has no idea that the parent now has a valid
fwnode.

I think this woudl be a better approach:

@@ -426,7 +419,6 @@ MODULE_DEVICE_TABLE(dmi, dmi_ids);
 
 static int __init p50_module_init(void)
 {
-	struct resource res = DEFINE_RES_IO(P50_GPIO_IO_PORT_BASE, P50_PORT_CMD + 1);
 	int ret;
 
 	if (!dmi_first_match(dmi_ids))
@@ -436,7 +428,15 @@ static int __init p50_module_init(void)
 	if (ret)
 		return ret;
 
-	gpio_pdev = platform_device_register_simple(DRIVER_NAME, PLATFORM_DEVID_NONE, &res, 1);
+	gpio_pdev = platform_device_register_full(&(struct platform_device_info){
+		.name = DRIVER_NAME,
+		.id = PLATFORM_DEVID_NONE,
+		.res = (const struct resource[]){
+			DEFINE_RES_IO(P50_GPIO_IO_PORT_BASE, P50_PORT_CMD + 1),
+		},
+		.num_res = 1,
+		.swnode = &gpiochip_node,
+	});
 	if (IS_ERR(gpio_pdev)) {
 		pr_err("failed registering %s: %ld\n", DRIVER_NAME, PTR_ERR(gpio_pdev));
 		platform_driver_unregister(&p50_gpio_driver);

This way you start with the right node right away.

Thanks.

-- 
Dmitry

Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Bartosz Golaszewski]

On Thu, Apr 2, 2026 at 5:53 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Apr 02, 2026 at 01:29:35AM -0700, Bartosz Golaszewski wrote:
> > On Wed, 1 Apr 2026 04:09:21 +0200, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> said:
> > > On Tue, Mar 31, 2026 at 01:28:19PM +0200, Bartosz Golaszewski wrote:
> > >> The software node representing the GPIO controller to consumers is
> > >> "dangling": it's not really attached to the device. The GPIO lookup
> > >> relies on matching the name of the node to the chip's label. Set it as
> > >> the secondary firmware node of the platform device to enable proper
> > >> fwnode-based GPIO lookup.
> > >>
> > >> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>
> > >> ---
> > >>  drivers/platform/x86/barco-p50-gpio.c | 2 ++
> > >>  1 file changed, 2 insertions(+)
> > >>
> > >> diff --git a/drivers/platform/x86/barco-p50-gpio.c b/drivers/platform/x86/barco-p50-gpio.c
> > >> index 2a6d8607c402..5f4ffa584295 100644
> > >> --- a/drivers/platform/x86/barco-p50-gpio.c
> > >> +++ b/drivers/platform/x86/barco-p50-gpio.c
> > >> @@ -365,6 +365,8 @@ static int p50_gpio_probe(struct platform_device *pdev)
> > >>    if (ret)
> > >>            return dev_err_probe(&pdev->dev, ret, "failed to register software nodes");
> > >>
> > >> +  set_secondary_fwnode(&pdev->dev, software_node_fwnode(&gpiochip_node));
> > >> +
> > >>    led_info.fwnode = software_node_fwnode(&gpio_leds_node);
> > >>    p50->leds_pdev = platform_device_register_full(&led_info);
> > >>    if (IS_ERR(p50->leds_pdev)) {
> > >
> > > I looks like http://sashiko.dev patch critique is on point:
> > >
> >
> > Ok, let's unpack it.
> >
> > > "
> > > Is the software node attached too late to take effect?
> > >
> > > In the probe function, devm_gpiochip_add_data() is called before this
> > > set_secondary_fwnode() call. During GPIO chip registration, the gpiolib
> > > core snapshots the parent device's fwnode.
> > >
> >
> > What does it mean "to snapshot" the parent's fwnode?
> >
> > What happens is this:
> >
> > static struct fwnode_handle *gpiochip_choose_fwnode(struct gpio_chip *gc)
> > {
> >       if (gc->fwnode)
> >               return gc->fwnode;
> >
> >       if (gc->parent)
> >               return dev_fwnode(gc->parent);
> >
> >       return NULL;
> > }
> >
> > gc->fwnode is NULL so we set gc->parent as the GPIO controller's fwnode.
>
> However if parent does not have fwnode assigned (dev->fwnode is NULL)
> this will return NULL as well. This effectively severs the link between
> the gpiochip and the parent device.
>
> >
> > > Because the secondary fwnode is not yet set on pdev->dev when this snapshot
> > > happens, the GPIO device is registered with a NULL fwnode, which seems to
> > > defeat the purpose of enabling fwnode-based lookups.
> > >
> >
> > Sashiko is completely wrong here: not only is the device registered with the
> > parent's fwnode assigned, the secondary fwnode is a property of the *fwnode*,
> > not of the device. We set the secondary fwnode of the parent's real fwnode.
> > This is carried over to the GPIO controller's device properties even after
> > it's been created.
>
> I do not think it is wrong. If the parent does not have fwnode assigned
> (and it does not) then you do not have a fwnode object to attach a
> secondary node to. The node you are attaching as a secondary becomes
> primary, but it is *too late*. The link has been severed, gpiochip's
> fwnode is a NULL pointer and has no idea that the parent now has a valid
> fwnode.
>
> I think this woudl be a better approach:
>
> @@ -426,7 +419,6 @@ MODULE_DEVICE_TABLE(dmi, dmi_ids);
>
>  static int __init p50_module_init(void)
>  {
> -       struct resource res = DEFINE_RES_IO(P50_GPIO_IO_PORT_BASE, P50_PORT_CMD + 1);
>         int ret;
>
>         if (!dmi_first_match(dmi_ids))
> @@ -436,7 +428,15 @@ static int __init p50_module_init(void)
>         if (ret)
>                 return ret;
>
> -       gpio_pdev = platform_device_register_simple(DRIVER_NAME, PLATFORM_DEVID_NONE, &res, 1);
> +       gpio_pdev = platform_device_register_full(&(struct platform_device_info){
> +               .name = DRIVER_NAME,
> +               .id = PLATFORM_DEVID_NONE,
> +               .res = (const struct resource[]){
> +                       DEFINE_RES_IO(P50_GPIO_IO_PORT_BASE, P50_PORT_CMD + 1),
> +               },
> +               .num_res = 1,
> +               .swnode = &gpiochip_node,
> +       });
>         if (IS_ERR(gpio_pdev)) {
>                 pr_err("failed registering %s: %ld\n", DRIVER_NAME, PTR_ERR(gpio_pdev));
>                 platform_driver_unregister(&p50_gpio_driver);
>
> This way you start with the right node right away.
>
> Thanks.
>
> --
> Dmitry

Ah, maybe sashiko wasn't wrong after all but it definitely could do a
better job explaining the actual issue. I missed the fact that this is
not a device described in firmware and as such doesn't have a firmware
node. I see it now. Ok, I'll fix it in v2. In fact, I'll probably wait
until v7.1-rc1 and use the new .swnode field.

Bart

Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Dmitry Torokhov]

On Thu, Apr 02, 2026 at 06:39:16PM +0200, Bartosz Golaszewski wrote:
> 
> Ah, maybe sashiko wasn't wrong after all but it definitely could do a
> better job explaining the actual issue.

Yeah, I agree. OTOH it did notice the problem. I missed it when I first
read the patch.

> I missed the fact that this is
> not a device described in firmware and as such doesn't have a firmware
> node. I see it now. Ok, I'll fix it in v2. In fact, I'll probably wait
> until v7.1-rc1 and use the new .swnode field.

Danilo tagged the commit adding swnode to platform_device_info so you
can ask to pull it in when applying the patch:

https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tag/?h=platform_device_info_swnode-7.1-rc1

Thanks.

-- 
Dmitry

Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device

[Bartosz Golaszewski]

On Thu, Apr 2, 2026 at 7:30 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Thu, Apr 02, 2026 at 06:39:16PM +0200, Bartosz Golaszewski wrote:
> >
> > Ah, maybe sashiko wasn't wrong after all but it definitely could do a
> > better job explaining the actual issue.
>
> Yeah, I agree. OTOH it did notice the problem. I missed it when I first
> read the patch.
>
> > I missed the fact that this is
> > not a device described in firmware and as such doesn't have a firmware
> > node. I see it now. Ok, I'll fix it in v2. In fact, I'll probably wait
> > until v7.1-rc1 and use the new .swnode field.
>
> Danilo tagged the commit adding swnode to platform_device_info so you
> can ask to pull it in when applying the patch:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git/tag/?h=platform_device_info_swnode-7.1-rc1
>

I'm not a maintainer for x86 platform drivers. It's not urgent, I'll
respin it after v7.1-rc1.

Bart