Re: [RFC v2 PATCH 2/3] /dev/mem: Add initial documentation of memory_open() and mem_fops

[Jonathan Corbet <corbet@lwn.net>]

Gabriele Paoloni <gpaoloni@redhat.com> writes:

> This patch proposes initial kernel-doc documentation for memory_open()
> and most of the functions in the mem_fops structure.
> The format used for the specifications follows the guidelines
> defined in Documentation/doc-guide/code-specifications.rst

I'll repeat my obnoxious question from the first patch: what does that
buy for us?

> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
>  drivers/char/mem.c | 231 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 225 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 48839958b0b1..e69c164e9465 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -75,9 +75,54 @@ static inline bool should_stop_iteration(void)
>  	return signal_pending(current);
>  }
>  
> -/*
> - * This funcion reads the *physical* memory. The f_pos points directly to the
> - * memory location.
> +/**
> + * read_mem - read from physical memory (/dev/mem).
> + * @file: struct file associated with /dev/mem.
> + * @buf: user-space buffer to copy data to.
> + * @count: number of bytes to read.
> + * @ppos: pointer to the current file position, representing the physical
> + *        address to read from.
> + *
> + * This function checks if the requested physical memory range is valid
> + * and accessible by the user, then it copies data to the input
> + * user-space buffer up to the requested number of bytes.
> + *
> + * Function's expectations:
> + *
> + * 1. This function shall check if the value pointed by ppos exceeds the
> + *    maximum addressable physical address;
> + *
> + * 2. This function shall check if the physical address range to be read
> + *    is valid (i.e. it falls within a memory block and if it can be mapped
> + *    to the kernel address space);
> + *
> + * 3. For each memory page falling in the requested physical range
> + *    [ppos, ppos + count - 1]:
> + *   3.1. this function shall check if user space access is allowed (if
> + *        config STRICT_DEVMEM is not set, access is always granted);
> + *
> + *   3.2. if access is allowed, the memory content from the page range falling
> + *        within the requested physical range shall be copied to the user space
> + *        buffer;
> + *
> + *   3.3. zeros shall be copied to the user space buffer (for the page range
> + *        falling within the requested physical range):
> + *     3.3.1. if access to the memory page is restricted or,
> + *     3.2.2. if the current page is page 0 on HW architectures where page 0 is
> + *            not mapped.
> + *
> + * 4. The file position '*ppos' shall be advanced by the number of bytes
> + *    successfully copied to user space (including zeros).

My kneejerk first reaction is: you are repeating the code of the
function in a different language.  If we are not convinced that the code
is correct, how can we be more confident that this set of specifications
is correct?  And again, what will consume this text?  How does going
through this effort get us to a better kernel?

Despite having been to a couple of your talks, I'm not fully
understanding how this comes together; people who haven't been to the
talks are not going to have an easier time getting the full picture.

Thanks,

jon