Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754007AbbGSVmc (ORCPT ); Sun, 19 Jul 2015 17:42:32 -0400 Received: from svenbrauch.de ([46.38.239.95]:48189 "EHLO svenbrauch.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753723AbbGSVmb (ORCPT ); Sun, 19 Jul 2015 17:42:31 -0400 X-Greylist: delayed 321 seconds by postgrey-1.27 at vger.kernel.org; Sun, 19 Jul 2015 17:42:30 EDT From: Sven Brauch To: Linux Kernel Mailing List Subject: [PATCH] Fix data loss in cdc-acm X-Enigmail-Draft-Status: N1110 Message-ID: <55AC1883.4050605@svenbrauch.de> Date: Sun, 19 Jul 2015 23:37:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="iScfRKCNtMobO0luwosQm5Me9kHfTEwcI" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8019 Lines: 238 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --iScfRKCNtMobO0luwosQm5Me9kHfTEwcI Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Since acm_process_read_urb does not check the return value of tty_insert_flip_string, it can happen that not all data is copied from the urb to the tty if the tty buffer is full and throttling does not set in quickly enough. This problem is very evident for devices with high data throughput; for a device with ~12 MB/s of data transfer, I get a few missing kB of data every few MB transferred randomly. To solve this problem, a check is introduced which verifies that indeed all data was copied from the urb to the tty buffer. If that is not the case, the urb is held in a queue instead of resubmitting it to the USB subsystem right away. When new data arrives or the tty is unthrottled, the queue is emptied again (as far as possible). Effectively, this change will force the transmitting USB device to wait until the tty buffer can accept new data again, instead of discarding the data in this case. Please excuse the poor code quality, I have no experience whatsoever in kernel development. Signed-off-by: Sven Brauch --- drivers/usb/class/cdc-acm.c | 93 +++++++++++++++++++++++++++++++++++++++= ++---- drivers/usb/class/cdc-acm.h | 3 ++ 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c index 5c8f581..eafe64c 100644 --- a/drivers/usb/class/cdc-acm.c +++ b/drivers/usb/class/cdc-acm.c @@ -402,14 +402,68 @@ static int acm_submit_read_urbs(struct acm *acm, gf= p_t mem_flags) return 0; } =20 -static void acm_process_read_urb(struct acm *acm, struct urb *urb) +static int acm_process_read_urb(struct acm *acm, struct urb *urb) { + int size; + if (!urb->actual_length) - return; + return 0; =20 - tty_insert_flip_string(&acm->port, urb->transfer_buffer, - urb->actual_length); + size =3D tty_insert_flip_string(&acm->port, urb->transfer_buffer, + urb->actual_length); tty_flip_buffer_push(&acm->port); + return size; +} + +static bool acm_push_leftover_data_to_tty(struct acm *acm) { + int j, k; + int urb_index; + unsigned long remaining, offset; + int written; + struct urb *urb; + struct acm_rb *rb; + bool may_submit_new =3D true; + + /* TODO: Does this locking mechanism make any sense? I don't know. + Some sort of lock is certainly required. */ + unsigned long flags; + spin_lock_irqsave(&acm->resubmit_lock, flags); + + /* Loop over the queued read urbs, and submit as much data as possible = to the tty. */ + for (j =3D 0; j < ACM_NR; j++) { + urb_index =3D acm->held_urbs[j]; + remaining =3D acm->remaining_data_in_read_urbs[urb_index]; + if (remaining =3D=3D 0) + continue; + + urb =3D acm->read_urbs[urb_index]; + rb =3D urb->context; + offset =3D urb->actual_length - remaining; + written =3D tty_insert_flip_string(&acm->port, (char*) (urb->transfer_= buffer) + offset, + remaining); + tty_flip_buffer_push(&acm->port); + acm->remaining_data_in_read_urbs[urb_index] =3D remaining - written; + if (remaining =3D=3D written) { + /* The urb's buffer was fully submitted to the tty, it can be passed + * to the USB subsystem again. */ + set_bit(rb->index, &acm->read_urbs_free); + acm_submit_read_urb(acm, rb->index, GFP_ATOMIC); + acm->num_held_urbs--; + } + else { + /* There is no point in continuing now, the buffer is full */ + may_submit_new =3D false; + break; + } + } + /* Move the remaining queued urbs to the beginning of the queue */ + for (k =3D 0; k < acm->num_held_urbs; k++) { + acm->held_urbs[k] =3D acm->held_urbs[k+j]; + } + + spin_unlock_irqrestore(&acm->resubmit_lock, flags); + + return may_submit_new; } =20 static void acm_read_bulk_callback(struct urb *urb) @@ -418,6 +472,11 @@ static void acm_read_bulk_callback(struct urb *urb) struct acm *acm =3D rb->instance; unsigned long flags; int status =3D urb->status; + int size; + bool may_resubmit, may_submit_new; + + /* First look if any filled urbs are still in the queue */ + may_submit_new =3D acm_push_leftover_data_to_tty(acm); =20 dev_vdbg(&acm->data->dev, "%s - urb %d, len %d\n", __func__, rb->index, urb->actual_length); @@ -437,20 +496,36 @@ static void acm_read_bulk_callback(struct urb *urb)= =20 usb_mark_last_busy(acm->dev); =20 - acm_process_read_urb(acm, urb); + /* + * Only try to post data of this new urb if the queue is empty, + * otherwise put it to the end of the queue. + */ + size =3D may_submit_new ? acm_process_read_urb(acm, urb) : 0; + + may_resubmit =3D true; /* * Unthrottle may run on another CPU which needs to see events * in the same order. Submission has an implict barrier */ smp_mb__before_atomic(); - set_bit(rb->index, &acm->read_urbs_free); + if (size !=3D urb->actual_length) { + /* do not resubmit the urb, but mark it as filled instead */ + acm->remaining_data_in_read_urbs[rb->index] =3D urb->actual_length - s= ize; + acm->held_urbs[acm->num_held_urbs++] =3D rb->index; + may_resubmit =3D false; + } + else { + set_bit(rb->index, &acm->read_urbs_free); + } =20 /* throttle device if requested by tty */ spin_lock_irqsave(&acm->read_lock, flags); acm->throttled =3D acm->throttle_req; if (!acm->throttled) { spin_unlock_irqrestore(&acm->read_lock, flags); - acm_submit_read_urb(acm, rb->index, GFP_ATOMIC); + if ( may_resubmit ) { + acm_submit_read_urb(acm, rb->index, GFP_ATOMIC); + } } else { spin_unlock_irqrestore(&acm->read_lock, flags); } @@ -774,6 +849,8 @@ static void acm_tty_unthrottle(struct tty_struct *tty= ) acm->throttle_req =3D 0; spin_unlock_irq(&acm->read_lock); =20 + acm_push_leftover_data_to_tty(acm); + if (was_throttled) acm_submit_read_urbs(acm, GFP_KERNEL); } @@ -1367,6 +1444,8 @@ made_compressed_probe: struct acm_rb *rb =3D &(acm->read_buffers[i]); struct urb *urb; =20 + acm->remaining_data_in_read_urbs[i] =3D 0; + rb->base =3D usb_alloc_coherent(acm->dev, readsize, GFP_KERNEL, &rb->dma); if (!rb->base) diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-acm.h index ffeb3c8..0946b0a 100644 --- a/drivers/usb/class/cdc-acm.h +++ b/drivers/usb/class/cdc-acm.h @@ -93,6 +93,9 @@ struct acm { struct acm_wb wb[ACM_NW]; unsigned long read_urbs_free; struct urb *read_urbs[ACM_NR]; + int held_urbs[ACM_NR+1], num_held_urbs; /* fixed-size queue for not-yet= -processed read urbs */ + unsigned long remaining_data_in_read_urbs[ACM_NR]; /* amount of bytes i= n each held urb */ + spinlock_t resubmit_lock; struct acm_rb read_buffers[ACM_NR]; int rx_buflimit; int rx_endpoint; --=20 2.4.6 --iScfRKCNtMobO0luwosQm5Me9kHfTEwcI Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJVrBiEAAoJEGjKjjjEuz9LGZ0P+waOyOpwozoww/M4juF20TlL 9K0nzUwjMQENEiSW9e3nh7zltFLDQU03UNWh/E4PLJs3bYy/CdyEgDLij0bYngGB /GBALsX9tuzE8ofmedMvjF9AkZV7eV8DLRHB2uAZ92ShzjoiCm0mimxpwNibCa0d /dQu40z7nvJJPYBeyRf4VHStunR99YJBwpdm4/mFYBPPwpbtVv37wDxyomuJBmRY zhB6FfHBGAagMOfYGXhY8cU0TOpCmSNe6u4zBn2jXLh087KmiZcACXPJqTYPoOTh B4+uFzRhhS9AQ+1A0PTCUXxXIcbr90mSlfZpBXhwLposww5iyeWBgFowGYpdyNMf CUmxHy1EwWQfc/A5KYsVPxbex8iaGno6vB1nH9CQXJcF6Grg4lw5sqFpvjNGRTWR 8cMgowN6XMFms1RkrlGHHCrrv9qSFpcICKWPIFJh+lThKRUjbLHXb00xQq8UOfga a/gctVaF+YfAWQUB6NzoeXEJ9NCgyJWhRPptZkVw799EMponRXt/gdxEhXBGTQ4k qlZpY8NWNvgK0guD4pt2B11N4oufWIAA0qaPNjxjzP+MQEoXX5n9NGAS53IYcvVs auwLEPXhXHiOmVjetjudVyZVpOUdlQFzMHOo6pmOJpQT28tV8jEChmq0sa4BCmW+ If8Tr/mduMxUmBoj/ixU =/cW+ -----END PGP SIGNATURE----- --iScfRKCNtMobO0luwosQm5Me9kHfTEwcI-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/