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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
>> ---
>>  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