Re: [RFC PATCH 3/4] PCI/DOE: Add DOE mailbox support for endpoint functions

[Aksh Garg]

On 04/03/26 19:47, Manivannan Sadhasivam wrote:
> On Fri, Feb 13, 2026 at 06:06:02PM +0530, Aksh Garg wrote:
> > From: Aksh Garg <a-garg7@xxxxxx>
> >
> > +/**
> > + * pci_ep_doe_add_mailbox() - Add a DOE mailbox for a physical function
> > + * @epc: PCI endpoint controller
> > + * @func_no: Physical function number
> > + * @cap_offset: Offset of the DOE capability
> > + *
> > + * Create and register a DOE mailbox for the specified physical function
> > + * and capability offset. The controller driver should call this for each
> > + * DOE capability it finds in its config space.
>
> Why can't we call this helper from the EPC core driver instead? We may have to
> introduce a new callback in 'struct pci_epc_ops' to read the endpoint config
> space from the EPC core driver. But it will avoid duplicating the code across
> the EPC drivers.
>
> End of the day, we can read the config space of the device and identify the
> capability in a generic way.

Thank you for the feedback. This looks better to me. Please let me know
if the following flow would be correct:

'struct pci_epc_ops' would have a new callback to find the extended
capability, taking inputs as struct pci_epc *epc, u8 func_no, u8 cap.
The EPC core driver would have an API, taking struct pci_epc* as
argument. This would find the extended DOE capability offsets for the
mailboxes for all the functions in epc->max_functions, and call
pci_ep_doe_add_mailbox() for each of them.

This newly created API would be called by the controller driver if it is
DOE capable. The DOE capability feature can be added in the 'struct
pci_epc_features' as well based on which this API would be called.

> > + *
> > + * RETURNS: 0 on success, -errno on failure
> > + */
> > +int pci_ep_doe_add_mailbox(struct pci_epc *epc, u8 func_no, u16 cap_offset)
> > +{
> > +	struct pci_ep_doe_mb *doe_mb;
> > +	unsigned long key;
> > +	int ret;
> > +
> > +	if (!epc)
> > +		return -EINVAL;
> > +
> > +	doe_mb = kzalloc(sizeof(*doe_mb), GFP_KERNEL);
> > +	if (!doe_mb)
> > +		return -ENOMEM;
> > +
> > +	doe_mb->epc = epc;
> > +	doe_mb->func_no = func_no;
> > +	doe_mb->cap_offset = cap_offset;
> > +
> > +	doe_mb->work_queue = alloc_ordered_workqueue("pci_ep_doe[%s:pf%d:offset%x]", 0,
> > +						     dev_name(&epc->dev),
> > +						     func_no, cap_offset);
>
> I believe you are trying to use cap_offset to differentiate between multiple DOE
> mailbox instances within a single function. In that case, I'd use a simple index
> instead of cap_offset.

Yes, the cap_offset is used to differentiate between the mailboxes.

There were two reasons I chose this as a differentiator instead of a
simple index:
- doe.c uses cap_offset as differentiator, hence I used the same to make
  doe-ep.c code compatible with it
- Because the cap_offset would be unique across the mailboxes for a
  single function, we do not have to create and manage an index for the
  uniqueness of the mailbox, which doesn't add the redundancy of
  introducing an index variable just as a differentiator.


Regards,
Aksh Garg

> Rest looks OK from a high level design perspective.
>
> - Mani