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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 20 May 2010 12:10:38 -0700
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Barry Song <21cnbao@...il.com>
CC:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] arch/tile: new multi-core architecture for Linux

On 5/20/2010 1:04 AM, Barry Song wrote:
> On Thu, May 20, 2010 at 1:43 PM, Chris Metcalf <cmetcalf@...era.com> 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 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/