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:     Alan Stern <stern@rowland.harvard.edu>
CC:     Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Shuah Khan <shuah@kernel.org>, Johan Hovold <johan@kernel.org>,
        Jaejoong Kim <climbbb.kim@gmail.com>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Roger Quadros <rogerq@ti.com>,
        Manu Gautam <mgautam@codeaurora.org>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Mike Christie <mchristi@redhat.com>,
        Matthew Wilcox <willy@infradead.org>,
        Colin Ian King <colin.king@canonical.com>,
        "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 v7 01/10] usb: gadget: udc: Add timer support for usb
 requests
Thread-Topic: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb
 requests
Thread-Index: AQHUiWbrxVzoMt2EkU67+RgGKmmQY6Vrp4EAgAEXBVCAAF3jgIAAAp+AgACIUoCAAM7bwIAAWNWAgAAcRaCAABENAIABKdpQ
Date:   Wed, 5 Dec 2018 15:43:41 +0000
Message-ID: <BL0PR02MB563386B88BDC306AF44EE736A7A80@BL0PR02MB5633.namprd02.prod.outlook.com>
References: <SN6PR02MB56484B8D1F891926DA7D7900A7AF0@SN6PR02MB5648.namprd02.prod.outlook.com>
 <Pine.LNX.4.44L0.1812041420200.1537-100000@iolanthe.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1812041420200.1537-100000@iolanthe.rowland.org>
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: 670710c9-b766-4ed1-f107-08d65ac86ef2
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Dec 2018 15:43:41.6504
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR02MB4276
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 Alan,

>-----Original Message-----
>From: Alan Stern [mailto:stern@rowland.harvard.edu]
>Sent: Wednesday, December 05, 2018 12:59 AM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>
>Cc: Felipe Balbi <balbi@kernel.org>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>; Shuah Khan <shuah@kernel.org>; Johan Hovold
><johan@kernel.org>; Jaejoong Kim <climbbb.kim@gmail.com>; Benjamin
>Herrenschmidt <benh@kernel.crashing.org>; Roger Quadros <rogerq@ti.com>; M=
anu
>Gautam <mgautam@codeaurora.org>; martin.petersen@oracle.com; Bart Van
>Assche <bvanassche@acm.org>; Mike Christie <mchristi@redhat.com>; Matthew
>Wilcox <willy@infradead.org>; Colin Ian King <colin.king@canonical.com>; l=
inux-
>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.co=
m>
>Subject: RE: [PATCH v7 01/10] usb: gadget: udc: Add timer support for usb =
requests
>
>On Tue, 4 Dec 2018, Anurag Kumar Vulisha wrote:
>
>> >Is there any way for the device controller driver to detect the problem=
 without
>relying
>> >on a timeout?
>> >
>> >In fact, is this problem a known bug in the existing device controller =
hardware?
>Have
>> >the controller manufacturers published a suggested scheme for working a=
round it?
>> >
>>
>> Yes, this problem is mentioned in dwc3 controller data book and the work=
around
>> mentioned  is the same that we are doing , to implement timeout and if n=
o valid
>> stream event is found , after timeout issue end transfer followed by sta=
rt transfer.
>
>Okay.  But this implies that the problem is confined to DWC3 and
>doesn't affect other types of controllers, which would mean modifying
>the UDC core would be inappropriate.
>

Both host & gadget are equally responsible for this issue and it may potent=
ially occur
with other controllers also(which are incapable of handling this situation)=
 . Because of
this reason I would not call this issue as a dwc3 alone bug. One thing is t=
hat, with dwc3 the
issue is easily reproduced. Because of these reasons I feel that it would b=
e better to have
a generic udc timers rather than having driver specific workaround. We had =
this same
discussion earlier in this mailing list https://lkml.org/lkml/2018/10/9/638

>Does the data book suggest a value for the timeout?
>

No, the databook doesn't mention about the timeout value

>> >At this point, it seems that the generic approach will be messier than =
having every
>> >controller driver implement its own fix.  At least, that's how it appea=
rs to me.
>
>(Especially if dwc3 is the only driver affected.)
>

As discussed above, the issue may happen with other gadgets too. As I got d=
ivide opinions
on this implementation and both the implementations looks fine to me, I am =
little confused
on which should be implemented.

@Felipe: Do you agree with Alan's implementation? Please let us know your s=
uggestion
on this.

>> With the dequeue approach there is an advantage(not related to this issu=
e) that the
>> class driver can have control of the timed out transfer whether to reque=
ue it or
>consider
>> it as an error (based on implementation). This approach gives more flexi=
bility to the
>class
>> drivers. This may be out of context of this issue but wanted to point ou=
t that we
>may lose
>> this advantage on switching to older implementation.
>
>Class drivers can cancel and requeue requests at any time.  There's no
>extra flexibility.
>

I agree with you, but the class drivers have to implement their own logic i=
nstead of
having a generic logic to implement the timeouts.=20

>> >Ideally it would not be necessary to rely on a timeout at all.
>> >
>> >Also, maintainers dislike module parameters.  It would be better not to=
 add one.
>>
>> Okay. I would be happy if any alternative for this issue is present but =
unfortunately
>> I am not able to figure out any alternative other than timers. If not
>module_params()
>> we can add an configfs entry in stream gadget to update the timeout. Ple=
ase
>provide
>> your opinion on this approach.
>
>Since the purpose of the timeout is to detect a deadlock caused by a
>hardware bug, I suggest a fixed and relatively short timeout value such
>as one second.  Cancelling and requeuing a few requests at 1-second
>intervals shouldn't add very much overhead.
>

Thanks for suggesting. 1 sec timeout should be fair enough. I will change t=
his
in next series=20

Best Regards,
Anurag Kumar Vulisha