Re: [PATCH 2/2] PCI/DOE: Provide synchronous API

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 28 Nov 2022 05:25:52 +0100
Lukas Wunner <lukas@wunner.de> 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@wunner.de>

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