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

[Venkateswara Naralasetty (QUIC)]

> -----Original Message-----
> From: Kalle Valo <kvalo at kernel.org>
> Sent: Tuesday, December 7, 2021 10:10 PM
> To: Venkateswara Naralasetty (QUIC) <quic_vnaralas at quicinc.com>
> Cc: ath11k at lists.infradead.org; linux-wireless at vger.kernel.org
> Subject: Re: [PATCHv2 1/2] ath11k: add dbring debug support
> 
> Venkateswara Naralasetty <quic_vnaralas at 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 at 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?
> 
Sure, I will update the commit log in the next version.

> 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.
> 
We faced few issues like head/tail pointer discrepancies between ath11k host  & FW during spectral scan feature development.
So, this debugfs entry would be helpful to debug such kind of issue in future.

> 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.

Sure, I will change it.

> 
> > --- 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.

Sure.

> 
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
> 
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingp
> atches