Re: [PATCH 24/24] Documentation: document ioctl interfaces better

[Arnd Bergmann]

On Thu, Dec 12, 2019 at 9:16 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
> On Wed, Dec 11, 2019 at 9:53 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > +``include/uapi/asm-generic/ioctl.h`` provides four macros for defining
> > +ioctl commands that follow modern conventions: ``_IOC``, ``_IOR``,
> > +``_IOW``, and ``_IORW``. These should be used for all new commands,
> > +with the correct parameters:
> > +
> > +_IO/_IOR/_IOW/_IOWR
>
> This says _IO....
>
> > +   The macro name determines whether the argument is used for passing
> > +   data into kernel (_IOW), from the kernel (_IOR), both (_IOWR) or is
>
> into the kernel
> , or is
>
> > +   not a pointer (_IOC). It is possible but not recommended to pass an
> > +   integer value instead of a pointer with _IOC.
>
> ...which is not explained here, but _IOC is?

I mean _IO() everywhere, I would consider _IOC an implementation
detail and not document that here.

s/_IOC/_IO/ throughout the document now.

> > +size
> > +  The name of the data type pointed to by the argument, the command
> > +  number encodes the ``sizeof(size)`` value in a 13-bit or 14-bit integer,
> > +  leading to a limit of 8191 bytes for the maximum size of the argument.
> > +  Note: do not pass sizeof(type) type into _IOR/IOW, as that will lead
> > +  to encoding sizeof(sizeof(type)), i.e. sizeof(size_t).
>
> Looks like "size" could be renamed, to make this more obvious?

Changed to data_type now, which is what I found in some other
documentation.


> > +space timespec exactly. The get_timespec64() and put_timespec64() helper
> > +functions canbe used to ensure that the layout remains compatible with
>
> can be

done.

> > +
> > +``ktime_get_real_ns()`` can be used for CLOCK_REALTIME timestamps that
> > +may be required for timestamps that need to be persistent across a reboot
>
> Drop "may be required for timestamps that"?

done.

> > +* ``long`` and ``unsigned long`` are the size of a register, so
> > +  they can be either 32 bit or 64 bit wide and cannot be used in portable
>
> 32-bit or 64-bit (for consistency with the rest of the document)

The convention I was trying to follow is to write

"32-bit userspace" or "32-bit processor"

but

"this type is 32 bit wide"

I wasn't consistent though, changed it now as you suggested.

> > +
> > +* On ARM OABI user space, 16-bit member variables have 32-bit
> > +  alignment, making them incompatible with modern EABI kernels.
> > +  Conversely, on the m68k architecture, all struct members have at most
> > +  16-bit alignment. These rarely cause problems as neither ARM-OABI nor
>
> "have at most 16-bit alignment" sounds a bit weird to me, as a member
> may have a greater alignment.
> "struct members are not guaranteed to have an alignment greater than 16-bit"?

done.

> > +
> > +As explained for the compat mode, it is best to not avoid any padding in
>
> best to avoid any implicit padding?

done.

> > +
> > +* The complexity of user space access and data structure layout at done
>
> is done

changed

> > +There are many cases in which ioctl is not the best solution for a
> > +problem. Alternatives include
>
> :

done

> > +* System calls are a better choice for a system-wide feature that
> > +  is not tied to a physical device or constrained by the file system
> > +  permissions of a character device node
> > +
> > +* netlink is the preferred way of configuring any network related
> > +  objects through sockets.
> > +
> > +* debugfs is used for ad-hoc interfaces for debugging functionality
> > +  that does not need to be exposed as a stable interface to applications.
> > +
> > +* sysfs is a good way to expose the state of an in-kernel object
> > +  that is not tied to a file descriptor.
> > +
> > +* configfs can be used for more complex configuration than sysfs
> > +
> > +* A custom file system can provide extra flexibility with a simple
> > +  user interface but add a lot of complexity in the implementation.
>
> adds ... to

done.

Thanks for all the suggestions!

      Arnd