[PATCH v2] USB: serial: cp210x: Add tx_empty()

[PATCH v2] USB: serial: cp210x: Add tx_empty()

[Konstantin Shkolnyy]

Added tx_empty callback needed for generic wait-until-sent support.
Without this function, when the port is closed usbserial can't know that
there are still data in the chip's transmit FIFO. The chip gets disabled
and untransmitted data lost. When the actual byte count is reported by
tx-empty the close can be delayed until all data are sent.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
Code style corrections and using sizeof instead of a defined constant.

 drivers/usb/serial/cp210x.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index e91b654..fd67958 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios *);
 static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *,
 							struct ktermios*);
+static bool cp210x_tx_empty(struct usb_serial_port *port);
 static int cp210x_tiocmget(struct tty_struct *);
 static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int);
 static int cp210x_tiocmset_port(struct usb_serial_port *port,
@@ -215,6 +216,7 @@ static struct usb_serial_driver cp210x_device = {
 	.close			= cp210x_close,
 	.break_ctl		= cp210x_break_ctl,
 	.set_termios		= cp210x_set_termios,
+	.tx_empty		= cp210x_tx_empty,
 	.tiocmget		= cp210x_tiocmget,
 	.tiocmset		= cp210x_tiocmset,
 	.port_probe		= cp210x_port_probe,
@@ -301,6 +303,17 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CONTROL_WRITE_DTR	0x0100
 #define CONTROL_WRITE_RTS	0x0200
 
+/* CP210X_GET_COMM_STATUS returns these 0x13 bytes */
+struct cp210x_comm_status {
+	__le32   ulErrors;
+	__le32   ulHoldReasons;
+	__le32   ulAmountInInQueue;
+	__le32   ulAmountInOutQueue;
+	u8       bEofReceived;
+	u8       bWaitForImmediate;
+	u8       bReserved;
+} __packed;
+
 /*
  * CP210X_PURGE - 16 bits passed in wValue of USB request.
  * SiLabs app note AN571 gives a strange description of the 4 bits:
@@ -551,6 +564,51 @@ static void cp210x_close(struct usb_serial_port *port)
 }
 
 /*
+ * Read how many bytes are waiting in the TX queue.
+ */
+static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port,
+		u32 *count)
+{
+	struct usb_serial *serial = port->serial;
+	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
+	struct cp210x_comm_status *sts;
+	int result;
+
+	sts = kmalloc(sizeof(*sts), GFP_KERNEL);
+	if (!sts)
+		return -ENOMEM;
+
+	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
+			CP210X_GET_COMM_STATUS, REQTYPE_INTERFACE_TO_HOST,
+			0, port_priv->bInterfaceNumber, sts, sizeof(*sts),
+			USB_CTRL_GET_TIMEOUT);
+	if (result == sizeof(*sts)) {
+		*count = le32_to_cpu(sts->ulAmountInOutQueue);
+		result = 0;
+	} else {
+		dev_err(&port->dev, "failed to get comm status: %d\n", result);
+		if (result >= 0)
+			result = -EPROTO;
+	}
+
+	kfree(sts);
+
+	return result;
+}
+
+static bool cp210x_tx_empty(struct usb_serial_port *port)
+{
+	int err;
+	u32 count;
+
+	err = cp210x_get_tx_queue_byte_count(port, &count);
+	if (!err && count)
+		return false;
+
+	return true;
+}
+
+/*
  * cp210x_get_termios
  * Reads the baud rate, data bits, parity, stop bits and flow control mode
  * from the device, corrects any unsupported values, and configures the
-- 
1.8.4.5

Re: [PATCH v2] USB: serial: cp210x: Add tx_empty()

[Andy Shevchenko]

On Wed, Nov 25, 2015 at 12:28 AM, Konstantin Shkolnyy
<konstantin.shkolnyy@gmail.com> wrote:
> Added tx_empty callback needed for generic wait-until-sent support.
> Without this function, when the port is closed usbserial can't know that
> there are still data in the chip's transmit FIFO. The chip gets disabled
> and untransmitted data lost. When the actual byte count is reported by
> tx-empty the close can be delayed until all data are sent.
>

>  /*
> + * Read how many bytes are waiting in the TX queue.
> + */

