| Message ID | 20240111-document-hal_rx_buf_rbm_sw4_bm-v1-1-ad277e8ab3cc@quicinc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9666ad011992b51da2a4623d20084a363a3a095e |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: ath11k: document HAL_RX_BUF_RBM_SW4_BM | expand |
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?
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
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
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.
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
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.
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
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.
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 {
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(-) --- base-commit: fba97a777dcb90896ab1dc32e796d85bb7bbcd69 change-id: 20240111-document-hal_rx_buf_rbm_sw4_bm-9afd96a0bc1f