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

[Lukas Wunner <lukas@wunner.de>]

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:
> > + * 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.

Okay, will drop this note when respinning, it just duplicates the
code comment inside the pci_doe_task declaration.


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

Going forward the PCI core will allocate DOE mailboxes on device
enumeration.  And the API will then consist of just two calls:

One call to get a pointer to a pci_doe_mb for a (pci_dev, vendor, protocol)
triple which I intend to call pci_find_doe_mailbox().

And one call to perform a synchronous query/response over a pci_doe_mb.
That's the pci_doe() I'm introducing here.

I initially went for pci_doe_exchange(), then realized that the "e" in
"doe" already stands for "exchange", so that would be redundant.

We generally try to use the same terms as the PCIe Base Spec to make it
easy for an uninitiated reader to connect the spec to the implementation.
The spec talks about "performing data object exchanges", but
pci_doe_perform() didn't seem very appealing to me.

I ended up just using pci_doe() for simplicity and because it makes it
easy to stay within 80 chars if the function name is short.

Passing in the request and response pointers directly instead of
populating a struct is likewise an attempt to make things as simple
as possible for driver authors.  It's modeled after the SPI and I2C
APIs which offer similar primitives to perform read/write exchanges
with devices.

(Side note: pci_find_doe_mailbox() will return the *first* mailbox found
for a given (pci_dev, vendor, protocol) triple.  Conceivably, there may
be devices with multiple mailboxes supporting the same protocols.  We may
have to add a pci_find_next_doe_mailbox(), should such devices appear.
Those names match what we already have for capabilities, i.e.
pci_find_ext_capability() and pci_find_next_ext_capability().)

Thanks,

Lukas