Re: [RFC PATCH 1/2] PCI/doe: Initial support PCI Data Object Exchange

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

On Tue, 16 Mar 2021 16:29:52 +0000
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:


> >   
> > > +               [0] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, 0001) |
> > > +               FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, 0),
> > > +               [1] = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, 3),
> > > +               [2] = FIELD_PREP(PCI_DOE_DATA_OBJECT_DISC_REQ_3_INDEX, *index)
> > > +       };
> > > +       u32 response[3];
> > > +       int ret;
> > > +
> > > +       ret = pcie_doe_exchange(doe, request, sizeof(request), response, sizeof(response));
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       *vid = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_VID, response[2]);
> > > +       *protocol = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL, response[2]);
> > > +       *index = FIELD_GET(PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX, response[2]);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +/**
> > > + * pcie_doe_protocol_check() - check if this DOE mailbox supports specific protocol
> > > + * @doe: DOE state structure
> > > + * @vid: Vendor ID
> > > + * @protocol: Protocol number as defined by Vendor
> > > + * Returns: 0 on success, <0 on error
> > > + */
> > > +int pcie_doe_protocol_check(struct pcie_doe *doe, u16 vid, u8 protocol)    
> > 
> > Not clear to me that this is a comfortable API for a driver. I would
> > expect that at registration time all the supported protocols would be
> > retrieved and cached in the 'struct pcie_doe' context and then the
> > host driver could query from there without going back to the device
> > again.  
> 
> I'm not sure I follow.
> 
> Any driver will fall into one of the following categories:
> a) Already knows what protocols are available on a
>    given DOE instance perhaps because that's a characteristic of the hardware
>    supported, in which case it has no reason to check (unless driver writer
>    is paranoid)
> b) It has no way to know (e.g. class driver), then it makes sense to query
>    the DOE instance to find out what protocols are available.
> 
> Absolutely we could cache them, but it wouldn't change the interface
> presented to the driver. I think doing so at this stage is
> premature optimization.
> 
> We could present this at a different level and wrap it up as a
> find_doe that will return a DOE instance that supports the desired
> protocol, but then that puts the burden of reference counting etc
> for the different DOE instances on the core - the one thing I think
> we want to avoid.
> 
> So far we have no evidence any device will actually need that.
> 
> Of the existing protocols, only a few are allowed to coexist with
> each other and in well defined sets (CMA and IDE for example).
> 
> An alternative model we could look at (which is much more complex)
> is to have something like the following: 
> 
> struct pcie_doe_set - Central location which is responsible for
> all DOE mailboxes on a PCI device.
> 
> At init that scans all DOE mailboxes and builds a look up table
> from [vid, protocol] to [struct pcie_doe]
> Note this is 1 to 1, so if a protocol is supported on multiple
> mailboxes we use the first one.
> 
> pcie_doe_exchange(struct pcie_doe_set, u16 vid, u8 protocol...)
> Looks up the relevant DOE instance and does exchange on that.
> 
> So far I'm not convinced we should engage in this complexity.
> Nothing stops us adding it if and when it becomes apparent we
> actually need it.
> 
> An intermediate point would be to add basic list and reference
> counting infrastructure so that a driver can call
> 
> struct pcie_doe *pcie_doe_get(struct pci_dev, u16 vid, u8 protocol)
> void pci_doe_put(struct pci_doe *doe);
> 
> That means at least a list_head and possibly a lock being added
> to pci_dev. Not sure how Bjorn will feel about that.
> 
> I might see how bad this looks for v2.

Lifetime element of the DOE could be avoided by simply having

pcie_doe_register_all()
and
pcie_doe_unregister_all()

and so managing all DOE instances in one unit.

I'm not sure I like it, but certainly makes things simple.
After pcie_doe_register_all() call, all DOEs are ready to use
and we can have simple pcie_doe_find() to get one with
appropriate protocols.  There is never any need to specifically
release it because they are all cleaned up together in remove
/release path.

I'll put this together for a v2 and we can see how it shapes
up.

Jonathan