[PATCH v3] powerpc: Use PFN_PHYS() to avoid truncating the physical address

Michael Ellerman mpe at ellerman.id.au
Mon Mar 30 12:40:24 AEDT 2015


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




More information about the Linuxppc-dev mailing list