On Tue, 16 Mar 2021 16:29:52 +0000 Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> 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
