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.
:-(
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
