Re: [next:akpm 798/1000] drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)

Re: [next:akpm 798/1000] drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)

[Andrew Morton]

On Sat, 20 Apr 2013 07:54:34 +0800 kbuild test robot <fengguang.wu@intel.com> wrote:

> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git akpm
> head:   c9941b7ec7840ad33f5822c7f238157558d40132
> commit: d5e42b5769899607e1e4b0c9200340d24f370e8c [798/1000] rtc: rtc-ds1286: use devm_*() functions
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)
>    drivers/rtc/rtc-ds1286.c:343:24:    expected void const *ptr
>    drivers/rtc/rtc-ds1286.c:343:24:    got unsigned int [noderef] [usertype] <asn:2>*rtcregs
> >> drivers/rtc/rtc-ds1286.c:344:36: sparse: incorrect type in argument 1 (different address spaces)
>    drivers/rtc/rtc-ds1286.c:344:36:    expected void const *ptr
>    drivers/rtc/rtc-ds1286.c:344:36:    got unsigned int [noderef] [usertype] <asn:2>*rtcregs
> 
> vim +343 drivers/rtc/rtc-ds1286.c
> 
>    337			return -ENODEV;
>    338		priv = devm_kzalloc(&pdev->dev, sizeof(struct ds1286_priv), GFP_KERNEL);
>    339		if (!priv)
>    340			return -ENOMEM;
>    341	
>    342		priv->rtcregs = devm_ioremap_resource(&pdev->dev, res);
>  > 343		if (IS_ERR(priv->rtcregs))
>  > 344			return PTR_ERR(priv->rtcregs);
>    345	
>    346		spin_lock_init(&priv->lock);
>    347		platform_set_drvdata(pdev, priv);

I think doing IS_ERR() and PTR_ERR() on __iomem pointers is a natural
thing, and we should be able to do this without adding call-site
trickery to make sparse happy.

Is there some sort of annotation which we can add to the
IS_ERR()/PTR_ERR() definitions so that sparse will stop warning about
this usage?

Re: [next:akpm 798/1000] drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)

[Fengguang Wu]

On Mon, Apr 22, 2013 at 04:56:29PM -0700, Andrew Morton wrote:
> On Sat, 20 Apr 2013 07:54:34 +0800 kbuild test robot <fengguang.wu@intel.com> wrote:
> 
> > tree:   git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git akpm
> > head:   c9941b7ec7840ad33f5822c7f238157558d40132
> > commit: d5e42b5769899607e1e4b0c9200340d24f370e8c [798/1000] rtc: rtc-ds1286: use devm_*() functions
> > 
> > 
> > sparse warnings: (new ones prefixed by >>)
> > 
> > >> drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)
> >    drivers/rtc/rtc-ds1286.c:343:24:    expected void const *ptr
> >    drivers/rtc/rtc-ds1286.c:343:24:    got unsigned int [noderef] [usertype] <asn:2>*rtcregs
> > >> drivers/rtc/rtc-ds1286.c:344:36: sparse: incorrect type in argument 1 (different address spaces)
> >    drivers/rtc/rtc-ds1286.c:344:36:    expected void const *ptr
> >    drivers/rtc/rtc-ds1286.c:344:36:    got unsigned int [noderef] [usertype] <asn:2>*rtcregs
> > 
> > vim +343 drivers/rtc/rtc-ds1286.c
> > 
> >    337			return -ENODEV;
> >    338		priv = devm_kzalloc(&pdev->dev, sizeof(struct ds1286_priv), GFP_KERNEL);
> >    339		if (!priv)
> >    340			return -ENOMEM;
> >    341	
> >    342		priv->rtcregs = devm_ioremap_resource(&pdev->dev, res);
> >  > 343		if (IS_ERR(priv->rtcregs))
> >  > 344			return PTR_ERR(priv->rtcregs);
> >    345	
> >    346		spin_lock_init(&priv->lock);
> >    347		platform_set_drvdata(pdev, priv);
> 
> I think doing IS_ERR() and PTR_ERR() on __iomem pointers is a natural
> thing, and we should be able to do this without adding call-site
> trickery to make sparse happy.
> 
> Is there some sort of annotation which we can add to the
> IS_ERR()/PTR_ERR() definitions so that sparse will stop warning about
> this usage?

