[PATCH] mos7840: fix chars_in_buffer() return value

[PATCH] mos7840: fix chars_in_buffer() return value

[Stas Sergeev]

The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
a correct behaviour of that.
mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
will return 32.
This patch should fix it by accounting the number of chars actually
written.
This patch, unfortunately, misses the Tested-by tag.
The reporter didn't test it, and I don't have the hardware in question.

Signed-off-by: Stas Sergeev <stsp@list.ru>
Reported-by: Caylan Van Larson <i@caylan.net>
CC: Caylan Van Larson <i@caylan.net>
CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
CC: Johan Hovold <johan@kernel.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: linux-usb@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: sergei.shtylyov@cogentembedded.com
---
 drivers/usb/serial/mos7840.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index ed378fb..2a1eb07 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -234,7 +234,7 @@ struct moschip_port {
 
 	spinlock_t pool_lock;
 	struct urb *write_urb_pool[NUM_URBS];
-	char busy[NUM_URBS];
+	int busy[NUM_URBS];
 	bool read_urb_busy;
 
 	/* For device(s) with LED indicator */
@@ -1139,8 +1139,7 @@ static int mos7840_chars_in_buffer(struct tty_struct *tty)
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (mos7840_port->busy[i]) {
-			struct urb *urb = mos7840_port->write_urb_pool[i];
-			chars += urb->transfer_buffer_length;
+			chars += mos7840_port->busy[i];
 		}
 	}
 	spin_unlock_irqrestore(&mos7840_port->pool_lock, flags);
@@ -1323,10 +1322,11 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 	/* try to find a free urb in the list */
 	urb = NULL;
 
+	transfer_size = min(count, URB_TRANSFER_BUFFER_SIZE);
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (!mos7840_port->busy[i]) {
-			mos7840_port->busy[i] = 1;
+			mos7840_port->busy[i] = transfer_size;
 			urb = mos7840_port->write_urb_pool[i];
 			dev_dbg(&port->dev, "URB:%d\n", i);
 			break;
@@ -1345,7 +1345,6 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 		if (!urb->transfer_buffer)
 			goto exit;
 	}
-	transfer_size = min(count, URB_TRANSFER_BUFFER_SIZE);
 
 	memcpy(urb->transfer_buffer, current_position, transfer_size);
 
-- 
2.7.4

Re: [PATCH] mos7840: fix chars_in_buffer() return value

[Johan Hovold]

On Sat, Sep 24, 2016 at 06:00:57PM +0300, Stas Sergeev wrote:
> The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
> a correct behaviour of that.
> mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
> will return 32.
> This patch should fix it by accounting the number of chars actually
> written.
> This patch, unfortunately, misses the Tested-by tag.
> The reporter didn't test it, and I don't have the hardware in question.

Why do you think the driver returns 32b in chars_in_buffer after writing
a single character?

As far as I can see, the driver correctly sums up the outstanding bytes
in its queue (represented by the submitted URBs). There is a tiny window
where the count may be a little off due to the write() implementation
claiming the URB before initialising it, but I'm not sure anyone cares.

But specifically, if you write just one character, TIOCOUTQ does indeed
return a correct count.

Also note that your patch would break the driver in case a line
discipline forwards an empty write request.

Thanks,
Johan

Re: [PATCH] mos7840: fix chars_in_buffer() return value

[Stas Sergeev]

29.09.2016 13:09, Johan Hovold пишет:
> On Sat, Sep 24, 2016 at 06:00:57PM +0300, Stas Sergeev wrote:
>> The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
>> a correct behaviour of that.
>> mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
>> will return 32.
>> This patch should fix it by accounting the number of chars actually
>> written.
>> This patch, unfortunately, misses the Tested-by tag.
>> The reporter didn't test it, and I don't have the hardware in question.
> Why do you think the driver returns 32b in chars_in_buffer after writing
> a single character?
Hi Johan, this actually came from this ancient bug report:
https://bugzilla.kernel.org/show_bug.cgi?id=45791
I was trying to add you back then and now, but your e-mail
doesn't seem to be registered in a bug tracker.
The code in question was different when I submitted the
original patch:
https://bugzilla.kernel.org/attachment.cgi?id=77241
Maybe the bug was since fixed.
In that case sorry for the noise.

