RE: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls

[Eads, Gage]

> -----Original Message-----
> From: gregkh <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, August 5, 2020 10:18 AM
> To: Eads, Gage <gage.eads@xxxxxxxxx>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> Karlsson, Magnus <magnus.karlsson@xxxxxxxxx>; Topel, Bjorn
> <bjorn.topel@xxxxxxxxx>
> Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
> 
> On Wed, Aug 05, 2020 at 03:07:50PM +0000, Eads, Gage wrote:
> >
> >
> > > -----Original Message-----
> > > From: gregkh <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Sent: Wednesday, August 5, 2020 1:46 AM
> > > To: Eads, Gage <gage.eads@xxxxxxxxx>
> > > Cc: Arnd Bergmann <arnd@xxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx;
> > > Karlsson, Magnus <magnus.karlsson@xxxxxxxxx>; Topel, Bjorn
> > > <bjorn.topel@xxxxxxxxx>
> > > Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
> > >
> > > On Tue, Aug 04, 2020 at 10:20:47PM +0000, Eads, Gage wrote:
> > > > > > +/* [7:0]: device revision, [15:8]: device version */
> > > > > > +#define DLB2_SET_DEVICE_VERSION(ver, rev) (((ver) << 8) |
> (rev))
> > > > > > +
> > > > > > +static int dlb2_ioctl_get_device_version(struct dlb2_dev *dev,
> > > > > > +                                        unsigned long user_arg,
> > > > > > +                                        u16 size)
> > > > > > +{
> > > > > > +       struct dlb2_get_device_version_args arg;
> > > > > > +       struct dlb2_cmd_response response;
> > > > > > +       int ret;
> > > > > > +
> > > > > > +       dev_dbg(dev->dlb2_device, "Entering %s()\n", __func__);
> > > > > > +
> > > > > > +       response.status = 0;
> > > > > > +       response.id = DLB2_SET_DEVICE_VERSION(2, DLB2_REV_A0);
> > > > > > +
> > > > > > +       ret = dlb2_copy_from_user(dev, user_arg, size, &arg,
> > > sizeof(arg));
> > > > > > +       if (ret)
> > > > > > +               return ret;
> > > > > > +
> > > > > > +       ret = dlb2_copy_resp_to_user(dev, arg.response,
> &response);
> > > > >
> > > > > Better avoid any indirect pointers. As you always return a constant
> > > > > here, I think the entire ioctl command can be removed until you
> > > > > actually need it. If you have an ioctl command that needs both
> > > > > input and output, use _IOWR() to define it and put all arguments
> > > > > into the same structure.
> > > >
> > > > I should've caught this in my earlier response, sorry. The device
> version
> > > > command is intentionally the first in the user interface enum. My
> > > > goal is for all device versions (e.g. DLB 1.0 in the future) to be
> accessible
> > > > through a /dev/dlb%d node. To allow this, all drivers would support
> the
> > > same
> > > > device-version command as command 0, then the subsequent
> commands
> > > can be
> > > > tailored to that particular device. User-space would query the version
> first
> > > > to determine which set of ioctl commands it needs to use.
> > > >
> > > > So even though the response is constant (for now), it must occupy
> > > command 0 for
> > > > this design to work.
> > >
> > > "versions" for ioctls just do not work, please don't go down that path,
> > > they should not be needed.  See the many different discussions about
> > > this topic on lkml for other subsystem submissions if you are curious.
> > >
> >
> > This approach is based on VFIO's modular ioctl design, which has a
> different
> > API for Type1 vs. SPAPR IOMMUs. Similarly a DLB driver could have a
> different
> > API for each device version (but each API would be fixed, not versioned). I
> > didn't see any concerns on lkml over VFIO when it was originally submitted
> -- though
> > that was 8 years ago, perhaps the community's feelings have changed
> since then.
> 
> Fixed apis for device types is usually the better way to go.  See the
> review comments on the nitro_enclaves driver submission a few weeks ago
> for the full details.

Thanks for the pointer -- I think we're on the same page regarding fixed APIs.
I'll clarify the documentation for this ioctl in v2 so it's clear that its
purpose is to support fixed APIs for different hardware versions/types.

Thanks,
Gage