Message-ID: <20170515142239.00389f0b@vento.lan>
Date:   Mon, 15 May 2017 14:22:39 -0300
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Darren Hart <dvhart@...radead.org>,
        linux-kernel@...r.kernel.org,
        Linux Doc Mailing List <linux-doc@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 04/36] mutex, futex: adjust kernel-doc markups to
 generate ReST

Em Mon, 15 May 2017 13:49:19 +0200
Peter Zijlstra <peterz@...radead.org> escreveu:

> On Mon, May 15, 2017 at 01:29:58PM +0300, Jani Nikula wrote:
> > On Mon, 15 May 2017, Peter Zijlstra <peterz@...radead.org> wrote:  
> > > The intention is to aid readability. Making comments worse so that some
> > > retarded script can generate better html or whatnot is just that,
> > > retarded.
> > >
> > > Code matters, generated documentation not so much. I'll take a comment
> > > that reads well over one that generates pretty html any day.  
> > 
> > The deal is that if you start your comments with "/**" they'll be
> > processed with the retarded script to produce pretty html.
> > 
> > For the most part the comments that generate pretty html also read well,
> > and we don't expect or want anyone to go overboard with markup. I don't
> > think it's unreasonable to make small concessions to improve generated
> > documentation for people who care about it even if you don't.  
> 
> No. Such a concession has pure negative value. It opens the door to more
> patches converting this or that comment to be prettier or whatnot. And
> before you know it there's a Markus like idiot spamming you with dozens
> of crap patches to prettify the generated crud.

I see your point. Nobody wants a pile of senseless random prettify patches
on their queue. Yet, on the other hand, nobody wants lots of warnings/errors
produced when building the Kernel or the documentation, as it can ride
important things that would require fixes. So, subsystem maintainers
need to find what works best for the subsystems they care of. That's
not different than accepting/rejecting a random patch.

That's said, from my side, I don't like the way ReST handle indentation.
I would have preferred some markup dialect that would be less sensitive
to it. My personal preference were to use docutils or doxygen (but I'm 
pretty sure they would have other limitations). 

Yet, ReST is not a bad choice, as it allows extending its syntax by 
writing Python scripts and adding  to our tree, with has been an 
interesting feature to extend it to our needs.

Yet, every parser/dialect have limitations. We have to deal with it
somehow.

Currently, kernel-doc avoids some indentation issues. For example,
in the code code below:

	/**
	 *<tab>@v:<tab>foo
	 *<tab><tab>bar
	 ...
	 */

The position of '@v' output, in ReST, would mangle indentation,
depending on the way it is converted. Yet, kernel-doc handles it
well. So, at least on some cases, kernel-doc works fine with 
indentation differences, making it transparent to the user. 
The above produces the following ReST output:

		**Parameters**
	
		``v``
		  foo
		  bar

Both "Parameters" and "v" will be bold; "v" will use a monospaced
font[1].

So, at least for most parameter/description indentation, kernel-doc
does the right thing.

[1 ] Btw, the *only* way I found on ReST notation to produce a bold
     monotonic font is to use:

	``foo``
	  bar

    As doing **``foo``** or ``**foo**`` won't work - at least with
    Sphinx up to version 1.4.

At least on media, some vars are enums, and we want to describe
the possible values used at enums, like on this kernel-doc comment
snippet:

 * Entities have flags that describe the entity capabilities and state:
 *
 * %MEDIA_ENT_FL_DEFAULT
 *    indicates the default entity for a given type.
 *    This can be used to report the default audio and video devices or the
 *    default camera sensor.
 *

For it to work, kernel-doc should not mangle with whitespaces, passing the
indentation to Sphinx.

So, I fail to see a way to avoid fixing the few cases where the
indentation doesn't follow what's expected by ReST.

Yet, if you prefer a minimalist change, I can remove the ReST-specific
dialect, as in the enclosed patch.

> Not to mention that this would mean having to learn this rest crud in
> order to write these comments.
> 
> All things I'm not prepared to do.
> 
> 
> I'm all for useful comments, but I see no value _at_all_ in this
> generated nonsense. The only reason I sometimes use the docbook comment
> style is because its fairly uniform and the build bot gets you a warning
> when your function signature no longer matches with the comment. But
> if you make this painful I'll simply stop using them.

Thanks,
Mauro

[PATCH v2] mutex, futex: adjust kernel-doc markups to generate ReST

There are a few issues on some kernel-doc markups that was
causing troubles with kernel-doc output on ReST format.
Fix them.

No functional changes.

Signed-off-by: Mauro Carvalho Chehab <mchehab@...pensource.com

diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index 1127fe31645d..ffcba1f337da 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -214,9 +214,9 @@ enum mutex_trylock_recursive_enum {
  * raisins, and once those are gone this will be removed.
  *
  * Returns:
- *  MUTEX_TRYLOCK_FAILED    - trylock failed,
- *  MUTEX_TRYLOCK_SUCCESS   - lock acquired,
- *  MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
+ *  - MUTEX_TRYLOCK_FAILED    - trylock failed,
+ *  - MUTEX_TRYLOCK_SUCCESS   - lock acquired,
+ *  - MUTEX_TRYLOCK_RECURSIVE - we already owned the lock.
  */
 static inline /* __deprecated */ __must_check enum mutex_trylock_recursive_enum
 mutex_trylock_recursive(struct mutex *lock)
diff --git a/kernel/futex.c b/kernel/futex.c
index 357348a6cf6b..b8ae87d227da 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -488,7 +488,7 @@ static void drop_futex_key_refs(union futex_key *key)
  *
  * Return: a negative error code or 0
  *
- * The key words are stored in *key on success.
+ * The key words are stored in @key on success.
  *
  * For shared mappings, it's (page->index, file_inode(vma->vm_file),
  * offset_within_page).  For private mappings, it's (uaddr, current->mm).
