[PATCH 0/9] workqueue / drivers: Add device-managed allocate workqueue

[PATCH 0/9] workqueue / drivers: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

Merging / Dependency
====================
All further patches depend on the first one, thus this probably should
go via one tree, e.g. power supply.  The first patch might be needed for
other trees as well, e.g. if more drivers are discovered, so the best if
it is on dedicated branch in case it has to be shared.

Description
===========
Add a Resource-managed version of alloc_workqueue() to fix common
problem of drivers mixing devm() calls with destroy_workqueue.  Such
naive and discouraged driver approach leads to difficult to debug bugs
when the driver:

1. Allocates workqueue in standard way and destroys it in driver
remove() callback,
2. Sets work struct with devm_work_autocancel(),
3. Registers interrupt handler with devm_request_threaded_irq().

Which leads to following unbind/removal path:

1. destroy_workqueue() via driver remove(),
Any interrupt coming now would still execute the interrupt handler,
which queues work on destroyed workqueue.
2. devm_irq_release(),
3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.

devm_alloc_workqueue() has two benefits:
1. Solves above problem of mix-and-match devres and non-devres code in
driver,
2. Simplify any sane drivers which were correctly using
alloc_workqueue() + devm_add_action_or_reset().

Best regards,
Krzysztof

---
Krzysztof Kozlowski (9):
      workqueue: devres: Add device-managed allocate workqueue
      power: supply: cw2015: Free allocated workqueue
      power: supply: max77705: Free allocated workqueue and fix removal order
      power: supply: mt6370: Simplify with devm_create_singlethread_workqueue
      power: supply: ipaq_micro: Simplify with devm
      mfd: ezx-pcap: Drop memory allocation error message
      mfd: ezx-pcap: Return directly instead of empty gotos
      mfd: ezx-pcap: Avoid rescheduling after destroying workqueue
      platform/chrome: cros_usbpd_logger: Simplify with devm

 Documentation/driver-api/driver-model/devres.rst |  7 ++++
 drivers/mfd/ezx-pcap.c                           | 27 +++++--------
 drivers/platform/chrome/cros_usbpd_logger.c      | 18 ++++-----
 drivers/power/supply/cw2015_battery.c            |  3 +-
 drivers/power/supply/ipaq_micro_battery.c        | 50 ++++++++----------------
 drivers/power/supply/max77705_charger.c          | 36 ++++++-----------
 drivers/power/supply/mt6370-charger.c            | 13 +-----
 include/linux/workqueue.h                        | 32 +++++++++++++++
 kernel/workqueue.c                               | 32 +++++++++++++++
 9 files changed, 117 insertions(+), 101 deletions(-)
---
base-commit: bc32aa8c2aea9fd3acda58dd8a5ea6c17e9dfc36
change-id: 20260220-workqueue-devm-d9591e5e70eb

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

[PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

Add a Resource-managed version of alloc_workqueue() to fix common
problem of drivers mixing devm() calls with destroy_workqueue.  Such
naive and discouraged driver approach leads to difficult to debug bugs
when the driver:

1. Allocates workqueue in standard way and destroys it in driver
   remove() callback,
2. Sets work struct with devm_work_autocancel(),
3. Registers interrupt handler with devm_request_threaded_irq().

Which leads to following unbind/removal path:

1. destroy_workqueue() via driver remove(),
   Any interrupt coming now would still execute the interrupt handler,
   which queues work on destroyed workqueue.
2. devm_irq_release(),
3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.

devm_alloc_workqueue() has two benefits:
1. Solves above problem of mix-and-match devres and non-devres code in
   driver,
2. Simplify any sane drivers which were correctly using
   alloc_workqueue() + devm_add_action_or_reset().

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---

All further patches depend on this one.
---
 Documentation/driver-api/driver-model/devres.rst |  7 ++++++
 include/linux/workqueue.h                        | 32 ++++++++++++++++++++++++
 kernel/workqueue.c                               | 32 ++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
index 7d2b897d66fa..dfc1e85ebfaa 100644
--- a/Documentation/driver-api/driver-model/devres.rst
+++ b/Documentation/driver-api/driver-model/devres.rst
@@ -464,3 +464,10 @@ SPI
 
 WATCHDOG
   devm_watchdog_register_device()
+
+WORKQUEUE
+  devm_alloc_workqueue()
+  devm_alloc_ordered_workqueue()
+  devm_create_workqueue()
+  devm_create_freezable_workqueue()
+  devm_create_singlethread_workqueue()
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index a4749f56398f..583b0c4bd55c 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -512,6 +512,26 @@ __printf(1, 4) struct workqueue_struct *
 alloc_workqueue_noprof(const char *fmt, unsigned int flags, int max_active, ...);
 #define alloc_workqueue(...)	alloc_hooks(alloc_workqueue_noprof(__VA_ARGS__))
 
+/**
+ * devm_alloc_workqueue - Resource-managed allocate a workqueue
+ * @dev: Device to allocate workqueue for
+ * @fmt: printf format for the name of the workqueue
+ * @flags: WQ_* flags
+ * @max_active: max in-flight work items, 0 for default
+ * @...: args for @fmt
+ *
+ * Resource managed workqueue, see alloc_workqueue() for details.
+ *
+ * The workqueue will be automatically destroyed on driver detach.  Typically
+ * this should be used in drivers already relying on devm interafaces.
+ *
+ * RETURNS:
+ * Pointer to the allocated workqueue on success, %NULL on failure.
+ */
+__printf(2, 5) struct workqueue_struct *
+devm_alloc_workqueue(struct device *dev, const char *fmt, unsigned int flags,
+		     int max_active, ...);
+
 #ifdef CONFIG_LOCKDEP
 /**
  * alloc_workqueue_lockdep_map - allocate a workqueue with user-defined lockdep_map
@@ -568,19 +588,31 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
  */
 #define alloc_ordered_workqueue(fmt, flags, args...)			\
 	alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)
+#define devm_alloc_ordered_workqueue(dev, fmt, flags, args...)		\
+	devm_alloc_workqueue(dev, fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)
 
 #define create_workqueue(name)						\
 	alloc_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_PERCPU, 1, (name))
+#define devm_create_workqueue(dev, name)				\
+	devm_alloc_workqueue(dev, "%s", __WQ_LEGACY | WQ_MEM_RECLAIM | WQ_PERCPU, 1, (name))
+
 #define create_freezable_workqueue(name)				\
 	alloc_workqueue("%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND |	\
 			WQ_MEM_RECLAIM, 1, (name))
+#define devm_create_freezable_workqueue(dev, name)			\
+	devm_alloc_workqueue(dev, "%s", __WQ_LEGACY | WQ_FREEZABLE | WQ_UNBOUND |	\
+			     WQ_MEM_RECLAIM, 1, (name))
+
 #define create_singlethread_workqueue(name)				\
 	alloc_ordered_workqueue("%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
+#define devm_create_singlethread_workqueue(dev, name)			\
+	devm_alloc_ordered_workqueue(dev, "%s", __WQ_LEGACY | WQ_MEM_RECLAIM, name)
 
 #define from_work(var, callback_work, work_fieldname)	\
 	container_of(callback_work, typeof(*var), work_fieldname)
 
 extern void destroy_workqueue(struct workqueue_struct *wq);
+extern void devm_destroy_workqueue(struct device *dev, void *res);
 
 struct workqueue_attrs *alloc_workqueue_attrs_noprof(void);
 #define alloc_workqueue_attrs(...)	alloc_hooks(alloc_workqueue_attrs_noprof(__VA_ARGS__))
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 72a8b64188b3..fd840c46ba55 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -41,6 +41,7 @@
 #include <linux/mempolicy.h>
 #include <linux/freezer.h>
 #include <linux/debug_locks.h>
+#include <linux/device/devres.h>
 #include <linux/lockdep.h>
 #include <linux/idr.h>
 #include <linux/jhash.h>
@@ -5895,6 +5896,31 @@ struct workqueue_struct *alloc_workqueue_noprof(const char *fmt,
 }
 EXPORT_SYMBOL_GPL(alloc_workqueue_noprof);
 
+__printf(2, 5) struct workqueue_struct *
+devm_alloc_workqueue(struct device *dev, const char *fmt, unsigned int flags,
+		     int max_active, ...)
+{
+	struct workqueue_struct **ptr, *wq;
+	va_list args;
+
+	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return NULL;
+
+	va_start(args, max_active);
+	wq = alloc_workqueue(fmt, flags, max_active, args);
+	va_end(args);
+	if (wq) {
+		*ptr = wq;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return wq;
+}
+EXPORT_SYMBOL_GPL(devm_alloc_workqueue);
+
 #ifdef CONFIG_LOCKDEP
 __printf(1, 5)
 struct workqueue_struct *
@@ -6025,6 +6051,12 @@ void destroy_workqueue(struct workqueue_struct *wq)
 }
 EXPORT_SYMBOL_GPL(destroy_workqueue);
 
+void devm_destroy_workqueue(struct device *dev, void *res)
+{
+	destroy_workqueue(*(struct workqueue_struct **)res);
+}
+EXPORT_SYMBOL_GPL(devm_destroy_workqueue);
+
 /**
  * workqueue_set_max_active - adjust max_active of a workqueue
  * @wq: target workqueue

-- 
2.51.0

[PATCH 2/9] power: supply: cw2015: Free allocated workqueue

[Krzysztof Kozlowski]

Use devm interface so allocated workqueue will be freed during device
removal and error paths, thus fixing memory leak.

Cc: <stable@vger.kernel.org>
Fixes: b4c7715c10c1 ("power: supply: add CellWise cw2015 fuel gauge driver")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---

Depends on devm_create_singlethread_workqueue() from earlier patches.
---
 drivers/power/supply/cw2015_battery.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c
index a05dcc4a48f2..1e4e3b4c7460 100644
--- a/drivers/power/supply/cw2015_battery.c
+++ b/drivers/power/supply/cw2015_battery.c
@@ -694,7 +694,8 @@ static int cw_bat_probe(struct i2c_client *client)
 			 "No monitored battery, some properties will be missing\n");
 	}
 
-	cw_bat->battery_workqueue = create_singlethread_workqueue("rk_battery");
+	cw_bat->battery_workqueue = devm_create_singlethread_workqueue(&client->dev,
+								       "rk_battery");
 	if (!cw_bat->battery_workqueue)
 		return -ENOMEM;
 

-- 
2.51.0

[PATCH 3/9] power: supply: max77705: Free allocated workqueue and fix removal order

[Krzysztof Kozlowski]

Use devm interface for allocating workqueue to fix two bugs at the same
time:

1. Driver leaks the memory on remove(), because the workqueue is not
   destroyed.

2. Driver allocates workqueue and then registers interrupt handlers
   with devm interface.  This means that probe error paths will not use a
   reversed order, but first the destroy workqueue and then, via devm
   release handlers, free the interrupt.

   The interrupt handler schedules work on this exact workqueue, thus if
   interrupt is hit in this short time window - after destroying
   workqueue, but before devm() frees the interrupt, the work scheduling
   will lead to use of freed memory.

Fixes: 11741b8e382d ("power: supply: max77705: Fix workqueue error handling in probe")
Fixes: a6a494c8e3ce ("power: supply: max77705: Add charger driver for Maxim 77705")
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/power/supply/max77705_charger.c | 36 ++++++++++-----------------------
 1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/drivers/power/supply/max77705_charger.c b/drivers/power/supply/max77705_charger.c
index 5dd02f658f5b..de12c215366c 100644
--- a/drivers/power/supply/max77705_charger.c
+++ b/drivers/power/supply/max77705_charger.c
@@ -646,51 +646,37 @@ static int max77705_charger_probe(struct i2c_client *i2c)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to add irq chip\n");
 
-	chg->wqueue = create_singlethread_workqueue(dev_name(dev));
+	chg->wqueue = devm_create_singlethread_workqueue(dev, dev_name(dev));
 	if (!chg->wqueue)
 		return -ENOMEM;
 
 	ret = devm_work_autocancel(dev, &chg->chgin_work, max77705_chgin_isr_work);
-	if (ret) {
-		dev_err_probe(dev, ret, "failed to initialize interrupt work\n");
-		goto destroy_wq;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize interrupt work\n");
 
 	ret = max77705_charger_initialize(chg);
-	if (ret) {
-		dev_err_probe(dev, ret, "failed to initialize charger IC\n");
-		goto destroy_wq;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to initialize charger IC\n");
 
 	ret = devm_request_threaded_irq(dev, regmap_irq_get_virq(irq_data, MAX77705_CHGIN_I),
 					NULL, max77705_chgin_irq,
 					IRQF_TRIGGER_NONE,
 					"chgin-irq", chg);
-	if (ret) {
-		dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
-		goto destroy_wq;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
 
 	ret = devm_request_threaded_irq(dev, regmap_irq_get_virq(irq_data, MAX77705_AICL_I),
 					NULL, max77705_aicl_irq,
 					IRQF_TRIGGER_NONE,
 					"aicl-irq", chg);
-	if (ret) {
-		dev_err_probe(dev, ret, "Failed to Request aicl IRQ\n");
-		goto destroy_wq;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to Request aicl IRQ\n");
 
 	ret = max77705_charger_enable(chg);
-	if (ret) {
-		dev_err_probe(dev, ret, "failed to enable charge\n");
-		goto destroy_wq;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable charge\n");
 
 	return devm_add_action_or_reset(dev, max77705_charger_disable, chg);
-
-destroy_wq:
-	destroy_workqueue(chg->wqueue);
-	return ret;
 }
 
 static const struct of_device_id max77705_charger_of_match[] = {

-- 
2.51.0

[PATCH 4/9] power: supply: mt6370: Simplify with devm_create_singlethread_workqueue

[Krzysztof Kozlowski]

Simplify the driver probe function by using
devm_create_singlethread_workqueue() which handles the cleanup already.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/power/supply/mt6370-charger.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/power/supply/mt6370-charger.c b/drivers/power/supply/mt6370-charger.c
index e6db961d5818..f671fe62876a 100644
--- a/drivers/power/supply/mt6370-charger.c
+++ b/drivers/power/supply/mt6370-charger.c
@@ -761,13 +761,6 @@ static int mt6370_chg_init_psy(struct mt6370_priv *priv)
 	return PTR_ERR_OR_ZERO(priv->psy);
 }
 
-static void mt6370_chg_destroy_wq(void *data)
-{
-	struct workqueue_struct *wq = data;
-
-	destroy_workqueue(wq);
-}
-
 static irqreturn_t mt6370_attach_i_handler(int irq, void *data)
 {
 	struct mt6370_priv *priv = data;
@@ -893,14 +886,10 @@ static int mt6370_chg_probe(struct platform_device *pdev)
 
 	priv->attach = MT6370_ATTACH_STAT_DETACH;
 
-	priv->wq = create_singlethread_workqueue(dev_name(priv->dev));
+	priv->wq = devm_create_singlethread_workqueue(dev, dev_name(priv->dev));
 	if (!priv->wq)
 		return -ENOMEM;
 
-	ret = devm_add_action_or_reset(dev, mt6370_chg_destroy_wq, priv->wq);
-	if (ret)
-		return ret;
-
 	ret = devm_work_autocancel(dev, &priv->bc12_work, mt6370_chg_bc12_work_func);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init bc12 work\n");

-- 
2.51.0

[PATCH 5/9] power: supply: ipaq_micro: Simplify with devm

[Krzysztof Kozlowski]

Simplify the driver by using devm interfaces, which allow to drop
probe() error paths and the remove() callback.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/power/supply/ipaq_micro_battery.c | 50 ++++++++++---------------------
 1 file changed, 16 insertions(+), 34 deletions(-)

diff --git a/drivers/power/supply/ipaq_micro_battery.c b/drivers/power/supply/ipaq_micro_battery.c
index ff8573a5ca6d..5e3fe3852d0e 100644
--- a/drivers/power/supply/ipaq_micro_battery.c
+++ b/drivers/power/supply/ipaq_micro_battery.c
@@ -7,6 +7,7 @@
  * Author : Linus Walleij <linus.walleij@linaro.org>
  */
 
+#include <linux/devm-helpers.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
@@ -232,49 +233,31 @@ static int micro_batt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mb->micro = dev_get_drvdata(pdev->dev.parent);
-	mb->wq = alloc_workqueue("ipaq-battery-wq",
-				 WQ_MEM_RECLAIM | WQ_PERCPU, 0);
+	mb->wq = devm_alloc_workqueue(&pdev->dev, "ipaq-battery-wq",
+				      WQ_MEM_RECLAIM | WQ_PERCPU, 0);
 	if (!mb->wq)
 		return -ENOMEM;
 
-	INIT_DELAYED_WORK(&mb->update, micro_battery_work);
+	ret = devm_delayed_work_autocancel(&pdev->dev, &mb->update, micro_battery_work);
+	if (ret)
+		return ret;
+
 	platform_set_drvdata(pdev, mb);
 	queue_delayed_work(mb->wq, &mb->update, 1);
 
-	micro_batt_power = power_supply_register(&pdev->dev,
-						 &micro_batt_power_desc, NULL);
-	if (IS_ERR(micro_batt_power)) {
-		ret = PTR_ERR(micro_batt_power);
-		goto batt_err;
-	}
+	micro_batt_power = devm_power_supply_register(&pdev->dev,
+						      &micro_batt_power_desc,
+						      NULL);
+	if (IS_ERR(micro_batt_power))
+		return PTR_ERR(micro_batt_power);
 
-	micro_ac_power = power_supply_register(&pdev->dev,
-					       &micro_ac_power_desc, NULL);
-	if (IS_ERR(micro_ac_power)) {
-		ret = PTR_ERR(micro_ac_power);
-		goto ac_err;
-	}
+	micro_ac_power = devm_power_supply_register(&pdev->dev,
+						    &micro_ac_power_desc, NULL);
+	if (IS_ERR(micro_ac_power))
+		return PTR_ERR(micro_ac_power);
 
 	dev_info(&pdev->dev, "iPAQ micro battery driver\n");
 	return 0;
-
-ac_err:
-	power_supply_unregister(micro_batt_power);
-batt_err:
-	cancel_delayed_work_sync(&mb->update);
-	destroy_workqueue(mb->wq);
-	return ret;
-}
-
-static void micro_batt_remove(struct platform_device *pdev)
-
-{
-	struct micro_battery *mb = platform_get_drvdata(pdev);
-
-	power_supply_unregister(micro_ac_power);
-	power_supply_unregister(micro_batt_power);
-	cancel_delayed_work_sync(&mb->update);
-	destroy_workqueue(mb->wq);
 }
 
 static int __maybe_unused micro_batt_suspend(struct device *dev)
@@ -303,7 +286,6 @@ static struct platform_driver micro_batt_device_driver = {
 		.pm	= &micro_batt_dev_pm_ops,
 	},
 	.probe		= micro_batt_probe,
-	.remove		= micro_batt_remove,
 };
 module_platform_driver(micro_batt_device_driver);
 

-- 
2.51.0

[PATCH 6/9] mfd: ezx-pcap: Drop memory allocation error message

[Krzysztof Kozlowski]

Drivers should not print error messages on memory allocation failures,
because core already does it.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---

Further patch depends on this one, thus it should not be picked
separately.
---
 drivers/mfd/ezx-pcap.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index 1be4557b7bdd..13cb185638a9 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -416,7 +416,6 @@ static int ezx_pcap_probe(struct spi_device *spi)
 	pcap->workqueue = create_singlethread_workqueue("pcapd");
 	if (!pcap->workqueue) {
 		ret = -ENOMEM;
-		dev_err(&spi->dev, "can't create pcap thread\n");
 		goto ret;
 	}
 

-- 
2.51.0

[PATCH 7/9] mfd: ezx-pcap: Return directly instead of empty gotos

[Krzysztof Kozlowski]

Code is easier to read if empty error paths simply return, instead of
jumping to empty label doing only "return ret".

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/mfd/ezx-pcap.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index 13cb185638a9..b929559d84ae 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -384,17 +384,15 @@ static int ezx_pcap_probe(struct spi_device *spi)
 	struct pcap_platform_data *pdata = dev_get_platdata(&spi->dev);
 	struct pcap_chip *pcap;
 	int i, adc_irq;
-	int ret = -ENODEV;
+	int ret;
 
 	/* platform data is required */
 	if (!pdata)
-		goto ret;
+		return -ENODEV;
 
 	pcap = devm_kzalloc(&spi->dev, sizeof(*pcap), GFP_KERNEL);
-	if (!pcap) {
-		ret = -ENOMEM;
-		goto ret;
-	}
+	if (!pcap)
+		return -ENOMEM;
 
 	spin_lock_init(&pcap->io_lock);
 	spin_lock_init(&pcap->adc_lock);
@@ -407,17 +405,15 @@ static int ezx_pcap_probe(struct spi_device *spi)
 	spi->mode = SPI_MODE_0 | (pdata->config & PCAP_CS_AH ? SPI_CS_HIGH : 0);
 	ret = spi_setup(spi);
 	if (ret)
-		goto ret;
+		return ret;
 
 	pcap->spi = spi;
 
 	/* setup irq */
 	pcap->irq_base = pdata->irq_base;
 	pcap->workqueue = create_singlethread_workqueue("pcapd");
-	if (!pcap->workqueue) {
-		ret = -ENOMEM;
-		goto ret;
-	}
+	if (!pcap->workqueue)
+		return -ENOMEM;
 
 	/* redirect interrupts to AP, except adcdone2 */
 	if (!(pdata->config & PCAP_SECOND_PORT))

-- 
2.51.0

[PATCH 8/9] mfd: ezx-pcap: Avoid rescheduling after destroying workqueue

[Krzysztof Kozlowski]

Driver allocates workqueue and then registers additional interrupt
handler with devm interface.  This means that device removal will not
use a reversed order, but first destroy workqueue and then, via devm
release handlers, free the interrupt.

The interrupt handler registered with devm does not directly
use/schedule work items on the workqueue and the remove() function
correctly removes other IRQs handlers, however the code mixing devm and
non-devm interfaces is difficult to analyze and read.

Make the code flow much more obvious by using devm interface for
allocating the workqueue, so it will be freed with the rest of devm
resources.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

---

Depends on devm_create_singlethread_workqueue() from earlier patches.
---
 drivers/mfd/ezx-pcap.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/ezx-pcap.c b/drivers/mfd/ezx-pcap.c
index b929559d84ae..a06fc3447104 100644
--- a/drivers/mfd/ezx-pcap.c
+++ b/drivers/mfd/ezx-pcap.c
@@ -375,8 +375,6 @@ static void ezx_pcap_remove(struct spi_device *spi)
 	/* cleanup irqchip */
 	for (i = pcap->irq_base; i < (pcap->irq_base + PCAP_NIRQS); i++)
 		irq_set_chip_and_handler(i, NULL, NULL);
-
-	destroy_workqueue(pcap->workqueue);
 }
 
 static int ezx_pcap_probe(struct spi_device *spi)
@@ -411,7 +409,7 @@ static int ezx_pcap_probe(struct spi_device *spi)
 
 	/* setup irq */
 	pcap->irq_base = pdata->irq_base;
-	pcap->workqueue = create_singlethread_workqueue("pcapd");
+	pcap->workqueue = devm_create_singlethread_workqueue(&spi->dev, "pcapd");
 	if (!pcap->workqueue)
 		return -ENOMEM;
 
@@ -463,9 +461,7 @@ static int ezx_pcap_probe(struct spi_device *spi)
 free_irqchip:
 	for (i = pcap->irq_base; i < (pcap->irq_base + PCAP_NIRQS); i++)
 		irq_set_chip_and_handler(i, NULL, NULL);
-/* destroy_workqueue: */
-	destroy_workqueue(pcap->workqueue);
-ret:
+
 	return ret;
 }
 

-- 
2.51.0

[PATCH 9/9] platform/chrome: cros_usbpd_logger: Simplify with devm

[Krzysztof Kozlowski]

Simplify the driver by using devm interfaces, which allow to drop
probe() error paths and the remove() callback.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/platform/chrome/cros_usbpd_logger.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/chrome/cros_usbpd_logger.c b/drivers/platform/chrome/cros_usbpd_logger.c
index 7ce75e2e039e..b0d176c0f4cc 100644
--- a/drivers/platform/chrome/cros_usbpd_logger.c
+++ b/drivers/platform/chrome/cros_usbpd_logger.c
@@ -5,6 +5,7 @@
  * Copyright 2018 Google LLC.
  */
 
+#include <linux/devm-helpers.h>
 #include <linux/ktime.h>
 #include <linux/math64.h>
 #include <linux/mod_devicetable.h>
@@ -199,6 +200,7 @@ static int cros_usbpd_logger_probe(struct platform_device *pd)
 	struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
 	struct device *dev = &pd->dev;
 	struct logger_data *logger;
+	int ret;
 
 	logger = devm_kzalloc(dev, sizeof(*logger), GFP_KERNEL);
 	if (!logger)
@@ -210,25 +212,20 @@ static int cros_usbpd_logger_probe(struct platform_device *pd)
 	platform_set_drvdata(pd, logger);
 
 	/* Retrieve PD event logs periodically */
-	INIT_DELAYED_WORK(&logger->log_work, cros_usbpd_log_check);
-	logger->log_workqueue =	create_singlethread_workqueue("cros_usbpd_log");
+	logger->log_workqueue =	devm_create_singlethread_workqueue(dev, "cros_usbpd_log");
 	if (!logger->log_workqueue)
 		return -ENOMEM;
 
+	ret = devm_delayed_work_autocancel(dev, &logger->log_work, cros_usbpd_log_check);
+	if (ret)
+		return ret;
+
 	queue_delayed_work(logger->log_workqueue, &logger->log_work,
 			   CROS_USBPD_LOG_UPDATE_DELAY);
 
 	return 0;
 }
 
