On Tue, Nov 22, 2022 at 09:46:27AM +0000, Jonathan Cameron wrote:
> On Mon, 21 Nov 2022 14:59:46 -0800
> Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
>
> > On Mon, Nov 21, 2022 at 10:07:56AM +0800, Li, Ming wrote:
> > > On 11/21/2022 9:39 AM, Li, Ming wrote:
> >
> > [snip]
> >
> > > >> @@ -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;
> > > >
> > > > We already implemented a pci_doe_wait(), I think we can use it to instead of this wait_event_timeout.
> > > >
> > > > Thanks
> > > > Ming
> > > >
> > >
> > > This wait_event_timeout() only check PCI_DOE_FLAG_CANCEL, that means it only detects the signal which the doe_mb has being destroyed.
> > > If current doe task is done correctly, I think we should wake up next task. Current implementation just waits utill timeout happens and try it again.
> > > Besides, If two threads are waiting a same doe_mb, thread #1 waited firstly, thread #2 waited secondly, there is a chance that thread #2 is processed before thread #1.
> > >
> >
> > Agreed.
> >
> > However, the real problem is that the doe_mb is probably free'ed at this point
> > and all this is going to crash and burn anyway. The implementation of
> > PCI_DOE_FLAG_CANCEL was fundamentally flawed even for the current work queue
> > implementation.
> >
> > This patch incorrectly tried to use that mechanism but upon looking closer I
> > see it does not work.
> >
> > I saw in another thread Jonathan discussing some sort of get/put on the doe_mb.
> > That is not currently necessary as the creators of doe_mb objects currently
> > hold references to the PCI device any time they call submit.
>
> The get / put would only matter if we wanted to manage the DOE resources separately
> from those of the PCI device. It may well never make sense to do so as they
> aren't substantial anyway.
Agreed. See the new series:
https://lore.kernel.org/all/20221122155324.1878416-1-ira.weiny@xxxxxxxxx/
Thanks,
Ira
> >
> > :-(
> >
> > For now all PCI_DOE_FLAG_CANCEL stuff needs to go away,
> > Ira
> >
> > > Thanks
> > > Ming
> > >
> > > >> + goto again;
> > > >> + }
> > > >> + exec_task(task);
> > > >> + mutex_unlock(&doe_mb->exec_lock);
> > > >> +
> > > >> return 0;
> > > >> }
> > > >> -EXPORT_SYMBOL_GPL(pci_doe_submit_task);
> > > >> +EXPORT_SYMBOL_GPL(pci_doe_submit_task_wait);
> > > >> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> > > >> index ed9b4df792b8..c94122a66221 100644
> > > >> --- a/include/linux/pci-doe.h
> > > >> +++ b/include/linux/pci-doe.h
> > > >> @@ -30,8 +30,6 @@ struct pci_doe_mb;
> > > >> * @response_pl_sz: Size of the response payload (bytes)
> > > >> * @rv: Return value. Length of received response or error (bytes)
> > > >> * @complete: Called when task is complete
> > > >> - * @private: Private data for the consumer
> > > >> - * @work: Used internally by the mailbox
> > > >> * @doe_mb: Used internally by the mailbox
> > > >> *
> > > >> * The payload sizes and rv are specified in bytes with the following
> > > >> @@ -50,11 +48,6 @@ struct pci_doe_task {
> > > >> u32 *response_pl;
> > > >> size_t response_pl_sz;
> > > >> int rv;
> > > >> - void (*complete)(struct pci_doe_task *task);
> > > >> - void *private;
> > > >> -
> > > >> - /* No need for the user to initialize these fields */
> > > >> - struct work_struct work;
> > > >> struct pci_doe_mb *doe_mb;
> > > >> };
> > > >>
> > > >> @@ -72,6 +65,5 @@ struct pci_doe_task {
> > > >>
> > > >> struct pci_doe_mb *pcim_doe_create_mb(struct pci_dev *pdev, u16 cap_offset);
> > > >> bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type);
> > > >> -int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> > > >> -
> > > >> +int pci_doe_submit_task_wait(struct pci_doe_mb *doe_mb, struct pci_doe_task *task);
> > > >> #endif
> > > >>
> > > >> base-commit: b6e7fdfd6f6a8bf88fcdb4a45da52c42ba238c25
>
