[PATCH] pinctrl: bcm2835: Fix a few error paths

[PATCH] pinctrl: bcm2835: Fix a few error paths

[Florian Fainelli]

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(-)

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 = {
-- 
2.25.1

Re: [PATCH] pinctrl: bcm2835: Fix a few error paths

[Andy Shevchenko]

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;
> +		}
>  	}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: bcm2835: Fix a few error paths

[Florian Fainelli]

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.
-- 
Florian

Re: [PATCH] pinctrl: bcm2835: Fix a few error paths

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] pinctrl: bcm2835: Fix a few error paths

[Linus Walleij]

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