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

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

[Gabriele Paoloni]

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 **Description** intends to define testable
function's expectations and Assumptions of Use to be met by the
user of the function.

Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.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.
+ * @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;
+ *
+ *   - 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).
+ *
+ * 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
+ * * %-EPERM - access to any of the required pages is not allowed
+ * * %-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:
+ * - 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).
+ *
+ * Context: process context.
+ *
+ * Return:
+ * * the number of bytes copied to user on success
+ * * %-EFBIG - the value pointed by ppos exceeds the maximum addressable
+ *   physical address
+ * * %-EFAULT - the physical address range is not valid or a fault happened
+ *   when copying from user
+ * * %-EPERM - access to any of the required pages is not allowed
+ */
 static ssize_t write_mem(struct file *file, const char __user *buf,
 			 size_t count, loff_t *ppos)
 {
@@ -322,6 +404,37 @@ static const struct vm_operations_struct mmap_mem_ops = {
 #endif
 };
 
+/**
+ * mmap_mem - map physical memory into user space (/dev/mem).
+ * @file: file structure for the device.
+ * @vma: virtual memory area structure describing the user mapping.
+ *
+ * Function's expectations:
+ * - This function shall check if the requested physical address range to be
+ *   mapped fits within the maximum addressable physical range;
+ *
+ * - This function shall check if the requested  physical range corresponds to
+ *   a valid physical range and if access is allowed on it;
+ *
+ * - This function shall check if the input virtual memory area can be used for
+ *   a private mapping (always OK if there is an MMU);
+ *
+ * - This function shall set the virtual memory area operations to
+ *   &mmap_mem_ops;
+ *
+ * - This function shall establish a mapping between the user-space
+ *   virtual memory area described by vma and the physical memory
+ *   range specified by vma->vm_pgoff and size;
+ *
+ * Context: process context.
+ *
+ * Return:
+ * * 0 on success
+ * * %-EAGAIN - invalid or unsupported mapping requested (remap_pfn_range()
+ *   fails)
+ * * %-EINVAL - requested physical range to be mapped is not valid
+ * * %-EPERM - no permission to access the requested physical range
+ */
 static int mmap_mem(struct file *file, struct vm_area_struct *vma)
 {
 	size_t size = vma->vm_end - vma->vm_start;
@@ -550,13 +663,44 @@ static loff_t null_lseek(struct file *file, loff_t offset, int orig)
 	return file->f_pos = 0;
 }
 
-/*
+/**
+ * memory_lseek - change the file position.
+ * @file: file structure for the device.
+ * @offset: file offset to seek to.
+ * @orig: where to start seeking from (see whence in the llseek manpage).
+ *
+ * Function's expectations:
+ * - This function shall lock the semaphore of the inode corresponding to the
+ *   input file before any operation and unlock it before returning.
+ *
+ * - This function shall check the orig value and accordingly:
+ *   - if it is equal to SEEK_CUR, the current file position shall be
+ *     incremented by the input offset;
+ *   - if it is equal to SEEK_SET, the current file position shall be
+ *     set to the input offset value;
+ *   - any other value shall result in an error condition.
+ *
+ * - Before writing the current file position, the new position value
+ *   shall be checked to not overlap with Linux ERRNO values.
+ *
+ * Assumptions of Use:
+ * - the input file pointer is expected to be valid.
+ *
+ * Notes:
  * The memory devices use the full 32/64 bits of the offset, and so we cannot
  * check against negative addresses: they are ok. The return value is weird,
  * though, in that case (0).
  *
- * also note that seeking relative to the "end of file" isn't supported:
- * it has no meaning, so it returns -EINVAL.
+ * Also note that seeking relative to the "end of file" isn't supported:
+ * it has no meaning, so passing orig equal to SEEK_END returns -EINVAL.
+ *
+ * Context: process context, locks/unlocks inode->i_rwsem
+ *
+ * Return:
+ * * the new file position on success
+ * * %-EOVERFLOW - the new position value equals or exceeds
+ *   (unsigned long long) -MAX_ERRNO
+ * * %-EINVAL - the orig parameter is invalid
  */
 static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 {
@@ -584,6 +728,32 @@ static loff_t memory_lseek(struct file *file, loff_t offset, int orig)
 	return ret;
 }
 
+/**
+ * open_port - open the I/O port device (/dev/port).
+ * @inode: inode of the device file.
+ * @filp: file structure for the device.
+ *
+ * Function's expectations:
+ * - This function shall check if the caller has sufficient capabilities to
+ *   perform raw I/O access;
+ *
+ * - This function shall check if the kernel is locked down with the
+ *   &LOCKDOWN_DEV_MEM restriction;
+ *
+ * - If the input inode corresponds to /dev/mem, the f_mapping pointer
+ *   of the input file structure shall be set to the i_mapping pointer
+ *   of the input inode;
+ *
+ * Assumptions of Use:
+ * - The input inode and filp are expected to be valid.
+ *
+ * Context: process context.
+ *
+ * Return:
+ * * 0 on success
+ * * %-EPERM - caller lacks the required capability (CAP_SYS_RAWIO)
+ * * any error returned by securty_locked_down()
+ */
 static int open_port(struct inode *inode, struct file *filp)
 {
 	int rc;
@@ -691,6 +861,33 @@ static const struct memdev {
 #endif
 };
 
+/**
+ * memory_open - set the filp f_op to the memory device fops and invoke open().
+ * @inode: inode of the device file.
+ * @filp: file structure for the device.
+ *
+ * Function's expectations:
+ * - This function shall retrieve the minor number associated with the input
+ *   inode and the memory device corresponding to such minor number;
+ *
+ * - The file operations pointer shall be set to the memory device file operations;
+ *
+ * - The file mode member of the input filp shall be OR'd with the device mode;
+ *
+ * - The memory device open() file operation shall be invoked.
+ *
+ * Assumptions of Use:
+ * - The input inode and filp are expected to be non-NULL.
+ *
+ * Context: process context.
+ *
+ * Return:
+ * * 0 on success
+ * * %-ENXIO - the minor number corresponding to the input inode cannot be
+ *   associated with any device or the corresponding device has a NULL fops
+ *   pointer
+ * * any error returned by the device specific open function pointer
+ */
 static int memory_open(struct inode *inode, struct file *filp)
 {
 	int minor;
-- 
2.48.1

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

[Greg KH]

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@redhat.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

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

[Greg KH]

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.
> 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.
> 
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.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())?

I think that check implies you don't want to read any more memory,
right?  Try changing it and see what happens :)

> 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...

How that you could have a negative address, or how you would return 0?

Also, you should cc: the mm developers, they touch this file all the
time and know it quite well (recent changes to /dev/zero just got added
in the past few days from them.)

thanks,

greg k-h

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

[Gabriele Paoloni]

Hi Greg

Many thanks for your feedbacks.

On Thu, Aug 21, 2025 at 7:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> 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?

When writing these comments I referred to
https://docs.kernel.org/doc-guide/kernel-doc.html; in these guideline I found
<<It is good practice to also provide kernel-doc formatted documentation for
functions externally visible to other kernel files (not marked static). We also
recommend providing kernel-doc formatted documentation for private (file
static) routines, for consistency of kernel source code layout. This is lower
priority and at the discretion of the maintainer of that kernel source file.>>
So it seems that it is OK to document static functions...

Also I ran "scripts/kernel-doc -v drivers/char/mem.c" to verify the doc to
be built correctly.
Am I missing something?

>
> > 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?

Good point. The short answer is that "function's expectations" and
"Assumptions of Use" are not defined today.
However I need to provide some background here...
Following the LPC 2024 session "Improving kernel design documentation
and involving experts" (see [1]), the Safety Architecture working group
from the ELISA project started defining a template for writing SW low level
requirements in the Kernel (see [2]); also in consideration of how-to
maintain these requirements along with the code evolution.

After the initial draft of such a template we had helpful feedbacks from
Steven Rostedt and we decided to get Kernel code documented using
the current kernel-doc format without all the formalisms defined in the
requirements template.
The reason for this is to focus on the semantic aspects of SW
requirements and show the value of defining clear and testable "code
expectations" before finalising the aspects of the syntax and automation
around it. So accordingly I have also sent out [3] and [4] that are using
the same formalism used here.

Having said that, maybe as intermediate step before introducing the
requirements formalism, I could propose a patch for the doc-guide that
would explain the goal of function's expectations and Assumptions of
Use (AoUs) and associated best practices...?

[1] https://lpc.events/event/18/contributions/1894/
[2] https://docs.google.com/document/d/1c7S7YAledHP2EEQ2nh26Ibegij-XPNuUFkrFLtJPlzs/edit?usp=sharing
[3] https://lore.kernel.org/linux-trace-kernel/20250814122141.109076-1-gpaoloni@redhat.com/
[4] https://lore.kernel.org/linux-trace-kernel/20250814122206.109096-1-gpaoloni@redhat.com/

>
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.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.

I meant that it is a special file under /dev/, however I see your point
so I can change as "file structure 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.
> > + *
> > + * 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.

Sorry, you are 100% right here. "Shall" is commonly used in the
requirements formalism to define an expected and verifiable
behaviour (a mandatory goal for the code implementation).
In contrast "should" would be used for a non-mandatory but
recommended goal.
As mentioned above I need probably to write a guideline that
define HowTo write function's expectations and AoUs

>
> > + *
> > + *   - 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?

I just meant to say that even when zeros are copied (effectively skipping the
data in memory), such zeros must be accounted as successfully copied bytes...

>
> > + *
> > + * 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"?

Yes better thanks, will fix it.

>
> > + * * %-EPERM - access to any of the required pages is not allowed
>
> Which pages?  userspace or kernel?

From my understanding -EPERM is returned when any of the page falling
in the requested physical range fails the check page_is_allowed(); so maybe
"access to any of the required physical pages is not allowed" is a better
phrasing...?

>
> > + * * %-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.

Right sorry again....this goes back to what I've written above...

>
> > + * - 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?

Function's expectations would define the testable behaviours (so they
are broken down into detailed expectations); nothing prevents to provide
an informative intro above "Function's expectations"; I could clarify this
in the patch for the doc-guide  and revisit this patch with informative intros
for all the functions....

