[SLOF] [PATCH] pci-scan: Fix pci-bridge-set-mem-base and pci-bridge-set-mem-limit

[Alexey Kardashevskiy]

On 20/07/17 18:58, Thomas Huth wrote:
> On 20.07.2017 08:18, Alexey Kardashevskiy wrote:
>> On 19/07/17 01:30, Thomas Huth wrote:
>>> The functions used a bogus mixture between programming the registers
>>> with pci-next-mem64 and pci-next-mem - the upper register bits were
>>> filled with the value from the 64-bit memory space while the lower
>>> bits were filled with the bits from the 32-bit memory space variable.
>>
>> It is a total rework of two pretty old pieces of code with fewer comments
>> than before (and no new stack comments or smaller words...) and no clear
>> indication of whether it changes the actual behaviour, or does not it?
> 
> Just have a closer look at https://github.com/aik/SLOF/commit/9633e036
> and you'll note what's wrong with the code: Initially, all PCI bridge
> prefetchable memory base registers were programmed with the value from
> pci-next-mem and the limit registers with the value from pci-max-mem.
> 
> After that patch, the upper bits were programmed with pci-next-mem64 and
> pci-max-mem64, while the lower bits were still programmed with
> pci-next-mem and pci-max-mem! A complete mess, I wonder why this did not
> cause any trouble before (well, likely because hardly anybody tested PCI
> bridges before)...

But I am pretty sure it has fixed something at the time though...

> I think we really should clean that up ... so would you be willing to
> accept my patch if I add some more comments to the code there?

I have no doubts the patch fixes the problem.

I just wonder if it could be made easier to follow by adding separate words
for mem64 - pci-bridge-set-mem64-base/pci-bridge-set-mem64-limit - just
like we have them for pci-{next|max}-{mem|mmio|io}. Also stack comments are
useful, more than sentences imho. And every time you want to add a comment
like "\ Align variable to 1MB boundary" to few lines of code - just make it
a separate word - "pci-next-mem-align-1mb" - which won't even need comments
(look who I am telling it to) :)




-- 
Alexey