Re: [PATCH rdma-next V2 6/6] RDMA/core: Unify style of IOCTL commands

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On Tue, Sep 06, 2016 at 05:03:14PM -0400, ira.weiny wrote:
> On Thu, Sep 01, 2016 at 08:52:22PM +0300, Leon Romanovsky wrote:
> > On Thu, Sep 01, 2016 at 11:33:20AM -0600, Jason Gunthorpe wrote:
> > > On Thu, Sep 01, 2016 at 05:17:26PM +0000, Dalessandro, Dennis wrote:
> > > > On Thu, 2016-09-01 at 11:11 -0600, Jason Gunthorpe wrote:
> > > > > On Thu, Sep 01, 2016 at 05:09:31PM +0000, Dalessandro, Dennis wrote:
> > > > >
> > > > > > > Dennis should use an internal definition in PSM if he wishes to
> > > > > > > continue to support the staging kernel ABI.
> > > > > >
> > > > > > It's not just the backward compatibility. PSM uses these command
> > > > > > definitions.So this breaks current support with current driver.
> > > > >
> > > > > How exactly? The HFI1_CMD_ASSIGN_CTXT constant is never used in the
> > > > > kernel?
> > > >
> > > > This is used in PSM library. Agree it's not used in the kernel, so I
> > > > can see the argument to get rid of it, or not care.
> > >
> > > Lets get rid of all the #defines. Leon you should just inline the
> > > ioctl numbers into the ioctl definition like normal and get rid of
> > > this extra layer of macros.
> >
> > Ohh, it looks like you returned to us filled with positive energy :)
> >
> > I tried to follow DRM subsystem [1] while converted these names with
> > minimal impact on the current implementation.
>
> I like the idea of borrowing from the DRM subsystem but that is not quite what
> I see here.

"followed" != ("borrowed" || "copy-pasted")

To make long story short.

1. You took 0xE0 as a base for HFI while claimed to take 0xF0. There is
nothing about 0x80 as a base. See commit 8d970cf991a6c38a5566572979487b906d643740

+/*
+ * Make the ioctls occupy the last 0xf0-0xff portion of the IB range
+ */
+#define __NUM(cmd) (HFI1_CMD_##cmd + 0xe0)

2. In new ABI, you will define your special objects, mark them as a
vendor specific and the driver will be called immediately. IMHO, there
is no need in special ioctl.

3. The Jason's point do not pollute code with defines/code which not in
use has very solid background. It is right time to stop misuse of UAPI
which you did. I'm pretty sure that you was supposed to update your PSM
library when you moved from staging and converted from write to ioctl.
I didn't hear complains about it.

Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]