RE: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests

[Felipe Balbi]

Hi,

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.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests

[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

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 3/6] usb: usbip tool: Check the return of get_nports()

[Yuyang Du]

On Tue, Apr 11, 2017 at 08:51:13AM -0600, Shuah Khan wrote:
>
> Please bear with me. I was out sick most of last week. till catching

Hope you are all well now :)

> Could you please pick this up.
> 
> Acked-by: Shuah Khan 

Thanks, Shuah.

Yuyang
--
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 3/6] usb: usbip tool: Check the return of get_nports()

[Yuyang Du]

Hi Greg,

On Tue, Apr 11, 2017 at 04:13:50PM +0200, Greg KH wrote:
> 
> Please send patches out, no need to wait, I'll just wait to apply them
> for the review, you can always build on top of previous series.
 
Got it. Thank you.

Yuyang
--
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] smsc95xx: Add comments to the registers definition

[David Miller]

From: Andrew Lunn 
Date: Mon, 10 Apr 2017 15:52:51 +0200

> Hi Martin
> 
>> @@ -2032,7 +2032,7 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet 
>> *dev,
>>  skb_push(skb, 4);
>>  tx_cmd_b = (u32)(skb->len - 4);
>>  if (csum)
>> -tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
>> +tx_cmd_b |= TX_CMD_B_CSUM_EN;
> 
> This changed seems a step backwards, ENABLE is much more readable than EN.
> 
>>  
>> -#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_EN(0x4000)/* TX Checksum Enable */
> 
> And there is space for ABLE here.

I completely agree, Martin please don't ENABLE to EN.
--
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 0/3] usb: host: xhci-plat: add support suspend/resume for R-Car

[Yoshihiro Shimoda]

Hi Mathias,

> From: Yoshihiro Shimoda, Sent: Friday, March 17, 2017 7:03 PM
> 
> This patch adds support suspend/resume for R-Car controllers.
> The controllers need firmware downloading and the current code has
> such process only when probe timing. After suspend the system and
> the power of controller down, the driver needs to re-download the firmware
> in resume timing. This patch adds such a process.
> Also, since previous xhci-plat code didnn't handle the clk in suspend/resume,
> this patch set has such process.

I'm afraid but would you review this patch set?
This can be applied on the latest Greg's usb.git / usb-next branch at the 
moment.

Best regards,
Yoshihiro Shimoda

> Yoshihiro Shimoda (3):
>   usb: host: xhci-plat: enable clk in resume timing
>   usb: host: xhci-plat: add resume_quirk()
>   usb: host: xhci-plat: set resume_quirk() for R-Car controllers
> 
>  drivers/usb/host/xhci-plat.c | 29 -
>  drivers/usb/host/xhci-plat.h |  1 +
>  drivers/usb/host/xhci-rcar.c | 11 +++
>  drivers/usb/host/xhci-rcar.h |  6 ++
>  4 files changed, 46 insertions(+), 1 deletion(-)
> 
> --
> 1.9.1

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

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

[John Youn]

On 04/11/2017 09:31 AM, John Youn wrote:
> On 04/11/2017 12:45 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> 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.

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.

Unfortunately the device controller does not support MSI interrupts,
where it would be edge triggered and you would only see an assertion
packet (no de-assertion). With that you would probably not see this
behavior.

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

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

[Thinh Nguyen]

On 4/11/2017 12:36 AM, Felipe Balbi wrote:


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?

Thanks,
Thinh
--
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: gadget: f_mass_storage: Serialize wake and sleep execution

[Thinh Nguyen]

On 4/11/2017 7:35 AM, Alan Stern wrote:

On Mon, 10 Apr 2017, Thinh Nguyen wrote:


To fix this problem, both the smp_rmb() in sleep_thread() and the
smp_wmb() in wakeup_thread() should be changed to smp_mb().


Wouldn't it be sufficient to have smp_mb() just either in the
sleep_thread() or the wakeup_thread() in this case?


I don't think so, not in this case.  The real problem here is that the
code in sleep_thread() tests thread_wakeup_needed, but the actual
condition it wants to see is bh->state == BH_STATE_FULL.  This is one
of the reasons the code should be rewritten.


With just 1 smp_mb(), either bh->state or thread_wakeup_needed is set in
time to break the loop depending on when the wakeup_thread() enters.

Case 1: smp_mb() in sleep_thread()
bh->state = BH_STATE_FULL;
smp_wmb();
thread_wakeup_needed = 0;   thread_wakeup_needed = 1;
-->  smp_mb();
if (bh->state != BH_STATE_FULL)
//breaks out of loop from bh->state check


This code would be okay on x86, but it wouldn't work on PowerPC.  You
could end up with thread_wakeup_needed = 0 at the end, but the first
CPU could still see bh->state != BH_STATE_FULL.  So it would put itself
back to sleep and never get woken up.


Case 2: smp_mb() in wakeup_thread()

thread_wakeup_needed = 0;   
smp_rmb();
if (bh->state != BH_STATE_FULL)
bh->state = BH_STATE_FULL;
-->  smp_mb();
thread_wakeup_needed = 1;
wake_up_process()
set_current_state(intr) //smp_wb
if (thread_wakeup_need)
//breaks out of loop from thread_wakeup_needed check


That is not the right description.  It should look like this:

Case 2: smp_mb() in wakeup_thread()
bh->state = BH_STATE_FULL;
-->  smp_mb();
thread_wakeup_needed = 0;   thread_wakeup_needed = 1;
smp_rmb();  wake_up_process();
if (bh->state != BH_STATE_FULL)
set_current_state(intr) //smp_wb
if (thread_wakeup_needed)
//breaks out of loop from thread_wakeup_needed check

This could fail even on x86; the CPU could still not see BH_STATE_FULL
because the smp_rmb and the read of bh->state could be reordered before
the write to thread_wakeup_needed.


The issue won't be seen when I tested for both cases with smp_mb() in
either the wakeup_thread() or the sleep_thread(). I notice that in
wait_queue (DEFINED_WAIT_FUNC()), it places the smp_mb() in the wait
function, so I follow its implementation.

However, my test maybe insufficient and I maybe wrong in my code review..


For now, I think it would be best to put smp_mb() in both places.
After I do a more thorough rewrite, the memory barriers will make more
sense.


I see.. I'll update the patch with your analysis.

Thanks Alan,
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

[bug report] usb: xhci: Add helper function xhci_set_power_on().

[Dan Carpenter]

Hello Guoqing Zhang,

The patch a6ff6cbf1fab: "usb: xhci: Add helper function
xhci_set_power_on()." from Apr 7, 2017, leads to the following static
checker warning:

drivers/usb/host/xhci-hub.c:575 xhci_set_port_power()
error: calling 'spin_unlock_irqrestore()' with bogus flags

drivers/usb/host/xhci-hub.c
   554  static void xhci_set_port_power(struct xhci_hcd *xhci, struct usb_hcd 
*hcd,
   555  u16 index, bool on)
   556  {
   557  __le32 __iomem *addr;
   558  u32 temp;
   559  unsigned long flags = 0;
   560  
   561  addr = xhci_get_port_io_addr(hcd, index);
   562  temp = readl(addr);
   563  temp = xhci_port_state_to_neutral(temp);
   564  if (on) {
   565  /* Power on */
   566  writel(temp | PORT_POWER, addr);
   567  temp = readl(addr);
   568  xhci_dbg(xhci, "set port power, actual port %d status  
= 0x%x\n",
   569  index, temp);
   570  } else {
   571  /* Power off */
   572  writel(temp & ~PORT_POWER, addr);
   573  }
   574  
   575  spin_unlock_irqrestore(>lock, flags);
^
This won't work at all.  This is zero, not the original flags.  Please
CC me on the fix because I'm not subscribed to linux-usb.

   576  temp = usb_acpi_power_manageable(hcd->self.root_hub,
   577  index);
   578  if (temp)
   579  usb_acpi_set_power_state(hcd->self.root_hub,
   580  index, on);
   581  spin_lock_irqsave(>lock, flags);
   582  }

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

