[PATCH] pwm: bcm-iproc: Free resources only after pwmchip_remove()

[PATCH] pwm: bcm-iproc: Free resources only after pwmchip_remove()

[Uwe Kleine-König]

Before pwmchip_remove() returns the PWM is expected to be functional. So
remove the pwmchip before disabling the clock.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-bcm-iproc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
index 529a66ab692d..edd2ce1760ab 100644
--- a/drivers/pwm/pwm-bcm-iproc.c
+++ b/drivers/pwm/pwm-bcm-iproc.c
@@ -253,9 +253,11 @@ static int iproc_pwmc_remove(struct platform_device *pdev)
 {
 	struct iproc_pwmc *ip = platform_get_drvdata(pdev);
 
+	pwmchip_remove(&ip->chip);
+
 	clk_disable_unprepare(ip->clk);
 
-	return pwmchip_remove(&ip->chip);
+	return 0;
 }
 
 static const struct of_device_id bcm_iproc_pwmc_dt[] = {
-- 
2.30.2

Re: [PATCH] pwm: bcm-iproc: Free resources only after pwmchip_remove()

[Ray Jui]

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



On 3/24/2021 1:01 PM, Uwe Kleine-König wrote:
> Before pwmchip_remove() returns the PWM is expected to be functional. So
> remove the pwmchip before disabling the clock.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-bcm-iproc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
> index 529a66ab692d..edd2ce1760ab 100644
> --- a/drivers/pwm/pwm-bcm-iproc.c
> +++ b/drivers/pwm/pwm-bcm-iproc.c
> @@ -253,9 +253,11 @@ static int iproc_pwmc_remove(struct platform_device *pdev)
>  {
>  	struct iproc_pwmc *ip = platform_get_drvdata(pdev);
>  
> +	pwmchip_remove(&ip->chip);
> +
>  	clk_disable_unprepare(ip->clk);
>  
> -	return pwmchip_remove(&ip->chip);
> +	return 0;

This is a good fix! Given that there appears to be a race condition
where the clock can be disabled before the PWM device unregisters from
the framework, I assume we might be seeing hangs in corner cases without
this fix, i.e., PWM device accessed with clock disabled. Then does it
make sense to add a fixes tag so this fix is also picked up by LTS?

Thanks,

Ray


>  }
>  
>  static const struct of_device_id bcm_iproc_pwmc_dt[] = {
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4194 bytes --]

Re: [PATCH] pwm: bcm-iproc: Free resources only after pwmchip_remove()

[Uwe Kleine-König]

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

Hello,

On Wed, Mar 24, 2021 at 02:15:23PM -0700, Ray Jui wrote:
> On 3/24/2021 1:01 PM, Uwe Kleine-König wrote:
> > Before pwmchip_remove() returns the PWM is expected to be functional. So
> > remove the pwmchip before disabling the clock.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-bcm-iproc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
> > index 529a66ab692d..edd2ce1760ab 100644
> > --- a/drivers/pwm/pwm-bcm-iproc.c
> > +++ b/drivers/pwm/pwm-bcm-iproc.c
> > @@ -253,9 +253,11 @@ static int iproc_pwmc_remove(struct platform_device *pdev)
> >  {
> >  	struct iproc_pwmc *ip = platform_get_drvdata(pdev);
> >  
> > +	pwmchip_remove(&ip->chip);
> > +
> >  	clk_disable_unprepare(ip->clk);
> >  
> > -	return pwmchip_remove(&ip->chip);
> > +	return 0;
> 
> This is a good fix! Given that there appears to be a race condition
> where the clock can be disabled before the PWM device unregisters from
> the framework, I assume we might be seeing hangs in corner cases without
> this fix, i.e., PWM device accessed with clock disabled. Then does it
> make sense to add a fixes tag so this fix is also picked up by LTS?

The hangs are usually short, so I'm unsure if it's worth the backport.
Also before commit b2c200e3f2fd ("pwm: Add consumer device link")---which
is in v5.3-rc1---you cannot ignore the return value of pwmchip_remove().

(And before that change if pwmchip_remove() returned an error the
situation was grave compared to that clock skew. So on the other hand we
could drop the return value check there, too, without making the
situation considerably worse.)

Anyhow, the offending commit is (little surprisingly)

	daa5abc41c80 (pwm: Add support for Broadcom iProc PWM controller)

which appeared in 4.7-rc1. I let Thierry decide if he want to add this
fixes line.

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

Re: [PATCH] pwm: bcm-iproc: Free resources only after pwmchip_remove()

[Thierry Reding]

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

On Thu, Mar 25, 2021 at 08:11:50AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Wed, Mar 24, 2021 at 02:15:23PM -0700, Ray Jui wrote:
> > On 3/24/2021 1:01 PM, Uwe Kleine-König wrote:
> > > Before pwmchip_remove() returns the PWM is expected to be functional. So
> > > remove the pwmchip before disabling the clock.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/pwm/pwm-bcm-iproc.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-bcm-iproc.c b/drivers/pwm/pwm-bcm-iproc.c
> > > index 529a66ab692d..edd2ce1760ab 100644
> > > --- a/drivers/pwm/pwm-bcm-iproc.c
> > > +++ b/drivers/pwm/pwm-bcm-iproc.c
> > > @@ -253,9 +253,11 @@ static int iproc_pwmc_remove(struct platform_device *pdev)
> > >  {
> > >  	struct iproc_pwmc *ip = platform_get_drvdata(pdev);
> > >  
> > > +	pwmchip_remove(&ip->chip);
> > > +
> > >  	clk_disable_unprepare(ip->clk);
> > >  
> > > -	return pwmchip_remove(&ip->chip);
> > > +	return 0;
> > 
> > This is a good fix! Given that there appears to be a race condition
> > where the clock can be disabled before the PWM device unregisters from
> > the framework, I assume we might be seeing hangs in corner cases without
> > this fix, i.e., PWM device accessed with clock disabled. Then does it
> > make sense to add a fixes tag so this fix is also picked up by LTS?
> 
> The hangs are usually short, so I'm unsure if it's worth the backport.

That depends on the hardware. I've worked with hardware that will hang
hard if you try to access a register of an unclocked IP block with no
way to recover other than resetting. Other hardware may raise a system
error that you may not be able to recover from, etc.

> Also before commit b2c200e3f2fd ("pwm: Add consumer device link")---which
> is in v5.3-rc1---you cannot ignore the return value of pwmchip_remove().
> 
> (And before that change if pwmchip_remove() returned an error the
> situation was grave compared to that clock skew. So on the other hand we
> could drop the return value check there, too, without making the
> situation considerably worse.)
> 
> Anyhow, the offending commit is (little surprisingly)
> 
> 	daa5abc41c80 (pwm: Add support for Broadcom iProc PWM controller)
> 
> which appeared in 4.7-rc1. I let Thierry decide if he want to add this
> fixes line.

Given that I've never seen a report of this actually being a problem,
I don't think it's worth backporting this.

Thierry

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

Re: [PATCH] pwm: bcm-iproc: Free resources only after pwmchip_remove()

[Thierry Reding]

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

On Wed, Mar 24, 2021 at 09:01:34PM +0100, Uwe Kleine-König wrote:
> Before pwmchip_remove() returns the PWM is expected to be functional. So
> remove the pwmchip before disabling the clock.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-bcm-iproc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Applied, thanks.

Thierry

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