diff mbox series

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

Message ID 20250509180813.2200312-1-sanastasio@raptorengineering.com
State New
Headers show
Series [RESEND,v6] PCI: PCI: Add pcie_link_is_active() to determine if the PCIe link is active | expand

Commit Message

Shawn Anastasio May 9, 2025, 6:08 p.m. UTC 
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(-)

--
2.30.2

Comments

Ilpo Järvinen May 12, 2025, 11:50 a.m. UTC | #1 
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)?

>  #else /* CONFIG_PCI is not enabled */
> 
>  static inline void pci_set_flags(int flags) { }
> @@ -2093,6 +2094,9 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>  {
>  	return -ENOSPC;
>  }
> +
> +static inline bool pcie_link_is_active(struct pci_dev *dev)
> +{ return false; }

This fits on one line just fine.
Krishna Chaitanya Chundru May 12, 2025, 4:12 p.m. UTC | #2 
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.

- Krishna Chaitanya.

>>   #else /* CONFIG_PCI is not enabled */
>>
>>   static inline void pci_set_flags(int flags) { }
>> @@ -2093,6 +2094,9 @@ pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
>>   {
>>   	return -ENOSPC;
>>   }
>> +
>> +static inline bool pcie_link_is_active(struct pci_dev *dev)
>> +{ return false; }
> 
> This fits on one line just fine.
>
Ilpo Järvinen May 12, 2025, 5:13 p.m. UTC | #3 
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.
Ilpo Järvinen May 12, 2025, 5:24 p.m. UTC | #4 
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?
Krishna Chaitanya Chundru May 13, 2025, 12:51 a.m. UTC | #5 
On 5/12/2025 10:54 PM, Ilpo Järvinen wrote:
> 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.
>
I looked to net drivers, I see there are places which use this [1].

> Now I even noticed that little detail got removed from the kerneldoc while
> moving the function. Why?
Shawn, can you add the removed details back.

[1]: 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wwan/iosm/iosm_ipc_pcie.c?h=v6.15-rc6#n178

- Krishna Chaitanya.
>
diff mbox series

Patch

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.
+ */
+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);
 #else /* CONFIG_PCI is not enabled */

 static inline void pci_set_flags(int flags) { }
@@ -2093,6 +2094,9 @@  pci_alloc_irq_vectors(struct pci_dev *dev, unsigned int min_vecs,
 {
 	return -ENOSPC;
 }
+
+static inline bool pcie_link_is_active(struct pci_dev *dev)
+{ return false; }
 #endif /* CONFIG_PCI */

 /* Include architecture-dependent settings and functions */