Re: [Bug 200917] 4.18 regression: I/O error on external icybox disk enclosures
Hello, On 09/09/2018 21:28, Alan Stern wrote: On Sun, 9 Sep 2018, Klaus Kusche wrote: Hello, On 01/09/2018 17:38, Alan Stern wrote: However, the USB layer does set certain quirk bits which can cause those other parts to avoid sending certain commands. Perhaps your controller needs the BROKEN_FUA flag (see the existing entries in drivers/usb/storage/unusual_devs.h with that flag). The following seems to fix the problem (I don't know if I did it right): --- unusual_uas.h.orig 2018-09-09 10:31:20.440751625 +0200 +++ unusual_uas.h 2018-09-09 10:32:30.491381466 +0200 @@ -95,7 +95,7 @@ "JMicron", "JMS566", USB_SC_DEVICE, USB_PR_DEVICE, NULL, - US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS), + US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS | US_FL_BROKEN_FUA), /* Reported-by: Hans de Goede */ UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x, Okay. You should submit this in the proper format for inclusion in the kernel. Follow the instructions in Documentation/process/submitting-patches.rst. Sorry for the long delay, I was not able to spare some time earlier (winter semester has started...). I just wanted to submit the patch as advised above, but noticed that it does not apply to the current usb git tree: Both the usb tree and linus's tree do not have US_FL_IGNORE_UAS set for device JMS566 (357d:7788). So I checked where my version came from and found a patch which gentoo applies to all its kernels, but which is not mainline: --- a/drivers/usb/storage/unusual_uas.h +++ b/drivers/usb/storage/unusual_uas.h @@ -141,12 +141,15 @@ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x, USB_SC_DEVICE, USB_PR_DEVICE, NULL, US_FL_NO_ATA_1X), -/* Reported-by: Takeo Nakayama */ +/* + * Initially Reported-by: Takeo Nakayama + * UAS Ignore Reported by Steven Ellis + */ UNUSUAL_DEV(0x357d, 0x7788, 0x, 0x, "JMicron", "JMS566", USB_SC_DEVICE, USB_PR_DEVICE, NULL, - US_FL_NO_REPORT_OPCODES), + US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS), /* Reported-by: Hans de Goede */ UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x, So is my patch still relevant or is it obsolete? Shall I add only US_FL_BROKEN_FUA even without US_FL_IGNORE_UAS, or shall I add both flags? (I noticed that US_FL_BROKEN_FUA without US_FL_IGNORE_UAS is set for two entries for "JMS567"). -- Prof. Dr. Klaus Kusche Private address: Rosenberg 41, 07546 Gera, Germany +49 365 20413058 klaus.kus...@computerix.info https://www.computerix.info Office address: DHGE Gera, Weg der Freundschaft 4, 07546 Gera, Germany +49 365 4341 306 klaus.kus...@dhge.de https://www.dhge.de
Re: [Bug 200917] 4.18 regression: I/O error on external icybox disk enclosures
On Wed, 3 Oct 2018, Klaus Kusche wrote: > Hello, > > On 09/09/2018 21:28, Alan Stern wrote: > > On Sun, 9 Sep 2018, Klaus Kusche wrote: > > > >> Hello, > >> > >> On 01/09/2018 17:38, Alan Stern wrote: > >>> However, the USB layer does set certain quirk bits which can cause > >>> those other parts to avoid sending certain commands. Perhaps your > >>> controller needs the BROKEN_FUA flag (see the existing entries in > >>> drivers/usb/storage/unusual_devs.h with that flag). > >> > >> The following seems to fix the problem (I don't know if I did it right): > >> > >> --- unusual_uas.h.orig 2018-09-09 10:31:20.440751625 +0200 > >> +++ unusual_uas.h 2018-09-09 10:32:30.491381466 +0200 > >> @@ -95,7 +95,7 @@ > >> "JMicron", > >> "JMS566", > >> USB_SC_DEVICE, USB_PR_DEVICE, NULL, > >> - US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS), > >> + US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS | > >> US_FL_BROKEN_FUA), > >> > >>/* Reported-by: Hans de Goede */ > >>UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x, > > > > Okay. You should submit this in the proper format for inclusion in the > > kernel. Follow the instructions in > > Documentation/process/submitting-patches.rst. > > Sorry for the long delay, I was not able to spare some time earlier > (winter semester has started...). > > I just wanted to submit the patch as advised above, > but noticed that it does not apply to the current usb git tree: > > Both the usb tree and linus's tree do not have US_FL_IGNORE_UAS > set for device JMS566 (357d:7788). > So I checked where my version came from and found a patch > which gentoo applies to all its kernels, but which is not mainline: > > --- a/drivers/usb/storage/unusual_uas.h > +++ b/drivers/usb/storage/unusual_uas.h > @@ -141,12 +141,15 @@ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x, > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_NO_ATA_1X), > > -/* Reported-by: Takeo Nakayama */ > +/* > + * Initially Reported-by: Takeo Nakayama > + * UAS Ignore Reported by Steven Ellis > + */ > UNUSUAL_DEV(0x357d, 0x7788, 0x, 0x, > "JMicron", > "JMS566", > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > - US_FL_NO_REPORT_OPCODES), > + US_FL_NO_REPORT_OPCODES | US_FL_IGNORE_UAS), > > /* Reported-by: Hans de Goede */ > UNUSUAL_DEV(0x4971, 0x1012, 0x, 0x, > > So is my patch still relevant or is it obsolete? > Shall I add only US_FL_BROKEN_FUA even without US_FL_IGNORE_UAS, > or shall I add both flags? > > (I noticed that US_FL_BROKEN_FUA without US_FL_IGNORE_UAS is set > for two entries for "JMS567"). Well, what happens if you add only US_FL_BROKEN_FUA without US_FL_IGNORE_UAS? Does it work? For that matter, does your kernel config include the uas driver? Alan Stern
Re: USB hotplug on HP 255 G6 laptop
On 02.10.2018 17:53, Jan Kara wrote: On Tue 02-10-18 17:01:54, Mathias Nyman wrote: On 02.10.2018 16:06, Jan Kara wrote: Hello, my wife has HP 255 G6 laptop. When it is attached to AC, everything works as expected however when it is running on battery, USB hotplug stops working - newly plugged devices do not appear to be visible to the kernel. Only when the AC is plugged back in, the kernel suddently wakes up and detects all newly attached devices. Maybe it is related to some power management? Any idea how to debug this? Dmesg from the system is attached. The kernels I've tested with (both behave the same way) are 4.18.11 kernel and also openSUSE Leap 15 kernel which is 4.12-based. Thanks in advance for any help. Are you running laptop mode tools or similar that would enable runtime suspend D3 state for xhci controller? It is openSUSE Leap 15 installation with xfce4 desktop so I assume there is some power-management going on. Not sure what I should look for but there's xfce4-power-manager running, also upowerd is running. what does lspci -vv say? Attached. check the content of the following files while running on battery: cat /sys/bus/pci/devices/:00:10.0/firmware_node/power_state D0 cat /sys/bus/pci/devices/:00:10.0/power/control auto - when on battery, on - when on AC. try: echo on > /sys/bus/pci/devices/:00:10.0/power/control before pluggin in a usb device, does it help? Yes, USB device gets recognized after this. To me this looks like xhci is runtime suspended, but controller is still in a PCI D0 state. Normally the xHC should be put to PCI D3 in runtime suspend, and woken up by a PCI PME# when a device is plugged in, but PME# is not enabled in D0 so in this case nothing wakes up the xHC. The xhci runtime suspend code already stopped the controller, so probably you're not getting an interrupt either. PCI code looks at firmware ACPI tables to choose the suspend D state, if something is off in the tables it's possible the wrong state is chosen. worth checking. Could you dump the DSDT ACPI table form /sys/firmware/acpi/tables/DSDT As a quick temporary workaround you could find and prevent the powersaving program from enabling runtime suspend. (make sure it won't write "auto" to /sys/bus/pci/devices/:00:10.0/power/control) -Mathias
Re: USB hotplug on HP 255 G6 laptop
On Wed 03-10-18 17:19:33, Mathias Nyman wrote: > On 02.10.2018 17:53, Jan Kara wrote: > > On Tue 02-10-18 17:01:54, Mathias Nyman wrote: > > > On 02.10.2018 16:06, Jan Kara wrote: > > > > Hello, > > > > > > > > my wife has HP 255 G6 laptop. When it is attached to AC, everything > > > > works > > > > as expected however when it is running on battery, USB hotplug stops > > > > working - newly plugged devices do not appear to be visible to the > > > > kernel. > > > > Only when the AC is plugged back in, the kernel suddently wakes up and > > > > detects all newly attached devices. Maybe it is related to some power > > > > management? Any idea how to debug this? Dmesg from the system is > > > > attached. > > > > The kernels I've tested with (both behave the same way) are 4.18.11 > > > > kernel > > > > and also openSUSE Leap 15 kernel which is 4.12-based. Thanks in advance > > > > for > > > > any help. > > > > > > > > > > Are you running laptop mode tools or similar that would enable runtime > > > suspend > > > D3 state for xhci controller? > > > > It is openSUSE Leap 15 installation with xfce4 desktop so I assume there is > > some power-management going on. Not sure what I should look for but there's > > xfce4-power-manager running, also upowerd is running. > > > > > what does lspci -vv say? > > > > Attached. > > > > > check the content of the following files while running on battery: > > > > > > cat /sys/bus/pci/devices/:00:10.0/firmware_node/power_state > > > > D0 > > > > > cat /sys/bus/pci/devices/:00:10.0/power/control > > > > auto - when on battery, on - when on AC. > > > > > try: echo on > /sys/bus/pci/devices/:00:10.0/power/control > > > before pluggin in a usb device, does it help? > > > > Yes, USB device gets recognized after this. > > > > To me this looks like xhci is runtime suspended, but controller is still > in a PCI D0 state. > > Normally the xHC should be put to PCI D3 in runtime suspend, and woken > up by a PCI PME# when a device is plugged in, but PME# is not enabled in > D0 so in this case nothing wakes up the xHC. > > The xhci runtime suspend code already stopped the controller, so probably > you're not getting an interrupt either. > > PCI code looks at firmware ACPI tables to choose the suspend D state, if > something is off in the tables it's possible the wrong state is chosen. > worth checking. > > Could you dump the DSDT ACPI table form /sys/firmware/acpi/tables/DSDT Sure. Attached. > As a quick temporary workaround you could find and prevent the > powersaving program from enabling runtime suspend. (make sure it won't > write "auto" to /sys/bus/pci/devices/:00:10.0/power/control) OK, thanks for the hint. Honza -- Jan Kara SUSE Labs, CR dsdt Description: Binary data
[PATCH v13 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. I think the patches should through usb tree because the main functionality is usb Type-C. The changes have been reviewed by Andy Shevchenko and Heikki Krogerus. Peter Rosin also helped review the patches and provided valuable comments. Thanks to all reviewers. 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 | 369 drivers/usb/typec/ucsi/Kconfig | 10 + drivers/usb/typec/ucsi/Makefile | 2 + drivers/usb/typec/ucsi/ucsi_ccg.c | 305 ++ 8 files changed, 721 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.7.4
[PATCH v13 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
Reviewed-by: Andy Shevchenko
Acked-by: Heikki Krogerus
---
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"
drivers/usb/typec/ucsi/Kconfig| 10 ++
drivers/usb/typec/ucsi/Makefile | 2 +
drivers/usb/typec/ucsi/ucsi_ccg.c | 305 ++
3 files changed, 317 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..0e0bac1
--- /dev/null
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -0,0 +1,305 @@
+// 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_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;
+ 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;
+ int status;
+
+ /* i2c adapter (ccgx-ucsi) can read 4 byte max */
+ while (rem_len > 0) {
+ msgs[1].buf = &data[len - rem_len];
+ rlen = min_t(u16, rem_len, 4);
+ msgs[1].len = rlen;
+ put_unaligned_le16(rab, buf);
+ status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
+ if (status < 0) {
+ dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+ return status;
+ }
+ rab += rlen;
+ rem_len -= rlen;
+ }
+
+ return 0;
+}
+
+static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
+{
+ struct i2c_client *client = uc->client;
+ unsigned char *buf;
+ struct i2c_msg msgs[] = {
+
[PATCH v13 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 Reviewed-by: Andy Shevchenko Reviewed-by: Heikki Krogerus --- 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 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 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 d637054..83df14c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6800,6 +6800,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..744f5e4 --- /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 + +/* I2C definitions */ +
Re: [PATCH -next] USB: cypress_m8: remove set but not used variables 'actual_size, iflag'
On 2018/10/1 15:19, Dan Carpenter wrote:
> On Sun, Sep 30, 2018 at 06:02:24PM +0200, Johan Hovold wrote:
>> On Sat, Sep 29, 2018 at 09:54:03AM +, YueHaibing wrote:
>>> Fixes gcc '-Wunused-but-set-variable' warning:
>>>
>>> drivers/usb/serial/cypress_m8.c: In function 'cypress_send':
>>> drivers/usb/serial/cypress_m8.c:689:33: warning:
>>> variable 'actual_size' set but not used [-Wunused-but-set-variable]
>>>
>>> drivers/usb/serial/cypress_m8.c: In function 'cypress_set_termios':
>>> drivers/usb/serial/cypress_m8.c:866:18: warning:
>>> variable 'iflag' set but not used [-Wunused-but-set-variable]
>>>
>>> Signed-off-by: YueHaibing
>>> ---
>>> drivers/usb/serial/cypress_m8.c | 11 ++-
>>> 1 file changed, 2 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/usb/serial/cypress_m8.c
>>> b/drivers/usb/serial/cypress_m8.c
>>> index 31c6091..98dff12 100644
>>> --- a/drivers/usb/serial/cypress_m8.c
>>> +++ b/drivers/usb/serial/cypress_m8.c
>>> @@ -686,7 +686,7 @@ static int cypress_write(struct tty_struct *tty, struct
>>> usb_serial_port *port,
>>>
>>> static void cypress_send(struct usb_serial_port *port)
>>> {
>>> - int count = 0, result, offset, actual_size;
>>> + int count = 0, result, offset;
>>> struct cypress_private *priv = usb_get_serial_port_data(port);
>>> struct device *dev = &port->dev;
>>> unsigned long flags;
>>> @@ -758,12 +758,6 @@ static void cypress_send(struct usb_serial_port *port)
>>> priv->write_urb_in_use = 1;
>>> spin_unlock_irqrestore(&priv->lock, flags);
>>>
>>> - if (priv->cmd_ctrl)
>>> - actual_size = 1;
>>> - else
>>> - actual_size = count +
>>> - (priv->pkt_fmt == packet_format_1 ? 2 : 1);
>>> -
>>
>> This looks like we have a bug in the driver, and sure enough we do.
>> Commit 9aa8dae7b1fa ("cypress_m8: use usb_fill_int_urb where
>> appropriate") incorrectly started setting the transfer length to the
>> size of the buffer.
>>
>
> This is like the third time recently where we've had found a real bug.
>
> YueHaibing, could you mention in the commit message where the variable
> became unused? It would help reviewing. It's not a Fixes tag but it
> would look like this:
Ok, will do it.
>
> "After commit 9aa8dae7b1fa ("cypress_m8: use usb_fill_int_urb where
> appropriate") then we don't use actual_size any more so that can
> be removed."
>
>
> regards,
> dan carpenter
>
>
> .
>
[PATCH v2 -next] USB: core: remove set but not used variable 'udev'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/usb/core/driver.c: In function 'usb_driver_claim_interface':
drivers/usb/core/driver.c:513:21: warning:
variable 'udev' set but not used [-Wunused-but-set-variable]
Since commit c183813fcee44a24 ("USB: remove LPM management from
usb_driver_claim_interface()"), 'udev' is not used.
Signed-off-by: YueHaibing
Acked-by: Alan Stern
---
v2: add fixes commit info
---
drivers/usb/core/driver.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index a1f225f..5356438 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -510,7 +510,6 @@ int usb_driver_claim_interface(struct usb_driver *driver,
struct usb_interface *iface, void *priv)
{
struct device *dev;
- struct usb_device *udev;
int retval = 0;
if (!iface)
@@ -524,8 +523,6 @@ int usb_driver_claim_interface(struct usb_driver *driver,
if (!iface->authorized)
return -ENODEV;
- udev = interface_to_usbdev(iface);
-
dev->driver = &driver->drvwrap.driver;
usb_set_intfdata(iface, priv);
iface->needs_binding = 0;
[PATCH v2 -next] USB: cypress_m8: remove set but not used variables 'iflag'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/usb/serial/cypress_m8.c: In function 'cypress_set_termios':
drivers/usb/serial/cypress_m8.c:866:18: warning:
variable 'iflag' set but not used [-Wunused-but-set-variable]
Signed-off-by: YueHaibing
---
v2: only fix 'iflag' warning
---
drivers/usb/serial/cypress_m8.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/serial/cypress_m8.c b/drivers/usb/serial/cypress_m8.c
index 31c6091..8236997 100644
--- a/drivers/usb/serial/cypress_m8.c
+++ b/drivers/usb/serial/cypress_m8.c
@@ -863,7 +863,7 @@ static void cypress_set_termios(struct tty_struct *tty,
struct cypress_private *priv = usb_get_serial_port_data(port);
struct device *dev = &port->dev;
int data_bits, stop_bits, parity_type, parity_enable;
- unsigned cflag, iflag;
+ unsigned int cflag;
unsigned long flags;
__u8 oldlines;
int linechange = 0;
@@ -899,7 +899,6 @@ static void cypress_set_termios(struct tty_struct *tty,
tty->termios.c_cflag &= ~(CMSPAR|CRTSCTS);
cflag = tty->termios.c_cflag;
- iflag = tty->termios.c_iflag;
/* check if there are new settings */
if (old_termios) {
[PATCH][usb-next] usb: core: fix memory leak on port_dev_path allocation
From: Colin Ian King
Currently the allocation of port_dev_path from the call to
kobject_get_path is not being kfree'd, causing a memory leak. Fix
this by kfree'ing this at the end of the function. Add an extra
error exit path to fix one of the early leaks when envp[0] fails
to be allocated.
Detected by CoverityScan, CID#1473771 ("Resource Leak")
Fixes: 201af55da8a3 ("usb: core: added uevent for over-current")
Signed-off-by: Colin Ian King
---
drivers/usb/core/hub.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 7801bb30bdba..8ae781ce1a80 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5168,7 +5168,7 @@ static void port_over_current_notify(struct usb_port
*port_dev)
envp[0] = kasprintf(GFP_KERNEL, "OVER_CURRENT_PORT=%s", port_dev_path);
if (!envp[0])
- return;
+ goto exit_path;
envp[1] = kasprintf(GFP_KERNEL, "OVER_CURRENT_COUNT=%u",
port_dev->over_current_count);
@@ -5180,6 +5180,8 @@ static void port_over_current_notify(struct usb_port
*port_dev)
kfree(envp[1]);
exit:
kfree(envp[0]);
+exit_path:
+ kfree(port_dev_path);
}
static void port_event(struct usb_hub *hub, int port1)
--
2.17.1
[PATCH] usb: host: xhci: Find usb-phy by phandle if of_node not supported
There are use cases where the usb phy is created at runtime and
added into sysdev (for example phy-generic.c creates the usb phy
during probe and adds the phy to phy list using usb_add_phy_dev).
In this case, the sysdev may not have the "usb-phy" phandle added.
So, the existing devm_usb_get_phy_by_phandle() doesn't find the
"usb-phy" and returns -ENODEV. This patch modifies the code to search
for usb phy in of_node if "usb-phy" phandle is not found in sysdev.
Signed-off-by: Anurag Kumar Vulisha
---
drivers/usb/host/xhci-plat.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 94e9392..997836f 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -293,6 +293,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
}
hcd->usb_phy = devm_usb_get_phy_by_phandle(sysdev, "usb-phy", 0);
+ if (PTR_ERR(hcd->usb_phy) == -ENODEV)
+ hcd->usb_phy =
+ devm_usb_get_phy_by_node(sysdev, sysdev->of_node, 0);
+
if (IS_ERR(hcd->usb_phy)) {
ret = PTR_ERR(hcd->usb_phy);
if (ret == -EPROBE_DEFER)
--
2.1.1
RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
From: Ben Dooks
> Sent: 02 October 2018 17:56
>
> The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
> put an 8-byte command header onto the packet. It would be easier
> to do one skb_push() and then copy the data in once the push is
> done.
>
> Signed-off-by: Ben Dooks
> ---
> drivers/net/usb/smsc95xx.c | 25 +
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index cb19aea139d3..813ab93ee2c3 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2006,6 +2006,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
> *dev,
> bool csum = skb->ip_summed == CHECKSUM_PARTIAL;
> int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD;
> u32 tx_cmd_a, tx_cmd_b;
> + void *ptr;
It might be useful to define a structure for the header.
You might need to find the 'store unaligned 32bit word' macro though.
(Actually that will probably be better than the memcpy() which might
end up doing memory-memory copies rather than storing the register.)
Although if/when you add the tx alignment that won't be needed because the
header will be aligned.
> /* We do not advertise SG, so skbs should be already linearized */
> BUG_ON(skb_shinfo(skb)->nr_frags);
> @@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet
> *dev,
> return NULL;
> }
>
> + tx_cmd_b = (u32)skb->len;
> + tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
> +
> if (csum) {
> if (skb->len <= 45) {
> /* workaround - hardware tx checksum does not work
> @@ -2035,21 +2039,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
> usbnet *dev,
> skb_push(skb, 4);
> cpu_to_le32s(&csum_preamble);
Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely to
generate better code (at least for some architectures).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH] usb: core: Check portchange C_PORT_RESET after resetting
On Wed, 3 Oct 2018, Kai-Heng Feng wrote: > Based on USB2.0 Spec Section 11.24.2.7.2.5: > > "This bit is set when the port transitions from the Resetting state (or, > if present, the Speed_eval state) to the Enabled state." > > Also Section 11.24.2.13: > > "Setting the reset feature PORT_RESET causes the hub to signal reset on > that port. When the reset signaling is complete, the hub sets the > C_PORT_RESET status change and immediately enables the port." > > So let's also check C_PORT_RESET for reset completion. > > Signed-off-by: Kai-Heng Feng > --- > drivers/usb/core/hub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index 7801bb30bdba..d96058372280 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2721,7 +2721,8 @@ static int hub_port_wait_reset(struct usb_hub *hub, int > port1, >* so also wait for the connection to be re-established. >*/ > if (!(portstatus & USB_PORT_STAT_RESET) && > - (portstatus & USB_PORT_STAT_CONNECTION)) > + (portstatus & USB_PORT_STAT_CONNECTION) && > + (portchange & USB_PORT_STAT_C_RESET)) > break; > > /* switch to the long delay after two short delay failures */ What is the purpose of this patch? Does it fix a problem? Alan Stern
RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
On 2018-10-03 14:36, David Laight wrote:
From: Ben Dooks
Sent: 02 October 2018 17:56
The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
put an 8-byte command header onto the packet. It would be easier
to do one skb_push() and then copy the data in once the push is
done.
Signed-off-by: Ben Dooks
---
drivers/net/usb/smsc95xx.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index cb19aea139d3..813ab93ee2c3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2006,6 +2006,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
usbnet *dev,
bool csum = skb->ip_summed == CHECKSUM_PARTIAL;
int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM :
SMSC95XX_TX_OVERHEAD;
u32 tx_cmd_a, tx_cmd_b;
+ void *ptr;
It might be useful to define a structure for the header.
You might need to find the 'store unaligned 32bit word' macro though.
(Actually that will probably be better than the memcpy() which might
end up doing memory-memory copies rather than storing the register.)
Although if/when you add the tx alignment that won't be needed because
the
header will be aligned.
Ok, might be worth doing.
I did try to do a "u32 tx_cmd[2]" but the code generated ended up
storing
stuff onto the stack before copying into the packet. I agree that
possibly
going to the "put_unaligned" function might be nicer too.
If we did enable tx-align all the time then we'd not have to care about
the
alignment, but I didn't want to do that if possible as that would end up
sending up to 3 bytes extra per packet.
I am trying not too do too many changes at one time to allow roll back.
/* We do not advertise SG, so skbs should be already linearized */
BUG_ON(skb_shinfo(skb)->nr_frags);
@@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
usbnet *dev,
return NULL;
}
+ tx_cmd_b = (u32)skb->len;
+ tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
+
if (csum) {
if (skb->len <= 45) {
/* workaround - hardware tx checksum does not work
@@ -2035,21 +2039,18 @@ static struct sk_buff
*smsc95xx_tx_fixup(struct usbnet *dev,
skb_push(skb, 4);
cpu_to_le32s(&csum_preamble);
Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely
to
generate better code (at least for some architectures).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
MK1 1PT, UK
Registration No: 1397386 (Wales)
[PATCH] usb: host: xhci: Set the controller as wakeup capable
This patch modifies the xhci_plat_probe() to set the
controller as wakeup capable if "wakeup-source" is
added into the device node.
Signed-off-by: Anurag Kumar Vulisha
---
drivers/usb/host/xhci-plat.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 94e9392..602e95c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -210,6 +210,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
pm_runtime_enable(&pdev->dev);
pm_runtime_get_noresume(&pdev->dev);
+ /* Set the controller as wakeup capable */
+ if (device_property_read_bool(&pdev->dev, "wakeup-source"))
+ device_set_wakeup_capable(&pdev->dev, true);
+
hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
dev_name(&pdev->dev), NULL);
if (!hcd) {
--
2.1.1
