RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

> Mark Lord [mailto:ml...@pobox.com]
> > Sent: Friday, November 25, 2016 12:44 AM
> [...]
> > The bad data in this case is ASCII:
> >
> >  "SRC=m3400:/ TARGET=/m340"
> >
> > This data is what is seen in /run/mount/utab, a file that is read/written 
> > over NFS
> on
> > each boot.
> >
> >  "SRC=m3400:/ TARGET=/m3400 ROOT=/
> > ATTRS=nolock,addr=192.168.8.1\n"
> >
> > But how does this ASCII data end up at offset zero of the rx buffer??
> > Not possible -- this isn't even stale data, because only an rx_desc could
> > be at that offset in that buffer.
> >
> > So even if this were a platform memory coherency issue, one should still
> > never see ASCII data at the beginning of an rx buffer.  The driver NEVER
> > writes anything to the rx buffers.  Only the USB hardware ever does.
> >
> > And only the r8152 dongle/driver exhibits this issue.
> > Other USB dongles do not.  They *might* still have such issues,
> > but because they use software checksums, the bad packets are 
> > caught/rejected.
> 
> Do you test it by rebooting? Maybe you could try a patch
> commit 93fe9b183840 ("r8152: reset the bmu"). However, it should
> only occur for the first urb buffer after rx is reset. I don't
> think you would reset the rx frequently, so the situation seems
> to be different.

Forgive me. I provide wrong information. This is about RTL8153,
not RTL8152.

Best Regards,
Hayes

--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

Mark Lord [mailto:ml...@pobox.com]
> Sent: Friday, November 25, 2016 12:44 AM
[...]
> The bad data in this case is ASCII:
> 
>  "SRC=m3400:/ TARGET=/m340"
> 
> This data is what is seen in /run/mount/utab, a file that is read/written 
> over NFS on
> each boot.
> 
>  "SRC=m3400:/ TARGET=/m3400 ROOT=/
> ATTRS=nolock,addr=192.168.8.1\n"
> 
> But how does this ASCII data end up at offset zero of the rx buffer??
> Not possible -- this isn't even stale data, because only an rx_desc could
> be at that offset in that buffer.
> 
> So even if this were a platform memory coherency issue, one should still
> never see ASCII data at the beginning of an rx buffer.  The driver NEVER
> writes anything to the rx buffers.  Only the USB hardware ever does.
> 
> And only the r8152 dongle/driver exhibits this issue.
> Other USB dongles do not.  They *might* still have such issues,
> but because they use software checksums, the bad packets are caught/rejected.

Do you test it by rebooting? Maybe you could try a patch
commit 93fe9b183840 ("r8152: reset the bmu"). However, it should
only occur for the first urb buffer after rx is reset. I don't
think you would reset the rx frequently, so the situation seems
to be different.

Best Regards,
Hayes

--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

Mark Lord [mailto:ml...@pobox.com]
> Sent: Thursday, November 24, 2016 11:25 PM
[...]
> x86 has near fully-coherent memory, so it is the "easy" platform
> to get things working on.  But Linux supports a very diverse number
> of platforms, with varying degrees of cache/memory coherency,
> and it can be tricky for things to work correctly on all of them.

However, I have test iperf on raspberry pi v1 which you suggest
for more than one day. I still couldn't reproduce your issue.

> If you are testing with the driver as currently in 4.4.34,
> then you won't even notice when things are screwing up,
> because the driver just silently drops packets.
> Or it passes them on without noticing that they have bad data.

I only drop the packet silently when the rx descriptor outside
the urb buffer. Then, I check the rx descriptor before checking
the length of the packet.

> Here (attached) is the instrumented driver I am using here now.
> I suggest you use it or something similar when testing,
> and not the stock driver.

I would test it again with your driver.

[...]
> Also, unrelated, but inside r8152_submit_rx() there is this code:
> 
>  /* The rx would be stopped, so skip submitting */
>  if (test_bit(RTL8152_UNPLUG, >flags) ||
>  !test_bit(WORK_ENABLE, >flags)
> || !netif_carrier_ok(tp->netdev))
> return 0;
> 
> If that "return 0" statement is ever executed, doesn't it result
> in the loss/leak of a buffer?

They would be found back by calling rtl_start_rx(), when the rx
is restarted.

Best Regards,
Hayes

--
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 v2] fsl/usb: Workarourd for USB erratum-A005697

[Changming Huang]

The EHCI specification states the following in the SUSP bit description:
In the Suspend state, the port is senstive to resume detection.
Note that the bit status does not change untile the port is suspended and
that there may be a delay in susupending a port if there is a transaction
currently in progress on the USB.

However, in NXP USBDR controller, the PORTSCx[SUSP] bit changes immediately
when the application sets it and not when the port is actually suspended.

So the application must wait for at least 10 milliseconds after a port
indicates that it is suspended, to make sure this port has entered
suspended state before initiating this port resume using the Force Port
Resume bit. This bit is for NXP controller, not EHCI compatible.

Signed-off-by: Changming Huang 
Signed-off-by: Ramneek Mehresh 
---
Change in v2:
- move sleep out of spin-lock and add more comment for this workaround

 drivers/usb/host/ehci-fsl.c  |3 +++
 drivers/usb/host/ehci-hub.c  |7 +++
 drivers/usb/host/ehci.h  |6 ++
 drivers/usb/host/fsl-mph-dr-of.c |2 ++
 include/linux/fsl_devices.h  |1 +
 5 files changed, 19 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 9f5ffb6..91701cc 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -286,6 +286,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
if (pdata->has_fsl_erratum_a005275 == 1)
ehci->has_fsl_hs_errata = 1;
 
+   if (pdata->has_fsl_erratum_a005697 == 1)
+   ehci->has_fsl_susp_errata = 1;
+
if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
(pdata->operating_mode == FSL_USB2_DR_OTG))
if (ehci_fsl_setup_phy(hcd, pdata->phy_mode, 0))
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 74f62d6..81e2310 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -310,6 +310,13 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
}
spin_unlock_irq(>lock);
 
+   if (changed && ehci_has_fsl_susp_errata(ehci))
+   /* Wait for at least 10 millisecondes to ensure the controller
+* enter the suspend status before initiating a port resume
+* using the Fore Port Resume bit (Not-EHCI compatible).
+*/
+   usleep_range(1, 2);
+
if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) {
/*
 * Wait for HCD to enter low-power mode or for the bus
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 3f3b74a..7706e4a 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -219,6 +219,7 @@ struct ehci_hcd {   /* one per controller */
unsignedno_selective_suspend:1;
unsignedhas_fsl_port_bug:1; /* FreeScale */
unsignedhas_fsl_hs_errata:1;/* Freescale HS quirk */
+   unsignedhas_fsl_susp_errata:1;  /*Freescale SUSP quirk*/
unsignedbig_endian_mmio:1;
unsignedbig_endian_desc:1;
unsignedbig_endian_capbase:1;
@@ -703,10 +704,15 @@ struct ehci_tt {
 #if defined(CONFIG_PPC_85xx)
 /* Some Freescale processors have an erratum (USB A-005275) in which
  * incoming packets get corrupted in HS mode
+ * Some Freescale processors have an erratum (USB A-005697) in which
+ * we need to wait for 10ms for bus to fo into suspend mode after
+ * setting SUSP bit
  */
 #define ehci_has_fsl_hs_errata(e)  ((e)->has_fsl_hs_errata)
+#define ehci_has_fsl_susp_errata(e)((e)->has_fsl_susp_errata)
 #else
 #define ehci_has_fsl_hs_errata(e)  (0)
+#define ehci_has_fsl_susp_errata(e)(0)
 #endif
 
 /*
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index f07ccb2..e90ddb5 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -226,6 +226,8 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device 
*ofdev)
of_property_read_bool(np, "fsl,usb-erratum-a007792");
pdata->has_fsl_erratum_a005275 =
of_property_read_bool(np, "fsl,usb-erratum-a005275");
+   pdata->has_fsl_erratum_a005697 =
+   of_property_read_bool(np, "fsl,usb_erratum-a005697");
 
/*
 * Determine whether phy_clk_valid needs to be checked
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index f291291..60cef82 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -100,6 +100,7 @@ struct fsl_usb2_platform_data {
unsignedalready_suspended:1;
unsignedhas_fsl_erratum_a007792:1;
unsignedhas_fsl_erratum_a005275:1;
+   unsignedhas_fsl_erratum_a005697:1;
unsigned

Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 07:27 PM, Francois Romieu wrote:
>
> Through aliasing the URB was given a page that contains said (previously)
> received file. The ethernet chip/usb host does not write anything in it.

I don't see how that could be possible.  Please elaborate.

The URB buffers are statically allocated by the driver at probe time,
ten of them in all, allocated with usb_alloc_coherent() in the copy of
the driver I am testing with.

There is no possibility for them to be used for anything other than
USB receive buffers, for this driver only.  Nothing in the driver
or kernel ever writes to those buffers after initial allocation,
and only the driver and USB host controller ever have pointers to the buffers.
-- 
Mark Lord
--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Francois Romieu]

Mark Lord  :
[...]
> >From tracing through the powerpc arch code, this is the buffer that
> is being directly DMA'd into.  And the USB layer does an invalidate_dcache
> on that entire buffer before initiating the DMA (confirmed via printk).
> 
> The driver itself NEVER writes anything to that buffer,
> and nobody else has a pointer to it other than the USB host controller,
> so there's nothing else that can write to it either.
> 
> According to the driver writer, the chip should only ever write a fresh
> rx_desc struct at the beginning of a buffer, never ASCII data.
> 
> So how does that buffer end up containing ASCII data from the NFS transfers?

Through aliasing the URB was given a page that contains said (previously)
received file. The ethernet chip/usb host does not write anything in it.
There could be a device or a driver problem but it may not be the real
problem.

So far the analysis focused on "how was this corrupted content written into
this receive buffer page ?". If I read David correctly (?) the "nobody
else has a pointer to it other than the USB host controller" point may be
replaced with "the pointer to it aliases some already used page".

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

Re: [PATCH v2 2/3] Revert "usb: dwc2: gadget: fix TX FIFO size and address initialization"

[Stefan Wahren]

Hi John,

> John Youn  hat am 18. Oktober 2016 um 02:36
> geschrieben:
> 
> 
> This reverts commit aa381a7259c3 ("usb: dwc2: gadget: fix TX FIFO size
> and address initialization").
> 
> The original commit removed the FIFO size programming per endpoint. The
> DPTXFSIZn register is also used for DIEPTXFn and the SIZE field is r/w
> in dedicated fifo mode. So it isn't appropriate to simply remove this
> initialization as it might break existing behavior.
> 
> Also, some cores might not have enough fifo space to handle the
> programming method used in the reverted patch, resulting in fifo
> initialization failure.

since the original patch is still necessary for bcm2835 gadget support i want to
know if there is an alternative solution?

Stefan
--
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: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Guenter Roeck]

On 11/23/2016 04:24 AM, Mathias Nyman wrote:

the tt_info provided by a HS hub might be in use to by a child device
Make sure we free the devices in the correct order.

This is needed in special cases such as when xhci controller is
reset when resuming from hibernate, and all virt_devices are freed.

Also free the virt_devices starting from max slot_id as children
more commonly have higher slot_id than parent.

CC: 
Signed-off-by: Mathias Nyman 

---

Guenter Roeck, does this work for you?



Sorry, I didn't have time this week. I'll test it first thing next week.

Guenter


A rework of how tt_info is stored and used might be needed,
but that will take some time and won't go to stable as easily.
---
 drivers/usb/host/xhci-mem.c | 38 --
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe323..b3a5cd8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int 
slot_id)
xhci->devs[slot_id] = NULL;
 }

+/*
+ * Free a virt_device structure.
+ * If the virt_device added a tt_info (a hub) and has children pointing to
+ * that tt_info, then free the child first. Recursive.
+ * We can't rely on udev at this point to find child-parent relationships.
+ */
+void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
+{
+   struct xhci_virt_device *vdev;
+   struct list_head *tt_list_head;
+   struct xhci_tt_bw_info *tt_info, *next;
+   int i;
+
+   vdev = xhci->devs[slot_id];
+   if (!vdev)
+   return;
+
+   tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
+   list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
+   /* is this a hub device that added a tt_info to the tts list */
+   if (tt_info->slot_id == slot_id) {
+   /* are any devices using this tt_info? */
+   for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
+   vdev = xhci->devs[i];
+   if (vdev && (vdev->tt_info == tt_info))
+   xhci_free_virt_devices_depth_first(
+   xhci, i);
+   }
+   }
+   }
+   /* we are now at a leaf device */
+   xhci_free_virt_device(xhci, slot_id);
+}
+
 int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
struct usb_device *udev, gfp_t flags)
 {
@@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
}
}

-   for (i = 1; i < MAX_HC_SLOTS; ++i)
-   xhci_free_virt_device(xhci, i);
+   for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)
+   xhci_free_virt_devices_depth_first(xhci, i);

dma_pool_destroy(xhci->segment_pool);
xhci->segment_pool = NULL;



--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Greg KH]

