[PATCH v15 2/2] usb: typec: ucsi: add support for Cypress CCGx
Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
over I2C interface.
This UCSI I2C driver uses I2C bus driver interface for communicating
with Type-C controller.
Signed-off-by: Ajay Gupta
---
Changes from v1 -> v2
Fixed identation in drivers/usb/typec/ucsi/Kconfig
Changes from v2 -> v3
Fixed most of comments from Heikki
Rename ucsi_i2c_ccg.c -> ucsi_ccg.c
Changes from v3 -> v4
Fixed comments from Andy
Changes from v4 -> v5
Fixed comments from Andy
Changes from v5 -> v6
Fixed review comments from Greg
Changes from v6 -> v7
None
Changes from v7 -> v8
Fixed review comments from Peter
- Removed empty STOP message
- Using stack memory for i2c_transfer()
Changes from v8 -> v9
None
Changes from v9 -> v10
Fixed review comments from Peter
- Use UCSI macros
- Cleanups
Changes from v10 -> v11
Fixed review comments from Peter
- Combined two writes into one
- Used offsetof() for ucsi_data struct
Changes from v11 -> v12
- some cleanup
Changes from v12 -> v13
- changed "u8 buf[1]" -> "u8 data"
Changes from v13 -> v14
- Fixed commend from Heikki and Andy
- Added i2c adapters quirks check in ccg_read
- Removed "irq" field from struct ucsi_ccg
- Reorganize do {} while (); loop
Changes from v14 -> v15
- Fix unnecessary check for "data" in while () condition
drivers/usb/typec/ucsi/Kconfig| 10 +
drivers/usb/typec/ucsi/Makefile | 2 +
drivers/usb/typec/ucsi/ucsi_ccg.c | 307 ++
3 files changed, 319 insertions(+)
create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c
diff --git a/drivers/usb/typec/ucsi/Kconfig b/drivers/usb/typec/ucsi/Kconfig
index e36d6c73c4a4..78118883f96c 100644
--- a/drivers/usb/typec/ucsi/Kconfig
+++ b/drivers/usb/typec/ucsi/Kconfig
@@ -23,6 +23,16 @@ config TYPEC_UCSI
if TYPEC_UCSI
+config UCSI_CCG
+ tristate "UCSI Interface Driver for Cypress CCGx"
+ depends on I2C
+ help
+ This driver enables UCSI support on platforms that expose a
+ Cypress CCGx Type-C controller over I2C interface.
+
+ To compile the driver as a module, choose M here: the module will be
+ called ucsi_ccg.
+
config UCSI_ACPI
tristate "UCSI ACPI Interface Driver"
depends on ACPI
diff --git a/drivers/usb/typec/ucsi/Makefile b/drivers/usb/typec/ucsi/Makefile
index 7afbea512207..2f4900b26210 100644
--- a/drivers/usb/typec/ucsi/Makefile
+++ b/drivers/usb/typec/ucsi/Makefile
@@ -8,3 +8,5 @@ typec_ucsi-y:= ucsi.o
typec_ucsi-$(CONFIG_TRACING) += trace.o
obj-$(CONFIG_UCSI_ACPI)+= ucsi_acpi.o
+
+obj-$(CONFIG_UCSI_CCG) += ucsi_ccg.o
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
b/drivers/usb/typec/ucsi/ucsi_ccg.c
new file mode 100644
index ..de8a43bdff68
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * UCSI driver for Cypress CCGx Type-C controller
+ *
+ * Copyright (C) 2017-2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta
+ *
+ * Some code borrowed from drivers/usb/typec/ucsi/ucsi_acpi.c
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include "ucsi.h"
+
+struct ucsi_ccg {
+ struct device *dev;
+ struct ucsi *ucsi;
+ struct ucsi_ppm ppm;
+ struct i2c_client *client;
+};
+
+#define CCGX_RAB_INTR_REG 0x06
+#define CCGX_RAB_UCSI_CONTROL 0x39
+#define CCGX_RAB_UCSI_CONTROL_STARTBIT(0)
+#define CCGX_RAB_UCSI_CONTROL_STOP BIT(1)
+#define CCGX_RAB_UCSI_DATA_BLOCK(offset) (0xf000 | ((offset) & 0xff))
+
+static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+ struct i2c_client *client = uc->client;
+ const struct i2c_adapter_quirks *quirks = client->adapter->quirks;
+ unsigned char buf[2];
+ struct i2c_msg msgs[] = {
+ {
+ .addr = client->addr,
+ .flags = 0x0,
+ .len= sizeof(buf),
+ .buf= buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .buf= data,
+ },
+ };
+ u32 rlen, rem_len = len, max_read_len = len;
+ int status;
+
+ /* check any max_read_len limitation on i2c adapter */
+ if (quirks && quirks->max_read_len)
+ max_read_len = quirks->max_read_len;
+
+ while (rem_len > 0) {
+ msgs[1].buf = &data[len - rem_len];
+ rlen = min_t(u16, rem_len, max_read_len);
+ msgs[1].len = rlen;
+ put_unaligned_le16(rab, buf);
+ status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE
[PATCH v15 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C controller which can be accessed over I2C. This driver adds I2C bus driver to communicate with Type-C controller. I2C client driver will be part of USB Type-C UCSI driver. Signed-off-by: Ajay Gupta --- Changes from v1 -> v2 None Changes from v2 -> v3 Fixed review comments from Andy and Thierry Rename i2c-gpu.c -> i2c-nvidia-gpu.c Changes from v3 -> v4 Fixed review comments from Andy Changes from v4 -> v5 Fixed review comments from Andy Changes from v5 -> v6 None Changes from v6 -> v7 -> v8 Fixed review comments from Peter - Add implicit STOP for last write message - Add i2c_adapter_quirks with max_read_len and I2C_AQ_COMB flags Changes from v8 -> v9 Fixed review comments from Peter - Drop do_start flag - Use i2c_8bit_addr_from_msg() Changes from v9 -> v10 Fixed review comments from Peter - Dropped I2C_FUNC_SMBUS_EMUL - Dropped local mutex Changes from v10 -> v11 Fixed review comments from Peter - Moved stop in master_xfer at end of message - Change i2c_read without STOP - Dropped I2C_AC_COMB* flags Changes from v11 -> v12 Fixed review comments from Peter - Removed clearing of empty bits - Fix master_xfer for correct stop use Changes from v12 -> v13 Fixed review comments from Peter - Added comments on 4 byte read limitation - Added I2C_AC_COMB* flags Changes from v13 -> v14 -> v15 None Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++ MAINTAINERS | 7 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-nvidia-gpu.c | 368 5 files changed, 403 insertions(+) create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu new file mode 100644 index ..31884d2b2eb5 --- /dev/null +++ b/Documentation/i2c/busses/i2c-nvidia-gpu @@ -0,0 +1,18 @@ +Kernel driver i2c-nvidia-gpu + +Datasheet: not publicly available. + +Authors: + Ajay Gupta + +Description +--- + +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA Turing +and later GPUs and it is used to communicate with Type-C controller on GPUs. + +If your 'lspci -v' listing shows something like the following, + +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1) + +then this driver should support the I2C controller of your GPU. diff --git a/MAINTAINERS b/MAINTAINERS index bd702ad56c7f..5885a0931de9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6843,6 +6843,13 @@ L: linux-a...@vger.kernel.org S: Maintained F: drivers/i2c/i2c-core-acpi.c +I2C CONTROLLER DRIVER FOR NVIDIA GPU +M: Ajay Gupta +L: linux-...@vger.kernel.org +S: Maintained +F: Documentation/i2c/busses/i2c-nvidia-gpu +F: drivers/i2c/busses/i2c-nvidia-gpu.c + I2C MUXES M: Peter Rosin L: linux-...@vger.kernel.org diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 451d4ae50e66..eed827b44068 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985 This driver can also be built as a module. If so, the module will be called i2c-nforce2-s4985. +config I2C_NVIDIA_GPU + tristate "NVIDIA GPU I2C controller" + depends on PCI + help + If you say yes to this option, support will be included for the + NVIDIA GPU I2C controller which is used to communicate with the GPU's + Type-C controller. This driver can also be built as a module called + i2c-nvidia-gpu. + config I2C_SIS5595 tristate "SiS 5595" depends on PCI diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 18b26af82b1c..d499813df038 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)+= i2c-sibyte.o obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o obj-$(CONFIG_SCx200_ACB) += scx200_acb.o obj-$(CONFIG_I2C_FSI) += i2c-fsi.o +obj-$(CONFIG_I2C_NVIDIA_GPU) += i2c-nvidia-gpu.o ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c new file mode 100644 index ..744f5e42636b --- /dev/null +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c @@ -0,0 +1,368 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Nvidia GPU I2C controller Driver + * + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved. + * Author: Ajay Gupta + */ +#include +#include +#include +#include +#include +#include +#include +#include + +#include
[PATCH v15 0/2] Add support for USB Type-C interface on latest NVIDIA GPU
Hi Heikki and Wolfram, These two changes add support for USB Type-C interface on latest NVIDIA GPU card. The Type-C controller used is Cypress CCGx and is over I2C interface. I2C host controller has known limitation of sending STOP after every read. Since each read can be of 4 byte maximum length so there is a limit of 4 byte read. This is mentioned in adapter quirks as "max_read_len = 4" I2C host controller is mainly used for "write-then-read" or "write" messages so added the flag I2C_AQ_COMB_WRITE_THEN_READ in adapter quirks. PATCH[2/2] on ucsi driver now have added logic to check i2c adapter quirks and issues i2c read transfer based on max_read_len quirk settings. This will make sure the read limitation is not affecting I2C host which do not have such limitation. I think the patches should through usb tree because the main functionality is usb Type-C. Thanks Ajay Ajay Gupta (2): i2c: buses: add i2c bus driver for NVIDIA GPU usb: typec: ucsi: add support for Cypress CCGx Documentation/i2c/busses/i2c-nvidia-gpu | 18 ++ MAINTAINERS | 7 + drivers/i2c/busses/Kconfig | 9 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-nvidia-gpu.c | 368 drivers/usb/typec/ucsi/Kconfig | 10 + drivers/usb/typec/ucsi/Makefile | 2 + drivers/usb/typec/ucsi/ucsi_ccg.c | 307 8 files changed, 722 insertions(+) create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c create mode 100644 drivers/usb/typec/ucsi/ucsi_ccg.c -- 2.17.1
RE: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Peter and Andy,
> Shouldn't you return -ETIMEDOUT if count == 0?
> >>> Yes. Good catch. Does the below fix looks ok?
> >>>
> >>> do {
> >>> status = ccg_write(uc, CCGX_RAB_INTR_REG, &data,
> sizeof(data));
> >>> if (status < 0)
> >>> return status;
> >>>
> >>> usleep_range(1, 11000);
> >>>
> >>> status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> >>> sizeof(data));
> >>> if (status < 0)
> >>> return status;
> >>>
> >>> if (!data)
> >>> return 0;
> >>> } while (data && count--);
> >>
> >> Doesn't that condition break out of the loop immediately?
> > How? I didn't get your point? We want to break out when data is zero
> > (interrupt status cleared).
>
> The statement
>
> if (!data)
> return 0;
>
> ensures that 'data' is non-zero when the loop continues, so checking that
> 'data' is non-zero in the while loop test is pointless.
>
> >>> Ah, I see, but why you not reorganize it to put this into do-while loop?
> > We actually need to check data after reading it so will reorganize
> accordingly.
Sorry, my bad, will revise.
Thanks
Ajay
--nvpublic
> > do {
> > read
> > check for data and break out if (!data)
> > write
> > sleep
> > } while (--count);
>
> Here, you have fixed the "issue" (but it doesn't match v14).
>
> Cheers,
> Peter
Re: [GIT PULL] USB driver patches for 4.20-rc1
On Fri, Oct 26, 2018 at 3:02 AM Greg KH wrote: > > Here is the big USB/PHY driver patches for 4.20-rc1 Pulled, Linus
Re: [PATCH v2] HID: hiddev: fix potential Spectre v1
On Fri, 19 Oct 2018, Breno Leitao wrote:
> uref->usage_index can be indirectly controlled by userspace, hence leading
> to a potential exploitation of the Spectre variant 1 vulnerability.
>
> This field is used as an array index by the hiddev_ioctl_usage() function,
> when 'cmd' is either HIDIOCGCOLLECTIONINDEX, HIDIOCGUSAGES or
> HIDIOCSUSAGES.
>
> For cmd == HIDIOCGCOLLECTIONINDEX case, uref->usage_index is compared to
> field->maxusage and then used as an index to dereference field->usage
> array. The same thing happens to the cmd == HIDIOC{G,S}USAGES cases, where
> uref->usage_index is checked against an array maximum value and then it is
> used as an index in an array.
>
> This is a summary of the HIDIOCGCOLLECTIONINDEX case, which matches the
> traditional Spectre V1 first load:
>
> copy_from_user(uref, user_arg, sizeof(*uref))
> if (uref->usage_index >= field->maxusage)
> goto inval;
> i = field->usage[uref->usage_index].collection_index;
> return i;
>
> This patch fixes this by sanitizing field uref->usage_index before using it
> to index field->usage (HIDIOCGCOLLECTIONINDEX) or field->value in
> HIDIOC{G,S}USAGES arrays, thus, avoiding speculation in the first load.
>
> Signed-off-by: Breno Leitao
> Cc:
Applied, thanks.
--
Jiri Kosina
SUSE Labs
[RESEND PATCH v2] usb: dwc2: disable power_down on rockchip for regression
>From 04fbf78e4e569bf872f1ffcb0a6f9b89569dc913 Mon Sep 17 00:00:00 2001 From: Hal Emmerich Date: Thu, 19 Jul 2018 21:48:08 -0500 Subject: [PATCH] usb: dwc2: disable power_down on rockchip devices The bug would let the usb controller enter partial power down, which was formally known as hibernate, upon boot if nothing was plugged in to the port. Partial power down couldn't be exited properly, so any usb devices plugged in after boot would not be usable. Before the name change, params.hibernation was false by default, so _dwc2_hcd_suspend() would skip entering hibernation. With the rename, _dwc2_hcd_suspend() was changed to use params.power_down to decide whether or not to enter partial power down. Since params.power_down is non-zero by default, it needs to be set to 0 for rockchip devices to restore functionality. This bug was reported in the linux-usb thread: REGRESSION: usb: dwc2: USB device not seen after boot The commit that caused this regression is: 6d23ee9caa6790aea047f9aca7f3c03cb8d96eb6 Signed-off-by: Hal Emmerich Acked-by: Minas Harutyunyan --- drivers/usb/dwc2/params.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c index bf7052e037d6..09292dc977e4 100644 --- a/drivers/usb/dwc2/params.c +++ b/drivers/usb/dwc2/params.c @@ -81,6 +81,7 @@ static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg) p->host_perio_tx_fifo_size = 256; p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 << GAHBCFG_HBSTLEN_SHIFT; + p->power_down = 0; } static void dwc2_set_ltq_params(struct dwc2_hsotg *hsotg) -- 2.11.0
Re: EPROTO when USB 3 GbE adapters are under load
On Fri, 26 Oct 2018, Mathias Nyman wrote: > On 25.10.2018 20:28, Alan Stern wrote: > > On Thu, 25 Oct 2018, Mathias Nyman wrote: > > > >> On 25.10.2018 12:52, Hao Wei Tee wrote: > >>> On 25/10/18 4:45 PM, Mathias Nyman wrote: > Reproducing the issue with a recent kernel with xhci traces enabled > should show the reason for EPROTO error. > > Add xhci traces before triggering the issue with: > > mount -t debugfs none /sys/kernel/debug > echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb > echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable > > after issue is triggered save and send the trace at > /sys/kernel/debug/tracing/trace > Note that it might be huge > >>> > >>> Thanks for the suggestion. > >>> > >>> Here[1] is (part of) the trace starting about 250 lines before the EPROTO > >>> happens. > >>> > >>> [1]: > >>> https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace > >>> > >>> The first error happens at line 243 (timestamp 8144.248398) coinciding > >>> with the start of errors spewed into dmesg: > >>> > >>> [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > >>> [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > >>> [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 > >>> [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 > >> > >> Thanks, > >> xHC controller reports that there was a transaction error on one of the > >> bulk TRBs. > >> > >> The transaction error causes the endpoint to halt (host side halt only). > >> Xhci driver resets the host side endpoint to recover from the halt, > >> then returns the broken URB (TRB) with -EPROTO status, and then moves past > >> this TRB. > > > > The host side of the endpoint should remain stopped until after the > > URB's completion routine has had a chance to carry out error recovery. > > Doesn't this imply the xHCI driver shouldn't reset the host-side > > endpoint until after the giveback call returns? > > True, on xhci side we could probably reset the endpoint, and even move the > dequeue pointer to the next TRB, but make sure the endpoint is not restarted > yet. > > The URB with -EPROTO status is given back in interrupt context, so this might > limit > a bit what the higher-layer drivers can do in giveback. One thing they can do is unlink any URBs still remaining in the endpoint's queue, thus preventing any confusion from stale data when the endpoint restarts. It's okay to call usb_unlink_urb() in interrupt context. > Now thinking about it, xhci driver calls the URB giveback in the same > "Transaction error" > interrupt handler, after first queuing areset endpoint and a set TR Deq > pointer command. > The endpoint is only restarted after those commands finish, in the command > completion interrupt > handler. > > So in that sense the endpoint shouldn't be restarted until the next interrupt > is handled, > which shouldn't be possible before the URB giveback call returned in the > previous interrupt handler. > > Well, at least not as long as we are in hard interrupt. > > I think I need to dig a bit more into this. > > > > >> Interesting thing here is that each TRB in the queue after the transaction > >> error > >> also triggers a transaction error. > >> > >> This might be a data toggle/sequence number sync issue. > > > > It's more likely to be a problem on the device side. Data toggle or > > sequence number issues tend to be self-repairing (albeit with some data > > loss) after a little while. > > Ok, thanks, not spending too much time looking into that then. Important point: The device's problem might be caused by the kernel sending it a command it can't handle. So maybe the way to fix the problem may be to change the upper-layer driver; this happens sometimes. Other times it really is just a bug in the device. > >> The host side endpoint reset clears the host side sequence number, > >> and host expects device side endpoint to be reset and sequence to be > >> cleared as well > >> as a result of returning -EPROTO. > >> If I remember correctly xhci driver does not wait for device side endpoint > >> to be reset, > >> so if there are TRBs in the queue they will be transferred, with a > >> cleared sequence number > >> out of sync with the device side. > > > > That's why it's important to wait until after the higher-layer driver > > has had a chance to unlink the URBs that may be in the endpoint queue. > > The driver may even want to reset the device. > > Would it make sense to prevent endpoint from running until usb core calls > hcd->driver->endpoint_reset? > That is for halted endpoints, that returned URB with -EPROTO status. The HCD shouldn't worry about that. The higher-layer driver is responsible for fixing the error that caused the endpoint to halt, unlinking any remaining URBs, and clearing the halt. Alan Stern
Re: USB-C device hotplug issue
On Fri, 26 Oct 2018, Dennis Wassenberg wrote: > >> --- a/drivers/usb/core/hub.c > >> +++ b/drivers/usb/core/hub.c > >> @@ -2815,7 +2815,9 @@ static int hub_port_reset(struct usb_hub *hub, int > >> port1, > >>USB_PORT_FEAT_C_BH_PORT_RESET); > >>usb_clear_port_feature(hub->hdev, port1, > >>USB_PORT_FEAT_C_PORT_LINK_STATE); > >> - usb_clear_port_feature(hub->hdev, port1, > >> + > >> + if (!warm) > >> + usb_clear_port_feature(hub->hdev, port1, > >>USB_PORT_FEAT_C_CONNECTION); > >> > >>/* > > > > The key fact is that connection events can get lost if they happen to > > occur during a port reset. > Yes. > > > > I'm not entirely certain of the logic here, but it looks like the > > correct test to add should be "if (udev != NULL)", not "if (!warm)". > > Perhaps Mathias can confirm this > I don't know if clearing the USB_PORT_FEAT_C_CONNECTION bit is > correct in case of a non warm reset. In my case I always observed a > warm reset because of the link state change. > Thats why I checked the warm variable to not change the behavoir for > cases a didn't checked for the first shot. (The syntax of that last sentence is pretty mangled; I can't understand it.) Think of it this way: If a device was not attached before the reset, we don't want to clear the connect-change status because then we wouldn't recognize a device that was plugged in during the reset. If a device was attached before the reset, we don't want any connect-change status which might be provoked by the reset to last, because we don't want the core to think that the device was unplugged and replugged when all that happened was a reset. So the important criterion is whether or not a device was attached to the port when the reset started. It's something of a coincidence that you only observe warm resets when there's nothing attached. > During the first run of usb_hub_reset the udev is NULL because in > this situation the device is not attached logically. > > [ 112.889810] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, portchange: > 0x40! > [ 113.201192] usb 4-1-port1: XXX: hub_port_reset: udev:(nil)! > [ 113.201198] usb 4-1-port1: XXX: hub_port_reset (not clearing > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! > [ 113.253612] usb 4-1-port1: XXX: port_event: portstatus: 0x203, portchange: > 0x1! > [ 113.377208] usb 4-1-port1: XXX: hub_port_reset: udev: 88046b302800! > [ 113.377214] usb 4-1-port1: XXX: hub_port_reset (not clearing > USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! > [ 113.429478] usb 4-1.1: new SuperSpeed USB device number 7 using xhci_hcd > [ 113.442370] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5596 > [ 113.442376] usb 4-1.1: New USB device strings: Mfr=1, Product=2, > SerialNumber=3 > [ 113.442381] usb 4-1.1: Product: Ultra T C > [ 113.442385] usb 4-1.1: Manufacturer: SanDisk > [ 113.442388] usb 4-1.1: SerialNumber: 4C530001131013121031 > > Or maybe we can skip clearing the USB_PORT_FEAT_C_CONNECTION bit in > case of hub_port_reset completely without any other check? See above. Alan Stern
Re: EPROTO when USB 3 GbE adapters are under load
On 25.10.2018 20:28, Alan Stern wrote: On Thu, 25 Oct 2018, Mathias Nyman wrote: On 25.10.2018 12:52, Hao Wei Tee wrote: On 25/10/18 4:45 PM, Mathias Nyman wrote: Reproducing the issue with a recent kernel with xhci traces enabled should show the reason for EPROTO error. Add xhci traces before triggering the issue with: mount -t debugfs none /sys/kernel/debug echo 81920 > /sys/kernel/debug/tracing/buffer_size_kb echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable after issue is triggered save and send the trace at /sys/kernel/debug/tracing/trace Note that it might be huge Thanks for the suggestion. Here[1] is (part of) the trace starting about 250 lines before the EPROTO happens. [1]: https://gist.githubusercontent.com/angelsl/fdd04d2bded3a41029122b0536c00944/raw/b8e9f7d2695ac030b7f3dd53a1a9c3f37da7b7a0/trace The first error happens at line 243 (timestamp 8144.248398) coinciding with the start of errors spewed into dmesg: [ 8144.245359] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.248837] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.252392] r8152 2-2:1.0 enp0s20f0u2: Rx status -71 [ 8144.255987] r8152 2-2:1.0 enp0s20f0u2: Stop submitting intr, status -71 Thanks, xHC controller reports that there was a transaction error on one of the bulk TRBs. The transaction error causes the endpoint to halt (host side halt only). Xhci driver resets the host side endpoint to recover from the halt, then returns the broken URB (TRB) with -EPROTO status, and then moves past this TRB. The host side of the endpoint should remain stopped until after the URB's completion routine has had a chance to carry out error recovery. Doesn't this imply the xHCI driver shouldn't reset the host-side endpoint until after the giveback call returns? True, on xhci side we could probably reset the endpoint, and even move the dequeue pointer to the next TRB, but make sure the endpoint is not restarted yet. The URB with -EPROTO status is given back in interrupt context, so this might limit a bit what the higher-layer drivers can do in giveback. Now thinking about it, xhci driver calls the URB giveback in the same "Transaction error" interrupt handler, after first queuing areset endpoint and a set TR Deq pointer command. The endpoint is only restarted after those commands finish, in the command completion interrupt handler. So in that sense the endpoint shouldn't be restarted until the next interrupt is handled, which shouldn't be possible before the URB giveback call returned in the previous interrupt handler. Well, at least not as long as we are in hard interrupt. I think I need to dig a bit more into this. Interesting thing here is that each TRB in the queue after the transaction error also triggers a transaction error. This might be a data toggle/sequence number sync issue. It's more likely to be a problem on the device side. Data toggle or sequence number issues tend to be self-repairing (albeit with some data loss) after a little while. Ok, thanks, not spending too much time looking into that then. The host side endpoint reset clears the host side sequence number, and host expects device side endpoint to be reset and sequence to be cleared as well as a result of returning -EPROTO. If I remember correctly xhci driver does not wait for device side endpoint to be reset, so if there are TRBs in the queue they will be transferred, with a cleared sequence number out of sync with the device side. That's why it's important to wait until after the higher-layer driver has had a chance to unlink the URBs that may be in the endpoint queue. The driver may even want to reset the device. Would it make sense to prevent endpoint from running until usb core calls hcd->driver->endpoint_reset? That is for halted endpoints, that returned URB with -EPROTO status. -Mathias
[PATCH] usb: quirks: Add delay-init quirk for Corsair K70 LUX RGB
Following on from this patch: https://lkml.org/lkml/2017/11/3/516,
Corsair K70 LUX RGB keyboards also require the DELAY_INIT quirk to
start correctly at boot.
Dmesg output:
usb 1-6: string descriptor 0 read error: -110
usb 1-6: New USB device found, idVendor=1b1c, idProduct=1b33
usb 1-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 1-6: can't set config #1, error -110
Signed-off-by: Emmanuel Pescosta
---
drivers/usb/core/quirks.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
index 178d6c6063c0..2632d86fd75e 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -391,6 +391,9 @@ static const struct usb_device_id usb_quirk_list[] = {
{ USB_DEVICE(0x1b1c, 0x1b20), .driver_info = USB_QUIRK_DELAY_INIT |
USB_QUIRK_DELAY_CTRL_MSG },
+ /* Corsair K70 LUX RGB */
+ { USB_DEVICE(0x1b1c, 0x1b33), .driver_info = USB_QUIRK_DELAY_INIT },
+
/* Corsair K70 LUX */
{ USB_DEVICE(0x1b1c, 0x1b36), .driver_info = USB_QUIRK_DELAY_INIT },
--
2.19.1
[PATCH -next] USB: serial: mos7840: remove set but not used variables 'st, data1, iflag'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/usb/serial/mos7840.c: In function 'mos7840_interrupt_callback':
drivers/usb/serial/mos7840.c:604:14: warning:
variable 'st' set but not used [-Wunused-but-set-variable]
drivers/usb/serial/mos7840.c: In function 'mos7840_write':
drivers/usb/serial/mos7840.c:1303:17: warning:
variable 'data1' set but not used [-Wunused-but-set-variable]
drivers/usb/serial/mos7840.c:1700:11: warning:
variable 'iflag' set but not used [-Wunused-but-set-variable]
They are never used since introduction in
commit 3f5429746d91 ("USB: Moschip 7840 USB-Serial Driver")
Signed-off-by: YueHaibing
---
drivers/usb/serial/mos7840.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 88828b4..bfbf75b 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -601,7 +601,7 @@ static void mos7840_interrupt_callback(struct urb *urb)
struct usb_serial *serial;
__u16 Data;
unsigned char *data;
- __u8 sp[5], st;
+ __u8 sp[5];
int i, rv = 0;
__u16 wval, wreg = 0;
int status = urb->status;
@@ -644,7 +644,6 @@ static void mos7840_interrupt_callback(struct urb *urb)
sp[1] = (__u8) data[1];
sp[2] = (__u8) data[2];
sp[3] = (__u8) data[3];
- st = (__u8) data[4];
for (i = 0; i < serial->num_ports; i++) {
mos7840_port = mos7840_get_port_private(serial->port[i]);
@@ -1300,7 +1299,6 @@ static int mos7840_write(struct tty_struct *tty, struct
usb_serial_port *port,
struct urb *urb;
/* __u16 Data; */
const unsigned char *current_position = data;
- unsigned char *data1;
if (mos7840_port_paranoia_check(port, __func__))
return -1;
@@ -1361,7 +1359,6 @@ static int mos7840_write(struct tty_struct *tty, struct
usb_serial_port *port,
mos7840_bulk_out_data_callback, mos7840_port);
}
- data1 = urb->transfer_buffer;
dev_dbg(&port->dev, "bulkout endpoint is %d\n",
port->bulk_out_endpointAddress);
if (mos7840_port->has_led)
@@ -1697,7 +1694,6 @@ static void mos7840_change_port_settings(struct
tty_struct *tty,
{
int baud;
unsigned cflag;
- unsigned iflag;
__u8 lData;
__u8 lParity;
__u8 lStop;
@@ -1729,7 +1725,6 @@ static void mos7840_change_port_settings(struct
tty_struct *tty,
lParity = LCR_PAR_NONE;
cflag = tty->termios.c_cflag;
- iflag = tty->termios.c_iflag;
/* Change the number of bits */
switch (cflag & CSIZE) {
--
2.7.0
Re: [PATCH v14 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Thu, Oct 25, 2018 at 03:33:53PM -0700, aj...@nvidia.com wrote:
> Latest NVIDIA GPU cards have a Cypress CCGx Type-C controller
> over I2C interface.
>
> This UCSI I2C driver uses I2C bus driver interface for communicating
> with Type-C controller.
> + /*
> + * Flush CCGx RESPONSE queue by acking interrupts. Above ucsi control
> + * register write will push response which must be cleared.
> + */
> + do {
> + status = ccg_read(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
> + if (status < 0)
> + return status;
> +
> + if (!data)
> + return 0;
> +
> + status = ccg_write(uc, CCGX_RAB_INTR_REG, &data, sizeof(data));
> + if (status < 0)
> + return status;
> +
> + usleep_range(1, 11000);
> + } while (data && --count);
I don't see any point to check data here. How can it be different from the
check above?
> + return -ETIMEDOUT;
--
With Best Regards,
Andy Shevchenko
[PATCH] Add support of TI ICDI to USB simple serial device
TI In-Circuit Debug Interface (ICDI) is a debugging interface for TI ARM
microcontrollers. It has four USB interfaces and the first two of them are
presented as standard ACM serial device. The 3rd interface is the debugging
interface and it can be driven as a Linux USB simple serial device. With it,
debugging sessions and firmware up/down loading are supported on Linux.
Signed-off-by: Dashi Cao
---
drivers/usb/serial/usb-serial-simple.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/usb/serial/usb-serial-simple.c
b/drivers/usb/serial/usb-serial-simple.c
index 4d0273508043..ae43088b659e 100644
--- a/drivers/usb/serial/usb-serial-simple.c
+++ b/drivers/usb/serial/usb-serial-simple.c
@@ -109,6 +109,11 @@ DEVICE(suunto, SUUNTO_IDS);
{ USB_DEVICE(0x908, 0x0004) }
DEVICE(siemens_mpi, SIEMENS_IDS);
+/* TI In-Circuit Debug Interface */
+#define ICDI_IDS() \
+ { USB_DEVICE_INTERFACE_CLASS(0x1cbe, 0x00fd, USB_CLASS_VENDOR_SPEC) }
+DEVICE(ti_icdi, ICDI_IDS);
+
/* All of the above structures mushed into two lists */
static struct usb_serial_driver * const serial_drivers[] = {
&carelink_device,
@@ -124,6 +129,7 @@ static struct usb_serial_driver * const serial_drivers[] = {
&hp4x_device,
&suunto_device,
&siemens_mpi_device,
+ &ti_icdi_device,
NULL
};
@@ -141,6 +147,7 @@ static const struct usb_device_id id_table[] = {
HP4X_IDS(),
SUUNTO_IDS(),
SIEMENS_IDS(),
+ ICDI_IDS(),
{ },
};
MODULE_DEVICE_TABLE(usb, id_table);
--
2.11.0
[GIT PULL] USB driver patches for 4.20-rc1
The following changes since commit 0238df646e6224016a45505d2c111a24669ebe21: Linux 4.19-rc7 (2018-10-07 17:26:02 +0200) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git tags/usb-4.20-rc1 for you to fetch changes up to b8d9ee24493d862fbfeb3d209c032647f6073d5d: usb: phy: ab8500: silence some uninitialized variable warnings (2018-10-18 19:44:39 +0200) USB/PHY patches for 4.20-rc1 Here is the big USB/PHY driver patches for 4.20-rc1 Lots of USB changes in here, primarily in these areas: - typec updates and new drivers - new PHY drivers - dwc2 driver updates and additions (this old core keeps getting added to new devices.) - usbtmc major update based on the industry group coming together and working to add new features and performance to the driver. - USB gadget additions for new features - USB gadget configfs updates - chipidea driver updates - other USB gadget updates - USB serial driver updates - renesas driver updates - xhci driver updates - other tiny USB driver updates All of these have been in linux-next for a while with no reported issues. Signed-off-by: Greg Kroah-Hartman Adam Thomson (7): dt-bindings: connector: Add support for USB-PD PPS APDOs to bindings dt-bindings: usb: fusb302: Use usb-connector bindings for configuration usb: typec: fusb302: Populate tcpc fwnode for TCPM property handling usb: typec: fusb302: Correct spelling mistake for toggling state usb: typec: fusb302: Resolve fixed power role contract setup usb: typec: tcpm: Fix APDO PPS order checking to be based on voltage usb: typec: tcpm: Report back negotiated PPS voltage and current Alan Stern (3): USB: OHCI: Remove USB bus reset delay from OHCI handover code USB: gadget core: Issue ->disconnect() callback from usb_gadget_disconnect() USB: net2280: Remove ->disconnect() callback from net2280_pullup() Alexandre Belloni (1): usb: gadget: udc: atmel: handle at91sam9rl PMC Andreas Kemnade (1): phy: phy-twl4030-usb: fix denied runtime access Andreas Pape (1): usb: gadget: f_uac2: disable IN/OUT ep if unused Andy Shevchenko (1): USB: wusbcore: Switch to bitmap_zalloc() Anshuman Gupta (1): xhci: Avoid USB autosuspend when resuming USB2 ports. Arnd Bergmann (1): usb: dwc3: add EXTCON dependency for qcom Biju Das (2): dt-bindings: usb: renesas_usbhs: Add support for r8a7744 dt-bindings: usb-xhci: Document r8a7744 support Bjørn Mork (1): usb: export firmware port location in sysfs Can Guo (4): phy: Update PHY power control sequence phy: General struct and field cleanup phy: Add QMP phy based UFS phy support for sdm845 dt-bindings: phy-qcom-qmp: Add UFS phy compatible string for sdm845 Chunfeng Yun (8): usb: mtu3: disable vbus rise/fall interrupts of ltssm usb: core: phy: clean up return value check about devm_of_phy_get_by_index() usb: xhci-mtk: use maximum ESIT payload of endpiont context usb: xhci-mtk: fix ISOC error when interval is zero usb: xhci-mtk: improve bandwidth scheduling usb: xhci-mtk: supports bandwidth scheduling with multi-TT usb: xhci-mtk: supports SSP without external USB3 gen2 hub usb: mtu3: disable vbus rise/fall interrupts of ltssm Colin Ian King (8): USB: typec: fsusb302: remove unused variables snk_pdo and toggling_mode_name usb: phy: mxs: fix spelling mistake "stardard" -> "standard" USB: serial: cypress_m8: fix spelling mistake "retreiving" -> "retrieving" usb: gadget: fix spelling mistakeis "[En]queing" -> "[En]queuing" usb: phy: mxs: fix spelling mistake "stardard" -> "standard" usb: gadget: fix spelling mistakeis "[En]queing" -> "[En]queuing" usb: core: fix memory leak on port_dev_path allocation usbip: tools: fix atoi() on non-null terminated string Corentin Labbe (1): usb: host: Replace empty define with do while Dan Carpenter (1): usb: phy: ab8500: silence some uninitialized variable warnings Ding Xiang (1): usb: misc: fix obsolete function Douglas Anderson (3): dt-bindings: phy: qcom-qmp: Cleanup the 'reg' documentation as per review phy: qcom-qmp: Quiet -EPROBE_DEFER from qcom_qmp_phy_probe() phy: qcom-qusb2: Quiet -EPROBE_DEFER from qusb2_phy_probe() Fabrice Gasnier (4): usb: dwc2: get optional vbus-supply regulator once usb: dwc2: fix a race with external vbus supply usb: dwc2: fix call to vbus supply exit routine, call it unlocked usb: dwc2: fix unbalanced use of external vbus-supply Fabrizio Castro (6): dt-bindings: usb-xhci: Add r8a774a1 support dt-bindings: usb: renesas_usbhs: Add r8a774a1 support usb: gadget: udc: renesas_usb3: Add r8a774a1 support
Re: USB-C device hotplug issue
Hi all, On 10/25/18 4:46 PM, Alan Stern wrote: > On Thu, 25 Oct 2018, Dennis Wassenberg wrote: > >> Hi all, >> >> I have a question regarding the usb hub driver (drivers/usb/core/hub.c). >> >> There is the following scenario. I am using the Lenovo T580 device with the >> Lenovo UltraDock CS18 docking station. If I plug an usb-c device to the >> docking station there is one device which will not be recognized (SanDisk >> Ultra USB-C Flash Drive, >> https://www.sandisk.com/home/mobile-device-storage/ultra-usb-type-c). An >> other usb-c devices (SanDisk Extreme 900 SSD, >> https://www.sandisk.com/home/ssd/extreme-900-ssd) works fine. I don’t have >> that much USB-C devices available, so there is one device working and the >> other one not. >> >> I made some analysis of the situation where I attached the SanDisk Ultra >> USB-C Flash Drive. >> I added some debug logs in drivers/usb/core/hub.c in port_event and >> hub_port_reset and activated all dynamic debug prints in hub.c. The output >> is the following: >> >> [ 724.110784] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, >> portchange: 0x40! >> [ 724.110789] usb 4-1-port1: link state change >> [ 724.114953] usb 4-1-port1: do warm reset >> [ 724.168109] usb 4-1-port1: not warm reset yet, waiting 50ms >> [ 724.220768] usb 4-1-port1: not warm reset yet, waiting 200ms >> [ 724.425188] usb 4-1-port1: XXX: hub_port_reset (before clear >> USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x1! >> [ 724.425906] usb 4-1-port1: XXX: hub_port_reset (after clear >> USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 724.477429] hub 4-1:1.0: state 7 ports 4 chg evt 0002 >> [ 724.477980] usb 4-1-port1: XXX: port_event: portstatus: 0x203, >> portchange: 0x0! >> >> The same situation with SanDisk Extreme 900 SSD: >> >> [ 863.647484] hub 4-1:1.0: state 7 ports 4 chg evt 0002 >> [ 863.647965] usb 4-1-port1: XXX: port_event: portstatus: 0x203, >> portchange: 0x1! >> [ 863.648305] usb 4-1-port1: status 0203, change 0001, 10.0 Gb/s >> [ 863.758573] usb 4-1-port1: debounce total 100ms stable 100ms status 0x203 >> [ 863.773495] usb 4-1-port1: XXX: hub_port_reset (before clear >> USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 863.773699] usb 4-1-port1: XXX: hub_port_reset (after clear >> USB_PORT_FEAT_C_CONNECTION): 0x203, portchange: 0x0! >> [ 863.826311] usb 4-1.1: new SuperSpeedPlus USB device number 6 using >> xhci_hcd >> [ 863.840002] usb 4-1.1: udev 6, busnum 4, minor = 389 >> [ 863.840010] usb 4-1.1: New USB device found, idVendor=0781, idProduct=5593 >> [ 863.840014] usb 4-1.1: New USB device strings: Mfr=2, Product=3, >> SerialNumber=1 >> [ 863.840018] usb 4-1.1: Product: EXTREME900 >> [ 863.840022] usb 4-1.1: Manufacturer: SanDisk >> [ 863.840026] usb 4-1.1: SerialNumber: 1019DF56 >> >> So, at first I am wondering if the usb hub port was in USB_SS_PORT_LS_U3 if >> I attach the SanDisk Ultra USB-C Flash Drive. In case of the SanDisk Extreme >> 900 the usb hub port is in USB_SS_PORT_LS_U0. What’ the reason for that? >> >> I read >> https://www.kernel.org/doc/html/v4.14/driver-api/usb/power-management.html. >> In this test usb core was bootet wird autosuspend=-1.Additionally the >> /power/pm_qos_no_power_off sysfs variable is always set to 1. >> Nevertheless the hub port is in LS U3, but only using SanDisk Ultra USB-C >> Flash Drive. >> >> I looked through the code and wondered why the USB_PORT_FEAT_C_CONNECTION >> bit is cleared at successful warm usb 3 reset in hub_port_reset. >> In case of the Ultra USB-C Flash Drive at first the link state change is >> detected. Directly after executing the actual hub_port_reset the >> USB_PORT_FEAT_C_CONNECTION bit will change to 1. But the hub_port_reset code >> will set this bit to 0. At the next run the bit remains 0 and the >> connect_change flag in port_event will not be set. That means the >> USB_PORT_FEAT_C_CONNECTION flag will be cleared without taking it into >> account. That’s why this device will not be detected correctly. >> >> If I modify the code in this way that I did’t clear the >> USB_PORT_FEAT_C_CONNECTION bit in hub_port_reset on warm usb3 reset this >> flag is still set during the next run. This makes sure that the >> connect_change flag in port_event is set and the usb device is detected >> correctly. >> Is this code change correct or will it break other use cases? Are there any >> other ways to make the kernel detect that usb device at the docking station? >> >> Please refer to the output of the modified usb_hub_reset code: >> >> [ 121.566344] hub 4-1:1.0: state 7 ports 4 chg evt 0002 >> [ 121.566805] usb 4-1-port1: XXX: port_event: portstatus: 0x2c0, >> portchange: 0x40! >> [ 121.566810] usb 4-1-port1: link state change >> [ 121.573481] usb 4-1-port1: do warm reset >> [ 121.625297] usb 4-1-port1: not warm reset yet, waiting 50ms >> [ 121.677854] usb 4-1-port1: not warm reset yet, waiting 200ms >> [ 121.8810
Re: [PATCH v13 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Thu, Oct 25, 2018 at 09:55:47PM +, Ajay Gupta wrote:
> Hi Heikki and Andy
> [...]
> > > > Shouldn't you return -ETIMEDOUT if count == 0?
> > > Yes. Good catch. Does the below fix looks ok?
> > >
> > > do {
> > > status = ccg_write(uc, CCGX_RAB_INTR_REG, &data,
> > > sizeof(data));
> > > if (status < 0)
> > > return status;
> > >
> > > usleep_range(1, 11000);
> > >
> > > status = ccg_read(uc, CCGX_RAB_INTR_REG, &data,
> > > sizeof(data));
> > > if (status < 0)
> > > return status;
> > >
> > > if (!data)
> > > return 0;
> > > } while (data && count--);
> >
> > Doesn't that condition break out of the loop immediately?
> How? I didn't get your point? We want to break out when data is
> zero (interrupt status cleared).
Sorry Ajay. My brain interpreted that "while" as an "if" statement :-)
thanks,
--
heikki
