[1/3] pwm: lpc18xx-sct: Initialize driver data and hardware before pwmchip_add()

When a driver calls pwmchip_add() it has to be prepared to immediately
get its callbacks called. So move allocation of driver data and hardware
initialization before the call to pwmchip_add().

This fixes a potential NULL pointer exception and a race condition on
register writes.

Fixes: 841e6f90bb78 ("pwm: NXP LPC18xx PWM/SCT driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)


base-commit: d2f38a3c6507b2520101f9a3807ed98f1bdc545a

On Wed, Nov 10, 2021 at 09:49:48AM +0100, Uwe Kleine-König wrote:
> When a driver calls pwmchip_add() it has to be prepared to immediately
> get its callbacks called. So move allocation of driver data and hardware
> initialization before the call to pwmchip_add().
> 
> This fixes a potential NULL pointer exception and a race condition on
> register writes.
> 
> Fixes: 841e6f90bb78 ("pwm: NXP LPC18xx PWM/SCT driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-lpc18xx-sct.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> index 8e461f3baa05..8cc8ae16553c 100644
> --- a/drivers/pwm/pwm-lpc18xx-sct.c
> +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> @@ -395,12 +395,6 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
>  	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
>  			   BIT(lpc18xx_pwm->period_event));
>  
> -	ret = pwmchip_add(&lpc18xx_pwm->chip);
> -	if (ret < 0) {
> -		dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
> -		goto disable_pwmclk;
> -	}
> -
>  	for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
>  		struct lpc18xx_pwm_data *data;
>  
> @@ -410,14 +404,12 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
>  				    GFP_KERNEL);
>  		if (!data) {
>  			ret = -ENOMEM;
> -			goto remove_pwmchip;
> +			goto disable_pwmclk;
>  		}
>  
>  		pwm_set_chip_data(pwm, data);
>  	}
>  
> -	platform_set_drvdata(pdev, lpc18xx_pwm);
> -
>  	val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
>  	val &= ~LPC18XX_PWM_BIDIR;
>  	val &= ~LPC18XX_PWM_CTRL_HALT;
> @@ -425,10 +417,16 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
>  	val |= LPC18XX_PWM_PRE(0);
>  	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
>  
> +	ret = pwmchip_add(&lpc18xx_pwm->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
> +		goto disable_pwmclk;
> +	}
> +
> +	platform_set_drvdata(pdev, lpc18xx_pwm);

Is there any particular reason why this needs to move? The driver only
call platform_get_drvdata() from the ->remove() implementation at this
point, so this should be fine, but it's possible that it would at some
future point try to access this data from some code path that may get
called as part of pwmchip_add(), so doing this prior to chip addition
may be a better option.

No need to resend, I can fix that up as I apply if there are no strong
reasons to keep this after pwmchip_add().

Thierry

On Tue, Feb 01, 2022 at 08:47:26AM +0100, Thierry Reding wrote:
> On Wed, Nov 10, 2021 at 09:49:48AM +0100, Uwe Kleine-König wrote:
> > When a driver calls pwmchip_add() it has to be prepared to immediately
> > get its callbacks called. So move allocation of driver data and hardware
> > initialization before the call to pwmchip_add().
> > 
> > This fixes a potential NULL pointer exception and a race condition on
> > register writes.
> > 
> > Fixes: 841e6f90bb78 ("pwm: NXP LPC18xx PWM/SCT driver")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-lpc18xx-sct.c | 20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
> > index 8e461f3baa05..8cc8ae16553c 100644
> > --- a/drivers/pwm/pwm-lpc18xx-sct.c
> > +++ b/drivers/pwm/pwm-lpc18xx-sct.c
> > @@ -395,12 +395,6 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
> >  	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
> >  			   BIT(lpc18xx_pwm->period_event));
> >  
> > -	ret = pwmchip_add(&lpc18xx_pwm->chip);
> > -	if (ret < 0) {
> > -		dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
> > -		goto disable_pwmclk;
> > -	}
> > -
> >  	for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
> >  		struct lpc18xx_pwm_data *data;
> >  
> > @@ -410,14 +404,12 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
> >  				    GFP_KERNEL);
> >  		if (!data) {
> >  			ret = -ENOMEM;
> > -			goto remove_pwmchip;
> > +			goto disable_pwmclk;
> >  		}
> >  
> >  		pwm_set_chip_data(pwm, data);
> >  	}
> >  
> > -	platform_set_drvdata(pdev, lpc18xx_pwm);
> > -
> >  	val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
> >  	val &= ~LPC18XX_PWM_BIDIR;
> >  	val &= ~LPC18XX_PWM_CTRL_HALT;
> > @@ -425,10 +417,16 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
> >  	val |= LPC18XX_PWM_PRE(0);
> >  	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
> >  
> > +	ret = pwmchip_add(&lpc18xx_pwm->chip);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
> > +		goto disable_pwmclk;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, lpc18xx_pwm);
> 
> Is there any particular reason why this needs to move? The driver only
> call platform_get_drvdata() from the ->remove() implementation at this
> point, so this should be fine, but it's possible that it would at some
> future point try to access this data from some code path that may get
> called as part of pwmchip_add(), so doing this prior to chip addition
> may be a better option.
> 
> No need to resend, I can fix that up as I apply if there are no strong
> reasons to keep this after pwmchip_add().

What I actually did was:

 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_LIMIT,
 			   BIT(lpc18xx_pwm->period_event));
 
+	for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
+		struct lpc18xx_pwm_data *data;
+
+		pwm = &lpc18xx_pwm->chip.pwms[i];
+
+		data = devm_kzalloc(lpc18xx_pwm->dev, sizeof(*data),
+				    GFP_KERNEL);
+		if (!data) {
+			ret = -ENOMEM;
+			goto disable_pwmclk;
+		}
+
+		pwm_set_chip_data(pwm, data);
+	}
+
+	val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
+	val &= ~LPC18XX_PWM_BIDIR;
+	val &= ~LPC18XX_PWM_CTRL_HALT;
+	val &= ~LPC18XX_PWM_PRE_MASK;
+	val |= LPC18XX_PWM_PRE(0);
+	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
+
 	ret = pwmchip_add(&lpc18xx_pwm->chip);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
 		goto disable_pwmclk;
 	}
 
-	for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
-		struct lpc18xx_pwm_data *data;
-
-		pwm = &lpc18xx_pwm->chip.pwms[i];
-
-		data = devm_kzalloc(lpc18xx_pwm->dev, sizeof(*data),
-				    GFP_KERNEL);
-		if (!data) {
-			ret = -ENOMEM;
-			goto remove_pwmchip;
-		}
-
-		pwm_set_chip_data(pwm, data);
-	}
-
 	platform_set_drvdata(pdev, lpc18xx_pwm);
 
-	val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
-	val &= ~LPC18XX_PWM_BIDIR;
-	val &= ~LPC18XX_PWM_CTRL_HALT;
-	val &= ~LPC18XX_PWM_PRE_MASK;
-	val |= LPC18XX_PWM_PRE(0);
-	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CTRL, val);
-
 	return 0;
 
-remove_pwmchip:
-	pwmchip_remove(&lpc18xx_pwm->chip);
 disable_pwmclk:
 	clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
 	return ret;
 }


and git simplified that making it look as if platform_set_drvdata was
moved.

Best regards
Uwe