Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 5 Aug 2020 08:46:22 +0200
From:   gregkh <gregkh@linuxfoundation.org>
To:     "Eads, Gage" <gage.eads@intel.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@intel.com>,
        "Topel, Bjorn" <bjorn.topel@intel.com>
Subject: Re: [PATCH 04/20] dlb2: add device ioctl layer and first 4 ioctls
Message-ID: <20200805064622.GA608152@kroah.com>
References: <20200712134331.8169-1-gage.eads@intel.com>
 <20200712134331.8169-5-gage.eads@intel.com>
 <CAK8P3a2OmSPGNghM+Y9ThH7hgKJKVSKRPaSPN17gUVRUh_o3bQ@mail.gmail.com>
 <SN6PR11MB2574422442C26A41FBC89DA1F64A0@SN6PR11MB2574.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <SN6PR11MB2574422442C26A41FBC89DA1F64A0@SN6PR11MB2574.namprd11.prod.outlook.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.

thanks,

greg k-h