Re: [PATCH v2 2/4] Documentation: usb: gadget-testing: add description for depth of queue
On Thu, Nov 19, 2015 at 12:24:28PM -0600, Felipe Balbi wrote: > > Hi, > > Peter Chen writes: > > Add both bulk and iso depth of queue for sourcesink. > > > > Signed-off-by: Peter Chen > > --- > > Documentation/usb/gadget-testing.txt | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/Documentation/usb/gadget-testing.txt > > b/Documentation/usb/gadget-testing.txt > > index b24d3ef..84b3d10 100644 > > --- a/Documentation/usb/gadget-testing.txt > > +++ b/Documentation/usb/gadget-testing.txt > > @@ -579,6 +579,8 @@ The SOURCESINK function provides these attributes in > > its function directory: > > isoc_mult - 0..2 (hs/ss only) > > isoc_maxburst - 0..15 (ss only) > > bulk_buflen - buffer length > > + bulk_qlen - depth of queue for bulk > > + iso_qlen- depth of queue for iso > > doesn't apply for me: > > Applying: Documentation: usb: gadget-testing: add description for depth of > queue > error: patch failed: Documentation/usb/gadget-testing.txt:579 > error: Documentation/usb/gadget-testing.txt: patch does not apply > Patch failed at 0001 Documentation: usb: gadget-testing: add description for > depth of queue > The copy of the patch that failed is found in: > workspace/linux/.git/rebase-apply/patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > Care to rebase on my testing/next ? > > Note that patch 1 is already applied. When rebasing, please collect > Krzysztof's Reviewed-by ;-) > I find the first three has already been applied, thanks for doing that. But the patch 4/4 has not applied, do you need me to re-send it after adding Krzysztof's Reviewed-by? -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: usbhid: add Logitech G710+ keyboard quirk NOGET
On Thu, Nov 19, 2015 at 6:26 PM, Greg KH wrote: > On Tue, Nov 17, 2015 at 12:40:12AM -0600, Jimmy Berry wrote: >> Without quirk keyboard repeats '6' until volume control is used since it >> indicates the key is pressed without ever releasing. >> >> Signed-off-by: Jimmy Berry >> --- >> drivers/hid/hid-ids.h | 1 + >> drivers/hid/usbhid/hid-quirks.c | 1 + >> 2 files changed, 2 insertions(+) > > Please use scripts/get_maintainer.pl to find the right people to cc: and > the correct mailing list for your patch (hint, it's not me...) Thats exactly what I did. Perhaps the output needs to be tweaked. I debated sending to linux-input, but the description seemed to fit too well. --- Jiri Kosina (maintainer:HID CORE LAYER) linux-in...@vger.kernel.org (open list:HID CORE LAYER) linux-ker...@vger.kernel.org (open list) linux-usb@vger.kernel.org (open list:USB HID/HIDBP DRIVERS (USB KEYBOARDS, MICE, REM...) --- Note the "USB HID" and "USB KEYBOARDS" sections. That happens to be exactly what this patch is against. Either way, I'll assume it is instead linux-input to which you wish me to send the patch. > > thanks, > > greg k-h > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] usb: phy: omap-otg: do not write to unallocated memory
Hi, The same patch was already reviewed and applied on usb.git repository[1] [1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/fixes&id=2c2025b41aeff57963f9ae2dd909fea704c625ab Thanks, Chanwoo Choi On 2015년 11월 20일 08:43, Heinrich Schuchardt wrote: > The current coding writes to memory before allocating it. > > Signed-off-by: Heinrich Schuchardt > --- > drivers/usb/phy/phy-omap-otg.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c > index 1270906..8bbf121 100644 > --- a/drivers/usb/phy/phy-omap-otg.c > +++ b/drivers/usb/phy/phy-omap-otg.c > @@ -105,12 +105,13 @@ static int omap_otg_probe(struct platform_device *pdev) > extcon = extcon_get_extcon_dev(config->extcon); > if (!extcon) > return -EPROBE_DEFER; > - otg_dev->extcon = extcon; > > otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); > if (!otg_dev) > return -ENOMEM; > > + otg_dev->extcon = extcon; > + > otg_dev->base = devm_ioremap_resource(&pdev->dev, &pdev->resource[0]); > if (IS_ERR(otg_dev->base)) > return PTR_ERR(otg_dev->base); > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: add support of hi6220
On 11/19/2015 11:04 AM, Felipe Balbi wrote: > > Hi, > > Zhangfei Gao writes: >> Support hisilicon,hi6220-usb for HiKey board >> >> Signed-off-by: Zhangfei Gao > > doesn't apply: > > Applying: usb: dwc2: add support of hi6220 > error: drivers/usb/dwc2/platform.c: does not match index > Patch failed at 0001 usb: dwc2: add support of hi6220 > The copy of the patch that failed is found in: > workspace/linux/.git/rebase-apply/patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > Care to rebase on my testing/next and also collect John's and Rob's ack ? > That's weird. I just sync'd to your testing/next and it seems to apply fine. Same with the series from Gregory Herrero. Any chance it's something to do with your local repo? Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub
On 11/19/2015 8:27 AM, Doug Anderson wrote: > Felipe, > > On Thu, Nov 19, 2015 at 7:34 AM, Felipe Balbi wrote: >> >> Hi, >> >> Douglas Anderson writes: >>> Until we have logic to determine which devices share the same TT let's >>> add logic to assume that all devices on a given dwc2 controller are on >>> one single_tt hub. This is better than the previous code that assumed >>> that all devices were on one multi_tt hub, since single_tt hubs >>> appear (in my experience) to be much more common and any schedule that >>> would work on a single_tt hub will also work on a multi_tt hub. This >>> will prevent more than 8 total low/full speed devices to be on the bus >>> at one time, but that's a reasonable restriction until we've made things >>> smarter. >>> >>> Signed-off-by: Douglas Anderson >>> --- >>> Changes in v3: >>> - Assuming single_tt is new for v3; not terribly well tested (yet). >>> >>> Changes in v2: None >>> >>> drivers/usb/dwc2/core.h | 1 + >>> drivers/usb/dwc2/hcd_queue.c | 40 +++- >>> 2 files changed, 40 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>> index 567ee2c9e69f..09aa2b5ae29e 100644 >>> --- a/drivers/usb/dwc2/core.h >>> +++ b/drivers/usb/dwc2/core.h >>> @@ -782,6 +782,7 @@ struct dwc2_hsotg { >>> u16 periodic_usecs; >>> unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC, >>> BITS_PER_LONG)]; >>> + bool has_split[8]; >> >> why don't you use a u8 instead then just set each bit for each >> "has_split" you need to take care of. This array is kinda ugly. > > Let's actually drop this patch completely. Julius and I sat down and > he talked me through things, and with my current understanding the > current microframe scheduler in dwc2 is broken enough that small > little band-aids like this will do little more than just push the > problems around. > > I'm a good portion of the way through a better microframe scheduler. > I have no doubt that it won't be perfect, but hopefully it will at > least be based in reality... > > My latest thinking on the patches in this series: > > 1. usb: dwc2: rockchip: Make the max_transfer_size automatic > > I'll probably separate this out into its own patch so I can stop > sending it as part of this series. ...or if someone wanted to land it > then I won't bother. > > > 2. usb: dwc2: host: Get aligned DMA in a more supported way > > Still can land any time and has good benefits. I believe that I can't > separate this because it will cause conflicts with scheduler patches. > > > 3. usb: dwc2: host: Add scheduler tracing > > Would be nice to land. > > > 4. usb: dwc2: host: Rewrite the microframe scheduler > 5. usb: dwc2: host: Keep track of and use our scheduled microframe > 6. usb: dwc2: host: Assume all devices are on one single_tt hub > > Please drop patches 4-6 right now. > > > 7. usb: dwc2: host: Add a delay before releasing periodic bandwidth > 8. usb: dwc2: host: Giveback URB in tasklet context > > I'll probably move these back up in the series (like in v2) and put > microframe rewrite atop them. With my current understanding the > scheduling is so broken today that the concerns Alan brought up can > wait until we have a proper scheduler to be addressed. In the > meantime getting the huge boost in interrupt speed will help with > dwc2's correctness (and performance) because it means we're much less > likely to miss SOF interrupts. > > If anyone has any review time, giving a review to v2 of these patches > would be helpful. Otherwise I'll double check that v2 still looks > good with my current understanding and eventually post them again. > > -Doug > Hi Doug, Patches 1-3: Acked-by: John Youn Patch 2: Tested-by: John Youn Tested on core version 3.20 using internal TE for un-aligned buffers. I haven't had time to look into the scheduling patches yet. But I agree with you that there are fundamental problems. I'll await your rewrite. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb-storage: Fix scsi-sd failure "Invalid field in cdb" for USB adapter JMicron
From: Dmitry Katsubo The patch extends the family of SATA-to-USB JMicron adapters that need FUA to be disabled and applies the same policy for uas driver. See details in http://unix.stackexchange.com/questions/237204/ Signed-off-by: Dmitry Katsubo Tested-by: Dmitry Katsubo --- The change is trivial, however it spans also to JMicron adapter with bcdDevice 1.15, which I haven't tested. Nevertheless it is very likely that it is buggy as well. Patch was applied and tested on Debian Jessie 4.2.5-1~bpo8+1. There is a checkpatch warning, but it is caused by original source code formatting. --- drivers/usb/storage/uas.c | 4 drivers/usb/storage/unusual_devs.h | 2 +- drivers/usb/storage/unusual_uas.h | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 48ca9c2..ce37e30 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -796,6 +796,10 @@ static int uas_slave_configure(struct scsi_device *sdev) if (devinfo->flags & US_FL_NO_REPORT_OPCODES) sdev->no_report_opcodes = 1; + /* A few buggy USB-ATA bridges don't understand FUA */ + if (devinfo->flags & US_FL_BROKEN_FUA) + sdev->broken_fua = 1; + scsi_change_queue_depth(sdev, devinfo->qdepth - 2); return 0; } diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h index 6b24791..7ffe420 100644 --- a/drivers/usb/storage/unusual_devs.h +++ b/drivers/usb/storage/unusual_devs.h @@ -1987,7 +1987,7 @@ UNUSUAL_DEV( 0x14cd, 0x6600, 0x0201, 0x0201, US_FL_IGNORE_RESIDUE ), /* Reported by Michael Büsch */ -UNUSUAL_DEV( 0x152d, 0x0567, 0x0114, 0x0114, +UNUSUAL_DEV( 0x152d, 0x0567, 0x0114, 0x0116, "JMicron", "USB to ATA/ATAPI Bridge", USB_SC_DEVICE, USB_PR_DEVICE, NULL, diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h index c85ea53..ccc113e 100644 --- a/drivers/usb/storage/unusual_uas.h +++ b/drivers/usb/storage/unusual_uas.h @@ -132,7 +132,7 @@ UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x, "JMicron", "JMS567", USB_SC_DEVICE, USB_PR_DEVICE, NULL, - US_FL_NO_REPORT_OPCODES), + US_FL_BROKEN_FUA | US_FL_NO_REPORT_OPCODES), /* Reported-by: Hans de Goede */ UNUSUAL_DEV(0x2109, 0x0711, 0x, 0x, -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] HID: usbhid: add Logitech G710+ keyboard quirk NOGET
On Tue, Nov 17, 2015 at 12:40:12AM -0600, Jimmy Berry wrote: > Without quirk keyboard repeats '6' until volume control is used since it > indicates the key is pressed without ever releasing. > > Signed-off-by: Jimmy Berry > --- > drivers/hid/hid-ids.h | 1 + > drivers/hid/usbhid/hid-quirks.c | 1 + > 2 files changed, 2 insertions(+) Please use scripts/get_maintainer.pl to find the right people to cc: and the correct mailing list for your patch (hint, it's not me...) thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: phy: omap-otg: do not write to unallocated memory
The current coding writes to memory before allocating it. Signed-off-by: Heinrich Schuchardt --- drivers/usb/phy/phy-omap-otg.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/usb/phy/phy-omap-otg.c b/drivers/usb/phy/phy-omap-otg.c index 1270906..8bbf121 100644 --- a/drivers/usb/phy/phy-omap-otg.c +++ b/drivers/usb/phy/phy-omap-otg.c @@ -105,12 +105,13 @@ static int omap_otg_probe(struct platform_device *pdev) extcon = extcon_get_extcon_dev(config->extcon); if (!extcon) return -EPROBE_DEFER; - otg_dev->extcon = extcon; otg_dev = devm_kzalloc(&pdev->dev, sizeof(*otg_dev), GFP_KERNEL); if (!otg_dev) return -ENOMEM; + otg_dev->extcon = extcon; + otg_dev->base = devm_ioremap_resource(&pdev->dev, &pdev->resource[0]); if (IS_ERR(otg_dev->base)) return PTR_ERR(otg_dev->base); -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] USB-serial fixes for v4.4-rc2
On Thu, Nov 19, 2015 at 11:20:47AM +0100, Johan Hovold wrote: > Hi Greg, > > Here are a few fixes and new device ids for v4.4-rc2. All have been in > linux-next for a few days now. > > Thanks, > Johan > > > The following changes since commit 8005c49d9aea74d382f474ce11afbbc7d7130bec: > > Linux 4.4-rc1 (2015-11-15 17:00:27 -0800) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git > tags/usb-serial-4.4-rc2 Pulled and pushed out, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Thu, 19 Nov 2015, Ioan-Adrian Ratiu wrote: > But please understand further my reasoning for submitting this patch. > Consider if this is a bug in the wacom driver or in the usbhid core? IMO > this is a usbhid bug: the critical region in hid_ctrl() is too big, > there is no reason for the call to hid_input_report() to be protected by > usbhid->lock. Hmm, it's actually true that we might not need usbhid->lock during hid_input_report() at the end of the day, as we shouldn't be doing any URB-related operations there, neither iofl are being manipulated. If you have already done the full analysis that shows that usbhid->lock is indeed not needed, this absolutely needs to go into changelog as proper justification. Could you please reformulate the changelog in this respect and resubmit? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/2] usb: dwc2: host: Clear interrupts before handling them
In general it is wise to clear interrupts before processing them. If you don't do that, you can get: 1. Interrupt happens 2. You look at system state and process interrupt 3. A new interrupt happens 4. You clear interrupt without processing it. This patch was actually a first attempt to fix missing device insertions as described in (usb: dwc2: host: Fix missing device insertions) and it did solve some of the signal bouncing problems but not all of them (which is why I submitted the other patch). Specifically, this patch itself would sometimes change: 1. hardware sees connect 2. hardware sees disconnect 3. hardware sees connect 4. dwc2_port_intr() - clears connect interrupt 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() ...to: 1. hardware sees connect 2. hardware sees disconnect 3. dwc2_port_intr() - clears connect interrupt 4. hardware sees connect 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() ...but with different timing then sometimes we'd still miss cable insertions. In any case, though this patch doesn't fix any (known) problems, it still seems wise as a general policy to clear interrupt before handling them. Note that for dwc2_handle_usb_port_intr(), instead of moving the clear of PRTINT to the beginning of the function we remove it completely. The only way to clear PRTINT is to clear the sources that set it in the first place. Signed-off-by: Douglas Anderson Acked-by: John Youn Tested-by: John Youn --- Changes in v4: - Don't replace dwc2_writel with writel (Antti Seppälä). - Update description to explain why we remove PRTINT clear. Changes in v3: - Don't (uselessly) clear the PRTINT anymore (Felipe Balbi). Changes in v2: None drivers/usb/dwc2/core_intr.c | 49 +--- drivers/usb/dwc2/hcd_intr.c | 17 --- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 61601d16e233..d85c5c9f96c1 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -86,9 +86,6 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) hprt0 &= ~HPRT0_ENA; dwc2_writel(hprt0, hsotg->regs + HPRT0); } - - /* Clear interrupt */ - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); } /** @@ -98,11 +95,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) */ static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg) { - dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n", -dwc2_is_host_mode(hsotg) ? "Host" : "Device"); - /* Clear interrupt */ dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS); + + dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n", +dwc2_is_host_mode(hsotg) ? "Host" : "Device"); } /** @@ -276,9 +273,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) */ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg) { - u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK); + u32 gintmsk; + + /* Clear interrupt */ + dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS); /* Need to disable SOF interrupt immediately */ + gintmsk = dwc2_readl(hsotg->regs + GINTMSK); gintmsk &= ~GINTSTS_SOF; dwc2_writel(gintmsk, hsotg->regs + GINTMSK); @@ -295,9 +296,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg) queue_work(hsotg->wq_otg, &hsotg->wf_otg); spin_lock(&hsotg->lock); } - - /* Clear interrupt */ - dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS); } /** @@ -315,12 +313,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg) { int ret; - dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n", - hsotg->lx_state); - /* Clear interrupt */ dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS); + dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n", + hsotg->lx_state); + if (dwc2_is_device_mode(hsotg)) { if (hsotg->lx_state == DWC2_L2) { ret = dwc2_exit_hibernation(hsotg, true); @@ -347,6 +345,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg) static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) { int ret; + + /* Clear interrupt */ + dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS); + dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n"); dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state); @@ -368,10 +370,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) /* Chan
[PATCH v4 1/2] usb: dwc2: host: Fix missing device insertions
If you've got your interrupt signals bouncing a bit as you insert your USB device, you might end up in a state when the device is connected but the driver doesn't know it. Specifically, the observed order is: 1. hardware sees connect 2. hardware sees disconnect 3. hardware sees connect 4. dwc2_port_intr() - clears connect interrupt 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() Now you'll be stuck with the cable plugged in and no further interrupts coming in but the driver will think we're disconnected. We'll fix this by checking for the missing connect interrupt and re-connecting after the disconnect is posted. We don't skip the disconnect because if there is a transitory disconnect we really want to de-enumerate and re-enumerate. Notes: 1. As part of this change we add a "force" parameter to dwc2_hcd_disconnect() so that when we're unloading the module we avoid the new behavior. The need for this was pointed out by John Youn. 2. The bit of code needed at the end of dwc2_hcd_disconnect() is exactly the same bit of code from dwc2_port_intr(). To avoid duplication, we refactor that code out into a new function dwc2_hcd_connect(). Signed-off-by: Douglas Anderson Acked-by: John Youn Tested-by: John Youn --- Changes in v4: None Changes in v3: - Add notes to device insertions commit message (Felipe Balbi) Changes in v2: - Don't reconnect when called from _dwc2_hcd_stop() (John Youn) drivers/usb/dwc2/core.h | 6 -- drivers/usb/dwc2/core_intr.c | 4 ++-- drivers/usb/dwc2/hcd.c | 40 ++-- drivers/usb/dwc2/hcd_intr.c | 6 +- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index a66d3cb62b65..daa1c342cdac 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg); -extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg); +extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg); +extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force); extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg); #else static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg) { return 0; } -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {} +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {} +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {} static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {} static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {} static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 27daa42788b1..61601d16e233 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n", hsotg->op_state); spin_unlock(&hsotg->lock); - dwc2_hcd_disconnect(hsotg); + dwc2_hcd_disconnect(hsotg, false); spin_lock(&hsotg->lock); hsotg->op_state = OTG_STATE_A_PERIPHERAL; } else { @@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg) dwc2_op_state_str(hsotg)); if (hsotg->op_state == OTG_STATE_A_HOST) - dwc2_hcd_disconnect(hsotg); + dwc2_hcd_disconnect(hsotg, false); dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS); } diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index e79baf73c234..e208eac7ff57 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) } /** + * dwc2_hcd_connect() - Handles connect of the HCD + * + * @hsotg: Pointer to struct dwc2_hsotg + * + * Must be called with interrupt disabled and spinlock held + */ +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) +{ + if (hsotg->lx_state != DWC2_L0) + usb_hcd_resume_root_hub(hsotg->priv); + + hsotg->flags.b.port_connect_status_change = 1; + hsotg->flags.b.port_connect_status = 1; +} + +/** * dwc2_hcd_disconnect() - Handles disconnect of the HCD * * @hsotg: Pointer to struct dwc2_hsotg + * @force: If true, we won't try to reconnect even if we see device connected. * * Must be called with interrupt disabled and spinlock held */ -void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) +void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) { u32 intr; + u32 hprt0; /* S
Re: [PATCH v3 2/2] usb: dwc2: host: Clear interrupts before handling them
Antti, On Thu, Nov 19, 2015 at 1:09 PM, Antti Seppälä wrote: > On 19 November 2015 at 21:45, Douglas Anderson wrote: >> In general it is wise to clear interrupts before processing them. If >> you don't do that, you can get: >> 1. Interrupt happens >> 2. You look at system state and process interrupt >> 3. A new interrupt happens >> 4. You clear interrupt without processing it. >> >> This patch was actually a first attempt to fix missing device insertions >> as described in (usb: dwc2: host: Fix missing device insertions) and it >> did solve some of the signal bouncing problems but not all of >> them (which is why I submitted the other patch). Specifically, this >> patch itself would sometimes change: >> 1. hardware sees connect >> 2. hardware sees disconnect >> 3. hardware sees connect >> 4. dwc2_port_intr() - clears connect interrupt >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >> >> ...to: >> 1. hardware sees connect >> 2. hardware sees disconnect >> 3. dwc2_port_intr() - clears connect interrupt >> 4. hardware sees connect >> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >> >> ...but with different timing then sometimes we'd still miss cable >> insertions. >> >> In any case, though this patch doesn't fix any (known) problems, it >> still seems wise as a general policy to clear interrupt before handling >> them. >> >> Signed-off-by: Douglas Anderson >> Acked-by: John Youn >> Tested-by: John Youn >> --- >> Changes in v3: >> - Don't (uselessly) clear the PRTINT anymore (Felipe Balbi). >> >> Changes in v2: None >> > > Hi. > > It seems that towards the end of hcd_intr.c you seem to be switching a > few calls of dwc2_writel() to plain writel(). It looks like something > that is easily overlooked but please always use dwc2_writel to > preserve correct register endianness on big-endian platforms. Oops! That was a very dumb oversight, thanks for catching. Sorry about that. Fix coming. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] usb: dwc2: host: Clear interrupts before handling them
On 19 November 2015 at 21:45, Douglas Anderson wrote: > In general it is wise to clear interrupts before processing them. If > you don't do that, you can get: > 1. Interrupt happens > 2. You look at system state and process interrupt > 3. A new interrupt happens > 4. You clear interrupt without processing it. > > This patch was actually a first attempt to fix missing device insertions > as described in (usb: dwc2: host: Fix missing device insertions) and it > did solve some of the signal bouncing problems but not all of > them (which is why I submitted the other patch). Specifically, this > patch itself would sometimes change: > 1. hardware sees connect > 2. hardware sees disconnect > 3. hardware sees connect > 4. dwc2_port_intr() - clears connect interrupt > 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() > > ...to: > 1. hardware sees connect > 2. hardware sees disconnect > 3. dwc2_port_intr() - clears connect interrupt > 4. hardware sees connect > 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() > > ...but with different timing then sometimes we'd still miss cable > insertions. > > In any case, though this patch doesn't fix any (known) problems, it > still seems wise as a general policy to clear interrupt before handling > them. > > Signed-off-by: Douglas Anderson > Acked-by: John Youn > Tested-by: John Youn > --- > Changes in v3: > - Don't (uselessly) clear the PRTINT anymore (Felipe Balbi). > > Changes in v2: None > Hi. It seems that towards the end of hcd_intr.c you seem to be switching a few calls of dwc2_writel() to plain writel(). It looks like something that is easily overlooked but please always use dwc2_writel to preserve correct register endianness on big-endian platforms. Br, -- Antti -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: pxa27x: fix suspend callback
Felipe Balbi writes: > pxa27x disconnects pullups on suspend but doesn't > notify the gadget driver about it, so gadget driver > can't disable the endpoints it was using. > > This causes problems on resume because gadget core > will think endpoints are still enabled and just > ignore the following usb_ep_enable(). > > Fix this problem by calling > gadget_driver->disconnect(). > > Cc: # v3.10+ > Signed-off-by: Felipe Balbi Tested-by: Robert Jarzmik Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/2] usb: dwc2: Make PHY optional
Hi John, John Youn writes: > Fixes commit 09a75e85 > "usb: dwc2: refactor common low-level hw code to platform.c" these two lines should be placed ... > > The above commit consolidated the low-level phy access into a common > location. This change introduced a check from the gadget requiring > that a PHY is specified. This requirement never existed on the host > side and broke some platforms when it was moved into platform.c. > > The gadget doesn't require the PHY either so remove the check. > ... here with the following format: Fixes: 09a75e857790 ("usb: dwc2: refactor common low-level hw code to platform.c") Just is just FYI, as I have already applied another version ;-) -- balbi signature.asc Description: PGP signature
[PATCH] usb: gadget: pxa27x: fix suspend callback
pxa27x disconnects pullups on suspend but doesn't notify the gadget driver about it, so gadget driver can't disable the endpoints it was using. This causes problems on resume because gadget core will think endpoints are still enabled and just ignore the following usb_ep_enable(). Fix this problem by calling gadget_driver->disconnect(). Cc: # v3.10+ Signed-off-by: Felipe Balbi --- drivers/usb/gadget/udc/pxa27x_udc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c index 670ac0b12f00..001a3b74a993 100644 --- a/drivers/usb/gadget/udc/pxa27x_udc.c +++ b/drivers/usb/gadget/udc/pxa27x_udc.c @@ -2536,6 +2536,9 @@ static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state) udc->pullup_resume = udc->pullup_on; dplus_pullup(udc, 0); + if (udc->driver) + udc->driver->disconnect(&udc->gadget); + return 0; } -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: pxa27x: fix suspend callback
Hi, Robert Jarzmik writes: > Felipe Balbi writes: > >> pxa27x disconnects pullups on suspend but doesn't >> notify the gadget driver about it, so gadget driver >> can't disable the endpoints it was using. >> >> This causes problems on resume because gadget core >> will think endpoints are still enabled and just >> ignore the following usb_ep_enable(). >> >> Fix this problem by calling >> gadget_driver->disconnect(). > Thanks for doing this for me. >> @@ -2535,6 +2535,7 @@ static int pxa_udc_suspend(struct platform_device >> *_dev, pm_message_t state) >> udc_disable(udc); >> udc->pullup_resume = udc->pullup_on; >> dplus_pullup(udc, 0); >> +udc->driver->disconnect(&udc->gadget); > If no driver is bound, this will segfault, right ? > Shouldn't an "if (udc->driver)" protect this line ? good catch. v2 coming shortly. -- balbi signature.asc Description: PGP signature
[PATCH v3 2/2] usb: dwc2: host: Clear interrupts before handling them
In general it is wise to clear interrupts before processing them. If you don't do that, you can get: 1. Interrupt happens 2. You look at system state and process interrupt 3. A new interrupt happens 4. You clear interrupt without processing it. This patch was actually a first attempt to fix missing device insertions as described in (usb: dwc2: host: Fix missing device insertions) and it did solve some of the signal bouncing problems but not all of them (which is why I submitted the other patch). Specifically, this patch itself would sometimes change: 1. hardware sees connect 2. hardware sees disconnect 3. hardware sees connect 4. dwc2_port_intr() - clears connect interrupt 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() ...to: 1. hardware sees connect 2. hardware sees disconnect 3. dwc2_port_intr() - clears connect interrupt 4. hardware sees connect 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() ...but with different timing then sometimes we'd still miss cable insertions. In any case, though this patch doesn't fix any (known) problems, it still seems wise as a general policy to clear interrupt before handling them. Signed-off-by: Douglas Anderson Acked-by: John Youn Tested-by: John Youn --- Changes in v3: - Don't (uselessly) clear the PRTINT anymore (Felipe Balbi). Changes in v2: None drivers/usb/dwc2/core_intr.c | 49 +--- drivers/usb/dwc2/hcd_intr.c | 16 +++ 2 files changed, 30 insertions(+), 35 deletions(-) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 61601d16e233..d85c5c9f96c1 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -86,9 +86,6 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) hprt0 &= ~HPRT0_ENA; dwc2_writel(hprt0, hsotg->regs + HPRT0); } - - /* Clear interrupt */ - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); } /** @@ -98,11 +95,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) */ static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg) { - dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n", -dwc2_is_host_mode(hsotg) ? "Host" : "Device"); - /* Clear interrupt */ dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS); + + dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n", +dwc2_is_host_mode(hsotg) ? "Host" : "Device"); } /** @@ -276,9 +273,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) */ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg) { - u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK); + u32 gintmsk; + + /* Clear interrupt */ + dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS); /* Need to disable SOF interrupt immediately */ + gintmsk = dwc2_readl(hsotg->regs + GINTMSK); gintmsk &= ~GINTSTS_SOF; dwc2_writel(gintmsk, hsotg->regs + GINTMSK); @@ -295,9 +296,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg) queue_work(hsotg->wq_otg, &hsotg->wf_otg); spin_lock(&hsotg->lock); } - - /* Clear interrupt */ - dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS); } /** @@ -315,12 +313,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg) { int ret; - dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n", - hsotg->lx_state); - /* Clear interrupt */ dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS); + dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n", + hsotg->lx_state); + if (dwc2_is_device_mode(hsotg)) { if (hsotg->lx_state == DWC2_L2) { ret = dwc2_exit_hibernation(hsotg, true); @@ -347,6 +345,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg) static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) { int ret; + + /* Clear interrupt */ + dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS); + dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n"); dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state); @@ -368,10 +370,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) /* Change to L0 state */ hsotg->lx_state = DWC2_L0; } else { - if (hsotg->core_params->hibernation) { - dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS); + if (hsotg->core_params->hibernation) return; - } + if (hsotg->lx_state != DWC2_
[PATCH v3 1/2] usb: dwc2: host: Fix missing device insertions
If you've got your interrupt signals bouncing a bit as you insert your USB device, you might end up in a state when the device is connected but the driver doesn't know it. Specifically, the observed order is: 1. hardware sees connect 2. hardware sees disconnect 3. hardware sees connect 4. dwc2_port_intr() - clears connect interrupt 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() Now you'll be stuck with the cable plugged in and no further interrupts coming in but the driver will think we're disconnected. We'll fix this by checking for the missing connect interrupt and re-connecting after the disconnect is posted. We don't skip the disconnect because if there is a transitory disconnect we really want to de-enumerate and re-enumerate. Notes: 1. As part of this change we add a "force" parameter to dwc2_hcd_disconnect() so that when we're unloading the module we avoid the new behavior. The need for this was pointed out by John Youn. 2. The bit of code needed at the end of dwc2_hcd_disconnect() is exactly the same bit of code from dwc2_port_intr(). To avoid duplication, we refactor that code out into a new function dwc2_hcd_connect(). Signed-off-by: Douglas Anderson Acked-by: John Youn Tested-by: John Youn --- Changes in v3: - Add notes to device insertions commit message (Felipe Balbi) Changes in v2: - Don't reconnect when called from _dwc2_hcd_stop() (John Youn) drivers/usb/dwc2/core.h | 6 -- drivers/usb/dwc2/core_intr.c | 4 ++-- drivers/usb/dwc2/hcd.c | 40 ++-- drivers/usb/dwc2/hcd_intr.c | 6 +- 4 files changed, 45 insertions(+), 11 deletions(-) diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index a66d3cb62b65..daa1c342cdac 100644 --- a/drivers/usb/dwc2/core.h +++ b/drivers/usb/dwc2/core.h @@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg, #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE) extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg); -extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg); +extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg); +extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force); extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg); #else static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg) { return 0; } -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {} +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {} +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {} static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {} static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {} static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 27daa42788b1..61601d16e233 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n", hsotg->op_state); spin_unlock(&hsotg->lock); - dwc2_hcd_disconnect(hsotg); + dwc2_hcd_disconnect(hsotg, false); spin_lock(&hsotg->lock); hsotg->op_state = OTG_STATE_A_PERIPHERAL; } else { @@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg) dwc2_op_state_str(hsotg)); if (hsotg->op_state == OTG_STATE_A_HOST) - dwc2_hcd_disconnect(hsotg); + dwc2_hcd_disconnect(hsotg, false); dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS); } diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index e79baf73c234..e208eac7ff57 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) } /** + * dwc2_hcd_connect() - Handles connect of the HCD + * + * @hsotg: Pointer to struct dwc2_hsotg + * + * Must be called with interrupt disabled and spinlock held + */ +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) +{ + if (hsotg->lx_state != DWC2_L0) + usb_hcd_resume_root_hub(hsotg->priv); + + hsotg->flags.b.port_connect_status_change = 1; + hsotg->flags.b.port_connect_status = 1; +} + +/** * dwc2_hcd_disconnect() - Handles disconnect of the HCD * * @hsotg: Pointer to struct dwc2_hsotg + * @force: If true, we won't try to reconnect even if we see device connected. * * Must be called with interrupt disabled and spinlock held */ -void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) +void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) { u32 intr; + u32 hprt0; /* Set status flags for
Re: [PATCH] usb: gadget: pxa27x: fix suspend callback
Felipe Balbi writes: > pxa27x disconnects pullups on suspend but doesn't > notify the gadget driver about it, so gadget driver > can't disable the endpoints it was using. > > This causes problems on resume because gadget core > will think endpoints are still enabled and just > ignore the following usb_ep_enable(). > > Fix this problem by calling > gadget_driver->disconnect(). Thanks for doing this for me. > @@ -2535,6 +2535,7 @@ static int pxa_udc_suspend(struct platform_device > *_dev, pm_message_t state) > udc_disable(udc); > udc->pullup_resume = udc->pullup_on; > dplus_pullup(udc, 0); > + udc->driver->disconnect(&udc->gadget); If no driver is bound, this will segfault, right ? Shouldn't an "if (udc->driver)" protect this line ? Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 10/13] usb: dwc2: host: enable descriptor dma for fs devices
Hi, Gregory Herrero writes: > From: Mian Yousaf Kaukab > > As descriptor dma mode does not support split transfers, it can't be > enabled for high speed devices. Add a core parameter to enable it for > full speed devices. > > Ensure frame list and descriptor list are correctly freed during > disconnect. > > Signed-off-by: Mian Yousaf Kaukab > Signed-off-by: Gregory Herrero this one doesn't apply: Applying: usb: dwc2: host: enable descriptor dma for fs devices error: drivers/usb/dwc2/core.h: does not match index error: drivers/usb/dwc2/hcd.c: does not match index error: drivers/usb/dwc2/hcd_intr.c: does not match index error: drivers/usb/dwc2/platform.c: does not match index Patch failed at 0001 usb: dwc2: host: enable descriptor dma for fs devices The copy of the patch that failed is found in: workspace/linux/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Care to rebase on my testing/next ? Patches 1-9 are already applied. -- balbi signature.asc Description: PGP signature
Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub
Hi, Doug Anderson writes: >> Douglas Anderson writes: >>> Until we have logic to determine which devices share the same TT let's >>> add logic to assume that all devices on a given dwc2 controller are on >>> one single_tt hub. This is better than the previous code that assumed >>> that all devices were on one multi_tt hub, since single_tt hubs >>> appear (in my experience) to be much more common and any schedule that >>> would work on a single_tt hub will also work on a multi_tt hub. This >>> will prevent more than 8 total low/full speed devices to be on the bus >>> at one time, but that's a reasonable restriction until we've made things >>> smarter. >>> >>> Signed-off-by: Douglas Anderson >>> --- >>> Changes in v3: >>> - Assuming single_tt is new for v3; not terribly well tested (yet). >>> >>> Changes in v2: None >>> >>> drivers/usb/dwc2/core.h | 1 + >>> drivers/usb/dwc2/hcd_queue.c | 40 +++- >>> 2 files changed, 40 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >>> index 567ee2c9e69f..09aa2b5ae29e 100644 >>> --- a/drivers/usb/dwc2/core.h >>> +++ b/drivers/usb/dwc2/core.h >>> @@ -782,6 +782,7 @@ struct dwc2_hsotg { >>> u16 periodic_usecs; >>> unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC, >>> BITS_PER_LONG)]; >>> + bool has_split[8]; >> >> why don't you use a u8 instead then just set each bit for each >> "has_split" you need to take care of. This array is kinda ugly. > > Let's actually drop this patch completely. Julius and I sat down and > he talked me through things, and with my current understanding the > current microframe scheduler in dwc2 is broken enough that small > little band-aids like this will do little more than just push the > problems around. > > I'm a good portion of the way through a better microframe scheduler. > I have no doubt that it won't be perfect, but hopefully it will at > least be based in reality... > > My latest thinking on the patches in this series: > > 1. usb: dwc2: rockchip: Make the max_transfer_size automatic > > I'll probably separate this out into its own patch so I can stop > sending it as part of this series. ...or if someone wanted to land it > then I won't bother. > > > 2. usb: dwc2: host: Get aligned DMA in a more supported way > > Still can land any time and has good benefits. I believe that I can't > separate this because it will cause conflicts with scheduler patches. > > > 3. usb: dwc2: host: Add scheduler tracing > > Would be nice to land. > > > 4. usb: dwc2: host: Rewrite the microframe scheduler > 5. usb: dwc2: host: Keep track of and use our scheduled microframe > 6. usb: dwc2: host: Assume all devices are on one single_tt hub > > Please drop patches 4-6 right now. > > > 7. usb: dwc2: host: Add a delay before releasing periodic bandwidth > 8. usb: dwc2: host: Giveback URB in tasklet context if you can, it's best to send a new series with the changes. This makes mine and John's life a lot easier :-) > I'll probably move these back up in the series (like in v2) and put > microframe rewrite atop them. With my current understanding the > scheduling is so broken today that the concerns Alan brought up can > wait until we have a proper scheduler to be addressed. In the > meantime getting the huge boost in interrupt speed will help with > dwc2's correctness (and performance) because it means we're much less > likely to miss SOF interrupts. > > If anyone has any review time, giving a review to v2 of these patches > would be helpful. Otherwise I'll double check that v2 still looks > good with my current understanding and eventually post them again. That would have to be someone experienced with that IP. I don't even have docs :-) -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them
On 11/19/2015 10:19 AM, Felipe Balbi wrote: > > Hi, > > Doug Anderson writes: > isn't this a regression ? You're first clearing the interrupts and only > then reading to check what's pending, however, what's pending has just > been cleared. Seems like this should be: > > hprt0 = dwc2_readl(HPRT0); > dwc2_writeal(PRTINT, GINTSTS); Actually, we could probably remove the setting of GINTSTS_PRTINT completely. The docs I have say that the GINTSTS_PRTINT is read only and that: > The core sets this bit to indicate a change in port status of one of the > DWC_otg core ports in Host mode. The application must read the > Host Port Control and Status (HPRT) register to determine the exact > event that caused this interrupt. The application must clear the > appropriate status bit in the Host Port Control and Status register to > clear this bit. ...so writing PRTINT is probably useless, but John can confirm. >>> >>> Yup, it seems it can be removed. >> >> How do you guys want this handled? Should I send up a new version of >> this patch? ...or should I send a followon patch that does this >> removal? > > I'll leave the final decision to John, but my opinion is that a new > version of the patch would be preferrable. > Hi Doug, Could you resend with the change? Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: phy: mxs: add "fsl,imx6ul-usbphy" compatible string
Hi, Peter Chen writes: > On Wed, Sep 16, 2015 at 03:52:33PM +0800, Li Jun wrote: >> From: Peter Chen >> >> Add "fsl,imx6ul-usbphy" compatible string for iMX6ul usb phy >> >> Signed-off-by: Peter Chen >> Signed-off-by: Li Jun >> --- >> drivers/usb/phy/phy-mxs-usb.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c >> index 4d863eb..630f643 100644 >> --- a/drivers/usb/phy/phy-mxs-usb.c >> +++ b/drivers/usb/phy/phy-mxs-usb.c >> @@ -143,12 +143,17 @@ static const struct mxs_phy_data imx6sx_phy_data = { >> .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS, >> }; >> >> +static const struct mxs_phy_data imx6ul_phy_data = { >> +.flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS, >> +}; >> + >> static const struct of_device_id mxs_phy_dt_ids[] = { >> { .compatible = "fsl,imx6sx-usbphy", .data = &imx6sx_phy_data, }, >> { .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_phy_data, }, >> { .compatible = "fsl,imx6q-usbphy", .data = &imx6q_phy_data, }, >> { .compatible = "fsl,imx23-usbphy", .data = &imx23_phy_data, }, >> { .compatible = "fsl,vf610-usbphy", .data = &vf610_phy_data, }, >> +{ .compatible = "fsl,imx6ul-usbphy", .data = &imx6ul_phy_data, }, >> { /* sentinel */ } >> }; >> MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids); >> -- >> 1.9.1 >> > > ping... https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/fixes&id=4c55c3cdabdcbe6f458ba5192e1df20cc2909488 -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc2: add support of hi6220
Hi, Zhangfei Gao writes: > Support hisilicon,hi6220-usb for HiKey board > > Signed-off-by: Zhangfei Gao doesn't apply: Applying: usb: dwc2: add support of hi6220 error: drivers/usb/dwc2/platform.c: does not match index Patch failed at 0001 usb: dwc2: add support of hi6220 The copy of the patch that failed is found in: workspace/linux/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Care to rebase on my testing/next and also collect John's and Rob's ack ? -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 2/4] Documentation: usb: gadget-testing: add description for depth of queue
Hi, Peter Chen writes: > Add both bulk and iso depth of queue for sourcesink. > > Signed-off-by: Peter Chen > --- > Documentation/usb/gadget-testing.txt | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/usb/gadget-testing.txt > b/Documentation/usb/gadget-testing.txt > index b24d3ef..84b3d10 100644 > --- a/Documentation/usb/gadget-testing.txt > +++ b/Documentation/usb/gadget-testing.txt > @@ -579,6 +579,8 @@ The SOURCESINK function provides these attributes in its > function directory: > isoc_mult - 0..2 (hs/ss only) > isoc_maxburst - 0..15 (ss only) > bulk_buflen - buffer length > + bulk_qlen - depth of queue for bulk > + iso_qlen- depth of queue for iso doesn't apply for me: Applying: Documentation: usb: gadget-testing: add description for depth of queue error: patch failed: Documentation/usb/gadget-testing.txt:579 error: Documentation/usb/gadget-testing.txt: patch does not apply Patch failed at 0001 Documentation: usb: gadget-testing: add description for depth of queue The copy of the patch that failed is found in: workspace/linux/.git/rebase-apply/patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". Care to rebase on my testing/next ? Note that patch 1 is already applied. When rebasing, please collect Krzysztof's Reviewed-by ;-) cheers -- balbi signature.asc Description: PGP signature
Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them
Hi, Doug Anderson writes: isn't this a regression ? You're first clearing the interrupts and only then reading to check what's pending, however, what's pending has just been cleared. Seems like this should be: hprt0 = dwc2_readl(HPRT0); dwc2_writeal(PRTINT, GINTSTS); >>> >>> Actually, we could probably remove the setting of GINTSTS_PRTINT >>> completely. The docs I have say that the GINTSTS_PRTINT is read only >>> and that: >>> The core sets this bit to indicate a change in port status of one of the DWC_otg core ports in Host mode. The application must read the Host Port Control and Status (HPRT) register to determine the exact event that caused this interrupt. The application must clear the appropriate status bit in the Host Port Control and Status register to clear this bit. >>> >>> ...so writing PRTINT is probably useless, but John can confirm. >>> >> >> Yup, it seems it can be removed. > > How do you guys want this handled? Should I send up a new version of > this patch? ...or should I send a followon patch that does this > removal? I'll leave the final decision to John, but my opinion is that a new version of the patch would be preferrable. -- balbi signature.asc Description: PGP signature
[PATCH] usb: dwc3: remove dwc3-qcom in favor of dwc3-of-simple
Now that we have a generic dwc3-of-simple.c, we can use that instead of maintaining dwc3-qcom.c which is extremely similar. Cc: Ivan T. Ivanov Signed-off-by: Felipe Balbi --- drivers/usb/dwc3/Kconfig | 8 --- drivers/usb/dwc3/Makefile| 1 - drivers/usb/dwc3/dwc3-qcom.c | 130 --- 3 files changed, 139 deletions(-) delete mode 100644 drivers/usb/dwc3/dwc3-qcom.c diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig index 070e704829e5..a64ce1c94d6d 100644 --- a/drivers/usb/dwc3/Kconfig +++ b/drivers/usb/dwc3/Kconfig @@ -105,12 +105,4 @@ config USB_DWC3_ST inside (i.e. STiH407). Say 'Y' or 'M' if you have one such device. -config USB_DWC3_QCOM - tristate "Qualcomm Platforms" - depends on ARCH_QCOM || COMPILE_TEST - default USB_DWC3 - help - Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside, - say 'Y' or 'M' if you have one such device. - endif diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile index 6491f9b474d4..22420e17d68b 100644 --- a/drivers/usb/dwc3/Makefile +++ b/drivers/usb/dwc3/Makefile @@ -38,5 +38,4 @@ obj-$(CONFIG_USB_DWC3_EXYNOS) += dwc3-exynos.o obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o obj-$(CONFIG_USB_DWC3_KEYSTONE)+= dwc3-keystone.o obj-$(CONFIG_USB_DWC3_OF_SIMPLE) += dwc3-of-simple.o -obj-$(CONFIG_USB_DWC3_QCOM)+= dwc3-qcom.o obj-$(CONFIG_USB_DWC3_ST) += dwc3-st.o diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c deleted file mode 100644 index 088026048f49.. --- a/drivers/usb/dwc3/dwc3-qcom.c +++ /dev/null @@ -1,130 +0,0 @@ -/* Copyright (c) 2013-2014, The Linux Foundation. All rights reserved. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 and - * only version 2 as published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#include -#include -#include -#include -#include -#include -#include - -struct dwc3_qcom { - struct device *dev; - - struct clk *core_clk; - struct clk *iface_clk; - struct clk *sleep_clk; -}; - -static int dwc3_qcom_probe(struct platform_device *pdev) -{ - struct device_node *node = pdev->dev.of_node; - struct dwc3_qcom *qdwc; - int ret; - - qdwc = devm_kzalloc(&pdev->dev, sizeof(*qdwc), GFP_KERNEL); - if (!qdwc) - return -ENOMEM; - - platform_set_drvdata(pdev, qdwc); - - qdwc->dev = &pdev->dev; - - qdwc->core_clk = devm_clk_get(qdwc->dev, "core"); - if (IS_ERR(qdwc->core_clk)) { - dev_err(qdwc->dev, "failed to get core clock\n"); - return PTR_ERR(qdwc->core_clk); - } - - qdwc->iface_clk = devm_clk_get(qdwc->dev, "iface"); - if (IS_ERR(qdwc->iface_clk)) { - dev_info(qdwc->dev, "failed to get optional iface clock\n"); - qdwc->iface_clk = NULL; - } - - qdwc->sleep_clk = devm_clk_get(qdwc->dev, "sleep"); - if (IS_ERR(qdwc->sleep_clk)) { - dev_info(qdwc->dev, "failed to get optional sleep clock\n"); - qdwc->sleep_clk = NULL; - } - - ret = clk_prepare_enable(qdwc->core_clk); - if (ret) { - dev_err(qdwc->dev, "failed to enable core clock\n"); - goto err_core; - } - - ret = clk_prepare_enable(qdwc->iface_clk); - if (ret) { - dev_err(qdwc->dev, "failed to enable optional iface clock\n"); - goto err_iface; - } - - ret = clk_prepare_enable(qdwc->sleep_clk); - if (ret) { - dev_err(qdwc->dev, "failed to enable optional sleep clock\n"); - goto err_sleep; - } - - ret = of_platform_populate(node, NULL, NULL, qdwc->dev); - if (ret) { - dev_err(qdwc->dev, "failed to register core - %d\n", ret); - goto err_clks; - } - - return 0; - -err_clks: - clk_disable_unprepare(qdwc->sleep_clk); -err_sleep: - clk_disable_unprepare(qdwc->iface_clk); -err_iface: - clk_disable_unprepare(qdwc->core_clk); -err_core: - return ret; -} - -static int dwc3_qcom_remove(struct platform_device *pdev) -{ - struct dwc3_qcom *qdwc = platform_get_drvdata(pdev); - - of_platform_depopulate(&pdev->dev); - - clk_disable_unprepare(qdwc->sleep_clk); - clk_disable_unprepare(qdwc->iface_clk); - clk_disable_unprepare(qdwc->core_clk); - - return 0; -} - -static const struct of_device_id of_dwc3_match[] = { -
RE: [PATCH] usb: dwc3: add generic OF glue layer
Hi, Subbaraya Sundeep Bhatta writes: > Hi Felipe, > >> -Original Message- >> From: Felipe Balbi [mailto:ba...@ti.com] >> Sent: Thursday, November 19, 2015 8:28 PM >> To: Linux USB Mailing List >> Cc: Subbaraya Sundeep Bhatta; Ivan T . Ivanov >> Subject: Re: [PATCH] usb: dwc3: add generic OF glue layer >> >> >> Hi, >> >> Felipe Balbi writes: >> > For simple platforms which merely enable some clocks and populate its >> > children, we can use this generic glue layer to avoid boilerplate code >> > duplication. >> > >> > For now this supports Qcom and Xilinx, but if we find a way to add >> > generic handling of regulators and optional PHYs, we can absorb exynos >> > as well. >> > >> > Signed-off-by: Felipe Balbi >> > --- >> > >> > Can you guys check if this works for your respective platforms ? If it >> > does we can some code duplication going forward. >> >> gentle ping on this one. > > Tested and working fine on Xilinx platform. thank you, I'll take your DTS with this generic glue, and also delete qcom glue. -- balbi signature.asc Description: PGP signature
RE: [PATCH] usb: dwc3: add generic OF glue layer
Hi Felipe, > -Original Message- > From: Felipe Balbi [mailto:ba...@ti.com] > Sent: Thursday, November 19, 2015 8:28 PM > To: Linux USB Mailing List > Cc: Subbaraya Sundeep Bhatta; Ivan T . Ivanov > Subject: Re: [PATCH] usb: dwc3: add generic OF glue layer > > > Hi, > > Felipe Balbi writes: > > For simple platforms which merely enable some clocks and populate its > > children, we can use this generic glue layer to avoid boilerplate code > > duplication. > > > > For now this supports Qcom and Xilinx, but if we find a way to add > > generic handling of regulators and optional PHYs, we can absorb exynos > > as well. > > > > Signed-off-by: Felipe Balbi > > --- > > > > Can you guys check if this works for your respective platforms ? If it > > does we can some code duplication going forward. > > gentle ping on this one. It is working on Xilinx platform. Thanks, Sundeep > > -- > balbi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb: dwc3: add generic OF glue layer
Hi Felipe, > -Original Message- > From: Felipe Balbi [mailto:ba...@ti.com] > Sent: Thursday, November 19, 2015 8:28 PM > To: Linux USB Mailing List > Cc: Subbaraya Sundeep Bhatta; Ivan T . Ivanov > Subject: Re: [PATCH] usb: dwc3: add generic OF glue layer > > > Hi, > > Felipe Balbi writes: > > For simple platforms which merely enable some clocks and populate its > > children, we can use this generic glue layer to avoid boilerplate code > > duplication. > > > > For now this supports Qcom and Xilinx, but if we find a way to add > > generic handling of regulators and optional PHYs, we can absorb exynos > > as well. > > > > Signed-off-by: Felipe Balbi > > --- > > > > Can you guys check if this works for your respective platforms ? If it > > does we can some code duplication going forward. > > gentle ping on this one. Tested and working fine on Xilinx platform. Thanks, Sundeep > > -- > balbi This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them
Hi, On Wed, Nov 18, 2015 at 5:43 PM, John Youn wrote: > On 11/16/2015 9:22 AM, Doug Anderson wrote: >> Felipe, >> >> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi wrote: >>> >>> Hi, >>> >>> Douglas Anderson writes: In general it is wise to clear interrupts before processing them. If you don't do that, you can get: 1. Interrupt happens 2. You look at system state and process interrupt 3. A new interrupt happens 4. You clear interrupt without processing it. This patch was actually a first attempt to fix missing device insertions as described in (usb: dwc2: host: Fix missing device insertions) and it did solve some of the signal bouncing problems but not all of them (which is why I submitted the other patch). Specifically, this patch itself would sometimes change: 1. hardware sees connect 2. hardware sees disconnect 3. hardware sees connect 4. dwc2_port_intr() - clears connect interrupt 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() ...to: 1. hardware sees connect 2. hardware sees disconnect 3. dwc2_port_intr() - clears connect interrupt 4. hardware sees connect 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() ...but with different timing then sometimes we'd still miss cable insertions. In any case, though this patch doesn't fix any (known) problems, it still seems wise as a general policy to clear interrupt before handling them. Signed-off-by: Douglas Anderson --- Changes in v2: None drivers/usb/dwc2/core_intr.c | 55 ++-- drivers/usb/dwc2/hcd_intr.c | 16 ++--- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c index 61601d16e233..2a166b7eec41 100644 --- a/drivers/usb/dwc2/core_intr.c +++ b/drivers/usb/dwc2/core_intr.c @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg) */ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) { - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0); + u32 hprt0; + /* Clear interrupt */ + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); + + hprt0 = dwc2_readl(hsotg->regs + HPRT0); if (hprt0 & HPRT0_ENACHG) { hprt0 &= ~HPRT0_ENA; dwc2_writel(hprt0, hsotg->regs + HPRT0); } - - /* Clear interrupt */ - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); >>> >>> isn't this a regression ? You're first clearing the interrupts and only >>> then reading to check what's pending, however, what's pending has just >>> been cleared. Seems like this should be: >>> >>> hprt0 = dwc2_readl(HPRT0); >>> dwc2_writeal(PRTINT, GINTSTS); >> >> Actually, we could probably remove the setting of GINTSTS_PRTINT >> completely. The docs I have say that the GINTSTS_PRTINT is read only >> and that: >> >>> The core sets this bit to indicate a change in port status of one of the >>> DWC_otg core ports in Host mode. The application must read the >>> Host Port Control and Status (HPRT) register to determine the exact >>> event that caused this interrupt. The application must clear the >>> appropriate status bit in the Host Port Control and Status register to >>> clear this bit. >> >> ...so writing PRTINT is probably useless, but John can confirm. >> > > Yup, it seems it can be removed. How do you guys want this handled? Should I send up a new version of this patch? ...or should I send a followon patch that does this removal? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help needed for EHCI problem: removing an active bulk-in QH
On Wed, Nov 18, 2015 at 10:23 AM, Alan Stern wrote: > For the last patch. And yes, do set usb_snoop_max. Dmesg output with usb_snoop and usb_snoop_max is attached below. > I hope so. There is one thing I'm still undecided about: Should this > workaround be applied only to AMD/ATI controllers or should it apply to > everything? It does have a small probability of slowing down transfers > under certain circumstances, but on the other hand, it's quite possible > that other controller types will have the same sort of weakness as the > AMD ones. We've tested this application on a handful of other platforms and have really only had issues with this desktop and our tablet device which is stuck on the 2.6 kernel. Looking at the output of that tablet when it gets "stuck" (this was posted back on the original libusb post) makes it also seem like it may have some other problem than the one we have been debugging here. Not sure if we can provide any other input here that would help in making that decision. Michael Reutman Software Engineer Epiq Solutions 847-598-0218 x68 www.epiqsolutions.com [ 834.390818] usb 5-4: opened by process 2868: main [ 834.390842] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.390845] usb 5-4: usbfs: process 2868 (main) did not claim interface 0 before use [ 834.390867] usb 5-4: urb submit 880328793a40 [ 834.390869] usb 5-4: userurb 009b9480, ep2 bulk-in, length 16384 [ 834.390902] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.390907] usb 5-4: urb submit 880328793080 [ 834.390908] usb 5-4: userurb 009b94c0, ep2 bulk-in, length 16384 [ 834.390911] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.390916] usb 5-4: urb submit 8803287926c0 [ 834.390917] usb 5-4: userurb 009b9500, ep2 bulk-in, length 16384 [ 834.390920] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.390924] usb 5-4: urb submit 880328793800 [ 834.390925] usb 5-4: userurb 009b9540, ep2 bulk-in, length 16384 [ 834.391349] usb 5-4: urb complete 880328793a40 [ 834.391350] usb 5-4: userurb 009b9480, ep2 bulk-in, actual_length 16384 status 0 [ 834.391352] data: 01 51 0e 0b 00 00 00 00 87 c7 .Q [ 834.391366] usb 5-4: usbdev_do_ioctl: REAPURBNDELAY [ 834.391369] usb 5-4: proc_reapurbnonblock: REAP 009b9480 [ 834.391718] usb 5-4: urb complete 880328793080 [ 834.391720] usb 5-4: userurb 009b94c0, ep2 bulk-in, actual_length 16384 status 0 [ 834.391721] data: 4d 8d 71 24 00 00 00 00 8b 8d M.q$.. [ 834.392218] usb 5-4: urb complete 8803287926c0 [ 834.392219] usb 5-4: userurb 009b9500, ep2 bulk-in, actual_length 16384 status 0 [ 834.392221] data: 35 9d 71 24 00 00 00 00 63 e0 5.q$c. [ 834.392718] usb 5-4: urb complete 880328793800 [ 834.392720] usb 5-4: userurb 009b9540, ep2 bulk-in, actual_length 16384 status 0 [ 834.392721] data: 1d ad 71 24 00 00 00 00 3b 33 ..q$;3 [ 834.393655] usb 5-4: usbdev_do_ioctl: REAPURBNDELAY [ 834.393658] usb 5-4: proc_reapurbnonblock: REAP 009b94c0 [ 834.393999] usb 5-4: usbdev_do_ioctl: REAPURBNDELAY [ 834.394001] usb 5-4: proc_reapurbnonblock: REAP 009b9500 [ 834.394023] usb 5-4: usbdev_do_ioctl: DISCARDURB 009b9540 [ 834.394152] usb 5-4: usbdev_do_ioctl: REAPURBNDELAY [ 834.394153] usb 5-4: proc_reapurbnonblock: REAP 009b9540 [ 834.394716] usb 5-4: usbdev_do_ioctl: REAPURBNDELAY [ 834.404826] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.404832] usb 5-4: urb submit 880328792a80 [ 834.404834] usb 5-4: userurb 009b9540, ep2 bulk-in, length 16384 [ 834.404856] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.404859] usb 5-4: urb submit 880328792540 [ 834.404860] usb 5-4: userurb 009b9500, ep2 bulk-in, length 16384 [ 834.404866] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.404869] usb 5-4: urb submit 8803287935c0 [ 834.404870] usb 5-4: userurb 009b94c0, ep2 bulk-in, length 16384 [ 834.404876] usb 5-4: usbdev_do_ioctl: SUBMITURB [ 834.404879] usb 5-4: urb submit 880328792780 [ 834.404881] usb 5-4: userurb 009b9480, ep2 bulk-in, length 16384 [ 834.405478] usb 5-4: urb complete 880328792a80 [ 834.405480] usb 5-4: userurb 009b9540, ep2 bulk-in, actual_length 16384 status 0 [ 834.405481] data: 05 bd 71 24 00 00 00 00 13 86 ..q$.. [ 834.405499] usb 5-4: usbdev_do_ioctl: REAPURBNDELAY [ 834.405501] usb 5-4: proc_reapurbnonblock: REAP 009b9540 [ 834.405852] usb 5-4: urb complete 880328792540 [ 834.405853] usb 5-4: userurb 009b9500, ep2 bulk-in, actual_length 16384 status 0 [ 834.405855] data: 6c 37 73 24 00 00 00 00 e5
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Thu, 19 Nov 2015 10:10:19 +0100 (CET) Jiri Kosina wrote: > On Thu, 19 Nov 2015, Ioan-Adrian Ratiu wrote: > > > First part of lockdep report: > > http://imgur.com/clLsCWe > > > > Second part: > > http://imgur.com/Wa2PzRl > > > > Here are some printk's of mine while reproducing + debugging the issue: > > http://imgur.com/SETOHT7 > > So the real problem is that Intuos driver is calling hid_hw_request() > (which tries to grab the lock in usbhid_submit_report()) while handling > the CTRL IRQ (lock gets acquired there). > > So the proper way to fix seems to be delaying the scheduling of the > proximity read event in wacom_intuos_inout() to workqueue. > > > I'll continue to research this more in depth, but progress is slow > > because I don't have much time, I'm doing this in my spare time because > > it's my girlfriend's tablet. > > Oh, now I understand the level of severity of this bug! :-) > > Thanks, > Yes, exactly, you are beginning to understand! :) When I've put my 2 variants above to solve this deadlock, by "removing the call from wacom" at 1) I was trying to say exactly this, removing it from the irq to a workqueue. But please understand further my reasoning for submitting this patch. Consider if this is a bug in the wacom driver or in the usbhid core? IMO this is a usbhid bug: the critical region in hid_ctrl() is too big, there is no reason for the call to hid_input_report() to be protected by usbhid->lock. The correct way to fix this deadlock is to fix the critical section in usbhid, not remove the call from the wacom irq. If wacom wants to reschedule in the irq, it should not deadlock on usbhid. "Fixing" the wacom call would just work around the critical region bug inside usbhid. I hope I've made myself clear this time; I really needed to explain this patch better :( sorry. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: scsi-sd fails with error "Invalid field in cdb" for SATA-to-USB adapter JMicron
On Thu, 19 Nov 2015, Dmitry Katsubo wrote: > On 2015-11-17 19:18, Alan Stern wrote: > > That line is completely inappropriate for uas; it applies only to > > usb-storage. Don't add it. > > I got it. My first thought was like you have said (every module uses its > own structure), but I blindly tried to guess. > > > Here you need to test devinfo->flags & US_FL_BROKEN_FUA. > > Great, that worked! > > scsi host6: uas > scsi 6:0:0:0: Direct-Access JMicron Generic 0116 PQ: 0 ANSI: 6 > sd 6:0:0:0: Attached scsi generic sg2 type 0 > sd 6:0:0:0: [sdb] 312581808 512-byte logical blocks: (160 GB/149 GiB) > sd 6:0:0:0: [sdb] 4096-byte physical blocks > sd 6:0:0:0: [sdb] Write Protect is off > sd 6:0:0:0: [sdb] Mode Sense: 53 00 10 08 > sd 6:0:0:0: [sdb] Disabling FUA > sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't > support DPO or FUA > sd 6:0:0:0: [sdb] Attached SCSI disk Good. > Actually there is an entry in unusual_uas.h but there is a minor difference: > > unusual_uas.h uses UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x, ... > unusual_devs.h uses UNUSUAL_DEV(0x152d, 0x0567, 0x0114, 0x0116, ... > > so uas module captures the wider set of devices (effectively ignores > bcdDevice). Should it be left like that (then behaviour could be > different for usb-storage vs uas driver) or it makes sense to align > these two? This is something we can never really answer, because it would require knowing what bugs the vendors will eliminate in future versions of their firmware. For now I suggest making the minimum necessary changes, even if it means the two drivers are out of sync. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub
Felipe, On Thu, Nov 19, 2015 at 7:34 AM, Felipe Balbi wrote: > > Hi, > > Douglas Anderson writes: >> Until we have logic to determine which devices share the same TT let's >> add logic to assume that all devices on a given dwc2 controller are on >> one single_tt hub. This is better than the previous code that assumed >> that all devices were on one multi_tt hub, since single_tt hubs >> appear (in my experience) to be much more common and any schedule that >> would work on a single_tt hub will also work on a multi_tt hub. This >> will prevent more than 8 total low/full speed devices to be on the bus >> at one time, but that's a reasonable restriction until we've made things >> smarter. >> >> Signed-off-by: Douglas Anderson >> --- >> Changes in v3: >> - Assuming single_tt is new for v3; not terribly well tested (yet). >> >> Changes in v2: None >> >> drivers/usb/dwc2/core.h | 1 + >> drivers/usb/dwc2/hcd_queue.c | 40 +++- >> 2 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h >> index 567ee2c9e69f..09aa2b5ae29e 100644 >> --- a/drivers/usb/dwc2/core.h >> +++ b/drivers/usb/dwc2/core.h >> @@ -782,6 +782,7 @@ struct dwc2_hsotg { >> u16 periodic_usecs; >> unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC, >> BITS_PER_LONG)]; >> + bool has_split[8]; > > why don't you use a u8 instead then just set each bit for each > "has_split" you need to take care of. This array is kinda ugly. Let's actually drop this patch completely. Julius and I sat down and he talked me through things, and with my current understanding the current microframe scheduler in dwc2 is broken enough that small little band-aids like this will do little more than just push the problems around. I'm a good portion of the way through a better microframe scheduler. I have no doubt that it won't be perfect, but hopefully it will at least be based in reality... My latest thinking on the patches in this series: 1. usb: dwc2: rockchip: Make the max_transfer_size automatic I'll probably separate this out into its own patch so I can stop sending it as part of this series. ...or if someone wanted to land it then I won't bother. 2. usb: dwc2: host: Get aligned DMA in a more supported way Still can land any time and has good benefits. I believe that I can't separate this because it will cause conflicts with scheduler patches. 3. usb: dwc2: host: Add scheduler tracing Would be nice to land. 4. usb: dwc2: host: Rewrite the microframe scheduler 5. usb: dwc2: host: Keep track of and use our scheduled microframe 6. usb: dwc2: host: Assume all devices are on one single_tt hub Please drop patches 4-6 right now. 7. usb: dwc2: host: Add a delay before releasing periodic bandwidth 8. usb: dwc2: host: Giveback URB in tasklet context I'll probably move these back up in the series (like in v2) and put microframe rewrite atop them. With my current understanding the scheduling is so broken today that the concerns Alan brought up can wait until we have a proper scheduler to be addressed. In the meantime getting the huge boost in interrupt speed will help with dwc2's correctness (and performance) because it means we're much less likely to miss SOF interrupts. If anyone has any review time, giving a review to v2 of these patches would be helpful. Otherwise I'll double check that v2 still looks good with my current understanding and eventually post them again. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Udoo support for chipidea
On Thu, 19 Nov 2015, Philipp Zabel wrote: > On Wed, Oct 21, 2015 at 10:39:00AM +0800, Peter Chen wrote: > > On Tue, Oct 20, 2015 at 02:09:38PM -0200, Fabio Estevam wrote: > > > Hi Peter, > > > > > > On Mon, Oct 19, 2015 at 12:50 AM, Peter Chen > > > wrote: > > > > > > > Add linux-usb. > > > > > > > > Patryk, your problem is you may need to open 24M OSC for HUB 2514 > > > > manually, and you have used IMX6QDL_CLK_CKO for it in the design, > > > > but this clock is not controller's clock, controller's clock has > > > > already decided at SoC dts file (imx6qdl.dtsi), you don't need to > > > > override it at board's dts file. > > > > > > > > You can try delete clock property at imx6qdl-udoo.dtsi, if it still > > > > can't work, try to open IMX6QDL_CLK_CKO at one place to test. > > > > > > What is the appropriate place to acquire and enable the USB hub clock? > > > > > > This issue has appeared several times and it seems we don't have a > > > solution for this yet. > > > > > > Any suggestions? > > > > Add Alan. > > > > Hi Alan, we have several designs that the on-board HUB need to > > be reset by gpio pin and its clock is also from the board or > > the SoC. Any suggestions how to add these platform information > > for HUB device? > > How about putting it in the device tree? > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps > clocks and reset-gpios properties could be added to the USB hub node. Something like this is necessary. Instead of making the hub driver take care of the reset gpio and the clock, I suggest you make the host controller's platform driver do these things. This is because USB hubs are generic devices, not specific to any platform and (usually) hot-pluggable. Associating platform-specific data with a hub is out of the ordinary, and it deserves to be handled by platform-specific code -- there is no such code in the hub driver. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 6/8] usb: dwc2: host: Assume all devices are on one single_tt hub
Hi, Douglas Anderson writes: > Until we have logic to determine which devices share the same TT let's > add logic to assume that all devices on a given dwc2 controller are on > one single_tt hub. This is better than the previous code that assumed > that all devices were on one multi_tt hub, since single_tt hubs > appear (in my experience) to be much more common and any schedule that > would work on a single_tt hub will also work on a multi_tt hub. This > will prevent more than 8 total low/full speed devices to be on the bus > at one time, but that's a reasonable restriction until we've made things > smarter. > > Signed-off-by: Douglas Anderson > --- > Changes in v3: > - Assuming single_tt is new for v3; not terribly well tested (yet). > > Changes in v2: None > > drivers/usb/dwc2/core.h | 1 + > drivers/usb/dwc2/hcd_queue.c | 40 +++- > 2 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > index 567ee2c9e69f..09aa2b5ae29e 100644 > --- a/drivers/usb/dwc2/core.h > +++ b/drivers/usb/dwc2/core.h > @@ -782,6 +782,7 @@ struct dwc2_hsotg { > u16 periodic_usecs; > unsigned long periodic_bitmap[DIV_ROUND_UP(TOTAL_PERIODIC_USEC, > BITS_PER_LONG)]; > + bool has_split[8]; why don't you use a u8 instead then just set each bit for each "has_split" you need to take care of. This array is kinda ugly. > @@ -386,6 +409,13 @@ static int dwc2_find_multi_uframe(struct dwc2_hsotg > *hsotg, struct dwc2_qh *qh) > bitmap_set(hsotg->periodic_bitmap, start, qh->usecs); > qh->start_usecs = start; > > + if (qh->do_split) { > + for (i = start / EARLY_FRAME_USEC; > + i < DIV_ROUND_UP(start + utime - 1, EARLY_FRAME_USEC); > + i++) > + hsotg->has_split[i] = true; hsotg->has_split |= BIT(i); > @@ -546,6 +577,13 @@ static void dwc2_deschedule_periodic(struct dwc2_hsotg > *hsotg, > } > > bitmap_clear(hsotg->periodic_bitmap, start, utime); > + > + if (qh->do_split) { > + for (i = start / EARLY_FRAME_USEC; > + i < DIV_ROUND_UP(start + utime - 1, EARLY_FRAME_USEC); > + i++) > + hsotg->has_split[i] = false; hsotg->has_split &= ~BIT(i); -- balbi signature.asc Description: PGP signature
[PATCH] usb: gadget: pxa27x: fix suspend callback
pxa27x disconnects pullups on suspend but doesn't notify the gadget driver about it, so gadget driver can't disable the endpoints it was using. This causes problems on resume because gadget core will think endpoints are still enabled and just ignore the following usb_ep_enable(). Fix this problem by calling gadget_driver->disconnect(). Cc: # v3.10+ Signed-off-by: Felipe Balbi --- drivers/usb/gadget/udc/pxa27x_udc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c index 670ac0b12f00..a08ae19ca410 100644 --- a/drivers/usb/gadget/udc/pxa27x_udc.c +++ b/drivers/usb/gadget/udc/pxa27x_udc.c @@ -2535,6 +2535,7 @@ static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state) udc_disable(udc); udc->pullup_resume = udc->pullup_on; dplus_pullup(udc, 0); + udc->driver->disconnect(&udc->gadget); return 0; } -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: add generic OF glue layer
Hi, Felipe Balbi writes: > For simple platforms which merely enable some clocks > and populate its children, we can use this generic > glue layer to avoid boilerplate code duplication. > > For now this supports Qcom and Xilinx, but if we > find a way to add generic handling of regulators and > optional PHYs, we can absorb exynos as well. > > Signed-off-by: Felipe Balbi > --- > > Can you guys check if this works for your respective platforms ? If it does we > can some code duplication going forward. gentle ping on this one. -- balbi signature.asc Description: PGP signature
Re: scsi-sd fails with error "Invalid field in cdb" for SATA-to-USB adapter JMicron
On 2015-11-17 19:18, Alan Stern wrote: > That line is completely inappropriate for uas; it applies only to > usb-storage. Don't add it. I got it. My first thought was like you have said (every module uses its own structure), but I blindly tried to guess. > Here you need to test devinfo->flags & US_FL_BROKEN_FUA. Great, that worked! scsi host6: uas scsi 6:0:0:0: Direct-Access JMicron Generic 0116 PQ: 0 ANSI: 6 sd 6:0:0:0: Attached scsi generic sg2 type 0 sd 6:0:0:0: [sdb] 312581808 512-byte logical blocks: (160 GB/149 GiB) sd 6:0:0:0: [sdb] 4096-byte physical blocks sd 6:0:0:0: [sdb] Write Protect is off sd 6:0:0:0: [sdb] Mode Sense: 53 00 10 08 sd 6:0:0:0: [sdb] Disabling FUA sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA sd 6:0:0:0: [sdb] Attached SCSI disk >> The show-stopper for me is to know, how the us->fflags is initialized >> from list of "unusual devices". There is some magic in there, at least >> this is not something on the surface. I can't get how the definition of >> these unusual devices is shared between scsiglue.c and uas.c, basically, >> where is the code that matches the USB vendor/product and sets >> us->fflags. That code should be called before uas.c: >> uas_slave_configure():792. > > The code that sets devinfo->flags is in uas_probe(). The flags value > comes from uas_use_uas_driver() in uas_detect.h. The table used by uas > is stored in unusual_uas.h, not unusual_devs.h, so you'll have to add a > completely new entry there. > > Actually, you should modify both unusual_*.h files, because someone > might want to use that device with the usb-storage driver rather than > the uas driver. Actually there is an entry in unusual_uas.h but there is a minor difference: unusual_uas.h uses UNUSUAL_DEV(0x152d, 0x0567, 0x, 0x, ... unusual_devs.h uses UNUSUAL_DEV(0x152d, 0x0567, 0x0114, 0x0116, ... so uas module captures the wider set of devices (effectively ignores bcdDevice). Should it be left like that (then behaviour could be different for usb-storage vs uas driver) or it makes sense to align these two? -- With best regards, Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 19 November 2015 at 18:28, Peter Hurley wrote: > On 11/18/2015 09:35 PM, Baolin Wang wrote: >> On 18 November 2015 at 23:32, Peter Hurley wrote: >>> Hi Baolin, >>> >>> On 11/16/2015 02:05 AM, Baolin Wang wrote: It dose not work when we want to use the usb-to-serial port based on one usb gadget as a console. Thus this patch adds the console initialization to support this request. >>> >>> I have some high level concerns. >>> >>> 1. I would defer registering the console until the port has at least been >>>allocated in gserial_alloc_line(), and unregister when the line is freed. >>>That also reduces many of the conditions that you shouldn't need to >>>check, like port number range and so on. >> >> The 'setup' callback of 'struct console' is just do some memory >> allocation and member initialization, that no need to defer >> registering the console in gserial_alloc_line(). But the >> 'gs_console_connect()' which will use the port need to be called in >> gserial_connect(). > > My point here was why are you registering the console before the port > table is even allocated and initialized? The console can't possibly > perform i/o that early because the port doesn't even exist. > Which is why I suggested waiting until gserial_alloc_line() to > register the console. > > A typical console setup() performs the cross-reference linking between > the console data structure and the port table. The reason for that > is the console needs to be cleaned up if the port is being torn down. > > For example, in gserial_disconnect() the port->port_usb is reset to NULL, > and later in gserial_console_exit(): > > if (port && port->port_usb) { > > gs_request_free(req, ep); > } > > which means your leaking the request allocation when the port has been > disconnected. > Yeah, that's right. I'll defer the console registration until gserial_connect() and unregistration in disconnect. > >>>Further, consider deferring the console registration until >>> gserial_connect(); >>>that would further simplify things. In this case, unregistration would >>>happen at disconnect. >> >> That sounds reasonable. I would try. >> >>> >>> 2. Why are you using a kworker for actual device i/o when all of the i/o >>>is done with interrupts disabled anyway? >>>Getting rid of the work would eliminate using the 8K intermediate buffer >>>as well. >> >> If remove the kworker, there are some deadlocks to call the printk() >> when in the process of transferring data with usb endpoint. So we need >> a async kworker to complete the io or it can not work. > > The commit log should detail the major design choices, including why you > used the kworker (because of re-entrancy issues with usb endpoint). > OK. > > >>> 3. If for some reason i/o deferral is really necessary, consider using a >>> kthread >>>kworker instead of the pooled kworker. The scheduler response will be >>> _way_ >>>better. >>> >> >> OK, make sense. >> >>> 4. If for some reason i/o deferral is really necessary, use a circular >>> buffer >>>for the intermediate buffer, preferably lockless since there is only >>>one producer and one consumer. >>> >> >> Yeah, the circular buffer is better but more tricky. I would try. >> >>> Some other review comments below; please ignore anything other reviewers >>> have already noted. >>> >>> Regards, >>> Peter Hurley >>> Signed-off-by: Baolin Wang --- drivers/usb/gadget/Kconfig |6 + drivers/usb/gadget/function/u_serial.c | 238 2 files changed, 244 insertions(+) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 33834aa..be5aab9 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS a module parameter as well. If unsure, say 2. +config U_SERIAL_CONSOLE + bool "Serial gadget console support" + depends on USB_G_SERIAL + help +It supports the serial gadget can be used as a console. + source "drivers/usb/gadget/udc/Kconfig" # diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c index f7771d8..4ade527 100644 --- a/drivers/usb/gadget/function/u_serial.c +++ b/drivers/usb/gadget/function/u_serial.c @@ -27,6 +27,7 @@ #include #include #include +#include #include "u_serial.h" @@ -79,6 +80,16 @@ */ #define QUEUE_SIZE 16 #define WRITE_BUF_SIZE 8192/* TX only */ +#define GS_BUFFER_SIZE (4096) +#define GS_CONSOLE_BUF_SIZE (2 * GS_BUFFER_SIZE) + +struct gscons_info { + struct gs_port *port; + struct tty_driver
Re: [PATCH RESEND] USB: serial: cp210x: Add tx_empty()
On Wed, Nov 11, 2015 at 03:47:21PM -0600, Konstantin Shkolnyy wrote: Please make the commit message self-contained even if it means repeating what callback you're implementing here. > Without this function, when the port is closed the data in the chip's > transmit FIFO are lost. If the actual byte count is reported the close > can be delayed until all data are sent. You could mention that the tx_empty callback is used to implement generic wait-until-sent support. > Signed-off-by: Konstantin Shkolnyy > --- > drivers/usb/serial/cp210x.c | 60 > + > 1 file changed, 60 insertions(+) > > diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c > index e91b654..756e432 100644 > --- a/drivers/usb/serial/cp210x.c > +++ b/drivers/usb/serial/cp210x.c > @@ -38,6 +38,7 @@ static void cp210x_change_speed(struct tty_struct *, struct > usb_serial_port *, > struct ktermios *); > static void cp210x_set_termios(struct tty_struct *, struct usb_serial_port *, > struct ktermios*); > +static bool cp210x_tx_empty(struct usb_serial_port *port); > static int cp210x_tiocmget(struct tty_struct *); > static int cp210x_tiocmset(struct tty_struct *, unsigned int, unsigned int); > static int cp210x_tiocmset_port(struct usb_serial_port *port, > @@ -215,6 +216,7 @@ static struct usb_serial_driver cp210x_device = { > .close = cp210x_close, > .break_ctl = cp210x_break_ctl, > .set_termios= cp210x_set_termios, > + .tx_empty = cp210x_tx_empty, > .tiocmget = cp210x_tiocmget, > .tiocmset = cp210x_tiocmset, > .port_probe = cp210x_port_probe, > @@ -301,6 +303,18 @@ static struct usb_serial_driver * const serial_drivers[] > = { > #define CONTROL_WRITE_DTR0x0100 > #define CONTROL_WRITE_RTS0x0200 > > +/* CP210X_GET_COMM_STATUS returns these 0x13 bytes */ > +#define CP210X_COMM_STATUS_SIZE 0x13 > +struct cp210x_comm_status { > + __le32 ulErrors; > + __le32 ulHoldReasons; > + __le32 ulAmountInInQueue; > + __le32 ulAmountInOutQueue; > + u8 bEofReceived; > + u8 bWaitForImmediate; > + u8 bReserved; > +}; Add a __packed attribute here so that you can use sizeof and drop the size-define. > + > /* > * CP210X_PURGE - 16 bits passed in wValue of USB request. > * SiLabs app note AN571 gives a strange description of the 4 bits: > @@ -551,6 +565,52 @@ static void cp210x_close(struct usb_serial_port *port) > } > > /* > + * Read how many bytes are waiting in the TX queue. > + */ > +static int cp210x_get_tx_queue_byte_count(struct usb_serial_port *port, > + u32 *count) Indent continuation lines at least two tabs (also below). > +{ > + struct usb_serial *serial = port->serial; > + struct cp210x_port_private *port_priv = usb_get_serial_port_data(port); > + struct cp210x_comm_status *sts; > + int result; > + > + sts = kmalloc(CP210X_COMM_STATUS_SIZE, GFP_KERNEL); > + if (!sts) > + return -ENOMEM; > + > + result = usb_control_msg(serial->dev, usb_rcvctrlpipe(serial->dev, 0), > + CP210X_GET_COMM_STATUS, REQTYPE_INTERFACE_TO_HOST, 0x, Just use 0 here. > + port_priv->bInterfaceNumber, sts, CP210X_COMM_STATUS_SIZE, And sizeof(*sts) here (and below). > + USB_CTRL_GET_TIMEOUT); > + if (result == CP210X_COMM_STATUS_SIZE) { > + *count = le32_to_cpu(sts->ulAmountInOutQueue); > + result = 0; > + } else { > + dev_dbg(&port->dev, "%s error: size=%d result=%d\n", > + __func__, CP210X_COMM_STATUS_SIZE, result); This should be a dev_err, skip the __func__ and spell out the error (e.g. "failed to get comm status: %d\n"). No need to report the constant size. > + if (result >= 0) > + result = -EPROTO; > + } > + > + kfree(sts); > + > + return result; > +} Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] USB: serial: Another Infineon flash loader USB ID
On Mon, Nov 16, 2015 at 01:34:15PM +0100, Jonas Jonsson wrote: > This has been seen on a Telit UE910 modem. Please expand this message as well and mention why this is not a CDC device so we do not forget. > Signed-off-by: Jonas Jonsson > Tested-by: Daniele Palmas > --- > drivers/usb/serial/usb-serial-simple.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/usb-serial-simple.c > b/drivers/usb/serial/usb-serial-simple.c > index 3658662..93ab784 100644 > --- a/drivers/usb/serial/usb-serial-simple.c > +++ b/drivers/usb/serial/usb-serial-simple.c > @@ -53,7 +53,9 @@ DEVICE(funsoft, FUNSOFT_IDS); > > /* Infineon Flashloader driver */ > #define FLASHLOADER_IDS()\ > - { USB_DEVICE(0x8087, 0x0716) } > + { USB_DEVICE(0x8087, 0x0716) }, \ > + { USB_DEVICE_INTERFACE_CLASS(0x058b, 0x0041, 0x0a) } Please use USB_CLASS_CDC_DATA here. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] cdc_acm: Ignore Infineon Flash Loader utility
On Mon, Nov 16, 2015 at 01:34:14PM +0100, Jonas Jonsson wrote: > Some modems, such as the Telit UE910, are using an Infineon Flash Loader > utility. It has two interfaces, 2/2/0 (Abstract Modem) and 10/0/0 (CDC > Data). The latter can be used as a serial interface to upgrade the > firmware of the modem. However, that isn't possible when the cdc-acm > driver takes control of the device. Could you expand the commit message with some more information from the thread were Daniele explained how the flash loader works here? Specifically, that the device looks like a CDC-ACM device but really is not until after the flash loader has timed out. Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 11/18/2015 09:35 PM, Baolin Wang wrote: > On 18 November 2015 at 23:32, Peter Hurley wrote: >> Hi Baolin, >> >> On 11/16/2015 02:05 AM, Baolin Wang wrote: >>> It dose not work when we want to use the usb-to-serial port based >>> on one usb gadget as a console. Thus this patch adds the console >>> initialization to support this request. >> >> I have some high level concerns. >> >> 1. I would defer registering the console until the port has at least been >>allocated in gserial_alloc_line(), and unregister when the line is freed. >>That also reduces many of the conditions that you shouldn't need to >>check, like port number range and so on. > > The 'setup' callback of 'struct console' is just do some memory > allocation and member initialization, that no need to defer > registering the console in gserial_alloc_line(). But the > 'gs_console_connect()' which will use the port need to be called in > gserial_connect(). My point here was why are you registering the console before the port table is even allocated and initialized? The console can't possibly perform i/o that early because the port doesn't even exist. Which is why I suggested waiting until gserial_alloc_line() to register the console. A typical console setup() performs the cross-reference linking between the console data structure and the port table. The reason for that is the console needs to be cleaned up if the port is being torn down. For example, in gserial_disconnect() the port->port_usb is reset to NULL, and later in gserial_console_exit(): if (port && port->port_usb) { gs_request_free(req, ep); } which means your leaking the request allocation when the port has been disconnected. >>Further, consider deferring the console registration until >> gserial_connect(); >>that would further simplify things. In this case, unregistration would >>happen at disconnect. > > That sounds reasonable. I would try. > >> >> 2. Why are you using a kworker for actual device i/o when all of the i/o >>is done with interrupts disabled anyway? >>Getting rid of the work would eliminate using the 8K intermediate buffer >>as well. > > If remove the kworker, there are some deadlocks to call the printk() > when in the process of transferring data with usb endpoint. So we need > a async kworker to complete the io or it can not work. The commit log should detail the major design choices, including why you used the kworker (because of re-entrancy issues with usb endpoint). >> 3. If for some reason i/o deferral is really necessary, consider using a >> kthread >>kworker instead of the pooled kworker. The scheduler response will be >> _way_ >>better. >> > > OK, make sense. > >> 4. If for some reason i/o deferral is really necessary, use a circular buffer >>for the intermediate buffer, preferably lockless since there is only >>one producer and one consumer. >> > > Yeah, the circular buffer is better but more tricky. I would try. > >> Some other review comments below; please ignore anything other reviewers >> have already noted. >> >> Regards, >> Peter Hurley >> >>> Signed-off-by: Baolin Wang >>> --- >>> drivers/usb/gadget/Kconfig |6 + >>> drivers/usb/gadget/function/u_serial.c | 238 >>> >>> 2 files changed, 244 insertions(+) >>> >>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig >>> index 33834aa..be5aab9 100644 >>> --- a/drivers/usb/gadget/Kconfig >>> +++ b/drivers/usb/gadget/Kconfig >>> @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS >>> a module parameter as well. >>> If unsure, say 2. >>> >>> +config U_SERIAL_CONSOLE >>> + bool "Serial gadget console support" >>> + depends on USB_G_SERIAL >>> + help >>> +It supports the serial gadget can be used as a console. >>> + >>> source "drivers/usb/gadget/udc/Kconfig" >>> >>> # >>> diff --git a/drivers/usb/gadget/function/u_serial.c >>> b/drivers/usb/gadget/function/u_serial.c >>> index f7771d8..4ade527 100644 >>> --- a/drivers/usb/gadget/function/u_serial.c >>> +++ b/drivers/usb/gadget/function/u_serial.c >>> @@ -27,6 +27,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include "u_serial.h" >>> >>> @@ -79,6 +80,16 @@ >>> */ >>> #define QUEUE_SIZE 16 >>> #define WRITE_BUF_SIZE 8192/* TX only */ >>> +#define GS_BUFFER_SIZE (4096) >>> +#define GS_CONSOLE_BUF_SIZE (2 * GS_BUFFER_SIZE) >>> + >>> +struct gscons_info { >>> + struct gs_port *port; >>> + struct tty_driver *tty_driver; >>> + struct work_struct work; >>> + int buf_tail; >>> + charbuf[GS_CONSOLE_BUF_SIZE]; >>> +}; >> >> Make struct gscons_info a static declaration instead of >> allocating it. > > But I think the dynamic allocation is more reasonable with reducing >
[GIT PULL] USB-serial fixes for v4.4-rc2
Hi Greg, Here are a few fixes and new device ids for v4.4-rc2. All have been in linux-next for a few days now. Thanks, Johan The following changes since commit 8005c49d9aea74d382f474ce11afbbc7d7130bec: Linux 4.4-rc1 (2015-11-15 17:00:27 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git tags/usb-serial-4.4-rc2 for you to fetch changes up to 59536da34513c594af2a6fd35ba65ea45b6960a1: USB: qcserial: Fix support for HP lt4112 LTE/HSPA+ Gobi 4G Modem (2015-11-16 18:29:07 +0100) USB-serial fixes for v4.4-rc2 Here are some new device ids, support for an odd qcserial Gobi interface layout and a fix for the qcserial Huawei interface layout. Signed-off-by: Johan Hovold Aleksander Morgado (1): USB: serial: option: add support for Novatel MiFi USB620L Bjørn Mork (1): USB: qcserial: Fix support for HP lt4112 LTE/HSPA+ Gobi 4G Modem David Woodhouse (1): USB: ti_usb_3410_5052: Add Honeywell HGI80 ID Petr Štetiar (1): USB: qcserial: Add support for Quectel EC20 Mini PCIe module drivers/usb/serial/option.c | 2 + drivers/usb/serial/qcserial.c | 94 +++ drivers/usb/serial/ti_usb_3410_5052.c | 2 + drivers/usb/serial/ti_usb_3410_5052.h | 4 ++ 4 files changed, 82 insertions(+), 20 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/13] usb: dwc2: descriptor dma mode bug fixes
Hi Felipe, Can you take this serie or would you like me to resend it? BR, Gregory On Fri, Nov 06, 2015 at 12:02:42AM +0100, John Youn wrote: > On 11/5/2015 12:41 AM, Gregory Herrero wrote: > > Hi, > > > > This patchset contains bug fixes for host descriptor dma mode. > > > > Descriptor dma mode can't be used as the default mode since controller > > does not support split transfers in this mode. > > So we add a new configuration parameter which allows descriptor dma mode > > to be enabled for full-speed devices only. > > > > All patches are verified on dwc2 v3.0a with dedicated fifos and our > > main test target was usb audio devices. > > > > All patches are based on Felipe's testing/next branch. > > > > Regards, > > Gregory > > > > History: > > v2: > > - Use dma cache in "usb: dwc2: host: use kmem cache to allocate > > descriptors" > > and replace usage of deprecated GFP_DMA32 flag. > > - Remove "usb: dwc2: host: free status_buf on hcd de-init". > > > > v1: > > - Fix compilation error introduced by: > > "usb: dwc2: host: avoid usage of dma_alloc_coherent with irqs disabled" > > - Fix warning introduced by: > > "usb: dwc2: host: fix descriptor list address masking" > > > > Gregory Herrero (11): > > usb: dwc2: host: ensure filling of isoc desc is correctly done > > usb: dwc2: host: set active bit in isochronous descriptors > > usb: dwc2: host: rework isochronous halt path > > usb: dwc2: host: fix use of qtd after free in desc dma mode > > usb: dwc2: host: spinlock release channel > > usb: dwc2: host: add function to compare frame index > > usb: dwc2: host: program descriptor for next frame > > usb: dwc2: host: always increment available host channel during > > release > > usb: dwc2: host: process all completed urbs > > usb: dwc2: host: avoid usage of dma_alloc_coherent with irqs disabled > > usb: dwc2: host: use kmem cache to allocate descriptors > > > > Mian Yousaf Kaukab (2): > > usb: dwc2: host: enable descriptor dma for fs devices > > usb: dwc2: host: fix descriptor list address masking > > > > drivers/usb/dwc2/core.c | 37 +-- > > drivers/usb/dwc2/core.h | 26 + > > drivers/usb/dwc2/hcd.c | 76 +- > > drivers/usb/dwc2/hcd.h | 19 > > drivers/usb/dwc2/hcd_ddma.c | 240 > > +++ > > drivers/usb/dwc2/hcd_intr.c | 15 ++- > > drivers/usb/dwc2/hcd_queue.c | 2 +- > > drivers/usb/dwc2/hw.h| 4 - > > drivers/usb/dwc2/platform.c | 4 + > > 9 files changed, 367 insertions(+), 56 deletions(-) > > > > For this series: > > Acked-by: John Youn > > > I'm not able to test DDMA on host. But I checked that it didn't > break anything else. > > Regards, > John > > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 19 November 2015 at 17:36, Peter Hurley wrote: > On 11/19/2015 01:48 AM, Baolin Wang wrote: >>> +{ + struct gscons_info *info = gserial_cons.data; + int port_num = gserial_cons.index; + struct usb_request *req; + struct gs_port *port; + struct usb_ep *ep; + + if (port_num >= MAX_U_SERIAL_PORTS || port_num < 0) { + pr_err("%s: port num [%d] exceeds the range.\n", +__func__, port_num); + return -ENXIO; + } + + port = ports[port_num].port; + if (!port) { + pr_err("%s: serial line [%d] not allocated.\n", +__func__, port_num); + return -ENODEV; + } + + if (!port->port_usb) { + pr_err("%s: no port usb.\n", __func__); + return -ENODEV; + } + + ep = port->port_usb->in; + if (!ep) { + pr_err("%s: no usb endpoint.\n", __func__); + return -ENXIO; + } >>> >>> Looking at the caller, gserial_connect(), none of the error >>> conditions above look possible. >>> >> >> I re-look the code and do some tests, I found the checking is >> necessary. Cause we get the port number from the console->index, if >> the cmdline is not set the ttyGS0 as the console, the console->index >> will be -1 that is a wrong value. Also the serial.c file will create 1 >> usb-to-seial port as default (default n_ports = 1), so we need to >> check the port and the endpoint of the port. So I think here checking >> is necessary and I have tested it. > > static void gs_console_connect(int port_num) > { > . > . > if (port_num != gserial_cons.index) > return; > . OK. Thanks. > . > > > @@ -1219,6 +1453,7 @@ int gserial_connect(struct gserial *gser, u8 port_num) > gser->disconnect(gser); > } > > + status = gs_console_connect(port_num); > spin_unlock_irqrestore(&port->port_lock, flags); > > return status; > > > > > > > -- Baolin.wang Best Regards -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 11/19/2015 01:48 AM, Baolin Wang wrote: >> >>> +{ >>> + struct gscons_info *info = gserial_cons.data; >>> + int port_num = gserial_cons.index; >>> + struct usb_request *req; >>> + struct gs_port *port; >>> + struct usb_ep *ep; >>> + >>> + if (port_num >= MAX_U_SERIAL_PORTS || port_num < 0) { >>> + pr_err("%s: port num [%d] exceeds the range.\n", >>> +__func__, port_num); >>> + return -ENXIO; >>> + } >>> + >>> + port = ports[port_num].port; >>> + if (!port) { >>> + pr_err("%s: serial line [%d] not allocated.\n", >>> +__func__, port_num); >>> + return -ENODEV; >>> + } >>> + >>> + if (!port->port_usb) { >>> + pr_err("%s: no port usb.\n", __func__); >>> + return -ENODEV; >>> + } >>> + >>> + ep = port->port_usb->in; >>> + if (!ep) { >>> + pr_err("%s: no usb endpoint.\n", __func__); >>> + return -ENXIO; >>> + } >> >> Looking at the caller, gserial_connect(), none of the error >> conditions above look possible. >> > > I re-look the code and do some tests, I found the checking is > necessary. Cause we get the port number from the console->index, if > the cmdline is not set the ttyGS0 as the console, the console->index > will be -1 that is a wrong value. Also the serial.c file will create 1 > usb-to-seial port as default (default n_ports = 1), so we need to > check the port and the endpoint of the port. So I think here checking > is necessary and I have tested it. static void gs_console_connect(int port_num) { . . if (port_num != gserial_cons.index) return; . . @@ -1219,6 +1453,7 @@ int gserial_connect(struct gserial *gser, u8 port_num) gser->disconnect(gser); } + status = gs_console_connect(port_num); spin_unlock_irqrestore(&port->port_lock, flags); return status; -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Thu, 19 Nov 2015, Ioan-Adrian Ratiu wrote: > First part of lockdep report: > http://imgur.com/clLsCWe > > Second part: > http://imgur.com/Wa2PzRl > > Here are some printk's of mine while reproducing + debugging the issue: > http://imgur.com/SETOHT7 So the real problem is that Intuos driver is calling hid_hw_request() (which tries to grab the lock in usbhid_submit_report()) while handling the CTRL IRQ (lock gets acquired there). So the proper way to fix seems to be delaying the scheduling of the proximity read event in wacom_intuos_inout() to workqueue. > I'll continue to research this more in depth, but progress is slow > because I don't have much time, I'm doing this in my spare time because > it's my girlfriend's tablet. Oh, now I understand the level of severity of this bug! :-) Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/4] Documentation: usb: gadget-testing: add description for depth of queue
On 11/19/2015 08:02 AM, Peter Chen wrote: Add both bulk and iso depth of queue for sourcesink. Signed-off-by: Peter Chen Reviewed-by: Krzysztof Opasiak --- Documentation/usb/gadget-testing.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/usb/gadget-testing.txt b/Documentation/usb/gadget-testing.txt index b24d3ef..84b3d10 100644 --- a/Documentation/usb/gadget-testing.txt +++ b/Documentation/usb/gadget-testing.txt @@ -579,6 +579,8 @@ The SOURCESINK function provides these attributes in its function directory: isoc_mult - 0..2 (hs/ss only) isoc_maxburst - 0..15 (ss only) bulk_buflen - buffer length + bulk_qlen - depth of queue for bulk + iso_qlen- depth of queue for iso I'd prefer "queue length for bulk/iso requests" but I'm not native speaker or even English expert. For me, your form is also understandable so you may leave this as is, change or wait for another opinion, as you wish;) Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] usb: gadget: f_sourcesink: quit if usb_ep_queue returns error
On 11/19/2015 08:02 AM, Peter Chen wrote: Since now, we may have more than one request during the test, and it is better we just quit once the error occurs instead of try queueing further requests. Signed-off-by: Peter Chen Suggested-by: Krzysztof Opasiak Reviewed-by: Krzysztof Opasiak --- drivers/usb/gadget/function/f_sourcesink.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c index 9df4aa1..807af32 100644 --- a/drivers/usb/gadget/function/f_sourcesink.c +++ b/drivers/usb/gadget/function/f_sourcesink.c @@ -635,6 +635,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in, is_iso ? "ISO-" : "", is_in ? "IN" : "OUT", ep->name, status); free_ep_req(ep, req); + return status; } } Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] Doc: ABI: configfs-usb-gadget-sourcesink: add two entries for depth of queue
On 11/19/2015 08:02 AM, Peter Chen wrote: Add both bulk and iso depth of queue entries. Signed-off-by: Peter Chen Reviewed-by: Krzysztof Opasiak --- Documentation/ABI/testing/configfs-usb-gadget-sourcesink | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/ABI/testing/configfs-usb-gadget-sourcesink b/Documentation/ABI/testing/configfs-usb-gadget-sourcesink index bc7ff73..f56335a 100644 --- a/Documentation/ABI/testing/configfs-usb-gadget-sourcesink +++ b/Documentation/ABI/testing/configfs-usb-gadget-sourcesink @@ -10,3 +10,5 @@ Description: isoc_mult - 0..2 (hs/ss only) isoc_maxburst - 0..15 (ss only) buflen - buffer length + bulk_qlen - depth of queue for bulk + iso_qlen- depth of queue for iso The same as in previous patch. Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] usb: gadget: f_sourcesink: add queue depth
On 11/19/2015 08:02 AM, Peter Chen wrote: Add queue depth for both iso and bulk transfer, with more queues, we can do performance and stress test using sourcesink, and update g_zero accordingly. Signed-off-by: Peter Chen Reviewed-by: Krzysztof Opasiak Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote: > > > The critical section protected by usbhid->lock in hid_ctrl() is too > > > big and in rare cases causes a recursive deadlock because of its call > > > to hid_input_report(). > > > > > > This deadlock reproduces on newer wacom tablets like 056a:033c because > > > the wacom driver in its irq handler ends up calling hid_hw_request() > > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means > > > is that it submits a report to reschedule a proximity read through a > > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb) > > > before calling hid_input_report(). When the irq kicks in on the same > > > cpu, it also tries to grab the lock resulting in a recursive deadlock. > > > > > > The proper fix is to shrink the critical section in hid_ctrl() to > > > protect only the instructions which modify usbhid, thus move the lock > > > after the hid_input_report() call and the deadlock dissapears. > > > > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), > > isn't it? > > That was my first attempt, yes, but the deadlock still happens with interrupts > disabled. That unfortunately however directly implies that your explanation above isn't actually correct description of the real problem. So we'd better first understand the problem rather than papering it over with more or less random fixes. First, have you tried to run your usecase on your system with lockdep enabled? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html