Re: [PATCH] [media] ddbridge: add IOCTLs

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Tue, 22 Aug 2017 17:21:16 +0200
Daniel Scheller <d.scheller.oss@gmail.com> escreveu:

> Am Tue, 22 Aug 2017 16:05:21 +0200
> schrieb Ralph Metzler <rjkm@metzlerbros.de>:
> 
> > Daniel Scheller writes:  
> >  > Am Sun, 20 Aug 2017 08:53:56 -0300
> >  > schrieb Mauro Carvalho Chehab <mchehab@s-opensource.com>:
> >  >     
> >  > > Em Sun, 20 Aug 2017 13:08:55 +0200
> >  > > Daniel Scheller <d.scheller.oss@gmail.com> escreveu:
> >  > >     
> >  > > > From: Daniel Scheller <d.scheller@gmx.net>
> >  > > > 
> >  > > > This patch adds back the IOCTL API/functionality which is
> >  > > > present in the upstream dddvb driver package. In comparison,
> >  > > > the IOCTL handler has been factored to a separate object (and
> >  > > > with that, some functionality from -core has been moved there
> >  > > > aswell), the IOCTLs are defined in an include in the uAPI, and
> >  > > > ioctl-number.txt is updated to document that there are IOCTLs
> >  > > > present in this driver.
> >  > > > 
> >  > > > Signed-off-by: Daniel Scheller <d.scheller@gmx.net>
> >  > > > ---
> >  > > > This patch depends on the ddbridge-0.9.29 bump, see [1]. The
> >  > > > functionality was part of the driver before.
> >  > > > 
> >  > > > [1] http://www.spinics.net/lists/linux-media/msg119911.html
> >  > > > 
> >  > > >  Documentation/ioctl/ioctl-number.txt        |   1 +
> >  > > >  MAINTAINERS                                 |   1 +
> >  > > >  drivers/media/pci/ddbridge/Makefile         |   2 +-
> >  > > >  drivers/media/pci/ddbridge/ddbridge-core.c  | 111 +--------
> >  > > >  drivers/media/pci/ddbridge/ddbridge-ioctl.c | 334
> >  > > > ++++++++++++++++++++++++++++
> >  > > > drivers/media/pci/ddbridge/ddbridge-ioctl.h |  32 +++
> >  > > > include/uapi/linux/ddbridge-ioctl.h         | 110 +++++++++ 7
> >  > > > files changed, 481 insertions(+), 110 deletions(-) create mode
> >  > > > 100644 drivers/media/pci/ddbridge/ddbridge-ioctl.c create mode
> >  > > > 100644 drivers/media/pci/ddbridge/ddbridge-ioctl.h create mode
> >  > > > 100644 include/uapi/linux/ddbridge-ioctl.h
> >  > > > 
> >  > > > diff --git a/Documentation/ioctl/ioctl-number.txt
> >  > > > b/Documentation/ioctl/ioctl-number.txt index
> >  > > > 3e3fdae5f3ed..d78d1cd092d2 100644 ---
> >  > > > a/Documentation/ioctl/ioctl-number.txt +++
> >  > > > b/Documentation/ioctl/ioctl-number.txt @@ -215,6 +215,7 @@ Code
> >  > > > Seq#(hex)	Include File		Comments 'c'
> >  > > > A0-AF   arch/x86/include/asm/msr.h	conflict! 'd'
> >  > > > 00-FF	linux/char/drm/drm.h	conflict! 'd'
> >  > > > 02-40	pcmcia/ds.h		conflict! +'d'
> >  > > > 00-0B	linux/ddbridge-ioctl.h	conflict!      
> >  > > 
> >  > > That's where the problem with this patch starts: we don't add
> >  > > conflicts here :-)
> >  > > 
> >  > > We need more discussions with regards to the features added by
> >  > > this patchset.    
> >  > 
> >  > Understood. The "good" thing is that this isn't a requirement to
> >  > drive any tuner boards (at the moment), however we shouldn't lose
> >  > track on this. Since this is the only complaint for now:
> >  > 
> >  > - We need to clear with Ralph if changing the MAGIC to something
> >  >   different is an option. In the end, if we change the userspace
> >  > apps to include the uAPI header from mainline if available (else
> >  > fallback to what ie. dddvb carries), I don't see an issue with
> >  > this. But if userspace apps keep on using private stuff, this will
> >  > break ofc.
> >  > - Other option: Fork dddvb and change userspace apps accordingly,
> >  > and keep them in sync with upstream. Since we already have to care
> >  > about the kernel part, this option is rather suboptimal.
> >  > 
> >  > Ralph, Ping :-)    
> > 
> > Changing to something different from 'd' should be fine.
> > Is there anything still free?  
> 
> We could use 0xDD (for *D*igital *D*evices :-) ), subrange 0xc0-0xff or
> so, that isn't declared "in use" in ioctl-number.txt (0xDD/0x00-0x3f
> is used by some ZFCP driver). Other options would be ie. 0xD0-0xDA,
> 0xDC, 0xDE-0xE4. There are also other single values and value-ranges
> free, all according to ioctl-number.txt ofcourse.

0xDD seems to work. We need a series whose first a patch just touches
ioctl-number.txt, that should be c/c to linux-doc ML and lkml.

Please double-check first if the s390's zfcp driver doesn't use higher
values. It is probably wise to c/c s390 maintainer on it.

> Still yet not sure if this is everything that needs changing to be
> accepted though.

I'll review when you resubmit the patches. Please be verbose while
documenting the patch(es) in this series, clearly explaining why
such ioctls are needed.

The best would be to add a documentation for ddbridge, explaining
those new ioctls, under Documentation/media/dvb-drivers.

I prefer if you place such documentation patch before the code, as
it makes easier to review the ioctls.

Thanks,
Mauro