-static void cros_usbpd_logger_remove(struct platform_device *pd)
-{
-	struct logger_data *logger = platform_get_drvdata(pd);
-
-	cancel_delayed_work_sync(&logger->log_work);
-	destroy_workqueue(logger->log_workqueue);
-}
-
 static int __maybe_unused cros_usbpd_logger_resume(struct device *dev)
 {
 	struct logger_data *logger = dev_get_drvdata(dev);
@@ -263,7 +260,6 @@ static struct platform_driver cros_usbpd_logger_driver = {
 		.pm = &cros_usbpd_logger_pm_ops,
 	},
 	.probe = cros_usbpd_logger_probe,
-	.remove = cros_usbpd_logger_remove,
 	.id_table = cros_usbpd_logger_id,
 };
 

-- 
2.51.0

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Andy Shevchenko]

On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
> Add a Resource-managed version of alloc_workqueue() to fix common
> problem of drivers mixing devm() calls with destroy_workqueue.  Such
> naive and discouraged driver approach leads to difficult to debug bugs
> when the driver:
> 
> 1. Allocates workqueue in standard way and destroys it in driver
>    remove() callback,
> 2. Sets work struct with devm_work_autocancel(),
> 3. Registers interrupt handler with devm_request_threaded_irq().
> 
> Which leads to following unbind/removal path:
> 
> 1. destroy_workqueue() via driver remove(),
>    Any interrupt coming now would still execute the interrupt handler,
>    which queues work on destroyed workqueue.
> 2. devm_irq_release(),
> 3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.
> 
> devm_alloc_workqueue() has two benefits:
> 1. Solves above problem of mix-and-match devres and non-devres code in
>    driver,
> 2. Simplify any sane drivers which were correctly using
>    alloc_workqueue() + devm_add_action_or_reset().

>  include/linux/workqueue.h                        | 32 ++++++++++++++++++++++++
>  kernel/workqueue.c                               | 32 ++++++++++++++++++++++++

Hmm... We have devm-helpers.h. Why the new one is in workqueue.h?
Can we have some consistency here?

...

> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return NULL;
> +
> +	va_start(args, max_active);
> +	wq = alloc_workqueue(fmt, flags, max_active, args);
> +	va_end(args);
> +	if (wq) {
> +		*ptr = wq;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}

Why not using devm_add_action_or_reset()?

...

> +void devm_destroy_workqueue(struct device *dev, void *res)
> +{
> +	destroy_workqueue(*(struct workqueue_struct **)res);
> +}
> +EXPORT_SYMBOL_GPL(devm_destroy_workqueue);

Is this going to be used?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/9] power: supply: max77705: Free allocated workqueue and fix removal order

[Andy Shevchenko]

On Mon, Feb 23, 2026 at 08:27:31AM +0100, Krzysztof Kozlowski wrote:
> Use devm interface for allocating workqueue to fix two bugs at the same
> time:
> 
> 1. Driver leaks the memory on remove(), because the workqueue is not
>    destroyed.
> 
> 2. Driver allocates workqueue and then registers interrupt handlers
>    with devm interface.  This means that probe error paths will not use a
>    reversed order, but first the destroy workqueue and then, via devm
>    release handlers, free the interrupt.
> 
>    The interrupt handler schedules work on this exact workqueue, thus if
>    interrupt is hit in this short time window - after destroying
>    workqueue, but before devm() frees the interrupt, the work scheduling
>    will lead to use of freed memory.

...

