Skip to contentSkip to conversation

PCI: x86 MMCONFIG: introduce pcibios_fix_bus_scan()]

PATCH In review Tony Camuso 2 participants 6 messages Dec 20, 2007

lore
Share & tools
lore

Tony Camuso

author
#0 lore raw

PATCH 4/5 PCI: x86 MMCONFIG: introduce pcibios_fix_bus_scan()]

-------- Original Message --------
Subject: Re: [PATCH 4/5]PCI: x86 MMCONFIG: introduce pcibios_fix_bus_scan()
Date: Wed, 19 Dec 2007 19:17:42 -0500
From: Tony Camuso <[email protected]>
Reply-To: [email protected]
To: Greg KH <[email protected]>
References: <20071219221746.20362.39243.sendpatchset@dhcp83-188.boston.redhat.com> <20071219221806.20362.25964.sendpatchset@dhcp83-188.boston.redhat.com> <[email protected]>

Greg,

First, let me thank you for your prompt replies!

Quoted 28 lines · Greg KH
Quoted 27 lines · On Wed, Dec 19, 2007 at 05:18:06PM -0500, [email protected] wrote:

commit ab28e1157e970f711c8451b66b3f940ec092db9d

Author: Tony Camuso <[email protected]>
Date:   Wed Dec 19 15:51:48 2007 -0500

Introduces the x86 arch-specific routine that will determine whether a device responds correctly to MMCONFIG accesses. This routine is given the generic name pcibios_fix_bus_scan_quirk()

The comment at the top of the routine explains its logic.

Signed-off-by Tony Camuso [email protected]
arch/x86/pci/common.c +7 -0
index 8627463..9b1742d 100644
@@ -525,3 +525,64 @@ struct pci_bus *pci_scan_bus_with_sysdata(int busno)
 
 	return bus;
 }
+
+/**
+ * This routine traps devices not correctly responding to MMCONFIG access.
+ * For each device on the current bus, compare a mmconf read of the
+ * vendor/device dword with a legacy PCI config read. If they're not the same,
+ * the bus serving this device must use legacy PCI config accesses instead of
+ * mmconf, as must all buses descending from this bus.
Quoted 8 lines

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 540

Quoted 6 lines
arch/x86/pci/common.c +6 -0
+ */
+
+#define CHECK_MMCFG_STR_1 \
+	"PCI: Device at %04x:%02x.%02x.%x is not MMCONFIG compliant.\n"
+#define CHECK_MMCFG_STR_2 \
+	"PCI: Bus %04x:%02x and its descendents cannot use MMCONFIG.\n"

Why define these if they are only used in one place?

If you object, I will be happy to move them into the routine body without the defines. I agree that It does look inconsistent to have these strings defined and other strings embedded in the routine body.

Quoted 4 lines

Also, as you use dev_info(), I think you are duplicating some of the information in the resulting printk(), right?

Actually, no. The strings do not contain redundant info. The pr_info routine is just a macro for printk(KERN_INFO ...)

Quoted 21 lines
Quoted 8 lines
arch/x86/pci/common.c +8 -0
+
+void __devinit pcibios_fix_bus_scan_quirk(struct pci_bus *bus)
+{
+	int devfn;
+	int fail;
+	int found_nommconf_dev = 0;
+	static int advised;
+	u32 mcfg_vendev;

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 558

Quoted 10 lines
arch/x86/pci/common.c +10 -0
+	u32 arch_vendev;
+	struct pci_ops *save_ops = bus->ops;
+
+	if (bus->parent != NULL)
+		if (bus->parent->ops == &pci_legacy_ops)
+			return;
+
+	if (!advised) {
+		pr_info("PCI: If a device isn't working, try \"pci=nommconf\". "
+			"If that helps, please post a report.\n");

Post a report where? Who is going to handle these reports?

Quoted 4 lines

The last time someone put a line like this in the kernel, I got a ton of email and didn't know what to do with it. If you really are trusting of this patch, please put your email address in this printk(), so that you can properly handle the resulting reports. I sure don't want to :)

Hmmm! Good point! I was actually copying that other message. I will remove the string that advises posting a report. I sure don't want 'em, and I can see that you don't, either.
:)

Quoted 12 lines

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 567

Quoted 10 lines
arch/x86/pci/common.c +9 -0
+		advised = 1;
+	}
+	pr_debug("PCI: Checking bus %04x:%02x for MMCONFIG compliance.\n",
+		 pci_domain_nr(bus), bus->number);
+
+	for (devfn = 0; devfn < 256; devfn++) {
+		bus->ops = &pci_legacy_ops;
+		fail = (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID,
+						  &arch_vendev));

What's with the extra () around the function?

