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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2617 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]