Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Jan 2021 14:58:41 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Anant Thazhemadam <anant.thazhemadam@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Xu Wang <vulab@iscas.ac.cn>,
        Liu Shixin <liushixin2@huawei.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the
 usb_control_msg_{send|recv}() API
Message-ID: <YBFxkSlWPQRMuaGo@hovoldconsulting.com>
References: <20210126183403.911653-1-anant.thazhemadam@gmail.com>
 <20210126183403.911653-2-anant.thazhemadam@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210126183403.911653-2-anant.thazhemadam@gmail.com>
Precedence: bulk

On Wed, Jan 27, 2021 at 12:03:52AM +0530, Anant Thazhemadam wrote:
> The newer usb_control_msg_{send|recv}() API are an improvement on the
> existing usb_control_msg() as it ensures that a short read/write is treated

As I mentioned in my comments on v2, a short write has always been
treated as an error so you shouldn't imply that it wasn't here (and in
the other commit messages).

> as an error, data can be used off the stack, and raw usb pipes need not be
> created in the calling functions.
> For this reason, instances of usb_control_msg() have been replaced with
> usb_control_msg_{recv|send}(), and all return value checking
> conditions have also been modified appropriately.
> 
> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com>
> ---
>  drivers/usb/misc/appledisplay.c | 46 ++++++++++++++-------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
> index c8098e9b432e..117deb2fdc29 100644
> --- a/drivers/usb/misc/appledisplay.c
> +++ b/drivers/usb/misc/appledisplay.c
> @@ -132,21 +132,17 @@ static int appledisplay_bl_update_status(struct backlight_device *bd)
>  	pdata->msgdata[0] = 0x10;
>  	pdata->msgdata[1] = bd->props.brightness;
>  
> -	retval = usb_control_msg(
> -		pdata->udev,
> -		usb_sndctrlpipe(pdata->udev, 0),
> -		USB_REQ_SET_REPORT,
> -		USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -		ACD_USB_BRIGHTNESS,
> -		0,
> -		pdata->msgdata, 2,

In this case, the buffer is already DMA-able (and is in fact only used
for this purpose) so this patch introduces an extra allocation and
memcpy for no really good reason.

> -		ACD_USB_TIMEOUT);
> +	retval = usb_control_msg_send(pdata->udev,
> +				      0,
> +				      USB_REQ_SET_REPORT,
> +				      USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +				      ACD_USB_BRIGHTNESS,
> +				      0,
> +				      pdata->msgdata, 2,
> +				      ACD_USB_TIMEOUT, GFP_KERNEL);
>  	mutex_unlock(&pdata->sysfslock);
>  
> -	if (retval < 0)
> -		return retval;
> -	else
> -		return 0;
> +	return retval;
>  }
>  
>  static int appledisplay_bl_get_brightness(struct backlight_device *bd)
> @@ -155,21 +151,17 @@ static int appledisplay_bl_get_brightness(struct backlight_device *bd)
>  	int retval, brightness;
>  
>  	mutex_lock(&pdata->sysfslock);
> -	retval = usb_control_msg(
> -		pdata->udev,
> -		usb_rcvctrlpipe(pdata->udev, 0),
> -		USB_REQ_GET_REPORT,
> -		USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -		ACD_USB_BRIGHTNESS,
> -		0,
> -		pdata->msgdata, 2,
> -		ACD_USB_TIMEOUT);
> -	if (retval < 2) {
> -		if (retval >= 0)
> -			retval = -EMSGSIZE;
> -	} else {
> +	retval = usb_control_msg_recv(pdata->udev,
> +				      0,
> +				      USB_REQ_GET_REPORT,
> +				      USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> +				      ACD_USB_BRIGHTNESS,
> +				      0,
> +				      pdata->msgdata, 2,
> +				      ACD_USB_TIMEOUT, GFP_KERNEL);
> +	if (retval == 0)
>  		brightness = pdata->msgdata[1];
> -	}
> +

Same here, this introduces an extra allocation and memcpy and the only
thing you gain is essentially the removal of the two lines for handling
a short read.

>  	mutex_unlock(&pdata->sysfslock);
>  
>  	if (retval < 0)

I'd consider dropping this one as well.

Johan