Re: Support Mac address pass through on Dell DS1000 dock
On Do, 2018-11-22 at 12:06 +0100, Frédéric Parrenin wrote:
> Le 22/11/2018 à 11:22, Oliver Neukum a écrit :
> > On Mi, 2018-11-21 at 16:50 +0100, Frédéric Parrenin wrote:
> > > which over rides the Mac on the dock. So, the answer is, "it's not an
> > > issue if the dock supports it, as the laptop BIOS is what is determining
> > > if it is supported".
> > >
> > > So what is the truth?
> >
> > For pass thru you must meet multiple conditions:
> >
> > /* if this is not an RTL8153-AD, no eFuse mac passthru set,
> > * or system doesn't provide valid _SB.AMAC this will be
> > * be expected to non-zero
> > */>
> >
> > They can be manually verified. Do you need a debugging patch?
>
> OK, let us try a debugging patch.
Here you go.
PLease enable dynamic debugging for the driver and ramp up
your logging level.
Regards
Oliver
From 3661ff35782f7b26df3204f4f7a18929e0c74ff7 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 22 Nov 2018 18:08:43 +0100
Subject: [PATCH] rtl8152: debugging for MAC pass-through
This adds debugging statements for failure cases in MAC
pass-through
Signed-off-by: Oliver Neukum
---
drivers/net/usb/r8152.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 7345a2258ee4..3ed52806164c 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -1168,19 +1168,28 @@ static int vendor_mac_passthru_addr_read(struct r8152 *tp, struct sockaddr *sa)
/* test for -AD variant of RTL8153 */
ocp_data = ocp_read_word(tp, MCU_TYPE_USB, USB_MISC_0);
- if ((ocp_data & AD_MASK) != 0x1000)
+ if ((ocp_data & AD_MASK) != 0x1000) {
+ netif_dbg(tp, probe, tp->netdev,
+"Not an AD type.\n");
return -ENODEV;
+ }
/* test for MAC address pass-through bit */
ocp_data = ocp_read_byte(tp, MCU_TYPE_USB, EFUSE);
- if ((ocp_data & PASS_THRU_MASK) != 1)
+ if ((ocp_data & PASS_THRU_MASK) != 1) {
+ netif_dbg(tp, probe, tp->netdev,
+"pass-through bit not set.\n");
return -ENODEV;
+ }
/* returns _AUXMAC_#AABBCCDDEEFF# */
status = acpi_evaluate_object(NULL, "\\_SB.AMAC", NULL, );
obj = (union acpi_object *)buffer.pointer;
- if (!ACPI_SUCCESS(status))
+ if (!ACPI_SUCCESS(status)) {
+ netif_warn(tp, probe, tp->netdev,
+"pass-through firmware failure.\n");
return -ENODEV;
+ }
if (obj->type != ACPI_TYPE_BUFFER || obj->string.length != 0x17) {
netif_warn(tp, probe, tp->netdev,
"Invalid buffer for pass-thru MAC addr: (%d, %d)\n",
--
2.16.4
Re: Support Mac address pass through on Dell DS1000 dock
On Mi, 2018-11-21 at 16:50 +0100, Frédéric Parrenin wrote:
>
> which over rides the Mac on the dock. So, the answer is, "it's not an
> issue if the dock supports it, as the laptop BIOS is what is determining
> if it is supported".
>
> So what is the truth?
For pass thru you must meet multiple conditions:
/* if this is not an RTL8153-AD, no eFuse mac passthru set,
* or system doesn't provide valid _SB.AMAC this will be
* be expected to non-zero
*/>
They can be manually verified. Do you need a debugging patch?
Regards
Oliver
Re: [PATCH v2] usb: hub: add retry routine after intr URB sumbit error
On Mo, 2018-11-19 at 15:02 +0100, Nicolas Saenz Julienne wrote:
>
> +static void hub_retry_irq_urb(struct timer_list *t)
> +{
> + struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
> + int status;
> +
> + if (hub->disconnected || hub->quiescing)
> + return;
> +
> + dev_err(hub->intfdev, "retrying int urb\n");
> + status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> + if (status && status != -ENODEV && status != -EPERM &&
> + status != -ESHUTDOWN)
> + mod_timer(>irq_urb_retry,
> + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> +}
> +
> static void kick_hub_wq(struct usb_hub *hub)
> {
> struct usb_interface *intf;
> @@ -713,8 +729,12 @@ static void hub_irq(struct urb *urb)
> return;
>
> status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> - if (status != 0 && status != -ENODEV && status != -EPERM)
> + if (status != 0 && status != -ENODEV && status != -EPERM &&
> + status != -ESHUTDOWN) {
> dev_err(hub->intfdev, "resubmit --> %d\n", status);
> + mod_timer(>irq_urb_retry,
> + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> + }
> }
>
> /* USB 2.0 spec Section 11.24.2.3 */
> @@ -1268,6 +1288,7 @@ static void hub_quiesce(struct usb_hub *hub, enum
> hub_quiescing_type type)
> }
>
> /* Stop hub_wq and related activity */
> + del_timer_sync(>irq_urb_retry);
That is a race condition. You kill the timer here, but the URB may
still be in flight. And if it fails, it will restart the error
handler. You have to introduce a flag or poison the URB.
> usb_kill_urb(hub->urb);
> if (hub->has_indicators)
> cancel_delayed_work_sync(>leds);
>
Regards
Oliver
Re: [PATCH] usbnet: use tasklet_init() for usbnet_bh handler
On Fr, 2018-11-16 at 15:50 +, Ben Dooks wrote: > The tasklet initialisation would be better done by tasklet_init() > instead of assuming all the fields are in an ok state by default. > > Note, this does not fix any known bug. > > Signed-off-by: Ben Dooks Acked-by: Oliver Neukum
Re: [PATCH] usb: hub: add I/O error retry & reset routine
On Do, 2018-11-15 at 18:14 +0100, Nicolas Saenz Julienne wrote:
Hi,
what Alan said, in addition you need to stop the error handling
when the device is suspended or reset.
> @@ -713,8 +751,10 @@ static void hub_irq(struct urb *urb)
> return;
>
> status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> - if (status != 0 && status != -ENODEV && status != -EPERM)
> + if (status != 0 && status != -ENODEV && status != -EPERM) {
This also needs to check for -ESHUTDOWN
Regards
Oliver
Re: Query on usb/core/devio.c
On Do, 2018-11-15 at 12:45 +, Mayuresh Kulkarni wrote: > > Understood this for remote-wake part. > > But still unclear about step (1) for host-wake as below (please note, I am > refering to host-wake and remote-wake as per my previous comment) - > Pre-condition: device in suspend and link in L2. > Use-case: end user wants to sends a control message to device. > Assumption: end user is NOT doing any activity that causes remote-wake. That is just an assumption you cannot make. Users are devious creatures. If it is physically possible, you have to be able to deal with it. Regards Oliver
Re: Query on usb/core/devio.c
On Mo, 2018-11-12 at 12:04 +, Mayuresh Kulkarni wrote: > I think I now understand the disconnect between us this point. Below is an > attempt to bridge that, so please bear with me: > 1. In our use-case(s), the end user can "interact" with composite USB device > either by physically interacting with or via an app on Android. > 2. When the interaction is "physical" (e.g.: something that is sensed by a > sensor or a button press), we need "remote-wake" from USB device to host. > 3. When the interaction is via app (e.g.: to toggle some control), we need > "host-wake" from host to USB device. All of these events can occur at the same time. And you cannot prevent that. > And this is where, the knowledge of wake-up source and difference in handling > comes into picture. > For (2), the host needs to first wake-up and then queue a request to read-out > "what" happened. > For (3), the host needs to first wake-up the device and then queue the > command(s) to the take action asked by the end user. > > I agree that in both of above cases, "a USB request" needs to be send from > user- > space to kernel to device. But in my opinion, each of the them has a different > context associated with it. E.g.: for (2), a single read request is sufficient > to "know" what happened while for (3), a write and a read request is needed. There is a natural race condition between remote and host wake up. You cannot rule out that your app wakes the device up, exactly when user interaction would have woken it anyway. Your app will know what it wants to do with the device. And you can detect that. Hence the correct algorithm is: 1. wake up the device 2. query it for a cause of wake up 3. If the host wants to do IO, do all of it. Regards Oliver
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
On Do, 2018-11-08 at 12:47 +0200, Felipe Balbi wrote:
Hi,
> Oliver Neukum writes:
> > On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
> > >
> > > --- a/drivers/usb/dwc3/core.h
> > > +++ b/drivers/usb/dwc3/core.h
> > > @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
> > > * @setup_packet_pending: true when there's a Setup Packet in FIFO.
> > > Workaround
> > > * @three_stage_setup: set if we perform a three phase setup
> > > * @usb3_lpm_capable: set if hadrware supports Link Power Management
> > > + * @usb2_lpm_disable: set to disable usb2 lpm
> > > * @disable_scramble_quirk: set if we enable the disable scramble quirk
> > > * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
> > > * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> > > @@ -1146,6 +1147,7 @@ struct dwc3 {
> > > unsignedsetup_packet_pending:1;
> > > unsignedthree_stage_setup:1;
> > > unsignedusb3_lpm_capable:1;
> > > + unsignedusb2_lpm_disable:1;
> >
> > Hi,
> >
> > that may be a bit late, but why would this be a property of dwc3?
> > Now, you may want to do this for a specific controller,
> > but there is no reason to limit the flag to dwc3. We want this
> > flag in the generic HCD attributes, so that other HCDs can share
> > it. Maybe even expose it to sysfs.
>
> this is used for the peripheral side of dwc3 too.
same argument. Whether a gadget supports LPM is a question
in no way specific to dwc3. And whether this exposes internal
registers does not really matter. It is a capability of the HC
for a generic issue.
Regards
Oliver
Re: USB Bluetooth dongle stop response with timeout error
On Mi, 2018-11-07 at 13:20 +0800, Morikazu Fumita wrote: Hi, > Hello Oliver, > > I got rid of the network bridge but the timeout error still happens so I > can rule out the bridge now. Good. > I also got USB packet dump and found that the error is happening > regardless of HCI commands. > > One example is below. I just inserted the USB dongle when I got the > following errors. > [ 145.046503] Bluetooth: hci0: command 0x1002 tx timeout > [ 147.086503] Bluetooth: hci0: command 0x0c52 tx timeout > [ 149.121499] Bluetooth: hci0: command 0x0c45 tx timeout > [ 151.166503] Bluetooth: hci0: command 0x0c58 tx timeout > > Please find the USB packet dump attached. I have trouble reading this. How did you make it? What does it view with? > In frame no. 161, the host sent HCI command 0x1002 (Read Local Supported > Commands). > Then the USB dongle tried to respond to that from frame no. 163 but it > did not sent all fragment packets. It seems to stop response in the middle. > Finally, above "0x1002 tx timeout" happened. Did you try to repeat this from cold boot? > As for frame no. 165, the host sent 0x0c52 (Write Extended Inquiry > Response) but there was no response to it then above "0x0c52 tx timeout" > happened. > > This is just one example. The USB dongle can be successfully inserted > and working for a while but suddenly stops response to HCI commands like > this example. > It seems there is no specific HCI commands to cause the problem. > > I do not find out what is the trigger of it yet. > Do you have any thoughts from this point? Have you tried ruling out LPM and other runtime PM? This looks like a bluetooth issue, not specifically USB so we are kind of the wrong mailing list. There is one for Bluetooth. Regards Oliver
Re: Adding NovAtel USB vendor & device ID to Kernel
On Do, 2018-11-08 at 01:07 +, SNELL James wrote: > Hello, > We produce extremely high-end GNSS (GPS, etc) receivers that are often used > for a very wide range of applications. Our receivers can be connected to via > USB, which will provide 3 USB-to-serial ports that can be used to issue > commands and get receiver data. > > We typically get Linux users to create a udev file so their systems attach > the USB serial ports to /dev. Hi, thank you for the extensive bug report. AFAICT you are unlucky enough to run into three separate but related issues 1. your udev rule does not work Your idea is basically correct, but for udev issues you are on the wrong mailing list. 2. Your company's name is wrong I looked at the most recent usb.ids http://www.linux-usb.org/usb.ids and it looks correct to me: 09d7 NovAtel Inc. 0100 NovAtel FlexPack GPS receiver Please double check this and the date of your usb.ids file 3. the kernel message > I just noticed that when my receiver enumerates, dmesg outputs: > [ 414.374523] usb 1-1.1.3: new full-speed USB device number 8 using dwc_otg > [ 414.508473] usb 1-1.1.3: New USB device found, idVendor=09d7, > idProduct=0100 > [ 414.508488] usb 1-1.1.3: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 414.508497] usb 1-1.1.3: Product: NovAtel GPS Receiver > [ 414.508505] usb 1-1.1.3: Manufacturer: NovAtel Inc. > [ 414.508514] usb 1-1.1.3: SerialNumber: DMGW18050122R > [ 414.511608] usbserial_generic 1-1.1.3:1.0: The "generic" usb-serial driver > is only for testing and one-off prototypes. > [ 414.511624] usbserial_generic 1-1.1.3:1.0: Tell > mailto:linux-usb@vger.kernel.org to add your device to a proper driver. > [ 414.511636] usbserial_generic 1-1.1.3:1.0: generic converter detected > [ 414.512004] usb 1-1.1.3: generic converter now attached to ttyUSB0 > [ 414.512352] usb 1-1.1.3: generic converter now attached to ttyUSB1 > [ 414.512805] usb 1-1.1.3: generic converter now attached to ttyUSB2 > Indeed the kernel ought to be patched for this. You need to tell us which driver your devices is best served by, so that your device ID can be added to the correct driver. Regards Oliver
Re: [PATCH 3/3] usb: dwc3: Support option to disable USB2 LPM
On Mi, 2018-11-07 at 18:10 -0800, Thinh Nguyen wrote:
>
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -971,6 +971,7 @@ struct dwc3_scratchpad_array {
> * @setup_packet_pending: true when there's a Setup Packet in FIFO.
> Workaround
> * @three_stage_setup: set if we perform a three phase setup
> * @usb3_lpm_capable: set if hadrware supports Link Power Management
> + * @usb2_lpm_disable: set to disable usb2 lpm
> * @disable_scramble_quirk: set if we enable the disable scramble quirk
> * @u2exit_lfps_quirk: set if we enable u2exit lfps quirk
> * @u2ss_inp3_quirk: set if we enable P3 OK for U2/SS Inactive quirk
> @@ -1146,6 +1147,7 @@ struct dwc3 {
> unsignedsetup_packet_pending:1;
> unsignedthree_stage_setup:1;
> unsignedusb3_lpm_capable:1;
> + unsignedusb2_lpm_disable:1;
Hi,
that may be a bit late, but why would this be a property of dwc3?
Now, you may want to do this for a specific controller,
but there is no reason to limit the flag to dwc3. We want this
flag in the generic HCD attributes, so that other HCDs can share
it. Maybe even expose it to sysfs.
Regards
Oliver
Re: USB Bluetooth dongle stop response with timeout error
On Mi, 2018-10-31 at 12:32 +0800, Morikazu Fumita wrote: > > My test procedure is below (assuming Bluetooth devices are already paired). > > 1. Adding a network bridge for PAN using "brctl". > 2. Link the bridge up. > 3. Run "hciconfig hci0 up" to power the USB Bluetooth dongle up. > 4. Register "nap" service and the network bridge to > "org.bluez.NetworkServer1" > via d-bus using a Python script. > 5. Bluez makes "bnep0" virtual network interface automatically. > 6. Connect to the PAN-NAP server from the client. Network is working > fine at > this point. > (Note: The hang does not happen even if turning promiscuous mode off at > this > point by running "ip link set bnep0 promisc off") > 7. Disconnect PAN from the client or make the "bnep0" virtual network > adapter > down by running "ip link set bnep0 down". > 8. The hang happens with "Bluetooth: hci0: command 0x tx timeout" > errors. > 9. No response from the USB Bluetooth dongle anymore. > For example, running "discoverable on" from "bluetoothctl" fails with error > message of "Failed to set discoverable on: org.bluez.Error.Failed". > The timeout error is logged in "dmesg" as well. > > From this fact, I believe this issue is related to Bluez but not to USB. > What do you think? Hi, you are using a slightly less common HC, are you not? Anyway, it seems to me that you can narrow this down a bit further. You are using bridging. You can try to remove that. And after that you can take an usbmon trace right as you give the command that crashes the device. The procedure is described in the kernel's Documentation directory. That will allow to determine which exact command is the problem. HTH Oliver
Re: USB Bluetooth dongle stop response with timeout error
On Sa, 2018-10-27 at 22:56 +0800, Morikazu Fumita wrote: > I always found error messages of > "Bluetooth: hci0: command 0x0406 tx timeout" and The problem is likely shortly before that. > "dwc2 ffb4.usb: --Host Channel x Interrupt: Frame Overrun--" > when this problem is happening. > > Here's an example log: > [ 251.748320] pan: port 1(bnep0) entered disabled state > [ 251.752153] device bnep0 left promiscuous mode > [ 251.754541] pan: port 1(bnep0) entered disabled state > [ 257.879162] Bluetooth: hci0: command 0x0406 tx timeout > Oct 25 04:47:14 cyclone5 systemd-networkd[132]: bnep0: Lost carrier > Oct 25 04:47:14 cyclone5 kernel: pan: port 1(bnep0) entered disabled state > Oct 25 04:47:14 cyclone5 kernel: dwc2 ffb4.usb: --Host Channel 15 > Interrupt: Frame Overrun-- You need to find out under which circumstances the hang happens. Is it always when leaving the promiscuous mode? Regards Oliver
Re: Query on usb/core/devio.c
On Do, 2018-10-18 at 13:42 -0400, Alan Stern wrote: > On Thu, 18 Oct 2018, Mayuresh Kulkarni wrote: > > > > The only way to make the ioctl work properly is to have it do a > > > runtime-PM put at the start and then a runtime-PM get before it If and only if you want to do this with one ioctl() If you separate the runtime-PM put and the get, you can do it without the waiting part. > > > returns. This is true regardless of the reason for returning: normal > > > termination, timeout, signal, whatever. Nothing else would be safe. > > > > > > > Will below steps work safely (sometimes pseudo-code/snippets help to > > align)? - > > > > "new" ioctl - > > > > timeout is parameter to ioctl. > > > > /* attempt suspend of device */ > > usb_autosuspend_device(dev); > > > > usb_unlock_device(dev); > > r = wait_event_interruptible_timeout(ps->resume_wait, > > (ps->resume_done == true), timeout * HZ); > > Not exactly. The condition to test here is whether the device has been > suspended, so it's more like this: > > r = wait_event_interruptible_timeout(ps->suspend_wait, > (ps->suspend_done == true), timeout * HZ); > > where ps->suspend_done is set by the runtime_suspend callback. After > this we will do: > > if (r > 0) /* Device suspended before the timeout expired */ > r = wait_event_interruptible(ps->resume_wait, > (ps->resume_done == true)); > > > usb_lock_device(dev); > > > > /* > > * There are 3 possibilities here: > > * 1. Device did suspend and resume (success) > > * 2. Signal was received (failed suspend) > > * 3. Time-out happened (failed suspend) > > 4. Device did suspend but a signal was received before the device > resumed. > > > * In any of above cases, we need to resume device. > > */ > > usb_autoresume_device(dev); Yes and that is the problem. Why do you want to wait for the result of runtime-PM put ? If we need a channel for notifying user space about resume of a device, why wait for the result of suspend instead of using the same channel? > > > > ps->resume_done = false; > > > > "ps->resume_done = true;" will be done by the runtime resume call-back. No. You cannot do that in this way. It needs to be a unified device state or a sequence of multiple suspends and resumes will have strange results. Regards Oliver
Re: Query on usb/core/devio.c
On Di, 2018-10-16 at 10:46 -0400, Alan Stern wrote: > On Tue, 16 Oct 2018, Mayuresh Kulkarni wrote: > > > 1. User space decides when it is time to suspend the device and calls > > this ioctl. The implmentation takes care to ensure, no URB is in > > flight from this caller to this device. Then proceed with suspend. > > Well, I did not imagine the ioctl would try to ensure that no URBs are > in flight. The user would be responsible for that. Well, we have to check for URBs in flight. In fact we would have to "plug" the device against new URBs. Now, we could use a new counter. But if we check asynclist, we may just as well walk it and kill the URBs. It just seems a bit silly not to do that. > Also, proceeding with the suspend is difficult, as Oliver has pointed > out. There is no guarantee that the device will go into suspend, > merely because the userspace driver isn't accessing it. (In fact, > there's no guarantee that the device will _ever_ go into suspend.) The > ioctl would probably have to include some sort of timeout; it would > return early if the timeout expired before the device was suspended. Exactly. The alternative is to have an ioctl() to just allow or block suspend, more or less just exporting autopm_get/put(). The disadvantage is that 1. The kernel has to cancel URBs in flight 2. There needs to be a mechanism to notify user space about events > > 2. In an another thread, the user-space calls poll(USB-device-fd). > > When poll() returns, that means the device is active now. User space > > can then request an URB to read out the async notification, if any. > > No; no other thread or polling is needed. The ioctl call returns when > the device resumes. At that point the user program can check whether > there is an async notification pending. This is problematic. For example, what do we do if a signal needs to be delivered? The obvious solution of just repeating the call will not work. It races with wakeup. You'd need to restart IO to query the device. But the device may be suspended. Or do you want a signal to up the PM counter again? Making Ctrl-C wake up an unrelated device? What do we do in case of a failed suspend or resume? How about reset_resume? > > If this is correct interpretation, what will happen when USB device > > sends remote-wake to host, but the reason is not async notification > > but something other than that (e.g.: something standard UAC or HID)? > > The reason for the wakeup won't make any difference; the behavior would > be the same regardless. Exactly. Anything else is a race that can drop events. Regards Oliver
Re: Query on usb/core/devio.c
On Di, 2018-10-16 at 12:10 +0100, Mayuresh Kulkarni wrote: > On Mon, 15 Oct 2018 09:50:46 -0400 > Alan Stern wrote: > > > It seems that a better approach would be to have an ioctl which would: > > > > fail if there are any active user URBs; > > > > drop the usbfs PM reference so the device can suspend; > > > > block interruptibly until the device resumes; > > > > reacquire the PM reference (forcing the device to resume) > > if the ioctl call is interrupted. > > > > Please correct me if wrong, but as I understand, with this new ioctl > proposal, below should be possible - > 1. User space decides when it is time to suspend the device and calls this > ioctl. The implmentation takes care to ensure, no URB is in flight from this > caller to this device. Then proceed with suspend. > The exact semantics this ioctl() should have is what we are currently discussing. > 2. In an another thread, the user-space calls poll(USB-device-fd). When > poll() returns, that means the device is active now. User space can then > request an URB to read out the async notification, if any. That is one of the designs we are looking at. > 3. Compatibility is maintained with current user-space implementation as only > "newer" user-space will call this new ioctl. Yes. > The USB device can suspend between (1) and (2). Yes. > If this is correct interpretation, what will happen when USB device sends > remote-wake to host, but the reason is not async notification but something > other than that (e.g.: something standard UAC or HID)? Here, the URB will be > queued forcing the link to be active (and we probably land into same issue). Remote wakeup does not carry information with it. In fact a wake up from the host can race with remote wake up. Any wake up would require a user space driver to do IO with the device and query it for a reason to wake up. > In such a case, is it expected from user-space to dequeue the URB after a > while and follow (1) and (2) above? Yes. > Apologies if this is implied but I am just trying to get my head around with > the proposal, hence being verbose. Please feel free and encouraged to state your requirements as clearly and verbosely as you like. It would suck to introduce an API only to find that it cannot do the job. Regards Oliver
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
On Di, 2018-10-16 at 10:41 +0200, Julian Xhokaxhiu wrote:
> Good morning Oliver,
>
> thank you very much for the reply. I will try to build a test kernel
> hopefully later this evening and I'll get back to you.
>
> Meanwhile you can see in the attachment in my first email the output
> of `$ lsusb -v -d 152d:1561`
That is the problem. I just cannot find a quirk for that device.
And the generic ASMedia code should not match.
> Is there a way to test UAS without recompiling the Kernel? Like a
> Kernel argument or modprobe option?
Unfortunately the existing option allow setting US_FL_IGNORE_UAS
but not clearing it. You need to recompile.
Regards
Oliver
Re: Query on usb/core/devio.c
On Mo, 2018-10-15 at 09:50 -0400, Alan Stern wrote: > > It seems that a better approach would be to have an ioctl which would: > > fail if there are any active user URBs; > > drop the usbfs PM reference so the device can suspend; > > block interruptibly until the device resumes; Thus we would require user space to have a thread for each device it wants to allow suspend for. [..] > What difference does it make if the URBs are killed by the user instead > of the kernel? We stay within the limits of the timing (as well as we can) and the case of a failed suspend is much easier to handle. Else we have an unlimited time between cessation of IO and going into suspend. > > And notify user space when the device is resumed. IMHO select() > > is the syscall designed for that. > > Apart from the difficulties mentioned above. Yes, I see. Regards Oliver
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
On Mo, 2018-10-15 at 14:12 +0200, Julian Xhokaxhiu wrote:
> Hi Oliver,
>
> I'm currently using the latest 4.18.12 mainline ( on Arch
> https://www.archlinux.org/packages/core/x86_64/linux/ ), and yes
> you're right I am NOT using UAS at the moment. The link I left is
> because I noticed those errors on dmesg, and I thought I had it
> enabled. This is why I am writing to you right now :)
>
> I personally think the SSD sometimes hangs because TRIM commands are
> not sent properly to the adapter. Although I'm not 100% sure, this is
> why I would like to test UAS.
>
> Can you please point me to the right instructions for trying UAS on my
> adapter? So I can report back the outcome.
>
> Also I noticed that is blacklisted because it's detected as ASMedia (
> although it's JMicron ) and falls in one of the four cases that checks
> for speed ( or something like that, I can't find the link to the
> source code again otherwise I would have pointed to the code line ).
If it is detected as ASMedia, you can make a test kernel that removes
this code:
if (le16_to_cpu(udev->descriptor.idVendor) == 0x174c &&
(le16_to_cpu(udev->descriptor.idProduct) == 0x5106 ||
le16_to_cpu(udev->descriptor.idProduct) == 0x55aa)) {
if (udev->actconfig->desc.bMaxPower == 0) {
/* ASM1153, do nothing */
} else if (udev->speed < USB_SPEED_SUPER) {
/* No streams info, assume ASM1051 */
flags |= US_FL_IGNORE_UAS;
} else if (usb_ss_max_streams([1]->ss_ep_comp) == 32) {
/* Possibly an ASM1051, disable uas */
flags |= US_FL_IGNORE_UAS;
} else {
/* ASM1053, these have issues with large transfers */
flags |= US_FL_MAX_SECTORS_240;
}
}
from uas-detect.h
Yet it seems not to match the log you posted. Could you post the output of
"lsusb -v" for your device?
A problem with TRIM is certainly worth investigating.
Regards
Oliver
Re: WARNING in usb_submit_urb (3)
On Mo, 2018-10-15 at 13:12 -0400, Alan Stern wrote: > On Mon, 15 Oct 2018, Andrey Konovalov wrote: > > Ah, I see the problem. In fact it is the same issue, but the commit > mentioned above contains an error (is_in gets tested too soon). The > fix is below; can you check it? > > Alan Stern > Thanks for the catch. Regards Oliver
Re: Sabrent USB 3.0 to SSD // "UAS is blacklisted for this device, using usb-storage instead"
On So, 2018-10-14 at 13:40 +0200, Julian Xhokaxhiu wrote: > Dear USB Driver module maintainers, Hi, thank you for the report > Although sometimes, the system "hangs" very rarely, but in an annoying > fashion that makes a bit sluggish the experience, which translates to > weird dmesg errors like these ones: > https://unix.stackexchange.com/questions/441668/debian-usb3-hdd-uas-i-o-errors Just for confirmation: You are seeing these error messages for the current kernel? I am asking because the link is a report about UAS and you write that UAS is not used. > Initially I thought as I'm using an USB 3.0 adapter[0], that maybe the > UAS driver was buggy, but at my own surprise I found out actually that > the UAS driver is not used at all, although the adapter supports it > fine. > > I am currently using it on a Dell Latitude 7480, and when I use > `lsusb` I see that the device is recognized as `JMicron JMS561U` > which, by spec, supports UAS ( see > http://www.jmicron.com/PDF/brief/jms561u.pdf ). > > I am wondering if this device was already tested by anyone of you > already, and if not, if I can be a tester for a patch to enable UAS on > this adapter. I actually cannot find a patch blacklisting the device which according the log you attached must exist. Which kernel are you using? You can certainly test your device with UAS. Regards Oliver
Re: Query on usb/core/devio.c
On Do, 2018-10-11 at 14:29 -0400, Alan Stern wrote: > On Thu, 11 Oct 2018, Mayuresh Kulkarni wrote: [..] > > We are looking into closing the device instance during normal operation > > i.e.: only open/close device when needed. > > > > However, we still have one particular use-case where our USB device sends > > async notification to USB-host via the above interface. For that, as of > > now, we queue a URB from user-space. But because, the link never enters > > suspend (L2), we receive this async notification. > > > > I am not sure, how this use-case will work, if we open-close device only > > when needed. > > > > Assuming we have PM calls moved from open/close to ioctl or some other > > appropriate method, is the following sequence possible - > > * Queue an URB -> suspend (L2) > > * Device does remote wake & sends async notification > > * URB above returns with that notification > > It won't work like that. When a device goes into suspend there can't > be any outstanding URBs. Exactly. Yet in case the device is active the URB must be kept running. > How about instead having a mechanism whereby your usrspace driver can > tell when the device does a remote wakeup? At that point it could select() and trigger a wake up from resume()? > submit an URB via usbfs and receive the async notification. I am afraid that design includes an inevitable race condition. It works fine for resume(). It fails for suspend(). If you require user space to cancel periodic transfers before the device can suspend, you will introduce a window of at least two seconds between the cancellation and the eventual suspend() during which the device will not work. In fact events may be outright lost. If you want to suspend with user space drivers active, the kernel will have to kill all active URBs originating from user space drivers. And notify user space when the device is resumed. IMHO select() is the syscall designed for that. Regards Oliver
Re: Query on usb/core/devio.c
On Di, 2018-10-09 at 10:51 +0100, Mayuresh Kulkarni wrote: > > The USB core driver has a module-param "usb_autosuspend_delay" whose default > value is 2 sec. AFAIU, the PM core will wait at-least 2 sec before scheduling > the suspend of this USB device. > So, if the user-space driving any of the above devices sends "new" URB > request before usb_autosuspend_delay expires, the PM core will cancel > suspend. This will essentially mean the device remained active and link never > entered L2. Yes, but you cannot depend on that. > I could be wrong, but 2 sec seems like a pretty good time-out to me, from end > user's perspective. Not really. It works well for devices with unpredictable bursts. It fails for periodic activity with known frequencies. Some storage devices have worse PM because we lack a more elaborate system. > > we had this discussion years ago. Then the majority view was that an > > application should close the device. Do we have a reason to revisit > > that decision? [..] > With all due respect, one of the possible reason for this could be, power > saving on mobile/tablet platforms (running Android). These platforms usually > have a single USB port. > Specifically from our point of view, these platforms are removing 3.5 mm jack > and hence the only interface available for headsets is USB. That is interesting. The idea Alan mentioned is old. We already discussed it at that time. It basically boiled down to the question why user space cannot just close devices it wants to be suspended. Adding ioctl()s to change the PM count wouldn't be hard. But we need a justification. If you have one, by all means speak up. Regards Oliver
Re: Query on usb/core/devio.c
On Mo, 2018-10-08 at 11:16 -0400, Alan Stern wrote: > On Mon, 8 Oct 2018, Oliver Neukum wrote: > > > On Mo, 2018-10-08 at 10:50 +0100, Mayuresh Kulkarni wrote: > > > > > > As a result of this, the USB suspend (L2) does not seem to happen, even > > > if all the interface drivers of a composite USB device report "idle" to > > > USB core driver. The USB suspend seem to happen only when the caller in > > > user-space (in our case) closes the device file. > > > > > > Is this correct understanding? > > > > Yes, it is. > > > > > If yes, could you please help understand - > > > 1. Any specific reason to choose this design approach? Apologies, but > > > "git blame" does not reveal much information (or maybe I did not do git > > > blame on correct kernel version). > > > > We cannot assume that a device is done executing a command as soon as > > communication is done. Think of a scanner moving its sensor bar > > or a printer printing a page. Or a display displaying something. > > > > > 2. Is it possible to modify this driver to take PM ref-count on USB > > > device, only when user-space is actively interacting with the USB device > > > (so in open/close and appropriate ioctl calls, with special handling for > > > async URB submission)? > > > > Technically this is possible, but it is a bad idea. > > > > > 3. Will (2) break any known existing device(s)? > > > > Yes, it would and that makes it a bad idea. > > In theory we could add ioctls to perform the runtime PM put and get > operations. Hi, we had this discussion years ago. Then the majority view was that an application should close the device. Do we have a reason to revisit that decision? Regards Oliver
Re: Query on usb/core/devio.c
On Mo, 2018-10-08 at 10:50 +0100, Mayuresh Kulkarni wrote: > > As a result of this, the USB suspend (L2) does not seem to happen, even if > all the interface drivers of a composite USB device report "idle" to USB core > driver. The USB suspend seem to happen only when the caller in user-space (in > our case) closes the device file. > > Is this correct understanding? Yes, it is. > If yes, could you please help understand - > 1. Any specific reason to choose this design approach? Apologies, but "git > blame" does not reveal much information (or maybe I did not do git blame on > correct kernel version). We cannot assume that a device is done executing a command as soon as communication is done. Think of a scanner moving its sensor bar or a printer printing a page. Or a display displaying something. > 2. Is it possible to modify this driver to take PM ref-count on USB device, > only when user-space is actively interacting with the USB device (so in > open/close and appropriate ioctl calls, with special handling for async URB > submission)? Technically this is possible, but it is a bad idea. > 3. Will (2) break any known existing device(s)? Yes, it would and that makes it a bad idea. Regards Oliver
[PATCH] cdc-acm: fix race between reset and control messaging
If a device splits up a control message and a reset() happens
between the parts, the message is lost and already recieved parts
must be dropped.
Signed-off-by: Oliver Neukum
Fixes: 1aba579f3cf51 ("cdc-acm: handle read pipe errors")
---
drivers/usb/class/cdc-acm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index f9b40a9dc4d3..86e477cd5c48 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -1636,6 +1636,7 @@ static int acm_pre_reset(struct usb_interface *intf)
struct acm *acm = usb_get_intfdata(intf);
clear_bit(EVENT_RX_STALL, >flags);
+ acm->nb_index = 0; /* pending control transfers are lost */
return 0;
}
--
2.16.4
Re: fixes for ioctl() of usbtmc in testing tree
On Mo, 2018-09-24 at 10:56 +, gu...@kiener-muenchen.de wrote: > Zitat von Greg Kroah-Hartman : > > > On Mon, Sep 24, 2018 at 11:24:10AM +0200, Oliver Neukum wrote: > > > Hi, > > > > > > how should I mark fixes intended for the testing branch? > > > I got one for the usbtmc driver. > > > > Just send it like normal. You can do a "Fixes:" tag with the sha1, that > > should be fine. I need to push out my testing branch now, 0-day seems > > to be stalled :( > > > > Big sorry! There is a superflous kmalloc line 1270 til 1272. > Shall I send the fix? Damn. That is the same allocation repeated, not a reuse of the buffer. I'll resend. There is also a leak in the error case. Regards Oliver
fixes for ioctl() of usbtmc in testing tree
Hi, how should I mark fixes intended for the testing branch? I got one for the usbtmc driver. Regards Oliver
Re: Kernel memory leak on CDC-ACM device plug/unplug
On Mi, 2018-09-19 at 16:11 +0200, Romain Izard wrote: > While trying to debug a memory leak problem, I encountered the following > problem: > > After plugging/unplugging an USB CDC-ACM device, kmemleak reports multiple > copies of the following leak. It is not necessary to open the port for the > leak to happen. Hi, nothing is immediately obvious to me. Could you compile a kernel with CONFIG_DEBUG_KMEMLEAK? Regards Oliver
Re: [PATCH] usbcore: Select UAC3 configuration for audio if present
On Di, 2018-09-11 at 23:36 +0530, saranya.go...@intel.com wrote:
> @@ -121,6 +132,23 @@ int usb_choose_configuration(struct usb_device *udev)
> #endif
> }
>
> + /*
> +* Select first configuration as default for audio so that
> +* devices that don't comply with UAC3 protocol are supported.
> +* But, still iterate through other configurations and
> +* select UAC3 compliant config if present.
> +*/
> + if (i == 0 && num_configs > 1 && desc && is_audio(desc)) {
> + best = c;
> + continue;
> + }
> +
> + if (i > 0 && desc && is_audio(desc)) {
> + if (is_uac3_config(desc))
> + best = c;
> + break;
> + }
Hi,
that looks like a special case. Couldn't we do
for (all configuration) {
1. take first
2. if this configuration is generic and the current is specific change
current
3. if if this configuration is the same interface class as current and
protocol is higher, change current
}
Regards
Oliver
Re: [PATCH 2/2] USB: usbdevfs: restore warning for nonsensical flags
On Mi, 2018-09-05 at 15:07 +0200, Greg KH wrote: > On Wed, Sep 05, 2018 at 03:02:48PM +0200, Oliver Neukum wrote: > > On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote: > > > On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote: > > > > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK) > > > > + dev_warn(>dev->dev, "Requested nonsensical > > > > USBDEVFS_URB_SHORT_NOT_OK.\n"); > > > > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET) > > > > + dev_warn(>dev->dev, "Requested nonsensical > > > > USBDEVFS_URB_ZERO_PACKET.\n"); > > > > > > We should not make it trivial for userspace to spam the kernel log if at > > > all possible. Returning an error is probably the better thing to do > > > here, not just silently fix it up or ignore it. > > > > That means a change in the API in a way that makes orking systems fail. > > Ah, good point. Well, but do we want to do this in the next major release even if we cannot do it in a stable release? > I guess they were hitting the same dev_WARN() messages > today anyway, right? Yes. And for a kernel problem you really want the stack traces. Still, that does not tell us that we want to print a message if user space messes up. So dev_warn() or nothing? > > Do you want an extra version for stable? > > No, but why was this patch not marked for stable? I was under the impression that it was. This is a separate patch because you could argue that it is unnecessary or that stable and the next release should diverge on whether to take it. Regards Oliver
Re: [PATCH 2/2] USB: usbdevfs: restore warning for nonsensical flags
On Mi, 2018-09-05 at 14:19 +0200, Greg KH wrote:
> On Wed, Sep 05, 2018 at 12:07:03PM +0200, Oliver Neukum wrote:
> > If we filter flags before they reach the core we need to generate our
> > own warnings.
> >
> > Signed-off-by: Oliver Neukum
> > Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
> > ---
> > drivers/usb/core/devio.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> > index 263dd2f309fb..244417d0dfd1 100644
> > --- a/drivers/usb/core/devio.c
> > +++ b/drivers/usb/core/devio.c
> > @@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state
> > *ps, struct usbdevfs_urb *uurb
> > u |= URB_NO_INTERRUPT;
> > as->urb->transfer_flags = u;
> >
> > + if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
> > + dev_warn(>dev->dev, "Requested nonsensical
> > USBDEVFS_URB_SHORT_NOT_OK.\n");
> > + if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
> > + dev_warn(>dev->dev, "Requested nonsensical
> > USBDEVFS_URB_ZERO_PACKET.\n");
>
> We should not make it trivial for userspace to spam the kernel log if at
> all possible. Returning an error is probably the better thing to do
> here, not just silently fix it up or ignore it.
That means a change in the API in a way that makes orking systems fail.
Do you want an extra version for stable?
Regards
Oliver
[PATCH 2/2] USB: usbdevfs: restore warning for nonsensical flags
If we filter flags before they reach the core we need to generate our
own warnings.
Signed-off-by: Oliver Neukum
Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
---
drivers/usb/core/devio.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 263dd2f309fb..244417d0dfd1 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1697,6 +1697,11 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
u |= URB_NO_INTERRUPT;
as->urb->transfer_flags = u;
+ if (!allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
+ dev_warn(>dev->dev, "Requested nonsensical
USBDEVFS_URB_SHORT_NOT_OK.\n");
+ if (!allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
+ dev_warn(>dev->dev, "Requested nonsensical
USBDEVFS_URB_ZERO_PACKET.\n");
+
as->urb->transfer_buffer_length = uurb->buffer_length;
as->urb->setup_packet = (unsigned char *)dr;
dr = NULL;
--
2.16.4
[PATCH 1/2] USB: usbdevfs: sanitize flags more
Requesting a ZERO_PACKET or not is sensible only for output.
In the input direction the device decides.
Likewise accepting short packets makes sense only for input.
This allows operation with panic_on_warn without opening up
a local DOS.
Signed-off-by: Oliver Neukum
Reported-by: syzbot+843efa30c8821bd69...@syzkaller.appspotmail.com
Fixes: 0cb54a3e47cb ("USB: debugging code shouldn't alter control flow")
---
drivers/usb/core/devio.c | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 6ce77b33da61..263dd2f309fb 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1434,10 +1434,13 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
struct async *as = NULL;
struct usb_ctrlrequest *dr = NULL;
unsigned int u, totlen, isofrmlen;
- int i, ret, is_in, num_sgs = 0, ifnum = -1;
+ int i, ret, num_sgs = 0, ifnum = -1;
int number_of_packets = 0;
unsigned int stream_id = 0;
void *buf;
+ bool is_in;
+ bool allow_short = false;
+ bool allow_zero = false;
unsigned long mask =USBDEVFS_URB_SHORT_NOT_OK |
USBDEVFS_URB_BULK_CONTINUATION |
USBDEVFS_URB_NO_FSBR |
@@ -1471,6 +1474,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
u = 0;
switch (uurb->type) {
case USBDEVFS_URB_TYPE_CONTROL:
+ if (is_in)
+ allow_short = true;
if (!usb_endpoint_xfer_control(>desc))
return -EINVAL;
/* min 8 byte setup packet */
@@ -1511,6 +1516,10 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
break;
case USBDEVFS_URB_TYPE_BULK:
+ if (!is_in)
+ allow_zero = true;
+ else
+ allow_short = true;
switch (usb_endpoint_type(>desc)) {
case USB_ENDPOINT_XFER_CONTROL:
case USB_ENDPOINT_XFER_ISOC:
@@ -1531,6 +1540,10 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
if (!usb_endpoint_xfer_int(>desc))
return -EINVAL;
interrupt_urb:
+ if (!is_in)
+ allow_zero = true;
+ else
+ allow_short = true;
break;
case USBDEVFS_URB_TYPE_ISO:
@@ -1676,9 +1689,9 @@ static int proc_do_submiturb(struct usb_dev_state *ps,
struct usbdevfs_urb *uurb
u = (is_in ? URB_DIR_IN : URB_DIR_OUT);
if (uurb->flags & USBDEVFS_URB_ISO_ASAP)
u |= URB_ISO_ASAP;
- if (uurb->flags & USBDEVFS_URB_SHORT_NOT_OK && is_in)
+ if (allow_short && uurb->flags & USBDEVFS_URB_SHORT_NOT_OK)
u |= URB_SHORT_NOT_OK;
- if (uurb->flags & USBDEVFS_URB_ZERO_PACKET)
+ if (allow_zero && uurb->flags & USBDEVFS_URB_ZERO_PACKET)
u |= URB_ZERO_PACKET;
if (uurb->flags & USBDEVFS_URB_NO_INTERRUPT)
u |= URB_NO_INTERRUPT;
--
2.16.4
Re: [PATCH] USB: change dev_WARN to dev_err triggerable from user space
On Di, 2018-09-04 at 15:18 -0400, Alan Stern wrote:
> On Tue, 4 Sep 2018, Johan Hovold wrote:
>
> > On Tue, Sep 04, 2018 at 12:21:09PM +0200, Oliver Neukum wrote:
> > > On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote:
> > > > On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote:
> > > > > For those people who run with panic_on_warn a WARN() triggered
> > > > > from user space is a DOS. It is worth returning to dev_err()
> > > >
> > > > I think this should be dev_warn() unless you want to bring back the
> > > > returning of errors on these conditions as well (i.e. as was the case
> > > > prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control
> > > > flow")).
> > >
> > > Should I? A warning in syslog is pretty hardcore, so I have no idea
> > > whether dev_warn() is enough.
> >
> > Perhaps there are two sides to this. If something really should not be
> > happening and needs to be addressed (i.e. it's a driver bug) that
> > dev_WARN is warranted. If user space can be pass in bogus flags that
> > gets propagated to USB core, perhaps those need to be sanitised sooner
> > (in the vain of "don't trust anything coming from user space").
>
> I'd go along with this. The usbfs code should fix or reject URBs
> submitted from userspace with bogus flags or an incorrect pipe value.
> (In fact, we already sanitize the flags to some extent, but we could do
> more: ISO_ASAP should apply only to isochronous URBs, and ZERO_PACKET
> should apply only to bulk-OUT URBS.)
>
> Similar errors coming from kernel drivers should be reported as actual
> bugs.
Very well, I am making a patch to do that.
Regards
Oliver
Re: [PATCH] USB: change dev_WARN to dev_err triggerable from user space
On Di, 2018-09-04 at 11:31 +0200, Johan Hovold wrote:
> On Tue, Sep 04, 2018 at 10:44:41AM +0200, Oliver Neukum wrote:
> > For those people who run with panic_on_warn a WARN() triggered
> > from user space is a DOS. It is worth returning to dev_err()
>
> I think this should be dev_warn() unless you want to bring back the
> returning of errors on these conditions as well (i.e. as was the case
> prior to 0cb54a3e47cb ("USB: debugging code shouldn't alter control
> flow")).
Should I? A warning in syslog is pretty hardcore, so I have no idea
whether dev_warn() is enough.
Regards
Oliver
[PATCH] USB: change dev_WARN to dev_err triggerable from user space
For those people who run with panic_on_warn a WARN() triggered from user space is a DOS. It is worth returning to dev_err() Signed-off-by: Oliver Neukum Fixes: 0cb54a3e47cb4baf0bc7463f0a64cfeae5e35697 Reported-by: syzbot+843efa30c8821bd69...@syzkaller.appspotmail.com --- drivers/usb/core/urb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c index f51750bcd152..3fe65a774e6c 100644 --- a/drivers/usb/core/urb.c +++ b/drivers/usb/core/urb.c @@ -475,7 +475,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) /* Check that the pipe's type matches the endpoint's type */ if (usb_urb_ep_type_check(urb)) - dev_WARN(>dev, "BOGUS urb xfer, pipe %x != type %x\n", + dev_err(>dev, "BOGUS urb xfer, pipe %x != type %x\n", usb_pipetype(urb->pipe), pipetypes[xfertype]); /* Check against a simple/standard policy */ @@ -499,7 +499,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags) /* warn if submitter gave bogus flags */ if (allowed != urb->transfer_flags) - dev_WARN(>dev, "BOGUS urb flags, %x --> %x\n", + dev_err(>dev, "BOGUS urb flags, %x --> %x\n", urb->transfer_flags, allowed); /* -- 2.16.4
[PATCH] Revert "cdc-acm: implement put_char() and flush_chars()"
This reverts commit a81cf9799ad7299b03a4dff020d9685f9ac5f3e0.
The patch causes a regression, which I cannot find the reason for.
So let's revert for now, as a revert hurts only performance.
I was trying to resolve the problem with Oliver but we don't get any
conclusion
for 5 months, so I am now sending this to mail list and cdc_acm authors.
I am using simple request-response protocol to obtain the boiller
parameters
in constant intervals.
A simple one transaction is:
1. opening the /dev/ttyACM0
2. sending the following 10-bytes request to the device:
unsigned char req[] = {0x02, 0xfe, 0x01, 0x05, 0x08, 0x02, 0x01,
0x69, 0xab, 0x03};
3. reading response (frame of 74 bytes length).
4. closing the descriptor
I am doing this transaction with 5 seconds intervals.
Before the bad commit everything was working correctly: I've got a
requests and
a responses in a timely manner.
After the bad commit more time I am using the kernel module, more
problems I have.
The graph [2] is showing the problem.
As you can see after module load all seems fine but after about 30
minutes I've got
a plenty of EAGAINs when doing read()'s and trying to read back the
data.
When I rmmod and insmod the cdc_acm module again, then the situation is
starting
over again: running ok shortly after load, and more time it is running,
more EAGAINs
I have when calling read().
As a bonus I can see the problem on the device itself:
The device is configured as you can see here on this screen [3].
It has two transmision LEDs: TX and RX. Blink duration is set for 100ms.
This is a recording before the bad commit when all is working fine: [4]
And this is with the bad commit: [5]
As you can see the TX led is blinking wrongly long (indicating
transmission?)
and I have problems doing read() calls (EAGAIN).
Reported-by: manio
Signed-off-by: Oliver Neukum
Fixes: a81cf9799ad7299b03a4dff020d9685f9ac5f3e0
---
drivers/usb/class/cdc-acm.c | 92 -
drivers/usb/class/cdc-acm.h | 1 -
2 files changed, 93 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index f95bbdc86578..f9b40a9dc4d3 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -780,20 +780,9 @@ static int acm_tty_write(struct tty_struct *tty,
}
if (acm->susp_count) {
- if (acm->putbuffer) {
- /* now to preserve order */
- usb_anchor_urb(acm->putbuffer->urb, >delayed);
- acm->putbuffer = NULL;
- }
usb_anchor_urb(wb->urb, >delayed);
spin_unlock_irqrestore(>write_lock, flags);
return count;
- } else {
- if (acm->putbuffer) {
- /* at this point there is no good way to handle errors
*/
- acm_start_wb(acm, acm->putbuffer);
- acm->putbuffer = NULL;
- }
}
stat = acm_start_wb(acm, wb);
@@ -804,85 +793,6 @@ static int acm_tty_write(struct tty_struct *tty,
return count;
}
-static void acm_tty_flush_chars(struct tty_struct *tty)
-{
- struct acm *acm = tty->driver_data;
- struct acm_wb *cur;
- int err;
- unsigned long flags;
-
- spin_lock_irqsave(>write_lock, flags);
-
- cur = acm->putbuffer;
- if (!cur) /* nothing to do */
- goto out;
-
- acm->putbuffer = NULL;
- err = usb_autopm_get_interface_async(acm->control);
- if (err < 0) {
- cur->use = 0;
- acm->putbuffer = cur;
- dev_dbg(>control->dev, "Resumption failure\n");
- goto out;
- }
-
- if (acm->susp_count) {
- dev_dbg(>control->dev, "Anchored buffer of %u at %p \n",
cur->len, cur);
- usb_anchor_urb(cur->urb, >delayed);
- } else {
- dev_dbg(>control->dev, "Writing out buffer of %u at %p
\n", cur->len, cur);
- acm_start_wb(acm, cur);
- }
-out:
- spin_unlock_irqrestore(>write_lock, flags);
- return;
-}
-
-static int acm_tty_put_char(struct tty_struct *tty, unsigned char ch)
-{
- struct acm *acm = tty->driver_data;
- struct acm_wb *cur;
- int wbn;
- unsigned long flags;
-
-overflow:
- cur = acm->putbuffer;
- if (!cur) {
- spin_lock_irqsave(>write_lock, flags);
- wbn = acm_wb_alloc(acm);
- if (wbn >= 0) {
- cur = >wb[wbn];
- acm->putbuffer = cur;
- }
- spin_unlock_irqrestore(>write_lock, flags);
- if (!cur) {
- dev_dbg(>control->dev, "Allocation failed\n");
-
Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters
On Do, 2018-08-16 at 01:28 -0700, Patong Yang wrote:
> Features not supported by the cdc-acm driver are
> enabling/disabling flow control, enabling/disabling RS-485 mode,
> GPIOs and GPIO modes, etc. Support for these features had to be added in this
> new driver.
Hi,
I am afraid RS-485 in cdc-acm is a no-go.
I don't like the duplication, but it is the lesser evil.
We might want to include more stuff in the new driver,
so we duplicate only compiled code.
Regards
Oliver
Re: USB: ftdi_sio: not detecting
On Di, 2018-08-14 at 14:49 +0530, Muni Sekhar wrote: > /: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci_hcd/6p, 480M > > |__ Port 2: Dev 91, If 0, Class=Hub, Driver=hub/4p, 480M > > |__ Port 2: Dev 100, If 0, Class=Hub, Driver=hub/3p, 480M > > |__ Port 1: Dev 101, If 0, Class=Vendor Specific Class, > Driver=ftdi_sio, 12M > > |__ Port 2: Dev 102, If 0, Class=Vendor Specific Class, > Driver=ftdi_sio, 12M > > |__ Port 3: Dev 103, If 0, Class=Vendor Specific Class, > Driver=ftdi_sio, 12M In this case XHCI. But you need the failing case. Presumably also XHCI, but that is not guaranteed. In case lsusb -t fails in the failure case, attach another device into the port your serial devices fails at. Regards Oliver
Re: USB: ftdi_sio: not detecting
On Di, 2018-08-14 at 14:29 +0530, Muni Sekhar wrote:
>
> Is there any way to know which HC I’m is using? I checked the
> /sys/kernel/debug/usb/devices file and I see three HC’s.
>
>
>
> T: Bus=03 Lev=00 Prnt=00 Port=00 Cnt=00 Dev#= 1 Spd=5000 MxCh= 1
Match the 'Bus' number or use 'lsusb -t'
HTH
Oliver
Re: USB: ftdi_sio: not detecting
On Di, 2018-08-14 at 12:17 +0530, Muni Sekhar wrote: > On Mon, Aug 13, 2018 at 7:52 PM, Muni Sekhar wrote: > > On Mon, Aug 13, 2018 at 6:57 PM, Oliver Neukum wrote: > > > On Mo, 2018-08-13 at 18:44 +0530, Muni Sekhar wrote: > > > > [ Please keep me in CC as I'm not subscribed to the list] > > > > > > > > Hello All, > > > > > > > > I’ve a FTDI serial device and it is not recognized if I connect > > > > directly to computer. However if I connect via an external USB hub > > > > it’s fine and I can see the FTDI serial device. What could be the > > > > issue and how to resolve this? > > If I connect the FTDI device directly to computer I don't see any > dmesg log, but after enabling the dynamic debugging for usbcore and > hcd's driver then I see kernel log flooded with the below mentioned > logs. Is it a bug in the hcd's driver? The same device works fine if I > connect via an external USB hub. Hi, you are looking at an issue with the HC driver. Unfortunately you have castrated the logs to an extent that prevents me from telling which HC you are using. However, in any case, you need to find out and contact its driver's maintainer. HTH Oliver
Re: USB: ftdi_sio: not detecting
On Mo, 2018-08-13 at 18:44 +0530, Muni Sekhar wrote: > [ Please keep me in CC as I'm not subscribed to the list] > > Hello All, > > I’ve a FTDI serial device and it is not recognized if I connect > directly to computer. However if I connect via an external USB hub > it’s fine and I can see the FTDI serial device. What could be the > issue and how to resolve this? > > > Also one more behaviour, when I ran “lsusb” it hangs forever and ‘ps’ > command output shows the state as “Dl+”. Any reasons for this > behaviour? Impossible to tell. Switch on dynamic debugging for usbcore and your hcd's driver and post dmesg. Sysrq-T during the hang would also help. Regards Oliver
[PATCH] usb: uas: add support for more quirk flags
The hope that UAS devices would be less broken than old style storage devices has turned out to be unfounded. Make UAS support more of the quirk flags of the old driver. Signed-off-by: Oliver Neukum --- drivers/usb/storage/uas.c | 21 + 1 file changed, 21 insertions(+) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 9e9de5452860..1f7b401c4d04 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -842,6 +842,27 @@ static int uas_slave_configure(struct scsi_device *sdev) sdev->skip_ms_page_8 = 1; sdev->wce_default_on = 1; } + + /* +* Some disks return the total number of blocks in response +* to READ CAPACITY rather than the highest block number. +* If this device makes that mistake, tell the sd driver. +*/ + if (devinfo->flags & US_FL_FIX_CAPACITY) + sdev->fix_capacity = 1; + + /* +* Some devices don't like MODE SENSE with page=0x3f, +* which is the command used for checking if a device +* is write-protected. Now that we tell the sd driver +* to do a 192-byte transfer with this command the +* majority of devices work fine, but a few still can't +* handle it. The sd driver will simply assume those +* devices are write-enabled. +*/ + if (devinfo->flags & US_FL_NO_WP_DETECT) + sdev->skip_ms_page_3f = 1; + scsi_change_queue_depth(sdev, devinfo->qdepth - 2); return 0; } -- 2.16.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: broken TRIM support for JMS578 in uas mode
On Mo, 2018-07-30 at 14:43 +0300, Mailing Lists wrote: > I cannot issue TRIM commands to SSD behind a JMS578-based sata to > usb-c adapter. Tried with Fedora 28 kernel and with latest > 4.18.0-0.rc6.git0.1.vanilla.knurd.1.fc28.x86_64 > lsblk -D shows that discard is not enabled, but the SSD has this > capability (see below) > > However Windows 10 successfully TRIMs the device. Also the > trimcheck.exe tool validates the TRIM operation. This makes me think > the linux uas driver needs additional reverse engineering or support > from Jmicron about this chipset. Hi, The VPD page for TRIM support should be 0xb0. Could you provide that? If all else fails: for each SCSI device (UAS devices internally are SCSI) there is a "provisioning_mode" attribute in sysfs. You can try to force TRIM to be used. https://gist.github.com/cathay4t/e80e02a737242a5f3824606543631bfe Strictly speaking this is a SCSI issue, not a UAS problem. HTH Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb storage: remove inherited SCSI dependency for USB_STORAGE subentries
On Do, 2018-08-09 at 11:21 +0200, Greg Kroah-Hartman wrote: > On Thu, Aug 09, 2018 at 09:38:40AM +0200, Oliver Neukum wrote: > > On Do, 2018-08-09 at 01:01 +0300, Vladimir Zapolskiy wrote: > > > Because USB_STORAGE build symbol strictly depends on SCSI build > > > symbol, there is no need to specify it again for USB_UAS and > > > USB_STORAGE_ENE_UB6250 build options. > > > > [..] > > > > > config USB_UAS > > > tristate "USB Attached SCSI" > > > - depends on SCSI > > > help > > > The USB Attached SCSI protocol is supported by some USB > > > storage devices. It permits higher performance by supporting > > > > Hi, > > > > I am sorry, but this is wrong. You can build and use UAS without > > old style storage support. I do not recommend that you do so, > > but in theory it is possible. > > And UAS very much depends on storage. > > But today UAS depends on SCSI and USB_STORAGE, so if this is true, it's > not very obvious :) Hi, yes, this is an issue. Nevertheless UAS depends on SCSI and this dependency better stay. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb storage: remove inherited SCSI dependency for USB_STORAGE subentries
On Do, 2018-08-09 at 10:50 +0300, Vladimir Zapolskiy wrote: > Hi Oliver, > > On 08/09/2018 10:38 AM, Oliver Neukum wrote: > > On Do, 2018-08-09 at 01:01 +0300, Vladimir Zapolskiy wrote: > > > Because USB_STORAGE build symbol strictly depends on SCSI build > > > symbol, there is no need to specify it again for USB_UAS and > > > USB_STORAGE_ENE_UB6250 build options. > > > > [..] > > > > > config USB_UAS > > > tristate "USB Attached SCSI" > > > - depends on SCSI > > > help > > > The USB Attached SCSI protocol is supported by some USB > > > storage devices. It permits higher performance by supporting > > > > Hi, > > > > I am sorry, but this is wrong. You can build and use UAS without > > can you please elaborate what is wrong exactly. > > The change is non-functional. Hi, as far as I can tell you are introducing an implicit dependency of CONFIG_USB_UAS on CONFIG_USB_STORAGE. Now, firstly implicit references are bad anyway, secondly in this case it is wrong. Uas works without usb-storage. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb storage: remove inherited SCSI dependency for USB_STORAGE subentries
On Do, 2018-08-09 at 01:01 +0300, Vladimir Zapolskiy wrote:
> Because USB_STORAGE build symbol strictly depends on SCSI build
> symbol, there is no need to specify it again for USB_UAS and
> USB_STORAGE_ENE_UB6250 build options.
[..]
> config USB_UAS
> tristate "USB Attached SCSI"
> - depends on SCSI
> help
> The USB Attached SCSI protocol is supported by some USB
> storage devices. It permits higher performance by supporting
Hi,
I am sorry, but this is wrong. You can build and use UAS without
old style storage support. I do not recommend that you do so,
but in theory it is possible.
And UAS very much depends on storage.
Regards
Oliver
Nacked-by: Oliver Neukum
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: usb: dynamic suspend
On Sa, 2018-08-04 at 23:10 +0530, Muni Sekhar wrote:
>
> Unloading the driver and killing the 'fwupd' daemon resulted that
> device to go into runtime suspend. Here I noticed that both the
> operations(i.e. rmmod btusb & kill -9 ) are mandatory to
> device to go into runtime suspend. I'd like to understand how the
> driver & daemon are preventing the device to go into suspend mode, can
> you please explain me on this?
Hi,
btusb has support for autosuspend.
1. Please check whether your device supports remote wake up and isn't
quirky
2. Try a 'hciconfig hciX down' (with X for your number
HTH
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ] Sandisk Ultra Fit USB 3.0 thumb drive overheating way more than same USB does in Windows
On Fr, 2018-08-03 at 16:15 +0100, Mustafa A wrote:
> The drive (128GB USB 3.0)
> https://www.sandisk.co.uk/home/usb-flash/ultra-fit-usb overheats to the point
> where the metal part would burn someone if they held it for more than a
> second.
>
> The overheating only happens on this brand of USB drives, other USB 3.0
> drives have been fine.
> The overheating only happens with Linux 4.15.0-29 (the default Kubuntu 18.04
> kernel), it's fine when transferring files in Windows (tried with 8.1).
> The overheating happens even when files aren't transferring, even when the
> drive is idle.
Please try enabling autosuspend for this device. Power management
for storage devices is by default off because some devices with
removable media fail horribly with it.
If this helps we could try to get a rule for such drives into udev.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: broken TRIM support for JMS578 in uas mode
On Mo, 2018-07-30 at 14:43 +0300, Mailing Lists wrote:
> I cannot issue TRIM commands to SSD behind a JMS578-based sata to
> usb-c adapter. Tried with Fedora 28 kernel and with latest
> 4.18.0-0.rc6.git0.1.vanilla.knurd.1.fc28.x86_64
> lsblk -D shows that discard is not enabled, but the SSD has this
> capability (see below)
Hi Johannes,
is there a way to signal the SCSI layer that a trim for a device should
be attempted, even if it claims not to support it?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters
On Di, 2018-07-24 at 15:36 -0700, Patong Yang wrote:
> +static int xrusb_ctrl_msg(struct xrusb *xrusb,
> + int request, int value, void *buf, int len)
> +{
> + int rv = usb_control_msg(xrusb->dev,
> + usb_sndctrlpipe(xrusb->dev, 0),
> + request,
> + USB_RT_XRUSB,
> + value,
> + xrusb->control->altsetting[0].desc.bInterfaceNumber,
> + buf,
> + len,
> + 5000);
Please use the symbolic constant.
> + return rv < 0 ? rv : 0;
> +}
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
documenting that transfer_buffer can be NULL if transfer_buffer_length is 0
Hi, I am getting coverity reports because usbnet checks for zero length transfers before it allocates buffers. Is the assumption that no buffer is needed actually correct for every HCD and should I document this? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 ] cdc_ncm: Handle multicast Ethernet traffic
On Fr, 2018-06-29 at 16:45 +0200, Miguel Rodríguez Pérez wrote:
> Subject: [PATCH 1/2] Hook into usbnet_change_mtu respecting usbnet
> driver_info
>
> Change the way cdc_ncm_change_mtu hooks into the netdev_ops
> structure so that changes into usbnet driver_info operations
> can be respected. Without this, is was not possible to hook
> into usbnet_set_rx_mode.
Hi,
what is wrong with the existing hook?
static void __handle_set_rx_mode(struct usbnet *dev)
{
if (dev->driver_info->set_rx_mode)
(dev->driver_info->set_rx_mode)(dev);
clear_bit(EVENT_SET_RX_MODE, >flags);
}
If you cannot use it, I would prefer you to actually fix that.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: serial: qcserial: add support for the Dell DW5821e module
On Di, 2018-06-26 at 09:40 +0200, Bjørn Mork wrote:
> This code can make Linux default to a MBIM configuration if the MBIM
> function uses the first interface in that configuration, even if this
> configuration is not the first one. Availability of a driver is not
> considered. Except for RNDIS, just to make it the whole mess even more
> confusing
How would you consider it? We chose a configuration before we load
drivers. Even if we looked at the currently available drivers we'd end
up with a choice depending on which devices were used in the past.
A nondeterministic choice would be awkward.
We can load drivers for all configurations' interfaces, but we cannot
really wait for the loads to happen at that stage.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: cdc-acm: Decrement tty port's refcount if probe() fail
On Do, 2018-06-21 at 17:45 +0900, Jaejoong Kim wrote: > The cdc-acm driver does not have a refcount of itself, but uses a > tty_port's refcount. That is, if the refcount of tty_port is '0', we > can clean up the cdc-acm driver by calling the .destruct() > callback function of struct tty_port_operations. > > The problem is the destruct() callback function is not called if > the probe() fails, because tty_port's refcount is not zero. So, > add tty_port_put() when probe() fails. > > Signed-off-by: Jaejoong Kim Acked-by: Oliver Neukum -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: cdc-acm: Decrement tty port's refcount if probe() fail
On Do, 2018-06-21 at 17:45 +0900, Jaejoong Kim wrote: > The cdc-acm driver does not have a refcount of itself, but uses a > tty_port's refcount. That is, if the refcount of tty_port is '0', we > can clean up the cdc-acm driver by calling the .destruct() > callback function of struct tty_port_operations. > > The problem is the destruct() callback function is not called if > the probe() fails, because tty_port's refcount is not zero. So, > add tty_port_put() when probe() fails. > > Signed-off-by: Jaejoong Kim Acked-by: Oliver Neukum -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Do, 2018-06-14 at 18:36 +0200, Sebastian Andrzej Siewior wrote:
> In the code path
> __usb_hcd_giveback_urb()
> -> wdm_in_callback()
>-> service_outstanding_interrupt()
>
> The function service_outstanding_interrupt() will unconditionally enable
> interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> If the HCD completes in BH (like ehci does) then the context remains
> atomic due local_bh_disable() and enabling interrupts does not change
> this.
>
> Defer the error case handling to a workqueue as suggested by Oliver
> Neukum. In case of an error the worker performs the read out and wakes
> the user.
>
> Fixes: c1da59dad0eb ("cdc-wdm: Clear read pipeline in case of error")
> Cc: Robert Foss
> Cc: Oliver Neukum
> Signed-off-by: Sebastian Andrzej Siewior
It fixes the identified issue.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Do, 2018-06-14 at 13:17 +0200, Sebastian Andrzej Siewior wrote: > @@ -964,6 +975,7 @@ static void wdm_disconnect(struct usb_interface *intf) > mutex_lock(>wlock); > kill_urbs(desc); > cancel_work_sync(>rxwork); > + cancel_work_sync(>service_outs_intr); > mutex_unlock(>wlock); > mutex_unlock(>rlock); > > @@ -1065,6 +1077,7 @@ static int wdm_pre_reset(struct usb_interface *intf) > mutex_lock(>wlock); > kill_urbs(desc); > cancel_work_sync(>rxwork); > + cancel_work_sync(>service_outs_intr); > return 0; Almost good. I am afraid the work also needs to be canceled in wdm_suspend() Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Mi, 2018-06-13 at 22:28 +0200, Sebastian Andrzej Siewior wrote: > On 2018-06-13 19:43:55 [+0200], Oliver Neukum wrote: > > > > Hi Oliver, Hi Sebastian, > > I am just looking at your patch and I am wondering why > > wdm_in_callback() won't just call service_outstanding_interrupt() > > again and again? OK, maybe I am dense and it may well be present now, > > but it just looks to me that way. > > But this part didn't change, did it? Right, it didn't change, but that does not make it correct. > The user blocks in wdmw_read() We can only hope that he does. The wait is interruptible. If a signal comes at the wrong time, nobody will be waiting. > Maybe we should delay the WDM_READ flag in the error case until the > worker is done (before the wakeup). I don't think that will help. It seems like we need to make sure that error recovery is a one shot activity. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Mi, 2018-06-13 at 18:28 +0200, Sebastian Andrzej Siewior wrote:
> The function service_outstanding_interrupt() will unconditionally enable
> interrupts during unlock and invoke usb_submit_urb() with GFP_KERNEL.
> If the HCD completes in BH (like ehci does) then the context remains
> atomic due local_bh_disable() and enabling interrupts does not change
> this.
Hi,
I am just looking at your patch and I am wondering why
wdm_in_callback() won't just call service_outstanding_interrupt()
again and again? OK, maybe I am dense and it may well be present now,
but it just looks to me that way.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: cdc-wdm: don't enable interrupts in USB-giveback
On Di, 2018-06-12 at 21:57 +0200, Sebastian Andrzej Siewior wrote:
> > Yes, the atomic case should be rare. It will only happen on errors, and
> > IIUC that's only to work around issues caused by reporting errors back
> > to userspace without actually wanting to err out anyway.
>
> Yup. The missing part is if this was done to workaround a specific
> userland application or most/all of them.
Yes, If possible we should not regress in that regard.
> > I believe it would be better to decide one an error policy and stick to
> > that. Then you could just simplify away that whole mess, by either
> > ignoring the error and continue or bailing out and die.
>
> "Bailing out and die" would be a revert of commit c1da59dad0eb
> ("cdc-wdm: Clear read pipeline in case of error")?
> And ignoring the error would be "not updating rerr" in
> wdm_in_callback().
> I don't care either way. I can do whatever works for you/users best.
It seems to me that the core of the problem is handling an error
in irq context potentially. How about shifting it to a work queue?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: cdc_acm: Add quirk for Uniden UBC125 scanner
On Mo, 2018-06-11 at 12:39 +0200, Houston Yaroschoff wrote: > Uniden UBC125 radio scanner has USB interface which fails to work > with cdc_acm driver: > usb 1-1.5: new full-speed USB device number 4 using xhci_hcd > cdc_acm 1-1.5:1.0: Zero length descriptor references > cdc_acm: probe of 1-1.5:1.0 failed with error -22 > > Adding the NO_UNION_NORMAL quirk for the device fixes the issue: > usb 1-4: new full-speed USB device number 15 using xhci_hcd > usb 1-4: New USB device found, idVendor=1965, idProduct=0018 > usb 1-4: New USB device strings: Mfr=1, Product=2, SerialNumber=3 > usb 1-4: Product: UBC125XLT > usb 1-4: Manufacturer: Uniden Corp. > usb 1-4: SerialNumber: 0001 > cdc_acm 1-4:1.0: ttyACM0: USB ACM device > > `lsusb -v` of the device: > > Bus 001 Device 015: ID 1965:0018 Uniden Corporation > Device Descriptor: > bLength18 > bDescriptorType 1 > bcdUSB 2.00 > bDeviceClass2 Communications > bDeviceSubClass 0 > bDeviceProtocol 0 > bMaxPacketSize064 > idVendor 0x1965 Uniden Corporation > idProduct 0x0018 > bcdDevice0.01 > iManufacturer 1 Uniden Corp. > iProduct2 UBC125XLT > iSerial 3 0001 > bNumConfigurations 1 > Configuration Descriptor: > bLength 9 > bDescriptorType 2 > wTotalLength 48 > bNumInterfaces 2 > bConfigurationValue 1 > iConfiguration 0 > bmAttributes 0x80 > (Bus Powered) > MaxPower 500mA > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber0 > bAlternateSetting 0 > bNumEndpoints 1 > bInterfaceClass 2 Communications > bInterfaceSubClass 2 Abstract (modem) > bInterfaceProtocol 0 None > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x87 EP 7 IN > bmAttributes3 > Transfer TypeInterrupt > Synch Type None > Usage Type Data > wMaxPacketSize 0x0008 1x 8 bytes > bInterval 10 > Interface Descriptor: > bLength 9 > bDescriptorType 4 > bInterfaceNumber1 > bAlternateSetting 0 > bNumEndpoints 2 > bInterfaceClass10 CDC Data > bInterfaceSubClass 0 Unused > bInterfaceProtocol 0 > iInterface 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x81 EP 1 IN > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Endpoint Descriptor: > bLength 7 > bDescriptorType 5 > bEndpointAddress 0x02 EP 2 OUT > bmAttributes2 > Transfer TypeBulk > Synch Type None > Usage Type Data > wMaxPacketSize 0x0040 1x 64 bytes > bInterval 0 > Device Status: 0x > (Bus Powered) > > Signed-off-by: Houston Yaroschoff Acked-by: Oliver Neukum -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: xhci: increase CRS timeout value
On Do, 2018-06-07 at 11:48 -0700, Ajay Gupta wrote:
> Some controllers take almost 55ms to complete controller
> restore state (CRS).
> There is no timeout limit mentioned in xhci specification so
> fix the issue by increasing the timeout limit to 55ms
Hi,
the chances that you saw the true worst case are slim.
I would suggest you include at least a bit of a safety margin
and a comment that explains it.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Dienstag, den 29.05.2018, 14:32 +0530 schrieb Tushar Nimkar: > Hi, > > On 2018-05-28 18:12, Oliver Neukum wrote: > > Am Donnerstag, den 24.05.2018, 20:13 +0530 schrieb Tushar Nimkar: > > > We have built SCSI as module will it cause any problem to enable > > > CONFIG_SCSI_SCAN_ASYNC ? > > > > No, that should work. But the failure is bizzare. You say > > yes there is no problem! I have run some test over night. > > synchronous scanning fails, but async scan works? > > yes! Odd. Extremely odd. Does this show on all architectures? I am asking because that is the crucial question. I see two possibilities 1. the sync probing code has a bug that shows only on some architectures 2. architectures were a coincidence - the drive is broken We absolutely need to know what is happening. I am afraid this will have to be tested on another architecture. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Am Montag, den 28.05.2018, 10:59 + schrieb guido@kiener-
muenchen.de:
> > No, the problem is that you will underflow io->mutex
> >
>
> Don't worry. The function usbtmc488_ioctl_wait_srq is called by
> usbtmc_ioctl which already locks the mutex. See
> https://elixir.bootlin.com/linux/v4.17-rc7/source/drivers/usb/class/usbtmc.c#L1189
> So the mutex is just unlocked here to allow other threads to still communicate
> with the device.
Hi,
thanks I had overlooked that.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
Am Donnerstag, den 24.05.2018, 12:31 + schrieb guido@kiener- muenchen.de: > Zitat von Oliver Neukum <oneu...@suse.com>: > > > Am Montag, den 21.05.2018, 21:00 + schrieb guido@kiener- > > muenchen.de: > > > > > > I looked for a race here, but I do not find a race between open and > > > release, > > > since a refcount of "file_data->data->kref" is always hold by > > > usbtmc_probe/disconnect. > > > > > > However I see a race between usbtmc_open and usbtmc_disconnect. Are these > > > callback functions called mutual exclusive? > > > > No, they are not. > > In the meantime I found these conflictive hints: > > https://github.com/torvalds/linux/commit/52a749992ca6a0fd304609af40ed3bfd6cef4660 > and > https://elixir.bootlin.com/linux/v4.17-rc6/source/include/linux/usb.h#L1164 > > What do you think? > My current feeling is that open/disconnect is mutual exclusive. > We also could verify what really happens. Ok, I remember. You are safe, if and only if you share the USB minor number space. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Donnerstag, den 24.05.2018, 20:13 +0530 schrieb Tushar Nimkar:
> We have built SCSI as module will it cause any problem to enable
> CONFIG_SCSI_SCAN_ASYNC ?
No, that should work. But the failure is bizzare. You say
synchronous scanning fails, but async scan works?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Am Donnerstag, den 24.05.2018, 12:59 + schrieb guido@kiener-
muenchen.de:
> Zitat von Oliver Neukum <oneu...@suse.com>:
>
> > Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> > > +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
> > > + unsigned int __user *arg)
> > > +{
> > > + struct usbtmc_device_data *data = file_data->data;
> > > + struct device *dev = >intf->dev;
> > > + int rv;
> > > + unsigned int timeout;
> > > + unsigned long expire;
> > > +
> > > + if (!data->iin_ep_present) {
> > > + dev_dbg(dev, "no interrupt endpoint present\n");
> > > + return -EFAULT;
> > > + }
> > > +
> > > + if (get_user(timeout, arg))
> > > + return -EFAULT;
> > > +
> > > + expire = msecs_to_jiffies(timeout);
> > > +
> > > + mutex_unlock(>io_mutex);
> >
> > There is such a thing as threads sharing file descriptors.
> > That leads to the question what happens to the mutex if this
> > ioctl() is called multiple times.
> >
> > Regards
> > Oliver
>
> Multiple threads can wait with the same or different file descriptors.
> When an SRQ interrupt occurs, all threads and file descriptors are
> informed concurrently with wake_up_interruptible_all(>waitq);
> The "_all" is already fixed in 02/12.
No, the problem is that you will underflow io->mutex
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/12] usb: usbtmc: Fix ioctl USBTMC_IOCTL_CLEAR
Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> Insert a sleep of 50 ms between subsequent CHECK_CLEAR_STATUS
> control requests to avoid stressing the instrument with repeated
> requests.
>
> Some instruments need time to cleanup internal I/O buffers.
> Polling and repeated requests slow down the response time of
> devices.
The connection between the patch and the description is loose.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/12] usb: usbtmc: Add ioctl USBTMC488_IOCTL_WAIT_SRQ
Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener:
> +static int usbtmc488_ioctl_wait_srq(struct usbtmc_file_data *file_data,
> + unsigned int __user *arg)
> +{
> + struct usbtmc_device_data *data = file_data->data;
> + struct device *dev = >intf->dev;
> + int rv;
> + unsigned int timeout;
> + unsigned long expire;
> +
> + if (!data->iin_ep_present) {
> + dev_dbg(dev, "no interrupt endpoint present\n");
> + return -EFAULT;
> + }
> +
> + if (get_user(timeout, arg))
> + return -EFAULT;
> +
> + expire = msecs_to_jiffies(timeout);
> +
> + mutex_unlock(>io_mutex);
There is such a thing as threads sharing file descriptors.
That leads to the question what happens to the mutex if this
ioctl() is called multiple times.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/12] usb: usbtmc: Add ioctls for trigger, EOM bit and TermChar
Am Donnerstag, den 17.05.2018, 19:03 +0200 schrieb Guido Kiener: > + retval = usb_bulk_msg(data->usb_dev, > + usb_sndbulkpipe(data->usb_dev, > + data->bulk_out), > + buffer, USBTMC_HEADER_SIZE, > + , file_data->timeout); > + > + /* Store bTag (in case we need to abort) */ > + data->bTag_last_write = data->bTag; > + > + /* Increment bTag -- and increment again if zero */ > + data->bTag++; > + if (!data->bTag) > + data->bTag++; > + Independent of whether this needs to be split up, do you really want to do this regardless of usb_bulk_msg() returning an error? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle
Am Montag, den 21.05.2018, 21:00 + schrieb guido@kiener- muenchen.de: > > I looked for a race here, but I do not find a race between open and release, > since a refcount of "file_data->data->kref" is always hold by > usbtmc_probe/disconnect. > > However I see a race between usbtmc_open and usbtmc_disconnect. Are these > callback functions called mutual exclusive? No, they are not. > I'm not sure, but if not, then I think we have the same problem in > usb-skeleton.c In usb-skeleton.c a race exists. You are right. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver
Am Freitag, den 18.05.2018, 21:50 -0700 schrieb Alexander Kappner: > The ALWAYS_SYNC flag is currently honored by the usb-storage driver but not > UAS > and is required to work around devices that become unstable upon being > queried for cache. This code is taken straight from: > drivers/usb/storage/scsiglue.c:284 > > Signed-off-by: Alexander Kappner <a...@godking.net> Acked-by: Oliver Neukum <oneu...@suse.com> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb-storage] Re: [PATCH v2 0/2] Re: usb-storage: Add quirks to make G-Technology "G-Drive" work
Am Montag, den 21.05.2018, 13:47 -0400 schrieb Alan Stern: > On Fri, 18 May 2018, Alexander Kappner wrote: > > > v2 of this patch implements the FL_ALWAYS_SYNC flag in the UAS driver, and > > then > > adds the flag as quirks for the device at issue. This allows the G-Drive to > > work > > under both UAS and usb-storage. > > > > Alexander Kappner (2): > > [usb-storage] Add support for FL_ALWAYS_SYNC flag in the UAS driver > > [usb-storage] Add compatibility quirk flags for G-Technologies G-Drive > > > > drivers/usb/storage/uas.c | 6 ++ > > drivers/usb/storage/unusual_devs.h | 9 + > > drivers/usb/storage/unusual_uas.h | 9 + > > 3 files changed, 24 insertions(+) > > Acked-by: Alan Stern <st...@rowland.harvard.edu> > > This is okay with me. Oliver, what do you think? Perfect. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Donnerstag, den 17.05.2018, 12:29 +0530 schrieb Tushar Nimkar:
> Those commands issued from different layers say: sd.. uas.. scsi..
> so making them to go one after other. Once REPORT_LUN done go with
> READ_CAPACITY_16.
> This is only for the UAS devices. I believe no disturb to BOT behavior.
Hi,
this is good news.
1. We cannot slow down all UAS devices because a few are broken. This
would need to be selective.
2. What is insufficient about "shost->async_scan" for your approach?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb-storage: Add quirks to make G-Technology "G-Drive" work
Am Donnerstag, den 17.05.2018, 01:15 -0700 schrieb Alexander Kappner:
> Yes. Without this flag, the device keeps throwing similar errors on
> usb-storage. That's the same result I get on a host that doesn't have UAS
> compiled in. Here's a dmesg:
Hi,
this is suspicious. You do not actually whether US_FL_NO_WP_DETECT
by itself would make the device work. Can you please test that?
You will need the attached patch for the quirk to be supported.
Regards
Oliver
From 1ff6c9c9cc66ddb4cf7ca95bea589d6a30c63623 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneu...@suse.com>
Date: Thu, 17 May 2018 14:46:47 +0200
Subject: [PATCH] UAS: add support for the quirk US_FL_NO_WP_DETECT
The assumption that this quirk can be limited to old storage
devices was too optimistic.
Signed-off-by: Oliver Neukum <oneu...@suse.com>
---
drivers/usb/storage/uas.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 6034c39b67d1..7ee67e8c1f1e 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -836,6 +836,10 @@ static int uas_slave_configure(struct scsi_device *sdev)
if (devinfo->flags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;
+ /* UAS also needs to support FL_NO_WP_DETECT */
+ if (devinfo->flags & US_FL_NO_WP_DETECT)
+ sdev->skip_ms_page_3f = 1;
+
scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
return 0;
}
--
2.13.6
Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
Am Freitag, den 04.05.2018, 08:59 + schrieb Jun Li: > > > > > Can one implement a device that can operate as either DFP or UFP, > > > > but not implements the dynamic role switch that a DRP must support? > > > > > > You mean a port with DRD on data but not DRP on power? > > > > > > The data-role is newly added as the data role is not coupled with > > > power > > > > No, I meant data role. As far as I can tell for a DRP you need to implement > > the > > detection logic described in chapter 4 of the spec. > > Could you please point me the "detection logic" of typec spec chapter 4 > you are referring to? Chapter 4.5.2.2, especially state diagramms 4.15 and 4.16 It just seems to me that a DRP and a physical port that can be switched between UFP and DFP are not the same thing, but can be implemented. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] usb: misc: xapea00x: add driver for Xaptum ENF Access Card
Am Montag, den 07.05.2018, 08:31 -0500 schrieb David R. Bild : > > > + spi_master->flags = 0; > > > + spi_master->setup = xapea00x_spi_setup; > > > + spi_master->transfer_one_message = > > > xapea00x_spi_transfer_one_message; > > > + > > > + retval = spi_register_master(spi_master); > > > + > > > + if (retval) > > > + goto free_spi; > > > + > > > + dev->spi_master = spi_master; > > > > Race condition. > > > > What race condition do you see? (I appreciate the review, but need > some more specific help here.) Hi, you have registered the master. So it is functional, but if any callback goes for dev->spi_master at that point, it will read an incorrect value. HTH Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [usb-storage] [PATCH] usb: storage: Fix a possible data race in uas_queuecommand_lck
Am Dienstag, den 08.05.2018, 15:47 +0800 schrieb Jia-Ju Bai: > The write operations to "cmnd->result" and "cmnd->scsi_done" > are protected by the lock on line 642-643, but the write operations > to these data on line 634-635 are not protected by the lock. > Thus, there may exist a data race for "cmnd->result" > and "cmnd->scsi_done". No, the write operations need no lock. The low level driver at this point owns the command. We cannot race with abort() for a command within queuecommand(). We take the lock where we take it to protect dev->resetting. I don't see why the scope of the lock would need to be enlarged. Regards Oliver > To fix this data race, the write operations on line 634-635 > should be also protected by the lock. > > Signed-off-by: Jia-Ju Bai <baijiaju1...@gmail.com> Nacked-by: Oliver Neukum <oneu...@suse.com> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bug - USB ports remain suspended after suspend/resume
Am Freitag, den 04.05.2018, 19:00 + schrieb Vasil Vasilev:
> ASUS UX305UA user. USB ports function normally after system boot. After the
> first and any subsequent suspend/resume, the USB ports remain suspended. In
> order for a newly plugged in USB device to be recognized, any autosuspended
> USB device has to be toggled (autosuspend on and off).
>
> This problem first manifested itself in kernel version 4.16. It is not
> present in any kernel version before 4.16.
Hi,
could you bisect this?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/2] usb: misc: xapea00x: add driver for Xaptum ENF Access Card
dev;
> +
> + kref_get(>kref);
> + spi_master_get(dev->spi_master);
> +}
> +
> +/**
> + * xapea00x_cleanup_async_probe - clean up the internals of the async
> + * probe. Call this method after the probe has completed.
> + *
> + * The caller is responsible for freeing the probe itself, if
> + * dynamically allocated.
> + *
> + * @probe: pointer to the async_probe to clean up
> + */
> +static void xapea00x_cleanup_async_probe(struct xapea00x_async_probe *probe)
> +{
> + spi_master_put(probe->dev->spi_master);
> + kref_put(>dev->kref, xapea00x_delete);
> +}
> +
> +static struct spi_board_info tpm_board_info = {
> + .modalias = XAPEA00X_TPM_MODALIAS,
> + .max_speed_hz = 43 * 1000 * 1000, // Hz
Are you hardcoding HZ ?
> + .chip_select= 0,
> + .mode = SPI_MODE_0
> +};
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
Am Donnerstag, den 03.05.2018, 08:35 + schrieb Jun Li: > Hi > > -Original Message- > > From: Oliver Neukum [mailto:oneu...@suse.com] > > Sent: 2018年5月3日 15:27 > > To: Jun Li <jun...@nxp.com>; robh...@kernel.org; > > heikki.kroge...@linux.intel.com; gre...@linuxfoundation.org; > > li...@roeck-us.net > > Cc: gso...@gmail.com; dl-linux-imx <linux-...@nxp.com>; Peter Chen > > <peter.c...@nxp.com>; shufan_...@richtek.com; a.ha...@samsung.com; > > cw00.c...@samsung.com; devicet...@vger.kernel.org; > > linux-usb@vger.kernel.org > > Subject: Re: [PATCH v5 01/14] dt-bindings: connector: add properties for > > typec > > > > Am Donnerstag, den 03.05.2018, 08:24 +0800 schrieb Li Jun: > > > +Optional properties for usb-c-connector: > > > +- power-role: should be one of "source", "sink" or "dual"(DRP) if > > > +typec > > > + connector has power support. > > > +- try-power-role: preferred power role if "dual"(DRP) can support > > > +Try.SNK > > > + or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. > > > +- data-role: should be one of "host", "device", "dual"(DRD) if typec > > > + connector supports USB data. > > > > Hi, > > > > is this really correct? > > > > Can one implement a device that can operate as either DFP or UFP, but not > > implements the dynamic role switch that a DRP must support? > > You mean a port with DRD on data but not DRP on power? > > The data-role is newly added as the data role is not coupled with power No, I meant data role. As far as I can tell for a DRP you need to implement the detection logic described in chapter 4 of the spec. I can see no reason why you couldn't build a port that can be switched between the data roles but not implement that logic. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 01/14] dt-bindings: connector: add properties for typec
Am Donnerstag, den 03.05.2018, 08:24 +0800 schrieb Li Jun: > +Optional properties for usb-c-connector: > +- power-role: should be one of "source", "sink" or "dual"(DRP) if typec > + connector has power support. > +- try-power-role: preferred power role if "dual"(DRP) can support Try.SNK > + or Try.SRC, should be "sink" for Try.SNK or "source" for Try.SRC. > +- data-role: should be one of "host", "device", "dual"(DRD) if typec > + connector supports USB data. Hi, is this really correct? Can one implement a device that can operate as either DFP or UFP, but not implements the dynamic role switch that a DRP must support? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Montag, den 23.04.2018, 18:35 +0530 schrieb Tushar Nimkar:
> hi,
>
> On 2018-04-23 18:20, Oliver Neukum wrote:
> > Am Donnerstag, den 19.04.2018, 20:18 +0530 schrieb Tushar Nimkar:
> > No. But for testing we don't need to. Can you confirm that
> > the problem is triggered by specific commands?
>
> Observed with REPORT_LUN or READ_CAP16 or INQUIRY command.
Well, that is not the same thing. It is possible that these commands
generally fail, but x86_64 has a working error handling but ARM does
not.
> Are you planning to test it with dwc3?
No, I am sorry. I would need to acquire some very specific hardware.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: USB regression for Android phone and sound card in 4.14
Am Dienstag, den 24.04.2018, 07:35 +0300 schrieb Nazar Mokrynskyi: > I've tried to bisect kernels from 4.13 to 4.14 and didn't find the reason. > Then I found that with upstream 4.13 the issue is still present on Ubuntu > 18.04, so there should be something more than just a kernel. > > Eventually, I found that issue is somehow related to USB hub I use for my > periferals (mouse, keyboard and sound card). > > When sound card is connected separately, it is properly initialized as USB > 2.0 device: https://pastebin.com/0rv1aUBz > > However, when connected to USB hub, it initializes as USB 1.1 device, in > which case it disables some of its capabilities, like 5.1 output mode: > https://pastebin.com/i4qE9JTZ > > Can't blame USB hub for this, since when I unplug it with mentioned 3 devices > and plug back - everything works fine, the issue only happens on system boot. > > USB hub I use at the moment (used another before with the same outcome): > https://pastebin.com/wkA6D9TP You need to make sure ehci/hcd is loaded before ohci or uhci, if indeed you have a setup with true companion controllers. Which HCD are you using? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Donnerstag, den 19.04.2018, 20:18 +0530 schrieb Tushar Nimkar: > On 2018-04-19 14:15, Oliver Neukum wrote: > > Am Mittwoch, den 18.04.2018, 12:44 +0530 schrieb Tushar Nimkar: > > > On 2018-04-17 12:03, Tushar Nimkar wrote: > > > > Hi, > > > > > I have doubt that sequential scan(scsi_sequential_lun_scan) work well > > > for uas device(SCSI>3).. > > > Why? As I have seen in most cases failed to enumerate during > > > REPORT_LUN > > > command...and there is older way to scan disk is also present, > > > so I was thinking to try that.. did following things > > > > > > starget->no_report_luns = 1 ---> for my target while uas_target_alloc > > > (for try) > > > > In general the spec is one thing and reality is another thing. > > You can depend really only on what recent versions of Windows do. > > did not get you! Devices often implement the spec only to the extent they need to in order to work with Windows. > oh, so no backward old way of scanning(sequential)? No. But for testing we don't need to. Can you confirm that the problem is triggered by specific commands? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/6] USB: show USB 3.2 Dual-lane devices as Gen Xx2 during device enumeration
Am Donnerstag, den 19.04.2018, 19:05 +0300 schrieb Mathias Nyman:
> USB 3.2 specification adds a Gen XxY notion for USB3 devices where
> X is the signaling rate on the wire. Gen 1xY is 5Gbps Superspeed
> and Gen 2xY is 10Gbps SuperSpeedPlus. Y is the lane count.
>
> For normal, non inter-chip (SSIC) devies the rx and tx lane count is
> symmetric, and the maximum lane count for USB 3.2 devices is 2 (dual-lane).
>
> SSIC devices may have asymmetric lane counts, with up to four
> lanes per direction. The USB 3.2 specification doesn't point out
> how to use the Gen XxY notion for these devices, so we limit the Gen Xx2
> notion to symmertic Dual lane devies.
> For other devices just show Gen1 or Gen2
If we detect an asymmetry, we should say so. Otherwise all looks good.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Bluetooth controller falling off USB bus after an hour of continuous BT headset usage
Am Mittwoch, den 18.04.2018, 16:42 +0200 schrieb Dominik 'Rathann' Mierzejewski: > On Thursday, 15 March 2018 at 16:53, Dominik 'Rathann' Mierzejewski wrote: > [...] > > Could you give me any pointers for debugging this? I cannot trigger > > this on-demand, but when I'm in a conference call, it occurs within > > an hour or two of continuous usage of my BT headset in HFP mode. > > > > I originally reported it against the Bluetooth stack here: > > https://bugzilla.kernel.org/show_bug.cgi?id=198803 > > but I'm not sure if it's an issue with Bluetooth stack or the USB stack. > > You can find dmesg and lsusb output attached there. > > Anyone? This is a USB issue. Your logs show merely disconnect with a subsequent reenummeration. Switch on dynamic debugging for XHCI at least. There is nothing useful in the logs to go on otherwise. At a hunch try whether "usbcore.autosuspend=-1" on the kernel command line helps. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: sr 6:0:0:0: [sr0] tag#0 FAILED Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
Am Mittwoch, den 18.04.2018, 10:34 -0300 schrieb Cristian:
> Hello,
>
> Open bug in launchpad.net
> https://bugs.launchpad.net/bugs/1765043
Hi,
some more details perhaps? Did this happen during enumeration?
With or without a medium? And so on.
sr should be able to deal with such things. Please take this to linux-
scsi, as Alan suggested and provide at least a full dmesg with your
report.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Mittwoch, den 18.04.2018, 12:34 +0530 schrieb Tushar Nimkar:
> Oliver/Greg,
> Weather you are able to reproduce the issue?
> Did you tested it on dwc3 previously?
I don't even have a dwc3.
May I suggest that you try to determine whether the issue
happens on the same SCSI command? We need to know what
triggers it.
Sorry
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Mittwoch, den 18.04.2018, 12:44 +0530 schrieb Tushar Nimkar: > On 2018-04-17 12:03, Tushar Nimkar wrote: Hi, > I have doubt that sequential scan(scsi_sequential_lun_scan) work well > for uas device(SCSI>3).. > Why? As I have seen in most cases failed to enumerate during REPORT_LUN > command...and there is older way to scan disk is also present, > so I was thinking to try that.. did following things > > starget->no_report_luns = 1 ---> for my target while uas_target_alloc > (for try) In general the spec is one thing and reality is another thing. You can depend really only on what recent versions of Windows do. > > Found it is doing sequential scan but popuating 256 entries in cat > proc/partiction > for one disk > root@OpenWrt:/# cat proc/partitions > major minor #blocks name > > 80 488386584 sda > 81 488384032 sda1 > 8 48 488386584 sdd > 8 49 488384032 sdd1 > 8 64 488386584 sde > 8 65 488384032 sde1 > 8 80 488386584 sdf > 8 81 488384032 sdf1 > 8 96 488386584 sdg > 8 97 488384032 sdg1 > 8 112 488386584 sdh > 8 113 488384032 sdh1 > 256 total. > > ...though it is SCSI>3 device ,it should support both sequential as well > as REPORT_LUN? In theory. > Do not know weather this is the cause for the issue or not ...but if so > need to think on this too :) Well, does your original issue go away if you use NO_REPORT_LUN and does your device have multiple LUNs? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Fwd: usb: uas: device reset most the time while enumeration- usb3.0
Am Montag, den 16.04.2018, 10:23 -0400 schrieb Alan Stern: > > > > > [57246.701096] sd 1:0:0:0: tag#0 uas_eh_abort_handler 0 uas-tag 1 > > > > > inflight: CMD IN > > > > > [57246.701130] sd 1:0:0:0: tag#0 CDB: opcode=0x1a 1a 00 3f 00 04 00 > > > > URB is canceled, maybe that URB never finished? > > No doubt. Perhaps the device doesn't support this particular command. Then we should expect the enumeration to always fail. Possibly the lower layers swallow the transfer. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] cdc_ether: flag the Cinterion AHS8 modem by gemalto as WWAN
Am Mittwoch, den 11.04.2018, 13:15 +0200 schrieb Bassem Boubaker: > The Cinterion AHS8 is a 3G device with one embedded WWAN interface > using cdc_ether as a driver. > > The modem is controlled via AT commands through the exposed TTYs. > > AT+CGDCONT write command can be used to activate or deactivate a WWAN > connection for a PDP context defined with the same command. UE supports > one WWAN adapter. > > Signed-off-by: Bassem Boubaker <bassem.bouba...@actia.fr> Acked-by: Oliver Neukum <oneu...@suse.com> -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.
Am Donnerstag, den 05.04.2018, 08:26 +0200 schrieb Greg KH: > > The USB device can describe itself properly. The SMBIOS function is a > > requirement from our customer who designed a board using our device where > > their CPU reads from a specific BIOS location and initializes GPIOs based > > on the settings. These GPIOs set the mode of the transceiver (LOOPBACK, > > RS232, RS485, or RS422). Therefore, the driver also reads the same > > settings from the BIOS, so that it can enable the appropriate mode. > > That logic can be done in userspace, no need to do that within the > kernel, right? No, not if you want to do full RS485 and similar stuff. Yet ACM is not a full serial interface. It is for modems. That raises two questions a) why not add this to cdc-acm? b) why custom ioctls? The kernel can do those serial protocols at upper levels. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbip: vhc_hcd: prevent module being removed while device are attached
Am Dienstag, den 03.04.2018, 09:56 -0600 schrieb Shuah Khan:
> This is a virtual device associated to a real physical device on a different
> system. My concern is that if the module gets removed accidentally then it
> could disrupt access to the remote device. The remote nature of the device
> with several players involved makes this scenario a bit more complex than
Hi,
you would doubtlessly lose connection to that device. Yet you would
also lose connections if you down your network. You need to be root
to unload a module. You could overwrite your root filesystems or flash
your firmware. In general we cannot and don't try to protect root
from such accidents.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial adapters.
Am Mittwoch, den 04.04.2018, 00:06 -0700 schrieb Patong Yang:
> The driver is based on the CDC-ACM driver. In addition to supporting
> the features of the MaxLinear/Exar USB UART devices, the driver also
Hi,
it is rather drastic a measure to duplicate a driver just for a low
number of devices. It makes fixing bugs double the work. At a minimum,
before we look at the code itself, could you please explain why
a) the code cannot be added to cdc-acm?
b) the check for SMBIOS is needed?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: UVC VGA WebCam: USB2.0 is listed as two video devices
Am Samstag, den 24.03.2018, 16:53 +0100 schrieb fademind:
> Manjaro Linux x86_64
>
> linux416 4.16.r180324.g99fec39-1
>
> Weird bug. My webcam is listed twice
Hi,
does it have buttons? That looks like the input layer
gets involved. Please post "lsusb -v".
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ASMedia SATA Bridge not enumerating without quirk
Am Mittwoch, den 21.03.2018, 17:09 +0100 schrieb Greg KH:
> > I'm guessing it doesn't quite match up with the rules already in place
> > in uas-detect.h
>
> That device has a quirk already as a "normal" usb-storage device.
> Oliver added it back in 2013 with 32c37fc30c52 ("usb-storage: add quirk
> for mandatory READ_CAPACITY_16")
That should cause different symptoms.
> > Looks like it's an ASM1053 that can't do UAS
>
> No, it's not a UAS device, is someone trying to recycle device ids to do
> different things now? That's not good :(
The second interface looks like UAS though. The second interface looks
like UAS though. What exactly does happen when you ennumerate?
Dmesg please.
We may need some exotic logic for these devices.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] USB 3.2 initial support
Am Donnerstag, den 15.03.2018, 09:33 +0200 schrieb Mathias Nyman: > On 14.03.2018 12:29, Oliver Neukum wrote: > > > > We should also export all raw data we have. User space can be trusted > > to get a multiplication right and it is not the kernels job > > to interpret such data. > > Do I understand correctly that you propose the "speed" sysfs entry > should only show the lane signaling rate, i.e. 5000 or 1 for USB 3.x. Yes. Well actually we should call it rate then. And if you are paranoid split it in tx and rx. > Adding rx_lanes and tx_lanes and keeping "speed" as lane signaling rate > is probably the cleanest and most straight forward approach. > > I still would like to add a "Gen XxY" or "SSIC" to the > "new/reset SuperSpeed USB device number using " dev_info string. Definitely > It's a quick way of checking if the device works at the expected speed when > connecting a device. Indeed. In the journal everything is fair and we definitely want to tell at glance whether we got the lane count or the rate wrong. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
