[PATCH RESEND v2 0/5] Add devm helper for work-queue initialization

[PATCH RESEND v2 0/5] Add devm helper for work-queue initialization

[Matti Vaittinen]

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

This series adds new devm_work_autocancel() helper.

Note:
"The beef" of this series is the new devm-helper. This means that
normally it would be picked-up by Hans. In this case Hans asked if this
series could be taken in extconn tree:
https://lore.kernel.org/lkml/fbbfba71-bdcc-b78f-48be-d7c657adce61@redhat.com/

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.

Changelog v2:
  - rebased on v5.13-rc2
  - split the extcon-max8997 change into two. First a simple,
    back-portable fix for omitting IRQ freeing at error path, second
    being the devm-simpification which does not need backporting.

---

Matti Vaittinen (5):
  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
  extcon: extcon-max8997: Simplify driver using devm

 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: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
-- 
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 =] 

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

[PATCH RESEND v2 1/5] devm-helpers: Add resource managed version of work init

[Matti Vaittinen]

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

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 <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 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 =] 

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

[PATCH RESEND v2 2/5] extcon: extcon-max14577: Fix potential work-queue cancellation race

[Matti Vaittinen]

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

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 <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---

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 <cw00.choi@samsung.com>
 // Krzysztof Kozlowski <krzk@kernel.org>
 
+#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 =] 

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

[PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Matti Vaittinen]

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

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 <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---

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 <cw00.choi@samsung.com>
 
+#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 =] 

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

[PATCH RESEND v2 4/5] extcon: extcon-max8997: Fix IRQ freeing at error path

[Matti Vaittinen]

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

If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver exits
without freeing the requested IRQs.

Free the IRQs prior returning if reading the status fails.

Fixes: 3e34c8198960 ("extcon: max8997: Avoid forcing UART path on drive probe")
Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
Changelog:
 v2:
   - new patch (avoid backporting devm_wq just to fix IRQ freeing)
---
 drivers/extcon/extcon-max8997.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
index e1408075ef7d..c15a612067af 100644
--- a/drivers/extcon/extcon-max8997.c
+++ b/drivers/extcon/extcon-max8997.c
@@ -733,7 +733,7 @@ static int max8997_muic_probe(struct platform_device *pdev)
 				2, info->status);
 	if (ret) {
 		dev_err(info->dev, "failed to read MUIC register\n");
-		return ret;
+		goto err_irq;
 	}
 	cable_type = max8997_muic_get_cable_type(info,
 					   MAX8997_CABLE_GROUP_ADC, &attached);
-- 
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 =] 

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

[PATCH RESEND v2 5/5] extcon: extcon-max8997: Simplify driver using devm

[Matti Vaittinen]

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

Simplify driver by switching to use the resource managed IRQ
requesting and resource managed work-queue initialization.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
Changelog:
 v2:
  - IRQ freeing fix splitted in own patch

Please note that the change is compile-tested only. All proper testing is
highly appreciated.
---
 drivers/extcon/extcon-max8997.c | 47 +++++++++++----------------------
 1 file changed, 16 insertions(+), 31 deletions(-)

diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
index c15a612067af..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 <dg77.kim@samsung.com>
 
+#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) {
@@ -733,7 +736,7 @@ static int max8997_muic_probe(struct platform_device *pdev)
 				2, info->status);
 	if (ret) {
 		dev_err(info->dev, "failed to read MUIC register\n");
-		goto err_irq;
+		return ret;
 	}
 	cable_type = max8997_muic_get_cable_type(info,
 					   MAX8997_CABLE_GROUP_ADC, &attached);
@@ -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 =] 

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

Re: [PATCH RESEND v2 0/5] Add devm helper for work-queue initialization

[Hans de Goede]

Hi All,

On 6/8/21 12:09 PM, Matti Vaittinen wrote:
> This series adds new devm_work_autocancel() helper.
> 
> Note:
> "The beef" of this series is the new devm-helper. This means that
> normally it would be picked-up by Hans. In this case Hans asked if this
> series could be taken in extconn tree:
> https://lore.kernel.org/lkml/fbbfba71-bdcc-b78f-48be-d7c657adce61@redhat.com/

