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=-8.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=ham 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 293E6C2BB48 for ; Wed, 9 Dec 2020 08:32:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EFE1823BE2 for ; Wed, 9 Dec 2020 08:32:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727439AbgLIIcf (ORCPT ); Wed, 9 Dec 2020 03:32:35 -0500 Received: from smtprelay0236.hostedemail.com ([216.40.44.236]:48406 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726075AbgLIIcY (ORCPT ); Wed, 9 Dec 2020 03:32:24 -0500 Received: from smtprelay.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by smtpgrave04.hostedemail.com (Postfix) with ESMTP id 07A3E1801999C for ; Wed, 9 Dec 2020 08:31:43 +0000 (UTC) Received: from filter.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay03.hostedemail.com (Postfix) with ESMTP id 0AA3F837F24C; Wed, 9 Dec 2020 08:31:02 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-HE-Tag: plot14_2110887273ee X-Filterd-Recvd-Size: 4116 Received: from XPS-9350.home (unknown [47.151.137.21]) (Authenticated sender: joe@perches.com) by omf18.hostedemail.com (Postfix) with ESMTPA; Wed, 9 Dec 2020 08:30:59 +0000 (UTC) Message-ID: Subject: Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command From: Joe Perches To: mgross@linux.intel.com, markgross@kernel.org, arnd@arndb.de, bp@suse.de, damien.lemoal@wdc.com, dragan.cvetic@xilinx.com, gregkh@linuxfoundation.org, corbet@lwn.net, leonard.crestez@nxp.com, palmerdabbelt@google.com, paul.walmsley@sifive.com, peng.fan@nxp.com, robh+dt@kernel.org, shawnguo@kernel.org Cc: linux-kernel@vger.kernel.org, Seamus Kelly In-Reply-To: <20201201223511.65542-23-mgross@linux.intel.com> References: <20201201223511.65542-1-mgross@linux.intel.com> <20201201223511.65542-23-mgross@linux.intel.com> Content-Type: text/plain; charset="ISO-8859-1" Date: Wed, 09 Dec 2020 00:30:46 -0800 MIME-Version: 1.0 User-Agent: Evolution 3.38.1-1 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2020-12-01 at 14:35 -0800, mgross@linux.intel.com wrote: > Refactor the too large IOCTL function to call helper functions. [] > diff --git a/drivers/misc/xlink-core/xlink-ioctl.c b/drivers/misc/xlink-core/xlink-ioctl.c [] > +int ioctl_write_data(unsigned long arg) > +{ > + struct xlink_handle devh = {0}; > + struct xlinkwritedata wr = {0}; > + int rc = 0; > + > + if (copy_from_user(&wr, (void __user *)arg, > + sizeof(struct xlinkwritedata))) > + return -EFAULT; > + if (copy_from_user(&devh, (void __user *)wr.handle, > + sizeof(struct xlink_handle))) > + return -EFAULT; > + if (wr.size <= XLINK_MAX_DATA_SIZE) { > + rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, > + wr.size); > + if (copy_to_user((void __user *)wr.return_code, (void *)&rc, > + sizeof(rc))) > + return -EFAULT; > + } else { > + return -EFAULT; > + } Please reverse the test to reduce indentation if (wr.size > XLINK_MAX_DATA_SIZE) return -EFAULT; rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size); if (copy_to_user((void __user *)wr.return_code, (void *)&rc, sizeof(rc))) return -EFAULT; return rc; The last 3 lines here are repeated multiple times in many functions. It might be sensible to add something like: int copy_result_to_user(u32 *where, int rc) { if (copy_to_user((void __user *)where, &rc, sizeof(rc))) return -EFAULT; return rc; } so this could be written rc = xlink_write_data_user(&devh, wr.chan, wr.pmessage, wr.size); return copy_result_to_user(wr.return_code, rc); IMO: return_code isn't a great name for a pointer as it rather indicates a value not an address and there's an awful lot of casting to __user in all this code that perhaps should be marked in the struct definitions rather than inside the function uses. > +} > + > +int ioctl_write_control_data(unsigned long arg) > +{ > + struct xlink_handle devh = {0}; All of these initializations with {0} should use {} instead as the first element of whatever struct is not guaranteed to be assignable as an int and gcc/clang guarantee 0 initialization > + struct xlinkwritedata wr = {0}; > + u8 volbuf[XLINK_MAX_BUF_SIZE]; > + int rc = 0; > + > + if (copy_from_user(&wr, (void __user *)arg, > + sizeof(struct xlinkwritedata))) > + return -EFAULT; > + if (copy_from_user(&devh, (void __user *)wr.handle, > + sizeof(struct xlink_handle))) > + return -EFAULT; > + if (wr.size <= XLINK_MAX_CONTROL_DATA_SIZE) { > + if (copy_from_user(volbuf, (void __user *)wr.pmessage, > + wr.size)) > + return -EFAULT; > + rc = xlink_write_control_data(&devh, wr.chan, volbuf, > + wr.size); > + if (copy_to_user((void __user *)wr.return_code, > + (void *)&rc, sizeof(rc))) > + return -EFAULT; > + } else { > + return -EFAULT; Same test reversal and deindentation please. > + } > + return rc; > +} > +