Re: [PATCH] arch/tile: new multi-core architecture for Linux

[Linus Torvalds]

On Thu, 20 May 2010, Barry Song wrote:

> On Thu, May 20, 2010 at 1:43 PM, Chris Metcalf <cmetcalf@xxxxxxxxxx> wrote:
> >
> > Âstatic __always_inline void *lowmem_page_address(struct page *page)
> > Â{
> > - Â Â Â return __va(page_to_pfn(page) << PAGE_SHIFT);
> > + Â Â Â return __va((phys_addr_t)page_to_pfn(page) << PAGE_SHIFT);
> 
> Here doesn't make sense. you give a u64 type cast, but change the
> meaning of pfn. Is pfn phys_addr_t? Anyway, page_to_pfn can be
> re-fulfilled in your arch, but not change it in common code.

No, it actually makes a lot of sense.

The PFN may well be 32-bit, but then shifting it by PAGE_SHIFT turns the 
PFN from a PFN to a physical address. So the cast makes sense as a way to 
make sure that the code allows a 32-bit PFN with a 64-bit physical 
address.

So I don't thionk there's anything tile-specific about it, and it looks 
like a sane patch. If anything, it might make some sense to make this an 
explicit thing, ie have a "pfn_to_phys()" helper, because there's a _lot_ 
of these things open-coded.

And some of them actually have the cast already. See for example

	#define pfn_to_nid(pfn)	pa_to_nid(((u64)(pfn) << PAGE_SHIFT))

in the alpha <asm/mmzone.h>. Also:

	resource_size_t offset = ((resource_size_t)pfn) << PAGE_SHIFT;

in the powerpc PCI code, of

	#define page_to_phys(page)  ((dma_addr_t)page_to_pfn(page) << PAGE_SHIFT)

in the x86 io code.

In fact, UM has that "pfn_to_phys()" helper already (and has a (phys_t) 
cast).

So we do already have a lot of casts (just grep for "pfn.*<<.*SHIFT" and 
you'll see them in generic code already, and the new one for tile makes 
100% sense. In fact, we should clean up the existing ones.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/