Re: [PATCH] Documentation: Reference kernel-doc for container_of

[Jonathan Corbet <corbet@lwn.net>]

René Nyffenegger <mail@renenyffenegger.ch> writes:

> The file include/linux/container_of.h contained kernel-doc but was not
> referenced in any .rst file. In addition, the file
> Documentation/core-api/kobject.rst wrongly located the definition
> of the macro `container_of` in linux/kernel.h while in reality
> it is defined in linux/container_of.h
>
> This patch adds a new .rst file that includes the kernel-doc of
> container_of.h and rectifies the wrong reference of the header
> file.
>
> Signed-off-by: René Nyffenegger <mail@renenyffenegger.ch>

Thank you for working to improve the kernel documentation!

There are, though, a few problems with this patch that will need to be
addressed before it can be accepted.  To start, please cc the maintainer
(i.e. me) on documentation changes.

>  Documentation/core-api/container_of.rst | 64 +++++++++++++++++++++++++
>  Documentation/core-api/index.rst        |  1 +
>  Documentation/core-api/kobject.rst      | 58 +++++++++++-----------
>  3 files changed, 93 insertions(+), 30 deletions(-)
>  create mode 100644 Documentation/core-api/container_of.rst
>
> diff --git a/Documentation/core-api/container_of.rst b/Documentation/core-api/container_of.rst
> new file mode 100644
> index 000000000000..f063d3f9e536
> --- /dev/null
> +++ b/Documentation/core-api/container_of.rst
> @@ -0,0 +1,64 @@
> +=====================================================================================
> +Given a pointer to a member of a struct, returning a pointer to the containing struct
> +=====================================================================================

Sticking to the 80-character limit is important for documentation, as it
greatly affects its readability.

> +.. _container_of:

This label is never referenced, so please do not add it.

> +Overview
> +========
> +
> +The two macros ``container_of`` and ``container_of_const``, defined in ``<include/linux/container_of.h>``, return a pointer to the ``struct`` (i. e. the *container*) from a pointer to a member of this ``struct``.

Please just refer to macros as, for example, container_of(), with
parentheses and without literal markup.  The build code will then do the
right thing.

> +These macros might be used when :ref:`embedding kobjects<embedding_kobjects>`, but see also :ref:`usage<container_of_usage>`.

Referencing a label that is immediately below is kind of pointless.  I'd
delete the label and say "see below".

Also, kobjects are far from the most common use of container_of(), not
sure why the are called out here.

> +
> +.. kernel-doc:: include/linux/container_of.h
> +
> +Usage
> +=====
> +
> +.. _container_of_usage:
> +
> +The following simple code demonstrates the usage of ``container_of``
> +
> +.. code-block:: c
> +
> + struct inner_struct
> + {
> + 	int abc;
> + 	int def;
> + }
> + 
> + struct container_struct
> + {
> + 	struct inner_struct inner;
> + 	char               *member_xyz;
> + 	int                 member_val;
> + };
> + 
> + static void f(struct inner_struct *inr)
> + {
> + 	struct container_struct *cont;
> + 
> + 	cont = container_of(inr, struct container_struct, inner);
> + 
> +        /* Use cont and its members */
> + }
> + 
> + static void g(char** member)
> + {
> + 	struct container_struct *cont;
> + 
> + 	cont = container_of(member, struct container_struct, member_xyz);
> +        /* Use cont and its members */
> + }

Not sure why two examples are needed, they are showing the same thing?

> + void somewhere()
> + {
> + 	struct container_struct cont;
> + 
> + 	/* Initialize cont */
> + 
> + 	f(& cont.inner      );
> + 	g(& cont.member_xyz );
> + }

Sample code should follow the kernel coding style.

> diff --git a/Documentation/core-api/index.rst b/Documentation/core-api/index.rst
> index 7a3a08d81f11..595af47d0d5f 100644
> --- a/Documentation/core-api/index.rst
> +++ b/Documentation/core-api/index.rst
> @@ -37,6 +37,7 @@ Library functionality that is used throughout the kernel.
>     kref
>     assoc_array
>     xarray
> +   container_of
>     maple_tree
>     idr
>     circular-buffers
> diff --git a/Documentation/core-api/kobject.rst b/Documentation/core-api/kobject.rst
> index 7310247310a0..058570694418 100644
> --- a/Documentation/core-api/kobject.rst
> +++ b/Documentation/core-api/kobject.rst
> @@ -49,6 +49,8 @@ approach will be taken, so we'll go back to kobjects.
>  Embedding kobjects
>  ==================
>  
> +.. _embedding_kobjects:
> +
>  It is rare for kernel code to create a standalone kobject, with one major
>  exception explained below.  Instead, kobjects are used to control access to
>  a larger, domain-specific object.  To this end, kobjects will be found
> @@ -66,50 +68,46 @@ their own, but are invariably found embedded in the larger objects of
>  interest.)
>  
>  So, for example, the UIO code in ``drivers/uio/uio.c`` has a structure that
> -defines the memory region associated with a uio device::
> +defines the memory region associated with a uio device:
>  
> -    struct uio_map {
> -            struct kobject kobj;
> -            struct uio_mem *mem;
> -    };
> +.. code-block:: c
>  
> -If you have a struct uio_map structure, finding its embedded kobject is
> -just a matter of using the kobj member.  Code that works with kobjects will
> -often have the opposite problem, however: given a struct kobject pointer,
> + struct uio_map {
> + 	struct kobject kobj;
> + 	struct uio_mem *mem;
> + };

At this point you are making changes to documentation unrelated to
container_of().  I'm not entirely sure why you are doing that.  If you
must, though, these changes need to be in their own patch with their own
justification.

> +If you have a ``struct uio_map`` structure, finding its embedded kobject is
> +just a matter of using the ``kobj`` member.  Code that works with kobjects will
> +often have the opposite problem, however: given a ``struct kobject *``,
>  what is the pointer to the containing structure?  You must avoid tricks
>  (such as assuming that the kobject is at the beginning of the structure)
> -and, instead, use the container_of() macro, found in ``<linux/kernel.h>``::
> -
> -    container_of(ptr, type, member)
> +and, instead, use the container_of() macro:
>  
> -where:
> +.. code-block:: c
>  
> -  * ``ptr`` is the pointer to the embedded kobject,
> -  * ``type`` is the type of the containing structure, and
> -  * ``member`` is the name of the structure field to which ``pointer`` points.
> -
> -The return value from container_of() is a pointer to the corresponding
> -container type. So, for example, a pointer ``kp`` to a struct kobject
> -embedded **within** a struct uio_map could be converted to a pointer to the
> -**containing** uio_map structure with::
> -
> -    struct uio_map *u_map = container_of(kp, struct uio_map, kobj);
> + struct kobject *kp =  ; /* Pointer to a kobj member of a uio_map */
> + struct uio_map *u_map = container_of(kp, struct uio_map, kobj);
>  
>  For convenience, programmers often define a simple macro for **back-casting**
>  kobject pointers to the containing type.  Exactly this happens in the
> -earlier ``drivers/uio/uio.c``, as you can see here::
> +earlier ``drivers/uio/uio.c``, as you can see here:
>  
> -    struct uio_map {
> -            struct kobject kobj;
> -            struct uio_mem *mem;
> -    };
> +.. code-block:: c
>  
> -    #define to_map(map) container_of(map, struct uio_map, kobj)
> + struct uio_map {
> + 	struct kobject kobj;
> + 	struct uio_mem *mem;
> + };
> + 
> + #define to_map(map) container_of(map, struct uio_map, kobj)

Gratuitous formatting changes aren't really useful.

Thanks,

jon