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 <[email protected]>
> ---
> 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.
> +
> + 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?
> +
> 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...
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
> -----Original Message-----
> From: Ben Greear <[email protected]>
> Sent: Saturday, August 1, 2020 12:08 AM
> To: Rakesh Pillai <[email protected]>; [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> Candela Technologies Inc http://www.candelatech.com