The function call used to be contained in an if statement. I changed the logic, but forgot to remove the extra parens. It's tough getting old.
:}

Quoted 21 lines
Quoted 8 lines
arch/x86/pci/common.c +7 -0
+		if ((arch_vendev == 0xFFFFFFFF) || (arch_vendev == 0) || fail)
+			continue;
+
+		bus->ops = save_ops;	/* Restore to original value */
+		pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID,
+					  &mcfg_vendev);
+		if (mcfg_vendev != arch_vendev) {

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 584

Quoted 10 lines
arch/x86/pci/common.c +10 -0
+			found_nommconf_dev = 1;
+			break;
+		}
+	}
+
+	if (found_nommconf_dev) {
+		pr_info(CHECK_MMCFG_STR_1, pci_domain_nr(bus), bus->number,
+			PCI_SLOT(devfn), PCI_FUNC(devfn));
+		pr_info(CHECK_MMCFG_STR_2, pci_domain_nr(bus), bus->number);
+		bus->ops = &pci_legacy_ops;	/* Use Legace PCI Config */

Spelling check for your comments :)

Quoted 3 lines

thanks,

greg k-h

Oops Legace ... too much language confusion in my life ... portugues ... italian ... oi ...

#1 lore raw
Quoted 10 lines · Tony Camuso
Quoted 6 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+
+#define CHECK_MMCFG_STR_1 \
+	"PCI: Device at %04x:%02x.%02x.%x is not MMCONFIG compliant.\n"
+#define CHECK_MMCFG_STR_2 \
+	"PCI: Bus %04x:%02x and its descendents cannot use MMCONFIG.\n"

Why define these if they are only used in one place?

If you object, I will be happy to move them into the routine body without the defines. I agree that It does look inconsistent to have these strings defined and other strings embedded in the routine body.

Yes, please fix this.

Quoted 4 lines
Quoted 2 lines

Also, as you use dev_info(), I think you are duplicating some of the information in the resulting printk(), right?

Actually, no. The strings do not contain redundant info. The pr_info routine is just a macro for printk(KERN_INFO ...)

Ah, sorry, I was thinking you were using dev_info(), which is what you should be using instead anyway :)

thanks,

greg k-h

author
#2 lore raw
Quoted 4 lines · Greg KH

Ah, sorry, I was thinking you were using dev_info(), which is what you should be using instead anyway :)

I found it in include/linux/device.h

#define dev_info(dev, format, arg...) \
dev_printk(KERN_INFO , dev , format , ## arg)

The info I'm trying to print is before a dev or pci_dev struct has been initialized for the device.

#3 lore raw
Quoted 11 lines · Tony Camuso
Quoted 2 lines · Greg KH

Ah, sorry, I was thinking you were using dev_info(), which is what you should be using instead anyway :)

I found it in include/linux/device.h

#define dev_info(dev, format, arg...) \
dev_printk(KERN_INFO , dev , format , ## arg)

The info I'm trying to print is before a dev or pci_dev struct has been initialized for the device.

Ok, fair enough.

But you never answered my questions about _who_ would be responding to those log messages about reporting things...

thanks,

greg k-h

author
#4 lore raw
Quoted 6 lines · Greg KH

But you never answered my questions about _who_ would be responding to those log messages about reporting things...

thanks,

greg k-h

I did.

I said I would remove that string, because I don't want all that email, either. It's a little past the middle of the post appended here.

-------- Original Message --------
Subject: Re: [PATCH 4/5]PCI: x86 MMCONFIG: introduce pcibios_fix_bus_scan()
Date: Wed, 19 Dec 2007 19:17:42 -0500
From: Tony Camuso <[email protected]>
Reply-To: [email protected]
To: Greg KH <[email protected]>
References: <20071219221746.20362.39243.sendpatchset@dhcp83-188.boston.redhat.com> <20071219221806.20362.25964.sendpatchset@dhcp83-188.boston.redhat.com> <[email protected]>

Greg,

First, let me thank you for your prompt replies!

Quoted 28 lines · Greg KH
Quoted 27 lines · On Wed, Dec 19, 2007 at 05:18:06PM -0500, [email protected] wrote:

commit ab28e1157e970f711c8451b66b3f940ec092db9d

Author: Tony Camuso <[email protected]>
Date:   Wed Dec 19 15:51:48 2007 -0500

Introduces the x86 arch-specific routine that will determine whether a device responds correctly to MMCONFIG accesses. This routine is given the generic name pcibios_fix_bus_scan_quirk()

The comment at the top of the routine explains its logic.

Signed-off-by Tony Camuso [email protected]
arch/x86/pci/common.c +7 -0
index 8627463..9b1742d 100644
@@ -525,3 +525,64 @@ struct pci_bus *pci_scan_bus_with_sysdata(int busno)
 
 	return bus;
 }
