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

[Edward Cree <ecree.xilinx@gmail.com>]

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@kernel.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@gmail.com>
for the whole thing to v2 with the double-Returns fixed.