[PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Konstantin Shkolnyy]

Existing register access functions cp210x_get_config and cp210x_set_config
are cumbersome to use. This change introduces new functions specifically
for 16-bit registers that read and write simple u16 values.

Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
---
 drivers/usb/serial/cp210x.c | 119 ++++++++++++++++++++++++++++++++------------
 1 file changed, 88 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index eac7cca..5a7c15e 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -301,6 +301,77 @@ static struct usb_serial_driver * const serial_drivers[] = {
 #define CONTROL_WRITE_RTS	0x0200
 
 /*
+ * Reads any 16-bit CP210X_ register (req).
+ */
+static int cp210x_read_u16_reg(struct usb_serial *serial, u8 req, u16 *pval)
+{
+	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
+	__le16 le16_value;
+	int result;
+
+	result = usb_control_msg(serial->dev,
+				usb_rcvctrlpipe(serial->dev, 0),
+				req, REQTYPE_INTERFACE_TO_HOST, 0,
+				spriv->bInterfaceNumber, &le16_value, 2,
+				USB_CTRL_SET_TIMEOUT);
+	if (result != 2) {
+		if (result > 0)
+			result = -EPROTO;
+		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
+			__func__, spriv->bInterfaceNumber, req, result);
+		return result;
+	}
+	*pval = le16_to_cpu(le16_value);
+	return 0;
+}
+
+/*
+ * Writes any 16-bit CP210X_ register (req) whose value is passed
+ * entirely in the wValue field of the USB request.
+ */
+static int cp210x_write_u16_reg(struct usb_serial *serial, u8 req, u16 val)
+{
+	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
+	int result;
+
+	result = usb_control_msg(serial->dev,
+				usb_sndctrlpipe(serial->dev, 0),
+				req, REQTYPE_HOST_TO_INTERFACE, val,
+				spriv->bInterfaceNumber, NULL, 0,
+				USB_CTRL_SET_TIMEOUT);
+	if (result < 0) {
+		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
+			__func__, spriv->bInterfaceNumber, req, result);
+	}
+	return result;
+}
+
+/*
+ * Command-specific wrappers around USB access functions
+ */
+static int cp210x_ifc_enable(struct usb_serial_port *port)
+{
+	return cp210x_write_u16_reg(port->serial,
+			CP210X_IFC_ENABLE, UART_ENABLE);
+}
+
+static int cp210x_ifc_disable(struct usb_serial_port *port)
+{
+	return cp210x_write_u16_reg(port->serial,
+			CP210X_IFC_ENABLE, UART_DISABLE);
+}
+
+static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *pctrl)
+{
+	return cp210x_read_u16_reg(port->serial, CP210X_GET_LINE_CTL, pctrl);
+}
+
+static int cp210x_set_line_ctl(struct usb_serial_port *port, u16 ctrl)
+{
+	return cp210x_write_u16_reg(port->serial, CP210X_SET_LINE_CTL, ctrl);
+}
+
+/*
  * cp210x_get_config
  * Reads from the CP210x configuration registers
  * 'size' is specified in bytes.
@@ -400,17 +471,6 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
 }
 
 /*
- * cp210x_set_config_single
- * Convenience function for calling cp210x_set_config on single data values
- * without requiring an integer pointer
- */
-static inline int cp210x_set_config_single(struct usb_serial_port *port,
-		u8 request, unsigned int data)
-{
-	return cp210x_set_config(port, request, &data, 2);
-}
-
-/*
  * cp210x_quantise_baudrate
  * Quantises the baud rate as per AN205 Table 1
  */
@@ -456,12 +516,9 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 {
 	int result;
 
-	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
-								UART_ENABLE);
-	if (result) {
-		dev_err(&port->dev, "%s - Unable to enable UART\n", __func__);
+	result = cp210x_ifc_enable(port);
+	if (result)
 		return result;
-	}
 
 	/* Configure the termios structure */
 	cp210x_get_termios(tty, port);
