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

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

[Uwe Kleine-König]

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);
+
 	return 0;
 
-remove_pwmchip:
-	pwmchip_remove(&lpc18xx_pwm->chip);
 disable_pwmclk:
 	clk_disable_unprepare(lpc18xx_pwm->pwm_clk);
 	return ret;

base-commit: d2f38a3c6507b2520101f9a3807ed98f1bdc545a
-- 
2.30.2

[PATCH 2/3] pwm: lpc18xx-sct: Reduce number of devm memory allocations

[Uwe Kleine-König]

Each devm allocations has an overhead of 24 bytes to store the related
struct devres_node additionally to the fragmentation of the allocator.
So allocating 16 struct lpc18xx_pwm_data (which only hold a single int)
adds quite some overhead. Instead put the per-channel data into the
driver data struct and allocate it in one go.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index 8cc8ae16553c..6cf02554066c 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -76,6 +76,8 @@
 #define LPC18XX_PWM_EVENT_PERIOD	0
 #define LPC18XX_PWM_EVENT_MAX		16
 
+#define LPC18XX_NUM_PWMS		16
+
 /* SCT conflict resolution */
 enum lpc18xx_pwm_res_action {
 	LPC18XX_PWM_RES_NONE,
@@ -101,6 +103,7 @@ struct lpc18xx_pwm_chip {
 	unsigned long event_map;
 	struct mutex res_lock;
 	struct mutex period_lock;
+	struct lpc18xx_pwm_data channeldata[LPC18XX_NUM_PWMS];
 };
 
 static inline struct lpc18xx_pwm_chip *
@@ -370,7 +373,7 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 
 	lpc18xx_pwm->chip.dev = &pdev->dev;
 	lpc18xx_pwm->chip.ops = &lpc18xx_pwm_ops;
-	lpc18xx_pwm->chip.npwm = 16;
+	lpc18xx_pwm->chip.npwm = LPC18XX_NUM_PWMS;
 
 	/* SCT counter must be in unify (32 bit) mode */
 	lpc18xx_pwm_writel(lpc18xx_pwm, LPC18XX_PWM_CONFIG,
@@ -400,12 +403,7 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 
 		pwm = &lpc18xx_pwm->chip.pwms[i];
 
-		data = devm_kzalloc(lpc18xx_pwm->dev, sizeof(*data),
-				    GFP_KERNEL);
-		if (!data) {
-			ret = -ENOMEM;
-			goto disable_pwmclk;
-		}
+		data = &lpc18xx_pwm->channeldata[i];
 
 		pwm_set_chip_data(pwm, data);
 	}
-- 
2.30.2

Re: [PATCH 2/3] pwm: lpc18xx-sct: Reduce number of devm memory allocations

[Thierry Reding]

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

On Wed, Nov 10, 2021 at 09:49:49AM +0100, Uwe Kleine-König wrote:
> Each devm allocations has an overhead of 24 bytes to store the related
> struct devres_node additionally to the fragmentation of the allocator.
> So allocating 16 struct lpc18xx_pwm_data (which only hold a single int)
> adds quite some overhead. Instead put the per-channel data into the
> driver data struct and allocate it in one go.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-lpc18xx-sct.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)

This could've been merged with patch 3 to make it a bit more obvious why
this is useful. Patch 2 itself is a bit half-baked without patch 3. But
I'll apply these anyway.

Thanks,
Thierry

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

[PATCH 3/3] pwm: lpc18xx-sct: Simplify driver by not using pwm_[gs]et_chip_data()

[Uwe Kleine-König]

The per-channel data is available directly in the driver data struct. So
use it without making use of pwm_[gs]et_chip_data().

The relevant change introduced by this patch to lpc18xx_pwm_disable() at
the assembler level (for an arm lpc18xx_defconfig build) is:

	push    {r3, r4, r5, lr}
	mov     r4, r0
	mov     r0, r1
	mov     r5, r1
	bl      0 <pwm_get_chip_data>
	ldr     r3, [r0, #0]

changes to

	ldr     r3, [r1, #8]
	push    {r4, lr}
	add.w   r3, r0, r3, lsl #2
	ldr     r3, [r3, #92]   ; 0x5c

So this reduces stack usage, has an improved runtime behavior because of
better pipeline usage, doesn't branch to an external function and the
generated code is a bit smaller occupying less memory.

The codesize of lpc18xx_pwm_probe() is reduced by 32 bytes.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index 6cf02554066c..b909096dba2f 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -166,7 +166,7 @@ static void lpc18xx_pwm_config_duty(struct pwm_chip *chip,
 				    struct pwm_device *pwm, int duty_ns)
 {
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
-	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+	struct lpc18xx_pwm_data *lpc18xx_data = &lpc18xx_pwm->channeldata[pwm->hwpwm];
 	u64 val;
 
 	val = (u64)duty_ns * lpc18xx_pwm->clk_rate;
@@ -236,7 +236,7 @@ static int lpc18xx_pwm_set_polarity(struct pwm_chip *chip,
 static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
-	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+	struct lpc18xx_pwm_data *lpc18xx_data = &lpc18xx_pwm->channeldata[pwm->hwpwm];
 	enum lpc18xx_pwm_res_action res_action;
 	unsigned int set_event, clear_event;
 
@@ -271,7 +271,7 @@ static int lpc18xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
-	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+	struct lpc18xx_pwm_data *lpc18xx_data = &lpc18xx_pwm->channeldata[pwm->hwpwm];
 
 	lpc18xx_pwm_writel(lpc18xx_pwm,
 			   LPC18XX_PWM_EVCTRL(lpc18xx_data->duty_event), 0);
@@ -282,7 +282,7 @@ static void lpc18xx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
-	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+	struct lpc18xx_pwm_data *lpc18xx_data = &lpc18xx_pwm->channeldata[pwm->hwpwm];
 	unsigned long event;
 
 	event = find_first_zero_bit(&lpc18xx_pwm->event_map,
@@ -303,7 +303,7 @@ static int lpc18xx_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
-	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
+	struct lpc18xx_pwm_data *lpc18xx_data = &lpc18xx_pwm->channeldata[pwm->hwpwm];
 
 	clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
 }
@@ -327,8 +327,7 @@ MODULE_DEVICE_TABLE(of, lpc18xx_pwm_of_match);
 static int lpc18xx_pwm_probe(struct platform_device *pdev)
 {
 	struct lpc18xx_pwm_chip *lpc18xx_pwm;
-	struct pwm_device *pwm;
-	int ret, i;
+	int ret;
 	u64 val;
 
 	lpc18xx_pwm = devm_kzalloc(&pdev->dev, sizeof(*lpc18xx_pwm),
@@ -398,16 +397,6 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 	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 = &lpc18xx_pwm->channeldata[i];
-
-		pwm_set_chip_data(pwm, data);
-	}
-
 	val = lpc18xx_pwm_readl(lpc18xx_pwm, LPC18XX_PWM_CTRL);
 	val &= ~LPC18XX_PWM_BIDIR;
 	val &= ~LPC18XX_PWM_CTRL_HALT;
-- 
2.30.2

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

[Thierry Reding]

[-- 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 --]

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

[Uwe Kleine-König]

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

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
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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