[PATCH] hid: cp2112: Fix duplicate workqueue initialization

[PATCH] hid: cp2112: Fix duplicate workqueue initialization

[Danny Kaehn]

Previously the cp2112 driver called INIT_DELAYED_WORK within
cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
workqueue on subsequent IRQ startups following an initial request. This
resulted in a warning in set_work_data in workqueue.c, as well as a rare
NULL dereference within process_one_work in workqueue.c.

Initialize the workqueue within _probe instead.

Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
---

Note -- the warning & NULL dereference that were caused by this were
completely decoupled from the driver code, making this a fairly tricky
bug to track down. I wonder if there would be a way to add a WARN into
__INIT_WORK if an already initialized workqueue is re-initialized
without a lot of overhead...

 drivers/hid/hid-cp2112.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 54c33a24f844..36f76c6dfa20 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1151,8 +1151,6 @@ static unsigned int cp2112_gpio_irq_startup(struct irq_data *d)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct cp2112_device *dev = gpiochip_get_data(gc);
 
-	INIT_DELAYED_WORK(&dev->gpio_poll_worker, cp2112_gpio_poll_callback);
-
 	if (!dev->gpio_poll) {
 		dev->gpio_poll = true;
 		schedule_delayed_work(&dev->gpio_poll_worker, 0);
@@ -1307,6 +1305,8 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	girq->handler = handle_simple_irq;
 	girq->threaded = true;
 
+	INIT_DELAYED_WORK(&dev->gpio_poll_worker, cp2112_gpio_poll_callback);
+
 	ret = gpiochip_add_data(&dev->gc, dev);
 	if (ret < 0) {
 		hid_err(hdev, "error registering gpio chip\n");
-- 
2.25.1

Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization

[Andy Shevchenko]

On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> Previously the cp2112 driver called INIT_DELAYED_WORK within
> cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> workqueue on subsequent IRQ startups following an initial request. This
> resulted in a warning in set_work_data in workqueue.c, as well as a rare
> NULL dereference within process_one_work in workqueue.c.
> 
> Initialize the workqueue within _probe instead.

Does it deserve a Fixes tag?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization

[Danny Kaehn]

On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > workqueue on subsequent IRQ startups following an initial request. This
> > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > NULL dereference within process_one_work in workqueue.c.
> > 
> > Initialize the workqueue within _probe instead.
> 
> Does it deserve a Fixes tag?

I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
(HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
revision (a.e. with git am)

(my understanding is that 'Fixes' is used for stable kernel maintenance?)

Thanks,

Danny Kaehn

Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization

[andriy.shevchenko]

On Wed, Sep 20, 2023 at 07:10:15PM +0000, Danny Kaehn wrote:
> On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> > On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > > workqueue on subsequent IRQ startups following an initial request. This
> > > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > > NULL dereference within process_one_work in workqueue.c.
> > > 
> > > Initialize the workqueue within _probe instead.
> > 
> > Does it deserve a Fixes tag?
> 
> I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
> (HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
> revision (a.e. with git am)

It's not a problem.

> (my understanding is that 'Fixes' is used for stable kernel maintenance?)

Not only, for anybody to track the issues and fixes.
For stable it's more important to follow their procedure, where the Fixes
is just one small piece of.

Fixes: 13de9cca514e ("HID: cp2112: add IRQ chip handling")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] hid: cp2112: Fix duplicate workqueue initialization

[Jiri Kosina]

On Thu, 21 Sep 2023, andriy.shevchenko@linux.intel.com wrote:

> On Wed, Sep 20, 2023 at 07:10:15PM +0000, Danny Kaehn wrote:
> > On Wed, 2023-09-20 at 16:04 +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 19, 2023 at 04:22:45PM -0500, Danny Kaehn wrote:
> > > > Previously the cp2112 driver called INIT_DELAYED_WORK within
> > > > cp2112_gpio_irq_startup, resulting in duplicate initilizations of the
> > > > workqueue on subsequent IRQ startups following an initial request. This
> > > > resulted in a warning in set_work_data in workqueue.c, as well as a rare
> > > > NULL dereference within process_one_work in workqueue.c.
> > > > 
> > > > Initialize the workqueue within _probe instead.

I have applied the patch now, thanks.

> > > Does it deserve a Fixes tag?
> > 
> > I'm not sure -- it does technically fix 13de9cca514ed63604263cad87ca8cb36e9b6489
> > (HID: cp2112: add IRQ chip handling), but does not apply cleanly to that
> > revision (a.e. with git am)

From my very own direct experience I can assure you, that the Fixes: tag 
is being heavily used outside of -stable process: e.g. by distros who 
don't base on -stable on purpose.

Thanks,

-- 
Jiri Kosina
SUSE Labs