Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Jan 2021 18:20:23 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Anant Thazhemadam <anant.thazhemadam@gmail.com>
Cc:     Johan Hovold <johan@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Xu Wang <vulab@iscas.ac.cn>,
        Liu Shixin <liushixin2@huawei.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 01/12] usb: misc: appledisplay: update to use the
 usb_control_msg_{send|recv}() API
Message-ID: <YBGg132/qWaPgjsI@hovoldconsulting.com>
References: <20210126183403.911653-1-anant.thazhemadam@gmail.com>
 <20210126183403.911653-2-anant.thazhemadam@gmail.com>
 <YBFxkSlWPQRMuaGo@hovoldconsulting.com>
 <f82c8fae-251f-12e9-6e43-28824a5ff3e5@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f82c8fae-251f-12e9-6e43-28824a5ff3e5@gmail.com>
Precedence: bulk

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