Re: [PATCH v3 1/2] HID: bigben: use spinlock to safely schedule workers

On Mon, Feb 13, 2023 at 11:37 AM Hillf Danton [off-list ref] wrote:

On Mon, 13 Feb 2023 09:25:37 +0100 Benjamin Tissoires [off-list ref]

quoted

The remove flag is just a way to prevent any other workqueue from
starting. It doesn't mean that the struct bigben has been freed, so
acquiring a lock at that point is fine.
We then rely on 2 things:
- devm_class_led to be released before struct bigben, because it was
devm-allocated *after* the struct bigben was devm-allocated

This is the current behavior and it is intact after this patch.

quoted

- we prevent any new workqueue to start and we guarantee that any
running workqueue is terminated before leaving the .remove function.

If spinlock is added for not scheduling new workqueue then it is not
needed, because the removed flag is set before running workqueue is
terminated. Checking the flag is enough upon queuing new work.

I tend to disagree (based on Pietro's v4:
- no worker is running
- a led sysfs call is made
- the line "if (!bigben->removed)" is true
  - this gets interrupted/or another CPU kicks in for the next one
-> .remove gets called
- bigben->removed is set to false
- cancel_work_sync() called

the led call continues, and schedules the work

.removes terminates, and devm kicks in, killing led_class and struct
bigben while the workqueue is running.

So having a simple spinlocks ensures the atomicity between checking
for bigben->removed and scheduling a workqueue.

Cheers,
Benjamin