From patchwork Fri May 27 20:41:01 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dan Williams X-Patchwork-Id: 9139007 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9FB3460759 for ; Fri, 27 May 2016 20:41:23 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9041527CEC for ; Fri, 27 May 2016 20:41:23 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 84914282ED; Fri, 27 May 2016 20:41:23 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_NONE, RDNS_NONE, T_DKIM_INVALID autolearn=no version=3.3.1 Received: from ml01.01.org (unknown [198.145.21.10]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 20FE827CEC for ; Fri, 27 May 2016 20:41:22 +0000 (UTC) Received: from [127.0.0.1] (localhost [IPv6:::1]) by ml01.01.org (Postfix) with ESMTP id B5FF91A1F75; Fri, 27 May 2016 13:41:14 -0700 (PDT) X-Original-To: linux-nvdimm@lists.01.org Delivered-To: linux-nvdimm@lists.01.org Received: from mail-oi0-x235.google.com (mail-oi0-x235.google.com [IPv6:2607:f8b0:4003:c06::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 105FA1A1F75 for ; Fri, 27 May 2016 13:41:14 -0700 (PDT) Received: by mail-oi0-x235.google.com with SMTP id k23so191530262oih.0 for ; Fri, 27 May 2016 13:41:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=ElX8X6CT36BBNoaBc4eN0dP0s8mZOtTcPqP6rO+/pi8=; b=0AlwNe7/B+Gyo/nmPqem9baFyGMmMrq88sN60SjnT3VGgOVyPa9YtkqEdBRQC+oh30 78R5WIvpj5s8ZXGiMwTn4fK7QZtgmL/UviTj9XdbTFR/aMsEtPRdSxxvIqmAsSfBKTBo BeI+eZvU4JCvXlnLpmDqiuOuRldpb2+8krW8eWePpGPosOFw8EuZPSbTt63YRKUgWsJG VXd7H2g51evOfhxFwD3MJ6XIkUVymOkyqeFe4xtoDwYLNDBgq0qbILxvbW+iDerGLLIQ 7q3WMRgAzxWgHDD3ir43VXkShkLX1UWBogan8opu/gv8YuOFng0VoVlEZ09hQZ/sl14+ V5pQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=ElX8X6CT36BBNoaBc4eN0dP0s8mZOtTcPqP6rO+/pi8=; b=GhKyrY2zp3S8FuURGgBtUADxq21D/HXFwicQkP+6HvTWPNErKDn2yDM9ydFkUOHkvg boxGNBD/Uo+0qDXNXKXiW67gWlPl6e2cOFms4zjWZDLoaPBTuuBlAwfL8pq8fXbDofhs 3FhkPGyLusWNIV6xGdznwPLnIKmtMbihnr27bxl4vE2AA2iMf77Sv/zDaTkXI6+4jtDv D6FLrUqGDySvME3KFWl2QnIjCGRDUQG2WY6nJRu2riYHV7P333vKjtKyh3Iqd/X37e7v M+EhPMIFPX0AJ8TtME/rLyAbGweD5vmO3++yVLyTOtXhDCQDlsp9irpkw7tmZYqP/ig+ BuHQ== X-Gm-Message-State: ALyK8tIM2X8RZpLtznucPO97EkXJqJYrojdaQ1HL54RmykXyld+pdmdZmFO3xEdCzQb6u002fTZLf1PQj1/sor9p MIME-Version: 1.0 X-Received: by 10.202.218.84 with SMTP id r81mr11039772oig.49.1464381661255; Fri, 27 May 2016 13:41:01 -0700 (PDT) Received: by 10.157.47.37 with HTTP; Fri, 27 May 2016 13:41:01 -0700 (PDT) In-Reply-To: References: Date: Fri, 27 May 2016 13:41:01 -0700 Message-ID: Subject: Re: IS_ERR_VALUE misuses From: Dan Williams To: Linus Torvalds X-BeenThere: linux-nvdimm@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Linux-nvdimm developer list." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Linux ACPI , "linux-nvdimm@lists.01.org" , "Rafael J. Wysocki" , Len Brown Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" X-Virus-Scanned: ClamAV using ClamSMTP On Fri, May 27, 2016 at 1:15 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. > > 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? { $ 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)) 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)