Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <cd9dbb3704d0a39a161c3e4df8fcd9f84bbc5b03.camel@gmail.com>
Subject: Re: [PATCH][RESEND] docs: filesystems: sysfs: convert sysfs.txt to
 reST
From:   Jaskaran Singh <jaskaransingh7654321@gmail.com>
To:     Jonathan Corbet <corbet@lwn.net>
Cc:     linux-kernel@vger.kernel.org, mchehab+samsung@kernel.org,
        christian@brauner.io, neilb@suse.com, willy@infradead.org,
        tobin@kernel.org, stefanha@redhat.com, hofrat@osadl.org,
        gregkh@linuxfoundation.org, jeffrey.t.kirsher@intel.com,
        linux-doc@vger.kernel.org, skhan@linuxfoundation.org,
        "linux-kernel-mentees@lists.linuxfoundation.org" 
        <linux-kernel-mentees@lists.linuxfoundation.org>
Date:   Sat, 09 Nov 2019 18:36:16 +0530
In-Reply-To: <20191107120455.29a4c155@lwn.net>
References: <20191105071846.GA28727@localhost.localdomain>
         <20191107120455.29a4c155@lwn.net>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.32.4 (3.32.4-1.fc30) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 2019-11-07 at 12:04 -0700, Jonathan Corbet wrote:
