Message-ID: <2025082120-emptiness-pencil-6d28@gregkh>
Date: Thu, 21 Aug 2025 19:14:35 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Gabriele Paoloni <gpaoloni@...hat.com>
Cc: arnd@...db.de, linux-kernel@...r.kernel.org,
	safety-architecture@...ts.elisa.tech
Subject: Re: [RFC PATCH] /dev/mem: Add initial documentation of memory_open()
 and mem_fops

On Thu, Aug 21, 2025 at 07:04:19PM +0200, Gabriele Paoloni wrote:
> This patch proposes initial kernel-doc documentation for memory_open()
> and most of the functions in the mem_fops structure.

You are adding kerneldoc documentation for static functions, are you
sure the tools will work with that?

> The format used for the **Description** intends to define testable
> function's expectations and Assumptions of Use to be met by the
> user of the function.

Where is this "format" documented?  Who will be parsing it?

> Signed-off-by: Gabriele Paoloni <gpaoloni@...hat.com>
> ---
> I have a couple of comments from this documentation activity:
> 1) Shouldn't the check in read_mem() <<if (p != *ppos)>> return
>    -EFBIG (as done in write_mem())?
> 2) There is a note in memory_lseek() that states the return value
>    to be (0) for negative addresses, however I cannot see how that
>    would happen in the current implementation...
> ---
> 
>  drivers/char/mem.c | 209 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 203 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 48839958b0b1..96d59066e8dc 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -75,9 +75,49 @@ 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: file structure for the device.

What do you mean by "device"?  It's a file pointer, no devices here.

> + * @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.
> + *
> + * Function's expectations:
> + *
> + * - This function shall check if the value pointed by ppos exceeds the
> + *   maximum addressable physical address;
> + *
> + * - 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);
> + *
> + * - For each memory page falling in the requested physical range
> + *   [ppos, ppos + count - 1]:
> + *   - this function shall check if user space access is allowed;

What does "shall check" mean?  That it does do this, or that you are
asserting that it MUST do it?  Again, documentation for how you are
wording all of this and most importantly, WHY you are wording it this
way is key.

Actually, why is any of this needed at all?  What is this for?  Is this
going to be some requirement of all new functions in the kernel?

I think I know the context here, but I bet no one else does, please fix
that.

> + *
> + *   - 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;
> + *
> + *   - zeros shall be copied to the user space buffer (for the page range
> + *     falling within the requested physical range):
> + *     - if access to the memory page is restricted or,
> + *     - if the current page is page 0 on HW architectures where page 0 is not
> + *       mapped.
> + *
> + * - The file position '*ppos' shall be advanced by the number of bytes
> + *   successfully copied to user space (including zeros).

Why is 0 special?

> + *
> + * Context: process context.
> + *
> + * Return:
> + * * the number of bytes copied to user on success
> + * * %-EFAULT - the requested address range is not valid or a fault happened
> + *   when copying to user

"userspace"?

> + * * %-EPERM - access to any of the required pages is not allowed

Which pages?  userspace or kernel?

> + * * %-ENOMEM - out of memory error for auxiliary kernel buffers supporting
> + *   the operation of copying content from the physical pages
>   */
>  static ssize_t read_mem(struct file *file, char __user *buf,
>  			size_t count, loff_t *ppos)
> @@ -166,6 +206,48 @@ static ssize_t read_mem(struct file *file, char __user *buf,
>  	return err;
>  }
>  
> +/**
> + * write_mem - write to physical memory (/dev/mem).
> + * @file: file structure for the device.
> + * @buf: user-space buffer containing the data to write.
> + * @count: number of bytes to write.
> + * @ppos: pointer to the current file position, representing the physical
> + *        address to write to.
> + *
> + * Function's expectations:

Expectation normally means "stuff that should be done before this is
called", not "what this function is going to do" which is what you have
documented here.

> + * - This function shall check if the value pointed by ppos exceeds the
> + *   maximum addressable physical address;
> + *
> + * - This function shall check if the physical address range to be written
> + *   is valid (i.e. it falls within a memory block and if it can be mapped
> + *   to the kernel address space);
> + *
> + * - For each memory page falling in the physical range to be written
> + *   [ppos, ppos + count - 1]:
> + *   - this function shall check if user space access is allowed;
> + *
> + *   - the content from the user space buffer shall be copied to the page range
> + *     falling within the physical range to be written if access is allowed;
> + *
> + *   - the data to be copied from the user space buffer (for the page range
> + *     falling within the range to be written) shall be skipped:
> + *     - if access to the memory page is restricted or,
> + *     - if the current page is page 0 on HW architectures where page 0 is not
> + *       mapped.
> + *
> + * - The file position '*ppos' shall be advanced by the number of bytes
> + *   successfully copied from user space (including skipped bytes).

No short summary first of what the function is supposed to do normally?
Or are you relying on the few words at the top to summarize that?

thanks,

gre gk-h

Powered by blists - more mailing lists