On Thu, Nov 24, 2016 at 02:10:36PM -0500, Mark Lord wrote:
> On 16-11-24 02:00 PM, Greg KH wrote:
> > On Thu, Nov 24, 2016 at 01:34:08PM -0500, Mark Lord wrote:
> >> One thought:  bulk data streams are byte streams, not packets.
> >> Scheduling on the USB bus can break up larger transfers across
> >> multiple in-kernel buffers.  A "real" URB buffer on USB2 is max 512 bytes.
> >> The driver is providing 16384-byte buffers, and assumes that data will
> >> never spill over from one such buffer to the next.
> >> Yet the observations here consistently show otherwise.
> > 
> > Wait, how do you know that data will not spill over?  What is making
> > that guarantee?  Will the USB device send a "zero packet" in order to
> > show that all of the "logical" data is now sent for this specific
> > endpoint?  Is there some sort of "framing" that the device does with the
> > USB data so that the driver "knows" where the end of packet is?
> 
> Exactly my point.
> 
> > Check the zero-packet stuff for this device, that's tripped up many a
> > USB driver writer over the years, myself included.
> 
> I haven't tripped over it myself, but only because we were careful
> to allow for such in the USB drivers I have worked on.
> 
> The r8152 driver just assumes it never happens.

Assumes what?  That the host will always consume data faster than the
device can create it?  If so, that sounds like your real problem
there...

good luck!

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 net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 02:00 PM, Greg KH wrote:
> On Thu, Nov 24, 2016 at 01:34:08PM -0500, Mark Lord wrote:
>> One thought:  bulk data streams are byte streams, not packets.
>> Scheduling on the USB bus can break up larger transfers across
>> multiple in-kernel buffers.  A "real" URB buffer on USB2 is max 512 bytes.
>> The driver is providing 16384-byte buffers, and assumes that data will
>> never spill over from one such buffer to the next.
>> Yet the observations here consistently show otherwise.
> 
> Wait, how do you know that data will not spill over?  What is making
> that guarantee?  Will the USB device send a "zero packet" in order to
> show that all of the "logical" data is now sent for this specific
> endpoint?  Is there some sort of "framing" that the device does with the
> USB data so that the driver "knows" where the end of packet is?

Exactly my point.

> Check the zero-packet stuff for this device, that's tripped up many a
> USB driver writer over the years, myself included.

I haven't tripped over it myself, but only because we were careful
to allow for such in the USB drivers I have worked on.

The r8152 driver just assumes it never happens.
--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Greg KH]

On Thu, Nov 24, 2016 at 01:34:08PM -0500, Mark Lord wrote:
> One thought:  bulk data streams are byte streams, not packets.
> Scheduling on the USB bus can break up larger transfers across
> multiple in-kernel buffers.  A "real" URB buffer on USB2 is max 512 bytes.
> The driver is providing 16384-byte buffers, and assumes that data will
> never spill over from one such buffer to the next.
> Yet the observations here consistently show otherwise.

Wait, how do you know that data will not spill over?  What is making
that guarantee?  Will the USB device send a "zero packet" in order to
show that all of the "logical" data is now sent for this specific
endpoint?  Is there some sort of "framing" that the device does with the
USB data so that the driver "knows" where the end of packet is?

Check the zero-packet stuff for this device, that's tripped up many a
USB driver writer over the years, myself included.

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 net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 01:42 PM, Greg KH wrote:
>
> Have you tried using usbmon?

This system is running rootfs over NFS, so usbmon
isn't realistically going to be usable in that scenario
without a lot of reconfiguration of the setup (which in itself
might obscure the original problem).

There is a hardware USB analyzer in the building though.

But it requires a MS-Windows machine (very scarce here, I don't have one)
for the incredibly user-unfriendly software.  I'm not sure if it can be
setup to stop the trace somehow at the right point either, as it takes
overnight runs usually to catch an occurrence of the issue.

I also seem to recall that it only exports data captures in a proprietary
format that only that brand of software/device can read, but perhaps
that might not be true.  Would still need to find a MS-Windows machine/license
to even check it out though.

--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 01:34 PM, Mark Lord wrote:
>From tracing through the powerpc arch code, this is the buffer that
> is being directly DMA'd into.  And the USB layer does an invalidate_dcache
> on that entire buffer before initiating the DMA (confirmed via printk).

Slight correction:  the invalidate_dcache_range() is only done when
using kmalloc'd buffers.  I have converted the driver here
to use usb_alloc_coherent() instead, so that now gets skipped
since the memory is never cached.
--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Greg KH]

On Thu, Nov 24, 2016 at 11:43:53AM -0500, Mark Lord wrote:
> On 16-11-24 11:21 AM, David Miller wrote:
> > From: Hayes Wang 
> > Date: Thu, 24 Nov 2016 13:26:55 +
> > 
> > > I don't think the garbage results from our driver or device.
> > This is my impression with what has been presented so far as well.
> 
> It's not garbage.
> 
> The latest run with the debug code I posted here earlier just spat out this 
> below.
> Using coherent (guarded, non-cacheable) RX buffers, with mb() calls:
> 
> [   15.199157] r8152_check_rx_desc: rx_desc looks bad.
> [   15.204270] r8152_rx_bottom: offset=0/3376 bad rx_desc
> [   15.209584] r8152_dump_rx_desc: 3d435253 3034336d 202f3a30 47524154 
> 2f3d5445 3034336d rx_len=21075
> 
> The bad data in this case is ASCII:
> 
> "SRC=m3400:/ TARGET=/m340"

Have you tried using usbmon?  Details for how to use it is in
Documentation/usbmon.txt and it might help you rule out the driver vs.
the USB host controller issues as it sees the raw data the USB host
controller sees before it sends it to the driver.

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 net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 12:11 PM, David Miller wrote:
> From: Mark Lord 
> Date: Thu, 24 Nov 2016 11:43:53 -0500
> 
>> So even if this were a platform memory coherency issue, one should
>> still never see ASCII data at the beginning of an rx buffer.
> 
> I'm not so convinced, since this is the kind of random corruption one
> would expect to see when dealing with virtual caches that have
> aliasing or similar issues.
> 
> Writes to address X that show up at address Y or not at all are
> precisely the signature of virtual cache aliasing problems.
> 
> Is it a case of the chip writing to X but the cpu is still seeing
> stale data from a previous CPU store?
> 
> For NFS the cpu is writing into the page cache, so we know that
> cpu side stores are where the ASCII text is coming from.
> 
> Now is the r8152 buffer one that the USB host controller is DMA'ing
> into directly, or is it one that SWIOMMU or similar bounce buffering
> is copying into?  In the latter case we are doing cpu stores into
> the area and the writes aren't coming from the device.

>From tracing through the powerpc arch code, this is the buffer that
is being directly DMA'd into.  And the USB layer does an invalidate_dcache
on that entire buffer before initiating the DMA (confirmed via printk).

The driver itself NEVER writes anything to that buffer,
and nobody else has a pointer to it other than the USB host controller,
so there's nothing else that can write to it either.

According to the driver writer, the chip should only ever write a fresh
rx_desc struct at the beginning of a buffer, never ASCII data.

So how does that buffer end up containing ASCII data from the NFS transfers?

The only explanation I can see, is if the URB itself contains
the data that we see in the URB buffer.  Which is what one would expect.
So for that to happen, the ethernet chip must be transferring that data.

The thing that is special about the situation here, is that the processor
is very slow (800Mhz 32-bit powerpc), and very busy elsewhere.
So it can easily fall way behind in servicing the ethernet dongle,
something that never happens with most modern faster machines.
So perhaps this results in a FIFO overflow somewhere in the chip.

We can boot/run this same machine from a USB memory stick, and nary a problem.
Ditto for other types of ethernet dongles.
But boot/run from that specific ethernet dongle, and we get regular
random segfaults from corrupted page fetches over NFS.

The only end-to-end data integrity available here is the rx checksum,
when verified by software rather than trusting it to the chip/driver.

One thought:  bulk data streams are byte streams, not packets.
Scheduling on the USB bus can break up larger transfers across
multiple in-kernel buffers.  A "real" URB buffer on USB2 is max 512 bytes.
The driver is providing 16384-byte buffers, and assumes that data will
never spill over from one such buffer to the next.
Yet the observations here consistently show otherwise.

Cheers
-- 
Mark Lord
--
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

Amlogic Meson GXL/GXM USB support (dwc2 and dwc3)

[Martin Blumenstingl]

Hello,

currently I am trying to get the USB controllers on the Amlogic Meson
GXL SoCs working: there is one dwc2 and dwc3 controller each.

The SoC itself provides 3x USB2 PHYs and 1x USB3 PHY.
I wrote drivers for both of them based on the vendor kernel, see [0]
The PHY registers of the USB2 PHYs seem to be identical, regardless of
whether they are connected to dwc2 or dwc3.
The USB3 PHY also takes care of the OTG interrupts (to switch between
host and device) and seems to inform the USB2 PHY about mode-changes.
USB3 seems to be disabled in the dwc3 configuration, meaning it
provides only high-speed support.

The dwc3 core fails to initialize currently due to some DMA issues
which will be fixed in Linux v4.10 - the corresponding patchset can be
found here: [1]
With these patches applied we get the dwc3 controller to initialize:
xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 1
xhci-hcd xhci-hcd.0.auto: hcc params 0x0228f664 hci version 0x100
quirks 0x00010010
xhci-hcd xhci-hcd.0.auto: irq 20, io mem 0xc900
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 3 ports detected
xhci-hcd xhci-hcd.0.auto: xHCI Host Controller
xhci-hcd xhci-hcd.0.auto: new USB bus registered, assigned bus number 2
usb usb2: We don't know the algorithms for LPM for this host, disabling LPM.
hub 2-0:1.0: USB hub found
hub 2-0:1.0: config failed, hub doesn't have any ports! (err -19)
(the last message seems fine, there are probably no USB3 ports enabled
in the dwc3 hardware configuration)

strange fact #1: there are 3 USB2 PHYs enabled in the dwc3 core
(regdump from the vendor kernel - a full version is attached):
GUSB2PHYCFG(0) = 0x40102500
GUSB2PHYCFG(1) = 0x40102540
GUSB2PHYCFG(2) = 0x40102540
(this explains the "hub 1-0:1.0: 3 ports detected" message on my GXM
board - other SoCs seem to have a different number of ports available
based on the vendor sources, GXL seem to have 2 ports, while "TXL"
seems to have 4 ports).

I tried enabling all available PHYs in the SoC and giving
GUSB2PHYCFG(1 and 2) the same tickle that is currently done in
dwc3_phy_setup() for GUSB2PHYCFG(0).
The LED on my thumb drive flashes when I plug it into the dwc3 port,
but I don't get any interrupts and the kernel does not recognize any
new USB device.

For the dwc2 controller I am probably missing a clock somewhere,
because it's reporting:
dwc2 c910.usb: Configuration mismatch. dr_mode forced to device
dwc2 c910.usb: dwc2_core_reset() HANG! Soft Reset GRSTCTL=8001
dwc2 c910.usb: Specified GNPTXFDEP=1024 > 768
dwc2 c910.usb: EPs: 7, dedicated fifos, 712 entries in SPRAM
I must admit that I have been focusing on the dwc3 controller so far,
so I don't have any more information here.

if any of these problems sound familiar to you or if you have any
advice on what I could experiment with: please don't hesitate and let
me know!
I'll also keep continue looking into this, because it's not unlikely
that I'm running into a platform specific issue.


Regards,
Martin


[0] 
https://github.com/xdarklight/linux/commits/meson-gx-integration-4.10-20161123
[1] http://marc.info/?l=linux-usb=147938307209685=2


sys_kernel_debug_dwc3_regdump_vendor_kernel
Description: Binary data

Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[David Miller]

From: Mark Lord 
Date: Thu, 24 Nov 2016 12:00:15 -0500

> It seems I am being overly helpful here.

Either you want to cry or you want to keep helping us track down
this problem.  It is your choice, and your choice alone.

Please do not pretend otherwise, everyone else in this thread is
operating with the best intentions and wants to see this through
to a full analysis and a proper solution for the corruptions.

Thank you.
--
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 1/2] r8152: fix the sw rx checksum is unavailable

[David Miller]

From: Mark Lord 
Date: Thu, 24 Nov 2016 11:43:53 -0500

> So even if this were a platform memory coherency issue, one should
> still never see ASCII data at the beginning of an rx buffer.

I'm not so convinced, since this is the kind of random corruption one
would expect to see when dealing with virtual caches that have
aliasing or similar issues.

Writes to address X that show up at address Y or not at all are
precisely the signature of virtual cache aliasing problems.

Is it a case of the chip writing to X but the cpu is still seeing
stale data from a previous CPU store?

For NFS the cpu is writing into the page cache, so we know that
cpu side stores are where the ASCII text is coming from.

Now is the r8152 buffer one that the USB host controller is DMA'ing
into directly, or is it one that SWIOMMU or similar bounce buffering
is copying into?  In the latter case we are doing cpu stores into
the area and the writes aren't coming from the device.

--
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 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 11:43 AM, Mark Lord wrote:
..

But how does this ASCII data end up at offset zero of the rx buffer??
Not possible -- this isn't even stale data, because only an rx_desc could
be at that offset in that buffer.


Answering my own question here, I suspect it ends up there as a result
of overrunning the previous URB.  So I have updated the test copy of the driver
here now to check for that exact situation.  It's running now, but could take
hours or a day for the bug to occur again.