Re: [PATCH v2 00/21] Convert USB documentation to ReST format

[Jonathan Corbet]

On Tue, 11 Apr 2017 16:58:40 +0200
Greg Kroah-Hartman  wrote:

> Nope, they don't apply to my tree, it was probably based on yours.  And
> the first two are ones I shouldn't be taking.
> 
> So, feel free to take all of these with a:
>   Acked-by: Greg Kroah-Hartman 
> for the USB-related patches (2-21).

I have now done that.  Thanks, Mauro!

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

Re: [PATCH v2 00/21] Convert USB documentation to ReST format

[Greg Kroah-Hartman]

On Tue, Apr 11, 2017 at 03:36:39PM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 11 Apr 2017 16:58:40 +0200
> Greg Kroah-Hartman  escreveu:
> 
> > On Sat, Apr 08, 2017 at 10:04:33PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Apr 08, 2017 at 11:23:28AM -0600, Jonathan Corbet wrote:  
> > > > On Wed,  5 Apr 2017 10:22:54 -0300
> > > > Mauro Carvalho Chehab  wrote:
> > > >   
> > > > > Currently, there are several USB core documents that are at either
> > > > > written in plain text or in DocBook format. Convert them to ReST
> > > > > and add to the driver-api book.  
> > > > 
> > > > Greg, do you see any reason not to apply these for 4.12?  A few of them
> > > > touch comments outside of Documentation/; I'm happy to carry those or
> > > > leave them to you, as you prefer.  
> > > 
> > > I'll queue them up in the next few days, thanks!  
> > 
> > Nope, they don't apply to my tree, it was probably based on yours.  And
> > the first two are ones I shouldn't be taking.
> 
> Yeah, I based it at the docs-next tree. If you prefer, I can rebase
> on your tree, but I guess that the docbook conversion patches
> would likely conflict with some patches at docs-next, because of
> the Makefile changes.

Doesn't bother me, it can go through the Documentation tree as-is.

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 v2 00/21] Convert USB documentation to ReST format

[Mauro Carvalho Chehab]

Em Tue, 11 Apr 2017 16:58:40 +0200
Greg Kroah-Hartman  escreveu:

> On Sat, Apr 08, 2017 at 10:04:33PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 08, 2017 at 11:23:28AM -0600, Jonathan Corbet wrote:  
> > > On Wed,  5 Apr 2017 10:22:54 -0300
> > > Mauro Carvalho Chehab  wrote:
> > >   
> > > > Currently, there are several USB core documents that are at either
> > > > written in plain text or in DocBook format. Convert them to ReST
> > > > and add to the driver-api book.  
> > > 
> > > Greg, do you see any reason not to apply these for 4.12?  A few of them
> > > touch comments outside of Documentation/; I'm happy to carry those or
> > > leave them to you, as you prefer.  
> > 
> > I'll queue them up in the next few days, thanks!  
> 
> Nope, they don't apply to my tree, it was probably based on yours.  And
> the first two are ones I shouldn't be taking.

Yeah, I based it at the docs-next tree. If you prefer, I can rebase
on your tree, but I guess that the docbook conversion patches
would likely conflict with some patches at docs-next, because of
the Makefile changes.

> So, feel free to take all of these with a:
>   Acked-by: Greg Kroah-Hartman 
> for the USB-related patches (2-21).
> 
> thanks,
> 
> greg k-h



Thanks,
Mauro
--
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

[kbuild test robot]

Hi Niranjan,

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.11-rc6]
[cannot apply to next-20170411]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Niranjan-Dighe/chipidea-Fix-issue-in-reconnecing-gadget-without-insmod-rmmod/20170411-235845
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: i386-randconfig-x0-04111920 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "ci_hdrc_otg_fsm_restart" [drivers/usb/chipidea/ci_hdrc.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH] chipidea: Fix issue in reconnecing gadget without insmod/rmmod

[kbuild test robot]

Hi Niranjan,

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.11-rc6]
[cannot apply to next-20170411]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Niranjan-Dighe/chipidea-Fix-issue-in-reconnecing-gadget-without-insmod-rmmod/20170411-235845
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 
usb-testing
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `ci_role_write':
>> debug.c:(.text+0x56ca41): undefined reference to `ci_hdrc_otg_fsm_restart'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

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

[John Youn]

On 04/11/2017 12:45 AM, Felipe Balbi wrote:
>
> Hi,
>
> 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.

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

RE: [PATCH 3/3] usb: dwc3: ep0: improve handling of unaligned OUT requests

[David Laight]

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.

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 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[John Youn]

On 04/10/2017 11:58 PM, Felipe Balbi wrote:
>
> Hi,
>
> John Youn  writes:
 Yes it will re-assert. The interrupt line will remain asserted as long
 events remain in the buffer and it is not masked. So when we
 eventually unmask, the interrupt will be reasserted again immediately.

> BTW, other option here could be using (like Baolin propose some time
> ago) dwc->lock in top half while this is held for BH.
> Question how long spin_lock() will be held in top half...
> I am not sure what option is the best.

 That won't help in this case since you can still have two calls top
 top-half before a call to bottom. The lock only prevents the two from
 stepping on each other, but that should already be the case without
 needing the lock.

>>> Really can we have two TH calls before BH?
>>> Interesting case :)
>>
>> That's what we seem to be seeing. I'm not sure if it is normal or not.
>
> no, that's not normal. If this is happening, there's either a HW bug
> somewhere, or we're not clearing events properly.
>
> Can you provide tracepoints of the failure? Perhaps add a trace_printk()
> call on both BH and TH just so we see when they're both called.
>
>>> You suggest we have something like this:
>>> dwc3 IRQ
>>>kernel irq_mask
>>>   dwc3 TH
>>>  mask dwc3 interrupts
>>>  get evt->count, evt->cache
>>>  write to hw (count)
>>>  return WAKE_THREAD
>>>kernel irq_unmask
>>> a) Before BH start we get another dwc3 IRQ?
>>> b) CPU0 starts with BH while we get dwc3 IRQ on CPU1?
>>>
>>> *) in normal case BH should start and in the end unmask
>>> dwc3_interrupts and after that we could get new irq
>>>
>>> If this could happen then for sure you need dwc->lock protection in TH
>>> while base on your description CPU0 can execute BH and CPU1 can handle
>>> TH - so you have to protect dwc3_event_buffer.
>>>
>>> Could you describe this more? How to reproduce?
>>
>> We can reproduce by running a file transfer continuously. It fails
>> fairly reliably, but it can take a while to hit the condition. We are
>> using our PCIe based HAPS FPGA on x86_64 using mainline kernel.
>
> Okay, please provide tracepoints of the problem in question.
>
>> Thinh can provide which IP versions we tried with.
>
> Thank you
>
>>> 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.

>
> 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 we can do that.

Regards,
John


