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

[Benjamin Tissoires]

On Mon, Feb 13, 2023 at 11:37 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> On Mon, 13 Feb 2023 09:25:37 +0100 Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> >
> > 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.
>
> > - 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