It seems I am being overly helpful here.

Perhaps I should have just stopped with the original regression report
(driver works in 3.12.xx, fails on all newer kernels, as a result of enabling
hardware checksums).

Had I left it there, one might reasonably expect the onus to be on the driver
developer to sort it out, with me providing retests of supplied patches as need 
be.

But I've gone WAY BEYOND that, even questioning the sanity of the platform on
which it is being used, just to avoid blaming a buggy USB dongle for some other 
issue.
And this is leading people to suspect that I really think the platform is buggy.

It isn't.   It's been running for years, with a variety of USB hardware 
attached,
and nary a problem.  Except with this r8152 dongle on kernels > 3.12.

So, yeah, the driver is fixed in our local tree, and has been for some time now.
I just was hoping that perhaps others might be interested in it too,
since the bug (whatever it is) corrupts data on the NFS server.

Cheers


--
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] fsl/usb: Workarourd for USB erratum-A005697

[Alan Stern]

On Thu, 24 Nov 2016, Sriram Dash wrote:

> >From: Changming Huang [mailto:jerry.hu...@nxp.com]
> >As per USB specification, in the Suspend state, the status bit does not 
> >change until
> >the port is suspended. However, there may be a delay in suspending a port if 
> >there
> >is a transaction currently in progress on the bus.
> >
> >In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when the
> >application sets it and not when the port is actually suspended
> >
> >Workaround for this issue involves waiting for a minimum of 10ms to allow the
> >controller to go into SUSPEND state before proceeding ahead

The USB core guarantees that there won't be any data transactions in 
progress when a root hub is suspended.  There might possibly be an SOF 
transaction, but that doesn't take more than a few microseconds at 
most.  Certainly nowhere near 10 ms!

Given that we already perform a 150-us delay, is this change really 
needed?

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 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 11:21 AM, David Miller wrote:

From: Hayes Wang 
Date: Thu, 24 Nov 2016 13:26:55 +


I don't think the garbage results from our driver or device.

This is my impression with what has been presented so far as well.


It's not garbage.

The latest run with the debug code I posted here earlier just spat out this 
below.
Using coherent (guarded, non-cacheable) RX buffers, with mb() calls:

[   15.199157] r8152_check_rx_desc: rx_desc looks bad.
[   15.204270] r8152_rx_bottom: offset=0/3376 bad rx_desc
[   15.209584] r8152_dump_rx_desc: 3d435253 3034336d 202f3a30 47524154 2f3d5445 
3034336d rx_len=21075

The bad data in this case is ASCII:

"SRC=m3400:/ TARGET=/m340"

This data is what is seen in /run/mount/utab, a file that is read/written over 
NFS on each boot.

"SRC=m3400:/ TARGET=/m3400 ROOT=/ ATTRS=nolock,addr=192.168.8.1\n"

But how does this ASCII data end up at offset zero of the rx buffer??
Not possible -- this isn't even stale data, because only an rx_desc could
be at that offset in that buffer.

So even if this were a platform memory coherency issue, one should still
never see ASCII data at the beginning of an rx buffer.  The driver NEVER
writes anything to the rx buffers.  Only the USB hardware ever does.

And only the r8152 dongle/driver exhibits this issue.
Other USB dongles do not.  They *might* still have such issues,
but because they use software checksums, the bad packets are caught/rejected.

The r8152 driver, without the debug/error-checking additions, would have tried
to interpret that ASCII data as an "rx_desc", and would have interpreted the
"checksum bits" therein as "valid checksum", and the packet would have passed
through the network stack, corrupting data.

This driver worked without noticeable issues in 3.12.xx.
It hasn't worked since.  Because it now trusts the hardware checksums,
without first checking to see if noise-on-the-line or something else
has corrupted the data before receipt in the rx buffer.

Based on the above capture, I suspect a bug in the chip itself, which perhaps
is only manifest on a very slow CPU.

Nobody here tests with slow CPUs, but they are very prevalent in embedded space.
And very few people use USB network dongles nowadays either, as nearly all 
"computers"
have built-in networking.  The market for USB network dongles is mostly 
embedded space.

Ergo.

Cheers
--
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 1/2] r8152: fix the sw rx checksum is unavailable

[David Miller]

From: Mark Lord 
Date: Thu, 24 Nov 2016 07:31:17 -0500

> Any way we look at it though, the chip/driver are simply unreliable,
> and relying upon hardware checksums (which fail due to the driver
> looking at garbage rather than the checksum bits) leads to data
> corruption.

If the cpu/DMA implementation is the problem, then turning off
checksums is not an appropriate fix at all.

In fact, we have no idea what the cause is yet.

That makes turning off random features no more than grasping at straws
and makes no sense at all upstream.

It may make sense for you to do such a change locally in _your_ tree
to fix your situation temporarily.  But upstream we shouldn't be doing
it.
--
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 1/2] r8152: fix the sw rx checksum is unavailable

[David Miller]

From: Hayes Wang 
Date: Thu, 24 Nov 2016 13:26:55 +

> I don't think the garbage results from our driver or device.

This is my impression with what has been presented so far as well.
--
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/2] usb: host: xhci: handle COMP_STOP from SETUP phase too

[Mathias Nyman]

On 24.11.2016 15:33, Felipe Balbi wrote:

Stop Endpoint command can come at any point and we
have no control of that. We should make sure to
handle COMP_STOP on SETUP phase as well, otherwise
urb->actual_lenght might be set to negative values
in some occasions such as below:

  urb->length = 4;
  build_control_transfer_td_for(urb, ep);

stop_endpoint(ep);

COMP_STOP:
[...]
urb->actual_length = urb->length - trb->length;

trb->length is 8 for SETUP stage (8 control request
bytes), so actual_length would be set to -4 in this
case.

While doing that, also make sure to use TRB_TYPE
field of the actual TRB instead of matching pointers
to figure out in which stage of the control transfer
we got our completion event.

Signed-off-by: Felipe Balbi 
---


This is awsome. This probably fixes tons of issues
related to failing getting port status and other control transfers.

Looking at it now it turns out that in COMP_STOP case we can't rely on
endpoint ring dequeue pointer to be correct, and still we used it to determine
the control transfer stage. (setup, data, status).
No wonder there has been issues related to control transfers.

Using the TRB_TYPE of the actual TRB on the endpoint ring is definitely the
way to go

Thanks
-Mathias


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

Re: [PATCH 1/2] usb: host: xhci: dynamically allocate devs array

[Mathias Nyman]

On 24.11.2016 15:33, Felipe Balbi wrote:

Instead of always defaulting to a 256-entry array,
we can dynamically allocate devs based on what HW
tells us it supports.

Note that we can't, yet, purge MAX_HC_SLOTS
completely because of struct
xhci_device_context_array reliance on it.

Signed-off-by: Felipe Balbi 
---
  drivers/usb/host/xhci-hub.c  |  2 +-
  drivers/usb/host/xhci-mem.c  |  4 ++--
  drivers/usb/host/xhci-ring.c |  2 +-
  drivers/usb/host/xhci.c  | 19 +++
  drivers/usb/host/xhci.h  |  2 +-
  5 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef16900efed..7b1b58ad6aac 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -356,7 +356,7 @@ int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct 
xhci_hcd *xhci,
enum usb_device_speed speed;

slot_id = 0;
-   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   for (i = 0; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {


Normally that is what it should look like, but as we in xhci driver don't use
xhci->devs[0], and xhci->devs[HCS_MAX_SLOTS(xhci->hcs_params1)] can be a valid
virt_device it needs a +1.
Same goes for everywhere else (also when allocating)

This is probably originally due to that xhci hw returns a slot_id 0 for 
failure, and
1 to, including HCD_MAX_SLOTS[hcs_params1) for successful enable device command.

the virt_dev is then straight allocated to xhci->devs[slot_id] = kzalloc(..)
   
-Mathias

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

Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-24 08:26 AM, Hayes Wang wrote:
..

Besides, it doesn't seem to occur for all platforms. I have
tested the iperf more than 26 hours, and it still works fine.
I think I would get the same result on x86 or x86_64 platform.

..

x86 has near fully-coherent memory, so it is the "easy" platform
to get things working on.  But Linux supports a very diverse number
of platforms, with varying degrees of cache/memory coherency,
and it can be tricky for things to work correctly on all of them.

If you are testing with the driver as currently in 4.4.34,
then you won't even notice when things are screwing up,
because the driver just silently drops packets.
Or it passes them on without noticing that they have bad data.

Here (attached) is the instrumented driver I am using here now.
I suggest you use it or something similar when testing,
and not the stock driver.

This one has also been converted to use non-cacheable RAM for the
receive buffers -- something that is probably a Good Thing
for it to do regardless of this investigation.

It also never drops a packet without logging the event,
so we can see just how often there's an issue.

This version behaves almost perfectly here, but I am still experimenting
to see what is actually necessary, and what is not.  In particular,
there are some mb() calls I had put in there that shouldn't be required,
so I have yet to try removing them again and see what changes.

It takes at least an overnight run to pop up one or two errors,
so do expect to hear back again until after the weekend at this point.

Also, unrelated, but inside r8152_submit_rx() there is this code:

/* The rx would be stopped, so skip submitting */
if (test_bit(RTL8152_UNPLUG, >flags) ||
!test_bit(WORK_ENABLE, >flags) || !netif_carrier_ok(tp->netdev))
   return 0;

If that "return 0" statement is ever executed, doesn't it result
in the loss/leak of a buffer?

Thanks


/*
 *  Copyright (c) 2014 Realtek Semiconductor Corp. All rights reserved.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * version 2 as published by the Free Software Foundation.
 *
 */

#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 

/* Information for net-next */
#define NETNEXT_VERSION		"08"

/* Information for net */
#define NET_VERSION		"2"

#define DRIVER_VERSION		"v1." NETNEXT_VERSION "." NET_VERSION
#define DRIVER_AUTHOR "Realtek linux nic maintainers "
#define DRIVER_DESC "Realtek RTL8152/RTL8153 Based USB Ethernet Adapters"
#define MODULENAME "r8152"

#define R8152_PHY_ID		32

#define PLA_IDR			0xc000
#define PLA_RCR			0xc010
#define PLA_RMS			0xc016
#define PLA_RXFIFO_CTRL0	0xc0a0
#define PLA_RXFIFO_CTRL1	0xc0a4
#define PLA_RXFIFO_CTRL2	0xc0a8
#define PLA_DMY_REG0		0xc0b0
#define PLA_FMC			0xc0b4
#define PLA_CFG_WOL		0xc0b6
#define PLA_TEREDO_CFG		0xc0bc
#define PLA_MAR			0xcd00
#define PLA_BACKUP		0xd000
#define PAL_BDC_CR		0xd1a0
#define PLA_TEREDO_TIMER	0xd2cc
#define PLA_REALWOW_TIMER	0xd2e8
#define PLA_LEDSEL		0xdd90
#define PLA_LED_FEATURE		0xdd92
#define PLA_PHYAR		0xde00
#define PLA_BOOT_CTRL		0xe004
#define PLA_GPHY_INTR_IMR	0xe022
#define PLA_EEE_CR		0xe040
#define PLA_EEEP_CR		0xe080
#define PLA_MAC_PWR_CTRL	0xe0c0
#define PLA_MAC_PWR_CTRL2	0xe0ca
#define PLA_MAC_PWR_CTRL3	0xe0cc
#define PLA_MAC_PWR_CTRL4	0xe0ce
#define PLA_WDT6_CTRL		0xe428
#define PLA_TCR0		0xe610
#define PLA_TCR1		0xe612
#define PLA_MTPS		0xe615
#define PLA_TXFIFO_CTRL		0xe618
#define PLA_RSTTALLY		0xe800
#define PLA_CR			0xe813
#define PLA_CRWECR		0xe81c
#define PLA_CONFIG12		0xe81e	/* CONFIG1, CONFIG2 */
#define PLA_CONFIG34		0xe820	/* CONFIG3, CONFIG4 */
#define PLA_CONFIG5		0xe822
#define PLA_PHY_PWR		0xe84c
#define PLA_OOB_CTRL		0xe84f
#define PLA_CPCR		0xe854
#define PLA_MISC_0		0xe858
#define PLA_MISC_1		0xe85a
#define PLA_OCP_GPHY_BASE	0xe86c
#define PLA_TALLYCNT		0xe890
#define PLA_SFF_STS_7		0xe8de
#define PLA_PHYSTATUS		0xe908
#define PLA_BP_BA		0xfc26
#define PLA_BP_0		0xfc28
#define PLA_BP_1		0xfc2a
#define PLA_BP_2		0xfc2c
#define PLA_BP_3		0xfc2e
#define PLA_BP_4		0xfc30
#define PLA_BP_5		0xfc32
#define PLA_BP_6		0xfc34
#define PLA_BP_7		0xfc36
#define PLA_BP_EN		0xfc38

#define USB_USB2PHY		0xb41e
#define USB_SSPHYLINK2		0xb428
#define USB_U2P3_CTRL		0xb460
#define USB_CSR_DUMMY1		0xb464
#define USB_CSR_DUMMY2		0xb466
#define USB_DEV_STAT		0xb808
#define USB_CONNECT_TIMER	0xcbf8
#define USB_BURST_SIZE		0xcfc0
#define USB_USB_CTRL		0xd406
#define USB_PHY_CTRL		0xd408
#define USB_TX_AGG		0xd40a
#define USB_RX_BUF_TH		0xd40c
#define USB_USB_TIMER		0xd428
#define USB_RX_EARLY_TIMEOUT	0xd42c
#define USB_RX_EARLY_SIZE	0xd42e
#define USB_PM_CTRL_STATUS	0xd432
#define USB_TX_DMA		0xd434
#define USB_TOLERANCE		0xd490
#define USB_LPM_CTRL		0xd41a
#define USB_UPS_CTRL	

Re: [PATCH V12 1/1] usb:serial: Add Fintek F81532/534 driver

[Johan Hovold]

On Mon, Nov 14, 2016 at 01:37:59PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> This driver is for Fintek F81532/F81534 USB to Serial Ports IC.
> 
> F81532 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFOTRRMmhWQVNvajQ/view?usp=
> sharing
> 
> F81534 spec:
> https://drive.google.com/file/d/0B8vRwwYO7aMFV29pQWJqbVBNc00/view?usp=
> sharing
> 
> Features:
> 1. F81532 is 1-to-2 & F81534 is 1-to-4 serial ports IC
> 2. Support Baudrate from B50 to B115200.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) 
> ---
> Changelog:
> V12
> 1. Max TX change from 100 to 124 bytes.
> 2. Add probe() to verify endpoints & packet size.
> 3. Rename function names.
>  set/get_normal_register() -> set/get_register()
>  set/get_register() -> set/get_port_register()
>  set/get_register_delay() -> set/get_spi_register()
>  read_data() -> read_flash()
>  command_delay() -> wait_for_spi()

> +static int f81534_probe(struct usb_serial *serial,
> + const struct usb_device_id *id)
> +{
> + struct usb_endpoint_descriptor *endpoint;
> + struct usb_host_interface *iface_desc;
> + struct device *dev;
> + int num_bulk_in = 0;
> + int num_bulk_out = 0;
> + int size_bulk_in = 0;
> + int size_bulk_out = 0;
> + int i;
> +
> + dev = >interface->dev;
> + iface_desc = serial->interface->cur_altsetting;
> +
> + for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> + endpoint = _desc->endpoint[i].desc;
> +
> + if (usb_endpoint_is_bulk_in(endpoint)) {
> + ++num_bulk_in;
> + size_bulk_in = usb_endpoint_maxp(endpoint);
> + }
> +
> + if (usb_endpoint_is_bulk_out(endpoint)) {
> + ++num_bulk_out;
> + size_bulk_out = usb_endpoint_maxp(endpoint);
> + }
> + }
> +
> + if (num_bulk_in != 1 || num_bulk_out != 1) {
> + dev_err(dev, "%s: endpoints not matched\n", __func__);

Better to spell out: "expected endpoints not found\n".

> + return -EINVAL;

And this should be -ENODEV;

> + }
> +
> + if (size_bulk_out != F81534_WRITE_BUFFER_SIZE ||
> + size_bulk_in != F81534_MAX_RECEIVE_BLOCK_SIZE) {
> + dev_err(dev, "%s: endpoints packet size not matched\n",
> + __func__);

Similarly: "unsupported endpoint max packet size\n".

But just to be clear: You do want to bail out if connected at full
speed? You could also ask usb-serial core to allocate large enough
buffers (e.g. by setting the bulk_out_size driver field) and the host
controller will handle partitioning.

> + return -EINVAL;

And -ENODEV.

> + }
> +
> + return 0;
> +}

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

[GIT PULL] USB driver fixes for 4.9-rc7

[Greg KH]

The following changes since commit a25f0944ba9b1d8a6813fd6f1a86f1bd59ac25a6:

  Linux 4.9-rc5 (2016-11-13 10:32:32 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/ tags/usb-4.9-rc7

for you to fetch changes up to c0da038d7afed2892346fdb9601e4fefee13a800:

  Merge tag 'usb-serial-4.9-rc6' of 
git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial into usb-linus 
(2016-11-18 15:49:31 +0100)


USB fixes for 4.9-rc7

Here are a few small USB fixes and new device ids for 4.9-rc7.

The majority of these fixes are in the musb driver, fixing a number of
regressions that have been reported but took a while to resolve.  The
other fixes are all small ones, to resolve other reported minor issues.

All have been in linux-next for a while with no reported issues.

Signed-off-by: Greg Kroah-Hartman 


Doug Brown (1):
  USB: serial: ftdi_sio: add support for TI CC3200 LaunchPad

Felix Hädicke (1):
  usb: gadget: f_fs: fix wrong parenthesis in ffs_func_req_match()

Greg Kroah-Hartman (2):
  Merge tag 'fixes-for-v4.9-rc6' of git://git.kernel.org/.../balbi/usb into 
usb-linus
  Merge tag 'usb-serial-4.9-rc6' of 
git://git.kernel.org/.../johan/usb-serial into usb-linus

Paul Jakma (1):
  USB: serial: cp210x: add ID for the Zone DPMX

Peter Chen (1):
  usb: chipidea: move the lock initialization to core file

Petr Vandrovec (1):
  Fix USB CB/CBI storage devices with CONFIG_VMAP_STACK=y

Tony Lindgren (6):
  usb: musb: Fix broken use of static variable for multiple instances
  usb: musb: Fix sleeping function called from invalid context for hdrc glue
  usb: musb: Fix PM for hub disconnect
  usb: musb: Add missing pm_runtime_disable and drop 2430 PM timeout
  usb: musb: Drop pointless PM runtime code for dsps glue
  phy: twl4030-usb: Fix for musb session bit based PM

 drivers/phy/phy-twl4030-usb.c  |   4 +-
 drivers/usb/chipidea/core.c|   1 +
 drivers/usb/chipidea/udc.c |   2 -
 drivers/usb/gadget/function/f_fs.c |   8 +-
 drivers/usb/musb/musb_core.c   | 147 -
 drivers/usb/musb/musb_core.h   |  13 +++-
 drivers/usb/musb/musb_dsps.c   |  58 +++
 drivers/usb/musb/musb_gadget.c |  39 --
 drivers/usb/musb/omap2430.c|  10 +--
 drivers/usb/musb/tusb6010.c|   6 +-
 drivers/usb/serial/cp210x.c|   1 +
 drivers/usb/serial/ftdi_sio.c  |   2 +
 drivers/usb/serial/ftdi_sio_ids.h  |   6 ++
 drivers/usb/storage/transport.c|   7 +-
 14 files changed, 229 insertions(+), 75 deletions(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] xhci: Remove busy loop from xhci_abort_cmd_ring()

[OGAWA Hirofumi]

Mathias Nyman  writes:

>> - Add xhci_handshake_sleep(), and use it.
>
> This seems a but overkill, I'd rather don't have xhci_handshake(),
> xhci_handshake_sleep() and __xhci_handshake() to maintain.

I agree about it. However, on other hand, I thought about the
possibility/effort to decreasing use-cases of xhci_handshake()
busyloop. (there are several places to use more than e.g. 500ms
timeout.)  Because long busyloop (especially with interrupt-off) has
really bad effect to whole system of non-usb.

> As we now can sleep in xhci_abort_cmd_ring() I would rather just first
> wait for the completion and then maybe handshake check the register.
> At that point it shouldn't need to busyloop anymore, one read should
> do it.

I worried about in the case of real internal confusing, and consider
about chip that doesn't have no stop/abort event.

To handle both case, timeout choice would not be straight-forward.

> But this is all just optimizing an error path, I'm actually fine with
> taking just patch 1/3 and 2/3 v3, and skipping 3/3. the udelay(1000)
> to msleep(1) I can add myself

Right. The main point of patchset is 1/3 and 2/3.

>> As related change, current xhci_handshake() is strange behavior,
>> E.g. xhci_handshake(ptr, mask, done, 1) does
>>
>>  result = readl(ptr);
>>  /* check result */
>>  udelay(1);  <= meaningless delay
>>  return -ETIMEDOUT;
>
> Only in the timeout case, so if we after 3 or 5 million register reads
> + udelays(1) still don't get the value we want then there is an
> unnecessary udelay(1).
>
> Not worth putting much effort on it now.

True. But actual effort for it was very small.

@@ -65,16 +65,18 @@ int xhci_handshake(void __iomem *ptr, u3
 {
u32 result;
 
-   do {
+   while (1) {
result = readl(ptr);
if (result == ~(u32)0)  /* card removed */
return -ENODEV;
result &= mask;
if (result == done)
return 0;
+   if (usec <= 0)
+   break;
udelay(1);
usec--;
-   } while (usec > 0);
+   }
return -ETIMEDOUT;
 }

Thanks.
-- 
OGAWA Hirofumi 
--
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 2/2] usb: host: xhci: handle COMP_STOP from SETUP phase too

[Felipe Balbi]

Stop Endpoint command can come at any point and we
have no control of that. We should make sure to
handle COMP_STOP on SETUP phase as well, otherwise
urb->actual_lenght might be set to negative values
in some occasions such as below:

 urb->length = 4;
 build_control_transfer_td_for(urb, ep);

stop_endpoint(ep);

COMP_STOP:
[...]
urb->actual_length = urb->length - trb->length;

trb->length is 8 for SETUP stage (8 control request
bytes), so actual_length would be set to -4 in this
case.

While doing that, also make sure to use TRB_TYPE
field of the actual TRB instead of matching pointers
to figure out in which stage of the control transfer
we got our completion event.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ring.c | 30 ++
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 44cade15792b..40be843a1b2a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1939,8 +1939,9 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
struct xhci_ep_ctx *ep_ctx;
u32 trb_comp_code;
u32 remaining, requested;
-   bool on_data_stage;
+   u32 trb_type;
 
+   trb_type = TRB_FIELD_TO_TYPE(le32_to_cpu(ep_trb->generic.field[3]));
slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
xdev = xhci->devs[slot_id];
ep_index = TRB_TO_EP_ID(le32_to_cpu(event->flags)) - 1;
@@ -1950,14 +1951,11 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
requested = td->urb->transfer_buffer_length;
remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
 
-   /* not setup (dequeue), or status stage means we are at data stage */
-   on_data_stage = (ep_trb != ep_ring->dequeue && ep_trb != td->last_trb);
-
switch (trb_comp_code) {
case COMP_SUCCESS:
-   if (ep_trb != td->last_trb) {
+   if (trb_type != TRB_STATUS) {
xhci_warn(xhci, "WARN: Success on ctrl %s TRB without 
IOC set?\n",
- on_data_stage ? "data" : "setup");
+   (trb_type == TRB_DATA) ? "data" : 
"setup");
*status = -ESHUTDOWN;
break;
}
@@ -1967,15 +1965,23 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
*status = 0;
break;
case COMP_STOP_SHORT:
-   if (on_data_stage)
+   if (trb_type == TRB_DATA)
td->urb->actual_length = remaining;
else
xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl 
setup or status TRB\n");
goto finish_td;
case COMP_STOP:
-   if (on_data_stage)
+   switch (trb_type) {
+   case TRB_SETUP:
+   td->urb->actual_length = 0;
+   goto finish_td;
+   case TRB_DATA:
td->urb->actual_length = requested - remaining;
-   goto finish_td;
+   goto finish_td;
+   default:
+   xhci_warn(xhci, "WARN: unexpected TRB Type %d\n", 
trb_type);
+   goto finish_td;
+   }
case COMP_STOP_INVAL:
goto finish_td;
default:
@@ -1987,7 +1993,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
/* else fall through */
case COMP_STALL:
/* Did we transfer part of the data (middle) phase? */
-   if (on_data_stage)
+   if (trb_type == TRB_DATA)
td->urb->actual_length = requested - remaining;
else if (!td->urb_length_set)
td->urb->actual_length = 0;
@@ -1995,14 +2001,14 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
}
 
/* stopped at setup stage, no data transferred */
-   if (ep_trb == ep_ring->dequeue)
+   if (trb_type == TRB_SETUP)
goto finish_td;
 
/*
 * if on data stage then update the actual_length of the URB and flag it
 * as set, so it won't be overwritten in the event for the last TRB.
 */
-   if (on_data_stage) {
+   if (trb_type == TRB_DATA) {
td->urb_length_set = true;
td->urb->actual_length = requested - remaining;
xhci_dbg(xhci, "Waiting for status stage event\n");
-- 
2.10.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

[PATCH 1/2] usb: host: xhci: dynamically allocate devs array

[Felipe Balbi]

Instead of always defaulting to a 256-entry array,
we can dynamically allocate devs based on what HW
tells us it supports.

Note that we can't, yet, purge MAX_HC_SLOTS
completely because of struct
xhci_device_context_array reliance on it.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-hub.c  |  2 +-
 drivers/usb/host/xhci-mem.c  |  4 ++--
 drivers/usb/host/xhci-ring.c |  2 +-
 drivers/usb/host/xhci.c  | 19 +++
 drivers/usb/host/xhci.h  |  2 +-
 5 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef16900efed..7b1b58ad6aac 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -356,7 +356,7 @@ int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct 
xhci_hcd *xhci,
enum usb_device_speed speed;
 
slot_id = 0;
-   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   for (i = 0; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
if (!xhci->devs[i])
continue;
speed = xhci->devs[i]->udev->speed;
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 321de2e0161b..05c8132f77c8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1828,7 +1828,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
}
}
 
-   for (i = 1; i < MAX_HC_SLOTS; ++i)
+   for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); ++i)
xhci_free_virt_device(xhci, i);
 
