| Message ID | 20210324200134.75513-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: bcm-iproc: Free resources only after pwmchip_remove() | expand |
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[] = { >
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
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
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
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[] = {
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(-)