Re: [PATCH] pci/pcie: Avoid unnecessary PCIe link retrains

[Bjorn Helgaas]

On Wednesday 04 November 2009 12:03:21 pm Jesse Barnes wrote:
> [Cc'ing linux-pci@xxxxxxxxxxxxxxx too]
> 
> On Tue, 3 Nov 2009 16:38:20 -0500
> RDH <rdh@xxxxxxxxxxxx> wrote:
> 
> > 
> > This patch avoids unnecessary PCIe link retraining sequences within
> > the PCIe ASPM common clock setup code by issuing a link retrain
> > command only if we are actually changing the PCIe clock configuration.
> > 
> > Signed-off-by: Robert D. Houk <rdh@xxxxxxxxxxxx>
> > ---
> >  aspm.c |   20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > --- a/drivers/pci/pcie/aspm.c	2009-10-15 20:41:50.000000000
> > -0400 +++ b/drivers/pci/pcie/aspm.c	2009-11-02
> > 14:29:35.000000000 -0500 @@ -183,6 +183,7 @@ static void
> > pcie_aspm_configure_common_c {
> >  	int ppos, cpos, same_clock = 1;
> >  	u16 reg16, parent_reg, child_reg[8];
> > +	u16 lnkctl_ccc_or, lnkctl_ccc_and;
> >  	unsigned long start_jiffies;
> >  	struct pci_dev *child, *parent = link->pdev;
> >  	struct pci_bus *linkbus = parent->subordinate;
> > @@ -205,6 +206,25 @@ static void pcie_aspm_configure_common_c
> >  	if (!(reg16 & PCI_EXP_LNKSTA_SLC))
> >  		same_clock = 0;
> >  
> > +	/* Check upstream component for Common Clock Config */
> > +	pci_read_config_word(parent, ppos + PCI_EXP_LNKCTL, &reg16);
> > +	lnkctl_ccc_and = lnkctl_ccc_or = (reg16 &
> > PCI_EXP_LNKCTL_CCC); +
> > +	/* Scan downstream component for CCC, all functions */
> > +	list_for_each_entry(child, &linkbus->devices, bus_list) {
> > +		cpos = pci_find_capability(child, PCI_CAP_ID_EXP);

Some other places in pcie/aspm (e.g., pcie_set_clkpm_nocheck() and
pcie_clkpm_cap_init()) check cpos for NULL.  Your code looks
superficially similar, so maybe you should check it, too, although
I do see many other place in aspm where we *don't* check it.

We look up this capability so often, I wonder if we should save it
in the struct pci_dev or struct pcie_link or something in such a
way that if we have a struct pcie_link, we are guaranteed that there
is a PCIe capability, and we don't have to search for it again.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/