>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6f6f0b3be3ad..3b8185d81f9f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3005,6 +3005,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void 
> *_evt)
>   unsigned long flags;
>   irqreturn_t ret = IRQ_NONE;
>
> + trace_printk("BH\n");
>   spin_lock_irqsave(>lock, flags);
>   ret = dwc3_process_event_buf(evt);
>   spin_unlock_irqrestore(>lock, flags);
> @@ -3019,6 +3020,8 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
>   u32 count;
>   u32 reg;
>
> + trace_printk("evt->flags %08x\n", evt->flags);
> +
>   if (pm_runtime_suspended(dwc->dev)) {
>   pm_runtime_get(dwc->dev);
>   disable_irq_nosync(dwc->irq_gadget);
> @@ -3027,7 +3030,9 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
>   }
>
>   count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> + trace_printk("GEVNTCOUNT %08x\n", count);
>   count &= DWC3_GEVNTCOUNT_MASK;
> + trace_printk("%u events pending\n", count);
>   if (!count)
>   return IRQ_NONE;
>
> @@ -3036,10 +3041,12 @@ static irqreturn_t dwc3_check_event_buf(struct 
> dwc3_event_buffer *evt)
>
>   /* Mask interrupt */
>   reg = 

[PATCH] chipidea: Fix issue in reconnecing gadget without insmod/rmmod

[Niranjan Dighe]

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);
}
}
-- 
2.1.4

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

Re: [PATCH v2 00/21] Convert USB documentation to ReST format

[Greg Kroah-Hartman]

On Sat, Apr 08, 2017 at 10:04:33PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 08, 2017 at 11:23:28AM -0600, Jonathan Corbet wrote:
> > On Wed,  5 Apr 2017 10:22:54 -0300
> > Mauro Carvalho Chehab  wrote:
> > 
> > > Currently, there are several USB core documents that are at either
> > > written in plain text or in DocBook format. Convert them to ReST
> > > and add to the driver-api book.
> > 
> > Greg, do you see any reason not to apply these for 4.12?  A few of them
> > touch comments outside of Documentation/; I'm happy to carry those or
> > leave them to you, as you prefer.
> 
> I'll queue them up in the next few days, thanks!

Nope, they don't apply to my tree, it was probably based on yours.  And
the first two are ones I shouldn't be taking.

So, feel free to take all of these with a:
Acked-by: Greg Kroah-Hartman 
for the USB-related patches (2-21).

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 v3 3/6] usb: usbip tool: Check the return of get_nports()

[Shuah Khan]

On 04/10/2017 04:38 PM, Yuyang Du wrote:
> Hi Shuah,
> 
> Could you please take a look at these patches? I got more patches
> that sit on these to send out.

Please bear with me. I was out sick most of last week. Still catching
up. Go ahead and send the patches, no need to wait.

> 
> Thanks,
> Yuyang
> 
> On Thu, Apr 06, 2017 at 06:03:24AM +0800, Yuyang Du wrote:
>> If we get nonpositive number of ports, there is no sense to
>> continue, then fail gracefully.
>>
>> In addition, the commit 0775a9cbc694e8c72 ("usbip: vhci extension:
>> modifications to vhci driver") introduced configurable numbers of
>> controllers and ports, but we have a static port number maximum,
>> MAXNPORT. If exceeded, the idev array will be overflown. We fix
>> it by validating the nports to make sure the port number max is
>> not exceeded.
>>
>> Signed-off-by: Yuyang Du 

Greg,

Could you please pick this up.

Acked-by: Shuah Khan 

thanks,
-- Shuah

>> ---
>>  tools/usb/usbip/libsrc/vhci_driver.c | 10 +-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
>> b/tools/usb/usbip/libsrc/vhci_driver.c
>> index f659c14..151580a 100644
>> --- a/tools/usb/usbip/libsrc/vhci_driver.c
>> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
>> @@ -220,9 +220,17 @@ int usbip_vhci_driver_open(void)
>>  }
>>  
>>  vhci_driver->nports = get_nports();
>> -
>>  dbg("available ports: %d", vhci_driver->nports);
>>  
>> +if (vhci_driver->nports <=0) {
>> +err("no available ports");
>> +goto err;
>> +}
>> +else if (vhci_driver->nports > MAXNPORT) {
>> +err("port number exceeds %d", MAXNPORT);
>> +goto err;
>> +}
>> +
>>  if (refresh_imported_device_list())
>>  goto err;
>>  
>> -- 
>> 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: [GIT PULL] USB changes for v4.12 merge window

[Greg KH]

On Tue, Apr 11, 2017 at 11:13:55AM +0300, Felipe Balbi wrote:
> 
> Hi Greg,
> 
> here's my pull request for v4.12 merge window. Changes have been tested
> for Intel Edison where applicable. They have also been soaking for
> linux-next for a while. Let me know if you want anything to be changed.
> 
> There will be a conflict on
> drivers/usb/gadget/udc/{Kconfig,amd5536udc.c} which I solved like this:
> 
> diff --cc drivers/usb/gadget/udc/Kconfig
> index c6cc9d3270ac,c90a4a223916..1c14c283cc47
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@@ -277,7 -293,8 +293,8 @@@ source "drivers/usb/gadget/udc/bdc/Kcon
>   
>   config USB_AMD5536UDC
> tristate "AMD5536 UDC"
>  -  depends on PCI
>  +  depends on USB_PCI
> +   select USB_SNP_CORE
> help
>The AMD5536 UDC is part of the AMD Geode CS5536, an x86 
> southbridge.
>It is a USB Highspeed DMA capable USB device controller. Beside ep0
> diff --cc drivers/usb/gadget/udc/amd5536udc.c
> index 270876b438ab,91d0f1a4dac1..4ecd2f20ea48
> --- a/drivers/usb/gadget/udc/amd5536udc.c
> +++ b/drivers/usb/gadget/udc/amd5536udc.c
> @@@ -618,17 -579,12 +579,12 @@@ static void udc_free_dma_chain(struct u
> DBG(dev, "free chain req = %p\n", req);
>   
> /* do not free first desc., will be done by free for request */
> -   td_last = req->td_data;
> -   td = phys_to_virt(td_last->next);
> - 
> for (i = 1; i < req->chain_len; i++) {
> -   dma_pool_free(dev->data_requests, td,
> - (dma_addr_t)td_last->next);
> -   td_last = td;
> -   td = phys_to_virt(td_last->next);
> +   td = phys_to_virt(addr);
> +   addr_next = (dma_addr_t)td->next;
>  -  pci_pool_free(dev->data_requests, td, addr);
> ++  dma_pool_free(dev->data_requests, td, addr);
> +   addr = addr_next;
> }
> - 
> -   return ret_val;
>   }
>   
>   /* Frees request packet, called by gadget driver */
> 
> Resulting tree still compiles fine, but I can't test. amd5536udc.c :-s

Thanks, I duplicated this merge, and pushed everything out now,
hopefully I got it right :)

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 3/3] usb: gadget: f_mass_storage: Serialize wake and sleep execution

[Alan Stern]

On Mon, 10 Apr 2017, Thinh Nguyen wrote:

>  To fix this problem, both the smp_rmb() in sleep_thread() and the
>  smp_wmb() in wakeup_thread() should be changed to smp_mb().
> 
> Wouldn't it be sufficient to have smp_mb() just either in the 
> sleep_thread() or the wakeup_thread() in this case?

I don't think so, not in this case.  The real problem here is that the
code in sleep_thread() tests thread_wakeup_needed, but the actual
condition it wants to see is bh->state == BH_STATE_FULL.  This is one
of the reasons the code should be rewritten.

> With just 1 smp_mb(), either bh->state or thread_wakeup_needed is set in 
> time to break the loop depending on when the wakeup_thread() enters.
> 
> Case 1: smp_mb() in sleep_thread()
>   bh->state = BH_STATE_FULL;
>   smp_wmb();
>   thread_wakeup_needed = 0;   thread_wakeup_needed = 1;
> -->   smp_mb();
>   if (bh->state != BH_STATE_FULL)
>   //breaks out of loop from bh->state check

