[PATCH] auxdisplay: seg-led-gpio: fix work initialization race and convert to devm_add_action_or_reset()

[PATCH] auxdisplay: seg-led-gpio: fix work initialization race and convert to devm_add_action_or_reset()

[kr494167]

From: Surendra Singh Chouhan <kr494167@gmail.com>

INIT_DELAYED_WORK(&priv->work, seg_led_update) was previously called inside
seg_led_linedisp_get_map_type(), which is invoked during/after
linedisp_register(). Initializing a delayed_work structure inside a map
query callback can re-initialize an active work item or race with
seg_led_linedisp_update().

In addition, seg_led_remove() called cancel_delayed_work_sync(&priv->work)
before linedisp_unregister(&priv->linedisp), allowing sysfs updates to
reschedule work after cancel_delayed_work_sync() completed.

Fix these by moving INIT_DELAYED_WORK() to probe() and using
devm_add_action_or_reset() for devm-managed cleanup. Registering
seg_led_unregister_linedisp after seg_led_cancel_work ensures proper LIFO
teardown order (sysfs interface unregistered first, followed by work
cancellation), allowing seg_led_remove() to be removed entirely.

Fixes: 899383f9ecf5 ("auxdisplay: Add 7-segment LED display driver")
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
 drivers/auxdisplay/seg-led-gpio.c | 35 +++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/auxdisplay/seg-led-gpio.c b/drivers/auxdisplay/seg-led-gpio.c
index bc463118fe51..c83246309701 100644
--- a/drivers/auxdisplay/seg-led-gpio.c
+++ b/drivers/auxdisplay/seg-led-gpio.c
@@ -40,9 +40,6 @@ static void seg_led_update(struct work_struct *work)
 
 static int seg_led_linedisp_get_map_type(struct linedisp *linedisp)
 {
-	struct seg_led_priv *priv = container_of(linedisp, struct seg_led_priv, linedisp);
-
-	INIT_DELAYED_WORK(&priv->work, seg_led_update);
 	return LINEDISP_MAP_SEG7;
 }
 
@@ -58,10 +55,25 @@ static const struct linedisp_ops seg_led_linedisp_ops = {
 	.update = seg_led_linedisp_update,
 };
 
+static void seg_led_cancel_work(void *data)
+{
+	struct seg_led_priv *priv = data;
+
+	cancel_delayed_work_sync(&priv->work);
+}
+
+static void seg_led_unregister_linedisp(void *data)
+{
+	struct seg_led_priv *priv = data;
+
+	linedisp_unregister(&priv->linedisp);
+}
+
 static int seg_led_probe(struct platform_device *pdev)
 {
 	struct seg_led_priv *priv;
 	struct device *dev = &pdev->dev;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -76,15 +88,17 @@ static int seg_led_probe(struct platform_device *pdev)
 	if (priv->segment_gpios->ndescs < 7 || priv->segment_gpios->ndescs > 8)
 		return -EINVAL;
 
-	return linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
-}
+	INIT_DELAYED_WORK(&priv->work, seg_led_update);
 
-static void seg_led_remove(struct platform_device *pdev)
-{
-	struct seg_led_priv *priv = platform_get_drvdata(pdev);
+	ret = devm_add_action_or_reset(dev, seg_led_cancel_work, priv);
+	if (ret)
+		return ret;
 
-	cancel_delayed_work_sync(&priv->work);
-	linedisp_unregister(&priv->linedisp);
+	ret = linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, seg_led_unregister_linedisp, priv);
 }
 
 static const struct of_device_id seg_led_of_match[] = {
@@ -95,7 +109,6 @@ MODULE_DEVICE_TABLE(of, seg_led_of_match);
 
 static struct platform_driver seg_led_driver = {
 	.probe = seg_led_probe,
-	.remove = seg_led_remove,
 	.driver = {
 		.name = "seg-led-gpio",
 		.of_match_table = seg_led_of_match,
-- 
2.55.0

Re: [PATCH] auxdisplay: seg-led-gpio: fix work initialization race and convert to devm_add_action_or_reset()

[Chris Packham]

Hi Surendra

On 24/07/2026 14:55, kr494167@gmail.com wrote:
> From: Surendra Singh Chouhan <kr494167@gmail.com>
>
> INIT_DELAYED_WORK(&priv->work, seg_led_update) was previously called inside
> seg_led_linedisp_get_map_type(), which is invoked during/after
> linedisp_register(). Initializing a delayed_work structure inside a map
> query callback can re-initialize an active work item or race with
> seg_led_linedisp_update().
>
> In addition, seg_led_remove() called cancel_delayed_work_sync(&priv->work)
> before linedisp_unregister(&priv->linedisp), allowing sysfs updates to
> reschedule work after cancel_delayed_work_sync() completed.
>
> Fix these by moving INIT_DELAYED_WORK() to probe() and using
> devm_add_action_or_reset() for devm-managed cleanup. Registering
> seg_led_unregister_linedisp after seg_led_cancel_work ensures proper LIFO
> teardown order (sysfs interface unregistered first, followed by work
> cancellation), allowing seg_led_remove() to be removed entirely.
>
> Fixes: 899383f9ecf5 ("auxdisplay: Add 7-segment LED display driver")
> Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>

Reviewed-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
Tested-by: Chris Packham <chris.packham@alliedtelesis.co.nz>

Thanks

> ---
>   drivers/auxdisplay/seg-led-gpio.c | 35 +++++++++++++++++++++----------
>   1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/auxdisplay/seg-led-gpio.c b/drivers/auxdisplay/seg-led-gpio.c
> index bc463118fe51..c83246309701 100644
> --- a/drivers/auxdisplay/seg-led-gpio.c
> +++ b/drivers/auxdisplay/seg-led-gpio.c
> @@ -40,9 +40,6 @@ static void seg_led_update(struct work_struct *work)
>   
>   static int seg_led_linedisp_get_map_type(struct linedisp *linedisp)
>   {
> -	struct seg_led_priv *priv = container_of(linedisp, struct seg_led_priv, linedisp);
> -
> -	INIT_DELAYED_WORK(&priv->work, seg_led_update);
>   	return LINEDISP_MAP_SEG7;
>   }
>   
> @@ -58,10 +55,25 @@ static const struct linedisp_ops seg_led_linedisp_ops = {
>   	.update = seg_led_linedisp_update,
>   };
>   
> +static void seg_led_cancel_work(void *data)
> +{
> +	struct seg_led_priv *priv = data;
> +
> +	cancel_delayed_work_sync(&priv->work);
> +}
> +
> +static void seg_led_unregister_linedisp(void *data)
> +{
> +	struct seg_led_priv *priv = data;
> +
> +	linedisp_unregister(&priv->linedisp);
> +}
> +
>   static int seg_led_probe(struct platform_device *pdev)
>   {
>   	struct seg_led_priv *priv;
>   	struct device *dev = &pdev->dev;
> +	int ret;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
> @@ -76,15 +88,17 @@ static int seg_led_probe(struct platform_device *pdev)
>   	if (priv->segment_gpios->ndescs < 7 || priv->segment_gpios->ndescs > 8)
>   		return -EINVAL;
>   
> -	return linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
> -}
> +	INIT_DELAYED_WORK(&priv->work, seg_led_update);
>   
> -static void seg_led_remove(struct platform_device *pdev)
> -{
> -	struct seg_led_priv *priv = platform_get_drvdata(pdev);
> +	ret = devm_add_action_or_reset(dev, seg_led_cancel_work, priv);
> +	if (ret)
> +		return ret;
>   
> -	cancel_delayed_work_sync(&priv->work);
> -	linedisp_unregister(&priv->linedisp);
> +	ret = linedisp_register(&priv->linedisp, dev, 1, &seg_led_linedisp_ops);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, seg_led_unregister_linedisp, priv);
>   }
>   
>   static const struct of_device_id seg_led_of_match[] = {
> @@ -95,7 +109,6 @@ MODULE_DEVICE_TABLE(of, seg_led_of_match);
>   
>   static struct platform_driver seg_led_driver = {
>   	.probe = seg_led_probe,
> -	.remove = seg_led_remove,
>   	.driver = {
>   		.name = "seg-led-gpio",
>   		.of_match_table = seg_led_of_match,

Re: [PATCH] auxdisplay: seg-led-gpio: fix work initialization race and convert to devm_add_action_or_reset()

[Andy Shevchenko]

On Fri, Jul 24, 2026 at 5:55 AM <kr494167@gmail.com> wrote:
>
> From: Surendra Singh Chouhan <kr494167@gmail.com>
>
> INIT_DELAYED_WORK(&priv->work, seg_led_update) was previously called inside
> seg_led_linedisp_get_map_type(), which is invoked during/after
> linedisp_register(). Initializing a delayed_work structure inside a map
> query callback can re-initialize an active work item or race with
> seg_led_linedisp_update().
>
> In addition, seg_led_remove() called cancel_delayed_work_sync(&priv->work)
> before linedisp_unregister(&priv->linedisp), allowing sysfs updates to
> reschedule work after cancel_delayed_work_sync() completed.
>
> Fix these by moving INIT_DELAYED_WORK() to probe() and using
> devm_add_action_or_reset() for devm-managed cleanup. Registering
> seg_led_unregister_linedisp after seg_led_cancel_work ensures proper LIFO
> teardown order (sysfs interface unregistered first, followed by work
> cancellation), allowing seg_led_remove() to be removed entirely.

I think we have more drivers than this one with the same issue, no? If
so, can we solve this once for all (the existing and possible future
ones)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] auxdisplay: seg-led-gpio: fix work initialization race and convert to devm_add_action_or_reset()

[Surendra Singh]

Hi Andy, Chris,

Thanks for the review!

I checked the other drivers in drivers/auxdisplay/ and found that
drivers/auxdisplay/max6959.c has the exact same pattern:

1. INIT_DELAYED_WORK(&priv->work, max6959_disp_update) is called inside
   max6959_linedisp_get_map_type() (map query callback).
2. max6959_i2c_remove() calls cancel_delayed_work_sync(&priv->work)
   before linedisp_unregister(&priv->linedisp).

I can prepare a 2-patch series fixing max6959.c as well.

Furthermore, we could add devm_linedisp_register() to line-display.c
to simplify linedisp teardown across all auxdisplay drivers.

Best regards,
Surendra Singh Chouhan


On Sun, 2 Aug 2026 at 14:59, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Fri, Jul 24, 2026 at 5:55 AM <kr494167@gmail.com> wrote:
> >
> > From: Surendra Singh Chouhan <kr494167@gmail.com>
> >
> > INIT_DELAYED_WORK(&priv->work, seg_led_update) was previously called inside
> > seg_led_linedisp_get_map_type(), which is invoked during/after
> > linedisp_register(). Initializing a delayed_work structure inside a map
> > query callback can re-initialize an active work item or race with
> > seg_led_linedisp_update().
> >
> > In addition, seg_led_remove() called cancel_delayed_work_sync(&priv->work)
> > before linedisp_unregister(&priv->linedisp), allowing sysfs updates to
> > reschedule work after cancel_delayed_work_sync() completed.
> >
> > Fix these by moving INIT_DELAYED_WORK() to probe() and using
> > devm_add_action_or_reset() for devm-managed cleanup. Registering
> > seg_led_unregister_linedisp after seg_led_cancel_work ensures proper LIFO
> > teardown order (sysfs interface unregistered first, followed by work
> > cancellation), allowing seg_led_remove() to be removed entirely.
>
> I think we have more drivers than this one with the same issue, no? If
> so, can we solve this once for all (the existing and possible future
> ones)?
>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH] auxdisplay: seg-led-gpio: fix work initialization race and convert to devm_add_action_or_reset()

[Andy Shevchenko]

On Sun, Aug 02, 2026 at 04:30:16PM +0530, Surendra Singh wrote:

> I checked the other drivers in drivers/auxdisplay/ and found that
> drivers/auxdisplay/max6959.c has the exact same pattern:
> 
> 1. INIT_DELAYED_WORK(&priv->work, max6959_disp_update) is called inside
>    max6959_linedisp_get_map_type() (map query callback).
> 2. max6959_i2c_remove() calls cancel_delayed_work_sync(&priv->work)
>    before linedisp_unregister(&priv->linedisp).
> 
> I can prepare a 2-patch series fixing max6959.c as well.

Yes, please do. Or did I miss them?

> Furthermore, we could add devm_linedisp_register() to line-display.c
> to simplify linedisp teardown across all auxdisplay drivers.

That's what we all are waiting for to avoid some UAF types of bugs.

> On Sun, 2 Aug 2026 at 14:59, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jul 24, 2026 at 5:55 AM <kr494167@gmail.com> wrote:

> > > INIT_DELAYED_WORK(&priv->work, seg_led_update) was previously called inside
> > > seg_led_linedisp_get_map_type(), which is invoked during/after
> > > linedisp_register(). Initializing a delayed_work structure inside a map
> > > query callback can re-initialize an active work item or race with
> > > seg_led_linedisp_update().
> > >
> > > In addition, seg_led_remove() called cancel_delayed_work_sync(&priv->work)
> > > before linedisp_unregister(&priv->linedisp), allowing sysfs updates to
> > > reschedule work after cancel_delayed_work_sync() completed.
> > >
> > > Fix these by moving INIT_DELAYED_WORK() to probe() and using
> > > devm_add_action_or_reset() for devm-managed cleanup. Registering
> > > seg_led_unregister_linedisp after seg_led_cancel_work ensures proper LIFO
> > > teardown order (sysfs interface unregistered first, followed by work
> > > cancellation), allowing seg_led_remove() to be removed entirely.
> >
> > I think we have more drivers than this one with the same issue, no? If
> > so, can we solve this once for all (the existing and possible future
> > ones)?

-- 
With Best Regards,
Andy Shevchenko