dma_pool_destroy(xhci->segment_pool);
@@ -2535,7 +2535,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
 * something other than the default (~1ms minimum between interrupts).
 * See section 5.5.1.2.
 */
-   for (i = 0; i < MAX_HC_SLOTS; ++i)
+   for (i = 0; i < HCS_MAX_SLOTS(xhci->hcs_params1); ++i)
xhci->devs[i] = NULL;
for (i = 0; i < USB_MAXCHILDREN; ++i) {
xhci->bus_state[0].resume_done[i] = 0;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index bdf6b13d9b67..44cade15792b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -895,7 +895,7 @@ void xhci_stop_endpoint_command_watchdog(unsigned long arg)
 * doesn't touch the memory.
 */
}
-   for (i = 0; i < MAX_HC_SLOTS; i++) {
+   for (i = 0; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
if (!xhci->devs[i])
continue;
for (j = 0; j < 31; j++)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 1cd56417cbec..c8bbff4e57db 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -766,6 +766,8 @@ void xhci_shutdown(struct usb_hcd *hcd)
/* Yet another workaround for spurious wakeups at shutdown with HSW */
if (xhci->quirks & XHCI_SPURIOUS_WAKEUP)
pci_set_power_state(to_pci_dev(hcd->self.controller), 
PCI_D3hot);
+
+   kfree(xhci->devs);
 }
 
 #ifdef CONFIG_PM
@@ -4896,6 +4898,11 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
 
xhci->quirks |= quirks;
 
+   xhci->devs = kcalloc(HCS_MAX_SLOTS(xhci->hcs_params1),
+   sizeof(struct xhci_virt_device *), GFP_KERNEL);
+   if (!xhci->devs)
+   return -ENOMEM;
+
get_quirks(dev, xhci);
 
/* In xhci controllers which follow xhci 1.0 spec gives a spurious
@@ -4908,13 +4915,13 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
/* Make sure the HC is halted. */
retval = xhci_halt(xhci);
if (retval)
-   return retval;
+   goto err;
 
xhci_dbg(xhci, "Resetting HCD\n");
/* Reset the internal HC memory state and registers. */
retval = xhci_reset(xhci);
if (retval)
-   return retval;
+   goto err;
xhci_dbg(xhci, "Reset complete\n");
 
/*
@@ -4940,7 +4947,7 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t 
get_quirks)
 */
retval = dma_set_mask(dev, DMA_BIT_MASK(32));
if (retval)
-   return retval;
+   goto err;
xhci_dbg(xhci, "Enabling 32-bit DMA addresses.\n");
dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
}
@@ -4949,13 +4956,17 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
/* Initialize HCD and host controller data structures. */
retval = xhci_init(hcd);
if (retval)
-   return retval;
+   goto err;
xhci_dbg(xhci, "Called HCD init\n");
 
xhci_info(xhci, "hcc params 0x%08x hci version 0x%x quirks 0x%08x\n",
  xhci->hcc_params, xhci->hci_version, xhci->quirks);
 
return 0;
+
+err:
+   kfree(xhci->devs);
+

[PATCH 0/2] usb: host: xhci: two small fixes

[Felipe Balbi]

Hi Mathias,

these two patches help with two minor details in XHCI.

Patch 1 reduces the amount of memory used by our virt_device array by
discovering from the HW how many slots we can support.

Patch 2 makes COMP_STOP work when a Stop Endpoint command is issued
while we're still in SETUP stage. Note how I have also converted
on_data_stage and all the pointer mathing to more robust TRB type
matching.

Tested (albeit lightly with USB storage and keyboard + rootfs on another
USB storage) with a SKL box. Patches on top of today's next/master

Felipe Balbi (2):
  usb: host: xhci: dynamically allocate devs array
  usb: host: xhci: handle COMP_STOP from SETUP phase too

 drivers/usb/host/xhci-hub.c  |  2 +-
 drivers/usb/host/xhci-mem.c  |  4 ++--
 drivers/usb/host/xhci-ring.c | 32 +++-
 drivers/usb/host/xhci.c  | 19 +++
 drivers/usb/host/xhci.h  |  2 +-
 5 files changed, 38 insertions(+), 21 deletions(-)

-- 
2.10.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 net 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

Mark Lord [mailto:ml...@pobox.com]
> Sent: Thursday, November 24, 2016 8:31 PM
[...]
> Nope.  Guard zones did not fix it, so it's probably not a prefetch issue.
> Oddly, adding a couple of memory barriers to specific places in the driver
> does help, A LOT.  Still not 100%, but it did pass 1800 reboot tests over 
> night
> with only three bad rx_desc's reported.
> 
> That's a new record here for the driver using kmalloc'd buffers,
> and put reliability on par with using non-cacheable buffers.
> 
> Any way we look at it though, the chip/driver are simply unreliable,
> and relying upon hardware checksums (which fail due to the driver
> looking at garbage rather than the checksum bits) leads to data corruption.

I don't think the garbage results from our driver or device.
If it is the issue about memory, I think the host driver ought
to deal with it, because it handles the DMA.

Besides, it doesn't seem to occur for all platforms. I have
tested the iperf more than 26 hours, and it still works fine.
I think I would get the same result on x86 or x86_64 platform.

Best Regards,
Hayes

--
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] xhci: Remove busy loop from xhci_abort_cmd_ring()

[Mathias Nyman]

On 16.11.2016 07:01, OGAWA Hirofumi wrote:

Now, xhci_abort_cmd_ring() is sleepable. So no reason to use busy loop
anymore.

- Convert udelay(1000) => msleep(1).


Sounds good.


- Add xhci_handshake_sleep(), and use it.


This seems a but overkill, I'd rather don't have xhci_handshake(), 
xhci_handshake_sleep()
and __xhci_handshake() to maintain.

As we now can sleep in xhci_abort_cmd_ring() I would rather just first wait for 
the completion
and then maybe handshake check the register.  At that point it shouldn't need 
to busyloop anymore,
one read should do it.

But this is all just optimizing an error path, I'm actually fine with taking 
just
patch 1/3 and 2/3 v3, and skipping 3/3. the udelay(1000) to msleep(1) I can add 
myself
 


As related change, current xhci_handshake() is strange behavior,
E.g. xhci_handshake(ptr, mask, done, 1) does

 result = readl(ptr);
 /* check result */
 udelay(1); <= meaningless delay
 return -ETIMEDOUT;


Only in the timeout case, so if we after 3 or 5 million register reads + 
udelays(1)
still don't get the value we want then there is an unnecessary udelay(1).

Not worth putting much effort on it now.

Sorry about the review delay, I got my hands quite full at the moment

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

RE: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Hayes Wang]

Mark Lord [mailto:ml...@pobox.com]
> Sent: Wednesday, November 23, 2016 9:41 PM
[...] 
> >static void r8153_set_rx_early_size(struct r8152 *tp)
> >{
> >u32 mtu = tp->netdev->mtu;
> >u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;
> >
> >ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
> >}
> 
> How is ocp_data used by the hardware?
> Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?

I check your question with our hw engineers, and you are right.
The size of rx descriptor should be calculated, too.

Best Regards,
Hayes


N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

Re: [PATCH v3 1/2] usb: host: plat: Enable xhci plat runtime PM

[Baolin Wang]