@@ -476,7 +533,7 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 static void cp210x_close(struct usb_serial_port *port)
 {
 	usb_serial_generic_close(port);
-	cp210x_set_config_single(port, CP210X_IFC_ENABLE, UART_DISABLE);
+	cp210x_ifc_disable(port);
 }
 
 /*
@@ -511,7 +568,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	struct device *dev = &port->dev;
 	unsigned int cflag, modem_ctl[4];
 	unsigned int baud;
-	unsigned int bits;
+	u16          bits;
 
 	cp210x_get_config(port, CP210X_GET_BAUDRATE, &baud, 4);
 
@@ -520,7 +577,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 
 	cflag = *cflagp;
 
-	cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+	cp210x_get_line_ctl(port, &bits);
 	cflag &= ~CSIZE;
 	switch (bits & BITS_DATA_MASK) {
 	case BITS_DATA_5:
@@ -544,14 +601,14 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		cflag |= CS8;
 		bits &= ~BITS_DATA_MASK;
 		bits |= BITS_DATA_8;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_line_ctl(port, bits);
 		break;
 	default:
 		dev_dbg(dev, "%s - Unknown number of data bits, using 8\n", __func__);
 		cflag |= CS8;
 		bits &= ~BITS_DATA_MASK;
 		bits |= BITS_DATA_8;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_line_ctl(port, bits);
 		break;
 	}
 
@@ -582,7 +639,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 		dev_dbg(dev, "%s - Unknown parity mode, disabling parity\n", __func__);
 		cflag &= ~PARENB;
 		bits &= ~BITS_PARITY_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_line_ctl(port, bits);
 		break;
 	}
 
@@ -594,7 +651,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	case BITS_STOP_1_5:
 		dev_dbg(dev, "%s - stop bits = 1.5 (not supported, using 1 stop bit)\n", __func__);
 		bits &= ~BITS_STOP_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_line_ctl(port, bits);
 		break;
 	case BITS_STOP_2:
 		dev_dbg(dev, "%s - stop bits = 2\n", __func__);
@@ -603,7 +660,7 @@ static void cp210x_get_termios_port(struct usb_serial_port *port,
 	default:
 		dev_dbg(dev, "%s - Unknown number of stop bits, using 1 stop bit\n", __func__);
 		bits &= ~BITS_STOP_MASK;
-		cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2);
+		cp210x_set_line_ctl(port, bits);
 		break;
 	}
 
@@ -677,7 +734,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 {
 	struct device *dev = &port->dev;
 	unsigned int cflag, old_cflag;
-	unsigned int bits;
+	u16          bits;
 	unsigned int modem_ctl[4];
 
 	cflag = tty->termios.c_cflag;
@@ -688,7 +745,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 
 	/* If the number of data bits is to be updated */
 	if ((cflag & CSIZE) != (old_cflag & CSIZE)) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_line_ctl(port, &bits);
 		bits &= ~BITS_DATA_MASK;
 		switch (cflag & CSIZE) {
 		case CS5:
@@ -716,13 +773,13 @@ static void cp210x_set_termios(struct tty_struct *tty,
 			bits |= BITS_DATA_8;
 			break;
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_set_line_ctl(port, bits))
 			dev_dbg(dev, "Number of data bits requested not supported by device\n");
 	}
 
 	if ((cflag     & (PARENB|PARODD|CMSPAR)) !=
 	    (old_cflag & (PARENB|PARODD|CMSPAR))) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_line_ctl(port, &bits);
 		bits &= ~BITS_PARITY_MASK;
 		if (cflag & PARENB) {
 			if (cflag & CMSPAR) {
@@ -743,12 +800,12 @@ static void cp210x_set_termios(struct tty_struct *tty,
 				}
 			}
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_set_line_ctl(port, bits))
 			dev_dbg(dev, "Parity mode not supported by device\n");
 	}
 
 	if ((cflag & CSTOPB) != (old_cflag & CSTOPB)) {
-		cp210x_get_config(port, CP210X_GET_LINE_CTL, &bits, 2);
+		cp210x_get_line_ctl(port, &bits);
 		bits &= ~BITS_STOP_MASK;
 		if (cflag & CSTOPB) {
 			bits |= BITS_STOP_2;
@@ -757,7 +814,7 @@ static void cp210x_set_termios(struct tty_struct *tty,
 			bits |= BITS_STOP_1;
 			dev_dbg(dev, "%s - stop bits = 1\n", __func__);
 		}
-		if (cp210x_set_config(port, CP210X_SET_LINE_CTL, &bits, 2))
+		if (cp210x_set_line_ctl(port, bits))
 			dev_dbg(dev, "Number of stop bits requested not supported by device\n");
 	}
 
-- 
1.8.4.5

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Oliver Neukum]

