Re: [PATCH v2 3/7] usb: typec: Separate the operations vector
On Sat, Oct 05, 2019 at 10:36:02AM -0700, Guenter Roeck wrote:
> On 10/4/19 8:08 AM, Heikki Krogerus wrote:
> > Introducing struct typec_operations which has the same
> > callbacks as struct typec_capability. The old callbacks are
> > kept for now, but after all users have been converted, they
> > will be removed.
> >
> > Signed-off-by: Heikki Krogerus
> > ---
> > drivers/usb/typec/class.c | 39 +--
> > include/linux/usb/typec.h | 20
> > 2 files changed, 49 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 89ffe370e426..f4972b7ee022 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -54,6 +54,7 @@ struct typec_port {
> > const struct typec_capability *orig_cap; /* to be removed */
> > const struct typec_capability *cap;
> > + const struct typec_operations *ops;
> > };
> > #define to_typec_port(_dev_) container_of(_dev_, struct typec_port, dev)
> > @@ -956,7 +957,7 @@ preferred_role_store(struct device *dev, struct
> > device_attribute *attr,
> > return -EOPNOTSUPP;
> > }
> > - if (!port->cap->try_role) {
> > + if (!port->cap->try_role || (!port->ops && !port->ops->try_role)) {
>
> Even though it is only temporary, this should be
> if (!port->cap->try_role && (!port->ops || !port->ops->try_role)) {
>
> otherwise both cap->try_role and ops->try_role must exist. Also, there would
> be a crash if port->cap->try_role and port->ops are both NULL. Same pretty
> much everywhere below.
Yes, this series is broken. I'll prepare v3.
I'm going to add two more patches to this series where I'll drop a few
more unused members from struct typec_capability.
thanks,
--
heikki
Re: CREAD ignored by almost all USB serial drivers
On Sat, Sep 28, 2019 at 10:49:55PM +0200, Harald Welte wrote: > [Copied on requst from https://bugzilla.kernel.org/show_bug.cgi?id=205033] > > It seems that a lot of Linux kernel USB serial device drivers are > ignoring the CREAD setting of termios.c_cflag. > > The man page is quite clear: >CREAD Enable receiver. > > The glibc man page at > https://ftp.gnu.org/old-gnu/Manuals/glibc-2.2.3/html_chapter/libc_17.html > states: > "Macro: tcflag_t CREAD >If this bit is set, input can be read from the terminal. >Otherwise, input is discarded when it arrives." > > When I mask this bit and then call tcsetattr(), I'm still receiving > input characters, at least on a pl2303 USB UART. Looking at the source > code of drivers/usb/serial/, the *only* driver appearing to respect > CREAD is digi_acceleport.c. All others seem to ignore it. To the > contrary, most classic serial drivers in drivers/tty/serial seem to > implement it. > > In absence of low-level support in individual drivers to actually > disable the receiver in hardware, I would have at least expected the > core tty/serial layer to drop/discard any characters received by the > hardware while CREAD is not set. But that also doesn't appear to be the > case. > > What's even more worrying is that the tcsetattr() call succeeds, i.e. it > is a silent error. I would expect the kernel to either implement the > functionalty in one way or another, or simply return tcsetattr() with > an error if an unsupported combination (i.e. CFLAG not set) is > configured. > > This is not a theoretical issue. Anyone implementing a half-duplex > protocol with shared Rx and Tx line will face the same issue. > > Am I missing something here? Please don't tell me that I just > discovered something that's broken for some 20-odd years, or at the very > least as far as normal linux.git history reaches back :/ You just discovered something that has been broken since the first usb-serial driver was written, all those years ago :) I did add support for this to the digi driver, as you saw, as the hardware had support for it. For everything else, they are all just dumb uarts and do not expose that information to the host computer and I think everyone just forgot about this option. Given that you are the first to report it that I can think of, I don't think very many people use half-duplex protocols with a shared Rx/Tx (which is crazy anyway...) > Please keep me in Cc of any responses, I'm not subscribed to linux-usb. Is there a specific usb-serial driver that you are using that you want to have this support added to? thanks, greg k-h
Re: CREAD ignored by almost all USB serial drivers
Hi Greg, On Mon, Oct 07, 2019 at 01:06:33PM +0200, Greg KH wrote: > On Sat, Sep 28, 2019 at 10:49:55PM +0200, Harald Welte wrote: > > It seems that a lot of Linux kernel USB serial device drivers are > > ignoring the CREAD setting of termios.c_cflag. > > You just discovered something that has been broken since the first > usb-serial driver was written, all those years ago :) Amazing and frightening at the same time. I would have expected somebody had built something like a hardware test fixture to test those drivers during all those decades. Something like a "well-known" serial device as the tester, attaching to all the handshake etc. lines of the "device under test" and then running through many of the possible settings from HW to SW flow control, baud rate, parity, number of stop bits, break characters, etc. I have no shortage of projects to work on, but if somebody else was interested to host a physical setup with many different [USB] serial ports and some CI around, I might be tempted to build the actual tester hardware and some test suite software for it. > I did add support for this to the digi driver, as you saw, as the > hardware had support for it. For everything else, they are all just > dumb uarts and do not expose that information to the host computer and I > think everyone just forgot about this option. I am aware that many USB serial adapters are rather "dumb", hence my suggestion to add a related option to the core usb-serial, or even to the core tty/serial layer: If the driver doesn't process CREAD, simply discard the received bytes at this common/shared layer. > Given that you are the first to report it that I can think of, I don't > think very many people use half-duplex protocols with a shared Rx/Tx > (which is crazy anyway...) Every smart card interface [1] on this planet, including every SIM card in every mobile phone uses such a setup: asynchronous half-duplex communication with shared Rx/Tx. Sure, not many people attach something like that to a USB-Serial adapter (as oppose to a USB-CCID reader), but I just wanted to clarify it's not as obscure as one may think. You can actually buy ultra-low-cost SIM card readers built that way. Also, I would assume that RS-485 is still used in lots of technology, including e.g. DMX and industrial control systems. Unless you go for a rather obscure 4-wire RS-485, then you have the same half-duplex operation on shared medium. Please note that USB-RS485 adapters exist, using a variety of USB-serial chipsets. > > Please keep me in Cc of any responses, I'm not subscribed to linux-usb. > > Is there a specific usb-serial driver that you are using that you want > to have this support added to? I'm typically using cp2102 or FTDI based devices, though of course in terms of "market footprint", PL2303 probably beats all of the others. However, as indicated, maybe the "catch-all fall-back" in the core is an easy way to adhere termios rules about CREAD without having to hack up every driver individually? Thanks, Harald [1] well, not every, but most smart cards follow ISO 7816-3, which is what I'm talking about -- - Harald Welte http://laforge.gnumonks.org/ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
On Mon, 7 Oct 2019, David Heinzelmann wrote:
> Hi,
>
> I hope it all fits now.
>
> David
>
>
> From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001
> From: David Heinzelmann
> Date: Fri, 4 Oct 2019 12:28:36 +0200
> Subject: [PATCH v4] usb: hub: Check device descriptor before resusciation
>
> If a device connected to an xHCI host controller disconnects from the USB bus
> and then reconnects, e.g. triggered by a firmware update, then the host
> controller automatically activates the connection and the port is enabled. The
> implementation of hub_port_connect_change() assumes that if the port is
> enabled then nothing has changed. There is no check if the USB descriptors
> have changed. As a result, the kernel's internal copy of the descriptors ends
> up being incorrect and the device doesn't work properly anymore.
>
> The solution to the problem is for hub_port_connect_change() always to
> check whether the device's descriptors have changed before resuscitating
> an enabled port.
>
> Signed-off-by: David Heinzelmann
Acked-by: Alan Stern
> ---
> Changes in v4:
> - changed commit description
> Changes in v3:
> - changed commit message and description
> - fix code style
> Changes in v2:
> - fix logic error to handle return code from usb_get_device_descriptor()
>properly
> - fix line endings
> ---
> drivers/usb/core/hub.c | 196 +++--
> 1 file changed, 111 insertions(+), 85 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 236313f41f4a..fdcfa85b5b12 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4930,6 +4930,91 @@ hub_power_remaining(struct usb_hub *hub)
> return remaining;
> }
>
> +
> +static int descriptors_changed(struct usb_device *udev,
> + struct usb_device_descriptor *old_device_descriptor,
> + struct usb_host_bos *old_bos)
> +{
> + int changed = 0;
> + unsignedindex;
> + unsignedserial_len = 0;
> + unsignedlen;
> + unsignedold_length;
> + int length;
> + char*buf;
> +
> + if (memcmp(&udev->descriptor, old_device_descriptor,
> + sizeof(*old_device_descriptor)) != 0)
> + return 1;
> +
> + if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
> + return 1;
> + if (udev->bos) {
> + len = le16_to_cpu(udev->bos->desc->wTotalLength);
> + if (len != le16_to_cpu(old_bos->desc->wTotalLength))
> + return 1;
> + if (memcmp(udev->bos->desc, old_bos->desc, len))
> + return 1;
> + }
> +
> + /* Since the idVendor, idProduct, and bcdDevice values in the
> + * device descriptor haven't changed, we will assume the
> + * Manufacturer and Product strings haven't changed either.
> + * But the SerialNumber string could be different (e.g., a
> + * different flash card of the same brand).
> + */
> + if (udev->serial)
> + serial_len = strlen(udev->serial) + 1;
> +
> + len = serial_len;
> + for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
> + old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
> + len = max(len, old_length);
> + }
> +
> + buf = kmalloc(len, GFP_NOIO);
> + if (!buf)
> + /* assume the worst */
> + return 1;
> +
> + for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
> + old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
> + length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
> + old_length);
> + if (length != old_length) {
> + dev_dbg(&udev->dev, "config index %d, error %d\n",
> + index, length);
> + changed = 1;
> + break;
> + }
> + if (memcmp(buf, udev->rawdescriptors[index], old_length)
> + != 0) {
> + dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
> + index,
> + ((struct usb_config_descriptor *) buf)->
> + bConfigurationValue);
> + changed = 1;
> + break;
> + }
> + }
> +
> + if (!changed && serial_len) {
> + length = usb_string(udev, udev->descriptor.iSerialNumber,
> + buf, serial_len);
> + if (length + 1 != serial_len) {
> + dev_dbg(&udev->dev, "serial string error %d\n",
> + length);
> + changed = 1;
> + } else if (memcmp(buf, udev->serial, length) != 0) {
> + dev_dbg(&udev-
Re: [PATCH 8/8] xhci: Fix NULL pointer dereference in xhci_clear_tt_buffer_complete()
[ +CC: Alan ]
On Fri, Oct 04, 2019 at 02:59:33PM +0300, Mathias Nyman wrote:
> udev stored in ep->hcpriv might be NULL if tt buffer is cleared
> due to a halted control endpoint during device enumeration
>
> xhci_clear_tt_buffer_complete is called by hub_tt_work() once it's
> scheduled, and by then usb core might have freed and allocated a
> new udev for the next enumeration attempt.
>
> Fixes: ef513be0a905 ("usb: xhci: Add Clear_TT_Buffer")
> Cc: # v5.3
> Reported-by: Johan Hovold
> Signed-off-by: Mathias Nyman
> ---
> drivers/usb/host/xhci.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 00f3804f7aa7..517ec3206f6e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -5238,8 +5238,16 @@ static void xhci_clear_tt_buffer_complete(struct
> usb_hcd *hcd,
> unsigned int ep_index;
> unsigned long flags;
>
> + /*
> + * udev might be NULL if tt buffer is cleared during a failed device
> + * enumeration due to a halted control endpoint. Usb core might
> + * have allocated a new udev for the next enumeration attempt.
> + */
> +
> xhci = hcd_to_xhci(hcd);
> udev = (struct usb_device *)ep->hcpriv;
> + if (!udev)
> + return;
I didn't have time to look into this myself last week, or comment on the
patch before Greg picked it up, but this clearly isn't the right fix.
As your comment suggests, ep->hcpriv may indeed be NULL here if USB core
have allocated a new udev. But this only happens after USB has freed the
old usb_device and the new one happens to get the same address.
Note that even the usb_host_endpoint itself (ep) has then been freed and
reallocated since it is member of struct usb_device, and it is the
use-after-free that needs fixing.
I've even been able to trigger another NULL-deref in this function
before a new udev has been allocated, due to the virt dev having been
freed by xhci_free_dev as part of usb_release_dev:
[ 19.627771] usb 2-2.4: unable to read config index 0 descriptor/start: -32
[ 19.627966] usb 2-2.4: chopping to 0 config(s)
[ 19.628133] usb 2-2.4: can't read configurations, error -32
[ 19.629017] usb 2-2.4: usb_release_dev - udev = 930b14d82000
udev is freed here
[ 19.629258] usb 2-2-port4: attempt power cycle
[ 19.629461] xhci_clear_tt_buffer_complete - udev = 930b14d82000
use-after-free when tt work is scheduled (note than udev is non-NULL
since udev hasn't been reallocated and initialised yet):
[ 19.629643] xhci_clear_tt_buffer_complete - xhci->devs[4] =
virt dev is NULL after having been freed by xhci_free_dev()
[ 19.629876] BUG: kernel NULL pointer dereference, address: 0030
and is later dereferenced
[ 19.630034] #PF: supervisor write access in kernel mode
[ 19.630155] #PF: error_code(0x0002) - not-present page
[ 19.630270] PGD 0 P4D 0
[ 19.630341] Oops: 0002 [#1] SMP
[ 19.630425] CPU: 2 PID: 110 Comm: kworker/2:2 Not tainted 5.4.0-rc1 #28
[ 19.630572] Hardware name: /D34010WYK, BIOS
WYLPT10H.86A.0051.2019.0322.1320 03/22/2019
[ 19.636141] Workqueue: events hub_tt_work
[ 19.638125] RIP: 0010:xhci_clear_tt_buffer_complete.cold.69+0x9b/0xcd
It seems the xhci clear-tt implementation was incomplete since it did
not take care to wait for any ongoing work before disabling the
endpoint. EHCI does this in ehci_endpoint_disable(), but xhci doesn't
even implement that callback.
As this may be something you could end up hitting in other paths as
well, perhaps we should even consider reverting the offending commit
pending a more complete implementation?
> slot_id = udev->slot_id;
> ep_index = xhci_get_endpoint_index(&ep->desc);
For reference, my original report is here:
https://lkml.kernel.org/r/20190930103107.GC13531@localhost
Johan
Re: [PATCH] Check for changed device descriptors when a connection-change occurs before validating the connection.
On Mon, Oct 07, 2019 at 10:01:47AM -0400, Alan Stern wrote: > On Mon, 7 Oct 2019, David Heinzelmann wrote: > > > Hi, > > > > I hope it all fits now. > > > > David > > > > > > From 8517ecfac0175aebba03bb0868dde652bc3c36e5 Mon Sep 17 00:00:00 2001 > > From: David Heinzelmann > > Date: Fri, 4 Oct 2019 12:28:36 +0200 > > Subject: [PATCH v4] usb: hub: Check device descriptor before resusciation > > > > If a device connected to an xHCI host controller disconnects from the USB > > bus > > and then reconnects, e.g. triggered by a firmware update, then the host > > controller automatically activates the connection and the port is enabled. > > The > > implementation of hub_port_connect_change() assumes that if the port is > > enabled then nothing has changed. There is no check if the USB descriptors > > have changed. As a result, the kernel's internal copy of the descriptors > > ends > > up being incorrect and the device doesn't work properly anymore. > > > > The solution to the problem is for hub_port_connect_change() always to > > check whether the device's descriptors have changed before resuscitating > > an enabled port. > > > > Signed-off-by: David Heinzelmann > > Acked-by: Alan Stern David, can you resend this in a format that I can apply it in? thanks, greg k-h
Re: [PATCH 7/8] xhci: Increase STS_SAVE timeout in xhci_suspend()
Hi Sasha,
> On Oct 6, 2019, at 20:07, Sasha Levin wrote:
>
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: f7fac17ca925 xhci: Convert xhci_handshake() to use
> readl_poll_timeout_atomic().
>
> The bot has tested the following trees: v5.3.2, v5.2.18, v4.19.76, v4.14.146,
> v4.9.194, v4.4.194.
>
> v5.3.2: Build OK!
> v5.2.18: Build OK!
> v4.19.76: Build OK!
> v4.14.146: Build OK!
> v4.9.194: Failed to apply! Possible dependencies:
>0b6c324c8b60 ("xhci: cleanup and refactor process_ctrl_td()")
>0f1d832ed1fb ("usb: xhci: Add port test modes support for usb2.")
>11644a765952 ("xhci: Add quirk to workaround the errata seen on Cavium
> Thunder-X2 Soc")
>191edc5e2e51 ("xhci: Fix front USB ports on ASUS PRIME B350M-A")
>1cc6d8617b91 ("usb: xhci: remove unnecessary second abort try")
>2a72126de1bb ("xhci: Remove duplicate xhci urb giveback functions")
>2d6d5769f82d ("xhci: fix non static symbol warning")
>30a65b45bfb1 ("xhci: cleanup and refactor process_bulk_intr_td()")
>446b31419cb1 ("xhci: refactor handle_tx_event() urb giveback")
>4750bc78efdb ("usb: host: xhci support option to disable the xHCI USB2 HW
> LPM")
>488dc164914f ("xhci: remove WARN_ON if dma mask is not set for platform
> devices")
>4c39d4b949d3 ("usb: xhci: use bus->sysdev for DMA configuration")
>505f581c48bc ("xhci: simplify if statement to make it more readable")
>52ab86852f74 ("xhci: remove extra URB_SHORT_NOT_OK checks in xhci, core
> handles most cases")
>6b7f40f71234 ("xhci: change xhci_set_link_state() to work with port
> structures")
>76a35293b901 ("usb: host: xhci: simplify irq handler return")
>9983a5fc39bf ("xhci: rename EP_HALT_PENDING to EP_STOP_CMD_PENDING")
>9ef7fbbb4fdf ("xhci: Rename variables related to transfer descritpors")
>a6ff6cbf1fab ("usb: xhci: Add helper function xhci_set_power_on().")
>a7d57abcc8a5 ("xhci: workaround CSS timeout on AMD SNPS 3.0 xHC")
>d3519b9d9606 ("xhci: Manually give back cancelled URB if we can't queue it
> for cancel")
>d9f11ba9f107 ("xhci: Rework how we handle unresponsive or hoptlug removed
> hosts")
>e740b019d7c6 ("xhci: xhci-hub: use new port structures to get port address
> instead of port array")
>eaefcf246b56 ("xhci: change xhci_test_and_clear_bit() to use new port
> structure")
>f97c08ae329b ("xhci: rename endpoint related trb variables")
>f99265965b32 ("xhci: detect stop endpoint race using pending timer instead
> of counter.")
>ffd4b4fc0b9a ("xhci: Add helper to get xhci roothub from hcd")
>
> v4.4.194: Failed to apply! Possible dependencies:
>11644a765952 ("xhci: Add quirk to workaround the errata seen on Cavium
> Thunder-X2 Soc")
>191edc5e2e51 ("xhci: Fix front USB ports on ASUS PRIME B350M-A")
>21939f003ad0 ("usb: host: xhci-plat: enable BROKEN_PED quirk if platform
> requested")
>41135de1e7fd ("usb: xhci: add quirk flag for broken PED bits")
>4750bc78efdb ("usb: host: xhci support option to disable the xHCI USB2 HW
> LPM")
>488dc164914f ("xhci: remove WARN_ON if dma mask is not set for platform
> devices")
>4c39d4b949d3 ("usb: xhci: use bus->sysdev for DMA configuration")
>4efb2f694114 ("usb: host: xhci-plat: add struct xhci_plat_priv")
>69307ccb9ad7 ("usb: xhci: bInterval quirk for TI TUSB73x0")
>76f9502fe761 ("xhci: plat: adapt to unified device property interface")
>9da5a1092b13 ("xhci: Bad Ethernet performance plugged in ASM1042A host")
>a3aef3793071 ("xhci: get rid of platform data")
>a7d57abcc8a5 ("xhci: workaround CSS timeout on AMD SNPS 3.0 xHC")
>dec08194ffec ("xhci: Limit USB2 port wake support for AMD Promontory
> hosts")
>def4e6f7b419 ("xhci: refactor and cleanup endpoint initialization.")
>
>
> NOTE: The patch will not be queued to stable trees until it is upstream.
Where do I send backport for v4.4 and v4.9?
Kai-Heng
>
> How should we proceed with this patch?
>
> --
> Thanks,
> Sasha
RE:PERSONAL LETTER FROM MRS RASHIA AMIRA
Greetings My name is Barrister Hans Erich. I have a client who is interested to invest in your country, she is a well known politician in her country and deserve a lucrative investment partnership with you outside her country without any delay Please can you manage such investment please Kindly reply for further details. Your full names Your urgent response will be appreciated Thank you and God bless you. Barrister Hans Erich Yours sincerely, Barrister Hans Erich CONTACT: hanserich9hel...@gmail.com
Possible bug with cdc_ether, triggers NETDEV WATCHDOG
I've been messing around with a Raspberry Pi Zero, in its ethernet gadget mode. This possible bug report is not against the Pi Zero linux kernel, but rather the host computer's linux kernel. I've been able to reproduce the same host computer issue with my normal laptop, and an embedded board (buildroot-based). Both run a newish version of 4.19. The bug report is that most of the time I cannot ping through the local link, and I get a kernel debug message: sometimes I can ping the Pi Zero with no kernel message, most of the time I can't ping and the message comes up, and occasionally I get the message right when I plug in the Pi Zero, before I issue the ping command. Here is the dmesg on my normal laptop (I've included the plug-in sequence also): [11728.029900] usb 1-1: new high-speed USB device number 10 using xhci_hcd [11728.434200] usb 1-1: device descriptor read/64, error -71 [11728.669543] usb 1-1: New USB device found, idVendor=0525, idProduct=a4a2, bcdDevice= 4.19 [11728.669548] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [11728.669551] usb 1-1: Product: RNDIS/Ethernet Gadget [11728.669554] usb 1-1: Manufacturer: Linux 4.19.75+ with 2098.usb [11728.674528] cdc_ether 1-1:1.0 usb0: register 'cdc_ether' at usb-:00:14.0-1, CDC Ethernet Device, 22:93:3a:1e:ac:5c [11730.725278] cdc_ether 1-1:1.0 enp0s20f0u1: renamed from usb0 [11768.174915] [ cut here ] [11768.174921] NETDEV WATCHDOG: enp0s20f0u1 (cdc_ether): transmit queue 0 timed out [11768.174950] WARNING: CPU: 3 PID: 0 at dev_watchdog+0x1f1/0x200 [11768.174951] Modules linked in: cdc_ether usbnet mii xt_hl xt_REDIRECT iptable_nat nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip_tables bpfilter algif_hash algif_skcipher af_alg bnep ipv6 crc_ccitt 8021q garp stp mrp snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic ipmi_msghandler joydev hid_logitech_hidpp hid_logitech_dj btusb btrtl btbcm btintel bluetooth ecdh_generic rtsx_pci_ms rtsx_pci_sdmmc mmc_core memstick i2c_designware_platform i2c_designware_core ppdev uvcvideo dell_rbtn dell_laptop dell_smm_hwmon tpm_crb intel_rapl vboxpci(O) vboxnetadp(O) vboxnetflt(O) x86_pkg_temp_thermal intel_powerclamp crct10dif_pclmul crc32c_intel ghash_clmulni_intel pcbc aesni_intel aes_x86_64 vboxdrv(O) dell_wmi crypto_simd sparse_keymap cryptd glue_helper e1000e input_leds dell_smbios [11768.175030] iwlmvm serio_raw dcdbas snd_hda_intel wmi_bmof dell_wmi_descriptor ptp snd_hda_codec snd_hwdep intel_wmi_thunderbolt snd_hda_core iwlwifi pps_core idma64 rtsx_pci nouveau virt_dma ttm i915 intel_lpss_pci intel_lpss processor_thermal_device intel_soc_dts_iosf intel_pch_thermal tpm_tis parport_pc i2c_hid tpm_tis_core pcc_cpufreq int3403_thermal parport dell_smo8800 int3400_thermal tpm int3402_thermal acpi_thermal_rel int340x_thermal_zone rng_core acpi_pad [11768.175077] CPU: 3 PID: 0 Comm: swapper/3 Tainted: P O 4.19.77-gentoo #1 [11768.175079] Hardware name: Dell Inc. Precision 7520/0DFG9Y, BIOS 1.15.1 05/30/2019 [11768.175086] RIP: 0010:dev_watchdog+0x1f1/0x200 [11768.175089] Code: 48 63 55 e8 eb 99 4c 89 f7 c6 05 bc 40 ad 00 01 e8 44 6c fd ff 44 89 e9 4c 89 f6 48 c7 c7 f0 30 0a 82 48 89 c2 e8 19 d1 88 ff <0f> 0b eb c5 66 66 2e 0f 1f 84 00 00 00 00 00 48 c7 47 08 00 00 00 [11768.175092] RSP: 0018:5f8c3e98 EFLAGS: 00010292 [11768.175096] RAX: 0044 RBX: 4b307e00 RCX: 0006 [11768.175097] RDX: 0007 RSI: 0086 RDI: 5f8d53f0 [11768.175099] RBP: 8886d7fa7438 R08: 0444 R09: 0001 [11768.175101] R10: R11: 0001 R12: 8886d7fa741c [11768.175103] R13: R14: 8886d7fa7000 R15: 4b307e80 [11768.175106] FS: () GS:5f8c() knlGS: [11768.175109] CS: 0010 DS: ES: CR0: 80050033 [11768.175111] CR2: 7f727ac4e180 CR3: 02209002 CR4: 001606e0 [11768.175113] DR0: DR1: DR2: [11768.175115] DR3: DR6: fffe0ff0 DR7: 0400 [11768.175116] Call Trace: [11768.175120] [11768.175127] ? qdisc_reset+0xd0/0xd0 [11768.175132] ? qdisc_reset+0xd0/0xd0 [11768.175140] call_timer_fn+0xe/0x80 [11768.175146] run_timer_softirq+0x337/0x390 [11768.175152] ? tick_sched_timer+0x4b/0x70 [11768.175159] ? timerqueue_add+0x56/0x90 [11768.175163] ? __hrtimer_run_queues+0x11a/0x170 [11768.175169] __do_softirq+0xbc/0x201 [11768.175174] irq_exit+0xe4/0xf0 [11768.175179] smp_apic_timer_interrupt+0x4d/0x80 [11768.175184] apic_timer_interrupt+0xf/0x20 [11768.175187] [11768.175194] RIP: 0010:cpuidle_enter_state+0x117/0x1a0 [11768.175196] Code: cf 6c 95 ff 65 8b 3d a8 fc 8d 7e e8 43 6b 95 ff 31 ff 48 89 c5 e8 29 79 95 ff 45 84 ed 75 64 fb 48 ba cf f7 53 e3 a5 9b c4 20 <48> 89 e9 48 2b 0c 24 48 89
Re: ASM2142 issues
Le mar. 24 sept. 2019 à 17:50, Loïc Yhuel a écrit : > > Looks like the xHCI PCI controller wasn't fully powered up to D0 state yet > > when xhci_resume was called. Looks similar to what is discussed in thread: > > > > https://marc.info/?l=linux-usb&m=156681068319529&w=2 In fact the kernel keeps the device, port, and the two bridges in D0 state. On suspend : [ 860.578478] xhci_hcd :08:00.0: PME# enabled [ 860.578511] xhci_hcd :08:00.0: PCI PM: Suspend power state: D0 ... [ 860.580329] pcieport :02:09.0: PCI PM: Suspend power state: D0 ... [ 860.603786] pcieport :01:00.2: PCI PM: Suspend power state: D0 [ 860.603836] pcieport :00:01.1: PCI PM: Suspend power state: D0 On resume : ... [ 861.135868] xhci_hcd :08:00.0: PME# disabled [ 861.135934] xhci_hcd :08:00.0: enabling bus mastering ... [ 861.235966] xhci_hcd :08:00.0: WARN: xHC restore state timeout [ 861.235972] xhci_hcd :08:00.0: PCI post-resume error -110! [ 861.235973] xhci_hcd :08:00.0: HC died; cleaning up [ 861.235981] PM: dpm_run_callback(): pci_pm_resume+0x0/0x90 returns -110 [ 861.235984] PM: Device :08:00.0 failed to resume async: error -110 Setting the XHCI_RESET_ON_RESUME quirk avoids the issue. But the real problem might be the device being marked as only supporting D0, which seems strange for suspend. > On my machine, the PCIe "port" is supposed to support 8GT/s (but the > controller is on the motherboard, > so there is no official specification about how the internal > peripherals are linked), the device too, > but it's always in 5GT/s mode (in Windows too), so perhaps it changes > the delays. This is unrelated to the other problems, but if someone else has the same HW, I found I can enable 8GT/s : setpci -v -s :08:00.0 CAP_EXP+0x30.w=0x0003:0x000f setpci -v -s :02:09.0 CAP_EXP+0x30.w=0x0003:0x000f setpci -v -s :02:09.0 CAP_EXP+0x10.w=0x0020:0x0020
RE: [bug report] usb: chipidea: imx: enable vbus and id wakeup only for OTG events
Hi Dan,
> -Original Message-
> From: Dan Carpenter
> Sent: 2019年10月2日 19:30
> To: Jun Li
> Cc: Pengutronix Kernel Team ; dl-linux-imx
> ; linux-usb@vger.kernel.org
> Subject: [bug report] usb: chipidea: imx: enable vbus and id wakeup only for
> OTG events
>
> Hello Li Jun,
>
> The patch 15b80f7c3a7f: "usb: chipidea: imx: enable vbus and id wakeup only
> for OTG
> events" from Sep 9, 2019, leads to the following static checker warning:
>
> drivers/usb/chipidea/ci_hdrc_imx.c:438 ci_hdrc_imx_probe()
> warn: 'data->usbmisc_data' can also be NULL
Thanks for the reporting.
>
> drivers/usb/chipidea/ci_hdrc_imx.c
>317 data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>318 if (!data)
>319 return -ENOMEM;
>320
>321 data->plat_data = imx_platform_flag;
>322 pdata.flags |= imx_platform_flag->flags;
>323 platform_set_drvdata(pdev, data);
>324 data->usbmisc_data = usbmisc_get_init_data(dev);
> ^^^
> This can return NULL or error pointer.
>
>325 if (IS_ERR(data->usbmisc_data))
>326 return PTR_ERR(data->usbmisc_data);
>327
>328 if ((of_usb_get_phy_mode(dev->of_node) ==
> USBPHY_INTERFACE_MODE_HSIC)
>329 && data->usbmisc_data) {
>^^ This code checks for NULL.
>
>330 pdata.flags |= CI_HDRC_IMX_IS_HSIC;
>331 data->usbmisc_data->hsic = 1;
>332 data->pinctrl = devm_pinctrl_get(dev);
>333 if (IS_ERR(data->pinctrl)) {
>334 dev_err(dev, "pinctrl get failed, err=%ld\n",
>335 PTR_ERR(data->pinctrl));
>336 return PTR_ERR(data->pinctrl);
>337 }
>338
>339 pinctrl_hsic_idle =
> pinctrl_lookup_state(data->pinctrl, "idle");
>340 if (IS_ERR(pinctrl_hsic_idle)) {
>341 dev_err(dev,
>342 "pinctrl_hsic_idle lookup failed,
> err=%ld\n",
>343 PTR_ERR(pinctrl_hsic_idle));
>344 return PTR_ERR(pinctrl_hsic_idle);
>345 }
>346
>347 ret = pinctrl_select_state(data->pinctrl,
> pinctrl_hsic_idle);
>348 if (ret) {
>349 dev_err(dev, "hsic_idle select failed,
> err=%d\n", ret);
>350 return ret;
>351 }
>352
>353 data->pinctrl_hsic_active =
> pinctrl_lookup_state(data->pinctrl,
>354
> "active");
>355 if (IS_ERR(data->pinctrl_hsic_active)) {
>356 dev_err(dev,
>357 "pinctrl_hsic_active lookup failed,
> err=%ld\n",
>358
> PTR_ERR(data->pinctrl_hsic_active));
>359 return PTR_ERR(data->pinctrl_hsic_active);
>360 }
>361
>362 data->hsic_pad_regulator = devm_regulator_get(dev,
> "hsic");
>363 if (PTR_ERR(data->hsic_pad_regulator) ==
> -EPROBE_DEFER) {
>364 return -EPROBE_DEFER;
>365 } else if (PTR_ERR(data->hsic_pad_regulator) ==
> -ENODEV)
> {
>366 /* no pad regualator is needed */
>367 data->hsic_pad_regulator = NULL;
>368 } else if (IS_ERR(data->hsic_pad_regulator)) {
>369 dev_err(dev, "Get HSIC pad regulator error:
> %ld\n",
>370
> PTR_ERR(data->hsic_pad_regulator));
>371 return PTR_ERR(data->hsic_pad_regulator);
>372 }
>373
>374 if (data->hsic_pad_regulator) {
>375 ret =
> regulator_enable(data->hsic_pad_regulator);
>376 if (ret) {
>377 dev_err(dev,
>378 "Failed to enable HSIC pad
> regulator\n");
>379 return ret;
>380 }
>381 }
>382 }
>383
>384 if (pdata.flags & CI_HDRC_PMQOS)
>385 pm_qos_add_request(&data->pm_qos_req,
>386 PM_QOS_CPU_DMA_LATENCY, 0);
>387
>388 ret = imx_get_clks(dev);
>389 if (ret)
>390 goto disable_hsic_regulator;
>391
>392 ret = imx_prepare_enable_clks(dev);
>393 if (ret)
>394
Re: [chipidea] continuous USB resets on NXP i.MX 6ULL device
On 19-10-02 19:15:11, Igor Opaniuk wrote: > + Li Jun > > Hi Peter, Li, > > On Mon, Sep 30, 2019 at 2:35 PM Igor Opaniuk wrote: > > > > Hi Peter, > > > > On Wed, Sep 25, 2019 at 3:44 AM Peter Chen wrote: > > > > > > > > > > > > > > Hi Fabio, Peter, Stefan, > > > > > > > > I've incidentally discovered your last year discussion in ML [1] (I > > > > hope it rings the > > > > bell) regarding the issue I'm still observing on the same device with > > > > the mainline > > > > kernel. > > > > > > > > The difference between i.MX 6ULL EVK and this particular device, is > > > > that usbotg2 is > > > > used only in host mode with the usb hub integrated on the carrier board > > > > [2] [3]. > > > > > > > > root@colibri-imx6:~# lsusb -s 1:1 --tree > > > > /: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=ci_hdrc/1p, 480M > > > > |__ Port 1: Dev 2, If 0, Class=Hub, Driver=hub/4p, 480M > > > > > > > > However I can't reproduce the same behavior with i.MX 6ULL EVK with > > > > connected > > > > usb hub to usbotg2. Also this behavior can't be reproduced with NXP > > > > downstream > > > > kernel (Linux version 4.9.144) on my device. > > > > > > > > After digging in a bit I found out that this happens only when > > > > autosuspend is > > > > enabled for the usb controller: > > > > echo auto > /sys/bus/usb/devices/1-1/power/control > > > > > > > > It tries to go to suspend mode, but everytime fails and resumes: > > > > root@colibri-imx6:~# cat /sys/bus/usb/devices/1-1/power/runtime_status > > > > suspended > > > > root@colibri-imx6:~# cat /sys/bus/usb/devices/1-1/power/runtime_status > > > > resuming > > > > root@colibri-imx6:~# cat /sys/bus/usb/devices/1-1/power/runtime_status > > > > suspended > > > > root@colibri-imx6:~# cat /sys/bus/usb/devices/1-1/power/runtime_status > > > > suspended > > > > root@colibri-imx6:~# cat /sys/bus/usb/devices/1-1/power/runtime_status > > > > suspended > > > > root@colibri-imx6:~# cat /sys/bus/usb/devices/1-1/power/runtime_status > > > > resuming > > > > > > > > I'm observing ~2 seconds wakeup interrupts handled in ci_irq() in > > > > core.c and > > > > subsequent invocation of imx_controller_resume(). > > > > > > > > Meantime usboh3 remains enabled all the time (though > > > > imx_disable_unprepare_clks() should disable it): > > > > root@colibri-imx6:~# cat /sys/kernel/debug/clk/clk_summary | grep usb > > > > usbphy2_gate 110 0 > > > > 0 0 5 > > > > usbphy1_gate 110 0 > > > > 0 0 5 > > > > pll7_usb_host 110 48000 > > > > 0 0 5 > > > > usbphy2 110 48000 > > > > 0 0 5 > > > > pll3_usb_otg230 48000 > > > > 0 0 5 > > > > usbphy1 000 48000 > > > > 0 0 5 > > > > usboh3 1106600 > > > > 0 0 5 > > > > > > > > While I'm trying to localize the root cause, maybe you can give some > > > > hints where to > > > > look into? > > > > > > > > > > Would you please look below two downstream patches see if it helps? > > > > > > commit e8e95658fe75f3873e06d5ad72a6bf0bad40f068 > > > Author: Li Jun > > > Date: Mon Oct 16 23:13:19 2017 +0800 > > > > > > MLK-16576 usb: phy: mxs: set hold_ring_off for USB2 PLL power up > > > > > > USB2 PLL use ring VCO, when the PLL power up, the ring VCO’s supply > > > also > > > ramp up. There is a possibility that the ring VCO start oscillation at > > > multi nodes in this phase, especially for VCO which has many stages, > > > then > > > the multiwave will kept until PLL power down. Hold_ring_off(bit11) can > > > force the VCO in one determined state when VCO supply start ramp up, > > > to > > > avoid this multiwave issue. Per IC design's suggestion it's better > > > this > > > bit can be off from 25us after pll power up to 25us before USB TX/RX. > > > > > > > > > commit aa4680d844a340a5b6b60484f6e04cb9ba613c65 > > > Author: Peter Chen > > > Date: Wed Sep 7 12:16:59 2016 +0800 > > > > > > MLK-13125 usb: phy: phy-mxs-usb: enable weak 1p1 regulator for imx6ul > > > during suspend > > > > > > For imx6ul PHY, when the system enters suspend, its 1p1 is off by > > > default, > > > that may cause the PHY get inaccurate USB DP/DM value. If the USB > > > wakeup > > > is enabled at this time, the unexpected wakeup may occur when the > > > system > > > enters suspend. > > > > > > In this patch, when the vbus is there, we enable weak 1p1 during the > > > PHY > > > suspend API, in that case, the USB DP/DM will be accurate for USB PHY, > > > then unexpected usb wakeup will not be occurred, especially for the > > > USB > > > charger i
Re: Ryzen7 3700U xhci fails on resume from sleep
Hi Rafael, On Wed, Sep 25, 2019 at 1:36 PM Daniel Drake wrote: > Should the s2idle resume path be modified to call into > pci_update_current_state() to change the current_state to D3hot based > on pmcsr (like the runtime resume path does)? > Or should pci_raw_set_power_state() be modified to also apply the > d3_delay when transitioning from D3cold to D0? Any thoughts here? I also sent a patch implementing the 2nd point above: https://patchwork.kernel.org/patch/11164089/ but no response yet. Thanks Daniel
[PATCH v4] usb: hub: Check device descriptor before resusciation
If a device connected to an xHCI host controller disconnects from the USB bus
and then reconnects, e.g. triggered by a firmware update, then the host
controller automatically activates the connection and the port is enabled. The
implementation of hub_port_connect_change() assumes that if the port is
enabled then nothing has changed. There is no check if the USB descriptors
have changed. As a result, the kernel's internal copy of the descriptors ends
up being incorrect and the device doesn't work properly anymore.
The solution to the problem is for hub_port_connect_change() always to
check whether the device's descriptors have changed before resuscitating
an enabled port.
Signed-off-by: David Heinzelmann
---
Changes in v4:
- changed commit description
Changes in v3:
- changed commit message and description
- fix code style
Changes in v2:
- fix logic error to handle return code from usb_get_device_descriptor()
properly
- fix line endings
---
drivers/usb/core/hub.c | 196 +++--
1 file changed, 111 insertions(+), 85 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 236313f41f4a..fdcfa85b5b12 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4930,6 +4930,91 @@ hub_power_remaining(struct usb_hub *hub)
return remaining;
}
+
+static int descriptors_changed(struct usb_device *udev,
+ struct usb_device_descriptor *old_device_descriptor,
+ struct usb_host_bos *old_bos)
+{
+ int changed = 0;
+ unsignedindex;
+ unsignedserial_len = 0;
+ unsignedlen;
+ unsignedold_length;
+ int length;
+ char*buf;
+
+ if (memcmp(&udev->descriptor, old_device_descriptor,
+ sizeof(*old_device_descriptor)) != 0)
+ return 1;
+
+ if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
+ return 1;
+ if (udev->bos) {
+ len = le16_to_cpu(udev->bos->desc->wTotalLength);
+ if (len != le16_to_cpu(old_bos->desc->wTotalLength))
+ return 1;
+ if (memcmp(udev->bos->desc, old_bos->desc, len))
+ return 1;
+ }
+
+ /* Since the idVendor, idProduct, and bcdDevice values in the
+* device descriptor haven't changed, we will assume the
+* Manufacturer and Product strings haven't changed either.
+* But the SerialNumber string could be different (e.g., a
+* different flash card of the same brand).
+*/
+ if (udev->serial)
+ serial_len = strlen(udev->serial) + 1;
+
+ len = serial_len;
+ for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
+ old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
+ len = max(len, old_length);
+ }
+
+ buf = kmalloc(len, GFP_NOIO);
+ if (!buf)
+ /* assume the worst */
+ return 1;
+
+ for (index = 0; index < udev->descriptor.bNumConfigurations; index++) {
+ old_length = le16_to_cpu(udev->config[index].desc.wTotalLength);
+ length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf,
+ old_length);
+ if (length != old_length) {
+ dev_dbg(&udev->dev, "config index %d, error %d\n",
+ index, length);
+ changed = 1;
+ break;
+ }
+ if (memcmp(buf, udev->rawdescriptors[index], old_length)
+ != 0) {
+ dev_dbg(&udev->dev, "config index %d changed (#%d)\n",
+ index,
+ ((struct usb_config_descriptor *) buf)->
+ bConfigurationValue);
+ changed = 1;
+ break;
+ }
+ }
+
+ if (!changed && serial_len) {
+ length = usb_string(udev, udev->descriptor.iSerialNumber,
+ buf, serial_len);
+ if (length + 1 != serial_len) {
+ dev_dbg(&udev->dev, "serial string error %d\n",
+ length);
+ changed = 1;
+ } else if (memcmp(buf, udev->serial, length) != 0) {
+ dev_dbg(&udev->dev, "serial string changed\n");
+ changed = 1;
+ }
+ }
+
+ kfree(buf);
+ return changed;
+}
+
static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus,
u16 portchange)
{
@@ -5167,7 +5252,9 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
{
struct usb_port *port_dev = hub->ports[port1 - 1];
struc