If it's too hard to fix in sparse, I can add a check in my scripts,
ignoring all "parse: incorrect type in argument 1 (different address
spaces)" warnings in the IS_ERR/PTR_ERR lines.

Thanks,
Fengguang

Re: [next:akpm 798/1000] drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)

[Christopher Li]

On Mon, Apr 22, 2013 at 4:56 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> I think doing IS_ERR() and PTR_ERR() on __iomem pointers is a natural
> thing, and we should be able to do this without adding call-site
> trickery to make sparse happy.
>
> Is there some sort of annotation which we can add to the
> IS_ERR()/PTR_ERR() definitions so that sparse will stop warning about
> this usage?

Yes, the force attribute should silent the address check on conversion.

Can some one try this patch (totally untested).

Chris


diff --git a/include/linux/err.h b/include/linux/err.h
index f2edce2..d226a3c 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -26,17 +26,17 @@ static inline void * __must_check ERR_PTR(long error)

 static inline long __must_check PTR_ERR(const void *ptr)
 {
-       return (long) ptr;
+       return (__force long) ptr;
 }

 static inline long __must_check IS_ERR(const void *ptr)
 {
-       return IS_ERR_VALUE((unsigned long)ptr);
+       return IS_ERR_VALUE((__force unsigned long)ptr);
 }

 static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
 {
-       return !ptr || IS_ERR_VALUE((unsigned long)ptr);
+       return !ptr || IS_ERR_VALUE((__force unsigned long)ptr);
 }

 /**

Re: [next:akpm 798/1000] drivers/rtc/rtc-ds1286.c:343:24: sparse: incorrect type in argument 1 (different address spaces)

[Dan Carpenter]

On Mon, Apr 22, 2013 at 07:56:19PM -0700, Christopher Li wrote:
> On Mon, Apr 22, 2013 at 4:56 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > I think doing IS_ERR() and PTR_ERR() on __iomem pointers is a natural
> > thing, and we should be able to do this without adding call-site
> > trickery to make sparse happy.
> >
> > Is there some sort of annotation which we can add to the
> > IS_ERR()/PTR_ERR() definitions so that sparse will stop warning about
> > this usage?
> 
> Yes, the force attribute should silent the address check on conversion.
> 
> Can some one try this patch (totally untested).
> 

That didn't work.  It's the the void * in the parameter list that's
the problem.  We'd need to do something like the patch below:

Otherwise we could add "__ok_to_cast" thing to Sparse maybe?

regards,
dan carpenter

diff --git a/include/linux/err.h b/include/linux/err.h
index f2edce2..2cbe8fb 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -24,20 +24,23 @@ static inline void * __must_check ERR_PTR(long error)
 	return (void *) error;
 }
 
-static inline long __must_check PTR_ERR(const void *ptr)
+static inline long __must_check _PTR_ERR(const void *ptr)
 {
 	return (long) ptr;
 }
+#define PTR_ERR(x) _PTR_ERR((const void __force *)(x))
 
-static inline long __must_check IS_ERR(const void *ptr)
+static inline long __must_check _IS_ERR(const void *ptr)
 {
 	return IS_ERR_VALUE((unsigned long)ptr);
 }
+#define IS_ERR(x) _IS_ERR((const void __force *)(x))
 
-static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
+static inline long __must_check _IS_ERR_OR_NULL(const void *ptr)
 {
 	return !ptr || IS_ERR_VALUE((unsigned long)ptr);
 }
+#define IS_ERR_OR_NULL(x) _IS_ERR_OR_NULL((const void __force *)(x))
 
 /**
  * ERR_CAST - Explicitly cast an error-valued pointer to another pointer type
@@ -46,19 +49,21 @@ static inline long __must_check IS_ERR_OR_NULL(const void *ptr)
  * Explicitly cast an error-valued pointer to another pointer type in such a
  * way as to make it clear that's what's going on.
  */
-static inline void * __must_check ERR_CAST(const void *ptr)
+static inline void * __must_check _ERR_CAST(const void *ptr)
 {
 	/* cast away the const */
 	return (void *) ptr;
 }
