On Thu, 2019-11-07 at 12:04 -0700, Jonathan Corbet wrote:
> On Tue, 5 Nov 2019 12:48:46 +0530
> Jaskaran Singh <jaskaransingh7654321@xxxxxxxxx> 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@xxxxxxxxx>
>
> 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@xxxxxxxx>
> > -Mike Murphy <mamurph@xxxxxxxxxxxxxx>
> > +- Patrick Mochel <mochel@xxxxxxxx>
> > +- Mike Murphy <mamurph@xxxxxxxxxxxxxx>
>
> 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
>
