Message-ID: <2025082209-laborer-hesitate-76cc@gregkh>
Date: Fri, 22 Aug 2025 17:08:08 +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,
	Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [RFC PATCH] /dev/mem: Add initial documentation of memory_open()
 and mem_fops

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

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

Powered by blists - more mailing lists