On Mon, Jun 20, 2022 at 01:39:45AM -0700, Zhuo, Qiuxu wrote: > > From: ira.weiny@xxxxxxxxx <ira.weiny@xxxxxxxxx> > > Sent: Sunday, June 5, 2022 8:51 AM > > ... > > +static void retire_cur_task(struct pci_doe_mb *doe_mb) { > > + spin_lock(&doe_mb->task_lock); > > + doe_mb->cur_task = NULL; > > + spin_unlock(&doe_mb->task_lock); > > + wake_up_interruptible(&doe_mb->wq); > > +} > > + > > +static void doe_statemachine_work(struct work_struct *work) { > > + struct delayed_work *w = to_delayed_work(work); > > + struct pci_doe_mb *doe_mb = container_of(w, struct pci_doe_mb, > > + statemachine); > > + struct pci_dev *pdev = doe_mb->pdev; > > + int offset = doe_mb->cap_offset; > > + enum pci_doe_state prev_state; > > + struct pci_doe_task *task; > > + u32 val; > > + int rc; > > + > > + spin_lock(&doe_mb->task_lock); > > + task = doe_mb->cur_task; > > + spin_unlock(&doe_mb->task_lock); > > I don't think it needs the lock protection here. > No matter "task" is !NULL or NULL, it is checked before it's used within this function. No it does not. However, Dan has suggested reworking the workqueue and I think it will eliminate this. I kept the lock more as a marker of where cur_task was being used even though it was not required. The fact that the rest of the function goes on to use a local alias was suspicious but was covered by the workqueue operation. I tried to explain that in the commit message but reworking as Dan has suggested is better overall. Thanks for the review! Ira