>  	ret = devm_request_threaded_irq(dev, regmap_irq_get_virq(irq_data, MAX77705_CHGIN_I),
>  					NULL, max77705_chgin_irq,
>  					IRQF_TRIGGER_NONE,
>  					"chgin-irq", chg);
> -	if (ret) {
> -		dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
> -		goto destroy_wq;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");

This should be just

		return ret;


devm_*_irq() prints the message. No need to repeat this in the caller(s).

...

>  	ret = devm_request_threaded_irq(dev, regmap_irq_get_virq(irq_data, MAX77705_AICL_I),
>  					NULL, max77705_aicl_irq,
>  					IRQF_TRIGGER_NONE,
>  					"aicl-irq", chg);
> -	if (ret) {
> -		dev_err_probe(dev, ret, "Failed to Request aicl IRQ\n");
> -		goto destroy_wq;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to Request aicl IRQ\n");

Ditto.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

On 23/02/2026 09:56, Andy Shevchenko wrote:
> On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
>> Add a Resource-managed version of alloc_workqueue() to fix common
>> problem of drivers mixing devm() calls with destroy_workqueue.  Such
>> naive and discouraged driver approach leads to difficult to debug bugs
>> when the driver:
>>
>> 1. Allocates workqueue in standard way and destroys it in driver
>>    remove() callback,
>> 2. Sets work struct with devm_work_autocancel(),
>> 3. Registers interrupt handler with devm_request_threaded_irq().
>>
>> Which leads to following unbind/removal path:
>>
>> 1. destroy_workqueue() via driver remove(),
>>    Any interrupt coming now would still execute the interrupt handler,
>>    which queues work on destroyed workqueue.
>> 2. devm_irq_release(),
>> 3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.
>>
>> devm_alloc_workqueue() has two benefits:
>> 1. Solves above problem of mix-and-match devres and non-devres code in
>>    driver,
>> 2. Simplify any sane drivers which were correctly using
>>    alloc_workqueue() + devm_add_action_or_reset().
> 
>>  include/linux/workqueue.h                        | 32 ++++++++++++++++++++++++
>>  kernel/workqueue.c                               | 32 ++++++++++++++++++++++++
> 
> Hmm... We have devm-helpers.h. Why the new one is in workqueue.h?
> Can we have some consistency here?

No problem, I can move it there.

> 
> ...
> 
>> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
>> +	if (!ptr)
>> +		return NULL;
>> +
>> +	va_start(args, max_active);
>> +	wq = alloc_workqueue(fmt, flags, max_active, args);
>> +	va_end(args);
>> +	if (wq) {
>> +		*ptr = wq;
>> +		devres_add(dev, ptr);
>> +	} else {
>> +		devres_free(ptr);
>> +	}
> 
> Why not using devm_add_action_or_reset()?

Where? Here? How the code would be simpler, exactly?

> 
> ...
> 
>> +void devm_destroy_workqueue(struct device *dev, void *res)
>> +{
>> +	destroy_workqueue(*(struct workqueue_struct **)res);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_destroy_workqueue);
> 
> Is this going to be used?

It is not used in this patchset, but most of devm-allocators have the
cleanup.


Best regards,
Krzysztof

Re: [PATCH 3/9] power: supply: max77705: Free allocated workqueue and fix removal order

[Krzysztof Kozlowski]

On 23/02/2026 09:57, Andy Shevchenko wrote:
> On Mon, Feb 23, 2026 at 08:27:31AM +0100, Krzysztof Kozlowski wrote:
>> Use devm interface for allocating workqueue to fix two bugs at the same
>> time:
>>
>> 1. Driver leaks the memory on remove(), because the workqueue is not
>>    destroyed.
>>
>> 2. Driver allocates workqueue and then registers interrupt handlers
>>    with devm interface.  This means that probe error paths will not use a
>>    reversed order, but first the destroy workqueue and then, via devm
>>    release handlers, free the interrupt.
>>
>>    The interrupt handler schedules work on this exact workqueue, thus if
>>    interrupt is hit in this short time window - after destroying
>>    workqueue, but before devm() frees the interrupt, the work scheduling
>>    will lead to use of freed memory.
> 
> ...
> 
>>  	ret = devm_request_threaded_irq(dev, regmap_irq_get_virq(irq_data, MAX77705_CHGIN_I),
>>  					NULL, max77705_chgin_irq,
>>  					IRQF_TRIGGER_NONE,
>>  					"chgin-irq", chg);
>> -	if (ret) {
>> -		dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
>> -		goto destroy_wq;
>> -	}
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
> 
> This should be just
> 
> 		return ret;
> 
> 
> devm_*_irq() prints the message. No need to repeat this in the caller(s).
> 

I guess separate commit then.

Best regards,
Krzysztof

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Danilo Krummrich]

On Mon Feb 23, 2026 at 8:27 AM CET, Krzysztof Kozlowski wrote:
> +__printf(2, 5) struct workqueue_struct *
> +devm_alloc_workqueue(struct device *dev, const char *fmt, unsigned int flags,
> +		     int max_active, ...)
> +{
> +	struct workqueue_struct **ptr, *wq;
> +	va_list args;
> +
> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);

The function pointer passed to devres_alloc() is commonly named *_release().

> +	if (!ptr)
> +		return NULL;
> +
> +	va_start(args, max_active);
> +	wq = alloc_workqueue(fmt, flags, max_active, args);
> +	va_end(args);
> +	if (wq) {
> +		*ptr = wq;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return wq;
> +}
> +EXPORT_SYMBOL_GPL(devm_alloc_workqueue);

<snip>

> +void devm_destroy_workqueue(struct device *dev, void *res)
> +{
> +	destroy_workqueue(*(struct workqueue_struct **)res);
> +}
> +EXPORT_SYMBOL_GPL(devm_destroy_workqueue);

I assume you did not mean to export the release callback (which doesn't seem to
be useful), but a function that calls devres_destroy(), i.e. something analogous
to devm_remove_action().

If you don't actually need it, I would prefer not to add something that calls
devres_destroy() for now.

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

On 23/02/2026 11:36, Danilo Krummrich wrote:
> On Mon Feb 23, 2026 at 8:27 AM CET, Krzysztof Kozlowski wrote:
>> +__printf(2, 5) struct workqueue_struct *
>> +devm_alloc_workqueue(struct device *dev, const char *fmt, unsigned int flags,
>> +		     int max_active, ...)
>> +{
>> +	struct workqueue_struct **ptr, *wq;
>> +	va_list args;
>> +
>> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
> 
> The function pointer passed to devres_alloc() is commonly named *_release().

devm-helpers.h disagree :), but I understand poor patterns spread all
over. I will use the release name.

> 
>> +	if (!ptr)
>> +		return NULL;
>> +
>> +	va_start(args, max_active);
>> +	wq = alloc_workqueue(fmt, flags, max_active, args);
>> +	va_end(args);
>> +	if (wq) {
>> +		*ptr = wq;
>> +		devres_add(dev, ptr);
>> +	} else {
>> +		devres_free(ptr);
>> +	}
>> +
>> +	return wq;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_alloc_workqueue);
> 
> <snip>
> 
>> +void devm_destroy_workqueue(struct device *dev, void *res)
>> +{
>> +	destroy_workqueue(*(struct workqueue_struct **)res);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_destroy_workqueue);
> 
> I assume you did not mean to export the release callback (which doesn't seem to
> be useful), but a function that calls devres_destroy(), i.e. something analogous
> to devm_remove_action().
> 
> If you don't actually need it, I would prefer not to add something that calls
> devres_destroy() for now.

Andy pointed out EXPORT is not actually needed, so I will just drop it.

Best regards,
Krzysztof

Re: [PATCH 3/9] power: supply: max77705: Free allocated workqueue and fix removal order

[Andy Shevchenko]

On Mon, Feb 23, 2026 at 11:19:13AM +0100, Krzysztof Kozlowski wrote:
> On 23/02/2026 09:57, Andy Shevchenko wrote:
> > On Mon, Feb 23, 2026 at 08:27:31AM +0100, Krzysztof Kozlowski wrote:

...

> >>  	ret = devm_request_threaded_irq(dev, regmap_irq_get_virq(irq_data, MAX77705_CHGIN_I),
> >>  					NULL, max77705_chgin_irq,
> >>  					IRQF_TRIGGER_NONE,
> >>  					"chgin-irq", chg);
> >> -	if (ret) {
> >> -		dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
> >> -		goto destroy_wq;
> >> -	}
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "Failed to Request chgin IRQ\n");
> > 
> > This should be just
> > 
> > 		return ret;
> > 
> > 
> > devm_*_irq() prints the message. No need to repeat this in the caller(s).
> > 
> 
> I guess separate commit then.

WFM!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Andy Shevchenko]

On Mon, Feb 23, 2026 at 11:18:41AM +0100, Krzysztof Kozlowski wrote:
> On 23/02/2026 09:56, Andy Shevchenko wrote:
> > On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:

...

> >> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
> >> +	if (!ptr)
> >> +		return NULL;
> >> +
> >> +	va_start(args, max_active);
> >> +	wq = alloc_workqueue(fmt, flags, max_active, args);
> >> +	va_end(args);
> >> +	if (wq) {
> >> +		*ptr = wq;
> >> +		devres_add(dev, ptr);
> >> +	} else {
> >> +		devres_free(ptr);
> >> +	}
> > 
> > Why not using devm_add_action_or_reset()?
> 
> Where? Here? How the code would be simpler, exactly?

static void devm_workqueue(struct device *dev, void *wq)
{
	destroy_workqueue(wq);
}
...
{
	...

	va_start(args, max_active);
	wq = alloc_workqueue(fmt, flags, max_active, args);
	va_end(args);
	if (!wq)
		return NULL; // or ERR_PTR(-ENOMEM) on your choice

	ret = devm_add_action_or_reset(dev, ..., wq);
	if (ret)
		return NULL; // ERR_PTR(ret) on your choice

	return wq;
}

Compare to yours :-)

...

> >> +void devm_destroy_workqueue(struct device *dev, void *res)
> >> +{
> >> +	destroy_workqueue(*(struct workqueue_struct **)res);
> >> +}
> >> +EXPORT_SYMBOL_GPL(devm_destroy_workqueue);
> > 
> > Is this going to be used?
> 
> It is not used in this patchset, but most of devm-allocators have the
> cleanup.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

