Re: [PATCH v3 4/8] PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The kernel-doc for pci_enable_link_state_locked() incorrectly advises callers to use an incompatible API, which can lead to a recursive read lock deadlock.
- [Medium] The commit message claims to fix the kernel-doc for `pci_disable_link_state()`, but no such change is included in the patch.
--

--- Patch [4]: [PATCH v3 4/8] PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs ---
commit 544fcb510ac42577bcfc280750b6446d3bc52764
Author: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

PCI/ASPM: Improve the kernel-doc for pci_{enable,disable}_link_state*() APIs

This commit updates the kernel-doc comments for the PCI ASPM link state
management functions. It standardizes the format for the enable APIs and
adds missing documentation for the locked disable API.

> Add kernel-doc for pci_disable_link_state_locked() API and fix the
> kernel-doc for pci_disable_link_state() API.

[Severity: Medium]
Is the commit message accurate about fixing the kernel-doc for
pci_disable_link_state()?

Looking at the diff, it does not appear that pci_disable_link_state() was
modified in this patch, and its return format seems to remain as "Return: 0
or a negative errno" instead of the standard format applied to the other
functions.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 6d6862fd2ebb7..5e7e199c78d26 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c

[ ... ]

> @@ -1574,19 +1588,22 @@ int pci_enable_link_state(struct pci_dev *pdev, int state)
>  EXPORT_SYMBOL(pci_enable_link_state);
>  
>  /**
> - * pci_enable_link_state_locked - Clear and set the default device link state
> - * so that the link may be allowed to enter the specified states. Note that if
> - * the BIOS didn't grant ASPM control to the OS, this does nothing because we
> - * can't touch the LNKCTL register. Also note that this does not enable states
> - * disabled by pci_disable_link_state(). Return 0 or a negative errno.
> + * pci_enable_link_state_locked - Enable device's link state
> + * @pdev: PCI device
> + * @state: Mask of ASPM link states to enable
> + *
> + * Enable device's link state, so the link will enter the specified states.
> + * Note that if the BIOS didn't grant ASPM control to the OS, this does
> + * nothing because we can't touch the LNKCTL register. Also note that this
> + * does not enable states disabled by pci_disable_link_state(). Use
> + * pci_force_enable_link_state() for that.

[Severity: High]
Does suggesting pci_force_enable_link_state() here introduce a deadlock risk?

The context for this function requires the caller to hold the pci_bus_sem
read lock. However, pci_force_enable_link_state() is an unlocked API that
internally calls down_read(&pci_bus_sem).

If a developer follows this documentation and replaces
pci_enable_link_state_locked() with pci_force_enable_link_state() while
holding pci_bus_sem, could this cause a recursive read lock deadlock if a
writer is queued in between the two acquisitions?

>  *
>  * Note: The device will be transitioned to D0 state if the PCI-PM L1 Substates
>  * are getting enabled.
>  *
> - * @pdev: PCI device
> - * @state: Mask of ASPM link states to enable
> - *
>  * Context: Caller holds pci_bus_sem read lock.
> + *
> + * Return: 0 on success, a negative errno otherwise.
>  */
>  int pci_enable_link_state_locked(struct pci_dev *pdev, int state)
>  {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-pci-aspm-fix-v3-0-6bd72451746e@kernel.org?part=4