Message-ID: <CA+wEVJZntQ6Fx8VWzL-PpKWw7=vxKQewxkUfu_f01Px2EFtRZQ@mail.gmail.com>
Date: Sat, 23 Aug 2025 09:48:32 +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

On Fri, Aug 22, 2025 at 5:08 PM Greg KH <gregkh@...uxfoundation.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@...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.

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
>

Powered by blists - more mailing lists