[PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Karoly Pados]

Current GPIO code in cp210x fails to take USB autosuspend into account,
making it practically impossible to use GPIOs with autosuspend enabled
without user configuration. Fix this like for ftdi_sio in a previous patch.
Tested on a CP2102N.

Signed-off-by: Karoly Pados <pados@pados.hu>
---
 drivers/usb/serial/cp210x.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index c0777a374a88..8f974eabce63 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -598,9 +598,15 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
 	void *dmabuf;
 	int result;
 
+	result = usb_autopm_get_interface(serial->interface);
+	if (result)
+		return result;
+
 	dmabuf = kmalloc(bufsize, GFP_KERNEL);
-	if (!dmabuf)
+	if (!dmabuf) {
+		usb_autopm_put_interface(serial->interface);
 		return -ENOMEM;
+	}
 
 	result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0),
 				 CP210X_VENDOR_SPECIFIC, type, val,
@@ -618,6 +624,7 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
 	}
 
 	kfree(dmabuf);
+	usb_autopm_put_interface(serial->interface);
 
 	return result;
 }
@@ -702,9 +709,15 @@ static int cp210x_write_vendor_block(struct usb_serial *serial, u8 type,
 	void *dmabuf;
 	int result;
 
+	result = usb_autopm_get_interface(serial->interface);
+	if (result)
+		return result;
+
 	dmabuf = kmemdup(buf, bufsize, GFP_KERNEL);
-	if (!dmabuf)
+	if (!dmabuf) {
+		usb_autopm_put_interface(serial->interface);
 		return -ENOMEM;
+	}
 
 	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
 				 CP210X_VENDOR_SPECIFIC, type, val,
@@ -712,6 +725,7 @@ static int cp210x_write_vendor_block(struct usb_serial *serial, u8 type,
 				 USB_CTRL_SET_TIMEOUT);
 
 	kfree(dmabuf);
+	usb_autopm_put_interface(serial->interface);
 
 	if (result == bufsize) {
 		result = 0;
@@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
 	} else {
 		u16 wIndex = buf.state << 8 | buf.mask;
 
+		usb_autopm_get_interface(serial->interface);
 		result = usb_control_msg(serial->dev,
 					 usb_sndctrlpipe(serial->dev, 0),
 					 CP210X_VENDOR_SPECIFIC,
@@ -1390,6 +1405,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
 					 CP210X_WRITE_LATCH,
 					 wIndex,
 					 NULL, 0, USB_CTRL_SET_TIMEOUT);
+		usb_autopm_put_interface(serial->interface);
 	}
 
 	if (result < 0) {
-- 
2.20.1

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Johan Hovold]

On Mon, Jan 14, 2019 at 08:43:52PM +0100, Karoly Pados wrote:
> Current GPIO code in cp210x fails to take USB autosuspend into account,
> making it practically impossible to use GPIOs with autosuspend enabled
> without user configuration. Fix this like for ftdi_sio in a previous patch.
> Tested on a CP2102N.
> 
> Signed-off-by: Karoly Pados <pados@pados.hu>
> ---
>  drivers/usb/serial/cp210x.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index c0777a374a88..8f974eabce63 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -598,9 +598,15 @@ static int cp210x_read_vendor_block(struct usb_serial *serial, u8 type, u16 val,
>  	void *dmabuf;
>  	int result;
>  
> +	result = usb_autopm_get_interface(serial->interface);
> +	if (result)
> +		return result;
> +

> @@ -702,9 +709,15 @@ static int cp210x_write_vendor_block(struct usb_serial *serial, u8 type,
>  	void *dmabuf;
>  	int result;
>  
> +	result = usb_autopm_get_interface(serial->interface);
> +	if (result)
> +		return result;
> +

I think it's better to add the autopm call to gpio210x_gpio_get/set
only. This will allow for a simpler patch, and keeps the autopm handling
confined to the gpio paths.

> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int value)
>  	} else {
>  		u16 wIndex = buf.state << 8 | buf.mask;
>  
> +		usb_autopm_get_interface(serial->interface);

Also make sure to always check for errors from autopm_get().

>  		result = usb_control_msg(serial->dev,
>  					 usb_sndctrlpipe(serial->dev, 0),
>  					 CP210X_VENDOR_SPECIFIC,

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Karoly Pados]

> I think it's better to add the autopm call to gpio210x_gpio_get/set
> only. This will allow for a simpler patch, and keeps the autopm handling
> confined to the gpio paths.

I'll submit a v2.

>> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
>> value)
>> } else {
>> u16 wIndex = buf.state << 8 | buf.mask;
>> 
>> + usb_autopm_get_interface(serial->interface);
> 
> Also make sure to always check for errors from autopm_get().

I checked everywhere else, the reason I didn't check here is on purpose based on your previous feedback. The caller function here doesn't have a return value, so the only way to return errors is to log, but in my last patch to ftdi_sio you made clear that errors from autopm_get shouldn't get logged. Trying to call usb_control_msg() even though the device could not wake does not cause issues, and the return value from usb_control_msg() clearly identifies the reason for failure (failure due to autosuspend), so error information is not lost either. So I thought not checking here has no real disadvantage and I still stay conformant to your previous guidance.

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Johan Hovold]

On Tue, Jan 15, 2019 at 09:17:58AM +0000, Karoly Pados wrote:
> > I think it's better to add the autopm call to gpio210x_gpio_get/set
> > only. This will allow for a simpler patch, and keeps the autopm handling
> > confined to the gpio paths.
> 
> I'll submit a v2.
> 
> >> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
> >> value)
> >> } else {
> >> u16 wIndex = buf.state << 8 | buf.mask;
> >> 
> >> + usb_autopm_get_interface(serial->interface);
> > 
> > Also make sure to always check for errors from autopm_get().
> 
> I checked everywhere else, the reason I didn't check here is on
> purpose based on your previous feedback. The caller function here
> doesn't have a return value, so the only way to return errors is to
> log, but in my last patch to ftdi_sio you made clear that errors from
> autopm_get shouldn't get logged. Trying to call usb_control_msg() even
> though the device could not wake does not cause issues, and the return
> value from usb_control_msg() clearly identifies the reason for failure
> (failure due to autosuspend), so error information is not lost either.
> So I thought not checking here has no real disadvantage and I still
> stay conformant to your previous guidance.

Ok, I understand your reasoning, but please do check for errors and bail
out early if autopm_get() fails. No need to log errors.

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Johan Hovold]

On Tue, Jan 15, 2019 at 10:26:07AM +0100, Johan Hovold wrote:
> On Tue, Jan 15, 2019 at 09:17:58AM +0000, Karoly Pados wrote:
> > > I think it's better to add the autopm call to gpio210x_gpio_get/set
> > > only. This will allow for a simpler patch, and keeps the autopm handling
> > > confined to the gpio paths.
> > 
> > I'll submit a v2.
> > 
> > >> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
> > >> value)
> > >> } else {
> > >> u16 wIndex = buf.state << 8 | buf.mask;
> > >> 
> > >> + usb_autopm_get_interface(serial->interface);
> > > 
> > > Also make sure to always check for errors from autopm_get().
> > 
> > I checked everywhere else, the reason I didn't check here is on
> > purpose based on your previous feedback. The caller function here
> > doesn't have a return value, so the only way to return errors is to
> > log, but in my last patch to ftdi_sio you made clear that errors from
> > autopm_get shouldn't get logged. Trying to call usb_control_msg() even
> > though the device could not wake does not cause issues, and the return
> > value from usb_control_msg() clearly identifies the reason for failure
> > (failure due to autosuspend), so error information is not lost either.
> > So I thought not checking here has no real disadvantage and I still
> > stay conformant to your previous guidance.
> 
> Ok, I understand your reasoning, but please do check for errors and bail
> out early if autopm_get() fails. No need to log errors.

Actually, we should probably add the missing error handling to the
callers and have gpio_set() propagate errors too. If you want to take a
stab at that, that could be a follow-on patch.

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Johan Hovold]

On Tue, Jan 15, 2019 at 11:29:42AM +0100, Johan Hovold wrote:
> On Tue, Jan 15, 2019 at 10:26:07AM +0100, Johan Hovold wrote:
> > On Tue, Jan 15, 2019 at 09:17:58AM +0000, Karoly Pados wrote:
> > > > I think it's better to add the autopm call to gpio210x_gpio_get/set
> > > > only. This will allow for a simpler patch, and keeps the autopm handling
> > > > confined to the gpio paths.
> > > 
> > > I'll submit a v2.
> > > 
> > > >> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
> > > >> value)
> > > >> } else {
> > > >> u16 wIndex = buf.state << 8 | buf.mask;
> > > >> 
> > > >> + usb_autopm_get_interface(serial->interface);
> > > > 
> > > > Also make sure to always check for errors from autopm_get().
> > > 
> > > I checked everywhere else, the reason I didn't check here is on
> > > purpose based on your previous feedback. The caller function here
> > > doesn't have a return value, so the only way to return errors is to
> > > log, but in my last patch to ftdi_sio you made clear that errors from
> > > autopm_get shouldn't get logged. Trying to call usb_control_msg() even
> > > though the device could not wake does not cause issues, and the return
> > > value from usb_control_msg() clearly identifies the reason for failure
> > > (failure due to autosuspend), so error information is not lost either.
> > > So I thought not checking here has no real disadvantage and I still
> > > stay conformant to your previous guidance.
> > 
> > Ok, I understand your reasoning, but please do check for errors and bail
> > out early if autopm_get() fails. No need to log errors.
> 
> Actually, we should probably add the missing error handling to the
> callers and have gpio_set() propagate errors too. If you want to take a
> stab at that, that could be a follow-on patch.

Karoly, did you plan on sending a v2 of this one?

Thanks,
Johan

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Karoly Pados]

Uhm, yes, sorry, it kind of went out of my head. I am doing a lot of travelling lately (in the
past 48 hours I've been on 3 airplanes), and I had travels on earlier weeks too, mixed with
some project releases and family celebrations. So my head is just somewhere else.
Realistically, I'll submit something by the weekend when I'm home again.

Karoly

February 4, 2019 5:09 PM, "Johan Hovold" <johan@kernel.org> wrote:

> On Tue, Jan 15, 2019 at 11:29:42AM +0100, Johan Hovold wrote:
> 
>> On Tue, Jan 15, 2019 at 10:26:07AM +0100, Johan Hovold wrote:
>> On Tue, Jan 15, 2019 at 09:17:58AM +0000, Karoly Pados wrote:
>>>> I think it's better to add the autopm call to gpio210x_gpio_get/set
>>>> only. This will allow for a simpler patch, and keeps the autopm handling
>>>> confined to the gpio paths.
>>> 
>>> I'll submit a v2.
>>> 
>>>>> @@ -1383,6 +1397,7 @@ static void cp210x_gpio_set(struct gpio_chip *gc, unsigned int gpio, int
>>>>> value)
>>>>> } else {
>>>>> u16 wIndex = buf.state << 8 | buf.mask;
>>>>> 
>>>>> + usb_autopm_get_interface(serial->interface);
>>>> 
>>>> Also make sure to always check for errors from autopm_get().
>>> 
>>> I checked everywhere else, the reason I didn't check here is on
>>> purpose based on your previous feedback. The caller function here
>>> doesn't have a return value, so the only way to return errors is to
>>> log, but in my last patch to ftdi_sio you made clear that errors from
>>> autopm_get shouldn't get logged. Trying to call usb_control_msg() even
>>> though the device could not wake does not cause issues, and the return
>>> value from usb_control_msg() clearly identifies the reason for failure
>>> (failure due to autosuspend), so error information is not lost either.
>>> So I thought not checking here has no real disadvantage and I still
>>> stay conformant to your previous guidance.
>> 
>> Ok, I understand your reasoning, but please do check for errors and bail
>> out early if autopm_get() fails. No need to log errors.
>> 
>> Actually, we should probably add the missing error handling to the
>> callers and have gpio_set() propagate errors too. If you want to take a
>> stab at that, that could be a follow-on patch.
> 
> Karoly, did you plan on sending a v2 of this one?
> 
> Thanks,
> Johan

Re: [PATCH] USB: serial: cp210x: Fix GPIO in autosuspend

[Johan Hovold]

On Wed, Feb 06, 2019 at 09:43:36PM +0000, Karoly Pados wrote:
> Uhm, yes, sorry, it kind of went out of my head. I am doing a lot of
> travelling lately (in the past 48 hours I've been on 3 airplanes), and
> I had travels on earlier weeks too, mixed with some project releases
> and family celebrations. So my head is just somewhere else.
> Realistically, I'll submit something by the weekend when I'm home
> again.

Sounds good. Just want to make sure this gets fixed in 5.1 (i.e. before
5.0 is released).

And just let me know if you can't find the time.

Johan