linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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>
Subject: Re: [PATCH][RESEND] docs: filesystems: sysfs: convert sysfs.txt to reST
Date: Sat, 09 Nov 2019 18:36:16 +0530	[thread overview]
Message-ID: <cd9dbb3704d0a39a161c3e4df8fcd9f84bbc5b03.camel@gmail.com> (raw)
In-Reply-To: <20191107120455.29a4c155@lwn.net>

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
> 


  reply	other threads:[~2019-11-09 13:06 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-05  7:18 [PATCH][RESEND] docs: filesystems: sysfs: convert sysfs.txt to reST Jaskaran Singh
2019-11-07 19:04 ` Jonathan Corbet
2019-11-09 13:06   ` Jaskaran Singh [this message]
2019-11-12 17:04     ` Jonathan Corbet
2019-11-13  7:30       ` Jaskaran Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cd9dbb3704d0a39a161c3e4df8fcd9f84bbc5b03.camel@gmail.com \
    --to=jaskaransingh7654321@gmail.com \
    --cc=christian@brauner.io \
    --cc=corbet@lwn.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=hofrat@osadl.org \
    --cc=jeffrey.t.kirsher@intel.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mchehab+samsung@kernel.org \
    --cc=neilb@suse.com \
    --cc=skhan@linuxfoundation.org \
    --cc=stefanha@redhat.com \
    --cc=tobin@kernel.org \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).