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

[Ulrich Eckhardt <eckhardt@satorlaser.com>]

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.

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