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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 980D2C3DA78 for ; Tue, 17 Jan 2023 14:31:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=vjtKQT4Wmpjg5i/uQ+vKwuECMFeGUIi2d1JPOvLvev8=; b=iwSCnS6XsUEOYJ 9XqKxoUuSKrx5EtUB3Q8HR2N4yQVeOxTMphTRsNvJxxuBYM/aOMcaGdOMznM4BsSrFsjpn0KKJ475 Hi2Kvd/VC2HqtzCyP0/QeLC+of9pyCZKFiVttc07RZtclHD863U3TwijiULiZPZ/lTG0jelnvWmzp 2usEbECLjTnLIERFyFeOos0+kSf/K1THU8QQlIeoTb0y2PLajkF9dJI+n/t4sTPwFyyVzBlXRB5D4 Gxp1ntU++UMxlbA3g0/ncRRtoqlS/YeOTYbUtEPDotf61EhvUzrbpwCYXYfWp+evVs9sNhcwEWFPK MLpxtxCUC1yXpwQyGpig==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pHmzL-00Ed8i-EJ; Tue, 17 Jan 2023 14:30:59 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pHmzH-00Ed7u-SV for linux-arm-kernel@lists.infradead.org; Tue, 17 Jan 2023 14:30:57 +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 ABE35AD7; Tue, 17 Jan 2023 06:31:36 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E887D3F67D; Tue, 17 Jan 2023 06:30:53 -0800 (PST) Date: Tue, 17 Jan 2023 14:30:43 +0000 From: Cristian Marussi To: Dan Carpenter Cc: linux-arm-kernel@lists.infradead.org, Matthew Wilcox Subject: Re: [bug report] firmware: arm_scmi: Add per-channel Raw injection support Message-ID: References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230117_063056_032825_FD2724E5 X-CRM114-Status: GOOD ( 21.88 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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, Jan 17, 2023 at 01:43:44PM +0300, Dan Carpenter wrote: > Hello Cristian Marussi, > Hi Dan, > The patch 0ae95d6e8b75: "firmware: arm_scmi: Add per-channel Raw > injection support" from Jan 13, 2023, leads to the following Smatch > static checker warning: > > drivers/firmware/arm_scmi/raw_mode.c:1115 scmi_raw_mode_setup() > warn: 'q' is an error pointer or valid > > drivers/firmware/arm_scmi/raw_mode.c > 1087 static int scmi_raw_mode_setup(struct scmi_raw_mode_info *raw, > 1088 u8 *channels, int num_chans) > 1089 { > 1090 int ret, idx; > 1091 void *gid; > 1092 struct device *dev = raw->handle->dev; > 1093 > 1094 gid = devres_open_group(dev, NULL, GFP_KERNEL); > 1095 if (!gid) > 1096 return -ENOMEM; > 1097 > 1098 for (idx = 0; idx < SCMI_RAW_MAX_QUEUE; idx++) { > 1099 raw->q[idx] = scmi_raw_queue_init(raw); > 1100 if (IS_ERR(raw->q[idx])) { > 1101 ret = PTR_ERR(raw->q[idx]); > 1102 goto err; > 1103 } > 1104 } > 1105 > 1106 if (num_chans > 1) { > 1107 int i; > 1108 > 1109 idr_init(&raw->queues_idr); > 1110 > 1111 for (i = 0; i < num_chans; i++) { > 1112 struct scmi_raw_queue *q; > 1113 > 1114 q = scmi_raw_queue_init(raw); > --> 1115 if (!q) > 1116 continue; > > This should be something like: > > if (IS_ERR(q)) { > ret = PTR_ERR(q); > goto err; > } > Beside the (sadly for me usual) bug on error-check the idea was here really not to bail out if these additional per-channels debugfs entries fails to be created since I have anyway a bare minimum support available so I can carry on. > 1117 > 1118 ret = idr_alloc(&raw->queues_idr, q, > 1119 channels[i], channels[i] + 1, > 1120 GFP_KERNEL); > 1121 if (ret != channels[i]) > 1122 dev_err(dev, > 1123 "Fail to allocate Raw queue 0x%02X\n", > 1124 channels[i]); > > Heh. I've never seen anyone use an IDR range of one value before... Well, the choice was between wasting an hashtable or a radix-tree IDR for a few mapping channel--->q and I went for the 1-range IDR which is already used in SCMI stack to map various refs to channel numbers. (I avoid in general to put a simple plain 256 array on the stack that can lead to stack-size issues especially while compiling on armv7) > Just printing an error is not correct error handling. I think these > IDR values have to be freed on the error path? Or is there a devm_ > trick happening here? > This comes from the above (maybe ill) choice of not bailing out on error for these entries; everything is freed on final exit and kept unused on errors But it can lead indeed to a situation in which only some entries are unexpectedly not working, so it has to better handled. I'll fix and repost. Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel