Patchworkβ pci/pcie: Avoid unnecessary PCIe link retrains

login
register
about
Submitter Bjorn Helgaas
Date 2009-11-05 18:59:25
Message ID <200911051159.26046.bjorn.helgaas@hp.com>
Download mbox | patch
Permalink /patch/57925/
State Rejected
Headers show

Comments

Bjorn Helgaas - 2009-11-05 18:59:25
On Wednesday 04 November 2009 08:05:11 pm Kenji Kaneshige wrote:
> Bjorn Helgaas wrote:
> > On Wednesday 04 November 2009 12:03:21 pm Jesse Barnes wrote:
> >> [Cc'ing linux-pci@vger.kernel.org too]
> >>
> >> On Tue, 3 Nov 2009 16:38:20 -0500
> >> RDH <rdh@east.sun.com> wrote:

> >>> +	/* 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.
> > 
> 
> I agree. There are a lot of codes using pci_find_capability() to
> search PCIe capability offset in addition to ASPM driver. So I
> think it's good idea to have PCIe cap offset in struct pci_dev.
> 
> To be honest, I have already made a following patch to add a new
> field into struct pci_dev and some other patches to use this new
> field a few months ago. But I have never posted them yet. If it
> is a right direction, I would like to update and post them soon.
> 
> Thanks,
> Kenji Kaneshige
> 
> 
> There are a lot of codes that searches PCI express capability offset
> in the PCI configuration space using pci_find_capability(). Caching it
> in the struct pci_dev will reduce unncecessary search. This patch adds
> an additional 'pcie_cap' fields into struct pci_dev, which is
> initialized at pci device scan time (in set_pcie_port_type()).
> 
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> 
> ---
>  drivers/pci/probe.c |    1 +
>  include/linux/pci.h |    1 +
>  2 files changed, 2 insertions(+)
> 
> Index: 20090825/drivers/pci/probe.c
> ===================================================================
> --- 20090825.orig/drivers/pci/probe.c
> +++ 20090825/drivers/pci/probe.c
> @@ -693,6 +693,7 @@ static void set_pcie_port_type(struct pc
>  	if (!pos)
>  		return;
>  	pdev->is_pcie = 1;
> +	pdev->pcie_cap = pos;
>  	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
>  	pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
>  }
> Index: 20090825/include/linux/pci.h
> ===================================================================
> --- 20090825.orig/include/linux/pci.h
> +++ 20090825/include/linux/pci.h
> @@ -218,6 +218,7 @@ struct pci_dev {
>  	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
>  	u8		revision;	/* PCI revision, low byte of class word */
>  	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
> +	u8		pcie_cap;	/* PCI-E capability offset */
>  	u8		pcie_type;	/* PCI-E device/port type */
>  	u8		rom_base_reg;	/* which config register controls the ROM */
>  	u8		pin;  		/* which interrupt pin this device uses */
> 

Here's another possibility, the idea being to collect all the PCIe
stuff in one place.  This would require a lot of changes in the PCIe
driver code, but most of them would be trivial.



--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew Wilcox - 2009-11-05 19:07:07
On Thu, Nov 05, 2009 at 12:59:25PM -0600, Bjorn Helgaas wrote:
> Here's another possibility, the idea being to collect all the PCIe
> stuff in one place.  This would require a lot of changes in the PCIe
> driver code, but most of them would be trivial.

I don't like the idea of kmallocing a 6- or 10-byte data structure
... better to keep it in the pci_dev.  Maybe embedding a pcie_dev inside
the pci_dev would be a good idea, but unless there're more things to
move to it, this seems like a net loss to me.
Bjorn Helgaas - 2009-11-05 20:29:40
On Thursday 05 November 2009 12:07:07 pm Matthew Wilcox wrote:
> On Thu, Nov 05, 2009 at 12:59:25PM -0600, Bjorn Helgaas wrote:
> > Here's another possibility, the idea being to collect all the PCIe
> > stuff in one place.  This would require a lot of changes in the PCIe
> > driver code, but most of them would be trivial.
> 
> I don't like the idea of kmallocing a 6- or 10-byte data structure
> ... better to keep it in the pci_dev.  Maybe embedding a pcie_dev inside
> the pci_dev would be a good idea, but unless there're more things to
> move to it, this seems like a net loss to me.

That's true, it's not worth it for such a small structure.  I figured
there would probably be more PCIe-related stuff that could go there,
e.g., embedding the link_state directly.  But I haven't looked to
see whether there actually is enough PCIe stuff to make it worthwhile.

Bjorn

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kenji Kaneshige - 2009-11-06 01:15:02
Bjorn Helgaas wrote:
> On Thursday 05 November 2009 12:07:07 pm Matthew Wilcox wrote:
>> On Thu, Nov 05, 2009 at 12:59:25PM -0600, Bjorn Helgaas wrote:
>>> Here's another possibility, the idea being to collect all the PCIe
>>> stuff in one place.  This would require a lot of changes in the PCIe
>>> driver code, but most of them would be trivial.
>> I don't like the idea of kmallocing a 6- or 10-byte data structure
>> ... better to keep it in the pci_dev.  Maybe embedding a pcie_dev inside
>> the pci_dev would be a good idea, but unless there're more things to
>> move to it, this seems like a net loss to me.
> 
> That's true, it's not worth it for such a small structure.  I figured
> there would probably be more PCIe-related stuff that could go there,
> e.g., embedding the link_state directly.  But I haven't looked to
> see whether there actually is enough PCIe stuff to make it worthwhile.
> 

In my understanding, there are the following PCIe specific data in
struct pci_dev. It is not many.

	u8 pcie_cap;			*I added this time
	u8 pcie_type;
	struct pcie_link_state *link_state;
	unsigned int ari_enabled:1;
	unsigned int is_pcie:1;		*No longer needed and will be removed.
	unsigned int aer_firmware_first:1;

How about keeping cap offset in pci_dev so far and introducing the
following wrapper function?

static inline pcie_cap(struct pci_dev *pdev)
{
        return pdev->pcie_cap;
}

When we actually need a new data structure for PCIe-related stuff in the
future, all we need to do would be changing this wrapper like below.

static inline pcie_cap(struct pci_dev *pdev)
{
	struct pcie_dev *pcie = pdev->pcie;
	if (pcie)
		return pcie->cap;
	return 0;
}

Thanks,
Kenji Kaneshige

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/pci.h b/include/linux/pci.h
index f5c7cd3..29272ab 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -198,6 +198,14 @@  struct pci_vpd;
 struct pci_sriov;
 struct pci_ats;
 
+struct pcie_dev {
+#ifdef CONFIG_PCIEASPM
+	struct pcie_link_state	*link_state;	/* ASPM link state. */
+#endif
+	u8		cap;	/* PCI-E capability offset */
+	u8		type;	/* PCI-E device/port type */
+};
+
 /*
  * The pci_dev structure is used to describe PCI devices.
  */
@@ -218,7 +226,6 @@  struct pci_dev {
 	unsigned int	class;		/* 3 bytes: (base,sub,prog-if) */
 	u8		revision;	/* PCI revision, low byte of class word */
 	u8		hdr_type;	/* PCI header type (`multi' flag masked out) */
-	u8		pcie_type;	/* PCI-E device/port type */
 	u8		rom_base_reg;	/* which config register controls the ROM */
 	u8		pin;  		/* which interrupt pin this device uses */
 
@@ -243,10 +250,6 @@  struct pci_dev {
 	unsigned int	no_d1d2:1;	/* Only allow D0 and D3 */
 	unsigned int	wakeup_prepared:1;
 
-#ifdef CONFIG_PCIEASPM
-	struct pcie_link_state	*link_state;	/* ASPM link state. */
-#endif
-
 	pci_channel_state_t error_state;	/* current connectivity state */
 	struct	device	dev;		/* Generic device interface */
 
@@ -273,7 +276,6 @@  struct pci_dev {
 	unsigned int	msix_enabled:1;
 	unsigned int	ari_enabled:1;	/* ARI forwarding */
 	unsigned int	is_managed:1;
-	unsigned int	is_pcie:1;
 	unsigned int    needs_freset:1; /* Dev requires fundamental reset */
 	unsigned int	state_saved:1;
 	unsigned int	is_physfn:1;
@@ -293,6 +295,7 @@  struct pci_dev {
 	struct list_head msi_list;
 #endif
 	struct pci_vpd *vpd;
+	struct pci_pcie *pcie;
 #ifdef CONFIG_PCI_IOV
 	union {
 		struct pci_sriov *sriov;	/* SR-IOV capability related */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5c4ce1b..f85f2e7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -684,13 +684,21 @@  static void set_pcie_port_type(struct pci_dev *pdev)
 {
 	int pos;
 	u16 reg16;
+	struct pcie_dev *pcie;
 
 	pos = pci_find_capability(pdev, PCI_CAP_ID_EXP);
 	if (!pos)
 		return;
-	pdev->is_pcie = 1;
+
+	pcie = kzalloc(sizeof(struct pci_dev), GFP_KERNEL);
+	if (!pcie)
+		return;
+
 	pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, &reg16);
-	pdev->pcie_type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
+
+	pcie->type = (reg16 & PCI_EXP_FLAGS_TYPE) >> 4;
+	pcie->cap = pos;
+	pdev->pcie = pcie;
 }
 
 static void set_pcie_hotplug_bridge(struct pci_dev *pdev)