> On Tue, 5 Nov 2019 12:48:46 +0530
> Jaskaran Singh <jaskaransingh7654321@gmail.com> wrote:
> 
> > This patch converts sysfs.txt to sysfs.rst, and adds a
> > corresponding
> > entry in index.rst.
> > 
> > Most of the whitespacing and indentation is kept similar to the
> > original document.
> > 
> > Changes to the original document include:
> > 
> >  - Adding an authors statement in the header.
> >  - Replacing the underscores in the title with asterisks. This is
> > so
> >    that the "The" in the title appears in italics in HTML.
> >  - Replacing the tilde (~) headings with equal signs, for reST
> > section
> >    headings.
> >  - List out the helper macros with backquotes and corresponding
> > description
> >    on the next line.
> >  - Placing C code and shell code in reST code blocks, with an
> > indentation
> >    of an 8 length tab.
> > 
> > Signed-off-by: Jaskaran Singh <jaskaransingh7654321@gmail.com>
> 
> Thanks for working to improve the documentation.  There are some
> problems
> here, none of which are your creation, but I would sure like to
> resolve
> them while working with this document.
> 
> The first of these is that Documentation/filesystems is really the
> wrong
> place for this file - it's covering the internal API for subsystems
> that
> want to create entries in sysfs.  IMO, it belongs in either the
> driver-API
> manual or the core-API manual - probably the latter.
> 
> >  Documentation/filesystems/index.rst           |   1 +
> >  .../filesystems/{sysfs.txt => sysfs.rst}      | 323 ++++++++++--
> > ------
> >  2 files changed, 189 insertions(+), 135 deletions(-)
> >  rename Documentation/filesystems/{sysfs.txt => sysfs.rst} (60%)
> > 
> > diff --git a/Documentation/filesystems/index.rst
> > b/Documentation/filesystems/index.rst
> > index 2c3a9f761205..18b5ea780b9b 100644
> > --- a/Documentation/filesystems/index.rst
> > +++ b/Documentation/filesystems/index.rst
> > @@ -46,4 +46,5 @@ Documentation for filesystem implementations.
> >  .. toctree::
> >     :maxdepth: 2
> >  
> > +   sysfs
> >     virtiofs
> > diff --git a/Documentation/filesystems/sysfs.txt
> > b/Documentation/filesystems/sysfs.rst
> > similarity index 60%
> > rename from Documentation/filesystems/sysfs.txt
> > rename to Documentation/filesystems/sysfs.rst
> > index ddf15b1b0d5a..de0de5869323 100644
> > --- a/Documentation/filesystems/sysfs.txt
> > +++ b/Documentation/filesystems/sysfs.rst
> > @@ -1,15 +1,18 @@
> > +======================================================
> > +sysfs - *The* filesystem for exporting kernel objects.
> > +======================================================
> >  
> > -sysfs - _The_ filesystem for exporting kernel objects. 
> 
> Nits: We can really just drop emphasis like that, it doesn't really
> help
> anybody.  Also the period can go on section headers.
> 
> > +Authors:
> >  
> > -Patrick Mochel	<mochel@osdl.org>
> > -Mike Murphy <mamurph@cs.clemson.edu>
> > +- Patrick Mochel	<mochel@osdl.org>
> > +- Mike Murphy   	<mamurph@cs.clemson.edu>
> 
> I would be absolutely amazed if either of those email addresses works
> at
> this point.  I'd take them out.
> 
> > -Revised:    16 August 2011
> > -Original:   10 January 2003
> > +| Revised:    16 August 2011
> > +| Original:   10 January 2003
> 
> Dates like that are a red flag.  See below.
> 
> >  What it is:
> > -~~~~~~~~~~~
> > +===========
> >  
> >  sysfs is a ram-based filesystem initially based on ramfs. It
> > provides
> >  a means to export kernel data structures, their attributes, and
> > the 
> > @@ -21,16 +24,18 @@ interface.
> >  
> >  
> >  Using sysfs
> > -~~~~~~~~~~~
> > +===========
> >  
> >  sysfs is always compiled in if CONFIG_SYSFS is defined. You can
> > access
> >  it by doing:
> >  
> > -    mount -t sysfs sysfs /sys 
> > +.. code-block:: sh
> > +
> > +	mount -t sysfs sysfs /sys
> 
> In the spirit of minimal markup, I'd do the above as:
> 
>    it by doing::
> 
> 	mount -t sysfs sysfs /sys
> 
> But then I know that others are much more fond of .. code-block and
> syntax
> highlighting than I am.
> 
> >  Directory Creation
> > -~~~~~~~~~~~~~~~~~~
> > +==================
> >  
> >  For every kobject that is registered with the system, a directory
> > is
> >  created for it in sysfs. That directory is created as a
> > subdirectory
> > @@ -48,7 +53,7 @@ only modified directly by the function
> > sysfs_schedule_callback().
> >  
> >  
> >  Attributes
> > -~~~~~~~~~~
> > +==========
> >  
> >  Attributes can be exported for kobjects in the form of regular
> > files in
> >  the filesystem. Sysfs forwards file I/O operations to methods
> > defined
> > @@ -67,15 +72,16 @@ you publicly humiliated and your code rewritten
> > without notice.
> >  
> >  An attribute definition is simply:
> >  
> > -struct attribute {
> > -        char                    * name;
> > -        struct module		*owner;
> > -        umode_t                 mode;
> > -};
> > +.. code-block:: c
> >  
> > +	struct attribute {
> > +		char                    * name;
> > +		struct module		*owner;
> > +		umode_t                 mode;
> > +	};
> 
> Here is where we go pretty far off the rails.  If you go looking in
> include/linux/sysfs.h, the actual definition of this structure is:
> 
> struct attribute {
> 	const char		*name;
> 	umode_t			mode;
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> 	bool			ignore_lockdep:1;
> 	struct lock_class_key	*key;
> 	struct lock_class_key	skey;
> #endif
> };
> 
> Most notably, the owner field went away quite some time ago.
> 
> Documentation like this is not really useful to anybody; once a
> reader
> realizes it doesn't describe current reality, they will justifiably
> disregard it.  This isn't your fault, of course, but converting
> something
> like this to RST gives the illusion that it has been updated, when
> that is
> very much not the case.
> 
> At a bare minimum, an effort like this needs to put a big flashing
> warning
> at the top of the file.  But it would be soooooo much better to
> actually
> update the content as well.
> 
> The best way to do that would be to annotate the source with proper
> kerneldoc comments, then pull them into the documentation rather than
> repeating the information here.  Is there any chance you might be up
> for
> taking on a task like this?
> 


Sure! I'll send the documentation patch(es) followed by a v2 for this
patch.

Cheers,
Jaskaran.


> Thanks,
> 
> jon
>