[PATCH -next] usb: udc: lpc32xx: remove set but not used 3 variables
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/usb/gadget/udc/lpc32xx_udc.c: In function ‘udc_protocol_cmd_r’:
drivers/usb/gadget/udc/lpc32xx_udc.c:744:6: warning: variable ‘tmp’ set but not
used [-Wunused-but-set-variable]
drivers/usb/gadget/udc/lpc32xx_udc.c: In function ‘udc_handle_dma_ep’:
drivers/usb/gadget/udc/lpc32xx_udc.c:1994:14: warning: variable ‘epstatus’ set
but not used [-Wunused-but-set-variable]
drivers/usb/gadget/udc/lpc32xx_udc.c: In function ‘udc_handle_ep0_setup’:
drivers/usb/gadget/udc/lpc32xx_udc.c:2200:22: warning: variable ‘wLength’ set
but not used [-Wunused-but-set-variable]
It is not used since commit 90fccb529d24 ("usb: gadget: Gadget directory
cleanup - group UDC drivers")
Signed-off-by: Mao Wenan
---
drivers/usb/gadget/udc/lpc32xx_udc.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c
b/drivers/usb/gadget/udc/lpc32xx_udc.c
index 606c8bc..b3e073f 100644
--- a/drivers/usb/gadget/udc/lpc32xx_udc.c
+++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
@@ -741,7 +741,6 @@ static inline void udc_protocol_cmd_data_w(struct
lpc32xx_udc *udc, u32 cmd,
* response data */
static u32 udc_protocol_cmd_r(struct lpc32xx_udc *udc, u32 cmd)
{
- u32 tmp;
int to = 1000;
/* Write a command and read data from the protocol engine */
@@ -751,7 +750,6 @@ static u32 udc_protocol_cmd_r(struct lpc32xx_udc *udc, u32
cmd)
/* Write command code */
udc_protocol_cmd_w(udc, cmd);
- tmp = readl(USBD_DEVINTST(udc->udp_baseaddr));
while ((!(readl(USBD_DEVINTST(udc->udp_baseaddr)) & USBD_CDFULL))
&& (to > 0))
to--;
@@ -1991,7 +1989,7 @@ void udc_handle_eps(struct lpc32xx_udc *udc, struct
lpc32xx_ep *ep)
/* DMA end of transfer completion */
static void udc_handle_dma_ep(struct lpc32xx_udc *udc, struct lpc32xx_ep *ep)
{
- u32 status, epstatus;
+ u32 status;
struct lpc32xx_request *req;
struct lpc32xx_usbd_dd_gad *dd;
@@ -2085,7 +2083,7 @@ static void udc_handle_dma_ep(struct lpc32xx_udc *udc,
struct lpc32xx_ep *ep)
if (udc_clearep_getsts(udc, ep->hwep_num) & EP_SEL_F) {
udc_clearep_getsts(udc, ep->hwep_num);
uda_enable_hwepint(udc, ep->hwep_num);
- epstatus = udc_clearep_getsts(udc, ep->hwep_num);
+ udc_clearep_getsts(udc, ep->hwep_num);
/* Let the EP interrupt handle the ZLP */
return;
@@ -2197,7 +2195,7 @@ static void udc_handle_ep0_setup(struct lpc32xx_udc *udc)
struct lpc32xx_ep *ep, *ep0 = &udc->ep[0];
struct usb_ctrlrequest ctrlpkt;
int i, bytes;
- u16 wIndex, wValue, wLength, reqtype, req, tmp;
+ u16 wIndex, wValue, reqtype, req, tmp;
/* Nuke previous transfers */
nuke(ep0, -EPROTO);
@@ -2213,7 +2211,6 @@ static void udc_handle_ep0_setup(struct lpc32xx_udc *udc)
/* Native endianness */
wIndex = le16_to_cpu(ctrlpkt.wIndex);
wValue = le16_to_cpu(ctrlpkt.wValue);
- wLength = le16_to_cpu(ctrlpkt.wLength);
reqtype = le16_to_cpu(ctrlpkt.bRequestType);
/* Set direction of EP0 */
--
2.7.4
Re: [PATCH] HID: hidraw: Fix invalid read in hidraw_ioctl
On Wed, 21 Aug 2019, Alan Stern wrote: > The syzbot fuzzer has reported a pair of problems in the > hidraw_ioctl() function: slab-out-of-bounds read and use-after-free > read. An example of the first: > > BUG: KASAN: slab-out-of-bounds in strlen+0x79/0x90 lib/string.c:525 > Read of size 1 at addr 8881c8035f38 by task syz-executor.4/2833 > > CPU: 1 PID: 2833 Comm: syz-executor.4 Not tainted 5.3.0-rc2+ #1 > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > Google 01/01/2011 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0xca/0x13e lib/dump_stack.c:113 > print_address_description+0x6a/0x32c mm/kasan/report.c:351 > __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482 > kasan_report+0xe/0x12 mm/kasan/common.c:612 > strlen+0x79/0x90 lib/string.c:525 > strlen include/linux/string.h:281 [inline] > hidraw_ioctl+0x245/0xae0 drivers/hid/hidraw.c:446 > vfs_ioctl fs/ioctl.c:46 [inline] > file_ioctl fs/ioctl.c:509 [inline] > do_vfs_ioctl+0xd2d/0x1330 fs/ioctl.c:696 > ksys_ioctl+0x9b/0xc0 fs/ioctl.c:713 > __do_sys_ioctl fs/ioctl.c:720 [inline] > __se_sys_ioctl fs/ioctl.c:718 [inline] > __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:718 > do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > RIP: 0033:0x459829 > Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 > 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff > ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:7f7a68f6dc78 EFLAGS: 0246 ORIG_RAX: 0010 > RAX: ffda RBX: 0003 RCX: 00459829 > RDX: RSI: 80404805 RDI: 0004 > RBP: 0075bf20 R08: R09: > R10: R11: 0246 R12: 7f7a68f6e6d4 > R13: 004c21de R14: 004d5620 R15: > > The two problems have the same cause: hidraw_ioctl() fails to test > whether the device has been removed. This patch adds the missing test. > > Reported-and-tested-by: syzbot+5a6c4ec678a0c6ee8...@syzkaller.appspotmail.com > Signed-off-by: Alan Stern Thanks a lot Alan for chasing this; I've applied the patch. -- Jiri Kosina SUSE Labs
Re: [PATCH] HID: USB: Fix general protection fault caused by Logitech driver
On Tue, 20 Aug 2019, Alan Stern wrote: > The syzbot fuzzer found a general protection fault in the HID subsystem: Applied, thanks a lot Alan. -- Jiri Kosina SUSE Labs
Re: [RFC 1/4] Add usb_get_address and usb_set_address support
Am Mittwoch, den 21.08.2019, 23:35 + schrieb charles.h...@dellteam.com: > > > > > This is a VERY cdc-net-specific function. It is not a "generic" USB > > function at all. Why does it belong in the USB core? Shouldn't it live > > in the code that handles the other cdc-net-specific logic? > > > > thanks, > > > > greg k-h > > > Thank you for this feedback, Greg. I was not sure about adding this to > message.c, because of the USB_CDC_GET_NET_ADDRESS. I had found references to > SET_ADDRESS in the USB protocol at > https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol. If one wanted a > generic USB function for SET_ADDRESS, to be used for both sending a MAC > address and receiving one, how would you suggest this be implemented? This > is a legit question because I am curious. Your implementation was, except for missing error handling, usable. The problem is where you put it. CDC messages exist only for CDC devices. Now it is true that there is no generic CDC driver. Creating a module just for that would cost more memory than it saves in most cases. But MACs are confined to network devices. Hence the functionality can be put into usbnet. It should not be put into any individual driver, so that every network driver can use it without duplication. > Your feedback led to moving the functionality into cdc_ncm.c for today's > testing, and removing all changes from messages.c, usb.h, usbnet.c, and > usbnet.h. This may be where I end up long term, but I would like to learn if > there is a possible solution that could live in message.c and be callable > from other USB-to-Ethernet aware drivers. All those drivers use usbnet. Hence there it should be. Regards Oliver
Re: [PATCH v3 10/11] RFC: usb-storage: export symbols in USB_STORAGE namespace
On Wed, Aug 21, 2019 at 04:13:29PM -0700, Christoph Hellwig wrote: On Wed, Aug 21, 2019 at 12:49:25PM +0100, Matthias Maennich wrote: Modules using these symbols are required to explicitly import the namespace. This patch was generated with the following steps and serves as a reference to use the symbol namespace feature: 1) Define DEFAULT_SYMBOL_NAMESPACE in the corresponding Makefile 2) make (see warnings during modpost about missing imports) 3) make nsdeps Instead of a DEFAULT_SYMBOL_NAMESPACE definition, the EXPORT_SYMBOL_NS variants can be used to explicitly specify the namespace. The advantage of the method used here is that newly added symbols are automatically exported and existing ones are exported without touching their respective EXPORT_SYMBOL macro expansion. So what is USB_STORAGE here? It isn't a C string, so where does it come from? To me using a C string would seem like the nicer interface vs a random cpp symbol that gets injected somewhere. To be honest, I would also prefer an interface that uses C strings or literals for the new EXPORT_SYMBOLS* macros: EXPORT_SYMBOL_NS(mysym, "USB_STORAGE"); or const char USB_STORAGE_NS[] = "USB_STORAGE"; EXPORT_SYMBOL_NS(mysym, USB_STORAGE_NS); The DEFAULT_SYMBOL_NAMESPACE define within Makefiles would get a bit more verbose in that case to express the literal: ccflags-y += -DDEFAULT_SYMBOL_NAMESPACE="\"USB_STORAGE\"" The main reason against that, is, that in the expansion of EXPORT_SYMBOL_NS, we define the ksymtab entry, which name is constructed partly by the name of the namespace: static const struct kernel_symbol __ksymtab_##sym##__##ns ... For that we depend on a cpp symbol to construct the name. I am not sure there is a reasonable way of getting rid of that without ending up constructing the ksymtab entries completely in asm as it is already done in case of PREL32_RELOCATIONS. But I am happy to be corrected. For reference that is done in patch 03/11 of this series: https://lore.kernel.org/lkml/20190821114955.12788-4-maenn...@google.com/ Cheers, Matthias
Re: [Patch V6 4/8] dt-bindings: usb: Add NVIDIA Tegra XUSB device mode controller binding
On Thu, Aug 08, 2019 at 03:07:22PM +0530, Nagarjuna Kristam wrote: > Add device-tree binding documentation for the XUSB device mode controller > present on Tegra210 SoC. This controller supports the USB 3.0 > specification. > > Signed-off-by: Nagarjuna Kristam > Reviewed-by: JC Kuo > --- > .../devicetree/bindings/usb/nvidia,tegra-xudc.txt | 110 > + > 1 file changed, 110 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.txt Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [Patch V6 3/8] phy: tegra: xusb: Add vbus override support on Tegra210
On Thu, Aug 08, 2019 at 03:07:21PM +0530, Nagarjuna Kristam wrote: > Tegra XUSB device control driver needs to control vbus override > during its operations, add API for the support. > > Signed-off-by: Nagarjuna Kristam > --- > drivers/phy/tegra/xusb-tegra210.c | 57 > +++ > drivers/phy/tegra/xusb.c | 22 +++ > drivers/phy/tegra/xusb.h | 2 ++ > include/linux/phy/tegra/xusb.h| 4 ++- > 4 files changed, 84 insertions(+), 1 deletion(-) Looks good to me now: Acked-by: Thierry Reding signature.asc Description: PGP signature
Re: [Patch V6 0/8] Tegra XUSB gadget driver support
On Thu, Aug 08, 2019 at 03:07:18PM +0530, Nagarjuna Kristam wrote: > This is the sixth version of series "Tegra XUSB gadget driver support" > > Patches 1-3 are phy driver changes to add support for device > mode. > Patches 4-7 are changes related to XUSB device mode > controller driver. > Patch 8 is to enable XUDC driver in defconfig > > Test Steps(USB 2.0): > - Enable "USB Gadget precomposed configurations" in defconfig > - Build, flash and boot Jetson TX1 > - Connect Jetson TX1 and Ubuntu device using USB A to Micro B > cable > - After boot on Jetson TX1 terminal usb0 network device should be > enumerated > - Assign static ip to usb0 on Jetson TX1 and corresponding net > device on ubuntu > - Run ping test and transfer test(used scp) to check data transfer > communication > > SS mode is verified by enabling Type A port as peripheral > > This patch series is dependent[1] on > https://patchwork.kernel.org/cover/11056429/ > > [1] Dependent series doesnot compile on Master branch due to removal of > switch_fwnode_match() in file drivers/usb/roles/class.c. > Hence verified current series changes on 5.3-RC3 branch. > --- > v6: > * Patches 1,2,3,7,8 - No changes > * Patch 4,5,6 - Comments from Rob addressed, updated usb connector driver > compatibility string. > --- > v5: > * Patches 1-3 - Commit subject updated as per inputs from Thierry > * Patch 4 - Added reg-names used on Tegra210 in the bindings doc > * Enabled xudc driver as module instead of part of kernel in patch 8 > * Patched 5-8 - No changes > --- > v4: > * patch 1 - no changes > * corrected companion device search based on inputs from Thierry in patch 2 > * removed unneeded dev variable and corrected value read in > tegra210_utmi_port_reset function in patch 3 > * dt binding doc and dtb files are corrected for alignments. > Replaced extcon-usb-gpio with usb role switch. > * Added support for USB role switch instead of extcon-usb-gpio and other minor > comments as suggested by Chunfeng. > * Enabled xudc driver as module instead of part of kernel in patch 8 > --- > V3: > * Rebased patch 1 to top of tree. > * Fixed bug in patch 2, where xudc interrupts dont get generated if USB host > mode fails to probe. Moved fake port detection logic to generic xusb.c. fake > usb port data is updated based on soc flag need_fake_usb3_port. > * Added extra lines whereever necessary to make code more readable in patch 3 > and 7. > * dt binding doc is corrected for typos and extcon references. Also added > details for clocks and removed xusb_ references to clock and power-domain > names and accordingly patch 5 is updated. > * removed avdd-pll-utmip-supply in patch 6, as its now part of padctl driver. > * Patch 8 has no changes. > --- > V2: > * Patches 1-3 are new patches in this series, which splits unified features > patch to speprated features and removes need of port-fake entry in DT. > * Patch 4 is re-arragend dt-bindings patch which incorporates previous > patch comments to sort DT entries alphabetically, addresses name changes > and PM domain details added. > * Patch 5-6 are re-arranged DT patches with major changes - sort entries > alphabetically, and adds clock names. > * Patch 7 is UDC driver tegra XUSB device mode controller with major > changes - remove un-used module params, lockinng for device_mode flag, > moving un-needed info logs to debug level, making changes feature flag > dependent rather than SOC based macros and other error handling in probe. > * Patch 8 has no changes. > > Nagarjuna Kristam (8): > phy: tegra: xusb: Add XUSB dual mode support on Tegra210 > phy: tegra: xusb: Add usb3 port fake support on Tegra210 > phy: tegra: xusb: Add vbus override support on Tegra210 I just noticed that you haven't Cc'ed the PHY framework maintainer (Kishon) on these patches. Please make sure to Cc him (on the whole set) when you send out v7. Thierry > dt-bindings: usb: Add NVIDIA Tegra XUSB device mode controller binding > arm64: tegra: Add xudc node for Tegra210 > arm64: tegra: Enable xudc on Jetson TX1 > usb: gadget: Add UDC driver for tegra XUSB device mode controller > arm64: defconfig: Enable tegra XUDC driver > > .../devicetree/bindings/usb/nvidia,tegra-xudc.txt | 110 + > arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 31 +- > arch/arm64/boot/dts/nvidia/tegra210.dtsi | 19 + > arch/arm64/configs/defconfig |1 + > drivers/phy/tegra/xusb-tegra210.c | 133 +- > drivers/phy/tegra/xusb.c | 87 + > drivers/phy/tegra/xusb.h |4 + > drivers/usb/gadget/udc/Kconfig | 11 + > drivers/usb/gadget/udc/Makefile|1 + > drivers/usb/gadget/udc/tegra_xudc.c| 3808 > > include/linux/phy/tegra/xusb.h |4 +- > 11 files changed, 4205 insertions(+), 4 deletions(-) > create mode
Re: [Patch V6 7/8] usb: gadget: Add UDC driver for tegra XUSB device mode controller
On Thu, Aug 08, 2019 at 03:07:25PM +0530, Nagarjuna Kristam wrote: > This patch adds UDC driver for tegra XUSB 3.0 device mode controller. > XUSB device mode controller supports SS, HS and FS modes > > Based on work by: > Mark Kuo > Hui Fu > Andrew Bresticker > > Signed-off-by: Nagarjuna Kristam > Acked-by: Thierry Reding > --- > drivers/usb/gadget/udc/Kconfig | 11 + > drivers/usb/gadget/udc/Makefile |1 + > drivers/usb/gadget/udc/tegra_xudc.c | 3808 > +++ > 3 files changed, 3820 insertions(+) > create mode 100644 drivers/usb/gadget/udc/tegra_xudc.c > > diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig > index ef0259a..fe6028e 100644 > --- a/drivers/usb/gadget/udc/Kconfig > +++ b/drivers/usb/gadget/udc/Kconfig > @@ -440,6 +440,17 @@ config USB_GADGET_XILINX > dynamically linked module called "udc-xilinx" and force all > gadget drivers to also be dynamically linked. > > +config USB_TEGRA_XUDC > + tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller" > + depends on ARCH_TEGRA > + select USB_ROLE_SWITCH > + help > + Enables NVIDIA Tegra USB 3.0 device mode controller driver. > + > + Say "y" to link the driver statically, or "m" to build a > + dynamically linked module called "tegra_xudc" and force all > + gadget drivers to also be dynamically linked. > + > source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig" > > # > diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile > index 897f648..1c55c96 100644 > --- a/drivers/usb/gadget/udc/Makefile > +++ b/drivers/usb/gadget/udc/Makefile > @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_BCM63XX_UDC) += bcm63xx_udc.o > obj-$(CONFIG_USB_FSL_USB2) += fsl_usb2_udc.o > fsl_usb2_udc-y := fsl_udc_core.o > fsl_usb2_udc-$(CONFIG_ARCH_MXC) += fsl_mxc_udc.o > +obj-$(CONFIG_USB_TEGRA_XUDC) += tegra_xudc.o Nit: I have a slight preference for tegra-xudc.o over tegra_xudc.o. We use dashes rather than underscores pretty consistently on Tegra, so it would be good to keep the same pattern here, unless somebody feels strongly about the underscore. It doesn't matter that much because module utilities treat them the same way I think, so the Acked-by remains valid either way. Thierry signature.asc Description: PGP signature
[PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
The optical sensor of the mouse gets turned off when it's runtime
suspended, so moving the mouse can't wake the mouse up, despite that
USB remote wakeup is successfully set.
Introduce a new quirk to prevent the mouse from getting runtime
suspended.
Signed-off-by: Kai-Heng Feng
---
drivers/hid/hid-quirks.c | 2 +-
drivers/hid/usbhid/hid-core.c | 3 ++-
include/linux/hid.h | 1 +
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index 166f41f3173b..40574f856a93 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -108,7 +108,7 @@ static const struct hid_device_id hid_quirks[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_MOUSE_C05A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
USB_DEVICE_ID_LOGITECH_MOUSE_C06A), HID_QUIRK_ALWAYS_POLL },
{ HID_USB_DEVICE(USB_VENDOR_ID_MCS, USB_DEVICE_ID_MCS_GAMEPADBLOCK),
HID_QUIRK_MULTI_INPUT },
- { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_ALWAYS_POLL },
+ { HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_PIXART_MOUSE), HID_QUIRK_NO_RUNTIME_PM },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_POWER_COVER), HID_QUIRK_NO_INIT_REPORTS },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_SURFACE_PRO_2), HID_QUIRK_NO_INIT_REPORTS },
{ HID_USB_DEVICE(USB_VENDOR_ID_MICROSOFT,
USB_DEVICE_ID_MS_TOUCH_COVER_2), HID_QUIRK_NO_INIT_REPORTS },
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index c7bc9db5b192..08a6b4f5cfb2 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -713,7 +713,8 @@ static int usbhid_open(struct hid_device *hid)
}
}
- usb_autopm_put_interface(usbhid->intf);
+ if (!(hid->quirks & HID_QUIRK_NO_RUNTIME_PM))
+ usb_autopm_put_interface(usbhid->intf);
/*
* In case events are generated while nobody was listening,
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d770ab1a0479..bec413226146 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -342,6 +342,7 @@ struct hid_item {
/* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */
#define HID_QUIRK_ALWAYS_POLL BIT(10)
#define HID_QUIRK_INPUT_PER_APPBIT(11)
+#define HID_QUIRK_NO_RUNTIME_PMBIT(12)
#define HID_QUIRK_SKIP_OUTPUT_REPORTS BIT(16)
#define HID_QUIRK_SKIP_OUTPUT_REPORT_IDBIT(17)
#define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP BIT(18)
--
2.17.1
Re: Unhandled fault: imprecise external abort (0x1406) when loading xhci_pci.ko - uPD720202, PEX8605, iMX6Q and linux-4.19.25
Hi all, I started back with the issue and now trying with latest kernel 5.2.7 and still seeing same error "Unhandled fault: imprecise external abort (0x1406) at xxx"; though this time it happens in different conditions as mentioned below (with logs): If I load the xhci-pci.ko driver, which then wait till timeout and then gives error -110 (as expected); then after loading the uPD720202 firmware (from user-space utility) I was able to see the USB 3.0 and USB2.0 devices available through uPD chip (I couldn't connect any external USB device to it yet as hardware prototype don't have power support for external devices yet). Then I did query usb-devices in a loop for 10 mins and it was good. When tried to reboot Linux then during driver unloading stage saw the crash from xhci-pci.ko driver (same error as above). So this means it was working somehow then when doing pci bus shutdown it crashed. (Didn't have log for it). Though after reboot tried to load the driver again _without_ firmware loaded into uPD and got crash number 1 and next power-cycle loaded the firmware of uPD first then loaded the driver and got crash number 2 (crashes logs are below). In all crash cases (when loading/unloading the driver); system stays response, so I can look into any possible device status using PCIe registers of PEX8605 switch or iMX6 PCIe root hub. Please suggest if something I can check to better understand the issue. Have a question that: Can I disable PCIe power management completely using some kernel CONFIG option OR boot parameter? If yes then how? Thanks === Crash log number 1 = barebox 2017.12.0-bsp-yocto-i.mx6-pd18.1.1 #1 Thu Jul 25 13:13:48 UTC 2019 Board: Phytec phyCORE-i.MX6 Quad with eMMC detected i.MX6 Quad revision 1.5 i.MX6 unique ID: ee7f9502211821d4 mdio_bus: miibus0: probed eth0: got preset MAC address: 50:2d:f4:15:a9:fe m25p80 flash@00: n25q128a13 (16384 Kbytes) imx-usb 2184200.usb: USB EHCI 1.00 imx-esdhc 219.usdhc: registered as 219.usdhc imx-esdhc 219c000.usdhc: registered as 219c000.usdhc da9063 da90620: da9062 with id 62.22.ff.1a detected state: New state registered 'state' state: Using bucket 0@0x netconsole: registered as netconsole-1 phySOM-i.MX6: Using environment in MMC malloc space: 0x4fefb480 -> 0x8fdf68ff (size 1023 MiB) mmc3: detected MMC card version 5.0 mmc3: registered mmc3.boot0 mmc3: registered mmc3.boot1 mmc3: registered mmc3 running /env/bin/init... Hit m for menu or any other key to stop autoboot:1 bootchooser: Target list $global.bootchooser.targets is empty Nothing bootable found on 'bootchooser' booting 'emmc' Loading ARM Linux zImage '/mnt/emmc/zImage' Loading devicetree from '/mnt/emmc/oftree' m25p0: Cannot find nodepath /soc/aips-bus@0200/spba-bus@0200/ecspi@02008000/flash@0, cannot fixup Failed to fixup node in of_partition_fixup+0x1/0x1f4: Invalid argument at24 24c320: Cannot find nodepath /soc/aips-bus@0210/i2c@021a8000/eeprom@50, cannot fixup Failed to fixup node in of_partition_fixup+0x1/0x1f4: Invalid argument Failed to fixup node in of_state_fixup+0x1/0x1f8: No such device mmc3: Cannot find nodepath /soc/aips-bus@0210/usdhc@0219c000, cannot fixup Failed to fixup node in of_partition_fixup+0x1/0x1f4: Invalid argument commandline: console=ttymxc1,115200n8 root=/dev/mmcblk1p2 rootwait rw Starting kernel in secure mode [0.00] Booting Linux on physical CPU 0x0 [0.00] Linux version 5.2.7-bsp-yocto-i.mx6-pd18.1.1 (flateef@flateef-XPS-13-9360) (gcc version 8.2.0 (Buildroot 2018.11.4-gebef590b)) #1 SMP Fri Aug 9 01:40:51 CEST 2019 [0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache [0.00] OF: fdt: Machine model: PHYTEC phyBOARD-Mira Quad full featured with eMMC [0.00] Memory policy: Data cache writealloc [0.00] cma: Reserved 128 MiB at 0x3800 [0.00] percpu: Embedded 16 pages/cpu s34764 r8192 d22580 u65536 [0.00] Built 1 zonelists, mobility grouping on. Total pages: 522752 [0.00] Kernel command line: console=ttymxc1,115200n8 root=/dev/mmcblk1p2 rootwait rw [0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes) [0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes) [0.00] Memory: 1934212K/2097152K available (8192K kernel code, 365K rwdata, 2628K rodata, 1024K init, 400K bss, 31868K reserved, 131072K cma-reserved, 1309804K highmem) [0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 [0.00] rcu: Hierarchical RCU implementation. [0.00] rcu: RCU event tracing is enabled. [0.00] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies. [0.00] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16 [0.00] L2C-310 errata 752271 769419 enabled [0.00] L2C-310 enabling early BRESP for C
Re: [v2 08/10] scripts: Coccinelle script for namespace dependencies
On Thu, Aug 15, 2019 at 03:50:38PM +0200, Markus Elfring wrote:
+generate_deps_for_ns() {
+$SPATCH --very-quiet --in-place --sp-file \
+ $srctree/scripts/coccinelle/misc/add_namespace.cocci -D ns=$1 $2
+}
* Where will the variable “srctree” be set for the file “scripts/nsdeps”?
$srctree is defined by kbuild in the toplevel Makefile.
* Would you like to support a separate build directory for desired adjustments?
No, as the purpose of this script is to directly patch the kernel
sources where applicable.
* How do you think about to check error handling around such commands?
spatch emits a descriptive message on error. I will add a 'set
-e' to the script so that it aborts on errors.
+generate_deps() {
…
+for source_file in $mod_source_files; do
+sed '/MODULE_IMPORT_NS/Q' $source_file > ${source_file}.tmp
…
I suggest to assign the name for the temporary file to a variable
which should be used by subsequent commands.
I somehow don't agree that this is an improvement to the code as the
variable would likely be something like ${source_file_tmp}. Sticking to
${source_file}.tmp does express the intent of a temporary file next to
the original source file and the reader of the code does not need to
reason about the value of ${source_file_tmp}.
Cheers,
Matthias
AW: Unhandled fault: imprecise external abort (0x1406) when loading xhci_pci.ko - uPD720202, PEX8605, iMX6Q and linux-4.19.25
> > Have a question that: Can I disable PCIe power management completely > using some kernel CONFIG option OR boot parameter? If yes then how? > > Thanks > See CONFIG_PM and what it depends on. BR Carsten
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng: > The optical sensor of the mouse gets turned off when it's runtime > suspended, so moving the mouse can't wake the mouse up, despite that > USB remote wakeup is successfully set. > > Introduce a new quirk to prevent the mouse from getting runtime > suspended. Hi, I am afraid this is a bad approach in principle. The device behaves according to spec. And it behaves like most hardware. If you do not want runtime PM for such devices, do not switch it on. The refcounting needs to be done correctly. This patch does something that udev should do and in a questionable manner. Regards Oliver
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Hi Oliver, at 17:45, Oliver Neukum wrote: Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng: The optical sensor of the mouse gets turned off when it's runtime suspended, so moving the mouse can't wake the mouse up, despite that USB remote wakeup is successfully set. Introduce a new quirk to prevent the mouse from getting runtime suspended. Hi, I am afraid this is a bad approach in principle. The device behaves according to spec. Can you please point out which spec it is? Is it USB 2.0 spec? And it behaves like most hardware. So seems like most hardware are broken. Maybe a more appropriate solution is to disable RPM for all USB mice. If you do not want runtime PM for such devices, do not switch it on. A device should work regardless of runtime PM status. The refcounting needs to be done correctly. Will do. This patch does something that udev should do and in a questionable manner. IMO if the device doesn’t support runtime suspend, then it needs to be disabled in kernel but not workaround in userspace. Kai-Heng Regards Oliver
Re: [Patch V6 7/8] usb: gadget: Add UDC driver for tegra XUSB device mode controller
On 09-08-2019 17:33, Felipe Balbi wrote: > > Hi, > > Nagarjuna Kristam writes: >> This patch adds UDC driver for tegra XUSB 3.0 device mode controller. >> XUSB device mode controller supports SS, HS and FS modes >> >> Based on work by: >> Mark Kuo >> Hui Fu >> Andrew Bresticker >> >> Signed-off-by: Nagarjuna Kristam >> Acked-by: Thierry Reding >> --- >> drivers/usb/gadget/udc/Kconfig | 11 + >> drivers/usb/gadget/udc/Makefile |1 + >> drivers/usb/gadget/udc/tegra_xudc.c | 3808 >> +++ >> 3 files changed, 3820 insertions(+) >> create mode 100644 drivers/usb/gadget/udc/tegra_xudc.c >> >> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig >> index ef0259a..fe6028e 100644 >> --- a/drivers/usb/gadget/udc/Kconfig >> +++ b/drivers/usb/gadget/udc/Kconfig >> @@ -440,6 +440,17 @@ config USB_GADGET_XILINX >>dynamically linked module called "udc-xilinx" and force all >>gadget drivers to also be dynamically linked. >> >> +config USB_TEGRA_XUDC >> +tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller" >> +depends on ARCH_TEGRA > > I need at least a COMPILE_TEST here. > Sure will add the same in next series. -Nagarjuna
Re: [Patch V6 7/8] usb: gadget: Add UDC driver for tegra XUSB device mode controller
On 22-08-2019 14:42, Thierry Reding wrote: > On Thu, Aug 08, 2019 at 03:07:25PM +0530, Nagarjuna Kristam wrote: >> This patch adds UDC driver for tegra XUSB 3.0 device mode controller. >> XUSB device mode controller supports SS, HS and FS modes >> >> Based on work by: >> Mark Kuo >> Hui Fu >> Andrew Bresticker >> >> Signed-off-by: Nagarjuna Kristam >> Acked-by: Thierry Reding >> --- >> drivers/usb/gadget/udc/Kconfig | 11 + >> drivers/usb/gadget/udc/Makefile |1 + >> drivers/usb/gadget/udc/tegra_xudc.c | 3808 >> +++ >> 3 files changed, 3820 insertions(+) >> create mode 100644 drivers/usb/gadget/udc/tegra_xudc.c >> >> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig >> index ef0259a..fe6028e 100644 >> --- a/drivers/usb/gadget/udc/Kconfig >> +++ b/drivers/usb/gadget/udc/Kconfig >> @@ -440,6 +440,17 @@ config USB_GADGET_XILINX >>dynamically linked module called "udc-xilinx" and force all >>gadget drivers to also be dynamically linked. >> >> +config USB_TEGRA_XUDC >> +tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller" >> +depends on ARCH_TEGRA >> +select USB_ROLE_SWITCH >> +help >> + Enables NVIDIA Tegra USB 3.0 device mode controller driver. >> + >> + Say "y" to link the driver statically, or "m" to build a >> + dynamically linked module called "tegra_xudc" and force all >> + gadget drivers to also be dynamically linked. >> + >> source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig" >> >> # >> diff --git a/drivers/usb/gadget/udc/Makefile >> b/drivers/usb/gadget/udc/Makefile >> index 897f648..1c55c96 100644 >> --- a/drivers/usb/gadget/udc/Makefile >> +++ b/drivers/usb/gadget/udc/Makefile >> @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_BCM63XX_UDC) += bcm63xx_udc.o >> obj-$(CONFIG_USB_FSL_USB2) += fsl_usb2_udc.o >> fsl_usb2_udc-y := fsl_udc_core.o >> fsl_usb2_udc-$(CONFIG_ARCH_MXC) += fsl_mxc_udc.o >> +obj-$(CONFIG_USB_TEGRA_XUDC)+= tegra_xudc.o > > Nit: I have a slight preference for tegra-xudc.o over tegra_xudc.o. We > use dashes rather than underscores pretty consistently on Tegra, so it > would be good to keep the same pattern here, unless somebody feels > strongly about the underscore. > > It doesn't matter that much because module utilities treat them the same > way I think, so the Acked-by remains valid either way. > > Thierry > Thierry, Reason to keep tegra_xudc.c instead of tegra-xudc.c is to inline with local file naming. I will use tegra-xudc.c name in next version, to be inline with other tegra files across kernel. Thanks, Nagarjuna
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng: > Hi Oliver, > > at 17:45, Oliver Neukum wrote: > > > Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng: > > > The optical sensor of the mouse gets turned off when it's runtime > > > suspended, so moving the mouse can't wake the mouse up, despite that > > > USB remote wakeup is successfully set. > > > > > > Introduce a new quirk to prevent the mouse from getting runtime > > > suspended. > > > > Hi, > > > > I am afraid this is a bad approach in principle. The device > > behaves according to spec. > > Can you please point out which spec it is? Is it USB 2.0 spec? Well, sort of. The USB spec merely states how to enter and exit a suspended state and that device state must not be lost. It does not tell you what a suspended device must be able to do. > > And it behaves like most hardware. > > So seems like most hardware are broken. > Maybe a more appropriate solution is to disable RPM for all USB mice. That is a decision a distro certainly can make. However, the kernel does not, by default, call usb_enable_autosuspend() for HID devices for this very reason. It is enabled by default only for hubs, BT dongles and UVC cameras (and some minor devices) In other words, if on your system it is on, you need to look at udev, not the kernel. > > If you do not want runtime PM for such devices, do not switch > > it on. > > A device should work regardless of runtime PM status. Well, no. Runtime PM is a trade off. You lose something if you use it. If it worked just as well as full power, you would never use full power, would you? Whether the loss of functionality or performance is worth the energy savings is a policy decision. Hence it belongs into udev. Ideally the kernel would tell user space what will work in a suspended state. Unfortunately HID does not provide support for that. This is a deficiency of user space. The kernel has an ioctl() to let user space tell it, whether a device is fully needed. X does not use them. > > The refcounting needs to be done correctly. > > Will do. Well, I am afraid your patch breaks it and if you do not break it, the patch is reduced to nothing. > > > > This patch does something that udev should do and in a > > questionable manner. > > IMO if the device doesn’t support runtime suspend, then it needs to be > disabled in kernel but not workaround in userspace. You switch it on from user space. Of course the kernel default must be safe, as you said. It already is. Regards Oliver
Re: Unhandled fault: imprecise external abort (0x1406) when loading xhci_pci.ko - uPD720202, PEX8605, iMX6Q and linux-4.19.25
Hi Carsten, Thanks for your reply. I though CONFIG_PM covers CPU only (but now I am compiling kernel with PM completely). Occasionally we see crash at boot too without even reaching stage of loading xhci-pci.ko driver. Log is below. Is this can be because of again uPD connected on PCIe bus (through PCIe switch)? barebox 2017.12.0-bsp-yocto-i.mx6-pd18.1.1 #1 Thu Jul 25 13:13:48 UTC 2019 Board: Phytec phyCORE-i.MX6 Quad with eMMC detected i.MX6 Quad revision 1.5 i.MX6 unique ID: ee7f9502211821d4 mdio_bus: miibus0: probed eth0: got preset MAC address: 50:2d:f4:15:a9:fe m25p80 flash@00: n25q128a13 (16384 Kbytes) imx-usb 2184200.usb: USB EHCI 1.00 imx-esdhc 219.usdhc: registered as 219.usdhc imx-esdhc 219c000.usdhc: registered as 219c000.usdhc da9063 da90620: da9062 with id 62.22.ff.1a detected state: New state registered 'state' state: Using bucket 0@0x netconsole: registered as netconsole-1 phySOM-i.MX6: Using environment in MMC malloc space: 0x4fefb480 -> 0x8fdf68ff (size 1023 MiB) mmc3: detected MMC card version 5.0 mmc3: registered mmc3.boot0 mmc3: registered mmc3.boot1 mmc3: registered mmc3 running /env/bin/init... Hit m for menu or any other key to stop autoboot:1 bootchooser: Target list $global.bootchooser.targets is empty Nothing bootable found on 'bootchooser' booting 'emmc' Loading ARM Linux zImage '/mnt/emmc/zImage' Loading devicetree from '/mnt/emmc/oftree' m25p0: Cannot find nodepath /soc/aips-bus@0200/spba-bus@0200/ecspi@02008000/flash@0, cannot fixup Failed to fixup node in of_partition_fixup+0x1/0x1f4: Invalid argument at24 24c320: Cannot find nodepath /soc/aips-bus@0210/i2c@021a8000/eeprom@50, cannot fixup Failed to fixup node in of_partition_fixup+0x1/0x1f4: Invalid argument Failed to fixup node in of_state_fixup+0x1/0x1f8: No such device mmc3: Cannot find nodepath /soc/aips-bus@0210/usdhc@0219c000, cannot fixup Failed to fixup node in of_partition_fixup+0x1/0x1f4: Invalid argument commandline: console=ttymxc1,115200n8 root=/dev/mmcblk1p2 rootwait rw Starting kernel in secure mode [0.00] Booting Linux on physical CPU 0x0 [0.00] Linux version 5.2.7-bsp-yocto-i.mx6-pd18.1.1 (flateef@flateef-XPS-13-9360) (gcc version 8.2.0 (Buildroot 2018.11.4-gebef590b)) #1 SMP Fri Aug 9 01:40:51 CEST 2019 [0.00] CPU: ARMv7 Processor [412fc09a] revision 10 (ARMv7), cr=10c5387d [0.00] CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache [0.00] OF: fdt: Machine model: PHYTEC phyBOARD-Mira Quad full featured with eMMC [0.00] Memory policy: Data cache writealloc [0.00] cma: Reserved 128 MiB at 0x3800 [0.00] percpu: Embedded 16 pages/cpu s34764 r8192 d22580 u65536 [0.00] Built 1 zonelists, mobility grouping on. Total pages: 522752 [0.00] Kernel command line: console=ttymxc1,115200n8 root=/dev/mmcblk1p2 rootwait rw [0.00] Dentry cache hash table entries: 131072 (order: 7, 524288 bytes) [0.00] Inode-cache hash table entries: 65536 (order: 6, 262144 bytes) [0.00] Memory: 1934212K/2097152K available (8192K kernel code, 365K rwdata, 2628K rodata, 1024K init, 400K bss, 31868K reserved, 131072K cma-reserved, 1309804K highmem) [0.00] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=4, Nodes=1 [0.00] rcu: Hierarchical RCU implementation. [0.00] rcu: RCU event tracing is enabled. [0.00] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies. [0.00] NR_IRQS: 16, nr_irqs: 16, preallocated irqs: 16 [0.00] L2C-310 errata 752271 769419 enabled [0.00] L2C-310 enabling early BRESP for Cortex-A9 [0.00] L2C-310 full line of zeros enabled for Cortex-A9 [0.00] L2C-310 ID prefetch enabled, offset 16 lines [0.00] L2C-310 dynamic clock gating enabled, standby mode enabled [0.00] L2C-310 cache controller enabled, 16 ways, 1024 kB [0.00] L2C-310: CACHE_ID 0x41c7, AUX_CTRL 0x76470001 [0.00] random: get_random_bytes called from start_kernel+0x250/0x424 with crng_init=0 [0.00] Switching to timer-based delay loop, resolution 333ns [0.14] sched_clock: 32 bits at 3000kHz, resolution 333ns, wraps every 715827882841ns [0.45] clocksource: mxc_timer1: mask: 0x max_cycles: 0x, max_idle_ns: 637086815595 ns [0.002929] Console: colour dummy device 80x30 [0.002992] Calibrating delay loop (skipped), value calculated using timer frequency.. 6.00 BogoMIPS (lpj=3) [0.003020] pid_max: default: 32768 minimum: 301 [0.003305] Mount-cache hash table entries: 2048 (order: 1, 8192 bytes) [0.003346] Mountpoint-cache hash table entries: 2048 (order: 1, 8192 bytes) [0.004458] CPU: Testing write buffer coherency: ok [0.004517] CPU0: Spectre v2: using BPIALL workaround [0.004985] CPU0: thread -1, cpu 0, socket 0, mpidr 8000 [0.006111] Setting up static identity map for 0x1010 - 0x10100078
AW: Unhandled fault: imprecise external abort (0x1406) when loading xhci_pci.ko - uPD720202, PEX8605, iMX6Q and linux-4.19.25
> > Occasionally we see crash at boot too without even reaching stage of > loading xhci-pci.ko driver. > > Log is below. Is this can be because of again uPD connected on PCIe > bus (through PCIe switch)? > > [8.263117] Unhandled fault: imprecise external abort (0x1406) at > 0xfaf06a5b Looks like a hardware issue on your board, like PCIe accesses sometimes fail. Out of my scope, sorry. Anyway, try with PM disabled. Best regards Carsten
Re: [v2 08/10] scripts: Coccinelle script for namespace dependencies
> $srctree is defined by kbuild in the toplevel Makefile.
How is this variable passed to the file “scripts/nsdeps”?
>> * Would you like to support a separate build directory for desired
>> adjustments?
>
> No, as the purpose of this script is to directly patch the kernel
> sources where applicable.
Will there occasionally be a need to provide a generated patch
(without in-place file modification)?
>> I suggest to assign the name for the temporary file to a variable
>> which should be used by subsequent commands.
>
> I somehow don't agree that this is an improvement to the code as the
> variable would likely be something like ${source_file_tmp}.
Would you dare to choose a shorter variable name?
> ${source_file}.tmp does express the intent of a temporary file next to
> the original source file and the reader of the code does not need to
> reason about the value of ${source_file_tmp}.
I would find a code variant with less suffix repetition nicer.
Regards,
Markus
Re: [PATCH 1/1] usb: xhci: avoid VBus glitch during controller reset operation
On 21.8.2019 6.18, Peter Chen wrote:
According to xHCI 1.1 CH4.19.4 Port Power:
While Chip Hardware Reset or HCRST is asserted,
the value of PP is undefined. If the xHC supports
power switches (PPC = ‘1’) then VBus may be deasserted
during this time. PP (and VBus) shall be enabled immediately
upon exiting the reset condition.
The VBus glitch may cause some USB devices work abnormal, we observe
it at NXP LS1012AFWRY/LS1043ARDB/LX2160AQDS/LS1088ARDB platforms. To
avoid this Vbus glitch, we could set PP as 0 before HCRST, and the PP
will back to 1 after HCRST according to spec.
Is this glitch causing issues already at the first xHC reset when we are
loading the xhci driver, or only later resets, like xHC reset at resume?
Reported-by: Ran Wang
Signed-off-by: Peter Chen
---
drivers/usb/host/xhci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6b34a573c3d9..f5a7b5d63031 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -167,7 +167,8 @@ int xhci_reset(struct xhci_hcd *xhci)
{
u32 command;
u32 state;
- int ret;
+ int ret, i;
+ u32 portsc;
state = readl(&xhci->op_regs->status);
@@ -181,6 +182,18 @@ int xhci_reset(struct xhci_hcd *xhci)
return 0;
}
+ /*
+* Keep PORTSC.PP as 0 before HCRST to eliminate
+* Vbus glitch, see CH 4.19.4.
+*/
+ for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
+ __le32 __iomem *port_addr = &xhci->op_regs->port_status_base +
+ NUM_PORT_REGS * i;
+ portsc = readl(port_addr);
+ portsc &= ~PORT_POWER;
+ writel(portsc, port_addr);
Not all bits read from PORTSC should be written back, you might need
portsc = xhci_port_state_to_neutral(portsc) here.
Normally I'd recommend using the xhci_set_port_power() helper instead, but
if this is is needed at driver load time then port arrays may not be set up yet.
xhci_set_port_power() would take care of possible ACPI method calls, and add
some debugging.
Not sure if this will impact some other platforms, maybe it would be better to
move this to
a separate function, and call it before xhci_reset() if a quirk is set.
-Mathias
Re: [PATCH] HID: USB: Fix general protection fault caused by Logitech driver
On Tue, Aug 20, 2019 at 10:00 PM Alan Stern wrote:
>
> The syzbot fuzzer found a general protection fault in the HID subsystem:
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: [#1] SMP KASAN
> CPU: 0 PID: 3715 Comm: syz-executor.3 Not tainted 5.2.0-rc6+ #15
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> RIP: 0010:__pm_runtime_resume+0x49/0x180 drivers/base/power/runtime.c:1069
> Code: ed 74 d5 fe 45 85 ed 0f 85 9a 00 00 00 e8 6f 73 d5 fe 48 8d bd c1 02
> 00 00 48 b8 00 00 00 00 00 fc ff df 48 89 fa 48 c1 ea 03 <0f> b6 04 02 48
> 89 fa 83 e2 07 38 d0 7f 08 84 c0 0f 85 fe 00 00 00
> RSP: 0018:8881d99d78e0 EFLAGS: 00010202
> RAX: dc00 RBX: 0020 RCX: c90003f3f000
> RDX: 000416d8686d RSI: 82676841 RDI: 0020b6c3436a
> RBP: 0020b6c340a9 R08: 8881c6d64800 R09: fbfff0e84c25
> R10: 8881d99d7940 R11: 87426127 R12: 0004
> R13: R14: 8881d9b94000 R15: 897f9048
> FS: 7f047f542700() GS:8881db20() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 001b30f21000 CR3: 0001ca032000 CR4: 001406f0
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Call Trace:
> pm_runtime_get_sync include/linux/pm_runtime.h:226 [inline]
> usb_autopm_get_interface+0x1b/0x50 drivers/usb/core/driver.c:1707
> usbhid_power+0x7c/0xe0 drivers/hid/usbhid/hid-core.c:1234
> hid_hw_power include/linux/hid.h:1038 [inline]
> hidraw_open+0x20d/0x740 drivers/hid/hidraw.c:282
> chrdev_open+0x219/0x5c0 fs/char_dev.c:413
> do_dentry_open+0x497/0x1040 fs/open.c:778
> do_last fs/namei.c:3416 [inline]
> path_openat+0x1430/0x3ff0 fs/namei.c:3533
> do_filp_open+0x1a1/0x280 fs/namei.c:3563
> do_sys_open+0x3c0/0x580 fs/open.c:1070
> do_syscall_64+0xb7/0x560 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> It turns out the fault was caused by a bug in the HID Logitech driver,
> which violates the requirement that every pathway calling
> hid_hw_start() must also call hid_hw_stop(). This patch fixes the bug
> by making sure the requirement is met.
>
> Reported-and-tested-by: syzbot+3cbe5cd105d2ad56a...@syzkaller.appspotmail.com
> Signed-off-by: Alan Stern
> CC:
>
> ---
>
> [as1909]
>
>
> drivers/hid/hid-lg.c| 10 ++
> drivers/hid/hid-lg4ff.c |1 -
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> Index: usb-devel/drivers/hid/hid-lg.c
> ===
> --- usb-devel.orig/drivers/hid/hid-lg.c
> +++ usb-devel/drivers/hid/hid-lg.c
> @@ -818,7 +818,7 @@ static int lg_probe(struct hid_device *h
>
> if (!buf) {
> ret = -ENOMEM;
> - goto err_free;
> + goto err_stop;
> }
>
> ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(cbuf),
> @@ -850,9 +850,12 @@ static int lg_probe(struct hid_device *h
> ret = lg4ff_init(hdev);
>
> if (ret)
> - goto err_free;
> + goto err_stop;
>
> return 0;
> +
> +err_stop:
> + hid_hw_stop(hdev);
> err_free:
> kfree(drv_data);
> return ret;
> @@ -863,8 +866,7 @@ static void lg_remove(struct hid_device
> struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
> if (drv_data->quirks & LG_FF4)
> lg4ff_deinit(hdev);
> - else
> - hid_hw_stop(hdev);
> + hid_hw_stop(hdev);
> kfree(drv_data);
> }
>
> Index: usb-devel/drivers/hid/hid-lg4ff.c
> ===
> --- usb-devel.orig/drivers/hid/hid-lg4ff.c
> +++ usb-devel/drivers/hid/hid-lg4ff.c
> @@ -1477,7 +1477,6 @@ int lg4ff_deinit(struct hid_device *hid)
> }
> }
> #endif
> - hid_hw_stop(hid);
> drv_data->device_props = NULL;
>
> kfree(entry);
>
Hi Alan,
I've ran the fuzzer with your patches applied overnight and noticed
another fallout of similar bugs. I think they are caused by a similar
issue in the sony HID driver. There's no hid_hw_stop() call in the "if
(!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it
look like a bug to you?
Thanks!
MY $25,000,000.00 INVESTMENT PROPOSAL WITH YOU AND IN YOUR COUNTRY.
-- Dear, With due respect this is not spam or Scam mail, because I have contacted you before and there was no response from you,I apologise if the contents of this mail are contrary to your moral ethics, which I feel may be of great disturbance to your person, but please treat this with absolute confidentiality, believing that this email reaches you in good faith. My contacting you is not a mistake or a coincidence because God can use any person known or unknown to accomplish great things. I am a lawyer and I have an investment business proposal to offer you. It is not official but should be considered as legal and confidential business. I have a customer's deposit of $US25 million dollars ready to be moved for investment if you can partner with us. We are ready to offer you 10% of this total amount as your compensation for supporting the transaction to completion. If you are interested to help me please reply me with your full details as stated below: (1) Your full names: (2) Your address: (3) Your occupation: (4) Your mobile telephone number: (5) Your nationality: (6) Your present location: (7) Your age: So that I will provide you more details on what to do and what is required for successful completion. Note: DO NOT REPLY ME IF YOU ARE NOT INTERESTED AND WITHOUT THE ABOVE MENTIONED DETAILS Sincèrement vôtre, Avocat Etienne Eku Esq.(Lawfirm) Procureur principal. De Cabinet d’avocats de l’Afrique de l’ouest. Skype:westafricalawfirm
Re: WARNING in rollback_registered_many (2)
On Wed, Aug 7, 2019 at 4:03 PM Andrey Konovalov wrote: > > On Fri, Apr 12, 2019 at 1:32 PM Andrey Konovalov > wrote: > > > > On Fri, Apr 12, 2019 at 1:29 AM syzbot > > wrote: > > > > > > syzbot has found a reproducer for the following crash on: > > > > > > HEAD commit:9a33b369 usb-fuzzer: main usb gadget fuzzer driver > > > git tree: https://github.com/google/kasan/tree/usb-fuzzer > > > console output: https://syzkaller.appspot.com/x/log.txt?x=10d552b720 > > > kernel config: https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15 > > > dashboard link: > > > https://syzkaller.appspot.com/bug?extid=40918e4d826fb2ff9b96 > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17a4c1af20 > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=121b274b20 > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit: > > > Reported-by: syzbot+40918e4d826fb2ff9...@syzkaller.appspotmail.com > > > > > > usb 1-1: r8712u: MAC Address from efuse = 00:e0:4c:87:00:00 > > > usb 1-1: r8712u: Loading firmware from "rtlwifi/rtl8712u.bin" > > > usb 1-1: USB disconnect, device number 2 > > > usb 1-1: Direct firmware load for rtlwifi/rtl8712u.bin failed with error > > > -2 > > > usb 1-1: r8712u: Firmware request failed > > > WARNING: CPU: 0 PID: 575 at net/core/dev.c:8152 > > > rollback_registered_many+0x1f3/0xe70 net/core/dev.c:8152 > > > Kernel panic - not syncing: panic_on_warn set ... > > > CPU: 0 PID: 575 Comm: kworker/0:4 Not tainted 5.1.0-rc4-319354-g9a33b36 #3 > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > > Google 01/01/2011 > > > Workqueue: usb_hub_wq hub_event > > > Call Trace: > > > __dump_stack lib/dump_stack.c:77 [inline] > > > dump_stack+0xe8/0x16e lib/dump_stack.c:113 > > > panic+0x29d/0x5f2 kernel/panic.c:214 > > > __warn.cold+0x20/0x48 kernel/panic.c:571 > > > report_bug+0x262/0x2a0 lib/bug.c:186 > > > fixup_bug arch/x86/kernel/traps.c:179 [inline] > > > fixup_bug arch/x86/kernel/traps.c:174 [inline] > > > do_error_trap+0x130/0x1f0 arch/x86/kernel/traps.c:272 > > > do_invalid_op+0x37/0x40 arch/x86/kernel/traps.c:291 > > > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973 > > > RIP: 0010:rollback_registered_many+0x1f3/0xe70 net/core/dev.c:8152 > > > Code: 05 00 00 31 ff 44 89 fe e8 5a 15 f3 f4 45 84 ff 0f 85 49 ff ff ff e8 > > > 1c 14 f3 f4 0f 1f 44 00 00 e8 12 14 f3 f4 e8 0d 14 f3 f4 <0f> 0b 4c 89 e7 > > > e8 33 72 f2 f6 31 ff 41 89 c4 89 c6 e8 27 15 f3 f4 > > > RSP: 0018:88809d087698 EFLAGS: 00010293 > > > RAX: 88809d058000 RBX: 88809624 RCX: 8c7eb146 > > > RDX: RSI: 8c7eb163 RDI: 0001 > > > RBP: 88809d0877c8 R08: 88809d058000 R09: fbfff2708111 > > > R10: fbfff2708110 R11: 93840887 R12: 888096240070 > > > R13: dc00 R14: 88809d087758 R15: > > > rollback_registered+0xf7/0x1c0 net/core/dev.c:8228 > > > unregister_netdevice_queue net/core/dev.c:9275 [inline] > > > unregister_netdevice_queue+0x1dc/0x2b0 net/core/dev.c:9268 > > > unregister_netdevice include/linux/netdevice.h:2655 [inline] > > > unregister_netdev+0x1d/0x30 net/core/dev.c:9316 > > > r871xu_dev_remove+0xe7/0x223 drivers/staging/rtl8712/usb_intf.c:604 > > > usb_unbind_interface+0x1c9/0x980 drivers/usb/core/driver.c:423 > > > __device_release_driver drivers/base/dd.c:1082 [inline] > > > device_release_driver_internal+0x436/0x4f0 drivers/base/dd.c:1113 > > > bus_remove_device+0x302/0x5c0 drivers/base/bus.c:556 > > > device_del+0x467/0xb90 drivers/base/core.c:2269 > > > usb_disable_device+0x242/0x790 drivers/usb/core/message.c:1235 > > > usb_disconnect+0x298/0x870 drivers/usb/core/hub.c:2197 > > > hub_port_connect drivers/usb/core/hub.c:4940 [inline] > > > hub_port_connect_change drivers/usb/core/hub.c:5204 [inline] > > > port_event drivers/usb/core/hub.c:5350 [inline] > > > hub_event+0xcd2/0x3b00 drivers/usb/core/hub.c:5432 > > > process_one_work+0x90f/0x1580 kernel/workqueue.c:2269 > > > process_scheduled_works kernel/workqueue.c:2331 [inline] > > > worker_thread+0x7b0/0xe20 kernel/workqueue.c:2417 > > > kthread+0x313/0x420 kernel/kthread.c:253 > > > ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352 > > > Kernel Offset: disabled > > > Rebooting in 86400 seconds.. > > > > > > > +linux-usb mailing list > > This USB bug is the most frequently triggered one right now with over > 27k kernel crashes. OK, this report is confusing. It was initially reported on the upstream instance a long time ago, but since then has stopped happening, as it was probably fixed. Then when we launched the USB fuzzing instance, it has started producing similarly named reports (with a different root cause though), and they were bucketed into this bug by syzkaller. I've improved parsing titles of such reports in syzkaller, so
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
at 18:38, Oliver Neukum wrote: Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng: Hi Oliver, at 17:45, Oliver Neukum wrote: Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng: The optical sensor of the mouse gets turned off when it's runtime suspended, so moving the mouse can't wake the mouse up, despite that USB remote wakeup is successfully set. Introduce a new quirk to prevent the mouse from getting runtime suspended. Hi, I am afraid this is a bad approach in principle. The device behaves according to spec. Can you please point out which spec it is? Is it USB 2.0 spec? Well, sort of. The USB spec merely states how to enter and exit a suspended state and that device state must not be lost. It does not tell you what a suspended device must be able to do. But shouldn’t remote wakeup signaling wakes the device up and let it exit suspend state? Or it’s okay to let the device be suspended when remote wakeup is needed but broken? And it behaves like most hardware. So seems like most hardware are broken. Maybe a more appropriate solution is to disable RPM for all USB mice. That is a decision a distro certainly can make. However, the kernel does not, by default, call usb_enable_autosuspend() for HID devices for this very reason. It is enabled by default only for hubs, BT dongles and UVC cameras (and some minor devices) In other words, if on your system it is on, you need to look at udev, not the kernel. So if a device is broken when “power/control” is flipped by user, we should deal it at userspace? That doesn’t sound right to me. If you do not want runtime PM for such devices, do not switch it on. A device should work regardless of runtime PM status. Well, no. Runtime PM is a trade off. You lose something if you use it. If it worked just as well as full power, you would never use full power, would you? I am not asking the suspended state to work as full power, but to prevent a device enters suspend state because of broken remote wakeup. Whether the loss of functionality or performance is worth the energy savings is a policy decision. Hence it belongs into udev. Ideally the kernel would tell user space what will work in a suspended state. Unfortunately HID does not provide support for that. I really don’t think “loss of functionally” belongs to policy decision. But that’s just my opinion. This is a deficiency of user space. The kernel has an ioctl() to let user space tell it, whether a device is fully needed. X does not use them. Ok, I’ll take a look at other device drivers that use it. The refcounting needs to be done correctly. Will do. Well, I am afraid your patch breaks it and if you do not break it, the patch is reduced to nothing. Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance the refcount? This patch does something that udev should do and in a questionable manner. IMO if the device doesn’t support runtime suspend, then it needs to be disabled in kernel but not workaround in userspace. You switch it on from user space. Of course the kernel default must be safe, as you said. It already is. I’d also like to hear maintainers' opinion on this issue. Kai-Heng Regards Oliver
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
Am Donnerstag, den 22.08.2019, 21:23 +0800 schrieb Kai-Heng Feng: > at 18:38, Oliver Neukum wrote: > > Well, sort of. The USB spec merely states how to enter and exit > > a suspended state and that device state must not be lost. > > It does not tell you what a suspended device must be able to do. > > But shouldn’t remote wakeup signaling wakes the device up and let it exit > suspend state? Yes. Have you tested using a button? If they indeed do not work, then the device lies about supporting remote wakeup. That would warrant a quirk, but for remote wakeup. > Or it’s okay to let the device be suspended when remote wakeup is needed > but broken? Again, the HID spec does not specify what should trigger a remote wakeup. Limiting this to mouse buttons but not movements is inconvinient, but not buggy. This is indeed what Windows does. The device is suspended when the screen saver switches on. That we do not do that is a deficiency of X. To use runtime PM regularly you need an .ini file > > In other words, if on your system it is on, you need to look > > at udev, not the kernel. > > So if a device is broken when “power/control” is flipped by user, we should > deal it at userspace? That doesn’t sound right to me. If it is broken, as in crashing we could talk about it. If it merely does not do what you want, then, yes, that is for user space to deal with. > > Well, no. Runtime PM is a trade off. You lose something if you use > > it. If it worked just as well as full power, you would never use > > full power, would you? > > I am not asking the suspended state to work as full power, but to prevent a > device enters suspend state because of broken remote wakeup. What then would be the difference between suspended and active? A small delay in data transfer? > > Whether the loss of functionality or performance is worth the energy > > savings is a policy decision. Hence it belongs into udev. > > Ideally the kernel would tell user space what will work in a > > suspended state. Unfortunately HID does not provide support for that. > > I really don’t think “loss of functionally” belongs to policy decision. But > that’s just my opinion. That is just what we do if, for example, you choose between the configs of a USB device or when you use authorization. > Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance > the refcount? No, the refcount is good. If remote wakeup is totally broken, you need to introduce a quirk that will prevent the kernel from believing the device when it claims to support it. Regards Oliver
Re: [PATCH] usb: gadget: udc: core: Fix error case while binding pending gadget drivers
On 21/08/2019 17:30, Alan Stern wrote:
> On Wed, 21 Aug 2019, Roger Quadros wrote:
>
>> If binding a pending gadget driver fails we should not
>> remove it from the pending driver list, otherwise it
>> will cause a segmentation fault later when the gadget driver is
>> unloaded.
>
>> Signed-off-by: Roger Quadros
>> ---
>> drivers/usb/gadget/udc/core.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index 7cf34beb50df..c272c8014772 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1142,7 +1142,7 @@ static int check_pending_gadget_drivers(struct usb_udc
>> *udc)
>> if (!driver->udc_name || strcmp(driver->udc_name,
>> dev_name(&udc->dev)) == 0) {
>> ret = udc_bind_to_driver(udc, driver);
>> -if (ret != -EPROBE_DEFER)
>> +if (!ret)
>> list_del(&driver->pending);
>> break;
>> }
>
> This is kind of a policy question. If binding a pending gadget driver
> fails, should the driver remain pending?
>
> Depending on the answer to this question, you might want to change the
> list_del to list_del_init. That should fix the segmentation fault
> just as well.
OK. I'll send a revised patch to retain existing policy.
cheers,
-roger
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCH v2] usb: gadget: udc: core: Fix segfault if udc_bind_to_driver() for pending driver fails
If a gadget driver is in the pending drivers list, a UDC
becomes available and udc_bind_to_driver() fails, then it
gets deleted from the pending list.
i.e. list_del(&driver->pending) in check_pending_gadget_drivers().
Then if that gadget driver is unregistered,
usb_gadget_unregister_driver() does a list_del(&driver->pending)
again thus causing a page fault as that list entry has been poisoned
by the previous list_del().
Fix this by using list_del_init() instead of list_del() in
check_pending_gadget_drivers().
Test case:
- Make sure no UDC is available
- modprobe g_mass_storage file=wrongfile
- Load UDC driver so it becomes available
lun0: unable to open backing file: wrongfile
- modprobe -r g_mass_storage
[ 60.900431] Unable to handle kernel paging request at virtual address
dead0108
[ 60.908346] Mem abort info:
[ 60.911145] ESR = 0x9644
[ 60.914227] Exception class = DABT (current EL), IL = 32 bits
[ 60.920162] SET = 0, FnV = 0
[ 60.923217] EA = 0, S1PTW = 0
[ 60.926354] Data abort info:
[ 60.929228] ISV = 0, ISS = 0x0044
[ 60.933058] CM = 0, WnR = 1
[ 60.936011] [dead0108] address between user and kernel address ranges
[ 60.943136] Internal error: Oops: 9644 [#1] PREEMPT SMP
[ 60.948691] Modules linked in: g_mass_storage(-) usb_f_mass_storage
libcomposite xhci_plat_hcd xhci_hcd usbcore ti_am335x_adc kfifo_buf omap_rng
cdns3 rng_core udc_core crc32_ce xfrm_user crct10dif_ce snd_so6
[ 60.993995] Process modprobe (pid: 834, stack limit = 0xc2aebc69)
[ 61.000765] CPU: 0 PID: 834 Comm: modprobe Not tainted
4.19.59-01963-g065f42a60499 #92
[ 61.008658] Hardware name: Texas Instruments SoC (DT)
[ 61.014472] pstate: 6005 (nZCv daif -PAN -UAO)
[ 61.019253] pc : usb_gadget_unregister_driver+0x7c/0x108 [udc_core]
[ 61.025503] lr : usb_gadget_unregister_driver+0x30/0x108 [udc_core]
[ 61.031750] sp : 1338fda0
[ 61.035049] x29: 1338fda0 x28: 800846d4
[ 61.040346] x27: x26:
[ 61.045642] x25: 5600 x24: 0800
[ 61.050938] x23: 08d7b0d0 x22: 088b07c8
[ 61.056234] x21: 0110 x20: 02020260
[ 61.061530] x19: 010ffd28 x18:
[ 61.066825] x17: x16:
[ 61.072121] x15: x14:
[ 61.077417] x13: x12:
[ 61.082712] x11: 0030 x10: 7f7f7f7f7f7f7f7f
[ 61.088008] x9 : fefefefefefefeff x8 :
[ 61.093304] x7 : x6 :
[ 61.098599] x5 : 8080 x4 :
[ 61.103895] x3 : 01100020 x2 : 800846d4
[ 61.109190] x1 : dead0100 x0 : dead0200
[ 61.114486] Call trace:
[ 61.116922] usb_gadget_unregister_driver+0x7c/0x108 [udc_core]
[ 61.122828] usb_composite_unregister+0x10/0x18 [libcomposite]
[ 61.128643] msg_cleanup+0x18/0xfce0 [g_mass_storage]
[ 61.133682] __arm64_sys_delete_module+0x17c/0x1f0
[ 61.138458] el0_svc_common+0x90/0x158
[ 61.142192] el0_svc_handler+0x2c/0x80
[ 61.145926] el0_svc+0x8/0xc
[ 61.148794] Code: eb03003f d10be033 5421 a94d0281 (f9000420)
[ 61.154869] ---[ end trace afb22e9b637bd9a7 ]---
Segmentation fault
Signed-off-by: Roger Quadros
---
Changelog:
v2
- Retain policy behaviour if pending gadget driver fails to bind.
drivers/usb/gadget/udc/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 7cf34beb50df..92af8dc98c3d 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1143,7 +1143,7 @@ static int check_pending_gadget_drivers(struct usb_udc
*udc)
dev_name(&udc->dev)) == 0) {
ret = udc_bind_to_driver(udc, driver);
if (ret != -EPROBE_DEFER)
- list_del(&driver->pending);
+ list_del_init(&driver->pending);
break;
}
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
[PATCH] typec: tcpm: fix a typo in the comparison of pdo_max_voltage
From: Colin Ian King
There appears to be a typo in the comparison of pdo_max_voltage[i]
with the previous value, currently it is checking against the
array pdo_min_voltage rather than pdo_max_voltage. I believe this
is a typo. Fix this.
Addresses-Coverity: ("Copy-paste error")
Fixes: 5007e1b5db73 ("typec: tcpm: Validate source and sink caps")
Signed-off-by: Colin Ian King
---
drivers/usb/typec/tcpm/tcpm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
index 166b28562395..96562744101c 100644
--- a/drivers/usb/typec/tcpm/tcpm.c
+++ b/drivers/usb/typec/tcpm/tcpm.c
@@ -1439,7 +1439,7 @@ static enum pdo_err tcpm_caps_err(struct tcpm_port *port,
const u32 *pdo,
else if ((pdo_min_voltage(pdo[i]) ==
pdo_min_voltage(pdo[i - 1])) &&
(pdo_max_voltage(pdo[i]) ==
- pdo_min_voltage(pdo[i - 1])))
+ pdo_max_voltage(pdo[i - 1])))
return PDO_ERR_DUPE_PDO;
break;
/*
--
2.20.1
Re: [PATCH] typec: tcpm: fix a typo in the comparison of pdo_max_voltage
On Thu, Aug 22, 2019 at 02:52:12PM +0100, Colin King wrote:
> From: Colin Ian King
>
> There appears to be a typo in the comparison of pdo_max_voltage[i]
> with the previous value, currently it is checking against the
> array pdo_min_voltage rather than pdo_max_voltage. I believe this
> is a typo. Fix this.
>
> Addresses-Coverity: ("Copy-paste error")
> Fixes: 5007e1b5db73 ("typec: tcpm: Validate source and sink caps")
> Signed-off-by: Colin Ian King
I think you are correct.
Reviewed-by: Guenter Roeck
> ---
> drivers/usb/typec/tcpm/tcpm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 166b28562395..96562744101c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1439,7 +1439,7 @@ static enum pdo_err tcpm_caps_err(struct tcpm_port
> *port, const u32 *pdo,
> else if ((pdo_min_voltage(pdo[i]) ==
> pdo_min_voltage(pdo[i - 1])) &&
>(pdo_max_voltage(pdo[i]) ==
> - pdo_min_voltage(pdo[i - 1])))
> + pdo_max_voltage(pdo[i - 1])))
> return PDO_ERR_DUPE_PDO;
> break;
> /*
> --
> 2.20.1
>
Re: WARNING in rollback_registered_many (2)
Am Mittwoch, den 07.08.2019, 16:03 +0200 schrieb Andrey Konovalov:
I may offer a preliminary analysis.
Regards
Oliver
> On Fri, Apr 12, 2019 at 1:32 PM Andrey Konovalov
> wrote:
> >
> > On Fri, Apr 12, 2019 at 1:29 AM syzbot
> > wrote:
> > >
> > > syzbot has found a reproducer for the following crash on:
> > >
> > > HEAD commit:9a33b369 usb-fuzzer: main usb gadget fuzzer driver
> > > git tree: https://github.com/google/kasan/tree/usb-fuzzer
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=10d552b720
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15
> > > dashboard link:
> > > https://syzkaller.appspot.com/bug?extid=40918e4d826fb2ff9b96
> > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17a4c1af20
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=121b274b20
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+40918e4d826fb2ff9...@syzkaller.appspotmail.com
> > >
> > > usb 1-1: r8712u: MAC Address from efuse = 00:e0:4c:87:00:00
> > > usb 1-1: r8712u: Loading firmware from "rtlwifi/rtl8712u.bin"
> > > usb 1-1: USB disconnect, device number 2
Disconnect will run which leads to
static void r871xu_dev_remove(struct usb_interface *pusb_intf)
{
struct net_device *pnetdev = usb_get_intfdata(pusb_intf);
struct usb_device *udev = interface_to_usbdev(pusb_intf);
if (pnetdev) {
^^^ This is supposed to save us
struct _adapter *padapter = netdev_priv(pnetdev);
usb_set_intfdata(pusb_intf, NULL);
release_firmware(padapter->fw);
/* never exit with a firmware callback pending */
wait_for_completion(&padapter->rtl8712_fw_ready);
if (drvpriv.drv_registered)
padapter->surprise_removed = true;
unregister_netdev(pnetdev); /* will call netdev_close() */
So we will call unregister_netdev()
> > > usb 1-1: Direct firmware load for rtlwifi/rtl8712u.bin failed with error
> > > -2
> > > usb 1-1: r8712u: Firmware request failed
So we ran into the error handling of:
static void rtl871x_load_fw_cb(const struct firmware *firmware, void *context)
{
struct _adapter *adapter = context;
complete(&adapter->rtl8712_fw_ready);
if (!firmware) {
struct usb_device *udev = adapter->dvobjpriv.pusbdev;
struct usb_interface *usb_intf = adapter->pusb_intf;
dev_err(&udev->dev, "r8712u: Firmware request failed\n");
usb_put_dev(udev);
usb_set_intfdata(usb_intf, NULL);
^^^ This is supposed to save us from deregistering an unregistered device
but it comes too late. We have already called complete.
return;
}
adapter->fw = firmware;
/* firmware available - start netdev */
register_netdev(adapter->pnetdev);
register_netdev() is not called.
> > > Kernel panic - not syncing: panic_on_warn set ...
> > > CPU: 0 PID: 575 Comm: kworker/0:4 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> > > Google 01/01/2011
> > > Workqueue: usb_hub_wq hub_event
> > > Call Trace:
> > > __dump_stack lib/dump_stack.c:77 [inline]
> > > dump_stack+0xe8/0x16e lib/dump_stack.c:113
> > > panic+0x29d/0x5f2 kernel/panic.c:214
> > > __warn.cold+0x20/0x48 kernel/panic.c:571
> > > report_bug+0x262/0x2a0 lib/bug.c:186
> > > fixup_bug arch/x86/kernel/traps.c:179 [inline]
> > > fixup_bug arch/x86/kernel/traps.c:174 [inline]
> > > do_error_trap+0x130/0x1f0 arch/x86/kernel/traps.c:272
> > > do_invalid_op+0x37/0x40 arch/x86/kernel/traps.c:291
> > > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973
This kills us.
> > > RIP: 0010:rollback_registered_many+0x1f3/0xe70 net/core/dev.c:8152
> > > Code: 05 00 00 31 ff 44 89 fe e8 5a 15 f3 f4 45 84 ff 0f 85 49 ff ff ff e8
> > > 1c 14 f3 f4 0f 1f 44 00 00 e8 12 14 f3 f4 e8 0d 14 f3 f4 <0f> 0b 4c 89 e7
> > > e8 33 72 f2 f6 31 ff 41 89 c4 89 c6 e8 27 15 f3 f4
> > > RSP: 0018:88809d087698 EFLAGS: 00010293
> > > RAX: 88809d058000 RBX: 88809624 RCX: 8c7eb146
> > > RDX: RSI: 8c7eb163 RDI: 0001
> > > RBP: 88809d0877c8 R08: 88809d058000 R09: fbfff2708111
> > > R10: fbfff2708110 R11: 93840887 R12: 888096240070
> > > R13: dc00 R14: 88809d087758 R15:
> > > rollback_registered+0xf7/0x1c0 net/core/dev.c:8228
> > > unregister_netdevice_queue net/core/dev.c:9275 [inline]
> > > unregister_netdevice_queue+0x1dc/0x2b0 net/core/dev.c:9268
> > > unregister_netdevice include/linux/netdevice.h:2655 [inline]
> > > unregister_netdev+0x1d/0x30 net/core/dev.c:9316
> > > r871xu_dev
WARNING in r871xu_dev_remove
Hello, syzbot found the following crash on: HEAD commit:eea39f24 usb-fuzzer: main usb gadget fuzzer driver git tree: https://github.com/google/kasan.git usb-fuzzer console output: https://syzkaller.appspot.com/x/log.txt?x=163ae01260 kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e dashboard link: https://syzkaller.appspot.com/bug?extid=80899a8a8efe8968cde7 compiler: gcc (GCC) 9.0.0 20181231 (experimental) syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1739cb0e60 C reproducer: https://syzkaller.appspot.com/x/repro.c?x=154fcc2e60 IMPORTANT: if you fix the bug, please add the following tag to the commit: Reported-by: syzbot+80899a8a8efe8968c...@syzkaller.appspotmail.com [ cut here ] WARNING: CPU: 1 PID: 83 at net/core/dev.c:8167 rollback_registered_many.cold+0x41/0x1bc net/core/dev.c:8167 Kernel panic - not syncing: panic_on_warn set ... CPU: 1 PID: 83 Comm: kworker/1:2 Not tainted 5.3.0-rc5+ #28 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xca/0x13e lib/dump_stack.c:113 panic+0x2a3/0x6da kernel/panic.c:219 __warn.cold+0x20/0x4a kernel/panic.c:576 report_bug+0x262/0x2a0 lib/bug.c:186 fixup_bug arch/x86/kernel/traps.c:179 [inline] fixup_bug arch/x86/kernel/traps.c:174 [inline] do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272 do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291 invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1028 RIP: 0010:rollback_registered_many.cold+0x41/0x1bc net/core/dev.c:8167 Code: ff e8 c7 26 90 fc 48 c7 c7 40 ec 63 86 e8 24 c8 7a fc 0f 0b e9 93 be ff ff e8 af 26 90 fc 48 c7 c7 40 ec 63 86 e8 0c c8 7a fc <0f> 0b 4c 89 e7 e8 f9 12 34 fd 31 ff 41 89 c4 89 c6 e8 bd 27 90 fc RSP: 0018:8881d938f6a8 EFLAGS: 00010286 RAX: 0024 RBX: 8881d2a1 RCX: RDX: RSI: 81288cfd RDI: ed103b271ec7 RBP: 8881d938f7d8 R08: 0024 R09: fbfff11ad794 R10: fbfff11ad793 R11: 88d6bc9f R12: 8881d2a10070 R13: 8881d938f768 R14: dc00 R15: rollback_registered+0xf2/0x1c0 net/core/dev.c:8243 unregister_netdevice_queue net/core/dev.c:9290 [inline] unregister_netdevice_queue+0x1d7/0x2b0 net/core/dev.c:9283 unregister_netdevice include/linux/netdevice.h:2631 [inline] unregister_netdev+0x18/0x20 net/core/dev.c:9331 r871xu_dev_remove+0xe2/0x215 drivers/staging/rtl8712/usb_intf.c:604 usb_unbind_interface+0x1bd/0x8a0 drivers/usb/core/driver.c:423 __device_release_driver drivers/base/dd.c:1134 [inline] device_release_driver_internal+0x42f/0x500 drivers/base/dd.c:1165 bus_remove_device+0x2dc/0x4a0 drivers/base/bus.c:556 device_del+0x420/0xb10 drivers/base/core.c:2339 usb_disable_device+0x211/0x690 drivers/usb/core/message.c:1237 usb_disconnect+0x284/0x8d0 drivers/usb/core/hub.c:2199 hub_port_connect drivers/usb/core/hub.c:4949 [inline] hub_port_connect_change drivers/usb/core/hub.c:5213 [inline] port_event drivers/usb/core/hub.c:5359 [inline] hub_event+0x1454/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 process_scheduled_works kernel/workqueue.c:2331 [inline] worker_thread+0x7ab/0xe20 kernel/workqueue.c:2417 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 Kernel Offset: disabled Rebooting in 86400 seconds.. --- This bug is generated by a bot. It may contain errors. See https://goo.gl/tpsmEJ for more information about syzbot. syzbot engineers can be reached at syzkal...@googlegroups.com. syzbot will keep track of this bug report. See: https://goo.gl/tpsmEJ#status for how to communicate with syzbot. syzbot can test patches for this bug, for details see: https://goo.gl/tpsmEJ#testing-patches
Re: WARNING in r871xu_dev_remove
Am Donnerstag, den 22.08.2019, 07:28 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:eea39f24 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=163ae01260
> kernel config: https://syzkaller.appspot.com/x/.config?x=d0c62209eedfd54e
> dashboard link: https://syzkaller.appspot.com/bug?extid=80899a8a8efe8968cde7
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1739cb0e60
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=154fcc2e60
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+80899a8a8efe8968c...@syzkaller.appspotmail.com
#syz test: https://github.com/google/kasan.git eea39f24
>From 4f21b5aabc448719aa612b9359d90a178cb485d8 Mon Sep 17 00:00:00 2001
From: Oliver Neukum
Date: Thu, 22 Aug 2019 16:37:33 +0200
Subject: [PATCH] rtl8712: fix race between firmware failing to load and
disconnect
We have to wait for the attempt to load the firmware to finish
before we evaluate the result.
Reported-by: syzbot+80899a8a8efe8968c...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum
---
drivers/staging/rtl8712/hal_init.c | 3 ++-
drivers/staging/rtl8712/usb_intf.c | 8 ++--
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/rtl8712/hal_init.c
b/drivers/staging/rtl8712/hal_init.c
index 40145c0338e4..42c0a3c947f1 100644
--- a/drivers/staging/rtl8712/hal_init.c
+++ b/drivers/staging/rtl8712/hal_init.c
@@ -33,7 +33,6 @@ static void rtl871x_load_fw_cb(const struct firmware
*firmware, void *context)
{
struct _adapter *adapter = context;
- complete(&adapter->rtl8712_fw_ready);
if (!firmware) {
struct usb_device *udev = adapter->dvobjpriv.pusbdev;
struct usb_interface *usb_intf = adapter->pusb_intf;
@@ -41,11 +40,13 @@ static void rtl871x_load_fw_cb(const struct firmware
*firmware, void *context)
dev_err(&udev->dev, "r8712u: Firmware request failed\n");
usb_put_dev(udev);
usb_set_intfdata(usb_intf, NULL);
+ complete(&adapter->rtl8712_fw_ready);
return;
}
adapter->fw = firmware;
/* firmware available - start netdev */
register_netdev(adapter->pnetdev);
+ complete(&adapter->rtl8712_fw_ready);
}
static const char firmware_file[] = "rtlwifi/rtl8712u.bin";
diff --git a/drivers/staging/rtl8712/usb_intf.c
b/drivers/staging/rtl8712/usb_intf.c
index d0daae0b8299..8d7b57073592 100644
--- a/drivers/staging/rtl8712/usb_intf.c
+++ b/drivers/staging/rtl8712/usb_intf.c
@@ -595,10 +595,13 @@ static void r871xu_dev_remove(struct usb_interface
*pusb_intf)
if (pnetdev) {
struct _adapter *padapter = netdev_priv(pnetdev);
- usb_set_intfdata(pusb_intf, NULL);
- release_firmware(padapter->fw);
/* never exit with a firmware callback pending */
wait_for_completion(&padapter->rtl8712_fw_ready);
+ pnetdev = usb_get_intfdata(pusb_intf);
+ usb_set_intfdata(pusb_intf, NULL);
+ if (!pnetdev)
+ goto raced_with_firmware_failure;
+ release_firmware(padapter->fw);
if (drvpriv.drv_registered)
padapter->surprise_removed = true;
unregister_netdev(pnetdev); /* will call netdev_close() */
@@ -609,6 +612,7 @@ static void r871xu_dev_remove(struct usb_interface
*pusb_intf)
r871x_dev_unload(padapter);
r8712_free_drv_sw(padapter);
+raced_with_firmware_failure:
/* decrease the reference count of the usb device structure
* when disconnect
*/
--
2.16.4
Re: [PATCH v2] usb: gadget: udc: core: Fix segfault if udc_bind_to_driver() for pending driver fails
On Thu, 22 Aug 2019, Roger Quadros wrote:
> If a gadget driver is in the pending drivers list, a UDC
> becomes available and udc_bind_to_driver() fails, then it
> gets deleted from the pending list.
> i.e. list_del(&driver->pending) in check_pending_gadget_drivers().
>
> Then if that gadget driver is unregistered,
> usb_gadget_unregister_driver() does a list_del(&driver->pending)
> again thus causing a page fault as that list entry has been poisoned
> by the previous list_del().
>
> Fix this by using list_del_init() instead of list_del() in
> check_pending_gadget_drivers().
>
> Test case:
>
> - Make sure no UDC is available
> - modprobe g_mass_storage file=wrongfile
> - Load UDC driver so it becomes available
> lun0: unable to open backing file: wrongfile
> - modprobe -r g_mass_storage
>
> [ 60.900431] Unable to handle kernel paging request at virtual address
> dead0108
> [ 60.908346] Mem abort info:
> [ 60.911145] ESR = 0x9644
> [ 60.914227] Exception class = DABT (current EL), IL = 32 bits
> [ 60.920162] SET = 0, FnV = 0
> [ 60.923217] EA = 0, S1PTW = 0
> [ 60.926354] Data abort info:
> [ 60.929228] ISV = 0, ISS = 0x0044
> [ 60.933058] CM = 0, WnR = 1
> [ 60.936011] [dead0108] address between user and kernel address
> ranges
> [ 60.943136] Internal error: Oops: 9644 [#1] PREEMPT SMP
> [ 60.948691] Modules linked in: g_mass_storage(-) usb_f_mass_storage
> libcomposite xhci_plat_hcd xhci_hcd usbcore ti_am335x_adc kfifo_buf omap_rng
> cdns3 rng_core udc_core crc32_ce xfrm_user crct10dif_ce snd_so6
> [ 60.993995] Process modprobe (pid: 834, stack limit = 0xc2aebc69)
> [ 61.000765] CPU: 0 PID: 834 Comm: modprobe Not tainted
> 4.19.59-01963-g065f42a60499 #92
> [ 61.008658] Hardware name: Texas Instruments SoC (DT)
> [ 61.014472] pstate: 6005 (nZCv daif -PAN -UAO)
> [ 61.019253] pc : usb_gadget_unregister_driver+0x7c/0x108 [udc_core]
> [ 61.025503] lr : usb_gadget_unregister_driver+0x30/0x108 [udc_core]
> [ 61.031750] sp : 1338fda0
> [ 61.035049] x29: 1338fda0 x28: 800846d4
> [ 61.040346] x27: x26:
> [ 61.045642] x25: 5600 x24: 0800
> [ 61.050938] x23: 08d7b0d0 x22: 088b07c8
> [ 61.056234] x21: 0110 x20: 02020260
> [ 61.061530] x19: 010ffd28 x18:
> [ 61.066825] x17: x16:
> [ 61.072121] x15: x14:
> [ 61.077417] x13: x12:
> [ 61.082712] x11: 0030 x10: 7f7f7f7f7f7f7f7f
> [ 61.088008] x9 : fefefefefefefeff x8 :
> [ 61.093304] x7 : x6 :
> [ 61.098599] x5 : 8080 x4 :
> [ 61.103895] x3 : 01100020 x2 : 800846d4
> [ 61.109190] x1 : dead0100 x0 : dead0200
> [ 61.114486] Call trace:
> [ 61.116922] usb_gadget_unregister_driver+0x7c/0x108 [udc_core]
> [ 61.122828] usb_composite_unregister+0x10/0x18 [libcomposite]
> [ 61.128643] msg_cleanup+0x18/0xfce0 [g_mass_storage]
> [ 61.133682] __arm64_sys_delete_module+0x17c/0x1f0
> [ 61.138458] el0_svc_common+0x90/0x158
> [ 61.142192] el0_svc_handler+0x2c/0x80
> [ 61.145926] el0_svc+0x8/0xc
> [ 61.148794] Code: eb03003f d10be033 5421 a94d0281 (f9000420)
> [ 61.154869] ---[ end trace afb22e9b637bd9a7 ]---
> Segmentation fault
>
> Signed-off-by: Roger Quadros
> ---
> Changelog:
> v2
> - Retain policy behaviour if pending gadget driver fails to bind.
>
> drivers/usb/gadget/udc/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 7cf34beb50df..92af8dc98c3d 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1143,7 +1143,7 @@ static int check_pending_gadget_drivers(struct usb_udc
> *udc)
> dev_name(&udc->dev)) == 0) {
> ret = udc_bind_to_driver(udc, driver);
> if (ret != -EPROBE_DEFER)
> - list_del(&driver->pending);
> + list_del_init(&driver->pending);
> break;
> }
>
Acked-by: Alan Stern
Re: [PATCH] HID: quirks: Disable runtime suspend on Microsoft Corp. Basic Optical Mouse v2.0
On Thu, 22 Aug 2019, Kai-Heng Feng wrote: > at 18:38, Oliver Neukum wrote: > > > Am Donnerstag, den 22.08.2019, 18:04 +0800 schrieb Kai-Heng Feng: > >> Hi Oliver, > >> > >> at 17:45, Oliver Neukum wrote: > >> > >>> Am Donnerstag, den 22.08.2019, 17:17 +0800 schrieb Kai-Heng Feng: > The optical sensor of the mouse gets turned off when it's runtime > suspended, so moving the mouse can't wake the mouse up, despite that > USB remote wakeup is successfully set. > > Introduce a new quirk to prevent the mouse from getting runtime > suspended. > >>> > >>> Hi, > >>> > >>> I am afraid this is a bad approach in principle. The device > >>> behaves according to spec. > >> > >> Can you please point out which spec it is? Is it USB 2.0 spec? > > > > Well, sort of. The USB spec merely states how to enter and exit > > a suspended state and that device state must not be lost. > > It does not tell you what a suspended device must be able to do. > > But shouldn’t remote wakeup signaling wakes the device up and let it exit > suspend state? > Or it’s okay to let the device be suspended when remote wakeup is needed > but broken? > > > > >>> And it behaves like most hardware. > >> > >> So seems like most hardware are broken. > >> Maybe a more appropriate solution is to disable RPM for all USB mice. > > > > That is a decision a distro certainly can make. However, the kernel > > does not, by default, call usb_enable_autosuspend() for HID devices > > for this very reason. It is enabled by default only for hubs, > > BT dongles and UVC cameras (and some minor devices) > > > > In other words, if on your system it is on, you need to look > > at udev, not the kernel. > > So if a device is broken when “power/control” is flipped by user, we should > deal it at userspace? That doesn’t sound right to me. > > > > >>> If you do not want runtime PM for such devices, do not switch > >>> it on. > >> > >> A device should work regardless of runtime PM status. > > > > Well, no. Runtime PM is a trade off. You lose something if you use > > it. If it worked just as well as full power, you would never use > > full power, would you? > > I am not asking the suspended state to work as full power, but to prevent a > device enters suspend state because of broken remote wakeup. > > > > > Whether the loss of functionality or performance is worth the energy > > savings is a policy decision. Hence it belongs into udev. > > Ideally the kernel would tell user space what will work in a > > suspended state. Unfortunately HID does not provide support for that. > > I really don’t think “loss of functionally” belongs to policy decision. But > that’s just my opinion. > > > > > This is a deficiency of user space. The kernel has an ioctl() > > to let user space tell it, whether a device is fully needed. > > X does not use them. > > Ok, I’ll take a look at other device drivers that use it. > > > > >>> The refcounting needs to be done correctly. > >> > >> Will do. > > > > Well, I am afraid your patch breaks it and if you do not break > > it, the patch is reduced to nothing. > > Maybe just calling usb_autopm_put_interface() in usbhid_close() to balance > the refcount? > > > > >>> This patch does something that udev should do and in a > >>> questionable manner. > >> > >> IMO if the device doesn’t support runtime suspend, then it needs to be > >> disabled in kernel but not workaround in userspace. > > > > You switch it on from user space. Of course the kernel default > > must be safe, as you said. It already is. > > I’d also like to hear maintainers' opinion on this issue. I agree with Oliver. There is no formal requirement on what actions should cause a mouse to generate a remote wakeup request. Some mice will do it when they are moved and some mice won't. If you don't like the way a particular mouse behaves then you should not allow it to go into runtime suspend. By default, the kernel prevents _all_ USB mice from being runtime suspended; the only way a mouse can be suspended is if some userspace program tells the kernel to allow it. It might be a udev script which does this, or a powertop setting, or something else. Regardless, what the kernel does is correct. Furthermore, the kernel has to accomodate users who don't mind pressing a mouse button to wake up their mice. For their sake, the kernel should not forbid a mouse from ever going into runtime suspend merely because it won't generate a wakeup request when it is moved. Alan Stern
Re: WARNING in r871xu_dev_remove
Hello, syzbot has tested the proposed patch but the reproducer still triggered crash: KASAN: use-after-free Read in device_release_driver_internal == BUG: KASAN: use-after-free in __mutex_lock_common kernel/locking/mutex.c:912 [inline] BUG: KASAN: use-after-free in __mutex_lock+0xf23/0x1360 kernel/locking/mutex.c:1077 Read of size 8 at addr 8881d644bd78 by task kworker/0:4/2855 CPU: 0 PID: 2855 Comm: kworker/0:4 Not tainted 5.3.0-rc5+ #1 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011 Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0xca/0x13e lib/dump_stack.c:113 print_address_description+0x6a/0x32c mm/kasan/report.c:351 __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482 kasan_report+0xe/0x12 mm/kasan/common.c:612 __mutex_lock_common kernel/locking/mutex.c:912 [inline] __mutex_lock+0xf23/0x1360 kernel/locking/mutex.c:1077 device_release_driver_internal+0x23/0x500 drivers/base/dd.c:1162 bus_remove_device+0x2dc/0x4a0 drivers/base/bus.c:556 device_del+0x420/0xb10 drivers/base/core.c:2339 usb_disconnect+0x4c3/0x8d0 drivers/usb/core/hub.c:2225 hub_port_connect drivers/usb/core/hub.c:4949 [inline] hub_port_connect_change drivers/usb/core/hub.c:5213 [inline] port_event drivers/usb/core/hub.c:5359 [inline] hub_event+0x1454/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 worker_thread+0x96/0xe20 kernel/workqueue.c:2415 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 Allocated by task 12: save_stack+0x1b/0x80 mm/kasan/common.c:69 set_track mm/kasan/common.c:77 [inline] __kasan_kmalloc mm/kasan/common.c:487 [inline] __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460 kmalloc include/linux/slab.h:552 [inline] kzalloc include/linux/slab.h:748 [inline] usb_alloc_dev+0x51/0xf95 drivers/usb/core/usb.c:583 hub_port_connect drivers/usb/core/hub.c:5004 [inline] hub_port_connect_change drivers/usb/core/hub.c:5213 [inline] port_event drivers/usb/core/hub.c:5359 [inline] hub_event+0x15c0/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 worker_thread+0x96/0xe20 kernel/workqueue.c:2415 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 Freed by task 2855: save_stack+0x1b/0x80 mm/kasan/common.c:69 set_track mm/kasan/common.c:77 [inline] __kasan_slab_free+0x130/0x180 mm/kasan/common.c:449 slab_free_hook mm/slub.c:1423 [inline] slab_free_freelist_hook mm/slub.c:1474 [inline] slab_free mm/slub.c:3016 [inline] kfree+0xe4/0x2f0 mm/slub.c:3957 device_release+0x71/0x200 drivers/base/core.c:1064 kobject_cleanup lib/kobject.c:693 [inline] kobject_release lib/kobject.c:722 [inline] kref_put include/linux/kref.h:65 [inline] kobject_put+0x171/0x280 lib/kobject.c:739 put_device+0x1b/0x30 drivers/base/core.c:2264 klist_put+0xce/0x170 lib/klist.c:221 bus_remove_device+0x3a4/0x4a0 drivers/base/bus.c:552 device_del+0x420/0xb10 drivers/base/core.c:2339 usb_disconnect+0x4c3/0x8d0 drivers/usb/core/hub.c:2225 hub_port_connect drivers/usb/core/hub.c:4949 [inline] hub_port_connect_change drivers/usb/core/hub.c:5213 [inline] port_event drivers/usb/core/hub.c:5359 [inline] hub_event+0x1454/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 worker_thread+0x96/0xe20 kernel/workqueue.c:2415 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 The buggy address belongs to the object at 8881d644bb80 which belongs to the cache kmalloc-2k of size 2048 The buggy address is located 504 bytes inside of 2048-byte region [8881d644bb80, 8881d644c380) The buggy address belongs to the page: page:ea0007591200 refcount:1 mapcount:0 mapping:8881da00c000 index:0x8881d6448000 compound_mapcount: 0 flags: 0x2010200(slab|head) raw: 02010200 ea000760ac00 00020002 8881da00c000 raw: 8881d6448000 800f000a 0001 page dumped because: kasan: bad access detected Memory state around the buggy address: 8881d644bc00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb 8881d644bc80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb 8881d644bd00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb ^ 8881d644bd80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb 8881d644be00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb == Tested on: commit: eea39f24 usb-fuzzer: main usb gadget fuzzer driver git tree: https://github.com/google/kasan.git console output: https://syzkaller.appspot.com/x/log.txt?x=1018661e60 kernel config: https://syzkaller.ap
Re: WARNING in rollback_registered_many (2)
On Thu, Aug 22, 2019 at 3:07 PM Andrey Konovalov wrote: > > On Wed, Aug 7, 2019 at 4:03 PM Andrey Konovalov wrote: > > > > On Fri, Apr 12, 2019 at 1:32 PM Andrey Konovalov > > wrote: > > > > > > On Fri, Apr 12, 2019 at 1:29 AM syzbot > > > wrote: > > > > > > > > syzbot has found a reproducer for the following crash on: > > > > > > > > HEAD commit:9a33b369 usb-fuzzer: main usb gadget fuzzer driver > > > > git tree: https://github.com/google/kasan/tree/usb-fuzzer > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=10d552b720 > > > > kernel config: > > > > https://syzkaller.appspot.com/x/.config?x=23e37f59d94ddd15 > > > > dashboard link: > > > > https://syzkaller.appspot.com/bug?extid=40918e4d826fb2ff9b96 > > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental) > > > > syz repro: > > > > https://syzkaller.appspot.com/x/repro.syz?x=17a4c1af20 > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=121b274b20 > > > > > > > > IMPORTANT: if you fix the bug, please add the following tag to the > > > > commit: > > > > Reported-by: syzbot+40918e4d826fb2ff9...@syzkaller.appspotmail.com > > > > > > > > usb 1-1: r8712u: MAC Address from efuse = 00:e0:4c:87:00:00 > > > > usb 1-1: r8712u: Loading firmware from "rtlwifi/rtl8712u.bin" > > > > usb 1-1: USB disconnect, device number 2 > > > > usb 1-1: Direct firmware load for rtlwifi/rtl8712u.bin failed with > > > > error -2 > > > > usb 1-1: r8712u: Firmware request failed > > > > WARNING: CPU: 0 PID: 575 at net/core/dev.c:8152 > > > > rollback_registered_many+0x1f3/0xe70 net/core/dev.c:8152 > > > > Kernel panic - not syncing: panic_on_warn set ... > > > > CPU: 0 PID: 575 Comm: kworker/0:4 Not tainted 5.1.0-rc4-319354-g9a33b36 > > > > #3 > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS > > > > Google 01/01/2011 > > > > Workqueue: usb_hub_wq hub_event > > > > Call Trace: > > > > __dump_stack lib/dump_stack.c:77 [inline] > > > > dump_stack+0xe8/0x16e lib/dump_stack.c:113 > > > > panic+0x29d/0x5f2 kernel/panic.c:214 > > > > __warn.cold+0x20/0x48 kernel/panic.c:571 > > > > report_bug+0x262/0x2a0 lib/bug.c:186 > > > > fixup_bug arch/x86/kernel/traps.c:179 [inline] > > > > fixup_bug arch/x86/kernel/traps.c:174 [inline] > > > > do_error_trap+0x130/0x1f0 arch/x86/kernel/traps.c:272 > > > > do_invalid_op+0x37/0x40 arch/x86/kernel/traps.c:291 > > > > invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:973 > > > > RIP: 0010:rollback_registered_many+0x1f3/0xe70 net/core/dev.c:8152 > > > > Code: 05 00 00 31 ff 44 89 fe e8 5a 15 f3 f4 45 84 ff 0f 85 49 ff ff ff > > > > e8 > > > > 1c 14 f3 f4 0f 1f 44 00 00 e8 12 14 f3 f4 e8 0d 14 f3 f4 <0f> 0b 4c 89 > > > > e7 > > > > e8 33 72 f2 f6 31 ff 41 89 c4 89 c6 e8 27 15 f3 f4 > > > > RSP: 0018:88809d087698 EFLAGS: 00010293 > > > > RAX: 88809d058000 RBX: 88809624 RCX: 8c7eb146 > > > > RDX: RSI: 8c7eb163 RDI: 0001 > > > > RBP: 88809d0877c8 R08: 88809d058000 R09: fbfff2708111 > > > > R10: fbfff2708110 R11: 93840887 R12: 888096240070 > > > > R13: dc00 R14: 88809d087758 R15: > > > > rollback_registered+0xf7/0x1c0 net/core/dev.c:8228 > > > > unregister_netdevice_queue net/core/dev.c:9275 [inline] > > > > unregister_netdevice_queue+0x1dc/0x2b0 net/core/dev.c:9268 > > > > unregister_netdevice include/linux/netdevice.h:2655 [inline] > > > > unregister_netdev+0x1d/0x30 net/core/dev.c:9316 > > > > r871xu_dev_remove+0xe7/0x223 drivers/staging/rtl8712/usb_intf.c:604 > > > > usb_unbind_interface+0x1c9/0x980 drivers/usb/core/driver.c:423 > > > > __device_release_driver drivers/base/dd.c:1082 [inline] > > > > device_release_driver_internal+0x436/0x4f0 drivers/base/dd.c:1113 > > > > bus_remove_device+0x302/0x5c0 drivers/base/bus.c:556 > > > > device_del+0x467/0xb90 drivers/base/core.c:2269 > > > > usb_disable_device+0x242/0x790 drivers/usb/core/message.c:1235 > > > > usb_disconnect+0x298/0x870 drivers/usb/core/hub.c:2197 > > > > hub_port_connect drivers/usb/core/hub.c:4940 [inline] > > > > hub_port_connect_change drivers/usb/core/hub.c:5204 [inline] > > > > port_event drivers/usb/core/hub.c:5350 [inline] > > > > hub_event+0xcd2/0x3b00 drivers/usb/core/hub.c:5432 > > > > process_one_work+0x90f/0x1580 kernel/workqueue.c:2269 > > > > process_scheduled_works kernel/workqueue.c:2331 [inline] > > > > worker_thread+0x7b0/0xe20 kernel/workqueue.c:2417 > > > > kthread+0x313/0x420 kernel/kthread.c:253 > > > > ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352 > > > > Kernel Offset: disabled > > > > Rebooting in 86400 seconds.. > > > > > > > > > > +linux-usb mailing list > > > > This USB bug is the most frequently triggered one right now with over > > 27k kernel crashes. > > OK, this report is confusing. It was initially reported on the > upstream instance a long time ago, but since then has
Re: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
On 16.8.2019 12.03, Schmid, Carsten wrote: On driver removal, the platform_device_unregister call attached through devm_add_action_or_reset was executed after usb_hcd_pci_remove. This lead to a use-after-free for the iomem resource of the xhci-ext-caps driver in the platform removal because the parent of the resource was freed earlier. Fix this by reordering of the removal sequence. Could all this be avoided if usb_hcd_pci_probe() used managed device resources as well? (using devm_request_mem_region(), and devm_ioremap_nocache()) This way the iomem resource would be added to the same devres list as the platform_unregister_call, and the iomem resource should be released after the platform_device_unregister as devres_release_all() releases the resources in reverse order. -Mathias
Re: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
Hi, On 22-08-19 17:23, Mathias Nyman wrote: On 16.8.2019 12.03, Schmid, Carsten wrote: On driver removal, the platform_device_unregister call attached through devm_add_action_or_reset was executed after usb_hcd_pci_remove. This lead to a use-after-free for the iomem resource of the xhci-ext-caps driver in the platform removal because the parent of the resource was freed earlier. Fix this by reordering of the removal sequence. Could all this be avoided if usb_hcd_pci_probe() used managed device resources as well? (using devm_request_mem_region(), and devm_ioremap_nocache()) This way the iomem resource would be added to the same devres list as the platform_unregister_call, and the iomem resource should be released after the platform_device_unregister as devres_release_all() releases the resources in reverse order. Yes I believe that that would work. Regards, Hans
Re: [PATCH] typec: tcpm: fix a typo in the comparison of pdo_max_voltage
On Thu, Aug 22, 2019 at 02:52:12PM +0100, Colin King wrote:
> From: Colin Ian King
>
> There appears to be a typo in the comparison of pdo_max_voltage[i]
> with the previous value, currently it is checking against the
> array pdo_min_voltage rather than pdo_max_voltage. I believe this
> is a typo. Fix this.
>
> Addresses-Coverity: ("Copy-paste error")
> Fixes: 5007e1b5db73 ("typec: tcpm: Validate source and sink caps")
> Signed-off-by: Colin Ian King
Reviewed-by: Heikki Krogerus
> ---
> drivers/usb/typec/tcpm/tcpm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index 166b28562395..96562744101c 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -1439,7 +1439,7 @@ static enum pdo_err tcpm_caps_err(struct tcpm_port
> *port, const u32 *pdo,
> else if ((pdo_min_voltage(pdo[i]) ==
> pdo_min_voltage(pdo[i - 1])) &&
>(pdo_max_voltage(pdo[i]) ==
> - pdo_min_voltage(pdo[i - 1])))
> + pdo_max_voltage(pdo[i - 1])))
> return PDO_ERR_DUPE_PDO;
> break;
> /*
> --
> 2.20.1
thanks,
--
heikki
AW: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
> On 22-08-19 17:23, Mathias Nyman wrote: > > On 16.8.2019 12.03, Schmid, Carsten wrote: > >> On driver removal, the platform_device_unregister call > >> attached through devm_add_action_or_reset was executed > >> after usb_hcd_pci_remove. > >> This lead to a use-after-free for the iomem resource of > >> the xhci-ext-caps driver in the platform removal > >> because the parent of the resource was freed earlier. > >> > >> Fix this by reordering of the removal sequence. > >> > > > > Could all this be avoided if usb_hcd_pci_probe() > > used managed device resources as well? > > (using devm_request_mem_region(), and devm_ioremap_nocache()) > > > > This way the iomem resource would be added to the same devres list > > as the platform_unregister_call, and the iomem resource should be > > released after the platform_device_unregister as devres_release_all() > > releases the resources in reverse order. > > Yes I believe that that would work. > I don't think so, because xhci_create_intel_xhci_sw_pdev registers it's resource through platform_device_add_resources which does not use devm_. The only thing what is done through devm in xhci_create_intel_xhci_sw_pdev is ret = devm_add_action_or_reset(...) Carsten
RK3288 dwc2 USB OTG + macOS
I'm having issues on a Firefly rk3288 board when trying to use USB gadget ethernet on macOS. The dr_mode is set to "otg" and it works fine with my Linux desktop. If I set the dr_mode to "peripheral" macOS will work, but still takes around 10 seconds to enumerate the device which makes me think it's only just working. However, I need the port to be in "otg" mode as it will switch between peripheral/host use cases. I've attached a log from the dwc2 driver from mainline Linux 5.2 when being plugged into the macOS device for 30 seconds, then removed. The mac in this case is a 2013 macbook pro. Any pointers in the right direction would be greatly appreciated. Regards, Jack. [ 141.300412] dwc2 ff58.usb: ep1in: req 58615452: 337@a3ab679a, noi=0, zero=1, snok=0 [ 141.300418] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_start: ep1in: buf=a3ab679a length=337 [ 142.285792] dwc2 ff58.usb: dwc2_hsotg_irq: 04008428 0400 (d88c3cc4) retry 8 [ 142.285797] dwc2 ff58.usb: GINTSTS_ErlySusp [ 142.288847] dwc2 ff58.usb: gintsts=04008828 gintmsk=d88c3cc4 [ 142.288851] dwc2 ff58.usb: USB SUSPEND [ 142.288855] dwc2 ff58.usb: dwc2_handle_usb_suspend_intr: DSTS=0x483901 [ 142.288859] dwc2 ff58.usb: DSTS.Suspend Status=1 HWCFG4.Power Optimize=1 HWCFG4.Hibernation=0 [ 142.288862] g_ether gadget: suspend [ 142.288868] dwc2 ff58.usb: dwc2_hsotg_irq: 04008028 (d88c3cc4) retry 8 [ 142.382505] dwc2 ff58.usb: dwc2_hsotg_irq: 04809028 00801000 (d88c3cc4) retry 8 [ 142.382510] dwc2 ff58.usb: dwc2_hsotg_irq: USBRstDet [ 142.382513] dwc2 ff58.usb: dwc2_hsotg_irq: USBRst [ 142.382516] dwc2 ff58.usb: GNPTXSTS=00080010 [ 142.382524] dwc2 ff58.usb: complete: ep 3944e9db ep0, req d536b4a1, -108 => 397b7f01 [ 142.382530] dwc2 ff58.usb: dwc2_hsotg_complete_setup: failed -108 [ 142.382535] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 5373e218, -108 => 5f11106e [ 142.382538] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382545] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 0c36dad6, -108 => 5f11106e [ 142.382548] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382553] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 9060b312, -108 => 5f11106e [ 142.382556] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382561] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 721cede2, -108 => 5f11106e [ 142.382564] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382568] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 220f7ef1, -108 => 5f11106e [ 142.382571] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382576] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 5beaf0e1, -108 => 5f11106e [ 142.382579] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382584] dwc2 ff58.usb: complete: ep a23a8600 ep1in, req 58615452, -108 => 5f11106e [ 142.382587] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep1in: status=-108 actual-length=0 [ 142.382592] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 8dad2bf7, -108 => d4895dfd [ 142.382596] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382601] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 6a45280f, -108 => d4895dfd [ 142.382605] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382610] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 82a9d402, -108 => d4895dfd [ 142.382613] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382618] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 7adb743e, -108 => d4895dfd [ 142.382622] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382626] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 2e0b57de, -108 => d4895dfd [ 142.382629] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382634] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 3920b6da, -108 => d4895dfd [ 142.382638] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382642] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 7941a31e, -108 => d4895dfd [ 142.382646] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382650] dwc2 ff58.usb: complete: ep 1a3e8513 ep2out, req 2fb2d569, -108 => d4895dfd [ 142.382654] dwc2 ff58.usb: dwc2_hsotg_handle_unaligned_buf_complete: ep2out: status=-108 actual-length=0 [ 142.382658] d
Re: [PATCH v5 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci
On Fri, Aug 09, 2019 at 12:54:34AM +0900, Suwan Kim wrote: > vhci doesn’t do DMA for remote device. Actually, the real DMA > operation is done by network card driver. vhci just passes virtual > address of the buffer to the network stack, so vhci doesn’t use and > need dma address of the buffer of the URB. > > But HCD provides DMA mapping and unmapping function by default. > Moreover, it causes unnecessary DMA mapping and unmapping which > will be done again at the NIC driver and it wastes CPU cycles. > So, implement map_urb_for_dma and unmap_urb_for_dma function for > vhci in order to skip the DMA mapping and unmapping procedure. > > When it comes to supporting SG for vhci, it is useful to use native > SG list (urb->num_sgs) instead of mapped SG list because DMA mapping > fnuction can adjust the number of SG list (urb->num_mapped_sgs). > And vhci_map_urb_for_dma() prevents isoc pipe from using SG as > hcd_map_urb_for_dma() does. > > Signed-off-by: Suwan Kim Can you please redo this patch based on my usb-next branch that has Christoph's DMA changes in it? It should make your change much simpler and smaller. Please do that and resend the whole series. thanks, greg k-h
Re: next take at setting up a dma mask by default for platform devices v2
On Fri, Aug 16, 2019 at 08:24:29AM +0200, Christoph Hellwig wrote: > Hi all, > > this is another attempt to make sure the dma_mask pointer is always > initialized for platform devices. Not doing so lead to lots of > boilerplate code, and makes platform devices different from all our > major busses like PCI where we always set up a dma_mask. In the long > run this should also help to eventually make dma_mask a scalar value > instead of a pointer and remove even more cruft. > > The bigger blocker for this last time was the fact that the usb > subsystem uses the presence or lack of a dma_mask to check if the core > should do dma mapping for the driver, which is highly unusual. So we > fix this first. Note that this has some overlap with the pending > desire to use the proper dma_mmap_coherent helper for mapping usb > buffers. The first two patches have already been queued up by Greg > and are only included for completeness. Note to everyone. The first two patches in this series is already in 5.3-rc5. I've applied the rest of the series to my usb-next branch (with the 6th patch landing there later today.) They are scheduled to be merge to Linus in 5.4-rc1. Christoph, thanks so much for these cleanups. greg k-h
Re: [PATCH] HID: USB: Fix general protection fault caused by Logitech driver
On Thu, 22 Aug 2019, Andrey Konovalov wrote: > Hi Alan, > > I've ran the fuzzer with your patches applied overnight and noticed > another fallout of similar bugs. I think they are caused by a similar > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > look like a bug to you? It looks like the relevant hid_hw_stop() call is the one at the end of sony_configure_input(). But I can't tell if doing that way is valid or not -- in practice the code would end up calling hid_disconnect() while hid_connect() was still running, which doesn't seem like a good idea. There's a comment about this near the end of sony_probe(). I suspect it would be better to call hid_hw_stop() in the conditional code following that comment rather than in sony_configure_input(). Either way, these are all things Jiri should know about or check up on. Have you gotten any test results from syzbot exercising these pathways? You ought to be able to tell which HID driver is involved by looking through the console output. Alan Stern
RE: [RFC 1/4] Add usb_get_address and usb_set_address support
> > > > > > > > This is a VERY cdc-net-specific function. It is not a "generic" USB > > > function at all. Why does it belong in the USB core? Shouldn't it > > > live in the code that handles the other cdc-net-specific logic? > > > > > > thanks, > > > > > > greg k-h > > > > > > Thank you for this feedback, Greg. I was not sure about adding this to > message.c, because of the USB_CDC_GET_NET_ADDRESS. I had found > references to SET_ADDRESS in the USB protocol at > https://wiki.osdev.org/Universal_Serial_Bus#USB_Protocol. If one wanted a > generic USB function for SET_ADDRESS, to be used for both sending a MAC > address and receiving one, how would you suggest this be implemented? This > is a legit question because I am curious. > > Your implementation was, except for missing error handling, usable. > The problem is where you put it. CDC messages exist only for CDC devices. Now > it is true that there is no generic CDC driver. > Creating a module just for that would cost more memory than it saves in most > cases. > But MACs are confined to network devices. Hence the functionality can be put > into usbnet. It should not be put into any individual driver, so that every > network driver can use it without duplication. > > > Your feedback led to moving the functionality into cdc_ncm.c for today's > testing, and removing all changes from messages.c, usb.h, usbnet.c, and > usbnet.h. This may be where I end up long term, but I would like to learn if > there is a possible solution that could live in message.c and be callable from > other USB-to-Ethernet aware drivers. > > All those drivers use usbnet. Hence there it should be. > > Regards > Oliver Some of the drivers in drivers/net/usb/ do call functions in drivers/net/usb/usbnet, but not all. As Greg pointed out, the USB change I developed is cdc specific, so putting it into usbnet would raise the same concerns Greg mentioned. Leaving my newest implementation in cdc_ncm.c will be most appropriate, as it also fits with what other drivers in this folder have done. My original code was rather short sighted, at best. Charles
Re: f_mass_storage vs drivers/target
On Thu, 22 Aug 2019, Benjamin Herrenschmidt wrote: > On Thu, 2019-08-22 at 14:58 +1000, Benjamin Herrenschmidt wrote: > > > > Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging... > > > > While digging I found this gem: > > > > /* USB3380: use same address for usb and hardware endpoints */ > > snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc), > > usb_endpoint_dir_in(desc) ? "in" : "out"); > > ep = gadget_find_ep_by_name(_gadget, name); > > if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp)) > > return ep; > > > > Any idea what's that supposed to achieve ? It looks like in one mode, the endpoint number has to be the value predetermined by the hardware. In the other mode, any hardware endpoint can be assigned any endpoint number. > > When ep_match is called, usb_endpoint_num() hasn't been set yet so > > it's always 0 and always fails... or am I missing something ? > > Two problems: > > - net2280.c doesn't set a max EP size, so autoconfig fails since > f_tcm specifies one. What about this ? > > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -940,12 +940,14 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget, > if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out) > return 0; > > - if (max > ep->maxpacket_limit) > + if (ep->maxpacket_limit && max > ep->maxpacket_limit) > return 0; > > (ie assume that ep->maxpacket_limit 0 means the UDC supports any > legal size) That looks reasonable. > - No UDC driver other than dummy sets max_streams, and f_tcm requires 4, > so f_tcm will fail with *any* superspeed UDC driver as far as I can tell. > > Was it ever tested with USB 3 ? Note that USB 2 does not support streams at all. > I'm not sure what the right fix here yet is as I yet have to learn about > what those USB3 streams are :-) For now I've commented things out. They are for multiplexing multiple data streams over a single USB endpoint. As far as I know, the only use case for such a thing is USB Mass Storage. Alan Stern > It's still not working yet as configuring f_tcm seems to be a black art > with no useful documentation or examples anywhere (the device shows up on > the host but doesn't bind to any mass storage driver ... yet). > > Cheers, > Ben.
Re: [PATCH] HID: USB: Fix general protection fault caused by Logitech driver
On Thu, Aug 22, 2019 at 7:11 PM Alan Stern wrote: > > On Thu, 22 Aug 2019, Andrey Konovalov wrote: > > > Hi Alan, > > > > I've ran the fuzzer with your patches applied overnight and noticed > > another fallout of similar bugs. I think they are caused by a similar > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > look like a bug to you? > > It looks like the relevant hid_hw_stop() call is the one at the end of > sony_configure_input(). But I can't tell if doing that way is valid or > not -- in practice the code would end up calling hid_disconnect() while > hid_connect() was still running, which doesn't seem like a good idea. > > There's a comment about this near the end of sony_probe(). I suspect > it would be better to call hid_hw_stop() in the conditional code > following that comment rather than in sony_configure_input(). > > Either way, these are all things Jiri should know about or check up on. > > Have you gotten any test results from syzbot exercising these pathways? > You ought to be able to tell which HID driver is involved by looking > through the console output. Yes, a typical crash is below, that's why I thought it's the sony driver. Adding hid_hw_stop() in sony_probe() stops the issue from happening, but I don't know whether it's the right fix. usb 1-1: new high-speed USB device number 3 using dummy_hcd usb 1-1: Using ep0 maxpacket: 8 usb 1-1: config 0 interface 0 altsetting 0 endpoint 0x81 has an invalid bInterval 0, changing7 usb 1-1: config 0 interface 0 altsetting 0 has 1 endpoint descriptor, different from the inte9 usb 1-1: New USB device found, idVendor=054c, idProduct=024b, bcdDevice= 0.00 usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0 usb 1-1: config 0 descriptor?? sony 0003:054C:024B.0002: unknown main item tag 0x0 sony 0003:054C:024B.0002: unknown main item tag 0x2 sony 0003:054C:024B.0002: unknown main item tag 0x0 sony 0003:054C:024B.0002: unknown main item tag 0x0 ... sony 0003:054C:024B.0002: unknown main item tag 0x0 == BUG: KASAN: use-after-free in usbhid_power+0xca/0xe0 Read of size 8 at addr 88805d590008 by task syz-executor/1808 CPU: 1 PID: 1808 Comm: syz-executor Not tainted 5.3.0-rc5+ #203 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 dump_stack+0xca/0x13e lib/dump_stack.c:113 print_address_description+0x6a/0x32c mm/kasan/report.c:351 __kasan_report.cold+0x1a/0x33 mm/kasan/report.c:482 kasan_report+0xe/0x12 mm/kasan/common.c:612 usbhid_power+0xca/0xe0 drivers/hid/usbhid/hid-core.c:1234 hid_hw_power ./include/linux/hid.h:1038 hidraw_open+0x20d/0x740 drivers/hid/hidraw.c:282 chrdev_open+0x219/0x5c0 fs/char_dev.c:414 do_dentry_open+0x494/0x1120 fs/open.c:797 do_last fs/namei.c:3416 path_openat+0x1430/0x3f50 fs/namei.c:3533 do_filp_open+0x1a1/0x280 fs/namei.c:3563 do_sys_open+0x3c0/0x580 fs/open.c:1089 do_syscall_64+0xb7/0x580 arch/x86/entry/common.c:296 entry_SYSCALL_64_after_hwframe+0x49/0xbe arch/x86/entry/entry_64.S:175 RIP: 0033:0x413a0e Code: 89 54 24 08 e8 a3 f9 ff ff 8b 74 24 0c 48 8b 3c 24 41 89 c0 44 8b 54 24 08 b8 01 01 00 4 RSP: 002b:7f7bf0a66730 EFLAGS: 0293 ORIG_RAX: 0101 RAX: ffda RBX: 6667 RCX: 00413a0e RDX: 0200 RSI: 7f7bf0a66840 RDI: ff9c RBP: 7f7bf0a66840 R08: R09: R10: R11: 0293 R12: 004a521c R13: 004ef7d0 R14: 004ae881 R15: 7f7bf0a676bc Allocated by task 78: save_stack+0x1b/0x80 mm/kasan/common.c:69 set_track mm/kasan/common.c:77 __kasan_kmalloc mm/kasan/common.c:487 __kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:460 slab_post_alloc_hook mm/slab.h:520 slab_alloc_node mm/slub.c:2770 __kmalloc_node_track_caller+0xfc/0x380 mm/slub.c:4365 __kmalloc_reserve.isra.0+0x39/0xe0 net/core/skbuff.c:141 __alloc_skb+0xef/0x5a0 net/core/skbuff.c:209 alloc_skb ./include/linux/skbuff.h:1055 alloc_uevent_skb+0x7b/0x210 lib/kobject_uevent.c:289 uevent_net_broadcast_untagged lib/kobject_uevent.c:325 kobject_uevent_net_broadcast lib/kobject_uevent.c:408 kobject_uevent_env+0x8ee/0x1160 lib/kobject_uevent.c:592 device_del+0x6b2/0xb10 drivers/base/core.c:2349 usb_disable_device+0x211/0x690 drivers/usb/core/message.c:1237 usb_disconnect+0x284/0x8d0 drivers/usb/core/hub.c:2199 hub_port_connect drivers/usb/core/hub.c:4949 hub_port_connect_change drivers/usb/core/hub.c:5213 port_event drivers/usb/core/hub.c:5359 hub_event+0x1454/0x3640 drivers/usb/core/hub.c:5441 process_one_work+0x92b/0x1530 kernel/workqueue.c:2269 worker_thread+0x96/0xe20 kernel/workqueue.c:2415 kthread+0x318/0x420 kernel/kthread.c:255 ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352 Freed by task 242: save_stack+0x1
Re: Unhandled fault: imprecise external abort (0x1406) when loading xhci_pci.ko - uPD720202, PEX8605, iMX6Q and linux-4.19.25
On Thu, Aug 22, 2019 at 11:18:44AM +0200, Fawad Lateef wrote: > In all crash cases (when loading/unloading the driver); system stays > response, so I can look into any possible device status using PCIe > registers of PEX8605 switch or iMX6 PCIe root hub. Please suggest if > something I can check to better understand the issue. You could collect the output of "sudo lspci -vvxxx" and see if the switch or root port logged any errors. Collect it before and after the crash and compare the two. Bjorn
Re: AW: [Resend] [PATCH v3] usb: xhci-pci: reorder removal to avoid use-after-free
Hi, On 22-08-19 17:48, Schmid, Carsten wrote: On 22-08-19 17:23, Mathias Nyman wrote: On 16.8.2019 12.03, Schmid, Carsten wrote: On driver removal, the platform_device_unregister call attached through devm_add_action_or_reset was executed after usb_hcd_pci_remove. This lead to a use-after-free for the iomem resource of the xhci-ext-caps driver in the platform removal because the parent of the resource was freed earlier. Fix this by reordering of the removal sequence. Could all this be avoided if usb_hcd_pci_probe() used managed device resources as well? (using devm_request_mem_region(), and devm_ioremap_nocache()) This way the iomem resource would be added to the same devres list as the platform_unregister_call, and the iomem resource should be released after the platform_device_unregister as devres_release_all() releases the resources in reverse order. Yes I believe that that would work. I don't think so, because xhci_create_intel_xhci_sw_pdev registers it's resource through platform_device_add_resources which does not use devm_. The only thing what is done through devm in xhci_create_intel_xhci_sw_pdev is ret = devm_add_action_or_reset(...) Right, so if I understand correctly the problem with the current code (before your patch) is: Probe: 1. usb_hcd_pci_probe() uses request_mem_region() 2. xhci_create_intel_xhci_sw_pde uses platform_device_add_resources() which are a child of the previous region Remove: 3. usb_hcd_pci_remove does release_region() while the child region is still in use 4. At end of remove() devm code calls xhci_intel_unregister_pdev And the problem is we want to swap 3 and 4. Now if we make usb_hcd_pci_probe() use devm_request_mem_region and drop the cleanup from usb_hcd_pci_remove Probe: 1. usb_hcd_pci_probe() uses devm_request_mem_region(), this registers a release_region() with devm as cleanup 2. xhci_create_intel_xhci_sw_pde uses platform_device_add_resources() which are a child of the previous region and calls devm_add_action_or_reset() to add xhci_intel_unregister_pdev as cleanup Remove: 3. usb_hcd_pci_remove does nothing of relevance 4. At end of remove() devm code runs and calls the cleanups in reverse order of registration! so it calls: xhci_intel_unregister_pdev() release_region() Note the trick here is that the devm framework calls devm registered cleanup functions in reverse order of their registration, putting things in the right order. Regards, Hans
Re: [PATCH] HID: USB: Fix general protection fault caused by Logitech driver
On Thu, 22 Aug 2019, Andrey Konovalov wrote: > On Thu, Aug 22, 2019 at 7:11 PM Alan Stern wrote: > > > > On Thu, 22 Aug 2019, Andrey Konovalov wrote: > > > > > Hi Alan, > > > > > > I've ran the fuzzer with your patches applied overnight and noticed > > > another fallout of similar bugs. I think they are caused by a similar > > > issue in the sony HID driver. There's no hid_hw_stop() call in the "if > > > (!(hdev->claimed & HID_CLAIMED_INPUT))" case in sony_probe(). Does it > > > look like a bug to you? > > > > It looks like the relevant hid_hw_stop() call is the one at the end of > > sony_configure_input(). But I can't tell if doing that way is valid or > > not -- in practice the code would end up calling hid_disconnect() while > > hid_connect() was still running, which doesn't seem like a good idea. > > > > There's a comment about this near the end of sony_probe(). I suspect > > it would be better to call hid_hw_stop() in the conditional code > > following that comment rather than in sony_configure_input(). > > > > Either way, these are all things Jiri should know about or check up on. > > > > Have you gotten any test results from syzbot exercising these pathways? > > You ought to be able to tell which HID driver is involved by looking > > through the console output. > > Yes, a typical crash is below, that's why I thought it's the sony > driver. Adding hid_hw_stop() in sony_probe() stops the issue from > happening, but I don't know whether it's the right fix. Probably you have to add hid_hw_stop() in sony_probe() and remove it from sony_configure_input(). But like I said above, Jiri should look into this. Alan Stern
[PATCH v6] usbip: Implement SG support to vhci-hcd and stub driver
There are bugs on vhci with usb 3.0 storage device. In USB, each SG list entry buffer should be divisible by the bulk max packet size. But with native SG support, this problem doesn't matter because the SG buffer is treated as contiguous buffer. But without native SG support, USB storage driver breaks SG list into several URBs and the error occurs because of a buffer size of URB that cannot be divided by the bulk max packet size. The error situation is as follows. When USB Storage driver requests 31.5 KB data and has SG list which has 3584 bytes buffer followed by 7 4096 bytes buffer for some reason. USB Storage driver splits this SG list into several URBs because VHCI doesn't support SG and sends them separately. So the first URB buffer size is 3584 bytes. When receiving data from device, USB 3.0 device sends data packet of 1024 bytes size because the max packet size of BULK pipe is 1024 bytes. So device sends 4096 bytes. But the first URB buffer has only 3584 bytes buffer size. So host controller terminates the transfer even though there is more data to receive. So, vhci needs to support SG transfer to prevent this error. In this patch, vhci supports SG regardless of whether the server's host controller supports SG or not, because stub driver splits SG list into several URBs if the server's host controller doesn't support SG. To support SG, vhci sets URB_DMA_MAP_SG flag in urb->transfer_flags if URB has SG list and this flag will tell stub driver to use SG list. After receiving urb from stub driver, vhci clear URB_DMA_MAP_SG flag to avoid unnecessary dma unmapping in HCD. vhci sends each SG list entry to stub driver. Then, stub driver sees the total length of the buffer and allocates SG table and pages according to the total buffer length calling sgl_alloc(). After stub driver receives completed URB, it again sends each SG list entry to vhci. If the server's host controller doesn't support SG, stub driver breaks a single SG request into several URBs and submits them to the server's host controller. When all the split URBs are completed, stub driver reassembles the URBs into a single return command and sends it to vhci. Moreover, in the situation where vhci supports SG, but stub driver does not, or vice versa, usbip works normally. Because there is no protocol modification, there is no problem in communication between server and client even if the one has a kernel without SG support. In the case of vhci supports SG and stub driver doesn't, because vhci sends only the total length of the buffer to stub driver as it did before the patch applied, stub driver only needs to allocate the required length of buffers using only kmalloc() regardless of whether vhci supports SG or not. But stub driver has to allocate buffer with kmalloc() as much as the total length of SG buffer which is quite huge when vhci sends SG request, so it has overhead in buffer allocation in this situation. If stub driver needs to send data buffer to vhci because of IN pipe, stub driver also sends only total length of buffer as metadata and then sends real data as vhci does. Then vhci receive data from stub driver and store it to the corresponding buffer of SG list entry. And for the case of stub driver supports SG and vhci doesn't, since the USB storage driver checks that vhci doesn't support SG and sends the request to stub driver by splitting the SG list into multiple URBs, stub driver allocates a buffer for each URB with kmalloc() as it did before this patch. * Test environment Test uses two difference machines and two different kernel version to make mismatch situation between the client and the server where vhci supports SG, but stub driver does not, or vice versa. All tests are conducted in both full SG support that both vhci and stub support SG and half SG support that is the mismatch situation. - Test kernel version - 5.2-rc6 with SG support - 5.1.20-200.fc29.x86_64 without SG support * SG support test - Test devices - Super-speed storage device - SanDisk Ultra USB 3.0 - High-speed storage device - SMI corporation USB 2.0 flash drive - Test description Test read and write operation of mass storage device that uses the BULK transfer. In test, the client reads and writes files whose size is over 1G and it works normally. * Regression test - Test devices - Super-speed device - Logitech Brio webcam - High-speed device - Logitech C920 HD Pro webcam - Full-speed device - Logitech bluetooth mouse - Britz BR-Orion speaker - Low-speed device - Logitech wired mouse - Test description Moving and click test for mouse. To test the webcam, use gnome-cheese. To test the speaker, play music and video on the client. All works normally. * VUDC compatibility test VUDC also works well with this patch. Tests are done with two USB gadget created by CONFIGFS USB gadget. Both use the BULK pipe. 1. Serial gadget 2. Mass storage gadget - Serial gadget
Re: [PATCH v5 1/2] usbip: Skip DMA mapping and unmapping for urb at vhci
On Thu, Aug 22, 2019 at 09:40:11AM -0700, Greg KH wrote: > On Fri, Aug 09, 2019 at 12:54:34AM +0900, Suwan Kim wrote: > > vhci doesn’t do DMA for remote device. Actually, the real DMA > > operation is done by network card driver. vhci just passes virtual > > address of the buffer to the network stack, so vhci doesn’t use and > > need dma address of the buffer of the URB. > > > > But HCD provides DMA mapping and unmapping function by default. > > Moreover, it causes unnecessary DMA mapping and unmapping which > > will be done again at the NIC driver and it wastes CPU cycles. > > So, implement map_urb_for_dma and unmap_urb_for_dma function for > > vhci in order to skip the DMA mapping and unmapping procedure. > > > > When it comes to supporting SG for vhci, it is useful to use native > > SG list (urb->num_sgs) instead of mapped SG list because DMA mapping > > fnuction can adjust the number of SG list (urb->num_mapped_sgs). > > And vhci_map_urb_for_dma() prevents isoc pipe from using SG as > > hcd_map_urb_for_dma() does. > > > > Signed-off-by: Suwan Kim > > Can you please redo this patch based on my usb-next branch that has > Christoph's DMA changes in it? It should make your change much simpler > and smaller. > > Please do that and resend the whole series. I just sent v6 patch. And I discarded patch1 and patch1 is no longer needed because vhci doesn't set set HCD_DMA flag that is introduced by Christoph's patch. So I sent only patch 2 as v6. Regards Suwan Kim
Re: [PATCH] net: usb: Delete unnecessary checks before the macro call “dev_kfree_skb”
From: Markus Elfring Date: Wed, 21 Aug 2019 22:24:16 +0200 > From: Markus Elfring > Date: Wed, 21 Aug 2019 22:16:02 +0200 > > The dev_kfree_skb() function performs also input parameter validation. > Thus the test around the shown calls is not needed. > > This issue was detected by using the Coccinelle software. > > Signed-off-by: Markus Elfring Applied.
Re: f_mass_storage vs drivers/target
On Thu, 2019-08-22 at 13:30 -0400, Alan Stern wrote: > On Thu, 22 Aug 2019, Benjamin Herrenschmidt wrote: > > > On Thu, 2019-08-22 at 14:58 +1000, Benjamin Herrenschmidt wrote: > > > > > > Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging... > > > > > > While digging I found this gem: > > > > > > /* USB3380: use same address for usb and hardware endpoints */ > > > snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc), > > > usb_endpoint_dir_in(desc) ? "in" : "out"); > > > ep = gadget_find_ep_by_name(_gadget, name); > > > if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp)) > > > return ep; > > > > > > Any idea what's that supposed to achieve ? > > It looks like in one mode, the endpoint number has to be the value > predetermined by the hardware. In the other mode, any hardware > endpoint can be assigned any endpoint number. Sure but as I wrote, this is ep_match, which when called, always has usb_endpoint_num() set to 0... this function is supposed to chose the EP number afaik. So I don't think the above ever works, it just returns NULL. Or do we ever call this again with already predetermined EP nums, for example when doing multifunction ? > > > When ep_match is called, usb_endpoint_num() hasn't been set yet so > > > it's always 0 and always fails... or am I missing something ? > > > > Two problems: > > > > - net2280.c doesn't set a max EP size, so autoconfig fails since > > f_tcm specifies one. What about this ? > > > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -940,12 +940,14 @@ int usb_gadget_ep_match_desc(struct usb_gadget > > *gadget, > > if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out) > > return 0; > > > > - if (max > ep->maxpacket_limit) > > + if (ep->maxpacket_limit && max > ep->maxpacket_limit) > > return 0; > > > > (ie assume that ep->maxpacket_limit 0 means the UDC supports any > > legal size) > > That looks reasonable. I'll send a patch. > > - No UDC driver other than dummy sets max_streams, and f_tcm requires 4, > > so f_tcm will fail with *any* superspeed UDC driver as far as I can tell. > > > > Was it ever tested with USB 3 ? > > Note that USB 2 does not support streams at all. Yes, f_tcm only requires them for superspeed, but it does *require* them in that case. > > I'm not sure what the right fix here yet is as I yet have to learn about > > what those USB3 streams are :-) For now I've commented things out. > > They are for multiplexing multiple data streams over a single USB > endpoint. As far as I know, the only use case for such a thing is USB > Mass Storage. So f_tcm could operate in a degraded mode in the absence of streams easily, the problem is the mechanics of EP matching in epautoconf. It will just fail. I wonder since f_tcm is also the only user, whether we could change the matching logic to either: - Don't try to match, return streams is available. This could be problematic if the UDC supports streams on some EPs and not others however. - Do two passes: one pass trying to match the streams, and one patch without matching them if the first one fails. Then f_tcm could check whether it got EPs with streams and enable stream usage accordingly. Opinions ? Other option ? Cheers, Ben.
Re: [PATCH 1/1] usb: xhci: avoid VBus glitch during controller reset operation
On 19-08-22 14:08:26, Mathias Nyman wrote:
> On 21.8.2019 6.18, Peter Chen wrote:
> > According to xHCI 1.1 CH4.19.4 Port Power:
> > While Chip Hardware Reset or HCRST is asserted,
> > the value of PP is undefined. If the xHC supports
> > power switches (PPC = ‘1’) then VBus may be deasserted
> > during this time. PP (and VBus) shall be enabled immediately
> > upon exiting the reset condition.
> >
> > The VBus glitch may cause some USB devices work abnormal, we observe
> > it at NXP LS1012AFWRY/LS1043ARDB/LX2160AQDS/LS1088ARDB platforms. To
> > avoid this Vbus glitch, we could set PP as 0 before HCRST, and the PP
> > will back to 1 after HCRST according to spec.
>
> Is this glitch causing issues already at the first xHC reset when we are
> loading the xhci driver, or only later resets, like xHC reset at resume?
The first time, Ran would you please confirm?
>
> >
> > Reported-by: Ran Wang
> > Signed-off-by: Peter Chen
> > ---
> > drivers/usb/host/xhci.c | 15 ++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 6b34a573c3d9..f5a7b5d63031 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -167,7 +167,8 @@ int xhci_reset(struct xhci_hcd *xhci)
> > {
> > u32 command;
> > u32 state;
> > - int ret;
> > + int ret, i;
> > + u32 portsc;
> > state = readl(&xhci->op_regs->status);
> > @@ -181,6 +182,18 @@ int xhci_reset(struct xhci_hcd *xhci)
> > return 0;
> > }
> > + /*
> > +* Keep PORTSC.PP as 0 before HCRST to eliminate
> > +* Vbus glitch, see CH 4.19.4.
> > +*/
> > + for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
> > + __le32 __iomem *port_addr = &xhci->op_regs->port_status_base +
> > + NUM_PORT_REGS * i;
> > + portsc = readl(port_addr);
> > + portsc &= ~PORT_POWER;
> > + writel(portsc, port_addr);
>
> Not all bits read from PORTSC should be written back, you might need
> portsc = xhci_port_state_to_neutral(portsc) here.
Will change.
>
> Normally I'd recommend using the xhci_set_port_power() helper instead, but
> if this is is needed at driver load time then port arrays may not be set up
> yet.
> xhci_set_port_power() would take care of possible ACPI method calls, and add
> some debugging.
>
It is needed at load time, so I did not use port array.
> Not sure if this will impact some other platforms, maybe it would be better
> to move this to
> a separate function, and call it before xhci_reset() if a quirk is set.
>
It follows spec, not at quirk. We talked with Synopsis engineer
(case: 8001235479), they confirmed this behaviour follows spec.
Besides, I tried at both dwc3 and cadence3 xHCI platforms,
the PORT_POWER will be set again after controller set.
What's potential issue you consider?
Thanks,
Peter
RE: [PATCH 1/1] usb: xhci: avoid VBus glitch during controller reset operation
HI Peter, Mathias,
Sorry for the late review.
On Friday, August 23, 2019 09:02, Peter Chen wrote:
>
> On 19-08-22 14:08:26, Mathias Nyman wrote:
> > On 21.8.2019 6.18, Peter Chen wrote:
> > > According to xHCI 1.1 CH4.19.4 Port Power:
> > > While Chip Hardware Reset or HCRST is asserted,
> > > the value of PP is undefined. If the xHC supports
> > > power switches (PPC = ‘1’) then VBus may be deasserted
> > > during this time. PP (and VBus) shall be enabled immediately
> > > upon exiting the reset condition.
> > >
> > > The VBus glitch may cause some USB devices work abnormal, we observe
> > > it at NXP LS1012AFWRY/LS1043ARDB/LX2160AQDS/LS1088ARDB platforms.
> To
> > > avoid this Vbus glitch, we could set PP as 0 before HCRST, and the
> > > PP will back to 1 after HCRST according to spec.
> >
> > Is this glitch causing issues already at the first xHC reset when we
> > are loading the xhci driver, or only later resets, like xHC reset at
> > resume?
>
> The first time, Ran would you please confirm?
My understand is this will happened whenever PP is set to 0, such as xHCI reset.
So I think it might also be observed during resume if xHC do reset.
However, according to my previous testing, it might be too late to
do this port power off in xhci_reset(), actually the VBUS will assert once DWC3
driver
set IP to host mode (before doing xHC reset). So the glitch still can be
observed on the scope;
I have more issue description in another patch discussion about this, please see
lore.kernel.org/patchwork/patch/1032425/
Quoted from it:
Actually I have done experiment like what you suggested (in xhci-plat.c), but
the timing
seems too late--making VBUS waveform look like a square wave as below:
Here DWC3 enable host mode, VBUS on
|
+5V /-\ 40ms /---
0V / 90ms \__/
| |
| Here do xhci reset, VBUS back to +5V again
Here set all PORTSC[PP] to 0 in xhci_gen_setup()
So I am afraid the solution might have to be added in DWC3 core driver where
just after host mode enabling code if want fix this :(
Regards,
Ran
> >
> > >
> > > Reported-by: Ran Wang
> > > Signed-off-by: Peter Chen
> > > ---
> > > drivers/usb/host/xhci.c | 15 ++-
> > > 1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
> > > 6b34a573c3d9..f5a7b5d63031 100644
> > > --- a/drivers/usb/host/xhci.c
> > > +++ b/drivers/usb/host/xhci.c
> > > @@ -167,7 +167,8 @@ int xhci_reset(struct xhci_hcd *xhci)
> > > {
> > > u32 command;
> > > u32 state;
> > > - int ret;
> > > + int ret, i;
> > > + u32 portsc;
> > > state = readl(&xhci->op_regs->status); @@ -181,6 +182,18 @@ int
> > > xhci_reset(struct xhci_hcd *xhci)
> > > return 0;
> > > }
> > > + /*
> > > + * Keep PORTSC.PP as 0 before HCRST to eliminate
> > > + * Vbus glitch, see CH 4.19.4.
> > > + */
> > > + for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
> > > + __le32 __iomem *port_addr = &xhci->op_regs-
> >port_status_base +
> > > + NUM_PORT_REGS * i;
> > > + portsc = readl(port_addr);
> > > + portsc &= ~PORT_POWER;
> > > + writel(portsc, port_addr);
> >
> > Not all bits read from PORTSC should be written back, you might need
> > portsc = xhci_port_state_to_neutral(portsc) here.
>
> Will change.
>
> >
> > Normally I'd recommend using the xhci_set_port_power() helper instead,
> > but if this is is needed at driver load time then port arrays may not be
> > set up
> yet.
> > xhci_set_port_power() would take care of possible ACPI method calls, and add
> some debugging.
> >
>
> It is needed at load time, so I did not use port array.
>
> > Not sure if this will impact some other platforms, maybe it would be
> > better to move this to a separate function, and call it before xhci_reset()
> > if a
> quirk is set.
> >
>
> It follows spec, not at quirk. We talked with Synopsis engineer
> (case: 8001235479), they confirmed this behaviour follows spec.
> Besides, I tried at both dwc3 and cadence3 xHCI platforms, the PORT_POWER
> will be set again after controller set.
>
> What's potential issue you consider?
>
> Thanks,
> Peter
[PATCH 1/2] usb: gadget: net2280: Move all "ll" registers in one structure
The split into multiple structures of the "ll" register bank is
impractical. It makes it hard to add ll_lfps_timers_2 which is
at offset 0x794, which is outside of the existing "lfps" structure
and would require us to add yet another one.
Instead, move all the "ll" registers into a single usb338x_ll_regs
structure, and add ll_lfps_timers_2 while at it. It will be used
in a subsequent patch.
Signed-off-by: Benjamin Herrenschmidt
---
drivers/usb/gadget/udc/net2280.c | 28 ++---
drivers/usb/gadget/udc/net2280.h | 3 ---
include/linux/usb/usb338x.h | 35 +++-
3 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index b6bbe2e448ba..e0191146ba22 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -2244,30 +2244,30 @@ static void usb_reinit_338x(struct net2280 *dev)
}
/* Hardware Defect and Workaround */
- val = readl(&dev->ll_lfps_regs->ll_lfps_5);
+ val = readl(&dev->llregs->ll_lfps_5);
val &= ~(0xf << TIMER_LFPS_6US);
val |= 0x5 << TIMER_LFPS_6US;
- writel(val, &dev->ll_lfps_regs->ll_lfps_5);
+ writel(val, &dev->llregs->ll_lfps_5);
- val = readl(&dev->ll_lfps_regs->ll_lfps_6);
+ val = readl(&dev->llregs->ll_lfps_6);
val &= ~(0x << TIMER_LFPS_80US);
val |= 0x0100 << TIMER_LFPS_80US;
- writel(val, &dev->ll_lfps_regs->ll_lfps_6);
+ writel(val, &dev->llregs->ll_lfps_6);
/*
* AA_AB Errata. Issue 4. Workaround for SuperSpeed USB
* Hot Reset Exit Handshake may Fail in Specific Case using
* Default Register Settings. Workaround for Enumeration test.
*/
- val = readl(&dev->ll_tsn_regs->ll_tsn_counters_2);
+ val = readl(&dev->llregs->ll_tsn_counters_2);
val &= ~(0x1f << HOT_TX_NORESET_TS2);
val |= 0x10 << HOT_TX_NORESET_TS2;
- writel(val, &dev->ll_tsn_regs->ll_tsn_counters_2);
+ writel(val, &dev->llregs->ll_tsn_counters_2);
- val = readl(&dev->ll_tsn_regs->ll_tsn_counters_3);
+ val = readl(&dev->llregs->ll_tsn_counters_3);
val &= ~(0x1f << HOT_RX_RESET_TS2);
val |= 0x3 << HOT_RX_RESET_TS2;
- writel(val, &dev->ll_tsn_regs->ll_tsn_counters_3);
+ writel(val, &dev->llregs->ll_tsn_counters_3);
/*
* Set Recovery Idle to Recover bit:
@@ -2276,10 +2276,10 @@ static void usb_reinit_338x(struct net2280 *dev)
* - It is safe to set for all connection speeds; all chip revisions.
* - R-M-W to leave other bits undisturbed.
* - Reference PLX TT-7372
-*/
- val = readl(&dev->ll_chicken_reg->ll_tsn_chicken_bit);
+ */
+ val = readl(&dev->llregs->ll_tsn_chicken_bit);
val |= BIT(RECOVERY_IDLE_TO_RECOVER_FMW);
- writel(val, &dev->ll_chicken_reg->ll_tsn_chicken_bit);
+ writel(val, &dev->llregs->ll_tsn_chicken_bit);
INIT_LIST_HEAD(&dev->gadget.ep0->ep_list);
@@ -3669,12 +3669,6 @@ static int net2280_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
(base + 0x00b4);
dev->llregs = (struct usb338x_ll_regs __iomem *)
(base + 0x0700);
- dev->ll_lfps_regs = (struct usb338x_ll_lfps_regs __iomem *)
- (base + 0x0748);
- dev->ll_tsn_regs = (struct usb338x_ll_tsn_regs __iomem *)
- (base + 0x077c);
- dev->ll_chicken_reg = (struct usb338x_ll_chi_regs __iomem *)
- (base + 0x079c);
dev->plregs = (struct usb338x_pl_regs __iomem *)
(base + 0x0800);
usbstat = readl(&dev->usb->usbstat);
diff --git a/drivers/usb/gadget/udc/net2280.h b/drivers/usb/gadget/udc/net2280.h
index b65a797544d7..85d3ca1698ba 100644
--- a/drivers/usb/gadget/udc/net2280.h
+++ b/drivers/usb/gadget/udc/net2280.h
@@ -178,9 +178,6 @@ struct net2280 {
struct net2280_dep_regs __iomem *dep;
struct net2280_ep_regs __iomem *epregs;
struct usb338x_ll_regs __iomem *llregs;
- struct usb338x_ll_lfps_regs __iomem *ll_lfps_regs;
- struct usb338x_ll_tsn_regs __iomem *ll_tsn_regs;
- struct usb338x_ll_chi_regs __iomem *ll_chicken_reg;
struct usb338x_pl_regs __iomem *plregs;
struct dma_pool *requests;
diff --git a/include/linux/usb/usb338x.h b/include/linux/usb/usb338x.h
index 7189e3387bf9..20020c1336d5 100644
--- a/include/linux/usb/usb338x.h
+++ b/include/linux/usb/usb338x.h
@@ -113,7 +113,10 @@ struct usb338x_ll_regs {
u32 ll_ltssm_ctrl1;
u32
[PATCH 1/2] usb: gadget: net2280: Move all "ll" registers in one structure
The split into multiple structures of the "ll" register bank is
impractical. It makes it hard to add ll_lfps_timers_2 which is
at offset 0x794, which is outside of the existing "lfps" structure
and would require us to add yet another one.
Instead, move all the "ll" registers into a single usb338x_ll_regs
structure, and add ll_lfps_timers_2 while at it. It will be used
in a subsequent patch.
Signed-off-by: Benjamin Herrenschmidt
---
drivers/usb/gadget/udc/net2280.c | 28 ++---
drivers/usb/gadget/udc/net2280.h | 3 ---
include/linux/usb/usb338x.h | 35 +++-
3 files changed, 36 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c
index b6bbe2e448ba..e0191146ba22 100644
--- a/drivers/usb/gadget/udc/net2280.c
+++ b/drivers/usb/gadget/udc/net2280.c
@@ -2244,30 +2244,30 @@ static void usb_reinit_338x(struct net2280 *dev)
}
/* Hardware Defect and Workaround */
- val = readl(&dev->ll_lfps_regs->ll_lfps_5);
+ val = readl(&dev->llregs->ll_lfps_5);
val &= ~(0xf << TIMER_LFPS_6US);
val |= 0x5 << TIMER_LFPS_6US;
- writel(val, &dev->ll_lfps_regs->ll_lfps_5);
+ writel(val, &dev->llregs->ll_lfps_5);
- val = readl(&dev->ll_lfps_regs->ll_lfps_6);
+ val = readl(&dev->llregs->ll_lfps_6);
val &= ~(0x << TIMER_LFPS_80US);
val |= 0x0100 << TIMER_LFPS_80US;
- writel(val, &dev->ll_lfps_regs->ll_lfps_6);
+ writel(val, &dev->llregs->ll_lfps_6);
/*
* AA_AB Errata. Issue 4. Workaround for SuperSpeed USB
* Hot Reset Exit Handshake may Fail in Specific Case using
* Default Register Settings. Workaround for Enumeration test.
*/
- val = readl(&dev->ll_tsn_regs->ll_tsn_counters_2);
+ val = readl(&dev->llregs->ll_tsn_counters_2);
val &= ~(0x1f << HOT_TX_NORESET_TS2);
val |= 0x10 << HOT_TX_NORESET_TS2;
- writel(val, &dev->ll_tsn_regs->ll_tsn_counters_2);
+ writel(val, &dev->llregs->ll_tsn_counters_2);
- val = readl(&dev->ll_tsn_regs->ll_tsn_counters_3);
+ val = readl(&dev->llregs->ll_tsn_counters_3);
val &= ~(0x1f << HOT_RX_RESET_TS2);
val |= 0x3 << HOT_RX_RESET_TS2;
- writel(val, &dev->ll_tsn_regs->ll_tsn_counters_3);
+ writel(val, &dev->llregs->ll_tsn_counters_3);
/*
* Set Recovery Idle to Recover bit:
@@ -2276,10 +2276,10 @@ static void usb_reinit_338x(struct net2280 *dev)
* - It is safe to set for all connection speeds; all chip revisions.
* - R-M-W to leave other bits undisturbed.
* - Reference PLX TT-7372
-*/
- val = readl(&dev->ll_chicken_reg->ll_tsn_chicken_bit);
+ */
+ val = readl(&dev->llregs->ll_tsn_chicken_bit);
val |= BIT(RECOVERY_IDLE_TO_RECOVER_FMW);
- writel(val, &dev->ll_chicken_reg->ll_tsn_chicken_bit);
+ writel(val, &dev->llregs->ll_tsn_chicken_bit);
INIT_LIST_HEAD(&dev->gadget.ep0->ep_list);
@@ -3669,12 +3669,6 @@ static int net2280_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
(base + 0x00b4);
dev->llregs = (struct usb338x_ll_regs __iomem *)
(base + 0x0700);
- dev->ll_lfps_regs = (struct usb338x_ll_lfps_regs __iomem *)
- (base + 0x0748);
- dev->ll_tsn_regs = (struct usb338x_ll_tsn_regs __iomem *)
- (base + 0x077c);
- dev->ll_chicken_reg = (struct usb338x_ll_chi_regs __iomem *)
- (base + 0x079c);
dev->plregs = (struct usb338x_pl_regs __iomem *)
(base + 0x0800);
usbstat = readl(&dev->usb->usbstat);
diff --git a/drivers/usb/gadget/udc/net2280.h b/drivers/usb/gadget/udc/net2280.h
index b65a797544d7..85d3ca1698ba 100644
--- a/drivers/usb/gadget/udc/net2280.h
+++ b/drivers/usb/gadget/udc/net2280.h
@@ -178,9 +178,6 @@ struct net2280 {
struct net2280_dep_regs __iomem *dep;
struct net2280_ep_regs __iomem *epregs;
struct usb338x_ll_regs __iomem *llregs;
- struct usb338x_ll_lfps_regs __iomem *ll_lfps_regs;
- struct usb338x_ll_tsn_regs __iomem *ll_tsn_regs;
- struct usb338x_ll_chi_regs __iomem *ll_chicken_reg;
struct usb338x_pl_regs __iomem *plregs;
struct dma_pool *requests;
diff --git a/include/linux/usb/usb338x.h b/include/linux/usb/usb338x.h
index 7189e3387bf9..20020c1336d5 100644
--- a/include/linux/usb/usb338x.h
+++ b/include/linux/usb/usb338x.h
@@ -113,7 +113,10 @@ struct usb338x_ll_regs {
u32 ll_ltssm_ctrl1;
u32
[PATCH 2/2] usb: gadget: net2280: Add workaround for AB chip Errata 11
The errata description is: Workaround for Default Duration of LFPS Handshake Signaling for Device-Initiated U1 Exit is too short. The default duration of the LFPS handshake generated by USB3380 for a device-initiated U1-exit may not be long enough for certain SuperSpeed downstream ports (SuperSpeed hubs/hosts) to recognize. This could lead to USB3380 entering the recovery state pre-maturely and ending up in the SS.Inactive state. I have observed various enumeration failures, seemingly related to lost transactions or SETUP status phases on modern hosts (typically thunderbolt capable systems) without this workaround. Signed-off-by: Benjamin Herrenschmidt --- drivers/usb/gadget/udc/net2280.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index e0191146ba22..51efee21915f 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -2269,6 +2269,16 @@ static void usb_reinit_338x(struct net2280 *dev) val |= 0x3 << HOT_RX_RESET_TS2; writel(val, &dev->llregs->ll_tsn_counters_3); + /* +* AB errata. Errata 11. Workaround for Default Duration of LFPS +* Handshake Signaling for Device-Initiated U1 Exit is too short. +* Without this, various enumeration failures observed with +* modern superspeed hosts. +*/ + val = readl(&dev->llregs->ll_lfps_timers_2); + writel((val & 0x) | LFPS_TIMERS_2_WORKAROUND_VALUE, + &dev->llregs->ll_lfps_timers_2); + /* * Set Recovery Idle to Recover bit: * - On SS connections, setting Recovery Idle to Recover Fmw improves
Re: [PATCH 1/1] usb: xhci: avoid VBus glitch during controller reset operation
On 19-08-23 01:59:24, Ran Wang wrote:
> HI Peter, Mathias,
>
> Sorry for the late review.
>
> On Friday, August 23, 2019 09:02, Peter Chen wrote:
> >
> > On 19-08-22 14:08:26, Mathias Nyman wrote:
> > > On 21.8.2019 6.18, Peter Chen wrote:
> > > > According to xHCI 1.1 CH4.19.4 Port Power:
> > > > While Chip Hardware Reset or HCRST is asserted,
> > > > the value of PP is undefined. If the xHC supports
> > > > power switches (PPC = ‘1’) then VBus may be deasserted
> > > > during this time. PP (and VBus) shall be enabled
> > > > immediately
> > > > upon exiting the reset condition.
> > > >
> > > > The VBus glitch may cause some USB devices work abnormal, we observe
> > > > it at NXP LS1012AFWRY/LS1043ARDB/LX2160AQDS/LS1088ARDB platforms.
> > To
> > > > avoid this Vbus glitch, we could set PP as 0 before HCRST, and the
> > > > PP will back to 1 after HCRST according to spec.
> > >
> > > Is this glitch causing issues already at the first xHC reset when we
> > > are loading the xhci driver, or only later resets, like xHC reset at
> > > resume?
> >
> > The first time, Ran would you please confirm?
>
> My understand is this will happened whenever PP is set to 0, such as xHCI
> reset.
> So I think it might also be observed during resume if xHC do reset.
Yes, Vbus glitch should exists whenever we do controller reset, I thought
you only meet this issue during boots up.
>
> However, according to my previous testing, it might be too late to
> do this port power off in xhci_reset(), actually the VBUS will assert once
> DWC3 driver
> set IP to host mode (before doing xHC reset). So the glitch still can be
> observed on the scope;
> I have more issue description in another patch discussion about this, please
> see
>
> lore.kernel.org/patchwork/patch/1032425/
> Quoted from it:
> Actually I have done experiment like what you suggested (in xhci-plat.c), but
> the timing
> seems too late--making VBUS waveform look like a square wave as below:
>
> Here DWC3 enable host mode, VBUS on
> |
> +5V /-\ 40ms /---
> 0V / 90ms \__/
>| |
>| Here do xhci reset, VBUS back to +5V again
>Here set all PORTSC[PP] to 0 in xhci_gen_setup()
>
> So I am afraid the solution might have to be added in DWC3 core driver where
> just after host mode enabling code if want fix this :(
>
Per spec 4.19.4 Port Power:
Whether an xHC implementation supports port power switches or not,
it shall automatically enable VBus on all Root Hub ports after a
Chip Hardware Reset or HCRST.
Ran's observation confirmed it, PP is asserted once xHCI is enabled.
From the code, the HCRST will be at boots up and system resume.
So, it seems we can't keep PP always asserted. For xHCI, to avoid
Vbus glitch totally, we may have to control Vbus without PP
(eg, configure PP pin as GPIO).
Peter
>
> Regards,
> Ran
> > >
> > > >
> > > > Reported-by: Ran Wang
> > > > Signed-off-by: Peter Chen
> > > > ---
> > > > drivers/usb/host/xhci.c | 15 ++-
> > > > 1 file changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index
> > > > 6b34a573c3d9..f5a7b5d63031 100644
> > > > --- a/drivers/usb/host/xhci.c
> > > > +++ b/drivers/usb/host/xhci.c
> > > > @@ -167,7 +167,8 @@ int xhci_reset(struct xhci_hcd *xhci)
> > > > {
> > > > u32 command;
> > > > u32 state;
> > > > - int ret;
> > > > + int ret, i;
> > > > + u32 portsc;
> > > > state = readl(&xhci->op_regs->status); @@ -181,6 +182,18 @@ int
> > > > xhci_reset(struct xhci_hcd *xhci)
> > > > return 0;
> > > > }
> > > > + /*
> > > > +* Keep PORTSC.PP as 0 before HCRST to eliminate
> > > > +* Vbus glitch, see CH 4.19.4.
> > > > +*/
> > > > + for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
> > > > + __le32 __iomem *port_addr = &xhci->op_regs-
> > >port_status_base +
> > > > + NUM_PORT_REGS * i;
> > > > + portsc = readl(port_addr);
> > > > + portsc &= ~PORT_POWER;
> > > > + writel(portsc, port_addr);
> > >
> > > Not all bits read from PORTSC should be written back, you might need
> > > portsc = xhci_port_state_to_neutral(portsc) here.
> >
> > Will change.
> >
> > >
> > > Normally I'd recommend using the xhci_set_port_power() helper instead,
> > > but if this is is needed at driver load time then port arrays may not be
> > > set up
> > yet.
> > > xhci_set_port_power() would take care of possible ACPI method calls, and
> > > add
> > some debugging.
> > >
> >
> > It is needed at load time, so I did not use port array.
> >
> > > Not sure if this will impact some other platforms, maybe it would b
RE: [PATCH 1/1] usb: xhci: avoid VBus glitch during controller reset operation
Hi Peter,
On Friday, August 23, 2019 11:34, Peter Chen wrote:
>
> On 19-08-23 01:59:24, Ran Wang wrote:
> > HI Peter, Mathias,
> >
> > Sorry for the late review.
> >
> > On Friday, August 23, 2019 09:02, Peter Chen wrote:
> > >
> > > On 19-08-22 14:08:26, Mathias Nyman wrote:
> > > > On 21.8.2019 6.18, Peter Chen wrote:
> > > > > According to xHCI 1.1 CH4.19.4 Port Power:
> > > > > While Chip Hardware Reset or HCRST is asserted,
> > > > > the value of PP is undefined. If the xHC supports
> > > > > power switches (PPC = ‘1’) then VBus may be deasserted
> > > > > during this time. PP (and VBus) shall be enabled
> > > > > immediately
> > > > > upon exiting the reset condition.
> > > > >
> > > > > The VBus glitch may cause some USB devices work abnormal, we
> > > > > observe it at NXP LS1012AFWRY/LS1043ARDB/LX2160AQDS/LS1088ARDB
> platforms.
> > > To
> > > > > avoid this Vbus glitch, we could set PP as 0 before HCRST, and
> > > > > the PP will back to 1 after HCRST according to spec.
> > > >
> > > > Is this glitch causing issues already at the first xHC reset when
> > > > we are loading the xhci driver, or only later resets, like xHC reset at
> resume?
> > >
> > > The first time, Ran would you please confirm?
> >
> > My understand is this will happened whenever PP is set to 0, such as xHCI
> > reset.
> > So I think it might also be observed during resume if xHC do reset.
>
> Yes, Vbus glitch should exists whenever we do controller reset, I thought you
> only meet this issue during boots up.
>
> >
> > However, according to my previous testing, it might be too late to do
> > this port power off in xhci_reset(), actually the VBUS will assert
> > once DWC3 driver set IP to host mode (before doing xHC reset). So the
> > glitch still can be observed on the scope; I have more issue
> > description in another patch discussion about this, please see
> >
> > lore.kernel.org/patchwork/patch/1032425/
> > Quoted from it:
> > Actually I have done experiment like what you suggested (in
> > xhci-plat.c), but the timing seems too late--making VBUS waveform look like
> > a
> square wave as below:
> >
> > Here DWC3 enable host mode, VBUS on
> > |
> > +5V /-\ 40ms /---
> > 0V / 90ms \__/
> >| |
> >| Here do xhci reset, VBUS back to +5V again
> >Here set all PORTSC[PP] to 0 in
> > xhci_gen_setup()
> >
> > So I am afraid the solution might have to be added in DWC3 core driver
> > where just after host mode enabling code if want fix this :(
> >
>
> Per spec 4.19.4 Port Power:
>
> Whether an xHC implementation supports port power switches or not, it shall
> automatically enable VBus on all Root Hub ports after a Chip Hardware Reset or
> HCRST.
>
> Ran's observation confirmed it, PP is asserted once xHCI is enabled.
> From the code, the HCRST will be at boots up and system resume.
> So, it seems we can't keep PP always asserted. For xHCI, to avoid Vbus glitch
> totally, we may have to control Vbus without PP (eg, configure PP pin as
> GPIO).
Yes, another option is to design a better VBUS driving circuit on HW side to
filter this glitch out.
I have found some earlier version of LS1043ARDB board design is perfect on
doing this.
Anyway, for boot, my suggestion is to do it in DWC3 driver once after enabling
host mode.
But that solution is ugly, I have to admit.
And for resume, frankly I didn’t notice this before (thanks for remind), would
do further survey
if can found a similar solution.
Regards,
Ran
> > > >
> > > > >
> > > > > Reported-by: Ran Wang
> > > > > Signed-off-by: Peter Chen
> > > > > ---
> > > > > drivers/usb/host/xhci.c | 15 ++-
> > > > > 1 file changed, 14 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > > > > index
> > > > > 6b34a573c3d9..f5a7b5d63031 100644
> > > > > --- a/drivers/usb/host/xhci.c
> > > > > +++ b/drivers/usb/host/xhci.c
> > > > > @@ -167,7 +167,8 @@ int xhci_reset(struct xhci_hcd *xhci)
> > > > > {
> > > > > u32 command;
> > > > > u32 state;
> > > > > - int ret;
> > > > > + int ret, i;
> > > > > + u32 portsc;
> > > > > state = readl(&xhci->op_regs->status); @@ -181,6 +182,18 @@
> > > > > int xhci_reset(struct xhci_hcd *xhci)
> > > > > return 0;
> > > > > }
> > > > > + /*
> > > > > + * Keep PORTSC.PP as 0 before HCRST to eliminate
> > > > > + * Vbus glitch, see CH 4.19.4.
> > > > > + */
> > > > > + for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
> > > > > + __le32 __iomem *port_addr = &xhci->op_regs-
> > > >port_status_base +
> > > > > + NUM_PORT_REGS * i;
> > > > > + portsc = readl(port_addr);
> > > > > + portsc &= ~PORT_POWER;
[RESEND PATCH v2 2/2] usb: xhci-mtk: add an optional xhci_ck clock
Some SoCs may have an optional clock xhci_ck (125M or 200M), it
usually uses the same PLL as sys_ck, so support it.
Signed-off-by: Chunfeng Yun
---
v2 no changes
---
drivers/usb/host/xhci-mtk.c | 13 +
drivers/usb/host/xhci-mtk.h | 1 +
2 files changed, 14 insertions(+)
diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 026fe18972d3..b18a6baef204 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -216,6 +216,10 @@ static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
return PTR_ERR(mtk->sys_clk);
}
+ mtk->xhci_clk = devm_clk_get_optional(dev, "xhci_ck");
+ if (IS_ERR(mtk->xhci_clk))
+ return PTR_ERR(mtk->xhci_clk);
+
mtk->ref_clk = devm_clk_get_optional(dev, "ref_ck");
if (IS_ERR(mtk->ref_clk))
return PTR_ERR(mtk->ref_clk);
@@ -244,6 +248,12 @@ static int xhci_mtk_clks_enable(struct xhci_hcd_mtk *mtk)
goto sys_clk_err;
}
+ ret = clk_prepare_enable(mtk->xhci_clk);
+ if (ret) {
+ dev_err(mtk->dev, "failed to enable xhci_clk\n");
+ goto xhci_clk_err;
+ }
+
ret = clk_prepare_enable(mtk->mcu_clk);
if (ret) {
dev_err(mtk->dev, "failed to enable mcu_clk\n");
@@ -261,6 +271,8 @@ static int xhci_mtk_clks_enable(struct xhci_hcd_mtk *mtk)
dma_clk_err:
clk_disable_unprepare(mtk->mcu_clk);
mcu_clk_err:
+ clk_disable_unprepare(mtk->xhci_clk);
+xhci_clk_err:
clk_disable_unprepare(mtk->sys_clk);
sys_clk_err:
clk_disable_unprepare(mtk->ref_clk);
@@ -272,6 +284,7 @@ static void xhci_mtk_clks_disable(struct xhci_hcd_mtk *mtk)
{
clk_disable_unprepare(mtk->dma_clk);
clk_disable_unprepare(mtk->mcu_clk);
+ clk_disable_unprepare(mtk->xhci_clk);
clk_disable_unprepare(mtk->sys_clk);
clk_disable_unprepare(mtk->ref_clk);
}
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index 8be8c5f7ff62..5ac458b7d2e0 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -139,6 +139,7 @@ struct xhci_hcd_mtk {
struct regulator *vusb33;
struct regulator *vbus;
struct clk *sys_clk;/* sys and mac clock */
+ struct clk *xhci_clk;
struct clk *ref_clk;
struct clk *mcu_clk;
struct clk *dma_clk;
--
2.23.0
[RESEND PATCH v2 1/2] dt-bindings: usb: mtk-xhci: add an optional xhci_ck clock
Add a new optional clock xhci_ck Signed-off-by: Chunfeng Yun --- v2 changes: 1. add the new clock at the end, suggested by Rob --- Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt index 266c2d917a28..f3e4acecabe8 100644 --- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt +++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.txt @@ -30,7 +30,8 @@ Required properties: the following ones are optional: "ref_ck": reference clock used by low power mode etc, "mcu_ck": mcu_bus clock for register access, - "dma_ck": dma_bus clock for data transfer by DMA + "dma_ck": dma_bus clock for data transfer by DMA, + "xhci_ck": controller clock - phys : see usb-hcd.txt in the current directory @@ -100,7 +101,7 @@ Required properties: - clocks : a list of phandle + clock-specifier pairs, one for each entry in clock-names - clock-names : must contain "sys_ck", and the following ones are optional: - "ref_ck", "mcu_ck" and "dma_ck" + "ref_ck", "mcu_ck" and "dma_ck", "xhci_ck" Optional properties: - vbus-supply : reference to the VBUS regulator; -- 2.23.0
