1:15 p.m.
This is just a heads-up: for some reason the acpi layer and nvdimm use
the IS_ERR_VALUE() macro, and they use it incorrectly.
To see warnings about it, change the macro from
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
to do a cast to a pointer and back (ie make the "(x)" part be
"(unsigned long)(void *)(x)" instead, which then will cause warnings
like
warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]
when passed an "int" argument.
The reason "int" arguments are wrong is that the macro really is
designed to test the upper range of a pointer value. It happens to
work for signed integers too, but looking at the users, pretty much
none of them are right. The ACPI and nvdimm users are all about the
perfectly standard "zero for success, negative error code for
failure", and so using
if (IS_ERROR_VALUE(rc))
return rc;
is just plain garbage. The code generally should just do
if (rc)
return rc;
which is simpler, smaller, and generates better code.
This bug seems to have been so common in the power management code
that we even have a coccinelle script for it. But for some reason
several uses remain in acpi_debug.c and now there are cases in
drivers/nvdimm/ too.
There are random various crap cases like that elsewhere too, but acpi
and nvdimm were just more dense with this bug than most other places.
The bug isn't fatal - as mentioned, IS_ERROR_VALUE() _does_ actually
work on the range of integers that are normal errors, but it's
pointless and actively misleading, and it's not meant for that use. So
it just adds complexity and worse code generation for no actual gain.
I noticed this when I was looking at similar idiocy in fs/gfs2, where
the code in question caused warnings (for other reasons, but the main
reason was "code too complex for gcc to understand it")
Linus
1:41 p.m.
On Fri, May 27, 2016 at 1:15 PM, Linus Torvalds
<torvalds(a)linux-foundation.org> wrote:
This is just a heads-up: for some reason the acpi layer and nvdimm
use
the IS_ERR_VALUE() macro, and they use it incorrectly.
To see warnings about it, change the macro from
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
to do a cast to a pointer and back (ie make the "(x)" part be
"(unsigned long)(void *)(x)" instead, which then will cause warnings
like
warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]
when passed an "int" argument.
The reason "int" arguments are wrong is that the macro really is
designed to test the upper range of a pointer value. It happens to
work for signed integers too, but looking at the users, pretty much
none of them are right. The ACPI and nvdimm users are all about the
perfectly standard "zero for success, negative error code for
failure", and so using
if (IS_ERROR_VALUE(rc))
return rc;
is just plain garbage. The code generally should just do
if (rc)
return rc;
which is simpler, smaller, and generates better code.
This bug seems to have been so common in the power management code
that we even have a coccinelle script for it. But for some reason
several uses remain in acpi_debug.c and now there are cases in
drivers/nvdimm/ too.
There are random various crap cases like that elsewhere too, but acpi
and nvdimm were just more dense with this bug than most other places.
The bug isn't fatal - as mentioned, IS_ERROR_VALUE() _does_ actually
work on the range of integers that are normal errors, but it's
pointless and actively misleading, and it's not meant for that use. So
it just adds complexity and worse code generation for no actual gain.
I noticed this when I was looking at similar idiocy in fs/gfs2, where
the code in question caused warnings (for other reasons, but the main
reason was "code too complex for gcc to understand it")
So, I recompiled with that change and didn't see any new warnings.
While "make coccicheck" did point out the following clean up, I did
not see where drivers/nvdimm/ passes anything but a pointer to IS_ERR,
what am I missing?
diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
index 8b2e3c4fb0ad..9997ff94a132 100644
--- a/drivers/nvdimm/claim.c
+++ b/drivers/nvdimm/claim.c
@@ -266,9 +266,8 @@ int devm_nsio_enable(struct device *dev, struct
nd_namespace_io *nsio)
nsio->addr = devm_memremap(dev, res->start, resource_size(res),
ARCH_MEMREMAP_PMEM);
- if (IS_ERR(nsio->addr))
- return PTR_ERR(nsio->addr);
- return 0;
+
+ return PTR_ERR_OR_ZERO(nsio->addr);
}
EXPORT_SYMBOL_GPL(devm_nsio_enable);
diff --git a/include/linux/err.h b/include/linux/err.h
index 56762ab41713..1e3558845e4c 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -18,7 +18,7 @@
#ifndef __ASSEMBLY__
-#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
+#define IS_ERR_VALUE(x) unlikely((unsigned long)(void *)(x) >=
(unsigned long)-MAX_ERRNO)
static inline void * __must_check ERR_PTR(long error)
{
$ git grep -n IS_ERR drivers/nvdimm/
drivers/nvdimm/blk.c:317: if (IS_ERR(ndns))
drivers/nvdimm/btt.c:209: if (IS_ERR_OR_NULL(d))
drivers/nvdimm/btt.c:241: if (IS_ERR_OR_NULL(btt->debugfs_dir))
drivers/nvdimm/btt.c:1424: if (IS_ERR_OR_NULL(debugfs_root))
drivers/nvdimm/bus.c:398: if (IS_ERR(dev)) {
drivers/nvdimm/bus.c:848: if (IS_ERR(nd_class)) {
drivers/nvdimm/pmem.c:223: if (IS_ERR(altmap))
drivers/nvdimm/pmem.c:277: if (IS_ERR(addr))
drivers/nvdimm/pmem.c:318: if (IS_ERR(ndns))
1:45 p.m.
[ Adding Srinivas ]
On Fri, May 27, 2016 at 1:41 PM, Dan Williams <dan.j.williams(a)intel.com> wrote:
On Fri, May 27, 2016 at 1:15 PM, Linus Torvalds
<torvalds(a)linux-foundation.org> wrote:
> This is just a heads-up: for some reason the acpi layer and nvdimm use
> the IS_ERR_VALUE() macro, and they use it incorrectly.
>
> To see warnings about it, change the macro from
>
> #define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
>
> to do a cast to a pointer and back (ie make the "(x)" part be
> "(unsigned long)(void *)(x)" instead, which then will cause warnings
> like
>
> warning: cast to pointer from integer of different size
> [-Wint-to-pointer-cast]
>
> when passed an "int" argument.
>
> The reason "int" arguments are wrong is that the macro really is
> designed to test the upper range of a pointer value. It happens to
> work for signed integers too, but looking at the users, pretty much
> none ofdrivers/nvmem them are right. The ACPI and nvdimm users are all about the
> perfectly standard "zero for success, negative error code for
> failure", and so using
>
> if (IS_ERROR_VALUE(rc))
> return rc;
>
> is just plain garbage. The code generally should just do
>
> if (rc)
> return rc;
>
> which is simpler, smaller, and generates better code.
>
> This bug seems to have been so common in the power management code
> that we even have a coccinelle script for it. But for some reason
> several uses remain in acpi_debug.c and now there are cases in
> drivers/nvdimm/ too.
>
> There are random various crap cases like that elsewhere too, but acpi
> and nvdimm were just more dense with this bug than most other places.
>
> The bug isn't fatal - as mentioned, IS_ERROR_VALUE() _does_ actually
> work on the range of integers that are normal errors, but it's
> pointless and actively misleading, and it's not meant for that use. So
> it just adds complexity and worse code generation for no actual gain.
>
> I noticed this when I was looking at similar idiocy in fs/gfs2, where
> the code in question caused warnings (for other reasons, but the main
> reason was "code too complex for gcc to understand it")
>
So, I recompiled with that change and didn't see any new warnings.
While "make coccicheck" did point out the following clean up, I did
not see where drivers/nvdimm/ passes anything but a pointer to IS_ERR,
what am I missing?
Ah, it looks like this feedback is meant for drivers/nvmem/ not drivers/nvdimm/
drivers/nvmem/core.c: In function ‘bin_attr_nvmem_read’:
drivers/nvmem/core.c:116:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘bin_attr_nvmem_write’:
drivers/nvmem/core.c:150:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_add_cells’:
drivers/nvmem/core.c:369:42: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘__nvmem_cell_read’:
drivers/nvmem/core.c:966:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_cell_read’:
drivers/nvmem/core.c:1001:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_cell_write’:
drivers/nvmem/core.c:1086:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_device_cell_read’:
drivers/nvmem/core.c:1114:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c:1118:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_device_cell_write’:
drivers/nvmem/core.c:1144:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_device_read’:
drivers/nvmem/core.c:1173:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
drivers/nvmem/core.c: In function ‘nvmem_device_write’:
drivers/nvmem/core.c:1201:41: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
5:13 a.m.
On Friday, May 27, 2016 01:15:12 PM Linus Torvalds wrote:
This is just a heads-up: for some reason the acpi layer and nvdimm
use
the IS_ERR_VALUE() macro, and they use it incorrectly.
To see warnings about it, change the macro from
#define IS_ERR_VALUE(x) unlikely((x) >= (unsigned long)-MAX_ERRNO)
to do a cast to a pointer and back (ie make the "(x)" part be
"(unsigned long)(void *)(x)" instead, which then will cause warnings
like
warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]
when passed an "int" argument.
The reason "int" arguments are wrong is that the macro really is
designed to test the upper range of a pointer value. It happens to
work for signed integers too, but looking at the users, pretty much
none of them are right. The ACPI and nvdimm users are all about the
perfectly standard "zero for success, negative error code for
failure", and so using
if (IS_ERROR_VALUE(rc))
return rc;
is just plain garbage. The code generally should just do
if (rc)
return rc;
which is simpler, smaller, and generates better code.
This bug seems to have been so common in the power management code
that we even have a coccinelle script for it. But for some reason
several uses remain in acpi_debug.c and now there are cases in
drivers/nvdimm/ too.
There are random various crap cases like that elsewhere too, but acpi
and nvdimm were just more dense with this bug than most other places.
Under drivers/acpi/ I could only find IS_ERR_VALUE() in acpi_dbg.c, but
those instances should be removed by the Arnd's patch if I'm not mistaken.
Thanks,
Rafael
12:15 p.m.
On Sat, May 28, 2016 at 5:13 AM, Rafael J. Wysocki <rjw(a)rjwysocki.net> wrote:
Under drivers/acpi/ I could only find IS_ERR_VALUE() in acpi_dbg.c, but
those instances should be removed by the Arnd's patch if I'm not mistaken.
Yes, I only saw that Arnd had a patch for this and asked him to just
send it to me after I had already looked at this and sent the email
out..
The acpi/nvdimm misuses are gone now, although other suspect uses of
that macro do remain..
Linus
6:01 p.m.
Hi, Rafael, Linus, Arnd
The instances in drivers/acpi/acpi_dbg.c were made by me.
Sorry for the issue and thanks for fixing it for me.
Best regards
-Lv
From: linus971(a)gmail.com [mailto:linus971@gmail.com] On Behalf Of
Linus Torvalds
Subject: Re: IS_ERR_VALUE misuses
On Sat, May 28, 2016 at 5:13 AM, Rafael J. Wysocki <rjw(a)rjwysocki.net>
wrote:
>
> Under drivers/acpi/ I could only find IS_ERR_VALUE() in acpi_dbg.c, but
> those instances should be removed by the Arnd's patch if I'm not
mistaken.
Yes, I only saw that Arnd had a patch for this and asked him to just
send it to me after I had already looked at this and sent the email
out..
The acpi/nvdimm misuses are gone now, although other suspect uses of
that macro do remain..
Linus
1699
days inactive
1702
days old
5 comments
4 participants
participants (4)
-
Dan Williams -
Linus Torvalds -
Rafael J. Wysocki -
Zheng, Lv