| Message ID | 20220127215033.267227-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: bcm2835: Fix a few error paths | expand |
On Thu, Jan 27, 2022 at 01:50:31PM -0800, Florian Fainelli wrote: > After commit 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio > hogs") a few error paths would not unwind properly the registration of > gpio ranges. Correct that by assigning a single error label and goto it > whenever we encounter a fatal error. > 1 file changed, 15 insertions(+), 8 deletions(-) While this seems legit per se, my eyes caught this: > if (!girq->parents) { > - pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); > - return -ENOMEM; > + err = -ENOMEM; > + goto out_remove; Non-devm.... > } > > if (is_7211) { > pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, > sizeof(*pc->wake_irq), > GFP_KERNEL); ...followed by devm. It means more ordering bugs in the ->remove() and error path are lurking around. Can you double check and be sure that we do not have a case where non-devm registration code followed by devm? If that is the case it has to be fixed as well. > - if (!pc->wake_irq) > - return -ENOMEM; > + if (!pc->wake_irq) { > + err = -ENOMEM; > + goto out_remove; > + } > }
On 1/28/2022 6:35 AM, Andy Shevchenko wrote: > On Thu, Jan 27, 2022 at 01:50:31PM -0800, Florian Fainelli wrote: >> After commit 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio >> hogs") a few error paths would not unwind properly the registration of >> gpio ranges. Correct that by assigning a single error label and goto it >> whenever we encounter a fatal error. > >> 1 file changed, 15 insertions(+), 8 deletions(-) > > While this seems legit per se, my eyes caught this: > > >> if (!girq->parents) { >> - pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); >> - return -ENOMEM; >> + err = -ENOMEM; >> + goto out_remove; > > Non-devm.... > >> } >> >> if (is_7211) { >> pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, >> sizeof(*pc->wake_irq), >> GFP_KERNEL); > > ...followed by devm. > > It means more ordering bugs in the ->remove() and error path are lurking > around. Can you double check and be sure that we do not have a case where > non-devm registration code followed by devm? It seems to me like we are fine with the patch as is, because: - girq->parents is allocated with devm - pc->wake_irq is allocated with devm - name is allocated with devm and those are the only variables being allocated for which we also process an error handling path.
On Fri, Jan 28, 2022 at 08:12:02AM -0800, Florian Fainelli wrote: > On 1/28/2022 6:35 AM, Andy Shevchenko wrote: > > On Thu, Jan 27, 2022 at 01:50:31PM -0800, Florian Fainelli wrote: > > > After commit 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio > > > hogs") a few error paths would not unwind properly the registration of > > > gpio ranges. Correct that by assigning a single error label and goto it > > > whenever we encounter a fatal error. > > > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > While this seems legit per se, my eyes caught this: > > > > > > > if (!girq->parents) { > > > - pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); > > > - return -ENOMEM; > > > + err = -ENOMEM; > > > + goto out_remove; > > > > Non-devm.... > > > > > } > > > if (is_7211) { > > > pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, > > > sizeof(*pc->wake_irq), > > > GFP_KERNEL); > > > > ...followed by devm. > > > > It means more ordering bugs in the ->remove() and error path are lurking > > around. Can you double check and be sure that we do not have a case where > > non-devm registration code followed by devm? > > It seems to me like we are fine with the patch as is, because: > > - girq->parents is allocated with devm > - pc->wake_irq is allocated with devm > - name is allocated with devm > > and those are the only variables being allocated for which we also process > an error handling path. Okay, thanks. My worries that it might be the case when the GPIO ranges have been removed by explicit call in ->remove() followed by some interrupt or so and oops or misbehaviour because of that.
On Thu, Jan 27, 2022 at 10:50 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > After commit 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio > hogs") a few error paths would not unwind properly the registration of > gpio ranges. Correct that by assigning a single error label and goto it > whenever we encounter a fatal error. > > Fixes: 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio hogs") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Patch applied. Yours, Linus Walleij
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index c4ebfa852b42..47e433e09c5c 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -1269,16 +1269,18 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) sizeof(*girq->parents), GFP_KERNEL); if (!girq->parents) { - pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); - return -ENOMEM; + err = -ENOMEM; + goto out_remove; } if (is_7211) { pc->wake_irq = devm_kcalloc(dev, BCM2835_NUM_IRQS, sizeof(*pc->wake_irq), GFP_KERNEL); - if (!pc->wake_irq) - return -ENOMEM; + if (!pc->wake_irq) { + err = -ENOMEM; + goto out_remove; + } } /* @@ -1306,8 +1308,10 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) len = strlen(dev_name(pc->dev)) + 16; name = devm_kzalloc(pc->dev, len, GFP_KERNEL); - if (!name) - return -ENOMEM; + if (!name) { + err = -ENOMEM; + goto out_remove; + } snprintf(name, len, "%s:bank%d", dev_name(pc->dev), i); @@ -1326,11 +1330,14 @@ static int bcm2835_pinctrl_probe(struct platform_device *pdev) err = gpiochip_add_data(&pc->gpio_chip, pc); if (err) { dev_err(dev, "could not add GPIO chip\n"); - pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); - return err; + goto out_remove; } return 0; + +out_remove: + pinctrl_remove_gpio_range(pc->pctl_dev, &pc->gpio_range); + return err; } static struct platform_driver bcm2835_pinctrl_driver = {
After commit 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio hogs") a few error paths would not unwind properly the registration of gpio ranges. Correct that by assigning a single error label and goto it whenever we encounter a fatal error. Fixes: 266423e60ea1 ("pinctrl: bcm2835: Change init order for gpio hogs") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- drivers/pinctrl/bcm/pinctrl-bcm2835.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)