On 24 November 2016 at 01:39, kbuild test robot <l...@intel.com> wrote:
> Hi Baolin,
>
> [auto build test ERROR on v4.9-rc6]
> [also build test ERROR on next-20161123]
> [cannot apply to balbi-usb/next usb/usb-testing]
> [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/Baolin-Wang/usb-host-plat-Enable-xhci-plat-runtime-PM/20161124-012323
> config: x86_64-randconfig-x008-201647 (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/usb/host/xhci-plat.c: In function 'xhci_plat_remove':
>>> drivers/usb/host/xhci-plat.c:281:28: error: 'pdev' undeclared (first use in 
>>> this function)
>  pm_runtime_set_suspended(>dev);

Oops, will fix my mistake in new patch. Please ignore this patch. Sorry.

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

Re: [PATCH net 1/2] r8152: fix the sw rx checksum is unavailable

[Mark Lord]

On 16-11-23 02:29 PM, Mark Lord wrote:

On 16-11-23 10:12 AM, Hayes Wang wrote:

Mark Lord [ml...@pobox.com]
[...]

What does this code do:



static void r8153_set_rx_early_size(struct r8152 *tp)
{
   u32 mtu = tp->netdev->mtu;
   u32 ocp_data = (agg_buf_sz - mtu - VLAN_ETH_HLEN - VLAN_HLEN) / 4;

   ocp_write_word(tp, MCU_TYPE_USB, USB_RX_EARLY_SIZE, ocp_data);
}


This only works for RTL8153. However, what you use is RTL8152.
It is like delay completion. It is used to reduce the loading of CPU
by letting a transfer contain more data to reduce the number of
transfers.


How is ocp_data used by the hardware?
Shouldn't the calculation also include sizeof(rx_desc) in there somewhere?


The algorithm is from our hw engineers, and it should be

   (agg_buf_sz - packet size) / 8

You could refer to commit a59e6d815226 ("r8152: correct the rx early size").


Thanks.

Right now I am working quite hard trying to narrow things down exactly.
You are correct that the driver does appear to be careful about accesses
beyond the filled portion of a URB buffer -- for some reason I thought
the original driver had issues there, but looking again it does not seem to.

One idea that is now looking more likely:
Things could be suffering from speculative CPU accesses to RAM
(the system here has non-coherent d-cache/RAM).
This could incorrectly pre-load data from adjacent URB buffers
into the d-cache, creating coherency issues.  I am testing now
with cacheline-sized guard zones between the buffers to see if
that is the issue or not.


Nope.  Guard zones did not fix it, so it's probably not a prefetch issue.
Oddly, adding a couple of memory barriers to specific places in the driver
does help, A LOT.  Still not 100%, but it did pass 1800 reboot tests over night
with only three bad rx_desc's reported.

That's a new record here for the driver using kmalloc'd buffers,
and put reliability on par with using non-cacheable buffers.

Any way we look at it though, the chip/driver are simply unreliable,
and relying upon hardware checksums (which fail due to the driver
looking at garbage rather than the checksum bits) leads to data corruption.

Cheers



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

[PATCHv13 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

The purpose of USB Type-C connector class is to provide
unified interface for the user space to get the status and
basic information about USB Type-C connectors on a system,
control over data role swapping, and when the port supports
USB Power Delivery, also control over power role swapping
and Alternate Modes.

Signed-off-by: Heikki Krogerus 
Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
---
 Documentation/ABI/testing/sysfs-class-typec |  220 ++
 Documentation/usb/typec.txt |  110 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |7 +
 drivers/usb/typec/Makefile  |1 +
 drivers/usb/typec/typec.c   | 1013 +++
 include/linux/usb/typec.h   |  254 +++
 9 files changed, 1618 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/Documentation/ABI/testing/sysfs-class-typec 
b/Documentation/ABI/testing/sysfs-class-typec
new file mode 100644
index 000..853fe2d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,220 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:  /sys/class/typec//current_data_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current USB data role the port is operating in. This
+   attribute can be used for requesting data role swapping on the
+   port. Swapping is supported as synchronous operation, so
+   write(2) to the attribute will not return until the operation
+   has finished. The attribute is notified about role changes so
+   that poll(2) on the attribute wakes up. Change on the role will
+   also generate uevent KOBJ_CHANGE on the port.
+
+   Valid values:
+   - host
+   - device
+
+What:  /sys/class/typec//current_power_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current power role of the port. This attribute can be used
+   to request power role swap on the port when the port supports
+   USB Power Delivery. Swapping is supported as synchronous
+   operation, so write(2) to the attribute will not return until
+   the operation has finished. The attribute is notified about role
+   changes so that poll(2) on the attribute wakes up. Change on the
+   role will also generate uevent KOBJ_CHANGE.
+
+   Valid values:
+   - source
+   - sink
+
+What:  /sys/class/typec//vconn_source
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows is the port VCONN Source. This attribute can be used to
+   request VCONN swap to change the VCONN Source during connection
+   when both the port and the partner support USB Power Delivery.
+   Swapping is supported as synchronous operation, so write(2) to
+   the attribute will not return until the operation has finished.
+   The attribute is notified about VCONN source changes so that
+   poll(2) on the attribute wakes up. Change on VCONN source also
+   generates uevent KOBJ_CHANGE.
+
+   Valid values are:
+   - 0 when the port is not the VCONN Source
+   - 1 when the port is the VCONN Source
+
+What:  /sys/class/typec//power_operation_mode
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows the current power operational mode the port is in.
+
+   Valid values:
+   - USB - Normal power levels defined in USB specifications
+   - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
+   specification.
+   - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
+   specification.
+- USB Power Delivery - The voltages and currents defined in USB
+  Power Delivery specification
+
+What:  /sys/class/typec//preferred_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   

[PATCHv13 1/3] lib/string: add sysfs_match_string helper

[Heikki Krogerus]

Make a simple helper for matching strings with sysfs
attribute files. In most parts the same as match_string(),
except sysfs_match_string() uses sysfs_streq() instead of
strcmp() for matching. This is more convenient when used
with sysfs attributes.

Signed-off-by: Heikki Krogerus 
Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
---
 include/linux/string.h | 10 ++
 lib/string.c   | 26 ++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a..c4011b2 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+int __sysfs_match_string(const char * const *array, size_t n, const char *s);
+
+/**
+ * sysfs_match_string - matches given string in an array
+ * @_a: array of strings
+ * @_s: string to match with
+ *
+ * Helper for __sysfs_match_string(). Calculates the size of @a automatically.
+ */
+#define sysfs_match_string(_a, _s) __sysfs_match_string(_a, ARRAY_SIZE(_a), _s)
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562..c7a20cb 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
const char *string)
 }
 EXPORT_SYMBOL(match_string);
 
+/**
+ * __sysfs_match_string - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq instead of strcmp for matching.
+ */
+int __sysfs_match_string(const char * const *array, size_t n, const char *str)
+{
+   const char *item;
+   int index;
+
+   for (index = 0; index < n; index++) {
+   item = array[index];
+   if (!item)
+   break;
+   if (!sysfs_streq(item, str))
+   return index;
+   }
+
+   return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_match_string);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value
-- 
2.10.2

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

[PATCHv13 3/3] usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

[Heikki Krogerus]

This adds driver for the USB Type-C PHY on Intel WhiskeyCove
PMIC which is available on some of the Intel Broxton SoC
based platforms.

Signed-off-by: Heikki Krogerus 
Reviewed-by: Guenter Roeck 
---
 drivers/usb/typec/Kconfig   |  14 ++
 drivers/usb/typec/Makefile  |   1 +
 drivers/usb/typec/typec_wcove.c | 373 
 3 files changed, 388 insertions(+)
 create mode 100644 drivers/usb/typec/typec_wcove.c

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 17792f9..2abbcb0 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -4,4 +4,18 @@ menu "USB Power Delivery and Type-C drivers"
 config TYPEC
tristate
 
+config TYPEC_WCOVE
+   tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
+   depends on ACPI
+   depends on INTEL_SOC_PMIC
+   depends on INTEL_PMC_IPC
+   select TYPEC
+   help
+ This driver adds support for USB Type-C detection on Intel Broxton
+ platforms that have Intel Whiskey Cove PMIC. The driver can detect the
+ role and cable orientation.
+
+ To compile this driver as module, choose M here: the module will be
+ called typec_wcove
+
 endmenu
diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
index 1012a8b..b9cb862 100644
--- a/drivers/usb/typec/Makefile
+++ b/drivers/usb/typec/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_TYPEC)+= typec.o
+obj-$(CONFIG_TYPEC_WCOVE)  += typec_wcove.o
diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
new file mode 100644
index 000..9065d7b
--- /dev/null
+++ b/drivers/usb/typec/typec_wcove.c
@@ -0,0 +1,373 @@
+/**
+ * typec_wcove.c - WhiskeyCove PMIC USB Type-C PHY driver
+ *
+ * Copyright (C) 2016 Intel Corporation
+ * Author: Heikki Krogerus 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Register offsets */
+#define WCOVE_CHGRIRQ0 0x4e09
+#define WCOVE_PHYCTRL  0x5e07
+
+#define USBC_CONTROL1  0x7001
+#define USBC_CONTROL2  0x7002
+#define USBC_CONTROL3  0x7003
+#define USBC_CC1_CTRL  0x7004
+#define USBC_CC2_CTRL  0x7005
+#define USBC_STATUS1   0x7007
+#define USBC_STATUS2   0x7008
+#define USBC_STATUS3   0x7009
+#define USBC_IRQ1  0x7015
+#define USBC_IRQ2  0x7016
+#define USBC_IRQMASK1  0x7017
+#define USBC_IRQMASK2  0x7018
+
+/* Register bits */
+
+#define USBC_CONTROL1_MODE_DRP(r)  (((r) & ~0x7) | 4)
+
+#define USBC_CONTROL2_UNATT_SNKBIT(0)
+#define USBC_CONTROL2_UNATT_SRCBIT(1)
+#define USBC_CONTROL2_DIS_ST   BIT(2)
+
+#define USBC_CONTROL3_PD_DIS   BIT(1)
+
+#define USBC_CC_CTRL_VCONN_EN  BIT(1)
+
+#define USBC_STATUS1_DET_ONGOING   BIT(6)
+#define USBC_STATUS1_RSLT(r)   ((r) & 0xf)
+#define USBC_RSLT_NOTHING  0
+#define USBC_RSLT_SRC_DEFAULT  1
+#define USBC_RSLT_SRC_1_5A 2
+#define USBC_RSLT_SRC_3_0A 3
+#define USBC_RSLT_SNK  4
+#define USBC_RSLT_DEBUG_ACC5
+#define USBC_RSLT_AUDIO_ACC6
+#define USBC_RSLT_UNDEF15
+#define USBC_STATUS1_ORIENT(r) (((r) >> 4) & 0x3)
+#define USBC_ORIENT_NORMAL 1
+#define USBC_ORIENT_REVERSE2
+
+#define USBC_STATUS2_VBUS_REQ  BIT(5)
+
+#define USBC_IRQ1_ADCDONE1 BIT(2)
+#define USBC_IRQ1_OVERTEMP BIT(1)
+#define USBC_IRQ1_SHORTBIT(0)
+
+#define USBC_IRQ2_CC_CHANGEBIT(7)
+#define USBC_IRQ2_RX_PDBIT(6)
+#define USBC_IRQ2_RX_HRBIT(5)
+#define USBC_IRQ2_RX_CRBIT(4)
+#define USBC_IRQ2_TX_SUCCESS   BIT(3)
+#define USBC_IRQ2_TX_FAIL  BIT(2)
+
+#define USBC_IRQMASK1_ALL  (USBC_IRQ1_ADCDONE1 | USBC_IRQ1_OVERTEMP | \
+USBC_IRQ1_SHORT)
+
+#define USBC_IRQMASK2_ALL  (USBC_IRQ2_CC_CHANGE | USBC_IRQ2_RX_PD | \
+USBC_IRQ2_RX_HR | USBC_IRQ2_RX_CR | \
+USBC_IRQ2_TX_SUCCESS | USBC_IRQ2_TX_FAIL)
+
+struct wcove_typec {
+   struct mutex lock; /* device lock */
+   struct device *dev;
+   struct regmap *regmap;
+   struct typec_port *port;
+   struct typec_capability cap;
+   struct typec_connection con;
+   struct typec_partner partner;
+};
+
+enum wcove_typec_func {
+   WCOVE_FUNC_DRIVE_VBUS = 1,
+   WCOVE_FUNC_ORIENTATION,
+   WCOVE_FUNC_ROLE,
+   WCOVE_FUNC_DRIVE_VCONN,
+};
+
+enum 

[PATCHv13 0/3] USB Type-C Connector class

[Heikki Krogerus]

The USB Type-C class is meant to provide unified interface to the
userspace to present the USB Type-C ports in a system.

Changes since v12:
- Added prefer_role member to typec_capability structure as requested by Guenter

Changes since v11:
- The port drivers are responsible of removing the alternate
  modes (just like the documentation already said).

Changes since v10:
- Using ATTRIBUTE_GROUPS and DEVICE_ATTR marcos everywhere
- Moved sysfs_match_string to lib/string.c
- Rationalized uevents
- Calling ida_destroy

Changes since v9:
- Minor typec_wcove.c cleanup as proposed by Guenter Roeck. No
  function affect.

Changes since v8:
- checking sysfs_streq() result correctly in sysfs_strmatch
- fixed accessory check in supported_accessory_mode
- using "none" as the only string that can clear the preferred role

Changes since v7:
- Removed "type" attribute from partners
- Added supports_usb_power_delivery attribute for partner and cable

Changes since v6:
- current_vconn_role attr renamed to vconn_source (no API changes)
- Small documentation improvements proposed by Vincent Palatin

Changes since v5:
- Only updating the roles based on driver notifications
- Added MODULE_ALIAS for the WhiskeyCove module
- Including the patch that creates the actual platform device for the
  WhiskeyCove Type-C PHY in this series.

Changes since v4:
- Remove the port lock completely

Changes since v3:
- Documentation cleanup as proposed by Roger Quadros
- Setting partner altmodes member to NULL on removal and fixing a
  warning, as proposed by Guenter Roeck
- Added the following attributes for partners and cables:
  * supports_usb_power_delivery
  * id_header_vdo
- "id_header_vdo" is visible only when the partner or cable supports
  USB Power Delivery communication.
- Partner attribute "accessory" is hidden when the partner type is not
  "Accessory".

Changes since v2:
- Notification on role and alternate mode changes
- cleanups

Changes since v1:
- Completely rewrote alternate mode support
- Patners, cables and cable plugs presented as devices.


Heikki Krogerus (3):
  lib/string: add sysfs_match_string helper
  usb: USB Type-C connector class
  usb: typec: add driver for Intel Whiskey Cove PMIC USB Type-C PHY

 Documentation/ABI/testing/sysfs-class-typec |  220 ++
 Documentation/usb/typec.txt |  110 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |   21 +
 drivers/usb/typec/Makefile  |2 +
 drivers/usb/typec/typec.c   | 1013 +++
 drivers/usb/typec/typec_wcove.c |  373 ++
 include/linux/string.h  |   10 +
 include/linux/usb/typec.h   |  254 +++
 lib/string.c|   26 +
 12 files changed, 2042 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 drivers/usb/typec/typec_wcove.c
 create mode 100644 include/linux/usb/typec.h

-- 
2.10.2

--
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: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 24.11.2016 13:03, Felipe Balbi wrote:


Hi,

Mathias Nyman  writes:

+   /* are any devices using this tt_info? */
+   for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {


off-by-one here ? Why is i starting from 1?


+   vdev = xhci->devs[i];


slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to
check it.


hmm... it's reserved for the HW, sure. Do you need to over allocate the
array by 1 just to keep this first member unused? Couldn't you handle
the +1/-1 (depending on the case) in xhci driver itself? Saves a bit of
memory there.



There are many things that needs fixing in this area, but not in this patch

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

RE: [PATCH] fsl/usb: Workarourd for USB erratum-A005697

[Sriram Dash]

>From: Changming Huang [mailto:jerry.hu...@nxp.com]
>As per USB specification, in the Suspend state, the status bit does not change 
>until
>the port is suspended. However, there may be a delay in suspending a port if 
>there
>is a transaction currently in progress on the bus.
>
>In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when the
>application sets it and not when the port is actually suspended
>
>Workaround for this issue involves waiting for a minimum of 10ms to allow the
>controller to go into SUSPEND state before proceeding ahead
>
>Signed-off-by: Changming Huang 
>Signed-off-by: Ramneek Mehresh 
>---
> drivers/usb/host/ehci-fsl.c  |3 +++
> drivers/usb/host/ehci-hub.c  |2 ++
> drivers/usb/host/ehci.h  |6 ++
> drivers/usb/host/fsl-mph-dr-of.c |2 ++
> include/linux/fsl_devices.h  |1 +
> 5 files changed, 14 insertions(+)
>
>diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index
>9f5ffb6..91701cc 100644
>--- a/drivers/usb/host/ehci-fsl.c
>+++ b/drivers/usb/host/ehci-fsl.c
>@@ -286,6 +286,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
>   if (pdata->has_fsl_erratum_a005275 == 1)
>   ehci->has_fsl_hs_errata = 1;
>
>+  if (pdata->has_fsl_erratum_a005697 == 1)
>+  ehci->has_fsl_susp_errata = 1;
>+
>   if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
>   (pdata->operating_mode == FSL_USB2_DR_OTG))
>   if (ehci_fsl_setup_phy(hcd, pdata->phy_mode, 0)) diff --git
>a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c index
>74f62d6..86d154e 100644
>--- a/drivers/usb/host/ehci-hub.c
>+++ b/drivers/usb/host/ehci-hub.c
>@@ -305,6 +305,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
>   USB_PORT_STAT_HIGH_SPEED)
>   fs_idle_delay = true;
>   ehci_writel(ehci, t2, reg);
>+  if (ehci_has_fsl_susp_errata(ehci))
>+  mdelay(10);

Hi Jerry,

Move the delay out of the spin lock. Other than that, it looks fine to me.

