Message-ID: <CA+wEVJb-yBYZqnVs_D4VYvqYsG1k=BFsPzp3ms1=646q3dCLKA@mail.gmail.com>
Date: Fri, 22 Aug 2025 16:43:24 +0200
From: Gabriele Paoloni <gpaoloni@...hat.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: arnd@...db.de, linux-kernel@...r.kernel.org, 
	safety-architecture@...ts.elisa.tech, Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC PATCH] /dev/mem: Add initial documentation of memory_open()
 and mem_fops

Hi Greg

Many thanks for your feedbacks.

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

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
>

Powered by blists - more mailing lists