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=-1.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,UNPARSEABLE_RELAY,URIBL_BLOCKED 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 EF216C433DF for ; Tue, 30 Jun 2020 18:01:08 +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 C760520724 for ; Tue, 30 Jun 2020 18:01:08 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="ALp8DLLg"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=oracle.com header.i=@oracle.com header.b="jaCwjttr" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C760520724 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=oracle.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:MIME-Version: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:In-Reply-To:References:List-Owner; bh=zFEEL13QdIC0cYT4OX8bBZ+bNA81rRccU3li1TUUvSU=; b=ALp8DLLgXpg70175lF0wZ2B4lr yM0weG86mrDwZF203q2l3Gr96gEOkZEVeBmiLwUxGNv3/SCPFUBqmgXKYd4ZlKZjdZXf7MPRBnCLp 0YToPtMzMI8wRTipPf1Gc7Z7YiG/weI6rqyRTho3EciAFKOdXRF3vsZvVDPf6V/OQrOBx7ibN2HM2 pQvH6EnZV82J7gHeOqA0mAr9kRwM9NJ1R9eSItu4LtZ10qVgcYqIzSKGqpsR/ikbrsoLhcIkgWYCC cmO2qEkE0wk+2p/xoetUF+d9CJwkxMsVKvZ3lJl4pBwaI49csrEnXG8xLZP753aBpFGGD8ZxfFvme 7LUvdnvw==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqKXi-0007ZU-Kf; Tue, 30 Jun 2020 17:59:38 +0000 Received: from userp2130.oracle.com ([156.151.31.86]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jqKXg-0007Yi-E5 for linux-arm-kernel@lists.infradead.org; Tue, 30 Jun 2020 17:59:37 +0000 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 05UHrQG2176852; Tue, 30 Jun 2020 17:59:26 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : mime-version : content-type; s=corp-2020-01-29; bh=OIjkM/HVfWxZgh5V0pY9R8Y8gZkicdyMDZqjx4+TEOE=; b=jaCwjttr8tHM+36uWlaDjKM/UkdWvXf+MkUKAKooqtOEmVBIXdUnnu103IeXleZC3PHh mtfVbID8awyLnXBqd1m6sRPyu8qycsR+0Ir1WqrGITbY7Cudok0rmuvv+zYhuPLezt0+ VF4JiEsSOge2cH6cKiVvU86voaD/vPEZu1ZAbpuXJ+MAcJAfqv/8xUgglKwlw0eN0ycU HIhGL8TxJZAUWlvoGCGFwypq5eJstK1HO4On6a4vBME+cfnNu34VUnYiynAE7lQK4Gyg qaha/b/uS21dOSdzU2Nd2rrUr7hntNl4qfxgQE86t9CoKu+CubsNJtEBcf0TAGHdHsmu 4g== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by userp2130.oracle.com with ESMTP id 31ywrbmab6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 30 Jun 2020 17:59:26 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 05UHqhqg038493; Tue, 30 Jun 2020 17:57:25 GMT Received: from aserv0121.oracle.com (aserv0121.oracle.com [141.146.126.235]) by aserp3020.oracle.com with ESMTP id 31xg14119n-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 30 Jun 2020 17:57:25 +0000 Received: from abhmp0016.oracle.com (abhmp0016.oracle.com [141.146.116.22]) by aserv0121.oracle.com (8.14.4/8.13.8) with ESMTP id 05UHvOIk018001; Tue, 30 Jun 2020 17:57:24 GMT Received: from mwanda (/41.57.98.10) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 30 Jun 2020 17:57:23 +0000 Date: Tue, 30 Jun 2020 20:57:18 +0300 From: Dan Carpenter To: cristian.marussi@arm.com Subject: [bug report] firmware: arm_scmi: Add notification callbacks-registration Message-ID: <20200630175718.GB34614@mwanda> MIME-Version: 1.0 Content-Disposition: inline X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9668 signatures=668680 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 malwarescore=0 mlxlogscore=999 suspectscore=3 bulkscore=0 mlxscore=0 adultscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2006300123 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9668 signatures=668680 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 mlxlogscore=999 clxscore=1011 cotscore=-2147483648 priorityscore=1501 lowpriorityscore=0 malwarescore=0 mlxscore=0 adultscore=0 suspectscore=3 impostorscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2004280000 definitions=main-2006300123 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200630_135936_657676_EEB6F593 X-CRM114-Status: GOOD ( 20.06 ) 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 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 Hello Cristian Marussi, 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. 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. 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. 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 } regards, dan carpenter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel