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=-2.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=no 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 A75E8C433E0 for ; Wed, 1 Jul 2020 12:31:21 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 72E55206CB for ; Wed, 1 Jul 2020 12:31:21 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Tjs95Fjk" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 72E55206CB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=PX0oGy+4znzHe/MHweSYsAlRt+/3OH2AghISX8p2gbw=; b=Tjs95FjkXatfn6s2WLx5NruQZ VmVa44pqs03DqLEsnvJw3MnGV171zkQ/FG+X8CZOe6QI9wGLRRrHsjRgQ9sKrhHeokKQlh1Aam8br xwi/qRXf4zwDcn2ptYACIC3Z2HSbahvI5h3PWia/QtJmtOMHLleNqXavd1K51VpaBDZeuvLd/OiZD LpzxQF9HmSFrA9+XQpkXGLfKhpPd+a390e3IMbtWFd7YtJfDyd1OVTuNgCG0fHFTh7KG2BoRXPH9X 7+jZBtvSa5G7K40alPdAnzdNRG2XvcTNqexmWTMvAqEm1QwPPL5a8cx1ZAM9UZ9tPoSRjqN77KsGT OrEPLHhKw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqbrt-0003Od-OY; Wed, 01 Jul 2020 12:29:37 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqbrq-0003Np-O9 for linux-arm-kernel@lists.infradead.org; Wed, 01 Jul 2020 12:29:35 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5156930E; Wed, 1 Jul 2020 05:29:31 -0700 (PDT) Received: from e119603-lin.cambridge.arm.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B742F3F73C; Wed, 1 Jul 2020 05:29:30 -0700 (PDT) Date: Wed, 1 Jul 2020 13:29:24 +0100 From: Cristian Marussi To: Dan Carpenter Subject: Re: [bug report] firmware: arm_scmi: Add notification callbacks-registration Message-ID: <20200701122639.GA31379@e119603-lin.cambridge.arm.com> References: <20200630175718.GB34614@mwanda> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200630175718.GB34614@mwanda> User-Agent: Mutt/1.5.24 (2015-08-30) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200701_082934_885129_0E8DC070 X-CRM114-Status: GOOD ( 27.47 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jun 30, 2020 at 08:57:18PM +0300, Dan Carpenter wrote: > Hello Cristian Marussi, Hi Dan Carpenter thanks for the feedback first of all. > > The patch 5b352c537930: "firmware: arm_scmi: Add notification > callbacks-registration" from Jun 19, 2020, leads to the following > static checker warning: > > drivers/firmware/arm_scmi/notify.c:1267 scmi_register_notifier() > warn: passing zero to 'PTR_ERR' > > drivers/firmware/arm_scmi/notify.c > 1248 static int scmi_register_notifier(const struct scmi_handle *handle, > 1249 u8 proto_id, u8 evt_id, u32 *src_id, > 1250 struct notifier_block *nb) > 1251 { > 1252 int ret = 0; > 1253 u32 evt_key; > 1254 struct scmi_event_handler *hndl; > 1255 struct scmi_notify_instance *ni; > 1256 > 1257 /* Ensure notify_priv is updated */ > 1258 smp_rmb(); > 1259 if (unlikely(!handle->notify_priv)) > 1260 return -ENODEV; > 1261 ni = handle->notify_priv; > 1262 > 1263 evt_key = MAKE_HASH_KEY(proto_id, evt_id, > 1264 src_id ? *src_id : SRC_ID_MASK); > 1265 hndl = scmi_get_or_create_handler(ni, evt_key); > 1266 if (IS_ERR_OR_NULL(hndl)) > ^^^^^^^^^^^^^^^^^^^^ > There are a lot of wrong uses of IS_ERR_OR_NULL() in this driver. > > When a function returns both NULL and error pointers, then NULL is a > special kind of success. For example, we could have code which does > "p = get_feature();" and maybe there is an allocation failure, then > get_feature() should return an error pointer and we return that to the > user. > > But if the get_feature() is optional and it has been deliberately > disabled by the user with CONFIG_FOO=n then that is *not* an error. But > we are still not able to return a valid pointer to the feature so it > returns NULL. The driver should continue to operate without the > optional feature. > > 1267 return PTR_ERR(hndl); > > In this situation the scmi_get_or_create_handler() never returns error > pointers. It's not an optional feature. It returns NULL on failure. > So that means we return PTR_ERR(NULL) which is zero which means success. > The code should instead a negative error code instead: > > if (!hndl) > return -EINVAL; > > All the uses of IS_ERR_OR_NULL() that I saw were wrong but this is the > only one that caused a user visible bug. I did not know about this intended usage, I'm removing all these unneded IS_ERR_OR_NULL() usages with plain NULL checks. > > 1268 > 1269 blocking_notifier_chain_register(&hndl->chain, nb); > 1270 > 1271 /* Enable events for not pending handlers */ > 1272 if (likely(!IS_HNDL_PENDING(hndl))) { > ^^^^^^ > The likely/unlikely() annotations should only be used where it afects > benchmarking. They should all be removed from this driver. I'm removing all the likely/unlikely from this series. > > 1273 if (!scmi_event_handler_enable_events(hndl)) { > > I really don't like that half these functions follow normal kernel error > style and half return true/false on failure. Normally we would want > boolean functions to be clear from the name like access_ok() which > clearly returns true/false. But I didn't see that it causes any bugs > yet. Ok I'll convert all the functions to classical 0-Success style dumping the booleans. I cannot see in fact any suitable func_ok() style function. > > One thing I did notice is that scmi_sensors_init() return non-zero > positive values on success because it's returning the result from > idr_alloc(). This should trigger a warning message, I believe. > > 1274 scmi_put_handler(ni, hndl); > 1275 ret = -EINVAL; > 1276 } > 1277 } > 1278 > 1279 return ret; > 1280 } > The scmi_sensors_init() retval is not part of this series so I'll make a note of it but do not change it right now, also because I'm also reviewing more generally the whole SCMI initialization core, aiming at full modularization of the protocols, so that this part is already changed/gone in another unpublished series. Thanks Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel