Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[Thinh Nguyen]

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

[Woojung.Huh]

> 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

[Christoph Hellwig]

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);
+   

Re: [PATCH v3] smsc95xx: Add comments to the registers definition

[martin]

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

[Frederic Weisbecker]

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(>sched_timer, now, tick_period);
tick_program_event(hrtimer_get_expires(>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(>sched_timer, now, tick_period);
 	tick_program_event(hrtimer_get_expires(>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

[Bjorn Helgaas]

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
> 

Re: [PATCH v3] smsc95xx: Add comments to the registers definition

[David Miller]

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

[Alan Stern]

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

[Woojung.Huh]

> +/* 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

[Niranjan Dighe]

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(>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(>gadget);
>>   usb_gadget_set_state(>gadget,
>>   USB_STATE_SUSPENDED);
>> + ci_hdrc_otg_fsm_restart(ci);
>>   spin_lock(>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

[Andrew Lunn]

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

[Peter Chen]

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(>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(>gadget);
>   usb_gadget_set_state(>gadget,
>   USB_STATE_SUSPENDED);
> + ci_hdrc_otg_fsm_restart(ci);
>   spin_lock(>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

[Sekhar Nori]

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= _clk,
 +};
 +
   static struct clk aemif_clk = {
   .name= "aemif",
   .parent= _sysclk3,
 @@ -413,6 +418,7 @@ static struct clk_lookup da830_clks[] = {
   CLK("davinci-mcasp.1",NULL,_clk),
   CLK("davinci-mcasp.2",NULL,_clk),
   CLK("musb-da8xx","usb20",_clk),
 +CLK("cppi41-dmaengine",NULL,_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=149080474124498=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

[Felipe Balbi]

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

[David Laight]

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

[Felipe Balbi]

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

[Felipe Balbi]

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

[David Laight]

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

[David Laight]

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

[Steve Glendinning]

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

[Martin Wetterwald]

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_ 

Re: [PATCH 7/9] usbip: Add USB_SPEED_SUPER as valid arg

[Oliver Neukum]

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

[Oliver Neukum]

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

[Ralph Sennhauser]

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 

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

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

[John Youn]

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.

>
> 

[PATCH 9/9] usbip: vhci-hcd: Clean up the code by adding a new macro

[Yuyang Du]

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(>ud.lock);
port_show_vhci(, HUB_SPEED_HIGH,
-  pdev_nr * VHCI_HC_PORTS * 2 + i, vdev);
+  pdev_nr * VHCI_PORTS + i, vdev);
spin_unlock(>ud.lock);
}
 
@@ -101,7 +101,7 @@ static ssize_t status_show_vhci(int pdev_nr, char *out)
 
spin_lock(>ud.lock);
port_show_vhci(, 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(>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

[Yuyang Du]

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

[Yuyang Du]

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

[Yuyang Du]

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;
-   

[PATCH 7/9] usbip: Add USB_SPEED_SUPER as valid arg

[Yuyang Du]

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

[Yuyang Du]

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(>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(>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(>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(>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(>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

[Yuyang Du]

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(>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(>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(>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(>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 = >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 

[PATCH 2/9] usbip: vhci-hcd: Add vhci struct

[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.

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 3/9] usbip: vhci-hcd: Move VHCI platform device into vhci struct

[Yuyang Du]

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(_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(_driver);
@@ -1237,7 +1236,7 @@ static int __init vhci_hcd_init(void)
del_platform_devices();
platform_driver_unregister(_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(_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

[PATCH 4/9] usbip: vhci-hcd: Rework vhci_hcd_init

[Yuyang Du]

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 = [i];
+   ret = platform_device_add_data(vhcis[i].pdev, , 
sizeof(void *));
+   if (ret)
+   goto err_driver_register;
+   }
+
ret = platform_driver_register(_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(_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

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Greg KH]

On Wed, Apr 12, 2017 at 09:01:44AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Greg KH  writes:
> > On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
> >> On Tue, 11 Apr 2017, Felipe Balbi wrote:
> >> 
> >> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
> >> > > up every time it gets called, in its current form.
> >> > 
> >> > Well, it does :-)
> >> > 
> >> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
> >> > 
> >> > We're just leaking memory. I guess a patch like below would be best:
> >> > 
> >> > diff --git a/drivers/usb/gadget/udc/net2280.c 
> >> > b/drivers/usb/gadget/udc/net2280.c
> >> > index 3828c2ec8623..4dc04253da61 100644
> >> > --- a/drivers/usb/gadget/udc/net2280.c
> >> > +++ b/drivers/usb/gadget/udc/net2280.c
> >> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void 
> >> > *_dev)
> >> >  
> >> >  
> >> > /*-*/
> >> >  
> >> > -static void gadget_release(struct device *_dev)
> >> > -{
> >> > -struct net2280  *dev = dev_get_drvdata(_dev);
> >> > -
> >> > -kfree(dev);
> >> > -}
> >> > -
> >> >  /* tear down the binding between this driver and the pci device */
> >> >  
> >> >  static void net2280_remove(struct pci_dev *pdev)
> >> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
> >> >  device_remove_file(>dev, _attr_registers);
> >> >  
> >> >  ep_info(dev, "unbind\n");
> >> > +
> >> > +kfree(dev);
> >> >  }
> >> >  
> >> >  /* wrap this driver around the specified device, but
> >> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, 
> >> > const struct pci_device_id *id)
> >> >  if (retval)
> >> >  goto done;
> >> >  
> >> > -retval = usb_add_gadget_udc_release(>dev, >gadget,
> >> > -gadget_release);
> >> > +retval = usb_add_gadget_udc(>dev, >gadget);
> >> >  if (retval)
> >> >  goto done;
> >> >  return 0;
> >> 
> >> 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
> :-)

Please don't say that, that's even worse :(

> 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.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[Felipe Balbi]

Hi,

(Thinh, for whatever I didn't receive your email via the list, replying
to myself)

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.

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;
 }
 
-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[Felipe Balbi]

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?

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, this is either a bug in IRQ subsystem
which only John and Thinh can reproduce at this moment, or a regression
with DWC3 IP Core :-s

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[Felipe Balbi]

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.

right, dwc3.ko isn't using MSI/MSI-X though. Unless PCI bus is handling
that for us, I suppose we would be relying on the legacy physical
interrupt line, no?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[Felipe Balbi]

Hi,

John Youn  writes:
 Do you think we introduce this when adding evt->cache in TH?
 Even without evt->cache we still could overwrite evt->count - so, was
 that seen before evt->cache?
>>>
>>> The multiple TH calls could have happened even before we introduced
>>> evt->cache, but only with the cache would it have caused a failure due
>>> to overwriting the cached events on multiple calls.
>>
>> Right, multiple TH calls should NEVER happen, because our IRQ line is
>> masked. Unless, and this is a long shot, IRQ line is shared and ANOTHER
>> device is causing IRQ to be raised. Can you show the output of:
>
> We thought about this and ensured that there is no sharing of the IRQ.
> We still see the dual calls to the top-half.
>
>>
>> # grep dwc3 /proc/interrupts
>>
>> If another device raises the interrupt, then we will get into our TH,
>> however we should just return IRQ_HANDLED in that case because we
>> shouldn't be generating events.
>
> No, we will still be generating events. The masking of the interrupt
> just deasserts the interrupt line. Events are still written out as
> usual.

Yes, events should be written to event buffer, but the IRQ line
shouldn't assert which means that our IRQ handler shouldn't get
called. If we really _are_ getting called, then we either have shared
IRQ line (which we must cope with, anyway) or there's a spurious IRQ
triggering.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: dwc3: gadget: Fix early exit in set/clear ep halt

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
 Felipe Balbi  writes:
> Thinh Nguyen  writes:
>> This patch fixes a commit that causes a hang from device waiting for
>> data with the wrong sequence number. The commit ffb80fc672c3 ("usb:
>> dwc3: gadget: skip Set/Clear Halt when invalid") adds a check to return
>> early depending on DWC3_EP_STALL is set or not, prevent sending the ep
>> halt command to HW endpoint to do CLEAR_FEATURE(ENDPOINT_HALT) request.
>> This was to workaround the issue for macOS where the device hangs from
>> sending DWC3 clear stall command.
>>
>> In USB 3.1 spec, 9.4.5, CLEAR_FEATURE(ENDPOINT_HALT) request always
>> results in the data sequence being reinitialized to zero regardless
>> whether the endpoint has been halted or not. Some device class depends
>> on this feature for its protocol. For instance, in mass storage class,
>> there is MSC reset protocol that does CLEAR_FEATURE(ENDPOINT_HALT) on
>> bulk endpoints. This protocol reinitializes the data sequence and
>> ensures that whatever pending data requested from previous CBW will be
>> reset. Otherwise this will cause a hang as the device can wait for the
>> data with the wrong sequence number from the previous CBW. We found this
>> failure in USB CV: MSC Error Recovery Test with f_mass_storage.
>>
>> This patch fixes this issue by checking to see whether the set/halt ep
>> call is a protocol call before early exit to make sure that set/clear
>> halt endpoint command can go through if it is a device class protocol.
>>
>> Fixes: ffb80fc672c3 ("usb: dwc3: gadget: skip Set/Clear Halt when 
>> invalid")
>> Signed-off-by: Thinh Nguyen 
>
> this will regress the macOS case I wrote that commit for. We need to

 no wait, it won't regress macOS, but we're still left with the problem
 of host and peripheral being able to get DataToggle/SeqN out of sync.

>>>
>>> This patch is for the regression we have. Can you provide more
>>> information for the macOS? I'm not sure if this is the case for macOS,
>> 
>> I need to find a way to reproduce it again first. When I first
>> reproduced it was with dwc3 running adb and connecting it to a macOS
>> machine.
>> 
>>> but maybe there is still pending transfer when it tries to send the
>>> request? (There shouldn't be any before issuing ClearStall command). Do
>> 
>> this could be, I don't remember if I checked this or not :-)
>> 
>> Really, the best way here, IMHO, would be to re-verify what's going on
>> with macOS and revert my orignal patch since it's, rather clearly,
>> wrong.
>> 
>
> Sure. Are you going to make a revert patch or I am?

Well, after we really know what's going on with macOS and have a better
fix, then who makes the revert is less important as long as problems get
sorted out :-) Either way is fine for me.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/3] usb: udc: allow adding and removing the same gadget device

[Felipe Balbi]

Hi,

Greg KH  writes:
> On Tue, Apr 11, 2017 at 10:12:01AM -0400, Alan Stern wrote:
>> On Tue, 11 Apr 2017, Felipe Balbi wrote:
>> 
>> > > Oddly enough, yes.  But it doesn't explain why this code doesn't blow 
>> > > up every time it gets called, in its current form.
>> > 
>> > Well, it does :-)
>> > 
>> > dev_get_drvdata(_dev) -> NULL -> kfree(NULL)
>> > 
>> > We're just leaking memory. I guess a patch like below would be best:
>> > 
>> > diff --git a/drivers/usb/gadget/udc/net2280.c 
>> > b/drivers/usb/gadget/udc/net2280.c
>> > index 3828c2ec8623..4dc04253da61 100644
>> > --- a/drivers/usb/gadget/udc/net2280.c
>> > +++ b/drivers/usb/gadget/udc/net2280.c
>> > @@ -3555,13 +3555,6 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>> >  
>> >  
>> > /*-*/
>> >  
>> > -static void gadget_release(struct device *_dev)
>> > -{
>> > -  struct net2280  *dev = dev_get_drvdata(_dev);
>> > -
>> > -  kfree(dev);
>> > -}
>> > -
>> >  /* tear down the binding between this driver and the pci device */
>> >  
>> >  static void net2280_remove(struct pci_dev *pdev)
>> > @@ -3598,6 +3591,8 @@ static void net2280_remove(struct pci_dev *pdev)
>> >device_remove_file(>dev, _attr_registers);
>> >  
>> >ep_info(dev, "unbind\n");
>> > +
>> > +  kfree(dev);
>> >  }
>> >  
>> >  /* wrap this driver around the specified device, but
>> > @@ -3775,8 +3770,7 @@ static int net2280_probe(struct pci_dev *pdev, const 
>> > struct pci_device_id *id)
>> >if (retval)
>> >goto done;
>> >  
>> > -  retval = usb_add_gadget_udc_release(>dev, >gadget,
>> > -  gadget_release);
>> > +  retval = usb_add_gadget_udc(>dev, >gadget);
>> >if (retval)
>> >goto done;
>> >return 0;
>> 
>> 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;
}

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.

-- 
balbi


signature.asc
Description: PGP signature