On Wed, Nov 30, 2022 at 03:33:30PM +0000, Jonathan Cameron wrote:
> On Mon, 28 Nov 2022 05:25:52 +0100
> Lukas Wunner <lukas@xxxxxxxxx> wrote:
>
> > The DOE API only allows asynchronous exchanges and forces callers to
> > provide a completion callback. Yet all existing callers only perform
> > synchronous exchanges. Upcoming patches for CMA (Component Measurement
> > and Authentication, PCIe r6.0.1 sec 6.31) likewise require only
> > synchronous DOE exchanges. Asynchronous users are currently not
> > foreseeable.
> >
> > Provide a synchronous pci_doe() API call which builds on the internal
> > asynchronous machinery. Should asynchronous users appear, reintroducing
> > a pci_doe_async() API call will be trivial.
> >
> > Convert all users to the new synchronous API and make the asynchronous
> > pci_doe_submit_task() as well as the pci_doe_task struct private.
> >
> > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx>
>
> Hi Lukas,
>
> Thanks for looking at this. A few trivial comments line.
>
> This covers the existing question around async vs sync
> but doesn't have the potential advantages that Ira's series
> has in terms of ripping out a bunch of complexity.
>
> I'm too tied up in the various implementations to offer a clear
> view on which way was should go on this - I'll end up spending
> all day arguing with myself!
>
> It's a bit of crystal ball gazing for how useful keeping the async stuff
I agree that this is much too 'crystal ball gazing' for me as well. See below
for more.
> around will be. Might be a case of taking your first patch then
> sitting on the current implementation for a cycle or two to see
> if it get users... Or take approach Ira proposed and only put the
> infrastructure back in when we have a user for async.
>
> Jonathan
>
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 52541eac17f1..7d1eb5bef4b5 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
>
> ...
>
> > +/**
> > + * struct pci_doe_task - represents a single query/response
> > + *
> > + * @prot: DOE Protocol
> > + * @request_pl: The request payload
> > + * @request_pl_sz: Size of the request payload (bytes)
> > + * @response_pl: The response payload
> > + * @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
> > + * restrictions concerning the protocol.
> > + *
> > + * 1) The request_pl_sz must be a multiple of double words (4 bytes)
> > + * 2) The response_pl_sz must be >= a single double word (4 bytes)
> > + * 3) rv is returned as bytes but it will be a multiple of double words
> > + *
> > + * NOTE there is no need for the caller to initialize work or doe_mb.
>
> Cut and paste from original, but what's the "caller" of a struct? I'd just
> drop this NOTE as it's better explained below.
>
> > + */
> > +struct pci_doe_task {
> > + struct pci_doe_protocol prot;
> > + u32 *request_pl;
> > + size_t request_pl_sz;
> > + u32 *response_pl;
> > + size_t response_pl_sz;
> > + int rv;
> > + void (*complete)(struct pci_doe_task *task);
> > + void *private;
> > +
> > + /* initialized by pci_doe_submit_task() */
> > + struct work_struct work;
> > + struct pci_doe_mb *doe_mb;
> > +};
> > +
>
> ...
>
> > /**
> > * pci_doe_for_each_off - Iterate each DOE capability
> > * @pdev: struct pci_dev to iterate
> > @@ -72,6 +29,8 @@ 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(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
> Whilst there is clearly a verb hidden in that doe, the fact that the
> whole spec section is called the same is confusing.
>
> pci_doe_query_response() maybe or pci_doe_do() perhaps?
Or just pci_doe_submit()?
Lukas and I discussed this off-line. Because he is going to need this stuff
going forward. I'm going to back off fixing this and let him handle it.
I agree with him that eventually something like a 'flush' operation will be
needed but right now that mechanism is broken. I'll let him determine if it
should be removed or fixed depending on his future needs.
Ira
>
>
> > + void *request, size_t request_sz,
> > + void *response, size_t response_sz);
> >
> > #endif
>
