Re: [PATCH 01/14] ARM: LPC32XX: Initial architecture header files

[Josh Triplett <josh@joshtriplett.org>]

On Tue, Feb 09, 2010 at 05:52:23PM +0100, Uwe Kleine-König wrote:
> [Added linux-sparse@vger.kernel.org to Cc:]
> 
> On Tue, Feb 09, 2010 at 09:59:34AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Feb 09, 2010 at 10:31:29AM +0100, Uwe Kleine-König wrote:
> > > > +#define io_p2v(x)	((void __iomem *) (unsigned long) IO_ADDRESS(x))
> > > Is this cast to unsigned long needed?  AFAIK IO_ADDRESS(x) has
> > > type unsigned for x in { 0x0 ... 0xffffffff } (provided that int uses a
> > > 32 bit 2s-complement representation).  If unsigned long is really
> > > needed, maybe put it into the IO_ADDRESS macro?
> > 
> > int -> void __iomem * = sparse warning
> > unsigned long -> void __iomem * = no sparse warning
> Ah, OK, I see.  But IMHO it's a poor reason to add the cast.  Either
> the cast is necessary/recommended or sparse is wrong.  In the first case
> the reasoning shouldn't have to do with sparse, in the latter sparse
> should be fixed.
>
> I found the responsible code in sparse.  It reads:
> 
> 	static struct symbol *evaluate_cast(struct expression *expr)
> 	{
> 		int as1 = 0, as2 = 0;
> 		...
> 		ctype = examine_symbol_type(expr->cast_type);
> 		...
> 		class1 = classify_type(ctype, &t1);
> 		...
> 		if (t1 == &ulong_ctype)
> 			as1 = -1;
> 		else if (class1 == TYPE_PTR) {
> 			examine_pointer_target(t1);
> 			as1 = t1->ctype.as;
> 		}
> 		
> 		if (t2 == &ulong_ctype)
> 			as2 = -1;
> 		else if (class2 == TYPE_PTR) {
> 			examine_pointer_target(t2);
> 			as2 = t2->ctype.as;
> 		}
> 		...
> 		if (as1 > 0 && !as2 &&
> 		    !is_null_pointer_constant(target) && Wcast_to_as)
> 			warning(expr->pos,
> 				"cast adds address space to expression (<asn:%d>)", as1);
> 
> so it seems to be explicitly allowed to make a pointer in any address
> space from an unsigned long, but not from a (signed or unsigned) int.
> Unfortunately there is no comment describing why unsigned long is
> allowed.
> 
> Is this intended?  What is the preferred way to define iomem pointers?
> 
> I found the following variants in the kernel[1]
> 
> 	#define ...	((void __iomem *)(unsigned long)0x12345678)
> 	#define ...	((void __iomem __force *)0x12345678)
> 	#define ...	((void __iomem *)0x12345678)
> 
> where __iomem is defined as __attribute__((noderef, address_space(2)))
> for sparse.
> 
> The first variant with the extra cast to unsigned long seems unnecessary
> long, the third results in the "cast adds address space to expression"
> warning.
> 
> So what do you recommend?

I don't know the reasoning behind allowing casts from unsigned long,
except perhaps to allow creating pointer literals, but explicitly
casting by way of unsigned long seems wrong.  In general, the couple of
places that legitimately convert addresses between address spaces should
use __force.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html