+
+/**
+ * This routine traps devices not correctly responding to MMCONFIG access.
+ * For each device on the current bus, compare a mmconf read of the
+ * vendor/device dword with a legacy PCI config read. If they're not the same,
+ * the bus serving this device must use legacy PCI config accesses instead of
+ * mmconf, as must all buses descending from this bus.
Quoted 8 lines

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 540

Quoted 6 lines
arch/x86/pci/common.c +6 -0
+ */
+
+#define CHECK_MMCFG_STR_1 \
+	"PCI: Device at %04x:%02x.%02x.%x is not MMCONFIG compliant.\n"
+#define CHECK_MMCFG_STR_2 \
+	"PCI: Bus %04x:%02x and its descendents cannot use MMCONFIG.\n"

Why define these if they are only used in one place?

If you object, I will be happy to move them into the routine body without the defines. I agree that It does look inconsistent to have these strings defined and other strings embedded in the routine body.

Quoted 4 lines

Also, as you use dev_info(), I think you are duplicating some of the information in the resulting printk(), right?

Actually, no. The strings do not contain redundant info. The pr_info routine is just a macro for printk(KERN_INFO ...)

Quoted 21 lines
Quoted 8 lines
arch/x86/pci/common.c +8 -0
+
+void __devinit pcibios_fix_bus_scan_quirk(struct pci_bus *bus)
+{
+	int devfn;
+	int fail;
+	int found_nommconf_dev = 0;
+	static int advised;
+	u32 mcfg_vendev;

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 558

Quoted 10 lines
arch/x86/pci/common.c +10 -0
+	u32 arch_vendev;
+	struct pci_ops *save_ops = bus->ops;
+
+	if (bus->parent != NULL)
+		if (bus->parent->ops == &pci_legacy_ops)
+			return;
+
+	if (!advised) {
+		pr_info("PCI: If a device isn't working, try \"pci=nommconf\". "
+			"If that helps, please post a report.\n");

Post a report where? Who is going to handle these reports?

Quoted 4 lines

The last time someone put a line like this in the kernel, I got a ton of email and didn't know what to do with it. If you really are trusting of this patch, please put your email address in this printk(), so that you can properly handle the resulting reports. I sure don't want to :)

Hmmm! Good point! I was actually copying that other message. I will remove the string that advises posting a report. I sure don't want 'em, and I can see that you don't, either.
:)

Quoted 12 lines

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 567

Quoted 10 lines
arch/x86/pci/common.c +9 -0
+		advised = 1;
+	}
+	pr_debug("PCI: Checking bus %04x:%02x for MMCONFIG compliance.\n",
+		 pci_domain_nr(bus), bus->number);
+
+	for (devfn = 0; devfn < 256; devfn++) {
+		bus->ops = &pci_legacy_ops;
+		fail = (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID,
+						  &arch_vendev));

What's with the extra () around the function?

The function call used to be contained in an if statement. I changed the logic, but forgot to remove the extra parens. It's tough getting old.
:}

Quoted 21 lines
Quoted 8 lines
arch/x86/pci/common.c +7 -0
+		if ((arch_vendev == 0xFFFFFFFF) || (arch_vendev == 0) || fail)
+			continue;
+
+		bus->ops = save_ops;	/* Restore to original value */
+		pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID,
+					  &mcfg_vendev);
+		if (mcfg_vendev != arch_vendev) {

arch/x86/pci/common.c struct pci_bus *pci_scan_bus_with_sysdata(int busno) · line 584

Quoted 10 lines
arch/x86/pci/common.c +10 -0
+			found_nommconf_dev = 1;
+			break;
+		}
+	}
+
+	if (found_nommconf_dev) {
+		pr_info(CHECK_MMCFG_STR_1, pci_domain_nr(bus), bus->number,
+			PCI_SLOT(devfn), PCI_FUNC(devfn));
+		pr_info(CHECK_MMCFG_STR_2, pci_domain_nr(bus), bus->number);
+		bus->ops = &pci_legacy_ops;	/* Use Legace PCI Config */

Spelling check for your comments :)

Quoted 3 lines

thanks,

greg k-h

Oops Legace ... too much language confusion in my life ... portugues ... italian ... oi ...

#5 lore raw
Quoted 11 lines · Tony Camuso
Quoted 4 lines · Greg KH

But you never answered my questions about _who_ would be responding to those log messages about reporting things...
thanks,
greg k-h

I did.

I said I would remove that string, because I don't want all that email, either. It's a little past the middle of the post appended here.

Oops, sorry, I missed that response, my apologies.

greg k-h