Re: [PATCH v6 2/4] PCI: endpoint: Add DOE mailbox support for endpoint functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `pci_ep_doe_process_request()` fallback stub leaks the request buffer when `CONFIG_PCI_ENDPOINT_DOE=n`.
- [High] Flawed DOE Abort mechanism causes a state leak of the `CANCEL` flag, silently dropping valid requests.
--

commit b18fff73f747154acf71d1ff81a4731b89d384ad
Author: Aksh Garg <a-garg7@ti.com>

PCI: endpoint: Add DOE mailbox support for endpoint functions

This patch adds Data Object Exchange (DOE) support for PCIe endpoint
devices. It implements framework APIs for the EPC core driver and controller
drivers to register mailboxes and process DOE requests from the host.

> diff --git a/drivers/pci/endpoint/pci-ep-doe.c b/drivers/pci/endpoint/pci-ep-doe.c
> index 0000000000000..b2832253eaca6
> --- /dev/null
> +++ b/drivers/pci/endpoint/pci-ep-doe.c

[ ... ]

> +static void signal_task_complete(struct pci_ep_doe_task *task, int status)
> +{
> +	struct pci_ep_doe_mb *doe_mb = task->doe_mb;
> +
> +	task->complete(doe_mb->epc, doe_mb->func_no, doe_mb->cap_offset,
> +		       status, task->feat.vid, task->feat.type,
> +		       task->response_pl, task->response_pl_sz);
> +
> +	/* Clear the CANCEL flag for next DOE request */
> +	clear_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);

[ ... ]

> +int pci_ep_doe_abort(struct pci_epc *epc, u8 func_no, u16 cap_offset)
> +{
> +	struct pci_ep_doe_mb *doe_mb;
> +
> +	doe_mb = pci_ep_doe_get_mailbox(epc, func_no, cap_offset);
> +	if (!doe_mb)
> +		return -ENODEV;
> +
> +	/* Set CANCEL flag - worker will abort queued requests */
> +	set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);

[Severity: High]
Can a race condition here cause the CANCEL flag to be permanently set,
silently dropping the next valid request?

If the host sends a DOE Abort just as a task completes, the controller
driver could evaluate the busy state and call pci_ep_doe_abort().

Concurrently, the worker thread might finish processing, invoke
signal_task_complete(), and clear the flag:

worker thread in signal_task_complete():
    clear_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);

If pci_ep_doe_abort() then executes on the controller thread:

controller thread in pci_ep_doe_abort():
    set_bit(PCI_DOE_FLAG_CANCEL, &doe_mb->flags);

The mailbox becomes idle but still has the CANCEL flag set. Would the
next valid request then be immediately cancelled when doe_ep_task_work()
checks the flag?

> +
> +	dev_dbg_ratelimited(epc->dev.parent,
> +			    "DOE mailbox abort initialized: PF%d offset 0x%x\n",
> +			    func_no, cap_offset);
> +
> +	pci_ep_doe_put_mailbox(doe_mb);
> +	return 0;
> +}

[ ... ]

> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 5844deee2b5f1..6d3b4b779d157 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h

[ ... ]

> +#else
> +static inline void pci_ep_doe_init(struct pci_epc *epc) { }
> +static inline int pci_ep_doe_add_mailbox(struct pci_epc *epc, u8 func_no,
> +					 u16 cap_offset)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int pci_ep_doe_process_request(struct pci_epc *epc,
> +					     u8 func_no, u16 cap_offset,
> +					     u16 vendor, u8 type,
> +					     void *request, size_t request_sz,
> +					     pci_ep_doe_complete_t complete)
> +{
> +	return -EOPNOTSUPP;

[Severity: High]
Does this fallback stub leak the request buffer when endpoint DOE support
is disabled?

The API documentation in pci-ep-doe.c states that ownership of the request
buffer is transferred to the DOE EP core, which frees it. Since this stub
just returns -EOPNOTSUPP, it looks like the buffer is never freed when
CONFIG_PCI_ENDPOINT_DOE is not enabled.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623090737.711656-1-a-garg7@ti.com?part=2