Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the usb_control_msg_{send|recv}() API
From: Johan Hovold <johan@kernel.org>
Date: 2021-01-27 14:01:45
Also in:
lkml
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 treatedAs 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).
quoted hunk ↗ jump to hunk
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 <redacted> --- 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.
quoted hunk ↗ jump to hunk
- 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