One line?

> +static bool cp210x_tx_empty(struct usb_serial_port *port)
> +{
> +       int err;
> +       u32 count;
> +
> +       err = cp210x_get_tx_queue_byte_count(port, &count);

> +       if (!err && count)
> +               return false;
> +
> +       return true;

return err || !count;

Btw, can be count left uninitialized?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] USB: serial: cp210x: Add tx_empty()

[Johan Hovold]

On Wed, Nov 25, 2015 at 10:26:12PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 25, 2015 at 12:28 AM, Konstantin Shkolnyy
> <konstantin.shkolnyy@gmail.com> wrote:
> > Added tx_empty callback needed for generic wait-until-sent support.
> > Without this function, when the port is closed usbserial can't know that
> > there are still data in the chip's transmit FIFO. The chip gets disabled
> > and untransmitted data lost. When the actual byte count is reported by
> > tx-empty the close can be delayed until all data are sent.
> >
> 
> >  /*
> > + * Read how many bytes are waiting in the TX queue.
> > + */
> 
> One line?

Not necessarily for a function header.

> > +static bool cp210x_tx_empty(struct usb_serial_port *port)
> > +{
> > +       int err;
> > +       u32 count;
> > +
> > +       err = cp210x_get_tx_queue_byte_count(port, &count);
> 
> > +       if (!err && count)
> > +               return false;
> > +
> > +       return true;
> 
> return err || !count;

Nah, I don't consider that an improvement.

I prefer separating the error and success paths, for example:

	if (err)
		return true;

	return !count;

but the current code is also ok.

> Btw, can be count left uninitialized?

Yes, but that's not an issue. Clearly separating the success and errors
paths as I suggested above would make that more obvious however.

I'll fix that up before applying.

Thanks,
Johan

Re: [PATCH v2] USB: serial: cp210x: Add tx_empty()

[Johan Hovold]

On Thu, Nov 26, 2015 at 08:35:12AM +0100, Johan Hovold wrote:
> On Wed, Nov 25, 2015 at 10:26:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 25, 2015 at 12:28 AM, Konstantin Shkolnyy
> > <konstantin.shkolnyy@gmail.com> wrote:
> > > Added tx_empty callback needed for generic wait-until-sent support.
> > > Without this function, when the port is closed usbserial can't know that
> > > there are still data in the chip's transmit FIFO. The chip gets disabled
> > > and untransmitted data lost. When the actual byte count is reported by
> > > tx-empty the close can be delayed until all data are sent.

> > Btw, can be count left uninitialized?
> 
> Yes, but that's not an issue. Clearly separating the success and errors
> paths as I suggested above would make that more obvious however.
> 
> I'll fix that up before applying.

Now applied, thanks.

Johan

Re: [PATCH v2] USB: serial: cp210x: Add tx_empty()

[Konstantin Shkolnyy]

On Wed, Nov 25, 2015 at 2:26 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Nov 25, 2015 at 12:28 AM, Konstantin Shkolnyy
> <konstantin.shkolnyy@gmail.com> wrote:

>> +static bool cp210x_tx_empty(struct usb_serial_port *port)
>> +{
>> +       int err;
>> +       u32 count;
>> +
>> +       err = cp210x_get_tx_queue_byte_count(port, &count);
>
>> +       if (!err && count)
>> +               return false;
>> +
>> +       return true;
>
> return err || !count;

To me, expressing it like this is harder to read. The code as I wrote
it, in my mind, reads like this:

if successfully read the counter and it's not 0 then
   tx is not empty, return false

> Btw, can be count left uninitialized?

When the function succeeds it assigns a value to count.