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