@@ -1259,9 +1259,9 @@ static int lock_pi_update_atomic(u32 __user *uaddr, u32 uval, u32 newval)
  * @set_waiters:	force setting the FUTEX_WAITERS bit (1) or not (0)
  *
  * Return:
- *  0 - ready to wait;
- *  1 - acquired the lock;
- * <0 - error
+ *  -  0 - ready to wait;
+ *  -  1 - acquired the lock;
+ *  - <0 - error
  *
  * The hb->lock and futex_key refs shall be held by the caller.
  */
@@ -1717,9 +1717,9 @@ void requeue_pi_wake_futex(struct futex_q *q, union futex_key *key,
  * hb1 and hb2 must be held by the caller.
  *
  * Return:
- *  0 - failed to acquire the lock atomically;
- * >0 - acquired the lock, return value is vpid of the top_waiter
- * <0 - error
+ *  -  0 - failed to acquire the lock atomically;
+ *  - >0 - acquired the lock, return value is vpid of the top_waiter
+ *  - <0 - error
  */
 static int futex_proxy_trylock_atomic(u32 __user *pifutex,
 				 struct futex_hash_bucket *hb1,
@@ -1785,8 +1785,8 @@ static int futex_proxy_trylock_atomic(u32 __user *pifutex,
  * uaddr2 atomically on behalf of the top waiter.
  *
  * Return:
- * >=0 - on success, the number of tasks requeued or woken;
- *  <0 - on error
+ *  - >=0 - on success, the number of tasks requeued or woken;
+ *  -  <0 - on error
  */
 static int futex_requeue(u32 __user *uaddr1, unsigned int flags,
 			 u32 __user *uaddr2, int nr_wake, int nr_requeue,
@@ -2142,8 +2142,8 @@ static inline void queue_me(struct futex_q *q, struct futex_hash_bucket *hb)
  * be paired with exactly one earlier call to queue_me().
  *
  * Return:
- *   1 - if the futex_q was still queued (and we removed unqueued it);
- *   0 - if the futex_q was already removed by the waking thread
+ *  - 1 - if the futex_q was still queued (and we removed unqueued it);
+ *  - 0 - if the futex_q was already removed by the waking thread
  */
 static int unqueue_me(struct futex_q *q)
 {
@@ -2333,9 +2333,9 @@ static long futex_wait_restart(struct restart_block *restart);
  * acquire the lock. Must be called with the hb lock held.
  *
  * Return:
- *  1 - success, lock taken;
- *  0 - success, lock not taken;
- * <0 - on error (-EFAULT)
+ *  -  1 - success, lock taken;
+ *  -  0 - success, lock not taken;
+ *  - <0 - on error (-EFAULT)
  */
 static int fixup_owner(u32 __user *uaddr, struct futex_q *q, int locked)
 {
@@ -2422,8 +2422,8 @@ static void futex_wait_queue_me(struct futex_hash_bucket *hb, struct futex_q *q,
  * with no q.key reference on failure.
  *
  * Return:
- *  0 - uaddr contains val and hb has been locked;
- * <1 - -EFAULT or -EWOULDBLOCK (uaddr does not contain val) and hb is unlocked
+ *  -  0 - uaddr contains val and hb has been locked;
+ *  - <1 - -EFAULT or -EWOULDBLOCK (uaddr does not contain val) and hb is unlocked
  */
 static int futex_wait_setup(u32 __user *uaddr, u32 val, unsigned int flags,
 			   struct futex_q *q, struct futex_hash_bucket **hb)
@@ -2895,8 +2895,8 @@ static int futex_unlock_pi(u32 __user *uaddr, unsigned int flags)
  * called with the hb lock held.
  *
  * Return:
- *  0 = no early wakeup detected;
- * <0 = -ETIMEDOUT or -ERESTARTNOINTR
+ *  -  0 = no early wakeup detected;
+ *  - <0 = -ETIMEDOUT or -ERESTARTNOINTR
  */
 static inline
 int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
@@ -2968,8 +2968,8 @@ int handle_early_requeue_pi_wakeup(struct futex_hash_bucket *hb,
  * If 4 or 7, we cleanup and return with -ETIMEDOUT.
  *
  * Return:
- *  0 - On success;
- * <0 - On error
+ *  -  0 - On success;
+ *  - <0 - On error
  */
 static int futex_wait_requeue_pi(u32 __user *uaddr, unsigned int flags,
 				 u32 val, ktime_t *abs_time, u32 bitset,
diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c
index 198527a62149..858a07590e39 100644
--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -227,9 +227,9 @@ static void __sched __mutex_lock_slowpath(struct mutex *lock);
  * (or statically defined) before it can be locked. memset()-ing
  * the mutex to 0 is not allowed.
  *
- * ( The CONFIG_DEBUG_MUTEXES .config option turns on debugging
- *   checks that will enforce the restrictions and will also do
- *   deadlock debugging. )
+ * (The CONFIG_DEBUG_MUTEXES .config option turns on debugging
+ * checks that will enforce the restrictions and will also do
+ * deadlock debugging)
  *
  * This function is similar to (but not equivalent to) down().
  */

Powered by blists - more mailing lists