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

[Jonathan Corbet]

On Wed, 11 Dec 2019 21:42:58 +0100
Arnd Bergmann <arnd@xxxxxxxx> wrote:

> Documentation/process/botching-up-ioctls.rst was orignally
> written as a blog post for DRM driver writers, so it it misses
> some points while going into a lot of detail on others.
> 
> Try to provide a replacement that addresses typical issues
> across a wider range of subsystems, and follows the style of
> the core-api documentation better.
> 
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>

Thanks for improving the docs!  I have a few nits outside of the content
itself.

>  Documentation/core-api/index.rst |   1 +
>  Documentation/core-api/ioctl.rst | 250 +++++++++++++++++++++++++++++++
>  2 files changed, 251 insertions(+)
>  create mode 100644 Documentation/core-api/ioctl.rst

So you left the old document in place; was that intentional?

> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index ab0eae1c153a..3f28b2f668be 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -39,6 +39,7 @@ Core utilities
>     ../RCU/index
>     gcc-plugins
>     symbol-namespaces
> +   ioctl
>  
>  
>  Interfaces for kernel debugging
> diff --git a/Documentation/core-api/ioctl.rst b/Documentation/core-api/ioctl.rst
> new file mode 100644
> index 000000000000..cb2c86ae63e7
> --- /dev/null
> +++ b/Documentation/core-api/ioctl.rst
> @@ -0,0 +1,250 @@
> +======================
> +ioctl based interfaces
> +======================
> +
> +:c:func:`ioctl` is the most common way for applications to interface

Please don't use :c:func: anymore.  If you just say "ioctl()" the right
thing will happen (which is nothing here, since there isn't anything that
makes sense to link to in the internal kernel context).

We need a checkpatch rule for :c:func: I guess.

Similarly, later on you have:

> +Timeout values and timestamps should ideally use CLOCK_MONOTONIC time,
> +as returned by ``ktime_get_ns()`` or ``ktime_get_ts64()``.  Unlike
> +CLOCK_REALTIME, this makes the timestamps immune from jumping backwards
> +or forwards due to leap second adjustments and clock_settime() calls.

Making those functions ``literal`` will defeat the automatic
cross-referencing.  Better to just say ktime_get_ns() without quotes.

[...]

> +* On the x86-32 (i386) architecture, the alignment of 64-bit variables
> +  is only 32 bit, but they are naturally aligned on most other
> +  architectures including x86-64. This means a structure like
> +
> +  ::

You don't need the extra lines here; just say "...a structure like::"

> +    struct foo {
> +        __u32 a;
> +        __u64 b;
> +        __u32 c;
> +    };
> +

Thanks,

jon