[PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Jeff Johnson]

Commit 7636c9a6e7d7 ("wifi: ath11k: Add multi TX ring support for WCN6750")
added HAL_RX_BUF_RBM_SW4_BM to enum hal_rx_buf_return_buf_manager. However,
as flagged by the kernel-doc script, the documentation was not updated:

drivers/net/wireless/ath/ath11k/hal.h:689: warning: Enum value 'HAL_RX_BUF_RBM_SW4_BM' not described in enum 'hal_rx_buf_return_buf_manager'

So update the documentation. No functional changes, compile tested only.

Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
---
 drivers/net/wireless/ath/ath11k/hal.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath11k/hal.h b/drivers/net/wireless/ath/ath11k/hal.h
index 80447f488954..65e8f244ebb9 100644
--- a/drivers/net/wireless/ath/ath11k/hal.h
+++ b/drivers/net/wireless/ath/ath11k/hal.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause-Clear */
 /*
  * Copyright (c) 2018-2019 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2022, 2024 Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #ifndef ATH11K_HAL_H
@@ -674,6 +674,7 @@ struct hal_srng_config {
  * @HAL_RX_BUF_RBM_SW1_BM: For Tx completion -- returned to host
  * @HAL_RX_BUF_RBM_SW2_BM: For Tx completion -- returned to host
  * @HAL_RX_BUF_RBM_SW3_BM: For Rx release -- returned to host
+ * @HAL_RX_BUF_RBM_SW4_BM: For Tx completion -- returned to host
  */
 
 enum hal_rx_buf_return_buf_manager {

---
base-commit: fba97a777dcb90896ab1dc32e796d85bb7bbcd69
change-id: 20240111-document-hal_rx_buf_rbm_sw4_bm-9afd96a0bc1f

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> Commit 7636c9a6e7d7 ("wifi: ath11k: Add multi TX ring support for WCN6750")
> added HAL_RX_BUF_RBM_SW4_BM to enum hal_rx_buf_return_buf_manager. However,
> as flagged by the kernel-doc script, the documentation was not updated:
>
> drivers/net/wireless/ath/ath11k/hal.h:689: warning: Enum value 'HAL_RX_BUF_RBM_SW4_BM' not described in enum 'hal_rx_buf_return_buf_manager'
>
> So update the documentation. No functional changes, compile tested only.
>
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>

I'm not really a fan of kernel-doc in wireless drivers, it feels more
unnecessary work. Should we remove the kernel-doc markings from ath11k
altogether?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Jeff Johnson]

On 1/14/2024 7:17 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> 
>> Commit 7636c9a6e7d7 ("wifi: ath11k: Add multi TX ring support for WCN6750")
>> added HAL_RX_BUF_RBM_SW4_BM to enum hal_rx_buf_return_buf_manager. However,
>> as flagged by the kernel-doc script, the documentation was not updated:
>>
>> drivers/net/wireless/ath/ath11k/hal.h:689: warning: Enum value 'HAL_RX_BUF_RBM_SW4_BM' not described in enum 'hal_rx_buf_return_buf_manager'
>>
>> So update the documentation. No functional changes, compile tested only.
>>
>> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> 
> I'm not really a fan of kernel-doc in wireless drivers, it feels more
> unnecessary work. Should we remove the kernel-doc markings from ath11k
> altogether?

Are you not a fan of kernel-doc format specifically, or not a fan of
documentation at all?

I'm personally a fan of documentation since good documentation makes the
code more maintainable. Yes, there is a cost in creating and maintaining
the documentation, but this is hopefully offset by cost saving when new
developers are trying to understand and modify the code.

I'm also a fan of consistency. And since kernel-doc is the standard
format defined for the kernel, it is my personal preference to use that
format.

I'm curious what others think of the ath10/11/12k level and style of
documentation.

/jeff

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 1/14/2024 7:17 AM, Kalle Valo wrote:
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>> 
>>> Commit 7636c9a6e7d7 ("wifi: ath11k: Add multi TX ring support for WCN6750")
>>> added HAL_RX_BUF_RBM_SW4_BM to enum hal_rx_buf_return_buf_manager. However,
>>> as flagged by the kernel-doc script, the documentation was not updated:
>>>
>>> drivers/net/wireless/ath/ath11k/hal.h:689: warning: Enum value
>>> 'HAL_RX_BUF_RBM_SW4_BM' not described in enum
>>> 'hal_rx_buf_return_buf_manager'
>>>
>>> So update the documentation. No functional changes, compile tested only.
>>>
>>> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>> 
>> I'm not really a fan of kernel-doc in wireless drivers, it feels more
>> unnecessary work. Should we remove the kernel-doc markings from ath11k
>> altogether?
>
> Are you not a fan of kernel-doc format specifically, or not a fan of
> documentation at all?