This code would be okay on x86, but it wouldn't work on PowerPC.  You 
could end up with thread_wakeup_needed = 0 at the end, but the first 
CPU could still see bh->state != BH_STATE_FULL.  So it would put itself 
back to sleep and never get woken up.

> Case 2: smp_mb() in wakeup_thread()
> 
>   thread_wakeup_needed = 0;   
>   smp_rmb();
>   if (bh->state != BH_STATE_FULL)
>   bh->state = BH_STATE_FULL;
> -->   smp_mb();
>   thread_wakeup_needed = 1;
>   wake_up_process()
>   set_current_state(intr) //smp_wb
>   if (thread_wakeup_need)
>   //breaks out of loop from thread_wakeup_needed check

That is not the right description.  It should look like this:

Case 2: smp_mb() in wakeup_thread()
bh->state = BH_STATE_FULL;
--> smp_mb();
thread_wakeup_needed = 0;   thread_wakeup_needed = 1;
smp_rmb();  wake_up_process();
if (bh->state != BH_STATE_FULL)
set_current_state(intr) //smp_wb
if (thread_wakeup_needed)
//breaks out of loop from thread_wakeup_needed check

This could fail even on x86; the CPU could still not see BH_STATE_FULL
because the smp_rmb and the read of bh->state could be reordered before
the write to thread_wakeup_needed.

> The issue won't be seen when I tested for both cases with smp_mb() in 
> either the wakeup_thread() or the sleep_thread(). I notice that in 
> wait_queue (DEFINED_WAIT_FUNC()), it places the smp_mb() in the wait 
> function, so I follow its implementation.
> 
> However, my test maybe insufficient and I maybe wrong in my code review..

For now, I think it would be best to put smp_mb() in both places.  
After I do a more thorough rewrite, the memory barriers will make more
sense.

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: usb: host: xhci: stalled endpoint ring not cleared on empty td_list

[Christian Gromm]

On 04/11/2017 04:13 PM, Rolf Evers-Fischer wrote:

Hi,

Christian Gromm  writes:

we observe an issue with a td_list running empty and an
endpoint being stalled at the same time on

Linux ihu-low 4.1.27-abl #1 SMP PREEMPT Mon Mar 20 13:51:51 CET 2017
x86_64 x86_64 x86_64 GNU/Linux.


we've recently observed the same problem with "Linux ihu-low 4.1.27-abl" kernel.
In our case it was discovered from the logs that URBs, that are being killed by 
“usb_kill_anchored_urbs()”, are still being used by “handle_tx_event()”. Sooner 
or later the same URB was used and killed at the same time, which caused the 
“WARN Event TRB for slot 2 ep 28 with no TDs queued?” error message, and the 
whole communication stopped.


Well, this sounds like a workaround to me as you are changing the timing 
by waiting for the URBs to finish. This might prevent the

race from happening.

I thought an USB device driver is allowed to kill anchored URBs at any 
time, isn't it?


thanks,
Chris



The developer, who introduced the USB anchors, has added some demo code in the 
file drivers/usb/usb-skeleton.c. In this file he is invoking 
“usb_wait_anchor_empty_timeout()” before he calls “usb_kill_anchored_urbs()”. 
If I understand it correctly, the purpose of this function is to wait until all 
URBs have been processed.
Therefore I suggested to do the same in the most driver. Fortunately it seems 
that this resolved our problem: We've never seen this problem anymore.

BR
Rolf


--
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 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 :)

--
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 3/6] usb: usbip tool: Check the return of get_nports()

[Greg KH]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Tue, Apr 11, 2017 at 06:38:03AM +0800, Yuyang Du wrote:
> Hi Shuah,
> 
> Could you please take a look at these patches? I got more patches
> that sit on these to send out.

Please send patches out, no need to wait, I'll just wait to apply them
for the review, you can always build on top of previous series.

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: usb: host: xhci: stalled endpoint ring not cleared on empty td_list

[Rolf Evers-Fischer]

Hi,

Christian Gromm  writes:
> we observe an issue with a td_list running empty and an
> endpoint being stalled at the same time on
>
> Linux ihu-low 4.1.27-abl #1 SMP PREEMPT Mon Mar 20 13:51:51 CET 2017 
> x86_64 x86_64 x86_64 GNU/Linux.

we've recently observed the same problem with "Linux ihu-low 4.1.27-abl" kernel.
In our case it was discovered from the logs that URBs, that are being killed by 
“usb_kill_anchored_urbs()”, are still being used by “handle_tx_event()”. Sooner 
or later the same URB was used and killed at the same time, which caused the 
“WARN Event TRB for slot 2 ep 28 with no TDs queued?” error message, and the 
whole communication stopped.

The developer, who introduced the USB anchors, has added some demo code in the 
file drivers/usb/usb-skeleton.c. In this file he is invoking 
“usb_wait_anchor_empty_timeout()” before he calls “usb_kill_anchored_urbs()”. 
If I understand it correctly, the purpose of this function is to wait until all 
URBs have been processed.
Therefore I suggested to do the same in the most driver. Fortunately it seems 
that this resolved our problem: We've never seen this problem anymore.

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

The issue is outstanding references to gadget.dev.  The driver core
does not guarantee that all references will have been dropped by the
time device_unregister() returns.  So what if some other part of the
kernel still has a reference to gadget.dev when we reach the end of
net2280_remove() and deallocate the private structure?

Unless you can be certain that there are no outstanding references when
the gadget is unregistered, this approach isn't safe.  (And even if you
can be certain, how can you know that future changes to the kernel
won't affect the situation?)

> > And it doesn't help with the fact that net2280_remove() continues to 
> > access the private data structure after calling usb_del_gadget_udc().  
> > Strictly speaking, that routine should do
> >
> > get_device(>gadget.dev);
> >
> > at the start, with a corresponding put_device() at the end.
> >
> > There's another problem.  Suppose a call to 
> > usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> > error pathway does put_device(>dev).  This will invoke the 
> > release callback, deallocating the private data structure without 
> > giving the caller (i.e., the UDC driver) a chance to clean up.
> 
> it won't deallocate anything :-) dev_set_drvdata() was never called,
> we will endup with kfree(NULL) which is safe and just silently returns.

But if you change gadget_release() to use the parent's drvdata field, 
like you proposed earlier, and fix up the refcounting in 
net2280_remove() so that the structure doesn't get deallocated too 
quickly, then this does become a real problem.

And it can affect other UDC drivers, not just net2280.

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 net-next 13/14] usbnet: kaweth: Use net_device_stats from struct net_device

[Oliver Neukum]

Am Freitag, den 07.04.2017, 10:17 +0200 schrieb Tobias Klauser:
> Instead of using a private copy of struct net_device_stats in struct
> kaweth_device, use stats from struct net_device. Also remove the now
> unnecessary .ndo_get_stats function.
> 
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Tobias Klauser 
Acked-by: Oliver Neukum 

--
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 net-next 14/14] usbnet: pegasus: Use net_device_stats from struct net_device

[Petko Manolov]

On 17-04-07 10:17:39, Tobias Klauser wrote:
> Instead of using a private copy of struct net_device_stats in struct pegasus, 
> use stats from struct net_device. Also remove the now unnecessary 
> .ndo_get_stats function.

Looks OK to me.


Petko


> Cc: Petko Manolov 
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: Tobias Klauser 
> ---
>  drivers/net/usb/pegasus.c | 36 +++-
>  drivers/net/usb/pegasus.h |  1 -
>  2 files changed, 15 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 321e059e13ae..6514c86f043e 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -501,13 +501,13 @@ static void read_bulk_callback(struct urb *urb)
>   if (rx_status & 0x1e) {
>   netif_dbg(pegasus, rx_err, net,
> "RX packet error %x\n", rx_status);
> - pegasus->stats.rx_errors++;
> + net->stats.rx_errors++;
>   if (rx_status & 0x06)   /* long or runt */
> - pegasus->stats.rx_length_errors++;
> + net->stats.rx_length_errors++;
>   if (rx_status & 0x08)
> - pegasus->stats.rx_crc_errors++;
> + net->stats.rx_crc_errors++;
>   if (rx_status & 0x10)   /* extra bits   */
> - pegasus->stats.rx_frame_errors++;
> + net->stats.rx_frame_errors++;
>   goto goon;
>   }
>   if (pegasus->chip == 0x8513) {
> @@ -535,8 +535,8 @@ static void read_bulk_callback(struct urb *urb)
>   skb_put(pegasus->rx_skb, pkt_len);
>   pegasus->rx_skb->protocol = eth_type_trans(pegasus->rx_skb, net);
>   netif_rx(pegasus->rx_skb);
> - pegasus->stats.rx_packets++;
> - pegasus->stats.rx_bytes += pkt_len;
> + net->stats.rx_packets++;
> + net->stats.rx_bytes += pkt_len;
>  
>   if (pegasus->flags & PEGASUS_UNPLUG)
>   return;
> @@ -670,13 +670,13 @@ static void intr_callback(struct urb *urb)
>   /* byte 0 == tx_status1, reg 2B */
>   if (d[0] & (TX_UNDERRUN|EXCESSIVE_COL
>   |LATE_COL|JABBER_TIMEOUT)) {
> - pegasus->stats.tx_errors++;
> + net->stats.tx_errors++;
>   if (d[0] & TX_UNDERRUN)
> - pegasus->stats.tx_fifo_errors++;
> + net->stats.tx_fifo_errors++;
>   if (d[0] & (EXCESSIVE_COL | JABBER_TIMEOUT))
> - pegasus->stats.tx_aborted_errors++;
> + net->stats.tx_aborted_errors++;
>   if (d[0] & LATE_COL)
> - pegasus->stats.tx_window_errors++;
> + net->stats.tx_window_errors++;
>   }
>  
>   /* d[5].LINK_STATUS lies on some adapters.
> @@ -685,7 +685,7 @@ static void intr_callback(struct urb *urb)
>*/
>  
>   /* bytes 3-4 == rx_lostpkt, reg 2E/2F */
> - pegasus->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
> + net->stats.rx_missed_errors += ((d[3] & 0x7f) << 8) | d[4];
>   }
>  
>   res = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -701,7 +701,7 @@ static void pegasus_tx_timeout(struct net_device *net)
>   pegasus_t *pegasus = netdev_priv(net);
>   netif_warn(pegasus, timer, net, "tx timeout\n");
>   usb_unlink_urb(pegasus->tx_urb);
> - pegasus->stats.tx_errors++;
> + net->stats.tx_errors++;
>  }
>  
>  static netdev_tx_t pegasus_start_xmit(struct sk_buff *skb,
> @@ -731,23 +731,18 @@ static netdev_tx_t pegasus_start_xmit(struct sk_buff 
> *skb,
>   netif_device_detach(pegasus->net);
>   break;
>   default:
> - pegasus->stats.tx_errors++;
> + net->stats.tx_errors++;
>   netif_start_queue(net);
>   }
>   } else {
> - pegasus->stats.tx_packets++;
> - pegasus->stats.tx_bytes += skb->len;
> + net->stats.tx_packets++;
> + net->stats.tx_bytes += skb->len;
>   }
>   dev_kfree_skb(skb);
>  
>   return NETDEV_TX_OK;
>  }
>  
> -static struct net_device_stats *pegasus_netdev_stats(struct net_device *dev)
> -{
> - return &((pegasus_t *) netdev_priv(dev))->stats;
> -}
> -
>  static inline void disable_net_traffic(pegasus_t *pegasus)
>  {
>   __le16 tmp = cpu_to_le16(0);
> @@ -1294,7 +1289,6 @@ static const struct net_device_ops pegasus_netdev_ops = 
> {
>   .ndo_do_ioctl = pegasus_ioctl,
>   .ndo_start_xmit =   pegasus_start_xmit,
>   .ndo_set_rx_mode =  pegasus_set_multicast,
> - .ndo_get_stats =pegasus_netdev_stats,
>   

[GIT PULL] USB changes for v4.12 merge window

[Felipe Balbi]

Hi Greg,

here's my pull request for v4.12 merge window. Changes have been tested
for Intel Edison where applicable. They have also been soaking for
linux-next for a while. Let me know if you want anything to be changed.

There will be a conflict on
drivers/usb/gadget/udc/{Kconfig,amd5536udc.c} which I solved like this:

diff --cc drivers/usb/gadget/udc/Kconfig
index c6cc9d3270ac,c90a4a223916..1c14c283cc47
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@@ -277,7 -293,8 +293,8 @@@ source "drivers/usb/gadget/udc/bdc/Kcon
  
  config USB_AMD5536UDC
tristate "AMD5536 UDC"
 -  depends on PCI
 +  depends on USB_PCI
+   select USB_SNP_CORE
help
   The AMD5536 UDC is part of the AMD Geode CS5536, an x86 southbridge.
   It is a USB Highspeed DMA capable USB device controller. Beside ep0
diff --cc drivers/usb/gadget/udc/amd5536udc.c
index 270876b438ab,91d0f1a4dac1..4ecd2f20ea48
--- a/drivers/usb/gadget/udc/amd5536udc.c
+++ b/drivers/usb/gadget/udc/amd5536udc.c
@@@ -618,17 -579,12 +579,12 @@@ static void udc_free_dma_chain(struct u
DBG(dev, "free chain req = %p\n", req);
  
/* do not free first desc., will be done by free for request */
-   td_last = req->td_data;
-   td = phys_to_virt(td_last->next);
- 
for (i = 1; i < req->chain_len; i++) {
-   dma_pool_free(dev->data_requests, td,
- (dma_addr_t)td_last->next);
-   td_last = td;
-   td = phys_to_virt(td_last->next);
+   td = phys_to_virt(addr);
+   addr_next = (dma_addr_t)td->next;
 -  pci_pool_free(dev->data_requests, td, addr);
++  dma_pool_free(dev->data_requests, td, addr);
+   addr = addr_next;
}
- 
-   return ret_val;
  }
  
  /* Frees request packet, called by gadget driver */

Resulting tree still compiles fine, but I can't test. amd5536udc.c :-s

cheers

The following changes since commit c02ed2e75ef4c74e41e421acb4ef1494671585e8:

  Linux 4.11-rc4 (2017-03-26 14:15:16 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tags/usb-for-v4.12

for you to fetch changes up to 48eab1f28d49a3eeda050ad03fddf24a594c1f79:

  usb: gadget: udc: atmel: Update Kconfig help for fifo_mode = 0 (2017-04-11 
10:58:31 +0300)


usb: changes for v4.12

With 51 non-merge commits, this is one of the smallest USB Gadget pull
requests. Apart from your expected set of non-critical fixes, and
other miscellaneous items, we have most of the changes in dwc3 (52.5%)
with all other UDCs following with 34.8%.

As for the actual changes, the most important of them are all the
recent changes to reduce memory footprint of dwc3, bare minimum
dual-role support on dwc3 and reworked endpoint count and
initialization routines.


Alexey Khoroshilov (1):
  usb: gadget: mv_u3d: fix error handling in mv_u3d_probe()

Baolin Wang (1):
  usb: phy: Remove unused config

Bruno Herrera (1):
  usb: dwc2: Add support for STM32F429/439/469 USB OTG HS/FS in FS mode 
(internal PHY)

Bryan O'Donoghue (2):
  usb: dwc3: refactor gadget endpoint count calculation
  usb: dwc3: remove dwc3_gadget_init_hw_endpoints

Chanwoo Choi (1):
  usb: mtu3: Replace the extcon API

Cristian Birsan (4):
  usb: gadget: udc: atmel: Minor code cleanup
  usb: gadget: udc: atmel: Check fifo configuration values against device 
tree
  usb: gadget: udc: atmel: Use dev_err() to display EP configuration error
  usb: gadget: udc: atmel: Update Kconfig help for fifo_mode = 0

Felipe Balbi (12):
  usb: dwc3: make sure UX_EXIT_PX is cleared
  usb: dwc3: trace: change format for string to cmd trace
  usb: gadget: u_ether: use better list accessors
  usb: gadget: u_ether: conditionally align transfer size
  usb: dwc3: debugfs: downcase OTG on 'mode' file
  usb: dwc3: debugfs: make use of dwc3_gadget_link_string()
  usb: dwc3: debugfs: return strings that match tracepoints
  usb: dwc3: expose dwc3_trb_type_string()
  usb: dwc3: ep0: use immediate SETUP on TRB
  usb: dwc3: ep0: pass dep as argument to internal functions
  usb: dwc3: ep0: improve handling of unaligned OUT requests
  usb: dwc3: simplify ZLP handling

Gustavo A. R. Silva (2):
  usb: gadget: udc: avoid use of freed pointer
  usb: gadget: udc: remove unnecessary variable and update function 
prototype

John Stultz (1):
  usb: dwc2: Make sure we disconnect the gadget state

John Youn (1):
  usb: dwc3: gadget: Fix starting microframe for ISOC

Lu Baolu (1):
  usb: dwc3: remove dwc3_log_msg trace class

Michael Grzeschik (1):
  fsl_udc_core: add support for devices provided by fsl-mph-dr-of

Michal Nazarewicz (2):
  usb: gadget: mv_udc: clarify a switch 

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

[Felipe Balbi]

Hi,

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.

> If you have any insight on how to debug this, we can investigate this
> more.

see the other subthread :-)

>>> see this as a hang in the file transfer and the host will attempt to
>>> reset the device. This shouldn't be possible in the dwc3 implementation
>>> if the HW and the SW are in sync. However, through testing and reading
>>
>> it's not about SW and HW being in sync, it's about the HW being quirky,
>> right?
>
> No, we don't see any problem in the hardware. Here, being out of
> "sync" just means the device hangs because of lost events. There are a
> number of pretty bad failure scenarios that could happen due to this.
>
>>
>>> the pcie trace, the top half IRQ handler occasionally still gets invoked
>>> one more time after HW interrupt deassertion. We suspect that there is a
>>> small detection delay in the SW.
>>
>> so you don't really know what's going on :-) Why send a patch if you
>> don't understand the problem fully?
>>
>>> Top half IRQ handler deasserts the interrupt line after it gets the
>>> event count by writing DWC3_GEVNTSIZ_INTMASK to DWC3_GEVNTSIZ. There's a
>>> small window for a new event coming in between reading the event count
>>> and the interrupt deassertion. More generally, we will see 0 event
>>> count, which should not affect anything.
>>
>> this new event can come, and it'll remain in the event buffer. We have
>> masked the IRQ line and this should be enough to make sure IRQ isn't
>> generated again. Why is the IRQ raising again? Which kernel version are
>> you testing? Which DWC3 revision? Do you have interrupt moderation?
>
> It's not about the new event. We see the case where the top-half is
> called again just after we mask the interrupt without any events too.
> That's probably the majority of the time it occurs. It's just that if
> this case happens to coincide with a new event, everything gets
> screwed up.
>
> An alternative fix is to not allow the top-half to overwrite the event
> cache. Just append and update it. The top and bottom are already
> mutually exclusive so it should be fine.

Right, we _can_ do that as further protection for the problem. But first
we need to really _KNOW_ why this is happening. Then provide a minimal
fix which can be backported to stable and only after the minimal fix, do
we improve the code (perhaps even add comments in the code explaining
this situation so we never lose track of it again) with further
protections.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 3/3] usb: gadget: f_mass_storage: Serialize wake and sleep execution

[Felipe Balbi]

Hi,

Alan Stern  writes:
> Felipe, was this patch meant to solve the problem you encountered in
> your "Memory barrier needed with wake_up_process()?" email thread last
> fall?

it certainly smells like it, but Thinh Nguyen didn't say for sure :-)

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

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Alan Stern  writes:
>> >> >> >> --- a/drivers/usb/gadget/udc/core.c
>> >> >> >> +++ b/drivers/usb/gadget/udc/core.c
>> >> >> >> @@ -1273,6 +1273,7 @@ void usb_del_gadget_udc(struct usb_gadget 
>> >> >> >> *gadget)
>> >> >> >> flush_work(>work);
>> >> >> >> device_unregister(>dev);
>> >> >> >> device_unregister(>dev);
>> >> >> >> +   memset(>dev, 0x00, sizeof(gadget->dev));
>> >> >> >>  }
>> >> >> >>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>> >> >> >
>> >> >> > Isn't this dangerous?  It's quite possible that the 
>> >> >> > device_unregister() 
>> >> >> 
>> >> >> not on the gadget API, no.
>> >> >> 
>> >> >> > call on the previous line invokes the gadget->dev.release callback, 
>> >> >> > which might deallocate gadget.  If that happens, your new memset 
>> >> >> > will 
>> >> >> > oops.
>> >> >> 
>> >> >> that won't happen. struct usb_gadget is a member of the UDC's private
>> >> >> structure, like this:
>> >> >> 
>> >> >> struct dwc3 {
>> >> >>[...]
>> >> >>struct usb_gadget   gadget;
>> >> >>struct usb_gadget_driver *gadget_driver;
>> >> >>[...]
>> >> >> };
>> >> >
>> >> > Yes.  So what?  Can't the UDC driver use the refcount inside struct 
>> >> > usb_gadget to control the lifetime of its private structure?
>> >> 
>> >> nope, not being used. At least not yet.
>> >
>> > I'm not convinced (yet)...
>> >
>> >> > (By the way, can you tell what's going on in net2280.c?  I must be
>> >> > missing something; it looks like gadget_release() would quickly run
>> >> > into problems because it calls dev_get_drvdata() for >dev, but
>> >> > net2280_probe() never calls dev_set_drvdata() for that device.  
>> >> > Furthermore, net2280_remove() continues to reference the net2280 struct
>> >> > after calling usb_del_gadget_udc(), and it never does seem to do a
>> >> > final put.)
>> >> 
>> >> static int net2280_probe(struct pci_dev *pdev, const struct pci_device_id 
>> >> *id)
>> >> {
>> >>   struct net2280  *dev;
>> >>   unsigned long   resource, len;
>> >>   void__iomem *base = NULL;
>> >>   int retval, i;
>> >> 
>> >>   /* alloc, and start init */
>> >>   dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> >>   if (dev == NULL) {
>> >>   retval = -ENOMEM;
>> >>   goto done;
>> >>   }
>> >> 
>> >>   pci_set_drvdata(pdev, dev);
>> >>   ^^^
>> >
>> > That sets the driver data in the struct pci_dev, not in
>> > dev->gadget.dev.  As far as I can see, _nothing_ in the driver sets the 
>> > driver data in dev->gadget.dev.
>> 
>> hmmm, indeed. The same is happening with other callers of
>> usb_add_gadget_udc_release().
>> 
>> I guess this should be enough?
>> 
>> @@ -3557,7 +3557,7 @@ static irqreturn_t net2280_irq(int irq, void *_dev)
>>  
>>  static void gadget_release(struct device *_dev)
>>  {
>> -struct net2280  *dev = dev_get_drvdata(_dev);
>> +struct net2280  *dev = dev_get_drvdata(_dev->parent);
>>  
>>  kfree(dev);
>>  }
>
> 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;

> And it doesn't help with the fact that net2280_remove() continues to 
> access the private data structure after calling usb_del_gadget_udc().  
> Strictly speaking, that routine should do
>
>   get_device(>gadget.dev);
>
> at the start, with a corresponding put_device() at the end.
>
> There's another problem.  Suppose a call to 
> usb_add_gadget_udc_release() fails.  At the end of that routine, the 
> error pathway does put_device(>dev).  This will invoke the 
> 

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

[Felipe Balbi]

Hi,

John Youn  writes:
>>> Yes it will re-assert. The interrupt line will remain asserted as long
>>> events remain in the buffer and it is not masked. So when we
>>> eventually unmask, the interrupt will be reasserted again immediately.
>>>
 BTW, other option here could be using (like Baolin propose some time
 ago) dwc->lock in top half while this is held for BH.
 Question how long spin_lock() will be held in top half...
 I am not sure what option is the best.
>>>
>>> That won't help in this case since you can still have two calls top
>>> top-half before a call to bottom. The lock only prevents the two from
>>> stepping on each other, but that should already be the case without
>>> needing the lock.
>>>
>> Really can we have two TH calls before BH?
>> Interesting case :)
>
> That's what we seem to be seeing. I'm not sure if it is normal or not.

