diff mbox series

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

Message ID 20210126183403.911653-2-anant.thazhemadam@gmail.com (mailing list archive)
State Superseded
Headers show
Series drivers: usb: misc: update to use usb_control_msg_{send|recv}() API | expand

Commit Message

Anant Thazhemadam Jan. 26, 2021, 6:33 p.m. UTC 
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 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(-)

Comments

Johan Hovold Jan. 27, 2021, 1:58 p.m. UTC | #1 
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
Anant Thazhemadam Jan. 27, 2021, 2:42 p.m. UTC | #2 
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@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.


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


Thanks,
Anant
Johan Hovold Jan. 27, 2021, 5:20 p.m. UTC | #3 
On Wed, Jan 27, 2021 at 08:12:21PM +0530, Anant Thazhemadam wrote:
> 
> 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).

But my point is that this claim isn't factually correct; there has never
been a need to check for short *writes* (even if a few drivers have such
redundant checks).

> That's what I was trying to say. But if the commit message isn't
> representative of that, I'll try and modify it.

Just drop the bit about "short writes".

> 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().

Even this is disputable; in some situations the usb_control_msg() is
still preferred as I hope my comments have shown.

Perhaps they are better described as "convenience wrappers" or similar
as the real gain from using these helpers is to replace a pattern like:

	f(data, ...) {
		buf = malloc(data);
		usb_control_msg(..., buf, ...);
		memcpy(data, buf, ...);
		kfree(buf);
	}

for when data is on the stack *and* you do not expect variable-length IN
transfers.

But as soon as a driver is able to reuse a single buffer for multiple
transfers or the data buffer is already DMA-able, usb_control_msg() may
still be a better choice.

> 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

recv only

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

Johan
diff mbox series

Patch

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,
-		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];
-	}
+
 	mutex_unlock(&pdata->sysfslock);
 
 	if (retval < 0)