Yes, and given that most of the changes are in the extcon code I still
believe this is best.

Alternatively I can create an immutable branch with these 5 patches on
top of 5.13-rc1 and then send a pull-req to Chanwoo and MyongJoo.

Chanwoo and/or MyongJoo can you please let us know how you want to proceed
with this series?

Regards,

Hans



> 
> 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.
> 
> Changelog v2:
>   - rebased on v5.13-rc2
>   - split the extcon-max8997 change into two. First a simple,
>     back-portable fix for omitting IRQ freeing at error path, second
>     being the devm-simpification which does not need backporting.
> 
> ---
> 
> Matti Vaittinen (5):
>   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
>   extcon: extcon-max8997: Simplify driver using devm
> 
>  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: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
> 

Re: [PATCH RESEND v2 0/5] Add devm helper for work-queue initialization

[Chanwoo Choi]

Hi Hans, 

On 6/10/21 12:23 AM, Hans de Goede wrote:
> Hi All,
> 
> On 6/8/21 12:09 PM, Matti Vaittinen wrote:
>> This series adds new devm_work_autocancel() helper.
>>
>> Note:
>> "The beef" of this series is the new devm-helper. This means that
>> normally it would be picked-up by Hans. In this case Hans asked if this
>> series could be taken in extconn tree:
>> https://lore.kernel.org/lkml/fbbfba71-bdcc-b78f-48be-d7c657adce61@redhat.com/
> 
> Yes, and given that most of the changes are in the extcon code I still
> believe this is best.
> 
> Alternatively I can create an immutable branch with these 5 patches on
> top of 5.13-rc1 and then send a pull-req to Chanwoo and MyongJoo.

Right. After creating the immutable branch, please send pull-request to me.
I'll merge them. Thanks.

> 
> Chanwoo and/or MyongJoo can you please let us know how you want to proceed
> with this series?
> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>> 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.
>>
>> Changelog v2:
>>   - rebased on v5.13-rc2
>>   - split the extcon-max8997 change into two. First a simple,
>>     back-portable fix for omitting IRQ freeing at error path, second
>>     being the devm-simpification which does not need backporting.
>>
>> ---
>>
>> Matti Vaittinen (5):
>>   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
>>   extcon: extcon-max8997: Simplify driver using devm
>>
>>  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: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
>>
> 
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RESEND v2 0/5] Add devm helper for work-queue initialization

[Vaittinen, Matti]

> Hi Hans, 
> 
> On 6/10/21 12:23 AM, Hans de Goede wrote:
> > Hi All,
> > 
> > On 6/8/21 12:09 PM, Matti Vaittinen wrote:
> > > This series adds new devm_work_autocancel() helper.
> > > 
> > > Note:
> > > "The beef" of this series is the new devm-helper. This means that
> > > normally it would be picked-up by Hans. In this case Hans asked
> > > if this
> > > series could be taken in extconn tree:
> > > https://lore.kernel.org/lkml/fbbfba71-bdcc-b78f-48be-d7c657adce61@redhat.com/
> > 
> > Yes, and given that most of the changes are in the extcon code I
> > still
> > believe this is best.
> > 
> > Alternatively I can create an immutable branch with these 5 patches
> > on
> > top of 5.13-rc1 and then send a pull-req to Chanwoo and MyongJoo.
> 
> Right. After creating the immutable branch, please send pull-request
> to me.
> I'll merge them. Thanks.

Chanwoo, I think the series has no formal ack from you... Can this be
treated as such?

Best Regards
	Matti Vaittinen


--
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 for the translation Simon)

Re: [PATCH RESEND v2 2/5] extcon: extcon-max14577: Fix potential work-queue cancellation race

[Chanwoo Choi]

On 6/8/21 7:09 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 
> 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 <cw00.choi@samsung.com>
>  // Krzysztof Kozlowski <krzk@kernel.org>
>  
> +#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,
>  };
>  
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Chanwoo Choi]

On 6/8/21 7:10 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 
> 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 <cw00.choi@samsung.com>
>  
> +#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);

I think that you have to keep the input_unregister_device().

> -
> -	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);
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Hans de Goede]

Hi,