no, that's not normal. If this is happening, there's either a HW bug
somewhere, or we're not clearing events properly.

Can you provide tracepoints of the failure? Perhaps add a trace_printk()
call on both BH and TH just so we see when they're both called.

>> You suggest we have something like this:
>> dwc3 IRQ
>>kernel irq_mask
>>   dwc3 TH
>>  mask dwc3 interrupts
>>  get evt->count, evt->cache
>>  write to hw (count)
>>  return WAKE_THREAD
>>kernel irq_unmask
>> a) Before BH start we get another dwc3 IRQ?
>> b) CPU0 starts with BH while we get dwc3 IRQ on CPU1?
>>
>> *) in normal case BH should start and in the end unmask
>> dwc3_interrupts and after that we could get new irq
>>
>> If this could happen then for sure you need dwc->lock protection in TH
>> while base on your description CPU0 can execute BH and CPU1 can handle
>> TH - so you have to protect dwc3_event_buffer.
>>
>> Could you describe this more? How to reproduce?
>
> We can reproduce by running a file transfer continuously. It fails
> fairly reliably, but it can take a while to hit the condition. We are
> using our PCIe based HAPS FPGA on x86_64 using mainline kernel.

Okay, please provide tracepoints of the problem in question.

> Thinh can provide which IP versions we tried with.

Thank you

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

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

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.

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 6f6f0b3be3ad..3b8185d81f9f 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3005,6 +3005,7 @@ static irqreturn_t dwc3_thread_interrupt(int irq, void 
*_evt)
unsigned long flags;
irqreturn_t ret = IRQ_NONE;
 
+   trace_printk("BH\n");
spin_lock_irqsave(>lock, flags);
ret = dwc3_process_event_buf(evt);
spin_unlock_irqrestore(>lock, flags);
@@ -3019,6 +3020,8 @@ static irqreturn_t dwc3_check_event_buf(struct 
dwc3_event_buffer *evt)
u32 count;
u32 reg;
 
+   trace_printk("evt->flags %08x\n", evt->flags);
+
if (pm_runtime_suspended(dwc->dev)) {
pm_runtime_get(dwc->dev);
disable_irq_nosync(dwc->irq_gadget);
@@ -3027,7 +3030,9 @@ static irqreturn_t dwc3_check_event_buf(struct 
dwc3_event_buffer *evt)
}
 
count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
+   trace_printk("GEVNTCOUNT %08x\n", count);
count &= DWC3_GEVNTCOUNT_MASK;
+   trace_printk("%u events pending\n", count);
if (!count)
return IRQ_NONE;
 
@@ -3036,10 +3041,12 @@ static irqreturn_t dwc3_check_event_buf(struct 
dwc3_event_buffer *evt)
 
/* Mask interrupt */
reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
+   trace_printk("GEVNTSIZ %08x\n", reg);
reg |= DWC3_GEVNTSIZ_INTMASK;
dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
 
amount = min(count, evt->length - evt->lpos);
+   trace_printk("copying events to cache\n");
memcpy(evt->cache + evt->lpos, evt->buf + evt->lpos, amount);
 
if (amount < count)
@@ -3053,7 +3060,7 @@ static irqreturn_t dwc3_check_event_buf(struct 
dwc3_event_buffer 

Re: [PATCH v3 3/6] usb: usbip tool: Check the return of get_nports()

[Yuyang Du]

Hi Shuah,

Could you please take a look at these patches? I got more patches
that sit on these to send out.

Thanks,
Yuyang

On Thu, Apr 06, 2017 at 06:03:24AM +0800, Yuyang Du wrote:
> If we get nonpositive number of ports, there is no sense to
> continue, then fail gracefully.
> 
> In addition, the commit 0775a9cbc694e8c72 ("usbip: vhci extension:
> modifications to vhci driver") introduced configurable numbers of
> controllers and ports, but we have a static port number maximum,
> MAXNPORT. If exceeded, the idev array will be overflown. We fix
> it by validating the nports to make sure the port number max is
> not exceeded.
> 
> Signed-off-by: Yuyang Du 
> ---
>  tools/usb/usbip/libsrc/vhci_driver.c | 10 +-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/usb/usbip/libsrc/vhci_driver.c 
> b/tools/usb/usbip/libsrc/vhci_driver.c
> index f659c14..151580a 100644
> --- a/tools/usb/usbip/libsrc/vhci_driver.c
> +++ b/tools/usb/usbip/libsrc/vhci_driver.c
> @@ -220,9 +220,17 @@ int usbip_vhci_driver_open(void)
>   }
>  
>   vhci_driver->nports = get_nports();
> -
>   dbg("available ports: %d", vhci_driver->nports);
>  
> + if (vhci_driver->nports <=0) {
> + err("no available ports");
> + goto err;
> + }
> + else if (vhci_driver->nports > MAXNPORT) {
> + err("port number exceeds %d", MAXNPORT);
> + goto err;
> + }
> +
>   if (refresh_imported_device_list())
>   goto err;
>  
> -- 
> 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 1/3] usb: dwc3: gadget: Prevent losing events in event cache

[John Youn]

