From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752170Ab1AXUJO (ORCPT ); Mon, 24 Jan 2011 15:09:14 -0500 Received: from mgw2.diku.dk ([130.225.96.92]:59295 "EHLO mgw2.diku.dk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751420Ab1AXUJN (ORCPT ); Mon, 24 Jan 2011 15:09:13 -0500 Date: Mon, 24 Jan 2011 21:09:07 +0100 (CET) From: Julia Lawall To: Vasiliy Kulikov Cc: Ryan Mallon , Russell King , kernel-janitors@vger.kernel.org, Nicolas Ferre , Jean-Christophe PLAGNIOL-VILLARD , Andrew Victor , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test In-Reply-To: <20110124200515.GA30963@albatros> Message-ID: References: <1295898922-18822-1-git-send-email-julia@diku.dk> <1295898922-18822-3-git-send-email-julia@diku.dk> <4D3DD964.9020107@bluewatersys.com> <20110124200515.GA30963@albatros> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 24 Jan 2011, Vasiliy Kulikov wrote: > On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote: > > On Tue, 25 Jan 2011, Ryan Mallon wrote: > > > > > On 01/25/2011 08:55 AM, Julia Lawall wrote: > > > > @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char > > > > { > > > > struct clk *clk = clk_get(NULL, id); > > > > > > > > - if (!dev || !clk || !IS_ERR(clk_get(dev, func))) > > > > + if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func))) > > > > return; > > > > > > I think we want: > > > > > > if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func))) > > > return; > > > > > > Since it is valid to return a NULL clk, and we don't want to try and > > > dereference it if that is the case. > > > > Looking at the given defintion of clk_get, I can't see how that could > > happen: > > clk_get() is defined per-architecture, sometimes it is NULL only. In this case there is a definition in the same file, so it doesn't seem necessary to worry about possible other ones. Unless there is some goal in the future to remove the local one? julia From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Date: Mon, 24 Jan 2011 20:09:07 +0000 Subject: Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test Message-Id: List-Id: References: <1295898922-18822-1-git-send-email-julia@diku.dk> <1295898922-18822-3-git-send-email-julia@diku.dk> <4D3DD964.9020107@bluewatersys.com> <20110124200515.GA30963@albatros> In-Reply-To: <20110124200515.GA30963@albatros> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-arm-kernel@lists.infradead.org On Mon, 24 Jan 2011, Vasiliy Kulikov wrote: > On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote: > > On Tue, 25 Jan 2011, Ryan Mallon wrote: > > > > > On 01/25/2011 08:55 AM, Julia Lawall wrote: > > > > @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char > > > > { > > > > struct clk *clk = clk_get(NULL, id); > > > > > > > > - if (!dev || !clk || !IS_ERR(clk_get(dev, func))) > > > > + if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func))) > > > > return; > > > > > > I think we want: > > > > > > if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func))) > > > return; > > > > > > Since it is valid to return a NULL clk, and we don't want to try and > > > dereference it if that is the case. > > > > Looking at the given defintion of clk_get, I can't see how that could > > happen: > > clk_get() is defined per-architecture, sometimes it is NULL only. In this case there is a definition in the same file, so it doesn't seem necessary to worry about possible other ones. Unless there is some goal in the future to remove the local one? julia From mboxrd@z Thu Jan 1 00:00:00 1970 From: julia@diku.dk (Julia Lawall) Date: Mon, 24 Jan 2011 21:09:07 +0100 (CET) Subject: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test In-Reply-To: <20110124200515.GA30963@albatros> References: <1295898922-18822-1-git-send-email-julia@diku.dk> <1295898922-18822-3-git-send-email-julia@diku.dk> <4D3DD964.9020107@bluewatersys.com> <20110124200515.GA30963@albatros> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Mon, 24 Jan 2011, Vasiliy Kulikov wrote: > On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote: > > On Tue, 25 Jan 2011, Ryan Mallon wrote: > > > > > On 01/25/2011 08:55 AM, Julia Lawall wrote: > > > > @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char > > > > { > > > > struct clk *clk = clk_get(NULL, id); > > > > > > > > - if (!dev || !clk || !IS_ERR(clk_get(dev, func))) > > > > + if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func))) > > > > return; > > > > > > I think we want: > > > > > > if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func))) > > > return; > > > > > > Since it is valid to return a NULL clk, and we don't want to try and > > > dereference it if that is the case. > > > > Looking at the given defintion of clk_get, I can't see how that could > > happen: > > clk_get() is defined per-architecture, sometimes it is NULL only. In this case there is a definition in the same file, so it doesn't seem necessary to worry about possible other ones. Unless there is some goal in the future to remove the local one? julia