On Sat, 20 Apr 2013 07:54:34 +0800 kbuild test robot <[email protected]> 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?
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 <[email protected]> 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
On Mon, Apr 22, 2013 at 4:56 PM, Andrew Morton
<[email protected]> 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);
}
/**
On Mon, Apr 22, 2013 at 07:56:19PM -0700, Christopher Li wrote:
> On Mon, Apr 22, 2013 at 4:56 PM, Andrew Morton
> <[email protected]> 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
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
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