Re: [PATCH] pci: fix bridge 64bit flag setting

[Yinghai Lu <yinghai@kernel.org>]

Bjorn Helgaas wrote:
> On Tuesday 01 December 2009 12:03:57 am Yinghai Lu wrote:
>> Alex found one system that one pci bridge pref mmio 64 is not set correctly.
>> aka, the upper32 base/limit is not cleaned.
>> he found that bridge is supporting 64 bit pref mmio, but device under that
>> does not support that. so that IORESOURCE_MEM_64 get cleared in pbus_size_mem()
> 
> I think it's wrong that pbus_size_mem() fiddles with IORESOURCE_MEM_64
> in bus resources based on where BARs of devices on that bus live.  That
> feels fragile.

yes. need more clean up.

> 
> The question of whether the bridge supports 64-bit apertures is
> strictly a hardware property of the bridge.  It has nothing to do
> with where we place downstream devices.
> 
> Is there really a problem with writing to PCI_PREF_BASE_UPPER32
> unconditionally?  As Alex pointed out, per 3.2.5.10 of the bridge
> spec, the UPPER32 registers are read-only if only 32-bit apertures
> are supported.  If you mentioned a problem with doing this
> unconditionally, I missed it.

remember that some x2apic registers say they are reserved, and read them could cause GP error.

and for pci devices, if it is read-only, for good design, then write it should be ok.
but if there is some design problem in devices, and they could say those are read-only.
why kernel write value to it?

> 
> The only place we test IORESOURCE_MEM_64 for a bus resource is when
> we're programming PCI_PREF_BASE_UPPER32.  If we think it's important
> to program it conditionally, why don't we skip IORESOURCE_MEM_64

IORESOURCE_MEM_64 is used to control if we can use MMIO > 4g.

> altogether, and just look at the bits in PCI_PREF_MEMORY_BASE directly?
> E.g., something like this:
> 
> 	pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &l);
> 	if ((l & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
> 		pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, bu);
> 		pci_write_config_dword(bridge, PCI_PREF_LIMIT_UPPER32, lu);
> 	}
> 
> Then we don't have to maintain flags at all, and it's easy to verify
> that the code corresponds to the spec.

that will have several extra read, and also we already store that bit in pci_read_bridge_bases()

but forget to set that in pci_check_bus_range()

YH

YH