On Fri, 2015-03-27 at 13:07 -0500, Scott Wood wrote: > On Fri, 2015-03-27 at 10:45 +1100, Michael Ellerman wrote: > > On Thu, 2015-03-26 at 10:31 -0500, Emil Medve wrote: > > > Hello Kumar, > > > > > > > > > On 03/26/2015 10:18 AM, Kumar Gala wrote: > > > > Why no commit message with what issue this change was trying to fix? > > > > > > A while back, when I attempted to remove bootmem (in favor of just plain > > > memblock as in powerpc land bootmem was just a wrapper to memblock > > > anyway) I run at some point into a problem with an intermediate address > > > value because of this '<< PAGE_SHIFT' on the wrong width variable. Using > > > PFN_PHYS() took care of it (it has a cast) so I decided to get this > > > defensive patch applied. Since, I dropped my bootmem/memblock patches in > > > favor to Anton's (Blanchard) work so my concrete issue example is > > > somewhat gone > > > > I'm not a big fan of it unless it's actually fixing an issue. It's a lot of > > churn and the end result is less readable IMHO. > > It is fixing an issue -- the issue is that there are overflow errors in > the code. Some of the places Emil fixed are only for platforms that > don't have physical addresses larger than pointers, or have the needed > casts, or are known to be dealing with lowmem, but others aren't. E.g. > page_is_ram() and devmem_is_allowed() are buggy on 32-bit with 64-bit > physical.
Right. So obviously I'm fine with all the cases where it fixes an actual bug. > flush_dcache_icache_page() is buggy on mpc86xx with more than 4 GiB RAM > -- though that would still be buggy even with this change, due to > __flush_dcache_icache_phys taking unsigned long. The entire concept of > that function doesn't work for sizeof(phys_addr_t) > sizeof(void *), so > in this case 86xx should be using the booke code instead. > > Even in the places where overflow can't happen due to the above > circumstances (other than having the needed cast), it's setting a bad > example that can be copied to places where it will break, or the > circumstances of the code could change (e.g. currently 64-bit-only code > being used on 32-bit). I agree with that in principle, but it looks like in a bunch of places this patch ends up assigning the result to unsigned long anyway. So those cases are just churn IMHO. The end result doesn't work with 64-bit phys if the code is ever used there, even though it looks like maybe it should, and it's also not setting a good example for other code. Those cases should either be left alone, or fixed properly to use phys_addr_t. cheers _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
