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
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?
> + void *request, size_t request_sz,
> + void *response, size_t response_sz);
>
> #endif
