René Nyffenegger <mail@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
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
