From patchwork Thu May 9 15:39:33 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tony Lindgren X-Patchwork-Id: 2544531 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id CF1DCDF24C for ; Thu, 9 May 2013 15:39:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753558Ab3EIPjh (ORCPT ); Thu, 9 May 2013 11:39:37 -0400 Received: from mho-02-ewr.mailhop.org ([204.13.248.72]:57911 "EHLO mho-02-ewr.mailhop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751222Ab3EIPjg (ORCPT ); Thu, 9 May 2013 11:39:36 -0400 Received: from c-50-131-214-131.hsd1.ca.comcast.net ([50.131.214.131] helo=localhost.localdomain) by mho-02-ewr.mailhop.org with esmtpa (Exim 4.72) (envelope-from ) id 1UaSwR-000EBa-V9; Thu, 09 May 2013 15:39:36 +0000 Received: from Mutt by mutt-smtp-wrapper.pl 1.2 (www.zdo.com/articles/mutt-smtp-wrapper.shtml) X-Mail-Handler: Dyn Standard SMTP by Dyn X-Originating-IP: 50.131.214.131 X-Report-Abuse-To: abuse@dyndns.com (see http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse reporting information) X-MHO-User: U2FsdGVkX193nZhLcY9Q2Uuyo9+mR59T Date: Thu, 9 May 2013 08:39:33 -0700 From: Tony Lindgren To: Russell King - ARM Linux Cc: linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: arch/arm/mach-omap2/id.c - IS_ERR_OR_NULL() Message-ID: <20130509153933.GA31554@atomide.com> References: <20130509090911.GM21614@n2100.arm.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130509090911.GM21614@n2100.arm.linux.org.uk> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org * Russell King - ARM Linux [130509 02:14]: > So, I eliminated all but a very few of these from arch/arm, and I notice > today that there's a new couple of instances introduced by: Sorry I should have noticed that fnord, I had it in my muttrc but had a an unnecessary \ in the expression so it did not work. Here's a patch to fix the issue. Hmm maybe we could redefine IS_ERR_OR_NULL as error in some ARM header as long as drivers don't include it? Regards, Tony From: Tony Lindgren Date: Thu, 9 May 2013 08:27:25 -0700 Subject: [PATCH] ARM: OMAP2+: Remove bogus IS_ERR_OR_NULL checking from id.c Commit 6770b211 (ARM: OMAP2+: Export SoC information to userspace) had some broken return value handling as noted by Russell King: + soc_dev = soc_device_register(soc_dev_attr); + if (IS_ERR_OR_NULL(soc_dev)) { + kfree(soc_dev_attr); + return; + } + + parent = soc_device_to_device(soc_dev); + if (!IS_ERR_OR_NULL(parent)) + device_create_file(parent, &omap_soc_attr); This is nonsense. For the first, IS_ERR() is sufficient. For the second, tell me what error checking is required in the return value of this function: struct device *soc_device_to_device(struct soc_device *soc_dev) { return &soc_dev->dev; } when you've already determined that the passed soc_dev is a valid pointer. If you read the comments against the prototype: /** * soc_device_to_device - helper function to fetch struct device * @soc: Previously registered SoC device container */ struct device *soc_device_to_device(struct soc_device *soc); if "soc" is valid, it means the "previously registered SoC device container" must have succeeded and that can only happen if the struct device has been registered. Ergo, there will always be a valid struct device pointer for any registered SoC device container. Therefore, if soc_device_register() succeeds, then the return value from soc_device_to_device() will always be valid and no error checking of it is required. Simples. The rule as ever applies here: get to know the APIs your using and don't fumble around in the dark hoping that you'll get this stuff right. Fix it as noted by Russell. Reported-by: Russell King Signed-off-by: Tony Lindgren --- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c index 9bc5a18..1272c41 100644 --- a/arch/arm/mach-omap2/id.c +++ b/arch/arm/mach-omap2/id.c @@ -648,13 +648,12 @@ void __init omap_soc_device_init(void) soc_dev_attr->revision = soc_rev; soc_dev = soc_device_register(soc_dev_attr); - if (IS_ERR_OR_NULL(soc_dev)) { + if (IS_ERR(soc_dev)) { kfree(soc_dev_attr); return; } parent = soc_device_to_device(soc_dev); - if (!IS_ERR_OR_NULL(parent)) - device_create_file(parent, &omap_soc_attr); + device_create_file(parent, &omap_soc_attr); } #endif /* CONFIG_SOC_BUS */