>   changed = 1;
>   }
>   }
>diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index
>3f3b74a..7706e4a 100644
>--- a/drivers/usb/host/ehci.h
>+++ b/drivers/usb/host/ehci.h
>@@ -219,6 +219,7 @@ struct ehci_hcd {  /* one per controller */
>   unsignedno_selective_suspend:1;
>   unsignedhas_fsl_port_bug:1; /* FreeScale */
>   unsignedhas_fsl_hs_errata:1;/* Freescale HS quirk */
>+  unsignedhas_fsl_susp_errata:1;  /*Freescale SUSP quirk*/
>   unsignedbig_endian_mmio:1;
>   unsignedbig_endian_desc:1;
>   unsignedbig_endian_capbase:1;
>@@ -703,10 +704,15 @@ struct ehci_tt {
> #if defined(CONFIG_PPC_85xx)
> /* Some Freescale processors have an erratum (USB A-005275) in which
>  * incoming packets get corrupted in HS mode
>+ * Some Freescale processors have an erratum (USB A-005697) in which
>+ * we need to wait for 10ms for bus to fo into suspend mode after
>+ * setting SUSP bit
>  */
> #define ehci_has_fsl_hs_errata(e) ((e)->has_fsl_hs_errata)
>+#define ehci_has_fsl_susp_errata(e)   ((e)->has_fsl_susp_errata)
> #else
> #define ehci_has_fsl_hs_errata(e) (0)
>+#define ehci_has_fsl_susp_errata(e)   (0)
> #endif
>
> /*
>diff --git a/drivers/usb/host/fsl-mph-dr-of.c 
>b/drivers/usb/host/fsl-mph-dr-of.c
>index f07ccb2..e90ddb5 100644
>--- a/drivers/usb/host/fsl-mph-dr-of.c
>+++ b/drivers/usb/host/fsl-mph-dr-of.c
>@@ -226,6 +226,8 @@ static int fsl_usb2_mph_dr_of_probe(struct
>platform_device *ofdev)
>   of_property_read_bool(np, "fsl,usb-erratum-a007792");
>   pdata->has_fsl_erratum_a005275 =
>   of_property_read_bool(np, "fsl,usb-erratum-a005275");
>+  pdata->has_fsl_erratum_a005697 =
>+  of_property_read_bool(np, "fsl,usb_erratum-a005697");
>
>   /*
>* Determine whether phy_clk_valid needs to be checked diff --git
>a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h index 
>f291291..60cef82
>100644
>--- a/include/linux/fsl_devices.h
>+++ b/include/linux/fsl_devices.h
>@@ -100,6 +100,7 @@ struct fsl_usb2_platform_data {
>   unsignedalready_suspended:1;
>   unsignedhas_fsl_erratum_a007792:1;
>   unsignedhas_fsl_erratum_a005275:1;
>+  unsignedhas_fsl_erratum_a005697:1;
>   unsignedcheck_phy_clk_valid:1;
>
>   /* register save area for suspend/resume */
>--
>1.7.9.5


Regards,
Sriram
--
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 v2] usb: xhci: Remove unuseful 'return' and 'break' statement

[Baolin Wang]

Since these 'return' statements are not generally useful in void
function, remove them. Also remove one unuseful 'break' statement
in xhci_setup_addressable_virt_dev() function.

Signed-off-by: Baolin Wang 
---
Changes since v1:
 - Add description of removing 'break' statement in commitlog.
---
 drivers/usb/host/xhci-hub.c  |2 --
 drivers/usb/host/xhci-mem.c  |1 -
 drivers/usb/host/xhci-ring.c |4 
 drivers/usb/host/xhci.c  |3 ---
 4 files changed, 10 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..470ad66 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -444,8 +444,6 @@ void xhci_ring_device(struct xhci_hcd *xhci, int slot_id)
xhci_ring_ep_doorbell(xhci, slot_id, i, 0);
}
}
-
-   return;
 }
 
 static void xhci_disable_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 48a26d378..d6f59a3 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1146,7 +1146,6 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd 
*xhci, struct usb_device *ud
case USB_SPEED_WIRELESS:
xhci_dbg(xhci, "FIXME xHCI doesn't support wireless speeds\n");
return -EINVAL;
-   break;
default:
/* Speed was set earlier, this shouldn't happen. */
return -EINVAL;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d415911..2057d08 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -157,7 +157,6 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring 
*ring)
ring->deq_seg = ring->deq_seg->next;
ring->dequeue = ring->deq_seg->trbs;
}
-   return;
 }
 
 /*
@@ -1167,7 +1166,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd 
*xhci, int slot_id,
ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
return;
}
-   return;
 }
 
 static void xhci_handle_cmd_reset_dev(struct xhci_hcd *xhci, int slot_id,
@@ -1258,7 +1256,6 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd 
*xhci,
mod_timer(>cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
xhci_ring_cmd_db(xhci);
}
-   return;
 }
 
 
@@ -1307,7 +1304,6 @@ void xhci_handle_command_timeout(unsigned long data)
xhci_dbg(xhci, "Command timeout on stopped ring\n");
xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
spin_unlock_irqrestore(>lock, flags);
-   return;
 }
 
 static void handle_cmd_completion(struct xhci_hcd *xhci,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index cf30cb6..dc337b3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -269,8 +269,6 @@ static void xhci_free_irq(struct xhci_hcd *xhci)
return;
if (pdev->irq > 0)
free_irq(pdev->irq, xhci_to_hcd(xhci));
-
-   return;
 }
 
 /*
@@ -351,7 +349,6 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci)
}
 
hcd->msix_enabled = 0;
-   return;
 }
 
 static void __maybe_unused xhci_msix_sync_irqs(struct xhci_hcd *xhci)
-- 
1.7.9.5

--
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: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
>>> +   /* are any devices using this tt_info? */
>>> +   for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {
>>
>> off-by-one here ? Why is i starting from 1?
>>
>>> +   vdev = xhci->devs[i];
>
> slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to
> check it.

hmm... it's reserved for the HW, sure. Do you need to over allocate the
array by 1 just to keep this first member unused? Couldn't you handle
the +1/-1 (depending on the case) in xhci driver itself? Saves a bit of
memory there.

> All other places that check xhci->devs[0] are avtually buggy

fair enough, sounds like an accessor guaranteeing the 'rules of
engagement' for this would be useful.

-- 
balbi


signature.asc
Description: PGP signature

[PATCHv2] USB: ohci: da8xx: Resume the entire host controller

[Axel Haslam]

The da8xx ohci controller is not working after suspend and resume.

This is because only the root hub is being resumed.

Balance the ohci_suspend of the suspend path with an ohci_resume
in the resume path so that we resume the entire controller, and not
just the root hub.

Also, while we are here, remove setting device power_state,
as this is no longer needed and scheduled for removal

Acked-by: Alan Stern 
Signed-off-by: Axel Haslam 
---
Changes v1->v2
* reword commit message (Alan Stern)

 drivers/usb/host/ohci-da8xx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 9e336f4..05da2cb 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -528,8 +528,7 @@ static int ohci_da8xx_resume(struct platform_device *dev)
if (ret)
return ret;
 
-   dev->dev.power.power_state = PMSG_ON;
-   usb_hcd_resume_root_hub(hcd);
+   ohci_resume(hcd, false);
 
return 0;
 }
-- 
2.9.3

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

Re: [PATCH] usb: xhci: Remove unuseful 'return' statement

[Baolin Wang]

Hi,

On 24 November 2016 at 17:30, Sergei Shtylyov
 wrote:
> Hello.
>
> On 11/24/2016 11:13 AM, Baolin Wang wrote:
>
>> Since these 'return' statements are not generally useful in void function,
>> remove them.
>>
>> Signed-off-by: Baolin Wang 
>
> [...]
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 48a26d378..d6f59a3 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1146,7 +1146,6 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd
>> *xhci, struct usb_device *ud
>> case USB_SPEED_WIRELESS:
>> xhci_dbg(xhci, "FIXME xHCI doesn't support wireless
>> speeds\n");
>> return -EINVAL;
>> -   break;
>
>
>Here you're removing *break* instead but not mentioning this in the patch
> description...

OK. I will add description to explain it in new patch. Thanks.

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

Re: [PATCH 2/3 v3] xhci: Fix race related to abort operation

[OGAWA Hirofumi]

Vincent Pelletier  writes:

> On Wed, 23 Nov 2016 22:48:35 +0900, OGAWA Hirofumi
>  wrote:
>> ping?
>
> FWIW, I intend to run this patch on the hardware which caused the
> issue (thanks Mathias for the CC !).
>
> So far, in the very short attempt I made, I failed to even build the
> kernel (some stack protection feature error when being probed in
> gcc...). I should have time to try again this week-end, but please do
> not wait for me.

If you need my help for something, let me know either publicly or
privately what way you want.

Well, I can't see why he is waiting something. My patchset is fix to
race, and your stuff is to remove unnecessary code, i.e. patchset has no
dependency actually.

Thanks.
-- 
OGAWA Hirofumi 
--
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: [PATCHv12 2/3] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 23, 2016 at 09:12:04PM -0800, Guenter Roeck wrote:
> Hello Heikki,
> 
> On 11/22/2016 06:11 AM, Heikki Krogerus wrote:
> [ ... ]
> > +
> > +struct typec_port *typec_register_port(struct device *dev,
> > +  const struct typec_capability *cap)
> > +{
> > +   struct typec_port *port;
> > +   int ret;
> > +   int id;
> > +
> > +   port = kzalloc(sizeof(*port), GFP_KERNEL);
> > +   if (!port)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   id = ida_simple_get(_index_ida, 0, 0, GFP_KERNEL);
> > +   if (id < 0) {
> > +   kfree(port);
> > +   return ERR_PTR(id);
> > +   }
> > +
> > +   port->prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > +
> 
> Following up on this:
> 
> In our implementation, the default preferred role is determined by the
> low level driver (as, in my understanding, is suggested by the standard).
> This means that the ABI will report "no preferred role", unless user space
> overwrites it, even though there _is_ in fact a preferred role, and the
> low level driver will execute try.src or try.snk based on that role.

I'm not sure which standard are you referring? Try.SNK and Try.SRC are
optional mechanisms for *policy-based* role preference according to
the USB Type-C spec. The policy really should always come from the
user space in our case, but I don't think that rules out for example
initial role preferences coming from the lower level drivers.

We will need a way the OS can set the initial preference for every
port. Note that once we can support that, what ever the lower level
drivers request will be overridden by it. So if for example the
platform has preference for an initial role, we will simply ignore it
if the policy says otherwise.

> It might make sense to add the preferred role to struct typec_capability
> and get the initial value from there. If a low level driver does not want
> to specify it, it can easily set its value to TYPEC_NO_PREFERRED_ROLE.

Well, ideally the port drivers would not need to do anything if there
is no preference, but I don't think it's a problem. Since this is API,
I guess we can even change this later if we come up with a better way
of doing this. I'll add it.


Thanks,

-- 
heikki
--
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: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Mathias Nyman]

On 24.11.2016 11:02, Felipe Balbi wrote:


Hi,

Mathias Nyman  writes:

the tt_info provided by a HS hub might be in use to by a child device
Make sure we free the devices in the correct order.

This is needed in special cases such as when xhci controller is
reset when resuming from hibernate, and all virt_devices are freed.

Also free the virt_devices starting from max slot_id as children
more commonly have higher slot_id than parent.

CC: 
Signed-off-by: Mathias Nyman 

---

Guenter Roeck, does this work for you?

A rework of how tt_info is stored and used might be needed,
but that will take some time and won't go to stable as easily.
---
  drivers/usb/host/xhci-mem.c | 38 --
  1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 6afe323..b3a5cd8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int 
slot_id)
xhci->devs[slot_id] = NULL;
  }

