* [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback @ 2021-05-05 16:28 Uwe Kleine-König 2021-05-05 16:28 ` [PATCH 2/2] pwm: stm32-lp: Don't check the return code of pwmchip_remove() Uwe Kleine-König ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Uwe Kleine-König @ 2021-05-05 16:28 UTC (permalink / raw) To: Fabrice Gasnier, Thierry Reding, Lee Jones, Maxime Coquelin, Alexandre Torgue Cc: linux-pwm, linux-stm32, kernel A consumer is expected to disable a PWM before calling pwm_put(). And if they didn't there is hopefully a good reason (or the consumer needs fixing). Also if disabling an enabled PWM was the right thing to do, this should better be done in the framework instead of in each low level driver. So drop the hardware modification from the .remove() callback. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-stm32-lp.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c index af08f564ef1d..2464f7a24983 100644 --- a/drivers/pwm/pwm-stm32-lp.c +++ b/drivers/pwm/pwm-stm32-lp.c @@ -224,8 +224,6 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) { struct stm32_pwm_lp *priv = platform_get_drvdata(pdev); - pwm_disable(&priv->chip.pwms[0]); - return pwmchip_remove(&priv->chip); } base-commit: a6efb35019d00f483a0e5f188747723371d659fe -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] pwm: stm32-lp: Don't check the return code of pwmchip_remove() 2021-05-05 16:28 [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Uwe Kleine-König @ 2021-05-05 16:28 ` Uwe Kleine-König 2021-05-12 7:41 ` [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Fabrice Gasnier 2021-07-05 13:11 ` Uwe Kleine-König 2 siblings, 0 replies; 6+ messages in thread From: Uwe Kleine-König @ 2021-05-05 16:28 UTC (permalink / raw) To: Fabrice Gasnier, Thierry Reding, Lee Jones, Maxime Coquelin, Alexandre Torgue Cc: linux-pwm, linux-stm32, kernel pwmchip_remove() always returns 0. Don't use the value to make it possible to eventually change the function to return void. This is a good thing as pwmchip_remove() is usually called from a remove function (mostly for platform devices) and their return value is ignored by the device core anyhow. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-stm32-lp.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c index 2464f7a24983..58bc75857b80 100644 --- a/drivers/pwm/pwm-stm32-lp.c +++ b/drivers/pwm/pwm-stm32-lp.c @@ -224,7 +224,9 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) { struct stm32_pwm_lp *priv = platform_get_drvdata(pdev); - return pwmchip_remove(&priv->chip); + pwmchip_remove(&priv->chip); + + return 0; } static int __maybe_unused stm32_pwm_lp_suspend(struct device *dev) -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback 2021-05-05 16:28 [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Uwe Kleine-König 2021-05-05 16:28 ` [PATCH 2/2] pwm: stm32-lp: Don't check the return code of pwmchip_remove() Uwe Kleine-König @ 2021-05-12 7:41 ` Fabrice Gasnier 2021-05-14 14:08 ` Uwe Kleine-König 2021-07-05 13:11 ` Uwe Kleine-König 2 siblings, 1 reply; 6+ messages in thread From: Fabrice Gasnier @ 2021-05-12 7:41 UTC (permalink / raw) To: Uwe Kleine-König, Fabrice Gasnier, Thierry Reding, Lee Jones, Maxime Coquelin, Alexandre Torgue Cc: linux-pwm, linux-stm32, kernel On 5/5/21 6:28 PM, Uwe Kleine-König wrote: > A consumer is expected to disable a PWM before calling pwm_put(). And if > they didn't there is hopefully a good reason (or the consumer needs > fixing). Hi Uwe, As you pointed out, (ideally) consumers need to disable the PWM before undind or remove of the driver. Calling pwm_disable in the remove is a "fail safe". > Also if disabling an enabled PWM was the right thing to do, > this should better be done in the framework instead of in each low level > driver. Not doing so, in case the driver gets unbind when the PWM is enabled, then bind again, it leads to unbalanced clock enable count. There's no change to recover the balance after it. Also, I'm also wondering if it's a good idea to let a free running PWM channel? (That's probably a more general discussion). > > So drop the hardware modification from the .remove() callback. For now, I'd prefer to keep the current implementation, e.g. not to simply drop this fail safe. Is there a reason to address only the STM32 LP Timer pwm driver in your patch ? (I see there are other drivers around, doing the same. I agree this could better be addressed by the framework, for all drivers). Best Regards, Fabrice > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/pwm-stm32-lp.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/pwm/pwm-stm32-lp.c b/drivers/pwm/pwm-stm32-lp.c > index af08f564ef1d..2464f7a24983 100644 > --- a/drivers/pwm/pwm-stm32-lp.c > +++ b/drivers/pwm/pwm-stm32-lp.c > @@ -224,8 +224,6 @@ static int stm32_pwm_lp_remove(struct platform_device *pdev) > { > struct stm32_pwm_lp *priv = platform_get_drvdata(pdev); > > - pwm_disable(&priv->chip.pwms[0]); > - > return pwmchip_remove(&priv->chip); > } > > > base-commit: a6efb35019d00f483a0e5f188747723371d659fe > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback 2021-05-12 7:41 ` [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Fabrice Gasnier @ 2021-05-14 14:08 ` Uwe Kleine-König 2021-05-25 20:24 ` Uwe Kleine-König 0 siblings, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2021-05-14 14:08 UTC (permalink / raw) To: Fabrice Gasnier Cc: Thierry Reding, Lee Jones, Maxime Coquelin, Alexandre Torgue, linux-pwm, linux-stm32, kernel [-- Attachment #1: Type: text/plain, Size: 1756 bytes --] Hello Fabrice, On Wed, May 12, 2021 at 09:41:45AM +0200, Fabrice Gasnier wrote: > On 5/5/21 6:28 PM, Uwe Kleine-König wrote: > > A consumer is expected to disable a PWM before calling pwm_put(). And if > > they didn't there is hopefully a good reason (or the consumer needs > > fixing). > > As you pointed out, (ideally) consumers need to disable the PWM before > undind or remove of the driver. Calling pwm_disable in the remove is a > "fail safe". > > > Also if disabling an enabled PWM was the right thing to do, > > this should better be done in the framework instead of in each low level > > driver. > > Not doing so, in case the driver gets unbind when the PWM is enabled, > then bind again, it leads to unbalanced clock enable count. Ah, the clk must indeed be properly freed, hmm. I will respin the patch. > There's no change to recover the balance after it. > > Also, I'm also wondering if it's a good idea to let a free running PWM > channel? (That's probably a more general discussion). It might make sense, e.g. if you don't want your backlight to go out for a reboot because the display shows a "I'm rebooting" message. > > So drop the hardware modification from the .remove() callback. > > For now, I'd prefer to keep the current implementation, e.g. not to > simply drop this fail safe. > > Is there a reason to address only the STM32 LP Timer pwm driver in your > patch ? Whenever I find a driver I fix it. So I already fixed some other drivers accordingly. See git lg --grep="modify HW state in .remove" 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback 2021-05-14 14:08 ` Uwe Kleine-König @ 2021-05-25 20:24 ` Uwe Kleine-König 0 siblings, 0 replies; 6+ messages in thread From: Uwe Kleine-König @ 2021-05-25 20:24 UTC (permalink / raw) To: Fabrice Gasnier Cc: linux-pwm, kernel, Alexandre Torgue, Thierry Reding, Maxime Coquelin, Lee Jones, linux-stm32 [-- Attachment #1: Type: text/plain, Size: 1958 bytes --] Hello Fabrice, On Fri, May 14, 2021 at 04:08:43PM +0200, Uwe Kleine-König wrote: > On Wed, May 12, 2021 at 09:41:45AM +0200, Fabrice Gasnier wrote: > > On 5/5/21 6:28 PM, Uwe Kleine-König wrote: > > > A consumer is expected to disable a PWM before calling pwm_put(). And if > > > they didn't there is hopefully a good reason (or the consumer needs > > > fixing). > > > > As you pointed out, (ideally) consumers need to disable the PWM before > > undind or remove of the driver. Calling pwm_disable in the remove is a > > "fail safe". > > > > > Also if disabling an enabled PWM was the right thing to do, > > > this should better be done in the framework instead of in each low level > > > driver. > > > > Not doing so, in case the driver gets unbind when the PWM is enabled, > > then bind again, it leads to unbalanced clock enable count. > > Ah, the clk must indeed be properly freed, hmm. I will respin the patch. I took a deeper look now, and I don't agree any more to what I wrote here. The clock is provided by the parent device, so .probe() doesn't call clk_get() (or one of its variants). The clock is only enabled when the .apply() callback is called with .enabled = true. So indeed if the driver is unloaded while the PWM is still running, the enable count is non-zero at the end. The unbalanced clock enable count isn't a big problem (it just never reaches zero any more, which is harmless compared to becoming negative). I don't think this should be fixed or handled at the driver level. Consumers should be fixed to not produce such a leak and if we are sure that this is really always wrong, a check (and maybe a call to pwm_disable()) should be added to the PWM core code. So I'm in favour of applying the patch as is. 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback 2021-05-05 16:28 [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Uwe Kleine-König 2021-05-05 16:28 ` [PATCH 2/2] pwm: stm32-lp: Don't check the return code of pwmchip_remove() Uwe Kleine-König 2021-05-12 7:41 ` [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Fabrice Gasnier @ 2021-07-05 13:11 ` Uwe Kleine-König 2 siblings, 0 replies; 6+ messages in thread From: Uwe Kleine-König @ 2021-07-05 13:11 UTC (permalink / raw) To: Thierry Reding Cc: linux-pwm, linux-stm32, kernel, Fabrice Gasnier, Lee Jones, Maxime Coquelin, Alexandre Torgue [-- Attachment #1: Type: text/plain, Size: 427 bytes --] Hi Thierry, this thread is marked as "Rejected" in patchwork without a word of you. Did this happen on purpose? Assuming it did: I would welcome a word from you in such a case. From my POV the patch set is still fine and should be applied. 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 --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-05 13:11 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-05 16:28 [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Uwe Kleine-König 2021-05-05 16:28 ` [PATCH 2/2] pwm: stm32-lp: Don't check the return code of pwmchip_remove() Uwe Kleine-König 2021-05-12 7:41 ` [PATCH 1/2] pwm: stm32-lp: Don't modify HW state in .remove callback Fabrice Gasnier 2021-05-14 14:08 ` Uwe Kleine-König 2021-05-25 20:24 ` Uwe Kleine-König 2021-07-05 13:11 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).