Re: [PATCHv2 1/2] ath11k: add dbring debug support

[Kalle Valo <kvalo@kernel.org>]

Venkateswara Naralasetty <quic_vnaralas@quicinc.com> writes:

> Target copies spectral report and CFR report through dbring to
> host for further processing. This mechanism involves ring and
> buffer management in the Host, FW, and uCode, where improper
> tail pointer update issues are seen.
>
> This dbring debug support help to debug such issues by tracking
> head and tail pointer movement along with the timestamp at which
> each buffer is received and replenished.
>
> Usage:
>
> echo <module_id> <val> > /sys/kernel/debug/ath11k/ipq8074_2/
> mac0/enable_dbr_debug
>
> module_id: 0 for spectral and 1 for CFR
> val: 0 - disable, 1 - enable.
>
> Tested-on: IPQ8074 WLAN.HK.2.4.0.1-01467-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Venkateswara Naralasetty <quic_vnaralas@quicinc.com>

The commit log is really minimal and is not really describing the whole
implementation. For example, I see that you create a new directory
debugfs and the commit log mentions nothing about that.

What about other hardware like QCA6390 or WCN6855? You mention nothing
about those, how are they affected?

And to be honest, I'm not sure if this debug feature is really worth to
have in upstream due to the extra complexity it creates.

Also usually it's a good idea to have the fix first in the patchset and
then extra patches after the fixes. That way it's easy to take the fixes
and skip the extra patches.

Few comments when I was skimming this over:

> --- a/drivers/net/wireless/ath/ath11k/core.h
> +++ b/drivers/net/wireless/ath/ath11k/core.h
> @@ -442,6 +442,7 @@ struct ath11k_debug {
>  	u32 pktlog_peer_valid;
>  	u8 pktlog_peer_addr[ETH_ALEN];
>  	u32 rx_filter;
> +	struct ath11k_db_module_debug *module_debug[WMI_DIRECT_BUF_MAX];

module_debug is not really a descriptive name for a field.

> --- a/drivers/net/wireless/ath/ath11k/debugfs.c
> +++ b/drivers/net/wireless/ath/ath11k/debugfs.c
> @@ -50,6 +50,45 @@ static const char *htt_bp_lmac_ring[HTT_SW_LMAC_RING_IDX_MAX] = {
>  	"MONITOR_DEST_RING",
>  };
>  
> +void ath11k_dbring_add_debug_entry(struct ath11k *ar,
> +				   enum wmi_direct_buffer_module id,
> +				   enum ath11k_db_ring_dbg_event event,
> +				   struct hal_srng *srng)
> +{

The style used in ath11k is that the second word describes the filename
the function is in. So this should be something like
ath11k_debugfs_add_dbring_entry().

> +	struct ath11k_db_module_debug *db_module_debug;

Similar naming for structs, please.

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

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