Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Felipe Balbi <balbi@kernel.org>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Paul Elder <paul.elder@ideasonboard.com>,
        Bin Liu <b-liu@ti.com>, kieran.bingham@ideasonboard.com,
        gregkh@linuxfoundation.org, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, rogerq@ti.com
Subject: Re: [PATCH 4/6] usb: gadget: add functions to signal udc driver to delay status stage
In-Reply-To: <Pine.LNX.4.44L0.1811060939160.1450-100000@iolanthe.rowland.org>
References: <Pine.LNX.4.44L0.1811060939160.1450-100000@iolanthe.rowland.org>
Date:   Wed, 07 Nov 2018 09:00:32 +0200
Message-ID: <87r2fxtlrj.fsf@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
>> DATA stage always depends on a usb_ep_queue() from gadget driver. So
>> it's always "delayed" in that sense.
>
> However, it's conceivable that some UDC drivers might behave=20
> differently depending on whether the usb_ep_queue call occurs within=20
> the setup callback or after that callback returns.  They _shouldn't_,=20
> but they might.

but now we're speculating. Should we really care before we catch
regressions?

>> it avoids all the special cases. UDC drivers can implement a single
>> handling for struct usb_request. We could do away with special return
>> values and so on...
>
> It's not quite so simple, because the UDC driver will need to keep=20
> track of whether a request queued on ep0 should be in the IN or the OUT=20
> direction.  (Maybe they have to do this already, I don't know.)

UDC drivers already have to do that.

>> > request and the UDC would then need to check whether that request corr=
esponds=20
>> > to a status stage and process it accordingly. A new operation specific=
 to this=20
>>=20
>> no, it wouldn't. UDC would have to check the size of request, that's
>> all:
>>=20
>> 	if (r->length =3D=3D 0)
>>         	special_zlp_handling();
>> 	else
>>         	regular_non_zlp_handling();
>
> Checking the length isn't enough.  A data stage can have 0 length.

apologies, I meant wLength, like so:

	len =3D le16_to_cpu(ctrl->wLength);
	if (!len) {
		dwc->three_stage_setup =3D false;
		dwc->ep0_expect_in =3D false;
		dwc->ep0_next_event =3D DWC3_EP0_NRDY_STATUS;
	} else {
		dwc->three_stage_setup =3D true;
		dwc->ep0_expect_in =3D !!(ctrl->bRequestType & USB_DIR_IN);
		dwc->ep0_next_event =3D DWC3_EP0_NRDY_DATA;
	}

>> But we don't need to care about special return values and the like. We
>> don't even need to care (from UDC perspective) if we're dealing with
>> 2-stage or 3-stage control transfers (well, dwc3 needs to care because
>> of different TRB types that needs to be used, but that's another story)
>
> No, we do need to care because of the direction issue.

special return values would be rendered uncessary if there's agreement
that status stage is always explicit. Why would need
USB_GADGET_DELAYED_STATUS if every case returns that?

>> > There's also the fact that requests can specify a completion handler, =
but only=20
>> > the data stage request would see its completion handler called (unless=
 we=20
>> > require UDCs to call completion requests at the completion of the stat=
us=20
>> > stage, but I'm not sure that all UDCs can report the event to the driv=
er, and=20
>> > that would likely be useless as nobody needs that feature).
>>=20
>> you still wanna know if the host actually processed your status
>> stage. udc-core can (and should) provide a generic status stage
>> completion function which, at a minimum, aids with some tracepoints.
>
> Helping with tracepoints is fine.  However, I don't think function=20
> drivers really need to know whether the status stage was processed by=20
> the host.  Can you point out any examples where such information would=20
> be useful?

If you know your STATUS stage completed, you have a guarantee that your
previous control transfer is complete. It's a very clear signal that you
should prepare for more control transfers.

>> >> (But it does involve a
>> >> race in cases where the host gets tired of waiting and issues another
>> >> SETUP packet before the processing of the first transfer is finished.)
>>=20
>> Host would stall first in that case.
>
> I don't follow.  Suppose the host sends a SETUP packet for an IN=20
> transfer, but the gadget takes so long to send the IN data back that=20
> the host times out.  So then the host sends a SETUP packet for a new=20
> transfer.  No stalls.
>
> (Besides, hosts never send STALL packets anyway.  Only peripherals do.)

oh okay. This is the setup_packet_pending case.

>> > To simplify function drivers, do you think the above proposal of addin=
g a flag=20
>> > to the (data stage) request to request an automatic transition to the =
status=20
>> > stage is a good idea ? We could even possibly invert the logic and tra=
nsition=20
>>=20
>> no, I don't think so. Making the status phase always explicit is far
>> better. UDCs won't have to check flags, or act on magic return
>> values. It just won't do anything until a request is queued.
>
> I don't agree.  This would be a simple test in a localized area (the=20
> completion callback for control requests).  It could even be=20
> implemented by a library routine; the UDC driver would simply have to=20
> call this routine immediately after invoking the callback.

I don't follow what you mean here.

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlvijZAACgkQzL64meEa
mQaAig/9EeGyVRELHHx4RBGS9zypenEkdvs6zDNWpWHddHDyreb2AoVbO77hKmV5
TYvrRokufGmBfMx4VMmkTa43zgLLwAet1G8uOMC/AdK8BA12KSLEsqSZnBF0U843
P8ZtJ0mu4cvt95JMEJ+pMTmLzqjdIV1KFekqyr/5C66RlxHkAp8CB2MuBaDOBRV2
/1YB9Vzx2zUnZTtnsBhFteK968Phk2VPajjMqdZaupsbQ2LhlgcfNFuaaQLJTPW2
hdS8eQfrVcUCG/KvkoXyUQ5bj8X3qJ0NkdBQmKNxJX6oHNOemIpi1amI7nprUhd8
P+sH5w1CAx35qhwmP17oUla5ENFvAcDu9iWBcWXygH0crDu90zBRhkuviJFJbPUe
Bj6oIfsDdNDZPazkv0FTjxGCwXCE8xSTbjqE16e3aRAkFfNITM8r5jKBHY96zD74
sNsnX5NrMnqcI7P1IOip+THJDH/dG9X5RrLZjiuV53CjSKiCw0pG8YrEgmGCB23O
FsnnbcHbePCObFUhriFvJecKVCzRzXSUCVXb3UlxXt051ej6w+LgsvgOTSJ8/++b
9rMFjMSaWWBkAwPT3Y7tzdiV4oTQbZMCpaKLxsY8cQIQFFpJ5tdNm/oPQ+IYgAcm
5DSRmZna4WyWKA5XpAtfq2vOAy1LKV80F/S1M4bXu5Mh35Zr0ZY=
=648u
-----END PGP SIGNATURE-----
--=-=-=--