On Mon, 2015-10-19 at 17:01 -0500, Konstantin Shkolnyy wrote:
> Existing register access functions cp210x_get_config and cp210x_set_config
> are cumbersome to use. This change introduces new functions specifically
> for 16-bit registers that read and write simple u16 values.

Hi,

I am afraid there are some issues.

> 
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 119 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 88 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..5a7c15e 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -301,6 +301,77 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CONTROL_WRITE_RTS	0x0200
>  
>  /*
> + * Reads any 16-bit CP210X_ register (req).
> + */
> +static int cp210x_read_u16_reg(struct usb_serial *serial, u8 req, u16 *pval)
> +{
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +	__le16 le16_value;

1. If you already get passed a pointer to a buffer, why use another
buffer?
2. You are doing DMA on the stack. That is forbidden.

> +	int result;
> +
> +	result = usb_control_msg(serial->dev,
> +				usb_rcvctrlpipe(serial->dev, 0),
> +				req, REQTYPE_INTERFACE_TO_HOST, 0,
> +				spriv->bInterfaceNumber, &le16_value, 2,
> +				USB_CTRL_SET_TIMEOUT);
> +	if (result != 2) {
> +		if (result > 0)
> +			result = -EPROTO;
> +		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
> +			__func__, spriv->bInterfaceNumber, req, result);

It would make more sense inverting the debug statement and the error
conversion, as you wouldn't lose information.

> +		return result;
> +	}
> +	*pval = le16_to_cpu(le16_value);
> +	return 0;
> +}

	Regards
		Oliver

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Johan Hovold]

On Mon, Oct 19, 2015 at 05:01:24PM -0500, Konstantin Shkolnyy wrote:
> Existing register access functions cp210x_get_config and cp210x_set_config
> are cumbersome to use. This change introduces new functions specifically
> for 16-bit registers that read and write simple u16 values.

There's a bit too much going on in both these patches. Remember, one
logical change per patch.

Also make sure to include the patch revision in the subject and bump it
(for the whole series) when resending.

> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 119 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 88 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..5a7c15e 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -301,6 +301,77 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CONTROL_WRITE_RTS	0x0200
>  
>  /*
> + * Reads any 16-bit CP210X_ register (req).
> + */
> +static int cp210x_read_u16_reg(struct usb_serial *serial, u8 req, u16 *pval)
> +{
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +	__le16 le16_value;
> +	int result;
> +
> +	result = usb_control_msg(serial->dev,
> +				usb_rcvctrlpipe(serial->dev, 0),
> +				req, REQTYPE_INTERFACE_TO_HOST, 0,
> +				spriv->bInterfaceNumber, &le16_value, 2,
> +				USB_CTRL_SET_TIMEOUT);

As Oliver already pointed out, you cannot use an automatic variable for
the data stage here.

> +	if (result != 2) {
> +		if (result > 0)
> +			result = -EPROTO;
> +		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
> +			__func__, spriv->bInterfaceNumber, req, result);
> +		return result;
> +	}
> +	*pval = le16_to_cpu(le16_value);
> +	return 0;
> +}
> +
> +/*
> + * Writes any 16-bit CP210X_ register (req) whose value is passed
> + * entirely in the wValue field of the USB request.
> + */
> +static int cp210x_write_u16_reg(struct usb_serial *serial, u8 req, u16 val)
> +{
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +	int result;
> +
> +	result = usb_control_msg(serial->dev,
> +				usb_sndctrlpipe(serial->dev, 0),
> +				req, REQTYPE_HOST_TO_INTERFACE, val,
> +				spriv->bInterfaceNumber, NULL, 0,
> +				USB_CTRL_SET_TIMEOUT);
> +	if (result < 0) {
> +		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
> +			__func__, spriv->bInterfaceNumber, req, result);
> +	}
> +	return result;
> +}
> +
> +/*
> + * Command-specific wrappers around USB access functions
> + */
> +static int cp210x_ifc_enable(struct usb_serial_port *port)
> +{
> +	return cp210x_write_u16_reg(port->serial,
> +			CP210X_IFC_ENABLE, UART_ENABLE);
> +}
> +
> +static int cp210x_ifc_disable(struct usb_serial_port *port)
> +{
> +	return cp210x_write_u16_reg(port->serial,
> +			CP210X_IFC_ENABLE, UART_DISABLE);
> +}

Why are these here? This is unrelated to adding the line_ctl helpers.

> +
> +static int cp210x_get_line_ctl(struct usb_serial_port *port, u16 *pctrl)
> +{
> +	return cp210x_read_u16_reg(port->serial, CP210X_GET_LINE_CTL, pctrl);
> +}
> +
> +static int cp210x_set_line_ctl(struct usb_serial_port *port, u16 ctrl)
> +{
> +	return cp210x_write_u16_reg(port->serial, CP210X_SET_LINE_CTL, ctrl);
> +}

Instead of adding the new helpers to read u16 as a prerequisite for
fixing the broken cp2108 support, just reuse the current config register
helpers for now (in order to keep the fixes minimal and potentially
backportable). Once the fixes are in place, feel free to clean up the
remaining register accesses.

> +
> +/*
>   * cp210x_get_config
>   * Reads from the CP210x configuration registers
>   * 'size' is specified in bytes.
> @@ -400,17 +471,6 @@ static int cp210x_set_config(struct usb_serial_port *port, u8 request,
>  }
>  
>  /*
> - * cp210x_set_config_single
> - * Convenience function for calling cp210x_set_config on single data values
> - * without requiring an integer pointer
> - */
> -static inline int cp210x_set_config_single(struct usb_serial_port *port,
> -		u8 request, unsigned int data)
> -{
> -	return cp210x_set_config(port, request, &data, 2);
> -}
> -
> -/*
>   * cp210x_quantise_baudrate
>   * Quantises the baud rate as per AN205 Table 1
>   */
> @@ -456,12 +516,9 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
>  {
>  	int result;
>  
> -	result = cp210x_set_config_single(port, CP210X_IFC_ENABLE,
> -								UART_ENABLE);
> -	if (result) {
> -		dev_err(&port->dev, "%s - Unable to enable UART\n", __func__);
> +	result = cp210x_ifc_enable(port);
> +	if (result)
>  		return result;
> -	}

So this is unrelated, and should be dropped from this patch.

Johan

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Konstantin Shkolnyy]

On Tue, Oct 20, 2015 at 1:45 AM, Oliver Neukum <oneukum@suse.com> wrote:
> On Mon, 2015-10-19 at 17:01 -0500, Konstantin Shkolnyy wrote:
[...]
>> +static int cp210x_read_u16_reg(struct usb_serial *serial, u8 req, u16 *pval)
>> +{
>> +     struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
>> +     __le16 le16_value;
>
> 1. If you already get passed a pointer to a buffer, why use another
> buffer?

Because I want to preserve the caller's value in the case of an error.

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Konstantin Shkolnyy]

On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
[...]
> Instead of adding the new helpers to read u16 as a prerequisite for
> fixing the broken cp2108 support, just reuse the current config register
> helpers for now (in order to keep the fixes minimal and potentially
> backportable). Once the fixes are in place, feel free to clean up the
> remaining register accesses.

The current helpers take "port" as a parameter. You pointed out
previously that port shouldn't be used in probe(). That made me
implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
that don't use port. Probe() now calls cp210x_activate_workarounds
which in turn calls these new helpers.

An alternative would be to call usb_control_msg from
cp210x_activate_workarounds, but I think it would make it look pretty
ugly.

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Johan Hovold]

On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
> [...]
> > Instead of adding the new helpers to read u16 as a prerequisite for
> > fixing the broken cp2108 support, just reuse the current config register
> > helpers for now (in order to keep the fixes minimal and potentially
> > backportable). Once the fixes are in place, feel free to clean up the
> > remaining register accesses.
> 
> The current helpers take "port" as a parameter. You pointed out
> previously that port shouldn't be used in probe(). That made me
> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
> that don't use port. Probe() now calls cp210x_activate_workarounds
> which in turn calls these new helpers.

Oh, that's right.

> An alternative would be to call usb_control_msg from
> cp210x_activate_workarounds, but I think it would make it look pretty
> ugly.

Or you move the quirk-detect (and private data allocation) to port_probe
instead (and remove startup/release). These devices have exactly one
port per interface, so you wouldn't introduce any redundancy.

Johan

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Konstantin Shkolnyy]

On Tue, Oct 20, 2015 at 8:02 AM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
>> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
>> [...]
>> > Instead of adding the new helpers to read u16 as a prerequisite for
>> > fixing the broken cp2108 support, just reuse the current config register
>> > helpers for now (in order to keep the fixes minimal and potentially
>> > backportable). Once the fixes are in place, feel free to clean up the
>> > remaining register accesses.
>>
>> The current helpers take "port" as a parameter. You pointed out
>> previously that port shouldn't be used in probe(). That made me
>> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
>> that don't use port. Probe() now calls cp210x_activate_workarounds
>> which in turn calls these new helpers.
>
> Oh, that's right.
>
>> An alternative would be to call usb_control_msg from
>> cp210x_activate_workarounds, but I think it would make it look pretty
>> ugly.
>
> Or you move the quirk-detect (and private data allocation) to port_probe
> instead (and remove startup/release). These devices have exactly one
> port per interface, so you wouldn't introduce any redundancy.

OK, how about this:
patch #1
- attach/release replaced with port_probe/port_release;
- changes in the code using the "private" data since it has moved from
serial to port;
- cp210x_activate_workarounds in the port_probe, using the current helpers;
- a new helper cp210x_get_line_ctl that calls the current helper and
swaps the bytes.
patch #2
- sending "purge" from close, using the current helper.

However, the Tx queue bug fixed by the "purge" doesn't have a
detection method. We only know that it exists in the same cp2108 that
has the "byte swap" bug. So the "purge" is activated by the same "byte
swap" flag and  patch #2 depends on patch #1. Also, there is no reason
to only apply one of the patches. Do you think we can merge these
patches?

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Johan Hovold]

On Tue, Oct 20, 2015 at 09:19:05AM -0500, Konstantin Shkolnyy wrote:
> On Tue, Oct 20, 2015 at 8:02 AM, Johan Hovold <johan@kernel.org> wrote:
> > On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
> >> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
> >> [...]
> >> > Instead of adding the new helpers to read u16 as a prerequisite for
> >> > fixing the broken cp2108 support, just reuse the current config register
> >> > helpers for now (in order to keep the fixes minimal and potentially
> >> > backportable). Once the fixes are in place, feel free to clean up the
> >> > remaining register accesses.
> >>
> >> The current helpers take "port" as a parameter. You pointed out
> >> previously that port shouldn't be used in probe(). That made me
> >> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
> >> that don't use port. Probe() now calls cp210x_activate_workarounds
> >> which in turn calls these new helpers.
> >
> > Oh, that's right.
> >
> >> An alternative would be to call usb_control_msg from
> >> cp210x_activate_workarounds, but I think it would make it look pretty
> >> ugly.
> >
> > Or you move the quirk-detect (and private data allocation) to port_probe
> > instead (and remove startup/release). These devices have exactly one
> > port per interface, so you wouldn't introduce any redundancy.
> 
> OK, how about this:
> patch #1
> - attach/release replaced with port_probe/port_release;
> - changes in the code using the "private" data since it has moved from
> serial to port;
> - cp210x_activate_workarounds in the port_probe, using the current helpers;
> - a new helper cp210x_get_line_ctl that calls the current helper and
> swaps the bytes.
> patch #2
> - sending "purge" from close, using the current helper.
> 
> However, the Tx queue bug fixed by the "purge" doesn't have a
> detection method. We only know that it exists in the same cp2108 that
> has the "byte swap" bug. So the "purge" is activated by the same "byte
> swap" flag and  patch #2 depends on patch #1.

So you only see the hang after close with data in fifo on cp2108? Is
this something that might get fixed as well in later revisions?

But doing a purge as part of close should not be a problem on the other
products, right? I think doing this unconditionally would be best (just
like you eventually add tx_empty for all devices). No need to depend on
the broken-lcr-quirk.

> Also, there is no reason
> to only apply one of the patches. Do you think we can merge these
> patches?

No, again you want one logical change per patch.

I suggest you split it as follows

 - fix hang with non-empty tx fifo
 - use port data for private data
 - fix broken cp2108 lcr

Thanks,
Johan

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Konstantin Shkolnyy]

On Tue, Oct 20, 2015 at 11:22 AM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Oct 20, 2015 at 09:19:05AM -0500, Konstantin Shkolnyy wrote:
>> On Tue, Oct 20, 2015 at 8:02 AM, Johan Hovold <johan@kernel.org> wrote:
>> > On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
>> >> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
>> >> [...]
>> >> > Instead of adding the new helpers to read u16 as a prerequisite for
>> >> > fixing the broken cp2108 support, just reuse the current config register
>> >> > helpers for now (in order to keep the fixes minimal and potentially
>> >> > backportable). Once the fixes are in place, feel free to clean up the
>> >> > remaining register accesses.
>> >>
>> >> The current helpers take "port" as a parameter. You pointed out
>> >> previously that port shouldn't be used in probe(). That made me
>> >> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
>> >> that don't use port. Probe() now calls cp210x_activate_workarounds
>> >> which in turn calls these new helpers.
>> >
>> > Oh, that's right.
>> >
>> >> An alternative would be to call usb_control_msg from
>> >> cp210x_activate_workarounds, but I think it would make it look pretty
>> >> ugly.
>> >
>> > Or you move the quirk-detect (and private data allocation) to port_probe
>> > instead (and remove startup/release). These devices have exactly one
>> > port per interface, so you wouldn't introduce any redundancy.
>>
>> OK, how about this:
>> patch #1
>> - attach/release replaced with port_probe/port_release;
>> - changes in the code using the "private" data since it has moved from
>> serial to port;
>> - cp210x_activate_workarounds in the port_probe, using the current helpers;
>> - a new helper cp210x_get_line_ctl that calls the current helper and
>> swaps the bytes.
>> patch #2
>> - sending "purge" from close, using the current helper.
>>
>> However, the Tx queue bug fixed by the "purge" doesn't have a
>> detection method. We only know that it exists in the same cp2108 that
>> has the "byte swap" bug. So the "purge" is activated by the same "byte
>> swap" flag and  patch #2 depends on patch #1.
>
> So you only see the hang after close with data in fifo on cp2108? Is
> this something that might get fixed as well in later revisions?
>
> But doing a purge as part of close should not be a problem on the other
> products, right? I think doing this unconditionally would be best (just
> like you eventually add tx_empty for all devices). No need to depend on
> the broken-lcr-quirk.

