RE: [PATCH v1 Resend] usb: dwc2: gadget: fix a memory use-after-free bug
> -Original Message- > From: Felipe Balbi [mailto:ba...@ti.com] > Sent: Thursday, October 1, 2015 7:21 PM > To: Kaukab, Yousaf > Cc: ba...@ti.com; linux-usb@vger.kernel.org; john.y...@synopsys.com; > l...@rock-chips.com; he...@sntech.de; c...@rock-chips.com; > h...@rock-chips.com; > y...@rock-chips.com; gaura...@google.com; albe...@google.com; wulf@rock- > chips.com; jwer...@chromium.org; jeffy.c...@rock-chips.com; Herrero, > Gregory; huang...@rock-chips.com; rockchip-disc...@chromium.org; > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org > Subject: Re: [PATCH v1 Resend] usb: dwc2: gadget: fix a memory use-after-free > bug > > On Thu, Oct 01, 2015 at 12:01:48PM +, Kaukab, Yousaf wrote: > > > From: Mian Yousaf Kaukab > > > Date: Tue, Sep 29, 2015 at 12:25 PM > > > Subject: [PATCH v1 Resend] usb: dwc2: gadget: fix a memory > > > use-after-free bug > > > To: linux-usb@vger.kernel.org, ba...@ti.com, john.y...@synopsys.com, > > > l...@rock-chips.com > > > Cc: he...@sntech.de, c...@rock-chips.com, h...@rock-chips.com, yk@rock- > > > chips.com, gaura...@google.com, albe...@google.com, > > > w...@rock-chips.com, jwer...@chromium.org, > > > jeffy.c...@rock-chips.com, gregory.herr...@intel.com, > > > huang...@rock-chips.com, rockchip- disc...@chromium.org, > > > gre...@linuxfoundation.org, linux- ker...@vger.kernel.org > > > > > > > > > From: Yunzhi Li > > > > > > When dwc2_hsotg_handle_unaligned_buf_complete() hs_req->req.buf > > > already destroyed, in dwc2_hsotg_unmap_dma(), it touches > > > hs_req->req.dma again, so > > > dwc2_hsotg_unmap_dma() should be called before > > > dwc2_hsotg_handle_unaligned_buf_complete(). Otherwise, it will cause > > > a bad_page BUG, when allocate this memory page next time. > > > > > > This bug led to the following crash: > > > > > > BUG: Bad page state in process swapper/0 pfn:2bdbc > > > [ 26.820440] page:eed76780 count:0 mapcount:0 mapping: (null) > index:0x0 > > > [ 26.854710] page flags: 0x200(arch_1) > > > [ 26.885836] page dumped because: PAGE_FLAGS_CHECK_AT_PREP flag > set > > > [ 26.919179] bad because of flags: > > > [ 26.948917] page flags: 0x200(arch_1) > > > [ 26.979100] Modules linked in: > > > [ 27.008401] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W3.14.0 #17 > > > [ 27.041816] [] (unwind_backtrace) from [] > > > (show_stack+0x20/0x24) > > > [ 27.076108] [] (show_stack) from [] > > > (dump_stack+0x70/0x8c) > > > [ 27.110246] [] (dump_stack) from [] > > > (bad_page+0xfc/0x12c) > > > [ 27.143958] [] (bad_page) from [] > > > (get_page_from_freelist+0x3e4/0x50c) > > > [ 27.179298] [] (get_page_from_freelist) from [] > > > (__alloc_pages_nodemask) > > > [ 27.216296] [] (__alloc_pages_nodemask) from [] > > > (__get_free_pages+0x20/) > > > [ 27.252326] [] (__get_free_pages) from [] > > > (kmalloc_order_trace+0x34/0xa) > > > [ 27.288295] [] (kmalloc_order_trace) from [] > > > (__kmalloc+0x40/0x1ac) > > > [ 27.323751] [] (__kmalloc) from [] > > > (dwc2_hsotg_ep_queue.isra.12+0x7c/0x1) > > > [ 27.359937] [] (dwc2_hsotg_ep_queue.isra.12) from > > > [] (dwc2_hsotg_ep_queue) > > > [ 27.397478] [] (dwc2_hsotg_ep_queue_lock) from > > > [] (rx_submit+0xfc/0x164) > > > [ 27.433619] [] (rx_submit) from [] > > > (rx_complete+0x22c/0x230) > > > [ 27.468872] [] (rx_complete) from [] > > > (dwc2_hsotg_complete_request+0xfc/0) > > > [ 27.506240] [] (dwc2_hsotg_complete_request) from > > > [] (dwc2_hsotg_handle_o) > > > [ 27.545401] [] (dwc2_hsotg_handle_outdone) from > > > [] (dwc2_hsotg_epint+0x2c) > > > [ 27.583689] [] (dwc2_hsotg_epint) from [] > > > (dwc2_hsotg_irq+0x1dc/0x4ac) > > > [ 27.621041] [] (dwc2_hsotg_irq) from [] > > > (handle_irq_event_percpu+0x70/0x) > > > [ 27.659066] [] (handle_irq_event_percpu) from > > > [] (handle_irq_event+0x4c) > > > [ 27.697322] [] (handle_irq_event) from [] > > > (handle_fasteoi_irq+0xc8/0x11) > > > [ 27.735451] [] (handle_fasteoi_irq) from [] > > > (generic_handle_irq+0x30/0x) > > > [ 27.773918] [] (generic_handle_irq) from [] > > > (__handle_domain_irq+0x84/0) > > > [ 27.812018] [] (__handle_domain_irq) from [] > > > (gic_handle_irq+0x48/0x6c) > > > [ 27.849695] [] (gic_handle_irq) from [] > > > (__irq_svc+0x40/0x50) > > > [ 27.886907] Exception stack(0xc0d01ee0 to 0xc0d01f28) > > > > > > Acked-by: John Youn > > > Tested-by: Heiko Stuebner > > > Tested-by: Jeffy Chen > > > Signed-off-by: Yunzhi Li > > > > Hi Felipe, > > This patch has been hanging around for a while now. Can you please apply > this? > > https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/nex > t&id=1c2e3377a933af9102f6c57c414c378a52d4e70d Thanks! > > -- > balbi BR, Yousaf -- 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 v8 4/4] USB / PM: Allow USB devices to remain runtime-suspended when sleeping
Have dev_pm_ops.prepare return 1 for USB devices and ports so that USB
devices can remain runtime-suspended when the system goes to a sleep
state, if their wakeup state is correct and they have runtime PM enabled.
Signed-off-by: Tomeu Vizoso
---
drivers/usb/core/port.c | 6 ++
drivers/usb/core/usb.c | 11 ++-
2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 210618319f10..f49707d73b5a 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -168,12 +168,18 @@ static int usb_port_runtime_suspend(struct device *dev)
return retval;
}
+
+static int usb_port_prepare(struct device *dev)
+{
+ return 1;
+}
#endif
static const struct dev_pm_ops usb_port_pm_ops = {
#ifdef CONFIG_PM
.runtime_suspend = usb_port_runtime_suspend,
.runtime_resume = usb_port_runtime_resume,
+ .prepare = usb_port_prepare,
#endif
};
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 8d5b2f4113cd..cf4dde11db1c 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -316,7 +316,16 @@ static int usb_dev_uevent(struct device *dev, struct
kobj_uevent_env *env)
static int usb_dev_prepare(struct device *dev)
{
- return 0; /* Implement eventually? */
+ struct usb_device *udev = to_usb_device(dev);
+
+ if (!pm_runtime_enabled(dev))
+ return 0;
+
+ /* Return 0 if the current wakeup setting is wrong, otherwise 1 */
+ if (udev->do_remote_wakeup != device_may_wakeup(dev))
+ return 0;
+
+ return 1;
}
static void usb_dev_complete(struct device *dev)
--
2.4.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
[PATCH v8 0/4] Allow USB devices to remain runtime-suspended when sleeping
Hi, this is v8 of an attempt to make it easier for devices to remain in runtime PM when the system goes to sleep, mainly to reduce the time spent resuming devices. For this, we interpret the absence of all PM callback implementations as it being safe to do direct_complete, so their ancestors aren't prevented from remaining runtime-suspended. Additionally, the prepare() callback of USB devices will return 1 if runtime PM is enabled and the current wakeup settings are correct. With these changes, a uvcvideo device (for example) stays in runtime suspend when the system goes to sleep and is left in that state when the system resumes, not delaying it unnecessarily. Thanks, Tomeu Changes in v8: - Add device_is_bound() - Add dev_pm_domain_set() and update code to use it - Move no_pm_callbacks field into CONFIG_PM_SLEEP - Call device_check_pm_callbacks only after a device is bound or unbound Changes in v7: - Reduce indentation by adding a label in device_prepare() Changes in v6: - Add stub for !CONFIG_PM. - Move implementation of device_check_pm_callbacks to power/main.c as it doesn't belong to CONFIG_PM_SLEEP. - Take dev->power.lock before modifying flag. Changes in v5: - Check for all dev_pm_ops instances associated to a device, updating a no_pm_callbacks flag at the times when that could change. Tomeu Vizoso (4): device core: add device_is_bound() PM / Domains: add setter for dev.pm_domain PM / sleep: Go direct_complete if driver has no callbacks USB / PM: Allow USB devices to remain runtime-suspended when sleeping arch/arm/mach-omap2/omap_device.c | 7 --- drivers/acpi/acpi_lpss.c | 5 +++-- drivers/acpi/device_pm.c | 5 +++-- drivers/base/dd.c | 12 ++-- drivers/base/power/clock_ops.c| 5 +++-- drivers/base/power/common.c | 8 drivers/base/power/domain.c | 6 -- drivers/base/power/main.c | 33 + drivers/base/power/power.h| 6 ++ drivers/gpu/vga/vga_switcheroo.c | 10 +- drivers/misc/mei/pci-me.c | 5 +++-- drivers/misc/mei/pci-txe.c| 5 +++-- drivers/usb/core/port.c | 6 ++ drivers/usb/core/usb.c| 11 ++- include/linux/device.h| 2 ++ include/linux/pm.h| 1 + include/linux/pm_domain.h | 3 +++ 17 files changed, 107 insertions(+), 23 deletions(-) -- 2.4.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
[PATCH v4 4/4] usb: dwc2: refactor common low-level hw code to platform.c
DWC2 module on some platforms needs three additional hardware
resources: phy controller, clock and power supply. All of them must be
enabled/activated to properly initialize and operate. This was initially
handled in s3c-hsotg driver, which has been converted to 'gadget' part
of dwc2 driver. Unfortunately, not all of this code got moved to common
platform code, what resulted in accessing DWC2 registers without
enabling low-level hardware resources. This fails for example on Exynos
SoCs. This patch moves all the code for managing those resources to
common platform.c file and provides convenient wrappers for controlling
them.
Signed-off-by: Marek Szyprowski
---
Changelog:
v4:
- fixed broken conditional compilation and adjusted comments in dwc2_hsotg
structure documentation
v3:
- rebased onto latest 'testing/next' from Felipe Balbi (includes
s3c_hsotg -> dwc2 rename)
v2:
- moved setting of ll_hw_enabled flag to enable/disable functions,
as suggested by John Youn
- moved setting of phy width to dwc2_lowlevel_init function
---
drivers/usb/dwc2/core.h | 24 +++--
drivers/usb/dwc2/gadget.c | 193
drivers/usb/dwc2/platform.c | 234 +---
3 files changed, 228 insertions(+), 223 deletions(-)
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 3056981..3db5ef2 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -578,6 +578,15 @@ struct dwc2_hregs_backup {
* - USB_DR_MODE_PERIPHERAL
* - USB_DR_MODE_HOST
* - USB_DR_MODE_OTG
+ * @hcd_enabledHost mode sub-driver initialization indicator.
+ * @gadget_enabled Peripheral mode sub-driver initialization indicator.
+ * l@l_hw_enabled Status of low-level hardware resources.
+ * @phy:The otg phy transceiver structure for phy control.
+ * @uphy: The otg phy transceiver structure for old USB phy
control.
+ * @plat: The platform specific configuration data. This can be
removed once
+ * all SoCs support usb transceiver.
+ * @supplies: Definition of USB power supplies
+ * @phyif: PHY interface width
* @lock: Spinlock that protects all the driver data structures
* @priv: Stores a pointer to the struct usb_hcd
* @queuing_high_bandwidth: True if multiple packets of a high-bandwidth
@@ -670,12 +679,6 @@ struct dwc2_hregs_backup {
* These are for peripheral mode:
*
* @driver: USB gadget driver
- * @phy:The otg phy transceiver structure for phy control.
- * @uphy: The otg phy transceiver structure for old USB phy
control.
- * @plat: The platform specific configuration data. This can be
removed once
- * all SoCs support usb transceiver.
- * @supplies: Definition of USB power supplies
- * @phyif: PHY interface width
* @dedicated_fifos:Set if the hardware has dedicated IN-EP fifos.
* @num_of_eps: Number of available EPs (excluding EP0)
* @debug_root: Root directrory for debugfs.
@@ -705,10 +708,13 @@ struct dwc2_hsotg {
enum usb_dr_mode dr_mode;
unsigned int hcd_enabled:1;
unsigned int gadget_enabled:1;
+ unsigned int ll_hw_enabled:1;
struct phy *phy;
struct usb_phy *uphy;
+ struct dwc2_hsotg_plat *plat;
struct regulator_bulk_data
supplies[ARRAY_SIZE(dwc2_hsotg_supply_names)];
+ u32 phyif;
spinlock_t lock;
struct mutex init_mutex;
@@ -812,9 +818,6 @@ struct dwc2_hsotg {
#if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) ||
IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
/* Gadget structures */
struct usb_gadget_driver *driver;
- struct dwc2_hsotg_plat *plat;
-
- u32 phyif;
int fifo_mem;
unsigned int dedicated_fifos:1;
unsigned char num_of_eps;
@@ -1103,7 +1106,8 @@ extern void dwc2_set_all_params(struct dwc2_core_params
*params, int value);
extern int dwc2_get_hwparams(struct dwc2_hsotg *hsotg);
-
+extern int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg);
+extern int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg);
/*
* Dump core registers and SPRAM
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 19202c1c..c283c9d 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -25,15 +25,11 @@
#include
#include
#include
-#include
-#include
#include
-#include
#include
#include
#include
-#include
#include "core.h"
#include "hw.h"
@@ -3022,50 +3018,6 @@ static struct usb_ep_ops dwc2_hsotg_ep_ops = {
};
/**
- * dwc2_hsotg_phy_enable - enable platform phy dev
- * @hsotg: The driver state
- *
- * A wrapper for platform code responsible for controlling
- * low-level USB code
- */
-static void dwc2_hsotg_phy_enable(struct dwc2_hs
Re: [PATCH v3 4/4] usb: dwc2: refactor common low-level hw code to platform.c
Hello, On 2015-10-02 00:21, John Youn wrote: On 10/1/2015 3:04 PM, Felipe Balbi wrote: On Thu, Oct 01, 2015 at 09:04:59PM +, John Youn wrote: On 10/1/2015 8:50 AM, Felipe Balbi wrote: On Mon, Sep 21, 2015 at 12:16:12PM +0200, Marek Szyprowski wrote: DWC2 module on some platforms needs three additional hardware resources: phy controller, clock and power supply. All of them must be enabled/activated to properly initialize and operate. This was initially handled in s3c-hsotg driver, which has been converted to 'gadget' part of dwc2 driver. Unfortunately, not all of this code got moved to common platform code, what resulted in accessing DWC2 registers without enabling low-level hardware resources. This fails for example on Exynos SoCs. This patch moves all the code for managing those resources to common platform.c file and provides convenient wrappers for controlling them. Signed-off-by: Marek Szyprowski I just caught several build errors which this patch. I hope you can send me a follow-up fix (which I can amend to $subject) otherwise I'll have to drop this series I forgot that this was initially part of a larger patch-set. Maybe that is causing issues? If this wasn't intended to go through Felipe's tree then my bad. Also, I noticed this patch causes deadlock warnings with lockdep enabled. Can you look into that also while you investigate this? John, do you want me to drop the series meanwhile ? I can do that no problems. You can drop this one. I think leaving in patch 1-3 is fine as they are small cleanup patches. Unless you or Marek prefer to drop all of them. Patches 1-3 are fixes, so please keep them. I've also just posted an updated patch no 4, I've missed the fact that dwc2_hsotg structure content was defined conditionally. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland -- 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 v1 3/5] asix: Simplify asix_rx_fixup_internal() netdev alloc
The code is checking that the Ethernet frame will fit into a
netdev allocated socket buffer within the constraints of MTU size,
Ethernet header length plus VLAN header length.
The original code was checking rx->remaining each loop of the while
loop that processes multiple Ethernet frames per URB and/or Ethernet
frames that span across URBs. rx->remaining decreases per while loop
so there is no point in potentially checking multiple times that the
Ethernet frame (remaining part) will fit into the netdev socket buffer.
The modification checks that the size of the Ethernet frame will fit
the netdev socket buffer before allocating the netdev socket buffer.
This avoids grabbing memory and then deciding that the Ethernet frame
is too big and then freeing the memory.
Signed-off-by: Dean Jenkins
Signed-off-by: Mark Craske
---
drivers/net/usb/asix_common.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 89efd6a..6a8eddf 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -87,19 +87,17 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
rx->header, offset);
return 0;
}
+ if (size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
+ netdev_err(dev->net, "asix_rx_fixup() Bad RX
Length %d\n",
+ size);
+ return 0;
+ }
+
rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
if (!rx->ax_skb)
return 0;
- rx->remaining = size;
- }
- if (rx->remaining > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
- netdev_err(dev->net, "asix_rx_fixup() Bad RX Length
%d\n",
- rx->remaining);
- kfree_skb(rx->ax_skb);
- rx->ax_skb = NULL;
- rx->remaining = 0;
- return 0;
+ rx->remaining = size;
}
if (rx->remaining > skb->len - offset) {
--
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
[PATCH v1 1/5] asix: Rename remaining and size for clarity
The Data header synchronisation is easier to understand
if the variables "remaining" and "size" are renamed.
Therefore, the lifetime of the "remaining" variable exists
outside of asix_rx_fixup_internal() and is used to indicate
any remaining pending bytes of the Ethernet frame that need
to be obtained from the next socket buffer. This allows an
Ethernet frame to span across multiple socket buffers.
"size" is now local to asix_rx_fixup_internal() and contains
the size read from the Data header 32-bit word.
Add "copy_length" to hold the number of the Ethernet frame
bytes (maybe a part of a full frame) that are to be copied
out of the socket buffer.
Signed-off-by: Dean Jenkins
Signed-off-by: Mark Craske
---
drivers/net/usb/asix.h|2 +-
drivers/net/usb/asix_common.c | 41 +
2 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/drivers/net/usb/asix.h b/drivers/net/usb/asix.h
index 5d049d0..a2d3ea6 100644
--- a/drivers/net/usb/asix.h
+++ b/drivers/net/usb/asix.h
@@ -168,7 +168,7 @@ struct asix_data {
struct asix_rx_fixup_info {
struct sk_buff *ax_skb;
u32 header;
- u16 size;
+ u16 remaining;
bool split_head;
};
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 75d6f26..2bd5bdd 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -54,12 +54,13 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
struct asix_rx_fixup_info *rx)
{
int offset = 0;
+ u16 size;
while (offset + sizeof(u16) <= skb->len) {
- u16 remaining = 0;
+ u16 copy_length;
unsigned char *data;
- if (!rx->size) {
+ if (!rx->remaining) {
if ((skb->len - offset == sizeof(u16)) ||
rx->split_head) {
if(!rx->split_head) {
@@ -81,42 +82,42 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
offset += sizeof(u32);
}
- /* get the packet length */
- rx->size = (u16) (rx->header & 0x7ff);
- if (rx->size != ((~rx->header >> 16) & 0x7ff)) {
+ /* take frame length from Data header 32-bit word */
+ size = (u16)(rx->header & 0x7ff);
+ if (size != ((~rx->header >> 16) & 0x7ff)) {
netdev_err(dev->net, "asix_rx_fixup() Bad
Header Length 0x%x, offset %d\n",
rx->header, offset);
- rx->size = 0;
return 0;
}
- rx->ax_skb = netdev_alloc_skb_ip_align(dev->net,
- rx->size);
+ rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
if (!rx->ax_skb)
return 0;
+ rx->remaining = size;
}
- if (rx->size > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
+ if (rx->remaining > dev->net->mtu + ETH_HLEN + VLAN_HLEN) {
netdev_err(dev->net, "asix_rx_fixup() Bad RX Length
%d\n",
- rx->size);
+ rx->remaining);
kfree_skb(rx->ax_skb);
rx->ax_skb = NULL;
- rx->size = 0U;
-
+ rx->remaining = 0;
return 0;
}
- if (rx->size > skb->len - offset) {
- remaining = rx->size - (skb->len - offset);
- rx->size = skb->len - offset;
+ if (rx->remaining > skb->len - offset) {
+ copy_length = skb->len - offset;
+ rx->remaining -= copy_length;
+ } else {
+ copy_length = rx->remaining;
+ rx->remaining = 0;
}
- data = skb_put(rx->ax_skb, rx->size);
- memcpy(data, skb->data + offset, rx->size);
- if (!remaining)
+ data = skb_put(rx->ax_skb, copy_length);
+ memcpy(data, skb->data + offset, copy_length);
+ if (!rx->remaining)
usbnet_skb_return(dev, rx->ax_skb);
- offset += (rx->size + 1) & 0xfffe;
- rx->size = remaining;
+ offset += (copy_length + 1) & 0xfffe;
}
if (skb->len != offset) {
--
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.kern
[PATCH v1 4/5] asix: On RX avoid creating bad Ethernet frames
When RX Ethernet frames span multiple URB socket buffers,
the data stream may suffer a discontinuity which will cause
the current Ethernet frame in the netdev socket buffer
to be incomplete. This frame needs to be discarded instead
of appending unrelated data from the current URB socket buffer
to the Ethernet frame in the netdev socket buffer. This avoids
creating a corrupted Ethernet frame in the netdev socket buffer.
A discontinuity can occur when the previous URB socket buffer
held an incomplete Ethernet frame due to truncation or a
URB socket buffer containing the end of the Ethernet frame
was missing.
Therefore, add a sanity test for when an Ethernet frame
spans multiple URB socket buffers to check that the remaining
bytes of the currently received Ethernet frame point to
a good Data header 32-bit word of the next Ethernet
frame. Upon error, reset the remaining bytes variable to
zero and discard the current netdev socket buffer.
Assume that the Data header is located at the start of
the current socket buffer and attempt to process the next
Ethernet frame from there. This avoids unnecessarily
discarding a good URB socket buffer that contains a new
Ethernet frame.
Signed-off-by: Dean Jenkins
Signed-off-by: Mark Craske
---
drivers/net/usb/asix_common.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 6a8eddf..1e4768d 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -56,6 +56,34 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
int offset = 0;
u16 size;
+ /* When an Ethernet frame spans multiple URB socket buffers,
+* do a sanity test for the Data header synchronisation.
+* Attempt to detect the situation of the previous socket buffer having
+* been truncated or a socket buffer was missing. These situations
+* cause a discontinuity in the data stream and therefore need to avoid
+* appending bad data to the end of the current netdev socket buffer.
+* Also avoid unnecessarily discarding a good current netdev socket
+* buffer.
+*/
+ if (rx->remaining && (rx->remaining + sizeof(u32) <= skb->len)) {
+ offset = ((rx->remaining + 1) & 0xfffe) + sizeof(u32);
+ rx->header = get_unaligned_le32(skb->data + offset);
+ offset = 0;
+
+ size = (u16)(rx->header & 0x7ff);
+ if (size != ((~rx->header >> 16) & 0x7ff)) {
+ netdev_err(dev->net, "asix_rx_fixup() Data Header
synchronisation was lost, remaining %d\n",
+ rx->remaining);
+ kfree_skb(rx->ax_skb);
+ rx->ax_skb = NULL;
+ /* Discard the incomplete netdev Ethernet frame and
+* assume the Data header is at the start of the current
+* URB socket buffer.
+*/
+ rx->remaining = 0;
+ }
+ }
+
while (offset + sizeof(u16) <= skb->len) {
u16 copy_length;
unsigned char *data;
--
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
[PATCH v2 0/5] Improve ASIX RX memory allocation error handling
From: Mark Craske The ASIX RX handler algorithm is weak on error handling. There is a design flaw in the ASIX RX handler algorithm because the implementation for handling RX Ethernet frames for the DUB-E100 C1 can have Ethernet frames spanning multiple URBs. This means that payload data from more than 1 URB is sometimes needed to fill the socket buffer with a complete Ethernet frame. When the URB with the start of an Ethernet frame is received then an attempt is made to allocate a socket buffer. If the memory allocation fails then the algorithm sets the buffer pointer member to NULL and the function exits (no crash yet). Subsequently, the RX hander is called again to process the next URB which assumes there is a socket buffer available and the kernel crashes when there is no buffer. This patchset implements an improvement to the RX handling algorithm to avoid a crash when no memory is available for the socket buffer. The patchset will apply cleanly to the net-next master branch but the created kernel has not been tested. The driver was tested on ARM kernels v3.8 and v3.14 for a commercial product. Dean Jenkins (5): asix: Rename remaining and size for clarity asix: Tidy-up 32-bit header word synchronisation asix: Simplify asix_rx_fixup_internal() netdev alloc asix: On RX avoid creating bad Ethernet frames asix: Continue processing URB if no RX netdev buffer drivers/net/usb/asix.h|2 +- drivers/net/usb/asix_common.c | 114 ++--- 2 files changed, 74 insertions(+), 42 deletions(-) -- 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
[PATCH v1 5/5] asix: Continue processing URB if no RX netdev buffer
Avoid a loss of synchronisation of the Ethernet Data header 32-bit
word due to a failure to get a netdev socket buffer.
The ASIX RX handling algorithm returned 0 upon a failure to get
an allocation of a netdev socket buffer. This causes the URB
processing to stop which potentially causes a loss of synchronisation
with the Ethernet Data header 32-bit word. Therefore, subsequent
processing of URBs may be rejected due to a loss of synchronisation.
This may cause additional good Ethernet frames to be discarded
along with outputting of synchronisation error messages.
Implement a solution which checks whether a netdev socket buffer
has been allocated before trying to copy the Ethernet frame into
the netdev socket buffer. But continue to process the URB so that
synchronisation is maintained. Therefore, only a single Ethernet
frame is discarded when no netdev socket buffer is available.
Signed-off-by: Dean Jenkins
Signed-off-by: Mark Craske
---
drivers/net/usb/asix_common.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 1e4768d..a186b0a 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -74,12 +74,14 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
if (size != ((~rx->header >> 16) & 0x7ff)) {
netdev_err(dev->net, "asix_rx_fixup() Data Header
synchronisation was lost, remaining %d\n",
rx->remaining);
- kfree_skb(rx->ax_skb);
- rx->ax_skb = NULL;
- /* Discard the incomplete netdev Ethernet frame and
-* assume the Data header is at the start of the current
-* URB socket buffer.
-*/
+ if (rx->ax_skb) {
+ kfree_skb(rx->ax_skb);
+ rx->ax_skb = NULL;
+ /* Discard the incomplete netdev Ethernet frame
+* and assume the Data header is at the start of
+* the current URB socket buffer.
+*/
+ }
rx->remaining = 0;
}
}
@@ -121,9 +123,12 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
return 0;
}
+ /* Sometimes may fail to get a netdev socket buffer but
+* continue to process the URB socket buffer so that
+* synchronisation of the Ethernet frame Data header
+* word is maintained.
+*/
rx->ax_skb = netdev_alloc_skb_ip_align(dev->net, size);
- if (!rx->ax_skb)
- return 0;
rx->remaining = size;
}
@@ -136,10 +141,12 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
rx->remaining = 0;
}
- data = skb_put(rx->ax_skb, copy_length);
- memcpy(data, skb->data + offset, copy_length);
- if (!rx->remaining)
- usbnet_skb_return(dev, rx->ax_skb);
+ if (rx->ax_skb) {
+ data = skb_put(rx->ax_skb, copy_length);
+ memcpy(data, skb->data + offset, copy_length);
+ if (!rx->remaining)
+ usbnet_skb_return(dev, rx->ax_skb);
+ }
offset += (copy_length + 1) & 0xfffe;
}
--
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
[PATCH v1 2/5] asix: Tidy-up 32-bit header word synchronisation
Tidy-up the Data header 32-bit word synchronisation logic in
asix_rx_fixup_internal() by removing redundant logic tests.
The code is looking at the following cases of the Data header
32-bit word that is present before each Ethernet frame:
a) all 32 bits of the Data header word are in the URB socket buffer
b) first 16 bits of the Data header word are at the end of the URB
socket buffer
c) last 16 bits of the Data header word are at the start of the URB
socket buffer eg. split_head = true
Note that the lifetime of rx->split_head exists outside of the
function call and is accessed per processing of each URB. Therefore,
split_head being true acts on the next URB to be processed.
To check for b) the offset will be 16 bits (2 bytes) from the end of
the buffer then indicate split_head is true.
To check for c) split_head must be true because the first 16 bits
have been found.
To check for a) else c)
Note that the || logic of the old code included the state
(skb->len - offset == sizeof(u16) && rx->split_head) which is not
possible because the split_head cannot be true whilst checking for b).
This is because the split_head indicates that the first 16 bits have
been found and that is not possible whilst checking for the first 16
bits. Therefore simplify the logic.
Signed-off-by: Dean Jenkins
Signed-off-by: Mark Craske
---
drivers/net/usb/asix_common.c | 28 +---
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
index 2bd5bdd..89efd6a 100644
--- a/drivers/net/usb/asix_common.c
+++ b/drivers/net/usb/asix_common.c
@@ -61,21 +61,19 @@ int asix_rx_fixup_internal(struct usbnet *dev, struct
sk_buff *skb,
unsigned char *data;
if (!rx->remaining) {
- if ((skb->len - offset == sizeof(u16)) ||
- rx->split_head) {
- if(!rx->split_head) {
- rx->header = get_unaligned_le16(
- skb->data + offset);
- rx->split_head = true;
- offset += sizeof(u16);
- break;
- } else {
- rx->header |= (get_unaligned_le16(
- skb->data + offset)
- << 16);
- rx->split_head = false;
- offset += sizeof(u16);
- }
+ if (skb->len - offset == sizeof(u16)) {
+ rx->header = get_unaligned_le16(
+ skb->data + offset);
+ rx->split_head = true;
+ offset += sizeof(u16);
+ break;
+ }
+
+ if (rx->split_head == true) {
+ rx->header |= (get_unaligned_le16(
+ skb->data + offset) << 16);
+ rx->split_head = false;
+ offset += sizeof(u16);
} else {
rx->header = get_unaligned_le32(skb->data +
offset);
--
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
[PATCH v1 0/5] Improve ASIX RX memory allocation error handling
From: Mark Craske Please ignore the cover letter PATCH v2 as sent in error. Patches are all v1, (there are no v2 patches yet) The ASIX RX handler algorithm is weak on error handling. There is a design flaw in the ASIX RX handler algorithm because the implementation for handling RX Ethernet frames for the DUB-E100 C1 can have Ethernet frames spanning multiple URBs. This means that payload data from more than 1 URB is sometimes needed to fill the socket buffer with a complete Ethernet frame. When the URB with the start of an Ethernet frame is received then an attempt is made to allocate a socket buffer. If the memory allocation fails then the algorithm sets the buffer pointer member to NULL and the function exits (no crash yet). Subsequently, the RX hander is called again to process the next URB which assumes there is a socket buffer available and the kernel crashes when there is no buffer. This patchset implements an improvement to the RX handling algorithm to avoid a crash when no memory is available for the socket buffer. The patchset will apply cleanly to the net-next master branch but the created kernel has not been tested. The driver was tested on ARM kernels v3.8 and v3.14 for a commercial product. Dean Jenkins (5): asix: Rename remaining and size for clarity asix: Tidy-up 32-bit header word synchronisation asix: Simplify asix_rx_fixup_internal() netdev alloc asix: On RX avoid creating bad Ethernet frames asix: Continue processing URB if no RX netdev buffer drivers/net/usb/asix.h|2 +- drivers/net/usb/asix_common.c | 114 ++--- 2 files changed, 74 insertions(+), 42 deletions(-) -- 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 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
HBi,
On Fri, Oct 02, 2015 at 03:09:57AM +, John Youn wrote:
> On 10/1/2015 7:03 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Fri, Sep 04, 2015 at 07:15:10PM -0700, John Youn wrote:
> >> This patch allows the dwc3 driver to run on the new Synopsys USB 3.1
> >> IP core, albeit in USB 3.0 mode only.
> >>
> >> The Synopsys USB 3.1 IP (DWC_usb31) retains mostly the same register
> >> interface and programming model as the existing USB 3.0 controller IP
> >> (DWC_usb3). However, the underlying IP is different and the GSNPSID
> >> and version numbers are different.
> >>
> >> The DWC_usb31 version register is actually lower in value than the
> >> full GSNPSID of the DWC_usb3 IP. So if we are on DWC_usb3 IP just
> >> store the lower word of the GSNPSID instead of the full register. Then
> >> adjust the revision constants to match. This will allow existing
> >> revision checks to continue to work when running on the new IP.
> >
> > I would rather not touch those constants at all. We can have the driver
> > set a is_dwc31 flag and use if !is_dwc31 && rev < XYZ for the revision
> > checks.
> >
> > It's more work, but it seems better IMO.
>
> I initially did it like that but it got really messy and it would
> make it harder to port back to stable kernels...
except that this doesn't apply to stable kernels :-) Stable is strictly for
regression fixes. We _do_ get the odd new device id into stable, but only when
it's really just a device id. dwc31 requires a bunch of other changes to get it
to work with current driver, it's not only a new device id, right ?
> >> Finally add a documentation note about the revision numbering and
> >> checking with regards to the old and new IP. Because these are
> >> different IPs which will both continue to be supported, feature sets
> >> and revisions checks may not sync-up across future versions.
> >
> > and this is why I prefer to have the flag :-) We can run different revision
> > check methods depending if we're running on dwc3 or dwc31.
>
> All of the existing checks apply to both. The mismatch condition
> will be the exception.
okay. Let's take one example:
if (dwc->revision < DWC3_REVISION_220A) {
reg |= DWC3_DCFG_SUPERSPEED;
} else {
...
So this is a check that's only valid for DWC3 because DWC3 was the one which had
this bug, not DWC31. In order to skip this for DWC31 we should have something
like:
if (!dwc->is_dwc31 && dwc->revision < DWC3_REVISION_220A) {
...
it looks awful, but this is only the case because SNPS made the revision of the
newer cores lower than the previous ones :-p if you used 0x5534 for example,
we wouldn't have this problem.
Yeah, difficult choice. This is_dwc31 will look awful. How about using bit31
of the revision as a is_dwc31 flag ? We don't touch the older revisions and
we're gonna be using a reserved bit as a flag. Then, the revision check would
look like:
reg = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
/**
* NOTICE: we're using bit 31 as a "is dwc3.1" flag. This is really
* just so dwc31 revisions are always larger than dwc3.
*/
reg |= DWC3_REVISION_IS_DWC31;
> >> From now, any check based on a revision (for STARS, workarounds, and
> >> new features) should take into consideration how it applies to both
> >> the 3.1/3.0 IP and make the check accordingly.
> >>
> >> Cc: # v3.18+
> >
> > I really fail to how any of these patches in this series apply for stable.
> > Care
> > to explain ?
>
> We have some prototyping products that are stuck on 3.18 stable
> kernels and will continue to ship with that for some time. We'd
> like to run the USB 3.1 controller on those platforms. Without
> these version id and version number updates dwc3 will not work
> with the USB 3.1 IP.
>
> I think the plan is to update those platforms to 4.2 eventually.
> So even then it will still need this patch.
>
> Also it will help out any customers stuck on earlier kernels.
>
> How would you advise we handle this, with the version id and
> number changes?
I have a feeling the answer to that will be that you will need to backport your
own patches :-( Or upgrade to the latest kernel around once your patches get
merged.
Would you care to explain why upgrading the kernel is so complex for this
prototyping solution ? I suppose you're not using HAPS as a PCIe card on a
common desktop PC, then ?
> I can make the changes as you suggest but it might be a pain
> to apply it to stable kernels.
>
> The other patches in this series tagged for stable are to
> support these same platforms on 3.18+. Either so that they
> can work at all (such as missing PCI IDs) or to pass some
> compliance tests (LPM flags in platform data, enblslpm quirk).
>
>
>
> >
> >> Signed-off-by: John Youn
> >> ---
> >> drivers/usb/dwc3/core.c | 9 ++--
> >> drivers/usb/dwc3/core.h | 56
> >> +++--
> >> 2 files changed, 43 insertions(+), 22 del
Mass Storage Gadget Kthread
Hi Alan, here's a question which I hope you can help me understand :-) Why do we have that kthread for the mass storage gadgets ? I noticed a very interesting behavior. Basically, the MSC class works in a loop like so: CBW Data Transfer CSW In our implemention, what we do is: CBW wake_up_process() Data Transfer wake_up_process() CSW wake_up_process() Now here's the interesting bit. Every time we wake_up_process(), we basically don't do anything until MSC's kthread gets finally scheduled and has a chance of doing its job. This means that the host keeps sending us tokens but the UDC doesn't have any request queued to start a transfer. This happens specially with IN endpoints, not so much on OUT directions. See figure one [1] we can see that host issues over 7 POLLs before UDC has finally started a usb_request, sometimes this goes for even longer (see image [3]). On figure two we can see that on this particular session, I had as much as 15% of the bandwidth wasted on POLLs. With this current setup I'm 34MB/sec and with the added 15% that would get really close to 40MB/sec. So the question is, why do we have to wait for that kthread to get scheduled ? Why couldn't we skip it completely ? Is there really anything left in there that couldn't be done from within usb_request->complete() itself ? I'll spend some time on that today and really dig that thing up, but if you know the answer off the top of your head, I'd be happy to hear. Thanks [1] http://imgur.com/RHJ5jxj [2] http://imgur.com/F9qA7j5 [3] http://imgur.com/LoaFunZ -- balbi signature.asc Description: PGP signature
Re: Queueing b0a688ddcc50 "usb: musb: cppi41: allow it to work again" for -stable
On Fri, Oct 02, 2015 at 02:23:45AM -0300, Ezequiel Garcia wrote: > Hello, > > Commit b0a688ddcc50 "usb: musb: cppi41: allow it to work again" seems > to fix a regression. It applies cleanly on v4.1 and removes the > "musb-hdrc musb-hdrc.1.auto: Need DT for the DMA engine." error. > > Any chance you can queue it for -stable? Have a look at Documentation/stable_kernel_rules.txt, you can do a proper request for Greg and he will make sure it gets there. -- balbi signature.asc Description: PGP signature
Re: [PATCH v4 1/5] gadget: Introduce the notifier functions
On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote: > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote: > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > > Frankly, I wanted all of this to be decided in userland with the > > > kernel just providing notification and basic safety checks (we don't > > > want to allow a bogus userspace daemon frying anybody's devices). > > What's the advantage of pushing this to userspace? By the time we > > provide enough discoverability to enable userspace to configure itself > > it seems like we'd have enough information to do the job anyway. > you're going to be dealing with a setup where each vendor does one thing > differently. Some will have it all in the SoC part of a single IP (dwc3 can be > configured that way), some will push it out to companion IC, some might even > use > some mostly discrete setup and so on... Right, and that was exactly what this was supposed to be all about when I was discussing this originally with Baolin (who's on holiday until sometime next week BTW, hence me answering). > We're gonna be dealing with a decision that involves information from multiple > subsystems (USB, regulator, hwmon, power supply to name a few). Sure, that was part of the idea here - provide a central point representing the logical port where we can aggregate all the information we get in and distribute it to consumers. > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and > it's > just plain difficult. To make matters worse, N900 had two USB PHYs, one for > actual runtime use and another just for detecting the charger type on the > other > end. Can you elaborate on what the difficulties are and how punting to userspace helps? If anything I'd expect punting to userspace to make things more difficult, if nothing else it means we need to get whatever userspace component deployed in all relevant userspace stacks with appropriate configuration in order to do things. We do punt a lot of configuration to userspace for audio because there are substantial device specific policy elements in the configuration and it's a far from painless experience getting the code and configuration deployed where people need it, definitely a not great for users. > On top of all that, we still need to figure out what to do when a particular > configuration gets chosen, and what to do when the bus goes into the different > suspend levels. > It's going to be a lot of policy getting added to the kernel. On top of all > that, when Type C and power delivery is finally a thing, there will an entire > new stack for USB C commands (using the Type C CC pins or modulated on VBUS > for > legacy connectors) which we will have to add to the kernel. And different > devices will support different charging profiles (there are at least 6 of > those, > IIRC). Yes, USB C was part of the thinking with starting this work - it's going to make ensuring that the system is paying attention to limits much more important. I think there's two things here. One is working out how the system is glued together and the other thing is working out what to do with that system. I think that where we can do it the bit where work out how the various components are connected and aggregate their functionality together is definitely a kernel job. It means that userspace doesn't need to worry about implementation details of the particular system and instead gets a consistent, standard view of the device (in this case a USB port and how much power we can draw from it). For the policy stuff we do want to have userspace be able to control things but we need to think about how to do that - for example does the entire flow need to be in userspace, or can we just expose settings which userspace can manage? > With all these different things going on, it's best to do what e.g. NFC folks > did. Just a small set of hooks in the kernel, but actual implementation done > by > a userspace daemon. This makes it even easier for middleware development since > we can provide a higher level API for middleware to talk to the charging > daemon. I'm not sure that the NFC model is working well for everyone - it's OK if you happen to be running Android but if you aren't you're often left sitting there working out what to do with an Android HAL. We can do the middleware thing without going quite that far, we do already have power management daemons in userspace even on desktop (GNOME has upowerd for example). > Another benefit is that this charging daemon can also give hints to power > management policy that e.g. we're charging at 10W or 100W and we can e.g. > switch > to performance governor safely even though battery is rather low. We definitely want userspace to be able to see the current state, even if it just passes it straight on to the user it's a common part of UIs on both desktop and mobile operating systems. > Anyway, there's really a lot that needs to h
Re: Queueing b0a688ddcc50 "usb: musb: cppi41: allow it to work again" for -stable
On 2 October 2015 at 13:09, Felipe Balbi wrote: > On Fri, Oct 02, 2015 at 02:23:45AM -0300, Ezequiel Garcia wrote: >> Hello, >> >> Commit b0a688ddcc50 "usb: musb: cppi41: allow it to work again" seems >> to fix a regression. It applies cleanly on v4.1 and removes the >> "musb-hdrc musb-hdrc.1.auto: Need DT for the DMA engine." error. >> >> Any chance you can queue it for -stable? > > Have a look at Documentation/stable_kernel_rules.txt, you can do a proper > request for Greg and he will make sure it gets there. > Sure, I will. Maybe next time you could add a Cc: stable to these kinds of fixes, along with a proper Fixes tag, so the -stable folks can pick them automatically. Thanks! -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar -- 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 v4 1/5] gadget: Introduce the notifier functions
Hi, On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote: > On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote: > > On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote: > > > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote: > > > > > Frankly, I wanted all of this to be decided in userland with the > > > > kernel just providing notification and basic safety checks (we don't > > > > want to allow a bogus userspace daemon frying anybody's devices). > > > > What's the advantage of pushing this to userspace? By the time we > > > provide enough discoverability to enable userspace to configure itself > > > it seems like we'd have enough information to do the job anyway. > > > > you're going to be dealing with a setup where each vendor does one thing > > differently. Some will have it all in the SoC part of a single IP (dwc3 can > > be > > configured that way), some will push it out to companion IC, some might > > even use > > some mostly discrete setup and so on... > > Right, and that was exactly what this was supposed to be all about when > I was discussing this originally with Baolin (who's on holiday until > sometime next week BTW, hence me answering). > > > We're gonna be dealing with a decision that involves information from > > multiple > > subsystems (USB, regulator, hwmon, power supply to name a few). > > Sure, that was part of the idea here - provide a central point > representing the logical port where we can aggregate all the information > we get in and distribute it to consumers. > > > We tried doing it all in the kernel back in N800, N810 and N900/N9 days and > > it's > > just plain difficult. To make matters worse, N900 had two USB PHYs, one for > > actual runtime use and another just for detecting the charger type on the > > other > > end. > > Can you elaborate on what the difficulties are and how punting to > userspace helps? If anything I'd expect punting to userspace to make the difficulty was mainly making sure all involved parties talk the same language. I mean, you plug cable and detect charger (this is usually done by the PMIC or by a BQ-type device - probably done at drivers/power_supply). First difficulty comes right here. Power_supply notifies that we're attached to a charger (sometimes it can't differentiate a host/hub charger from a wall charger), a few ms later you get a notification from the gadget that it got enumerated with a 500mA configuration. Depending on the order of things, your load will be configured either to 2A (maximum host/hub charger current) or 500mA. Yes, this can be mitigated :-) Focussing on this alone, you can have as much as 4 different subsystems involved, all throwing raw_notifiers at each other. Now each of these subsystems need to maintain their own state machine about what notification we have received and what are the valid next states. I would rather have userspace be the single place getting notifications because then we solve these details at a single location. > Things more difficult, if nothing else it means we need to get whatever > userspace component deployed in all relevant userspace stacks with > appropriate configuration in order to do things. but that will be a thing for the kernel too. We will have to deploy this new kernel component in all relevant stacks with appropriate configuration in order to do things :-) No changes there. > We do punt a lot of configuration to userspace for audio because there > are substantial device specific policy elements in the configuration and > it's a far from painless experience getting the code and configuration > deployed where people need it, definitely a not great for users. it's the same for this situation. We will have a ton of device specific configuration, specially with power delivery class, billboard class, the alternate modes and so on. If we already do this part in userland, adding all those extras will be, IMO, far simpler. > > On top of all that, we still need to figure out what to do when a particular > > configuration gets chosen, and what to do when the bus goes into the > > different > > suspend levels. > > > It's going to be a lot of policy getting added to the kernel. On top of all > > that, when Type C and power delivery is finally a thing, there will an > > entire > > new stack for USB C commands (using the Type C CC pins or modulated on VBUS > > for > > legacy connectors) which we will have to add to the kernel. And different > > devices will support different charging profiles (there are at least 6 of > > those, > > IIRC). > > Yes, USB C was part of the thinking with starting this work - it's going > to make ensuring that the system is paying attention to limits much more > important. I think there's two things here. One is working out how the > system is glued together and the other thing is working out what to do > with that system. right, and IMO, what to do should be a decision made outside of the kernel as long
Re: Queueing b0a688ddcc50 "usb: musb: cppi41: allow it to work again" for -stable
On Fri, Oct 02, 2015 at 02:16:50PM -0300, Ezequiel Garcia wrote: > On 2 October 2015 at 13:09, Felipe Balbi wrote: > > On Fri, Oct 02, 2015 at 02:23:45AM -0300, Ezequiel Garcia wrote: > >> Hello, > >> > >> Commit b0a688ddcc50 "usb: musb: cppi41: allow it to work again" seems > >> to fix a regression. It applies cleanly on v4.1 and removes the > >> "musb-hdrc musb-hdrc.1.auto: Need DT for the DMA engine." error. > >> > >> Any chance you can queue it for -stable? > > > > Have a look at Documentation/stable_kernel_rules.txt, you can do a proper > > request for Greg and he will make sure it gets there. > > > > Sure, I will. > > Maybe next time you could add a Cc: stable to these kinds of fixes, along with > a proper Fixes tag, so the -stable folks can pick them automatically. I usually do, in this case, nobody had complained about this board in any stable kernel so I just didn't. -- balbi signature.asc Description: PGP signature
Re: [PATCH v4 1/5] gadget: Introduce the notifier functions
Hi,
On Fri, Oct 02, 2015 at 07:41:07AM +0200, Greg KH wrote:
> On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:
> > On Thu, Sep 24, 2015 at 10:39:23AM -0700, Baolin Wang wrote:
> > > The usb charger framework is based on usb gadget. The usb charger
> > > need to be notified the state changing of usb gadget to confirm the
> > > usb charger state.
> > >
> > > Thus this patch adds a notifier mechanism for usb gadget to report a
> > > event to usb charger when the usb gadget state is changed.
> > >
> > > Signed-off-by: Baolin Wang
> > > ---
> > > drivers/usb/gadget/udc/udc-core.c | 32
> > > include/linux/usb/gadget.h| 18 ++
> > > 2 files changed, 50 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/udc/udc-core.c
> > > b/drivers/usb/gadget/udc/udc-core.c
> > > index f660afb..4238fc3 100644
> > > --- a/drivers/usb/gadget/udc/udc-core.c
> > > +++ b/drivers/usb/gadget/udc/udc-core.c
> > > @@ -129,6 +129,32 @@ void usb_gadget_giveback_request(struct usb_ep *ep,
> > > }
> > > EXPORT_SYMBOL_GPL(usb_gadget_giveback_request);
> > >
> > > +int usb_gadget_register_notify(struct usb_gadget *gadget,
> > > +struct notifier_block *nb)
> > > +{
> > > + int ret;
> > > +
> > > + mutex_lock(&gadget->lock);
> > > + ret = raw_notifier_chain_register(&gadget->nh, nb);
> > > + mutex_unlock(&gadget->lock);
> > > +
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_gadget_register_notify);
> > > +
> > > +int usb_gadget_unregister_notify(struct usb_gadget *gadget,
> > > + struct notifier_block *nb)
> > > +{
> > > + int ret;
> > > +
> > > + mutex_lock(&gadget->lock);
> > > + ret = raw_notifier_chain_unregister(&gadget->nh, nb);
> >
> > Greg, this is the kind of thing I wanted to avoid adding more of.
> >
> > I was wondering if you would accept subsystems using kdbus for
> > this sort of notification. I'm okay waiting for kdbus for another
> > couple merge windows (if we have to) before that's merged, but
> > if we take this raw notifier approach now, we will end up having
> > to support it forever.
>
> kdbus is userspace <-> userspace messages, it doesn't do kernel <->
> userspace messages, sorry.
oh well, tough luck :-)
having a more well-constructed notification method to userspace would be a
little better, but I guess we will have to resort to sysfs_notify() or something
like that.
> > Also, because soon enough we will have to support USB Power Delivery
> > with Type C connector, this is bound to change in the coming months.
> >
> > Frankly, I wanted all of this to be decided in userland with the
> > kernel just providing notification and basic safety checks (we don't
> > want to allow a bogus userspace daemon frying anybody's devices).
> >
> > How would you feel about that ?
>
> I agree I don't like new notifier chains being added, but as this is all
> in-kernel, we can change the api in the future and there would not be a
> problem, right?
not necessarily. This will, eventually, get exposed to userspace. At minimum for
adding eye candy to the UI. Chaging battery icon or whatever. The danger here is
that when that something gets exposed, we will have to support it for the next
30 years :-)
> And yeah, I'm worried about the USB Power delivery patches, I haven't
> seen them yet, but I hear about different groups doing different things
> here, and that worries me.
my worries exactly :-) This is why I'm looking for a flexible enough approach
which puts as little into the kernel as necessary.
--
balbi
signature.asc
Description: PGP signature
Re: Mass Storage Gadget Kthread
On Fri, 2 Oct 2015, Felipe Balbi wrote: > Hi Alan, > > here's a question which I hope you can help me understand :-) > > Why do we have that kthread for the mass storage gadgets ? I noticed a very > interesting behavior. Basically, the mass-storage gadget uses a kthread in order to access the backing file. Obviously a kernel driver doesn't _need_ to use a separate thread to read or write a file (think of /dev/loop, for example). But doing it that way is very easy because the driver merely has to imitate read(2) and write(2) system calls, which have a simple and well defined interface. To do anything more direct would mean getting deeply involved in the block and filesystem layers, and that wasn't my goal -- I wanted to write a USB gadget driver, not re-implement the loop driver. Also, remember that at the time the MS Gadget was written, speed over USB wasn't a pressing issue. You couldn't transfer more than about 40 MB/s anyway, so efficiency in the gadget wasn't a terribly high priority. > Basically, the MSC class works in a loop like so: > > CBW > Data Transfer > CSW > > In our implemention, what we do is: > > CBW > wake_up_process() > Data Transfer > wake_up_process() > CSW > wake_up_process() The wake_up_process() call is the notification from the completion routine telling the kthread that a request has completed. The kthread doesn't necessarily stop and wait for these notifications; it can continue processing in parallel. > Now here's the interesting bit. Every time we wake_up_process(), we basically > don't do anything until MSC's kthread gets finally scheduled and has a chance > of > doing its job. That's not entirely true; the kthread may already be running. For example, the default MS gadget uses two I/O buffers (there's a Kconfig option to use more if you want). When carrying out a READ, the kthread fills up the first buffer and submits it to the UDC. But it doesn't stop and wait for wake_up_process(); instead it goes on to fill up the second buffer while the first is being sent back to the host. The wake_up_process() may very well occur while the second buffer is being filled, in which case it won't do anything (since the kthread isn't asleep). > This means that the host keeps sending us tokens but the UDC > doesn't have any request queued to start a transfer. This happens specially > with > IN endpoints, not so much on OUT directions. See figure one [1] we can see > that > host issues over 7 POLLs before UDC has finally started a usb_request, > sometimes > this goes for even longer (see image [3]). Figure [1] is misleading. The 7 POLLs you see are at the very start of a READ. It's not surprising that the host can poll 7 times before the gadget manages to read the first block of data from the backing file. This isn't a case where the kthread could have been doing something useful instead of waiting around to hear from the host. Figure [3] is difficult to interpret because it doesn't include the transfer lengths. I can't tell what was going on during the 37 + 50 POLLs before the first OUT transfer. If that was a CDB starting a new command, I don't see why it would take that long to schedule the kthread unless the CPU was busy with other tasks. > On figure two we can see that on this particular session, I had as much as 15% > of the bandwidth wasted on POLLs. With this current setup I'm 34MB/sec and > with > the added 15% that would get really close to 40MB/sec. So high speed, right? Are the numbers in the figure handshake _counts_ or handshake _times_? A simple NAK doesn't use much bandwidth. Even if 15% of the handshakes are NAKs, it doesn't mean you're wasting 15% of the bandwidth. > So the question is, why do we have to wait for that kthread to get scheduled ? > Why couldn't we skip it completely ? Is there really anything left in there > that > couldn't be done from within usb_request->complete() itself ? The real answer is the calls to vfs_read() and vfs_write() -- those have to occur in process context. In theory, the CBW packet doesn't have to be processed by the kthread. But processing it in the completion routine wouldn't help, because the kthread would still have to be scheduled in order to carry out the READ or WRITE command. In other words, changing: Completion handler Kthread -- --- Receive CBW Wake up kthread Process CBW Start READ or WRITE into: Completion handler Kthread -- --- Receive CBW Process CBW Wake up kthread Start READ or WRITE doesn't really give any significant advantage. > I'll spend some time on that today and really dig that thing up, but if you > know > the answer off the top of your head, I'd be happy to hear. I ho
Re: [PATCH 02/14] RFC: usb/host/fotg210: remove KERN_WARNING from pr_info
On Mon, Sep 21, 2015 at 05:01:06PM +0200, Peter Senna Tschudin wrote:
> This patch remove KERN_WARNING from a call to pr_info().
s/pr_info/pr_warn/
other than that, looks good
>
> Signed-off-by: Peter Senna Tschudin
> ---
> drivers/usb/host/fotg210-hcd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 48eac34..36413b2 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -5740,7 +5740,7 @@ static int __init fotg210_hcd_init(void)
> set_bit(USB_EHCI_LOADED, &usb_hcds_loaded);
> if (test_bit(USB_UHCI_LOADED, &usb_hcds_loaded) ||
> test_bit(USB_OHCI_LOADED, &usb_hcds_loaded))
> - pr_warn(KERN_WARNING "Warning! fotg210_hcd should always be
> loaded before uhci_hcd and ohci_hcd, not after\n");
> + pr_warn("Warning! fotg210_hcd should always be loaded before
> uhci_hcd and ohci_hcd, not after\n");
>
> pr_debug("%s: block sizes: qh %Zd qtd %Zd itd %Zd\n",
>hcd_name,
> --
> 2.1.0
>
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 01/14] RFC: usb/host/fotg210: Fix coding style issues
On Mon, Sep 21, 2015 at 05:01:05PM +0200, Peter Senna Tschudin wrote: > This patch fix coding style issues reported by checkpatch that do not > change semantics of the code. > > Signed-off-by: Peter Senna Tschudin > --- > drivers/usb/host/fotg210-hcd.c | 1248 > +--- > drivers/usb/host/fotg210.h | 36 +- > 2 files changed, 558 insertions(+), 726 deletions(-) > > diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c > index 000ed80..48eac34 100644 > --- a/drivers/usb/host/fotg210-hcd.c > +++ b/drivers/usb/host/fotg210-hcd.c > @@ -50,32 +50,30 @@ > #include > #include > > -/*-*/ > #define DRIVER_AUTHOR "Yuan-Hsin Chen" > #define DRIVER_DESC "FOTG210 Host Controller (EHCI) Driver" > - > -static const charhcd_name[] = "fotg210_hcd"; > +static const char hcd_name[] = "fotg210_hcd"; > > #undef FOTG210_URB_TRACE > - > #define FOTG210_STATS > > /* magic numbers that can affect system performance */ > -#define FOTG210_TUNE_CERR 3 /* 0-3 qtd retries; 0 == > don't stop */ > -#define FOTG210_TUNE_RL_HS 4 /* nak throttle; see 4.9 */ > -#define FOTG210_TUNE_RL_TT 0 > -#define FOTG210_TUNE_MULT_HS1 /* 1-3 transactions/uframe; > 4.10.3 */ > -#define FOTG210_TUNE_MULT_TT1 > +#define FOTG210_TUNE_CERR3 /* 0-3 qtd retries; 0 == don't stop */ > +#define FOTG210_TUNE_RL_HS 4 /* nak throttle; see 4.9 */ > +#define FOTG210_TUNE_RL_TT 0 > +#define FOTG210_TUNE_MULT_HS 1 /* 1-3 transactions/uframe; 4.10.3 */ > +#define FOTG210_TUNE_MULT_TT 1 > + > /* > - * Some drivers think it's safe to schedule isochronous transfers more than > - * 256 ms into the future (partly as a result of an old bug in the scheduling > + * Some drivers think it's safe to schedule isochronous transfers more than > 256 > + * ms into the future (partly as a result of an old bug in the scheduling > * code). In an attempt to avoid trouble, we will use a minimum scheduling > * length of 512 frames instead of 256. > */ > -#define FOTG210_TUNE_FLS1 /* (medium) 512-frame > schedule */ > +#define FOTG210_TUNE_FLS 1 /* (medium) 512-frame schedule */ > > /* Initial IRQ latency: faster than hw default */ > -static int log2_irq_thresh; /* 0 to 6 */ > +static int log2_irq_thresh; /* 0 to 6 */ > module_param(log2_irq_thresh, int, S_IRUGO); > MODULE_PARM_DESC(log2_irq_thresh, "log2 IRQ latency, 1-64 microframes"); > > @@ -89,66 +87,57 @@ static unsigned int hird; > module_param(hird, int, S_IRUGO); > MODULE_PARM_DESC(hird, "host initiated resume duration, +1 for each 75us"); > > -#define INTR_MASK (STS_IAA | STS_FATAL | STS_PCD | STS_ERR | STS_INT) > +#define INTR_MASK (STS_IAA | STS_FATAL | STS_PCD | STS_ERR | STS_INT) > > #include "fotg210.h" > > -/*-*/ > - > #define fotg210_dbg(fotg210, fmt, args...) \ > - dev_dbg(fotg210_to_hcd(fotg210)->self.controller , fmt , ## args) > + dev_dbg(fotg210_to_hcd(fotg210)->self.controller, fmt, ## args) > #define fotg210_err(fotg210, fmt, args...) \ > - dev_err(fotg210_to_hcd(fotg210)->self.controller , fmt , ## args) > + dev_err(fotg210_to_hcd(fotg210)->self.controller, fmt, ## args) > #define fotg210_info(fotg210, fmt, args...) \ > - dev_info(fotg210_to_hcd(fotg210)->self.controller , fmt , ## args) > + dev_info(fotg210_to_hcd(fotg210)->self.controller, fmt, ## args) > #define fotg210_warn(fotg210, fmt, args...) \ > - dev_warn(fotg210_to_hcd(fotg210)->self.controller , fmt , ## args) > + dev_warn(fotg210_to_hcd(fotg210)->self.controller, fmt, ## args) > > -/* check the values in the HCSPARAMS register > - * (host controller _Structural_ parameters) > - * see EHCI spec, Table 2-4 for each value > +/* check the values in the HCSPARAMS register (host controller _Structural_ if you're fixing coding style issues, you might as well go ahead and fix this comment style too: /* * check the values -- balbi signature.asc Description: PGP signature
Re: [PATCH 06/14] RFC: usb/host/fotg210: replace msleep by usleep_range
On Mon, Sep 21, 2015 at 05:01:10PM +0200, Peter Senna Tschudin wrote: > msleep under 20ms can result in sleeping up to 20ms, which may not be > intended. Replace msleep(5) by usleep_range(5000, 6000). > > Signed-off-by: Peter Senna Tschudin good catch. I'd apply this straight away. Alan ? > --- > drivers/usb/host/fotg210-hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c > index e7e9991..55c2279 100644 > --- a/drivers/usb/host/fotg210-hcd.c > +++ b/drivers/usb/host/fotg210-hcd.c > @@ -5122,7 +5122,7 @@ static int fotg210_run(struct usb_hcd *hcd) > fotg210->rh_state = FOTG210_RH_RUNNING; > /* unblock posted writes */ > fotg210_readl(fotg210, &fotg210->regs->command); > - msleep(5); > + usleep_range(5000, 6000); > up_write(&ehci_cf_port_reset_rwsem); > fotg210->last_periodic_enable = ktime_get_real(); > > -- > 2.1.0 > -- balbi signature.asc Description: PGP signature
Re: [PATCH 05/14] RFC: usb/host/fotg210: change kmalloc by kmalloc_array
On Mon, Sep 21, 2015 at 05:01:09PM +0200, Peter Senna Tschudin wrote: > This patch change: > > kmalloc(DBG_SCHED_LIMIT * sizeof(*seen), GFP_ATOMIC) > > by: > > kmalloc_array(DBG_SCHED_LIMIT, sizeof(*seen), GFP_ATOMIC) > > as kmalloc_array() should be used for allocating arrays. > > Signed-off-by: Peter Senna Tschudin looks good to me > --- > drivers/usb/host/fotg210-hcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c > index 51feb61..e7e9991 100644 > --- a/drivers/usb/host/fotg210-hcd.c > +++ b/drivers/usb/host/fotg210-hcd.c > @@ -501,7 +501,7 @@ static ssize_t fill_periodic_buffer(struct debug_buffer > *buf) > unsigned i; > __hc32 tag; > > - seen = kmalloc(DBG_SCHED_LIMIT * sizeof(*seen), GFP_ATOMIC); > + seen = kmalloc_array(DBG_SCHED_LIMIT, sizeof(*seen), GFP_ATOMIC); > if (!seen) > return 0; > > -- > 2.1.0 > -- balbi signature.asc Description: PGP signature
Re: [PATCH 04/14] RFC: usb/host/fotg210: Remove NULL checks dma_pool_destroy
On Mon, Sep 21, 2015 at 05:01:08PM +0200, Peter Senna Tschudin wrote:
> This patch remove NULL checks before calls to dma_pool_destroy() as the
> function now can handle NULL pointers.
>
> Signed-off-by: Peter Senna Tschudin
looks good to me
> ---
> drivers/usb/host/fotg210-hcd.c | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index f574143..51feb61 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -1925,17 +1925,13 @@ static void fotg210_mem_cleanup(struct fotg210_hcd
> *fotg210)
> fotg210->dummy = NULL;
>
> /* DMA consistent memory and pools */
> - if (fotg210->qtd_pool)
> - dma_pool_destroy(fotg210->qtd_pool);
> + dma_pool_destroy(fotg210->qtd_pool);
> fotg210->qtd_pool = NULL;
>
> - if (fotg210->qh_pool) {
> - dma_pool_destroy(fotg210->qh_pool);
> - fotg210->qh_pool = NULL;
> - }
> + dma_pool_destroy(fotg210->qh_pool);
> + fotg210->qh_pool = NULL;
>
> - if (fotg210->itd_pool)
> - dma_pool_destroy(fotg210->itd_pool);
> + dma_pool_destroy(fotg210->itd_pool);
> fotg210->itd_pool = NULL;
>
> if (fotg210->periodic)
> --
> 2.1.0
>
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 07/14] RFC: usb/host/fotg210: Remove a macro from snprintf
On Mon, Sep 21, 2015 at 05:01:11PM +0200, Peter Senna Tschudin wrote:
> This patch removes a macro from a call to snprintf() and moves it's
> content to just before the call to snprintf() assigning a value to a new
> variable named tmp. The goal of this patch is to make the code easier to
> understand.
>
> Signed-off-by: Peter Senna Tschudin
it's a little unnecessary, but no objections.
> ---
> drivers/usb/host/fotg210-hcd.c | 43
> ++
> 1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 55c2279..4032ed0 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -367,6 +367,8 @@ static void qh_lines(struct fotg210_hcd *fotg210, struct
> fotg210_qh *qh,
> unsigned size = *sizep;
> char *next = *nextp;
> char mark;
> + char *tmp;
> +
> __le32 list_end = FOTG210_LIST_END(fotg210);
> struct fotg210_qh_hw *hw = qh->hw;
>
> @@ -411,28 +413,29 @@ static void qh_lines(struct fotg210_hcd *fotg210,
> struct fotg210_qh *qh,
> else if (td->hw_alt_next != list_end)
> mark = '/';
> }
> - temp = snprintf(next, size,
> - "\n\t%p%c%s len=%d %08x urb %p",
> - td, mark, ({ char *tmp;
> - switch ((scratch>>8)&0x03) {
> - case 0:
> - tmp = "out";
> - break;
> - case 1:
> - tmp = "in";
> - break;
> - case 2:
> - tmp = "setup";
> - break;
> - default:
> - tmp = "?";
> - break;
> - } tmp; }),
> - (scratch >> 16) & 0x7fff,
> - scratch,
> - td->urb);
> +
> + switch ((scratch >> 8) & 0x03) {
> + case 0:
> + tmp = "out";
> + break;
> + case 1:
> + tmp = "in";
> + break;
> + case 2:
> + tmp = "setup";
> + break;
> + default:
> + tmp = "?";
> + break;
> + }
> +
> + temp = snprintf(next, size, "\n\t%p%c%s len=%d %08x urb %p",
> + td, mark, tmp, (scratch >> 16) & 0x7fff,
> + scratch, td->urb);
> +
> if (size < temp)
> temp = size;
> +
> size -= temp;
> next += temp;
> if (temp == size)
> --
> 2.1.0
>
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 03/14] RFC: usb/host/fotg210: Remove useless else statement
On Mon, Sep 21, 2015 at 05:01:07PM +0200, Peter Senna Tschudin wrote:
> This patch remove an else statement after a return to make the code
> easier to understand.
>
> Signed-off-by: Peter Senna Tschudin
looks good to me.
> ---
> drivers/usb/host/fotg210-hcd.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 36413b2..f574143 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -1410,10 +1410,9 @@ static int check_reset_complete(struct fotg210_hcd
> *fotg210, int index,
> "Failed to enable port %d on root hub TT\n",
> index+1);
> return port_status;
> - } else {
> - fotg210_dbg(fotg210, "port %d reset complete, port enabled\n",
> - index + 1);
> }
> + fotg210_dbg(fotg210, "port %d reset complete, port enabled\n",
> + index + 1);
>
> return port_status;
> }
> --
> 2.1.0
>
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 09/14] RFC: usb/host/fotg210: Add function: output_buf_tds_dir()
On Mon, Sep 21, 2015 at 05:01:13PM +0200, Peter Senna Tschudin wrote:
> checkpatch complains about too many leading tabs because the switch
> statement starts after 6 tabs.
>
> fill_periodic_buffer() -> for() -> do -> switch() -> if() ->
> list_for_each_entry() and finally the last switch().
>
> This patch moves the list_for_each_entry() and the last switch() to a
> new inline function named output_buf_tds_dir(). This change makes the
> code easier to read and calm down checkpatch. This patch changes it to:
>
> fill_periodic_buffer() -> for() -> do -> switch() -> if() ->
> output_buf_tds_dir()
>
> Signed-off-by: Peter Senna Tschudin
if you fix Sergei's comment, then I'm okay with $subject
> ---
> drivers/usb/host/fotg210-hcd.c | 64
> ++
> 1 file changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 82cd5da..13cca41 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -497,6 +497,36 @@ static ssize_t fill_async_buffer(struct debug_buffer
> *buf)
> return strlen(buf->output_buf);
> }
>
> +/* count tds, get ep direction */
> +static inline unsigned output_buf_tds_dir(char *buf,
> + struct fotg210_hcd *fotg210,
> + struct fotg210_qh_hw *hw,
> + struct fotg210_qh *qh, unsigned size)
> +{
> + u32 scratch = hc32_to_cpup(fotg210, &hw->hw_info1);
> + struct fotg210_qtd *qtd;
> + char *type = "";
> + unsigned temp = 0;
> +
> + /* count tds, get ep direction */
> + list_for_each_entry(qtd, &qh->qtd_list, qtd_list) {
> + temp++;
> + switch (0x03 & (hc32_to_cpu(fotg210, qtd->hw_token) >> 8)) {
> + case 0:
> + type = "out";
> + continue;
> + case 1:
> + type = "in";
> + continue;
> + }
> + }
> +
> + return scnprintf(buf, size, "(%c%d ep%d%s [%d/%d] q%d p%d)",
> + speed_char(scratch), scratch & 0x007f,
> + (scratch >> 8) & 0x000f, type, qh->usecs,
> + qh->c_usecs, temp, 0x7ff & (scratch >> 16));
> +}
> +
> #define DBG_SCHED_LIMIT 64
> static ssize_t fill_periodic_buffer(struct debug_buffer *buf)
> {
> @@ -568,37 +598,9 @@ static ssize_t fill_periodic_buffer(struct debug_buffer
> *buf)
> }
> /* show more info the first time around */
> if (temp == seen_count) {
> - u32 scratch = hc32_to_cpup(fotg210,
> - &hw->hw_info1);
> - struct fotg210_qtd *qtd;
> - char *type = "";
> -
> - /* count tds, get ep direction */
> - temp = 0;
> - list_for_each_entry(qtd,
> - &p.qh->qtd_list,
> - qtd_list) {
> - temp++;
> - switch (0x03 & (hc32_to_cpu(
> - fotg210,
> - qtd->hw_token) >> 8)) {
> - case 0:
> - type = "out";
> - continue;
> - case 1:
> - type = "in";
> - continue;
> - }
> - }
> -
> - temp = scnprintf(next, size,
> - "(%c%d ep%d%s [%d/%d] q%d p%d)",
> - speed_char(scratch),
> - scratch & 0x007f,
> - (scratch >> 8) & 0x000f, type,
> - p.qh->usecs, p.qh->c_usecs,
> - temp,
> - 0x7ff & (scratch >> 16));
> + temp = output_buf_tds_dir(next,
> + fotg210, hw,
> + p.qh, size);
>
> if (seen_count < DBG_SCHED_LIMIT)
> seen[
Re: [PATCH 08/14] RFC: usb/host/fotg210: convert macro to inline function
On Mon, Sep 21, 2015 at 05:01:12PM +0200, Peter Senna Tschudin wrote:
> This patch convert the macro speed_char in an inline function. The goal
> of this patch is to make the code easier to read.
>
> Signed-off-by: Peter Senna Tschudin
looks good
> ---
> drivers/usb/host/fotg210-hcd.c | 27 ---
> 1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 4032ed0..82cd5da 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -331,17 +331,22 @@ struct debug_buffer {
> size_t alloc_size;
> };
>
> -#define speed_char(info1)({ char tmp; \
> - switch (info1 & (3 << 12)) { \
> - case QH_FULL_SPEED: \
> - tmp = 'f'; break; \
> - case QH_LOW_SPEED: \
> - tmp = 'l'; break; \
> - case QH_HIGH_SPEED: \
> - tmp = 'h'; break; \
> - default:\
> - tmp = '?'; break; \
> - } tmp; })
> +static inline char speed_char(u32 scratch)
> +{
> + switch (scratch & (3 << 12)) {
> + case QH_FULL_SPEED:
> + return 'f';
> +
> + case QH_LOW_SPEED:
> + return 'l';
> +
> + case QH_HIGH_SPEED:
> + return 'h';
> +
> + default:
> + return '?';
> + }
> +}
>
> static inline char token_mark(struct fotg210_hcd *fotg210, __hc32 token)
> {
> --
> 2.1.0
>
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 10/14] RFC: usb/host/fotg210: Add function scan_frame_queue()
On Mon, Sep 21, 2015 at 05:01:14PM +0200, Peter Senna Tschudin wrote:
> checkpatch complains about too many leading tabs because the if
> statement starts after 6 tabs:
>
> scan_iosoc() -> for() -> while() -> switch() -> if() -> for() -> if()
>
> There is also a goto statement going backwards in case of failure. This
> patch creates a new inline function named scan_frame_queue() containing
> the last 4 nesting levels, and removes the need of backwards goto,
> making the code easier to read. After the patch it becomes:
>
> scan_iosoc() -> for() -> while() -> scan_frame_queue()
>
> Signed-off-by: Peter Senna Tschudin
> ---
> drivers/usb/host/fotg210-hcd.c | 142
> ++---
> 1 file changed, 76 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/usb/host/fotg210-hcd.c b/drivers/usb/host/fotg210-hcd.c
> index 13cca41..e60a239 100644
> --- a/drivers/usb/host/fotg210-hcd.c
> +++ b/drivers/usb/host/fotg210-hcd.c
> @@ -4645,13 +4645,81 @@ done:
> return status;
> }
>
> -/*-*/
> +static inline int scan_frame_queue(struct fotg210_hcd *fotg210, unsigned
> frame,
> +unsigned now_frame, bool live)
> +{
> + unsigned uf;
> + bool modified;
> + union fotg210_shadow q, *q_p;
> + __hc32 type, *hw_p;
> +
> + /* scan each element in frame's queue for completions */
> + q_p = &fotg210->pshadow[frame];
> + hw_p = &fotg210->periodic[frame];
> + q.ptr = q_p->ptr;
> + type = Q_NEXT_TYPE(fotg210, *hw_p);
> + modified = false;
> +
> + while (q.ptr != NULL) {
while (q.ptr) is enough
> + switch (hc32_to_cpu(fotg210, type)) {
> + case Q_TYPE_ITD:
> + /* If this ITD is still active, leave it for
comment style
> + * later processing ... check the next entry.
> + * No need to check for activity unless the
> + * frame is current.
> + */
> + if (frame == now_frame && live) {
> + rmb();
> + for (uf = 0; uf < 8; uf++) {
> + if (q.itd->hw_transaction[uf] &
> + ITD_ACTIVE(fotg210))
> + break;
> + }
> + if (uf < 8) {
> + q_p = &q.itd->itd_next;
> + hw_p = &q.itd->hw_next;
> + type = Q_NEXT_TYPE(fotg210,
> + q.itd->hw_next);
> + q = *q_p;
> + break;
> + }
> + }
> +
> + /* Take finished ITDs out of the schedule
comment style
> + * and process them: recycle, maybe report
> + * URB completion. HC won't cache the
> + * pointer for much longer, if at all.
> + */
> + *q_p = q.itd->itd_next;
> + *hw_p = q.itd->hw_next;
> + type = Q_NEXT_TYPE(fotg210, q.itd->hw_next);
> + wmb();
> + modified = itd_complete(fotg210, q.itd);
> + q = *q_p;
> + break;
> + default:
> + fotg210_dbg(fotg210, "corrupt type %d frame %d shadow
> %p\n",
> + type, frame, q.ptr);
> + /* FALL THROUGH */
> + case Q_TYPE_QH:
> + case Q_TYPE_FSTN:
> + /* End of the iTDs and siTDs */
> + q.ptr = NULL;
> + break;
> + }
> +
> + /* assume completion callbacks modify the queue */
> + if (unlikely(modified && fotg210->isoc_count > 0))
> + return -1;
can't you add a proper error code here ?
other than these, patch looks good.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 11/14] RFC: usb/host: Rename fotg210-hcd to faraday-hcd
On Mon, Sep 21, 2015 at 05:01:15PM +0200, Peter Senna Tschudin wrote: > This patch renames fotg210-hcd to faraday-hcd as a first step of > consolitating Faraday fotg210 and fusbh200 EHCI-like drivers. > > The patch also updates Kconfig and Makefile. > > Signed-off-by: Peter Senna Tschudin please regenerate the patch with -C -M added to git format-patch -- balbi signature.asc Description: PGP signature
Re: [PATCH 12/14] RFC: usb/host/faraday-hcd: Replace fotg210 by fhcd2xx
On Mon, Sep 21, 2015 at 05:01:16PM +0200, Peter Senna Tschudin wrote: > This patch replaces the strings: > FOTG210 by FHCD2XX > fotg210 by fhcd2xx > > The goal is to remove all references to fotg210 as the driver will > support both fotg210 and fusbh200. > > Signed-off-by: Peter Senna Tschudin after a quick look, it looks okay. Keep in mind I don't have HW. -- balbi signature.asc Description: PGP signature
Re: [PATCH 14/14] RFC: usb/host/faraday-hcd: Import FUSBH200 parameters
On Mon, Sep 21, 2015 at 05:01:18PM +0200, Peter Senna Tschudin wrote: > This patch adds FUSBH200 parameters to faraday-hcd.h. > > Signed-off-by: Peter Senna Tschudin fine by me -- balbi signature.asc Description: PGP signature
Re: [PATCH 13/14] RFC: usb/host/faraday-hcd: Move #defines outside struct
On Mon, Sep 21, 2015 at 05:01:17PM +0200, Peter Senna Tschudin wrote:
> For making the code more readable and to facilitate supporting multiple
> hardware versions, move #defines to outside the struct declaration. This
> patch also renames fhcd2xx_regs to fotg210_regs as this struct is
> specific to fotg210.
>
> Signed-off-by: Peter Senna Tschudin
> ---
> drivers/usb/host/faraday-hcd.h | 137
> ++---
> 1 file changed, 74 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/usb/host/faraday-hcd.h b/drivers/usb/host/faraday-hcd.h
> index cf29edf..f75c467 100644
> --- a/drivers/usb/host/faraday-hcd.h
> +++ b/drivers/usb/host/faraday-hcd.h
> @@ -85,7 +85,7 @@ struct fhcd2xx_hcd {/* one per
> controller */
>
> /* glue to PCI and HCD framework */
> struct fhcd2xx_caps __iomem *caps;
> - struct fhcd2xx_regs __iomem *regs;
> + struct fotg210_regs __iomem *regs;
should this be in previous patch ?
personally, I don't see what's the benefit of this patch, however not against it
either.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb-host: Remove fusbh200 driver
On Fri, Oct 02, 2015 at 01:18:27PM +0200, Peter Senna Tschudin wrote: > fusbh200 and fotg210 are very similar. The initial idea was to consolidate > both drivers but I'm afraid fusbh200 is not being used. > > This patch remove the fusbh200 source code, update Kconfig and two > Makefiles. > > Signed-off-by: Peter Senna Tschudin after all this work on these previous patches, you just remove fusbh200 ? that's a bit odd. Are you sure there are no users for this driver ? It has been in tree since 2013. -- balbi signature.asc Description: PGP signature
Re: Mass Storage Gadget Kthread
On Fri, Oct 02, 2015 at 01:29:05PM -0400, Alan Stern wrote: > On Fri, 2 Oct 2015, Felipe Balbi wrote: > > > Hi Alan, > > > > here's a question which I hope you can help me understand :-) > > > > Why do we have that kthread for the mass storage gadgets ? I noticed a very > > interesting behavior. > > Basically, the mass-storage gadget uses a kthread in order to access > the backing file. > > Obviously a kernel driver doesn't _need_ to use a separate thread to > read or write a file (think of /dev/loop, for example). But doing it > that way is very easy because the driver merely has to imitate read(2) > and write(2) system calls, which have a simple and well defined > interface. To do anything more direct would mean getting deeply > involved in the block and filesystem layers, and that wasn't my goal -- > I wanted to write a USB gadget driver, not re-implement the loop > driver. > > Also, remember that at the time the MS Gadget was written, speed over > USB wasn't a pressing issue. You couldn't transfer more than about 40 > MB/s anyway, so efficiency in the gadget wasn't a terribly high > priority. > > > Basically, the MSC class works in a loop like so: > > > > CBW > > Data Transfer > > CSW > > > > In our implemention, what we do is: > > > > CBW > > wake_up_process() > > Data Transfer > > wake_up_process() > > CSW > > wake_up_process() > > The wake_up_process() call is the notification from the completion > routine telling the kthread that a request has completed. The kthread > doesn't necessarily stop and wait for these notifications; it can > continue processing in parallel. > > > Now here's the interesting bit. Every time we wake_up_process(), we > > basically > > don't do anything until MSC's kthread gets finally scheduled and has a > > chance of > > doing its job. > > That's not entirely true; the kthread may already be running. For > example, the default MS gadget uses two I/O buffers (there's a Kconfig > option to use more if you want). When carrying out a READ, the kthread > fills up the first buffer and submits it to the UDC. But it doesn't > stop and wait for wake_up_process(); instead it goes on to fill up the > second buffer while the first is being sent back to the host. The > wake_up_process() may very well occur while the second buffer is being > filled, in which case it won't do anything (since the kthread isn't > asleep). > > > This means that the host keeps sending us tokens but the UDC > > doesn't have any request queued to start a transfer. This happens specially > > with > > IN endpoints, not so much on OUT directions. See figure one [1] we can see > > that > > host issues over 7 POLLs before UDC has finally started a usb_request, > > sometimes > > this goes for even longer (see image [3]). > > Figure [1] is misleading. The 7 POLLs you see are at the very start of > a READ. It's not surprising that the host can poll 7 times before the > gadget manages to read the first block of data from the backing file. > This isn't a case where the kthread could have been doing something > useful instead of waiting around to hear from the host. > > Figure [3] is difficult to interpret because it doesn't include the > transfer lengths. I can't tell what was going on during the 37 + 50 > POLLs before the first OUT transfer. If that was a CDB starting a new yeah, sorry about that. The 37 + 50 POLLs out is a CBW (31-byte). Before those we had a CSW. > command, I don't see why it would take that long to schedule the > kthread unless the CPU was busy with other tasks. there's nothing else running on this board (other than the normal PID1 and stuff like that). > > On figure two we can see that on this particular session, I had as much as > > 15% > > of the bandwidth wasted on POLLs. With this current setup I'm 34MB/sec and > > with > > the added 15% that would get really close to 40MB/sec. > > So high speed, right? Are the numbers in the figure handshake _counts_ counts > or handshake _times_? A simple NAK doesn't use much bandwidth. Even > if 15% of the handshakes are NAKs, it doesn't mean you're wasting 15% > of the bandwidth. sure it means. Given a uFrame, I can stuff (theoretically) 13 bulk transactions in it. If I have a token (IN/OUT) which gets a NAK, that's one less transaction I can stuff in this uFrame, right ? Note that when I have the 37 POLLs, you can see 8 SOFs. This is just the sniffer SW trying to aggregate SOFs (I should drop that aggregation, it's really annoying). > > So the question is, why do we have to wait for that kthread to get > > scheduled ? > > Why couldn't we skip it completely ? Is there really anything left in there > > that > > couldn't be done from within usb_request->complete() itself ? > > The real answer is the calls to vfs_read() and vfs_write() -- those > have to occur in process context. would a threaded IRQ handler be enough ? Fort he sake of argument, let's assume that all UDC drivers use threaded i
Re: [PATCH 06/14] RFC: usb/host/fotg210: replace msleep by usleep_range
On Fri, 2 Oct 2015, Felipe Balbi wrote: > On Mon, Sep 21, 2015 at 05:01:10PM +0200, Peter Senna Tschudin wrote: > > msleep under 20ms can result in sleeping up to 20ms, which may not be > > intended. Replace msleep(5) by usleep_range(5000, 6000). > > > > Signed-off-by: Peter Senna Tschudin > > good catch. I'd apply this straight away. Alan ? It really doesn't matter. As long as the delay is at least 5 ms, it can be arbitrarily long. This won't hurt, and if it prevents automated tools from complaining then it's worthwhile. Peter, a lot of the changes you have been making will also apply to the ehci-hcd driver. Do you want to update it as well? One caution: The style used for continuation lines is to add two extra tab stops, not to align things with an open paren on the original line. 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] usb: dwc2: gadget: parity fix in isochronous mode
> -Original Message-
> From: John Youn [mailto:john.y...@synopsys.com]
> Sent: September-10-15 6:14 PM
> To: Scott Branden; Roman Bacik; linux-usb@vger.kernel.org
> Cc: bcm-kernel-feedback-list
> Subject: [PATCH] usb: dwc2: gadget: parity fix in isochronous mode
>
> From: Roman Bacik
>
> USB OTG driver in isochronous mode has to set the parity of the receiving
> microframe. The parity is set to even by default. This causes problems for an
> audio gadget, if the host starts transmitting on odd microframes.
>
> This fix uses Incomplete Periodic Transfer interrupt to toggle between even
> and odd parity until the Transfer Complete interrupt is received.
>
> Signed-off-by: Roman Bacik
> Reviewed-by: Abhinav Ratna
> Reviewed-by: Srinath Mannam
> Signed-off-by: Scott Branden
> Signed-off-by: John Youn
> ---
>
> Hi Roman, Scott,
>
> I have updated this patch for slave mode ISOC OUT and rebased against
> latest code.
>
> It shouldn't affect your platform in dma mode but just in case could you try
> it
> and make sure it still works the same?
>
> Regards,
> John
John,
I have verified that your modification still works the same on our platform.
Thank you very much.
Regards,
Roman
>
>
> drivers/usb/dwc2/core.h | 1 +
> drivers/usb/dwc2/gadget.c | 71
> +--
> drivers/usb/dwc2/hw.h | 1 +
> 3 files changed, 65 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
> 3056981..ebf2504 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -166,6 +166,7 @@ struct dwc2_hsotg_ep {
> unsigned intperiodic:1;
> unsigned intisochronous:1;
> unsigned intsend_zlp:1;
> + unsigned inthas_correct_parity:1;
>
> charname[10];
> };
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
> 19202c1c..7e5670c 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -1513,6 +1513,19 @@ static void dwc2_hsotg_ep0_zlp(struct
> dwc2_hsotg *hsotg, bool dir_in)
> dwc2_hsotg_program_zlp(hsotg, hsotg->eps_out[0]); }
>
> +static void dwc2_hsotg_change_ep_iso_parity(struct dwc2_hsotg *hsotg,
> + u32 epctl_reg)
> +{
> + u32 ctrl;
> +
> + ctrl = dwc2_readl(hsotg->regs + epctl_reg);
> + if (ctrl & DXEPCTL_EOFRNUM)
> + ctrl |= DXEPCTL_SETEVENFR;
> + else
> + ctrl |= DXEPCTL_SETODDFR;
> + dwc2_writel(ctrl, hsotg->regs + epctl_reg); }
> +
> /**
> * dwc2_hsotg_handle_outdone - handle receiving OutDone/SetupDone
> from RXFIFO
> * @hsotg: The device instance
> @@ -1583,6 +1596,16 @@ static void dwc2_hsotg_handle_outdone(struct
> dwc2_hsotg *hsotg, int epnum)
> return;
> }
>
> + /*
> + * Slave mode OUT transfers do not go through XferComplete so
> + * adjust the ISOC parity here.
> + */
> + if (!using_dma(hsotg)) {
> + hs_ep->has_correct_parity = 1;
> + if (hs_ep->isochronous && hs_ep->interval == 1)
> + dwc2_hsotg_change_ep_iso_parity(hsotg,
> DOEPCTL(epnum));
> + }
> +
> dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req, result); }
>
> @@ -1955,13 +1978,9 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg
> *hsotg, unsigned int idx,
> ints &= ~DXEPINT_XFERCOMPL;
>
> if (ints & DXEPINT_XFERCOMPL) {
> - if (hs_ep->isochronous && hs_ep->interval == 1) {
> - if (ctrl & DXEPCTL_EOFRNUM)
> - ctrl |= DXEPCTL_SETEVENFR;
> - else
> - ctrl |= DXEPCTL_SETODDFR;
> - dwc2_writel(ctrl, hsotg->regs + epctl_reg);
> - }
> + hs_ep->has_correct_parity = 1;
> + if (hs_ep->isochronous && hs_ep->interval == 1)
> + dwc2_hsotg_change_ep_iso_parity(hsotg,
> epctl_reg);
>
> dev_dbg(hsotg->dev,
> "%s: XferCompl: DxEPCTL=0x%08x,
> DXEPTSIZ=%08x\n", @@ -2321,7 +2340,8 @@ void
> dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
> GINTSTS_GOUTNAKEFF | GINTSTS_GINNAKEFF |
> GINTSTS_USBRST | GINTSTS_RESETDET |
> GINTSTS_ENUMDONE | GINTSTS_OTGINT |
> - GINTSTS_USBSUSP | GINTSTS_WKUPINT;
> + GINTSTS_USBSUSP | GINTSTS_WKUPINT |
> + GINTSTS_INCOMPL_SOIN | GINTSTS_INCOMPL_SOOUT;
>
> if (hsotg->core_params->external_id_pin_ctl <= 0)
> intmsk |= GINTSTS_CONIDSTSCHNG;
> @@ -2582,6 +2602,40 @@ irq_retry:
> dwc2_hsotg_dump(hsotg);
> }
>
> + if (gintsts & GINTSTS_INCOMPL_SOIN) {
> + u32 idx, epctl_reg;
> + struct dwc2_hsotg_ep *hs_ep;
> +
> + dev_dbg(hsotg->dev, "%s: GINTSTS_INCOMPL_SOIN\n",
> __func__);
> + for (idx = 1; idx
[GIT PULL] usb fixes for v4.3-rc4
Hi Greg, here's three more fixes for current -rc. They have been in next for a couple days or so and no build problems have been found. regards The following changes since commit 9ffecb10283508260936b96022d4ee43a7798b4c: Linux 4.3-rc3 (2015-09-27 07:50:08 -0400) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git tags/fixes-for-v4.3-rc4 for you to fetch changes up to f5f6afa85aa82a8ee59072f2d09f2b381f24c871: usb: renesas_usbhs: Add support for R-Car H3 (2015-09-30 11:21:11 -0500) usb: fixes for v4.3-rc4 A memory leak fix for the BCD UDC driver, a build warning fix on Renesas and a new device ID for R-Car H3. Nothing major. Signed-off-by: Felipe Balbi Sudip Mukherjee (1): usb: gadget: bdc: fix memory leak Yoshihiro Shimoda (2): usb: renesas_usbhs: fix build warning if 64-bit architecture usb: renesas_usbhs: Add support for R-Car H3 Documentation/devicetree/bindings/usb/renesas_usbhs.txt | 1 + drivers/usb/gadget/udc/bdc/bdc_ep.c | 4 +++- drivers/usb/renesas_usbhs/common.c | 7 ++- include/linux/usb/renesas_usbhs.h | 2 +- 4 files changed, 11 insertions(+), 3 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 v4 1/5] gadget: Introduce the notifier functions
On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote: > On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote: > > Can you elaborate on what the difficulties are and how punting to > > userspace helps? If anything I'd expect punting to userspace to make > the difficulty was mainly making sure all involved parties talk the same > language. I mean, you plug cable and detect charger (this is usually done by > the > PMIC or by a BQ-type device - probably done at drivers/power_supply). > First difficulty comes right here. Power_supply notifies that we're attached > to > a charger (sometimes it can't differentiate a host/hub charger from a wall > charger), a few ms later you get a notification from the gadget that it got > enumerated with a 500mA configuration. Depending on the order of things, your > load will be configured either to 2A (maximum host/hub charger current) or > 500mA. Yes, this can be mitigated :-) > Focussing on this alone, you can have as much as 4 different subsystems > involved, all throwing raw_notifiers at each other. Now each of these > subsystems > need to maintain their own state machine about what notification we have > received and what are the valid next states. OK, so part of the goal here was to outsource all the state machines to some generic code - what you're describing here is definitely a problem and the idea was to provide a central place that has the logic and makes the decisions about what we should be doing. If we don't have that it's going to get messy. > I would rather have userspace be the single place getting notifications > because > then we solve these details at a single location. No argument on the place, making that place be userspace I'm less sold on. > > Things more difficult, if nothing else it means we need to get whatever > > userspace component deployed in all relevant userspace stacks with > > appropriate configuration in order to do things. > but that will be a thing for the kernel too. We will have to deploy this new > kernel component in all relevant stacks with appropriate configuration in > order > to do things :-) No changes there. Sure, but it's one project which is developed entirely in the open - that's a lot more manageable. > > We do punt a lot of configuration to userspace for audio because there > > are substantial device specific policy elements in the configuration and > > it's a far from painless experience getting the code and configuration > > deployed where people need it, definitely a not great for users. > it's the same for this situation. We will have a ton of device specific > configuration, specially with power delivery class, billboard class, the > alternate modes and so on. If we already do this part in userland, adding all > those extras will be, IMO, far simpler. Can you elaborate a bit? As far as I can tell all those are still in the generic USB space rather than the sort of device specifics I'm thinking of. The things we've got with audio are a combination of tuning to taste and (even more importantly) very different ideas about what you want to do with an audio subsystem that influence how you set things up in a given use case. > > I think that where we can do it the bit where work out how the various > > components are connected and aggregate their functionality together is > > definitely a kernel job. It means that userspace doesn't need to worry > > about implementation details of the particular system and instead gets a > > consistent, standard view of the device (in this case a USB port and how > > much power we can draw from it). > Actually, I was thinking about it in a more granular fashion. Kernel exposes a > charger/power_supply, a few regulators, a UDC view and this single userspace > daemon figures out how to tie those together. That sounds worrying - it means that new hardware support needs supporting in every userspace (it seems inevitable that there will be at least some reimplementations because that's fun) which gets old really fast, and every userspace needs to understand every topology. > The view here isn't really a USB port, it's just a source of power. But how > much > power we can draw from it depends on, possibly, a ton of different chips and > different notifications. Right, yes - it's the power input/output associated with the USB port. The USB port is just the physical thing users can see so it's a good way to label it. That could mean that we ought to move the aggregation bit into the power supply code of course, but then a lot of the decisions are going to be specific to USB. > > For the policy stuff we do want to have userspace be able to control > > things but we need to think about how to do that - for example does the > > entire flow need to be in userspace, or can we just expose settings > > which userspace can manage? > considering the amount of different designs that are already out there and all > the extras that are going to show up due to type-c,
Re: Mass Storage Gadget Kthread
On Fri, 2 Oct 2015, Felipe Balbi wrote: > > Figure [1] is misleading. The 7 POLLs you see are at the very start of > > a READ. It's not surprising that the host can poll 7 times before the > > gadget manages to read the first block of data from the backing file. > > This isn't a case where the kthread could have been doing something > > useful instead of waiting around to hear from the host. > > > > Figure [3] is difficult to interpret because it doesn't include the > > transfer lengths. I can't tell what was going on during the 37 + 50 > > POLLs before the first OUT transfer. If that was a CDB starting a new > > yeah, sorry about that. The 37 + 50 POLLs out is a CBW (31-byte). Before those > we had a CSW. Okay. I don't know why there was such a long delay -- more than 1 ms since there are 11 SOF packets. A context switch doesn't take that long. In any case, the kthread submits the usb_request for the next CBW without waiting for the previous CSW to complete (although it does have to wait for the IN transfer preceding the CSW to complete if you're using only two I/O buffers). Note that in Figure 1, there was no delay between the CSW and the following CBW. How does the throughput change if you increase the num_buffers module parameter? > > > On figure two we can see that on this particular session, I had as much > > > as 15% > > > of the bandwidth wasted on POLLs. With this current setup I'm 34MB/sec > > > and with > > > the added 15% that would get really close to 40MB/sec. > > > > So high speed, right? Are the numbers in the figure handshake _counts_ > > counts > > > or handshake _times_? A simple NAK doesn't use much bandwidth. Even > > if 15% of the handshakes are NAKs, it doesn't mean you're wasting 15% > > of the bandwidth. > > sure it means. Given a uFrame, I can stuff (theoretically) 13 bulk > transactions > in it. 13 512-byte bulk transactions. > If I have a token (IN/OUT) which gets a NAK, that's one less transaction > I can stuff in this uFrame, right ? No, because a NAKed IN transaction doesn't transfer 512 bytes. There's the IN token and the NAK handshake, but no DATAx packet. You can fit several of those in the same uframe with 13 512-byte transfers. In fact, the second-to-last line in Table 5-10 of the USB-2 spec shows that with 13 512-byte transfers, there still are 129 bytes of bus time available in a uframe. A NAKed bulk-OUT transfer would indeed uselessly transfer 512 data bytes. But they (almost) never occur in high-speed connections; instead the controllers exchange PING and NYET packets. > Note that when I have the 37 POLLs, you can see 8 SOFs. This is just the > sniffer > SW trying to aggregate SOFs (I should drop that aggregation, it's really > annoying). Does the software let you do that? Last time I checked the TotalPhase Control Center program, there were some things it would not do at all. Separating out all the components of a split transaction, for example. > > > So the question is, why do we have to wait for that kthread to get > > > scheduled ? > > > Why couldn't we skip it completely ? Is there really anything left in > > > there that > > > couldn't be done from within usb_request->complete() itself ? > > > > The real answer is the calls to vfs_read() and vfs_write() -- those > > have to occur in process context. > > would a threaded IRQ handler be enough ? Fort he sake of argument, let's > assume > that all UDC drivers use threaded irqs and they are properly masking their > IRQs > in in the top half for the bottom half (the IRQ thread) to run. > > This means, unless I'm missing something, that we could switch a chunk of the > gadget framework to mutexes instead of spin locks (not all of it though, but > let's ignore that side for now). In that case, we could safely remove spin > locks > from ->complete() and use mutexes and we then we wouldn't need this extra > kthread at all, right ? Well, for sure you wouldn't need the kthread. It's not clear whether this would be an advantage, though. Now the CPU would have to schedule the bottom-half IRQ thread instead of scheduling the kthread, so the total number of context switches would be the same. In fact, if you're using an SMP system then moving to threaded IRQs is likely to make things worse. A bottom-half handler can run on only one CPU at a time, whereas in the current code the entire IRQ handler can run in parallel with the kthread on a different CPU. In other words, moving to threaded IRQs would serialize all the bottom-half processing in the UDC driver with the work done in the gadget's kthread. 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 v4 1/5] gadget: Introduce the notifier functions
On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote: > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote: > > On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote: > > > > Can you elaborate on what the difficulties are and how punting to > > > userspace helps? If anything I'd expect punting to userspace to make > > > the difficulty was mainly making sure all involved parties talk the same > > language. I mean, you plug cable and detect charger (this is usually done > > by the > > PMIC or by a BQ-type device - probably done at drivers/power_supply). > > > First difficulty comes right here. Power_supply notifies that we're > > attached to > > a charger (sometimes it can't differentiate a host/hub charger from a wall > > charger), a few ms later you get a notification from the gadget that it got > > enumerated with a 500mA configuration. Depending on the order of things, > > your > > load will be configured either to 2A (maximum host/hub charger current) or > > 500mA. Yes, this can be mitigated :-) > > > Focussing on this alone, you can have as much as 4 different subsystems > > involved, all throwing raw_notifiers at each other. Now each of these > > subsystems > > need to maintain their own state machine about what notification we have > > received and what are the valid next states. > > OK, so part of the goal here was to outsource all the state machines to > some generic code - what you're describing here is definitely a problem > and the idea was to provide a central place that has the logic and makes > the decisions about what we should be doing. If we don't have that it's > going to get messy. > > > I would rather have userspace be the single place getting notifications > > because > > then we solve these details at a single location. > > No argument on the place, making that place be userspace I'm less sold > on. fair enough. This would be an entire new entity, though, and users (chargers, battery chips, USB controllers, USB PHYs, etc) should blindly notify this entity and no care about what happened before or what are possible next states. As long as we can guarantee that, then I'd be okay adding this to the kernel. > > > Things more difficult, if nothing else it means we need to get whatever > > > userspace component deployed in all relevant userspace stacks with > > > appropriate configuration in order to do things. > > > but that will be a thing for the kernel too. We will have to deploy this new > > kernel component in all relevant stacks with appropriate configuration in > > order > > to do things :-) No changes there. > > Sure, but it's one project which is developed entirely in the open - > that's a lot more manageable. We can have a fully open source userland daemon too. Much like systemd, bluez, neard, and many, many others. Why wouldn't that be a thing ? > > > We do punt a lot of configuration to userspace for audio because there > > > are substantial device specific policy elements in the configuration and > > > it's a far from painless experience getting the code and configuration > > > deployed where people need it, definitely a not great for users. > > > it's the same for this situation. We will have a ton of device specific > > configuration, specially with power delivery class, billboard class, the > > alternate modes and so on. If we already do this part in userland, adding > > all > > those extras will be, IMO, far simpler. > > Can you elaborate a bit? As far as I can tell all those are still in > the generic USB space rather than the sort of device specifics I'm > thinking of. let's consider the billboard class, for example. With the new Type-C connector, the extra CC pins get added and power delivery messaging goes on top of that. Billboard class gives us the opportunity to send a power delivery request to mux the data pins on the TypeC connector to something completely non-USB. So we could run NATIVE PCIe (single lane though) on top of this. Native DisplayPort, Native Thunderbolt (which the Fruit company announced will switch to type-C, so that will be a thing), native VGA... the list goes on and on. At that point, this is not entirely USB realm anymore :-) Similar applies to power delivery charging profile themselves. Not all profiles are mandatory. Some are optional and that will be completely device/board specific. How an OEM implements that in the PCB is also completely board-specific :-) Some might have a dumb IC hardcoding some messages, some might have something largely more flexible. > The things we've got with audio are a combination of tuning to taste and > (even more importantly) very different ideas about what you want to do > with an audio subsystem that influence how you set things up in a given > use case. the same thing will happen with Type-C and power delivery. There won't be tuning to taste, of course :-) But there will be "very different ideas what what you want to do with" your charging methodology. > >
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On 10/2/2015 7:05 AM, Felipe Balbi wrote:
> HBi,
>
> On Fri, Oct 02, 2015 at 03:09:57AM +, John Youn wrote:
>> On 10/1/2015 7:03 PM, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Fri, Sep 04, 2015 at 07:15:10PM -0700, John Youn wrote:
This patch allows the dwc3 driver to run on the new Synopsys USB 3.1
IP core, albeit in USB 3.0 mode only.
The Synopsys USB 3.1 IP (DWC_usb31) retains mostly the same register
interface and programming model as the existing USB 3.0 controller IP
(DWC_usb3). However, the underlying IP is different and the GSNPSID
and version numbers are different.
The DWC_usb31 version register is actually lower in value than the
full GSNPSID of the DWC_usb3 IP. So if we are on DWC_usb3 IP just
store the lower word of the GSNPSID instead of the full register. Then
adjust the revision constants to match. This will allow existing
revision checks to continue to work when running on the new IP.
>>>
>>> I would rather not touch those constants at all. We can have the driver
>>> set a is_dwc31 flag and use if !is_dwc31 && rev < XYZ for the revision
>>> checks.
>>>
>>> It's more work, but it seems better IMO.
>>
>> I initially did it like that but it got really messy and it would
>> make it harder to port back to stable kernels...
>
> except that this doesn't apply to stable kernels :-) Stable is strictly for
> regression fixes. We _do_ get the odd new device id into stable, but only when
> it's really just a device id. dwc31 requires a bunch of other changes to get
> it
> to work with current driver, it's not only a new device id, right ?
My understanding is that small fixes, new device IDs, and quirks
are some of the things appropriate to submit to stable. I think
everything here tagged for stable is in one of those categories.
This patch is the minimal change required just to get the driver
loaded and running with this new hardware. I would put it in the
same category of change as a new device ID. We just have a new
version ID that needs to be checked against so that the probe
doesn't fail.
We don't plan to do any more than this for older kernels, for
example to add support for USB 3.1 and gen 2 speed.
>
Finally add a documentation note about the revision numbering and
checking with regards to the old and new IP. Because these are
different IPs which will both continue to be supported, feature sets
and revisions checks may not sync-up across future versions.
>>>
>>> and this is why I prefer to have the flag :-) We can run different revision
>>> check methods depending if we're running on dwc3 or dwc31.
>>
>> All of the existing checks apply to both. The mismatch condition
>> will be the exception.
>
> okay. Let's take one example:
>
> if (dwc->revision < DWC3_REVISION_220A) {
> reg |= DWC3_DCFG_SUPERSPEED;
> } else {
> ...
>
> So this is a check that's only valid for DWC3 because DWC3 was the one which
> had
> this bug, not DWC31. In order to skip this for DWC31 we should have something
> like:
>
> if (!dwc->is_dwc31 && dwc->revision < DWC3_REVISION_220A) {
> ...
>
> it looks awful, but this is only the case because SNPS made the revision of
> the
> newer cores lower than the previous ones :-p if you used 0x5534 for
> example,
> we wouldn't have this problem.
>
> Yeah, difficult choice. This is_dwc31 will look awful. How about using bit31
> of the revision as a is_dwc31 flag ? We don't touch the older revisions and
> we're gonna be using a reserved bit as a flag. Then, the revision check would
> look like:
>
> reg = dwc3_readl(dwc->regs, DWC3_VER_NUMBER);
>
> /**
> * NOTICE: we're using bit 31 as a "is dwc3.1" flag. This is really
> * just so dwc31 revisions are always larger than dwc3.
> */
> reg |= DWC3_REVISION_IS_DWC31;
>
I'm good with that.
From now, any check based on a revision (for STARS, workarounds, and
new features) should take into consideration how it applies to both
the 3.1/3.0 IP and make the check accordingly.
Cc: # v3.18+
>>>
>>> I really fail to how any of these patches in this series apply for stable.
>>> Care
>>> to explain ?
>>
>> We have some prototyping products that are stuck on 3.18 stable
>> kernels and will continue to ship with that for some time. We'd
>> like to run the USB 3.1 controller on those platforms. Without
>> these version id and version number updates dwc3 will not work
>> with the USB 3.1 IP.
>>
>> I think the plan is to update those platforms to 4.2 eventually.
>> So even then it will still need this patch.
>>
>> Also it will help out any customers stuck on earlier kernels.
>>
>> How would you advise we handle this, with the version id and
>> number changes?
>
> I have a feeling the answer to that will be that you will need to backport
> your
> own patches :-( Or upgrade to the latest kernel around once your patches get
> merged.
>
> Woul
Re: Mass Storage Gadget Kthread
Hi, On Fri, Oct 02, 2015 at 02:57:54PM -0400, Alan Stern wrote: > On Fri, 2 Oct 2015, Felipe Balbi wrote: > > > > Figure [1] is misleading. The 7 POLLs you see are at the very start of > > > a READ. It's not surprising that the host can poll 7 times before the > > > gadget manages to read the first block of data from the backing file. > > > This isn't a case where the kthread could have been doing something > > > useful instead of waiting around to hear from the host. > > > > > > Figure [3] is difficult to interpret because it doesn't include the > > > transfer lengths. I can't tell what was going on during the 37 + 50 > > > POLLs before the first OUT transfer. If that was a CDB starting a new > > > > yeah, sorry about that. The 37 + 50 POLLs out is a CBW (31-byte). Before > > those > > we had a CSW. > > Okay. I don't know why there was such a long delay -- more than 1 ms > since there are 11 SOF packets. A context switch doesn't take that > long. In any case, the kthread submits the usb_request for the next > CBW without waiting for the previous CSW to complete (although it does > have to wait for the IN transfer preceding the CSW to complete if > you're using only two I/O buffers). > > Note that in Figure 1, there was no delay between the CSW and the > following CBW. > > How does the throughput change if you increase the num_buffers module > parameter? I was using 32, IIRC. I'll try again just to make sure. > > > > On figure two we can see that on this particular session, I had as much > > > > as 15% > > > > of the bandwidth wasted on POLLs. With this current setup I'm 34MB/sec > > > > and with > > > > the added 15% that would get really close to 40MB/sec. > > > > > > So high speed, right? Are the numbers in the figure handshake _counts_ > > > > counts > > > > > or handshake _times_? A simple NAK doesn't use much bandwidth. Even > > > if 15% of the handshakes are NAKs, it doesn't mean you're wasting 15% > > > of the bandwidth. > > > > sure it means. Given a uFrame, I can stuff (theoretically) 13 bulk > > transactions > > in it. > > 13 512-byte bulk transactions. I read it as 13 up to 512-byte bulk transactions, no ? I mean, we can stuff 26 256-byte bulk transactions :-) It's either a full packet or a short-packet which terminates the transfer. > > If I have a token (IN/OUT) which gets a NAK, that's one less transaction > > I can stuff in this uFrame, right ? > > No, because a NAKed IN transaction doesn't transfer 512 bytes. > There's the IN token and the NAK handshake, but no DATAx packet. You > can fit several of those in the same uframe with 13 512-byte transfers. > In fact, the second-to-last line in Table 5-10 of the USB-2 spec shows > that with 13 512-byte transfers, there still are 129 bytes of bus time > available in a uframe. > > A NAKed bulk-OUT transfer would indeed uselessly transfer 512 data > bytes. But they (almost) never occur in high-speed connections; > instead the controllers exchange PING and NYET packets. right. > > Note that when I have the 37 POLLs, you can see 8 SOFs. This is just the > > sniffer > > SW trying to aggregate SOFs (I should drop that aggregation, it's really > > annoying). > > Does the software let you do that? Last time I checked the TotalPhase > Control Center program, there were some things it would not do at all. > Separating out all the components of a split transaction, for example. I'll check. There's a way to tell it you want sequential sniffing, and never aggregate. > > > > So the question is, why do we have to wait for that kthread to get > > > > scheduled ? > > > > Why couldn't we skip it completely ? Is there really anything left in > > > > there that > > > > couldn't be done from within usb_request->complete() itself ? > > > > > > The real answer is the calls to vfs_read() and vfs_write() -- those > > > have to occur in process context. > > > > would a threaded IRQ handler be enough ? Fort he sake of argument, let's > > assume > > that all UDC drivers use threaded irqs and they are properly masking their > > IRQs > > in in the top half for the bottom half (the IRQ thread) to run. > > > > This means, unless I'm missing something, that we could switch a chunk of > > the > > gadget framework to mutexes instead of spin locks (not all of it though, but > > let's ignore that side for now). In that case, we could safely remove spin > > locks > > from ->complete() and use mutexes and we then we wouldn't need this extra > > kthread at all, right ? > > Well, for sure you wouldn't need the kthread. It's not clear whether > this would be an advantage, though. Now the CPU would have to schedule > the bottom-half IRQ thread instead of scheduling the kthread, so the > total number of context switches would be the same. we're already using threaded IRQs in some UDC drivers. > In fact, if you're using an SMP system then moving to threaded IRQs is > likely to make things worse. A bottom-half hand
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
Hi, On Fri, Oct 02, 2015 at 07:16:05PM +, John Youn wrote: > From now, any check based on a revision (for STARS, workarounds, and > new features) should take into consideration how it applies to both > the 3.1/3.0 IP and make the check accordingly. > > Cc: # v3.18+ > >>> > >>> I really fail to how any of these patches in this series apply for > >>> stable. Care > >>> to explain ? > >> > >> We have some prototyping products that are stuck on 3.18 stable > >> kernels and will continue to ship with that for some time. We'd > >> like to run the USB 3.1 controller on those platforms. Without > >> these version id and version number updates dwc3 will not work > >> with the USB 3.1 IP. > >> > >> I think the plan is to update those platforms to 4.2 eventually. > >> So even then it will still need this patch. > >> > >> Also it will help out any customers stuck on earlier kernels. > >> > >> How would you advise we handle this, with the version id and > >> number changes? > > > > I have a feeling the answer to that will be that you will need to backport > > your > > own patches :-( Or upgrade to the latest kernel around once your patches get > > merged. > > > > Would you care to explain why upgrading the kernel is so complex for this > > prototyping solution ? I suppose you're not using HAPS as a PCIe card on a > > common desktop PC, then ? > > I don't know the technical reasons why. One platform is ARM based > and using the 3.18 Linaro stable kernel. Another uses our ARC > platform which is also on 3.18. > > We're trying to avoid the situation where where we have to ship > patches or maintain a separate kernel tree. > > Do you have any objections to these going into stable? I'm not the one you need to convince. That would be Greg :-) -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On 10/1/2015 8:09 PM, John Youn wrote: > On 10/1/2015 7:03 PM, Felipe Balbi wrote: >> Hi, >> >> On Fri, Sep 04, 2015 at 07:15:10PM -0700, John Youn wrote: >>> + >>> +/* DWC_usb31 revisions */ >>> +#define DWC3_USB31_REVISION_110A 0x3131302a >> >> are you sure you tested this ? Above you check for 0x3331 but here you >> use >> 0x3131 ? What gives ? Also, it seems odd that revision 1.10a is actually >> 3.02a >> in HW, is this really correct ? >> > > The one in the source file is wrong. I did run it but not sure > how it was working... maybe wrong bitfile. I'll look into it > and fix it. > > The version value is actually ASCII using all 4 > bytes: "110*". The last 'a' is replaced with '*' in the register > as that indicates a documentation only change with no IP changes. > Correcting myself, the source is right the first time. The reason we check for "3331" in probe is because the 3.1 core uses GSNPSID strictly as an ID register not a version. 3.0 IP: GSNPSID = 0x5533 (ID) + 0x260a (VERSION) 3.1 IP: GSNPSID = "33313130" (ID) VER_NUMBER = "3131302a" (VERSION) Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On Fri, Oct 02, 2015 at 07:47:24PM +, John Youn wrote: > On 10/1/2015 8:09 PM, John Youn wrote: > > On 10/1/2015 7:03 PM, Felipe Balbi wrote: > >> Hi, > >> > >> On Fri, Sep 04, 2015 at 07:15:10PM -0700, John Youn wrote: > >>> + > >>> +/* DWC_usb31 revisions */ > >>> +#define DWC3_USB31_REVISION_110A 0x3131302a > >> > >> are you sure you tested this ? Above you check for 0x3331 but here you > >> use > >> 0x3131 ? What gives ? Also, it seems odd that revision 1.10a is actually > >> 3.02a > >> in HW, is this really correct ? > >> > > > > The one in the source file is wrong. I did run it but not sure > > how it was working... maybe wrong bitfile. I'll look into it > > and fix it. > > > > The version value is actually ASCII using all 4 > > bytes: "110*". The last 'a' is replaced with '*' in the register > > as that indicates a documentation only change with no IP changes. > > > > Correcting myself, the source is right the first time. > > The reason we check for "3331" in probe is because the 3.1 > core uses GSNPSID strictly as an ID register not a version. > > 3.0 IP: > GSNPSID = 0x5533 (ID) + 0x260a (VERSION) > > 3.1 IP: > GSNPSID = "33313130" (ID) > VER_NUMBER = "3131302a" (VERSION) oh all right, thanks for clarifying. :-) -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/3][v4] Documentation: dt: dwc3: Add snps,quirk-frame-length-adjustment property
On Thu, Sep 24, 2015 at 09:42:59AM +, RAJESH BHAGAT wrote: > Hi Felipe, > > Any comments on the below [v4] patches? > > [PATCH 1/3][v4] Documentation: dt: dwc3: Add > snps,quirk-frame-length-adjustment property https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=next&id=3737c54418c35034127bf2837e9b27a3c3c67940 > [PATCH 2/3][v4] drivers: usb: dwc3: Add frame length adjustment quirk https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=next&id=db2be4e9e30c6e43e48c5749d3fc74cee0a6bbb3 > [PATCH 3/3][v4] arm: dts: ls1021a: Add quirk for Erratum A009116 talk to you ARM-SoC maintainer. > I will be taking forward these patches, as Nikhil has left freescale. okay, thanks for letting me know. -- balbi signature.asc Description: PGP signature
Re: USB stops working
It happened again. When I attached my mobile phone, it wasn't recognized. The MXT-USB did work an hour earlier. [42097.629651] scsi 14:0:0:0: Direct-Access MXT-USB Storage Device 1308 PQ: 0 ANSI: 0 CCS [42097.630393] sd 14:0:0:0: [sdb] 251131904 512-byte logical blocks: (128 GB/119 GiB) [42097.630595] sd 14:0:0:0: [sdb] Write Protect is off [42097.630600] sd 14:0:0:0: [sdb] Mode Sense: 03 00 00 00 [42097.630716] sd 14:0:0:0: [sdb] No Caching mode page found [42097.630720] sd 14:0:0:0: [sdb] Assuming drive cache: write through [42097.634637] sdb: sdb1 [42097.635396] sd 14:0:0:0: [sdb] Attached SCSI removable disk [48165.278681] usb 3-3: reset high-speed USB device number 2 using xhci_hcd [48180.768880] usb 3-3: device descriptor read/64, error -110 [48196.355958] usb 3-3: device descriptor read/64, error -110 [48196.619581] usb 3-3: reset high-speed USB device number 2 using xhci_hcd [48212.100098] usb 3-3: device descriptor read/64, error -110 [48227.687135] usb 3-3: device descriptor read/64, error -110 [48227.950756] usb 3-3: reset high-speed USB device number 2 using xhci_hcd [48232.969584] xhci_hcd :00:14.0: Command completion event does not match command [48232.969610] xhci_hcd :00:14.0: Timeout while waiting for setup device command [48238.188632] xhci_hcd :00:14.0: Timeout while waiting for setup device command [48238.392191] usb 3-3: device not accepting address 2, error -62 [48238.552451] usb 3-3: reset high-speed USB device number 2 using xhci_hcd [48243.567989] xhci_hcd :00:14.0: Command completion event does not match command [48243.568038] xhci_hcd :00:14.0: Timeout while waiting for setup device command [48248.786997] xhci_hcd :00:14.0: Timeout while waiting for setup device command [48248.990625] usb 3-3: device not accepting address 2, error -62 [48248.990984] usb 3-3: USB disconnect, device number 2 [48413.364578] INFO: task rtsx_usb_ms_1:465 blocked for more than 120 seconds. [48413.364590] Not tainted 4.2.1-1-ARCH #1 [48413.364594] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [48413.364598] rtsx_usb_ms_1 D 8801af215200 0 465 2 0x [48413.364609] 8801a73abce8 0046 81812500 8800c51c5280 [48413.364617] 8801a73abcc8 8801a73ac000 fffe 8801a73abd88 [48413.364623] 0258 8801a73abd08 8156d2ae [48413.364630] Call Trace: [48413.364648] [] schedule+0x3e/0x90 [48413.364666] [] usb_kill_urb.part.4+0x53/0xa0 [usbcore] [48413.364676] [] ? wake_atomic_t_function+0x60/0x60 [48413.364688] [] usb_kill_urb+0x21/0x30 [usbcore] [48413.364700] [] usb_start_wait_urb+0xe5/0x170 [usbcore] [48413.364708] [] ? del_timer_sync+0x4c/0x60 [48413.364720] [] usb_bulk_msg+0xbd/0x160 [usbcore] [48413.364730] [] rtsx_usb_send_cmd+0x63/0x90 [rtsx_usb] [48413.364737] [] rtsx_usb_read_register+0x6c/0xc0 [rtsx_usb] [48413.364746] [] rtsx_usb_detect_ms_card+0x6b/0x100 [rtsx_usb_ms] [48413.364753] [] ? ms_pull_ctl_enable_lqfp48+0x100/0x100 [rtsx_usb_ms] [48413.364761] [] kthread+0xd8/0xf0 [48413.364770] [] ? kthread_worker_fn+0x170/0x170 [48413.364776] [] ret_from_fork+0x3f/0x70 [48413.364783] [] ? kthread_worker_fn+0x170/0x170 [48413.364856] INFO: task kworker/u8:0:19452 blocked for more than 120 seconds. [48413.364860] Not tainted 4.2.1-1-ARCH #1 [48413.364862] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [48413.364866] kworker/u8:0D 8801af295200 0 19452 2 0x [48413.364884] Workqueue: kmmcd mmc_rescan [mmc_core] [48413.364888] 880174c3fcc8 0046 8801a79f9b80 8800ab3cb700 [48413.364894] 8801af295200 880174c4 8801a666cb0c 8800ab3cb700 [48413.364900] 8801a666cb10 880174c3fce8 8156d2ae [48413.364906] Call Trace: [48413.364913] [] schedule+0x3e/0x90 [48413.364920] [] schedule_preempt_disabled+0x15/0x20 [48413.364927] [] __mutex_lock_slowpath+0xca/0x140 [48413.364934] [] mutex_lock+0x1b/0x30 [48413.364942] [] sdmmc_get_cd+0x49/0xb0 [rtsx_usb_sdmmc] [48413.364952] [] mmc_rescan+0x1f6/0x310 [mmc_core] [48413.364960] [] process_one_work+0x14b/0x440 [48413.364966] [] worker_thread+0x48/0x4a0 [48413.364972] [] ? process_one_work+0x440/0x440 [48413.364979] [] kthread+0xd8/0xf0 [48413.364987] [] ? kthread_worker_fn+0x170/0x170 [48413.364992] [] ret_from_fork+0x3f/0x70 [48413.364999] [] ? kthread_worker_fn+0x170/0x170 [48413.365006] INFO: task kworker/0:1:20096 blocked for more than 120 seconds. [48413.365009] Not tainted 4.2.1-1-ARCH #1 [48413.365012] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [48413.365015] kworker/0:1 D 8801af215200 0 20096 2 0x [48413.365030] Workqueue: usb_hub_wq hub_event [usbcore] [48413.365034] 8801322b39f8 0046 81812500 8800afae0dc0 [48413.365040] 8801322b39d8 8801322b4
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On Fri, Oct 02, 2015 at 02:21:01PM -0500, Felipe Balbi wrote: > > We're trying to avoid the situation where where we have to ship > > patches or maintain a separate kernel tree. > > > > Do you have any objections to these going into stable? > > I'm not the one you need to convince. That would be Greg :-) What's the git commit id of the patch in Linus's tree? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On Sat, Oct 03, 2015 at 12:02:54AM +0200, Greg KH wrote: > On Fri, Oct 02, 2015 at 02:21:01PM -0500, Felipe Balbi wrote: > > > We're trying to avoid the situation where where we have to ship > > > patches or maintain a separate kernel tree. > > > > > > Do you have any objections to these going into stable? > > > > I'm not the one you need to convince. That would be Greg :-) > > What's the git commit id of the patch in Linus's tree? the patch doesn't exist yet. John sent a series wich I don't feel it fits stable. The new device IDs, sure, but all the other changes which, granted, are related, but don't seem like stable material IMO. For reference, here are all the patches: http://www.spinics.net/lists/linux-usb/msg130712.html http://www.spinics.net/lists/linux-usb/msg130711.html http://www.spinics.net/lists/linux-usb/msg130717.html http://www.spinics.net/lists/linux-usb/msg130713.html http://www.spinics.net/lists/linux-usb/msg130716.html http://www.spinics.net/lists/linux-usb/msg130714.html -- balbi signature.asc Description: PGP signature
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On 10/2/2015 5:33 PM, Felipe Balbi wrote: > On Sat, Oct 03, 2015 at 12:02:54AM +0200, Greg KH wrote: >> On Fri, Oct 02, 2015 at 02:21:01PM -0500, Felipe Balbi wrote: We're trying to avoid the situation where where we have to ship patches or maintain a separate kernel tree. Do you have any objections to these going into stable? >>> >>> I'm not the one you need to convince. That would be Greg :-) >> >> What's the git commit id of the patch in Linus's tree? > > the patch doesn't exist yet. John sent a series wich I don't feel it fits > stable. The new device IDs, sure, but all the other changes which, granted, > are > related, but don't seem like stable material IMO. > > For reference, here are all the patches: > > http://www.spinics.net/lists/linux-usb/msg130712.html > http://www.spinics.net/lists/linux-usb/msg130711.html > http://www.spinics.net/lists/linux-usb/msg130717.html > http://www.spinics.net/lists/linux-usb/msg130713.html > http://www.spinics.net/lists/linux-usb/msg130716.html > http://www.spinics.net/lists/linux-usb/msg130714.html > Hi Greg, I'm in the process of simplifying and resubmitting these. But to summarize, these are all small changes to get newer hardware to work with older drivers and to address failures with certain hardware for compliance testing. Patch 1 allows the dwc3 driver to work with newer hardware. An ID register in the hardware has changed causing the probe to fail. And we ensure the new version numbers are higher than older versions. Patch 2-3 adds PCI Device IDs so that dwc3 will load with various versions of our hardware. Patch 4 adds the correct driver parameters for our hardware. Patch 5 adds a quirk for a problem with some PHYs. Patch 6 is not tagged for stable (formatting only). Are these kind of changes appropriate for stable? The reason we want them in stable is because we have some products that are stuck on 3.18 currently and later will be updated to 4.2. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] usb: dwc3: Support Synopsys USB 3.1 IP
On Sat, Oct 03, 2015 at 01:14:27AM +, John Youn wrote: > On 10/2/2015 5:33 PM, Felipe Balbi wrote: > > On Sat, Oct 03, 2015 at 12:02:54AM +0200, Greg KH wrote: > >> On Fri, Oct 02, 2015 at 02:21:01PM -0500, Felipe Balbi wrote: > We're trying to avoid the situation where where we have to ship > patches or maintain a separate kernel tree. > > Do you have any objections to these going into stable? > >>> > >>> I'm not the one you need to convince. That would be Greg :-) > >> > >> What's the git commit id of the patch in Linus's tree? > > > > the patch doesn't exist yet. John sent a series wich I don't feel it fits > > stable. The new device IDs, sure, but all the other changes which, granted, > > are > > related, but don't seem like stable material IMO. > > > > For reference, here are all the patches: > > > > http://www.spinics.net/lists/linux-usb/msg130712.html > > http://www.spinics.net/lists/linux-usb/msg130711.html > > http://www.spinics.net/lists/linux-usb/msg130717.html > > http://www.spinics.net/lists/linux-usb/msg130713.html > > http://www.spinics.net/lists/linux-usb/msg130716.html > > http://www.spinics.net/lists/linux-usb/msg130714.html > > > > Hi Greg, > > I'm in the process of simplifying and resubmitting these. > > But to summarize, these are all small changes to get newer > hardware to work with older drivers and to address failures > with certain hardware for compliance testing. > > Patch 1 allows the dwc3 driver to work with newer hardware. An ID > register in the hardware has changed causing the probe to > fail. And we ensure the new version numbers are higher than > older versions. > > Patch 2-3 adds PCI Device IDs so that dwc3 will load with various > versions of our hardware. > > Patch 4 adds the correct driver parameters for our hardware. > > Patch 5 adds a quirk for a problem with some PHYs. > > Patch 6 is not tagged for stable (formatting only). > > Are these kind of changes appropriate for stable? Read Documentation/stable_kernel_rules.txt and you tell me. I'm not going to dig up the web links at the moment, sorry, as I have over 400 patches in my queue to work on to be applied to the stable trees that are already in Linus's tree. Let me work on those now, instead of hypothetical ones that have not been accepted yet :) > The reason we want them in stable is because we have some > products that are stuck on 3.18 currently and later will be > updated to 4.2. Then submit them properly, and when they are in Linus's tree, I'll get notified of them and if I think they are not acceptable, I'll let you know. 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
