This series adds new devm_work_autocancel() helper.
Many drivers which use work-queues must ensure the work is not queued when
driver is detached. Often this is done by ensuring new work is not added and
then calling cancel_work_sync() at remove(). In many cases this also requires
cleanup at probe error path - which is easy to forget (or get wrong).
Also the "by ensuring new work is not added" has a gotcha.
It is not strange to see devm managed IRQs scheduling work.
Mixing this with manual wq clean-up is hard to do correctly because the
devm is likely to free the IRQ only after the remove() is ran. So manual
wq cancellation and devm-based IRQ management do not mix well - there is
a short(?) time-window after the wq clean-up when IRQs are still not
freed and may schedule new work.
When both WQs and IRQs are managed by devm things are likely to just
work. WQs should be initialized before IRQs (when IRQs need to schedule
work) and devm unwinds things in "FILO" order.
This series implements wq cancellation on top of devm and replaces
the obvious cases where only thing remove call-back in a driver does is
cancelling the work. There might be other cases where we could switch
more than just work cancellation to use managed version and thus get rid
of remove or mixed (manual and devm) resource management.
---
Matti Vaittinen (4):
devm-helpers: Add resource managed version of work init
extcon: extcon-max14577: Fix potential work-queue cancellation race
extcon: extcon-max77693.c: Fix potential work-queue cancellation race
extcon: extcon-max8997: Fix IRQ freeing at error path
drivers/extcon/extcon-max14577.c | 16 ++++--------
drivers/extcon/extcon-max77693.c | 17 ++++--------
drivers/extcon/extcon-max8997.c | 45 +++++++++++---------------------
include/linux/devm-helpers.h | 25 ++++++++++++++++++
4 files changed, 50 insertions(+), 53 deletions(-)
base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
--
2.25.4
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
A few drivers which need a work-queue must cancel work at driver detach.
Some of those implement remove() solely for this purpose. Help drivers to
avoid unnecessary remove and error-branch implementation by adding managed
verision of work initialization. This will also help drivers to avoid
mixing manual and devm based unwinding when other resources are handled by
devm.
Signed-off-by: Matti Vaittinen <[email protected]>
---
include/linux/devm-helpers.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/include/linux/devm-helpers.h b/include/linux/devm-helpers.h
index f40f77717a24..74891802200d 100644
--- a/include/linux/devm-helpers.h
+++ b/include/linux/devm-helpers.h
@@ -51,4 +51,29 @@ static inline int devm_delayed_work_autocancel(struct device *dev,
return devm_add_action(dev, devm_delayed_work_drop, w);
}
+static inline void devm_work_drop(void *res)
+{
+ cancel_work_sync(res);
+}
+
+/**
+ * devm_work_autocancel - Resource-managed work allocation
+ * @dev: Device which lifetime work is bound to
+ * @w: Work to be added (and automatically cancelled)
+ * @worker: Worker function
+ *
+ * Initialize work which is automatically cancelled when driver is detached.
+ * A few drivers need to queue work which must be cancelled before driver
+ * is detached to avoid accessing removed resources.
+ * devm_work_autocancel() can be used to omit the explicit
+ * cancelleation when driver is detached.
+ */
+static inline int devm_work_autocancel(struct device *dev,
+ struct work_struct *w,
+ work_func_t worker)
+{
+ INIT_WORK(w, worker);
+ return devm_add_action(dev, devm_work_drop, w);
+}
+
#endif
--
2.25.4
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
The extcon IRQ schedules a work item. IRQ is requested using devm while
WQ is cancelld at remove(). This mixing of devm and manual unwinding has
potential case where the WQ has been emptied (.remove() was ran) but
devm unwinding of IRQ was not yet done. It may be possible the IRQ is
triggered at this point scheduling new work item to the already flushed
queue.
According to the input documentation the input device allocated by
devm_input_allocate_device() does not need to be explicitly unregistered.
Use the new devm_work_autocancel() and remove the remove() to simplify the
code.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Please note that the change is compile-tested only. All proper testing is
highly appreciated.
---
drivers/extcon/extcon-max77693.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
index 92af97e00828..1f1d9ab0c5c7 100644
--- a/drivers/extcon/extcon-max77693.c
+++ b/drivers/extcon/extcon-max77693.c
@@ -5,6 +5,7 @@
// Copyright (C) 2012 Samsung Electrnoics
// Chanwoo Choi <[email protected]>
+#include <linux/devm-helpers.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/i2c.h>
@@ -1127,7 +1128,10 @@ static int max77693_muic_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, info);
mutex_init(&info->mutex);
- INIT_WORK(&info->irq_work, max77693_muic_irq_work);
+ ret = devm_work_autocancel(&pdev->dev, &info->irq_work,
+ max77693_muic_irq_work);
+ if (ret)
+ return ret;
/* Support irq domain for MAX77693 MUIC device */
for (i = 0; i < ARRAY_SIZE(muic_irqs); i++) {
@@ -1254,22 +1258,11 @@ static int max77693_muic_probe(struct platform_device *pdev)
return ret;
}
-static int max77693_muic_remove(struct platform_device *pdev)
-{
- struct max77693_muic_info *info = platform_get_drvdata(pdev);
-
- cancel_work_sync(&info->irq_work);
- input_unregister_device(info->dock);
-
- return 0;
-}
-
static struct platform_driver max77693_muic_driver = {
.driver = {
.name = DEV_NAME,
},
.probe = max77693_muic_probe,
- .remove = max77693_muic_remove,
};
module_platform_driver(max77693_muic_driver);
--
2.25.4
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver exits
without freeing the requested IRQs.
Simplify driver and fix the IRQ problem by switching to use the
resource managed IRQ requesting and resource managed work-queue
initialization.
Fixes: 3e34c8198960 ("extcon: max8997: Avoid forcing UART path on drive probe")
Signed-off-by: Matti Vaittinen <[email protected]>
---
Please note that the change is compile-tested only. All proper testing is
highly appreciated.
---
drivers/extcon/extcon-max8997.c | 45 +++++++++++----------------------
1 file changed, 15 insertions(+), 30 deletions(-)
diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
index e1408075ef7d..bbc592823570 100644
--- a/drivers/extcon/extcon-max8997.c
+++ b/drivers/extcon/extcon-max8997.c
@@ -5,6 +5,7 @@
// Copyright (C) 2012 Samsung Electronics
// Donggeun Kim <[email protected]>
+#include <linux/devm-helpers.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/i2c.h>
@@ -650,27 +651,30 @@ static int max8997_muic_probe(struct platform_device *pdev)
mutex_init(&info->mutex);
INIT_WORK(&info->irq_work, max8997_muic_irq_work);
+ ret = devm_work_autocancel(&pdev->dev, &info->irq_work,
+ max8997_muic_irq_work);
+ if (ret)
+ return ret;
for (i = 0; i < ARRAY_SIZE(muic_irqs); i++) {
struct max8997_muic_irq *muic_irq = &muic_irqs[i];
unsigned int virq = 0;
virq = irq_create_mapping(max8997->irq_domain, muic_irq->irq);
- if (!virq) {
- ret = -EINVAL;
- goto err_irq;
- }
+ if (!virq)
+ return -EINVAL;
+
muic_irq->virq = virq;
- ret = request_threaded_irq(virq, NULL,
- max8997_muic_irq_handler,
- IRQF_NO_SUSPEND,
- muic_irq->name, info);
+ ret = devm_request_threaded_irq(&pdev->dev, virq, NULL,
+ max8997_muic_irq_handler,
+ IRQF_NO_SUSPEND,
+ muic_irq->name, info);
if (ret) {
dev_err(&pdev->dev,
"failed: irq request (IRQ: %d, error :%d)\n",
muic_irq->irq, ret);
- goto err_irq;
+ return ret;
}
}
@@ -678,14 +682,13 @@ static int max8997_muic_probe(struct platform_device *pdev)
info->edev = devm_extcon_dev_allocate(&pdev->dev, max8997_extcon_cable);
if (IS_ERR(info->edev)) {
dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
- ret = PTR_ERR(info->edev);
- goto err_irq;
+ return PTR_ERR(info->edev);
}
ret = devm_extcon_dev_register(&pdev->dev, info->edev);
if (ret) {
dev_err(&pdev->dev, "failed to register extcon device\n");
- goto err_irq;
+ return ret;
}
if (pdata && pdata->muic_pdata) {
@@ -756,23 +759,6 @@ static int max8997_muic_probe(struct platform_device *pdev)
delay_jiffies);
return 0;
-
-err_irq:
- while (--i >= 0)
- free_irq(muic_irqs[i].virq, info);
- return ret;
-}
-
-static int max8997_muic_remove(struct platform_device *pdev)
-{
- struct max8997_muic_info *info = platform_get_drvdata(pdev);
- int i;
-
- for (i = 0; i < ARRAY_SIZE(muic_irqs); i++)
- free_irq(muic_irqs[i].virq, info);
- cancel_work_sync(&info->irq_work);
-
- return 0;
}
static struct platform_driver max8997_muic_driver = {
@@ -780,7 +766,6 @@ static struct platform_driver max8997_muic_driver = {
.name = DEV_NAME,
},
.probe = max8997_muic_probe,
- .remove = max8997_muic_remove,
};
module_platform_driver(max8997_muic_driver);
--
2.25.4
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
The extcon IRQ schedules a work item. IRQ is requested using devm while
WQ is cancelld at remove(). This mixing of devm and manual unwinding has
potential case where the WQ has been emptied (.remove() was ran) but
devm unwinding of IRQ was not yet done. It is possible the IRQ is triggered
at this point scheduling new work item to the already flushed queue.
Use new devm_work_autocancel() to remove the remove() and to kill the bug.
Signed-off-by: Matti Vaittinen <[email protected]>
---
Please note that the change is compile-tested only. All proper testing is
highly appreciated.
---
drivers/extcon/extcon-max14577.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
index ace523924e58..5476f48ed74b 100644
--- a/drivers/extcon/extcon-max14577.c
+++ b/drivers/extcon/extcon-max14577.c
@@ -6,6 +6,7 @@
// Chanwoo Choi <[email protected]>
// Krzysztof Kozlowski <[email protected]>
+#include <linux/devm-helpers.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/i2c.h>
@@ -673,7 +674,10 @@ static int max14577_muic_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, info);
mutex_init(&info->mutex);
- INIT_WORK(&info->irq_work, max14577_muic_irq_work);
+ ret = devm_work_autocancel(&pdev->dev, &info->irq_work,
+ max14577_muic_irq_work);
+ if (ret)
+ return ret;
switch (max14577->dev_type) {
case MAXIM_DEVICE_TYPE_MAX77836:
@@ -766,15 +770,6 @@ static int max14577_muic_probe(struct platform_device *pdev)
return ret;
}
-static int max14577_muic_remove(struct platform_device *pdev)
-{
- struct max14577_muic_info *info = platform_get_drvdata(pdev);
-
- cancel_work_sync(&info->irq_work);
-
- return 0;
-}
-
static const struct platform_device_id max14577_muic_id[] = {
{ "max14577-muic", MAXIM_DEVICE_TYPE_MAX14577, },
{ "max77836-muic", MAXIM_DEVICE_TYPE_MAX77836, },
@@ -797,7 +792,6 @@ static struct platform_driver max14577_muic_driver = {
.of_match_table = of_max14577_muic_dt_match,
},
.probe = max14577_muic_probe,
- .remove = max14577_muic_remove,
.id_table = max14577_muic_id,
};
--
2.25.4
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On 10/05/2021 04:11, Matti Vaittinen wrote:
> The extcon IRQ schedules a work item. IRQ is requested using devm while
> WQ is cancelld at remove(). This mixing of devm and manual unwinding has
> potential case where the WQ has been emptied (.remove() was ran) but
> devm unwinding of IRQ was not yet done. It is possible the IRQ is triggered
> at this point scheduling new work item to the already flushed queue.
>
> Use new devm_work_autocancel() to remove the remove() and to kill the bug.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
>
> Please note that the change is compile-tested only. All proper testing is
> highly appreciated.
> ---
> drivers/extcon/extcon-max14577.c | 16 +++++-----------
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
I don't have the HW anymore for this testing, but looks good:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 10/05/2021 04:10, Matti Vaittinen wrote:
> A few drivers which need a work-queue must cancel work at driver detach.
> Some of those implement remove() solely for this purpose. Help drivers to
> avoid unnecessary remove and error-branch implementation by adding managed
> verision of work initialization. This will also help drivers to avoid
> mixing manual and devm based unwinding when other resources are handled by
> devm.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
> include/linux/devm-helpers.h | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 10/05/2021 04:12, Matti Vaittinen wrote:
> The extcon IRQ schedules a work item. IRQ is requested using devm while
> WQ is cancelld at remove(). This mixing of devm and manual unwinding has
> potential case where the WQ has been emptied (.remove() was ran) but
> devm unwinding of IRQ was not yet done. It may be possible the IRQ is
> triggered at this point scheduling new work item to the already flushed
> queue.
>
> According to the input documentation the input device allocated by
> devm_input_allocate_device() does not need to be explicitly unregistered.
> Use the new devm_work_autocancel() and remove the remove() to simplify the
> code.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
>
> Please note that the change is compile-tested only. All proper testing is
> highly appreciated.
> ---
> drivers/extcon/extcon-max77693.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
Seems correct:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 10/05/2021 04:12, Matti Vaittinen wrote:
> If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver exits
> without freeing the requested IRQs.
The driver frees IRQ on probe failure, so maybe you meant missing IRQ
mapping dispose?
>
> Simplify driver and fix the IRQ problem by switching to use the
> resource managed IRQ requesting and resource managed work-queue
> initialization.
>
> Fixes: 3e34c8198960 ("extcon: max8997: Avoid forcing UART path on drive probe")
> Signed-off-by: Matti Vaittinen <[email protected]>
> ---
>
> Please note that the change is compile-tested only. All proper testing is
> highly appreciated.
> ---
> drivers/extcon/extcon-max8997.c | 45 +++++++++++----------------------
> 1 file changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
> index e1408075ef7d..bbc592823570 100644
> --- a/drivers/extcon/extcon-max8997.c
> +++ b/drivers/extcon/extcon-max8997.c
> @@ -5,6 +5,7 @@
> // Copyright (C) 2012 Samsung Electronics
> // Donggeun Kim <[email protected]>
>
> +#include <linux/devm-helpers.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/i2c.h>
> @@ -650,27 +651,30 @@ static int max8997_muic_probe(struct platform_device *pdev)
> mutex_init(&info->mutex);
>
> INIT_WORK(&info->irq_work, max8997_muic_irq_work);
> + ret = devm_work_autocancel(&pdev->dev, &info->irq_work,
> + max8997_muic_irq_work);
> + if (ret)
> + return ret;
>
> for (i = 0; i < ARRAY_SIZE(muic_irqs); i++) {
> struct max8997_muic_irq *muic_irq = &muic_irqs[i];
> unsigned int virq = 0;
>
> virq = irq_create_mapping(max8997->irq_domain, muic_irq->irq);
> - if (!virq) {
> - ret = -EINVAL;
> - goto err_irq;
> - }
> + if (!virq)
> + return -EINVAL;
> +
> muic_irq->virq = virq;
>
> - ret = request_threaded_irq(virq, NULL,
> - max8997_muic_irq_handler,
> - IRQF_NO_SUSPEND,
> - muic_irq->name, info);
> + ret = devm_request_threaded_irq(&pdev->dev, virq, NULL,
> + max8997_muic_irq_handler,
> + IRQF_NO_SUSPEND,
> + muic_irq->name, info);
> if (ret) {
> dev_err(&pdev->dev,
> "failed: irq request (IRQ: %d, error :%d)\n",
> muic_irq->irq, ret);
> - goto err_irq;
> + return ret;
> }
> }
>
> @@ -678,14 +682,13 @@ static int max8997_muic_probe(struct platform_device *pdev)
> info->edev = devm_extcon_dev_allocate(&pdev->dev, max8997_extcon_cable);
> if (IS_ERR(info->edev)) {
> dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
> - ret = PTR_ERR(info->edev);
> - goto err_irq;
> + return PTR_ERR(info->edev);
> }
>
> ret = devm_extcon_dev_register(&pdev->dev, info->edev);
> if (ret) {
> dev_err(&pdev->dev, "failed to register extcon device\n");
> - goto err_irq;
> + return ret;
> }
>
> if (pdata && pdata->muic_pdata) {
> @@ -756,23 +759,6 @@ static int max8997_muic_probe(struct platform_device *pdev)
> delay_jiffies);
>
> return 0;
> -
> -err_irq:
> - while (--i >= 0)
> - free_irq(muic_irqs[i].virq, info);
Here it will unwind what was done.
> - return ret;
> -}
> -
Best regards,
Krzysztof
Hi Krzysztof,
On Mon, 2021-05-10 at 10:21 -0400, Krzysztof Kozlowski wrote:
> On 10/05/2021 04:12, Matti Vaittinen wrote:
> > If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver exits
> > without freeing the requested IRQs.
>
> The driver frees IRQ on probe failure, so maybe you meant missing IRQ
> mapping dispose?
No. The commit 3e34c8198960 ("extcon: max8997: Avoid forcing UART path
on drive probe") introduced a return w/o IRQ freeing if reading the
MAX8997_MUIC_REG_STATUS1 fails at the end of the probe. This is not
visible in the patch though - as the return is Ok after the IRQs and
work-queue cancellation are managed by devm.
Best Regards
--Matti Vaittinen
On 10/05/2021 23:32, Matti Vaittinen wrote:
> Hi Krzysztof,
>
> On Mon, 2021-05-10 at 10:21 -0400, Krzysztof Kozlowski wrote:
>> On 10/05/2021 04:12, Matti Vaittinen wrote:
>>> If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver exits
>>> without freeing the requested IRQs.
>>
>> The driver frees IRQ on probe failure, so maybe you meant missing IRQ
>> mapping dispose?
>
> No. The commit 3e34c8198960 ("extcon: max8997: Avoid forcing UART path
> on drive probe") introduced a return w/o IRQ freeing if reading the
> MAX8997_MUIC_REG_STATUS1 fails at the end of the probe. This is not
> visible in the patch though - as the return is Ok after the IRQs and
> work-queue cancellation are managed by devm.
I see it now, right. The fix is big and changes too much to be
backportable. I would prefer to simply fix the problem with "goto
err_irq" and convert to devm in next patch.
Best regards,
Krzysztof
On Tue, 2021-05-11 at 12:27 -0400, Krzysztof Kozlowski wrote:
> On 10/05/2021 23:32, Matti Vaittinen wrote:
> > Hi Krzysztof,
> >
> > On Mon, 2021-05-10 at 10:21 -0400, Krzysztof Kozlowski wrote:
> > > On 10/05/2021 04:12, Matti Vaittinen wrote:
> > > > If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver
> > > > exits
> > > > without freeing the requested IRQs.
> > >
> > > The driver frees IRQ on probe failure, so maybe you meant missing
> > > IRQ
> > > mapping dispose?
> >
> > No. The commit 3e34c8198960 ("extcon: max8997: Avoid forcing UART
> > path
> > on drive probe") introduced a return w/o IRQ freeing if reading the
> > MAX8997_MUIC_REG_STATUS1 fails at the end of the probe. This is not
> > visible in the patch though - as the return is Ok after the IRQs
> > and
> > work-queue cancellation are managed by devm.
>
> I see it now, right. The fix is big and changes too much to be
> backportable. I would prefer to simply fix the problem with "goto
> err_irq" and convert to devm in next patch.
Agree. Backporting devm WQs to stable just to fix this seems like an
overkill. I'll respin the series.
Best Regards
--Matti