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