Re: [PATCH] mos7840: fix chars_in_buffer() return value

[Johan Hovold]

On Fri, Sep 30, 2016 at 12:00:30AM +0300, Stas Sergeev wrote:
> 29.09.2016 13:09, Johan Hovold пишет:
> > On Sat, Sep 24, 2016 at 06:00:57PM +0300, Stas Sergeev wrote:
> >> The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
> >> a correct behaviour of that.
> >> mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
> >> will return 32.
> >> This patch should fix it by accounting the number of chars actually
> >> written.
> >> This patch, unfortunately, misses the Tested-by tag.
> >> The reporter didn't test it, and I don't have the hardware in question.

> > Why do you think the driver returns 32b in chars_in_buffer after writing
> > a single character?

> Hi Johan, this actually came from this ancient bug report:
> https://bugzilla.kernel.org/show_bug.cgi?id=45791
> I was trying to add you back then and now, but your e-mail
> doesn't seem to be registered in a bug tracker.

Yeah, Greg does good job at direction bug reports to the usb mailing
list.

> The code in question was different when I submitted the
> original patch:
> https://bugzilla.kernel.org/attachment.cgi?id=77241
> Maybe the bug was since fixed.

It does seem to have been addressed by commit 5c263b92f828 ("usb:
serial: mos7840: Fixup mos7840_chars_in_buffer()") around the time of
your original report in 2012.

> In that case sorry for the noise.

No worries. 

Thanks,
Johan

Re: [PATCH] mos7840: fix chars_in_buffer() return value

[Stas Sergeev]

24.09.2016 16:57, Sergei Shtylyov пишет:
> Hello.
>
> On 9/24/2016 4:47 PM, Stas Sergeev wrote:
>
>> The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
>> a correct behaviour of that.
>> mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
>> will return 32.
>> This patch should fix it by accounting the number of chars actually
>> writCaylan Van Larson <i@caylan.net>ten.
>
>    ???
>
>> This patch, unfortunately, misses the Tesed-by tag.
>
>    Tested-by.
>
>> The reported didn't test it, and I don't have the hardware in question.
>
>    Reporter? 
:)
Thanks, will re-send with corrections.

Re: [PATCH] mos7840: fix chars_in_buffer() return value

[Sergei Shtylyov]

Hello.

On 9/24/2016 4:47 PM, Stas Sergeev wrote:

> The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
> a correct behaviour of that.
> mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
> will return 32.
> This patch should fix it by accounting the number of chars actually
> writCaylan Van Larson <i@caylan.net>ten.

    ???

> This patch, unfortunately, misses the Tesed-by tag.

    Tested-by.

> The reported didn't test it, and I don't have the hardware in question.

    Reporter?

> Signed-off-by: Stas Sergeev <stsp@list.ru>
> Reported-by: Caylan Van Larson <i@caylan.net>
> CC: Caylan Van Larson <i@caylan.net>
> CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
> CC: Johan Hovold <johan@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> CC: linux-usb@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
[...]

MBR, Sergei

[PATCH] mos7840: fix chars_in_buffer() return value

[Stas Sergeev]

The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
a correct behaviour of that.
mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
will return 32.
This patch should fix it by accounting the number of chars actually
written.
This patch, unfortunately, misses the Tesed-by tag.
The reported didn't test it, and I don't have the hardware in question.

Signed-off-by: Stas Sergeev <stsp@list.ru>
Reported-by: Caylan Van Larson <i@caylan.net>
CC: Caylan Van Larson <i@caylan.net>
CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
CC: Johan Hovold <johan@kernel.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: linux-usb@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/usb/serial/mos7840.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index ed378fb..2a1eb07 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -234,7 +234,7 @@ struct moschip_port {
 
 	spinlock_t pool_lock;
 	struct urb *write_urb_pool[NUM_URBS];
-	char busy[NUM_URBS];
+	int busy[NUM_URBS];
 	bool read_urb_busy;
 
 	/* For device(s) with LED indicator */
@@ -1139,8 +1139,7 @@ static int mos7840_chars_in_buffer(struct tty_struct *tty)
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (mos7840_port->busy[i]) {
-			struct urb *urb = mos7840_port->write_urb_pool[i];
-			chars += urb->transfer_buffer_length;
+			chars += mos7840_port->busy[i];
 		}
 	}
 	spin_unlock_irqrestore(&mos7840_port->pool_lock, flags);
