[PATCH] xhci: use correct flags for spin_lock_irqrestore() when setting port power
commit a6ff6cbf1fab ("usb: xhci: Add helper function xhci_set_power_on().") created a helper to control port power that needs to be called with xhci->lock held and interrupts disabled. It relased the lock with spin_unlock_irqerstore using a new zero flag variable instead of the origial flag from spin_lock_irqsave. This regression triggered a static checker warning about bogus flags, and a null pointer dereference on armada-385. Fix it by passing a pointer to the correct flags and using it instead Fixes: a6ff6cbf1fab ("usb: xhci: Add helper function xhci_set_power_on().") Cc: Guoqing Zhang Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci-hub.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c index ab818bd..5e3e9d4 100644 --- a/drivers/usb/host/xhci-hub.c +++ b/drivers/usb/host/xhci-hub.c @@ -552,11 +552,10 @@ static __le32 __iomem *xhci_get_port_io_addr(struct usb_hcd *hcd, int index) * method. */ static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, - u16 index, bool on) + u16 index, bool on, unsigned long *flags) { __le32 __iomem *addr; u32 temp; - unsigned long flags = 0; addr = xhci_get_port_io_addr(hcd, index); temp = readl(addr); @@ -572,13 +571,13 @@ static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd *hcd, writel(temp & ~PORT_POWER, addr); } - spin_unlock_irqrestore(&xhci->lock, flags); + spin_unlock_irqrestore(&xhci->lock, *flags); temp = usb_acpi_power_manageable(hcd->self.root_hub, index); if (temp) usb_acpi_set_power_state(hcd->self.root_hub, index, on); - spin_lock_irqsave(&xhci->lock, flags); + spin_lock_irqsave(&xhci->lock, *flags); } static void xhci_port_set_test_mode(struct xhci_hcd *xhci, @@ -598,7 +597,7 @@ static void xhci_port_set_test_mode(struct xhci_hcd *xhci, } static int xhci_enter_test_mode(struct xhci_hcd *xhci, - u16 test_mode, u16 wIndex) + u16 test_mode, u16 wIndex, unsigned long *flags) { int i, retval; @@ -614,10 +613,10 @@ static int xhci_enter_test_mode(struct xhci_hcd *xhci, xhci_dbg(xhci, "Disable all port (PP = 0)\n"); /* Power off USB3 ports*/ for (i = 0; i < xhci->num_usb3_ports; i++) - xhci_set_port_power(xhci, xhci->shared_hcd, i, false); + xhci_set_port_power(xhci, xhci->shared_hcd, i, false, flags); /* Power off USB2 ports*/ for (i = 0; i < xhci->num_usb2_ports; i++) - xhci_set_port_power(xhci, xhci->main_hcd, i, false); + xhci_set_port_power(xhci, xhci->main_hcd, i, false, flags); /* Stop the controller */ xhci_dbg(xhci, "Stop controller\n"); retval = xhci_halt(xhci); @@ -1209,7 +1208,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, * However, hub_wq will ignore the roothub events until * the roothub is registered. */ - xhci_set_port_power(xhci, hcd, wIndex, true); + xhci_set_port_power(xhci, hcd, wIndex, true, &flags); break; case USB_PORT_FEAT_RESET: temp = (temp | PORT_RESET); @@ -1254,7 +1253,8 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, goto error; if (test_mode > TEST_FORCE_EN || test_mode < TEST_J) goto error; - retval = xhci_enter_test_mode(xhci, test_mode, wIndex); + retval = xhci_enter_test_mode(xhci, test_mode, wIndex, + &flags); break; default: goto error; @@ -1322,7 +1322,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, port_array[wIndex], temp); break; case USB_PORT_FEAT_POWER: - xhci_set_port_power(xhci, hcd, wIndex, false); + xhci_set_port_power(xhci, hcd, wIndex, false, &flags); break; case USB_PORT_FEAT_TEST: retval = xhci_exit_test_mode(xhci); -- 1.9.1 -- 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: [REGRESSION next-20170410] Commit a6ff6cbf1fab ("usb: xhci: Add helper function xhci_set_power_on().") breaks armada-385
On 12.04.2017 10:47, Ralph Sennhauser wrote: Hi Guoqing Zhang, Commit a6ff6cbf1fabe7500d8ac25e133e3346db0a0fca ("usb: xhci: Add helper function xhci_set_power_on().") causes a null pointer dereference on an armada-385. Below the kernel as well as the bisect log. Seems Guoqing Zhang is not currently reachable I'll send a fix for this, adding you to cc. Can you check if it solves the issue on armada-385? Thanks -Mathias -- 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: [bug report] usb: xhci: Add helper function xhci_set_power_on().
Hi On 12.04.2017 01:06, Dan Carpenter wrote: Hello Guoqing Zhang, The patch a6ff6cbf1fab: "usb: xhci: Add helper function xhci_set_power_on()." from Apr 7, 2017, leads to the following static checker warning: Seems Guoqing Zhang is currently not reachable I'll post a fix for this on the list shortly, adding you to cc -Mathias -- 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/3] usb: dwc3: gadget: Prevent losing events in event cache
Hi Felipe, On 4/11/2017 11:31 PM, Felipe Balbi wrote: Hi, (Thinh, for whatever I didn't receive your email via the list, replying to myself) Could it be because of the attached file? Felipe Balbi writes: Hmm, can you apply this patch and provide output when the failure happens? I suggest setting up a 100MiB trace buffer. You don't need to enable any tracers nor any event. trace_printk() is always enabled. Sure. Attached is a 7z with the full kernel trace with the failure. Thank you :-) More often than not, we will see something like below, where the TH coming in back to back with 0 event count on the second TH. 2990.496588: dwc3_interrupt: TH 2990.496588: dwc3_interrupt: evt->flags 2990.496590: dwc3_readl: addr c90001b2840c value 0004 2990.496590: dwc3_interrupt: GEVNTCOUNT 0004 2990.496590: dwc3_interrupt: 4 events pending 2990.496591: dwc3_readl: addr c90001b28408 value 1000 2990.496591: dwc3_interrupt: GEVNTSIZ 1000 2990.496592: dwc3_writel: addr c90001b28408 value 80001000 2990.496592: dwc3_interrupt: copying events to cache 2990.496592: dwc3_writel: addr c90001b2840c value 0004 2990.496614: dwc3_interrupt: TH 2990.496614: dwc3_interrupt: evt->flags 0001 2990.496615: dwc3_readl: addr c90001b2840c value 2990.496615: dwc3_interrupt: GEVNTCOUNT 2990.496616: dwc3_interrupt: 0 events pending 2990.496616: dwc3_thread_interrupt: BH 2990.496616: dwc3_event: event (6084): ep1out: Transfer In-Progress Here is the snippet of the attached trace where the TH is called twice with a new event coming in and overwrite the old one: 2990.601357: dwc3_readl: addr c90001b2883c value 00030007 2990.601357: dwc3_gadget_ep_cmd: ep1in: cmd 'Update Transfer' [196615].. 2990.601360: dwc3_interrupt: TH 2990.601360: dwc3_interrupt: evt->flags 2990.601362: dwc3_readl: addr c90001b2840c value 0004 2990.601362: dwc3_interrupt: GEVNTCOUNT 0004 2990.601362: dwc3_interrupt: 4 events pending 2990.601364: dwc3_readl: addr c90001b28408 value 1000 2990.601364: dwc3_interrupt: GEVNTSIZ 1000 2990.601364: dwc3_writel: addr c90001b28408 value 80001000 2990.601364: dwc3_interrupt: copying events to cache 2990.601364: dwc3_writel: addr c90001b2840c value 0004 2990.601369: dwc3_interrupt: TH 2990.601369: dwc3_interrupt: evt->flags 0001 2990.601369: dwc3_readl: addr c90001b2840c value 0004 2990.601370: dwc3_interrupt: GEVNTCOUNT 0004 2990.601370: dwc3_interrupt: 4 events pending 2990.601372: dwc3_readl: addr c90001b28408 value 80001000 2990.601372: dwc3_interrupt: GEVNTSIZ 80001000 2990.601372: dwc3_writel: addr c90001b28408 value 80001000 2990.601372: dwc3_interrupt: copying events to cache 2990.601372: dwc3_writel: addr c90001b2840c value 0004 2990.601375: dwc3_thread_interrupt: BH 2990.601376: dwc3_event: event (4086): ep1in: Transfer In-Progress.. The device hangs at the end of the log. I repeated the test with USB 3.0 IP version 2.90a and USB 3.1 IP version 1.60, and I found the same result where the TH is occasionally called twice back-to-back. Right, found this in the logs. So the only thing that I can think of here, is that we need to flush posted writes. Does this help? diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 79e7a3480d51..6f06738273f2 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -3099,6 +3099,10 @@ static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); + /* flush posted writes */ + dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); + dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); + return IRQ_WAKE_THREAD; } With the change above, I don't see the TH get called twice in in a row. However, I also tested with replacing the code above with a udelay(4), from reading the kernel trace, to take in account the latency of reading the registers. I also don't see TH get called twice in a row. The extra latency from reading the registers above can mask this issue. BR, Thinh Nguyen -- 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] smsc95xx: Add comments to the registers definition
> I based my comments on the datasheet. For the LED_GPIO_CFG register, the > datasheet says: > > This register configures the external GPIO[2:0] pins. > > QFN package description also indicates GPIOs 0, 1 & 2. > As an example for the LAN9514, pin 22 of the QFN indicates: > > nSPD_LED/GPIO2 > > In LED_GPIO_CFG register, GPCTL2 description indicates: > > The value of this field determines the function of the external GPIO2 > > pin as follows > > Do you confirm it's actually GPIO 10, 9 and 8? > I think I may have misunderstood something. Sorry forgetting that you are referring RPi which uses LAN9514. Because these LEDs' GPIO can vary per chip (LAN9500, 9514..), it would be better not putting GPIO number. LAN9500 are GPIO 10/9/8 as described. > While we are here, could you indicate the meaning of the bit 2 of > HW_CFG register (it's named HW_CFG_PSEL_)? It's the only bit I didn't > succeed to comment because I didn't find it in the datasheet. > I will then add it to the patch! It indicates internal & external phy, PSEL means PHY Select. You can find at LAN9500 doc in http://ww1.microchip.com/downloads/en/DeviceDoc/1875C.pdf. > I'm also wondering what the meaning of STRAP_STATUS is. I could also > comment it if you or Steve can provide the information. It is marked as reserved in above LAN9500 manual. You may guess from configuration straps in the manual and define names. Woojung -- 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] xhci: switch to pci_alloc_irq_vectors
Use the modern API to request MSI or MSI-X interrupts, which allows us to get rid of the msix_entries array, as well as cleaning up the cleanup code. Signed-off-by: Christoph Hellwig --- drivers/usb/host/xhci.c | 99 ++--- drivers/usb/host/xhci.h | 1 - 2 files changed, 28 insertions(+), 72 deletions(-) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 953fd8f62df0..5498271f4af6 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -217,20 +217,6 @@ int xhci_reset(struct xhci_hcd *xhci) } #ifdef CONFIG_PCI -static int xhci_free_msi(struct xhci_hcd *xhci) -{ - int i; - - if (!xhci->msix_entries) - return -EINVAL; - - for (i = 0; i < xhci->msix_count; i++) - if (xhci->msix_entries[i].vector) - free_irq(xhci->msix_entries[i].vector, - xhci_to_hcd(xhci)); - return 0; -} - /* * Set up MSI */ @@ -239,8 +225,8 @@ static int xhci_setup_msi(struct xhci_hcd *xhci) int ret; struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); - ret = pci_enable_msi(pdev); - if (ret) { + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI); + if (ret < 0) { xhci_dbg_trace(xhci, trace_xhci_dbg_init, "failed to allocate MSI entry"); return ret; @@ -251,35 +237,13 @@ static int xhci_setup_msi(struct xhci_hcd *xhci) if (ret) { xhci_dbg_trace(xhci, trace_xhci_dbg_init, "disable MSI interrupt"); - pci_disable_msi(pdev); + pci_free_irq_vectors(pdev); } return ret; } /* - * Free IRQs - * free all IRQs request - */ -static void xhci_free_irq(struct xhci_hcd *xhci) -{ - struct pci_dev *pdev = to_pci_dev(xhci_to_hcd(xhci)->self.controller); - int ret; - - /* return if using legacy interrupt */ - if (xhci_to_hcd(xhci)->irq > 0) - return; - - ret = xhci_free_msi(xhci); - if (!ret) - return; - if (pdev->irq > 0) - free_irq(pdev->irq, xhci_to_hcd(xhci)); - - return; -} - -/* * Set up MSI-X */ static int xhci_setup_msix(struct xhci_hcd *xhci) @@ -298,28 +262,17 @@ static int xhci_setup_msix(struct xhci_hcd *xhci) xhci->msix_count = min(num_online_cpus() + 1, HCS_MAX_INTRS(xhci->hcs_params1)); - xhci->msix_entries = - kmalloc((sizeof(struct msix_entry))*xhci->msix_count, - GFP_KERNEL); - if (!xhci->msix_entries) - return -ENOMEM; - - for (i = 0; i < xhci->msix_count; i++) { - xhci->msix_entries[i].entry = i; - xhci->msix_entries[i].vector = 0; - } - - ret = pci_enable_msix_exact(pdev, xhci->msix_entries, xhci->msix_count); - if (ret) { + ret = pci_alloc_irq_vectors(pdev, xhci->msix_count, xhci->msix_count, + PCI_IRQ_MSIX); + if (ret < 0) { xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Failed to enable MSI-X"); - goto free_entries; + return ret; } for (i = 0; i < xhci->msix_count; i++) { - ret = request_irq(xhci->msix_entries[i].vector, - xhci_msi_irq, - 0, "xhci_hcd", xhci_to_hcd(xhci)); + ret = request_irq(pci_irq_vector(pdev, i), xhci_msi_irq, 0, + "xhci_hcd", xhci_to_hcd(xhci)); if (ret) goto disable_msix; } @@ -329,11 +282,9 @@ static int xhci_setup_msix(struct xhci_hcd *xhci) disable_msix: xhci_dbg_trace(xhci, trace_xhci_dbg_init, "disable MSI-X interrupt"); - xhci_free_irq(xhci); - pci_disable_msix(pdev); -free_entries: - kfree(xhci->msix_entries); - xhci->msix_entries = NULL; + while (--i >= 0) + free_irq(pci_irq_vector(pdev, i), xhci_to_hcd(xhci)); + pci_free_irq_vectors(pdev); return ret; } @@ -346,27 +297,33 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci) if (xhci->quirks & XHCI_PLAT) return; - xhci_free_irq(xhci); + /* return if using legacy interrupt */ + if (hcd->irq > 0) + return; + + if (hcd->msix_enabled) { + int i; - if (xhci->msix_entries) { - pci_disable_msix(pdev); - kfree(xhci->msix_entries); - xhci->msix_entries = NULL; + for (i = 0; i < xhci->msix_count; i++) + free_irq(pci_irq_vector(pdev, i), xhci_to_hcd(xhci)); } else { - pci_disable_msi(pdev); + free_irq(pci_irq_vector(pdev, 0), xhci
Re: [PATCH v3] smsc95xx: Add comments to the registers definition
On 12/04/17 14:25, woojung@microchip.com wrote: > > +/* LED General Purpose IO Configuration Register */ > > +#define LED_GPIO_CFG (0x24) > > +#define LED_GPIO_CFG_SPD_LED (0x0100)/* GPIO2 as SPD LED > > */ > > +#define LED_GPIO_CFG_LNK_LED (0x0010)/* GPIO1 as LNK LED > > */ > > +#define LED_GPIO_CFG_FDX_LED (0x0001)/* GPIO0 as FDX LED > > */ > Because comments are one of main reason for this patch. > It is GPIO 10, 9 & 8. > > Woojung Hello Woojung, I based my comments on the datasheet. For the LED_GPIO_CFG register, the datasheet says: > This register configures the external GPIO[2:0] pins. QFN package description also indicates GPIOs 0, 1 & 2. As an example for the LAN9514, pin 22 of the QFN indicates: > nSPD_LED/GPIO2 In LED_GPIO_CFG register, GPCTL2 description indicates: > The value of this field determines the function of the external GPIO2 > pin as follows Do you confirm it's actually GPIO 10, 9 and 8? I think I may have misunderstood something. While we are here, could you indicate the meaning of the bit 2 of HW_CFG register (it's named HW_CFG_PSEL_)? It's the only bit I didn't succeed to comment because I didn't find it in the datasheet. I will then add it to the patch! I'm also wondering what the meaning of STRAP_STATUS is. I could also comment it if you or Steve can provide the information. Thank you! Martin -- 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: v4.10-rc8 (-rc6) boot regression on Intel desktop, does not boot after cold boots, boots after reboot
On Mon, Apr 03, 2017 at 08:20:50PM +0200, Pavel Machek wrote: > > > > > > ...1d.7: PCI fixup... pass 2 > > > > > > ...1d.7: PCI fixup... pass 3 > > > > > > ...1d.7: PCI fixup... pass 3 done > > > > > > > > > > > > ...followed by hang. So yes, it looks USB related. > > > > > > > > > > > > (Sometimes it hangs with some kind backtrace involving secondary CPU > > > > > > startup, unfortunately useful info is off screen at that point). > > > > > > > > > > Forgot to say, 1d.7 is EHCI controller. > > > > > > > > > > 00:1d.7 USB controller: Intel Corporation NM10/ICH7 Family USB2 EHCI > > > > > Controller (rev 01) > > > > > > > > Ok, I should have access soon to a EeePc 1015CX (which seem to have > > > > this controller). > > > > I hope I'll be able to reproduce the issue there. If not, I'm sorry but > > > > I'll have to > > > > burden you again :-) > > > > > > Go through more mails. It is only reproducible after cold boot. .. so > > > I doubt it will be easy to reproduce on another machine. > > > > > > Now... I do have serial port, and I even might have serial cable > > > somewhere, but Giving how sensitive it is, it is probably going to > > > go away with console on ttyS... > > > > I also tried on an eeepc (which has ICH7/NM10 as well), with your config. > > I even plugged a usb keyboard but even then I have been unable to > > reproduce either :-( > > Ok, give me some time. I'm no longer using the affected machine, so no > promises. Actually someone reported me a very similar issue than yours lately. It's probably the same. And I have a potential fix. The scenario is a bit tricky again, and still theoretical. If you're interested in gory details: a tick which is scheduled at jiffies = N + 1, in order to expire a timer_list timer, fires a tiny bit too early (ie: very few microseconds in advance). So it doesn't update the jiffies on irq entry and still sees jiffies = N. The timer_list timer doesnt expire yet and on IRQ exit we reschedule the tick at the same time. But we see that ts->next_tick already has that value, therefore we don't reprogram it again, leaving the clockevent unprogrammed. So in case you have the time and opportunity to test the fix, you'll need to: 1) Revert back to the offending change: git revert 558e8e27e73f53f8a512485be538b07115fe5f3c 2) Apply a delta fix: diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index a3b8154..ae66515 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -1071,8 +1071,10 @@ static void tick_nohz_handler(struct clock_event_device *dev) tick_sched_handle(ts, regs); /* No need to reprogram if we are running tickless */ - if (unlikely(ts->tick_stopped)) + if (unlikely(ts->tick_stopped)) { + ts->next_tick = 0; return; + } hrtimer_forward(&ts->sched_timer, now, tick_period); tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1); @@ -1172,8 +1174,10 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer) tick_sched_handle(ts, regs); /* No need to reprogram if we are in idle or full dynticks mode */ - if (unlikely(ts->tick_stopped)) + if (unlikely(ts->tick_stopped)) { + ts->next_tick = 0; return HRTIMER_NORESTART; + } hrtimer_forward(timer, now, tick_period); Thanks! diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index a3b8154..ae66515 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -1071,8 +1071,10 @@ static void tick_nohz_handler(struct clock_event_device *dev) tick_sched_handle(ts, regs); /* No need to reprogram if we are running tickless */ - if (unlikely(ts->tick_stopped)) + if (unlikely(ts->tick_stopped)) { + ts->next_tick = 0; return; + } hrtimer_forward(&ts->sched_timer, now, tick_period); tick_program_event(hrtimer_get_expires(&ts->sched_timer), 1); @@ -1172,8 +1174,10 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer) tick_sched_handle(ts, regs); /* No need to reprogram if we are in idle or full dynticks mode */ - if (unlikely(ts->tick_stopped)) + if (unlikely(ts->tick_stopped)) { + ts->next_tick = 0; return HRTIMER_NORESTART; + } hrtimer_forward(timer, now, tick_period);
Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache
On Wed, Apr 12, 2017 at 1:13 AM, Felipe Balbi wrote: > > Hi, > > John Youn writes: John Youn writes: >> Thinh Nguyen writes: >>> The dwc3 driver can overwite its previous events if its top half IRQ >>> handler gets invoked again before processing the events in the cache. We >> >> interrupts are masked, why would top half get invoked again? Is this, >> perhaps, related to DWC3 3.00a which has the "Interrupt line doesn't >> lower when masked" problem? We've added a lot of code to workaround that >> problem and, apparently, it wasn't enough. > > No, it is not related to that. We verified with PCIe traces. The > interrupt line gets deasserted after we mask it. And we put the > masking as close to the beginning of the top-half as possible. > >> >> In any case, there's no way top half would be invoked again in a >> properly working DWC3. > > Yet we still see it sometimes. Usually it doesn't create a problem, that's fair, but it's not for the reason you're describing :-) There might be another problem going on, because since we masked the interrupt and cleared all events, IRQ shouldn't be raised at all; unless, as I mentioned on the other subthread, the IRQ line is shared. > but if there happens to be a new event there, we get the failure. > > We didn't trace into that part of the kernel so we can't explain why. > But if there is any chance the interrupt line deassertion wasn't > detected in time, whatever part of the kernel that thinks it is still > asserted might just call our top-half again. This could be a totally > wrong assumption, but it doesn't seem too far-fetched. The kernel doesn't detect IRQ line assertion/deassertion. CPU gets an exception when that happens and calls Kernel IRQ handler vector. That will, in turn, figure out which line triggered, call the handler and so on. >>> >>> We're talking about PCIe though, where interrupt assertion and >>> deassertion are packets. So I would imagine the kernel has to do >>> something and there could be some latency associated with that. >> >> Also, another thing is that the device uses legacy, level-triggered, >> PCIe interrupts, so for as long as the interrupt is asserted, the TH >> is called repeatedly. > > yes, and that's why we have: > >> static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> { >> struct dwc3 *dwc = evt->dwc; >> u32 amount; >> u32 count; >> u32 reg; > >> if (pm_runtime_suspended(dwc->dev)) { >> pm_runtime_get(dwc->dev); >> disable_irq_nosync(dwc->irq_gadget); >> dwc->pending_events = true; >> return IRQ_HANDLED; >> } >> >> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); >> count &= DWC3_GEVNTCOUNT_MASK; > > check how many events are pending in the event buffer. > >> if (!count) >> return IRQ_NONE; >> >> evt->count = count; >> evt->flags |= DWC3_EVENT_PENDING; >> >> /* Mask interrupt */ >> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >> reg |= DWC3_GEVNTSIZ_INTMASK; > > mask interrupt generation > >> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg); >> >> amount = min(count, evt->length - evt->lpos); >> memcpy(evt->cache + evt->lpos, evt->buf + evt->lpos, amount); >> >> if (amount < count) >> memcpy(evt->cache, evt->buf, count - amount); >> >> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); > > clear ALL events from event buffer. This brings the line down, so we > shouldn't re-enter. > >> return IRQ_WAKE_THREAD; >> } > >> So we mask the interrupt in the TH and a short time later, the >> interrupt de-assertion packet is sent on PCIe bus and if that's not >> seen right away we may already have another call to TH before the BH >> gets scheduled. > > not sure this can happen. If that's the case, every PCI driver would > have all sorts of tricks to cope with this, not only dwc3 :-) > > Bjorn, is this something that can happen on PCIe? I'm pretty sure that the device will send Deassert_INTx *after* the driver tells the device to stop interrupting. That should eventually result in deassertion of the level-triggered interrupt. I can't speak to the mechanics of interrupt masking and the IRQ subsystem. I do think all IRQ handlers should be prepared to handle spurious interrupts gracefully. > Quick summary of the problem: > > John and Thinh are experiencing a re-entrant top-half handler even > though we have cleared pending IRQ status _and_ masked Interrupts. SNPS > is using an FPGA model of the latest DWC3 core under x86. > > I have never seen this behavior on ARM or any of the x86 devices > containing this core (and this includes all the newest x86 cores, see > drivers/usb/dwc3/dwc3-pci.c for PCI IDs if you care enough :-) > > Anyway, from my point of view, th
Re: [PATCH v3] smsc95xx: Add comments to the registers definition
From: Martin Wetterwald Date: Wed, 12 Apr 2017 11:24:05 +0200 > This chip is used by a lot of embedded devices and also by the Raspberry > Pi 1, 2 & 3 which were created to promote the study of computer > sciences. Students wanting to learn kernel / network device driver > programming through those devices can only rely on the Linux kernel > driver source to make their own. > > This commit adds a lot of comments to the registers definition to expand > the register names. > > Cc: Steve Glendinning > Cc: Microchip Linux Driver Support > CC: David Miller > Signed-off-by: Martin Wetterwald > --- > Changes in v3: > * Restore previous name for TX_CMD_B_CSUM_ENABLE Please fix the GPIO numbers as indicated by Woojung Huh. -- 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 1/3] usb: udc: allow adding and removing the same gadget device
On Wed, 12 Apr 2017, Felipe Balbi wrote: > >> Maybe... But I can't shake the feeling that Greg KH would strongly > >> disagree. Hasn't he said, many times in the past, that any dynamically > >> allocated device structure _must_ have a real release routine? > >> usb_udc_nop_release() doesn't qualify. > > > > Aw, I wanted to publically yell at someone like the kernel documentation > > says I am allowed to do so if anyone does such a foolish thing :) > > heh, except that we're not dynamically allocating struct device at all > :-) Here's what we have for most UDCs (net2280.c included): > > struct my_udc { > struct gadget gadget; > [...] > }; > > probe() > { > struct my_udc *u; > > u = kzalloc(sizeof(*u), GFP_KERNEL); > [...] > return 0; > } Allow me to point out that the struct device is embedded inside the struct gadget (actually struct usb_gadget) embedded inside the struct my_udc, which _is_ dynamically allocated. Therefore the struct device is located in dynamically allocated memory. > Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't > this result on a functionally equivalent execution to the patch I > proposed above? It would, and it would be equally wrong. 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] smsc95xx: Add comments to the registers definition
> +/* LED General Purpose IO Configuration Register */ > +#define LED_GPIO_CFG (0x24) > +#define LED_GPIO_CFG_SPD_LED (0x0100)/* GPIO2 as SPD LED > */ > +#define LED_GPIO_CFG_LNK_LED (0x0010)/* GPIO1 as LNK LED > */ > +#define LED_GPIO_CFG_FDX_LED (0x0001)/* GPIO0 as FDX LED > */ Because comments are one of main reason for this patch. It is GPIO 10, 9 & 8. Woojung -- 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] chipidea: Fix issue in reconnecing gadget without insmod/rmmod
On Wed, Apr 12, 2017 at 5:21 PM, Peter Chen wrote: > On Tue, Apr 11, 2017 at 08:46:24PM +0530, Niranjan Dighe wrote: >> Currently usb_gadget_connect() is called only through gadget >> registration via composite_driver_probe(). As a result, after a >> disconnection, if the role transitions to host and back to gadget, >> the gadget is not recognized by the host anymore. >> This is a typical scenario with an iAP device in the following >> usecase - >> conn iAP dev -> send roleswitch command -> roleswitch ourself >> to gadget -> disconnect iAP device -> roleswitch back to host to >> enumerate the device again -> reconnect device -> resend the >> roleswitch command -> roleswitch ourself to gadget -> ISSUE. >> >> To workaround this, do the following - >> >> 1. Restart OTG FSM on SLi interrupt and on switching role to gadget >> so that device transitions to B_IDLE state from B_PERIPHERAL state. >> A transition from B_IDLE to B_PERIPHERAL is needed to enable >> interrups and restore the correct state of the chipidea controller >> so that communication with host is possible >> >> 2. usb_gadget_connect() after roleswitch to gadget so that >> gadget->ops->pullup() is called and D+ line is asserted. This >> causes host to "see" the device and enumeration can happen. >> >> Signed-off-by: Niranjan Dighe >> --- >> drivers/usb/chipidea/debug.c | 10 +- >> drivers/usb/chipidea/udc.c | 3 +++ >> 2 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c >> index a4f7db2..66c1485 100644 >> --- a/drivers/usb/chipidea/debug.c >> +++ b/drivers/usb/chipidea/debug.c >> @@ -16,7 +16,7 @@ >> #include "udc.h" >> #include "bits.h" >> #include "otg.h" >> - >> +extern void ci_hdrc_otg_fsm_restart(struct ci_hdrc *); >> /** >> * ci_device_show: prints information about device capabilities and status >> */ >> @@ -325,6 +325,14 @@ static ssize_t ci_role_write(struct file *file, const >> char __user *ubuf, >> ci_role_stop(ci); >> ret = ci_role_start(ci, role); >> enable_irq(ci->irq); >> + >> + /* REVISIT - Avoid repeated FSM restart*/ >> + >> + if (role == CI_ROLE_GADGET) { >> + ci_hdrc_otg_fsm_restart(ci); >> + usb_gadget_connect(&ci->gadget); >> + } >> + >> pm_runtime_put_sync(ci->dev); >> >> return ret ? ret : count; >> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> index a0accd5..1e3b827 100644 >> --- a/drivers/usb/chipidea/udc.c >> +++ b/drivers/usb/chipidea/udc.c >> @@ -29,6 +29,8 @@ >> #include "otg.h" >> #include "otg_fsm.h" >> >> +extern void ci_hdrc_otg_fsm_restart(struct ci_hdrc *); >> + >> /* control endpoint description */ >> static const struct usb_endpoint_descriptor >> ctrl_endpt_out_desc = { >> @@ -1881,6 +1883,7 @@ static irqreturn_t udc_irq(struct ci_hdrc *ci) >> ci->driver->suspend(&ci->gadget); >> usb_gadget_set_state(&ci->gadget, >> USB_STATE_SUSPENDED); >> + ci_hdrc_otg_fsm_restart(ci); >> spin_lock(&ci->lock); >> } >> } >> -- > > Hi Niranjan, > > When working with iAP device, there are two role-switch methods > - Through OTG FSM, and using sysfs entries under > /sys/bus/platform/devices/ci_hdrc.0/inputs > but you may need to patch code to keep vbus always on for A-device, > it is not compliance with OTG spec. > - Using role interface under debugfs (I move it under sysfs for v4.12). > But you need to patch the code and let the A-device switch back to > host after iAP is disconnected. > > You don't need to use above two methods together, I suggest using the > 2nd method since OTG FSM is hard to maintain due to no mandatory use > case for it. > > -- > > Best Regards, > Peter Chen Thank you Peter for your response. Yes, I will try to switch the role on disconnection of iAP device to Host. If I understand correctly I have to do something like this - ci_role_stop(ci); //Gadget role stop ret = ci_role_start(ci, role); //Host role start correct? Regards, Niranjan -- 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] smsc95xx: Add comments to the registers definition
On Wed, Apr 12, 2017 at 11:24:05AM +0200, Martin Wetterwald wrote: > This chip is used by a lot of embedded devices and also by the Raspberry > Pi 1, 2 & 3 which were created to promote the study of computer > sciences. Students wanting to learn kernel / network device driver > programming through those devices can only rely on the Linux kernel > driver source to make their own. > > This commit adds a lot of comments to the registers definition to expand > the register names. > > Cc: Steve Glendinning > Cc: Microchip Linux Driver Support > CC: David Miller > Signed-off-by: Martin Wetterwald Reviewed-by: Andrew Lunn Andrew -- 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] chipidea: Fix issue in reconnecing gadget without insmod/rmmod
On Tue, Apr 11, 2017 at 08:46:24PM +0530, Niranjan Dighe wrote: > Currently usb_gadget_connect() is called only through gadget > registration via composite_driver_probe(). As a result, after a > disconnection, if the role transitions to host and back to gadget, > the gadget is not recognized by the host anymore. > This is a typical scenario with an iAP device in the following > usecase - > conn iAP dev -> send roleswitch command -> roleswitch ourself > to gadget -> disconnect iAP device -> roleswitch back to host to > enumerate the device again -> reconnect device -> resend the > roleswitch command -> roleswitch ourself to gadget -> ISSUE. > > To workaround this, do the following - > > 1. Restart OTG FSM on SLi interrupt and on switching role to gadget > so that device transitions to B_IDLE state from B_PERIPHERAL state. > A transition from B_IDLE to B_PERIPHERAL is needed to enable > interrups and restore the correct state of the chipidea controller > so that communication with host is possible > > 2. usb_gadget_connect() after roleswitch to gadget so that > gadget->ops->pullup() is called and D+ line is asserted. This > causes host to "see" the device and enumeration can happen. > > Signed-off-by: Niranjan Dighe > --- > drivers/usb/chipidea/debug.c | 10 +- > drivers/usb/chipidea/udc.c | 3 +++ > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c > index a4f7db2..66c1485 100644 > --- a/drivers/usb/chipidea/debug.c > +++ b/drivers/usb/chipidea/debug.c > @@ -16,7 +16,7 @@ > #include "udc.h" > #include "bits.h" > #include "otg.h" > - > +extern void ci_hdrc_otg_fsm_restart(struct ci_hdrc *); > /** > * ci_device_show: prints information about device capabilities and status > */ > @@ -325,6 +325,14 @@ static ssize_t ci_role_write(struct file *file, const > char __user *ubuf, > ci_role_stop(ci); > ret = ci_role_start(ci, role); > enable_irq(ci->irq); > + > + /* REVISIT - Avoid repeated FSM restart*/ > + > + if (role == CI_ROLE_GADGET) { > + ci_hdrc_otg_fsm_restart(ci); > + usb_gadget_connect(&ci->gadget); > + } > + > pm_runtime_put_sync(ci->dev); > > return ret ? ret : count; > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index a0accd5..1e3b827 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -29,6 +29,8 @@ > #include "otg.h" > #include "otg_fsm.h" > > +extern void ci_hdrc_otg_fsm_restart(struct ci_hdrc *); > + > /* control endpoint description */ > static const struct usb_endpoint_descriptor > ctrl_endpt_out_desc = { > @@ -1881,6 +1883,7 @@ static irqreturn_t udc_irq(struct ci_hdrc *ci) > ci->driver->suspend(&ci->gadget); > usb_gadget_set_state(&ci->gadget, > USB_STATE_SUSPENDED); > + ci_hdrc_otg_fsm_restart(ci); > spin_lock(&ci->lock); > } > } > -- Hi Niranjan, When working with iAP device, there are two role-switch methods - Through OTG FSM, and using sysfs entries under /sys/bus/platform/devices/ci_hdrc.0/inputs but you may need to patch code to keep vbus always on for A-device, it is not compliance with OTG spec. - Using role interface under debugfs (I move it under sysfs for v4.12). But you need to patch the code and let the A-device switch back to host after iAP is disconnected. You don't need to use above two methods together, I suggest using the 2nd method since OTG FSM is hard to maintain due to no mandatory use case for it. -- 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 v2] ARM: davinci: Add the clock for the CPPI 4.1 DMA engine
On Friday 07 April 2017 11:01 PM, Alexandre Bailon wrote: > > > On 04/07/2017 06:15 PM, Alexandre Bailon wrote: >> >> >> On 04/07/2017 04:36 PM, Sekhar Nori wrote: >>> On Wednesday 05 April 2017 10:47 PM, Alexandre Bailon wrote: The CPPI 4.1 DMA is sharing its clock with the USB OTG, and most of the time, the clock will be enabled by USB. But during the init of the DMA, USB is not enabled (waiting for DMA), and then we must enable the clock before doing anything. Add the clock for the CPPI 4.1 DMA engine. Signed-off-by: Alexandre Bailon --- arch/arm/mach-davinci/da830.c | 6 ++ arch/arm/mach-davinci/da850.c | 6 ++ 2 files changed, 12 insertions(+) diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c index 073c458..bd88470 100644 --- a/arch/arm/mach-davinci/da830.c +++ b/arch/arm/mach-davinci/da830.c @@ -304,6 +304,11 @@ static struct clk usb20_clk = { .gpsc= 1, }; +static struct clk cppi41_clk = { +.name= "cppi41", +.parent= &usb20_clk, +}; + static struct clk aemif_clk = { .name= "aemif", .parent= &pll0_sysclk3, @@ -413,6 +418,7 @@ static struct clk_lookup da830_clks[] = { CLK("davinci-mcasp.1",NULL,&mcasp1_clk), CLK("davinci-mcasp.2",NULL,&mcasp2_clk), CLK("musb-da8xx","usb20",&usb20_clk), +CLK("cppi41-dmaengine",NULL,&cppi41_clk), >>> I dont see this device name being used in current linux-next. Is this >>> name accepted ? >> There is here a typo. The name should be cppi41-dma-engine. >> I will fix it. > Actually, it is not a typo. It would have be more logical to name it > cppi41-dma-engine > (like the driver name) but the name is correct. > The device name is not yet in linux-next as the device is created in > da8xx driver. > http://marc.info/?l=linux-usb&m=149080474124498&w=2 Alright, applied this and sent a pull request. Made some minor changes to commit message text. Thanks, Sekhar -- 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 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests
Hi, David Laight writes: > From: Felipe Balbi >> David Laight writes: >> >> > From: Felipe Balbi >> >> >> Sent: 07 April 2017 12:37 >> >> >> Just like we did for all other endpoint types, let's rely on a chained >> >> >> TRB pointing to ep0_bounce_addr in order to align transfer size. This >> >> >> will make the code simpler. >> >> > ... >> >> > >> >> > Is the dwc3 similar enough to xhci to have an 'immediate data' bit? >> >> > If so the aligning data could come from the 8 byte 'address' field. >> >> >> >> you mean like patch 1 in this very series? >> >> >> >> https://marc.info/?i=20170407113655.27569-1-felipe.ba...@linux.intel.com >> > >> > Not exactly, that seems to be using the trb memory itself as the buffer. >> > I'm guessing that can only work for 'read' TRB. >> > >> > If you look at the xhci docs (eg table 71 in section 6.4.1.2.1 of the rev >> > 1.0 >> > copy I have) bit 6 of the last word is 'IDT'. >> > So 8 bytes of data are put into the bpl/bph fields - not a pointer at all. >> >> Right, it works similarly for DWC3 :-) However, instead of IDT we just >> set BPH/BPL to the address of the TRB itself. > > Ah, not quite... > > You are setting BPH/BPL to point to BPH/BPL rather than the TRB. > This works for 'read' type transactions because the pointer isn't > needed after the data has been written. right, note that the address passed in _is_ the address of the TRB, but I *know* only 8 bytes will be used, thus only BPH/BPL get overwritten This is documented in DWC3 databook. > For short writes you'd need additional buffer memory somewhere. > Could be allocated after the TRB array. For that I have a single 1024 byte throw-away buffer that *all* endpoints use. I don't care about the data that goes there (there should never be *any* data there, actually), I'm just interested in guarateeing that a faulty host will never cause a buffer overflow. -- balbi signature.asc Description: PGP signature
RE: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests
From: Felipe Balbi > David Laight writes: > >> > From: Felipe Balbi > >> >> Sent: 07 April 2017 12:37 > >> >> Just like we did for all other endpoint types, let's rely on a chained > >> >> TRB pointing to ep0_bounce_addr in order to align transfer size. This > >> >> will make the code simpler. > >> > ... > >> > > >> > Is the dwc3 similar enough to xhci to have an 'immediate data' bit? > >> > If so the aligning data could come from the 8 byte 'address' field. > >> > >> you mean like patch 1 in this very series? > >> > >> https://marc.info/?i=20170407113655.27569-1-felipe.ba...@linux.intel.com > > > > Not exactly, that seems to be using the trb memory itself as the buffer. > > I'm guessing that can only work for 'read' TRB. > > > > If you look at the xhci docs (eg table 71 in section 6.4.1.2.1 of the rev > > 1.0 > > copy I have) bit 6 of the last word is 'IDT'. > > So 8 bytes of data are put into the bpl/bph fields - not a pointer at all. > > Right, it works similarly for DWC3 :-) However, instead of IDT we just > set BPH/BPL to the address of the TRB itself. Ah, not quite... You are setting BPH/BPL to point to BPH/BPL rather than the TRB. This works for 'read' type transactions because the pointer isn't needed after the data has been written. For short writes you'd need additional buffer memory somewhere. Could be allocated after the TRB array. David -- 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 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests
Hi, David Laight writes: >> > From: Felipe Balbi >> >> Sent: 07 April 2017 12:37 >> >> Just like we did for all other endpoint types, let's rely on a chained >> >> TRB pointing to ep0_bounce_addr in order to align transfer size. This >> >> will make the code simpler. >> > ... >> > >> > Is the dwc3 similar enough to xhci to have an 'immediate data' bit? >> > If so the aligning data could come from the 8 byte 'address' field. >> >> you mean like patch 1 in this very series? >> >> https://marc.info/?i=20170407113655.27569-1-felipe.ba...@linux.intel.com > > Not exactly, that seems to be using the trb memory itself as the buffer. > I'm guessing that can only work for 'read' TRB. > > If you look at the xhci docs (eg table 71 in section 6.4.1.2.1 of the rev 1.0 > copy I have) bit 6 of the last word is 'IDT'. > So 8 bytes of data are put into the bpl/bph fields - not a pointer at all. Right, it works similarly for DWC3 :-) However, instead of IDT we just set BPH/BPL to the address of the TRB itself. If I were to guess, it was made this way because peripheral controllers don't send SETUP packets, only receive them from a host. -- balbi signature.asc Description: PGP signature
RE: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests
Hi, David Laight writes: > From: Felipe >> Sent: 12 April 2017 06:57 >> Felipe Balbi writes: >> >> From: Felipe Balbi >> >>> Sent: 07 April 2017 12:37 >> >>> Just like we did for all other endpoint types, let's rely on a chained >> >>> TRB pointing to ep0_bounce_addr in order to align transfer size. This >> >>> will make the code simpler. >> >> ... >> >> >> >> Is the dwc3 similar enough to xhci to have an 'immediate data' bit? >> >> If so the aligning data could come from the 8 byte 'address' field. >> > >> > you mean like patch 1 in this very series? >> >> Oh no, you mean for the remaining bytes for alignment. Well, 8 bytes >> might not be enough to align anything, right? :-) >> >> Besides, "aligned" here refers to length not memory address :-) OUT >> transfers on dwc3 need to have their length setup so that it is >> divisible by wMaxPacketSize of the endpoint in question. > > Hardware engineers seem to be making things that appear to be useful, > but in practise are almost to program for. > > I might be mixing up what is fixing what... > > I was thinking of the borked hardware than can only dma from aligned > addresses. oh, DWC3 doesn't have that limitation. That's DWC2 :-) USB2-only IP from same company. DWC3 has USB3.1 GEN1 and GEN2 versions ;-) -- balbi signature.asc Description: PGP signature
RE: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests
From: Felipe > Sent: 12 April 2017 06:57 > Felipe Balbi writes: > >> From: Felipe Balbi > >>> Sent: 07 April 2017 12:37 > >>> Just like we did for all other endpoint types, let's rely on a chained > >>> TRB pointing to ep0_bounce_addr in order to align transfer size. This > >>> will make the code simpler. > >> ... > >> > >> Is the dwc3 similar enough to xhci to have an 'immediate data' bit? > >> If so the aligning data could come from the 8 byte 'address' field. > > > > you mean like patch 1 in this very series? > > Oh no, you mean for the remaining bytes for alignment. Well, 8 bytes > might not be enough to align anything, right? :-) > > Besides, "aligned" here refers to length not memory address :-) OUT > transfers on dwc3 need to have their length setup so that it is > divisible by wMaxPacketSize of the endpoint in question. Hardware engineers seem to be making things that appear to be useful, but in practise are almost to program for. I might be mixing up what is fixing what... I was thinking of the borked hardware than can only dma from aligned addresses. David -- 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 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests
From: Felipe Balbi > Sent: 12 April 2017 06:55 > David Laight writes: > > > From: Felipe Balbi > >> Sent: 07 April 2017 12:37 > >> Just like we did for all other endpoint types, let's rely on a chained > >> TRB pointing to ep0_bounce_addr in order to align transfer size. This > >> will make the code simpler. > > ... > > > > Is the dwc3 similar enough to xhci to have an 'immediate data' bit? > > If so the aligning data could come from the 8 byte 'address' field. > > you mean like patch 1 in this very series? > > https://marc.info/?i=20170407113655.27569-1-felipe.ba...@linux.intel.com Not exactly, that seems to be using the trb memory itself as the buffer. I'm guessing that can only work for 'read' TRB. If you look at the xhci docs (eg table 71 in section 6.4.1.2.1 of the rev 1.0 copy I have) bit 6 of the last word is 'IDT'. So 8 bytes of data are put into the bpl/bph fields - not a pointer at all. David -- 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] smsc95xx: Add comments to the registers definition
On 12 April 2017 at 10:24, Martin Wetterwald wrote: > This chip is used by a lot of embedded devices and also by the Raspberry > Pi 1, 2 & 3 which were created to promote the study of computer > sciences. Students wanting to learn kernel / network device driver > programming through those devices can only rely on the Linux kernel > driver source to make their own. > > This commit adds a lot of comments to the registers definition to expand > the register names. > > Cc: Steve Glendinning > Cc: Microchip Linux Driver Support > CC: David Miller > Signed-off-by: Martin Wetterwald Acked-by: Steve Glendinning -- 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 v3] smsc95xx: Add comments to the registers definition
This chip is used by a lot of embedded devices and also by the Raspberry Pi 1, 2 & 3 which were created to promote the study of computer sciences. Students wanting to learn kernel / network device driver programming through those devices can only rely on the Linux kernel driver source to make their own. This commit adds a lot of comments to the registers definition to expand the register names. Cc: Steve Glendinning Cc: Microchip Linux Driver Support CC: David Miller Signed-off-by: Martin Wetterwald --- Changes in v3: * Restore previous name for TX_CMD_B_CSUM_ENABLE drivers/net/usb/smsc95xx.c | 4 +- drivers/net/usb/smsc95xx.h | 473 ++--- 2 files changed, 271 insertions(+), 206 deletions(-) diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 831aa33..2e720cd 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -33,7 +33,7 @@ #include "smsc95xx.h" #define SMSC_CHIPNAME "smsc95xx" -#define SMSC_DRIVER_VERSION"1.0.5" +#define SMSC_DRIVER_VERSION"1.0.6" #define HS_USB_PKT_SIZE(512) #define FS_USB_PKT_SIZE(64) #define DEFAULT_HS_BURST_CAP_SIZE (16 * 1024 + 5 * HS_USB_PKT_SIZE) @@ -1498,7 +1498,7 @@ static int smsc95xx_enter_suspend3(struct usbnet *dev) if (ret < 0) return ret; - if (val & 0x) { + if (val & RX_FIFO_INF_USED_) { netdev_info(dev->net, "rx fifo not empty in autosuspend\n"); return -EBUSY; } diff --git a/drivers/net/usb/smsc95xx.h b/drivers/net/usb/smsc95xx.h index 29a4d9e..13bc32a 100644 --- a/drivers/net/usb/smsc95xx.h +++ b/drivers/net/usb/smsc95xx.h @@ -21,128 +21,158 @@ #define _SMSC95XX_H /* Tx command words */ -#define TX_CMD_A_DATA_OFFSET_ (0x001F) -#define TX_CMD_A_FIRST_SEG_(0x2000) -#define TX_CMD_A_LAST_SEG_ (0x1000) -#define TX_CMD_A_BUF_SIZE_ (0x07FF) +#define TX_CMD_A_DATA_OFFSET_ (0x001F)/* Data Start Offset */ +#define TX_CMD_A_FIRST_SEG_(0x2000)/* First Segment */ +#define TX_CMD_A_LAST_SEG_ (0x1000)/* Last Segment */ +#define TX_CMD_A_BUF_SIZE_ (0x07FF)/* Buffer Size */ -#define TX_CMD_B_CSUM_ENABLE (0x4000) -#define TX_CMD_B_ADD_CRC_DISABLE_ (0x2000) -#define TX_CMD_B_DISABLE_PADDING_ (0x1000) -#define TX_CMD_B_PKT_BYTE_LENGTH_ (0x07FF) +#define TX_CMD_B_CSUM_ENABLE (0x4000)/* TX Checksum Enable */ +#define TX_CMD_B_ADD_CRC_DIS_ (0x2000)/* Add CRC Disable */ +#define TX_CMD_B_DIS_PADDING_ (0x1000)/* Disable Frame Padding */ +#define TX_CMD_B_FRAME_LENGTH_ (0x07FF)/* Frame Length (bytes) */ /* Rx status word */ -#define RX_STS_FF_ (0x4000)/* Filter Fail */ -#define RX_STS_FL_ (0x3FFF)/* Frame Length */ -#define RX_STS_ES_ (0x8000)/* Error Summary */ -#define RX_STS_BF_ (0x2000)/* Broadcast Frame */ -#define RX_STS_LE_ (0x1000)/* Length Error */ -#define RX_STS_RF_ (0x0800)/* Runt Frame */ -#define RX_STS_MF_ (0x0400)/* Multicast Frame */ -#define RX_STS_TL_ (0x0080)/* Frame too long */ -#define RX_STS_CS_ (0x0040)/* Collision Seen */ -#define RX_STS_FT_ (0x0020)/* Frame Type */ -#define RX_STS_RW_ (0x0010)/* Receive Watchdog */ -#define RX_STS_ME_ (0x0008)/* Mii Error */ -#define RX_STS_DB_ (0x0004)/* Dribbling */ -#define RX_STS_CRC_(0x0002)/* CRC Error */ - -/* SCSRs */ -#define ID_REV (0x00) -#define ID_REV_CHIP_ID_MASK_ (0x) -#define ID_REV_CHIP_REV_MASK_ (0x) -#define ID_REV_CHIP_ID_9500_ (0x9500) -#define ID_REV_CHIP_ID_9500A_ (0x9E00) -#define ID_REV_CHIP_ID_9512_ (0xEC00) -#define ID_REV_CHIP_ID_9530_ (0x9530) -#define ID_REV_CHIP_ID_89530_ (0x9E08) -#define ID_REV_CHIP_ID_9730_ (0x9730) - -#define INT_STS(0x08) -#define INT_STS_TX_STOP_ (0x0002) -#define INT_STS_RX_STOP_ (0x0001) -#define INT_STS_PHY_INT_ (0x8000) -#define INT_STS_TXE_ (0x4000) -#define INT_STS_TDFU_ (0x2000) -#define INT_STS_TDFO_ (0x1000) -#define INT_STS_RXDF_ (0x0800) -#define INT_STS_GPIOS_ (0x07FF) -#define INT_STS_CLEAR_ALL_ (0x) - -#define RX_CFG (0x0C) -#define RX_FIFO_FLUSH_ (0x000
Re: [PATCH 7/9] usbip: Add USB_SPEED_SUPER as valid arg
Am Mittwoch, den 12.04.2017, 07:13 +0800 schrieb Yuyang Du: > With this patch, USB_SPEED_SUPER is a valid speed when attaching > a USB3 SuperSpeed device. > Hi, sorry this is just short sighted. If you do this, also add SS+ Regards Oliver -- 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 2/9] usbip: vhci-hcd: Add vhci struct
Am Mittwoch, den 12.04.2017, 07:13 +0800 schrieb Yuyang Du: > In order to support SuperSpeed devices, a USB3 HCD is added to > share the USB2 HCD. As a result, a "VHCI" is composed of two > vhci_hcds associated with the two HCDs respectively. So we add > another level of abstraction, vhci, and thus this vhci structure. > Hi, what is the motivation for this change? That is the way XHCI does it. But why do that when you export something over a network? Are you ever going to use both VHCIs? Regards Oliver -- 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
[REGRESSION next-20170410] Commit a6ff6cbf1fab ("usb: xhci: Add helper function xhci_set_power_on().") breaks armada-385
Hi Guoqing Zhang, Commit a6ff6cbf1fabe7500d8ac25e133e3346db0a0fca ("usb: xhci: Add helper function xhci_set_power_on().") causes a null pointer dereference on an armada-385. Below the kernel as well as the bisect log. Regards Ralph [2.481636] Unable to handle kernel NULL pointer dereference at virtual address [2.489758] pgd = c0004000 [2.492479] [] *pgd= [2.496069] Internal error: Oops: 5 [#1] SMP ARM [2.500702] Modules linked in: [2.503769] CPU: 0 PID: 605 Comm: kworker/0:2 Not tainted 4.11.0-rc6-next-20170410 #3 [2.511629] Hardware name: Marvell Armada 380/385 (Device Tree) [2.517578] Workqueue: events deferred_probe_work_func [2.522737] task: def09600 task.stack: de688000 [2.527285] PC is at _raw_spin_unlock_irqrestore+0x28/0x2c [2.532791] LR is at 0x0 [2.535333] pc : []lr : [<>]psr: 6010 [2.535333] sp : ip : de689720 fp : de68971c [2.546856] r10: r9 : 8013 r8 : 0001 [2.552100] r7 : 0001 r6 : 000206e1 r5 : e0aa4430 r4 : df575168 [2.558651] r3 : 0003 r2 : de6d1340 r1 : r0 : df575168 [2.565204] Flags: nZCv IRQs on FIQs on Mode USER_32 ISA ARM Segment kernel [2.572628] Control: 10c5387d Table: 404a DAC: 0055 [2.578395] Process kworker/0:2 (pid: 605, stack limit = 0xde688210) [2.584782] ---[ end trace 6795d9561f630b41 ]--- [2.589417] Kernel panic - not syncing: Fatal exception [2.594663] CPU1: stopping [2.597382] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G D 4.11.0-rc6-next-20170410 #3 [2.606115] Hardware name: Marvell Armada 380/385 (Device Tree) [2.612056] Backtrace: [2.614518] [] (dump_backtrace) from [] (show_stack+0x18/0x1c) [2.622118] r7:df467f28 r6:6193 r5:c0a15c70 r4: [2.627801] [] (show_stack) from [] (dump_stack+0x94/0xa8) [2.635054] [] (dump_stack) from [] (handle_IPI+0x178/0x198) [2.642478] r7:df467f28 r6: r5:0001 r4:c0a2fcb0 [2.648160] [] (handle_IPI) from [] (gic_handle_irq+0x90/0x94) [2.655759] r7:df467f28 r6:e080210c r5:c0a03fac r4:c0a16080 [2.661440] [] (gic_handle_irq) from [] (__irq_svc+0x6c/0x90) [2.668951] Exception stack(0xdf467f28 to 0xdf467f70) [2.674023] 7f20: 0001 c0118e60 df466000 c0a03cf8 [2.682233] 7f40: c0a03cac c0969488 df467f98 df467f84 df467f88 df467f78 [2.690443] 7f60: c01085ac c01085b0 6013 [2.695514] r9:df466000 r8: r7:df467f5c r6: r5:6013 r4:c01085b0 [2.703290] [] (arch_cpu_idle) from [] (default_idle_call+0x28/0x34) [2.711416] [] (default_idle_call) from [] (do_idle+0x1a4/0x1d0) [2.719192] [] (do_idle) from [] (cpu_startup_entry+0x20/0x24) [2.726793] r10: r9:414fc091 r8:406a r7:c0a2fcc0 r6:10c0387d r5:0001 [2.734652] r4:0086 [2.737195] [] (cpu_startup_entry) from [] (secondary_start_kernel+0x150/0x15c) [2.746277] [] (secondary_start_kernel) from [<0010162c>] (0x10162c) [2.753527] r5:0051 r4:1f45c06a [2.757117] Rebooting in 1 seconds.. --- git bisect start # bad: [f8c97bdb49832d2b0edaa0c05db873aa2f6101ff] Add linux-next specific files for 20170410 git bisect bad f8c97bdb49832d2b0edaa0c05db873aa2f6101ff # good: [c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201] Linux 4.11-rc1 git bisect good c1ae3cfa0e89fa1a7ecc4c99031f5e9ae99d9201 # good: [1902aaff4d92a3839ab1bb8764b8ce34d7302050] Merge remote-tracking branch 'crypto/master' git bisect good 1902aaff4d92a3839ab1bb8764b8ce34d7302050 # good: [ebfd6b2f85a82b2833187961e97b2d9bf91b996e] Merge remote-tracking branch 'devicetree/for-next' git bisect good ebfd6b2f85a82b2833187961e97b2d9bf91b996e # bad: [5c1e203c899abd637f89234344ddd30044d9b77c] Merge remote-tracking branch 'extcon/extcon-next' git bisect bad 5c1e203c899abd637f89234344ddd30044d9b77c # good: [508fe66c67f2201e3ea873b679c6e0112302db43] Merge remote-tracking branch 'kvm/linux-next' git bisect good 508fe66c67f2201e3ea873b679c6e0112302db43 # bad: [e8c7b5db5108ea627d088dffd3a7c235aafe9343] Merge remote-tracking branch 'usb/usb-next' git bisect bad e8c7b5db5108ea627d088dffd3a7c235aafe9343 # good: [e8ce3e093a5ae3547b804413b1853e8ca2397940] Merge remote-tracking branch 'workqueues/for-next' git bisect good e8ce3e093a5ae3547b804413b1853e8ca2397940 # bad: [96d9a6eb97d77d6a3768f101f400c42743799bb2] usb: xhci: fix link trb decoding git bisect bad 96d9a6eb97d77d6a3768f101f400c42743799bb2 # good: [50129f74548b5075187fa4908c2ba3a081ec1b67] USB: ftdi-elan: refactor endpoint retrieval git bisect good 50129f74548b5075187fa4908c2ba3a081ec1b67 # good: [2c930e3d0aed1505e86e0928d323df5027817740] usb: misc: add missing continue in switch git bisect good 2c930e3d0aed1505e86
Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device
Hi, Greg KH writes: >> Here's what we have for most UDCs (net2280.c included): >> >> struct my_udc { >> struct gadget gadget; >> [...] >> }; >> >> probe() >> { >> struct my_udc *u; >> >> u = kzalloc(sizeof(*u), GFP_KERNEL); >> [...] >> return 0; >> } >> >> Now, if this kzalloc() would be replaced with devm_kzalloc() wouldn't >> this result on a functionally equivalent execution to the patch I >> proposed above? >> >> Iff we change struct gadget to contain a struct device *dev instead of a >> struct device dev, then sure, we will need to cope with proper >> ->release() implementations. >> >> As it is, it brings nothing to the table, IMO. > > You always have to have a release function for a kobject, no matter > where it is, as it is being reference counted. To not do so, is a huge > indication of a problem in the design. okay, this goes all the way back to when Dave B wrote the API, it has always had empty ->release() functions (not all UDCs, though). I'll make sure to review all of this for v4.13. -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache
On 04/11/2017 11:14 PM, Felipe Balbi wrote: > > Hi, > > John Youn writes: John Youn writes: >> Thinh Nguyen writes: >>> The dwc3 driver can overwite its previous events if its top half IRQ >>> handler gets invoked again before processing the events in the cache. We >> >> interrupts are masked, why would top half get invoked again? Is this, >> perhaps, related to DWC3 3.00a which has the "Interrupt line doesn't >> lower when masked" problem? We've added a lot of code to workaround that >> problem and, apparently, it wasn't enough. > > No, it is not related to that. We verified with PCIe traces. The > interrupt line gets deasserted after we mask it. And we put the > masking as close to the beginning of the top-half as possible. > >> >> In any case, there's no way top half would be invoked again in a >> properly working DWC3. > > Yet we still see it sometimes. Usually it doesn't create a problem, that's fair, but it's not for the reason you're describing :-) There might be another problem going on, because since we masked the interrupt and cleared all events, IRQ shouldn't be raised at all; unless, as I mentioned on the other subthread, the IRQ line is shared. > but if there happens to be a new event there, we get the failure. > > We didn't trace into that part of the kernel so we can't explain why. > But if there is any chance the interrupt line deassertion wasn't > detected in time, whatever part of the kernel that thinks it is still > asserted might just call our top-half again. This could be a totally > wrong assumption, but it doesn't seem too far-fetched. The kernel doesn't detect IRQ line assertion/deassertion. CPU gets an exception when that happens and calls Kernel IRQ handler vector. That will, in turn, figure out which line triggered, call the handler and so on. >>> >>> We're talking about PCIe though, where interrupt assertion and >>> deassertion are packets. So I would imagine the kernel has to do >>> something and there could be some latency associated with that. >> >> Also, another thing is that the device uses legacy, level-triggered, >> PCIe interrupts, so for as long as the interrupt is asserted, the TH >> is called repeatedly. > > yes, and that's why we have: > >> static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt) >> { >> struct dwc3 *dwc = evt->dwc; >> u32 amount; >> u32 count; >> u32 reg; > >> if (pm_runtime_suspended(dwc->dev)) { >> pm_runtime_get(dwc->dev); >> disable_irq_nosync(dwc->irq_gadget); >> dwc->pending_events = true; >> return IRQ_HANDLED; >> } >> >> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); >> count &= DWC3_GEVNTCOUNT_MASK; > > check how many events are pending in the event buffer. > >> if (!count) >> return IRQ_NONE; >> >> evt->count = count; >> evt->flags |= DWC3_EVENT_PENDING; >> >> /* Mask interrupt */ >> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0)); >> reg |= DWC3_GEVNTSIZ_INTMASK; > > mask interrupt generation > >> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg); >> >> amount = min(count, evt->length - evt->lpos); >> memcpy(evt->cache + evt->lpos, evt->buf + evt->lpos, amount); >> >> if (amount < count) >> memcpy(evt->cache, evt->buf, count - amount); >> >> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count); > > clear ALL events from event buffer. This brings the line down, so we > shouldn't re-enter. Right, I get all that, but you're not addressing the fact that the "interrupts" are just packets on the PCIe and there could be some latency in detecting the de-assertion. The "line" doesn't go down immediately when we mask it. I don't see any obvious way to guarantee that it will be de-asserted by the time we exit TH. There is both an "assert" and "de-assert" packet. Between those, the TH gets called continuously. You can see multiple TH before BH behavior just by omitting the mask and leaving it asserted. > >> return IRQ_WAKE_THREAD; >> } > >> So we mask the interrupt in the TH and a short time later, the >> interrupt de-assertion packet is sent on PCIe bus and if that's not >> seen right away we may already have another call to TH before the BH >> gets scheduled. > > not sure this can happen. If that's the case, every PCI driver would > have all sorts of tricks to cope with this, not only dwc3 :-) Well, the DWC3 is now doing something destructive in the TH. Otherwise multiple TH calls wouldn't have affected anything. We should probably use a queue for the events instead. I think most drivers should be robust against this type of thing, as you never know when you can get an interrupt, and "spurious" interrupts are a thing. > > Bjorn, is this something that can happen on PCIe? > > Quick summary of the prob
[PATCH 9/9] usbip: vhci-hcd: Clean up the code by adding a new macro
Each vhci has 2*VHCI_HC_PORTS ports, in which VHCI_HC_PORTS ports are HighSpeed (or below), and VHCI_HC_PORTS are SuperSpeed. This new macro VHCI_PORTS reflects this configuration. Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci.h | 5 - drivers/usb/usbip/vhci_sysfs.c | 10 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h index db28eb5..5cfb59e 100644 --- a/drivers/usb/usbip/vhci.h +++ b/drivers/usb/usbip/vhci.h @@ -84,6 +84,9 @@ enum hub_speed { #define VHCI_HC_PORTS 8 #endif +/* Each VHCI has 2 hubs (USB2 and USB3), each has VHCI_HC_PORTS ports */ +#define VHCI_PORTS (VHCI_HC_PORTS*2) + #ifdef CONFIG_USBIP_VHCI_NR_HCS #define VHCI_NR_HCS CONFIG_USBIP_VHCI_NR_HCS #else @@ -145,7 +148,7 @@ static inline __u32 port_to_rhport(__u32 port) static inline int port_to_pdev_nr(__u32 port) { - return port / (VHCI_HC_PORTS * 2); + return port / VHCI_PORTS; } static inline struct vhci_hcd *hcd_to_vhci_hcd(struct usb_hcd *hcd) diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index 3ad68ff..5778b64 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -92,7 +92,7 @@ static ssize_t status_show_vhci(int pdev_nr, char *out) spin_lock(&vdev->ud.lock); port_show_vhci(&out, HUB_SPEED_HIGH, - pdev_nr * VHCI_HC_PORTS * 2 + i, vdev); + pdev_nr * VHCI_PORTS + i, vdev); spin_unlock(&vdev->ud.lock); } @@ -101,7 +101,7 @@ static ssize_t status_show_vhci(int pdev_nr, char *out) spin_lock(&vdev->ud.lock); port_show_vhci(&out, HUB_SPEED_SUPER, - pdev_nr * VHCI_HC_PORTS * 2 + VHCI_HC_PORTS + i, vdev); + pdev_nr * VHCI_PORTS + VHCI_HC_PORTS + i, vdev); spin_unlock(&vdev->ud.lock); } @@ -117,7 +117,7 @@ static ssize_t status_show_not_ready(int pdev_nr, char *out) for (i = 0; i < VHCI_HC_PORTS; i++) { out += sprintf(out, "hs %04u %03u ", - (pdev_nr * VHCI_HC_PORTS * 2) + i, + (pdev_nr * VHCI_PORTS) + i, VDEV_ST_NOTASSIGNED); out += sprintf(out, "000 0-0"); out += sprintf(out, "\n"); @@ -125,7 +125,7 @@ static ssize_t status_show_not_ready(int pdev_nr, char *out) for (i = 0; i < VHCI_HC_PORTS; i++) { out += sprintf(out, "ss %04u %03u ", - (pdev_nr * VHCI_HC_PORTS * 2) + VHCI_HC_PORTS + i, + (pdev_nr * VHCI_PORTS) + VHCI_HC_PORTS + i, VDEV_ST_NOTASSIGNED); out += sprintf(out, "000 0-0"); out += sprintf(out, "\n"); @@ -176,7 +176,7 @@ static ssize_t nports_show(struct device *dev, struct device_attribute *attr, /* * Half the ports are for SPEED_HIGH and half for SPEED_SUPER, thus the * 2. */ - out += sprintf(out, "%d\n", VHCI_HC_PORTS * vhci_num_controllers * 2); + out += sprintf(out, "%d\n", VHCI_PORTS * vhci_num_controllers); return out - s; } static DEVICE_ATTR_RO(nports); -- 2.7.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
[PATCH 8/9] usbip: vhci-hcd: Add USB3 port status bits
As USB3 has (slightly) different bit meanings in the port status. Add a new status bit array for USB3. Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci_hcd.c | 56 +++- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 019b268..e66a3c8 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -66,7 +66,7 @@ static const char * const bit_desc[] = { "SUSPEND", /*2*/ "OVER_CURRENT", /*3*/ "RESET",/*4*/ - "R5", /*5*/ + "L1", /*5*/ "R6", /*6*/ "R7", /*7*/ "POWER",/*8*/ @@ -82,7 +82,7 @@ static const char * const bit_desc[] = { "C_SUSPEND",/*18*/ "C_OVER_CURRENT", /*19*/ "C_RESET", /*20*/ - "R21", /*21*/ + "C_L1", /*21*/ "R22", /*22*/ "R23", /*23*/ "R24", /*24*/ @@ -95,10 +95,49 @@ static const char * const bit_desc[] = { "R31", /*31*/ }; -static void dump_port_status_diff(u32 prev_status, u32 new_status) +static const char * const bit_desc_ss[] = { + "CONNECTION", /*0*/ + "ENABLE", /*1*/ + "SUSPEND", /*2*/ + "OVER_CURRENT", /*3*/ + "RESET",/*4*/ + "L1", /*5*/ + "R6", /*6*/ + "R7", /*7*/ + "R8", /*8*/ + "POWER",/*9*/ + "HIGHSPEED",/*10*/ + "PORT_TEST",/*11*/ + "INDICATOR",/*12*/ + "R13", /*13*/ + "R14", /*14*/ + "R15", /*15*/ + "C_CONNECTION", /*16*/ + "C_ENABLE", /*17*/ + "C_SUSPEND",/*18*/ + "C_OVER_CURRENT", /*19*/ + "C_RESET", /*20*/ + "C_BH_RESET", /*21*/ + "C_LINK_STATE", /*22*/ + "C_CONFIG_ERROR", /*23*/ + "R24", /*24*/ + "R25", /*25*/ + "R26", /*26*/ + "R27", /*27*/ + "R28", /*28*/ + "R29", /*29*/ + "R30", /*30*/ + "R31", /*31*/ +}; + +static void dump_port_status_diff(u32 prev_status, u32 new_status, bool usb3) { int i = 0; u32 bit = 1; + const char * const *desc = bit_desc; + + if (usb3) + desc = bit_desc_ss; pr_debug("status prev -> new: %08x -> %08x\n", prev_status, new_status); while (bit) { @@ -113,8 +152,12 @@ static void dump_port_status_diff(u32 prev_status, u32 new_status) else change = ' '; - if (prev || new) - pr_debug(" %c%s\n", change, bit_desc[i]); + if (prev || new) { + pr_debug(" %c%s\n", change, desc[i]); + + if (bit == 1) /* USB_PORT_STAT_CONNECTION */ + pr_debug(" %c%s\n", change, "USB_PORT_STAT_SPEED_5GBPS"); + } bit <<= 1; i++; } @@ -577,7 +620,8 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, /* Only dump valid port status */ if (rhport >= 0) { dump_port_status_diff(prev_port_status[rhport], - vhci_hcd->port_status[rhport]); + vhci_hcd->port_status[rhport], + hcd->speed == HCD_USB3); } } usbip_dbg_vhci_rh(" bye\n"); -- 2.7.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
[PATCH 0/9] usbip: Enable USB3 SuperSpeed
Hi Shuah, SuperSpeed (only) USB devices cannot be shared via usbip. This patch series attempts to fix it. The first 5 patches refactors the existing code to prepare for the SuperSpeed addition. With this series, our SuperSpeed device works fine. Many thanks to Greg and Krzysztof for their patience to answer my non-usb-professional questions, and special thanks to Krzysztof for the pointer to the SuperSpeed patch in dummy_hcd. This series is based on the series: "usb: usbip: Fix ports and port status v3" (https://www.spinics.net/lists/linux-usb/msg155834.html). Regards, Yuyang -- Yuyang Du (9): usbip: vhci-hcd: Rename function names to reflect their struct names usbip: vhci-hcd: Add vhci struct usbip: vhci-hcd: Move VHCI platform device into vhci struct usbip: vhci-hcd: Rework vhci_hcd_init usbip: vhci-hcd: Set the vhci structure up to work usbip: vhci-hcd: Add USB3 SuperSpeed support usbip: Add USB_SPEED_SUPER as valid arg usbip: vhci-hcd: Add USB3 port status bits usbip: vhci-hcd: Clean up the code by adding a new macro drivers/usb/usbip/vhci.h | 36 +- drivers/usb/usbip/vhci_hcd.c | 619 ++- drivers/usb/usbip/vhci_rx.c | 16 +- drivers/usb/usbip/vhci_sysfs.c | 138 +--- tools/usb/usbip/libsrc/vhci_driver.c | 25 +- tools/usb/usbip/libsrc/vhci_driver.h | 9 +- tools/usb/usbip/src/usbip_attach.c | 3 +- 7 files changed, 615 insertions(+), 231 deletions(-) -- 2.7.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
[PATCH 6/9] usbip: vhci-hcd: Add USB3 SuperSpeed support
This patch adds a USB3 HCD to an existing USB2 HCD and provides the support of SuperSpeed, in case the device can only be enumerated with SuperSpeed. The bulk of the added code in usb3_bos_desc and hub_control to support SuperSpeed is borrowed from the commit 1cd8fd2887e162ad ("usb: gadget: dummy_hcd: add SuperSpeed support"). With this patch, each vhci will have VHCI_HC_PORTS HighSpeed ports and VHCI_HC_PORTS SuperSpeed ports. Suggested-by: Krzysztof Opasiak Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci.h | 7 +- drivers/usb/usbip/vhci_hcd.c | 337 --- drivers/usb/usbip/vhci_sysfs.c | 109 +++ tools/usb/usbip/libsrc/vhci_driver.c | 23 ++- tools/usb/usbip/libsrc/vhci_driver.h | 8 +- tools/usb/usbip/src/usbip_attach.c | 3 +- 6 files changed, 384 insertions(+), 103 deletions(-) diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h index 8a979fc..db28eb5 100644 --- a/drivers/usb/usbip/vhci.h +++ b/drivers/usb/usbip/vhci.h @@ -72,6 +72,11 @@ struct vhci_unlink { unsigned long unlink_seqnum; }; +enum hub_speed { + HUB_SPEED_HIGH = 0, + HUB_SPEED_SUPER, +}; + /* Number of supported ports. Value has an upperbound of USB_MAXCHILDREN */ #ifdef CONFIG_USBIP_VHCI_HC_PORTS #define VHCI_HC_PORTS CONFIG_USBIP_VHCI_HC_PORTS @@ -140,7 +145,7 @@ static inline __u32 port_to_rhport(__u32 port) static inline int port_to_pdev_nr(__u32 port) { - return port / VHCI_HC_PORTS; + return port / (VHCI_HC_PORTS * 2); } static inline struct vhci_hcd *hcd_to_vhci_hcd(struct usb_hcd *hcd) diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 8cfba1d..019b268 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -232,6 +232,45 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf) return changed ? retval : 0; } +/* caller must hold lock */ +static void set_link_state(struct vhci_hcd *vhci_hcd) +{ +} + +/* usb 3.0 root hub device descriptor */ +static struct { + struct usb_bos_descriptor bos; + struct usb_ss_cap_descriptor ss_cap; +} __packed usb3_bos_desc = { + + .bos = { + .bLength= USB_DT_BOS_SIZE, + .bDescriptorType= USB_DT_BOS, + .wTotalLength = cpu_to_le16(sizeof(usb3_bos_desc)), + .bNumDeviceCaps = 1, + }, + .ss_cap = { + .bLength= USB_DT_USB_SS_CAP_SIZE, + .bDescriptorType= USB_DT_DEVICE_CAPABILITY, + .bDevCapabilityType = USB_SS_CAP_TYPE, + .wSpeedSupported= cpu_to_le16(USB_5GBPS_OPERATION), + .bFunctionalitySupport = ilog2(USB_5GBPS_OPERATION), + }, +}; + +static inline void +ss_hub_descriptor(struct usb_hub_descriptor *desc) +{ + memset(desc, 0, sizeof *desc); + desc->bDescriptorType = USB_DT_SS_HUB; + desc->bDescLength = 12; + desc->wHubCharacteristics = cpu_to_le16( + HUB_CHAR_INDV_PORT_LPSM | HUB_CHAR_COMMON_OCPM); + desc->bNbrPorts = VHCI_HC_PORTS; + desc->u.ss.bHubHdrDecLat = 0x04; /* Worst case: 0.4 micro sec*/ + desc->u.ss.DeviceRemovable = 0x; +} + static inline void hub_descriptor(struct usb_hub_descriptor *desc) { memset(desc, 0, sizeof(*desc)); @@ -260,13 +299,15 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, /* * NOTE: -* wIndex shows the port number and begins from 1. +* wIndex (bits 0-7) shows the port number and begins from 1? */ + wIndex = ((__u8)(wIndex & 0x00ff)); usbip_dbg_vhci_rh("typeReq %x wValue %x wIndex %x\n", typeReq, wValue, wIndex); + if (wIndex > VHCI_HC_PORTS) pr_err("invalid port number %d\n", wIndex); - rhport = ((__u8)(wIndex & 0x00ff)) - 1; + rhport = wIndex - 1; vhci_hcd = hcd_to_vhci_hcd(hcd); vhci = vhci_hcd->vhci; @@ -286,45 +327,58 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, case ClearPortFeature: switch (wValue) { case USB_PORT_FEAT_SUSPEND: + if (hcd->speed == HCD_USB3) { + pr_err(" ClearPortFeature: USB_PORT_FEAT_SUSPEND req not " + "supported for USB 3.0 roothub\n"); + goto error; + } + usbip_dbg_vhci_rh( + " ClearPortFeature: USB_PORT_FEAT_SUSPEND\n"); if (vhci_hcd->port_status[rhport] & USB_PORT_STAT_SUSPEND) { /* 20msec signaling */ vhci_hcd->resuming = 1; - vhci_hcd->re_timeout = -
[PATCH 7/9] usbip: Add USB_SPEED_SUPER as valid arg
With this patch, USB_SPEED_SUPER is a valid speed when attaching a USB3 SuperSpeed device. Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci_sysfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index cac2319..3ad68ff 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -277,6 +277,7 @@ static int valid_args(__u32 pdev_nr, __u32 rhport, enum usb_device_speed speed) case USB_SPEED_FULL: case USB_SPEED_HIGH: case USB_SPEED_WIRELESS: + case USB_SPEED_SUPER: break; default: pr_err("Failed attach request for unsupported USB speed: %s\n", -- 2.7.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
[PATCH 5/9] usbip: vhci-hcd: Set the vhci structure up to work
This patch enables the new vhci structure. Its lock protects both the USB2 hub and the shared USB3 hub. Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci.h | 2 - drivers/usb/usbip/vhci_hcd.c | 206 - drivers/usb/usbip/vhci_rx.c| 16 ++-- drivers/usb/usbip/vhci_sysfs.c | 26 -- 4 files changed, 145 insertions(+), 105 deletions(-) diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h index 62ee537..8a979fc 100644 --- a/drivers/usb/usbip/vhci.h +++ b/drivers/usb/usbip/vhci.h @@ -100,8 +100,6 @@ struct vhci { struct vhci_hcd { struct vhci *vhci; - spinlock_t lock; - u32 port_status[VHCI_HC_PORTS]; unsigned resuming:1; diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 2bc77ee..8cfba1d 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -123,7 +123,8 @@ static void dump_port_status_diff(u32 prev_status, u32 new_status) void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed) { - struct vhci_hcd *vhci = vdev_to_vhci_hcd(vdev); + struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev); + struct vhci *vhci = vhci_hcd->vhci; int rhport = vdev->rhport; u32 status; unsigned long flags; @@ -132,7 +133,7 @@ void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed) spin_lock_irqsave(&vhci->lock, flags); - status = vhci->port_status[rhport]; + status = vhci_hcd->port_status[rhport]; status |= USB_PORT_STAT_CONNECTION | (1 << USB_PORT_FEAT_C_CONNECTION); @@ -147,16 +148,17 @@ void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed) break; } - vhci->port_status[rhport] = status; + vhci_hcd->port_status[rhport] = status; spin_unlock_irqrestore(&vhci->lock, flags); - usb_hcd_poll_rh_status(vhci_hcd_to_hcd(vhci)); + usb_hcd_poll_rh_status(vhci_hcd_to_hcd(vhci_hcd)); } static void rh_port_disconnect(struct vhci_device *vdev) { - struct vhci_hcd *vhci = vdev_to_vhci_hcd(vdev); + struct vhci_hcd *vhci_hcd = vdev_to_vhci_hcd(vdev); + struct vhci *vhci = vhci_hcd->vhci; int rhport = vdev->rhport; u32 status; unsigned long flags; @@ -165,15 +167,15 @@ static void rh_port_disconnect(struct vhci_device *vdev) spin_lock_irqsave(&vhci->lock, flags); - status = vhci->port_status[rhport]; + status = vhci_hcd->port_status[rhport]; status &= ~USB_PORT_STAT_CONNECTION; status |= (1 << USB_PORT_FEAT_C_CONNECTION); - vhci->port_status[rhport] = status; + vhci_hcd->port_status[rhport] = status; spin_unlock_irqrestore(&vhci->lock, flags); - usb_hcd_poll_rh_status(vhci_hcd_to_hcd(vhci)); + usb_hcd_poll_rh_status(vhci_hcd_to_hcd(vhci_hcd)); } #define PORT_C_MASK\ @@ -196,17 +198,15 @@ static void rh_port_disconnect(struct vhci_device *vdev) */ static int vhci_hub_status(struct usb_hcd *hcd, char *buf) { - struct vhci_hcd *vhci; - int retval; + struct vhci_hcd *vhci_hcd = hcd_to_vhci_hcd(hcd); + struct vhci *vhci = vhci_hcd->vhci; + int retval = DIV_ROUND_UP(VHCI_HC_PORTS + 1, 8); int rhport; int changed = 0; unsigned long flags; - retval = DIV_ROUND_UP(VHCI_HC_PORTS + 1, 8); memset(buf, 0, retval); - vhci = hcd_to_vhci_hcd(hcd); - spin_lock_irqsave(&vhci->lock, flags); if (!HCD_HW_ACCESSIBLE(hcd)) { usbip_dbg_vhci_rh("hw accessible flag not on?\n"); @@ -215,7 +215,7 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf) /* check pseudo status register for each port */ for (rhport = 0; rhport < VHCI_HC_PORTS; rhport++) { - if ((vhci->port_status[rhport] & PORT_C_MASK)) { + if ((vhci_hcd->port_status[rhport] & PORT_C_MASK)) { /* The status of a port has been changed, */ usbip_dbg_vhci_rh("port %d status changed\n", rhport); @@ -247,7 +247,8 @@ static inline void hub_descriptor(struct usb_hub_descriptor *desc) static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex, char *buf, u16 wLength) { - struct vhci_hcd *dum; + struct vhci_hcd *vhci_hcd; + struct vhci *vhci; int retval = 0; int rhport; unsigned long flags; @@ -267,13 +268,14 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, pr_err("invalid port number %d\n", wIndex); rhport = ((__u8)(wIndex & 0x00ff)) - 1; - dum = hcd_to_vhci_hcd(hcd); + vhci_hcd = hcd_to_vhci_hcd(hcd); +
[PATCH 1/9] usbip: vhci-hcd: Rename function names to reflect their struct names
These helper function names are renamed to have their full struct names to avoid confusion: - hcd_to_vhci() -> hcd_to_vhci_hcd() - vhci_to_hcd() -> vhci_hcd_to_hcd() - vdev_to_vhci() -> vdev_to_vhci_hcd() Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci.h | 11 +-- drivers/usb/usbip/vhci_hcd.c | 34 +- drivers/usb/usbip/vhci_rx.c| 12 ++-- drivers/usb/usbip/vhci_sysfs.c | 6 +++--- 4 files changed, 31 insertions(+), 32 deletions(-) diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h index 88b71c4..bff472f 100644 --- a/drivers/usb/usbip/vhci.h +++ b/drivers/usb/usbip/vhci.h @@ -134,7 +134,7 @@ static inline int port_to_pdev_nr(__u32 port) return port / VHCI_HC_PORTS; } -static inline struct vhci_hcd *hcd_to_vhci(struct usb_hcd *hcd) +static inline struct vhci_hcd *hcd_to_vhci_hcd(struct usb_hcd *hcd) { return (struct vhci_hcd *) (hcd->hcd_priv); } @@ -149,15 +149,14 @@ static inline const char *hcd_name(struct usb_hcd *hcd) return (hcd)->self.bus_name; } -static inline struct usb_hcd *vhci_to_hcd(struct vhci_hcd *vhci) +static inline struct usb_hcd *vhci_hcd_to_hcd(struct vhci_hcd *vhci_hcd) { - return container_of((void *) vhci, struct usb_hcd, hcd_priv); + return container_of((void *) vhci_hcd, struct usb_hcd, hcd_priv); } -static inline struct vhci_hcd *vdev_to_vhci(struct vhci_device *vdev) +static inline struct vhci_hcd *vdev_to_vhci_hcd(struct vhci_device *vdev) { - return container_of( - (void *)(vdev - vdev->rhport), struct vhci_hcd, vdev); + return container_of((void *)(vdev - vdev->rhport), struct vhci_hcd, vdev); } #endif /* __USBIP_VHCI_H */ diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 5d8b2c2..624265a 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -124,7 +124,7 @@ static void dump_port_status_diff(u32 prev_status, u32 new_status) void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed) { - struct vhci_hcd *vhci = vdev_to_vhci(vdev); + struct vhci_hcd *vhci = vdev_to_vhci_hcd(vdev); int rhport = vdev->rhport; u32 status; unsigned long flags; @@ -152,12 +152,12 @@ void rh_port_connect(struct vhci_device *vdev, enum usb_device_speed speed) spin_unlock_irqrestore(&vhci->lock, flags); - usb_hcd_poll_rh_status(vhci_to_hcd(vhci)); + usb_hcd_poll_rh_status(vhci_hcd_to_hcd(vhci)); } static void rh_port_disconnect(struct vhci_device *vdev) { - struct vhci_hcd *vhci = vdev_to_vhci(vdev); + struct vhci_hcd *vhci = vdev_to_vhci_hcd(vdev); int rhport = vdev->rhport; u32 status; unsigned long flags; @@ -174,7 +174,7 @@ static void rh_port_disconnect(struct vhci_device *vdev) vhci->port_status[rhport] = status; spin_unlock_irqrestore(&vhci->lock, flags); - usb_hcd_poll_rh_status(vhci_to_hcd(vhci)); + usb_hcd_poll_rh_status(vhci_hcd_to_hcd(vhci)); } #define PORT_C_MASK\ @@ -206,7 +206,7 @@ static int vhci_hub_status(struct usb_hcd *hcd, char *buf) retval = DIV_ROUND_UP(VHCI_HC_PORTS + 1, 8); memset(buf, 0, retval); - vhci = hcd_to_vhci(hcd); + vhci = hcd_to_vhci_hcd(hcd); spin_lock_irqsave(&vhci->lock, flags); if (!HCD_HW_ACCESSIBLE(hcd)) { @@ -268,7 +268,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, pr_err("invalid port number %d\n", wIndex); rhport = ((__u8)(wIndex & 0x00ff)) - 1; - dum = hcd_to_vhci(hcd); + dum = hcd_to_vhci_hcd(hcd); spin_lock_irqsave(&dum->lock, flags); @@ -440,7 +440,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) pr_err("could not get virtual device"); return; } - vhci = vdev_to_vhci(vdev); + vhci = vdev_to_vhci_hcd(vdev); priv = kzalloc(sizeof(struct vhci_priv), GFP_ATOMIC); if (!priv) { @@ -468,7 +468,7 @@ static void vhci_tx_urb(struct urb *urb, struct vhci_device *vdev) static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flags) { - struct vhci_hcd *vhci = hcd_to_vhci(hcd); + struct vhci_hcd *vhci = hcd_to_vhci_hcd(hcd); struct device *dev = &urb->dev->dev; u8 portnum = urb->dev->portnum; int ret = 0; @@ -635,7 +635,7 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, */ static int vhci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status) { - struct vhci_hcd *vhci = hcd_to_vhci(hcd); + struct vhci_hcd *vhci = hcd_to_vhci_hcd(hcd); struct vhci_priv *priv; struct vhci_device *vdev; unsigned long flags; @@ -686,7 +686,7 @@ static int vhci
[PATCH 2/9] usbip: vhci-hcd: Add vhci struct
In order to support SuperSpeed devices, a USB3 HCD is added to share the USB2 HCD. As a result, a "VHCI" is composed of two vhci_hcds associated with the two HCDs respectively. So we add another level of abstraction, vhci, and thus this vhci structure. Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci.h | 12 +++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h index bff472f..9959584 100644 --- a/drivers/usb/usbip/vhci.h +++ b/drivers/usb/usbip/vhci.h @@ -87,8 +87,17 @@ struct vhci_unlink { #define MAX_STATUS_NAME 16 -/* for usb_bus.hcpriv */ +struct vhci { + spinlock_t lock; + + struct vhci_hcd *vhci_hcd_hs; + struct vhci_hcd *vhci_hcd_ss; +}; + +/* for usb_hcd.hcd_priv[0] */ struct vhci_hcd { + struct vhci *vhci; + spinlock_t lock; u32 port_status[VHCI_HC_PORTS]; @@ -108,6 +117,7 @@ struct vhci_hcd { extern int vhci_num_controllers; extern struct platform_device **vhci_pdevs; +extern struct vhci *vhcis; extern struct attribute_group vhci_attr_group; /* vhci_hcd.c */ -- 2.7.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
[PATCH 4/9] usbip: vhci-hcd: Rework vhci_hcd_init
A vhci struct is added as the platform-specific data to the vhci platform device, in order to get the vhci by its platform device. This is done in vhci_hcd_init(). Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci_hcd.c | 51 tools/usb/usbip/libsrc/vhci_driver.c | 2 +- tools/usb/usbip/libsrc/vhci_driver.h | 1 + 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 7a04497..2bc77ee 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -1173,24 +1173,6 @@ static struct platform_driver vhci_driver = { }, }; -static int add_platform_device(int id) -{ - struct platform_device *pdev; - int dev_nr; - - if (id == 0) - dev_nr = -1; - else - dev_nr = id; - - pdev = platform_device_register_simple(driver_name, dev_nr, NULL, 0); - if (IS_ERR(pdev)) - return PTR_ERR(pdev); - - vhcis[id].pdev = pdev; - return 0; -} - static void del_platform_devices(void) { struct platform_device *pdev; @@ -1219,23 +1201,46 @@ static int __init vhci_hcd_init(void) if (vhcis == NULL) return -ENOMEM; + for (i = 0; i < vhci_num_controllers; i++) { + vhcis[i].pdev = platform_device_alloc(driver_name, i); + if (!vhcis[i].pdev) { + i--; + while (i >= 0) + platform_device_put(vhcis[i--].pdev); + ret = -ENOMEM; + goto err_device_alloc; + } + } + for (i = 0; i < vhci_num_controllers; i++) { + void *vhci = &vhcis[i]; + ret = platform_device_add_data(vhcis[i].pdev, &vhci, sizeof(void *)); + if (ret) + goto err_driver_register; + } + ret = platform_driver_register(&vhci_driver); if (ret) goto err_driver_register; for (i = 0; i < vhci_num_controllers; i++) { - ret = add_platform_device(i); - if (ret) - goto err_platform_device_register; + ret = platform_device_add(vhcis[i].pdev); + if (ret < 0) { + i--; + while (i >= 0) + platform_device_del(vhcis[i--].pdev); + goto err_add_hcd; + } } pr_info(DRIVER_DESC " v" USBIP_VERSION "\n"); return ret; -err_platform_device_register: - del_platform_devices(); +err_add_hcd: platform_driver_unregister(&vhci_driver); err_driver_register: + for (i = 0; i < vhci_num_controllers; i++) + platform_device_put(vhcis[i].pdev); +err_device_alloc: kfree(vhcis); return ret; } diff --git a/tools/usb/usbip/libsrc/vhci_driver.c b/tools/usb/usbip/libsrc/vhci_driver.c index af88447..5b19a32 100644 --- a/tools/usb/usbip/libsrc/vhci_driver.c +++ b/tools/usb/usbip/libsrc/vhci_driver.c @@ -248,7 +248,7 @@ int usbip_vhci_driver_open(void) vhci_driver->hc_device = udev_device_new_from_subsystem_sysname(udev_context, USBIP_VHCI_BUS_TYPE, - USBIP_VHCI_DRV_NAME); + USBIP_VHCI_DEVICE_NAME); if (!vhci_driver->hc_device) { err("udev_device_new_from_subsystem_sysname failed"); goto err; diff --git a/tools/usb/usbip/libsrc/vhci_driver.h b/tools/usb/usbip/libsrc/vhci_driver.h index 33add14..dfe19c1 100644 --- a/tools/usb/usbip/libsrc/vhci_driver.h +++ b/tools/usb/usbip/libsrc/vhci_driver.h @@ -11,6 +11,7 @@ #include "usbip_common.h" #define USBIP_VHCI_BUS_TYPE "platform" +#define USBIP_VHCI_DEVICE_NAME "vhci_hcd.0" #define MAXNPORT 128 struct usbip_imported_device { -- 2.7.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
[PATCH 3/9] usbip: vhci-hcd: Move VHCI platform device into vhci struct
Every VHCI is a platform device, so move the platform_device struct into the VHCI struct. Signed-off-by: Yuyang Du --- drivers/usb/usbip/vhci.h | 3 ++- drivers/usb/usbip/vhci_hcd.c | 17 - drivers/usb/usbip/vhci_sysfs.c | 6 +++--- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/drivers/usb/usbip/vhci.h b/drivers/usb/usbip/vhci.h index 9959584..62ee537 100644 --- a/drivers/usb/usbip/vhci.h +++ b/drivers/usb/usbip/vhci.h @@ -90,6 +90,8 @@ struct vhci_unlink { struct vhci { spinlock_t lock; + struct platform_device *pdev; + struct vhci_hcd *vhci_hcd_hs; struct vhci_hcd *vhci_hcd_ss; }; @@ -116,7 +118,6 @@ struct vhci_hcd { }; extern int vhci_num_controllers; -extern struct platform_device **vhci_pdevs; extern struct vhci *vhcis; extern struct attribute_group vhci_attr_group; diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c index 624265a..7a04497 100644 --- a/drivers/usb/usbip/vhci_hcd.c +++ b/drivers/usb/usbip/vhci_hcd.c @@ -58,8 +58,7 @@ static const char driver_name[] = "vhci_hcd"; static const char driver_desc[] = "USB/IP Virtual Host Controller"; int vhci_num_controllers = VHCI_NR_HCS; - -struct platform_device **vhci_pdevs; +struct vhci *vhcis; static const char * const bit_desc[] = { "CONNECTION", /*0*/ @@ -1188,7 +1187,7 @@ static int add_platform_device(int id) if (IS_ERR(pdev)) return PTR_ERR(pdev); - *(vhci_pdevs + id) = pdev; + vhcis[id].pdev = pdev; return 0; } @@ -1198,10 +1197,10 @@ static void del_platform_devices(void) int i; for (i = 0; i < vhci_num_controllers; i++) { - pdev = *(vhci_pdevs + i); + pdev = vhcis[i].pdev; if (pdev != NULL) platform_device_unregister(pdev); - *(vhci_pdevs + i) = NULL; + vhcis[i].pdev = NULL; } sysfs_remove_link(&platform_bus.kobj, driver_name); } @@ -1216,8 +1215,8 @@ static int __init vhci_hcd_init(void) if (vhci_num_controllers < 1) vhci_num_controllers = 1; - vhci_pdevs = kcalloc(vhci_num_controllers, sizeof(void *), GFP_KERNEL); - if (vhci_pdevs == NULL) + vhcis = kcalloc(vhci_num_controllers, sizeof(struct vhci), GFP_KERNEL); + if (vhcis == NULL) return -ENOMEM; ret = platform_driver_register(&vhci_driver); @@ -1237,7 +1236,7 @@ static int __init vhci_hcd_init(void) del_platform_devices(); platform_driver_unregister(&vhci_driver); err_driver_register: - kfree(vhci_pdevs); + kfree(vhcis); return ret; } @@ -1245,7 +1244,7 @@ static void __exit vhci_hcd_exit(void) { del_platform_devices(); platform_driver_unregister(&vhci_driver); - kfree(vhci_pdevs); + kfree(vhcis); } module_init(vhci_hcd_init); diff --git a/drivers/usb/usbip/vhci_sysfs.c b/drivers/usb/usbip/vhci_sysfs.c index d878faa..07f0d37 100644 --- a/drivers/usb/usbip/vhci_sysfs.c +++ b/drivers/usb/usbip/vhci_sysfs.c @@ -32,7 +32,7 @@ /* Sysfs entry to show port status */ static ssize_t status_show_vhci(int pdev_nr, char *out) { - struct platform_device *pdev = *(vhci_pdevs + pdev_nr); + struct platform_device *pdev = vhcis[pdev_nr].pdev; struct vhci_hcd *vhci; char *s = out; int i = 0; @@ -206,7 +206,7 @@ static ssize_t store_detach(struct device *dev, struct device_attribute *attr, if (!valid_port(pdev_nr, rhport)) return -EINVAL; - hcd = platform_get_drvdata(*(vhci_pdevs + pdev_nr)); + hcd = platform_get_drvdata(vhcis[pdev_nr].pdev); if (hcd == NULL) { dev_err(dev, "port is not ready %u\n", port); return -EAGAIN; @@ -287,7 +287,7 @@ static ssize_t store_attach(struct device *dev, struct device_attribute *attr, if (!valid_args(pdev_nr, rhport, speed)) return -EINVAL; - hcd = platform_get_drvdata(*(vhci_pdevs + pdev_nr)); + hcd = platform_get_drvdata(vhcis[pdev_nr].pdev); if (hcd == NULL) { dev_err(dev, "port %d is not ready\n", port); return -EAGAIN; -- 2.7.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