Message ID | 20191111090357.13903-1-u.kleine-koenig@pengutronix.de |
---|---|
State | Accepted |
Headers | show |
Series | [1/4] pwm: omap-dmtimer: remove pwmchip in .remove before making it unfunctional | expand |
Hello, I created a cover letter but failed to send it together with the series :-| It said: | I promised a cleanup patch but as I found a few more issues I have four | patches now. The third patch replaces Markus' patch with a more complete | fix that also drops the reference in .remove not only the error path of | .probe. | | The last patch allows to compile the driver in more configurations, it | compiled successfully on amd64 and arm. Best regards Uwe
> I created a cover letter but failed to send it together with the series > :-| Did you omit the address “linux-kernel@vger.kernel.org” from the recipient list intentionally? Regards, Markus
> I created a cover letter but failed to send it together with the series > :-| Did you omit the address “linux-kernel@vger.kernel.org” from the recipient list intentionally? Regards, Markus
> In the old code (e.g.) mutex_destroy() was called before > pwmchip_remove(). Between these two calls it is possible that a pwm > callback is used which tries to grab the mutex. How do you think about to add a more “imperative mood” for your change description? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151 > +++ b/drivers/pwm/pwm-omap-dmtimer.c > @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) > static int pwm_omap_dmtimer_remove(struct platform_device *pdev) > { > struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev); > + int ret; > + > + ret = pwmchip_remove(&omap->chip); > + if (ret) > + return ret; > > if (pm_runtime_active(&omap->dm_timer_pdev->dev)) > omap->pdata->stop(omap->dm_timer); How do you think about to use the following statement variant? + int ret = pwmchip_remove(&omap->chip); Regards, Markus
On Mon, Nov 11, 2019 at 02:28:52PM +0100, Markus Elfring wrote: > > I created a cover letter but failed to send it together with the series > > :-| > > Did you omit the address “linux-kernel@vger.kernel.org” from > the recipient list intentionally? Yes, even though it's documented to Cc: all patches there, there is IMHO little use. If there is a dedicated mailing list, not adding lkml is fine for all maintainers I interacted with in the last few years. The volume on lkml is that high that sending all patches there only adds noise. Best regards Uwe
Hello Markus, On Mon, Nov 11, 2019 at 02:30:42PM +0100, Markus Elfring wrote: > > In the old code (e.g.) mutex_destroy() was called before > > pwmchip_remove(). Between these two calls it is possible that a pwm > > callback is used which tries to grab the mutex. > > How do you think about to add a more “imperative mood” for your > change description? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151 I described the old behaviour and like my wording. > > +++ b/drivers/pwm/pwm-omap-dmtimer.c > > @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) > > static int pwm_omap_dmtimer_remove(struct platform_device *pdev) > > { > > struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev); > > + int ret; > > + > > + ret = pwmchip_remove(&omap->chip); > > + if (ret) > > + return ret; > > > > if (pm_runtime_active(&omap->dm_timer_pdev->dev)) > > omap->pdata->stop(omap->dm_timer); > > How do you think about to use the following statement variant? > > + int ret = pwmchip_remove(&omap->chip); I think that between the declarations and code should be an empty line and between the assignment to ret and the respective check there shouldn't be one. Best regards Uwe
>>> In the old code (e.g.) mutex_destroy() was called before >>> pwmchip_remove(). Between these two calls it is possible that a pwm >>> callback is used which tries to grab the mutex. >> >> How do you think about to add a more “imperative mood” for your >> change description? >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=31f4f5b495a62c9a8b15b1c3581acd5efeb9af8c#n151 > > I described the old behaviour and like my wording. I find that the first paragraph contains useful information. Would you like to specify any corresponding actions then at this place? Regards, Markus
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c index 00772fc53490..bdf94c78655f 100644 --- a/drivers/pwm/pwm-omap-dmtimer.c +++ b/drivers/pwm/pwm-omap-dmtimer.c @@ -351,6 +351,11 @@ static int pwm_omap_dmtimer_probe(struct platform_device *pdev) static int pwm_omap_dmtimer_remove(struct platform_device *pdev) { struct pwm_omap_dmtimer_chip *omap = platform_get_drvdata(pdev); + int ret; + + ret = pwmchip_remove(&omap->chip); + if (ret) + return ret; if (pm_runtime_active(&omap->dm_timer_pdev->dev)) omap->pdata->stop(omap->dm_timer); @@ -359,7 +364,7 @@ static int pwm_omap_dmtimer_remove(struct platform_device *pdev) mutex_destroy(&omap->mutex); - return pwmchip_remove(&omap->chip); + return 0; } static const struct of_device_id pwm_omap_dmtimer_of_match[] = {
In the old code (e.g.) mutex_destroy() was called before pwmchip_remove(). Between these two calls it is possible that a pwm callback is used which tries to grab the mutex. Fixes: 6604c6556db9 ("pwm: Add PWM driver for OMAP using dual-mode timers") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/pwm-omap-dmtimer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)