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

[Ira Weiny <ira.weiny@intel.com>]

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@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

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
>