Re: [patch 2.6.12 (repost w/ corrected subject)] pci: restore BAR values in pci_enable_device_bars

[Grant Grundler <grundler@parisc-linux.org>]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On Thu, Jun 30, 2005 at 10:26:37PM -0400, John W. Linville wrote:
...
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -378,9 +378,56 @@ pci_restore_state(struct pci_dev *dev)
>  int
>  pci_enable_device_bars(struct pci_dev *dev, int bars)
>  {
> -	int err;
> +	int i, numres, err;
>  
>  	pci_set_power_state(dev, PCI_D0);
> +
> +	/* Some devices lose PCI config header data during D3hot->D0

Can you name some of those devices here?
I just want to know what sort of devices need to be tested 
if this code changes in the future.

> +	   transition.	Since some firmware leaves devices in D3hot
> +	   state at boot, this information needs to be restored.

Again, which firmware?
Examples are good since it makes it possible to track down
the offending devices for testing.

The following chunk looks like it will have issues with 64-bit BARs:
...
> +	for (i = 0; i < numres; i ++) {
> +		struct pci_bus_region region;
> +		u32 val;
> +		int reg;
...
> +		val = region.start
> +		    | (dev->resource[i].flags & PCI_REGION_FLAG_MASK);
> +
> +		reg = PCI_BASE_ADDRESS_0 + (i * 4);

ISTR dev->resource[i] doesn't necessarily correspond directly BAR[i].
If BAR0 is a 64-bit BAR, then dev->resource[1] will point at BAR2.

I'm not sure how to fix this since I'm not quite sure where
state is being saved off from.

> +		pci_write_config_dword(dev, reg, val);
> +
> +		if ((val & (PCI_BASE_ADDRESS_SPACE
> +		          | PCI_BASE_ADDRESS_MEM_TYPE_MASK))
> +		 == (PCI_BASE_ADDRESS_SPACE_MEMORY
> +		   | PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> +			pci_write_config_dword(dev, reg + 4, 0);

64-bit BARs need the upper half of dev->resource[] written.
I expect something like:
		val = region.start >> 4;
		pci_write_config_dword(dev, reg + 4, val);

This should put zero in the upper 32-bit if it's only a 32-bit value.
I suspect "i" needs to be split into two indices: one for dev->resource[]
and another for offset into PCI config space (BARs).
But it really depends on how dev->resource[] maps to the BARs.

hth,
grant

> +		}
> +	}
> +
>  	if ((err = pcibios_enable_device(dev, bars)) < 0)
>  		return err;
>  	return 0;
> -- 
> John W. Linville
> linville@tuxdriver.com

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]