Re: BUG/PATCH: cp210x.c - reenable support for chips which don't report a partnum

2017-08-28 Thread Johan Hovold 
On Wed, Aug 09, 2017 at 02:18:06PM +0200, Sebastian Frei wrote:

Thanks for the patch. I have few comments below, and also make sure to
send the patch with a proper subject line, such as:

[PATCH v2] USB: serial: cp210x: fix partnum regression

> Make cp210x not abort if part number could not be read from device.

Please expand the commit message somewhat and mention your specific
device which now fails to probe. You should also mention the offending
commit that introduced the regression, preferably using a Fixes-tag:

Fixes: cf5276ce7867 ("USB: serial: cp210x: Adding GPIO support for CP2105")

> Signed-off-by: Sebastian Frei 
> ---
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index f64e914a8985..d6afa823d6f0 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -1487,13 +1487,17 @@ static int cp210x_attach(struct usb_serial *serial)
>   if (!priv)
>   return -ENOMEM;
>  
> + usb_set_serial_data(serial, priv);
> +
>   result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> CP210X_GET_PARTNUM, >partnum,
> sizeof(priv->partnum));
> - if (result < 0)
> - goto err_free_priv;
>  
> - usb_set_serial_data(serial, priv);
> + if (result < 0) {
> + dev_err(>interface->dev,
> + "querying part number failed, continuing without GPIO 
> support\n");
> + return 0;

Don't mention GPIO support in the error message, and instead of
returning early just continue with partnum set to a new constant:

#define CP210X_PARTNUM_UNKNOWN  255

> + }
>  
>   if (priv->partnum == CP210X_PARTNUM_CP2105) {
>   result = cp2105_shared_gpio_init(serial);
> @@ -1504,10 +1508,6 @@ static int cp210x_attach(struct usb_serial *serial)
>   }
>  
>   return 0;
> -err_free_priv:
> - kfree(priv);
> -
> - return result;
>  }
>  
>  static void cp210x_disconnect(struct usb_serial *serial)

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG/PATCH: cp210x.c - reenable support for chips which don't report a partnum

2017-08-09 Thread Sebastian Frei 
Make cp210x not abort if part number could not be read from device.

Signed-off-by: Sebastian Frei 
---

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index f64e914a8985..d6afa823d6f0 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -1487,13 +1487,17 @@ static int cp210x_attach(struct usb_serial *serial)
if (!priv)
return -ENOMEM;
 
+   usb_set_serial_data(serial, priv);
+
result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
  CP210X_GET_PARTNUM, >partnum,
  sizeof(priv->partnum));
-   if (result < 0)
-   goto err_free_priv;
 
-   usb_set_serial_data(serial, priv);
+   if (result < 0) {
+   dev_err(>interface->dev,
+   "querying part number failed, continuing without GPIO 
support\n");
+   return 0;
+   }
 
if (priv->partnum == CP210X_PARTNUM_CP2105) {
result = cp2105_shared_gpio_init(serial);
@@ -1504,10 +1508,6 @@ static int cp210x_attach(struct usb_serial *serial)
}
 
return 0;
-err_free_priv:
-   kfree(priv);
-
-   return result;
 }
 
 static void cp210x_disconnect(struct usb_serial *serial)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: BUG/PATCH: cp210x.c - reenable support for chips which don't report a partnum

2017-08-08 Thread Greg KH 
On Tue, Aug 08, 2017 at 08:59:01PM +0200, Sebastian Frei wrote:
> Hi,
> 
> I own a data cable for Siemens mobile phones:
> 10ab:10c5 USI Co., Ltd Sony-Ericsson / Samsung DataCable
> 
> The cp210x chip of this cable seems to not give an answer to the 
> CP210X_GET_PARTNUM command. So since this commit:
> 
> 2016-10-24 USB: serial: cp210x: Adding GPIO support for CP2105
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/commit/drivers/usb/serial/cp210x.c?id=cf5276ce7867d6d87c02fbe4977646ed342e323a
> 
> the driver aborts while loading:
> 
> [  242.446506] usb 1-5.1: new full-speed USB device number 7 using xhci_hcd
> [  242.682993] usb 1-5.1: New USB device found, idVendor=10ab, idProduct=10c5
> [  242.682996] usb 1-5.1: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=3
> [  242.682998] usb 1-5.1: Product: usb data cable
> [  242.682999] usb 1-5.1: Manufacturer: Silicon Labs
> [  242.683000] usb 1-5.1: SerialNumber: 0001
> [  242.711590] usbcore: registered new interface driver usbserial
> [  242.711602] usbcore: registered new interface driver usbserial_generic
> [  242.711611] usbserial: USB Serial support registered for generic
> [  242.712947] usbcore: registered new interface driver cp210x
> [  242.712958] usbserial: USB Serial support registered for cp210x
> [  242.712981] cp210x 1-5.1:1.0: cp210x converter detected
> [  242.716630] cp210x 1-5.1:1.0: failed to get vendor val 0x370b size 1: -32
> [  242.716645] cp210x: probe of 1-5.1:1.0 failed with error -5
> 
> I'm proposing a patch to continue loading but skip the GPIO initialization:
> 
> --- drivers/usb/serial/cp210x.c.orig  2017-08-08 20:11:34.327209672 +0200
> +++ drivers/usb/serial/cp210x.c   2017-08-08 20:12:57.735833959 +0200
> @@ -1480,31 +1480,31 @@
>  static int cp210x_attach(struct usb_serial *serial)
>  {
>   int result;
>   struct cp210x_serial_private *priv;
>  
>   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>   if (!priv)
>   return -ENOMEM;
>  
> + usb_set_serial_data(serial, priv);
> +
>   result = cp210x_read_vendor_block(serial, REQTYPE_DEVICE_TO_HOST,
> CP210X_GET_PARTNUM, >partnum,
> sizeof(priv->partnum));
> - if (result < 0)
> - goto err_free_priv;
>  
> - usb_set_serial_data(serial, priv);
> + if (result < 0) {
> + dev_err(>interface->dev,
> + "querying part number failed, continuing without GPIO 
> support\n");
> + return 0;
> + }
>  
>   if (priv->partnum == CP210X_PARTNUM_CP2105) {
>   result = cp2105_shared_gpio_init(serial);
>   if (result < 0) {
>   dev_err(>interface->dev,
>   "GPIO initialisation failed, continuing without 
> GPIO support\n");
>   }
>   }
>  
>   return 0;
> -err_free_priv:
> - kfree(priv);
> -
> - return result;
>  }
>  
> Signed-off-by: Sebastian Frei 
> 
> Output now with the patch:
> [  298.994093] usb 1-5.1: new full-speed USB device number 8 using xhci_hcd
> [  299.230631] usb 1-5.1: New USB device found, idVendor=10ab, idProduct=10c5
> [  299.230634] usb 1-5.1: New USB device strings: Mfr=1, Product=2, 
> SerialNumber=3
> [  299.230635] usb 1-5.1: Product: usb data cable
> [  299.230637] usb 1-5.1: Manufacturer: Silicon Labs
> [  299.230638] usb 1-5.1: SerialNumber: 0001
> [  299.247270] cp210x: no symbol version for module_layout
> [  299.247750] usbcore: registered new interface driver cp210x
> [  299.247764] usbserial: USB Serial support registered for cp210x
> [  299.247793] cp210x 1-5.1:1.0: cp210x converter detected
> [  299.251637] cp210x 1-5.1:1.0: failed to get vendor val 0x370b size 1: -32
> [  299.251642] cp210x 1-5.1:1.0: querying part number failed, continuing 
> without GPIO support
> [  299.271749] usb 1-5.1: cp210x converter now attached to ttyUSB0
> 
> The cable is working, TX & RX sends and receives data.

Seems reasonable, can you fix it up and resend it in a format that we
can apply it in?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html