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

[Johan Hovold <johan@kernel.org>]

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