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:     Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alan Stern <stern@rowland.harvard.edu>,
        Johan Hovold <johan@kernel.org>,
        Jaejoong Kim <climbbb.kim@gmail.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Roger Quadros <rogerq@ti.com>
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        v.anuragkumar@gmail.com, Thinh Nguyen <thinhn@synopsys.com>,
        Tejas Joglekar <tejas.joglekar@synopsys.com>,
        Ajay Yugalkishore Pandey <APANDEY@xilinx.com>,
        Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
Subject: Re: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable endpoints
In-Reply-To: <1539436498-24892-2-git-send-email-anurag.kumar.vulisha@xilinx.com>
References: <1539436498-24892-1-git-send-email-anurag.kumar.vulisha@xilinx.com> <1539436498-24892-2-git-send-email-anurag.kumar.vulisha@xilinx.com>
Date:   Wed, 14 Nov 2018 15:57:44 +0200
Message-ID: <87k1lfiwx3.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,

Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> writes:
> When bulk streams are enabled for an endpoint, there can
> be a condition where the gadget controller waits for the
> host to issue prime transaction and the host controller
> waits for the gadget to issue ERDY. This condition could
> create a deadlock. To avoid such potential deadlocks, a
> timer is started after queuing any request for the stream
> capable endpoint in usb_ep_queue(). The gadget driver is
> expected to stop the timer if a valid stream event is found.
> If no stream event is found, the timer expires after the
> STREAM_TIMEOUT_MS value and a callback function registered
> by gadget driver to endpoint ops->stream_timeout API would be
> called, so that the gadget driver can handle this situation.
> This kind of behaviour is observed in dwc3 controller and
> expected to be generic issue with other controllers supporting
> bulk streams also.
>
> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
> ---
>  Changes in v6:
> 	1. This patch is newly added in this series to add timer into udc/core.c
> ---
>  drivers/usb/gadget/udc/core.c | 71 +++++++++++++++++++++++++++++++++++++=
+++++-
>  include/linux/usb/gadget.h    | 12 ++++++++
>  2 files changed, 82 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index af88b48..41cc23b 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -52,6 +52,24 @@ static int udc_bind_to_driver(struct usb_udc *udc,
>  /* ---------------------------------------------------------------------=
---- */
>=20=20
>  /**
> + * usb_ep_stream_timeout - callback function for endpoint stream timeout=
 timer
> + * @arg: pointer to struct timer_list
> + *
> + * This function gets called only when bulk streams are enabled in the e=
ndpoint
> + * and only after ep->stream_timeout_timer has expired. The timer gets e=
xpired
> + * only when the gadget controller failed to find a valid stream event f=
or this
> + * endpoint. On timer expiry, this function calls the endpoint-specific =
timeout
> + * handler registered to endpoint ops->stream_timeout API.
> + */
> +static void usb_ep_stream_timeout(struct timer_list *arg)
> +{
> +	struct usb_ep *ep =3D from_timer(ep, arg, stream_timeout_timer);
> +
> +	if (ep->stream_capable && ep->ops->stream_timeout)

why is the timer only for stream endpoints? What if we want to run this
on non-stream capable eps?

> +		ep->ops->stream_timeout(ep);

you don't ned an extra operation here, ep_dequeue should be enough.

> @@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
>  	if (ret)
>  		goto out;
>=20=20
> +	if (ep->stream_capable)
> +		timer_setup(&ep->stream_timeout_timer,
> +			    usb_ep_stream_timeout, 0);

the timer should be per-request, not per-endpoint. Otherwise in case of
multiple requests being queued, you can't give them different timeouts

> @@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
>=20=20
>  	ret =3D ep->ops->queue(ep, req, gfp_flags);
>=20=20
> +	if (ep->stream_capable) {
> +		ep->stream_timeout_timer.expires =3D jiffies +
> +				msecs_to_jiffies(STREAM_TIMEOUT_MS);

timeout value should be passed by the gadget driver. Add a new
usb_ep_queue_timeout() that takes the new argument. Rename the current
usb_ep_queue() to static int __usb_ep_queue() with an extra argument for
timeout and introduce usb_ep_queue() without the argument, calling
__usb_ep_queue() passing timeout as 0.

> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index e5cd84a..2ebaef0 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -144,6 +144,7 @@ struct usb_ep_ops {
>=20=20
>  	int (*fifo_status) (struct usb_ep *ep);
>  	void (*fifo_flush) (struct usb_ep *ep);
> +	void (*stream_timeout) (struct usb_ep *ep);

why?

=2D-=20
balbi

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

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

iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlvsKdgACgkQzL64meEa
mQalAQ/9EEqb4x4ZVovOafpFs7dKEojviFtYs7DcqBHeEwlQyO0C1tK9pDJ2balL
boEwWvNC3sxVnZE3D+S7rdPspYrt3yP8KfXHNWuOyfQXQzrg4CCXSymSzUKjl9Z9
IX7zA+ok1BIiPNwLKr5JIS2kRoaepMG9IgUb0fQTV42qUkRbzvrrgAPEPumqhgwn
O+7kH0LZ5DPi2Dc0tckBvG9PQ0MiedO6ye7uCXoXPi+tNMKqy36bezH27/hcckBQ
ENo54ABHViy4k0fp1+8nLtGLb7vEGULmrvdna7e+9mkaliFTvzb7bL0G2zLaoHQn
09ywSN14ExtlE1plkdTQsQvCWR0YKCTAtZhZvYJY3G/OQM1FBsrSx+/FXoGOfM7B
YRwb93ZbDI+dVmx/4wEcBhHmBAySi7ibZcsj9ODxdzdcgoRClLHe7sJcfMl4UcV7
Yr8PVrK6z33lXTcyQLyI6Z9sBA7khZ5Vwm72GhqGuC3HbugQLPq5Aq4RpDholvUv
Wdb0biYbgj22b5YoweCq7lY20MDiIGI9f+iXhFluC7A4E84UwGDA0qLhrz6uF7fx
K8N+Ex9Kn9oQvce2jnKBYL1AaATc/u1MwH6otAGQfA9Kg1fPcnOT/Vhwg4ahIr3b
S0WRX2VlM6lTyT+ItzTFMHqx/iXNH2+/hnSiq8jR0qD6G7aaVQ8=
=sVay
-----END PGP SIGNATURE-----
--=-=-=--