+#define ERR_CAST(x) _ERR_CAST((const void __force *)(x))
 
-static inline int __must_check PTR_RET(const void *ptr)
+static inline int __must_check _PTR_RET(const void *ptr)
 {
 	if (IS_ERR(ptr))
 		return PTR_ERR(ptr);
 	else
 		return 0;
 }
+#define PTR_RET(x) _PTR_RET((const void __force *)(x))
 
 #endif
 

[PATCH] forced argument Was Re: sparse: incorrect type in argument 1 (different address spaces)

[Christopher Li]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On 04/22/2013 11:16 PM, Dan Carpenter wrote:
> That didn't work.  It's the the void * in the parameter list that's
> the problem.  We'd need to do something like the patch below:
> 
> Otherwise we could add "__ok_to_cast" thing to Sparse maybe?

Thanks for the insight. I make a small patch to test the __ok_to_cast
feature. The syntax is adding the force attribute to the argument
declaration.

it will look like this:
static inline long __must_check PTR_ERR( __force const void *ptr)

That means the "ptr" argument will perform a forced cast when receiving
the argument. It is OK to pass __iomem pointer to "ptr".

The example are in the patch. It need to patch both sparse and the
Linux tree.

What do you say?

Chris


[-- Attachment #2: 0001-Allow-forced-attribute-in-function-argument.patch --]
[-- Type: text/x-patch, Size: 2072 bytes --]

>From a0974ed0fc1e67c41608c780b748c205622956b8 Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Thu, 25 Apr 2013 18:09:43 -0700
Subject: [PATCH] Allow forced attribute in function argument

It will indicate this argument will skip the compatible check.
---
 evaluate.c             |  2 +-
 parse.c                |  1 +
 symbol.h               |  3 ++-
 validation/fored_arg.c | 18 ++++++++++++++++++
 4 files changed, 22 insertions(+), 2 deletions(-)
 create mode 100644 validation/fored_arg.c

diff --git a/evaluate.c b/evaluate.c
index 9f2c4ac..0dfa519 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2137,7 +2137,7 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
 				else
 					degenerate(expr);
 			}
