Re: [PATCH v5 7/9] PCI: PCI: Add pcie_link_is_active() to determine if the PCIe link is active

[Krishna Chaitanya Chundru]

On 4/12/2025 9:22 AM, Lukas Wunner wrote:
> On Sat, Apr 12, 2025 at 07:19:56AM +0530, Krishna Chaitanya Chundru wrote:
> > 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@xxxxxxxxxxxxxxxx>
>
> Reviewed-by: Lukas Wunner <lukas@xxxxxxxxx>
>
> One heads-up and one nit:
>
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -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);
> >   }
>
> Heads-up:  There's a trivial merge conflict here with what's queued on
> pci.git/hotplug.  No need for you to respin because I expect this will be
> merged through a different topic branch anyway, but Bjorn and the other
> maintainers will see the merge conflict when generating the next branch.
>
>
> > --- 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));
> >   }
> >   
> > +bool 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 */
>
> Nit:  Seems like this would still fit within 80 chars:
>
> static inline bool pcie_link_is_active(struct pci_dev *dev) { return false; }
>
> That said, all existing callers of this function as well as the new one
> introduced by this series are only compiled in the CONFIG_PCI=y case,
> so I'm not sure the inline stub is necessary at all?
If any other driver other than these two drivers in future wants to use
this API, it can be useful in that case.

- Krishna Chaitanya.
> Thanks,
>
> Lukas