-------- 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!
Greg KH wrote:
> 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]
>>
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index 8627463..9b1742d 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -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.
>> + */
>> +
>> +#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.
>
> 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 ...)
>> +
>> +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;
>> + 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?
>
> 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.
:)
>
>> + 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.
:}
>
>> + 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) {
>> + 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 :)
>
> thanks,
>
> greg k-h
Oops Legace ... too much language confusion in my life ... portugues ...
italian ... oi ...
On Thu, Dec 20, 2007 at 07:26:17AM -0500, Tony Camuso wrote:
>>> +
>>> +#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.
>> 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
Greg KH wrote:
>
> 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.
On Thu, Dec 20, 2007 at 12:39:48PM -0500, Tony Camuso wrote:
> Greg KH wrote:
>> 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
Greg KH wrote:
> 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!
Greg KH wrote:
> 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]
>>
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index 8627463..9b1742d 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -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.
>> + */
>> +
>> +#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.
>
> 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 ...)
>> +
>> +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;
>> + 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?
>
> 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.
:)
>
>> + 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.
:}
>
>> + 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) {
>> + 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 :)
>
> thanks,
>
> greg k-h
Oops Legace ... too much language confusion in my life ... portugues ...
italian ... oi ...
On Thu, Dec 20, 2007 at 01:08:55PM -0500, Tony Camuso wrote:
> Greg KH wrote:
>
>> 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