Re: [RESEND PATCH v6] PCI: PCI: Add pcie_link_is_active() to determine if the PCIe link is active

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 9387 bytes --]

On Mon, 12 May 2025, Ilpo Järvinen wrote:

> On Mon, 12 May 2025, Krishna Chaitanya Chundru wrote:
> 
> > 
> > 
> > On 5/12/2025 5:20 PM, Ilpo Järvinen wrote:
> > > On Fri, 9 May 2025, Shawn Anastasio wrote:
> > > 
> > > In shortlog:
> > > 
> > > PCI: PCI: ... -> PCI: ...
> > > 
> > > > From: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > > > 
> > > > Date: Sat, 12 Apr 2025 07:19:56 +0530
> > > > 
> > > > Introduce a common API to check if the PCIe link is active, replacing
> > > > duplicate code in multiple locations.
> > > > 
> > > > Signed-off-by: Krishna Chaitanya Chundru
> > > > <krishna.chundru@oss.qualcomm.com>
> > > > Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
> > > > ---
> > > > (Resent since git-send-email failed to detect the Subject field from the
> > > > patch
> > > > file previously -- apologies!)
> > > > 
> > > > This is an updated patch pulled from Krishna's v5 series:
> > > > https://patchwork.kernel.org/project/linux-pci/list/?series=952665
> > > > 
> > > > The following changes to Krishna's v5 were made by me:
> > > >    - Revert pcie_link_is_active return type back to int per Lukas' review
> > > >      comments
> > > >    - Handle non-zero error returns at call site of the new function in
> > > >      pci.c/pci_bridge_wait_for_secondary_bus
> > > > 
> > > >   drivers/pci/hotplug/pciehp.h      |  1 -
> > > >   drivers/pci/hotplug/pciehp_ctrl.c |  2 +-
> > > >   drivers/pci/hotplug/pciehp_hpc.c  | 33 +++----------------------------
> > > >   drivers/pci/pci.c                 | 26 +++++++++++++++++++++---
> > > >   include/linux/pci.h               |  4 ++++
> > > >   5 files changed, 31 insertions(+), 35 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
> > > > index 273dd8c66f4e..acef728530e3 100644
> > > > --- a/drivers/pci/hotplug/pciehp.h
> > > > +++ b/drivers/pci/hotplug/pciehp.h
> > > > @@ -186,7 +186,6 @@ int pciehp_query_power_fault(struct controller *ctrl);
> > > >   int pciehp_card_present(struct controller *ctrl);
> > > >   int pciehp_card_present_or_link_active(struct controller *ctrl);
> > > >   int pciehp_check_link_status(struct controller *ctrl);
> > > > -int pciehp_check_link_active(struct controller *ctrl);
> > > >   void pciehp_release_ctrl(struct controller *ctrl);
> > > > 
> > > >   int pciehp_sysfs_enable_slot(struct hotplug_slot *hotplug_slot);
> > > > diff --git a/drivers/pci/hotplug/pciehp_ctrl.c
> > > > b/drivers/pci/hotplug/pciehp_ctrl.c
> > > > index d603a7aa7483..4bb58ba1c766 100644
> > > > --- a/drivers/pci/hotplug/pciehp_ctrl.c
> > > > +++ b/drivers/pci/hotplug/pciehp_ctrl.c
> > > > @@ -260,7 +260,7 @@ void pciehp_handle_presence_or_link_change(struct
> > > > controller *ctrl, u32 events)
> > > >   	/* Turn the slot on if it's occupied or link is up */
> > > >   	mutex_lock(&ctrl->state_lock);
> > > >   	present = pciehp_card_present(ctrl);
> > > > -	link_active = pciehp_check_link_active(ctrl);
> > > > +	link_active = pcie_link_is_active(ctrl->pcie->port);
> > > >   	if (present <= 0 && link_active <= 0) {
> > > >   		if (ctrl->state == BLINKINGON_STATE) {
> > > >   			ctrl->state = OFF_STATE;
> > > > diff --git a/drivers/pci/hotplug/pciehp_hpc.c
> > > > b/drivers/pci/hotplug/pciehp_hpc.c
> > > > index 8a09fb6083e2..278bc21d531d 100644
> > > > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > > > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > > > @@ -221,33 +221,6 @@ static void pcie_write_cmd_nowait(struct controller
> > > > *ctrl, u16 cmd, u16 mask)
> > > >   	pcie_do_write_cmd(ctrl, cmd, mask, false);
> > > >   }
> > > > 
> > > > -/**
> > > > - * pciehp_check_link_active() - Is the link active
> > > > - * @ctrl: PCIe hotplug controller
> > > > - *
> > > > - * Check whether the downstream link is currently active. Note it is
> > > > - * possible that the card is removed immediately after this so the
> > > > - * caller may need to take it into account.
> > > > - *
> > > > - * If the hotplug controller itself is not available anymore returns
> > > > - * %-ENODEV.
> > > > - */
> > > > -int pciehp_check_link_active(struct controller *ctrl)
> > > > -{
> > > > -	struct pci_dev *pdev = ctrl_dev(ctrl);
> > > > -	u16 lnk_status;
> > > > -	int ret;
> > > > -
> > > > -	ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> > > > -	if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
> > > > -		return -ENODEV;
> > > > -
> > > > -	ret = !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> > > > -	ctrl_dbg(ctrl, "%s: lnk_status = %x\n", __func__, lnk_status);
> > > > -
> > > > -	return ret;
> > > > -}
> > > > -
> > > >   static bool pci_bus_check_dev(struct pci_bus *bus, int devfn)
> > > >   {
> > > >   	u32 l;
> > > > @@ -467,7 +440,7 @@ int pciehp_card_present_or_link_active(struct
> > > > controller *ctrl)
> > > >   	if (ret)
> > > >   		return ret;
> > > > 
> > > > -	return pciehp_check_link_active(ctrl);
> > > > +	return pcie_link_is_active(ctrl_dev(ctrl));
> > > >   }
> > > > 
> > > >   int pciehp_query_power_fault(struct controller *ctrl)
> > > > @@ -584,7 +557,7 @@ static void pciehp_ignore_dpc_link_change(struct
> > > > controller *ctrl,
> > > >   	 * Synthesize it to ensure that it is acted on.
> > > >   	 */
> > > >   	down_read_nested(&ctrl->reset_lock, ctrl->depth);
> > > > -	if (!pciehp_check_link_active(ctrl))
> > > > +	if (!pcie_link_is_active(ctrl_dev(ctrl)))
> > > >   		pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
> > > >   	up_read(&ctrl->reset_lock);
> > > >   }
> > > > @@ -884,7 +857,7 @@ int pciehp_slot_reset(struct pcie_device *dev)
> > > >   	pcie_capability_write_word(dev->port, PCI_EXP_SLTSTA,
> > > >   				   PCI_EXP_SLTSTA_DLLSC);
> > > > 
> > > > -	if (!pciehp_check_link_active(ctrl))
> > > > +	if (!pcie_link_is_active(ctrl_dev(ctrl)))
> > > >   		pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
> > > > 
> > > >   	return 0;
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index e77d5b53c0ce..3bb8354b14bf 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -4926,7 +4926,6 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev
> > > > *dev, char *reset_type)
> > > >   		return 0;
> > > > 
> > > >   	if (pcie_get_speed_cap(dev) <= PCIE_SPEED_5_0GT) {
> > > > -		u16 status;
> > > > 
> > > >   		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
> > > >   		msleep(delay);
> > > > @@ -4942,8 +4941,7 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev
> > > > *dev, char *reset_type)
> > > >   		if (!dev->link_active_reporting)
> > > >   			return -ENOTTY;
> > > > 
> > > > -		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &status);
> > > > -		if (!(status & PCI_EXP_LNKSTA_DLLLA))
> > > > +		if (pcie_link_is_active(dev) <= 0)
> > > >   			return -ENOTTY;
> > > > 
> > > >   		return pci_dev_wait(child, reset_type,
> > > > @@ -6247,6 +6245,28 @@ void pcie_print_link_status(struct pci_dev *dev)
> > > >   }
> > > >   EXPORT_SYMBOL(pcie_print_link_status);
> > > > 
> > > > +/**
> > > > + * pcie_link_is_active() - Checks if the link is active or not
> > > > + * @pdev: PCI device to query
> > > > + *
> > > > + * Check whether the link is active or not.
> > > > + *
> > > > + * Return: link state, or -ENODEV if the config read failes.
> > > 
> > > "link state" is bit vague, it would be better to document the values
> > > returned more precisely.
> > > 
> > > > + */
> > > > +int pcie_link_is_active(struct pci_dev *pdev)
> > > > +{
> > > > +	u16 lnk_status;
> > > > +	int ret;
> > > > +
> > > > +	ret = pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, &lnk_status);
> > > > +	if (ret == PCIBIOS_DEVICE_NOT_FOUND || PCI_POSSIBLE_ERROR(lnk_status))
> > > > +		return -ENODEV;
> > > > +
> > > > +	pci_dbg(pdev, "lnk_status = %x\n", lnk_status);
> > > > +	return !!(lnk_status & PCI_EXP_LNKSTA_DLLLA);
> > > > +}
> > > > +EXPORT_SYMBOL(pcie_link_is_active);
> > > > +
> > > >   /**
> > > >    * pci_select_bars - Make BAR mask from the type of resource
> > > >    * @dev: the PCI device for which BAR mask is made
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 51e2bd6405cd..a79a9919320c 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -1945,6 +1945,7 @@ pci_release_mem_regions(struct pci_dev *pdev)
> > > >   			    pci_select_bars(pdev, IORESOURCE_MEM));
> > > >   }
> > > > 
> > > > +int pcie_link_is_active(struct pci_dev *dev);
> > > 
> > > Is this really needed in include/linux/pci.h, isn't drivers/pci/pci.h
> > > enough (for pwrctrl in the Krishna's series)?
> > > 
> > As this is generic functions, the endpoint drivers can also
> > use this API to check if link is active or not in future.
> 
> Hi again,
> 
> Shouldn't the endpoint drivers use the generic RPM interfaces, not mess
> with the Link state themselves? If Link is found to be up using this 
> function, how does that prove what's the state of the link the next 
> moment? To me this does not look like a very safe interface to be used by 
> the endpoint drivers.

Now I even noticed that little detail got removed from the kerneldoc while 
moving the function. Why?

-- 
 i.