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: Felipe Balbi Message-Id: <87y36vmyeb.fsf@linux.intel.com> Date: Mon, 04 Feb 2019 16:17:00 +0200 To: Greg KH , Pawel Laszczak Cc: 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: SGksCgpHcmVnIEtIIDxncmVna2hAbGludXhmb3VuZGF0aW9uLm9yZz4gd3JpdGVzOgo+IE9uIFRo dSwgSmFuIDMxLCAyMDE5IGF0IDExOjUyOjI5QU0gKzAwMDAsIFBhd2VsIExhc3pjemFrIHdyb3Rl Ogo+PiBQYXRjaCBtb3ZlcyBzb21lIGRlY29kaW5nIGZ1bmN0aW9ucyBmcm9tIGRyaXZlci91c2Iv ZHdjMy9kZWJ1Zy5oIGRyaXZlcgo+PiB0byBkcml2ZXIvdXNiL2NvbW1vbi9kZWJ1Zy5jIGZpbGUu IFRoZXNlIG1vdmVkIGZ1bmN0aW9ucyBpbmNsdWRlOgo+PiAgICAgZHdjM19kZWNvZGVfZ2V0X3N0 YXR1cwo+PiAgICAgZHdjM19kZWNvZGVfc2V0X2NsZWFyX2ZlYXR1cmUKPj4gICAgIGR3YzNfZGVj b2RlX3NldF9hZGRyZXNzCj4+ICAgICBkd2MzX2RlY29kZV9nZXRfc2V0X2Rlc2NyaXB0b3IKPj4g ICAgIGR3YzNfZGVjb2RlX2dldF9jb25maWd1cmF0aW9uCj4+ICAgICBkd2MzX2RlY29kZV9zZXRf Y29uZmlndXJhdGlvbgo+PiAgICAgZHdjM19kZWNvZGVfZ2V0X2ludGYKPj4gICAgIGR3YzNfZGVj b2RlX3NldF9pbnRmCj4+ICAgICBkd2MzX2RlY29kZV9zeW5jaF9mcmFtZQo+PiAgICAgZHdjM19k ZWNvZGVfc2V0X3NlbAo+PiAgICAgZHdjM19kZWNvZGVfc2V0X2lzb2NoX2RlbGF5Cj4+ICAgICBk d2MzX2RlY29kZV9jdHJsCj4+IAo+PiBUaGVzZSBmdW5jdGlvbnMgYXJlIHVzZWQgYWxzbyBpbiBp bnJvZHVjZWQgY2RuczMgZHJpdmVyLgo+PiAKPj4gQWxsIGZ1bmN0aW9ucyBwcmVmaXhlcyB3ZXJl IGNoYW5nZWQgZnJvbSBkd2MzIHRvIHVzYi4KPgo+IEljaywgd2h5PwoKbW92aW5nIGR3YzMtc3Bl Y2lmaWMgY29kZSBpbnRvIGdlbmVyaWMgY29kZS4KCj4+IEFsc28sIGZ1bmN0aW9uJ3MgcGFyYW1l dGVycyBoYXMgYmVlbiBleHRlbmRlZCBhY2NvcmRpbmcgdG8gdGhlIG5hbWUKPj4gb2YgZmllbGRz IGluIHN0YW5kYXJkIFNFVFVQIHBhY2tldC4KPj4gQWRkaXRpb25hbGx5LCBwYXRjaCBhZGRzIHVz Yl9kZWNvZGVfY3RybCBmdW5jdGlvbiB0bwo+PiBpbmNsdWRlL2xpbnV4L3VzYi9jaDkuaCBmaWxl Lgo+Cj4gV2h5IGNoOS5oPyAgSXQncyBub3Qgc29tZXRoaW5nIHRoYXQgaXMgc3BlY2lmaWVkIGlu IHRoZSBzcGVjLCBpdCdzIGEKPiB1c2Itc3BlY2lmaWMgdGhpbmcgOikKCmFncmVlLgoKPj4gKy8q Kgo+PiArICogdXNiX2RlY29kZV9jdHJsIC0gUmV0dXJucyBodW1hbiByZWFkYWJsZSByZXByZXNl bnRhdGlvbiBvZiBjb250cm9sIHJlcXVlc3QuCj4+ICsgKiBAc3RyOiBidWZmZXIgdG8gcmV0dXJu IGEgaHVtYW4tcmVhZGFibGUgcmVwcmVzZW50YXRpb24gb2YgY29udHJvbCByZXF1ZXN0Lgo+PiAr ICogICAgICAgVGhpcyBidWZmZXIgc2hvdWxkIGhhdmUgYWJvdXQgMjAwIGJ5dGVzLgo+Cj4gImFi b3V0IDIwMCBieXRlcyIgaXMgbm90IHZlcnkgc3BlY2lmaWMuCj4KPiBQYXNzIGluIHRoZSBsZW5n dGggc28gd2Uga25vdyB3ZSBkb24ndCBvdmVyZmxvdyBpdC4KCmFncmVlLgoKPj4gKyAqIEBiUmVx dWVzdFR5cGU6IG1hdGNoZXMgdGhlIFVTQiBibVJlcXVlc3RUeXBlIGZpZWxkCj4+ICsgKiBAYlJl cXVlc3Q6IG1hdGNoZXMgdGhlIFVTQiBiUmVxdWVzdCBmaWVsZAo+PiArICogQHdWYWx1ZTogbWF0 Y2hlcyB0aGUgVVNCIHdWYWx1ZSBmaWVsZCAoQ1BVIGJ5dGUgb3JkZXIpCj4+ICsgKiBAd0luZGV4 OiBtYXRjaGVzIHRoZSBVU0Igd0luZGV4IGZpZWxkIChDUFUgYnl0ZSBvcmRlcikKPj4gKyAqIEB3 TGVuZ3RoOiBtYXRjaGVzIHRoZSBVU0Igd0xlbmd0aCBmaWVsZCAoQ1BVIGJ5dGUgb3JkZXIpCj4+ ICsgKgo+PiArICogRnVuY3Rpb24gcmV0dXJucyBkZWNvZGVkLCBmb3JtYXR0ZWQgYW5kIGh1bWFu LXJlYWRhYmxlIGRlc2NyaXB0aW9uIG9mCj4+ICsgKiBjb250cm9sIHJlcXVlc3QgcGFja2V0Lgo+ PiArICoKPj4gKyAqIEltcG9ydGFudDogd1ZhbHVlLCB3SW5kZXgsIHdMZW5ndGggcGFyYW1ldGVy cyBiZWZvcmUgaW52b2tpbmcgdGhpcyBmdW5jdGlvbgo+PiArICogc2hvdWxkIGJlIHByb2Nlc3Nl ZCBieSBsZTE2X3RvX2NwdSBtYWNyby4KPj4gKyAqLwo+PiArY29uc3QgY2hhciAqdXNiX2RlY29k ZV9jdHJsKGNoYXIgKnN0ciwgX191OCBiUmVxdWVzdFR5cGUsIF9fdTggYlJlcXVlc3QsCj4+ICsJ CQkgICAgX191MTYgd1ZhbHVlLCAgX191MTYgd0luZGV4LCBfX3UxNiB3TGVuZ3RoKTsKPgo+IFdo eSBhcmUgeW91IHJldHVybmluZyBhIHZhbHVlLCBpc24ndCB0aGUgZGF0YSBzdG9yZWQgaW4gc3Ry PyAgV2h5IG5vdAo+IGp1c3QgcmV0dXJuIGFuIGludCBzYXlpbmcgaWYgdGhlIGNhbGwgc3VjY2Vl ZGVkIG9yIG5vdD8KClRoZXJlIGlzIG9uZSBkZXRhaWwgaGVyZS4gVGhlIHVzYWdlIHNjZW5hcmlv IGZvciB0aGlzIGlzIGZvcgp0cmFjZXBvaW50cy4gV2hlbiBkZWFsaW5nIHdpdGggdHJhY2Vwb2lu dHMgd2Ugd2FudCB0byBkZWxheSBkZWNvZGluZyBvZgp0aGUgZGF0YSBpbnRvIHN0cmluZ3MgdW50 aWwgcHJpbnQtdGltZS4gSSBndWVzcyBpdCdzIGJlc3QgdG8gaWxsdXN0cmF0ZQp3aXRoIGFuIGV4 YW1wbGU6CgpERUNMQVJFX0VWRU5UX0NMQVNTKGR3YzNfbG9nX2N0cmwsCglUUF9QUk9UTyhzdHJ1 Y3QgdXNiX2N0cmxyZXF1ZXN0ICpjdHJsKSwKCVRQX0FSR1MoY3RybCksCglUUF9TVFJVQ1RfX2Vu dHJ5KAoJCV9fZmllbGQoX191OCwgYlJlcXVlc3RUeXBlKQoJCV9fZmllbGQoX191OCwgYlJlcXVl c3QpCgkJX19maWVsZChfX3UxNiwgd1ZhbHVlKQoJCV9fZmllbGQoX191MTYsIHdJbmRleCkKCQlf X2ZpZWxkKF9fdTE2LCB3TGVuZ3RoKQoJCV9fZHluYW1pY19hcnJheShjaGFyLCBzdHIsIERXQzNf TVNHX01BWCkKCSksCglUUF9mYXN0X2Fzc2lnbigKCQlfX2VudHJ5LT5iUmVxdWVzdFR5cGUgPSBj dHJsLT5iUmVxdWVzdFR5cGU7CgkJX19lbnRyeS0+YlJlcXVlc3QgPSBjdHJsLT5iUmVxdWVzdDsK CQlfX2VudHJ5LT53VmFsdWUgPSBsZTE2X3RvX2NwdShjdHJsLT53VmFsdWUpOwoJCV9fZW50cnkt PndJbmRleCA9IGxlMTZfdG9fY3B1KGN0cmwtPndJbmRleCk7CgkJX19lbnRyeS0+d0xlbmd0aCA9 IGxlMTZfdG9fY3B1KGN0cmwtPndMZW5ndGgpOwoJKSwKCVRQX3ByaW50aygiJXMiLCBkd2MzX2Rl Y29kZV9jdHJsKF9fZ2V0X3N0cihzdHIpLCBEV0MzX01TR19NQVgsCgkJCQkJX19lbnRyeS0+YlJl cXVlc3RUeXBlLAoJCQkJCV9fZW50cnktPmJSZXF1ZXN0LCBfX2VudHJ5LT53VmFsdWUsCgkJCQkJ X19lbnRyeS0+d0luZGV4LCBfX2VudHJ5LT53TGVuZ3RoKQoJKQopOwoKVGhlIGFib3ZlIGlzIHRo ZSBjb2RlIGlzIHRvZGF5ICh3ZWxsLCBJJ3ZlIGFkZGVkIGJ1ZmZlciBzaXplIGFzIGFuCmFyZ3Vt ZW50KS4gSWYgSSBtYWtlIGR3YzNfZGVjb2RlX2N0cmwoKSByZXR1cm4gYW4gaW50ZWdlciwgSSBj YW4ndCBjYWxsCml0IGZyb20gVFBfcHJpbnRrKCkgdGltZS4gSSdkIGhhdmUgdG8gbW92ZSBpdCB0 byBUUF9mYXN0X2Fzc2lnbigpIHRpbWUKd2hpY2ggaXMgc3VwcG9zZWQgdG8gYmUsIHNpbXBseSwg YSBjb3B5IG9mIHRoZSBuZWNlc3NhcnkgZGF0YS4gSU9XLCBJCndvdWxkIGhhdmUgdGhpczoKCkRF Q0xBUkVfRVZFTlRfQ0xBU1MoZHdjM19sb2dfY3RybCwKCVRQX1BST1RPKHN0cnVjdCB1c2JfY3Ry bHJlcXVlc3QgKmN0cmwpLAoJVFBfQVJHUyhjdHJsKSwKCVRQX1NUUlVDVF9fZW50cnkoCgkJX19k eW5hbWljX2FycmF5KGNoYXIsIHN0ciwgRFdDM19NU0dfTUFYKQoJKSwKCVRQX2Zhc3RfYXNzaWdu KAoJCWR3YzNfZGVjb2RlX2N0cmwoX19nZXRfc3RyKHN0ciksIERXQzNfTVNHX01BWCwKCQkJCWN0 cmwtPmJSZXF1ZXN0VHlwZSwKCQkJCWN0cmwtPmJSZXF1ZXN0LAogICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgIGxlMTZfdG9fY3B1KGN0cmwtPndWYWx1ZSksCgkJCQlsZTE2X3RvX2NwdShj dHJsLT53SW5kZXgpLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGxlMTZfdG9fY3B1 KGN0cmwtPndMZW5ndGgpKTsKCSksCglUUF9wcmludGsoIiVzIiwgX19nZXRfc3RyKHN0cikKCSkK KTsKCkVzc2VudGlhbGx5LCB3ZSBlbmQgdXAgbW92aW5nIGRlY29kaW5nIG9mIHRoZSB0cmFjZXBv aW50IHRvIHRoZSB0aW1lIGl0CmlzIGNhcHR1cmVkOyBJT1csIHdlIHJlaW50cm9kdWNlIHJlZ3Vs YXIgbGF0ZW5jeSBvZiBzdHJpbmcgZm9ybWF0dGluZy4KCldoYXQgd2UgY291bGQgZG8sIGlzIG1v dmUgYWxsIGZ1bmN0aW9ucyBjYWxsZWQgYnkgZHdjM19kZWNvZGVfY3RybCgpIHRvCnJldHVybiBp bnQsIGJ1dCBsZWF2ZSBkd2MzX2RlY29kZV9jdHJsKCkgcmV0dXJuaW5nIGEgcG9pbnRlciB0byBz dHIganVzdApzbyB3ZSBjb250aW51ZSBkZWNvZGluZyB0aGUgZGF0YSBhdCBwcmludGluZyB0aW1l Lgo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: Felipe Balbi Subject: Re: [PATCH v3 2/6] usb:common Separated decoding functions from dwc3 driver. Date: Mon, 04 Feb 2019 16:17:00 +0200 Message-ID: <87y36vmyeb.fsf@linux.intel.com> References: <1548935553-452-1-git-send-email-pawell@cadence.com> <1548935553-452-3-git-send-email-pawell@cadence.com> <20190204114502.GA28608@kroah.com> Mime-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Return-path: In-Reply-To: <20190204114502.GA28608@kroah.com> Sender: linux-kernel-owner@vger.kernel.org To: Greg KH , Pawel Laszczak Cc: 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 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable 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 >>=20 >> These functions are used also in inroduced cdns3 driver. >>=20 >> All functions prefixes were changed from dwc3 to usb. > > Ick, why? moving dwc3-specific code into generic code. >> Also, function's parameters has been extended according to the name >> of fields in standard SETUP packet. >> Additionally, patch adds usb_decode_ctrl function to >> include/linux/usb/ch9.h file. > > Why ch9.h? It's not something that is specified in the spec, it's a > usb-specific thing :) agree. >> +/** >> + * usb_decode_ctrl - Returns human readable representation of control r= equest. >> + * @str: buffer to return a human-readable representation of control re= quest. >> + * This buffer should have about 200 bytes. > > "about 200 bytes" is not very specific. > > Pass in the length so we know we don't overflow it. agree. >> + * @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 f= unction >> + * 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 =3D ctrl->bRequestType; __entry->bRequest =3D ctrl->bRequest; __entry->wValue =3D le16_to_cpu(ctrl->wValue); __entry->wIndex =3D le16_to_cpu(ctrl->wIndex); __entry->wLength =3D 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. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlxYSVwACgkQzL64meEa mQYMAg/9GRynhyFknrnHfzpIDuI7ppCIBy4YOBd0LtU7TkwS72pB2FssCdls0yr1 apKfiI3E33CKAm+bCRXzVpSfe/w0cC/5P4ERacKLhH86MKbP+WHgsZbHc5U0NpSW 5GnBIY5zUjNfLlBGZ8ljRvWgsDHVMEPQtXyPU1GRJ9LyMGSdBdWCKRAMA2z+rqEb 1HsugGWdT5tE8oHWe6o+5dnbkKMHFwVMT2AQwm/grl0FvZ9GZ8rDGRUyy0wZFmA3 FOkdtnb2/j6QBvdz2Fpi/TWDVzHQUhi0w77izBmJeMvfgORTdLdTiH4AcVrqcrgd ncZHz+nUoWC85jq9Pc/MCQ0F9Ts7uZaqAJo0Zl8dtSoSKpXSWHaKdWWRnZ28XCx2 HptOxuoNC16cFfuKOJa5Gl7KHANKiUyE7s1QWGvIYC2qReuAVCeeFycTNRkp0omR GYmFn9+4L0aBLMWZa/E0q/kRd8txWi1+kjoMW47MgPAr3C+8DZolzaByfsn1SX8f HTrVTIesZ4872l8RSjN5j5os7J4F1Wb//Xbq1Jsc9nw1NLyLJcYGZcYsp6R6Iir3 xsXb/gQkjF6btg68QYeF6PLFmMT4ktXlm1DxtuZI+PoCv/lyTZmCO1MNr1+wqvKf Xav2TQtvOeARWYWZVG1Me6iiU+2+sSsghuhQivAHugV3CM3pvSc= =vcs9 -----END PGP SIGNATURE----- --=-=-=--