Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command

From: mark gross <mgross@linux.intel.com>
To: Joe Perches <joe@perches.com>
Cc: 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, linux-kernel@vger.kernel.org,
	Seamus Kelly <seamus.kelly@intel.com>
Subject: Re: [PATCH 22/22] xlink-core: factorize xlink_ioctl function by creating sub-functions for each ioctl command
Date: Mon, 7 Dec 2020 13:59:19 -0800	[thread overview]
Message-ID: <20201207215919.GF49179@linux.intel.com> (raw)
In-Reply-To: <3595d2e5e5de009730b3374bf65e96359c02392d.camel@perches.com>

On Sun, Dec 06, 2020 at 07:05:34PM -0800, Joe Perches wrote:
> On Tue, 2020-12-01 at 14:35 -0800, mgross@linux.intel.com wrote:
> > From: Seamus Kelly <seamus.kelly@intel.com>
> > 
> > Refactor the too large IOCTL function to call helper functions.
> 
> This should not be sent as a known poor patch as patch 21 of 22
> and then updated in patch 22 of 22 with a better style.
> 
> This should be sent in the as desired final form the first time
> so that people don't give you useless notes.

We will re-work the changes to better integrate this one in the earlier
changes.

--mark

> 
> > @@ -342,427 +323,84 @@ static int kmb_xlink_remove(struct platform_device *pdev)
> >   * IOCTL function for User Space access to xlink kernel functions
> >   *
> >   */
> > +int ioctl_connect(unsigned long arg);
> >  
> > 
> >  static long xlink_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >  {
> > -	struct xlink_handle		devh	= {0};
> > -	struct xlinkopenchannel		op	= {0};
> > -	struct xlinkwritedata		wr	= {0};
> > -	struct xlinkreaddata		rd	= {0};
> > -	struct xlinkreadtobuffer	rdtobuf = {0};
> > -	struct xlinkconnect		con	= {0};
> > -	struct xlinkrelease		rel	= {0};
> > -	struct xlinkstartvpu		startvpu = {0};
> > -	struct xlinkcallback		cb	= {0};
> > -	struct xlinkgetdevicename	devn	= {0};
> > -	struct xlinkgetdevicelist	devl	= {0};
> > -	struct xlinkgetdevicestatus	devs	= {0};
> > -	struct xlinkbootdevice		boot	= {0};
> > -	struct xlinkresetdevice		res	= {0};
> > -	struct xlinkdevmode		devm	= {0};
> > -	struct xlinkregdevevent		regdevevent = {0};
> > -	u32 sw_device_id_list[XLINK_MAX_DEVICE_LIST_SIZE];
> > -	char name[XLINK_MAX_DEVICE_NAME_SIZE];
> > -	int interface = NULL_INTERFACE;
> > -	u32 device_status = 0;
> > -	u32 num_devices = 0;
> > -	u32 device_mode = 0;
> > -	u32 num_events = 0;
> > -	char filename[64];
> > -	u32 *ev_list;
> > -	u8 reladdr;
> > -	u8 *rdaddr;
> > -	u32 size;
> >  	int rc;
> >  
> > 
> >  	switch (cmd) {
> >  	case XL_CONNECT:
> > -		if (copy_from_user(&con, (void __user *)arg,
> > -				   sizeof(struct xlinkconnect)))
> > -			return -EFAULT;
> > -		if (copy_from_user(&devh, (void __user *)con.handle,
> > -				   sizeof(struct xlink_handle)))
> > -			return -EFAULT;
> > -		rc = xlink_connect(&devh);
> > -		if (rc == X_LINK_SUCCESS) {
> > -			if (copy_to_user((void __user *)con.handle,
> > -					 &devh, sizeof(struct xlink_handle)))
> > -				return -EFAULT;
> > -		}
> > -		if (copy_to_user((void __user *)con.return_code, (void *)&rc,
> > -				 sizeof(rc)))
> > -			return -EFAULT;
> > +		rc = ioctl_connect(arg);
> >  		break;
> 
> etc...
> 