[PATCH 1/4] input: tablet: pegasus_notetaker: Track usb control msg errors

[PATCH 1/4] input: tablet: pegasus_notetaker: Track usb control msg errors

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/input/tablet/pegasus_notetaker.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index 83aa583..27cb352 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -79,7 +79,7 @@ struct pegasus {
 	struct work_struct init;
 };
 
-static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
+static int pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
 {
 	const int sizeof_buf = len + 2;
 	int result;
@@ -87,7 +87,7 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
 
 	cmd_buf = kmalloc(sizeof_buf, GFP_KERNEL);
 	if (!cmd_buf)
-		return;
+		return -ENOMEM;
 
 	cmd_buf[0] = NOTETAKER_REPORT_ID;
 	cmd_buf[1] = len;
@@ -100,17 +100,23 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
 				 0, 0, cmd_buf, sizeof_buf,
 				 USB_CTRL_SET_TIMEOUT);
 
-	if (result != sizeof_buf)
-		dev_err(&pegasus->usbdev->dev, "control msg error\n");
+	if (result != sizeof_buf) {
+		if (result >= 0)
+			result = -EIO;
+		dev_err(&pegasus->usbdev->dev, "control msg error: %d\n",
+			result);
+	}
 
 	kfree(cmd_buf);
+
+	return result;
 }
 
-static void pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
+static int pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
 {
 	u8 cmd[] = {NOTETAKER_SET_CMD, NOTETAKER_SET_MODE, led, mode};
 
-	pegasus_control_msg(pegasus, cmd, sizeof(cmd));
+	return pegasus_control_msg(pegasus, cmd, sizeof(cmd));
 }
 
 static void pegasus_parse_packet(struct pegasus *pegasus)
@@ -184,8 +190,12 @@ static void pegasus_irq(struct urb *urb)
 static void pegasus_init(struct work_struct *work)
 {
 	struct pegasus *pegasus = container_of(work, struct pegasus, init);
+	int retval;
 
-	pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
+	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
+	if (retval < 0)
+		dev_err(&pegasus->usbdev->dev, "pegasus_set_mode error: %d\n",
+			retval);
 }
 
 static int pegasus_open(struct input_dev *dev)
@@ -201,7 +211,7 @@ static int pegasus_open(struct input_dev *dev)
 	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
 		retval = -EIO;
 
-	pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
+	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
 
 	usb_autopm_put_interface(pegasus->intf);
 
-- 
2.1.4

[PATCH 2/4] input: tablet: pegasus_notetaker: Fix usb_autopm calls to be balanced

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/input/tablet/pegasus_notetaker.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index 27cb352..fdbc5e8 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -208,27 +208,30 @@ static int pegasus_open(struct input_dev *dev)
 		return retval;
 
 	pegasus->irq->dev = pegasus->usbdev;
-	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
+	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
 		retval = -EIO;
+		goto out;
+	}
 
 	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
 
-	usb_autopm_put_interface(pegasus->intf);
+out:
 
-	return retval;
+	if (retval < 0) {
+		usb_autopm_put_interface(pegasus->intf);
+		return retval;
+	} else {
+		return 0;
+	}
 }
 
 static void pegasus_close(struct input_dev *dev)
 {
 	struct pegasus *pegasus = input_get_drvdata(dev);
-	int autopm_error;
 
-	autopm_error = usb_autopm_get_interface(pegasus->intf);
 	usb_kill_urb(pegasus->irq);
 	cancel_work_sync(&pegasus->init);
-
-	if (!autopm_error)
-		usb_autopm_put_interface(pegasus->intf);
+	usb_autopm_put_interface(pegasus->intf);
 }
 
 static int pegasus_probe(struct usb_interface *intf,
-- 
2.1.4

[PATCH 3/4] input: tablet: pegasus_notetaker: Cancel workqueue's work in suspend()

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/input/tablet/pegasus_notetaker.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index fdbc5e8..07436c6 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -377,6 +377,7 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
 
 	mutex_lock(&pegasus->dev->mutex);
 	usb_kill_urb(pegasus->irq);
+	cancel_work_sync(&pegasus->init);
 	mutex_unlock(&pegasus->dev->mutex);
 
 	return 0;
-- 
2.1.4

[PATCH 4/4] input: tablet: pegasus_notetaker: Set device mode in reset_resume() if in use

[Martin Kepplinger]

Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
 drivers/input/tablet/pegasus_notetaker.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
index 07436c6..b172247 100644
--- a/drivers/input/tablet/pegasus_notetaker.c
+++ b/drivers/input/tablet/pegasus_notetaker.c
@@ -398,7 +398,19 @@ static int pegasus_resume(struct usb_interface *intf)
 
 static int pegasus_reset_resume(struct usb_interface *intf)
 {
-	return pegasus_resume(intf);
+	struct pegasus *pegasus = usb_get_intfdata(intf);
+	int retval = 0;
+
+	mutex_lock(&pegasus->dev->mutex);
+	if (pegasus->dev->users) {
+		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
+					  NOTETAKER_LED_MOUSE);
+		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
+			retval = -EIO;
+	}
+	mutex_unlock(&pegasus->dev->mutex);
+
+	return retval;
 }
 
 static const struct usb_device_id pegasus_ids[] = {
-- 
2.1.4

Re: [PATCH 1/4] input: tablet: pegasus_notetaker: Track usb control msg errors

[Dmitry Torokhov]

Hi Martin,

On Mon, Jul 18, 2016 at 04:29:06PM +0200, Martin Kepplinger wrote:
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
>  drivers/input/tablet/pegasus_notetaker.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 83aa583..27cb352 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -79,7 +79,7 @@ struct pegasus {
>  	struct work_struct init;
>  };
>  
> -static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
> +static int pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>  {
>  	const int sizeof_buf = len + 2;
>  	int result;
> @@ -87,7 +87,7 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>  
>  	cmd_buf = kmalloc(sizeof_buf, GFP_KERNEL);
>  	if (!cmd_buf)
> -		return;
> +		return -ENOMEM;
>  
>  	cmd_buf[0] = NOTETAKER_REPORT_ID;
>  	cmd_buf[1] = len;
> @@ -100,17 +100,23 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>  				 0, 0, cmd_buf, sizeof_buf,
>  				 USB_CTRL_SET_TIMEOUT);
>  
> -	if (result != sizeof_buf)
> -		dev_err(&pegasus->usbdev->dev, "control msg error\n");
> +	if (result != sizeof_buf) {
> +		if (result >= 0)
> +			result = -EIO;
> +		dev_err(&pegasus->usbdev->dev, "control msg error: %d\n",
> +			result);
> +	}
>  
>  	kfree(cmd_buf);
> +
> +	return result;

Your 4th patch relies on pegasus_control_msg() returning 0 on success,
otherwise you will not restart the URB when doing reset resume. I
rearranged the code here so we free cmd_buf right after calling
usb_control_msg() and the explicitly returnig 0 in success branch.

>  }
>  
> -static void pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
> +static int pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
>  {
>  	u8 cmd[] = {NOTETAKER_SET_CMD, NOTETAKER_SET_MODE, led, mode};
>  
> -	pegasus_control_msg(pegasus, cmd, sizeof(cmd));
> +	return pegasus_control_msg(pegasus, cmd, sizeof(cmd));
>  }
>  
>  static void pegasus_parse_packet(struct pegasus *pegasus)
> @@ -184,8 +190,12 @@ static void pegasus_irq(struct urb *urb)
>  static void pegasus_init(struct work_struct *work)
>  {
>  	struct pegasus *pegasus = container_of(work, struct pegasus, init);
> +	int retval;
>  
> -	pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +	if (retval < 0)
> +		dev_err(&pegasus->usbdev->dev, "pegasus_set_mode error: %d\n",
> +			retval);
>  }
>  
>  static int pegasus_open(struct input_dev *dev)
> @@ -201,7 +211,7 @@ static int pegasus_open(struct input_dev *dev)
>  	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
>  		retval = -EIO;
>  
> -	pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> +	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
>  
>  	usb_autopm_put_interface(pegasus->intf);
>  

Thanks.

-- 
Dmitry

Re: [PATCH 2/4] input: tablet: pegasus_notetaker: Fix usb_autopm calls to be balanced

[Dmitry Torokhov]

On Mon, Jul 18, 2016 at 04:29:07PM +0200, Martin Kepplinger wrote:
> Signed-off-by: Martin Kepplinger <martink@posteo.de>
> ---
>  drivers/input/tablet/pegasus_notetaker.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 27cb352..fdbc5e8 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -208,27 +208,30 @@ static int pegasus_open(struct input_dev *dev)
>  		return retval;
>  
>  	pegasus->irq->dev = pegasus->usbdev;
> -	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
> +	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
>  		retval = -EIO;
> +		goto out;
> +	}
>  
>  	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);

If this fails I we should kill URB and cancel work. I adjusted and
applied.

>  
> -	usb_autopm_put_interface(pegasus->intf);
> +out:
>  
> -	return retval;
> +	if (retval < 0) {
> +		usb_autopm_put_interface(pegasus->intf);
> +		return retval;
> +	} else {
> +		return 0;
> +	}
>  }
>  
>  static void pegasus_close(struct input_dev *dev)
>  {
>  	struct pegasus *pegasus = input_get_drvdata(dev);
> -	int autopm_error;
>  
> -	autopm_error = usb_autopm_get_interface(pegasus->intf);
>  	usb_kill_urb(pegasus->irq);
>  	cancel_work_sync(&pegasus->init);
> -
> -	if (!autopm_error)
> -		usb_autopm_put_interface(pegasus->intf);
> +	usb_autopm_put_interface(pegasus->intf);
>  }
>  
>  static int pegasus_probe(struct usb_interface *intf,
> -- 
> 2.1.4
> 

-- 
Dmitry

Re: [PATCH 3/4] input: tablet: pegasus_notetaker: Cancel workqueue's work in suspend()

[Dmitry Torokhov]

On Mon, Jul 18, 2016 at 04:29:08PM +0200, Martin Kepplinger wrote:
> Signed-off-by: Martin Kepplinger <martink@posteo.de>

Applied, thank you.

> ---
>  drivers/input/tablet/pegasus_notetaker.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index fdbc5e8..07436c6 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -377,6 +377,7 @@ static int pegasus_suspend(struct usb_interface *intf, pm_message_t message)
>  
>  	mutex_lock(&pegasus->dev->mutex);
>  	usb_kill_urb(pegasus->irq);
> +	cancel_work_sync(&pegasus->init);
>  	mutex_unlock(&pegasus->dev->mutex);

-- 
Dmitry

Re: [PATCH 4/4] input: tablet: pegasus_notetaker: Set device mode in reset_resume() if in use

[Dmitry Torokhov]

On Mon, Jul 18, 2016 at 04:29:09PM +0200, Martin Kepplinger wrote:
> Signed-off-by: Martin Kepplinger <martink@posteo.de>

Applied, thank you.

> ---
>  drivers/input/tablet/pegasus_notetaker.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
> index 07436c6..b172247 100644
> --- a/drivers/input/tablet/pegasus_notetaker.c
> +++ b/drivers/input/tablet/pegasus_notetaker.c
> @@ -398,7 +398,19 @@ static int pegasus_resume(struct usb_interface *intf)
>  
>  static int pegasus_reset_resume(struct usb_interface *intf)
>  {
> -	return pegasus_resume(intf);
> +	struct pegasus *pegasus = usb_get_intfdata(intf);
> +	int retval = 0;
> +
> +	mutex_lock(&pegasus->dev->mutex);
> +	if (pegasus->dev->users) {
> +		retval = pegasus_set_mode(pegasus, PEN_MODE_XY,
> +					  NOTETAKER_LED_MOUSE);
> +		if (!retval && usb_submit_urb(pegasus->irq, GFP_NOIO) < 0)
> +			retval = -EIO;
> +	}
> +	mutex_unlock(&pegasus->dev->mutex);
> +
> +	return retval;
>  }

-- 
Dmitry

Re: [PATCH 2/4] input: tablet: pegasus_notetaker: Fix usb_autopm calls to be balanced

[Martin Kepplinger]

Am 2016-07-20 um 23:29 schrieb Dmitry Torokhov:
> On Mon, Jul 18, 2016 at 04:29:07PM +0200, Martin Kepplinger wrote:
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> ---
>>  drivers/input/tablet/pegasus_notetaker.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
>> index 27cb352..fdbc5e8 100644
>> --- a/drivers/input/tablet/pegasus_notetaker.c
>> +++ b/drivers/input/tablet/pegasus_notetaker.c
>> @@ -208,27 +208,30 @@ static int pegasus_open(struct input_dev *dev)
>>  		return retval;
>>  
>>  	pegasus->irq->dev = pegasus->usbdev;
>> -	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
>> +	if (usb_submit_urb(pegasus->irq, GFP_KERNEL)) {
>>  		retval = -EIO;
>> +		goto out;
>> +	}
>>  
>>  	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
> 
> If this fails I we should kill URB and cancel work. I adjusted and
> applied.

You're right. Thanks for this!

> 
>>  
>> -	usb_autopm_put_interface(pegasus->intf);
>> +out:
>>  
>> -	return retval;
>> +	if (retval < 0) {
>> +		usb_autopm_put_interface(pegasus->intf);
>> +		return retval;
>> +	} else {
>> +		return 0;
>> +	}
>>  }
>>  
>>  static void pegasus_close(struct input_dev *dev)
>>  {
>>  	struct pegasus *pegasus = input_get_drvdata(dev);
>> -	int autopm_error;
>>  
>> -	autopm_error = usb_autopm_get_interface(pegasus->intf);
>>  	usb_kill_urb(pegasus->irq);
>>  	cancel_work_sync(&pegasus->init);
>> -
>> -	if (!autopm_error)
>> -		usb_autopm_put_interface(pegasus->intf);
>> +	usb_autopm_put_interface(pegasus->intf);
>>  }
>>  
>>  static int pegasus_probe(struct usb_interface *intf,
>> -- 
>> 2.1.4
>>
> 

Re: [PATCH 1/4] input: tablet: pegasus_notetaker: Track usb control msg errors

[Martin Kepplinger]

Am 2016-07-20 um 23:06 schrieb Dmitry Torokhov:
> Hi Martin,
> 
> On Mon, Jul 18, 2016 at 04:29:06PM +0200, Martin Kepplinger wrote:
>> Signed-off-by: Martin Kepplinger <martink@posteo.de>
>> ---
>>  drivers/input/tablet/pegasus_notetaker.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/input/tablet/pegasus_notetaker.c b/drivers/input/tablet/pegasus_notetaker.c
>> index 83aa583..27cb352 100644
>> --- a/drivers/input/tablet/pegasus_notetaker.c
>> +++ b/drivers/input/tablet/pegasus_notetaker.c
>> @@ -79,7 +79,7 @@ struct pegasus {
>>  	struct work_struct init;
>>  };
>>  
>> -static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>> +static int pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>>  {
>>  	const int sizeof_buf = len + 2;
>>  	int result;
>> @@ -87,7 +87,7 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>>  
>>  	cmd_buf = kmalloc(sizeof_buf, GFP_KERNEL);
>>  	if (!cmd_buf)
>> -		return;
>> +		return -ENOMEM;
>>  
>>  	cmd_buf[0] = NOTETAKER_REPORT_ID;
>>  	cmd_buf[1] = len;
>> @@ -100,17 +100,23 @@ static void pegasus_control_msg(struct pegasus *pegasus, u8 *data, int len)
>>  				 0, 0, cmd_buf, sizeof_buf,
>>  				 USB_CTRL_SET_TIMEOUT);
>>  
>> -	if (result != sizeof_buf)
>> -		dev_err(&pegasus->usbdev->dev, "control msg error\n");
>> +	if (result != sizeof_buf) {
>> +		if (result >= 0)
>> +			result = -EIO;
>> +		dev_err(&pegasus->usbdev->dev, "control msg error: %d\n",
>> +			result);
>> +	}
>>  
>>  	kfree(cmd_buf);
>> +
>> +	return result;
> 
> Your 4th patch relies on pegasus_control_msg() returning 0 on success,
> otherwise you will not restart the URB when doing reset resume. I
> rearranged the code here so we free cmd_buf right after calling
> usb_control_msg() and the explicitly returnig 0 in success branch.
> 

Yes, I saw it. Thanks for your explaining and fixing!

>>  }
>>  
>> -static void pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
>> +static int pegasus_set_mode(struct pegasus *pegasus, u8 mode, u8 led)
>>  {
>>  	u8 cmd[] = {NOTETAKER_SET_CMD, NOTETAKER_SET_MODE, led, mode};
>>  
>> -	pegasus_control_msg(pegasus, cmd, sizeof(cmd));
>> +	return pegasus_control_msg(pegasus, cmd, sizeof(cmd));
>>  }
>>  
>>  static void pegasus_parse_packet(struct pegasus *pegasus)
>> @@ -184,8 +190,12 @@ static void pegasus_irq(struct urb *urb)
>>  static void pegasus_init(struct work_struct *work)
>>  {
>>  	struct pegasus *pegasus = container_of(work, struct pegasus, init);
>> +	int retval;
>>  
>> -	pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
>> +	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
>> +	if (retval < 0)
>> +		dev_err(&pegasus->usbdev->dev, "pegasus_set_mode error: %d\n",
>> +			retval);
>>  }
>>  
>>  static int pegasus_open(struct input_dev *dev)
>> @@ -201,7 +211,7 @@ static int pegasus_open(struct input_dev *dev)
>>  	if (usb_submit_urb(pegasus->irq, GFP_KERNEL))
>>  		retval = -EIO;
>>  
>> -	pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
>> +	retval = pegasus_set_mode(pegasus, PEN_MODE_XY, NOTETAKER_LED_MOUSE);
>>  
>>  	usb_autopm_put_interface(pegasus->intf);
>>  
> 
> Thanks.
>