I'm definitely a fan of documentation where it makes sense, but I'm not
fan of kernel-doc if there are no users or readers. For example, using
kernel-doc in cfg80211 or mac80211 makes a lot of sense, and is
important, but I'm not convinced about using kernel-doc in wireless
drivers.

> I'm personally a fan of documentation since good documentation makes the
> code more maintainable. Yes, there is a cost in creating and maintaining
> the documentation, but this is hopefully offset by cost saving when new
> developers are trying to understand and modify the code.
>
> I'm also a fan of consistency. And since kernel-doc is the standard
> format defined for the kernel, it is my personal preference to use that
> format.

I understand your points and if we had plenty of free time I would be
onboard with this. To keep my mail short few quick points:

* To make sure there are no kernel-doc warnings we would have to add
  checks to ath11k-check, which would slow down it considerably and it
  would again slow down our workflow (I run it several times a day).

* To use kernel-doc formatting alone doesn't really make sense so we
  would have to start creating a kernel-doc book or something. But who
  would read it?

* kernel-doc moves field documentation in structures away from the
  actual fields which I find confusing.

* The risk of having outdated kernel-doc documentation is high, it would
  need active maintenance etc.

* I'm worried about creating useless documentation, like "Count number
  foo" for foo_count() just because of kernel-doc.

This is why I consider return on investment is low here :) My preference
is to make the code understandable (good symbol names etc) and document
the special cases, which are not obvious from the code, with a normal
code comment.

> I'm curious what others think of the ath10/11/12k level and style of
> documentation.

IIRC iwlwifi uses kernel-doc to document the firmware interface, not
sure how much it's used elsewhere in the driver.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Jeff Johnson]

On 1/18/2024 3:08 AM, Kalle Valo wrote:
> * To make sure there are no kernel-doc warnings we would have to add
>   checks to ath11k-check, which would slow down it considerably and it
>   would again slow down our workflow (I run it several times a day).

I currently run the following on every upstream patch series I review:

scripts/kernel-doc -Werror -none \
	$(find drivers/net/wireless/ath/ath1*k -name '*.[ch]')

It takes a trivial amount of time.

> * To use kernel-doc formatting alone doesn't really make sense so we
>   would have to start creating a kernel-doc book or something. But who
>   would read it?

Due to the sparseness, nobody would read the actual rendered
documentation; only the documentation as it exists in the code would be
read. However note that Linux cross-reference tool also links to the
documentation, which I often find useful when looking at core kernel
code, and would find it useful when looking at driver code.

> * kernel-doc moves field documentation in structures away from the
>   actual fields which I find confusing.

kernel-doc does support in-line commenting as well:
<https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#in-line-member-documentation-comments>

Although I don't see that used much.

> * The risk of having outdated kernel-doc documentation is high, it would
>   need active maintenance etc.

Agree, but that is true of any documentation. The nice thing about
kernel-doc is that it can actively tell you when the documentation
doesn't match the code.

> * I'm worried about creating useless documentation, like "Count number
>   foo" for foo_count() just because of kernel-doc.

Valid concern

> This is why I consider return on investment is low here :) My preference
> is to make the code understandable (good symbol names etc) and document
> the special cases, which are not obvious from the code, with a normal
> code comment.

Which I'm also fine with.

>> I'm curious what others think of the ath10/11/12k level and style of
>> documentation.
> 
> IIRC iwlwifi uses kernel-doc to document the firmware interface, not
> sure how much it's used elsewhere in the driver.

They have the same problem I'm trying to fix ;)
% scripts/kernel-doc -Werror -none \
	$(find drivers/net/wireless/intel/iwlwifi -name '*.[ch]')
...
322 warnings as Errors
%

I'm not looking to change the status quo. Let me get the last of the
ath10k cleanups in place, and I'll continue to run kernel-doc to make
sure the existing ath1*k documentation is kept up to date.

/jeff

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 1/18/2024 3:08 AM, Kalle Valo wrote:
>> * To make sure there are no kernel-doc warnings we would have to add
>>   checks to ath11k-check, which would slow down it considerably and it
>>   would again slow down our workflow (I run it several times a day).
>
> I currently run the following on every upstream patch series I review:
>
> scripts/kernel-doc -Werror -none \
> 	$(find drivers/net/wireless/ath/ath1*k -name '*.[ch]')
>
> It takes a trivial amount of time.

Hehe, indeed. It takes 0.2 seconds on my workstation, that's fast enough
for me :)

Thanks for the idea, I added this to ath12-check:

https://github.com/qca/qca-swiss-army-knife/commit/ef11ea4c7a866247f23f3d0825f9b08bd29c4989

