linux-kernel - Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the usb_control_msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 27 Jan 2021 20:12:21 +0530
From:   Anant Thazhemadam <anant.thazhemadam@...il.com>
To:     Johan Hovold <johan@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Xu Wang <vulab@...as.ac.cn>,
        Liu Shixin <liushixin2@...wei.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the
 usb_control_msg_{send|recv}() API


On 27/01/21 7:28 pm, Johan Hovold wrote:
> 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).

The newer API ensures that a short send/recv is an error, as they either
complete the complete translation, or return an error, and remove the need
for explicit return value checking that was previously expected to detect the short
read/write (which wasn't present in a lot of places).
That's what I was trying to say. But if the commit message isn't representative of
that, I'll try and modify it.

Does this sound like a better commit message?

"The newer usb_control_msg_{send|recv}() API are an improvement on the
existing usb_control_msg().

The new API ensures either the full translation is completed,
or an error is returned. This ensures that all short send/recv are detected as
errors even if there is no explicit return value checking performed.

The new API also allows us to use data off the stack, and don't require raw usb
pipes to be created in the calling functions."


>> 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@...il.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.


Yes, that's probably the better thing to do here.


Thanks,
Anant