Message ID | 3f6fc17e-2ab4-43f2-b166-2393a369a263@web.de |
---|---|
State | New |
Headers | show |
Series | pinctrl: pistachio: Use scope-based resource management in pistachio_gpio_register() | expand |
On Wed, Jun 5, 2024 at 6:02 PM Markus Elfring <Markus.Elfring@web.de> wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Wed, 5 Jun 2024 17:46:52 +0200 > > Scope-based resource management became supported also for another > programming interface by contributions of Jonathan Cameron on 2024-02-17. > See also the commit 59ed5e2d505bf5f9b4af64d0021cd0c96aec1f7c ("device > property: Add cleanup.h based fwnode_handle_put() scope based cleanup."). > > * Thus use the attribute “__free(fwnode_handle)”. > > * Reduce the scope for the local variable “child”. > > * Omit explicit fwnode_handle_put() calls accordingly. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> Looks reasonable but I'd like Jonathan's and Andy's review tags on this. Yours, Linus Walleij
On Mon, 17 Jun 2024 10:32:03 +0200 Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Jun 5, 2024 at 6:02 PM Markus Elfring <Markus.Elfring@web.de> wrote: > > > From: Markus Elfring <elfring@users.sourceforge.net> > > Date: Wed, 5 Jun 2024 17:46:52 +0200 > > > > Scope-based resource management became supported also for another > > programming interface by contributions of Jonathan Cameron on 2024-02-17. > > See also the commit 59ed5e2d505bf5f9b4af64d0021cd0c96aec1f7c ("device > > property: Add cleanup.h based fwnode_handle_put() scope based cleanup."). > > > > * Thus use the attribute “__free(fwnode_handle)”. > > > > * Reduce the scope for the local variable “child”. > > > > * Omit explicit fwnode_handle_put() calls accordingly. > > > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > > Looks reasonable but I'd like Jonathan's and Andy's review tags on this. > > Yours, > Linus Walleij I took a look and it makes me a little nervous. Markus' patch isn't changing behaviour (I think) but i is mixing scoped handling with non scoped handling which I'm not keen on. I'm also struggling to understand how the code was right prior to this patch. In particular what cleans up the fwnode_handle after the point where Markus has used no_free_ptr()? Whilst it's assigned to the gpio_chip that chip hasn't been added when girq->parents is allocated so even if there is some magic path to clearing it up I'm not immediately seeing how that can be working here. So this feels like there are more gremlins hiding here and Markus' patch may not help flush them out. Jonathan
diff --git a/drivers/pinctrl/pinctrl-pistachio.c b/drivers/pinctrl/pinctrl-pistachio.c index 53408344927a..d0a18296fb27 100644 --- a/drivers/pinctrl/pinctrl-pistachio.c +++ b/drivers/pinctrl/pinctrl-pistachio.c @@ -1368,11 +1368,12 @@ static int pistachio_gpio_register(struct pistachio_pinctrl *pctl) for (i = 0; i < pctl->nbanks; i++) { char child_name[sizeof("gpioXX")]; - struct fwnode_handle *child; struct gpio_irq_chip *girq; snprintf(child_name, sizeof(child_name), "gpio%d", i); - child = device_get_named_child_node(pctl->dev, child_name); + + struct fwnode_handle *child __free(fwnode_handle) + = device_get_named_child_node(pctl->dev, child_name); if (!child) { dev_err(pctl->dev, "No node for bank %u\n", i); ret = -ENODEV; @@ -1380,7 +1381,6 @@ static int pistachio_gpio_register(struct pistachio_pinctrl *pctl) } if (!fwnode_property_present(child, "gpio-controller")) { - fwnode_handle_put(child); dev_err(pctl->dev, "No gpio-controller property for bank %u\n", i); ret = -ENODEV; @@ -1389,12 +1389,10 @@ static int pistachio_gpio_register(struct pistachio_pinctrl *pctl) ret = fwnode_irq_get(child, 0); if (ret < 0) { - fwnode_handle_put(child); dev_err(pctl->dev, "Failed to retrieve IRQ for bank %u\n", i); goto err; } if (!ret) { - fwnode_handle_put(child); dev_err(pctl->dev, "No IRQ for bank %u\n", i); ret = -EINVAL; goto err; @@ -1406,7 +1404,7 @@ static int pistachio_gpio_register(struct pistachio_pinctrl *pctl) bank->base = pctl->base + GPIO_BANK_BASE(i); bank->gpio_chip.parent = pctl->dev; - bank->gpio_chip.fwnode = child; + bank->gpio_chip.fwnode = no_free_ptr(child); girq = &bank->gpio_chip.irq; gpio_irq_chip_set_chip(girq, &pistachio_gpio_irq_chip);