| Message ID | CAPcyv4hpZENy-bh_4a_fAar3Ufe4g6Fxd2mpazCYn_xQJAwxKg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[ Adding Srinivas ] On Fri, May 27, 2016 at 1:41 PM, Dan Williams <dan.j.williams@intel.com> wrote: > On Fri, May 27, 2016 at 1:15 PM, Linus Torvalds > <torvalds@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]
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)