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

[Chen, Mike Ximing]

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Saturday, January 9, 2021 3:34 AM
> To: Chen, Mike Ximing <mike.ximing.chen@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; arnd@xxxxxxxx; Williams, Dan J
> <dan.j.williams@xxxxxxxxx>; pierre-louis.bossart@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v8 04/20] dlb: add device ioctl layer and first three ioctls
> 
> 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...
> 

It was my fault:(. Will  make sure it doesn't happen again.

> 
> > > > +#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?
> 
For now, it is primarily informational to user. Different HW revision may have different features enabled. For example,  certain features may not be available in the earlier revision A0 and A1, but are enabled in  the later revisions (B0, C0, etc). User applications that depends on these features may fail on A0/A1 devices. They can check the device revision/version to confirm that the failure is expected. Users can also implement workarounds to avoid failures. 

Thanks
Mike