-		} else {
+		} else if (!target->forced_arg){
 			static char where[30];
 			examine_symbol_type(target);
 			sprintf(where, "argument %d", i);
diff --git a/parse.c b/parse.c
index 45ffc10..890e56b 100644
--- a/parse.c
+++ b/parse.c
@@ -1841,6 +1841,7 @@ static struct token *parameter_declaration(struct token *token, struct symbol *s
 	sym->ctype = ctx.ctype;
 	sym->ctype.modifiers |= storage_modifiers(&ctx);
 	sym->endpos = token->pos;
+	sym->forced_arg = ctx.storage_class == SForced;
 	return token;
 }
 
diff --git a/symbol.h b/symbol.h
index 1e74579..1c6ad66 100644
--- a/symbol.h
+++ b/symbol.h
@@ -157,7 +157,8 @@ struct symbol {
 					expanding:1,
 					evaluated:1,
 					string:1,
-					designated_init:1;
+					designated_init:1,
+					forced_arg:1;
 			struct expression *array_size;
 			struct ctype ctype;
 			struct symbol_list *arguments;
diff --git a/validation/fored_arg.c b/validation/fored_arg.c
new file mode 100644
index 0000000..4ab7141
--- /dev/null
+++ b/validation/fored_arg.c
@@ -0,0 +1,18 @@
+/*
+ * check-name: Forced function argument type.
+ */
+
+#define __iomem	__attribute__((noderef, address_space(2)))
+#define __force __attribute__((force))
+
+static void foo(__force void * addr)
+{
+}
+
+
+static void bar(void)
+{
+	void __iomem  *a;
+	foo(a);
+}
+
-- 
1.8.1.4

[PATCH] forced argument Was Re: sparse: incorrect type in argument 1 (different address spaces)

[Christopher Li]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On 04/22/2013 11:16 PM, Dan Carpenter wrote:
> That didn't work.  It's the the void * in the parameter list that's
> the problem.  We'd need to do something like the patch below:
> 
> Otherwise we could add "__ok_to_cast" thing to Sparse maybe?

Thanks for the insight. I make a small patch to test the __ok_to_cast
feature. The syntax is adding the force attribute to the argument
declaration.

it will look like this:
static inline long __must_check PTR_ERR( __force const void *ptr)

That means the "ptr" argument will perform a forced cast when receiving
the argument. It is OK to pass __iomem pointer to "ptr".

The example are in the patch. It need to patch both sparse and the
Linux tree.

What do you say?

Chris


[-- Attachment #2: 0001-Allow-forced-attribute-in-function-argument.patch --]
[-- Type: text/x-patch, Size: 2071 bytes --]

From a0974ed0fc1e67c41608c780b748c205622956b8 Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Thu, 25 Apr 2013 18:09:43 -0700
Subject: [PATCH] Allow forced attribute in function argument

It will indicate this argument will skip the compatible check.
---
 evaluate.c             |  2 +-
 parse.c                |  1 +
 symbol.h               |  3 ++-
 validation/fored_arg.c | 18 ++++++++++++++++++
 4 files changed, 22 insertions(+), 2 deletions(-)
 create mode 100644 validation/fored_arg.c

diff --git a/evaluate.c b/evaluate.c
index 9f2c4ac..0dfa519 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2137,7 +2137,7 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
 				else
 					degenerate(expr);
 			}
-		} else {
+		} else if (!target->forced_arg){
 			static char where[30];
 			examine_symbol_type(target);
 			sprintf(where, "argument %d", i);
diff --git a/parse.c b/parse.c
index 45ffc10..890e56b 100644
--- a/parse.c
+++ b/parse.c
@@ -1841,6 +1841,7 @@ static struct token *parameter_declaration(struct token *token, struct symbol *s
 	sym->ctype = ctx.ctype;
 	sym->ctype.modifiers |= storage_modifiers(&ctx);
 	sym->endpos = token->pos;
+	sym->forced_arg = ctx.storage_class == SForced;
 	return token;
 }
 
diff --git a/symbol.h b/symbol.h
index 1e74579..1c6ad66 100644
--- a/symbol.h
+++ b/symbol.h
@@ -157,7 +157,8 @@ struct symbol {
 					expanding:1,
 					evaluated:1,
 					string:1,
-					designated_init:1;
+					designated_init:1,
+					forced_arg:1;
 			struct expression *array_size;
 			struct ctype ctype;
 			struct symbol_list *arguments;
diff --git a/validation/fored_arg.c b/validation/fored_arg.c
new file mode 100644
index 0000000..4ab7141
--- /dev/null
+++ b/validation/fored_arg.c
@@ -0,0 +1,18 @@
+/*
+ * check-name: Forced function argument type.
+ */
+
+#define __iomem	__attribute__((noderef, address_space(2)))
+#define __force __attribute__((force))
+
+static void foo(__force void * addr)
+{
+}
+
+
+static void bar(void)
+{
+	void __iomem  *a;
+	foo(a);
+}
+
-- 
1.8.1.4

Re: [PATCH] forced argument Was Re: sparse: incorrect type in argument 1 (different address spaces)

[Dan Carpenter]

On Thu, Apr 25, 2013 at 07:09:37PM -0700, Christopher Li wrote:
> On 04/22/2013 11:16 PM, Dan Carpenter wrote:
> > That didn't work.  It's the the void * in the parameter list that's
> > the problem.  We'd need to do something like the patch below:
> > 
> > Otherwise we could add "__ok_to_cast" thing to Sparse maybe?
> 
> Thanks for the insight. I make a small patch to test the __ok_to_cast
> feature. The syntax is adding the force attribute to the argument
> declaration.
> 
> it will look like this:
> static inline long __must_check PTR_ERR( __force const void *ptr)
> 
> That means the "ptr" argument will perform a forced cast when receiving
> the argument. It is OK to pass __iomem pointer to "ptr".
> 
> The example are in the patch. It need to patch both sparse and the
> Linux tree.
> 
> What do you say?

That's looks great.  :)

I tested a patched kernel with an unpatched kernel as well and that
doesn't cause any new problems.

regards,
dan carpenter