Re: Re: Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for usb interface

Re: Re: Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for usb interface

[Xinming Hu]

SGkgQnJpYW4sDQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQnJpYW4g
Tm9ycmlzIFttYWlsdG86YnJpYW5ub3JyaXNAY2hyb21pdW0ub3JnXQ0KPiBTZW50OiAyMDE3xOox
MtTCMcjVIDA6MzMNCj4gVG86IFhpbm1pbmcgSHUgPGh1eG1AbWFydmVsbC5jb20+DQo+IENjOiBY
aW5taW5nIEh1IDxodXhpbm1pbmc4MjBAZ21haWwuY29tPjsgTGludXggV2lyZWxlc3MNCj4gPGxp
bnV4LXdpcmVsZXNzQHZnZXIua2VybmVsLm9yZz47IEthbGxlIFZhbG8gPGt2YWxvQGNvZGVhdXJv
cmEub3JnPjsgRG1pdHJ5DQo+IFRvcm9raG92IDxkdG9yQGdvb2dsZS5jb20+OyByYWphdGphQGdv
b2dsZS5jb207IFpoaXl1YW4gWWFuZw0KPiA8eWFuZ3p5QG1hcnZlbGwuY29tPjsgVGltIFNvbmcg
PHNvbmd0YW9AbWFydmVsbC5jb20+OyBDYXRoeSBMdW8NCj4gPGNsdW9AbWFydmVsbC5jb20+OyBH
YW5hcGF0aGkgQmhhdCA8Z2JoYXRAbWFydmVsbC5jb20+OyBKYW1lcyBDYW8NCj4gPGpjYW9AbWFy
dmVsbC5jb20+DQo+IFN1YmplY3Q6IFtFWFRdIFJlOiBSZTogW1BBVENIIDMvM10gbXdpZmlleDog
ZGVidWdmczogdHJpZ2dlciBkZXZpY2UgZHVtcCBmb3INCj4gdXNiIGludGVyZmFjZQ0KPiANCj4g
RXh0ZXJuYWwgRW1haWwNCj4gDQo+IC0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCj4gSGkgU2ltb24sDQo+IA0KPiBP
biBXZWQsIE5vdiAxNSwgMjAxNyBhdCAxMTozMTowNUFNICswMDAwLCBYaW5taW5nIEh1IHdyb3Rl
Og0KPiA+ID4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gPiA+IEZyb206IEJyaWFuIE5v
cnJpcyBbbWFpbHRvOmJyaWFubm9ycmlzQGNocm9taXVtLm9yZ10NCj4gPiA+DQo+ID4gPiBPbiBN
b24sIEF1ZyAxNCwgMjAxNyBhdCAxMjoxOTowM1BNICswMDAwLCBYaW5taW5nIEh1IHdyb3RlOg0K
PiA+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9uZXQvd2lyZWxlc3MvbWFydmVsbC9td2lmaWV4
L2RlYnVnZnMuYw0KPiA+ID4gPiBiL2RyaXZlcnMvbmV0L3dpcmVsZXNzL21hcnZlbGwvbXdpZmll
eC9kZWJ1Z2ZzLmMNCj4gPiA+ID4gaW5kZXggNmY0MjM5Yi4uNWQ0NzZkZSAxMDA2NDQNCj4gPiA+
ID4gLS0tIGEvZHJpdmVycy9uZXQvd2lyZWxlc3MvbWFydmVsbC9td2lmaWV4L2RlYnVnZnMuYw0K
PiA+ID4gPiArKysgYi9kcml2ZXJzL25ldC93aXJlbGVzcy9tYXJ2ZWxsL213aWZpZXgvZGVidWdm
cy5jDQo+ID4gPiA+IEBAIC0xNjgsMTAgKzE2OCwxMSBAQA0KPiA+ID4gPiAgew0KPiA+ID4gPiAg
CXN0cnVjdCBtd2lmaWV4X3ByaXZhdGUgKnByaXYgPSBmaWxlLT5wcml2YXRlX2RhdGE7DQo+ID4g
PiA+DQo+ID4gPiA+IC0JaWYgKCFwcml2LT5hZGFwdGVyLT5pZl9vcHMuZGV2aWNlX2R1bXApDQo+
ID4gPiA+IC0JCXJldHVybiAtRUlPOw0KPiA+ID4gPiAtDQo+ID4gPiA+IC0JcHJpdi0+YWRhcHRl
ci0+aWZfb3BzLmRldmljZV9kdW1wKHByaXYtPmFkYXB0ZXIpOw0KPiA+ID4gPiArCWlmIChwcml2
LT5hZGFwdGVyLT5pZmFjZV90eXBlID09IE1XSUZJRVhfVVNCKQ0KPiA+ID4gPiArCQltd2lmaWV4
X3NlbmRfY21kKHByaXYsIEhvc3RDbWRfQ01EX0ZXX0RVTVBfRVZFTlQsDQo+ID4gPiA+ICsJCQkJ
IEhvc3RDbWRfQUNUX0dFTl9TRVQsIDAsIE5VTEwsIHRydWUpOw0KPiA+ID4NCj4gPiA+IFdoeSBj
b3VsZG4ndCB5b3UganVzdCBpbXBsZW1lbnQgdGhlIGRldmljZV9kdW1wKCkgY2FsbGJhY2s/DQo+
ID4NCj4gPiBDdXJyZW50bHkgbXdpZmlleF9zZW5kX2NtZCBmdW5jdGlvbiBpcyBub3QgZXhwb3J0
ZWQgdG8gZXh0ZXJuYWwgbW9kdWxlcywgaXQNCj4gaXMgZGVzaWduZWQgZm9yIG1vZHVsZSBtd2lm
aWV4IGludGVybmFsIHVzZS4NCj4gDQo+IElmIHlvdSByZWFsbHkgZG9uJ3Qgd2FudCB0byBleHBv
cnQgbXdpZmlleF9zZW5kX2NtZCgpLCB5b3UgY2FuIGV4cG9ydCBzb21lDQo+IG90aGVyIHdyYXBw
ZXIgd2hpY2ggZG9lcyB0aGUgbG9naWMgZm9yIHlvdS4gVGhlIHBvaW50IGlzIHRoYXQgdGhlcmUn
cyBhIHdlbGwNCj4gZGVmaW5lZCBwb2ludCBmb3IgZGV0ZXJtaW5pbmcgaG93IHRvIGR1bXAgdGhl
IGZpcm13YXJlIGJhc2VkIG9uIHdoaWNoDQo+IGludGVyZmFjZSB5b3UncmUgdXNpbmcuIFlvdSBz
aG91bGQgdXNlIGl0Lg0KPiANCg0KDQpUaGFua3MgZm9yIHRoZSBzdWdnZXN0aW9uLCBpdCBzb3Vu
ZHMgYmV0dGVyIHRoYW4gZXhwb3J0aW5nIG13aWZpZXhfc2VuZF9jbWQoKSBkaXJlY3RseS4NCklu
IGFkZGl0aW9uIHRvIHVzZWQgYXMgZGVidWdmcyB0aXJnZ2VyLCB0aGUgImRlZmluZWQgcG9pbnQi
IGlmX29wcy5kZXZpY2VfZHVtcCBpcyBvbmx5IHVzZWQgaW4gY29tbWFuZCB0aW1lb3V0IGNvbnRl
eHQuDQpGb3Igc2Rpby9wY2llIGludGVyZmFjZSwgcmVnaXN0ZXIgb3BlcmF0aW9uIHdpbGwgYmUg
bWFkZSB0byB0cmlnZ2VyIGZpcm13YXJlIGR1bXAgYW5kIGdldCBkdW1wIGNvbnRleHQgdW5kZXIg
c3BlY2lmaWMgYWxnb3JpdGhtLg0KRm9yIHVzYiBpbnRlcmZhY2UsIGhvd2V2ZXIsICB0aGlzIGlz
IG5vdCBuZWVkZWQsIHNpbmNlIGZpcm13YXJlIHdpbGwgYXV0b21hdGljYWxseSBzZW5kIGR1bXAg
ZXZlbnQgdG8gaG9zdCB3aXRob3V0IGFueSB0cmlnZ2VyLCBhbmQgd2hhdCdzIG1vcmUgLCBob3N0
IGlzIGFsc28gbm90IGFibGUgdG8gaXNzdWUgY29tbWFuZCBpbiB0aGUgc2l0dWF0aW9uLg0KDQpT
byBwZXIgbXkgdW5kZXJzdGFuZCwgIGhlcmUgd2Ugb25seSBuZWVkIHByb3ZpZGUgYSBzaW1wbGUg
d2F5IHRvIHRyaWdnZXIgLCBpbnN0ZWFkIG9mIGEgdG90YWxseSBmdW5jdGlvbmFsIGNvbXBsZXRl
IGR1bXAgZW50cnkgcG9pbnQuIA0KU3VwcG9zZSBpZiB3ZSBtYWtlIHRoZSBjb21tYW5kIHRyaWdn
ZXIgYSBwYXJ0IG9mIGlmX29wcy0+ZGV2aWNlX2R1bXAsICB0aGVuIHdlIGFsc28gbmVlZCBhZGQg
Y2hlY2sgZm9yICJNV0lGSUVYX1VTQiIgaW4gbXdpZmlleF9jbWRfdG1vX2Z1bmMuIA0KDQppdCBh
bHNvIGxvb2tzIGluZWxlZ2FudCwgYW5kIHdoYXQgd2UgZGlkIGxvb2tzIHdlaXJkLCB0aGV5IGFy
ZQ0KKDEpIGV4cG9ydCBhIG5ldyAga2VybmVsIHN5bWJvbCwgIHRoZSB3cmFwcGVyIG9mIG13aWZp
ZXhfc2VuZF9jb21tYW5kDQooMikgYWRkIHVzYiBpZl9vcHMtPmRldmljZV9kdW1wLCBpdCAgc2Vu
ZCB0aGUgY29tbWFuZCBpbiBtd2lmaWV4X3VzYiwgaW5zdGVhZCBvZiBpbiBtd2lmaWV4IGl0c2Vs
Zi4NCigzKSBieXBhc3MgYWJvdmUgImlmX29wcy0+ZGV2aWNlX2R1bXAiIGluIG13aWZpZXhfY21k
X3Rtb19mdW5jLCB3aGljaCBpcyB0aGUgbWFpbmx5IHVzZXIgY2FzZS4NCg0KSSBhbSBub3Qgc3Vy
ZSB3aGV0aGVyIHRoZXJlIGlzIGEgYmV0dGVyIHdheSBvbiB0aGlzLCBwZXJoYXBzIHdlIG5lZWQg
YSB0cmFkZS1vZmYgb24gZGlmZmVyZW50IHNvbHV0aW9ucywgcGxlYXNlIGxldCB1cyBrbm93IGlm
IHlvdSBoYXZlIG1vcmUgc3VnZ2VzdGlvbnMuDQoNClRoYW5rcyAmIFJlZ2FyZHMsDQpTSW1vbg0K
DQo+ID4gSWYgd2UgZXhwb3J0IG13aWZpZXhfc2VuZF9jbWQsIGFuZCBjYWxsIGl0IGluIG13aWZp
ZXhfdXNiLiBUaGVyZSB3aWxsDQo+ID4gYmUgYSBsb29wLA0KPiANCj4gU28/IFRoZXJlIGFyZSBh
bGwgc29ydHMgb2YgaW50ZXJkZXBlbmRlbmNpZXMgYmV0d2VlbiBtd2lmaWV4LmtvIGFuZA0KPiBt
d2lmaWV4X3VzYi5rbyAob3IgaW4geW91ciB3b3JkcywgImxvb3BzIikuDQo+IA0KPiA+IC5EZXZp
Y2VfZHVtcCAobW9kdWxlIG13aWZpZXhfdXNiKSAgLS0+ICBtd2lmaWV4X3NlbmRfY21kKG1vZHVs
ZQ0KPiA+IG13aWZpZXgpICAtLT4gLmhvc3RfdG9fY2FyZCAobW9kdWxlIG13aWZpZXhfdXNiKQ0K
PiA+DQo+ID4gVGhpcyBzZWVtcyBub3QgYW4gZWxlZ2FudCBkZXNpZ24sIHJpZ2h0Pw0KPiANCj4g
Tm8gbGVzcyBlbGVnYW50IHRoYW4gc2NhdHRlcmluZzoNCj4gDQo+IAlpZiAoIVVTQikNCj4gCQlk
cml2ZXItPnRoaXMoKTsNCj4gCWVsc2UNCj4gCQl0aGF0KCk7DQo+IA0KPiBhbGwgb3ZlciB0aGUg
cGxhY2UuDQo+IA0KPiA+IFJlZ2FyZHMsDQo+ID4gU2ltb24NCj4gPiA+DQo+ID4gPiA+ICsJZWxz
ZQ0KPiA+ID4gPiArCQlwcml2LT5hZGFwdGVyLT5pZl9vcHMuZGV2aWNlX2R1bXAocHJpdi0+YWRh
cHRlcik7DQo+ID4gPiA+DQo+ID4gPiA+ICAJcmV0dXJuIDA7DQo+ID4gPiA+ICB9DQo+IA0KPiBC
cmlhbg0K

Re: Re: Re: [PATCH 3/3] mwifiex: debugfs: trigger device dump for usb interface

[Brian Norris]

On Fri, Dec 01, 2017 at 08:36:01AM +0000, Xinming Hu wrote:
> 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. 

Ah, I see. Your explanation makes some more sense then. It would be nice
if you could include some of this in
(a) the commit message
(b) the entry point in debugfs.c, where you trigger this

Something along the lines of "For command timeouts, USB firmware will
automatically emit firmware dump events, so we don't implement
device_dump(). For user-initiated dumps, we trigger it ourselves."

> 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.

No, I'm not sure that solution would be much better. Your existing
solution with additional comments is probably fine.

> 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

Brian