From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com ([134.134.136.65]:46441 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750944AbeEUIGc (ORCPT ); Mon, 21 May 2018 04:06:32 -0400 From: Felipe Balbi To: Yoshihiro Shimoda Cc: "gregkh\@linuxfoundation.org" , "linux-usb\@vger.kernel.org" , "linux-renesas-soc\@vger.kernel.org" Subject: RE: [PATCH/RFC] usb: gadget: function: printer: avoid wrong list handling in printer_write() In-Reply-To: References: <1526632641-30086-1-git-send-email-yoshihiro.shimoda.uh@renesas.com> <87r2m58pir.fsf@linux.intel.com> Date: Mon, 21 May 2018 11:04:45 +0300 Message-ID: <87603h8mea.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Yoshihiro Shimoda writes: > Hi, > >> From: Felipe Balbi, Sent: Monday, May 21, 2018 3:57 PM >>=20 >> Hi, >>=20 >> Yoshihiro Shimoda writes: >> > The usb_ep_queue() in printer_write() is possible to call req->complet= e(). >> > In that case, since tx_complete() calls list_add(&req->list, &dev->tx_= reqs), >> > printer_write() should not call list_add(&req->list, &dev->tx_reqs_act= ive) >> > because the transfer has already finished. So, this patch checks >> > the condition of req->list before adding the list in printer_write(). >> > >> > Signed-off-by: Yoshihiro Shimoda >> > --- >> > This issue can be caused by renesas_usbhs udc driver. I'm not sure >> > this patch is acceptable or not. So, I marked RFC on this patch. >>=20 >> can you explain this a little more? How do you trigger the problem? > > Sure. If printer_write() called usb_ep_queue() with 63 bytes or less data, > the renesas_usbhs udc driver transfers data as PIO. In this case, the udc > driver calls usb_gadget_giveback_reuqest() in .queue ops (usbhsg_ep_queue= ()) > immediately. Then, printer_write() calls list_add(&req->list, &dev->tx_re= qs_active); wrongly. > After that, if we do rmmod g_printer, WARN_ON(!list_empty(&dev->tx_reqs_a= ctive); happens in > printer_func_unbind() because the list entry is not removed. > > < Reference: calltrace (very long though...) > > usb_ep_queue(...); at f_printer.c / printer_write() > 1-> usbhsg_ep_queue(); at renesas_usbhs/mod_gadget.c > 2-> usbhsg_queue_push(); > 3-> usbhs_pkt_start(); at renesas_usbhs/fifo.c > 4-> usbhsf_pkt_handler(); > 5-> func() =3D usbhsf_dma_prepare_push(); > 6-> goto usbhsf_pio_prepare_push; // Because len is 63 > 7-> usbhsf_pio_prepare_push(); > 8-> usbhsf_pio_try_push(); > 5-> done() =3D usbhsg_queue_done(); at renesas_usbhs/mod_gadg= et.c > 6-> __usbsg_queue_pop(); > 7-> usb_gadget_giveback_reuqest(); > 8-> tx_complete(); at f_printer.c > 9-> list_del_init(&req->list); > 9-> list_add(&req->list, &dev->tx_reqs); > list_add(&req->list, &dev->tx_reqs_active); // Even if the transaction a= lready finished, this driver is possible to add the list to "active". seems like it would be better to just move this like before usb_ep_queue(): modified drivers/usb/gadget/function/f_printer.c @@ -631,19 +631,19 @@ printer_write(struct file *fd, const char __user *buf= , size_t len, loff_t *ptr) return -EAGAIN; } =20 + list_add(&req->list, &dev->tx_reqs_active); + /* here, we unlock, and only unlock, to avoid deadlock. */ spin_unlock(&dev->lock); value =3D usb_ep_queue(dev->in_ep, req, GFP_ATOMIC); spin_lock(&dev->lock); if (value) { + list_del(&req->list); list_add(&req->list, &dev->tx_reqs); spin_unlock_irqrestore(&dev->lock, flags); mutex_unlock(&dev->lock_printer_io); return -EAGAIN; } =2D =2D list_add(&req->list, &dev->tx_reqs_active); =2D } =20 spin_unlock_irqrestore(&dev->lock, flags); =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlsCfZ0ACgkQzL64meEa mQYXKg//frwLouy+TW0wSb7Xhu/t9GqT1Ss8CuLDnh1mlLn/PmhOa/iu9WnP4y1/ e7pJF/I3xjxEHDl8AUPz1fE5dh5lVJIgdAZpsgwmNVzYs7rIi8pwfoOcPwAGQMhV aS+aWs9LAKgpHqEj+zel13i0nQmEmz+NjDrLlCnC8J0M3y5aIDUXtOob2MPiXfLl 7GArjnlGr/1JSY9E/IRMrc8QcEA9Kb6QwJW1AdR90co/vjLes+auWZvAd6oJEFvO RpFyHkwD1XIwGVfr+wKGUk2pGYYAb3FdsqfYxf/+eIcYhxF2I0+DdCQHL7H6Pa9w 3pgpTnqeKqYHVIrk6jsCVrJQjoQPSA7N1UIlQO4FbE7Jgi2K3B1sLZjX1esn6998 hhXRfL4wCrKKF07/RVulpl3vbxnhkGomH9M9PD0fpr2IToEY38KMmGZX966rvB/N pk93VVe01SW3C0WwiOwnokwzlojnFgQDqiSmVVfMw8/dmeevBxCecvTSMlCCaYBu hS15ylmJYTPzqZbUzvyzjPxLn1nuxFOobaKMaNPlfijMK0SfPWAgIovjISBR3ecA iU+vJ1j9jlFheI9rzU7fr2svRV+C9k67SHMZqrh35Fgf+8SGdLuuuSrdV+zxvPKU 3ZFLBIDi7Ja6ybGkloLCjK5aitC2SpFdjxKrPCwt9l/LDXTjcLg= =s1bR -----END PGP SIGNATURE----- --=-=-=-- 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: [PATCH/RFC] usb: gadget: function: printer: avoid wrong list handling in printer_write() From: Felipe Balbi Message-Id: <87603h8mea.fsf@linux.intel.com> Date: Mon, 21 May 2018 11:04:45 +0300 To: Yoshihiro Shimoda Cc: "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-renesas-soc@vger.kernel.org" List-ID: SGksCgpZb3NoaWhpcm8gU2hpbW9kYSA8eW9zaGloaXJvLnNoaW1vZGEudWhAcmVuZXNhcy5jb20+ IHdyaXRlczoKCj4gSGksCj4KPj4gRnJvbTogRmVsaXBlIEJhbGJpLCBTZW50OiBNb25kYXksIE1h eSAyMSwgMjAxOCAzOjU3IFBNCj4+IAo+PiBIaSwKPj4gCj4+IFlvc2hpaGlybyBTaGltb2RhIDx5 b3NoaWhpcm8uc2hpbW9kYS51aEByZW5lc2FzLmNvbT4gd3JpdGVzOgo+PiA+IFRoZSB1c2JfZXBf cXVldWUoKSBpbiBwcmludGVyX3dyaXRlKCkgaXMgcG9zc2libGUgdG8gY2FsbCByZXEtPmNvbXBs ZXRlKCkuCj4+ID4gSW4gdGhhdCBjYXNlLCBzaW5jZSB0eF9jb21wbGV0ZSgpIGNhbGxzIGxpc3Rf YWRkKCZyZXEtPmxpc3QsICZkZXYtPnR4X3JlcXMpLAo+PiA+IHByaW50ZXJfd3JpdGUoKSBzaG91 bGQgbm90IGNhbGwgbGlzdF9hZGQoJnJlcS0+bGlzdCwgJmRldi0+dHhfcmVxc19hY3RpdmUpCj4+ ID4gYmVjYXVzZSB0aGUgdHJhbnNmZXIgaGFzIGFscmVhZHkgZmluaXNoZWQuIFNvLCB0aGlzIHBh dGNoIGNoZWNrcwo+PiA+IHRoZSBjb25kaXRpb24gb2YgcmVxLT5saXN0IGJlZm9yZSBhZGRpbmcg dGhlIGxpc3QgaW4gcHJpbnRlcl93cml0ZSgpLgo+PiA+Cj4+ID4gU2lnbmVkLW9mZi1ieTogWW9z aGloaXJvIFNoaW1vZGEgPHlvc2hpaGlyby5zaGltb2RhLnVoQHJlbmVzYXMuY29tPgo+PiA+IC0t LQo+PiA+ICBUaGlzIGlzc3VlIGNhbiBiZSBjYXVzZWQgYnkgcmVuZXNhc191c2JocyB1ZGMgZHJp dmVyLiBJJ20gbm90IHN1cmUKPj4gPiAgdGhpcyBwYXRjaCBpcyBhY2NlcHRhYmxlIG9yIG5vdC4g U28sIEkgbWFya2VkIFJGQyBvbiB0aGlzIHBhdGNoLgo+PiAKPj4gY2FuIHlvdSBleHBsYWluIHRo aXMgYSBsaXR0bGUgbW9yZT8gSG93IGRvIHlvdSB0cmlnZ2VyIHRoZSBwcm9ibGVtPwo+Cj4gU3Vy ZS4gSWYgcHJpbnRlcl93cml0ZSgpIGNhbGxlZCB1c2JfZXBfcXVldWUoKSB3aXRoIDYzIGJ5dGVz IG9yIGxlc3MgZGF0YSwKPiB0aGUgcmVuZXNhc191c2JocyB1ZGMgZHJpdmVyIHRyYW5zZmVycyBk YXRhIGFzIFBJTy4gSW4gdGhpcyBjYXNlLCB0aGUgdWRjCj4gZHJpdmVyIGNhbGxzIHVzYl9nYWRn ZXRfZ2l2ZWJhY2tfcmV1cWVzdCgpIGluIC5xdWV1ZSBvcHMgKHVzYmhzZ19lcF9xdWV1ZSgpKQo+ IGltbWVkaWF0ZWx5LiBUaGVuLCBwcmludGVyX3dyaXRlKCkgY2FsbHMgbGlzdF9hZGQoJnJlcS0+ bGlzdCwgJmRldi0+dHhfcmVxc19hY3RpdmUpOyB3cm9uZ2x5Lgo+IEFmdGVyIHRoYXQsIGlmIHdl IGRvIHJtbW9kIGdfcHJpbnRlciwgV0FSTl9PTighbGlzdF9lbXB0eSgmZGV2LT50eF9yZXFzX2Fj dGl2ZSk7IGhhcHBlbnMgaW4KPiBwcmludGVyX2Z1bmNfdW5iaW5kKCkgYmVjYXVzZSB0aGUgbGlz dCBlbnRyeSBpcyBub3QgcmVtb3ZlZC4KPgo+IDwgUmVmZXJlbmNlOiBjYWxsdHJhY2UgKHZlcnkg bG9uZyB0aG91Z2guLi4pID4KPiAJdXNiX2VwX3F1ZXVlKC4uLik7CQlhdCBmX3ByaW50ZXIuYyAv IHByaW50ZXJfd3JpdGUoKQo+IAkxLT4gdXNiaHNnX2VwX3F1ZXVlKCk7CQlhdCByZW5lc2FzX3Vz YmhzL21vZF9nYWRnZXQuYwo+IAkgMi0+IHVzYmhzZ19xdWV1ZV9wdXNoKCk7Cj4gCSAgMy0+IHVz YmhzX3BrdF9zdGFydCgpOwkJYXQgcmVuZXNhc191c2Jocy9maWZvLmMKPiAJICAgNC0+IHVzYmhz Zl9wa3RfaGFuZGxlcigpOwo+IAkgICAgNS0+IGZ1bmMoKSA9IHVzYmhzZl9kbWFfcHJlcGFyZV9w dXNoKCk7Cj4gCSAgICAgNi0+IGdvdG8gdXNiaHNmX3Bpb19wcmVwYXJlX3B1c2g7IC8vIEJlY2F1 c2UgbGVuIGlzIDYzCj4gCSAgICAgIDctPiB1c2Joc2ZfcGlvX3ByZXBhcmVfcHVzaCgpOwo+IAkg ICAgICAgOC0+IHVzYmhzZl9waW9fdHJ5X3B1c2goKTsKPiAgICAgICAgICAgICA1LT4gZG9uZSgp ID0gdXNiaHNnX3F1ZXVlX2RvbmUoKTsJYXQgcmVuZXNhc191c2Jocy9tb2RfZ2FkZ2V0LmMKPiAJ ICAgICA2LT4gX191c2JzZ19xdWV1ZV9wb3AoKTsKPiAJICAgICAgNy0+IHVzYl9nYWRnZXRfZ2l2 ZWJhY2tfcmV1cWVzdCgpOwo+IAkgICAgICAgOC0+IHR4X2NvbXBsZXRlKCk7CQlhdCBmX3ByaW50 ZXIuYwo+ICAgICAgICAgICAgICAgICA5LT4gbGlzdF9kZWxfaW5pdCgmcmVxLT5saXN0KTsKPiAg ICAgICAgICAgICAgICAgOS0+IGxpc3RfYWRkKCZyZXEtPmxpc3QsICZkZXYtPnR4X3JlcXMpOwo+ IAlsaXN0X2FkZCgmcmVxLT5saXN0LCAmZGV2LT50eF9yZXFzX2FjdGl2ZSk7CS8vIEV2ZW4gaWYg dGhlIHRyYW5zYWN0aW9uIGFscmVhZHkgZmluaXNoZWQsIHRoaXMgZHJpdmVyIGlzIHBvc3NpYmxl IHRvIGFkZCB0aGUgbGlzdCB0byAiYWN0aXZlIi4KCnNlZW1zIGxpa2UgaXQgd291bGQgYmUgYmV0 dGVyIHRvIGp1c3QgbW92ZSB0aGlzIGxpa2UgYmVmb3JlCnVzYl9lcF9xdWV1ZSgpOgoKbW9kaWZp ZWQgICBkcml2ZXJzL3VzYi9nYWRnZXQvZnVuY3Rpb24vZl9wcmludGVyLmMKQEAgLTYzMSwxOSAr NjMxLDE5IEBAIHByaW50ZXJfd3JpdGUoc3RydWN0IGZpbGUgKmZkLCBjb25zdCBjaGFyIF9fdXNl ciAqYnVmLCBzaXplX3QgbGVuLCBsb2ZmX3QgKnB0cikKIAkJCXJldHVybiAtRUFHQUlOOwogCQl9 CiAKKwkJbGlzdF9hZGQoJnJlcS0+bGlzdCwgJmRldi0+dHhfcmVxc19hY3RpdmUpOworCiAJCS8q IGhlcmUsIHdlIHVubG9jaywgYW5kIG9ubHkgdW5sb2NrLCB0byBhdm9pZCBkZWFkbG9jay4gKi8K IAkJc3Bpbl91bmxvY2soJmRldi0+bG9jayk7CiAJCXZhbHVlID0gdXNiX2VwX3F1ZXVlKGRldi0+ aW5fZXAsIHJlcSwgR0ZQX0FUT01JQyk7CiAJCXNwaW5fbG9jaygmZGV2LT5sb2NrKTsKIAkJaWYg KHZhbHVlKSB7CisJCQlsaXN0X2RlbCgmcmVxLT5saXN0KTsKIAkJCWxpc3RfYWRkKCZyZXEtPmxp c3QsICZkZXYtPnR4X3JlcXMpOwogCQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmZGV2LT5sb2Nr LCBmbGFncyk7CiAJCQltdXRleF91bmxvY2soJmRldi0+bG9ja19wcmludGVyX2lvKTsKIAkJCXJl dHVybiAtRUFHQUlOOwogCQl9Ci0KLQkJbGlzdF9hZGQoJnJlcS0+bGlzdCwgJmRldi0+dHhfcmVx c19hY3RpdmUpOwotCiAJfQogCiAJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmZGV2LT5sb2NrLCBm bGFncyk7Cg==