On 04/10/2017 11:17 PM, Janusz Dziedzic wrote:
> On 10 April 2017 at 20:43, John Youn  wrote:
>> On 04/08/2017 12:38 PM, Janusz Dziedzic wrote:
>>> 2017-04-08 1:57 GMT+02:00 Thinh Nguyen :
 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
 see this as a hang in the file transfer and the host will attempt to
 reset the device. This shouldn't be possible in the dwc3 implementation
 if the HW and the SW are in sync. However, through testing and reading
 the pcie trace, the top half IRQ handler occasionally still gets invoked
 one more time after HW interrupt deassertion. We suspect that there is a
 small detection delay in the SW.

 Top half IRQ handler deasserts the interrupt line after it gets the
 event count by writing DWC3_GEVNTSIZ_INTMASK to DWC3_GEVNTSIZ. There's a
 small window for a new event coming in between reading the event count
 and the interrupt deassertion. More generally, we will see 0 event
 count, which should not affect anything.

 To avoid this issue, we ensure that the events in the cache are
 processed before checking for the new ones. The bottom half IRQ will
 unmask the DWC3_GEVNTSIZ_INTMASK once it finishes processing the events.
 Top half IRQ handler needs to check whether the DWC3_GEVNTSIZ_INTMASK is
 still set before storing the new events. It also should return
 IRQ_HANDLED if the mask is still set since IRQ handler was invoked for
 our usb interrupt.

 Signed-off-by: Thinh Nguyen 
 ---
  drivers/usb/dwc3/gadget.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 4dc80729ae11..93d98fb7215e 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -3045,6 +3045,10 @@ static irqreturn_t dwc3_check_event_buf(struct 
 dwc3_event_buffer *evt)
 return IRQ_HANDLED;
 }

 +   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
 +   if (reg & DWC3_GEVNTSIZ_INTMASK)
 +   return IRQ_HANDLED;
 +
 count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
 count &= DWC3_GEVNTCOUNT_MASK;
 if (!count)
 @@ -3054,7 +3058,6 @@ static irqreturn_t dwc3_check_event_buf(struct 
 dwc3_event_buffer *evt)
 evt->flags |= DWC3_EVENT_PENDING;

 /* Mask interrupt */
 -   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
 reg |= DWC3_GEVNTSIZ_INTMASK;
 dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);

 --
 2.11.0

>>> This seems like a workaround, while when we mask interrupts we should
>>> not get IRQ before BH will done. Current driver implementation base on
>>> this.
>>> We are using 2.60a and didn't see problem you describe. Is it specyfic
>>> for some HW revision?
>>
>> Doesn't seem to be. We can try other HW versions.
>>
>>>
>>> I can imagine we can have the "last" interrupt and you will return
>>> IRQ_HANDLED. Will HW issue this interrupt once again? If not will we
>>> loose this IRQ (eg. disconnect event)?
>>>
>>
>> Yes it will re-assert. The interrupt line will remain asserted as long
>> events remain in the buffer and it is not masked. So when we
>> eventually unmask, the interrupt will be reasserted again immediately.
>>
>>> BTW, other option here could be using (like Baolin propose some time
>>> ago) dwc->lock in top half while this is held for BH.
>>> Question how long spin_lock() will be held in top half...
>>> I am not sure what option is the best.
>>
>> That won't help in this case since you can still have two calls top
>> top-half before a call to bottom. The lock only prevents the two from
>> stepping on each other, but that should already be the case without
>> needing the lock.
>>
> Really can we have two TH calls before BH?
> Interesting case :)

That's what we seem to be seeing. I'm not sure if it is normal or not.

>
> You suggest we have something like this:
> dwc3 IRQ
>kernel irq_mask
>   dwc3 TH
>  mask dwc3 interrupts
>  get evt->count, evt->cache
>  write to hw (count)
>  return WAKE_THREAD
>kernel irq_unmask
> a) Before BH start we get another dwc3 IRQ?
> b) CPU0 starts with BH while we get dwc3 IRQ on CPU1?
>
> *) in normal case BH should start and in the end unmask
> dwc3_interrupts and after that we could get new irq
>
> If this could happen then for sure you need dwc->lock protection in TH
> while base on your description CPU0 can execute BH and CPU1 can handle
> TH - so you have to protect dwc3_event_buffer.
>
> Could you describe this more? How to reproduce?

We can reproduce by running a file transfer continuously. It fails
fairly reliably, but it can take a 

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

[Janusz Dziedzic]

On 10 April 2017 at 20:43, John Youn  wrote:
> On 04/08/2017 12:38 PM, Janusz Dziedzic wrote:
>> 2017-04-08 1:57 GMT+02:00 Thinh Nguyen :
>>> 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
>>> see this as a hang in the file transfer and the host will attempt to
>>> reset the device. This shouldn't be possible in the dwc3 implementation
>>> if the HW and the SW are in sync. However, through testing and reading
>>> the pcie trace, the top half IRQ handler occasionally still gets invoked
>>> one more time after HW interrupt deassertion. We suspect that there is a
>>> small detection delay in the SW.
>>>
>>> Top half IRQ handler deasserts the interrupt line after it gets the
>>> event count by writing DWC3_GEVNTSIZ_INTMASK to DWC3_GEVNTSIZ. There's a
>>> small window for a new event coming in between reading the event count
>>> and the interrupt deassertion. More generally, we will see 0 event
>>> count, which should not affect anything.
>>>
>>> To avoid this issue, we ensure that the events in the cache are
>>> processed before checking for the new ones. The bottom half IRQ will
>>> unmask the DWC3_GEVNTSIZ_INTMASK once it finishes processing the events.
>>> Top half IRQ handler needs to check whether the DWC3_GEVNTSIZ_INTMASK is
>>> still set before storing the new events. It also should return
>>> IRQ_HANDLED if the mask is still set since IRQ handler was invoked for
>>> our usb interrupt.
>>>
>>> Signed-off-by: Thinh Nguyen 
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 5 -
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 4dc80729ae11..93d98fb7215e 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3045,6 +3045,10 @@ static irqreturn_t dwc3_check_event_buf(struct 
>>> dwc3_event_buffer *evt)
>>> return IRQ_HANDLED;
>>> }
>>>
>>> +   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>> +   if (reg & DWC3_GEVNTSIZ_INTMASK)
>>> +   return IRQ_HANDLED;
>>> +
>>> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>>> count &= DWC3_GEVNTCOUNT_MASK;
>>> if (!count)
>>> @@ -3054,7 +3058,6 @@ static irqreturn_t dwc3_check_event_buf(struct 
>>> dwc3_event_buffer *evt)
>>> evt->flags |= DWC3_EVENT_PENDING;
>>>
>>> /* Mask interrupt */
>>> -   reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>>> reg |= DWC3_GEVNTSIZ_INTMASK;
>>> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>>>
>>> --
>>> 2.11.0
>>>
>> This seems like a workaround, while when we mask interrupts we should
>> not get IRQ before BH will done. Current driver implementation base on
>> this.
>> We are using 2.60a and didn't see problem you describe. Is it specyfic
>> for some HW revision?
>
> Doesn't seem to be. We can try other HW versions.
>
>>
>> I can imagine we can have the "last" interrupt and you will return
>> IRQ_HANDLED. Will HW issue this interrupt once again? If not will we
>> loose this IRQ (eg. disconnect event)?
>>
>
> Yes it will re-assert. The interrupt line will remain asserted as long
> events remain in the buffer and it is not masked. So when we
> eventually unmask, the interrupt will be reasserted again immediately.
>
>> BTW, other option here could be using (like Baolin propose some time
>> ago) dwc->lock in top half while this is held for BH.
>> Question how long spin_lock() will be held in top half...
>> I am not sure what option is the best.
>
> That won't help in this case since you can still have two calls top
> top-half before a call to bottom. The lock only prevents the two from
> stepping on each other, but that should already be the case without
> needing the lock.
>
Really can we have two TH calls before BH?
Interesting case :)

You suggest we have something like this:
dwc3 IRQ
   kernel irq_mask
  dwc3 TH
 mask dwc3 interrupts
 get evt->count, evt->cache
 write to hw (count)
 return WAKE_THREAD
   kernel irq_unmask
a) Before BH start we get another dwc3 IRQ?
b) CPU0 starts with BH while we get dwc3 IRQ on CPU1?

*) in normal case BH should start and in the end unmask
dwc3_interrupts and after that we could get new irq

If this could happen then for sure you need dwc->lock protection in TH
while base on your description CPU0 can execute BH and CPU1 can handle
TH - so you have to protect dwc3_event_buffer.

Could you describe this more? How to reproduce?
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?

BR
Janusz

> Regards,
> John
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at