On Fri, May 27, 2016 at 1:15 PM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> 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))
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
