RE: [PATCH v2 1/3] ath10k: Add history for tracking certain events

[Rakesh Pillai]

> -----Original Message-----
> From: Ben Greear <greearb@xxxxxxxxxxxxxxx>
> Sent: Saturday, August 1, 2020 12:08 AM
> To: Rakesh Pillai <pillair@xxxxxxxxxxxxxx>; ath10k@xxxxxxxxxxxxxxxxxxx
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kvalo@xxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 1/3] ath10k: Add history for tracking certain events
> 
> On 7/31/20 11:27 AM, Rakesh Pillai wrote:
> > Add history for tracking the below events
> > - register read
> > - register write
> > - IRQ trigger
> > - NAPI poll
> > - CE service
> > - WMI cmd
> > - WMI event
> > - WMI tx completion
> >
> > This will help in debugging any crash or any
> > improper behaviour.
> >
> > Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
> >
> > Signed-off-by: Rakesh Pillai <pillair@xxxxxxxxxxxxxx>
> > ---
> >   drivers/net/wireless/ath/ath10k/ce.c      |   1 +
> >   drivers/net/wireless/ath/ath10k/core.h    |  74 +++++++++++++++++
> >   drivers/net/wireless/ath/ath10k/debug.c   | 133
> ++++++++++++++++++++++++++++++
> >   drivers/net/wireless/ath/ath10k/debug.h   |  74 +++++++++++++++++
> >   drivers/net/wireless/ath/ath10k/snoc.c    |  15 +++-
> >   drivers/net/wireless/ath/ath10k/wmi-tlv.c |   1 +
> >   drivers/net/wireless/ath/ath10k/wmi.c     |  10 +++
> >   7 files changed, 307 insertions(+), 1 deletion(-)
> >
> 
> > +void ath10k_record_wmi_event(struct ath10k *ar, enum
> ath10k_wmi_type type,
> > +			     u32 id, unsigned char *data)
> > +{
> > +	struct ath10k_wmi_event_entry *entry;
> > +	u32 idx;
> > +
> > +	if (type == ATH10K_WMI_EVENT) {
> > +		if (!ar->wmi_event_history.record)
> > +			return;
> 
> This check above is duplicated below, add it once at top of the method
> instead.

The same function is used to record WMI events and CMD, which are stored in different memory locations.
Hence the check  " if (type == ATH10K_WMI_EVENT) {" is necessary.


> 
> > +
> > +		spin_lock_bh(&ar->wmi_event_history.hist_lock);
> > +		idx = ath10k_core_get_next_idx(&ar-
> >reg_access_history.index,
> > +					       ar-
> >wmi_event_history.max_entries);
> > +		spin_unlock_bh(&ar->wmi_event_history.hist_lock);
> > +		entry = &ar->wmi_event_history.record[idx];
> > +	} else {
> > +		if (!ar->wmi_cmd_history.record)
> > +			return;
> > +
> > +		spin_lock_bh(&ar->wmi_cmd_history.hist_lock);
> > +		idx = ath10k_core_get_next_idx(&ar-
> >reg_access_history.index,
> > +					       ar-
> >wmi_cmd_history.max_entries);
> > +		spin_unlock_bh(&ar->wmi_cmd_history.hist_lock);
> > +		entry = &ar->wmi_cmd_history.record[idx];
> > +	}
> > +
> > +	entry->timestamp = ath10k_core_get_timestamp();
> > +	entry->cpu_id = smp_processor_id();
> > +	entry->type = type;
> > +	entry->id = id;
> > +	memcpy(&entry->data, data + 4, ATH10K_WMI_DATA_LEN);
> > +}
> > +EXPORT_SYMBOL(ath10k_record_wmi_event);
> 
> > @@ -1660,6 +1668,11 @@ static int ath10k_snoc_probe(struct
> platform_device *pdev)
> >   	ar->ce_priv = &ar_snoc->ce;
> >   	msa_size = drv_data->msa_size;
> >
> > +	ath10k_core_reg_access_history_init(ar,
> ATH10K_REG_ACCESS_HISTORY_MAX);
> > +	ath10k_core_wmi_event_history_init(ar,
> ATH10K_WMI_EVENT_HISTORY_MAX);
> > +	ath10k_core_wmi_cmd_history_init(ar,
> ATH10K_WMI_CMD_HISTORY_MAX);
> > +	ath10k_core_ce_event_history_init(ar,
> ATH10K_CE_EVENT_HISTORY_MAX);
> 
> Maybe only enable this once user turns it on?  It sucks up a bit of memory?


This memory will be allocated only if the history is enabled via module param, else the function just returns 0.


> 
> > +
> >   	ath10k_snoc_quirks_init(ar);
> >
> >   	ret = ath10k_snoc_resource_init(ar);
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> > index 932266d..9df5748 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> > @@ -627,6 +627,7 @@ static void ath10k_wmi_tlv_op_rx(struct ath10k *ar,
> struct sk_buff *skb)
> >   	if (skb_pull(skb, sizeof(struct wmi_cmd_hdr)) == NULL)
> >   		goto out;
> >
> > +	ath10k_record_wmi_event(ar, ATH10K_WMI_EVENT, id, skb->data);
> >   	trace_ath10k_wmi_event(ar, id, skb->data, skb->len);
> >
> >   	consumed = ath10k_tm_event_wmi(ar, id, skb);
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c
> b/drivers/net/wireless/ath/ath10k/wmi.c
> > index a81a1ab..8ebd05c 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > @@ -1802,6 +1802,15 @@ struct sk_buff *ath10k_wmi_alloc_skb(struct
> ath10k *ar, u32 len)
> >
> >   static void ath10k_wmi_htc_tx_complete(struct ath10k *ar, struct sk_buff
> *skb)
> >   {
> > +	struct wmi_cmd_hdr *cmd_hdr;
> > +	enum wmi_tlv_event_id id;
> > +
> > +	cmd_hdr = (struct wmi_cmd_hdr *)skb->data;
> > +	id = MS(__le32_to_cpu(cmd_hdr->cmd_id),
> WMI_CMD_HDR_CMD_ID);
> > +
> > +	ath10k_record_wmi_event(ar, ATH10K_WMI_TX_COMPL, id,
> > +				skb->data + sizeof(struct wmi_cmd_hdr));
> > +
> >   	dev_kfree_skb(skb);
> >   }
> 
> I think guard the above new code with if (unlikely(ar-
> >ce_event_history.record)) { ... }
> 
> All in all, I think I'd want to compile this out (while leaving other debug
> compiled
> in) since it seems this stuff would be rarely used and it adds method calls to
> hot
> paths.
> 
> That is a decision for Kalle though, so see what he says...


Sure let me add this check.


> 
> Thanks,
> Ben
> 
> 
> --
> Ben Greear <greearb@xxxxxxxxxxxxxxx>
> Candela Technologies Inc  http://www.candelatech.com