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

[Scott Wood <scottwood@freescale.com>]

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.

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).

-Scott