Re: Weirdness in pci_read_bases()

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thursday, February 28, 2008 6:37 pm Benjamin Herrenschmidt wrote:
> Hi !
>
> There is something dodgy going on in pci_read_bases().
>
> In addition to the fact (or related to it) that we tend to treat
> res->start == 0 as meaning "unassigned" (which at this stage is mostly
> incorrect, but let's say I can cope),

Yeah, that sounds like trouble.  I expect this may become a problem even for 
PC architectures in the future (IOMMUs on multiple busses for example), so 
maybe it makes sense to change the core to not assume 0 == unassigned?

> > however, there is some bit of code  
> that I think is just plain wrong:
>
> 		reg = PCI_BASE_ADDRESS_0 + (pos << 2);
> 		pci_read_config_dword(dev, reg, &l);
> 		pci_write_config_dword(dev, reg, ~0);
> 		pci_read_config_dword(dev, reg, &sz);
> 		pci_write_config_dword(dev, reg, l);
> 		if (!sz || sz == 0xffffffff)
> 			continue;
> 		if (l == 0xffffffff)
> 			l = 0;
>
> So here, we read the BAR value, which at this stage could either be some
> assigned address or some ff's from a freshly unassigned BAR. _however_
> that test against 0xffffffff looks totally bogus to me.
>
> If we read that value, we write 0 in l. Which means that if we read some
> unassigned resource that had the address space bits set to IO, we
> overwrite this with a bit encoding that means Memory, and further along,
> start sizing & treating this as a memory resource.

But we should never hit the l == 0xffffffff case if it's a memory BAR, since 
the low bit will be 0, right?

I think Grant is probably right that the only time we'd hit this is if the bus 
is down and we're getting back all 1s for every read (though that's a PC 
specific assumption).

> This isn't a problem I'm encountering in practice. I just noticed that
> while trying to audit what it would take to fix the code to stop using
> res->start = 0 as a way to mark unassigned resources (unrelated), so it
> doesn't -need- to be fixed per-se, but I'm curious where that came from
> in the first place.

Yeah, it seems like it could be safely removed, but this is an area where we 
should probably move slowly (i.e. one, very small change to the probing code 
at a time) or it may be difficult to isolate any bugs in the wild.

Jesse