Re: [PATCH] PCI/DOE: Remove asynchronous task support

[Ira Weiny]

On Sun, Nov 20, 2022 at 10:27:35AM +0800, Hillf Danton wrote:
> On Sat, 19 Nov 2022 14:25:27 -0800 Ira Weiny <ira.weiny@xxxxxxxxx>
> > @@ -529,8 +492,18 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> >  		return -EIO;
> >  
> >  	task->doe_mb = doe_mb;
> > -	INIT_WORK(&task->work, doe_statemachine_work);
> > -	queue_work(doe_mb->work_queue, &task->work);
> > +
> > +again:
> > +	if (!mutex_trylock(&doe_mb->exec_lock)) {
> > +		if (wait_event_timeout(task->doe_mb->wq,
> > +				test_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags),
> > +				PCI_DOE_POLL_INTERVAL))
> > +			return -EIO;
> 
> Is EIO worth a line of pr_warn()?

Maybe but I'm not sure it is worth it.  This was paralleling the original code
which called pci_doe_flush_mb() to shut down the mailbox.  So this is likely to
never happen.  The callers could print something if needed.

> 
> > +		goto again;
> > +	}
> > +	exec_task(task);
> > +	mutex_unlock(&doe_mb->exec_lock);
> > +
> 
> If it is likely to take two minutes to acquire the exec_lock after
> rounds of trying again, trylock + wait timeout barely make sense given EIO.

I'm not sure where 2 minutes come from?

#define PCI_DOE_TIMEOUT HZ
#define PCI_DOE_POLL_INTERVAL   (PCI_DOE_TIMEOUT / 128)

It is also not anticipated that more than 1 task is being given to the mailbox
but the protection needs to be there because exec_task() will get confused if
more than 1 thread submits at the same time.

All this said I've now convinced myself that there is a race in the use of
PCI_DOE_FLAG_CANCEL even with the existing code.

I believe that if the pci device goes away the doe_mb structure may get free'ed
prior to other threads having a chance to check doe_mb->flags.  Worse yet the
work queue itself (doe_mb->wq) may become invalid...

I don't believe this can currently happen because anyone using the doe_mb
structure has a reference to the pci device.

With this patch I think all the doe_mb->flags and the wait queue can go away.
pci_doe_wait() can be replaced with a simple msleep_interruptible().

Let me work through that a bit.

Ira

> 
> Hillf
> 
> /**
>  * wait_event_timeout - sleep until a condition gets true or a timeout elapses
>  * @wq_head: the waitqueue to wait on
>  * @condition: a C expression for the event to wait for
>  * @timeout: timeout, in jiffies
>  *
>  * The process is put to sleep (TASK_UNINTERRUPTIBLE) until the
>  * @condition evaluates to true. The @condition is checked each time
>  * the waitqueue @wq_head is woken up.
>  *
>  * wake_up() has to be called after changing any variable that could
>  * change the result of the wait condition.
>  *
>  * Returns:
>  * 0 if the @condition evaluated to %false after the @timeout elapsed,
>  * 1 if the @condition evaluated to %true after the @timeout elapsed,
>  * or the remaining jiffies (at least 1) if the @condition evaluated
>  * to %true before the @timeout elapsed.
>  */