@@ -1323,10 +1322,11 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 	/* try to find a free urb in the list */
 	urb = NULL;
 
+	transfer_size = min(count, URB_TRANSFER_BUFFER_SIZE);
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (!mos7840_port->busy[i]) {
-			mos7840_port->busy[i] = 1;
+			mos7840_port->busy[i] = transfer_size;
 			urb = mos7840_port->write_urb_pool[i];
 			dev_dbg(&port->dev, "URB:%d\n", i);
 			break;
@@ -1345,7 +1345,6 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 		if (!urb->transfer_buffer)
 			goto exit;
 	}
-	transfer_size = min(count, URB_TRANSFER_BUFFER_SIZE);
 
 	memcpy(urb->transfer_buffer, current_position, transfer_size);
 
-- 
2.7.4

[PATCH] mos7840: fix chars_in_buffer() return value

[Stas Sergeev]

The TIOCOUTQ ioctl calls chars_in_buffer(), and some apps depend on
a correct behaviour of that.
mos7840 implements it wrongly: if you write just one char, TIOCOUTQ
will return 32.
This patch should fix it by accounting the number of chars actually
writCaylan Van Larson <i@caylan.net>ten.
This patch, unfortunately, misses the Tesed-by tag.
The reported didn't test it, and I don't have the hardware in question.

Signed-off-by: Stas Sergeev <stsp@list.ru>
Reported-by: Caylan Van Larson <i@caylan.net>
CC: Caylan Van Larson <i@caylan.net>
CC: Alan Cox <alan@lxorguk.ukuu.org.uk>
CC: Johan Hovold <johan@kernel.org>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC: linux-usb@vger.kernel.org
CC: linux-kernel@vger.kernel.org
---
 drivers/usb/serial/mos7840.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index ed378fb..2a1eb07 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -234,7 +234,7 @@ struct moschip_port {
 
 	spinlock_t pool_lock;
 	struct urb *write_urb_pool[NUM_URBS];
-	char busy[NUM_URBS];
+	int busy[NUM_URBS];
 	bool read_urb_busy;
 
 	/* For device(s) with LED indicator */
@@ -1139,8 +1139,7 @@ static int mos7840_chars_in_buffer(struct tty_struct *tty)
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (mos7840_port->busy[i]) {
-			struct urb *urb = mos7840_port->write_urb_pool[i];
-			chars += urb->transfer_buffer_length;
+			chars += mos7840_port->busy[i];
 		}
 	}
 	spin_unlock_irqrestore(&mos7840_port->pool_lock, flags);
@@ -1323,10 +1322,11 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 	/* try to find a free urb in the list */
 	urb = NULL;
 
+	transfer_size = min(count, URB_TRANSFER_BUFFER_SIZE);
 	spin_lock_irqsave(&mos7840_port->pool_lock, flags);
 	for (i = 0; i < NUM_URBS; ++i) {
 		if (!mos7840_port->busy[i]) {
-			mos7840_port->busy[i] = 1;
+			mos7840_port->busy[i] = transfer_size;
 			urb = mos7840_port->write_urb_pool[i];
 			dev_dbg(&port->dev, "URB:%d\n", i);
 			break;
@@ -1345,7 +1345,6 @@ static int mos7840_write(struct tty_struct *tty, struct usb_serial_port *port,
 		if (!urb->transfer_buffer)
 			goto exit;
 	}
-	transfer_size = min(count, URB_TRANSFER_BUFFER_SIZE);
 
 	memcpy(urb->transfer_buffer, current_position, transfer_size);
 
-- 
2.7.4