From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v3,2/6] usb:common Separated decoding functions from dwc3 driver. From: Greg Kroah-Hartman Message-Id: <20190204144725.GA31360@kroah.com> Date: Mon, 4 Feb 2019 15:47:25 +0100 To: Felipe Balbi Cc: Pawel Laszczak , devicetree@vger.kernel.org, mark.rutland@arm.com, linux-usb@vger.kernel.org, hdegoede@redhat.com, heikki.krogerus@linux.intel.com, andy.shevchenko@gmail.com, robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org, jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com, pjez@cadence.com, kurahul@cadence.com List-ID: T24gTW9uLCBGZWIgMDQsIDIwMTkgYXQgMDQ6MTc6MDBQTSArMDIwMCwgRmVsaXBlIEJhbGJpIHdy b3RlOgo+IAo+IEhpLAo+IAo+IEdyZWcgS0ggPGdyZWdraEBsaW51eGZvdW5kYXRpb24ub3JnPiB3 cml0ZXM6Cj4gPiBPbiBUaHUsIEphbiAzMSwgMjAxOSBhdCAxMTo1MjoyOUFNICswMDAwLCBQYXdl bCBMYXN6Y3phayB3cm90ZToKPiA+PiBQYXRjaCBtb3ZlcyBzb21lIGRlY29kaW5nIGZ1bmN0aW9u cyBmcm9tIGRyaXZlci91c2IvZHdjMy9kZWJ1Zy5oIGRyaXZlcgo+ID4+IHRvIGRyaXZlci91c2Iv Y29tbW9uL2RlYnVnLmMgZmlsZS4gVGhlc2UgbW92ZWQgZnVuY3Rpb25zIGluY2x1ZGU6Cj4gPj4g ICAgIGR3YzNfZGVjb2RlX2dldF9zdGF0dXMKPiA+PiAgICAgZHdjM19kZWNvZGVfc2V0X2NsZWFy X2ZlYXR1cmUKPiA+PiAgICAgZHdjM19kZWNvZGVfc2V0X2FkZHJlc3MKPiA+PiAgICAgZHdjM19k ZWNvZGVfZ2V0X3NldF9kZXNjcmlwdG9yCj4gPj4gICAgIGR3YzNfZGVjb2RlX2dldF9jb25maWd1 cmF0aW9uCj4gPj4gICAgIGR3YzNfZGVjb2RlX3NldF9jb25maWd1cmF0aW9uCj4gPj4gICAgIGR3 YzNfZGVjb2RlX2dldF9pbnRmCj4gPj4gICAgIGR3YzNfZGVjb2RlX3NldF9pbnRmCj4gPj4gICAg IGR3YzNfZGVjb2RlX3N5bmNoX2ZyYW1lCj4gPj4gICAgIGR3YzNfZGVjb2RlX3NldF9zZWwKPiA+ PiAgICAgZHdjM19kZWNvZGVfc2V0X2lzb2NoX2RlbGF5Cj4gPj4gICAgIGR3YzNfZGVjb2RlX2N0 cmwKPiA+PiAKPiA+PiBUaGVzZSBmdW5jdGlvbnMgYXJlIHVzZWQgYWxzbyBpbiBpbnJvZHVjZWQg Y2RuczMgZHJpdmVyLgo+ID4+IAo+ID4+IEFsbCBmdW5jdGlvbnMgcHJlZml4ZXMgd2VyZSBjaGFu Z2VkIGZyb20gZHdjMyB0byB1c2IuCj4gPgo+ID4gSWNrLCB3aHk/Cj4gCj4gbW92aW5nIGR3YzMt c3BlY2lmaWMgY29kZSBpbnRvIGdlbmVyaWMgY29kZS4KClRoYXQgc2F5cyB3aGF0IGl0IGRvZXMs IGJ1dCBub3Qgd2h5IDopCgpBbmQgaWYgdGhpcyByZWFsbHkgd2FzIGp1c3QgbW92aW5nIHRoaW5n cyBhcm91bmQsIHdoeSB3YXMgb25seSBvbmUKc3ltYm9sIG5lZWRlZCB0byBiZSBleHBvcnRlZCBh bmQgbm90IGFsbCBvZiB0aGVtPwoKPiA+PiArICogQGJSZXF1ZXN0VHlwZTogbWF0Y2hlcyB0aGUg VVNCIGJtUmVxdWVzdFR5cGUgZmllbGQKPiA+PiArICogQGJSZXF1ZXN0OiBtYXRjaGVzIHRoZSBV U0IgYlJlcXVlc3QgZmllbGQKPiA+PiArICogQHdWYWx1ZTogbWF0Y2hlcyB0aGUgVVNCIHdWYWx1 ZSBmaWVsZCAoQ1BVIGJ5dGUgb3JkZXIpCj4gPj4gKyAqIEB3SW5kZXg6IG1hdGNoZXMgdGhlIFVT QiB3SW5kZXggZmllbGQgKENQVSBieXRlIG9yZGVyKQo+ID4+ICsgKiBAd0xlbmd0aDogbWF0Y2hl cyB0aGUgVVNCIHdMZW5ndGggZmllbGQgKENQVSBieXRlIG9yZGVyKQo+ID4+ICsgKgo+ID4+ICsg KiBGdW5jdGlvbiByZXR1cm5zIGRlY29kZWQsIGZvcm1hdHRlZCBhbmQgaHVtYW4tcmVhZGFibGUg ZGVzY3JpcHRpb24gb2YKPiA+PiArICogY29udHJvbCByZXF1ZXN0IHBhY2tldC4KPiA+PiArICoK PiA+PiArICogSW1wb3J0YW50OiB3VmFsdWUsIHdJbmRleCwgd0xlbmd0aCBwYXJhbWV0ZXJzIGJl Zm9yZSBpbnZva2luZyB0aGlzIGZ1bmN0aW9uCj4gPj4gKyAqIHNob3VsZCBiZSBwcm9jZXNzZWQg YnkgbGUxNl90b19jcHUgbWFjcm8uCj4gPj4gKyAqLwo+ID4+ICtjb25zdCBjaGFyICp1c2JfZGVj b2RlX2N0cmwoY2hhciAqc3RyLCBfX3U4IGJSZXF1ZXN0VHlwZSwgX191OCBiUmVxdWVzdCwKPiA+ PiArCQkJICAgIF9fdTE2IHdWYWx1ZSwgIF9fdTE2IHdJbmRleCwgX191MTYgd0xlbmd0aCk7Cj4g Pgo+ID4gV2h5IGFyZSB5b3UgcmV0dXJuaW5nIGEgdmFsdWUsIGlzbid0IHRoZSBkYXRhIHN0b3Jl ZCBpbiBzdHI/ICBXaHkgbm90Cj4gPiBqdXN0IHJldHVybiBhbiBpbnQgc2F5aW5nIGlmIHRoZSBj YWxsIHN1Y2NlZWRlZCBvciBub3Q/Cj4gCj4gVGhlcmUgaXMgb25lIGRldGFpbCBoZXJlLiBUaGUg dXNhZ2Ugc2NlbmFyaW8gZm9yIHRoaXMgaXMgZm9yCj4gdHJhY2Vwb2ludHMuIFdoZW4gZGVhbGlu ZyB3aXRoIHRyYWNlcG9pbnRzIHdlIHdhbnQgdG8gZGVsYXkgZGVjb2Rpbmcgb2YKPiB0aGUgZGF0 YSBpbnRvIHN0cmluZ3MgdW50aWwgcHJpbnQtdGltZS4gSSBndWVzcyBpdCdzIGJlc3QgdG8gaWxs dXN0cmF0ZQo+IHdpdGggYW4gZXhhbXBsZToKPiAKPiBERUNMQVJFX0VWRU5UX0NMQVNTKGR3YzNf bG9nX2N0cmwsCj4gCVRQX1BST1RPKHN0cnVjdCB1c2JfY3RybHJlcXVlc3QgKmN0cmwpLAo+IAlU UF9BUkdTKGN0cmwpLAo+IAlUUF9TVFJVQ1RfX2VudHJ5KAo+IAkJX19maWVsZChfX3U4LCBiUmVx dWVzdFR5cGUpCj4gCQlfX2ZpZWxkKF9fdTgsIGJSZXF1ZXN0KQo+IAkJX19maWVsZChfX3UxNiwg d1ZhbHVlKQo+IAkJX19maWVsZChfX3UxNiwgd0luZGV4KQo+IAkJX19maWVsZChfX3UxNiwgd0xl bmd0aCkKPiAJCV9fZHluYW1pY19hcnJheShjaGFyLCBzdHIsIERXQzNfTVNHX01BWCkKPiAJKSwK PiAJVFBfZmFzdF9hc3NpZ24oCj4gCQlfX2VudHJ5LT5iUmVxdWVzdFR5cGUgPSBjdHJsLT5iUmVx dWVzdFR5cGU7Cj4gCQlfX2VudHJ5LT5iUmVxdWVzdCA9IGN0cmwtPmJSZXF1ZXN0Owo+IAkJX19l bnRyeS0+d1ZhbHVlID0gbGUxNl90b19jcHUoY3RybC0+d1ZhbHVlKTsKPiAJCV9fZW50cnktPndJ bmRleCA9IGxlMTZfdG9fY3B1KGN0cmwtPndJbmRleCk7Cj4gCQlfX2VudHJ5LT53TGVuZ3RoID0g bGUxNl90b19jcHUoY3RybC0+d0xlbmd0aCk7Cj4gCSksCj4gCVRQX3ByaW50aygiJXMiLCBkd2Mz X2RlY29kZV9jdHJsKF9fZ2V0X3N0cihzdHIpLCBEV0MzX01TR19NQVgsCj4gCQkJCQlfX2VudHJ5 LT5iUmVxdWVzdFR5cGUsCj4gCQkJCQlfX2VudHJ5LT5iUmVxdWVzdCwgX19lbnRyeS0+d1ZhbHVl LAo+IAkJCQkJX19lbnRyeS0+d0luZGV4LCBfX2VudHJ5LT53TGVuZ3RoKQo+IAkpCj4gKTsKPiAK PiBUaGUgYWJvdmUgaXMgdGhlIGNvZGUgaXMgdG9kYXkgKHdlbGwsIEkndmUgYWRkZWQgYnVmZmVy IHNpemUgYXMgYW4KPiBhcmd1bWVudCkuIElmIEkgbWFrZSBkd2MzX2RlY29kZV9jdHJsKCkgcmV0 dXJuIGFuIGludGVnZXIsIEkgY2FuJ3QgY2FsbAo+IGl0IGZyb20gVFBfcHJpbnRrKCkgdGltZS4g SSdkIGhhdmUgdG8gbW92ZSBpdCB0byBUUF9mYXN0X2Fzc2lnbigpIHRpbWUKPiB3aGljaCBpcyBz dXBwb3NlZCB0byBiZSwgc2ltcGx5LCBhIGNvcHkgb2YgdGhlIG5lY2Vzc2FyeSBkYXRhLiBJT1cs IEkKPiB3b3VsZCBoYXZlIHRoaXM6Cj4gCj4gREVDTEFSRV9FVkVOVF9DTEFTUyhkd2MzX2xvZ19j dHJsLAo+IAlUUF9QUk9UTyhzdHJ1Y3QgdXNiX2N0cmxyZXF1ZXN0ICpjdHJsKSwKPiAJVFBfQVJH UyhjdHJsKSwKPiAJVFBfU1RSVUNUX19lbnRyeSgKPiAJCV9fZHluYW1pY19hcnJheShjaGFyLCBz dHIsIERXQzNfTVNHX01BWCkKPiAJKSwKPiAJVFBfZmFzdF9hc3NpZ24oCj4gCQlkd2MzX2RlY29k ZV9jdHJsKF9fZ2V0X3N0cihzdHIpLCBEV0MzX01TR19NQVgsCj4gCQkJCWN0cmwtPmJSZXF1ZXN0 VHlwZSwKPiAJCQkJY3RybC0+YlJlcXVlc3QsCj4gICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICBsZTE2X3RvX2NwdShjdHJsLT53VmFsdWUpLAo+IAkJCQlsZTE2X3RvX2NwdShjdHJsLT53 SW5kZXgpLAo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgbGUxNl90b19jcHUoY3Ry bC0+d0xlbmd0aCkpOwo+IAkpLAo+IAlUUF9wcmludGsoIiVzIiwgX19nZXRfc3RyKHN0cikKPiAJ KQo+ICk7Cj4gCj4gRXNzZW50aWFsbHksIHdlIGVuZCB1cCBtb3ZpbmcgZGVjb2Rpbmcgb2YgdGhl IHRyYWNlcG9pbnQgdG8gdGhlIHRpbWUgaXQKPiBpcyBjYXB0dXJlZDsgSU9XLCB3ZSByZWludHJv ZHVjZSByZWd1bGFyIGxhdGVuY3kgb2Ygc3RyaW5nIGZvcm1hdHRpbmcuCj4gCj4gV2hhdCB3ZSBj b3VsZCBkbywgaXMgbW92ZSBhbGwgZnVuY3Rpb25zIGNhbGxlZCBieSBkd2MzX2RlY29kZV9jdHJs KCkgdG8KPiByZXR1cm4gaW50LCBidXQgbGVhdmUgZHdjM19kZWNvZGVfY3RybCgpIHJldHVybmlu ZyBhIHBvaW50ZXIgdG8gc3RyIGp1c3QKPiBzbyB3ZSBjb250aW51ZSBkZWNvZGluZyB0aGUgZGF0 YSBhdCBwcmludGluZyB0aW1lLgoKT2ssIGl0IHdhc24ndCBvYnZpb3VzIHRoYXQgdGhpcyB3YXMg dXNlZCBpbiBhIHRyYWNlcG9pbnQgbGlrZSB0aGlzLCB0aGF0Cm1ha2VzIG1vcmUgc2Vuc2UuCgpT bywgaXQgc2hvdWxkIGJlIGRvY3VtZW50ZWQgYXMgd2VsbCA6KQoKdGhhbmtzLAoKZ3JlZyBrLWgK From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg KH Subject: Re: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver. Date: Mon, 4 Feb 2019 15:47:25 +0100 Message-ID: <20190204144725.GA31360@kroah.com> References: <1548935553-452-1-git-send-email-pawell@cadence.com> <1548935553-452-3-git-send-email-pawell@cadence.com> <20190204114502.GA28608@kroah.com> <87y36vmyeb.fsf@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <87y36vmyeb.fsf@linux.intel.com> Sender: linux-kernel-owner@vger.kernel.org To: Felipe Balbi Cc: Pawel Laszczak , devicetree@vger.kernel.org, mark.rutland@arm.com, linux-usb@vger.kernel.org, hdegoede@redhat.com, heikki.krogerus@linux.intel.com, andy.shevchenko@gmail.com, robh+dt@kernel.org, rogerq@ti.com, linux-kernel@vger.kernel.org, jbergsagel@ti.com, nsekhar@ti.com, nm@ti.com, sureshp@cadence.com, peter.chen@nxp.com, pjez@cadence.com, kurahul@cadence.com List-Id: devicetree@vger.kernel.org On Mon, Feb 04, 2019 at 04:17:00PM +0200, Felipe Balbi wrote: > > Hi, > > Greg KH writes: > > On Thu, Jan 31, 2019 at 11:52:29AM +0000, Pawel Laszczak wrote: > >> Patch moves some decoding functions from driver/usb/dwc3/debug.h driver > >> to driver/usb/common/debug.c file. These moved functions include: > >> dwc3_decode_get_status > >> dwc3_decode_set_clear_feature > >> dwc3_decode_set_address > >> dwc3_decode_get_set_descriptor > >> dwc3_decode_get_configuration > >> dwc3_decode_set_configuration > >> dwc3_decode_get_intf > >> dwc3_decode_set_intf > >> dwc3_decode_synch_frame > >> dwc3_decode_set_sel > >> dwc3_decode_set_isoch_delay > >> dwc3_decode_ctrl > >> > >> These functions are used also in inroduced cdns3 driver. > >> > >> All functions prefixes were changed from dwc3 to usb. > > > > Ick, why? > > moving dwc3-specific code into generic code. That says what it does, but not why :) And if this really was just moving things around, why was only one symbol needed to be exported and not all of them? > >> + * @bRequestType: matches the USB bmRequestType field > >> + * @bRequest: matches the USB bRequest field > >> + * @wValue: matches the USB wValue field (CPU byte order) > >> + * @wIndex: matches the USB wIndex field (CPU byte order) > >> + * @wLength: matches the USB wLength field (CPU byte order) > >> + * > >> + * Function returns decoded, formatted and human-readable description of > >> + * control request packet. > >> + * > >> + * Important: wValue, wIndex, wLength parameters before invoking this function > >> + * should be processed by le16_to_cpu macro. > >> + */ > >> +const char *usb_decode_ctrl(char *str, __u8 bRequestType, __u8 bRequest, > >> + __u16 wValue, __u16 wIndex, __u16 wLength); > > > > Why are you returning a value, isn't the data stored in str? Why not > > just return an int saying if the call succeeded or not? > > There is one detail here. The usage scenario for this is for > tracepoints. When dealing with tracepoints we want to delay decoding of > the data into strings until print-time. I guess it's best to illustrate > with an example: > > DECLARE_EVENT_CLASS(dwc3_log_ctrl, > TP_PROTO(struct usb_ctrlrequest *ctrl), > TP_ARGS(ctrl), > TP_STRUCT__entry( > __field(__u8, bRequestType) > __field(__u8, bRequest) > __field(__u16, wValue) > __field(__u16, wIndex) > __field(__u16, wLength) > __dynamic_array(char, str, DWC3_MSG_MAX) > ), > TP_fast_assign( > __entry->bRequestType = ctrl->bRequestType; > __entry->bRequest = ctrl->bRequest; > __entry->wValue = le16_to_cpu(ctrl->wValue); > __entry->wIndex = le16_to_cpu(ctrl->wIndex); > __entry->wLength = le16_to_cpu(ctrl->wLength); > ), > TP_printk("%s", dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX, > __entry->bRequestType, > __entry->bRequest, __entry->wValue, > __entry->wIndex, __entry->wLength) > ) > ); > > The above is the code is today (well, I've added buffer size as an > argument). If I make dwc3_decode_ctrl() return an integer, I can't call > it from TP_printk() time. I'd have to move it to TP_fast_assign() time > which is supposed to be, simply, a copy of the necessary data. IOW, I > would have this: > > DECLARE_EVENT_CLASS(dwc3_log_ctrl, > TP_PROTO(struct usb_ctrlrequest *ctrl), > TP_ARGS(ctrl), > TP_STRUCT__entry( > __dynamic_array(char, str, DWC3_MSG_MAX) > ), > TP_fast_assign( > dwc3_decode_ctrl(__get_str(str), DWC3_MSG_MAX, > ctrl->bRequestType, > ctrl->bRequest, > le16_to_cpu(ctrl->wValue), > le16_to_cpu(ctrl->wIndex), > le16_to_cpu(ctrl->wLength)); > ), > TP_printk("%s", __get_str(str) > ) > ); > > Essentially, we end up moving decoding of the tracepoint to the time it > is captured; IOW, we reintroduce regular latency of string formatting. > > What we could do, is move all functions called by dwc3_decode_ctrl() to > return int, but leave dwc3_decode_ctrl() returning a pointer to str just > so we continue decoding the data at printing time. Ok, it wasn't obvious that this was used in a tracepoint like this, that makes more sense. So, it should be documented as well :) thanks, greg k-h