Re: [PATCH 1/5]PCI: x86 MMCONFIG: introduce PCI_USING_MMCONF

Re: [PATCH 1/5]PCI: x86 MMCONFIG: introduce PCI_USING_MMCONF

[Greg KH]

On Wed, Dec 19, 2007 at 05:17:51PM -0500, tcamuso@redhat.com wrote:
> commit d1f2a573ca7083d237cfc354f42e089de9fa0f0b
> Author: Tony Camuso <tony.camuso@hp.com>
> Date:   Wed Dec 19 15:33:53 2007 -0500
> 
>     Introduces a flag bit to arch/x86/pci/pci.h to indicate that
>     MMCONFIG is the system-wide default PCI config access mechanisim.
>     
>     Function pci_mmcfg_init() in arch/x86/pci/mmconfig-shared.c sets
>     the bit in the pci_probe mask when MMCONFIG has been successfully
>     initialized for the platform.
>     
>     Signed-off-by: Tony Camuso tony.camuso@hp.com
> 
> diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
> index 4df637e..fc0f866 100644
> --- a/arch/x86/pci/mmconfig-shared.c
> +++ b/arch/x86/pci/mmconfig-shared.c
> @@ -274,7 +274,8 @@ void __init pci_mmcfg_init(int type)
>  			unreachable_devices();
>  		if (known_bridge)
>  			pci_mmcfg_insert_resources(IORESOURCE_BUSY);
> -		pci_probe = (pci_probe & ~PCI_PROBE_MASK) | PCI_PROBE_MMCONF;
> +		pci_probe = pci_probe & ~PCI_PROBE_MASK;
> +		pci_probe = pci_probe | PCI_PROBE_MMCONF | PCI_USING_MMCONF;
>  	} else {
>  		/*
>  		 * Signal not to attempt to insert mmcfg resources because
> diff --git a/arch/x86/pci/pci.h b/arch/x86/pci/pci.h
> index ac56d39..53f4051 100644
> --- a/arch/x86/pci/pci.h
> +++ b/arch/x86/pci/pci.h
> @@ -28,6 +28,7 @@
>  #define PCI_ASSIGN_ALL_BUSSES	0x4000
>  #define PCI_CAN_SKIP_ISA_ALIGN	0x8000
>  #define PCI_USE__CRS		0x10000
> +#define PCI_USING_MMCONF	0x20000
>  
>  extern unsigned int pci_probe;
>  extern unsigned long pirq_table_addr;
> @@ -39,6 +40,8 @@ enum pci_bf_sort_state {
>  	pci_dmi_bf,
>  };
>  
> +extern struct pci_ops pci_legacy_ops; 	/* direct.c */

This isn't needed in this patch at all, and might make the compiler
confused if you were to build with only this patch present :(

thanks,

greg k-h

Re: [PATCH 1/5]PCI: x86 MMCONFIG: introduce PCI_USING_MMCONF

[Tony Camuso]

Greg KH wrote:

> Please do not respond privately, please respond back on the list so that
> everyone can see.  That way I will respond, to do otherwise is rude to
> the rest of the community...
> 
> thanks,
> 
> greg k-h

I'm very sorry.

It is certainly not my intention to be rude.

Re: [PATCH 2/5]PCI: x86 MMCONFIG: add legacy pci conf functions

[Greg KH]

On Wed, Dec 19, 2007 at 05:17:56PM -0500, tcamuso@redhat.com wrote:
> commit 307e3aecde8060af3802590f8c5bffb5456fe22b
> Author: Tony Camuso <tony.camuso@hp.com>
> Date:   Wed Dec 19 15:38:31 2007 -0500
> 
>     Modifies arch/x86/pci/direct.c to add the Legacy PCI Config routines
>     that will be made available, even when MMCONFIG is the platform default
>     PCI Config access mechanism. It also provides logic for selecting the
>     correct Legacy PCI Config access mechanism and the corresponding boot log
>     messages.
>     
>     Signed-off-by: Tony Camuso tony.camuso@hp.com
> 
> diff --git a/arch/x86/pci/direct.c b/arch/x86/pci/direct.c
> index 431c9a5..4e78002 100644
> --- a/arch/x86/pci/direct.c
> +++ b/arch/x86/pci/direct.c
> @@ -175,6 +175,32 @@ static struct pci_raw_ops pci_direct_conf2 = {
>  	.write =	pci_conf2_write,
>  };
>  
> +/*
> + * Legacy PCI Config read and write routines for buses that can't use
> + * MMCONFIG accesses in systems where MMCONFIG is the default PCI config
> + * access mechanism.
> + */
> +static struct pci_raw_ops *pci_legacy_conf;
> +
> +static int pci_ops_legacy_read(struct pci_bus *bus, unsigned int devfn,
> +			      int where, int size, u32 *value)
> +{
> +	return pci_legacy_conf->read(0, bus->number, devfn, where,
> +				     size, value);
> +}
> +
> +static int pci_ops_legacy_write(struct pci_bus *bus, unsigned int devfn,
> +			       int where, int size, u32 value)
> +{
> +	return pci_legacy_conf->write(0, bus->number, devfn, where,
> +				      size, value);
> +}
> +
> +struct pci_ops pci_legacy_ops = {
> +	.read =		pci_ops_legacy_read,
> +	.write =	pci_ops_legacy_write,
> +};

This whole structure is never used in this patch, why add it now?

>  /*
>   * Before we decide to use direct hardware access mechanisms, we try to do some
> @@ -258,18 +284,28 @@ void __init pci_direct_init(int type)
>  {
>  	if (type == 0)
>  		return;
> -	printk(KERN_INFO "PCI: Using configuration type %d\n", type);
> -	if (type == 1)
> +	if (type == 1) {
>  		raw_pci_ops = &pci_direct_conf1;
> -	else
> +		pci_legacy_conf = &pci_direct_conf1;
> +	} else {
>  		raw_pci_ops = &pci_direct_conf2;
> +		pci_legacy_conf = &pci_direct_conf2;

Why have two pointers to the same thing?  What is pci_legacy_conf going
to be used for?  Why not just use raw_pci_ops?

thanks,

greg k-h

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

[Greg KH]

On Wed, Dec 19, 2007 at 05:18:06PM -0500, tcamuso@redhat.com wrote:
> commit ab28e1157e970f711c8451b66b3f940ec092db9d
> Author: Tony Camuso <tony.camuso@hp.com>
> 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 tony.camuso@hp.com
> 
> 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?

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

> +
> +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 :)

> +		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?

> +		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

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

[Greg KH]

On Wed, Dec 19, 2007 at 05:18:06PM -0500, tcamuso@redhat.com wrote:
> commit ab28e1157e970f711c8451b66b3f940ec092db9d
> Author: Tony Camuso <tony.camuso@hp.com>
> 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 tony.camuso@hp.com
> 
> 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.
> + */

Don't user kerneldoc comments for functions that are not really in
kerneldoc format.  Just a simple /* at the start will be fine, otherwise
the tools get confused.

So what happens today with these "broken" devices?  Does simply reading
the vendor id not work properly?  What specific hardware does not work
today that this is trying to fix?

I think this patch set needs a bit of reworking at the very least...

thanks,

greg k-h

Re: [PATCH 0/5]PCI: x86 MMCONFIG

[Greg KH]

On Wed, Dec 19, 2007 at 05:17:46PM -0500, tcamuso@redhat.com wrote:
> OVERVIEW
> =======
> 
> The patches should be applied in sequence to obviate any
> possible build problems.
> 
> The patch-set was built against 2.6.24-rc5
> 
> Description
> ===========
> 
> There exist devices that do not respond correctly to PCI
> MMCONFIG accesses in x86 platforms.

What devices are these?  Do you have reports of them somewhere?

> This patch-set detects the problem by comparing an MMCONFIG
> read to a Legacy PCI config read of the vendor/device dword
> of every device discovered during the PCI probing sequence.
> 
> A miscompare means that a device does not correctly respond
> to MMCONFIG accesses. When the patch code detects this condition,
> the bus that serves this device, and all subordinate buses, will
> be programmed to use Legacy PCI Config accesses.
> 
> This patch-set DOES NOT detect devices that generate machine
> checks against MMCONFIG accesses. For such systems,
> "pci=nommconf" is required in the boot command.

That sounds like this patchset can cause bad side affects on hardware
that currently works just fine.  That is not a good thing to be adding
to the kernel, right?

thanks,

greg k-h