On 6/10/21 11:43 AM, Chanwoo Choi wrote:
> On 6/8/21 7:10 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>
>> 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 <cw00.choi@samsung.com>
>>  
>> +#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);
> 
> I think that you have to keep the input_unregister_device().

As mentioned in the commit message, in input_unregister_device
is not necessary for input-devices created with
devm_input_allocate_device():

"According to the input documentation the input device allocated by
devm_input_allocate_device() does not need to be explicitly unregistered."

I have verified that the documentation is correct here, so there is
no need to keep the input_unregister_device() as it was never necessary
to have that there.

Regards,

Hans



> 
>> -
>> -	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);
>>
> 

Re: [PATCH RESEND v2 4/5] extcon: extcon-max8997: Fix IRQ freeing at error path

[Chanwoo Choi]

On 6/8/21 7:10 PM, Matti Vaittinen wrote:
> If reading MAX8997_MUIC_REG_STATUS1 fails at probe the driver exits
> without freeing the requested IRQs.
> 
> Free the IRQs prior returning if reading the status fails.
> 
> Fixes: 3e34c8198960 ("extcon: max8997: Avoid forcing UART path on drive probe")
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changelog:
>  v2:
>    - new patch (avoid backporting devm_wq just to fix IRQ freeing)
> ---
>  drivers/extcon/extcon-max8997.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
> index e1408075ef7d..c15a612067af 100644
> --- a/drivers/extcon/extcon-max8997.c
> +++ b/drivers/extcon/extcon-max8997.c
> @@ -733,7 +733,7 @@ static int max8997_muic_probe(struct platform_device *pdev)
>  				2, info->status);
>  	if (ret) {
>  		dev_err(info->dev, "failed to read MUIC register\n");
> -		return ret;
> +		goto err_irq;
>  	}
>  	cable_type = max8997_muic_get_cable_type(info,
>  					   MAX8997_CABLE_GROUP_ADC, &attached);
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Matti Vaittinen]

On Thu, 2021-06-10 at 18:43 +0900, Chanwoo Choi wrote:
> On 6/8/21 7:10 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com
> > >
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > 
> > 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 <cw00.choi@samsung.com>
> >  
> > +#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);
> 
> I think that you have to keep the input_unregister_device().

Are you sure? I can add back the remove() if required - but the
kerneldoc for devm_input_allocate_device() seems to be suggesting that
this would not be needed:

 * Managed input devices do not need to be explicitly unregistered or
 * freed as it will be done automatically when owner device unbinds
from
 * its driver (or binding fails). Once managed input device is
allocated,
 * it is ready to be set up and registered in the same fashion as
regular
 * input device. There are no special devm_input_device_[un]register()
 * variants, regular ones work with both managed and unmanaged devices,
 * should you need them. In most cases however, managed input device
need
 * not be explicitly unregistered or freed.

https://elixir.bootlin.com/linux/v5.13-rc5/source/drivers/input/input.c#L1955

I am not going to argue with you though - I am not really familiar with
the input subsystem. I'd appreciate if someone could shed some light on
when the input_unregister_device() can be omitted? 

Best Regards
	Matti Vaittinen

Re: [PATCH RESEND v2 5/5] extcon: extcon-max8997: Simplify driver using devm

[Chanwoo Choi]

On 6/8/21 7:10 PM, Matti Vaittinen wrote:
> Simplify driver by switching to use the resource managed IRQ
> requesting and resource managed work-queue initialization.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changelog:
>  v2:
>   - IRQ freeing fix splitted in own patch
> 
> Please note that the change is compile-tested only. All proper testing is
> highly appreciated.
> ---
>  drivers/extcon/extcon-max8997.c | 47 +++++++++++----------------------
>  1 file changed, 16 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
> index c15a612067af..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 <dg77.kim@samsung.com>
>  
> +#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) {
> @@ -733,7 +736,7 @@ static int max8997_muic_probe(struct platform_device *pdev)
>  				2, info->status);
>  	if (ret) {
>  		dev_err(info->dev, "failed to read MUIC register\n");
> -		goto err_irq;
> +		return ret;
>  	}
>  	cable_type = max8997_muic_get_cable_type(info,
>  					   MAX8997_CABLE_GROUP_ADC, &attached);
> @@ -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);
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Dmitry Torokhov]