Many thanks again
Gab

>
> thanks,
>
> gre gk-h
>

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

[Greg KH]

On Fri, Aug 22, 2025 at 04:43:24PM +0200, Gabriele Paoloni wrote:
> Hi Greg
> 
> Many thanks for your feedbacks.
> 
> On Thu, Aug 21, 2025 at 7:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > 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?
> 
> When writing these comments I referred to
> https://docs.kernel.org/doc-guide/kernel-doc.html; in these guideline I found
> <<It is good practice to also provide kernel-doc formatted documentation for
> functions externally visible to other kernel files (not marked static). We also
> recommend providing kernel-doc formatted documentation for private (file
> static) routines, for consistency of kernel source code layout. This is lower
> priority and at the discretion of the maintainer of that kernel source file.>>
> So it seems that it is OK to document static functions...
> 
> Also I ran "scripts/kernel-doc -v drivers/char/mem.c" to verify the doc to
> be built correctly.
> Am I missing something?

No, that's good, I haven't checked to make sure that static functions
properly show up in the documentation in a while, just wanted to make
sure.

> > > 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?
> 
> Good point. The short answer is that "function's expectations" and
> "Assumptions of Use" are not defined today.

Then how are we supposed to be able to review this proposed patch?

> However I need to provide some background here...
> Following the LPC 2024 session "Improving kernel design documentation
> and involving experts" (see [1]), the Safety Architecture working group
> from the ELISA project started defining a template for writing SW low level
> requirements in the Kernel (see [2]); also in consideration of how-to
> maintain these requirements along with the code evolution.
> 
> After the initial draft of such a template we had helpful feedbacks from
> Steven Rostedt and we decided to get Kernel code documented using
> the current kernel-doc format without all the formalisms defined in the
> requirements template.
> The reason for this is to focus on the semantic aspects of SW
> requirements and show the value of defining clear and testable "code
> expectations" before finalising the aspects of the syntax and automation
> around it. So accordingly I have also sent out [3] and [4] that are using
> the same formalism used here.
> 
> Having said that, maybe as intermediate step before introducing the
> requirements formalism, I could propose a patch for the doc-guide that
> would explain the goal of function's expectations and Assumptions of
> Use (AoUs) and associated best practices...?

