Messages in this thread |  | | From | Bartosz Golaszewski <> | | Date | Thu, 2 Apr 2026 01:29:35 -0700 | | Subject | Re: [PATCH] platform/x86: barco-p50-gpio: attach software node to its target GPIO device |
| |
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
|  |