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

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

[Uwe Kleine-König]

[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?

Best regards and thanks
Uwe

[1] I only checked a few files, so maybe there are more.
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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

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

[H Hartley Sweeten]

On Tuesday, February 09, 2010 9:52 AM, 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:

[snip]

> 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?

Would this fix the third variant?

	#define ... ((void __iomem *)0x12345678UL)

Regards,
Hartley

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

[Russell King - ARM Linux]

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.

The point is that on 64-bit architectures, a pointer may be representable
by an unsigned long, but not an int.

> Is this intended?  What is the preferred way to define iomem pointers?

Define a numerical address with a UL suffix is the simplest way.
--
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

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

[Josh Triplett]

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