On 23/02/2026 12:34, Andy Shevchenko wrote:
> On Mon, Feb 23, 2026 at 11:18:41AM +0100, Krzysztof Kozlowski wrote:
>> On 23/02/2026 09:56, Andy Shevchenko wrote:
>>> On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
> 
> ...
> 
>>>> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
>>>> +	if (!ptr)
>>>> +		return NULL;
>>>> +
>>>> +	va_start(args, max_active);
>>>> +	wq = alloc_workqueue(fmt, flags, max_active, args);
>>>> +	va_end(args);
>>>> +	if (wq) {
>>>> +		*ptr = wq;
>>>> +		devres_add(dev, ptr);
>>>> +	} else {
>>>> +		devres_free(ptr);
>>>> +	}
>>>
>>> Why not using devm_add_action_or_reset()?
>>
>> Where? Here? How the code would be simpler, exactly?
> 
> static void devm_workqueue(struct device *dev, void *wq)
> {
> 	destroy_workqueue(wq);
> }
> ...
> {
> 	...
> 
> 	va_start(args, max_active);
> 	wq = alloc_workqueue(fmt, flags, max_active, args);
> 	va_end(args);
> 	if (!wq)
> 		return NULL; // or ERR_PTR(-ENOMEM) on your choice
> 
> 	ret = devm_add_action_or_reset(dev, ..., wq);
> 	if (ret)
> 		return NULL; // ERR_PTR(ret) on your choice
> 
> 	return wq;
> }
> 
> Compare to yours :-)

Ah, so dropping the devres_alloc()? Yeah, that would be simpler.


Best regards,
Krzysztof

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Andy Shevchenko]

On Mon, Feb 23, 2026 at 12:43:42PM +0100, Krzysztof Kozlowski wrote:
> On 23/02/2026 12:34, Andy Shevchenko wrote:
> > On Mon, Feb 23, 2026 at 11:18:41AM +0100, Krzysztof Kozlowski wrote:
> >> On 23/02/2026 09:56, Andy Shevchenko wrote:
> >>> On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:

...

> >>>> +	ptr = devres_alloc(devm_destroy_workqueue, sizeof(*ptr), GFP_KERNEL);
> >>>> +	if (!ptr)
> >>>> +		return NULL;
> >>>> +
> >>>> +	va_start(args, max_active);
> >>>> +	wq = alloc_workqueue(fmt, flags, max_active, args);
> >>>> +	va_end(args);
> >>>> +	if (wq) {
> >>>> +		*ptr = wq;
> >>>> +		devres_add(dev, ptr);
> >>>> +	} else {
> >>>> +		devres_free(ptr);
> >>>> +	}
> >>>
> >>> Why not using devm_add_action_or_reset()?
> >>
> >> Where? Here? How the code would be simpler, exactly?
> > 
> > static void devm_workqueue_release(void *wq)
> > {
> > 	destroy_workqueue(wq);
> > }
> > ...
> > {
> > 	...
> > 
> > 	va_start(args, max_active);
> > 	wq = alloc_workqueue(fmt, flags, max_active, args);
> > 	va_end(args);
> > 	if (!wq)
> > 		return NULL; // or ERR_PTR(-ENOMEM) on your choice
> > 
> > 	ret = devm_add_action_or_reset(dev, devm_workqueue_release, wq);
> > 	if (ret)
> > 		return NULL; // ERR_PTR(ret) on your choice
> > 
> > 	return wq;
> > }
> > 
> > Compare to yours :-)
> 
> Ah, so dropping the devres_alloc()? Yeah, that would be simpler.

Yep! The side effect is dropping that ugly casting in _release() along with
unused dev.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

On 23/02/2026 09:56, Andy Shevchenko wrote:
> On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
>> Add a Resource-managed version of alloc_workqueue() to fix common
>> problem of drivers mixing devm() calls with destroy_workqueue.  Such
>> naive and discouraged driver approach leads to difficult to debug bugs
>> when the driver:
>>
>> 1. Allocates workqueue in standard way and destroys it in driver
>>    remove() callback,
>> 2. Sets work struct with devm_work_autocancel(),
>> 3. Registers interrupt handler with devm_request_threaded_irq().
>>
>> Which leads to following unbind/removal path:
>>
>> 1. destroy_workqueue() via driver remove(),
>>    Any interrupt coming now would still execute the interrupt handler,
>>    which queues work on destroyed workqueue.
>> 2. devm_irq_release(),
>> 3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.
>>
>> devm_alloc_workqueue() has two benefits:
>> 1. Solves above problem of mix-and-match devres and non-devres code in
>>    driver,
>> 2. Simplify any sane drivers which were correctly using
>>    alloc_workqueue() + devm_add_action_or_reset().
> 
>>  include/linux/workqueue.h                        | 32 ++++++++++++++++++++++++
>>  kernel/workqueue.c                               | 32 ++++++++++++++++++++++++
> 
> Hmm... We have devm-helpers.h. Why the new one is in workqueue.h?
> Can we have some consistency here?
> 

Answering with update:
I don't think this should go to devm-helpers.h. The definition is in
workqueue.c, thus the declaration should be in corresponding header.
It's logical and consistent.

Otherwise, I could move it entirely - definition and declaration - to
devm-helpers.h, but then the release (devm_destroy_workqueue()) will be
essentially exported to everyone through the header.

So kind of conflicting choices.

Best regards,
Krzysztof

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Andy Shevchenko]

+Cc: devm-helpers maintainers/reviewers

On Mon, Feb 23, 2026 at 12:52:14PM +0100, Krzysztof Kozlowski wrote:
> On 23/02/2026 09:56, Andy Shevchenko wrote:
> > On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
> >> Add a Resource-managed version of alloc_workqueue() to fix common
> >> problem of drivers mixing devm() calls with destroy_workqueue.  Such
> >> naive and discouraged driver approach leads to difficult to debug bugs
> >> when the driver:
> >>
> >> 1. Allocates workqueue in standard way and destroys it in driver
> >>    remove() callback,
> >> 2. Sets work struct with devm_work_autocancel(),
> >> 3. Registers interrupt handler with devm_request_threaded_irq().
> >>
> >> Which leads to following unbind/removal path:
> >>
> >> 1. destroy_workqueue() via driver remove(),
> >>    Any interrupt coming now would still execute the interrupt handler,
> >>    which queues work on destroyed workqueue.
> >> 2. devm_irq_release(),
> >> 3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.
> >>
> >> devm_alloc_workqueue() has two benefits:
> >> 1. Solves above problem of mix-and-match devres and non-devres code in
> >>    driver,
> >> 2. Simplify any sane drivers which were correctly using
> >>    alloc_workqueue() + devm_add_action_or_reset().
> > 
> >>  include/linux/workqueue.h                        | 32 ++++++++++++++++++++++++
> >>  kernel/workqueue.c                               | 32 ++++++++++++++++++++++++
> > 
> > Hmm... We have devm-helpers.h. Why the new one is in workqueue.h?
> > Can we have some consistency here?
> 
> Answering with update:
> I don't think this should go to devm-helpers.h. The definition is in
> workqueue.c, thus the declaration should be in corresponding header.
> It's logical and consistent.
> 
> Otherwise, I could move it entirely - definition and declaration - to
> devm-helpers.h, but then the release (devm_destroy_workqueue()) will be
> essentially exported to everyone through the header.
> 
> So kind of conflicting choices.

