Hi Brian, > -----Original Message----- > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: 2017年12月1日 0:33 > To: Xinming Hu <huxm@xxxxxxxxxxx> > Cc: Xinming Hu <huxinming820@xxxxxxxxx>; Linux Wireless > <linux-wireless@xxxxxxxxxxxxxxx>; Kalle Valo <kvalo@xxxxxxxxxxxxxx>; Dmitry > Torokhov <dtor@xxxxxxxxxx>; rajatja@xxxxxxxxxx; Zhiyuan Yang > <yangzy@xxxxxxxxxxx>; Tim Song <songtao@xxxxxxxxxxx>; Cathy Luo > <cluo@xxxxxxxxxxx>; Ganapathi Bhat <gbhat@xxxxxxxxxxx>; James Cao > <jcao@xxxxxxxxxxx> > Subject: [EXT] Re: Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for > usb interface > > External Email > > ---------------------------------------------------------------------- > Hi Simon, > > On Wed, Nov 15, 2017 at 11:31:05AM +0000, Xinming Hu wrote: > > > -----Original Message----- > > > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > > > > > > On Mon, Aug 14, 2017 at 12:19:03PM +0000, Xinming Hu wrote: > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c > > > > b/drivers/net/wireless/marvell/mwifiex/debugfs.c > > > > index 6f4239b..5d476de 100644 > > > > --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c > > > > +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c > > > > @@ -168,10 +168,11 @@ > > > > { > > > > struct mwifiex_private *priv = file->private_data; > > > > > > > > - if (!priv->adapter->if_ops.device_dump) > > > > - return -EIO; > > > > - > > > > - priv->adapter->if_ops.device_dump(priv->adapter); > > > > + if (priv->adapter->iface_type == MWIFIEX_USB) > > > > + mwifiex_send_cmd(priv, HostCmd_CMD_FW_DUMP_EVENT, > > > > + HostCmd_ACT_GEN_SET, 0, NULL, true); > > > > > > Why couldn't you just implement the device_dump() callback? > > > > Currently mwifiex_send_cmd function is not exported to external modules, it > is designed for module mwifiex internal use. > > If you really don't want to export mwifiex_send_cmd(), you can export some > other wrapper which does the logic for you. The point is that there's a well > defined point for determining how to dump the firmware based on which > interface you're using. You should use it. > Thanks for the suggestion, it sounds better than exporting mwifiex_send_cmd() directly. In addition to used as debugfs tirgger, the "defined point" if_ops.device_dump is only used in command timeout context. For sdio/pcie interface, register operation will be made to trigger firmware dump and get dump context under specific algorithm. For usb interface, however, this is not needed, since firmware will automatically send dump event to host without any trigger, and what's more , host is also not able to issue command in the situation. So per my understand, here we only need provide a simple way to trigger , instead of a totally functional complete dump entry point. Suppose if we make the command trigger a part of if_ops->device_dump, then we also need add check for "MWIFIEX_USB" in mwifiex_cmd_tmo_func. it also looks inelegant, and what we did looks weird, they are (1) export a new kernel symbol, the wrapper of mwifiex_send_command (2) add usb if_ops->device_dump, it send the command in mwifiex_usb, instead of in mwifiex itself. (3) bypass above "if_ops->device_dump" in mwifiex_cmd_tmo_func, which is the mainly user case. I am not sure whether there is a better way on this, perhaps we need a trade-off on different solutions, please let us know if you have more suggestions. Thanks & Regards, SImon > > If we export mwifiex_send_cmd, and call it in mwifiex_usb. There will > > be a loop, > > So? There are all sorts of interdependencies between mwifiex.ko and > mwifiex_usb.ko (or in your words, "loops"). > > > .Device_dump (module mwifiex_usb) --> mwifiex_send_cmd(module > > mwifiex) --> .host_to_card (module mwifiex_usb) > > > > This seems not an elegant design, right? > > No less elegant than scattering: > > if (!USB) > driver->this(); > else > that(); > > all over the place. > > > Regards, > > Simon > > > > > > > + else > > > > + priv->adapter->if_ops.device_dump(priv->adapter); > > > > > > > > return 0; > > > > } > > Brian
