RE: [PATCH v4 2/2] usb: typec: ucsi: add support for Cypress CCGx
Hi Andy,
> > 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.
>
> > Changes from v3 -> v4
> > Fixed comments from Andy
>
> Unfortunatelly not all comments was addressed, see below.
>
> > + if (err == ARRAY_SIZE(msgs)) {
> > + err = 0;
> > + } else if (err >= 0) {
> > + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> > + return -EIO;
> > + }
>
> Shouldn't be simple
> if (err < 0) {
> ...
> return err;
> }
>
> ?
Ok, will fix
>
> > + if (err == ARRAY_SIZE(msgs)) {
> > + err = 0;
> > + } else if (err >= 0) {
> > + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> > + return -EIO;
> > + }
>
> Ditto.
ok
>
> > + struct device *dev = uc->dev;
> > + u8 data[64];
> > + int err;
>
> > + unsigned int count = 10;
>
> Move this line upper a bit.
ok
>
> > + unsigned char buf[3] = {
> > + CCGX_I2C_RAB_INTR_REG & 0xff, CCGX_I2C_RAB_INTR_REG >>
> > + 8, 0x0};
>
> This should follow the style you applied earlier in the same file.
>
> Also, & 0xff is redundant (better just to use >> 0).
Ok, even ">> 0" will be redundant so will just drop "& 0xff".
> > + struct ucsi_ccg *uc = container_of(ppm,
> > + struct ucsi_ccg, ppm);
>
> One line?
ok
>
> > +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control
> > +*ctrl) {
> > + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
>
> > + int err = 0;
>
> Redundant assignment.
ok
>
> > +
> > + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> > + err = ucsi_ccg_send_data(uc);
> > +
> > + return err;
> > +}
>
> > +static int ucsi_ccg_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
>
> One line?
It crosses column width of 80. It takes total 85.
>
> > +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> > + {"ccgx-ucsi", 0},
>
> > + {},
>
> Terminator better w/o comma.
Ok.
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
>
Thanks
Ajay
--
nvpublic
--
> --
> With Best Regards,
> Andy Shevchenko
[PATCH v5 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
drivers/usb/typec/ucsi/Kconfig| 10 +
drivers/usb/typec/ucsi/Makefile | 2 +
drivers/usb/typec/ucsi/ucsi_ccg.c | 390 ++
3 files changed, 402 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 e36d6c7..7811888 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 7afbea5..2f4900b 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 000..1e7c3b2
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -0,0 +1,390 @@
+// 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;
+ int irq;
+};
+
+#define CCGX_I2C_RAB_DEVICE_MODE 0x00
+#define CCGX_I2C_RAB_READ_SILICON_ID 0x2
+#define CCGX_I2C_RAB_INTR_REG 0x06
+#define CCGX_I2C_RAB_FW1_VERSION 0x28
+#define CCGX_I2C_RAB_FW2_VERSION 0x20
+#define CCGX_I2C_RAB_UCSI_CONTROL 0x39
+#define CCGX_I2C_RAB_UCSI_CONTROL_STARTBIT(0)
+#define CCGX_I2C_RAB_UCSI_CONTROL_STOP BIT(1)
+#define CCGX_I2C_RAB_RESPONSE_REG 0x7E
+#define CCGX_I2C_RAB_UCSI_DATA_BLOCK 0xf000
+
+static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+ struct device *dev = uc->dev;
+ struct i2c_client *client = uc->client;
+ unsigned char buf[2];
+ struct i2c_msg msgs[] = {
+ {
+ .addr = client->addr,
+ .flags = 0x0,
+ .len= 0x2,
+ .buf= buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = I2C_M_RD,
+ .buf= data,
+ },
+ };
+ u32 rlen, rem_len = len;
+ int err;
+
+ while (rem_len > 0) {
+ msgs[1].buf = [len - rem_len];
+ rlen = min_t(u16, rem_len, 4);
+ msgs[1].len = rlen;
+ put_unaligned_le16(rab, buf);
+ err = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (err < 0) {
+ dev_err(dev, "i2c_transfer failed, err %d\n", err);
+ return err;
+ }
+ rab += rlen;
+ rem_len -= rlen;
+ }
+
+ return 0;
+}
+
+static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+ struct device *dev = uc->dev;
+ struct i2c_client *client = uc->client;
+ unsigned char buf[2];
+ struct i2c_msg msgs[] = {
+ {
+ .addr = client->addr,
+ .flags = 0x0,
+ .len= 0x2,
+ .buf= buf,
+ },
+ {
+ .addr = client->addr,
+ .flags = 0x0,
+ .buf= data,
+ .len= len,
+
[PATCH v5 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 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 | 405 5 files changed, 440 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 000..31884d2 --- /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 9ad052a..2d1c5a1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6797,6 +6797,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 451d4ae..eed827b 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 18b26af..d499813 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 000..2ce6706 --- /dev/null +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c @@ -0,0 +1,405 @@ +// 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 + +/* I2C definitions */ +#define I2C_MST_CNTL 0x00 +#define I2C_MST_CNTL_GEN_START BIT(0) +#define I2C_MST_CNTL_GEN_STOP BIT(1) +#define I2C_MST_CNTL_CMD_NONE (0 << 2) +#define I2C_MST_CNTL_CMD_READ (1 << 2) +#define I2C_MST_CNTL_CMD_WRITE (2 << 2) +#define I2C_MST_CNTL_GEN_RAB BIT(4) +#define I2C_MST_CNTL_BURST_SIZE_SHIFT 6 +#define I2C_MST_CNTL_GEN_NACK BIT(28) +#define I2C_MST_CNTL_STATUSGENMASK(30, 29) +#define I2C_MST_CNTL_STATUS_OKAY (0 << 29) +#define I2C_MST_CNTL_STATUS_NO_ACK (1 << 29) +#define I2C_MST_CNTL_STATUS_TIMEOUT(2 << 29) +#define I2C_MST_CNTL_STATUS_BUS_BUSY (3 << 29) +#define I2C_MST_CNTL_CYCLE_TRIGGER BIT(31) + +#define I2C_MST_ADDR 0x04 +#define I2C_MST_ADDR_DAB 0 + +#define I2C_MST_I2C0_TIMING0x08 +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ
RE: [PATCH v4 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
Hi Andy
> > > Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C
> > > controller which can be accessed over I2C.
> > > + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP
> > > + | I2C_MST_CNTL_GEN_RAB);
> >
> > "|" should be on previous line to follow common style in this module.
> The "|" here is to clear the bit together. [val &= ~(X | Y | Z)] Style in
> this
> module is still followed. Please see first line which does "|" to val.
Ok, I got your point of putting "|" on line above and will fix it.
Thanks
Ajay
--
nvpublic
--
>
> > > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > > +
> > > + return i2c_check_status(i2cd); }
> > > +
> > > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> > > + struct i2c_msg *msgs, int num) {
> > > + struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> > > + struct device *dev = i2cd->dev;
> > > + int status;
> > > + int i, j;
> >
Re: A question about 'lsusb'
On Fri, Aug 31, 2018 at 09:13:31AM -0700, Greg KH wrote: > On Thu, Aug 30, 2018 at 05:10:53PM -0400, Alan Stern wrote: > > On Thu, 30 Aug 2018, Faisal Mehmood wrote: > > > > > Based on my (limited) understanding if I were to disable udev, the > > > userspace should not be able to enumerate/interact with any newly > > > connected device since udev handles uevents generated by kernel. > > > (right?) > > > > That's not quite right. Yes, udev does handle uevents. But a newly > > connected device may be usable from userspace, to some extent, without > > any handling of uevents. > > > > > So as a test, I disabled systemd-udevd and then plugged in a flash > > > drive. I disabled support for usb mass storage in the kernel. So 'lsblk' > > > didn't show the device, just as expected. However, 'lsusb' still > > > dynamically updates the list as I repeatedly plugin and remove a flash > > > drive. > > > > An example of what I described above. > > > > > Then I studied libusb/lsusb code for a bit. It seems that lsusb makes use > > > of 'libusb_get_device_list()' to find the connected usb devices. But I > > > couldn't find any detail regarding implementation of > > > libusb_get_device_list. > > > > That's an internal aspect of libusb, so it might not be described in > > the documentation. You can always check the source code if you really > > want to know how it works. I think it may look at the files under > > /sys/bus/usb/devices/ -- that's certainly one possibility which would > > always work even without udev. > > > > > I am sure I'm missing something. Could someone please help me understand > > > what is happening behind the scenes? > > > > udev does things like loading drivers into the kernel (modprobe'ing > > them) and creating nodes under /dev. But a driver that is already > > present in the kernel doesn't need to be modprobe'd, and the files > > under /sys are created automatically by the kernel rather than by udev. > > Also, udev has not created /dev nodes for probably a decade now. That's > what devtmpfs does. udev can create symlinks to existing dev nodes, and > some "special-case" device nodes where the kernel name is not what > userspace wants to use. > > But really, why would anyone _want_ to remove udev, it's such a nice > program and solves so many problems :) > I wanted to learn how Linux handles plug and play devices. Thanks for pointing out devtmpfs. I guess making and breaking things is a good way of learning. > thanks, > > greg k-h BTW great work with Linux Kernel in a Nutshell. It's still a fantastic resource for kernel modders/newbies despite being over a decade old. Thanks for the helpful comments guys. Regards, Faisal
RE: [PATCH v4 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
Hi Andy,
> > 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.
>
> Thanks for an update, my comments below.
Thanks for reviewing.
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> + blank line.
Ok.
I could not find kernel documentation which requires this blank line.
Can you please point me to it?
> > +#include
>
> > +struct gpu_i2c_dev {
> > + struct device *dev;
> > + void __iomem *regs;
> > + struct i2c_adapter adapter;
> > + struct i2c_client *client;
> > + struct mutex mutex; /* to sync read/write */
> > + bool do_start;
> > +};
>
> > +static int i2c_check_status(struct gpu_i2c_dev *i2cd) {
> > + unsigned long target = jiffies + msecs_to_jiffies(1000);
> > + u32 val;
> > +
> > + do {
> > + val = readl(i2cd->regs + I2C_MST_CNTL);
>
> > + if ((val & I2C_MST_CNTL_CYCLE_TRIGGER) !=
> > + I2C_MST_CNTL_CYCLE_TRIGGER)
>
> Part after != redundant since it's one bit.
> But I'm fine with current as well.
Ok, will fix.
>
> > + break;
> > + if ((val & I2C_MST_CNTL_STATUS) !=
> > + I2C_MST_CNTL_STATUS_BUS_BUSY)
> > + break;
> > + usleep_range(1000, 2000);
> > + } while (time_is_after_jiffies(target));
>
> > +
>
> Redundant.
Ok, I will remove it but I didn't understand why its redundant?
I thought adding extra line would be more readable.
> > + if (time_is_before_jiffies(target))
> > + return -EIO;
> > +
> > + val = readl(i2cd->regs + I2C_MST_CNTL);
> > + switch (val & I2C_MST_CNTL_STATUS) {
> > + case I2C_MST_CNTL_STATUS_OKAY:
> > + return 0;
> > + case I2C_MST_CNTL_STATUS_NO_ACK:
> > + return -EIO;
> > + case I2C_MST_CNTL_STATUS_TIMEOUT:
> > + return -ETIME;
> > + case I2C_MST_CNTL_STATUS_BUS_BUSY:
> > + return -EBUSY;
> > + default:
>
> > + break;
>
> return 0; ?
Ok, will fix.
>
> > + }
>
> > + return 0;
>
> See above.
Ok
>
> > +}
>
> > +static int i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
> > + u32 val;
> > +
> > + writel(data, i2cd->regs + I2C_MST_DATA);
> > +
> > + val = I2C_MST_CNTL_CMD_WRITE | (1 <<
> I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> > + I2C_MST_CNTL_GEN_NACK;
>
> > + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP
> > + | I2C_MST_CNTL_GEN_RAB);
>
> "|" should be on previous line to follow common style in this module.
The "|" here is to clear the bit together. [val &= ~(X | Y | Z)]
Style in this module is still followed. Please see first line which does "|" to
val.
> > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > + return i2c_check_status(i2cd); }
> > +
> > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> > + struct i2c_msg *msgs, int num) {
> > + struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> > + struct device *dev = i2cd->dev;
> > + int status;
> > + int i, j;
>
> > +stop:
> > + status = i2c_stop(i2cd);
> > + if (status < 0)
> > + dev_err(dev, "i2c_stop error %x", status);
> > +unlock:
> > + mutex_unlock(>mutex);
>
> > + return i;
>
> Shouldn't it return status in case of error?
Thanks, will fix.
> > +}
>
> > +/*
> > + * This driver is for the Nvidia GPU cards with USB Type-C interface.
> > + * We want to identify the cards using vendor ID and class code.
> > + * Currently there is no class code defined for UCSI device over PCI
> > + * so using UNKNOWN.
> > + */
>
> So, I didn't see how it *guarantees* no collision with other devices of the
> same class...
I checked and there is no other NVIDIA cards with UNKNOWN class code.
Moreover, even if the driver gets loaded for a wrong card then eventually
i2c_read() (initiated from UCSI driver) will timeout or UCSI commands will
timeout. I think this is safe enough for now. We will update the class code
when UCSI gets a PCI class code.
We don't want to add dependency of adding product id for any new card
which requires this driver.
> > +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80
> > +static const struct pci_device_id gpu_i2c_ids[] = {
>
> > + { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> > + PCI_CLASS_SERIAL_UNKNOWN << 8, 0xff00},
>
> ...for now, I would suggest to be more stricted, i.e.
>
> { PCI_VDEVICE(NVIDIA, 0x1ad9) },
>
> Whenever the class appears it can be added later on.
See above.
>
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(pci,
[PATCH] usb: core: added uevent for over-current
After commit 1cbd53c8cd85 ("usb: core: introduce per-port over-current
counters") usb ports expose a sysfs value 'over_current_count'
to user space. This value on its own is not very useful as it requires
manual polling.
As a solution, fire a udev event from the usb hub device that specifies
the values 'OVER_CURRENT_PORT' and 'OVER_CURRENT_COUNT' that indicate
the path of the usb port where the over-current event occurred and the
value of 'over_current_count' in sysfs. Additionally, call
sysfs_notify() so the sysfs value supports poll().
Signed-off-by: Jon Flatley
---
drivers/usb/core/hub.c | 36
1 file changed, 36 insertions(+)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 65feedd69323..c986b0fc2daa 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
@@ -5096,6 +5097,40 @@ static void hub_port_connect_change(struct usb_hub *hub,
int port1,
usb_lock_port(port_dev);
}
+/* Handle notifying userspace about hub over-current events */
+static void port_over_current_notify(struct usb_port *port_dev)
+{
+ static char *envp[] = { NULL, NULL, NULL };
+ struct device *hub_dev;
+ char *port_dev_path;
+
+ sysfs_notify(_dev->dev.kobj, NULL, "over_current_count");
+
+ hub_dev = port_dev->dev.parent;
+
+ if (!hub_dev)
+ return;
+
+ port_dev_path = kobject_get_path(_dev->dev.kobj, GFP_KERNEL);
+ if (!port_dev_path)
+ return;
+
+ envp[0] = kasprintf(GFP_KERNEL, "OVER_CURRENT_PORT=%s", port_dev_path);
+ if (!envp[0])
+ return;
+
+ envp[1] = kasprintf(GFP_KERNEL, "OVER_CURRENT_COUNT=%u",
+ port_dev->over_current_count);
+ if (!envp[1])
+ goto exit;
+
+ kobject_uevent_env(_dev->kobj, KOBJ_CHANGE, envp);
+
+ kfree(envp[1]);
+exit:
+ kfree(envp[0]);
+}
+
static void port_event(struct usb_hub *hub, int port1)
__must_hold(_dev->status_lock)
{
@@ -5138,6 +5173,7 @@ static void port_event(struct usb_hub *hub, int port1)
if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
port_dev->over_current_count++;
+ port_over_current_notify(port_dev);
dev_dbg(_dev->dev, "over-current change #%u\n",
port_dev->over_current_count);
--
2.19.0.rc1.350.ge57e33dbd1-goog
Re: A question about 'lsusb'
On Thu, Aug 30, 2018 at 05:10:53PM -0400, Alan Stern wrote: > On Thu, 30 Aug 2018, Faisal Mehmood wrote: > > > Based on my (limited) understanding if I were to disable udev, the > > userspace should not be able to enumerate/interact with any newly > > connected device since udev handles uevents generated by kernel. > > (right?) > > That's not quite right. Yes, udev does handle uevents. But a newly > connected device may be usable from userspace, to some extent, without > any handling of uevents. > > > So as a test, I disabled systemd-udevd and then plugged in a flash > > drive. I disabled support for usb mass storage in the kernel. So 'lsblk' > > didn't show the device, just as expected. However, 'lsusb' still > > dynamically updates the list as I repeatedly plugin and remove a flash > > drive. > > An example of what I described above. > > > Then I studied libusb/lsusb code for a bit. It seems that lsusb makes use > > of 'libusb_get_device_list()' to find the connected usb devices. But I > > couldn't find any detail regarding implementation of > > libusb_get_device_list. > > That's an internal aspect of libusb, so it might not be described in > the documentation. You can always check the source code if you really > want to know how it works. I think it may look at the files under > /sys/bus/usb/devices/ -- that's certainly one possibility which would > always work even without udev. > > > I am sure I'm missing something. Could someone please help me understand > > what is happening behind the scenes? > > udev does things like loading drivers into the kernel (modprobe'ing > them) and creating nodes under /dev. But a driver that is already > present in the kernel doesn't need to be modprobe'd, and the files > under /sys are created automatically by the kernel rather than by udev. Also, udev has not created /dev nodes for probably a decade now. That's what devtmpfs does. udev can create symlinks to existing dev nodes, and some "special-case" device nodes where the kernel name is not what userspace wants to use. But really, why would anyone _want_ to remove udev, it's such a nice program and solves so many problems :) thanks, greg k-h
Re: A few questions about gadgetfs
On Fri, Aug 31, 2018 at 4:34 PM, Alan Stern wrote: > On Fri, 31 Aug 2018, Andrey Konovalov wrote: [...] >> Yes, I understand this. The idea is to stall/wait until the userspace >> provides a response. Like gadgetfs, but for every USB request. > > If all you want to do is wait, STALL is not the appropriate strategy. > Instead the device should send NAK packets to the host. > > STALL means "I can't process that request, because it is erroneous or I > don't understand it". NAK means "I'm not yet ready to reply; try > again later". [...] >> Is there a way to tell the UDC to not send the STALL packet, but to >> just wait? Assuming the userspace will read the request and provide a >> response within the timeout (50ms? [1]). Would this approach be >> supported by more hosts than sending STALLs? >> >> [1] https://www.beyondlogic.org/usbnutshell/usb6.shtml > > Sure. Just don't submit any response. With no data in its FIFO, the > UDC will respond with NAK packets. Perfect, that's what I need, thanks a lot!
Re: [PATCH] usb: Avoid use-after-free by flushing endpoints early in usb_set_interface()
On Fri, 31 Aug 2018, Mathias Nyman wrote: > The steps taken by usb core to set a new interface is very different from > what is done on the xHC host side. > > xHC hardware will do everything in one go. One command is used to set up > new endpoints, free old endpoints, check bandwidth, and run the new > endpoints. > > All this is done by xHC when usb core asks the hcd to check for > available bandwidth. At this point usb core has not yet flushed the old > endpoints, which will cause use-after-free issues in xhci driver as > queued URBs are cancelled on a re-allocated endpoint. > > To resolve this add a call to usb_disable_interface() which will flush > the endpoints before calling usb_hcd_alloc_bandwidth() > > Additional checks in xhci driver will also be implemented to gracefully > handle stale URB cancel on freed and re-allocated endpoints > > Cc: > Reported-by: Sudip Mukherjee > Signed-off-by: Mathias Nyman > --- > drivers/usb/core/message.c | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c > index 228672f..304bef2 100644 > --- a/drivers/usb/core/message.c > +++ b/drivers/usb/core/message.c > @@ -1377,6 +1377,13 @@ int usb_set_interface(struct usb_device *dev, int > interface, int alternate) > return -EINVAL; > } > > + /* > + * usb3 hosts configure the interface in usb_hcd_alloc_bandwidth, > + * including freeing dropped endpoint ring buffers. > + * Make sure the interface endpoints are flushed before that > + */ > + usb_disable_interface(dev, iface, false); > + > /* Make sure we have enough bandwidth for this alternate interface. >* Remove the current alt setting and add the new alt setting. >*/ Please also update the kerneldoc for this function. It should now specify that if the request fails, the original interface altsetting may be disabled. Drivers cannot rely on any particular alternate setting being in effect after a failure. Alan Stern
Re: A few questions about gadgetfs
On Fri, 31 Aug 2018, Andrey Konovalov wrote: > On Thu, Aug 30, 2018 at 10:50 PM, Alan Stern > wrote: > > On Thu, 30 Aug 2018, Andrey Konovalov wrote: > > > >> Hi Alan, > >> > >> I have a few questions about gadgetfs. > >> > >> According to documentation usb_gadget_driver->setup "queues a response > >> to ep0, or returns negative to stall". > >> > >> Do I understand correctly, that "stall" in this case means "retry the > >> same request later" and it's unrelated to the STALL USB packet? > > > > No; it _is_ related to USB STALL packets. In fact, it means "send a > > STALL packet back to the host". > > OK, understood. I read the part of the USB spec about this (functional > and protocol stalls). > > > > >> Is it OK to stall the initial control requests (the one that requests > >> the device descriptor for example)? > > > > Well, the USB spec allows a device to do this. However, a device that > > always returns STALL for a Get-Device-Descriptor request will not be > > usable for anything, since the host will not be able to enumerate it. > > Yes, I understand this. The idea is to stall/wait until the userspace > provides a response. Like gadgetfs, but for every USB request. If all you want to do is wait, STALL is not the appropriate strategy. Instead the device should send NAK packets to the host. STALL means "I can't process that request, because it is erroneous or I don't understand it". NAK means "I'm not yet ready to reply; try again later". > > The Linux USB stack will try such requests up to three times. And > > there are retry loops at higher layers in the code too, so it can take > > quite a while before the stack finally gives up. > > > >> What exactly happens when we stall between the gadget and the host? > >> Does the UDC driver/hardware see that we don't have a response yet and > >> waits? Or does it somehow communicate that stall to the host? > > > > It sends a STALL packet to the host. > > Is there a way to tell the UDC to not send the STALL packet, but to > just wait? Assuming the userspace will read the request and provide a > response within the timeout (50ms? [1]). Would this approach be > supported by more hosts than sending STALLs? > > [1] https://www.beyondlogic.org/usbnutshell/usb6.shtml Sure. Just don't submit any response. With no data in its FIFO, the UDC will respond with NAK packets. Some gadget drivers use the special USB_GADGET_DELAYED_STATUS return code for this purpose in their ->setup() routine. > >> Does it make any difference whether we stall or reply right away from > >> the host point of view? > > > > It depends on how the host-side code is written. Some code will retry > > STALLed requests a few times, and some code will fail right away. > > So the USB spec doesn't specify how the host should react to these > protocol STALL packets, but some USB hosts (e.g. Linux) just resend > the request? I assume that other popular hosts (Windows, MacOS, etc.) > do that as well, otherwise gadgetfs wouldn't be usable to emulate USB > devices for them. > > Although while searching info about this I found an article [2], which > states: "The EFM8 HID device sends a STALL handshake in response to > the get DEVICE_QUALIFIER descriptor request because the EFM8 HID > device does not support the certain request. After that, the USB host > will issue the next SETUP transaction". So the host skips the request > and goes to the next one instead of rerequesting. > > [2] > https://www.silabs.com/community/mcu/8-bit/knowledge-base.entry.html/2017/06/18/the_role_of_stallha-pQTe The Linux stack will send the Get-Device-Qualifier request three times before giving up and moving on. See the usb_get_descriptor() routine in drivers/usb/core/message.c. > Do you know of any particular hosts that fail right away when they > receive a protocol STALL? Like I said before, it depends on the particular code. For example, Linux's usb-storage driver will give up after only one attempt at sending a Bulk-only Get-Max-LUN request. Alan Stern
[PATCH 0/2] xhci fixes for usb-linus
Hi Greg A couple of xhci fixes, one finding all quirks for platform xhci devices, and another fixing use-after-free case if stale URBs are cancelled on already re-allocated endpoints. -Mathias Anurag Kumar Vulisha (1): usb: host: xhci-plat: Iterate over parent nodes for finding quirks Mathias Nyman (1): xhci: Fix use after free for URB cancellation on a reallocated endpoint drivers/usb/host/xhci-plat.c | 27 --- drivers/usb/host/xhci.c | 30 ++ 2 files changed, 46 insertions(+), 11 deletions(-) -- 2.7.4
[PATCH 1/2] usb: host: xhci-plat: Iterate over parent nodes for finding quirks
From: Anurag Kumar Vulisha
In xhci_plat_probe() both sysdev and pdev->dev are being used
for finding quirks. There are some drivers(like dwc3 host.c)
which adds quirks(like usb3-lpm-capable) into pdev and the logic
present in xhci_plat_probe() checks for quirks in either sysdev
or pdev for finding the quirks. Because of this logic, some of
the quirks are getting missed(usb3-lpm-capable quirk added by dwc3
host.c driver is getting missed).This patch fixes this by iterating
over all the available parents for finding the quirks. In this way
all the quirks which are present in child or parent are correctly
updated.
Signed-off-by: Anurag Kumar Vulisha
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-plat.c | 27 ---
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 8dc77e3..94e9392 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -153,7 +153,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
{
const struct xhci_plat_priv *priv_match;
const struct hc_driver *driver;
- struct device *sysdev;
+ struct device *sysdev, *tmpdev;
struct xhci_hcd *xhci;
struct resource *res;
struct usb_hcd *hcd;
@@ -273,19 +273,24 @@ static int xhci_plat_probe(struct platform_device *pdev)
goto disable_clk;
}
- if (device_property_read_bool(sysdev, "usb2-lpm-disable"))
- xhci->quirks |= XHCI_HW_LPM_DISABLE;
+ /* imod_interval is the interrupt moderation value in nanoseconds. */
+ xhci->imod_interval = 4;
- if (device_property_read_bool(sysdev, "usb3-lpm-capable"))
- xhci->quirks |= XHCI_LPM_SUPPORT;
+ /* Iterate over all parent nodes for finding quirks */
+ for (tmpdev = >dev; tmpdev; tmpdev = tmpdev->parent) {
- if (device_property_read_bool(>dev, "quirk-broken-port-ped"))
- xhci->quirks |= XHCI_BROKEN_PORT_PED;
+ if (device_property_read_bool(tmpdev, "usb2-lpm-disable"))
+ xhci->quirks |= XHCI_HW_LPM_DISABLE;
- /* imod_interval is the interrupt moderation value in nanoseconds. */
- xhci->imod_interval = 4;
- device_property_read_u32(sysdev, "imod-interval-ns",
->imod_interval);
+ if (device_property_read_bool(tmpdev, "usb3-lpm-capable"))
+ xhci->quirks |= XHCI_LPM_SUPPORT;
+
+ if (device_property_read_bool(tmpdev, "quirk-broken-port-ped"))
+ xhci->quirks |= XHCI_BROKEN_PORT_PED;
+
+ device_property_read_u32(tmpdev, "imod-interval-ns",
+>imod_interval);
+ }
hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
if (IS_ERR(hcd->usb_phy)) {
--
2.7.4
[PATCH 2/2] xhci: Fix use after free for URB cancellation on a reallocated endpoint
Make sure the cancelled URB is on the current endpoint ring.
If the endpoint ring has been reallocated since the URB was enqueued
then the URB may contain TD and TRB pointers to a already freed ring.
In this the case return the URB without touching any of the freed ring
structure data.
Don't try to stop the ring. It would be useless.
This can occur if endpoint is not flushed before it is dropped and
re-added, which is the case in usb_set_interface() as xhci does
things in an odd order.
Cc:
Tested-by: Sudip Mukherjee
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 61f48b1..0420eef 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -37,6 +37,21 @@ static unsigned long long quirks;
module_param(quirks, ullong, S_IRUGO);
MODULE_PARM_DESC(quirks, "Bit flags for quirks to be enabled as default");
+static bool td_on_ring(struct xhci_td *td, struct xhci_ring *ring)
+{
+ struct xhci_segment *seg = ring->first_seg;
+
+ if (!td || !td->start_seg)
+ return false;
+ do {
+ if (seg == td->start_seg)
+ return true;
+ seg = seg->next;
+ } while (seg && seg != ring->first_seg);
+
+ return false;
+}
+
/* TODO: copied from ehci-hcd.c - can this be refactored? */
/*
* xhci_handshake - spin reading hc until handshake completes or fails
@@ -1571,6 +1586,21 @@ static int xhci_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
goto done;
}
+ /*
+* check ring is not re-allocated since URB was enqueued. If it is, then
+* make sure none of the ring related pointers in this URB private data
+* are touched, such as td_list, otherwise we overwrite freed data
+*/
+ if (!td_on_ring(_priv->td[0], ep_ring)) {
+ xhci_err(xhci, "Canceled URB td not found on endpoint ring");
+ for (i = urb_priv->num_tds_done; i < urb_priv->num_tds; i++) {
+ td = _priv->td[i];
+ if (!list_empty(>cancelled_td_list))
+ list_del_init(>cancelled_td_list);
+ }
+ goto err_giveback;
+ }
+
if (xhci->xhc_state & XHCI_STATE_HALTED) {
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"HC halted, freeing TD manually.");
--
2.7.4
[PATCH 8/8] plarform: x86: intel_cht_int33fe: Remove the old connections for the muxes
USB Type-C class driver now expects the muxes to be always
assigned to the ports and not controllers, so the
connections for the mux and fusb302 can be removed.
Signed-off-by: Heikki Krogerus
---
drivers/platform/x86/intel_cht_int33fe.c | 19 ++-
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index 4a41c546ce2f..f4449d546459 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -35,7 +35,7 @@ struct cht_int33fe_data {
struct i2c_client *fusb302;
struct i2c_client *pi3usb30532;
/* Contain a list-head must be per device */
- struct device_connection connections[6];
+ struct device_connection connections[4];
};
/*
@@ -176,27 +176,20 @@ static int cht_int33fe_probe(struct i2c_client *client)
return -EPROBE_DEFER; /* Wait for i2c-adapter to load */
}
- data->connections[0].endpoint[0] = "i2c-fusb302";
+ data->connections[0].endpoint[0] = "port0";
data->connections[0].endpoint[1] = "i2c-pi3usb30532";
data->connections[0].id = "typec-switch";
- data->connections[1].endpoint[0] = "i2c-fusb302";
+ data->connections[1].endpoint[0] = "port0";
data->connections[1].endpoint[1] = "i2c-pi3usb30532";
data->connections[1].id = "idff01m01";
- data->connections[2].endpoint[0] = "port0";
- data->connections[2].endpoint[1] = "i2c-pi3usb30532";
- data->connections[2].id = "typec-switch";
- data->connections[3].endpoint[0] = "port0";
- data->connections[3].endpoint[1] = "i2c-pi3usb30532";
- data->connections[3].id = "idff01m01";
-
/* Only adding connection for role switch if UDC exists */
udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL);
if (udc) {
pci_dev_put(udc);
- data->connections[4].endpoint[0] = "i2c-fusb302";
- data->connections[4].endpoint[1] =
"intel_xhci_usb_sw-role-switch";
- data->connections[4].id = "usb-role-switch";
+ data->connections[2].endpoint[0] = "i2c-fusb302";
+ data->connections[2].endpoint[1] =
"intel_xhci_usb_sw-role-switch";
+ data->connections[2].id = "usb-role-switch";
}
device_connections_add(data->connections);
--
2.18.0
[PATCH 7/8] usb: typec: class: Don't use port parent for getting mux handles
It is not possible to use the parent of the port device when
requesting mux handles as the parent may be a multiport USB
Type-C or PD controller. The muxes must be assigned to the
ports, not the controllers.
This will also move the requesting of the muxes after the
port device is initialized.
Signed-off-by: Heikki Krogerus
---
drivers/usb/typec/class.c | 38 +++---
1 file changed, 15 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index c202975f8097..5b969339d1b3 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1501,7 +1501,7 @@ typec_port_register_altmode(struct typec_port *port,
sprintf(id, "id%04xm%02x", desc->svid, desc->mode);
- mux = typec_mux_get(port->dev.parent, id);
+ mux = typec_mux_get(>dev, id);
if (IS_ERR(mux))
return ERR_CAST(mux);
@@ -1541,18 +1541,6 @@ struct typec_port *typec_register_port(struct device
*parent,
return ERR_PTR(id);
}
- port->sw = typec_switch_get(cap->fwnode ? >dev : parent);
- if (IS_ERR(port->sw)) {
- ret = PTR_ERR(port->sw);
- goto err_switch;
- }
-
- port->mux = typec_mux_get(parent, "typec-mux");
- if (IS_ERR(port->mux)) {
- ret = PTR_ERR(port->mux);
- goto err_mux;
- }
-
switch (cap->type) {
case TYPEC_PORT_SRC:
port->pwr_role = TYPEC_SOURCE;
@@ -1593,13 +1581,26 @@ struct typec_port *typec_register_port(struct device
*parent,
port->port_type = cap->type;
port->prefer_role = cap->prefer_role;
+ device_initialize(>dev);
port->dev.class = typec_class;
port->dev.parent = parent;
port->dev.fwnode = cap->fwnode;
port->dev.type = _port_dev_type;
dev_set_name(>dev, "port%d", id);
- ret = device_register(>dev);
+ port->sw = typec_switch_get(>dev);
+ if (IS_ERR(port->sw)) {
+ put_device(>dev);
+ return ERR_CAST(port->sw);
+ }
+
+ port->mux = typec_mux_get(>dev, "typec-mux");
+ if (IS_ERR(port->mux)) {
+ put_device(>dev);
+ return ERR_CAST(port->mux);
+ }
+
+ ret = device_add(>dev);
if (ret) {
dev_err(parent, "failed to register port (%d)\n", ret);
put_device(>dev);
@@ -1607,15 +1608,6 @@ struct typec_port *typec_register_port(struct device
*parent,
}
return port;
-
-err_mux:
- typec_switch_put(port->sw);
-
-err_switch:
- ida_simple_remove(_index_ida, port->id);
- kfree(port);
-
- return ERR_PTR(ret);
}
EXPORT_SYMBOL_GPL(typec_register_port);
--
2.18.0
[PATCH 4/8] usb: xhci: pci: Only create Intel mux device when it's needed
Only create thre Intel role mux device if the platform has
USB peripheral controller PCI device.
While here, enable the role mux on Apollo Lake platforms.
Signed-off-by: Heikki Krogerus
Cc: Mathias Nyman
---
drivers/usb/host/xhci-pci.c | 20 +---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 6372edf339d9..ea651c2adcfd 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -75,6 +75,17 @@ static int xhci_pci_reinit(struct xhci_hcd *xhci, struct
pci_dev *pdev)
return 0;
}
+static int xhci_pci_board_has_udc(void)
+{
+ struct pci_dev *udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL);
+
+ if (udc) {
+ pci_dev_put(udc);
+ return true;
+ }
+ return false;
+}
+
static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
{
struct pci_dev *pdev = to_pci_dev(dev);
@@ -179,15 +190,18 @@ static void xhci_pci_quirks(struct device *dev, struct
xhci_hcd *xhci)
xhci->quirks |= XHCI_PME_STUCK_QUIRK;
}
if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
-pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
+ pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI)
xhci->quirks |= XHCI_SSIC_PORT_UNUSED;
- xhci->quirks |= XHCI_INTEL_USB_ROLE_SW;
- }
if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
(pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI ||
pdev->device == PCI_DEVICE_ID_INTEL_DNV_XHCI))
xhci->quirks |= XHCI_MISSING_CAS;
+ if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
+ (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
+pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI) &&
+ xhci_pci_board_has_udc())
+ xhci->quirks |= XHCI_INTEL_USB_ROLE_SW;
if (pdev->vendor == PCI_VENDOR_ID_ETRON &&
pdev->device == PCI_DEVICE_ID_EJ168) {
--
2.18.0
[PATCH 6/8] plarform: x86: intel_cht_int33fe: Add connections for the USB Type-C port
Assigning the mux to the USB Type-C port on top of fusb302.
That will prepare this driver for the change in the USB
Type-C class code, where the class driver will assume the
muxes to be always assigned to the ports and not the
controllers.
Once the USB Type-C class driver has been updated, the
connections between the mux and fusb302 can be dropped.
Signed-off-by: Heikki Krogerus
---
drivers/platform/x86/intel_cht_int33fe.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index a5d27f06b2fb..4a41c546ce2f 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -35,7 +35,7 @@ struct cht_int33fe_data {
struct i2c_client *fusb302;
struct i2c_client *pi3usb30532;
/* Contain a list-head must be per device */
- struct device_connection connections[4];
+ struct device_connection connections[6];
};
/*
@@ -183,13 +183,20 @@ static int cht_int33fe_probe(struct i2c_client *client)
data->connections[1].endpoint[1] = "i2c-pi3usb30532";
data->connections[1].id = "idff01m01";
+ data->connections[2].endpoint[0] = "port0";
+ data->connections[2].endpoint[1] = "i2c-pi3usb30532";
+ data->connections[2].id = "typec-switch";
+ data->connections[3].endpoint[0] = "port0";
+ data->connections[3].endpoint[1] = "i2c-pi3usb30532";
+ data->connections[3].id = "idff01m01";
+
/* Only adding connection for role switch if UDC exists */
udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL);
if (udc) {
pci_dev_put(udc);
- data->connections[2].endpoint[0] = "i2c-fusb302";
- data->connections[2].endpoint[1] =
"intel_xhci_usb_sw-role-switch";
- data->connections[2].id = "usb-role-switch";
+ data->connections[4].endpoint[0] = "i2c-fusb302";
+ data->connections[4].endpoint[1] =
"intel_xhci_usb_sw-role-switch";
+ data->connections[4].id = "usb-role-switch";
}
device_connections_add(data->connections);
--
2.18.0
[PATCH 5/8] plarform: x86: intel_cht_int33fe: Fix the identifier for the mux connection
PI3USB30532 is used for muxing the port to DisplayPort on CHT platforms, so changing the connection ID so that the mux will get assigned to the alternate mode device and not the port device. Connection ID "typec-mux" is now reserved for Accessory Modes. Signed-off-by: Heikki Krogerus --- drivers/platform/x86/intel_cht_int33fe.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c index 4d11f5fb23cd..a5d27f06b2fb 100644 --- a/drivers/platform/x86/intel_cht_int33fe.c +++ b/drivers/platform/x86/intel_cht_int33fe.c @@ -181,7 +181,7 @@ static int cht_int33fe_probe(struct i2c_client *client) data->connections[0].id = "typec-switch"; data->connections[1].endpoint[0] = "i2c-fusb302"; data->connections[1].endpoint[1] = "i2c-pi3usb30532"; - data->connections[1].id = "typec-mux"; + data->connections[1].id = "idff01m01"; /* Only adding connection for role switch if UDC exists */ udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL); -- 2.18.0
[PATCH 1/8] drivers: base: Helpers for adding device connection descriptions
Introducing helpers for adding and removing multiple device
connection descriptions at once.
Signed-off-by: Heikki Krogerus
---
include/linux/device.h | 24
1 file changed, 24 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 8f882549edee..3f1066a9e1c3 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -773,6 +773,30 @@ struct device *device_connection_find(struct device *dev,
const char *con_id);
void device_connection_add(struct device_connection *con);
void device_connection_remove(struct device_connection *con);
+/**
+ * device_connections_add - Add multiple device connections at once
+ * @cons: Zero terminated array of device connection descriptors
+ */
+static inline void device_connections_add(struct device_connection *cons)
+{
+ struct device_connection *c;
+
+ for (c = cons; c->endpoint[0]; c++)
+ device_connection_add(c);
+}
+
+/**
+ * device_connections_remove - Remove multiple device connections at once
+ * @cons: Zero terminated array of device connection descriptors
+ */
+static inline void device_connections_remove(struct device_connection *cons)
+{
+ struct device_connection *c;
+
+ for (c = cons; c->endpoint[0]; c++)
+ device_connection_remove(c);
+}
+
/**
* enum device_link_state - Device link states.
* @DL_STATE_NONE: The presence of the drivers is not being tracked.
--
2.18.0
[PATCH 3/8] plarform: x86: intel_cht_int33fe: Use the USB role switch conditionally
Only adding connection between the USB role switch and
FUSB302 when the board has USB Device Controller (UDC).
Several CHT based products do not enable the UDC PCI device
by default.
Signed-off-by: Heikki Krogerus
---
drivers/platform/x86/intel_cht_int33fe.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index b0cef48f77af..4d11f5fb23cd 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
#include
#include
@@ -96,6 +97,7 @@ static int cht_int33fe_probe(struct i2c_client *client)
struct i2c_client *max17047;
struct regulator *regulator;
unsigned long long ptyp;
+ struct pci_dev *udc;
acpi_status status;
int fusb302_irq;
int ret;
@@ -180,9 +182,15 @@ static int cht_int33fe_probe(struct i2c_client *client)
data->connections[1].endpoint[0] = "i2c-fusb302";
data->connections[1].endpoint[1] = "i2c-pi3usb30532";
data->connections[1].id = "typec-mux";
- data->connections[2].endpoint[0] = "i2c-fusb302";
- data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
- data->connections[2].id = "usb-role-switch";
+
+ /* Only adding connection for role switch if UDC exists */
+ udc = pci_get_class(PCI_CLASS_SERIAL_USB_DEVICE, NULL);
+ if (udc) {
+ pci_dev_put(udc);
+ data->connections[2].endpoint[0] = "i2c-fusb302";
+ data->connections[2].endpoint[1] =
"intel_xhci_usb_sw-role-switch";
+ data->connections[2].id = "usb-role-switch";
+ }
device_connections_add(data->connections);
--
2.18.0
[PATCH 2/8] plarform: x86: intel_cht_int33fe: Register all connections at once
We can register all device connection descriptors with a
single call to device_connections_add().
Signed-off-by: Heikki Krogerus
---
drivers/platform/x86/intel_cht_int33fe.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/intel_cht_int33fe.c
b/drivers/platform/x86/intel_cht_int33fe.c
index 39d4100c60a2..b0cef48f77af 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -34,7 +34,7 @@ struct cht_int33fe_data {
struct i2c_client *fusb302;
struct i2c_client *pi3usb30532;
/* Contain a list-head must be per device */
- struct device_connection connections[3];
+ struct device_connection connections[4];
};
/*
@@ -184,9 +184,7 @@ static int cht_int33fe_probe(struct i2c_client *client)
data->connections[2].endpoint[1] = "intel_xhci_usb_sw-role-switch";
data->connections[2].id = "usb-role-switch";
- device_connection_add(>connections[0]);
- device_connection_add(>connections[1]);
- device_connection_add(>connections[2]);
+ device_connections_add(data->connections);
memset(_info, 0, sizeof(board_info));
strlcpy(board_info.type, "typec_fusb302", I2C_NAME_SIZE);
@@ -217,9 +215,7 @@ static int cht_int33fe_probe(struct i2c_client *client)
if (data->max17047)
i2c_unregister_device(data->max17047);
- device_connection_remove(>connections[2]);
- device_connection_remove(>connections[1]);
- device_connection_remove(>connections[0]);
+ device_connections_remove(data->connections);
return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
}
@@ -233,9 +229,7 @@ static int cht_int33fe_remove(struct i2c_client *i2c)
if (data->max17047)
i2c_unregister_device(data->max17047);
- device_connection_remove(>connections[2]);
- device_connection_remove(>connections[1]);
- device_connection_remove(>connections[0]);
+ device_connections_remove(data->connections);
return 0;
}
--
2.18.0
[PATCH 0/8] usb: typec: A few more improvements for Intel CHT
Hi, The second last patch in this series will make it possible to use multiport USB Type-C and PD controllers with the muxes. The CHT connections are simply adapted to that. The rest of the series will mainly allow us to use the USB Type-C on CHT boards even without a USB device controller. Heikki Krogerus (8): drivers: base: Helpers for adding device connection descriptions plarform: x86: intel_cht_int33fe: Register all connections at once plarform: x86: intel_cht_int33fe: Use the USB role switch conditionally usb: xhci: pci: Only create Intel mux device when it's needed plarform: x86: intel_cht_int33fe: Fix the identifier for the mux connection plarform: x86: intel_cht_int33fe: Add connections for the USB Type-C port usb: typec: class: Don't use port parent for getting mux handles plarform: x86: intel_cht_int33fe: Remove the old connections for the muxes drivers/platform/x86/intel_cht_int33fe.c | 34 +++-- drivers/usb/host/xhci-pci.c | 20 +++-- drivers/usb/typec/class.c| 38 ++-- include/linux/device.h | 24 +++ 4 files changed, 74 insertions(+), 42 deletions(-) -- 2.18.0
[PATCH] usb: Avoid use-after-free by flushing endpoints early in usb_set_interface()
The steps taken by usb core to set a new interface is very different from what is done on the xHC host side. xHC hardware will do everything in one go. One command is used to set up new endpoints, free old endpoints, check bandwidth, and run the new endpoints. All this is done by xHC when usb core asks the hcd to check for available bandwidth. At this point usb core has not yet flushed the old endpoints, which will cause use-after-free issues in xhci driver as queued URBs are cancelled on a re-allocated endpoint. To resolve this add a call to usb_disable_interface() which will flush the endpoints before calling usb_hcd_alloc_bandwidth() Additional checks in xhci driver will also be implemented to gracefully handle stale URB cancel on freed and re-allocated endpoints Cc: Reported-by: Sudip Mukherjee Signed-off-by: Mathias Nyman --- drivers/usb/core/message.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index 228672f..304bef2 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1377,6 +1377,13 @@ int usb_set_interface(struct usb_device *dev, int interface, int alternate) return -EINVAL; } + /* +* usb3 hosts configure the interface in usb_hcd_alloc_bandwidth, +* including freeing dropped endpoint ring buffers. +* Make sure the interface endpoints are flushed before that +*/ + usb_disable_interface(dev, iface, false); + /* Make sure we have enough bandwidth for this alternate interface. * Remove the current alt setting and add the new alt setting. */ -- 2.7.4
Re: A few questions about gadgetfs
On Thu, Aug 30, 2018 at 10:50 PM, Alan Stern wrote: > On Thu, 30 Aug 2018, Andrey Konovalov wrote: > >> Hi Alan, >> >> I have a few questions about gadgetfs. >> >> According to documentation usb_gadget_driver->setup "queues a response >> to ep0, or returns negative to stall". >> >> Do I understand correctly, that "stall" in this case means "retry the >> same request later" and it's unrelated to the STALL USB packet? > > No; it _is_ related to USB STALL packets. In fact, it means "send a > STALL packet back to the host". OK, understood. I read the part of the USB spec about this (functional and protocol stalls). > >> Is it OK to stall the initial control requests (the one that requests >> the device descriptor for example)? > > Well, the USB spec allows a device to do this. However, a device that > always returns STALL for a Get-Device-Descriptor request will not be > usable for anything, since the host will not be able to enumerate it. Yes, I understand this. The idea is to stall/wait until the userspace provides a response. Like gadgetfs, but for every USB request. > > The Linux USB stack will try such requests up to three times. And > there are retry loops at higher layers in the code too, so it can take > quite a while before the stack finally gives up. > >> What exactly happens when we stall between the gadget and the host? >> Does the UDC driver/hardware see that we don't have a response yet and >> waits? Or does it somehow communicate that stall to the host? > > It sends a STALL packet to the host. Is there a way to tell the UDC to not send the STALL packet, but to just wait? Assuming the userspace will read the request and provide a response within the timeout (50ms? [1]). Would this approach be supported by more hosts than sending STALLs? [1] https://www.beyondlogic.org/usbnutshell/usb6.shtml > >> Does it make any difference whether we stall or reply right away from >> the host point of view? > > It depends on how the host-side code is written. Some code will retry > STALLed requests a few times, and some code will fail right away. So the USB spec doesn't specify how the host should react to these protocol STALL packets, but some USB hosts (e.g. Linux) just resend the request? I assume that other popular hosts (Windows, MacOS, etc.) do that as well, otherwise gadgetfs wouldn't be usable to emulate USB devices for them. Although while searching info about this I found an article [2], which states: "The EFM8 HID device sends a STALL handshake in response to the get DEVICE_QUALIFIER descriptor request because the EFM8 HID device does not support the certain request. After that, the USB host will issue the next SETUP transaction". So the host skips the request and goes to the next one instead of rerequesting. [2] https://www.silabs.com/community/mcu/8-bit/knowledge-base.entry.html/2017/06/18/the_role_of_stallha-pQTe Do you know of any particular hosts that fail right away when they receive a protocol STALL? > >> The questions arose when I started implementing a gadgetfs like >> interface, that allows to respond to every USB packet (including the >> initial control requests) from a userspace app. >> >> Thanks! > > You're welcome. > > Alan Stern >
Re: [PATCH v4 2/2] usb: typec: ucsi: add support for Cypress CCGx
On Fri, Aug 31, 2018 at 12:41 AM Ajay Gupta 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.
> Changes from v3 -> v4
> Fixed comments from Andy
Unfortunatelly not all comments was addressed, see below.
> + if (err == ARRAY_SIZE(msgs)) {
> + err = 0;
> + } else if (err >= 0) {
> + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> + return -EIO;
> + }
Shouldn't be simple
if (err < 0) {
...
return err;
}
?
> + if (err == ARRAY_SIZE(msgs)) {
> + err = 0;
> + } else if (err >= 0) {
> + dev_err(dev, "i2c_transfer failed, err %d\n", err);
> + return -EIO;
> + }
Ditto.
> + struct device *dev = uc->dev;
> + u8 data[64];
> + int err;
> + unsigned int count = 10;
Move this line upper a bit.
> + unsigned char buf[3] = {
> + CCGX_I2C_RAB_INTR_REG & 0xff, CCGX_I2C_RAB_INTR_REG >> 8,
> 0x0};
This should follow the style you applied earlier in the same file.
Also, & 0xff is redundant (better just to use >> 0).
> + struct ucsi_ccg *uc = container_of(ppm,
> + struct ucsi_ccg, ppm);
One line?
> +static int ucsi_ccg_cmd(struct ucsi_ppm *ppm, struct ucsi_control *ctrl)
> +{
> + struct ucsi_ccg *uc = container_of(ppm, struct ucsi_ccg, ppm);
> + int err = 0;
Redundant assignment.
> +
> + ppm->data->ctrl.raw_cmd = ctrl->raw_cmd;
> + err = ucsi_ccg_send_data(uc);
> +
> + return err;
> +}
> +static int ucsi_ccg_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
One line?
> +static const struct i2c_device_id ucsi_ccg_device_id[] = {
> + {"ccgx-ucsi", 0},
> + {},
Terminator better w/o comma.
> +};
> +MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
On Fri, Aug 31, 2018 at 12:41 AM Ajay Gupta wrote:
>
> 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.
Thanks for an update, my comments below.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
+ blank line.
> +#include
> +struct gpu_i2c_dev {
> + struct device *dev;
> + void __iomem *regs;
> + struct i2c_adapter adapter;
> + struct i2c_client *client;
> + struct mutex mutex; /* to sync read/write */
> + bool do_start;
> +};
> +static int i2c_check_status(struct gpu_i2c_dev *i2cd)
> +{
> + unsigned long target = jiffies + msecs_to_jiffies(1000);
> + u32 val;
> +
> + do {
> + val = readl(i2cd->regs + I2C_MST_CNTL);
> + if ((val & I2C_MST_CNTL_CYCLE_TRIGGER) !=
> + I2C_MST_CNTL_CYCLE_TRIGGER)
Part after != redundant since it's one bit.
But I'm fine with current as well.
> + break;
> + if ((val & I2C_MST_CNTL_STATUS) !=
> + I2C_MST_CNTL_STATUS_BUS_BUSY)
> + break;
> + usleep_range(1000, 2000);
> + } while (time_is_after_jiffies(target));
> +
Redundant.
> + if (time_is_before_jiffies(target))
> + return -EIO;
> +
> + val = readl(i2cd->regs + I2C_MST_CNTL);
> + switch (val & I2C_MST_CNTL_STATUS) {
> + case I2C_MST_CNTL_STATUS_OKAY:
> + return 0;
> + case I2C_MST_CNTL_STATUS_NO_ACK:
> + return -EIO;
> + case I2C_MST_CNTL_STATUS_TIMEOUT:
> + return -ETIME;
> + case I2C_MST_CNTL_STATUS_BUS_BUSY:
> + return -EBUSY;
> + default:
> + break;
return 0; ?
> + }
> + return 0;
See above.
> +}
> +static int i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
> +{
> + u32 val;
> +
> + writel(data, i2cd->regs + I2C_MST_DATA);
> +
> + val = I2C_MST_CNTL_CMD_WRITE | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> + I2C_MST_CNTL_GEN_NACK;
> + val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP
> + | I2C_MST_CNTL_GEN_RAB);
"|" should be on previous line to follow common style in this module.
> + writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> + return i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> + struct i2c_msg *msgs, int num)
> +{
> + struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> + struct device *dev = i2cd->dev;
> + int status;
> + int i, j;
> +stop:
> + status = i2c_stop(i2cd);
> + if (status < 0)
> + dev_err(dev, "i2c_stop error %x", status);
> +unlock:
> + mutex_unlock(>mutex);
> + return i;
Shouldn't it return status in case of error?
> +}
> +/*
> + * This driver is for the Nvidia GPU cards with USB Type-C interface.
> + * We want to identify the cards using vendor ID and class code.
> + * Currently there is no class code defined for UCSI device over PCI
> + * so using UNKNOWN.
> + */
So, I didn't see how it *guarantees* no collision with other devices
of the same class...
> +#define PCI_CLASS_SERIAL_UNKNOWN 0x0c80
> +static const struct pci_device_id gpu_i2c_ids[] = {
> + { PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> + PCI_CLASS_SERIAL_UNKNOWN << 8, 0xff00},
...for now, I would suggest to be more stricted, i.e.
{ PCI_VDEVICE(NVIDIA, 0x1ad9) },
Whenever the class appears it can be added later on.
> + { }
> +};
> +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> +
> +static int gpu_i2c_probe(struct pci_dev *dev, const struct pci_device_id *id)
Use pdev instead of dev to distinguish struct device from struct
pci_dev type in variable name.
> +static void gpu_i2c_remove(struct pci_dev *dev)
Ditto.
> + struct gpu_i2c_dev *i2cd = pci_get_drvdata(to_pci_dev(dev));
Isn't the same as dev_get_drvdata() ?
> + struct gpu_i2c_dev *i2cd = pci_get_drvdata(to_pci_dev(dev));
Ditto.
--
With Best Regards,
Andy Shevchenko
Re: musb_hdrc HNP?
Hello. I just confirmed what I wanted to see. I could do lsusb to list A-device (from b_host) and B-device (from a_host). Suspending from either side kicked role change between A-device and B-device (in both direction). I needed to wait 20ms after B-device seeing MUSB_INTR_CONNECT and before calling musb_host_poke_root_hub(). I suppose seeing CONNECT inturrupt B-device can expect that A-device is reset, but it may take some time and B-device may need to wait. On technial nodes, this is mentioned as something like this: "Reset Signaling. If the RESET bit in the POWER register (bit 3) is set while the controller is in Host mode, it will generate Reset signaling on the bus. If the HSENAB bit in the POWER register (bit 5) was set, it will also try to negotiate for high-speed operation. The software should keep the RESET bit set for at least 20 ms to ensure correct resetting of the target device." Note I still see errors like below after role change (b_host -> b_peripheral), perhaps some necessary cleanup is not there. But anyway they role-switched in both direction.. [ 1204.225843] usb 2-1: USB disconnect, device number 7 [ 1204.274238] hub 2-0:1.0: USB hub found [ 1204.282564] hub 2-0:1.0: 1 port detected [ 1204.496301] musb-hdrc musb-hdrc.1: Babble [ 1204.622799] musb_h_ep0_irq 1192: no URB for end 0 [ 1208.474661] usb usb2-port1: Cannot enable. Maybe the USB cable is bad? [ 1210.063965] zero gadget: high-speed config #3: source/sink [ 1212.566697] usb usb2-port1: Cannot enable. Maybe the USB cable is bad? [ 1212.573607] usb usb2-port1: attempt power cycle [ 1216.974544] usb usb2-port1: Cannot enable. Maybe the USB cable is bad? [ 1221.066539] usb usb2-port1: Cannot enable. Maybe the USB cable is bad? [ 1221.073328] usb usb2-port1: unable to enumerate USB device debian@beaglebone:~/wk-b$ === From: linux-usb-ow...@vger.kernel.org on behalf of Takashi Matsuzawa Sent: Friday, August 31, 2018 1:50 PM To: Bin Liu Cc: linux-usb@vger.kernel.org Subject: Re: musb_hdrc HNP? Hello. FYI. I made a progress on this, but no solution yet. >The smartphone does use HNP, it is not iPhone Carplay, correct? At this point, I am trying to see original HNP behavior between two pocketbeagles. (After seeing it works, I plan to replace B-device with a phone, and so customization on A-device stack.) >1. MUSB uses one register bit to report babble and reset event, so driver bug >could report bus reset as babble if it doesn't trace the controller role >correctly; As mentioned below, it may be related to how driver on B-device responds to RESET/BABBLE interrupts (or its sideeffets). 1) I could see CONFIG_USB_OTG was not set on "Debian 9.4 2018-06-17" image so I turned it on. This improved the situation a bit, like A-device side b_hnp_enable flag (which is set when B-device b_hnp_enable is set.) 2) Now I can see A-device and B-device turns to expected modes. A-device: a_host -> a_peripheral After suspending port, sees DISCONNECT and RESET events. B-device: b_peripheral -> b_host Sees SUSPEND, CONNECT events. 3) But I do not see B-device enumerate A-device at this point, and instead on B-device (now b_host) RESET(or BUBBLE) events are seen. Then after that, immediately, SUSPEND is seen on A-device, looks like now A-device is resuming as a_host and B-device back to b_peripheral. 4) I expect at 2) B-device should enumerate A-device and both stays in new mode (and I can, say do lsusb on B-device and see A-device listed), but it does not happen. Ignoring RESET(BUBBLE) events on B-device (b_host) at 3) did not improve the situgation. (Now I see SUSPEND on B-device instead of DISCONNECT.) It may be that driver behavior after 2) (to be initiated as a_peripheral and b_host and restarting) having some problem. From: linux-usb-ow...@vger.kernel.org on behalf of Bin Liu Sent: Monday, August 27, 2018 10:33 PM To: Takashi Matsuzawa Cc: linux-usb@vger.kernel.org Subject: Re: musb_hdrc HNP? Hi, On Mon, Aug 27, 2018 at 12:57:55AM +, Takashi Matsuzawa wrote: > Thank you for your suggestion. > Yes, I am aware that full-OTG support code is being wiped out of the > latest mainline kernels. Okay. Let me know if reverting that patch can magically make HNP works. > I am trying this for smartphone connectivity where OTG based > role-switch is being used, which may not be of interest of everyone. The smartphone does use HNP, it is not iPhone Carplay, correct? > I picked pocketbeagle since it has 2.0 OTG controller (without hub), > which matched what I wanted to prototype. Pocketbeagle should be good, it uses TI AM335x which is the device I have. > (If anyone is aware similar low-cost board with proven kernel version, > I would interested in hearing about it.) > > I think I try debugging a bit further through ftrace, etc. and bus > monitoring. > > One thing I am curious is the "Babble" errors. > If they are caused
