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

[Barry Song]

On Fri, May 21, 2010 at 3:10 AM, Chris Metcalf <cmetcalf@xxxxxxxxxx> wrote:
> On 5/20/2010 1:04 AM, Barry Song wrote:
>> On Thu, May 20, 2010 at 1:43 PM, Chris Metcalf <cmetcalf@xxxxxxxxxx> wrote:
>>
>>> --- linux-2.6.34/include/linux/mm.h   2010-05-16 17:17:36.000000000 -0400
>>> +++ tilera-source/include/linux/mm.h  Â2010-05-17 12:54:33.540145000 -0400
>>> @@ -592,7 +592,7 @@
>>>
>>> Â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.
>> [...]
>> Your patch is not compilable, and the subject doesn't match well with
>> the content. I think you need re-organize patches.

Where does the blade_pci symbol come from? "No matches found" by a
grep. Is it exported in your local codes?
Why not just use #if CONFIG_TILE to cover your quirk_tile_blade. And
where is CONFIG_TILE?
I guess all will be explained by your arch codes, but this patch
depends on your arch codes, so it shouldn't be here by itself.

>>
>
> Where do you see the compilation failure? ÂI tested this with the only
> other architecture I have handy (x86_64) and it built OK. ÂAnd by code
> inspection, <linux/mm.h> includes <linux/mm_types.h> includes
> <linux/types.h>, which always provides phys_addr_t suitably (based on
> CONFIG_PHYS_ADDR_T_64BIT).
>
> In any case, a better solution might be to #include <linux/pfn.h> in
> <linux/mm.h> and write this function as:
>
> static __always_inline void *lowmem_page_address(struct page *page)
> {
> Â Â Â Âreturn __va(PFN_PHYS(page_to_pfn(page));
> }
>
>
> Note that PFN_PHYS() is already defined to include the cast to
> phys_addr_t. ÂJeremy Fitzhardinge added the cast in Sep 2008 with a
> comment that echoes this discussion:
>
> Â Âgeneric: make PFN_PHYS explicitly return phys_addr_t
>
> Â ÂPFN_PHYS, as its name suggests, turns a pfn into a physical address.
> Â ÂHowever, it is a macro which just operates on its argument without
> Â Âmodifying its type. Âpfns are typed unsigned long, but an unsigned
> Â Âlong may not be long enough to hold a physical address (32-bit systems
> Â Âwith more than 32 bits of physcial address).
>
> Â ÂMake sure we cast to phys_addr_t to return a complete result.
>
>
> Linus, does this seem like the right generic answer, or would it make
> more sense, as you suggested, to try to provide a new pfn_to_phys()
> function in the architecture-independent code?
>
> In any case, in the spirit of providing a complete answer, I'll provide
> a proper patch in a following email.
>
> --
> Chris Metcalf, Tilera Corp.
> http://www.tilera.com
>
>
>
--
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/