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/SBDozaVPfhoAgBXv3pCAAZCbAIAAMLaA
Date:   Thu, 29 Nov 2018 15:51:47 +0000
Message-ID: <BL0PR02MB563301AF77DA0EDE524F0774A7D20@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>
 <BL0PR02MB563317DEAFAD7C06B52FC23DA7D10@BL0PR02MB5633.namprd02.prod.outlook.com>
 <87h8g0yrl7.fsf@linux.intel.com>
In-Reply-To: <87h8g0yrl7.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: dd13a72a-9a33-4d04-e489-08d656129224
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Nov 2018 15:51:47.9388
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR02MB4388
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,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Thursday, November 29, 2018 6:22 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>
>Subject: RE: [PATCH V6 01/10] usb: gadget: udc: Add timer for stream capab=
le
>endpoints
>
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>>>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/co=
re.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 time=
out
>timer
>>>> + * @arg: pointer to struct timer_list
>>>> + *
>>>> + * This function gets called only when bulk streams are enabled in th=
e endpoint
>>>> + * and only after ep->stream_timeout_timer has expired. The timer get=
s expired
>>>> + * only when the gadget controller failed to find a valid stream even=
t for this
>>>> + * endpoint. On timer expiry, this function calls the endpoint-specif=
ic 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 stream capable checks in next series of patch.
>
>you're adding support for transfer timeouts, which some gadgets may want
>regardless of endpoint type. One use is to correct cases of streams
>going out of sync.
>

Thanks for making me understand, will implement your suggestions and resend
the patches soon.

Best Regards,
Anurag Kumar Vulisha
=20
>>>> +		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=
-
>>stream_timeout().
>> This is because, every gadget driver has their own way of handling ep_de=
queue.
>Some
>> drivers (like dwc3) may sleep for an event (wait_event_lock_irq) in the =
ep_dequeue
>routine.
>
>not anymore. There's, now, a requirement that ->dequeue can be called
>from any context.
>
>> Since we are calling ep_dequeue from timer timeout callback which is in =
softirq
>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 th=
eir
>implementation.
>
>no problems
>
>> Another advantage is the drivers which doesn't have this timeout issue d=
oesn't have
>to register the
>> timeout handler and can avoid the logic of deleting the timer for every =
request. So, I
>think
>> ep->ops->stream_timeout() would be better than having a generic handler =
which
>does
>> ep_dequeue() & ep_queue(). Please provide your suggestion on this
>implementation.
>
>call ep_dequeue()
>
>--
>balbi