On Thu, Jun 10, 2021 at 12:57:40PM +0300, Matti Vaittinen wrote:
> 
> On Thu, 2021-06-10 at 18:43 +0900, Chanwoo Choi wrote:
> > On 6/8/21 7:10 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com
> > > >
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > 
> > > 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 <cw00.choi@samsung.com>
> > >  
> > > +#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);
> > 
> > I think that you have to keep the input_unregister_device().
> 
> Are you sure? I can add back the remove() if required - but the
> kerneldoc for devm_input_allocate_device() seems to be suggesting that
> this would not be needed:
> 
>  * Managed input devices do not need to be explicitly unregistered or
>  * freed as it will be done automatically when owner device unbinds
> from
>  * its driver (or binding fails). Once managed input device is
> allocated,
>  * it is ready to be set up and registered in the same fashion as
> regular
>  * input device. There are no special devm_input_device_[un]register()
>  * variants, regular ones work with both managed and unmanaged devices,
>  * should you need them. In most cases however, managed input device
> need
>  * not be explicitly unregistered or freed.
> 
> https://elixir.bootlin.com/linux/v5.13-rc5/source/drivers/input/input.c#L1955
> 
> I am not going to argue with you though - I am not really familiar with
> the input subsystem. I'd appreciate if someone could shed some light on
> when the input_unregister_device() can be omitted? 

That is correct, you do not need to call input_unregister_device() for
input devices allocated with devm_input_allocate_device().

Thanks.

-- 
Dmitry

Re: [PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Chanwoo Choi]

On 6/10/21 6:57 PM, Matti Vaittinen wrote:
> 
> On Thu, 2021-06-10 at 18:43 +0900, Chanwoo Choi wrote:
>> On 6/8/21 7:10 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com
>>>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>
>>> 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 <cw00.choi@samsung.com>
>>>  
>>> +#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);
>>
>> I think that you have to keep the input_unregister_device().
> 
> Are you sure? I can add back the remove() if required - but the
> kerneldoc for devm_input_allocate_device() seems to be suggesting that
> this would not be needed:
> 
>  * Managed input devices do not need to be explicitly unregistered or
>  * freed as it will be done automatically when owner device unbinds
> from
>  * its driver (or binding fails). Once managed input device is
> allocated,
>  * it is ready to be set up and registered in the same fashion as
> regular
>  * input device. There are no special devm_input_device_[un]register()
>  * variants, regular ones work with both managed and unmanaged devices,
>  * should you need them. In most cases however, managed input device
> need
>  * not be explicitly unregistered or freed.
> 
> https://protect2.fireeye.com/v1/url?k=ffe6f053-a07dc951-ffe77b1c-0cc47a312ab0-aa86636d08cba7ad&q=1&e=f8aab92a-f090-4b48-91f8-4dffa41042e9&u=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.13-rc5%2Fsource%2Fdrivers%2Finput%2Finput.c%23L1955
> 
> I am not going to argue with you though - I am not really familiar with
> the input subsystem. I'd appreciate if someone could shed some light on
> when the input_unregister_device() can be omitted? 

You're right. Thanks for pointing out.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH RESEND v2 3/5] extcon: extcon-max77693.c: Fix potential work-queue cancellation race

[Chanwoo Choi]

On 6/10/21 6:49 PM, Hans de Goede wrote:
> Hi,
> 
> On 6/10/21 11:43 AM, Chanwoo Choi wrote:
>> On 6/8/21 7:10 PM, 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 <matti.vaittinen@fi.rohmeurope.com>
>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>
>>> 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 <cw00.choi@samsung.com>
>>>  
>>> +#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);
>>
>> I think that you have to keep the input_unregister_device().
> 
> As mentioned in the commit message, in input_unregister_device
> is not necessary for input-devices created with
> devm_input_allocate_device():
> 
> "According to the input documentation the input device allocated by
> devm_input_allocate_device() does not need to be explicitly unregistered."
> 
> I have verified that the documentation is correct here, so there is
> no need to keep the input_unregister_device() as it was never necessary
> to have that there.

You're right. I got it from Matti Vaittinen review.
I replied the ack message.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics