diff mbox

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

Message ID 1468852149-2614-2-git-send-email-martink@posteo.de (mailing list archive)
State Accepted
Headers show

Commit Message

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

Comments

Dmitry Torokhov July 20, 2016, 9:06 p.m. UTC | #1 
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.
Martin Kepplinger-Novakovic July 21, 2016, 11:06 a.m. UTC | #2 
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.
> 

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

Patch

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);