Yes, that would be good, we can bikeshed over that before worrying about
how to review any patches.  But, include this patch in a series so that
we can see how it looks together, otherwise it's going to be hard to
determine usage.

> > > -/*
> > > - * 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.
> 
> I meant that it is a special file under /dev/, however I see your point
> so I can change as "file structure associated with /dev/mem"

Why not actually say the structure name?  Otherwise over time you are
going to have to get very creative describing structures without using
their types.

> > > + * @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.
> 
> Sorry, you are 100% right here. "Shall" is commonly used in the
> requirements formalism to define an expected and verifiable
> behaviour (a mandatory goal for the code implementation).
> In contrast "should" would be used for a non-mandatory but
> recommended goal.
> As mentioned above I need probably to write a guideline that
> define HowTo write function's expectations and AoUs

Yes, you need to define that.  And define if "shall" or "must" should be
used, as some specs like to use vs the other, and some both.  Let's pick
one and be done with it.

> > > + *
> > > + *   - 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?
> 
> I just meant to say that even when zeros are copied (effectively skipping the
> data in memory), such zeros must be accounted as successfully copied bytes...

But again, why is zero here an issue?  "number of bytes successfully
copied to user space" defines that, otherwise you are asking for someone
to actually start parsing the data for no good reason.

> > > + *
> > > + * 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"?
> 
> Yes better thanks, will fix it.
> 
> >
> > > + * * %-EPERM - access to any of the required pages is not allowed
> >
> > Which pages?  userspace or kernel?
> 
> >From my understanding -EPERM is returned when any of the page falling
> in the requested physical range fails the check page_is_allowed(); so maybe
> "access to any of the required physical pages is not allowed" is a better
> phrasing...?

Yes.

> > > + * - 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?
> 
> Function's expectations would define the testable behaviours (so they
> are broken down into detailed expectations); nothing prevents to provide
> an informative intro above "Function's expectations"; I could clarify this
> in the patch for the doc-guide  and revisit this patch with informative intros
> for all the functions....

"testable behavior" is going to be very hard given that you are
describing an internal-to-the-kernel function.  Good luck!

thanks,

greg k-h

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

[Gabriele Paoloni]

On Thu, Aug 21, 2025 at 7:35 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> 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.
> > 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.
> >
> > Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.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())?
>
> I think that check implies you don't want to read any more memory,
> right?  Try changing it and see what happens :)

Right I see, 0 is better to gracefully terminate a read routine of
a program that may have read the whole physical range..

>
> > 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...
>
> How that you could have a negative address, or how you would return 0?

Today the note above memory_lseek() states

 * The memory devices use the full 32/64 bits of the offset, and so we cannot
 * check against negative addresses: they are ok. The return value is weird,
 * though, in that case (0).

I interpret this as "if offset and orig lead to an f_pos that is
negative the function
returns 0"....however I cannot see where this happen...

>
> Also, you should cc: the mm developers, they touch this file all the
> time and know it quite well (recent changes to /dev/zero just got added
> in the past few days from them.)

Sorry, I just used the addresses returned by get_maintainers...
would adding linux-mm@kvack.org work ok?

Thanks
Gab

> thanks,
>
> greg k-h
>

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

[Gabriele Paoloni]

On Fri, Aug 22, 2025 at 5:08 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Aug 22, 2025 at 04:43:24PM +0200, Gabriele Paoloni wrote:
> > Hi Greg
> >
> > Many thanks for your feedbacks.
> >
> > On Thu, Aug 21, 2025 at 7:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > 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?
> >
> > When writing these comments I referred to
> > https://docs.kernel.org/doc-guide/kernel-doc.html; in these guideline I found
> > <<It is good practice to also provide kernel-doc formatted documentation for
> > functions externally visible to other kernel files (not marked static). We also
> > recommend providing kernel-doc formatted documentation for private (file
> > static) routines, for consistency of kernel source code layout. This is lower
> > priority and at the discretion of the maintainer of that kernel source file.>>
> > So it seems that it is OK to document static functions...
> >
> > Also I ran "scripts/kernel-doc -v drivers/char/mem.c" to verify the doc to
> > be built correctly.
> > Am I missing something?
>
> No, that's good, I haven't checked to make sure that static functions
> properly show up in the documentation in a while, just wanted to make
> sure.

Thanks!

>
> > > > 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?
> >
> > Good point. The short answer is that "function's expectations" and
> > "Assumptions of Use" are not defined today.
>
> Then how are we supposed to be able to review this proposed patch?
>
> > However I need to provide some background here...
> > Following the LPC 2024 session "Improving kernel design documentation
> > and involving experts" (see [1]), the Safety Architecture working group
> > from the ELISA project started defining a template for writing SW low level
> > requirements in the Kernel (see [2]); also in consideration of how-to
> > maintain these requirements along with the code evolution.
> >
> > After the initial draft of such a template we had helpful feedbacks from
> > Steven Rostedt and we decided to get Kernel code documented using
> > the current kernel-doc format without all the formalisms defined in the
> > requirements template.
> > The reason for this is to focus on the semantic aspects of SW
> > requirements and show the value of defining clear and testable "code
> > expectations" before finalising the aspects of the syntax and automation
> > around it. So accordingly I have also sent out [3] and [4] that are using
> > the same formalism used here.
> >
> > Having said that, maybe as intermediate step before introducing the
> > requirements formalism, I could propose a patch for the doc-guide that
> > would explain the goal of function's expectations and Assumptions of
> > Use (AoUs) and associated best practices...?
>
> Yes, that would be good, we can bikeshed over that before worrying about
> how to review any patches.  But, include this patch in a series so that
> we can see how it looks together, otherwise it's going to be hard to
> determine usage.

Got it, I'll add such a guideline proposal in v2, thanks.

>
> > > > -/*
> > > > - * 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.
> >
> > I meant that it is a special file under /dev/, however I see your point
> > so I can change as "file structure associated with /dev/mem"
>
> Why not actually say the structure name?  Otherwise over time you are
> going to have to get very creative describing structures without using
> their types.

Sure it can work; as in <<"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.
> > > > + *
> > > > + * 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.
> >
> > Sorry, you are 100% right here. "Shall" is commonly used in the
> > requirements formalism to define an expected and verifiable
> > behaviour (a mandatory goal for the code implementation).
> > In contrast "should" would be used for a non-mandatory but
> > recommended goal.
> > As mentioned above I need probably to write a guideline that
> > define HowTo write function's expectations and AoUs
>
> Yes, you need to define that.  And define if "shall" or "must" should be
> used, as some specs like to use vs the other, and some both.  Let's pick
> one and be done with it.

Yep, will do in v2, thanks!

>
> > > > + *
> > > > + *   - 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?
> >
> > I just meant to say that even when zeros are copied (effectively skipping the
> > data in memory), such zeros must be accounted as successfully copied bytes...
>
> But again, why is zero here an issue?  "number of bytes successfully
> copied to user space" defines that, otherwise you are asking for someone
> to actually start parsing the data for no good reason.

Well, when I read "successfully copied" I interpret it as "content successfully
copied", whereas in some cases the content is not copied and the user space
buffer is filled with zeros. So here I was trying to explain that
"successfully copied"
also includes the case where zeros are passed in place of the actual content...

>
> > > > + *
> > > > + * 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"?
> >
> > Yes better thanks, will fix it.
> >
> > >
> > > > + * * %-EPERM - access to any of the required pages is not allowed
> > >
> > > Which pages?  userspace or kernel?
> >
> > >From my understanding -EPERM is returned when any of the page falling
> > in the requested physical range fails the check page_is_allowed(); so maybe
> > "access to any of the required physical pages is not allowed" is a better
> > phrasing...?
>
> Yes.
>
> > > > + * - 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?
> >
> > Function's expectations would define the testable behaviours (so they
> > are broken down into detailed expectations); nothing prevents to provide
> > an informative intro above "Function's expectations"; I could clarify this
> > in the patch for the doc-guide  and revisit this patch with informative intros
> > for all the functions....
>
> "testable behavior" is going to be very hard given that you are
> describing an internal-to-the-kernel function.  Good luck!

Well that is something to be figured out (step by step :-) )

Many thanks
Gab

>
> thanks,
>
> greg k-h
>

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

[Greg KH]

On Sat, Aug 23, 2025 at 09:48:32AM +0200, Gabriele Paoloni wrote:
> > > > > + * - 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?
> > >
> > > Function's expectations would define the testable behaviours (so they
> > > are broken down into detailed expectations); nothing prevents to provide
> > > an informative intro above "Function's expectations"; I could clarify this
> > > in the patch for the doc-guide  and revisit this patch with informative intros
> > > for all the functions....
> >
> > "testable behavior" is going to be very hard given that you are
> > describing an internal-to-the-kernel function.  Good luck!
> 
> Well that is something to be figured out (step by step :-) )

Well what is your end-goal here?  Do you need/want all functions in the
kernel to be documented like this, or do you only want those that are
describing user-visable functionality?

What is the requirement that is causing you to do this work?

thanks,

greg k-h

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

[Gabriele Paoloni]

On Sat, Aug 23, 2025 at 10:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, Aug 23, 2025 at 09:48:32AM +0200, Gabriele Paoloni wrote:
> > > > > > + * - 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?
> > > >
> > > > Function's expectations would define the testable behaviours (so they
> > > > are broken down into detailed expectations); nothing prevents to provide
> > > > an informative intro above "Function's expectations"; I could clarify this
> > > > in the patch for the doc-guide  and revisit this patch with informative intros
> > > > for all the functions....
> > >
> > > "testable behavior" is going to be very hard given that you are
> > > describing an internal-to-the-kernel function.  Good luck!
> >
> > Well that is something to be figured out (step by step :-) )
>
> Well what is your end-goal here?  Do you need/want all functions in the
> kernel to be documented like this, or do you only want those that are
> describing user-visable functionality?

Yes indeed. Possibly we could discuss with the KernelCI folks how
to define test cases and trace these to the code specifications.
In terms of priorities I would start from the functions that are
directly exposed
externally. I.e. those that specify the behaviour of syscalls, interrupt and
exception handlers, exported Kernel symbols.

>
> What is the requirement that is causing you to do this work?

The end goal would be to enhance the dependability of Linux so to make it
easier to adopt it in mission critical industries.
Depending on the industry and on the target integrity level, there can be
different demands on the documentation depth, however in general there
are two key principles that determine the dependability of SW:
a) The SW must be specified so that the integrator knows the expected
behaviour and the assumptions of use to be met to use the SW correctly
b) The SW must be verified (through tests and other measures) so that
it can be trusted to behave as specified in point a).

Now regardless of the target integrity level, the starting point would be
to specify the behaviour that is directly visible to the user and to kernel
drivers (as mentioned above).

Many Thanks
Gab


>
> thanks,
>
> greg k-h
>