On Wed, May 29, 2024 at 09:29:23AM +0200, Jiri Slaby wrote:
> On 18. 04. 24, 8:18, Greg Kroah-Hartman wrote:
> > On Wed, Apr 17, 2024 at 07:37:35PM +0200, Alexey Gladkov wrote:
> > > All other headers use _IOC() macros to describe ioctls for a long time
> > > now. This header is stuck in the last century.
> > >
> > > Simply use the _IO() macro. No other changes.
> > >
> > > Signed-off-by: Alexey Gladkov <legion@xxxxxxxxxx>
> > > ---
> > > include/uapi/linux/kd.h | 96 +++++++++++++++++++++--------------------
> > > 1 file changed, 49 insertions(+), 47 deletions(-)
> >
> > This is a nice cleanup, thanks for doing it, I'll just take this one
> > change now if you don't object.
>
> Unfortunately, _IOC_NONE is 1 on some archs as noted by Arnd, and this
> commit changed the kd ioctl values in there which broke stuff as noted by
> Al.
>
> We either:
> * use _IOC(0, X, Y) in here, instead of _IO(X, Y), or
> * define KDIOC(X) as _IOC(0, KD_IOCTL_BASE, X), or
> * revert the commit which landed to -rc1 already.
Revert, IMO. If nothing else, _IO... macros are there to
* document the copyin/copyout direction
* make sure that argument size mismatches between the kernel
and userland get caught
Turning 16bit hex constants into that won't serve any purpose other
than preventing such patches in the future; adding a comment would
deal with that just fine.
BTW, the original odd choice for _IOC_NONE appears to have been motivated
by avoiding conflicts with legacy ioctl numbers: in CSRG repo there's
this:
commit 02cbc6a653b48bb4ec6052cbe6b3979530011c46
Author: Sam Leffler <sam@xxxxxxxxxxxxxxxxxxx>
Date: Mon Aug 2 02:22:17 1982 -0800
new ioctl's
...
+/*
+ * Ioctl's have the command encoded in the lower word,
+ * and the size of any in or out parameters in the upper
+ * word. The high 2 bits of the upper word are used
+ * to encode the in/out status of the parameter; for now
+ * we restrict parameters to at most 128 bytes.
+ */
+#define IOCPARM_MASK 0x7f /* parameters must be < 128 bytes */
+#define IOC_VOID 0x20000000 /* no parameters */
+#define IOC_OUT 0x40000000 /* copy out parameters */
+#define IOC_IN 0x80000000 /* copy in parameters */
+#define IOC_INOUT (IOC_IN|IOC_OUT)
+/* the 0x20000000 is so we can distinguish new ioctl's from old */
+#define _IO(x,y) (IOC_VOID|('x'<<8)|y)
+#define _IOR(x,y,t) (IOC_OUT|((sizeof(t)&IOCPARM_MASK)<<16)|('x'<<8)|y)
+#define _IOW(x,y,t) (IOC_IN|((sizeof(t)&IOCPARM_MASK)<<16)|('x'<<8)|y)
+/* this should be _IORW, but stdio got there first */
+#define _IOWR(x,y,t) (IOC_INOUT|((sizeof(t)&IOCPARM_MASK)<<16)|('x'<<8)|y)
FWIW, the upper limit on parameter size went up from 128 bytes to 8Kb in
commit 856ed9ecd2795280feed42baf3889ac7c7f9a26d
Author: Mike Karels <karels@xxxxxxxxxxxxxxxxxxx>
Date: Thu Feb 19 22:16:28 1987 -0800
larger ioctl's (for disklabels)
Wonder what got us (in sparc port?) to lift it from 8Kb to 16Kb...
In our historical tree that has happened in
commit 1c29224f169362b671a4bef4cd864464ef810d42
Author: Pete Zaitcev <zaitcev@xxxxxxxxxx>
Date: Sun Mar 2 08:45:12 2003 -0800
[SPARC32/64]: Expand ioctl size field in backwards-compatible way.
No explanations there...