Theoretically it shouldn't be a problem, but since it hasn't been
there for other devices, not causing any complaints, I'm reluctant to
do this.

>> Also, there is no reason
>> to only apply one of the patches. Do you think we can merge these
>> patches?
>
> No, again you want one logical change per patch.
>
> I suggest you split it as follows
>
>  - fix hang with non-empty tx fifo
>  - use port data for private data
>  - fix broken cp2108 lcr

Yes, this looks OK for an "unconditional purge" option.
Otherwise it would be
- use port data for private data
- fix both bugs

Since the purge workaround is almost a one-liner, it won't noticeably
increase the diff, and nobody would ever want to apply just one
workaround of the 2 and continue to experience device hangs.

Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Konstantin Shkolnyy]

On Tue, Oct 20, 2015 at 11:22 AM, Johan Hovold <johan@kernel.org> wrote:
> On Tue, Oct 20, 2015 at 09:19:05AM -0500, Konstantin Shkolnyy wrote:
>> On Tue, Oct 20, 2015 at 8:02 AM, Johan Hovold <johan@kernel.org> wrote:
>> > On Tue, Oct 20, 2015 at 07:52:31AM -0500, Konstantin Shkolnyy wrote:
>> >> On Tue, Oct 20, 2015 at 2:45 AM, Johan Hovold <johan@kernel.org> wrote:
>> >> [...]
>> >> > Instead of adding the new helpers to read u16 as a prerequisite for
>> >> > fixing the broken cp2108 support, just reuse the current config register
>> >> > helpers for now (in order to keep the fixes minimal and potentially
>> >> > backportable). Once the fixes are in place, feel free to clean up the
>> >> > remaining register accesses.
>> >>
>> >> The current helpers take "port" as a parameter. You pointed out
>> >> previously that port shouldn't be used in probe(). That made me
>> >> implement new helpers cp210x_write_u16_reg and cp210x_read_u16_reg
>> >> that don't use port. Probe() now calls cp210x_activate_workarounds
>> >> which in turn calls these new helpers.
>> >
>> > Oh, that's right.
>> >
>> >> An alternative would be to call usb_control_msg from
>> >> cp210x_activate_workarounds, but I think it would make it look pretty
>> >> ugly.
>> >
>> > Or you move the quirk-detect (and private data allocation) to port_probe
>> > instead (and remove startup/release). These devices have exactly one
>> > port per interface, so you wouldn't introduce any redundancy.
>>
>> OK, how about this:
>> patch #1
>> - attach/release replaced with port_probe/port_release;
>> - changes in the code using the "private" data since it has moved from
>> serial to port;
>> - cp210x_activate_workarounds in the port_probe, using the current helpers;
>> - a new helper cp210x_get_line_ctl that calls the current helper and
>> swaps the bytes.
>> patch #2
>> - sending "purge" from close, using the current helper.
>>
>> However, the Tx queue bug fixed by the "purge" doesn't have a
>> detection method. We only know that it exists in the same cp2108 that
>> has the "byte swap" bug. So the "purge" is activated by the same "byte
>> swap" flag and  patch #2 depends on patch #1.
>
> So you only see the hang after close with data in fifo on cp2108? Is
> this something that might get fixed as well in later revisions?
>
> But doing a purge as part of close should not be a problem on the other
> products, right? I think doing this unconditionally would be best (just
> like you eventually add tx_empty for all devices). No need to depend on
> the broken-lcr-quirk.

Sorry, sent the previous message too quickly. I see your point now.
There is no way to know if the Tx fifo bug will be fixed together with
"swapped bytes". And then the failure will return. I guess we'll just
make it unconditional and I'll make the patches according to your
plan:

>  - fix hang with non-empty tx fifo
>  - use port data for private data
>  - fix broken cp2108 lcr