| Message ID | 20250103030402.81954-2-koichiro.den@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: sim, virtuser: minor usability enhancements | expand |
On Fri, Jan 03, 2025 at 02:15:17PM +0100, Bartosz Golaszewski wrote: > On Fri, Jan 3, 2025 at 4:04 AM Koichiro Den <koichiro.den@canonical.com> wrote: > > > > When a virtuser device is created via configfs and the probe fails due > > to an incorrect lookup table, the table is not removed. This prevents > > subsequent probe attempts from succeeding, even if the issue is > > corrected, unless the device is released. Additionally, cleanup is also > > needed in the less likely case of platform_device_register_full() > > failure. > > > > Ensure the lookup table is removed whenever the device activation fails. > > > > Fixes: 91581c4b3f29 ("gpio: virtuser: new virtual testing driver for the GPIO API") > > Signed-off-by: Koichiro Den <koichiro.den@canonical.com> > > --- > > drivers/gpio/gpio-virtuser.c | 25 ++++++++++++++++--------- > > 1 file changed, 16 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c > > index 91b6352c957c..ec5abfebca3d 100644 > > --- a/drivers/gpio/gpio-virtuser.c > > +++ b/drivers/gpio/gpio-virtuser.c > > @@ -1487,10 +1487,8 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev) > > pdevinfo.fwnode = swnode; > > > > ret = gpio_virtuser_make_lookup_table(dev); > > - if (ret) { > > - fwnode_remove_software_node(swnode); > > - return ret; > > - } > > + if (ret) > > + goto err_remove_swnode; > > > > reinit_completion(&dev->probe_completion); > > dev->driver_bound = false; > > @@ -1498,23 +1496,32 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev) > > > > pdev = platform_device_register_full(&pdevinfo); > > if (IS_ERR(pdev)) { > > + ret = PTR_ERR(pdev); > > bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier); > > - fwnode_remove_software_node(swnode); > > - return PTR_ERR(pdev); > > + goto err_remove_lookup_table; > > } > > > > wait_for_completion(&dev->probe_completion); > > bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier); > > > > if (!dev->driver_bound) { > > - platform_device_unregister(pdev); > > - fwnode_remove_software_node(swnode); > > - return -ENXIO; > > + ret = -ENXIO; > > + goto err_unregister_pdev; > > } > > > > dev->pdev = pdev; > > > > return 0; > > + > > +err_unregister_pdev: > > + platform_device_unregister(pdev); > > +err_remove_lookup_table: > > + gpiod_remove_lookup_table(dev->lookup_table); > > + kfree(dev->lookup_table); > > Just one more thing: now we open-code this but the actual allocation > and adding of the table happens in a dedicated function. Can you > package these calls into their own function > (gpio_virtuser_remove_lookup_table() maybe?) and use it here and in > gpio_virtuser_device_deactivate()? Sounds reasonable and cleaner. Thanks again for reviewing, I'll send v3 soon. -Koichiro > > Bart > > > +err_remove_swnode: > > + fwnode_remove_software_node(swnode); > > + > > + return ret; > > } > > > > static void > > -- > > 2.43.0 > >
diff --git a/drivers/gpio/gpio-virtuser.c b/drivers/gpio/gpio-virtuser.c index 91b6352c957c..ec5abfebca3d 100644 --- a/drivers/gpio/gpio-virtuser.c +++ b/drivers/gpio/gpio-virtuser.c @@ -1487,10 +1487,8 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev) pdevinfo.fwnode = swnode; ret = gpio_virtuser_make_lookup_table(dev); - if (ret) { - fwnode_remove_software_node(swnode); - return ret; - } + if (ret) + goto err_remove_swnode; reinit_completion(&dev->probe_completion); dev->driver_bound = false; @@ -1498,23 +1496,32 @@ gpio_virtuser_device_activate(struct gpio_virtuser_device *dev) pdev = platform_device_register_full(&pdevinfo); if (IS_ERR(pdev)) { + ret = PTR_ERR(pdev); bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier); - fwnode_remove_software_node(swnode); - return PTR_ERR(pdev); + goto err_remove_lookup_table; } wait_for_completion(&dev->probe_completion); bus_unregister_notifier(&platform_bus_type, &dev->bus_notifier); if (!dev->driver_bound) { - platform_device_unregister(pdev); - fwnode_remove_software_node(swnode); - return -ENXIO; + ret = -ENXIO; + goto err_unregister_pdev; } dev->pdev = pdev; return 0; + +err_unregister_pdev: + platform_device_unregister(pdev); +err_remove_lookup_table: + gpiod_remove_lookup_table(dev->lookup_table); + kfree(dev->lookup_table); +err_remove_swnode: + fwnode_remove_software_node(swnode); + + return ret; } static void
When a virtuser device is created via configfs and the probe fails due to an incorrect lookup table, the table is not removed. This prevents subsequent probe attempts from succeeding, even if the issue is corrected, unless the device is released. Additionally, cleanup is also needed in the less likely case of platform_device_register_full() failure. Ensure the lookup table is removed whenever the device activation fails. Fixes: 91581c4b3f29 ("gpio: virtuser: new virtual testing driver for the GPIO API") Signed-off-by: Koichiro Den <koichiro.den@canonical.com> --- drivers/gpio/gpio-virtuser.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)