+/*
+ * Free a virt_device structure.
+ * If the virt_device added a tt_info (a hub) and has children pointing to
+ * that tt_info, then free the child first. Recursive.
+ * We can't rely on udev at this point to find child-parent relationships.
+ */
+void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
+{
+   struct xhci_virt_device *vdev;
+   struct list_head *tt_list_head;
+   struct xhci_tt_bw_info *tt_info, *next;
+   int i;
+
+   vdev = xhci->devs[slot_id];
+   if (!vdev)
+   return;
+
+   tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
+   list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
+   /* is this a hub device that added a tt_info to the tts list */
+   if (tt_info->slot_id == slot_id) {


if (tt_info->slot_id != slot_id)
continue;


+   /* are any devices using this tt_info? */
+   for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {


off-by-one here ? Why is i starting from 1?


+   vdev = xhci->devs[i];


slit_id 0 is reserved and xhci->devs[0] is not used, so ne need to check it.

All other places that check xhci->devs[0] are avtually buggy

-Mathias  


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

[PATCH] fsl/usb: Workarourd for USB erratum-A005697

[Changming Huang]

As per USB specification, in the Suspend state, the status bit does
not change until the port is suspended. However, there may be a delay
in suspending a port if there is a transaction currently in progress
on the bus.

In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
the application sets it and not when the port is actually suspended

Workaround for this issue involves waiting for a minimum of 10ms to
allow the controller to go into SUSPEND state before proceeding ahead

Signed-off-by: Changming Huang 
Signed-off-by: Ramneek Mehresh 
---
 drivers/usb/host/ehci-fsl.c  |3 +++
 drivers/usb/host/ehci-hub.c  |2 ++
 drivers/usb/host/ehci.h  |6 ++
 drivers/usb/host/fsl-mph-dr-of.c |2 ++
 include/linux/fsl_devices.h  |1 +
 5 files changed, 14 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 9f5ffb6..91701cc 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -286,6 +286,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
if (pdata->has_fsl_erratum_a005275 == 1)
ehci->has_fsl_hs_errata = 1;
 
+   if (pdata->has_fsl_erratum_a005697 == 1)
+   ehci->has_fsl_susp_errata = 1;
+
if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
(pdata->operating_mode == FSL_USB2_DR_OTG))
if (ehci_fsl_setup_phy(hcd, pdata->phy_mode, 0))
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 74f62d6..86d154e 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -305,6 +305,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
USB_PORT_STAT_HIGH_SPEED)
fs_idle_delay = true;
ehci_writel(ehci, t2, reg);
+   if (ehci_has_fsl_susp_errata(ehci))
+   mdelay(10);
changed = 1;
}
}
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 3f3b74a..7706e4a 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -219,6 +219,7 @@ struct ehci_hcd {   /* one per controller */
unsignedno_selective_suspend:1;
unsignedhas_fsl_port_bug:1; /* FreeScale */
unsignedhas_fsl_hs_errata:1;/* Freescale HS quirk */
+   unsignedhas_fsl_susp_errata:1;  /*Freescale SUSP quirk*/
unsignedbig_endian_mmio:1;
unsignedbig_endian_desc:1;
unsignedbig_endian_capbase:1;
@@ -703,10 +704,15 @@ struct ehci_tt {
 #if defined(CONFIG_PPC_85xx)
 /* Some Freescale processors have an erratum (USB A-005275) in which
  * incoming packets get corrupted in HS mode
+ * Some Freescale processors have an erratum (USB A-005697) in which
+ * we need to wait for 10ms for bus to fo into suspend mode after
+ * setting SUSP bit
  */
 #define ehci_has_fsl_hs_errata(e)  ((e)->has_fsl_hs_errata)
+#define ehci_has_fsl_susp_errata(e)((e)->has_fsl_susp_errata)
 #else
 #define ehci_has_fsl_hs_errata(e)  (0)
+#define ehci_has_fsl_susp_errata(e)(0)
 #endif
 
 /*
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index f07ccb2..e90ddb5 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -226,6 +226,8 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device 
*ofdev)
of_property_read_bool(np, "fsl,usb-erratum-a007792");
pdata->has_fsl_erratum_a005275 =
of_property_read_bool(np, "fsl,usb-erratum-a005275");
+   pdata->has_fsl_erratum_a005697 =
+   of_property_read_bool(np, "fsl,usb_erratum-a005697");
 
/*
 * Determine whether phy_clk_valid needs to be checked
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index f291291..60cef82 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -100,6 +100,7 @@ struct fsl_usb2_platform_data {
unsignedalready_suspended:1;
unsignedhas_fsl_erratum_a007792:1;
unsignedhas_fsl_erratum_a005275:1;
+   unsignedhas_fsl_erratum_a005697:1;
unsignedcheck_phy_clk_valid:1;
 
/* register save area for suspend/resume */
-- 
1.7.9.5

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

Re: [PATCH] usb: xhci: Remove unuseful 'return' statement

[Sergei Shtylyov]

Hello.

On 11/24/2016 11:13 AM, Baolin Wang wrote:


Since these 'return' statements are not generally useful in void function,
remove them.

Signed-off-by: Baolin Wang 

[...]

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 48a26d378..d6f59a3 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1146,7 +1146,6 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd 
*xhci, struct usb_device *ud
case USB_SPEED_WIRELESS:
xhci_dbg(xhci, "FIXME xHCI doesn't support wireless speeds\n");
return -EINVAL;
-   break;


   Here you're removing *break* instead but not mentioning this in the patch 
description...


[...]

MBR, Sergei

--
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: [RFT PATCH 1/1] xhci: free xhci virtual devices with leaf nodes first

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
> the tt_info provided by a HS hub might be in use to by a child device
> Make sure we free the devices in the correct order.
>
> This is needed in special cases such as when xhci controller is
> reset when resuming from hibernate, and all virt_devices are freed.
>
> Also free the virt_devices starting from max slot_id as children
> more commonly have higher slot_id than parent.
>
> CC: 
> Signed-off-by: Mathias Nyman 
>
> ---
>
> Guenter Roeck, does this work for you?
>
> A rework of how tt_info is stored and used might be needed,
> but that will take some time and won't go to stable as easily.
> ---
>  drivers/usb/host/xhci-mem.c | 38 --
>  1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 6afe323..b3a5cd8 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -979,6 +979,40 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, int 
> slot_id)
>   xhci->devs[slot_id] = NULL;
>  }
>  
> +/*
> + * Free a virt_device structure.
> + * If the virt_device added a tt_info (a hub) and has children pointing to
> + * that tt_info, then free the child first. Recursive.
> + * We can't rely on udev at this point to find child-parent relationships.
> + */
> +void xhci_free_virt_devices_depth_first(struct xhci_hcd *xhci, int slot_id)
> +{
> + struct xhci_virt_device *vdev;
> + struct list_head *tt_list_head;
> + struct xhci_tt_bw_info *tt_info, *next;
> + int i;
> +
> + vdev = xhci->devs[slot_id];
> + if (!vdev)
> + return;
> +
> + tt_list_head = &(xhci->rh_bw[vdev->real_port - 1].tts);
> + list_for_each_entry_safe(tt_info, next, tt_list_head, tt_list) {
> + /* is this a hub device that added a tt_info to the tts list */
> + if (tt_info->slot_id == slot_id) {

if (tt_info->slot_id != slot_id)
continue;

> + /* are any devices using this tt_info? */
> + for (i = 1; i < HCS_MAX_SLOTS(xhci->hcs_params1); i++) {

off-by-one here ? Why is i starting from 1?

> + vdev = xhci->devs[i];
> + if (vdev && (vdev->tt_info == tt_info))

if (!vdev || vdev->tt_info != tt_info)
continue;

> + xhci_free_virt_devices_depth_first(
> + xhci, i);
> + }
> + }
> + }
> + /* we are now at a leaf device */
> + xhci_free_virt_device(xhci, slot_id);
> +}
> +
>  int xhci_alloc_virt_device(struct xhci_hcd *xhci, int slot_id,
>   struct usb_device *udev, gfp_t flags)
>  {
> @@ -1829,8 +1863,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>   }
>   }
>  
> - for (i = 1; i < MAX_HC_SLOTS; ++i)
> - xhci_free_virt_device(xhci, i);
> + for (i = HCS_MAX_SLOTS(xhci->hcs_params1); i > 0; i--)

converting MAX_HC_SLOTS to HCS_MAX_SLOTS(xhci->hcs_params1) seems
unrelated to $subject. Perhaps just mention on commit log?

-- 
balbi


signature.asc
Description: PGP signature

Re: Issue with Telit LE922 and cdc_mbim

[Daniele Palmas]

2016-11-23 19:38 GMT+01:00 Bjørn Mork :
> Daniele Palmas  writes:
>> 2016-11-23 15:49 GMT+01:00 Bjørn Mork :
>>> Daniele Palmas  writes:
 2016-11-21 10:49 GMT+01:00 Bjørn Mork :
> Daniele Palmas  writes:
>
>> it turned out that resetting the interface in cdc_ncm_init after
>> getting the NTB parameters removes the need for the sleep, making the
>> modem to work fine.
>
> Sounds very good, although I must admit that it isn't perfectly clear to
> me what kind of reset we're talking about here.  But no worries, the
> patch will make that clear :)
>

 Sorry for the confusion, I meant resetting the MBIM function with
 RESET_FUNCTION request code.
>>>
>>> Yes, the patch did make that clear, as expected :)
>>>
>> I wonder if this is an acceptable solution and can be applied also for 
>> MC7455.
>
> Quite possible.  I will definitely test it.  If we can avoid an
> arbitrary and pointless delay, then that's great.  But I guess this also
> requires testing with a wide range of other MBIM devices to find out if
> we can apply it unconditionally without breaking anything. Avoiding
> device-specific or vendor-specific code is important in a class driver,
> if possible.
>

 Ok, unfortunately I can test only with Telit modems, so I'm not sure
 if the change I did is harmless for all the other modems:
 RESET_FUNCTION should not cause issues,
>>>
>>> Agreed.  Unfortunately, we cannot depend on sane firmware behaviour ;)
>>>
>>> I did a semi-quick test on a Sierra Wireless EM7455 and can confirm that
>>> your patch makes it work without the delay.  Very nice.
>>>
>>
>> Cool!
>>
>>> Do you happen to know if the Windows MBIM driver use  RESET_FUNCTION
>>> during initialisation?  That would make this feel much safer.
>>>
>>
>> Yes, Windows driver seems to send RESET_FUNCTION two times: after
>> getting NTB parameters and after setting NTB input size. But it seems
>> that only the first one is mandatory.
>>
>> Windows USB traces taken with TotalPhase Datacenter are available at
>>
>> https://drive.google.com/drive/folders/0B1kPnH2g8ISIZWJiV05qeWN5dVE
>>
>> file LE922win10_2.tdc
>
> This is comforting. It means that the risk introducing the
> RESET_FUNCTION call is close to zero.  The only remaining issue is
> whether we can safely remove the altsetting toggle.
>
>>> I see that your testing also included Intel based modems. That's good.
>>> It would still be nice to have test results from a few more MBIM
>>> implementations, in particular the more "difficult" ones. But I don't
>>
>> I agree, especially Huawei ones for which the alternate setting
>> toggling was introduced.
>
> I'll see what I can do about that. Looks like I have to dig through my
> mailbox.  It seems I didn't add any Reported-by tags on the relevant
> commits.  Wonder why...  Well, there you have the reason why you always
> should.  Now I got to do the work instead of just loading it on you :)
>
>>> know how much help I can promise here. At the moment I don't even know
>>> which box my modems are in.  Moved a month ago and am still living in a
>>> box.  Or more like a hundred boxes ;)
>>>
>>> The easiest way to get this thoroughly tested is of course to push it
>>> now, and try to respond quickly in case it breaks something. Don't know
>>> what others think about that?
>>>
 but I also removed altsetting
 toggling for all MBIM modems and maybe this is not acceptable.
>>>
>>> I wonder about that.  It was added specifically for modems which seemed
>>> to need it.  Don't remember if we ever tried RESET_FUNCTION as an
>>> alternative. Removing workaorunds which have proven to be necessary at
>>> some point in time is a bit risky.
>>>
>>
>> I totally agree, but the other way would be to add another quirk at
>> the cdc_mbim driver level, isn't it?
>
> Yes, so I'm definitely with you here - let's try without the quirk
> first.  I'm ready to say "commit it", if noone else objects.
>
>> If this is a preferred solution I can try to modify the patch
>> according to this path.
>
> No, the patch is fine as is.  Just one question: If keeping the
> altsetting toggle proves necessary, will that break the Qualcomm
> devices?  Yes, I could test that myself, but I'm lazy and I guess you
> already have tested it?

Unfortunately, yes: for Telit LE922A6 altsetting toggle seems to be
part of the problem.

Regards,
Daniele

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

[PATCH] usb: xhci: Remove unuseful 'return' statement

[Baolin Wang]

Since these 'return' statements are not generally useful in void function,
remove them.

Signed-off-by: Baolin Wang 
---
 drivers/usb/host/xhci-hub.c  |2 --
 drivers/usb/host/xhci-mem.c  |1 -
 drivers/usb/host/xhci-ring.c |4 
 drivers/usb/host/xhci.c  |3 ---
 4 files changed, 10 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..470ad66 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -444,8 +444,6 @@ void xhci_ring_device(struct xhci_hcd *xhci, int slot_id)
xhci_ring_ep_doorbell(xhci, slot_id, i, 0);
}
}
-
-   return;
 }
 
 static void xhci_disable_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 48a26d378..d6f59a3 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1146,7 +1146,6 @@ int xhci_setup_addressable_virt_dev(struct xhci_hcd 
*xhci, struct usb_device *ud
case USB_SPEED_WIRELESS:
xhci_dbg(xhci, "FIXME xHCI doesn't support wireless speeds\n");
return -EINVAL;
-   break;
default:
/* Speed was set earlier, this shouldn't happen. */
return -EINVAL;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d415911..2057d08 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -157,7 +157,6 @@ static void inc_deq(struct xhci_hcd *xhci, struct xhci_ring 
*ring)
ring->deq_seg = ring->deq_seg->next;
ring->dequeue = ring->deq_seg->trbs;
}
-   return;
 }
 
 /*
@@ -1167,7 +1166,6 @@ static void xhci_handle_cmd_config_ep(struct xhci_hcd 
*xhci, int slot_id,
ring_doorbell_for_active_rings(xhci, slot_id, ep_index);
return;
}
-   return;
 }
 
 static void xhci_handle_cmd_reset_dev(struct xhci_hcd *xhci, int slot_id,
@@ -1258,7 +1256,6 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd 
*xhci,
mod_timer(>cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
xhci_ring_cmd_db(xhci);
}
-   return;
 }
 
 
@@ -1307,7 +1304,6 @@ void xhci_handle_command_timeout(unsigned long data)
xhci_dbg(xhci, "Command timeout on stopped ring\n");
xhci_handle_stopped_cmd_ring(xhci, xhci->current_cmd);
spin_unlock_irqrestore(>lock, flags);
-   return;
 }
 
 static void handle_cmd_completion(struct xhci_hcd *xhci,
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index cf30cb6..dc337b3 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -269,8 +269,6 @@ static void xhci_free_irq(struct xhci_hcd *xhci)
return;
if (pdev->irq > 0)
free_irq(pdev->irq, xhci_to_hcd(xhci));
-
-   return;
 }
 
 /*
@@ -351,7 +349,6 @@ static void xhci_cleanup_msix(struct xhci_hcd *xhci)
}
 
hcd->msix_enabled = 0;
-   return;
 }
 
 static void __maybe_unused xhci_msix_sync_irqs(struct xhci_hcd *xhci)
-- 
1.7.9.5

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