| Message ID | 20230919212245.483646-1-danny.kaehn@plexus.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | hid: cp2112: Fix duplicate workqueue initialization | expand |
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?
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
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")
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,
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");
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(-)