Hi Brian,
> -----Original Message-----
> From: Brian Norris [mailto:[email protected]]
> Sent: 2017年8月15日 8:01
> To: Xinming Hu <[email protected]>
> Cc: Linux Wireless <[email protected]>; Kalle Valo
> <[email protected]>; Dmitry Torokhov <[email protected]>;
> [email protected]; Zhiyuan Yang <[email protected]>; Tim Song
> <[email protected]>; Cathy Luo <[email protected]>; Ganapathi Bhat
> <[email protected]>; Xinming Hu <[email protected]>
> Subject: [EXT] Re: [PATCH 2/3] mwifiex: device dump support for usb interface
>
> External Email
>
> ----------------------------------------------------------------------
> Hi,
>
> On Mon, Aug 14, 2017 at 12:19:02PM +0000, Xinming Hu wrote:
> > From: Xinming Hu <[email protected]>
> >
> > Firmware dump on usb interface is different with current sdio/pcie
> > chipset, which is based on register operation.
> >
> > When firmware hang on usb interface, context dump will be upload to
> > host using 0x73 firmware debug event.
> >
> > This patch store dump data from debug event and send to userspace
> > using device coredump API.
> >
> > Signed-off-by: Xinming Hu <[email protected]>
> > Signed-off-by: Cathy Luo <[email protected]>
> > Signed-off-by: Ganapathi Bhat <[email protected]>
> > ---
> > drivers/net/wireless/marvell/mwifiex/fw.h | 1 +
> > drivers/net/wireless/marvell/mwifiex/init.c | 3 ++
> > drivers/net/wireless/marvell/mwifiex/main.h | 2 ++
> > drivers/net/wireless/marvell/mwifiex/sta_event.c | 39
> > ++++++++++++++++++++++++
> > 4 files changed, 45 insertions(+)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h
> > b/drivers/net/wireless/marvell/mwifiex/fw.h
> > index 9e75522..610a3ea 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> > @@ -568,6 +568,7 @@ enum mwifiex_channel_flags {
> > #define EVENT_BG_SCAN_STOPPED 0x00000065
> > #define EVENT_REMAIN_ON_CHAN_EXPIRED 0x0000005f
> > #define EVENT_MULTI_CHAN_INFO 0x0000006a
> > +#define EVENT_FW_DUMP_INFO 0x00000073
> > #define EVENT_TX_STATUS_REPORT 0x00000074
> > #define EVENT_BT_COEX_WLAN_PARA_CHANGE 0X00000076
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/init.c
> > b/drivers/net/wireless/marvell/mwifiex/init.c
> > index e11919d..389d725 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/init.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/init.c
> > @@ -315,6 +315,8 @@ static void mwifiex_init_adapter(struct
> mwifiex_adapter *adapter)
> > adapter->active_scan_triggered = false;
> > setup_timer(&adapter->wakeup_timer, wakeup_timer_fn,
> > (unsigned long)adapter);
> > + setup_timer(&adapter->devdump_timer, mwifiex_upload_device_dump,
> > + (unsigned long)adapter);
> > }
> >
> > /*
> > @@ -397,6 +399,7 @@ static void mwifiex_invalidate_lists(struct
> > mwifiex_adapter *adapter) mwifiex_adapter_cleanup(struct
> > mwifiex_adapter *adapter) {
> > del_timer(&adapter->wakeup_timer);
> > + del_timer_sync(&adapter->devdump_timer);
> > mwifiex_cancel_all_pending_cmd(adapter);
> > wake_up_interruptible(&adapter->cmd_wait_q.wait);
> > wake_up_interruptible(&adapter->hs_activate_wait_q);
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h
> > b/drivers/net/wireless/marvell/mwifiex/main.h
> > index e308b8a..6b00294 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> > @@ -1038,6 +1038,7 @@ struct mwifiex_adapter {
> > /* Device dump data/length */
> > char *devdump_data;
> > int devdump_len;
> > + struct timer_list devdump_timer;
> > };
> >
> > void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter); @@
> > -1680,6 +1681,7 @@ void mwifiex_process_multi_chan_event(struct
> > mwifiex_private *priv, void mwifiex_multi_chan_resync(struct
> > mwifiex_adapter *adapter); int mwifiex_set_mac_address(struct
> mwifiex_private *priv,
> > struct net_device *dev);
> > +void mwifiex_devdump_tmo_func(unsigned long function_context);
> >
> > #ifdef CONFIG_DEBUG_FS
> > void mwifiex_debugfs_init(void);
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> > b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> > index 839df8a..bcf2fa2 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> > @@ -586,6 +586,40 @@ void
> mwifiex_bt_coex_wlan_param_update_event(struct mwifiex_private *priv,
> > adapter->coex_rx_win_size);
> > }
> >
> > +static void
> > +mwifiex_fw_dump_info_event(struct mwifiex_private *priv,
> > + struct sk_buff *event_skb)
> > +{
> > + struct mwifiex_adapter *adapter = priv->adapter;
> > +
> > + if (adapter->iface_type != MWIFIEX_USB) {
> > + mwifiex_dbg(adapter, MSG,
> > + "event is not on usb interface, ignore it\n");
> > + return;
> > + }
> > +
> > + if (!adapter->devdump_data) {
> > + /* When receive the first event, allocate device dump
> > + * buffer, dump driver info.
> > + */
> > + adapter->devdump_data = vzalloc(MWIFIEX_FW_DUMP_SIZE);
>
> Does this ever get freed?
Sure, it will be freed by devcoredump framework, 5mins after driver call
dev_coredump
>
> > + if (!adapter->devdump_data) {
> > + mwifiex_dbg(adapter, ERROR,
> > + "vzalloc devdump data failure!\n");
> > + return;
> > + }
> > +
> > + mwifiex_drv_info_dump(adapter);
> > + }
> > +
> > + memmove(adapter->devdump_data + adapter->devdump_len,
> > + adapter->event_body, event_skb->len -
> MWIFIEX_EVENT_HEADER_LEN);
> > + adapter->devdump_len += (event_skb->len -
> MWIFIEX_EVENT_HEADER_LEN);
>
> Are you going to try to check for overflow?
Thanks for the point, will add the check in V2.
>
> > + /* If no proceeded event arrive in 10s, upload device dump data*/
>
> You missed a space at the end of the comment. Should be:
>
> /* If no proceeded event arrive in 10s, upload device dump data. */
>
> Also, is that really the only way to signal that the firmware dump has
> completed? By timing out? That seems like a very bad design. Can you
> implement an "end of transmission" signal?
Good suggestion, Latest firmware will set the signal in the last FW_DUMP_INFO
event, will apply it in V2.
In case the "end of transmission" event lost in some cornel case, we would like
to add a 10s timer to trigger the dump upload to userspace.
Regards,
Simon
>
> Brian
>
> > + mod_timer(&adapter->devdump_timer,
> > + jiffies + msecs_to_jiffies(MWIFIEX_TIMER_10S));
> > +}
> > +
> > /*
> > * This function handles events generated by firmware.
> > *
> > @@ -638,6 +672,7 @@ void
> mwifiex_bt_coex_wlan_param_update_event(struct mwifiex_private *priv,
> > * - EVENT_DELBA
> > * - EVENT_BA_STREAM_TIEMOUT
> > * - EVENT_AMSDU_AGGR_CTRL
> > + * - EVENT_FW_DUMP_INFO
> > */
> > int mwifiex_process_sta_event(struct mwifiex_private *priv) { @@
> > -1009,6 +1044,10 @@ int mwifiex_process_sta_event(struct
> mwifiex_private *priv)
> > adapter->event_skb->len -
> > sizeof(eventcause));
> > break;
> > + case EVENT_FW_DUMP_INFO:
> > + mwifiex_dbg(adapter, EVENT, "event: firmware debug info\n");
> > + mwifiex_fw_dump_info_event(priv, adapter->event_skb);
> > + break;
> > /* Debugging event; not used, but let's not print an ERROR for it. */
> > case EVENT_UNKNOWN_DEBUG:
> > mwifiex_dbg(adapter, EVENT, "event: debug\n");
> > --
> > 1.9.1
> >
