Re: [PATCH 2/3] docs: split up the driver book

From: Mauro Carvalho Chehab
Date: Tue Aug 23 2016 - 10:30:41 EST


Em Mon, 22 Aug 2016 14:57:42 -0600
Jonathan Corbet <corbet@xxxxxxx> escreveu:

> We don't need to keep it as a single large file anymore; split it up so
> that it is easier to manage and the individual sections can be read
> directly as plain files.
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jonathan Corbet <corbet@xxxxxxx>
> ---
> Documentation/driver-api/basics.rst | 120 +++++

Hi Jon,

I noticed several issues on the converted document. Just commenting
a few of them, as they all follow a pattern: kernel-doc markups
needs review during the conversion to RST, because, unfortunately,
the conversion is not transparent, as we would want to.

So, I'm pointing a few issues I noticed below.

> Documentation/driver-api/basics.rst | 120 +++++

At kernel/kthread.c, on the documentation for:

struct task_struct * kthread_create_on_node(int (*threadfn) (void *data, void * data, int node, const char namefmt[], ...)

The original description is:

* If thread is going to be bound on a particular cpu, give its node
* in @node, to get NUMA affinity for kthread stack, or else give NUMA_NO_NODE.
* When woken, the thread will run @threadfn() with @data as its
* argument. @threadfn() can either call do_exit() directly if it is a
* standalone thread for which no one will call kthread_stop(), or
* return when 'kthread_should_stop()' is true (which means
* kthread_stop() has been called). The return value should be zero
* or a negative error number; it will be passed to kthread_stop().

On the output text, you'll see two places with "@:c:func:threadfn()".

The problem here is that threadfn() is a function argument. While this
used to work with DocBooks, now with Sphinx this is not handled well.

I got some other similar cases on media. There, I opted to just remove
the () on some places, or to replace it by \(\) to avoid kernel-doc
to do the wrong thing. See changeset 564aaf69208d ("[media] doc-rst:
add some needed escape codes").

We could, instead, teach kernel-doc to be smarter, but I'm afraid
that there will always be border cases that it won't cover.

The same problem happened with:
void kmsg_dump_rewind(struct kmsg_dumper * dumper)

Also, you should notice that it added several references to
kthread_create(), with is actually a define:

include/linux/kthread.h:#define kthread_create(threadfn, data, namefmt, arg...) \

It probably makes sense to add some markup for kernel-doc to parse it.

> Documentation/driver-api/drivers.rst | 654 -------------------------

At struct device description:

bool:1 offline
Set after successful invocation of bus typeâs .:c:func:offline().

At request_firmware_nowait() description, you'all also see:

->:c:func:probe()

(won't mention about other occurrences of c: - you got the idea)

> Documentation/driver-api/message-based.rst | 30 ++

You should probably change NOTES by:

.. note::

Like on this changeset:
f58cad224796 [media] media-entry.h: Fix a note markup


The description for:

int KickStart(MPT_ADAPTER * ioc, int force, int sleepFlag)

Looked weird on my eyes. The original kernel-nano tag is:

/**
* KickStart - Perform hard reset of MPT adapter.
* @ioc: Pointer to MPT_ADAPTER structure
* @force: Force hard reset
* @sleepFlag: Specifies whether the process can sleep
*
* This routine places MPT adapter in diagnostic mode via the
* WriteSequence register, and then performs a hard reset of adapter
* via the Diagnostic register.
*
* Inputs: sleepflag - CAN_SLEEP (non-interrupt thread)
* or NO_SLEEP (interrupt thread, use mdelay)
* force - 1 if doorbell active, board fault state
* board operational, IOC_RECOVERY or
* IOC_BRINGUP and there is an alt_ioc.
* 0 else
*
* Returns:
* 1 - hard reset, READY
* 0 - no reset due to History bit, READY
* -1 - no reset due to History bit but not READY
* OR reset but failed to come READY
* -2 - no reset, could not enter DIAG mode
* -3 - reset but bad FW bit
*/
static int
KickStart(MPT_ADAPTER *ioc, int force, int sleepFlag)

The first input like:

* Inputs: sleepflag - CAN_SLEEP (non-interrupt thread)

Is internally converted to a bold output like:
*Inputs sleepflag - CAN_SLEEP (non-interrupt thread)*

And the remaining arguments will be mangled.

At returns arguments, for example, it should be changed to something
like (untested):

* Returns:
*
* - 1 - hard reset, READY
*
* - 0 - no reset due to History bit, READY
*
* - 1 - no reset due to History bit but not READY
* OR reset but failed to come READY
*
* - -2 - no reset, could not enter DIAG mode
*
* - -3 - reset but bad FW bit

So, basically, all kernel-doc tags should be reviewed for continuation
lines.

...

> diff --git a/Documentation/driver-api/index.rst b/Documentation/driver-api/index.rst
> new file mode 100644
> index 000000000000..b50c41011e47
> --- /dev/null
> +++ b/Documentation/driver-api/index.rst
> @@ -0,0 +1,24 @@
> +========================================
> +The Linux driver implementer's API guide
> +========================================
> +
> +The kernel offers a wide variety of interfaces to support the development
> +of device drivers. This document is an only somewhat organized collection
> +of some of those interfaces â it will hopefully get better over time! The
> +available subsections can be seen below.
> +
> +.. class:: toc-title
> +
> + Table of contents
> +
> +.. toctree::
> + :maxdepth: 2

I would add here a
:numbered:

This way, the entire section will be numbered, and you can remove the
manual numbers from the sections from
Documentation/driver-api/serial-interfaces.rst (patch 3/3).


> +
> + basics
> + infrastructure
> + message-based
> + sound
> + frame-buffer
> + input
> + serial-interfaces
> + miscellaneous


Regards,
Mauro