> -----Original Message-----
> From: Kalle Valo <kvalo@xxxxxxxxxx>
> Sent: Tuesday, December 7, 2021 10:10 PM
> To: Venkateswara Naralasetty (QUIC) <quic_vnaralas@xxxxxxxxxxx>
> Cc: ath11k@xxxxxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx
> Subject: Re: [PATCHv2 1/2] ath11k: add dbring debug support
>
> Venkateswara Naralasetty <quic_vnaralas@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>
> 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
