From: Benjamin Tissoires <[email protected]>
If the device is plugged/unplugged without giving time for mcp_init_work()
to complete, we might kick in the devm free code path and thus have
unavailable struct mcp_2221 while in delayed work.
Add a boolean and a spinlock to prevent scheduling the deleyed work if
we are in the operation of removing the device.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
Similar to Pietro's series, we can see the pattern in hid-mcp2221,
except that this time the ledclass is not involved.
Link: https://lore.kernel.org/linux-input/[email protected]/
---
drivers/hid/hid-mcp2221.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index e61dd039354b..de8b988f4a48 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -95,6 +95,8 @@ struct mcp2221 {
struct mutex lock;
struct completion wait_in_report;
struct delayed_work init_work;
+ spinlock_t init_work_lock;
+ bool removing;
u8 *rxbuf;
u8 txbuf[64];
int rxbuf_idx;
@@ -922,6 +924,14 @@ static void mcp2221_hid_unregister(void *ptr)
/* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
static void mcp2221_remove(struct hid_device *hdev)
{
+ struct mcp2221 *mcp = hid_get_drvdata(hdev);
+ unsigned long flags;
+
+ spin_lock_irqsave(&mcp->init_work_lock, flags);
+ mcp->removing = true;
+ spin_unlock_irqrestore(&mcp->init_work_lock, flags);
+
+ cancel_delayed_work_sync(&mcp->init_work);
}
#if IS_REACHABLE(CONFIG_IIO)
@@ -1040,6 +1050,7 @@ static void mcp_init_work(struct work_struct *work)
struct mcp2221_iio *data;
static int retries = 5;
int ret, num_channels;
+ unsigned long flags;
hid_hw_power(mcp->hdev, PM_HINT_FULLON);
mutex_lock(&mcp->lock);
@@ -1090,7 +1101,10 @@ static void mcp_init_work(struct work_struct *work)
return;
/* Device is not ready to read SRAM or FLASH data, try again */
- schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+ spin_lock_irqsave(&mcp->init_work_lock, flags);
+ if (!mcp->removing)
+ schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+ spin_unlock_irqrestore(&mcp->init_work_lock, flags);
}
#endif
@@ -1131,6 +1145,7 @@ static int mcp2221_probe(struct hid_device *hdev,
}
mutex_init(&mcp->lock);
+ spin_lock_init(&mcp->init_work_lock);
init_completion(&mcp->wait_in_report);
hid_set_drvdata(hdev, mcp);
mcp->hdev = hdev;
---
base-commit: d883fd110dc17308a1506c5bf17e00ce9fe7b2a2
change-id: 20230215-wip-mcp2221-979d4115efb5
Best regards,
--
Benjamin Tissoires <[email protected]>
On Feb 15 2023, Benjamin Tissoires via B4 Submission Endpoint wrote:
> From: Benjamin Tissoires <[email protected]>
>
> If the device is plugged/unplugged without giving time for mcp_init_work()
> to complete, we might kick in the devm free code path and thus have
> unavailable struct mcp_2221 while in delayed work.
>
> Add a boolean and a spinlock to prevent scheduling the deleyed work if
> we are in the operation of removing the device.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> Similar to Pietro's series, we can see the pattern in hid-mcp2221,
> except that this time the ledclass is not involved.
>
> Link: https://lore.kernel.org/linux-input/[email protected]/
> ---
> drivers/hid/hid-mcp2221.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index e61dd039354b..de8b988f4a48 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -95,6 +95,8 @@ struct mcp2221 {
> struct mutex lock;
> struct completion wait_in_report;
> struct delayed_work init_work;
> + spinlock_t init_work_lock;
> + bool removing;
> u8 *rxbuf;
> u8 txbuf[64];
> int rxbuf_idx;
> @@ -922,6 +924,14 @@ static void mcp2221_hid_unregister(void *ptr)
> /* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
> static void mcp2221_remove(struct hid_device *hdev)
> {
> + struct mcp2221 *mcp = hid_get_drvdata(hdev);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mcp->init_work_lock, flags);
> + mcp->removing = true;
> + spin_unlock_irqrestore(&mcp->init_work_lock, flags);
> +
> + cancel_delayed_work_sync(&mcp->init_work);
Actually, given that the only re-submission of this work is from the
work item itself, I wonder if I really need the boolean and the
spinlock. cancel_delayed_work_sync() might already prevent a
resubmission by itself as it does in cancel_work_sync().
Cheers,
Benjamin
> }
>
> #if IS_REACHABLE(CONFIG_IIO)
> @@ -1040,6 +1050,7 @@ static void mcp_init_work(struct work_struct *work)
> struct mcp2221_iio *data;
> static int retries = 5;
> int ret, num_channels;
> + unsigned long flags;
>
> hid_hw_power(mcp->hdev, PM_HINT_FULLON);
> mutex_lock(&mcp->lock);
> @@ -1090,7 +1101,10 @@ static void mcp_init_work(struct work_struct *work)
> return;
>
> /* Device is not ready to read SRAM or FLASH data, try again */
> - schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
> + spin_lock_irqsave(&mcp->init_work_lock, flags);
> + if (!mcp->removing)
> + schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
> + spin_unlock_irqrestore(&mcp->init_work_lock, flags);
> }
> #endif
>
> @@ -1131,6 +1145,7 @@ static int mcp2221_probe(struct hid_device *hdev,
> }
>
> mutex_init(&mcp->lock);
> + spin_lock_init(&mcp->init_work_lock);
> init_completion(&mcp->wait_in_report);
> hid_set_drvdata(hdev, mcp);
> mcp->hdev = hdev;
>
> ---
> base-commit: d883fd110dc17308a1506c5bf17e00ce9fe7b2a2
> change-id: 20230215-wip-mcp2221-979d4115efb5
>
> Best regards,
> --
> Benjamin Tissoires <[email protected]>
>