linux-mips
[Top] [All Lists]

Re: [PATCH] Put cast inside macro instead of all the callers

To: linux-mips <linux-mips@linux-mips.org>
Subject: Re: [PATCH] Put cast inside macro instead of all the callers
From: Ulrich Eckhardt <eckhardt@satorlaser.com>
Date: Thu, 1 Nov 2007 17:04:01 +0100
In-reply-to: <20071031141124.185599da@ripper.onstor.net>
Organization: Sator Laser GmbH
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <20071031141124.185599da@ripper.onstor.net>
Sender: linux-mips-bounce@linux-mips.org
User-agent: KMail/1.9.7
I'm by far not a MIPS expert, but I'm puzzled by the code and how it uses 
signed integers for addresses. I just added some comments below, but I'm not 
sure if they are valid. Thank you for any clarification!

On Wednesday 31 October 2007, Andrew Sharp wrote:
> Since all the callers of the PHYS_TO_XKPHYS macro call with a constant,
> put the cast to LL inside the macro where it really should be rather
> than in all the callers.  This makes macros like PHYS_TO_XKSEG_UNCACHED
> work without gcc whining.

I'm not sure if this is always a compile-time constant so that you can adorn 
it with a LL. However, note that this is not a cast, a cast is at runtime.

>       if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
>               usp = CKSEG1ADDR(sp);
>  #ifdef CONFIG_64BIT
> -     else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0LL, 0) &&
> -              (long long)sp < (long long)PHYS_TO_XKPHYS(8LL, 0))
> -             usp = PHYS_TO_XKPHYS((long long)K_CALG_UNCACHED,
> +     else if ((long long)sp >= (long long)PHYS_TO_XKPHYS(0, 0) &&
> +              (long long)sp < (long long)PHYS_TO_XKPHYS(8, 0))
> +             usp = PHYS_TO_XKPHYS(K_CALG_UNCACHED,
>                                    XKPHYS_TO_PHYS((long long)sp));

I'd say this code is broken in way too many aspects:
1. A plethora of casts. PHYS_TO_XKPHYS() should return a physical address 
(i.e. 32 or 64 bits unsigned integer) already, so casting its result should 
not be necessary.
2. Using a signed integer of undefined size for an address. At least use an 
explicit 64 bit unsigned integer (__u64).
3. The use of signed types makes me wonder about intended overflow semantics. 
Just for the record, signed overflow in C causes undefined behaviour, no 
diagnostic required, and recent GCC even assume that no overflow occurs as an 
optimisation!

>  #define PHYS_TO_XKSEG_CACHED(p)              
> PHYS_TO_XKPHYS(K_CALG_COH_SHAREABLE,(p))
>  #define XKPHYS_TO_PHYS(p)            ((p) & TO_PHYS_MASK)
>  #define PHYS_TO_XKPHYS(cm,a)         (_CONST64_(0x8000000000000000) | \
> -                                      ((cm)<<59) | (a))
> +                                      (_CONST64_(cm)<<59) | (a))

This macro will always(!!!) generate a negative number, is that intended?

Uli
- slightly puzzled -

-- 
Sator Laser GmbH
Geschäftsführer: Michael Wöhrmann, Amtsgericht Hamburg HR B62 932

**************************************************************************************
           Visit our website at <http://www.satorlaser.de/>
**************************************************************************************
Diese E-Mail einschließlich sämtlicher Anhänge ist nur für den Adressaten 
bestimmt und kann vertrauliche Informationen enthalten. Bitte benachrichtigen 
Sie den Absender umgehend, falls Sie nicht der beabsichtigte Empfänger sein 
sollten. Die E-Mail ist in diesem Fall zu löschen und darf weder gelesen, 
weitergeleitet, veröffentlicht oder anderweitig benutzt werden.
E-Mails können durch Dritte gelesen werden und Viren sowie nichtautorisierte 
Änderungen enthalten. Sator Laser GmbH ist für diese Folgen nicht 
verantwortlich.

**************************************************************************************


<Prev in Thread] Current Thread [Next in Thread>