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:   Anurag Kumar Vulisha <anuragku@xilinx.com>
To:     Felipe Balbi <balbi@kernel.org>,
        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-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "v.anuragkumar@gmail.com" <v.anuragkumar@gmail.com>,
        Thinh Nguyen <thinhn@synopsys.com>,
        Tejas Joglekar <tejas.joglekar@synopsys.com>,
        Ajay Yugalkishore Pandey <APANDEY@xilinx.com>
Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
 endpoints
Thread-Topic: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capable
 endpoints
Thread-Index: AQHUYvbRwEYgzRXr0k+ojhd/SBDozaVPfhoAgBXv3pA=
Date:   Wed, 28 Nov 2018 16:15:06 +0000
Message-ID: <BL0PR02MB563317DEAFAD7C06B52FC23DA7D10@BL0PR02MB5633.namprd02.prod.outlook.com>
References: <1539436498-24892-1-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <1539436498-24892-2-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <87k1lfiwx3.fsf@linux.intel.com>
In-Reply-To: <87k1lfiwx3.fsf@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 43445c24-abf5-4b61-3915-08d6554ca930
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Nov 2018 16:15:06.3107
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR02MB4420
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Felipe,

Thanks a lot for spending your time in reviewing this patch. Please find
my comments inline

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Wednesday, November 14, 2018 7:28 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Alan Stern <stern@rowland.harvard.edu>; Joha=
n
>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 Jogleka=
r
><tejas.joglekar@synopsys.com>; Ajay Yugalkishore Pandey <APANDEY@xilinx.co=
m>;
>Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capab=
le
>endpoints
>
>
>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,
>>  /* --------------------------------------------------------------------=
----- */
>>
>>  /**
>> + * usb_ep_stream_timeout - callback function for endpoint stream timeou=
t timer
>> + * @arg: pointer to struct timer_list
>> + *
>> + * This function gets called only when bulk streams are enabled in the =
endpoint
>> + * and only after ep->stream_timeout_timer has expired. The timer gets =
expired
>> + * only when the gadget controller failed to find a valid stream event =
for 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?
>

I have seen this issue only with stream capable eps between PRIME & EPRDY. =
But this issue
can potentially occur with non-stream endpoints also. Will remove this stre=
am capable checks
in next series of patch.

>> +		ep->ops->stream_timeout(ep);
>
>you don't ned an extra operation here, ep_dequeue should be enough.
>

I think issuing ep_dequeue() would be more trickier than calling ep->ops->s=
tream_timeout().
This is because, every gadget driver has their own way of handling ep_deque=
ue. Some
drivers (like dwc3) may sleep for an event (wait_event_lock_irq) in the ep_=
dequeue routine.
Since we are calling ep_dequeue from timer timeout callback which is in sof=
tirq context,
sleeping or waiting for an event would hang the system. Also adding ep->ops=
->stream_timeout()
would make the gadget drivers handle the issue in better way based on their=
 implementation.
Another advantage is the drivers which doesn't have this timeout issue does=
n't have to register the
timeout handler and can avoid the logic of deleting the timer for every req=
uest. So, I think
ep->ops->stream_timeout() would be better than having a generic handler whi=
ch does
ep_dequeue() & ep_queue(). Please provide your suggestion on this implement=
ation.

>> @@ -102,6 +132,10 @@ int usb_ep_enable(struct usb_ep *ep)
>>  	if (ret)
>>  		goto out;
>>
>> +	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

I agree with you. Will correct this in next series of patch.

>
>> @@ -269,6 +321,13 @@ int usb_ep_queue(struct usb_ep *ep,
>>
>>  	ret =3D ep->ops->queue(ep, req, gfp_flags);
>>
>> +	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.
>

Thanks for correcting and providing the steps for implementing. Will modify=
 as
suggested in next series of patch.

>> 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 {
>>
>>  	int (*fifo_status) (struct usb_ep *ep);
>>  	void (*fifo_flush) (struct usb_ep *ep);
>> +	void (*stream_timeout) (struct usb_ep *ep);
>
>why?
>

As discussed above, the common timeout handler with ep_dequeue() and
ep_queue() may not work for all the gadget drivers, adding the stream_timeo=
ut()
in ep->ops would enable the drivers to implement their own handler for
handling the stream timeout issue.

Thanks,
Anurag Kumar Vulisha
>--
>balbi