Hmm... An alternative I see is more intrusive but should make it less
inconsistent: Treat the devm-helpers as devres like header for workqueue
and collect there all devm_*wq* related stuff with maybe something putting
back to / holding in the c-file.

OTOH we may leave devm_destroy_workqueue() visible for now with a comment
saying do not use, it's internal or something like that.

Hans, what would be your opinion as you IIRC is the author of devm-helpers.h?

Matti, I also Cc'ed to you, you have usually non-standard thinkig and
insightful solutions (besides being reviewer of devm-helpers).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Matti Vaittinen]

ma 23.2.2026 klo 14.13 Andy Shevchenko
(andriy.shevchenko@linux.intel.com) kirjoitti:
>
> +Cc: devm-helpers maintainers/reviewers
>
> On Mon, Feb 23, 2026 at 12:52:14PM +0100, Krzysztof Kozlowski wrote:
> > On 23/02/2026 09:56, Andy Shevchenko wrote:
> > > On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
> > >> Add a Resource-managed version of alloc_workqueue() to fix common
> > >> problem of drivers mixing devm() calls with destroy_workqueue.  Such
> > >> naive and discouraged driver approach leads to difficult to debug bugs
> > >> when the driver:
> > >>
> > >> 1. Allocates workqueue in standard way and destroys it in driver
> > >>    remove() callback,
> > >> 2. Sets work struct with devm_work_autocancel(),
> > >> 3. Registers interrupt handler with devm_request_threaded_irq().
> > >>
> > >> Which leads to following unbind/removal path:
> > >>
> > >> 1. destroy_workqueue() via driver remove(),
> > >>    Any interrupt coming now would still execute the interrupt handler,
> > >>    which queues work on destroyed workqueue.
> > >> 2. devm_irq_release(),
> > >> 3. devm_work_drop() -> cancel_work_sync() on destroyed workqueue.
> > >>
> > >> devm_alloc_workqueue() has two benefits:
> > >> 1. Solves above problem of mix-and-match devres and non-devres code in
> > >>    driver,
> > >> 2. Simplify any sane drivers which were correctly using
> > >>    alloc_workqueue() + devm_add_action_or_reset().
> > >
> > >>  include/linux/workqueue.h                        | 32 ++++++++++++++++++++++++
> > >>  kernel/workqueue.c                               | 32 ++++++++++++++++++++++++
> > >
> > > Hmm... We have devm-helpers.h. Why the new one is in workqueue.h?
> > > Can we have some consistency here?
> >
> > Answering with update:
> > I don't think this should go to devm-helpers.h. The definition is in
> > workqueue.c, thus the declaration should be in corresponding header.
> > It's logical and consistent.
> >
> > Otherwise, I could move it entirely - definition and declaration - to
> > devm-helpers.h, but then the release (devm_destroy_workqueue()) will be
> > essentially exported to everyone through the header.
> >
> > So kind of conflicting choices.
>
> Hmm... An alternative I see is more intrusive but should make it less
> inconsistent: Treat the devm-helpers as devres like header for workqueue
> and collect there all devm_*wq* related stuff with maybe something putting
> back to / holding in the c-file.
>
> OTOH we may leave devm_destroy_workqueue() visible for now with a comment
> saying do not use, it's internal or something like that.
>
> Hans, what would be your opinion as you IIRC is the author of devm-helpers.h?
>
> Matti, I also Cc'ed to you, you have usually non-standard thinkig and
> insightful solutions (besides being reviewer of devm-helpers).

Thanks Andy for Cc'ing (and flattering!) me ;)

My personal opinion is that the devm-helpers.h as a "collect 'em all
here" -place might not have been the best idea. Perhaps the original
author didn't know what he was doing :) Problem lurking there is that
such a file collecting misc devm -interfaces in one place, will end up
slowing down the compilation while some people put effort on splitting
the headers.

Currently the devm-helpers.h contains only workqueue related
devm-helpers. I'd consider renaming it to something like "devm-wq.h" -
and then just stuffing the devm-wq stuff in it. Optionally, I'd
considered adding all the wq-devm stiff from the devm-helpers.h to the
regular workqueue headers - but I think this was not liked by
everyone.

So sorry, I have no helpful or strong opinion on this. Besides, You,
Krzysztof and Hans all probably have better insight on this than I do.

Yours,
    -- Matti
-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Tejun Heo]

Hello,

On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
> @@ -568,19 +588,31 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
>   */
>  #define alloc_ordered_workqueue(fmt, flags, args...)			\
>  	alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)
> +#define devm_alloc_ordered_workqueue(dev, fmt, flags, args...)		\
> +	devm_alloc_workqueue(dev, fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)

Let's just add devm_alloc_workqueue() and devm_alloc_ordered_workqueue() and
skip the legacy wrappers.

Thanks.

-- 
tejun

Re: [PATCH 1/9] workqueue: devres: Add device-managed allocate workqueue

[Krzysztof Kozlowski]

On 23/02/2026 16:42, Tejun Heo wrote:
> Hello,
> 
> On Mon, Feb 23, 2026 at 08:27:29AM +0100, Krzysztof Kozlowski wrote:
>> @@ -568,19 +588,31 @@ alloc_workqueue_lockdep_map(const char *fmt, unsigned int flags, int max_active,
>>   */
>>  #define alloc_ordered_workqueue(fmt, flags, args...)			\
>>  	alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)
>> +#define devm_alloc_ordered_workqueue(dev, fmt, flags, args...)		\
>> +	devm_alloc_workqueue(dev, fmt, WQ_UNBOUND | __WQ_ORDERED | (flags), 1, ##args)
> 
> Let's just add devm_alloc_workqueue() and devm_alloc_ordered_workqueue() and
> skip the legacy wrappers.

Ack, I will replace legacy calls then in the drivers.

Best regards,
Krzysztof