From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 15DB8C433E6 for ; Mon, 1 Mar 2021 07:13:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DBAE764E54 for ; Mon, 1 Mar 2021 07:13:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232408AbhCAHM6 (ORCPT ); Mon, 1 Mar 2021 02:12:58 -0500 Received: from mga07.intel.com ([134.134.136.100]:24872 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232378AbhCAHM2 (ORCPT ); Mon, 1 Mar 2021 02:12:28 -0500 IronPort-SDR: Q8aSsrMocQFKD9cYWi8+Z8Qo/ORr6YXw8C4SXRBYNzQtNgU02muMpcWLbfdgr/HirsiKEWpbLw c1u1aHroKnTw== X-IronPort-AV: E=McAfee;i="6000,8403,9909"; a="250432278" X-IronPort-AV: E=Sophos;i="5.81,214,1610438400"; d="scan'208";a="250432278" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Feb 2021 23:10:41 -0800 IronPort-SDR: /OugGBR8rq5lqEjdZkAUSg9yc1F4w7ipJkCTgTmnf3ciFPsl1O78JsfAcuoQjOoHABUNi24xuH nTBUd5pH2URA== X-IronPort-AV: E=Sophos;i="5.81,214,1610438400"; d="scan'208";a="517326248" Received: from paasikivi.fi.intel.com ([10.237.72.42]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Feb 2021 23:10:39 -0800 Received: from paasikivi.fi.intel.com (localhost [127.0.0.1]) by paasikivi.fi.intel.com (Postfix) with SMTP id C065C202DD; Mon, 1 Mar 2021 09:10:37 +0200 (EET) Date: Mon, 1 Mar 2021 09:10:37 +0200 From: Sakari Ailus To: Dan Carpenter Cc: Colin King , Mauro Carvalho Chehab , Arnd Bergmann , Srinivas Kandagatla , linux-media@vger.kernel.org, kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] media: i2c: adp1653: fix error handling from a call to adp1653_get_fault Message-ID: <20210301071037.GP3@paasikivi.fi.intel.com> References: <20210226232229.1076199-1-colin.king@canonical.com> <20210227101719.GG2087@kadam> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210227101719.GG2087@kadam> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Dan, Colin, On Sat, Feb 27, 2021 at 01:17:20PM +0300, Dan Carpenter wrote: > On Fri, Feb 26, 2021 at 11:22:29PM +0000, Colin King wrote: > > From: Colin Ian King > > > > The error check on rval from the call to adp1653_get_fault currently > > returns if rval is non-zero. This appears to be incorrect as the > > following if statement checks for various bit settings in rval so > > clearly rval is expected to be non-zero at that point. Coverity > > flagged the if statement up as deadcode. Fix this so the error > > return path only occurs when rval is negative. > > > > Addresses-Coverity: ("Logically dead code") > > Fixes: 287980e49ffc ("remove lots of IS_ERR_VALUE abuses") > > Signed-off-by: Colin Ian King > > --- > > drivers/media/i2c/adp1653.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > > index 522a0b10e415..1a4878385394 100644 > > --- a/drivers/media/i2c/adp1653.c > > +++ b/drivers/media/i2c/adp1653.c > > @@ -170,7 +170,7 @@ static int adp1653_set_ctrl(struct v4l2_ctrl *ctrl) > > int rval; > > > > rval = adp1653_get_fault(flash); > > - if (rval) > > + if (rval < 0) > > return rval; > > This is good, but all the other callers need to fixed as well: > > > 140 static int adp1653_get_ctrl(struct v4l2_ctrl *ctrl) > 141 { > 142 struct adp1653_flash *flash = > 143 container_of(ctrl->handler, struct adp1653_flash, ctrls); > 144 int rval; > 145 > 146 rval = adp1653_get_fault(flash); > 147 if (rval) > 148 return rval; > 149 > 150 ctrl->cur.val = 0; > 151 > 152 if (flash->fault & ADP1653_REG_FAULT_FLT_SCP) > ^^^^^^^^^^^^ > flash->fault is the equivalent of "rval" for non-negative returns so > this condition can never be true. We should never be returning these > weird firmware ADP1653_REG_FAULT_FLT_SCP fault codes to the v4l2 layers. I think this could be fixed and cleaned up by always retuning zero on success, and checking for flash->faults while holding the mutex in adp1653_init_device. I could fix that, too, just let me know... -- Regards, Sakari Ailus