netdev - Re: [PATCH net-next] net: reformat kdoc return statements

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3c75efb4-dc04-ebad-ce1d-98bcc6569c84@gmail.com>
Date: Mon, 18 Nov 2024 10:32:47 +0000
From: Edward Cree <ecree.xilinx@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, davem@...emloft.net
Cc: netdev@...r.kernel.org, edumazet@...gle.com, pabeni@...hat.com,
 pablo@...filter.org, linux@...linux.org.uk, richardcochran@...il.com,
 johannes@...solutions.net, loic.poulain@...aro.org, ryazanov.s.a@...il.com,
 dsahern@...nel.org, wintera@...ux.ibm.com, hawk@...nel.org,
 ilias.apalodimas@...aro.org, jhs@...atatu.com, jiri@...nulli.us,
 przemyslaw.kitszel@...el.com, netfilter-devel@...r.kernel.org,
 linux-wireless@...r.kernel.org
Subject: Re: [PATCH net-next] net: reformat kdoc return statements

On 15/11/2024 16:36, Jakub Kicinski wrote:
> kernel-doc -Wall warns about missing Return: statement for non-void
> functions. We have a number of kdocs in our headers which are missing
> the colon, IOW they use
>  * Return some value
> or
>  * Returns some value
> 
> Having the colon makes some sense, it should help kdoc parser avoid
> false positives. So add them. This is mostly done with a sed script,
> and removing the unnecessary cases (mostly the comments which aren't
> kdoc).
> 
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
...
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0aae346d919e..ed549a2e02b2 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
...
> @@ -3548,7 +3548,7 @@ static inline void netdev_tx_sent_queue(struct netdev_queue *dev_queue,
>   * that they should not test BQL status themselves.
>   * We do want to change __QUEUE_STATE_STACK_XOFF only for the last
>   * skb of a batch.
> - * Returns true if the doorbell must be used to kick the NIC.
> + * Return true if the doorbell must be used to kick the NIC.

Think the colon went missing here.

>   */
>  static inline bool __netdev_tx_sent_queue(struct netdev_queue *dev_queue,
>  					  unsigned int bytes,
> @@ -3802,7 +3802,7 @@ static inline bool netif_attr_test_mask(unsigned long j,
>   *	@online_mask: bitmask for CPUs/Rx queues that are online
>   *	@nr_bits: number of bits in the bitmask
>   *
> - * Returns true if a CPU/Rx queue is online.
> + * Returns: true if a CPU/Rx queue is online.
>   */
>  static inline bool netif_attr_test_online(unsigned long j,
>  					  const unsigned long *online_mask,
> @@ -3822,7 +3822,7 @@ static inline bool netif_attr_test_online(unsigned long j,
>   *	@srcp: the cpumask/Rx queue mask pointer
>   *	@nr_bits: number of bits in the bitmask
>   *
> - * Returns >= nr_bits if no further CPUs/Rx queues set.
> + * Returns: >= nr_bits if no further CPUs/Rx queues set.
>   */
>  static inline unsigned int netif_attrmask_next(int n, const unsigned long *srcp,
>  					       unsigned int nr_bits)

I agree with Johannes here, it ought to be something like
 * Returns: next CPU in mask, or >= nr_bits if no further CPUs/Rx
 * queues set.
but understand if you don't want to include a semantic change in
this mechanical reformat patch.

> diff --git a/include/linux/phylink.h b/include/linux/phylink.h
> index 5c01048860c4..fe0d005cd5d8 100644
> --- a/include/linux/phylink.h
> +++ b/include/linux/phylink.h
...> @@ -464,8 +464,8 @@ struct phylink_pcs_ops {
>   * mask. Phylink will propagate the changes to the advertising mask. See the
>   * &struct phylink_mac_ops validate() method.
>   *
> - * Returns -EINVAL if the interface mode/autoneg mode is not supported.
> - * Returns non-zero positive if the link state can be supported.
> + * Returns: -EINVAL if the interface mode/autoneg mode is not supported.
> + * Returns: non-zero positive if the link state can be supported.

Does having multiple 'Returns:' sections in kdoc work?  I think the
right way to write this is
 * Returns:
 * * -EINVAL if the interface mode/autoneg mode is not supported.
 * * non-zero positive if the link state can be supported.
(Although I'm not sure about the accuracy of this documentation; it
looks like the calling code only treats <0 as error, and several
implementations of the method return 0 in what look like success
cases.  So that "non-zero positive" looks sus.)

IDK which part of the patch got me on the CC list but fwiw you can
add my
Reviewed-by: Edward Cree <ecree.xilinx@...il.com>
for the whole thing to v2 with the double-Returns fixed.