Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation.

[David Daney <ddaney@caviumnetworks.com>]

On 09/15/2015 10:49 AM, Will Deacon wrote:
> On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> There are two problems with the bus_max calculation:
>>
>> 1) The u8 data type can overflow for large config space windows.
>>
>> 2) The calculation is incorrect for a bus range that doesn't start at
>>     zero.
>>
>> Since the configuration space is relative to bus zero, make bus_max
>> just be the size of the config window scaled by bus_shift.  Then clamp
>> it to a maximum of 255, per PCI.  Use a data type of int to avoid
>> overflow problems.
>>
>> Signed-off-by: David Daney <david.daney@cavium.com>
>> ---
>>   drivers/pci/host/pci-host-generic.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
>> index cd6f898..fce5bf7 100644
>> --- a/drivers/pci/host/pci-host-generic.c
>> +++ b/drivers/pci/host/pci-host-generic.c
>> @@ -164,7 +164,7 @@ out_release_res:
>>   static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>>   {
>>   	int err;
>> -	u8 bus_max;
>> +	int bus_max;
>>   	resource_size_t busn;
>>   	struct resource *bus_range;
>>   	struct device *dev = pci->host.dev.parent;
>> @@ -177,8 +177,9 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
>>   	}
>>
>>   	/* Limit the bus-range to fit within reg */
>> -	bus_max = pci->cfg.bus_range->start +
>> -		  (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
>> +	bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1;
>> +	if (bus_max > 255)
>> +		bus_max = 255;
>>   	pci->cfg.bus_range->end = min_t(resource_size_t,
>>   					pci->cfg.bus_range->end, bus_max);
>
> Hmm, this is changing the meaning of the bus-range property in the
> device-tree, which really needs to match what IEEE Std 1275-1994 requires.

I doesn't change the bus-range.

>
> My understanding was that the bus-range could be used to offset the config
> space, which is why it's subtracted from the bus number in
> gen_pci_map_cfg_bus_[e]cam.

There is an inconsistency in the current code.  The calculation of the 
cfg.win[?] pointers is done such that the beginning of the config space 
specified in the "reg" property corresponds to bus 0.

The calculation that I am changing, was done such that the beginning of 
the config space specified in the "reg" property corresponds to the 
first bus of the "bus-range"

Which is correct?  I assumed that the config space specified in the 
"reg" property corresponds to bus 0.  Based on this assumption, I made 
the bus_max calculation match.

Due to hardware peculiarities, our bus-range starts at a non-zero bus 
number.  So, something has to be done to make all the code agree on a 
single interpretation of the meaning "reg" property.

> Also, why is your config space so large that
> we end up overflowing bus_max?

It isn't.  The part of the patch that changes the type from u8 to int 
was just to add some sanity.  The  code was easily susceptible to 
overflow failures, it seemed best to change to int.


David Daney