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