Re: [PATCH 2/2] PCI: unset the resource if we can't get the correct CPU address

[Liu Jiang <liuj97@gmail.com>]

On Sun 19 May 2013 10:24:45 AM CST, Kevin Hao wrote:
> On Fri, May 17, 2013 at 10:51:20PM +0800, Liu Jiang wrote:
>> On Fri 17 May 2013 10:11:29 AM CST, Kevin Hao wrote:
>>> On Fri, May 17, 2013 at 12:18:07AM +0800, Liu Jiang wrote:
>>>> On Tue 14 May 2013 09:07:56 PM CST, Kevin Hao wrote:
>>>>> In the current kernel, we just set the CPU address to the bus address
>>>>> if we can't find the match region for one specific bus address. If BAR
>>>>> of one pci device is set to address which happens to be a legitimate
>>>>> CPU address by firmware, the kernel will think this resource is legal
>>>>> and will not try to reassign later. In cases the CPU address and bus
>>>>> address isn't equal, the device will not work. So we should check
>>>>> if we can translate the bus address to CPU address correctly. If not,
>>>>> we should unset this resource and wish the kernel will reassign it
>>>>> later.
>>>>>
>>>>> Since we will invoke pcibios_bus_to_resource unconditionally if we
>>>>> don't goto fail, move it out of if/else wrap.
>>>>>
>>>>> Signed-off-by: Kevin Hao <haokexin@gmail.com>
>>>>> ---
>>>>>   drivers/pci/probe.c | 9 ++++++---
>>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>>> index 70f10fa..c96772f 100644
>>>>> --- a/drivers/pci/probe.c
>>>>> +++ b/drivers/pci/probe.c
>>>>> @@ -250,12 +250,10 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>>>>>   			pci_write_config_dword(dev, pos + 4, 0);
>>>>>   			region.start = 0;
>>>>>   			region.end = sz64;
>>>>> -			pcibios_bus_to_resource(dev, res, &region);
>>>>>   			bar_disabled = true;
>>>>>   		} else {
>>>>>   			region.start = l64;
>>>>>   			region.end = l64 + sz64;
>>>>> -			pcibios_bus_to_resource(dev, res, &region);
>>>>>   		}
>>>>>   	} else {
>>>>>   		sz = pci_size(l, sz, mask);
>>>>> @@ -265,7 +263,12 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>>>>>
>>>>>   		region.start = l;
>>>>>   		region.end = l + sz;
>>>>> -		pcibios_bus_to_resource(dev, res, &region);
>>>>> +	}
>>>>> +
>>>>> +	if (!pcibios_bus_to_resource(dev, res, &region)) {
>>>>> +		res->flags |= IORESOURCE_UNSET;
>>>>> +		res->end -= res->start;
>>>>> +		res->start = 0;
>>>>>   	}
>>>>>
>>>>>   	goto out;
>>>>
>>>> Hi Kevin,
>>>>       Will this break subtractive decode PCI bridges and devices?
>>>
>>> No. A subtractive decode occurs only when no other pci bridge or device
>>> claim the transactions. As you can see that when we are trying to translate
>>> a bus address to cpu address we search the address regions of pci host
>>> bridge instead of a pci bridge. The pci host bridge address regions should
>>> cover all the bus address we can use for the pci device or bridge under this
>>> pci controller. This definitely also include the bus address regions used by
>>> the expansion bus for subtractive decode. So if a pci device use a bus address
>>> that is unknown to this pci host bridge, there is definitely something wrong
>>> here. This is the case we are trying to fix.
>>>
>> Hi Kevin,
>>        I'm not sure about this assumption "the pci host bridge
>> address regions should cover
>> all the bus address we can use for the pci device or bridge under
>> this pci controller".
>> I have a fear that it may cause regressions to some legacy x86
>> platforms, such as an AMD
>> platform with PCI based IOAPICs, though I have no evidences for it.
>
> Does any x86 expert can confirm whether the assumption that "the pci host
> bridge address regions should cover all the bus address we can use for the
> pci device or bridge under this pci host bridge" is true or not on x86?
> It seems definitely wrong in logical. But things always happen out of
> expectation in reality. :-)
>
> Thanks,
> Kevin
>
>> Regards!
>> Gerry
>>> Thanks,
>>> Kevin
>>>
>>>> Regards!
>>>> Gerry
>>
>>

Hi Yinghai,
      Any comments here?