From mboxrd@z Thu Jan 1 00:00:00 1970 From: Martin Kelly Subject: Re: CAN-USB adapter unplug Date: Tue, 5 Dec 2017 15:42:03 -0800 Message-ID: References: <20171127234916.25865-1-mkelly@xevo.com> <48029f4a-5137-5f1f-6970-5668fb8da599@pengutronix.de> <2f999f02-028e-1e6c-9020-beb5a76889cc@esd.eu> <4f6687db-269d-d8ff-7a47-5094ad63043a@xevo.com> <29dbc892-e468-e828-3f5b-78153930d540@kvaser.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Return-path: Received: from mail-dm3nam03on0052.outbound.protection.outlook.com ([104.47.41.52]:30240 "EHLO NAM03-DM3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752996AbdLEXmK (ORCPT ); Tue, 5 Dec 2017 18:42:10 -0500 In-Reply-To: <29dbc892-e468-e828-3f5b-78153930d540@kvaser.com> Content-Language: en-US Sender: linux-can-owner@vger.kernel.org List-ID: To: Jimmy Assarsson , Marc Kleine-Budde , "linux-can@vger.kernel.org" Cc: =?UTF-8?Q?Stefan_M=c3=a4tje?= , =?UTF-8?B?UmVtaWdpdXN6IEtvxYLFgsSFdGFq?= , Wolfgang Grandegger On 12/04/2017 12:39 AM, Jimmy Assarsson wrote: > On 2017-11-30 19:19, Martin Kelly wrote: >> On 11/30/2017 02:18 AM, Jimmy Assarsson wrote: >>> On 2017-11-29 18:07, Martin Kelly wrote: >>>> On 11/29/2017 07:21 AM, Stefan Mätje wrote: >>>>> Am 29.11.2017 um 13:20 schrieb Marc Kleine-Budde: >>>>>> On 11/28/2017 10:09 PM, Martin Kelly wrote: >>>>>>>> Both applied to can. >>>>>>> >>>>>>> Thanks! By the way as far as I can tell from code inspection, it >>>>>>> appears >>>>>>> that most of the other drivers in net/can/usb should have the same >>>>>>> disconnect bug. gs_usb appears to be clear, as it returns in its >>>>>>> default >>>>>>> case. Unfortunately mcba_usb is the only device I have to test >>>>>>> with, but >>>>>>> those with other devices may want to check for this. >>>>>> >>>>>> Can you create patches for the affected drivers and send them to the >>>>>> list and the maintainers of the driver on Cc? >>>>>> >>>>>> I don't have access to every USB adapter neither. >>>>>> >>>>>> Marc >>>>>> >>>>> >>>>> Hi, >>>>> >>>>> I have seen Martin's emails these days and tried to reproduce the >>>>> error here >>>>> with an esd CAN-USB/2 device (handled by esd_usb2.c). The only >>>>> thing I get >>>>> in the log are some messages like this: >>>>> >>>>> kernel: [14776.152459] usb 2-1: Rx URB aborted (-71) >>>>> >>>>> There is no endless loop. How could I reproduce the bad behavior? >>>>> For the >>>>> quick test I used an Ubuntu 4.4.0-101-generic kernel. >>> >>> Hi, >>> >>> I got same result for kvaser_usb, a single "Rx URB aborted (-71)". >>> When tested on Ubuntu with 4.4.0-93-generic. >>> >>>> Interesting. Do you see just a few -71 RX URB aborted messages (one >>>> per outstanding URB) rather than an endless loop? If so, then I >>>> think everything is OK on that device, as the URBs are not being >>>> resubmitted. >>>> >>>> In case it helps, my test case for the mcba_usb is on a Raspberry Pi >>>> 3. I don't know whether or not that could influence the USB error >>>> code we see, since you are seeing EPROTO instead of EPIPE when the >>>> device gets unplugged. >>> >>> However, I do get lots of (500+) "Rx URB aborted (-71)" printouts, on >>> Raspberry Pi 3, running Raspbian with kernel 4.9.35-v7. But no EPIPE >>> seen. >>> >> >> Very interesting; this means that there are multiple possible error >> codes from a USB disconnect. If that's the case, is it possible that >> the best thing to do is what gs_usb does? In gs_usb's receive >> callback, we have: >> >>      switch (urb->status) { >>      case 0: /* success */ >>          break; >>      case -ENOENT: >>      case -ESHUTDOWN: >>          return; >>      default: >>          /* do not resubmit aborted urbs. eg: when device goes down */ >>          return; >>      } >> >> The default case is to *not* resubmit URBs, rather than to resubmit is >> a default and try to catch all possible error codes in the non-default >> case, as the other drivers are doing. >> >> If we don't do something like this, then we need to understand all the >> possible error codes that could occur and catch them all. > > Well, gs_usb never resubmit any URBs if urb->status is non-zero. As long > as any error (urb->status != 0) is the result of a disconnect, it should > be safe to never resubmit? I doubt this is the case, but I really don't > know. > Yeah, it's unclear to me whether what gs_usb does is safe. I have kept my patches just using -EPIPE/-EPROTO rather than a catch-all default, just to be safe. >>> Regards, >>> Jimmy >>> >>>>> >>>>> In any case I will add the patch handling EPIPE on a test system >>>>> and have a >>>>> look what it might change. >>>>> >>>>> Regards, >>>>> Stefan Mätje