So from now on I will always run kernel-doc for ath12k.

>> * To use kernel-doc formatting alone doesn't really make sense so we
>>   would have to start creating a kernel-doc book or something. But who
>>   would read it?
>
> Due to the sparseness, nobody would read the actual rendered
> documentation; only the documentation as it exists in the code would be
> read. However note that Linux cross-reference tool also links to the
> documentation, which I often find useful when looking at core kernel
> code, and would find it useful when looking at driver code.

It's just that in my experience it's SO hard to get people reading
documentation, even something really small. I'm not really optimistic
that using kernel-doc in drivers is any different and people ignore that
as well, like any other documentation.

>> * kernel-doc moves field documentation in structures away from the
>>   actual fields which I find confusing.
>
> kernel-doc does support in-line commenting as well:
> <https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#in-line-member-documentation-comments>
>
> Although I don't see that used much.

That's good to know. But in a way I understand why that style is not
used, at least the example in the link is hard to read.

>> * The risk of having outdated kernel-doc documentation is high, it would
>>   need active maintenance etc.
>
> Agree, but that is true of any documentation.

But the existing style is to provide documentation only when it's
necessary. With kernel-doc we have to provide documentation when it's
not even needed, no? For example, if only one enum needs documentation
and all others obvious,

>>> I'm curious what others think of the ath10/11/12k level and style of
>>> documentation.
>> 
>> IIRC iwlwifi uses kernel-doc to document the firmware interface, not
>> sure how much it's used elsewhere in the driver.
>
> They have the same problem I'm trying to fix ;)
> % scripts/kernel-doc -Werror -none \
> 	$(find drivers/net/wireless/intel/iwlwifi -name '*.[ch]')
> ...
> 322 warnings as Errors
> %

Ouch. But I'm surprised why nobody has reported these before? Are the
kernel-doc warnings ignored by everyone?

> I'm not looking to change the status quo.

I have no problems changing the status quo but having extra work without
a clear benefit is my concern here.

> Let me get the last of the ath10k cleanups in place, and I'll continue
> to run kernel-doc to make sure the existing ath1*k documentation is
> kept up to date.

Thanks. If you have time, maybe you could update ath11k-check and
ath10k-check? Every developer should use those scripts so having them
notice the warnings earlier is easier for everyone.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Jeff Johnson]

On 1/26/2024 8:58 AM, Kalle Valo wrote:
> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>> They have the same problem I'm trying to fix ;)
>> % scripts/kernel-doc -Werror -none \
>> 	$(find drivers/net/wireless/intel/iwlwifi -name '*.[ch]')
>> ...
>> 322 warnings as Errors
>> %
> 
> Ouch. But I'm surprised why nobody has reported these before? Are the
> kernel-doc warnings ignored by everyone?

There are folks who use the kernel 'make' command to build the
documentation, and report issues from that. But my understanding is that
'make' only processes the source files that are referenced by the
kernel's .rst files. So if a source file has documentation, but that
source file is not included by one of the .rst files, it won't normally
be processed.

It is only by running kernel-doc directly against the sources that you
can spot these issues.

/jeff

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 1/26/2024 8:58 AM, Kalle Valo wrote:
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
>>> They have the same problem I'm trying to fix ;)
>>> % scripts/kernel-doc -Werror -none \
>>> 	$(find drivers/net/wireless/intel/iwlwifi -name '*.[ch]')
>>> ...
>>> 322 warnings as Errors
>>> %
>> 
>> Ouch. But I'm surprised why nobody has reported these before? Are the
>> kernel-doc warnings ignored by everyone?
>
> There are folks who use the kernel 'make' command to build the
> documentation, and report issues from that. But my understanding is that
> 'make' only processes the source files that are referenced by the
> kernel's .rst files. So if a source file has documentation, but that
> source file is not included by one of the .rst files, it won't normally
> be processed.
>
> It is only by running kernel-doc directly against the sources that you
> can spot these issues.

Ah, thanks for the explanation.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> wrote:

> Commit 7636c9a6e7d7 ("wifi: ath11k: Add multi TX ring support for WCN6750")
> added HAL_RX_BUF_RBM_SW4_BM to enum hal_rx_buf_return_buf_manager. However,
> as flagged by the kernel-doc script, the documentation was not updated:
> 
> drivers/net/wireless/ath/ath11k/hal.h:689: warning: Enum value 'HAL_RX_BUF_RBM_SW4_BM' not described in enum 'hal_rx_buf_return_buf_manager'
> 
> So update the documentation. No functional changes, compile tested only.
> 
> Signed-off-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

9666ad011992 wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240111-document-hal_rx_buf_rbm_sw4_bm-v1-1-ad277e8ab3cc@quicinc.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches