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
