Re: [PATCH v8 04/20] dlb: add device ioctl layer and first three ioctls

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Jan 09, 2021 at 07:49:24AM +0000, Chen, Mike Ximing wrote:
> > > +static int dlb_ioctl_arg_size[NUM_DLB_CMD] = {
> > > +	sizeof(struct dlb_get_device_version_args),
> > > +	sizeof(struct dlb_create_sched_domain_args),
> > > +	sizeof(struct dlb_get_num_resources_args)
> > 
> > That list.
> > 
> > Ugh, no.  that's no way to write maintainable code that you will be able
> > to understand in 2 years.
> > 
> 
> dlb_ioctl() was implemented with switch-case and real function calls previously. 
> We changed to the table/list implementation during a code restructure. I will move
> back to the old implementation. 

Who said to change this?  And why did they say that?  Please go back to
those developers and point them at this thread so that doesn't happen
again...

> > > +{
> > > +	struct dlb *dlb;
> > > +	dlb_ioctl_fn_t fn;
> > > +	u32 cmd_nr;
> > > +	void *karg;
> > > +	int size;
> > > +	int ret;
> > > +
> > > +	dlb = f->private_data;
> > > +
> > > +	if (!dlb) {
> > > +		pr_err("dlb: [%s()] Invalid DLB data\n", __func__);
> > > +		return -EFAULT;
> > 
> > This error value is only allowed if you really do have a memory fault.
> > 
> > Hint, you do not here.
> > 
> > How can that value ever be NULL?
> >
> 
> It is targeted at some very rare cases, in which an ioctl command are called immediately after a device unbind (by a different process/application).

And how can that happen?  If it really can happen, where is the lock
that you are holding to keep that pointer from becoming "stale" right
after you assign it?

So either this never can happen, or your logic here for this type of
thing is totally wrong.  Please work to determine which it is.

> > > +#define DLB_DEVICE_VERSION(x) (((x) >> 8) & 0xFF)
> > > +#define DLB_DEVICE_REVISION(x) ((x) & 0xFF)
> > > +
> > > +enum dlb_revisions {
> > > +	DLB_REV_A0 = 0,
> > 
> > What is a "revision" and why do you care about it?
> 
> This is for different revisions of hardware. Each revision of hardware may have a slight different configuration/feature.

So what does this mean?  What are you going to do based on it?

> > > +/*
> > > + * DLB_CMD_GET_DEVICE_VERSION: Query the DLB device version.
> > > + *
> > > + *	Each DLB device version has its own unique ioctl API, but all share
> > > + *	this as the first command in their interface, which tells user-space
> > > + *	which API to use. The device revision is provided in case of any
> > > + *	hardware errata.
> > > + *
> > > + * Output parameters:
> > > + * - response.status: Detailed error code. In certain cases, such as if the
> > > + *	ioctl request arg is invalid, the driver won't set status.
> > > + * - response.id[7:0]: Device revision.
> > > + * - response.id[15:8]: Device version.
> > 
> > So userspace has to do different things depending on what the hardware
> > type is?  Why not make a totally different driver for new hardware
> > types if things are going to change in the future?
> > 
> 
> This comment is not correct (sorry about this).  There will be different 
> versions of  DLB hardware (of the same type). All DLB devices will have the same ioctl
> API and userspace interface.

Good, please fix then :)

thanks,

greg k-h