Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the usb_control_msg_{send|recv}() API

[Anant Thazhemadam]

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


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


Thanks,
Anant