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@xxxxxxxxx>
> ---
>  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