Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command

[Mathias Nyman]

On 01.12.2016 06:54, Baolin Wang wrote:

On 30 November 2016 at 22:09, Mathias Nyman
 wrote:

On 30.11.2016 11:02, Baolin Wang wrote:


If the hardware never responds to the stop endpoint command, the
URBs will never be completed, and we might hang the USB subsystem.
The original watchdog timer is used to watch if one stop endpoint
command is timeout, if timeout, then the watchdog timer will set
XHCI_STATE_DYING, try to halt the xHCI host, and give back all
pending URBs.

But now we already have one command timer to control command timeout,
thus we can also use the command timer to watch the stop endpoint
command, instead of one duplicate watchdog timer which need to be
removed.

Meanwhile we don't need the 'stop_cmds_pending' flag to identy if
this is the last stop endpoint command of one endpoint. Since we
can make sure we only set one stop endpoint command for one endpoint
by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove
this flag.

We also need to clean up the command queue before trying to halt the
xHCI host in xhci_stop_endpoint_command_timeout() function.



This isn't a bad idea.

There are anyway some corner cases and details that need to be
checked, such as suspend (which will clear the command queue), module unload
and abrupt host removal (like pci hotplug removal of host controller)
we need to make sure we can trust the command timer to always return the
canceled URB


Yes, you are right, we need to check these carefully.

Suspend process, module unload and abrupt host removal, they all will
issue usb_disconnect() firstly before clear the command queue, it will
check URBs for every endpoint by
usb_disconnect()--->usb_disable_device()--->usb_disable_endpoint(),
which will make sure every URBs of endpoints will be cancelled by the
stop endpoint command responding or the timeout function of stop
endpoint command (xhci_stop_endpoint_command_timeout()) in
usb_hcd_flush_endpoint(). From that point, we can make sure the
command timer will be useful to handle stop endpoint command timeout.
Please correct me if I said something wrong. Thanks.



This relies on current queued command that times out to be the stop endpoint 
command.

If host partially, or completely hangs there might be any number of commands in 
the
command queue, and we would need to wait for each one of them to timeout, finish
before we finally get to the stop endpoint command, and give back the urb.

I think it would be better to first fix the issues with the current watchdog 
function, get
those fixes into stable, and then think about moving to the command queue timer.

In short, this patch doesn't currently fix any existing issue, but might cause 
the
timeout to be more unreliable

-Mathias   
 
--

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

[PATCH 00/25] usb: host: xhci: cleanup series

[Felipe Balbi]

hi Mathias,

here's a much longer series of cleanups which I have been working on for
the past few days. Let me know what you think about it.

I did some light tests SKL and everything still works as before. I know
you have some reservations about my changes to trb_in_td() but IMHO, if
we can assume this function to be always correct, there's no need to add
debugging messages to it and as for the trb_in_td() call which existed
only for debugging purposes, I guess we need to find a better way of
adding debug statements for that part of the code.

Let me know what you think.

cheers

Felipe Balbi (25):
  usb: host: xhci: dynamically allocate devs array
  usb: host: xhci: handle COMP_STOP from SETUP phase too
  usb: host: xhci: change pre-increments to post-increments
  usb: host: xhci: print HCIVERSION on debug
  usb: host: xhci: rename completion codes to match spec
  usb: host: xhci: WARN on unexpected COMP_SUCCESS
  usb: host: xhci: WARN() if we interrupt without event_ring
  usb: host: xhci: simplify irq handler return
  usb: host: xhci: clear only STS_EINT
  usb: host: xhci: remove unneded semicolon
  usb: host: xhci: use slightly better list helpers
  usb: host: xhci: major rewrite of process_ctrl_td()
  usb: host: xhci: major rewrite of process_bulk_intr_td()
  usb: host: xhci: cleanup finish_td()
  usb: host: xhci: reorder variable definitions
  usb: host: xhci: introduce xhci_td_cleanup()
  usb: host: xhci: remove bogus __releases()/__acquires() annotation
  usb: host: xhci: check for a valid ring when unmapping bounce buffer
  usb: host: xhci: unconditionally call xhci_unmap_td_bounce_buffer()
  usb: host: xhci: don't try to mask critical errors
  usb: host: xhci: remove debug argument from trb_in_td()
  usb: host: xhci: remove unnecessary list_for_each_entry_safe()
  usb: host: xhci: introduce helper to convert a single TRB in no-op
  usb: host: xhci: use trb_to_noop() from xhci_handle_stopped_cmd_ring()
  usb: xhci: host: simplify implementation of trb_in_td()

 drivers/usb/host/xhci-dbg.c  |  22 +-
 drivers/usb/host/xhci-ext-caps.h |   2 +-
 drivers/usb/host/xhci-hub.c  |   5 +-
 drivers/usb/host/xhci-mem.c  | 171 +--
 drivers/usb/host/xhci-ring.c | 595 ---
 drivers/usb/host/xhci.c  |  81 +++---
 drivers/usb/host/xhci.h  | 113 +++-
 7 files changed, 354 insertions(+), 635 deletions(-)

-- 
2.10.1

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

[PATCH 03/25] usb: host: xhci: change pre-increments to post-increments

[Felipe Balbi]

This is a cleanup patch only, no functional changes. The idea is just to
make sure for loops look the same all over the driver.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-dbg.c | 20 ++--
 drivers/usb/host/xhci-mem.c | 10 +-
 drivers/usb/host/xhci.c | 14 +++---
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index 74c42f722678..a3b67f33d4d8 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -177,7 +177,7 @@ static void xhci_print_ports(struct xhci_hcd *xhci)
ports = HCS_MAX_PORTS(xhci->hcs_params1);
addr = >op_regs->port_status_base;
for (i = 0; i < ports; i++) {
-   for (j = 0; j < NUM_PORT_REGS; ++j) {
+   for (j = 0; j < NUM_PORT_REGS; j++) {
xhci_dbg(xhci, "%p port %s reg = 0x%x\n",
addr, names[j],
(unsigned int) readl(addr));
@@ -240,7 +240,7 @@ void xhci_print_run_regs(struct xhci_hcd *xhci)
xhci_dbg(xhci, "  %p: Microframe index = 0x%x\n",
>run_regs->microframe_index,
(unsigned int) temp);
-   for (i = 0; i < 7; ++i) {
+   for (i = 0; i < 7; i++) {
temp = readl(>run_regs->rsvd[i]);
if (temp != XHCI_INIT_VALUE)
xhci_dbg(xhci, "  WARN: %p: Rsvd[%i] = 0x%x\n",
@@ -259,7 +259,7 @@ void xhci_print_registers(struct xhci_hcd *xhci)
 void xhci_print_trb_offsets(struct xhci_hcd *xhci, union xhci_trb *trb)
 {
int i;
-   for (i = 0; i < 4; ++i)
+   for (i = 0; i < 4; i++)
xhci_dbg(xhci, "Offset 0x%x = 0x%x\n",
i*4, trb->generic.field[i]);
 }
@@ -332,7 +332,7 @@ void xhci_debug_segment(struct xhci_hcd *xhci, struct 
xhci_segment *seg)
u64 addr = seg->dma;
union xhci_trb *trb = seg->trbs;
 
-   for (i = 0; i < TRBS_PER_SEGMENT; ++i) {
+   for (i = 0; i < TRBS_PER_SEGMENT; i++) {
trb = >trbs[i];
xhci_dbg(xhci, "@%016llx %08x %08x %08x %08x\n", addr,
 lower_32_bits(le64_to_cpu(trb->link.segment_ptr)),
@@ -413,7 +413,7 @@ void xhci_dbg_erst(struct xhci_hcd *xhci, struct xhci_erst 
*erst)
int i;
struct xhci_erst_entry *entry;
 
-   for (i = 0; i < erst->num_entries; ++i) {
+   for (i = 0; i < erst->num_entries; i++) {
entry = >entries[i];
xhci_dbg(xhci, "@%016llx %08x %08x %08x %08x\n",
 addr,
@@ -440,7 +440,7 @@ void xhci_dbg_cmd_ptrs(struct xhci_hcd *xhci)
 static void dbg_rsvd64(struct xhci_hcd *xhci, u64 *ctx, dma_addr_t dma)
 {
int i;
-   for (i = 0; i < 4; ++i) {
+   for (i = 0; i < 4; i++) {
xhci_dbg(xhci, "@%p (virt) @%08llx "
 "(dma) %#08llx - rsvd64[%d]\n",
 [4 + i], (unsigned long long)dma,
@@ -496,7 +496,7 @@ static void xhci_dbg_slot_ctx(struct xhci_hcd *xhci, struct 
xhci_container_ctx *
_ctx->dev_state,
(unsigned long long)dma, slot_ctx->dev_state);
dma += field_size;
-   for (i = 0; i < 4; ++i) {
+   for (i = 0; i < 4; i++) {
xhci_dbg(xhci, "@%p (virt) @%08llx (dma) %#08x - rsvd[%d]\n",
_ctx->reserved[i], (unsigned long long)dma,
slot_ctx->reserved[i], i);
@@ -519,7 +519,7 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
 
if (last_ep < 31)
last_ep_ctx = last_ep + 1;
-   for (i = 0; i < last_ep_ctx; ++i) {
+   for (i = 0; i < last_ep_ctx; i++) {
unsigned int epaddr = xhci_get_endpoint_address(i);
struct xhci_ep_ctx *ep_ctx = xhci_get_ep_ctx(xhci, ctx, i);
dma_addr_t dma = ctx->dma +
@@ -544,7 +544,7 @@ static void xhci_dbg_ep_ctx(struct xhci_hcd *xhci,
_ctx->tx_info,
(unsigned long long)dma, ep_ctx->tx_info);
dma += field_size;
-   for (j = 0; j < 3; ++j) {
+   for (j = 0; j < 3; j++) {
xhci_dbg(xhci, "@%p (virt) @%08llx (dma) %#08x - 
rsvd[%d]\n",
_ctx->reserved[j],
(unsigned long long)dma,
@@ -583,7 +583,7 @@ void xhci_dbg_ctx(struct xhci_hcd *xhci,
 _ctx->add_flags, (unsigned long long)dma,
 ctrl_ctx->add_flags);
dma += field_size;
-   for (i = 0; i < 6; ++i) {
+   for (i = 0; i < 6; i++) {
xhci_dbg(xhci, "@%p (virt) @%08llx (dma) %#08x - 
rsvd2[%d]\n",
 _ctx->rsvd2[i], (unsigned 

[PATCH 02/25] usb: host: xhci: handle COMP_STOP from SETUP phase too

[Felipe Balbi]

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

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

stop_endpoint(ep);

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

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

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

Signed-off-by: Felipe Balbi 

---

changes since v1


  o handle multi-trb Data Stage
---
 drivers/usb/host/xhci-ring.c | 34 ++
 1 file changed, 22 insertions(+), 12 deletions(-)

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

--

Re: [PATCH v2] USB: OHCI: ohci-s3c2410: remove useless functions

[Alan Stern]

On Thu, 1 Dec 2016 csmanjuvi...@gmail.com wrote:

> From: Manjunath Goudar 
> 
> The ohci_hcd_s3c2410_drv_probe and ohci_hcd_s3c2410_drv_remove
> functions are removed as these are useless functions except calling
> usb_hcd_s3c2410_probe and usb_hcd_s3c2410_remove functions.
> 
> The usb_hcd_s3c2410_probe function renamed to ohci_hcd_s3c2410_drv_probe
> and usb_hcd_s3c2410_remove function renamed to ohci_hcd_s3c2410_drv_remove
> for proper naming.
> 
> Signed-off-by: Manjunath Goudar 
> Cc: Kukjin Kim 
> Cc: Krzysztof Kozlowski 
> Cc: Javier Martinez Canillas 
> Cc: Alan Stern 
> Cc: Greg Kroah-Hartman 
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> 
> ---
> Changelog v1 -> v2:
> Removed checkpatch.pl warnings and errors cleanup code which is not related
> to this patch.

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

[PATCH 0/1] USB: serial: option: add support for Telit LE922A PIDs 0x1040, 0x1041

[Daniele Palmas]

This patch adds support for PIDs 0x1040, 0x1041 of Telit LE922A.

Since the interface positions are the same than the ones used
for other Telit compositions, previous defined blacklists are used.

Following verbose lsusb output of the two compositions:

Bus 003 Device 006: ID 1bc7:1040 Telit Wireless Solutions 
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 (Defined at Interface level)
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x1bc7 Telit Wireless Solutions
  idProduct  0x1040 
  bcdDevice3.10
  iManufacturer   1 Android
  iProduct2 Android
  iSerial 3 359fb2
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength  281
bNumInterfaces  7
bConfigurationValue 1
iConfiguration  0 
bmAttributes 0x80
  (Bus Powered)
MaxPower  500mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass   255 Vendor Specific Class
  bInterfaceSubClass255 Vendor Specific Subclass
  bInterfaceProtocol255 Vendor Specific Protocol
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01  EP 1 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber1
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass   255 Vendor Specific Class
  bInterfaceSubClass 66 
  bInterfaceProtocol  1 
  iInterface  4 ADB Interface
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02  EP 2 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82  EP 2 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber2
  bAlternateSetting   0
  bNumEndpoints   3
  bInterfaceClass   255 Vendor Specific Class
  bInterfaceSubClass255 Vendor Specific Subclass
  bInterfaceProtocol255 Vendor Specific Protocol
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84  EP 4 IN
bmAttributes3
  Transfer TypeInterrupt
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0008  1x 8 bytes
bInterval   9
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83  EP 3 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x03  EP 3 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0200  1x 512 bytes
bInterval   0
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber3
  bAlternateSetting   0

[PATCH 1/1] USB: serial: option: add support for Telit LE922A PIDs 0x1040, 0x1041

[Daniele Palmas]

This patch adds support for PIDs 0x1040, 0x1041 of Telit LE922A.

Since the interface positions are the same than the ones used
for other Telit compositions, previous defined blacklists are used.

Signed-off-by: Daniele Palmas 
---
 drivers/usb/serial/option.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 9894e34..33ae203 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -268,6 +268,8 @@ static void option_instat_callback(struct urb *urb);
 #define TELIT_PRODUCT_CC864_SINGLE 0x1006
 #define TELIT_PRODUCT_DE910_DUAL   0x1010
 #define TELIT_PRODUCT_UE910_V2 0x1012
+#define TELIT_PRODUCT_LE922_USBCFG10x1040
+#define TELIT_PRODUCT_LE922_USBCFG20x1041
 #define TELIT_PRODUCT_LE922_USBCFG00x1042
 #define TELIT_PRODUCT_LE922_USBCFG30x1043
 #define TELIT_PRODUCT_LE922_USBCFG50x1045
@@ -1210,6 +1212,10 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(TELIT_VENDOR_ID, TELIT_PRODUCT_UE910_V2) },
{ USB_DEVICE(TELIT_VENDOR_ID, TELIT_PRODUCT_LE922_USBCFG0),
.driver_info = (kernel_ulong_t)_le922_blacklist_usbcfg0 },
+   { USB_DEVICE(TELIT_VENDOR_ID, TELIT_PRODUCT_LE922_USBCFG1),
+   .driver_info = (kernel_ulong_t)_le910_blacklist },
+   { USB_DEVICE(TELIT_VENDOR_ID, TELIT_PRODUCT_LE922_USBCFG2),
+   .driver_info = (kernel_ulong_t)_le922_blacklist_usbcfg3 },
{ USB_DEVICE(TELIT_VENDOR_ID, TELIT_PRODUCT_LE922_USBCFG3),
.driver_info = (kernel_ulong_t)_le922_blacklist_usbcfg3 },
{ USB_DEVICE_INTERFACE_CLASS(TELIT_VENDOR_ID, 
TELIT_PRODUCT_LE922_USBCFG5, 0xff),
-- 
2.7.4

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

[PATCH 23/25] usb: host: xhci: introduce helper to convert a single TRB in no-op

[Felipe Balbi]

this will be used in several places to convert a single TRB into
No-Op. No functional changes in this patch.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ebb52ffab805..80fa3dbdbdd8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -523,6 +523,22 @@ void xhci_find_new_dequeue_state(struct xhci_hcd *xhci,
(unsigned long long) addr);
 }
 
+static void trb_to_noop(union xhci_trb *trb)
+{
+   if (trb_is_link(trb)) {
+   /* unchain chained link TRBs */
+   trb->link.control &= cpu_to_le32(~TRB_CHAIN);
+   } else {
+   trb->generic.field[0] = 0;
+   trb->generic.field[1] = 0;
+   trb->generic.field[2] = 0;
+   /* Preserve only the cycle bit of this TRB */
+   trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE);
+   trb->generic.field[3] |= cpu_to_le32(
+   TRB_TYPE(TRB_TR_NOOP));
+   }
+}
+
 /* flip_cycle means flip the cycle bit of all but the first and last TRB.
  * (The last TRB actually points to the ring enqueue pointer, which is not part
  * of this TD.)  This is used to remove partially enqueued isoc TDs from a 
ring.
@@ -534,18 +550,8 @@ static void td_to_noop(struct xhci_hcd *xhci, struct 
xhci_ring *ep_ring,
union xhci_trb *trb = td->first_trb;
 
while (1) {
-   if (trb_is_link(trb)) {
-   /* unchain chained link TRBs */
-   trb->link.control &= cpu_to_le32(~TRB_CHAIN);
-   } else {
-   trb->generic.field[0] = 0;
-   trb->generic.field[1] = 0;
-   trb->generic.field[2] = 0;
-   /* Preserve only the cycle bit of this TRB */
-   trb->generic.field[3] &= cpu_to_le32(TRB_CYCLE);
-   trb->generic.field[3] |= cpu_to_le32(
-   TRB_TYPE(TRB_TR_NOOP));
-   }
+   trb_to_noop(trb);
+
/* flip cycle if asked to */
if (flip_cycle && trb != td->first_trb && trb != td->last_trb)
trb->generic.field[3] ^= cpu_to_le32(TRB_CYCLE);
-- 
2.10.1

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

[RFC/PATCH 25/25] usb: xhci: host: simplify implementation of trb_in_td()

[Felipe Balbi]

trb_in_td() has the task of checking whether a given TRB belongs to a
given TD. Currently implementation is far too complex and used wrongly
in some places.

Let's simplify the implementation to the point that we can assume it to
be working always, this means we can remove xhci_test_trb_in_td() and
xhci_check_trb_in_td_math() because we can assume those will always
work.

While at that, also remove two calls for trb_in_td() which existed for
the sole purpose of printing out debugging messages!!

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-mem.c  | 160 ---
 drivers/usb/host/xhci-ring.c |  67 --
 drivers/usb/host/xhci.h  |   5 +-
 3 files changed, 15 insertions(+), 217 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index e5ead94847e1..9ec426d2ed10 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1890,163 +1890,6 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
xhci->bus_state[1].bus_suspended = 0;
 }
 
-static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
-   struct xhci_segment *input_seg,
-   union xhci_trb *start_trb,
-   union xhci_trb *end_trb,
-   dma_addr_t input_dma,
-   struct xhci_segment *result_seg,
-   char *test_name, int test_number)
-{
-   unsigned long long start_dma;
-   unsigned long long end_dma;
-   struct xhci_segment *seg;
-
-   start_dma = xhci_trb_virt_to_dma(input_seg, start_trb);
-   end_dma = xhci_trb_virt_to_dma(input_seg, end_trb);
-
-   seg = trb_in_td(xhci, input_seg, start_trb, end_trb, input_dma);
-   if (seg != result_seg) {
-   xhci_warn(xhci, "WARN: %s TRB math test %d failed!\n",
-   test_name, test_number);
-   xhci_warn(xhci, "Tested TRB math w/ seg %p and "
-   "input DMA 0x%llx\n",
-   input_seg,
-   (unsigned long long) input_dma);
-   xhci_warn(xhci, "starting TRB %p (0x%llx DMA), "
-   "ending TRB %p (0x%llx DMA)\n",
-   start_trb, start_dma,
-   end_trb, end_dma);
-   xhci_warn(xhci, "Expected seg %p, got seg %p\n",
-   result_seg, seg);
-   trb_in_td(xhci, input_seg, start_trb, end_trb, input_dma);
-   return -1;
-   }
-   return 0;
-}
-
-/* TRB math checks for xhci_trb_in_td(), using the command and event rings. */
-static int xhci_check_trb_in_td_math(struct xhci_hcd *xhci)
-{
-   struct {
-   dma_addr_t  input_dma;
-   struct xhci_segment *result_seg;
-   } simple_test_vector [] = {
-   /* A zeroed DMA field should fail */
-   { 0, NULL },
-   /* One TRB before the ring start should fail */
-   { xhci->event_ring->first_seg->dma - 16, NULL },
-   /* One byte before the ring start should fail */
-   { xhci->event_ring->first_seg->dma - 1, NULL },
-   /* Starting TRB should succeed */
-   { xhci->event_ring->first_seg->dma, xhci->event_ring->first_seg 
},
-   /* Ending TRB should succeed */
-   { xhci->event_ring->first_seg->dma + (TRBS_PER_SEGMENT - 1)*16,
-   xhci->event_ring->first_seg },
-   /* One byte after the ring end should fail */
-   { xhci->event_ring->first_seg->dma + (TRBS_PER_SEGMENT - 1)*16 
+ 1, NULL },
-   /* One TRB after the ring end should fail */
-   { xhci->event_ring->first_seg->dma + (TRBS_PER_SEGMENT)*16, 
NULL },
-   /* An address of all ones should fail */
-   { (dma_addr_t) (~0), NULL },
-   };
-   struct {
-   struct xhci_segment *input_seg;
-   union xhci_trb  *start_trb;
-   union xhci_trb  *end_trb;
-   dma_addr_t  input_dma;
-   struct xhci_segment *result_seg;
-   } complex_test_vector [] = {
-   /* Test feeding a valid DMA address from a different ring */
-   {   .input_seg = xhci->event_ring->first_seg,
-   .start_trb = xhci->event_ring->first_seg->trbs,
-   .end_trb = 
>event_ring->first_seg->trbs[TRBS_PER_SEGMENT - 1],
-   .input_dma = xhci->cmd_ring->first_seg->dma,
-   .result_seg = NULL,
-   },
-   /* Test feeding a valid end TRB from a different ring */
-   {   .input_seg = xhci->event_ring->first_seg,
-   .start_trb = xhci->event_ring->first_seg->trbs,
-   .end_trb = 

[PATCH 15/25] usb: host: xhci: reorder variable definitions

[Felipe Balbi]

no functional changes. Simple cleanup to make sure variables are ordered
in a 'reverse christmas tree' fashion. While at that, also remove an
obsolete comment which doesn't apply anymore.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f951850eb57f..4744eaa94eb1 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1785,22 +1785,18 @@ int xhci_is_vendor_info_code(struct xhci_hcd *xhci, 
unsigned int trb_comp_code)
return 0;
 }
 
-/*
- * Finish the td processing, remove the td from td list;
- * Return 1 if the urb can be given back.
- */
 static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
union xhci_trb *ep_trb, struct xhci_transfer_event *event,
struct xhci_virt_ep *ep, int *status, bool skip)
 {
struct xhci_virt_device *xdev;
-   struct xhci_ring *ep_ring;
-   unsigned int slot_id;
-   int ep_index;
-   struct urb *urb = NULL;
struct xhci_ep_ctx *ep_ctx;
+   struct xhci_ring *ep_ring;
struct urb_priv *urb_priv;
+   struct urb *urb = NULL;
+   unsigned int slot_id;
u32 trb_comp_code;
+   int ep_index;
 
slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
xdev = xhci->devs[slot_id];
-- 
2.10.1

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

[PATCH 18/25] usb: host: xhci: check for a valid ring when unmapping bounce buffer

[Felipe Balbi]

This way we can remove checks for valid ring from call sites of
xhci_unmap_td_bounce_buffer()

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 64d9fc490f80..397a74a4ea82 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -601,7 +601,7 @@ static void xhci_unmap_td_bounce_buffer(struct xhci_hcd 
*xhci,
struct xhci_segment *seg = td->bounce_seg;
struct urb *urb = td->urb;
 
-   if (!seg || !urb)
+   if (!ring || !seg || !urb)
return;
 
if (usb_urb_dir_out(urb)) {
-- 
2.10.1

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

[PATCH 20/25] usb: host: xhci: don't try to mask critical errors

[Felipe Balbi]

Instead of fixing actual_length and setting status to 0, let's make sure
the error is propagated and we print a big scary stack trace so errors
are reported and we have a chance of fixing them.

While at that, also remove unnecessary initialization of urb variable.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1e1b54d077e2..1fee418b4fb3 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1787,7 +1787,10 @@ static int xhci_td_cleanup(struct xhci_hcd *xhci, struct 
xhci_td *td,
struct xhci_ring *ep_ring, int *status)
 {
struct urb_priv *urb_priv;
-   struct urb *urb = NULL;
+   struct device *dev;
+   struct urb *urb;
+
+   dev = xhci_to_hcd(xhci)->self.controller;
 
/* Clean up the endpoint's TD list */
urb = td->urb;
@@ -1796,17 +1799,10 @@ static int xhci_td_cleanup(struct xhci_hcd *xhci, 
struct xhci_td *td,
/* if a bounce buffer was used to align this td then unmap it */
xhci_unmap_td_bounce_buffer(xhci, ep_ring, td);
 
-   /* Do one last check of the actual transfer length.
-* If the host controller said we transferred more data than the buffer
-* length, urb->actual_length will be a very big number (since it's
-* unsigned).  Play it safe and say we didn't transfer anything.
-*/
-   if (urb->actual_length > urb->transfer_buffer_length) {
-   xhci_warn(xhci, "URB req %u and actual %u transfer length 
mismatch\n",
- urb->transfer_buffer_length, urb->actual_length);
-   urb->actual_length = 0;
-   *status = 0;
-   }
+   dev_WARN_ONCE(dev, urb->actual_length > urb->transfer_buffer_length,
+   "URB transfer length mismatch. Requested %u got %u\n",
+   urb->transfer_buffer_length, urb->actual_length);
+
list_del_init(>td_list);
/* Was this TD slated to be cancelled but completed anyway? */
if (!list_empty(>cancelled_td_list))
-- 
2.10.1

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

[PATCH 24/25] usb: host: xhci: use trb_to_noop() from xhci_handle_stopped_cmd_ring()

[Felipe Balbi]

instead of open coding how to convert a TRB to no-op, let's use our
newly introduced helper.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 80fa3dbdbdd8..973182ee6954 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1218,7 +1218,6 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd 
*xhci,
 struct xhci_command *cur_cmd)
 {
struct xhci_command *cmd;
-   u32 cycle_state;
 
/* Turn all aborted commands in list to no-ops, then restart */
list_for_each_entry(cmd, >cmd_list,
@@ -1231,15 +1230,8 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd 
*xhci,
 
xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
 cmd->command_trb);
-   /* get cycle state from the original cmd trb */
-   cycle_state = le32_to_cpu(
-   cmd->command_trb->generic.field[3]) & TRB_CYCLE;
-   /* modify the command trb to no-op command */
-   cmd->command_trb->generic.field[0] = 0;
-   cmd->command_trb->generic.field[1] = 0;
-   cmd->command_trb->generic.field[2] = 0;
-   cmd->command_trb->generic.field[3] = cpu_to_le32(
-   TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
+
+   trb_to_noop(cmd->command_trb);
 
/*
 * caller waiting for completion is called when command
-- 
2.10.1

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

[PATCH 16/25] usb: host: xhci: introduce xhci_td_cleanup()

[Felipe Balbi]

By extracting xhci_td_cleanup() from finish_td(), code before clearer
and easier to follow.

There are no functional changes with this patch. It's merely a cleanup.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 4744eaa94eb1..f189c49f4b8b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1785,6 +1785,55 @@ int xhci_is_vendor_info_code(struct xhci_hcd *xhci, 
unsigned int trb_comp_code)
return 0;
 }
 
+static int xhci_td_cleanup(struct xhci_hcd *xhci, struct xhci_td *td,
+   struct xhci_ring *ep_ring, int *status)
+{
+   struct urb_priv *urb_priv;
+   struct urb *urb = NULL;
+
+   /* Clean up the endpoint's TD list */
+   urb = td->urb;
+   urb_priv = urb->hcpriv;
+
+   /* if a bounce buffer was used to align this td then unmap it */
+   if (td->bounce_seg)
+   xhci_unmap_td_bounce_buffer(xhci, ep_ring, td);
+
+   /* Do one last check of the actual transfer length.
+* If the host controller said we transferred more data than the buffer
+* length, urb->actual_length will be a very big number (since it's
+* unsigned).  Play it safe and say we didn't transfer anything.
+*/
+   if (urb->actual_length > urb->transfer_buffer_length) {
+   xhci_warn(xhci, "URB req %u and actual %u transfer length 
mismatch\n",
+ urb->transfer_buffer_length, urb->actual_length);
+   urb->actual_length = 0;
+   *status = 0;
+   }
+   list_del_init(>td_list);
+   /* Was this TD slated to be cancelled but completed anyway? */
+   if (!list_empty(>cancelled_td_list))
+   list_del_init(>cancelled_td_list);
+
+   inc_td_cnt(urb);
+   /* Giveback the urb when all the tds are completed */
+   if (last_td_in_urb(td)) {
+   if ((urb->actual_length != urb->transfer_buffer_length &&
+(urb->transfer_flags & URB_SHORT_NOT_OK)) ||
+   (*status != 0 && !usb_endpoint_xfer_isoc(>ep->desc)))
+   xhci_dbg(xhci, "Giveback URB %p, len = %d, expected = 
%d, status = %d\n",
+urb, urb->actual_length,
+urb->transfer_buffer_length, *status);
+
+   /* set isoc urb status to 0 just as EHCI, UHCI, and OHCI */
+   if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS)
+   *status = 0;
+   xhci_giveback_urb_in_irq(xhci, td, *status);
+   }
+
+   return 0;
+}
+
 static int finish_td(struct xhci_hcd *xhci, struct xhci_td *td,
union xhci_trb *ep_trb, struct xhci_transfer_event *event,
struct xhci_virt_ep *ep, int *status, bool skip)
@@ -1792,8 +1841,6 @@ static int finish_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
struct xhci_virt_device *xdev;
struct xhci_ep_ctx *ep_ctx;
struct xhci_ring *ep_ring;
-   struct urb_priv *urb_priv;
-   struct urb *urb = NULL;
unsigned int slot_id;
u32 trb_comp_code;
int ep_index;
@@ -1853,46 +1900,7 @@ static int finish_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
}
 
 td_cleanup:
-   /* Clean up the endpoint's TD list */
-   urb = td->urb;
-   urb_priv = urb->hcpriv;
-
-   /* if a bounce buffer was used to align this td then unmap it */
-   if (td->bounce_seg)
-   xhci_unmap_td_bounce_buffer(xhci, ep_ring, td);
-
-   /* Do one last check of the actual transfer length.
-* If the host controller said we transferred more data than the buffer
-* length, urb->actual_length will be a very big number (since it's
-* unsigned).  Play it safe and say we didn't transfer anything.
-*/
-   if (urb->actual_length > urb->transfer_buffer_length) {
-   xhci_warn(xhci, "URB req %u and actual %u transfer length 
mismatch\n",
- urb->transfer_buffer_length, urb->actual_length);
-   urb->actual_length = 0;
-   *status = 0;
-   }
-   list_del_init(>td_list);
-   /* Was this TD slated to be cancelled but completed anyway? */
-   if (!list_empty(>cancelled_td_list))
-   list_del_init(>cancelled_td_list);
-
-   inc_td_cnt(urb);
-   /* Giveback the urb when all the tds are completed */
-   if (last_td_in_urb(td)) {
-   if ((urb->actual_length != urb->transfer_buffer_length &&
-(urb->transfer_flags & URB_SHORT_NOT_OK)) ||
-   (*status != 0 && !usb_endpoint_xfer_isoc(>ep->desc)))
-   xhci_dbg(xhci, "Giveback URB %p, len = %d, expected = 
%d, status = %d\n",
-urb, 

[PATCH 21/25] usb: host: xhci: remove debug argument from trb_in_td()

[Felipe Balbi]

By just replacing xhci_warn() with xhci_dbg() we can get rid of the
extra argument.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-mem.c  |  5 ++---
 drivers/usb/host/xhci-ring.c | 10 --
 drivers/usb/host/xhci.h  |  2 +-
 3 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index cff1ddfd03fb..e5ead94847e1 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1905,7 +1905,7 @@ static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
start_dma = xhci_trb_virt_to_dma(input_seg, start_trb);
end_dma = xhci_trb_virt_to_dma(input_seg, end_trb);
 
-   seg = trb_in_td(xhci, input_seg, start_trb, end_trb, input_dma, false);
+   seg = trb_in_td(xhci, input_seg, start_trb, end_trb, input_dma);
if (seg != result_seg) {
xhci_warn(xhci, "WARN: %s TRB math test %d failed!\n",
test_name, test_number);
@@ -1919,8 +1919,7 @@ static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
end_trb, end_dma);
xhci_warn(xhci, "Expected seg %p, got seg %p\n",
result_seg, seg);
-   trb_in_td(xhci, input_seg, start_trb, end_trb, input_dma,
- true);
+   trb_in_td(xhci, input_seg, start_trb, end_trb, input_dma);
return -1;
}
return 0;
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 1fee418b4fb3..50244dee6b43 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1690,8 +1690,7 @@ struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
struct xhci_segment *start_seg,
union xhci_trb  *start_trb,
union xhci_trb  *end_trb,
-   dma_addr_t  suspect_dma,
-   booldebug)
+   dma_addr_t  suspect_dma)
 {
dma_addr_t start_dma;
dma_addr_t end_seg_dma;
@@ -1710,8 +1709,7 @@ struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
/* If the end TRB isn't in this segment, this is set to 0 */
end_trb_dma = xhci_trb_virt_to_dma(cur_seg, end_trb);
 
-   if (debug)
-   xhci_warn(xhci,
+   xhci_dbg(xhci,
"Looking for event-dma %016llx trb-start 
%016llx trb-end %016llx seg-start %016llx seg-end %016llx\n",
(unsigned long long)suspect_dma,
(unsigned long long)start_dma,
@@ -2358,7 +2356,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 
/* Is this a TRB in the currently executing TD? */
ep_seg = trb_in_td(xhci, ep_ring->deq_seg, ep_ring->dequeue,
-   td->last_trb, ep_trb_dma, false);
+   td->last_trb, ep_trb_dma);
 
/*
 * Skip the Force Stopped Event. The event_trb(event_dma) of FSE
@@ -2393,7 +2391,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
trb_comp_code);
trb_in_td(xhci, ep_ring->deq_seg,
  ep_ring->dequeue, td->last_trb,
- ep_trb_dma, true);
+ ep_trb_dma);
return -ESHUTDOWN;
}
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 395f3dc20db8..3320b9c240a9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1858,7 +1858,7 @@ void xhci_reset_bandwidth(struct usb_hcd *hcd, struct 
usb_device *udev);
 dma_addr_t xhci_trb_virt_to_dma(struct xhci_segment *seg, union xhci_trb *trb);
 struct xhci_segment *trb_in_td(struct xhci_hcd *xhci,
struct xhci_segment *start_seg, union xhci_trb *start_trb,
-   union xhci_trb *end_trb, dma_addr_t suspect_dma, bool debug);
+   union xhci_trb *end_trb, dma_addr_t suspect_dma);
 int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int 
trb_comp_code);
 void xhci_ring_cmd_db(struct xhci_hcd *xhci);
 int xhci_queue_slot_control(struct xhci_hcd *xhci, struct xhci_command *cmd,
-- 
2.10.1

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

[PATCH 17/25] usb: host: xhci: remove bogus __releases()/__acquires() annotation

[Felipe Balbi]

handle_tx_event() is not releasing xhci->lock nor reacquiring it, remove
the bogus annotation.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ring.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index f189c49f4b8b..64d9fc490f80 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2159,8 +2159,6 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
  */
 static int handle_tx_event(struct xhci_hcd *xhci,
struct xhci_transfer_event *event)
-   __releases(>lock)
-   __acquires(>lock)
 {
struct xhci_virt_device *xdev;
struct xhci_virt_ep *ep;
-- 
2.10.1

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

[PATCH 19/25] usb: host: xhci: unconditionally call xhci_unmap_td_bounce_buffer()

[Felipe Balbi]

xhci_unmap_td_bounce_buffer() already checks for a valid td->bounce_seg
and bails out early if that's invalid. There's no need to check for this
twice.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 397a74a4ea82..1e1b54d077e2 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -737,8 +737,7 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, 
int slot_id,
 * just overwrite it (because the URB has been unlinked).
 */
ep_ring = xhci_urb_to_transfer_ring(xhci, cur_td->urb);
-   if (ep_ring && cur_td->bounce_seg)
-   xhci_unmap_td_bounce_buffer(xhci, ep_ring, cur_td);
+   xhci_unmap_td_bounce_buffer(xhci, ep_ring, cur_td);
inc_td_cnt(cur_td->urb);
if (last_td_in_urb(cur_td))
xhci_giveback_urb_in_irq(xhci, cur_td, 0);
@@ -764,8 +763,7 @@ static void xhci_kill_ring_urbs(struct xhci_hcd *xhci, 
struct xhci_ring *ring)
if (!list_empty(_td->cancelled_td_list))
list_del_init(_td->cancelled_td_list);
 
-   if (cur_td->bounce_seg)
-   xhci_unmap_td_bounce_buffer(xhci, ring, cur_td);
+   xhci_unmap_td_bounce_buffer(xhci, ring, cur_td);
 
inc_td_cnt(cur_td->urb);
if (last_td_in_urb(cur_td))
@@ -1796,8 +1794,7 @@ static int xhci_td_cleanup(struct xhci_hcd *xhci, struct 
xhci_td *td,
urb_priv = urb->hcpriv;
 
/* if a bounce buffer was used to align this td then unmap it */
-   if (td->bounce_seg)
-   xhci_unmap_td_bounce_buffer(xhci, ep_ring, td);
+   xhci_unmap_td_bounce_buffer(xhci, ep_ring, td);
 
/* Do one last check of the actual transfer length.
 * If the host controller said we transferred more data than the buffer
-- 
2.10.1

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

Re: USB gadget: configfs hid mouse device

[Krzysztof Opasiak]

On 12/01/2016 02:35 PM, Sven Geggus wrote:
> Hello,
> 
> I'm trying to setup a custom mouse-like device on a pi zero.
> 
> As I have never been using the USB gadget stack before, the following
> problem might likely be a configuration failure on my side as well as a
> driver issue.
> 
> Using the documentation from kernel source
> (Documentation/usb/gadget_hid.txt, Documentation/usb/gadget_configfs.txt and
> Documentation/ABI/testing/configfs-usb-gadget-hid) it was more or less
> straight forward to set up a keyboard emulating device using the provided
> example code (hid_gadget_test).
> 
> Unfortunately this is not the case with the mouse emulation.
> 
> I build the following shell-script to generate a mouse device.  Due to the
> lack of an example "report_desc" has been stolen from a logitech mouse:
> 
> --cut--
> #!/bin/bash
> 
> CONFIGFS_HOME=/sys/kernel/config
> C=1
> N="usb0"
> 
> cd $CONFIGFS_HOME/usb_gadget/
> mkdir g1
> cd g1
> echo 0x1d6b > idVendor # Linux Foundation
> echo 0x0104 > idProduct # Multifunction Composite Gadget
> echo 0x0100 > bcdDevice # v1.0.0
> echo 0x0200 > bcdUSB # USB2
> mkdir -p strings/0x409
> echo "fedcba9876543210" > strings/0x409/serialnumber
> echo "Sirius Cybernetics Corp." > strings/0x409/manufacturer 
> echo "dummymouse" > strings/0x409/product
> mkdir -p functions/hid.$N
> # protocol 2 seems to be mouse
> echo 2 > functions/hid.$N/protocol
> echo 1 > functions/hid.$N/subclass
> echo 8 > functions/hid.$N/report_length
> # HID Mouse e.g. copy from logitech mouse as follows:
> # for i in $(usbhid-dump -a XXX:YYY |tail -n +2); do echo -n x$i; done 
> |tr '[:upper:]' '[:lower:]'
> echo -ne 
> \\x05\\x01\\x09\\x02\\xa1\\x01\\x09\\x01\\xa1\\x00\\x05\\x09\\x19\\x01\\x29\\x08\\x15\\x00\\x25\\x01\\x75\\x01\\x95\\x08\\x81\\x02\\x05\\x01\\x16\\x01\\xf8\\x26\\xff\\x07\\x75\\x0c\\x95\\x02\\x09\\x30\\x09\\x31\\x81\\x06\\x15\\x81\\x25\\x7f\\x75\\x08\\x95\\x01\\x09\\x38\\x81\\x06\\x05\\x0c\\x0a\\x38\\x02\\x95\\x01\\x81\\x06\\xc0\\xc0
>  > functions/hid.$N/report_desc
> hexdump -C functions/hid.$N/report_desc
> mkdir -p configs/c.$C/strings/0x409
> echo "Config $C: dummymouse" > configs/c.$C/strings/0x409/configuration 
> echo 250 > configs/c.$C/MaxPower 
> ln -s functions/hid.$N configs/c.$C/
> ls /sys/class/udc > UDC
> --cut--
> 
> However, this does not work.  First of all, the report_desc is not written
> to functions/hid.$N/report_desc as expected.
> 
> Instead I get the following truncated stuff which also shows up on the usb 
> host
> using usbhid-dump:
>   38 02 95 01 81 06 c0 c0   |8...|
> 0008
> 
> Thus I enabled debugging on USB hid gadget driver and get the following:
> [12381.564157] configfs-gadget gadget: high-speed config #1: c
> [12381.577963]  configfs-gadget gadget: hidg_set_alt intf:0 alt:0
> [12381.593012]  configfs-gadget gadget: non-core control req21.0a v 
> i l0
> [12381.608761]  configfs-gadget gadget: hidg_setup crtl_request : 
> bRequestType:0x21 bRequest:0xa Value:0x0
> [12381.626650]  configfs-gadget gadget: Unknown request 0xa
> [12381.640731]  configfs-gadget gadget: non-core control req81.06 v2200 
> i l8
> [12381.656464]  configfs-gadget gadget: hidg_setup crtl_request : 
> bRequestType:0x81 bRequest:0x6 Value:0x2200
> [12381.674655]  configfs-gadget gadget: USB_REQ_GET_DESCRIPTOR: REPORT
> 
> As a result, the device in not detected correctly on the host side as well:
> usb 2-1: Product: dummymouse
> usb 2-1: Manufacturer: Sirius Cybernetics Corp.
> usb 2-1: SerialNumber: fedcba9876543210
> hid-generic 0003:1D6B:0104.004F: unknown main item tag 0x0
> hid-generic 0003:1D6B:0104.004F: collection stack underflow
> hid-generic 0003:1D6B:0104.004F: item 0 0 0 12 parsing failed
> hid-generic: probe of 0003:1D6B:0104.004F failed with error -22
> 
> Kernel Version is 4.4.35+ from Raspberr Pi repo, but looking at the current
> version of drivers/usb/gadget/function/f_hid.c did not seem to have changes
> related to thsi topic:
> 
> https://github.com/raspberrypi/linux/blob/rpi-4.4.y/drivers/usb/gadget/function/f_hid.c
> 
> Any hint?

Try to setup everything as described in [1].

You may also try to use your own C program instead of echo to check if
hex value is correctly written.

Footnotes:
1 - http://marc.info/?l=linux-usb=141526874831676=2

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG 4.9] New led trigger usbport gets the kernel to panic

[Ralph Sennhauser]

On Mon, 28 Nov 2016 23:20:13 +0100
Rafal Milecki  wrote:

> Hi Ralph,
> 
> On 11/21/2016 09:40 AM, Ralph Sennhauser wrote:
> > I tried your new usbport trigger in Linux 4.9 with little luck as
> > can be seen in the following output of the serial console.  
> 
> I'm really happy my trigger got some attention. I'm sorry it crashes
> for you, I never experienced any crash so far. I also got few ppl
> using it as part of LEDE project but no reports from them neither.
> 
> Also sorry for the late reply, my private life took all my time
> previous week.
> 
> 

Hi Rafal

no worries, I know well how it is not to have enough time.

Below the oops with your debug patch applied.

Thanks
Ralph



root@wrt1900acs:/# cd sys/class/leds/pca963x\:shelby\:white\:usb2/
root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
 echo usbport > trigger
[  124.727665] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port:dd708240
[  124.735266] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port->port_name:usb1-port1 port->data:dd708140
[  124.745671] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port:dd7083c0
[  124.753194] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port->port_name:usb2-port1 port->data:dd708140
[  124.763594] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port:dd708300
[  124.771114] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port->port_name:usb3-port1 port->data:dd708140
root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
 echo 1 > ports/usb2-port1[  171.649751] leds pca963x:shelby:white:usb2: 
[usbport_trig_port_store] buf:1
[  171.649751]  size:2

[  171.660160] leds pca963x:shelby:white:usb2: [usbport_trig_port_store] 
port:dd7083c0
[  171.668103] leds pca963x:shelby:white:usb2: [usbport_trig_port_store] 
port->port_name:usb2-port1 port->data:dd708140
[  171.678678] [usbport_trig_update_count] usbport_data->count:0
[  171.684457] [usbport_trig_update_count] usbport_data->count:0
[  171.690253] Unable to handle kernel NULL pointer dereference at virtual 
address 
[  171.698382] pgd = de71c000
[  171.701102] [] *pgd=1e362831, *pte=, *ppte=
[  171.707427] Internal error: Oops: 8007 [#1] SMP ARM
[  171.712671] Modules linked in: iptable_nat nft_chain_nat_ipv4 nf_tables_inet
nf_nat_ipv4 nf_conntrack_ipv6 nf_conntrack_ipv4 ipt_REJECT ipt_MASQUERADE
xt_time xt_tcpudp xt_state xt_nat xt_multiport xt_mark xt_mac xt_limit
xt_conntrack xt_comment xt_TCP MSS xt_REDIRECT xt_LOG xt_CT nft_set_rbtree
nft_set_hash nft_reject_inet nft_reject nft_redir_ipv4 nft_redir nft_nat
nft_meta nft_masq_ipv4 nft_masq nft_log nft_limit nft_hash nft_exthdr nft_ct
nft_counter nft_chain_route_ipv6 nft_chain_route_ipv4 nf_tabl es_ipv6
nf_tables_ipv4 nf_tables nf_reject_ipv4 nf_nat_redirect nf_nat_masquerade_ipv4
nf_nat nf_log_ipv4 nf_defrag_ipv6 nf_defrag_ipv4 nf_conntrack_netlink
nf_conntrack macvlan libcrc32c iptable_raw iptable_mangle iptable_filter
ip_tables act_skbedit act _mirred em_u32 cls_u32 cls_tcindex cls_flow cls_route
cls_fw sch_hfsc sch_ingress mwlwifi(O) mac80211 cfg80211 ledtrig_netdev(O)
ip_set_list_set ip_set_hash_netiface ip_set_hash_netport ip_set_hash_netnet
ip_set_hash_net ip_set_hash_netportnet ip_set_hash _mac ip_set_hash_ipportnet
ip_set_hash_ipportip ip_set_hash_ipport ip_set_hash_ipmark ip_set_hash_ip
ip_set_bitmap_port ip_set_bitmap_ipmac ip_set_bitmap_ip ip_set nfnetlink
ip6t_REJECT nf_reject_ipv6 nf_log_ipv6 nf_log_common ip6table_raw
ip6table_mangle ip6table_filter ip6_tables x_tables ifb sit tunnel4 ip_tunnel
vfat fat nls_utf8 nls_iso8859_1 nls_cp850 nls_cp437 rfkill input_core
sha256_generic seqiv jitterentropy_rng drbg hmac ghash_generic gf128mul gcm ctr
ccm ledtrig_usbport ext4 jbd2 mbcache btrf
s xor raid6_pq crc32c_generic ubifs ubi
[  171.851028] CPU: 1 PID: 724 Comm: ash Tainted: G   O4.9.0-rc7 #2
[  171.858103] Hardware name: Marvell Armada 380/385 (Device Tree)
[  171.864046] task: df71bc80 task.stack: df7d
[  171.868594] PC is at 0x0
[  171.871139] LR is at usbport_trig_port_store+0x10c/0x17c [ledtrig_usbport]
[  171.878042] pc : [<>]lr : []psr: 6013
[  171.878042] sp : df7d1e48  ip : 0007  fp : df7d1e6c
[  171.889566] r10:   r9 :   r8 : 
[  171.894811] r7 : df63c4ec  r6 : deeec700  r5 : dd708140  r4 : 0002
[  171.901363] r3 :   r2 :   r1 :   r0 : df63c4ec
[  171.907916] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[  171.915079] Control: 10c5387d  Table: 1e71c04a  DAC: 0051
[  171.920847] Process ash (pid: 724, stack limit = 0xdf7d0210)
[  171.926528] Stack: (0xdf7d1e48 to 0xdf7d2000)
[  171.930902] 1e40:   dd708140 ddc7a840 bf228164 0002 
df7d1f80 dd708500
[  171.939114] 1e60: df7d1e84 df7d1e70 c0237e00 bf228170 

Re: [GIT PULL] USB-serial updates for v4.10-rc1

[Greg Kroah-Hartman]

On Thu, Dec 01, 2016 at 12:18:49PM +0100, Johan Hovold wrote:
> Hi Greg,
> 
> Here are my updates for 4.10. Details below.
> 
> Thanks,
> Johan
> 
> 
> The following changes since commit 07d9a380680d1c0eb51ef87ff2eab5c994949e69:
> 
>   Linux 4.9-rc2 (2016-10-23 17:10:14 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git 
> tags/usb-serial-4.10-rc1

Pulled and pushed out, 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

[PATH net v2] cdc_ether: Fix handling connection notification

[Kristian Evensen]

Commit bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
introduced a work-around in usbnet_cdc_status() for devices that exported
cdc carrier on twice on connect. Before the commit, this behavior caused
the link state to be incorrect. It was assumed that all CDC Ethernet
devices would either export this behavior, or send one off and then one on
notification (which seems to be the default behavior).

Unfortunately, it turns out multiple devices sends a connection
notification multiple times per second (via an interrupt), even when
connection state does not change. This has been observed with several
different USB LAN dongles (at least), for example 13b1:0041 (Linksys).
After bfe9b9d2df66, the link state has been set as down and then up for
each notification. This has caused a flood of Netlink NEWLINK messages and
syslog to be flooded with messages similar to:

cdc_ether 2-1:2.0 eth1: kevent 12 may have been dropped

This commit fixes the behavior by reverting usbnet_cdc_status() to how it
was before bfe9b9d2df66. The work-around has been moved to a separate
status-function which is only called when a known, affect device is
detected.

v1->v2:

* Do not open-code netif_carrier_ok() (thanks Henning Schild).
* Call netif_carrier_off() instead of usb_link_change(). This prevents
calling schedule_work() twice without giving the work queue a chance to be
processed (thanks Bjørn Mork).

Fixes: bfe9b9d2df66 ("cdc_ether: Improve ZTE MF823/831/910 handling")
Reported-by: Henning Schild 
Signed-off-by: Kristian Evensen 
---
 drivers/net/usb/cdc_ether.c | 38 +++---
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/cdc_ether.c b/drivers/net/usb/cdc_ether.c
index 45e5e43..fe7b288 100644
--- a/drivers/net/usb/cdc_ether.c
+++ b/drivers/net/usb/cdc_ether.c
@@ -388,12 +388,6 @@ void usbnet_cdc_status(struct usbnet *dev, struct urb *urb)
case USB_CDC_NOTIFY_NETWORK_CONNECTION:
netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
  event->wValue ? "on" : "off");
-
-   /* Work-around for devices with broken off-notifications */
-   if (event->wValue &&
-   !test_bit(__LINK_STATE_NOCARRIER, >net->state))
-   usbnet_link_change(dev, 0, 0);
-
usbnet_link_change(dev, !!event->wValue, 0);
break;
case USB_CDC_NOTIFY_SPEED_CHANGE:   /* tx/rx rates */
@@ -466,6 +460,36 @@ static int usbnet_cdc_zte_rx_fixup(struct usbnet *dev, 
struct sk_buff *skb)
return 1;
 }
 
+/* Ensure correct link state
+ *
+ * Some devices (ZTE MF823/831/910) export two carrier on notifications when
+ * connected. This causes the link state to be incorrect. Work around this by
+ * always setting the state to off, then on.
+ */
+void usbnet_cdc_zte_status(struct usbnet *dev, struct urb *urb)
+{
+   struct usb_cdc_notification *event;
+
+   if (urb->actual_length < sizeof(*event))
+   return;
+
+   event = urb->transfer_buffer;
+
+   if (event->bNotificationType != USB_CDC_NOTIFY_NETWORK_CONNECTION) {
+   usbnet_cdc_status(dev, urb);
+   return;
+   }
+
+   netif_dbg(dev, timer, dev->net, "CDC: carrier %s\n",
+ event->wValue ? "on" : "off");
+
+   if (event->wValue &&
+   netif_carrier_ok(dev->net))
+   netif_carrier_off(dev->net);
+
+   usbnet_link_change(dev, !!event->wValue, 0);
+}
+
 static const struct driver_infocdc_info = {
.description =  "CDC Ethernet Device",
.flags =FLAG_ETHER | FLAG_POINTTOPOINT,
@@ -481,7 +505,7 @@ static const struct driver_info zte_cdc_info = {
.flags =FLAG_ETHER | FLAG_POINTTOPOINT,
.bind = usbnet_cdc_zte_bind,
.unbind =   usbnet_cdc_unbind,
-   .status =   usbnet_cdc_status,
+   .status =   usbnet_cdc_zte_status,
.set_rx_mode =  usbnet_cdc_update_filter,
.manage_power = usbnet_manage_power,
.rx_fixup = usbnet_cdc_zte_rx_fixup,
-- 
2.9.3

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

Re: [PATCH v3] USB: OHCI: ohci-pxa27x: remove useless functions

[Alan Stern]

On Thu, 1 Dec 2016 csmanjuvi...@gmail.com wrote:

> From: Manjunath Goudar 
> 
> The ohci_hcd_pxa27x_drv_probe function is not doing anything other
> than calling usb_hcd_pxa27x_probe function so ohci_hcd_pxa27x_drv_probe
> function is useless that is why removed ohci_hcd_pxa27x_drv_probe
> function and renamed usb_hcd_pxa27x_probe function to
> ohci_hcd_pxa27x_drv_probe for proper naming.
> 
> The ohci_hcd_pxa27x_remove function is also not doing anything other than
> calling usb_hcd_pxa27x_remove that is why removed ohci_hcd_pxa27x_remove
> function and renamed usb_hcd_pxa27x_remove to ohci_hcd_pxa27x_remove for
> proper naming.
> 
> Signed-off-by: Manjunath Goudar 
> Cc: Alan Stern 
> Cc: Greg Kroah-Hartman 
> Cc: linux-usb@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> ---
> Changelog v1 -> v2:
> Removed warnings and errors of checkpatch.pl script.
> Replaced unuseful with useless in patch commit message for proper meaning.
> Removed usb_disabled() from ohci_hcd_pxa27x_drv_probe function because it
> is already existing in ohci_pxa27x_init function
> 
> Changelog v2 -> v3:
> Removed checkpatch.pl warnings and errors cleanup code which is not related
> to this patch.
>  drivers/usb/host/ohci-pxa27x.c | 38 --
>  1 file changed, 12 insertions(+), 26 deletions(-)

Just one little thing...

> diff --git a/drivers/usb/host/ohci-pxa27x.c b/drivers/usb/host/ohci-pxa27x.c
> index a667cf2..1b6b46e 100644
> --- a/drivers/usb/host/ohci-pxa27x.c
> +++ b/drivers/usb/host/ohci-pxa27x.c
> @@ -404,7 +404,7 @@ static int ohci_pxa_of_init(struct platform_device *pdev)
>  
>  
>  /**
> - * usb_hcd_pxa27x_probe - initialize pxa27x-based HCDs
> + * ohci_hcd_pxa27x_probe - initialize pxa27x-based HCDs
>   * Context: !in_interrupt()
>   *
>   * Allocates basic resources for this USB host controller, and
> @@ -412,7 +412,7 @@ static int ohci_pxa_of_init(struct platform_device *pdev)
>   * through the hotplug entry's driver_data.
>   *
>   */
> -int usb_hcd_pxa27x_probe (const struct hc_driver *driver, struct 
> platform_device *pdev)
> +static int ohci_hcd_pxa27x_probe(struct platform_device *pdev)
>  {
>   int retval, irq;
>   struct usb_hcd *hcd;
> @@ -423,6 +423,8 @@ int usb_hcd_pxa27x_probe (const struct hc_driver *driver, 
> struct platform_device
>   struct clk *usb_clk;
>   unsigned int i;
>  
> + pr_debug("In ohci_hcd_pxa27x_drv_probe");
> +

Don't insert this pr_debug line.  After fixing that up, you can add:

Acked-by: Alan Stern 

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

[PATCH 22/25] usb: host: xhci: remove unnecessary list_for_each_entry_safe()

[Felipe Balbi]

the _save() version of list iterators are supposed to be used when
list_entry is going to be removed from the list while iterating. Since
xhci_handle_stopped_cmd_ring() is not removing anything from the list,
just converting commands into No-Op TRBs, we don't need to use the safe
version.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 50244dee6b43..ebb52ffab805 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1211,28 +1211,28 @@ void xhci_cleanup_command_queue(struct xhci_hcd *xhci)
 static void xhci_handle_stopped_cmd_ring(struct xhci_hcd *xhci,
 struct xhci_command *cur_cmd)
 {
-   struct xhci_command *i_cmd, *tmp_cmd;
+   struct xhci_command *cmd;
u32 cycle_state;
 
/* Turn all aborted commands in list to no-ops, then restart */
-   list_for_each_entry_safe(i_cmd, tmp_cmd, >cmd_list,
+   list_for_each_entry(cmd, >cmd_list,
 cmd_list) {
 
-   if (i_cmd->status != COMP_COMMAND_ABORTED)
+   if (cmd->status != COMP_COMMAND_ABORTED)
continue;
 
-   i_cmd->status = COMP_STOPPED;
+   cmd->status = COMP_STOPPED;
 
xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
-i_cmd->command_trb);
+cmd->command_trb);
/* get cycle state from the original cmd trb */
cycle_state = le32_to_cpu(
-   i_cmd->command_trb->generic.field[3]) & TRB_CYCLE;
+   cmd->command_trb->generic.field[3]) & TRB_CYCLE;
/* modify the command trb to no-op command */
-   i_cmd->command_trb->generic.field[0] = 0;
-   i_cmd->command_trb->generic.field[1] = 0;
-   i_cmd->command_trb->generic.field[2] = 0;
-   i_cmd->command_trb->generic.field[3] = cpu_to_le32(
+   cmd->command_trb->generic.field[0] = 0;
+   cmd->command_trb->generic.field[1] = 0;
+   cmd->command_trb->generic.field[2] = 0;
+   cmd->command_trb->generic.field[3] = cpu_to_le32(
TRB_TYPE(TRB_CMD_NOOP) | cycle_state);
 
/*
@@ -1250,7 +1250,6 @@ static void xhci_handle_stopped_cmd_ring(struct xhci_hcd 
*xhci,
mod_timer(>cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
xhci_ring_cmd_db(xhci);
}
-   return;
 }
 
 
-- 
2.10.1

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

USB gadget: configfs hid mouse device

[Sven Geggus]

Hello,

I'm trying to setup a custom mouse-like device on a pi zero.

As I have never been using the USB gadget stack before, the following
problem might likely be a configuration failure on my side as well as a
driver issue.

Using the documentation from kernel source
(Documentation/usb/gadget_hid.txt, Documentation/usb/gadget_configfs.txt and
Documentation/ABI/testing/configfs-usb-gadget-hid) it was more or less
straight forward to set up a keyboard emulating device using the provided
example code (hid_gadget_test).

Unfortunately this is not the case with the mouse emulation.

I build the following shell-script to generate a mouse device.  Due to the
lack of an example "report_desc" has been stolen from a logitech mouse:

--cut--
#!/bin/bash

CONFIGFS_HOME=/sys/kernel/config
C=1
N="usb0"

cd $CONFIGFS_HOME/usb_gadget/
mkdir g1
cd g1
echo 0x1d6b > idVendor # Linux Foundation
echo 0x0104 > idProduct # Multifunction Composite Gadget
echo 0x0100 > bcdDevice # v1.0.0
echo 0x0200 > bcdUSB # USB2
mkdir -p strings/0x409
echo "fedcba9876543210" > strings/0x409/serialnumber
echo "Sirius Cybernetics Corp." > strings/0x409/manufacturer 
echo "dummymouse" > strings/0x409/product
mkdir -p functions/hid.$N
# protocol 2 seems to be mouse
echo 2 > functions/hid.$N/protocol
echo 1 > functions/hid.$N/subclass
echo 8 > functions/hid.$N/report_length
# HID Mouse e.g. copy from logitech mouse as follows:
# for i in $(usbhid-dump -a XXX:YYY |tail -n +2); do echo -n x$i; done |tr 
'[:upper:]' '[:lower:]'
echo -ne 
\\x05\\x01\\x09\\x02\\xa1\\x01\\x09\\x01\\xa1\\x00\\x05\\x09\\x19\\x01\\x29\\x08\\x15\\x00\\x25\\x01\\x75\\x01\\x95\\x08\\x81\\x02\\x05\\x01\\x16\\x01\\xf8\\x26\\xff\\x07\\x75\\x0c\\x95\\x02\\x09\\x30\\x09\\x31\\x81\\x06\\x15\\x81\\x25\\x7f\\x75\\x08\\x95\\x01\\x09\\x38\\x81\\x06\\x05\\x0c\\x0a\\x38\\x02\\x95\\x01\\x81\\x06\\xc0\\xc0
 > functions/hid.$N/report_desc
hexdump -C functions/hid.$N/report_desc
mkdir -p configs/c.$C/strings/0x409
echo "Config $C: dummymouse" > configs/c.$C/strings/0x409/configuration 
echo 250 > configs/c.$C/MaxPower 
ln -s functions/hid.$N configs/c.$C/
ls /sys/class/udc > UDC
--cut--

However, this does not work.  First of all, the report_desc is not written
to functions/hid.$N/report_desc as expected.

Instead I get the following truncated stuff which also shows up on the usb host
using usbhid-dump:
  38 02 95 01 81 06 c0 c0   |8...|
0008

Thus I enabled debugging on USB hid gadget driver and get the following:
[12381.564157] configfs-gadget gadget: high-speed config #1: c
[12381.577963]  configfs-gadget gadget: hidg_set_alt intf:0 alt:0
[12381.593012]  configfs-gadget gadget: non-core control req21.0a v 
i l0
[12381.608761]  configfs-gadget gadget: hidg_setup crtl_request : 
bRequestType:0x21 bRequest:0xa Value:0x0
[12381.626650]  configfs-gadget gadget: Unknown request 0xa
[12381.640731]  configfs-gadget gadget: non-core control req81.06 v2200 
i l8
[12381.656464]  configfs-gadget gadget: hidg_setup crtl_request : 
bRequestType:0x81 bRequest:0x6 Value:0x2200
[12381.674655]  configfs-gadget gadget: USB_REQ_GET_DESCRIPTOR: REPORT

As a result, the device in not detected correctly on the host side as well:
usb 2-1: Product: dummymouse
usb 2-1: Manufacturer: Sirius Cybernetics Corp.
usb 2-1: SerialNumber: fedcba9876543210
hid-generic 0003:1D6B:0104.004F: unknown main item tag 0x0
hid-generic 0003:1D6B:0104.004F: collection stack underflow
hid-generic 0003:1D6B:0104.004F: item 0 0 0 12 parsing failed
hid-generic: probe of 0003:1D6B:0104.004F failed with error -22

Kernel Version is 4.4.35+ from Raspberr Pi repo, but looking at the current
version of drivers/usb/gadget/function/f_hid.c did not seem to have changes
related to thsi topic:

https://github.com/raspberrypi/linux/blob/rpi-4.4.y/drivers/usb/gadget/function/f_hid.c

Any hint?

Regards

Sven

-- 
"Remember, democracy never lasts long. It soon wastes itself,
exhausts and murders itself. There never was a democracy yet
that did not commit suicide." (John Quincy Adams)
/me is giggls@ircnet, http://sven.gegg.us/ on the Web
--
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 05/25] usb: host: xhci: rename completion codes to match spec

[Felipe Balbi]

Cleanup only. This patch is a mechaninal rename to make sure our macros
for TRB completion codes match what the specification uses to refer to
such errors. The idea behind this is that it makes it far easier to grep
the specification and match it with implementation.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-hub.c  |   3 +-
 drivers/usb/host/xhci-ring.c | 126 +--
 drivers/usb/host/xhci.c  |  48 -
 drivers/usb/host/xhci.h  | 106 +---
 4 files changed, 125 insertions(+), 158 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index ba5ffeef305d..dd6de282e48f 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -418,7 +418,8 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int 
slot_id, int suspend)
/* Wait for last stop endpoint command to finish */
wait_for_completion(cmd->completion);
 
-   if (cmd->status == COMP_CMD_ABORT || cmd->status == COMP_CMD_STOP) {
+   if (cmd->status == COMP_COMMAND_ABORTED ||
+   cmd->status == COMP_STOPPED) {
xhci_warn(xhci, "Timeout while waiting for stop endpoint 
command\n");
ret = -ETIME;
}
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 02506c44380d..17b878b53b46 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -995,10 +995,10 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd 
*xhci, int slot_id,
unsigned int slot_state;
 
switch (cmd_comp_code) {
-   case COMP_TRB_ERR:
+   case COMP_TRB_ERROR:
xhci_warn(xhci, "WARN Set TR Deq Ptr cmd invalid 
because of stream ID configuration\n");
break;
-   case COMP_CTX_STATE:
+   case COMP_CONTEXT_STATE_ERROR:
xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed due to 
incorrect slot or ep state.\n");
ep_state = GET_EP_CTX_STATE(ep_ctx);
slot_state = le32_to_cpu(slot_ctx->dev_state);
@@ -1007,7 +1007,7 @@ static void xhci_handle_cmd_set_deq(struct xhci_hcd 
*xhci, int slot_id,
"Slot state = %u, EP state = %u",
slot_state, ep_state);
break;
-   case COMP_EBADSLT:
+   case COMP_SLOT_NOT_ENABLED_ERROR:
xhci_warn(xhci, "WARN Set TR Deq Ptr cmd failed because 
slot %u was not enabled.\n",
slot_id);
break;
@@ -1204,7 +1204,7 @@ void xhci_cleanup_command_queue(struct xhci_hcd *xhci)
 {
struct xhci_command *cur_cmd, *tmp_cmd;
list_for_each_entry_safe(cur_cmd, tmp_cmd, >cmd_list, cmd_list)
-   xhci_complete_del_and_free_cmd(cur_cmd, COMP_CMD_ABORT);
+   xhci_complete_del_and_free_cmd(cur_cmd, COMP_COMMAND_ABORTED);
 }
 
 /*
@@ -1222,10 +1222,10 @@ static void xhci_handle_stopped_cmd_ring(struct 
xhci_hcd *xhci,
list_for_each_entry_safe(i_cmd, tmp_cmd, >cmd_list,
 cmd_list) {
 
-   if (i_cmd->status != COMP_CMD_ABORT)
+   if (i_cmd->status != COMP_COMMAND_ABORTED)
continue;
 
-   i_cmd->status = COMP_CMD_STOP;
+   i_cmd->status = COMP_STOPPED;
 
xhci_dbg(xhci, "Turn aborted command %p to no-op\n",
 i_cmd->command_trb);
@@ -1270,9 +1270,9 @@ void xhci_handle_command_timeout(unsigned long data)
/* mark this command to be cancelled */
spin_lock_irqsave(>lock, flags);
if (xhci->current_cmd) {
-   if (xhci->current_cmd->status == COMP_CMD_ABORT)
+   if (xhci->current_cmd->status == COMP_COMMAND_ABORTED)
second_timeout = true;
-   xhci->current_cmd->status = COMP_CMD_ABORT;
+   xhci->current_cmd->status = COMP_COMMAND_ABORTED;
}
 
/* Make sure command ring is running before aborting it */
@@ -1340,7 +1340,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
cmd_comp_code = GET_COMP_CODE(le32_to_cpu(event->status));
 
/* If CMD ring stopped we own the trbs between enqueue and dequeue */
-   if (cmd_comp_code == COMP_CMD_STOP) {
+   if (cmd_comp_code == COMP_STOPPED) {
xhci_handle_stopped_cmd_ring(xhci, cmd);
return;
}
@@ -1357,9 +1357,9 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 * The command ring is stopped now, but the xHC will issue a Command
 * Ring Stopped event which will cause us to restart it.
 */
-   if (cmd_comp_code == COMP_CMD_ABORT) {
+   if (cmd_comp_code == 

[PATCH 06/25] usb: host: xhci: WARN on unexpected COMP_SUCCESS

[Felipe Balbi]

COMP_SUCCESS should only be asserted on a *true* sucessful transfer. Any
other cases are bogus and we should aim to catch them. One easy way to
get bug reports is to trigger a WARN_ONCE() on such cases.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 17b878b53b46..772e07e2ef16 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1934,6 +1934,7 @@ static int process_ctrl_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
 {
struct xhci_virt_device *xdev;
struct xhci_ring *ep_ring;
+   struct device *dev;
unsigned int slot_id;
int ep_index;
struct xhci_ep_ctx *ep_ctx;
@@ -1950,15 +1951,15 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
trb_comp_code = GET_COMP_CODE(le32_to_cpu(event->transfer_len));
requested = td->urb->transfer_buffer_length;
remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
+   dev = xhci_to_hcd(xhci)->self.controller;
 
switch (trb_comp_code) {
case COMP_SUCCESS:
-   if (trb_type != TRB_STATUS) {
-   xhci_warn(xhci, "WARN: Success on ctrl %s TRB without 
IOC set?\n",
-   (trb_type == TRB_DATA) ? "data" : 
"setup");
-   *status = -ESHUTDOWN;
-   break;
-   }
+   dev_WARN_ONCE(dev, trb_type != TRB_STATUS,
+   "ep%d%s: unexpected success! TRB Type %d\n",
+   usb_endpoint_num(>urb->ep->desc),
+   usb_endpoint_dir_in(>urb->ep->desc) ?
+   "in" : "out", trb_type);
*status = 0;
break;
case COMP_SHORT_PACKET:
@@ -2150,6 +2151,7 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
struct xhci_virt_ep *ep, int *status)
 {
struct xhci_ring *ep_ring;
+   struct device *dev;
u32 trb_comp_code;
u32 remaining, requested, ep_trb_len;
 
@@ -2158,16 +2160,16 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
ep_trb_len = TRB_LEN(le32_to_cpu(ep_trb->generic.field[2]));
requested = td->urb->transfer_buffer_length;
+   dev = xhci_to_hcd(xhci)->self.controller;
 
switch (trb_comp_code) {
case COMP_SUCCESS:
-   /* handle success with untransferred data as short packet */
-   if (ep_trb != td->last_trb || remaining) {
-   xhci_warn(xhci, "WARN Successful completion on short 
TX\n");
-   xhci_dbg(xhci, "ep %#x - asked for %d bytes, %d bytes 
untransferred\n",
-td->urb->ep->desc.bEndpointAddress,
-requested, remaining);
-   }
+   dev_WARN_ONCE(dev, (ep_trb != td->last_trb) || remaining,
+   "ep%d%s: unexpected success! TRB %p/%p size 
%d/%d\n",
+   usb_endpoint_num(>urb->ep->desc),
+   usb_endpoint_dir_in(>urb->ep->desc) ?
+   "in" : "out", ep_trb, td->last_trb, remaining,
+   requested);
*status = 0;
break;
case COMP_SHORT_PACKET:
-- 
2.10.1

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

[PATCH 12/25] usb: host: xhci: major rewrite of process_ctrl_td()

[Felipe Balbi]

process_ctrl_td() is too complex and difficult to read. We can clean it
up a lot and maintain same functionality as before. Note that while
cleaning up, one comment was introduced to explain the special case - it
wasn't clear before from code.

Much of the changes are related to removal of duplicated work done in
both process_ctrl_td() and finish_td(). By removing the duplicated code,
we could remove a few local variables and shuffle things around so the
implementation is more straight forward.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7acfecc6639a..607bd2d2ab11 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1930,99 +1930,60 @@ static int process_ctrl_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
union xhci_trb *ep_trb, struct xhci_transfer_event *event,
struct xhci_virt_ep *ep, int *status)
 {
-   struct xhci_virt_device *xdev;
-   struct xhci_ring *ep_ring;
-   struct device *dev;
-   unsigned int slot_id;
-   int ep_index;
-   struct xhci_ep_ctx *ep_ctx;
u32 trb_comp_code;
-   u32 remaining, requested;
+   u32 remaining;
+   u32 requested;
u32 trb_type;
 
trb_type = TRB_FIELD_TO_TYPE(le32_to_cpu(ep_trb->generic.field[3]));
-   slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->flags));
-   xdev = xhci->devs[slot_id];
-   ep_index = TRB_TO_EP_ID(le32_to_cpu(event->flags)) - 1;
-   ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));
-   ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
trb_comp_code = GET_COMP_CODE(le32_to_cpu(event->transfer_len));
requested = td->urb->transfer_buffer_length;
remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
-   dev = xhci_to_hcd(xhci)->self.controller;
+
+   td->urb->actual_length = requested - remaining;
 
switch (trb_comp_code) {
case COMP_SUCCESS:
-   dev_WARN_ONCE(dev, trb_type != TRB_STATUS,
-   "ep%d%s: unexpected success! TRB Type %d\n",
-   usb_endpoint_num(>urb->ep->desc),
-   usb_endpoint_dir_in(>urb->ep->desc) ?
-   "in" : "out", trb_type);
-   *status = 0;
-   break;
case COMP_SHORT_PACKET:
*status = 0;
break;
case COMP_STOPPED_SHORT_PACKET:
-   if (trb_type == TRB_DATA ||
-   trb_type == TRB_NORMAL)
-   td->urb->actual_length = remaining;
-   else
+   if (trb_type == TRB_SETUP ||
+   trb_type == TRB_STATUS)
xhci_warn(xhci, "WARN: Stopped Short Packet on ctrl 
setup or status TRB\n");
-   goto finish_td;
+   *status = -ESHUTDOWN;
+
+   /*
+* NOTICE: according to section 6.4.2 Table 91 of xHCI rev 1.1
+* Specification, if completion code is Stopped - Short Packet,
+* Transfer Length field (referred to as 'remaining' here)
+* contain the value of EDTLA (see section 4.11.5.2 for
+* details), which means that remaining contains actual_length,
+* not the amount of bytes remaining to be transferred as usual.
+*/
+   td->urb->actual_length = remaining;
+   break;
case COMP_STOPPED:
-   switch (trb_type) {
-   case TRB_SETUP:
+   if (trb_type == TRB_SETUP)
td->urb->actual_length = 0;
-   goto finish_td;
-   case TRB_DATA:
-   case TRB_NORMAL:
-   td->urb->actual_length = requested - remaining;
-   goto finish_td;
-   default:
-   xhci_warn(xhci, "WARN: unexpected TRB Type %d\n", 
trb_type);
-   goto finish_td;
-   }
+   *status = -ESHUTDOWN;
+   break;
case COMP_STOPPED_LENGTH_INVALID:
-   goto finish_td;
-   default:
-   if (!xhci_requires_manual_halt_cleanup(xhci,
-  ep_ctx, trb_comp_code))
-   break;
-   xhci_dbg(xhci, "TRB error %u, halted endpoint index = %u\n",
-trb_comp_code, ep_index);
-   /* else fall through */
+   td->urb->actual_length = 0;
+   *status = -EINVAL;
+   break;
case COMP_STALL_ERROR:
-   /* Did we transfer part of the data (middle) phase? */
-   if (trb_type == 

[PATCH 07/25] usb: host: xhci: WARN() if we interrupt without event_ring

[Felipe Balbi]

If we get an interrupt while we don't have an allocated event_ring, that
means we're enabling IRQs far too early. Instead of just printing an
error message, let's make sure to add a scary-looking Stack Trace so
people report these things and we fix them.

Eventually, when we're happy that this doesn't happen, we should just
remove this altogether.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 772e07e2ef16..0fd990603f77 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2526,14 +2526,16 @@ static int handle_tx_event(struct xhci_hcd *xhci,
 static int xhci_handle_event(struct xhci_hcd *xhci)
 {
union xhci_trb *event;
+   struct device *dev;
int update_ptrs = 1;
int ret;
 
+   dev = xhci_to_hcd(xhci)->self.controller;
+
/* Event ring hasn't been allocated yet. */
-   if (!xhci->event_ring || !xhci->event_ring->dequeue) {
-   xhci_err(xhci, "ERROR event ring not ready\n");
+   if (dev_WARN_ONCE(dev, !xhci->event_ring || !xhci->event_ring->dequeue,
+   "event ring not ready\n"))
return -ENOMEM;
-   }
 
event = xhci->event_ring->dequeue;
/* Does the HC or OS own the TRB? */
-- 
2.10.1

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

[PATCH 04/25] usb: host: xhci: print HCIVERSION on debug

[Felipe Balbi]

When calling xhci_dbg_regs() we actually _do_ want to know XHCI's
version. This might help figure out why certain problems only happen
in some cases.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-dbg.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/host/xhci-dbg.c b/drivers/usb/host/xhci-dbg.c
index a3b67f33d4d8..363d125300ea 100644
--- a/drivers/usb/host/xhci-dbg.c
+++ b/drivers/usb/host/xhci-dbg.c
@@ -37,10 +37,8 @@ void xhci_dbg_regs(struct xhci_hcd *xhci)
>cap_regs->hc_capbase, temp);
xhci_dbg(xhci, "//   CAPLENGTH: 0x%x\n",
(unsigned int) HC_LENGTH(temp));
-#if 0
xhci_dbg(xhci, "//   HCIVERSION: 0x%x\n",
(unsigned int) HC_VERSION(temp));
-#endif
 
xhci_dbg(xhci, "// xHCI operational registers at %p:\n", xhci->op_regs);
 
-- 
2.10.1

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

[PATCH 01/25] usb: host: xhci: dynamically allocate devs array

[Felipe Balbi]

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

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

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

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

[PATCH 13/25] usb: host: xhci: major rewrite of process_bulk_intr_td()

[Felipe Balbi]

Similar to previous patch on process_ctrl_td(), we can also clean up
process_bulk_intr_td() and turn it into an easier to read and maintain
function.

This patch shuffles code around, defines each variable in one line,
removes unnecessary debugging messages and an unnecessary goto
statement.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 607bd2d2ab11..49712beef328 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2110,58 +2110,50 @@ static int process_bulk_intr_td(struct xhci_hcd *xhci, 
struct xhci_td *td,
struct xhci_virt_ep *ep, int *status)
 {
struct xhci_ring *ep_ring;
-   struct device *dev;
u32 trb_comp_code;
-   u32 remaining, requested, ep_trb_len;
+   u32 ep_trb_len;
+   u32 remaining;
+   u32 requested;
 
ep_ring = xhci_dma_to_transfer_ring(ep, le64_to_cpu(event->buffer));
trb_comp_code = GET_COMP_CODE(le32_to_cpu(event->transfer_len));
remaining = EVENT_TRB_LEN(le32_to_cpu(event->transfer_len));
ep_trb_len = TRB_LEN(le32_to_cpu(ep_trb->generic.field[2]));
requested = td->urb->transfer_buffer_length;
-   dev = xhci_to_hcd(xhci)->self.controller;
+
+   if (ep_trb == td->last_trb)
+   td->urb->actual_length = requested - remaining;
+   else
+   td->urb->actual_length =
+   sum_trb_lengths(xhci, ep_ring, ep_trb) +
+   ep_trb_len - remaining;
 
switch (trb_comp_code) {
case COMP_SUCCESS:
-   dev_WARN_ONCE(dev, (ep_trb != td->last_trb) || remaining,
-   "ep%d%s: unexpected success! TRB %p/%p size 
%d/%d\n",
-   usb_endpoint_num(>urb->ep->desc),
-   usb_endpoint_dir_in(>urb->ep->desc) ?
-   "in" : "out", ep_trb, td->last_trb, remaining,
-   requested);
-   *status = 0;
-   break;
case COMP_SHORT_PACKET:
-   xhci_dbg(xhci, "ep %#x - asked for %d bytes, %d bytes 
untransferred\n",
-td->urb->ep->desc.bEndpointAddress,
-requested, remaining);
*status = 0;
break;
case COMP_STOPPED_SHORT_PACKET:
+   /*
+* NOTICE: according to section 6.4.2 Table 91 of xHCI rev 1.1
+* Specification, if completion code is Stopped - Short Packet,
+* Transfer Length field (referred to as 'remaining' here)
+* contain the value of EDTLA (see section 4.11.5.2 for
+* details), which means that remaining contains actual_length,
+* not the amount of bytes remaining to be transferred as usual.
+*/
td->urb->actual_length = remaining;
-   goto finish_td;
+   *status = -ESHUTDOWN;
+   break;
case COMP_STOPPED_LENGTH_INVALID:
-   /* stopped on ep trb with invalid length, exclude it */
-   ep_trb_len  = 0;
-   remaining   = 0;
+   td->urb->actual_length = 0;
+   *status = -ESHUTDOWN;
break;
default:
/* do nothing */
break;
}
 
-   if (ep_trb == td->last_trb)
-   td->urb->actual_length = requested - remaining;
-   else
-   td->urb->actual_length =
-   sum_trb_lengths(xhci, ep_ring, ep_trb) +
-   ep_trb_len - remaining;
-finish_td:
-   if (remaining > requested) {
-   xhci_warn(xhci, "bad transfer trb length %d in event trb\n",
- remaining);
-   td->urb->actual_length = 0;
-   }
return finish_td(xhci, td, ep_trb, event, ep, status, false);
 }
 
-- 
2.10.1

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

[PATCH 11/25] usb: host: xhci: use slightly better list helpers

[Felipe Balbi]

Replace list_entry() with list_first_entry() and list_for_each() with
list_for_each_entry(). This makes the code slightly more readable.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 116b4a0dadbb..7acfecc6639a 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -635,7 +635,6 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, 
int slot_id,
unsigned int ep_index;
struct xhci_ring *ep_ring;
struct xhci_virt_ep *ep;
-   struct list_head *entry;
struct xhci_td *cur_td = NULL;
struct xhci_td *last_unlinked_td;
 
@@ -665,8 +664,7 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, 
int slot_id,
 * it.  We're also in the event handler, so we can't get re-interrupted
 * if another Stop Endpoint command completes
 */
-   list_for_each(entry, >cancelled_td_list) {
-   cur_td = list_entry(entry, struct xhci_td, cancelled_td_list);
+   list_for_each_entry(cur_td, >cancelled_td_list, cancelled_td_list) {
xhci_dbg_trace(xhci, trace_xhci_dbg_cancel_urb,
"Removing canceled TD starting at 0x%llx 
(dma).",
(unsigned long long)xhci_trb_virt_to_dma(
@@ -730,7 +728,7 @@ static void xhci_handle_cmd_stop_ep(struct xhci_hcd *xhci, 
int slot_id,
 * So stop when we've completed the URB for the last TD we unlinked.
 */
do {
-   cur_td = list_entry(ep->cancelled_td_list.next,
+   cur_td = list_first_entry(>cancelled_td_list,
struct xhci_td, cancelled_td_list);
list_del_init(_td->cancelled_td_list);
 
@@ -1331,7 +1329,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
return;
}
 
-   cmd = list_entry(xhci->cmd_list.next, struct xhci_command, cmd_list);
+   cmd = list_first_entry(>cmd_list, struct xhci_command, cmd_list);
 
del_timer(>cmd_timer);
 
@@ -1419,7 +1417,7 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
 
/* restart timer if this wasn't the last command */
if (cmd->cmd_list.next != >cmd_list) {
-   xhci->current_cmd = list_entry(cmd->cmd_list.next,
+   xhci->current_cmd = list_first_entry(>cmd_list,
   struct xhci_command, cmd_list);
mod_timer(>cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
}
@@ -2415,7 +2413,7 @@ static int handle_tx_event(struct xhci_hcd *xhci,
goto cleanup;
}
 
-   td = list_entry(ep_ring->td_list.next, struct xhci_td, td_list);
+   td = list_first_entry(_ring->td_list, struct xhci_td, 
td_list);
if (ep->skip)
td_num--;
 
-- 
2.10.1

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

[PATCH 14/25] usb: host: xhci: cleanup finish_td()

[Felipe Balbi]

Now that finish_td() is the only place calling
xhci_requires_manual_halt_cleanup() we can remove that function and use
that to cleanup finish_td() by converting it into a switch statement
using trb_comp_code.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 49712beef328..f951850eb57f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1771,32 +1771,6 @@ static void xhci_cleanup_halted_endpoint(struct xhci_hcd 
*xhci,
xhci_ring_cmd_db(xhci);
 }
 
-/* Check if an error has halted the endpoint ring.  The class driver will
- * cleanup the halt for a non-default control endpoint if we indicate a stall.
- * However, a babble and other errors also halt the endpoint ring, and the 
class
- * driver won't clear the halt in that case, so we need to issue a Set Transfer
- * Ring Dequeue Pointer command manually.
- */
-static int xhci_requires_manual_halt_cleanup(struct xhci_hcd *xhci,
-   struct xhci_ep_ctx *ep_ctx,
-   unsigned int trb_comp_code)
-{
-   /* TRB completion codes that may require a manual halt cleanup */
-   if (trb_comp_code == COMP_USB_TRANSACTION_ERROR ||
-   trb_comp_code == COMP_BABBLE_DETECTED_ERROR ||
-   trb_comp_code == COMP_SPLIT_TRANSACTION_ERROR)
-   /* The 0.95 spec says a babbling control endpoint
-* is not halted. The 0.96 spec says it is.  Some HW
-* claims to be 0.95 compliant, but it halts the control
-* endpoint anyway.  Check if a babble halted the
-* endpoint.
-*/
-   if (GET_EP_CTX_STATE(ep_ctx) == EP_STATE_HALTED)
-   return 1;
-
-   return 0;
-}
-
 int xhci_is_vendor_info_code(struct xhci_hcd *xhci, unsigned int trb_comp_code)
 {
if (trb_comp_code >= 224 && trb_comp_code <= 255) {
@@ -1838,19 +1812,35 @@ static int finish_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
if (skip)
goto td_cleanup;
 
-   if (trb_comp_code == COMP_STOPPED_LENGTH_INVALID ||
-   trb_comp_code == COMP_STOPPED ||
-   trb_comp_code == COMP_STOPPED_SHORT_PACKET) {
+   switch (trb_comp_code) {
+   case COMP_STOPPED_LENGTH_INVALID:
+   case COMP_STOPPED:
+   case COMP_STOPPED_SHORT_PACKET:
/* The Endpoint Stop Command completion will take care of any
 * stopped TDs.  A stopped TD may be restarted, so don't update
 * the ring dequeue pointer or take this TD off any lists yet.
 */
ep->stopped_td = td;
return 0;
-   }
-   if (trb_comp_code == COMP_STALL_ERROR ||
-   xhci_requires_manual_halt_cleanup(xhci, ep_ctx,
-   trb_comp_code)) {
+   case COMP_USB_TRANSACTION_ERROR:
+   case COMP_BABBLE_DETECTED_ERROR:
+   case COMP_SPLIT_TRANSACTION_ERROR:
+   /* Check if an error has halted the endpoint ring.  The class
+* driver will cleanup the halt for a non-default control
+* endpoint if we indicate a stall.  However, a babble and other
+* errors also halt the endpoint ring, and the class driver
+* won't clear the halt in that case, so we need to issue a Set
+* Transfer Ring Dequeue Pointer command manually.
+*
+* Note that the 0.95 spec says a babbling control endpoint is
+* not halted. The 0.96 spec says it is.  Some HW claims to be
+* 0.95 compliant, but it halts the control endpoint anyway.
+* Check if a babble halted the endpoint.
+*/
+   if (GET_EP_CTX_STATE(ep_ctx) != EP_STATE_HALTED)
+   break;
+   /* FALLTHROUGH */
+   case COMP_STALL_ERROR:
/* Issue a reset endpoint command to clear the host side
 * halt, followed by a set dequeue command to move the
 * dequeue pointer past the TD.
@@ -1858,7 +1848,8 @@ static int finish_td(struct xhci_hcd *xhci, struct 
xhci_td *td,
 */
xhci_cleanup_halted_endpoint(xhci, slot_id, ep_index,
ep_ring->stream_id, td, ep_trb);
-   } else {
+   break;
+   default:
/* Update ring dequeue pointer */
while (ep_ring->dequeue != td->last_trb)
inc_deq(xhci, ep_ring);
-- 
2.10.1

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

[PATCH 09/25] usb: host: xhci: clear only STS_EINT

[Felipe Balbi]

Many other bits in USBSTS register are "clear-by-writing-1". Let's make
sure that we clear *only* STS_EINT and not any of the other bits as they
might be needed later.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ring.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b4ec80d18078..116b4a0dadbb 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2630,8 +2630,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
 * so we can receive interrupts from other MSI-X interrupters.
 * Write 1 to clear the interrupt status.
 */
-   status |= STS_EINT;
-   writel(status, >op_regs->status);
+   writel(STS_EINT, >op_regs->status);
/* FIXME when MSI-X is supported and there are multiple vectors */
/* Clear the MSI-X event interrupt status */
 
-- 
2.10.1

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

[PATCH 08/25] usb: host: xhci: simplify irq handler return

[Felipe Balbi]

Instead of having several return points, let's use a local variable and
a single place to return. This makes the code slightly easier to read.

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

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0fd990603f77..b4ec80d18078 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2601,27 +2601,28 @@ static int xhci_handle_event(struct xhci_hcd *xhci)
 irqreturn_t xhci_irq(struct usb_hcd *hcd)
 {
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-   u32 status;
-   u64 temp_64;
union xhci_trb *event_ring_deq;
+   irqreturn_t ret = IRQ_NONE;
dma_addr_t deq;
+   u64 temp_64;
+   u32 status;
 
spin_lock(>lock);
/* Check if the xHC generated the interrupt, or the irq is shared */
status = readl(>op_regs->status);
-   if (status == 0x)
-   goto hw_died;
-
-   if (!(status & STS_EINT)) {
-   spin_unlock(>lock);
-   return IRQ_NONE;
+   if (status == 0x) {
+   ret = IRQ_HANDLED;
+   goto out;
}
+
+   if (!(status & STS_EINT))
+   goto out;
+
if (status & STS_FATAL) {
xhci_warn(xhci, "WARNING: Host System Error\n");
xhci_halt(xhci);
-hw_died:
-   spin_unlock(>lock);
-   return IRQ_HANDLED;
+   ret = IRQ_HANDLED;
+   goto out;
}
 
/*
@@ -2652,9 +2653,8 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
temp_64 = xhci_read_64(xhci, >ir_set->erst_dequeue);
xhci_write_64(xhci, temp_64 | ERST_EHB,
>ir_set->erst_dequeue);
-   spin_unlock(>lock);
-
-   return IRQ_HANDLED;
+   ret = IRQ_HANDLED;
+   goto out;
}
 
event_ring_deq = xhci->event_ring->dequeue;
@@ -2680,9 +2680,10 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
temp_64 |= ERST_EHB;
xhci_write_64(xhci, temp_64, >ir_set->erst_dequeue);
 
+out:
spin_unlock(>lock);
 
-   return IRQ_HANDLED;
+   return ret;
 }
 
 irqreturn_t xhci_msi_irq(int irq, void *hcd)
-- 
2.10.1

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

[PATCH 10/25] usb: host: xhci: remove unneded semicolon

[Felipe Balbi]

it does no good, let's remove it.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ext-caps.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
index e0244fb3903d..28deea584884 100644
--- a/drivers/usb/host/xhci-ext-caps.h
+++ b/drivers/usb/host/xhci-ext-caps.h
@@ -117,7 +117,7 @@ static inline int xhci_find_next_ext_cap(void __iomem 
*base, u32 start, int id)
offset = XHCI_HCC_EXT_CAPS(val) << 2;
if (!offset)
return 0;
-   };
+   }
do {
val = readl(base + offset);
if (val == ~0)
-- 
2.10.1

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

[PATCH] usbip: vudc: fix: Clear already_seen flag also for ep0

[Krzysztof Opasiak]

ep_list inside gadget structure doesn't contain ep0.
It is stored separately in ep0 field.

This causes an urb hang if gadget driver decides to
delay setup handling. On host side this is visible as
timeout error when setting configuration.

This bug can be reproduced using for example any gadget
with mass storage function.

Fixes: abdb2957 ("usbip: vudc: Add vudc_transfer")
Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/usbip/vudc_transfer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/usbip/vudc_transfer.c 
b/drivers/usb/usbip/vudc_transfer.c
index aba6bd4..bc0296d 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -339,6 +339,8 @@ static void v_timer(unsigned long _vudc)
total = timer->frame_limit;
}
 
+   /* We have to clear ep0 flags separately as it's not on the list */
+   udc->ep[0].already_seen = 0;
list_for_each_entry(_ep, >gadget.ep_list, ep_list) {
ep = to_vep(_ep);
ep->already_seen = 0;
-- 
2.9.3

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

Re: [BUG 4.9] New led trigger usbport gets the kernel to panic

[Rafał Miłecki]

On 12/01/2016 03:28 PM, Ralph Sennhauser wrote:

Below the oops with your debug patch applied.

(...)

root@wrt1900acs:/# cd sys/class/leds/pca963x\:shelby\:white\:usb2/
root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
 echo usbport > trigger
[  124.727665] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port:dd708240
[  124.735266] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port->port_name:usb1-port1 port->data:dd708140
[  124.745671] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port:dd7083c0
[  124.753194] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port->port_name:usb2-port1 port->data:dd708140
[  124.763594] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port:dd708300
[  124.771114] leds pca963x:shelby:white:usb2: [usbport_trig_add_port] 
port->port_name:usb3-port1 port->data:dd708140
root@wrt1900acs:/sys/devices/platform/soc/soc:internal-regs/f1011000.i2c/i2c-0/0-0068/leds/pca963x:shelby:white:usb2#
 echo 1 > ports/usb2-port1[  171.649751] leds pca963x:shelby:white:usb2: 
[usbport_trig_port_store] buf:1
[  171.649751]  size:2

[  171.660160] leds pca963x:shelby:white:usb2: [usbport_trig_port_store] 
port:dd7083c0
[  171.668103] leds pca963x:shelby:white:usb2: [usbport_trig_port_store] 
port->port_name:usb2-port1 port->data:dd708140
[  171.678678] [usbport_trig_update_count] usbport_data->count:0
[  171.684457] [usbport_trig_update_count] usbport_data->count:0
[  171.690253] Unable to handle kernel NULL pointer dereference at virtual 
address 


Oh, so this happens a bit later than I expected or I could read from the
backtrace. Anyway this debugging was still helpful, I think I can see a possible
problem.

So most likely the crash happens at the:
led_cdev->brightness_set(led_cdev, ...);
call. I'm not sure what I was thinking when writing that code. It looks wrong.

The thing is some LEDs (drivers) don't provide brightness_set op. It's fine, we
should just use brightness_set_blocking op when that happens. Of course kernel
has proper helpers for that, we don't have to worry about the choice of op or
scheduling the work. I have no idea why I didn't use a proper helper in the
first place.

So we should simply replace above call with one of following ones:
1) led_set_brightness(led_cdev, ...);
2) led_set_brightness_nosleep(led_cdev, ...);
3) led_set_brightness_sync(led_cdev, ...);

I still have to check which one is correct. In theory we don't deal blinking at
this point so we shouldn't need to use led_set_brightness.

led_set_brightness_nosleep looks like the most likely correct choice.

led_set_brightness_sync requires brightness_set_blocking which is not always
present so most likely we don't want this one.


If you have some free time and you want to play with this, please replace
led_cdev->brightness_set
with
led_set_brightness_nosleep
and give it a try. This should fix crashes for you.

I'll look at this again during next days.
--
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] usbip: vudc: Refactor init_vudc_hw() to be more obvious

[Krzysztof Opasiak]

Current implementation of init_vudc_hw() adds ep0 to ep_list
and then after looping through all endpoints removes it from
that list.

As this may be misleading let's refactor this function
and avoid adding and removing ep0 to eplist and place it
immediately in correct place.

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/usbip/vudc_dev.c | 38 +-
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 7091848..a5ca29b 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc)
sprintf(ep->name, "ep%d%s", num,
i ? (is_out ? "out" : "in") : "");
ep->ep.name = ep->name;
+
+   ep->ep.ops = _ops;
+
+   ep->halted = ep->wedged = ep->already_seen =
+   ep->setup_stage = 0;
+   usb_ep_set_maxpacket_limit(>ep, ~0);
+   ep->ep.max_streams = 16;
+   ep->gadget = >gadget;
+   ep->desc = NULL;
+   INIT_LIST_HEAD(>req_queue);
+
if (i == 0) {
+   /* ep0 */
ep->ep.caps.type_control = true;
ep->ep.caps.dir_out = true;
ep->ep.caps.dir_in = true;
+
+   udc->gadget.ep0 = >ep;
} else {
+   /* All other eps */
ep->ep.caps.type_iso = true;
ep->ep.caps.type_int = true;
ep->ep.caps.type_bulk = true;
-   }
 
-   if (is_out)
-   ep->ep.caps.dir_out = true;
-   else
-   ep->ep.caps.dir_in = true;
+   if (is_out)
+   ep->ep.caps.dir_out = true;
+   else
+   ep->ep.caps.dir_in = true;
 
-   ep->ep.ops = _ops;
-   list_add_tail(>ep.ep_list, >gadget.ep_list);
-   ep->halted = ep->wedged = ep->already_seen =
-   ep->setup_stage = 0;
-   usb_ep_set_maxpacket_limit(>ep, ~0);
-   ep->ep.max_streams = 16;
-   ep->gadget = >gadget;
-   ep->desc = NULL;
-   INIT_LIST_HEAD(>req_queue);
+   list_add_tail(>ep.ep_list, >gadget.ep_list);
+   }
}
 
spin_lock_init(>lock);
@@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc)
ud->eh_ops.reset= vudc_device_reset;
ud->eh_ops.unusable = vudc_device_unusable;
 
-   udc->gadget.ep0 = >ep[0].ep;
-   list_del_init(>ep[0].ep.ep_list);
-
v_init_timer(udc);
return 0;
 
-- 
2.9.3

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

Re: [PATCH] usbip: vudc: fix: Clear already_seen flag also for ep0

[Krzysztof Opasiak]

Hi,

On 12/01/2016 06:26 PM, Sergei Shtylyov wrote:
> Hello!
> 
> On 12/01/2016 07:20 PM, Krzysztof Opasiak wrote:
> 
>> ep_list inside gadget structure doesn't contain ep0.
>> It is stored separately in ep0 field.
>>
>> This causes an urb hang if gadget driver decides to
>> delay setup handling. On host side this is visible as
>> timeout error when setting configuration.
>>
>> This bug can be reproduced using for example any gadget
>> with mass storage function.
>>
>> Fixes: abdb2957 ("usbip: vudc: Add vudc_transfer")
> 
>At least 12 digits please, the format is standardized.

You are right. I forgot about this and just used a short-id. I will post
a fix in a minute.

Thank you.
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
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: host: xhci: remove braces for single statement

[Greg KH]

On Thu, Dec 01, 2016 at 10:42:59PM +0530, Amit Kushwaha wrote:
> This patch fixes checkpatch.pl warning:
> braces {} are not necessary for single statement blocks
> 
> Signed-off-by: Amit Kushwaha 

{sigh}

Why are you now not using your samsung email address?

Please don't do cleanup patches for anything in the kernel other than
drivers/staging/ until you really know what you are doing.  Right now,
that is not true at all.

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

[PATCH v2] usbip: vudc: fix: Clear already_seen flag also for ep0

[Krzysztof Opasiak]

ep_list inside gadget structure doesn't contain ep0.
It is stored separately in ep0 field.

This causes an urb hang if gadget driver decides to
delay setup handling. On host side this is visible as
timeout error when setting configuration.

This bug can be reproduced using for example any gadget
with mass storage function.

Fixes: abdb29574322 ("usbip: vudc: Add vudc_transfer")
Signed-off-by: Krzysztof Opasiak 
---
Changes since v1:
- Use first 12 digits of commit sha in fixes tag

---
 drivers/usb/usbip/vudc_transfer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/usbip/vudc_transfer.c 
b/drivers/usb/usbip/vudc_transfer.c
index aba6bd4..bc0296d 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -339,6 +339,8 @@ static void v_timer(unsigned long _vudc)
total = timer->frame_limit;
}
 
+   /* We have to clear ep0 flags separately as it's not on the list */
+   udc->ep[0].already_seen = 0;
list_for_each_entry(_ep, >gadget.ep_list, ep_list) {
ep = to_vep(_ep);
ep->already_seen = 0;
-- 
2.9.3

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

[PATCH] usb: host: xhci: remove braces for single statement

[Amit Kushwaha]

This patch fixes checkpatch.pl warning:
braces {} are not necessary for single statement blocks

Signed-off-by: Amit Kushwaha 
---
 drivers/usb/host/xhci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 1a4ca02..168f5ac0 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -2808,9 +2808,9 @@ int xhci_check_bandwidth(struct usb_hcd *hcd,
struct usb_device *udev)
  /* Only cache or free the old ring if it exists.
  * It may not if this is the first add of an endpoint.
  */
- if (virt_dev->eps[i].ring) {
+ if (virt_dev->eps[i].ring)
  xhci_free_or_cache_endpoint_ring(xhci, virt_dev, i);
- }
+
  xhci_check_bw_drop_ep_streams(xhci, virt_dev, i);
  virt_dev->eps[i].ring = virt_dev->eps[i].new_ring;
  virt_dev->eps[i].new_ring = NULL;
-- 
2.10.2.windows.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usbip: vudc: fix: Clear already_seen flag also for ep0

[Sergei Shtylyov]

Hello!

On 12/01/2016 07:20 PM, Krzysztof Opasiak wrote:


ep_list inside gadget structure doesn't contain ep0.
It is stored separately in ep0 field.

This causes an urb hang if gadget driver decides to
delay setup handling. On host side this is visible as
timeout error when setting configuration.

This bug can be reproduced using for example any gadget
with mass storage function.

Fixes: abdb2957 ("usbip: vudc: Add vudc_transfer")


   At least 12 digits please, the format is standardized.


Signed-off-by: Krzysztof Opasiak 

[...]

MBR, Sergei

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

Re: [PATCH v13 03/10] usbip: exporting devices: new connect operation

[Krzysztof Opasiak]

Hi,

On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> Implementation of new connect operation. This is linked as a part of 
> usbip commnad. With this patch, usbip command has following operations.
> 
>  bind
>  unbind
>  list (local/remote)
>  attach
>  detach
>  port
>  connect ... this patch


In my humble opinion connect is not a best name for this operation.
it not only starts the connection to a remote server but also exports a
device. I think that a name like export or attach_to_remote would be
more suitable.
(...)

> +static const char usbip_connect_usage_string[] =
> + "usbip connect \n"
> + "-r, --remote=Address of a remote computer\n"
> + "-b, --busid=Bus ID of a device to be connected\n"
> + "-d, --device   Run with an alternate driver, e.g. vUDC\n";
> +
> +void usbip_connect_usage(void)
> +{
> + printf("usage: %s", usbip_connect_usage_string);
> +}
> +
> +static int send_export_device(int sockfd, struct usbip_usb_device *udev)
> +{
> + int rc;
> + struct op_export_request request;
> + struct op_export_reply   reply;
> + uint16_t code = OP_REP_EXPORT;
> +
> + memset(, 0, sizeof(request));
> + memset(, 0, sizeof(reply));

Probably you don't need to 0 the reply as it is buffer for receiving and
it's not being send anywhere.

> +
> + /* send a request */
> + rc = usbip_net_send_op_common(sockfd, OP_REQ_EXPORT, 0);
> + if (rc < 0) {
> + err("send op_common");
> + return -1;
> + }
> +
> + memcpy(, udev, sizeof(struct usbip_usb_device));

how about sizeof(request.udev)?

> +
> + PACK_OP_EXPORT_REQUEST(0, );
> +
> + rc = usbip_net_send(sockfd, (void *) , sizeof(request));
> + if (rc < 0) {
> + err("send op_export_request");
> + return -1;
> + }
> +
> + /* receive a reply */
> + rc = usbip_net_recv_op_common(sockfd, );
> + if (rc < 0) {
> + err("recv op_common");
> + return -1;
> + }
> +
> + rc = usbip_net_recv(sockfd, (void *) , sizeof(reply));
> + if (rc < 0) {
> + err("recv op_export_reply");
> + return -1;
> + }
> +
> + PACK_OP_EXPORT_REPLY(0, );
> +
> + /* check the reply */
> + if (reply.returncode) {
> + err("recv error return %d", reply.returncode);
> + return -1;
> + }

This check should probably look different or at least should be
documented. Please refer to my comments to patch #1.

> +
> + return 0;
> +}
> +
> +static int export_device(char *busid, int sockfd)
> +{
> + int rc;
> + struct usbip_exported_device *edev;
> +
> + rc = usbip_driver_open(driver);
> + if (rc) {
> + err("open driver");
> + return -1;
> + }
> +
> + rc = usbip_refresh_device_list(driver);
> + if (rc < 0) {
> + err("could not refresh device list");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + edev = usbip_get_device(driver, busid);
> + if (edev == NULL) {
> + err("find device");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + rc = send_export_device(sockfd, >udev);
> + if (rc < 0) {
> + err("send export");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + rc = usbip_export_device(edev, sockfd);
> + if (rc < 0) {
> + err("export device");
> + usbip_driver_close(driver);
> + return -1;
> + }

Take a look at all above function calls and your if.

You have usbip_driver_close() called 4 times in 4 error path. This is
just asking to replace this with goto and place error path below return
just like you did one function below.

Not to mention that you should propagate the error code to caller.

> +
> + usbip_driver_close(driver);
> +
> + return 0;
> +}

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
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 v13 01/10] usbip: exporting devices: modifications to network header

[Krzysztof Opasiak]

Hi,

On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> Modification to export and un-export response in 
> tools/usb/usbip/src/usbip_network.h. It just changes return code type 
> from int to uint32_t as same as other responses.
> 
> Signed-off-by: Nobuo Iwata 
> ---
>  tools/usb/usbip/src/usbip_network.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/usb/usbip/src/usbip_network.h 
> b/tools/usb/usbip/src/usbip_network.h
> index c1e875c..e1ca86a 100644
> --- a/tools/usb/usbip/src/usbip_network.h
> +++ b/tools/usb/usbip/src/usbip_network.h
> @@ -93,7 +93,7 @@ struct op_export_request {
>  } __attribute__((packed));
>  
>  struct op_export_reply {
> - int returncode;
> + uint32_t returncode;
>  } __attribute__((packed));
>  
>  
> @@ -115,7 +115,7 @@ struct op_unexport_request {
>  } __attribute__((packed));
>  
>  struct op_unexport_reply {
> - int returncode;
> + uint32_t returncode;
>  } __attribute__((packed));
>  
>  #define PACK_OP_UNEXPORT_REQUEST(pack, request)  do {\
> 

The field name is returncode but we have no suitable defines with
"return codes". I ran through USBIP documentation but didn't find any
list of allowed return codes. Is there any?

You change the value type to unsigned but later you use:

+   if (!error)
+   reply.returncode = 0;
+   else
+   reply.returncode = -1;

It looks a little bit ugly to me that we change the value to be unsigned
and then assign a signed number to it. In addition your compiler is
going to complain if you use -Wconversion flag.

In my opinion we should have suitable defines for error codes and those
code should be documented in protocol documentation.

Best regards
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
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 v13 02/10] usbip: exporting devices: modifications to host side libraries

[Krzysztof Opasiak]

On 11/24/2016 06:59 AM, fx IWATA NOBUO wrote:
> Hello,
> 
>> This doesn't look like a simple change to rename and reuse an unused
>> function. This patch does lot more and is changing the user interface.
>> Looks like instead of taking an integer value for device lookup, you
>> are changing it to char *.
>>
>> Any reason why you have to change the user interface to go to char
>> *busid?
>>
>> I would like to see a good explanation why this user interface change
>> is necessary.
> 
> I can't figure out why the second argument was int because it was
> unused.
> 

I also cannot figure out this but using busid here looks much more
reasonable to me.

> usbip_get_device() is a stub-side (usb-host) function.
> As you know, only port number in vhci-side can be int.
> Others are bus-id.
> Furthermore, the unused code searches list node position. It doesn't
> make sense.
> 

Agree.

> Here, this patch set needs to find bound device with bus-id in
> stub-side.

stub-side or vudc side. In case of vudc the busid is set to usbip-vudc.%d

> 
> I may add explanation above to change log.
> Or I can change new function name like usbip_find_device() and leave the
> unused function once.
> 
> Please, let me know how should I do.

In my humble opinion the code itself is good. The problem is commit
message. Current commit massage has nothing to do with patch content and
should be totally changed.

Please elaborate that this function is unused, write that using indexes
on a list is generally not a good idea and so on.

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] net: asix: Fix AX88772_suspend() USB vendor commands failure issues

[David Miller]

From: "ASIX_Allan [Office]" 
Date: Wed, 30 Nov 2016 16:29:08 +0800

> The change fixes AX88772_suspend() USB vendor commands failure issues.
> 
> Signed-off-by: Allan Chou 
> Tested-by: Allan Chou 
> Tested-by: Jon Hunter 

Patch applied, thanks.
--
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: [RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver

[John Stultz]

On Thu, Dec 1, 2016 at 12:23 AM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Wednesday 23 November 2016 09:16 AM, John Stultz wrote:
>> This wires extconn support to hikey's phy driver, and
>> connects it to the usb UDC layer via a usb_phy structure.
>>
>> Not sure if this is the right way to connect phy -> UDC,
>> but I'm lacking a clear example.
>>
>> Cc: Wei Xu 
>> Cc: Guodong Xu 
>> Cc: Amit Pundir 
>> Cc: Rob Herring 
>> Cc: John Youn 
>> Cc: Douglas Anderson 
>> Cc: Chen Yu 
>> Cc: Kishon Vijay Abraham I 
>> Cc: Felipe Balbi 
>> Cc: Greg Kroah-Hartman 
>> Cc: linux-usb@vger.kernel.org
>> Signed-off-by: John Stultz 
>> ---
>>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  11 +++
>>  drivers/phy/Kconfig   |   2 +
>>  drivers/phy/phy-hi6220-usb.c  | 139 
>> ++
>>  3 files changed, 152 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
>> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>> index 17839db..171fbb2 100644
>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>> @@ -732,10 +732,21 @@
>>   regulator-always-on;
>>   };
>>
>> + usb_vbus: usb-vbus {
>> + compatible = "linux,extcon-usb-gpio";
>> + id-gpio = < 6 1>;
>> + };
>> +
>> + usb_id: usb-id {
>> + compatible = "linux,extcon-usb-gpio";
>> + id-gpio = < 5 1>;
>> + };
>> +
>>   usb_phy: usbphy {
>>   compatible = "hisilicon,hi6220-usb-phy";
>>   #phy-cells = <0>;
>>   phy-supply = <_5v_hub>;
>> + extcon = <_vbus>, <_id>;
>>   hisilicon,peripheral-syscon = <_ctrl>;
>>   };
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index fe00f91..76f4f17 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
>>  config PHY_HI6220_USB
>>   tristate "hi6220 USB PHY support"
>>   depends on (ARCH_HISI && ARM64) || COMPILE_TEST
>> + depends on EXTCON
>>   select GENERIC_PHY
>>   select MFD_SYSCON
>> + select USB_PHY
>>   help
>> Enable this to support the HISILICON HI6220 USB PHY.
>>
>> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
>> index b2141cb..89d8475 100644
>> --- a/drivers/phy/phy-hi6220-usb.c
>> +++ b/drivers/phy/phy-hi6220-usb.c
>> @@ -12,7 +12,12 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>>  #include 
>> +#include 
>>
>>  #define SC_PERIPH_CTRL4  0x00c
>>
>> @@ -44,9 +49,21 @@
>>
>>  #define EYE_PATTERN_PARA 0x7053348c
>>
>> +
>> +struct hi6220_usb_cable {
>> + struct notifier_block   nb;
>> + struct extcon_dev   *extcon;
>> + int state;
>> +};
>> +
>>  struct hi6220_priv {
>>   struct regmap *reg;
>>   struct device *dev;
>> + struct usb_phy phy;
>> +
>> + struct delayed_work work;
>> + struct hi6220_usb_cable vbus;
>> + struct hi6220_usb_cable id;
>>  };
>>
>>  static void hi6220_phy_init(struct hi6220_priv *priv)
>> @@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
>>   return hi6220_phy_setup(priv, false);
>>  }
>>
>> +
>>  static struct phy_ops hi6220_phy_ops = {
>>   .init   = hi6220_phy_start,
>>   .exit   = hi6220_phy_exit,
>>   .owner  = THIS_MODULE,
>>  };
>>
>> +static void hi6220_detect_work(struct work_struct *work)
>> +{
>> + struct hi6220_priv *priv =
>> + container_of(to_delayed_work(work), struct hi6220_priv, work);
>> + struct usb_otg *otg = priv->phy.otg;
>> +
>> + if (!IS_ERR(priv->vbus.extcon))
>> + priv->vbus.state = extcon_get_cable_state_(priv->vbus.extcon,
>> +  EXTCON_USB);
>> + if (!IS_ERR(priv->id.extcon))
>> + priv->id.state = extcon_get_cable_state_(priv->id.extcon,
>> +  EXTCON_USB_HOST);
>> + if (otg->gadget) {
>> + if (priv->id.state)
>> + usb_gadget_vbus_connect(otg->gadget);
>> + else
>> + usb_gadget_vbus_disconnect(otg->gadget);
>> + }
>> +}
>> +
>> +static int hi6220_otg_vbus_notifier(struct notifier_block *nb,
>> + unsigned long event, void *ptr)
>> +{
>> + struct hi6220_usb_cable *vbus = container_of(nb,
>> + 

Re: [PATCH v13 04/10] usbip: exporting devices: new disconnect operation

[Krzysztof Opasiak]

On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
(...)
> +
> +static int send_unexport_device(int sockfd, struct usbip_usb_device *udev)
> +{
> + int rc;
> + struct op_unexport_request request;
> + struct op_unexport_reply   reply;
> + uint16_t code = OP_REP_UNEXPORT;
> +
> + memset(, 0, sizeof(request));
> + memset(, 0, sizeof(reply));

As in previous patch, you don't need to 0 the reply.

> +
> + /* send a request */
> + rc = usbip_net_send_op_common(sockfd, OP_REQ_UNEXPORT, 0);
> + if (rc < 0) {
> + err("send op_common");
> + return -1;
> + }
> +
> + memcpy(, udev, sizeof(struct usbip_usb_device));

sizeof(request.udev)?

> +
> + PACK_OP_UNEXPORT_REQUEST(0, );
> +
> + rc = usbip_net_send(sockfd, (void *) , sizeof(request));
> + if (rc < 0) {
> + err("send op_export_request");
> + return -1;
> + }
> +
> + /* receive a reply */
> + rc = usbip_net_recv_op_common(sockfd, );
> + if (rc < 0) {
> + err("recv op_common");
> + return -1;
> + }
> +
> + rc = usbip_net_recv(sockfd, (void *) , sizeof(reply));
> + if (rc < 0) {
> + err("recv op_unexport_reply");
> + return -1;
> + }
> +
> + PACK_OP_EXPORT_REPLY(0, );
> +
> + /* check the reply */
> + if (reply.returncode) {
> + err("recv error return %d", reply.returncode);
> + return -1;
> + }

The same case with error code as in previous patch.

> +
> + return 0;
> +}
> +
> +static int unexport_device(char *busid, int sockfd)
> +{
> + int rc;
> + struct usbip_exported_device *edev;
> +
> + rc = usbip_driver_open(driver);
> + if (rc < 0) {
> + err("open driver");
> + return -1;
> + }
> +
> + rc = usbip_refresh_device_list(driver);
> + if (rc < 0) {
> + err("could not refresh device list");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + edev = usbip_get_device(driver, busid);
> + if (edev == NULL) {
> + err("find device");
> + usbip_driver_close(driver);
> + return -1;
> + }
> +
> + rc = send_unexport_device(sockfd, >udev);
> + if (rc < 0) {
> + err("send unexport");
> + usbip_driver_close(driver);
> + return -1;
> + }

Once again the same pattern as in previous patch 3 times in a row
copy-pasted error path. Please use goto here and place error path below
return.

> +
> + usbip_driver_close(driver);
> +
> + return 0;
> +}
> +
> +static int disconnect_device(char *host, char *busid, int unbind)
> +{
> + int sockfd;
> + int rc;
> +
> + sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> + if (sockfd < 0) {
> + err("tcp connect");
> + return -1;
> + }
> +
> + rc = unexport_device(busid, sockfd);
> + if (rc < 0) {
> + err("unexport");
> + close(sockfd);
> + return -1;
> + }

close(sockfd) in case of error, otherwise close(sockfd)...

Probably you can place close(sockfd) above if to avoid this weird
copy-paste.

> +
> + close(sockfd);
> +
> + if (unbind) {
> + rc = usbip_unbind_device(busid);
> + if (rc) {
> + err("unbind");
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int usbip_disconnect(int argc, char *argv[])
> +{
> + static const struct option opts[] = {
> + { "remote", required_argument, NULL, 'r' },
> + { "busid",  required_argument, NULL, 'b' },
> + { "device", no_argument,   NULL, 'd' },
> + { NULL, 0,  NULL, 0 }
> + };
> + char *host = NULL;
> + char *busid = NULL;
> + int opt;
> + int unbind = 1;
> + int ret = -1;
> +
> + for (;;) {
> + opt = getopt_long(argc, argv, "r:b:d", opts, NULL);
> +
> + if (opt == -1)
> + break;
> +
> + switch (opt) {
> + case 'r':
> + host = optarg;
> + break;
> + case 'b':
> + busid = optarg;
> + break;
> + case 'd':
> + driver = _driver;
> + unbind = 0;
> + break;
> + default:
> + goto err_out;
> + }
> + }
> +
> + if (!host || !busid)
> + goto err_out;
> +
> + ret = disconnect_device(host, busid, unbind);
> + goto out;

This looks a little bit weird... why you use goto out instead of just
return ret?

> +
> +err_out:
> + usbip_disconnect_usage();
> +out:
> + return ret;
> +}
> 

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line 

Re: [PATCH] usb: gadget: udc: core: fix return code of usb_gadget_probe_driver()

[Felix Hädicke]

Hi,

> Hi,
>
> Good catch but I have a few comments to commit message:
>
> On 12/01/2016 12:24 AM, Felix Hädicke wrote:
>> This fixes a regression which was introduced by commit f1bddbb, by
>> reverting a small fragment of commit 855ed04.
>>
> Not exactly. The regression has been introduced by commit 855ed04
> itself.This commit replaced previous construction similar what you sent
> now with simple if():
>
> @@ -535,11 +556,7 @@ int usb_gadget_probe_driver(struct
> usb_gadget_driver *driver)
> if (!ret)
> break;
> }
> -   if (ret)
> -   ret = -ENODEV;
> -   else if (udc->driver)
> -   ret = -EBUSY;
> -   else
> +   if (!ret && !udc->driver)
> goto found;
> } else {
> list_for_each_entry(udc, _list, list) {

The regression bug I want to fix with this patch was introduced with
commit f1bddbb, not 855ed04. With 855ed04, the this if/else construct
was changed. But concerning the usb_gadget_probe_driver() return code,
this was ok. There was no code path were a value which was not meant to
be the function return code was accidentally returned. With commit
f1bddbb, and the introduction of a new code path for the flag
match_existing_only, this changed.

> After that commit, if UDC is busy, gadget is added to the list of
> pending gadgets. Unfortunately this list is not rescanned in
> usb_gadget_unregister_driver(). This means that such gadget is going to
> stay on this list forever so it's a bug.

Ok, I can confirm this bug. But it is not the issue which I am
addressing with this patch. This is just about the
usb_gadget_probe_driver() return code (on which other functions,
paritcularly gadget configfs's|gadget_dev_desc_UDC_store|() rely).

> Commit f1bddbb make this bug more visible (as it causes NULL pointer
> dereference) as gadget has not been added to the list of pending gadgets
> and we try to remove it from there.
>
> Summing up, in my personal opinion I think that you should add:
>
> Fixes: 855ed04 ("usb: gadget: udc-core: independent registration of
> gadgets and gadget drivers")

As explained above, I think this would be wrong.

> above your sign off and fix the commit message as this bug has been
> introduced before f1bddbb, just symptoms were a little bit different.
>
> After fixing this you may add:
>
> Tested-by: Krzysztof Opasiak 

Ok.

>
> Best regards,

Best Regards,
Felix
--
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 v10 2/8] power: add power sequence library

[Rafael J. Wysocki]

On Tue, Nov 22, 2016 at 4:53 AM, Peter Chen  wrote:
> On Tue, Nov 22, 2016 at 03:23:12AM +0100, Rafael J. Wysocki wrote:
>> > @@ -0,0 +1,237 @@
>> > +/*
>> > + * core.c  power sequence core file
>> > + *
>> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
>> > + * Author: Peter Chen 
>> > + *
>> > + * This program is free software: you can redistribute it and/or modify
>> > + * it under the terms of the GNU General Public License version 2  of
>> > + * the License as published by the Free Software Foundation.
>> > + *
>> > + * This program is distributed in the hope that it will be useful,
>> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > + * GNU General Public License for more details.
>> > + *
>> > + * You should have received a copy of the GNU General Public License
>> > + * along with this program.  If not, see .
>>
>> The last paragraph is not necessary AFAICS.
>
> I just copy it from:
>
> https://www.gnu.org/licenses/gpl-howto.en.html
>
> If you are concerns about it, I can delete it.

It is redundant, so yes, please.

>> > +
>> > +static struct pwrseq *pwrseq_find_available_instance(struct device_node 
>> > *np)
>> > +{
>> > +   struct pwrseq *pwrseq;
>> > +
>> > +   list_for_each_entry(pwrseq, _list, node) {
>> > +   if (pwrseq->used)
>> > +   continue;
>> > +
>> > +   /* compare compatible string for pwrseq node */
>> > +   if (of_match_node(pwrseq->pwrseq_of_match_table, np)) {
>> > +   pwrseq->used = true;
>> > +   return pwrseq;
>> > +   }
>> > +
>> > +   /* return generic pwrseq instance */
>> > +   if (!strcmp(pwrseq->pwrseq_of_match_table->compatible,
>> > +   "generic")) {
>> > +   pr_debug("using generic pwrseq instance for %s\n",
>> > +   np->full_name);
>> > +   pwrseq->used = true;
>> > +   return pwrseq;
>> > +   }
>> > +   }
>> > +   pr_warn("Can't find any pwrseq instances for %s\n", np->full_name);
>>
>> pr_debug() ?
>
> If there is no pwrseq instance for that node, the power sequence on routine 
> will
> return fail, so I think an warning message is useful for user.

Useful in what way?  How is the user supposed to know what happened
from this message?

>>
>> > +
>> > +   return NULL;
>> > +}
>> > +
>> > +/**
>> > + * of_pwrseq_on: do power sequence on for device node
>>
>> of_pwrseq_on - Carry out power sequence on for device node
>>
>> Argument description should follow this line.
>>
>> > + *
>> > + * This API is used to power on single device, if the host
>> > + * controller only needs to handle one child device (this device
>> > + * node points to), use this API. If multiply devices are needed
>> > + * to handle on bus, use of_pwrseq_on_list.
>>
>> That's unclear.
>>
>> What about "Carry out a single device power on.  If multiple devices
>> need to be handled, use of_pwrseq_on_list() instead."
>>
>> > + *
>> > + * @np: the device node would like to power on
>> > + *
>> > + * On successful, it returns pwrseq instance, otherwise an error value.
>>
>> "Return a pointer to the power sequence instance on success, or an
>> error code otherwise."
>>
>
> Ok, will change.
>
>> > + */
>> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
>> > +{
>> > +   struct pwrseq *pwrseq;
>> > +   int ret;
>> > +
>> > +   pwrseq = pwrseq_find_available_instance(np);
>>
>> What does guarantee the integrity of ths list at this point?
>
> Once the use selects the specific pwrseq library, the library will
> create an empty one instance during the initialization, and it
> will be called at postcore_initcall, the device driver has not
> probed yet.

Which doesn't matter really, because the list is global and some other
driver using it might have been probed already.

You have a mutex here and it is used for add/remove.  Why isn't it
used for list browsing?

>
>>
>> > +   if (!pwrseq)
>> > +   return ERR_PTR(-ENONET);
>>
>> ENOENT I suppose?
>>
>
> Good catch, thanks.
>
>> > +/**
>> > + * of_pwrseq_off: do power sequence off for this pwrseq instance
>> > + *
>> > + * This API is used to power off single device, it is the opposite
>> > + * operation for of_pwrseq_on.
>> > + *
>> > + * @pwrseq: the pwrseq instance which related device would like to be off
>> > + */
>> > +void of_pwrseq_off(struct pwrseq *pwrseq)
>> > +{
>> > +   pwrseq_off(pwrseq);
>> > +   pwrseq_put(pwrseq);
>> > +}
>> > +EXPORT_SYMBOL_GPL(of_pwrseq_off);
>>
>> What happens if two code paths attempt to turn the same power sequence
>> off in parallel?  Can it ever happen?  If not, then why not?
>>
>
> I don't think the same pwrseq instance off will 

Re: [PATCH v13 00/10] usbip: exporting devices

[Krzysztof Opasiak]

On 11/23/2016 10:30 AM, Greg KH wrote:
> On Tue, Nov 22, 2016 at 03:48:09PM +0900, Nobuo Iwata wrote:
>> Dear all,
>>
>> This series of patches adds exporting device operation to USB/IP.
> 
> I would _love_ it if some of the people who are listed as MAINTAINERS of
> this code could actually review these patch series.  I don't think I've
> seen that happen yet, which isn't good...
> 

I'm not listed as maintainer but I used to be a tutor for students doing
vudc. I have some experience with USB/IP so I will try to help.

I tried to find some spare time for this series for quite a long time
and finally I got it!;)

I managed to review first 5 patches but it's getting late in Poland (at
least for sitting in an office in a front of computer;)) so I will get
back to this tomorrow.

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: hub: Wait for connection to be reestablished after port reset

[Guenter Roeck]

On a system with a defective USB device connected to an USB hub,
an endless sequence of port connect events was observed. The sequence
of events as observed is as follows:

- Port reports connected event (port status=USB_PORT_STAT_CONNECTION).
- Event handler debounces port and resets it by calling hub_port_reset().
- hub_port_reset() calls hub_port_wait_reset() to wait for the reset
  to complete.
- The reset completes, but USB_PORT_STAT_CONNECTION is not immediately
  set in the port status register.
- hub_port_wait_reset() returns -ENOTCONN.
- Port initialization sequence is aborted.
- A few milliseconds later, the port again reports a connected event,
  and the sequence repeats.

This continues either forever or, randomly, stops if the connection
is already re-established when the port status is read. It results in
a high rate of udev events. This in turn destabilizes userspace since
the above sequence holds the device mutex pretty much continuously
and prevents userspace from actually reading the device status.

To prevent the problem from happening, let's wait for the connection
to be re-established after a port reset. If the device was actually
disconnected, the code will still return an error, but it will do so
only after the long reset timeout.

Cc: Douglas Anderson 
Signed-off-by: Guenter Roeck 
---
 drivers/usb/core/hub.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index cbb146736f57..9bcb649e0e8c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2731,8 +2731,15 @@ static int hub_port_wait_reset(struct usb_hub *hub, int 
port1,
if (ret < 0)
return ret;
 
-   /* The port state is unknown until the reset completes. */
-   if (!(portstatus & USB_PORT_STAT_RESET))
+   /*
+* The port state is unknown until the reset completes.
+*
+* On top of that, some chips may require additional time
+* to re-establish a connection after the reset is complete,
+* so also wait for the connection to be re-established.
+*/
+   if (!(portstatus & USB_PORT_STAT_RESET) &&
+   (portstatus & USB_PORT_STAT_CONNECTION))
break;
 
/* switch to the long delay after two short delay failures */
-- 
2.5.0

--
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 v13 05/10] usbip: exporting devices: modifications to daemon

[Krzysztof Opasiak]

Hi,

On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
(...)

>  
> -static int do_accept(int listenfd)
> +static int do_accept(int listenfd, char *host, char *port)
>  {
>   int connfd;
>   struct sockaddr_storage ss;
>   socklen_t len = sizeof(ss);
> - char host[NI_MAXHOST], port[NI_MAXSERV];
>   int rc;
>  
>   memset(, 0, sizeof(ss));
> @@ -319,8 +124,8 @@ static int do_accept(int listenfd)
>   return -1;
>   }
>  
> - rc = getnameinfo((struct sockaddr *), len, host, sizeof(host),
> -  port, sizeof(port), NI_NUMERICHOST | NI_NUMERICSERV);
> + rc = getnameinfo((struct sockaddr *), len, host, NI_MAXHOST,
> +  port, NI_MAXSERV, NI_NUMERICHOST | NI_NUMERICSERV);

You have removed the host and port variables from here and placed them
in caller of this function, but you left here sizes of that variables.

It's not very good as at some point we may change the size of host and
port arrays and can easily forget to change those sizes here.

In my opinion you should pass also the size of those arrays to this
function instead of hardcoding them like this.

>   if (rc)
>   err("getnameinfo: %s", gai_strerror(rc));
>  
> @@ -334,6 +139,9 @@ static int do_accept(int listenfd)
>  #endif
>   info("connection from %s:%s", host, port);
>  
> + /* should set TCP_NODELAY for usbip */
> + usbip_net_set_nodelay(connfd);
> +
>   return connfd;
>  }
>  
> @@ -341,14 +149,15 @@ int process_request(int listenfd)
>  {
>   pid_t childpid;
>   int connfd;
> + char host[NI_MAXHOST], port[NI_MAXSERV];
>  
> - connfd = do_accept(listenfd);
> + connfd = do_accept(listenfd, host, port);
>   if (connfd < 0)
>   return -1;
>   childpid = fork();
>   if (childpid == 0) {
>   close(listenfd);
> - recv_pdu(connfd);
> + usbip_recv_pdu(connfd, host, port);
>   exit(0);
>   }
>   close(connfd);
> @@ -496,13 +305,13 @@ static int do_standalone_mode(int daemonize, int ipv4, 
> int ipv6)
>   struct timespec timeout;
>   sigset_t sigmask;
>  
> - if (usbip_driver_open(driver))
> + if (usbip_open_driver())

What's the purpose of this function?

>   return -1;
>  
>   if (daemonize) {
>   if (daemon(0, 0) < 0) {
>   err("daemonizing failed: %s", strerror(errno));
> - usbip_driver_close(driver);
> + usbip_close_driver();
>   return -1;
>   }
>   umask(0);
> @@ -511,7 +320,7 @@ static int do_standalone_mode(int daemonize, int ipv4, 
> int ipv6)
>   set_signal();
>   write_pid_file();
>  
> - info("starting " PROGNAME " (%s)", usbip_version_string);
> + info("starting %s (%s)", usbip_progname, usbip_version_string);
>  
>   /*
>* To suppress warnings on systems with bindv6only disabled
> @@ -527,7 +336,7 @@ static int do_standalone_mode(int daemonize, int ipv4, 
> int ipv6)
>  
>   ai_head = do_getaddrinfo(NULL, family);
>   if (!ai_head) {
> - usbip_driver_close(driver);
> + usbip_close_driver();

As above? Why not make this a global variable and leave those calls as
they were?

>   return -1;
>   }
>   nsockfd = listen_all_addrinfo(ai_head, sockfdlist,
> @@ -535,7 +344,7 @@ static int do_standalone_mode(int daemonize, int ipv4, 
> int ipv6)
>   freeaddrinfo(ai_head);
>   if (nsockfd <= 0) {
>   err("failed to open a listening socket");
> - usbip_driver_close(driver);
> + usbip_close_driver();
>   return -1;
>   }
>  
> @@ -574,9 +383,9 @@ static int do_standalone_mode(int daemonize, int ipv4, 
> int ipv6)
>   }
>   }
>  
> - info("shutting down " PROGNAME);
> + info("shutting down %s", usbip_progname);
>   free(fds);
> - usbip_driver_close(driver);
> + usbip_close_driver();
>  
>   return 0;
>  }
> @@ -587,7 +396,6 @@ int main(int argc, char *argv[])
>   { "ipv4", no_argument,   NULL, '4' },
>   { "ipv6", no_argument,   NULL, '6' },
>   { "daemon",   no_argument,   NULL, 'D' },
> - { "daemon",   no_argument,   NULL, 'D' },
>   { "debug",no_argument,   NULL, 'd' },
>   { "device",   no_argument,   NULL, 'e' },
>   { "pid",  optional_argument, NULL, 'P' },
> @@ -616,7 +424,7 @@ int main(int argc, char *argv[])
>   err("not running as root?");
>  
>   cmd = cmd_standalone_mode;
> - driver = _driver;
> + usbip_init_driver();
>   for (;;) {
>   opt = getopt_long(argc, argv, "46DdeP::t:hv", longopts, NULL);
>  
> @@ -640,7 +448,7 @@ int main(int argc, char *argv[])
>   cmd = cmd_help;
>   break;
>   case 'P':
> - 

Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command

[Baolin Wang]

On 2 December 2016 at 09:17, Lu Baolu  wrote:
> Hi,
>
> On 12/01/2016 04:03 PM, Baolin Wang wrote:
>> On 1 December 2016 at 15:44, Lu Baolu  wrote:
>>> Hi,
>>>
>>> On 12/01/2016 03:35 PM, Baolin Wang wrote:
 On 1 December 2016 at 14:35, Lu Baolu  wrote:
> Hi,
>
> On 12/01/2016 02:04 PM, Baolin Wang wrote:
>> Hi Baolu,
>>
>> On 1 December 2016 at 13:45, Lu Baolu  wrote:
>>> Hi,
>>>
>>> On 11/30/2016 05:02 PM, Baolin Wang wrote:
 If the hardware never responds to the stop endpoint command, the
 URBs will never be completed, and we might hang the USB subsystem.
 The original watchdog timer is used to watch if one stop endpoint
 command is timeout, if timeout, then the watchdog timer will set
 XHCI_STATE_DYING, try to halt the xHCI host, and give back all
 pending URBs.

 But now we already have one command timer to control command timeout,
 thus we can also use the command timer to watch the stop endpoint
 command, instead of one duplicate watchdog timer which need to be
 removed.

 Meanwhile we don't need the 'stop_cmds_pending' flag to identy if
 this is the last stop endpoint command of one endpoint. Since we
 can make sure we only set one stop endpoint command for one endpoint
 by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove
 this flag.
>>> I am afraid you can't do this. "stop_cmds_pending" was added
>>> to fix the problem described in the comments that you want to
>>> remove. But I didn't find any fix of this problem in your patch.
>> Now we can not pending another stop endpoint command for the same one
>> endpoint, since will check 'EP_HALT_PENDING' flag in
>> xhci_urb_dequeue() function to avoid this. But after some
>> investigation, I think I missed the stop endpoint command in
>> xhci_stop_device() which did not check the 'EP_HALT_PENDING' flag,
>> maybe need to add 'EP_HALT_PENDING' flag checking in
>> xhci_stop_device() function. DId I miss something else? Thanks.
> Consider below three threads running on different CPUs at the same time.
>
> Thread A: xhci_handle_cmd_stop_ep()  --- in interrupt handler
> Thread B: xhci_stop_endpoint_command_timeout() --- timer expired
> Thread C: xhci_urb_dequeue --- called by usb core
>
> They are serialized by xhci->lock. Let's consider below sequence:
>
> Thread A:
> - delete xhci->cmd_timer), but will fail due to Thread B.
> - clear EP_HALT_PENDING bit
>
> Thread B:
> - halt the host controller
>
> Thread C:
> - set EP_HALT_PENDING bit
> - enqueue another stop endpoint command
> - add the timer back
 Ah, thanks for you comments.
 If thread B halted the host, then the thread C xhci_urb_dequeue() will
 check host state failed, then just return and can not set another stop
 endpoint command.
>>> Not so simple. Thread B will release xhci->lock before xhci_halt().
>> Yes.
>>
 But from your example, I think your meaning is we
 should not halt the host by thread B, since we have handled the stop
 endpoint command event.

 So I think we need to check the xhci command of this stop endpoint
 command in xhci_stop_endpoint_command_timeout(), if the xhci command
 of this stop endpoint command is not in the command list (which means
 the stop endpoint command event has been handled), then just return
 and do not halt the controller. Right?

>>> I'd like suggest you to put this change into a separated patch.
>>> It's actually a different topic from the main purpose of this patch.
>> OK, I will. Thanks for your comments.
>>
>
> If you are going to work out a v2 patch, please consider whether
> we can totally leverage the common command mechanism to
> handle this stop endpoint command.
>
> Currently, when a stop endpoint command is issued for urb unlink,
> there are two timers for this command. This is duplicated and we
> should remove the stop-endpoint-only timer. The timeout functions
> are also different. The stop-endpoint-only timer just halt the host
> controller (this should be the last sort of way), while the common
> command timer will try to recover the situation by aborting and
> restart the command ring.

Yes, thanks for your reminding and I will think about that.

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

Re: [PATCH 1/1] usb: xhci: fix possible wild pointer

[Baolin Wang]

On 2 December 2016 at 10:29, Lu Baolu  wrote:
> handle_cmd_completion() frees a command structure which might
> be still referenced by xhci->current_cmd. This might cause
> problem when xhci->current_cmd is accessed after that.
>
> A real-life case could be like this. The host takes a very long
> time to respond to a command, and the command timer is fired at
> the same time when the command completion event arrives. The
> command completion handler frees xhci->current_cmd before the
> timer function can grab xhci->lock. Afterward, timer function
> grabs the lock and go ahead with checking and setting members
> of xhci->current_cmd.
>
> Cc:  # v3.16+
> Signed-off-by: Lu Baolu 

Nice catch. I was also curious where set xhci->current_cmd to be NULL
when current command is freed.
Tested-by: Baolin Wang 

> ---
>  drivers/usb/host/xhci-ring.c | 16 +++-
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index bdf6b13..13e05f6 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -1267,14 +1267,16 @@ void xhci_handle_command_timeout(unsigned long data)
> bool second_timeout = false;
> xhci = (struct xhci_hcd *) data;
>
> -   /* mark this command to be cancelled */
> spin_lock_irqsave(>lock, flags);
> -   if (xhci->current_cmd) {
> -   if (xhci->current_cmd->status == COMP_CMD_ABORT)
> -   second_timeout = true;
> -   xhci->current_cmd->status = COMP_CMD_ABORT;
> +   if (!xhci->current_cmd) {
> +   spin_unlock_irqrestore(>lock, flags);
> +   return;
> }
>
> +   if (xhci->current_cmd->status == COMP_CMD_ABORT)
> +   second_timeout = true;
> +   xhci->current_cmd->status = COMP_CMD_ABORT;
> +
> /* Make sure command ring is running before aborting it */
> hw_ring_state = xhci_read_64(xhci, >op_regs->cmd_ring);
> if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
> @@ -1422,6 +1424,10 @@ static void handle_cmd_completion(struct xhci_hcd 
> *xhci,
> xhci->current_cmd = list_entry(cmd->cmd_list.next,
>struct xhci_command, cmd_list);
> mod_timer(>cmd_timer, jiffies + 
> XHCI_CMD_DEFAULT_TIMEOUT);
> +   } else if (xhci->current_cmd == cmd) {
> +   xhci->current_cmd = NULL;
> +   } else {
> +   xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
> }
>
>  event_handled:
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command

[Baolin Wang]

On 1 December 2016 at 21:28, Mathias Nyman
 wrote:
> On 01.12.2016 06:54, Baolin Wang wrote:
>>
>> On 30 November 2016 at 22:09, Mathias Nyman
>>  wrote:
>>>
>>> On 30.11.2016 11:02, Baolin Wang wrote:


 If the hardware never responds to the stop endpoint command, the
 URBs will never be completed, and we might hang the USB subsystem.
 The original watchdog timer is used to watch if one stop endpoint
 command is timeout, if timeout, then the watchdog timer will set
 XHCI_STATE_DYING, try to halt the xHCI host, and give back all
 pending URBs.

 But now we already have one command timer to control command timeout,
 thus we can also use the command timer to watch the stop endpoint
 command, instead of one duplicate watchdog timer which need to be
 removed.

 Meanwhile we don't need the 'stop_cmds_pending' flag to identy if
 this is the last stop endpoint command of one endpoint. Since we
 can make sure we only set one stop endpoint command for one endpoint
 by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove
 this flag.

 We also need to clean up the command queue before trying to halt the
 xHCI host in xhci_stop_endpoint_command_timeout() function.
>>>
>>>
>>>
>>> This isn't a bad idea.
>>>
>>> There are anyway some corner cases and details that need to be
>>> checked, such as suspend (which will clear the command queue), module
>>> unload
>>> and abrupt host removal (like pci hotplug removal of host controller)
>>> we need to make sure we can trust the command timer to always return the
>>> canceled URB
>>
>>
>> Yes, you are right, we need to check these carefully.
>>
>> Suspend process, module unload and abrupt host removal, they all will
>> issue usb_disconnect() firstly before clear the command queue, it will
>> check URBs for every endpoint by
>> usb_disconnect()--->usb_disable_device()--->usb_disable_endpoint(),
>> which will make sure every URBs of endpoints will be cancelled by the
>> stop endpoint command responding or the timeout function of stop
>> endpoint command (xhci_stop_endpoint_command_timeout()) in
>> usb_hcd_flush_endpoint(). From that point, we can make sure the
>> command timer will be useful to handle stop endpoint command timeout.
>> Please correct me if I said something wrong. Thanks.
>>
>
> This relies on current queued command that times out to be the stop endpoint
> command.
>
> If host partially, or completely hangs there might be any number of commands
> in the
> command queue, and we would need to wait for each one of them to timeout,
> finish
> before we finally get to the stop endpoint command, and give back the urb.
>
> I think it would be better to first fix the issues with the current watchdog
> function, get
> those fixes into stable, and then think about moving to the command queue
> timer.

OK, make sense. Thanks.

>
> In short, this patch doesn't currently fix any existing issue, but might
> cause the
> timeout to be more unreliable
>
> -Mathias



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

[PATCH 1/1] usb: xhci: fix possible wild pointer

[Lu Baolu]

handle_cmd_completion() frees a command structure which might
be still referenced by xhci->current_cmd. This might cause
problem when xhci->current_cmd is accessed after that.

A real-life case could be like this. The host takes a very long
time to respond to a command, and the command timer is fired at
the same time when the command completion event arrives. The
command completion handler frees xhci->current_cmd before the
timer function can grab xhci->lock. Afterward, timer function
grabs the lock and go ahead with checking and setting members
of xhci->current_cmd.

Cc:  # v3.16+
Signed-off-by: Lu Baolu 
---
 drivers/usb/host/xhci-ring.c | 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index bdf6b13..13e05f6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1267,14 +1267,16 @@ void xhci_handle_command_timeout(unsigned long data)
bool second_timeout = false;
xhci = (struct xhci_hcd *) data;
 
-   /* mark this command to be cancelled */
spin_lock_irqsave(>lock, flags);
-   if (xhci->current_cmd) {
-   if (xhci->current_cmd->status == COMP_CMD_ABORT)
-   second_timeout = true;
-   xhci->current_cmd->status = COMP_CMD_ABORT;
+   if (!xhci->current_cmd) {
+   spin_unlock_irqrestore(>lock, flags);
+   return;
}
 
+   if (xhci->current_cmd->status == COMP_CMD_ABORT)
+   second_timeout = true;
+   xhci->current_cmd->status = COMP_CMD_ABORT;
+
/* Make sure command ring is running before aborting it */
hw_ring_state = xhci_read_64(xhci, >op_regs->cmd_ring);
if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
@@ -1422,6 +1424,10 @@ static void handle_cmd_completion(struct xhci_hcd *xhci,
xhci->current_cmd = list_entry(cmd->cmd_list.next,
   struct xhci_command, cmd_list);
mod_timer(>cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
+   } else if (xhci->current_cmd == cmd) {
+   xhci->current_cmd = NULL;
+   } else {
+   xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
}
 
 event_handled:
-- 
2.1.4

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

Re: [PATCH v13 00/10] usbip: exporting devices

[Shuah Khan]

On 11/23/2016 10:01 PM, fx IWATA NOBUO wrote:
> Hello,
> 
Looks like you removed the text. Let's go back to the goals.
Can we use server and client terminology which is we use in
instead of App and Dev. It makes lot easier to understand.

https://www.kernel.org/doc/readme/tools-usb-usbip-README

>>1. Goal

>>1-1) To give flexibility to direction of connection
>>Using USB/IP in internet, there can be two cases.
>> a) an application is inside firewall and devices are outside.

This is the case Client is behind firewall and server is outside
the firewall. 

>>b) devices are inside firewall and an application is inside.

I am not clear on this. Is Client and server behind different
firewalls?

>>In case-a, import works because the connection is from inside.

> EXISTING)
> APP# usbip attach ---(passed)> DEV# usbipd
> DEV# usbipd (blocked)|| <- APP# usbip attach

So the above doesn't apply to a) and it works?

>> In case-b, import doesn't works because the connection is from outside.

What does this mean? Why is this connection from outside?
 
>> Connection from device side is needed. This patch set adds the 
>> direction of connection establishment.

Is this something that could be solved by opening ports in the firewall?


>> Can you elaborate on the use-case a bit more? What does it mean to
>> "Connection from device side is needed"?
> 
> I'd like to update ending part of use case as following.
> ---
> Firewall, proxy, or router in front of internet usually blocks
> connections from internet regarding all TCP ports. They opens some
> ports, usually HTTP(80) and HTTPS(443), for connection from inside.
> In combination with WebSocket proxy, USB/IP can establish connection
> from inside of the firewall.
> 
> Enterprise/SOHO/Home   Firewall/Proxy/Router   Internet
> 
> EXISTING)
> APP# usbip attach ---(passed)> DEV# usbipd
> DEV# usbipd (blocked)|| <- APP# usbip attach
> 
> NEW)
> DEV# usbip connect --(passed)> DEV# usbipa
> APP# usbipa (blocked)|| <- APP# usbip connect
> 
> Attach operation can invite devices in internet but cannot invite
> devices from internet. On the other hand, connect operation can dedicate
> devices to internet but cannot dedicate devices in internet.

The above isn't very clear. Do you mean to say, client can find devices
exported by a server? When you say Internet how many servers are we
looking at? Random servers or known servers?

Current USB/IP use-cases are:

- Server could export USB devices to virtual machines running on it.
  Access is confined and limited to that one system.
- Server could export USB devices and clients as long as no firewalls
  in between.

In the above two models, user has permissions to use server and client.
In this new proposed model, how does this work? Who sets up the server
to export or push exports to clients. How does server know which clients
to push exports to?

With a feature like this, it is important to understand the use-cases in
detail.

thanks,
-- Shuah

> ---
> 
>> I would like to see server side and client side operations clearly.
>> It would help me understand the use-case you are trying to add.
> 
> It's shown in README and manual page added by patch 9/10 as below.
> 
> app:# insmod usbip-core.ko
> app:# insmod vhci-hcd.ko
> 
> app:# usbipa -D
> - Start usbip daemon.
> 
> dev:# (Physically attach your USB device.)
> 
> dev:# insmod usbip-core.ko
> dev:# insmod usbip-host.ko
> 
> dev:# usbip list -l
> - List driver assignments for USB devices and their busid.
> 
> dev:# usbip connect --remote  --busid 
> - Bind usbip-host.ko to the device with .
> - The USB device of  is connected to remote host!
> 
> dev:# usbip disconnect --remote  --busid 
> - The USB device with  is disconnected from remote host.
> - Unbind usbip-host.ko from the device.
> 
>> I do have some concerns about security on client side. User sits on
>> the client side and it should be a pull from client side as opposed to
>> push from server side.
> 
> Connection level consideration is described in "5. Security
>  Consideration". As you know, USB/IP already has TCP wrapper option.
> With SSL or WebSocket(wss) tunneling proxy, client authentication can
> be applied using the proxies.
> 
> I didn't write device level consideration. It's same as local device
> plug-and-play. I will add description below in cover letter and README.
> 
> ---
> Udev rules can allow only known devices. To identify a device is remote,
> the local bus-id (KERNEL parameter in the rule) will be found the last
> in column of /sys/devices/platform/vhci_hcd/status[.N]. When device is
> found, the port number in USB/IP can be found in the first column of the
> matched line. The udev can finish the connection using detach operation
> with the port number.
> ---
> 
>> It sounds like this patch series adds push from server 

Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command

[Lu Baolu]

Hi,

On 12/01/2016 04:03 PM, Baolin Wang wrote:
> On 1 December 2016 at 15:44, Lu Baolu  wrote:
>> Hi,
>>
>> On 12/01/2016 03:35 PM, Baolin Wang wrote:
>>> On 1 December 2016 at 14:35, Lu Baolu  wrote:
 Hi,

 On 12/01/2016 02:04 PM, Baolin Wang wrote:
> Hi Baolu,
>
> On 1 December 2016 at 13:45, Lu Baolu  wrote:
>> Hi,
>>
>> On 11/30/2016 05:02 PM, Baolin Wang wrote:
>>> If the hardware never responds to the stop endpoint command, the
>>> URBs will never be completed, and we might hang the USB subsystem.
>>> The original watchdog timer is used to watch if one stop endpoint
>>> command is timeout, if timeout, then the watchdog timer will set
>>> XHCI_STATE_DYING, try to halt the xHCI host, and give back all
>>> pending URBs.
>>>
>>> But now we already have one command timer to control command timeout,
>>> thus we can also use the command timer to watch the stop endpoint
>>> command, instead of one duplicate watchdog timer which need to be
>>> removed.
>>>
>>> Meanwhile we don't need the 'stop_cmds_pending' flag to identy if
>>> this is the last stop endpoint command of one endpoint. Since we
>>> can make sure we only set one stop endpoint command for one endpoint
>>> by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove
>>> this flag.
>> I am afraid you can't do this. "stop_cmds_pending" was added
>> to fix the problem described in the comments that you want to
>> remove. But I didn't find any fix of this problem in your patch.
> Now we can not pending another stop endpoint command for the same one
> endpoint, since will check 'EP_HALT_PENDING' flag in
> xhci_urb_dequeue() function to avoid this. But after some
> investigation, I think I missed the stop endpoint command in
> xhci_stop_device() which did not check the 'EP_HALT_PENDING' flag,
> maybe need to add 'EP_HALT_PENDING' flag checking in
> xhci_stop_device() function. DId I miss something else? Thanks.
 Consider below three threads running on different CPUs at the same time.

 Thread A: xhci_handle_cmd_stop_ep()  --- in interrupt handler
 Thread B: xhci_stop_endpoint_command_timeout() --- timer expired
 Thread C: xhci_urb_dequeue --- called by usb core

 They are serialized by xhci->lock. Let's consider below sequence:

 Thread A:
 - delete xhci->cmd_timer), but will fail due to Thread B.
 - clear EP_HALT_PENDING bit

 Thread B:
 - halt the host controller

 Thread C:
 - set EP_HALT_PENDING bit
 - enqueue another stop endpoint command
 - add the timer back
>>> Ah, thanks for you comments.
>>> If thread B halted the host, then the thread C xhci_urb_dequeue() will
>>> check host state failed, then just return and can not set another stop
>>> endpoint command.
>> Not so simple. Thread B will release xhci->lock before xhci_halt().
> Yes.
>
>>> But from your example, I think your meaning is we
>>> should not halt the host by thread B, since we have handled the stop
>>> endpoint command event.
>>>
>>> So I think we need to check the xhci command of this stop endpoint
>>> command in xhci_stop_endpoint_command_timeout(), if the xhci command
>>> of this stop endpoint command is not in the command list (which means
>>> the stop endpoint command event has been handled), then just return
>>> and do not halt the controller. Right?
>>>
>> I'd like suggest you to put this change into a separated patch.
>> It's actually a different topic from the main purpose of this patch.
> OK, I will. Thanks for your comments.
>

If you are going to work out a v2 patch, please consider whether
we can totally leverage the common command mechanism to
handle this stop endpoint command.

Currently, when a stop endpoint command is issued for urb unlink,
there are two timers for this command. This is duplicated and we
should remove the stop-endpoint-only timer. The timeout functions
are also different. The stop-endpoint-only timer just halt the host
controller (this should be the last sort of way), while the common
command timer will try to recover the situation by aborting and
restart the command ring.

Best regards,
Lu Baolu
--
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 06/12] usb: dwc3: omap: Replace the extcon API

[Chanwoo Choi]

Hi Felipe,

On 2016년 11월 30일 19:36, Felipe Balbi wrote:
> 
> Hi,
> 
> Chanwoo Choi  writes:
>> This patch uses the resource-managed extcon API for 
>> extcon_register_notifier()
>> and replaces the deprecated extcon API as following:
>> - extcon_get_cable_state_() -> extcon_get_state()
>>
>> Signed-off-by: Chanwoo Choi 
> 
> Acked-by: Felipe Balbi 
> 

Thanks for your review.

Each patch has no any dependency among patches.
So, If possible, could you pick the patch6/8/9/10/11/12 on your tree?

-- 
Best Regards,
Chanwoo Choi
--
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 v13 03/10] usbip: exporting devices: new connect operation

[fx IWATA NOBUO]

Hello,

> In my humble opinion connect is not a best name for this operation.

Yes. I think connect is not perfect.

> it not only starts the connection to a remote server but also> exports
> a device. I think that a name like export or attach_to_remote would be
> more suitable. (...)

I think one word name is better.

Considering that attach is corresponding to import request, I think
export is not good. Furthermore, export is already used in many places
of the code, ex. usbip_host_common.c: usbip_export_device(). So I don't
want to use export.

Operation | PDU | Description
--+-+-
attach|import   |invite a device
detach|NA   |quit invited
( )   |export   |dedicate a device
( )   |un-export|quit dedicated

I think connect/disconnect is not bad.
I will change it if there's user friendly, well-known and suit with
existing name.

> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> Probably you don't need to 0 the reply as it is buffer for receiving
> and it's not being send anywhere.

I will remove it.
I will remove from send_unexport_device() in 4/10 too.

usbip_attach.c:query_import_device() has same memset.
I will fix it outside of this patch set.

> > +   memcpy(, udev, sizeof(struct usbip_usb_device));
> 
> how about sizeof(request.udev)?

I will change it.

I will grep sizeof and change to fit to nearby them.

> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> This check should probably look different or at least should be
> documented. Please refer to my comments to patch #1.

It will be removed.

> Take a look at all above function calls and your if.
> 
> You have usbip_driver_close() called 4 times in 4 error path. This is
> just asking to replace this with goto and place error path below
> return just like you did one function below.

I will fix it.

usbip_attach.c has same logic so I will add refactoring patch to
usbip_attach.c. regarding memset() and multiple driver_close.

Thank you for your help,

n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 5:28 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 03/10] usbip: exporting devices: new connect
> operation
> 
> Hi,
> 
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> > Implementation of new connect operation. This is linked as a part of
> > usbip commnad. With this patch, usbip command has following operations.
> >
> >  bind
> >  unbind
> >  list (local/remote)
> >  attach
> >  detach
> >  port
> >  connect ... this patch
> 
> 
> In my humble opinion connect is not a best name for this operation.
> it not only starts the connection to a remote server but also exports a
> device. I think that a name like export or attach_to_remote would be more
> suitable.
> (...)
> 
> > +static const char usbip_connect_usage_string[] =
> > +   "usbip connect \n"
> > +   "-r, --remote=Address of a remote computer\n"
> > +   "-b, --busid=Bus ID of a device to be connected\n"
> > +   "-d, --device   Run with an alternate driver, e.g.
> vUDC\n";
> > +
> > +void usbip_connect_usage(void)
> > +{
> > +   printf("usage: %s", usbip_connect_usage_string); }
> > +
> > +static int send_export_device(int sockfd, struct usbip_usb_device
> > +*udev) {
> > +   int rc;
> > +   struct op_export_request request;
> > +   struct op_export_reply   reply;
> > +   uint16_t code = OP_REP_EXPORT;
> > +
> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> Probably you don't need to 0 the reply as it is buffer for receiving and
> it's not being send anywhere.
> 
> > +
> > +   /* send a request */
> > +   rc = usbip_net_send_op_common(sockfd, OP_REQ_EXPORT, 0);
> > +   if (rc < 0) {
> > +   err("send op_common");
> > +   return -1;
> > +   }
> > +
> > +   memcpy(, udev, sizeof(struct usbip_usb_device));
> 
> how about sizeof(request.udev)?
> 
> > +
> > +   PACK_OP_EXPORT_REQUEST(0, );
> > +
> > +   rc = usbip_net_send(sockfd, (void *) , sizeof(request));
> > +   if (rc < 0) {
> > +   err("send op_export_request");
> > +   return -1;
> > +   }
> > +
> > +   /* receive a reply */
> > +   rc = usbip_net_recv_op_common(sockfd, );
> > +   if (rc < 0) {
> > +   err("recv op_common");
> > +   return -1;
> > +   }
> > +
> > +   rc = usbip_net_recv(sockfd, (void *) , sizeof(reply));
> > +   if (rc < 0) {
> > +   err("recv op_export_reply");
> > +   return -1;
> > +   }
> > +
> > +   PACK_OP_EXPORT_REPLY(0, );
> > +
> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> 

RE: [PATCH v13 04/10] usbip: exporting devices: new disconnect operation

[fx IWATA NOBUO]

> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> As in previous patch, you don't need to 0 the reply.

I will remove the memset.

> sizeof(request.udev)?

I will modify.

> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> The same case with error code as in previous patch.

It will be removed.

> Once again the same pattern as in previous patch 3 times in a row copy-pasted
> error path. Please use goto here and place error path below return.

I will fix it.

> close(sockfd) in case of error, otherwise close(sockfd)...
> 
> Probably you can place close(sockfd) above if to avoid this weird
> copy-paste.

I will fix it.

> > +   ret = disconnect_device(host, busid, unbind);
> > +   goto out;
> 
> This looks a little bit weird... why you use goto out instead of just return
> ret?

err_out has usbip_disconnect_usage().

Each usbip_.c has same pattern.

Thank you,

n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 5:37 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 04/10] usbip: exporting devices: new disconnect
> operation
> 
> 
> 
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> (...)
> > +
> > +static int send_unexport_device(int sockfd, struct usbip_usb_device
> > +*udev) {
> > +   int rc;
> > +   struct op_unexport_request request;
> > +   struct op_unexport_reply   reply;
> > +   uint16_t code = OP_REP_UNEXPORT;
> > +
> > +   memset(, 0, sizeof(request));
> > +   memset(, 0, sizeof(reply));
> 
> As in previous patch, you don't need to 0 the reply.
> 
> > +
> > +   /* send a request */
> > +   rc = usbip_net_send_op_common(sockfd, OP_REQ_UNEXPORT, 0);
> > +   if (rc < 0) {
> > +   err("send op_common");
> > +   return -1;
> > +   }
> > +
> > +   memcpy(, udev, sizeof(struct usbip_usb_device));
> 
> sizeof(request.udev)?
> 
> > +
> > +   PACK_OP_UNEXPORT_REQUEST(0, );
> > +
> > +   rc = usbip_net_send(sockfd, (void *) , sizeof(request));
> > +   if (rc < 0) {
> > +   err("send op_export_request");
> > +   return -1;
> > +   }
> > +
> > +   /* receive a reply */
> > +   rc = usbip_net_recv_op_common(sockfd, );
> > +   if (rc < 0) {
> > +   err("recv op_common");
> > +   return -1;
> > +   }
> > +
> > +   rc = usbip_net_recv(sockfd, (void *) , sizeof(reply));
> > +   if (rc < 0) {
> > +   err("recv op_unexport_reply");
> > +   return -1;
> > +   }
> > +
> > +   PACK_OP_EXPORT_REPLY(0, );
> > +
> > +   /* check the reply */
> > +   if (reply.returncode) {
> > +   err("recv error return %d", reply.returncode);
> > +   return -1;
> > +   }
> 
> The same case with error code as in previous patch.
> 
> > +
> > +   return 0;
> > +}
> > +
> > +static int unexport_device(char *busid, int sockfd) {
> > +   int rc;
> > +   struct usbip_exported_device *edev;
> > +
> > +   rc = usbip_driver_open(driver);
> > +   if (rc < 0) {
> > +   err("open driver");
> > +   return -1;
> > +   }
> > +
> > +   rc = usbip_refresh_device_list(driver);
> > +   if (rc < 0) {
> > +   err("could not refresh device list");
> > +   usbip_driver_close(driver);
> > +   return -1;
> > +   }
> > +
> > +   edev = usbip_get_device(driver, busid);
> > +   if (edev == NULL) {
> > +   err("find device");
> > +   usbip_driver_close(driver);
> > +   return -1;
> > +   }
> > +
> > +   rc = send_unexport_device(sockfd, >udev);
> > +   if (rc < 0) {
> > +   err("send unexport");
> > +   usbip_driver_close(driver);
> > +   return -1;
> > +   }
> 
> Once again the same pattern as in previous patch 3 times in a row copy-pasted
> error path. Please use goto here and place error path below return.
> 
> > +
> > +   usbip_driver_close(driver);
> > +
> > +   return 0;
> > +}
> > +
> > +static int disconnect_device(char *host, char *busid, int unbind) {
> > +   int sockfd;
> > +   int rc;
> > +
> > +   sockfd = usbip_net_tcp_connect(host, usbip_port_string);
> > +   if (sockfd < 0) {
> > +   err("tcp connect");
> > +   return -1;
> > +   }
> > +
> > +   rc = unexport_device(busid, sockfd);
> > +   if (rc < 0) {
> > +   err("unexport");
> > +   close(sockfd);
> > +   return -1;
> > +   }
> 
> close(sockfd) in case of error, otherwise close(sockfd)...
> 
> Probably you can place close(sockfd) above if to avoid this weird
> copy-paste.
> 
> > +
> > +   close(sockfd);
> > +
> > +   if (unbind) {
> > +   rc = usbip_unbind_device(busid);
> > +   if (rc) {
> > +   err("unbind");
> > +   return -1;
> > +   }
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > 

Re: [RFC] usb: host: xhci: Remove the watchdog timer and use command timer to watch stop endpoint command

[Baolin Wang]

On 1 December 2016 at 15:44, Lu Baolu  wrote:
> Hi,
>
> On 12/01/2016 03:35 PM, Baolin Wang wrote:
>> On 1 December 2016 at 14:35, Lu Baolu  wrote:
>>> Hi,
>>>
>>> On 12/01/2016 02:04 PM, Baolin Wang wrote:
 Hi Baolu,

 On 1 December 2016 at 13:45, Lu Baolu  wrote:
> Hi,
>
> On 11/30/2016 05:02 PM, Baolin Wang wrote:
>> If the hardware never responds to the stop endpoint command, the
>> URBs will never be completed, and we might hang the USB subsystem.
>> The original watchdog timer is used to watch if one stop endpoint
>> command is timeout, if timeout, then the watchdog timer will set
>> XHCI_STATE_DYING, try to halt the xHCI host, and give back all
>> pending URBs.
>>
>> But now we already have one command timer to control command timeout,
>> thus we can also use the command timer to watch the stop endpoint
>> command, instead of one duplicate watchdog timer which need to be
>> removed.
>>
>> Meanwhile we don't need the 'stop_cmds_pending' flag to identy if
>> this is the last stop endpoint command of one endpoint. Since we
>> can make sure we only set one stop endpoint command for one endpoint
>> by 'EP_HALT_PENDING' flag in xhci_urb_dequeue() function. Thus remove
>> this flag.
> I am afraid you can't do this. "stop_cmds_pending" was added
> to fix the problem described in the comments that you want to
> remove. But I didn't find any fix of this problem in your patch.
 Now we can not pending another stop endpoint command for the same one
 endpoint, since will check 'EP_HALT_PENDING' flag in
 xhci_urb_dequeue() function to avoid this. But after some
 investigation, I think I missed the stop endpoint command in
 xhci_stop_device() which did not check the 'EP_HALT_PENDING' flag,
 maybe need to add 'EP_HALT_PENDING' flag checking in
 xhci_stop_device() function. DId I miss something else? Thanks.
>>> Consider below three threads running on different CPUs at the same time.
>>>
>>> Thread A: xhci_handle_cmd_stop_ep()  --- in interrupt handler
>>> Thread B: xhci_stop_endpoint_command_timeout() --- timer expired
>>> Thread C: xhci_urb_dequeue --- called by usb core
>>>
>>> They are serialized by xhci->lock. Let's consider below sequence:
>>>
>>> Thread A:
>>> - delete xhci->cmd_timer), but will fail due to Thread B.
>>> - clear EP_HALT_PENDING bit
>>>
>>> Thread B:
>>> - halt the host controller
>>>
>>> Thread C:
>>> - set EP_HALT_PENDING bit
>>> - enqueue another stop endpoint command
>>> - add the timer back
>> Ah, thanks for you comments.
>> If thread B halted the host, then the thread C xhci_urb_dequeue() will
>> check host state failed, then just return and can not set another stop
>> endpoint command.
>
> Not so simple. Thread B will release xhci->lock before xhci_halt().

Yes.

>
>> But from your example, I think your meaning is we
>> should not halt the host by thread B, since we have handled the stop
>> endpoint command event.
>>
>> So I think we need to check the xhci command of this stop endpoint
>> command in xhci_stop_endpoint_command_timeout(), if the xhci command
>> of this stop endpoint command is not in the command list (which means
>> the stop endpoint command event has been handled), then just return
>> and do not halt the controller. Right?
>>
>
> I'd like suggest you to put this change into a separated patch.
> It's actually a different topic from the main purpose of this patch.

OK, I will. Thanks for your comments.

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

Re: [RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver

[Kishon Vijay Abraham I]

Hi,

On Wednesday 23 November 2016 09:16 AM, John Stultz wrote:
> This wires extconn support to hikey's phy driver, and
> connects it to the usb UDC layer via a usb_phy structure.
> 
> Not sure if this is the right way to connect phy -> UDC,
> but I'm lacking a clear example.
> 
> Cc: Wei Xu 
> Cc: Guodong Xu 
> Cc: Amit Pundir 
> Cc: Rob Herring 
> Cc: John Youn 
> Cc: Douglas Anderson 
> Cc: Chen Yu 
> Cc: Kishon Vijay Abraham I 
> Cc: Felipe Balbi 
> Cc: Greg Kroah-Hartman 
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: John Stultz 
> ---
>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  11 +++
>  drivers/phy/Kconfig   |   2 +
>  drivers/phy/phy-hi6220-usb.c  | 139 
> ++
>  3 files changed, 152 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> index 17839db..171fbb2 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
> @@ -732,10 +732,21 @@
>   regulator-always-on;
>   };
>  
> + usb_vbus: usb-vbus {
> + compatible = "linux,extcon-usb-gpio";
> + id-gpio = < 6 1>;
> + };
> +
> + usb_id: usb-id {
> + compatible = "linux,extcon-usb-gpio";
> + id-gpio = < 5 1>;
> + };
> +
>   usb_phy: usbphy {
>   compatible = "hisilicon,hi6220-usb-phy";
>   #phy-cells = <0>;
>   phy-supply = <_5v_hub>;
> + extcon = <_vbus>, <_id>;
>   hisilicon,peripheral-syscon = <_ctrl>;
>   };
>  
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index fe00f91..76f4f17 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
>  config PHY_HI6220_USB
>   tristate "hi6220 USB PHY support"
>   depends on (ARCH_HISI && ARM64) || COMPILE_TEST
> + depends on EXTCON
>   select GENERIC_PHY
>   select MFD_SYSCON
> + select USB_PHY
>   help
> Enable this to support the HISILICON HI6220 USB PHY.
>  
> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
> index b2141cb..89d8475 100644
> --- a/drivers/phy/phy-hi6220-usb.c
> +++ b/drivers/phy/phy-hi6220-usb.c
> @@ -12,7 +12,12 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +#include 
>  #include 
> +#include 
>  
>  #define SC_PERIPH_CTRL4  0x00c
>  
> @@ -44,9 +49,21 @@
>  
>  #define EYE_PATTERN_PARA 0x7053348c
>  
> +
> +struct hi6220_usb_cable {
> + struct notifier_block   nb;
> + struct extcon_dev   *extcon;
> + int state;
> +};
> +
>  struct hi6220_priv {
>   struct regmap *reg;
>   struct device *dev;
> + struct usb_phy phy;
> +
> + struct delayed_work work;
> + struct hi6220_usb_cable vbus;
> + struct hi6220_usb_cable id;
>  };
>  
>  static void hi6220_phy_init(struct hi6220_priv *priv)
> @@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
>   return hi6220_phy_setup(priv, false);
>  }
>  
> +
>  static struct phy_ops hi6220_phy_ops = {
>   .init   = hi6220_phy_start,
>   .exit   = hi6220_phy_exit,
>   .owner  = THIS_MODULE,
>  };
>  
> +static void hi6220_detect_work(struct work_struct *work)
> +{
> + struct hi6220_priv *priv =
> + container_of(to_delayed_work(work), struct hi6220_priv, work);
> + struct usb_otg *otg = priv->phy.otg;
> +
> + if (!IS_ERR(priv->vbus.extcon))
> + priv->vbus.state = extcon_get_cable_state_(priv->vbus.extcon,
> +  EXTCON_USB);
> + if (!IS_ERR(priv->id.extcon))
> + priv->id.state = extcon_get_cable_state_(priv->id.extcon,
> +  EXTCON_USB_HOST);
> + if (otg->gadget) {
> + if (priv->id.state)
> + usb_gadget_vbus_connect(otg->gadget);
> + else
> + usb_gadget_vbus_disconnect(otg->gadget);
> + }
> +}
> +
> +static int hi6220_otg_vbus_notifier(struct notifier_block *nb,
> + unsigned long event, void *ptr)
> +{
> + struct hi6220_usb_cable *vbus = container_of(nb,
> + struct hi6220_usb_cable, nb);
> + struct hi6220_priv *priv = container_of(vbus,
> + struct hi6220_priv, vbus);
> +
> + 

Re: [PATCH 2/2] Synopsys USB 2.0 Device Controller (UDC) Driver

[Felipe Balbi]

Hi,

John Youn  writes:
> On 11/30/2016 4:47 AM, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Raviteja Garimella  writes:
>>> Hi Balbi,
>>>
>>> On Wed, Nov 30, 2016 at 4:10 PM, Felipe Balbi  wrote:

 Hi,

 Raviteja Garimella  writes:
> This is driver for Synopsys Designware Cores USB Device
> Controller (UDC) Subsystem with the AMBA Advanced High-Performance
> Bus (AHB). This driver works with Synopsys UDC20 products.
>
> Signed-off-by: Raviteja Garimella 

 use drivers/usb/dwc2 instead of duplicating it.
>>>
>>> The ones we have in drivers/usb/dwc2 is for Designware high speed OTG
>>> controller IP. The one that I submitted for review is for USB Device
>>> controller IP (UDC). The IPs are different.
>> 
>> I'll wait for John's confirmation that this really isn't compatible with
>> dwc2. John?
>> 
>
> Hi Felipe,
>
> This is our older UDC IP, not compatible with HSOTG.
>
> It is also no longer supported by Synopsys and considered EOL.

Is it the same one used by amd5536udc.c? If it is, then it's much better
to refactor that driver so it can be used as a library of sorts by PCI
and non-PCI systems. We really don't want duplicated drivers upstream.

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb: dwc2: fix flags for DMA descriptor allocation in dwc2_hsotg_ep_enable

[Marek Szyprowski]

dwc2_hsotg_ep_enable can be called from interrupt context, so all
allocations should be done with GFP_ATOMIC flags. This fixes following
issue on ARM architecture:

[] (unwind_backtrace) from [] (show_stack+0x10/0x14)
[] (show_stack) from [] (dump_stack+0x74/0x94)
[] (dump_stack) from [] (__warn+0xd4/0x100)
[] (__warn) from [] (warn_slowpath_null+0x20/0x28)
[] (warn_slowpath_null) from [] 
(smp_call_function_many+0xcc/0x2a4)
[] (smp_call_function_many) from [] 
(on_each_cpu_mask+0x38/0xa8)
[] (on_each_cpu_mask) from [] 
(start_isolate_page_range+0x134/0x1b8)
[] (start_isolate_page_range) from [] 
(alloc_contig_range+0xac/0x2f8)
[] (alloc_contig_range) from [] (cma_alloc+0xe0/0x1a8)
[] (cma_alloc) from [] (__alloc_from_contiguous+0x38/0xe0)
[] (__alloc_from_contiguous) from [] 
(cma_allocator_alloc+0x30/0x38)
[] (cma_allocator_alloc) from [] (__dma_alloc+0x1c0/0x2c8)
[] (__dma_alloc) from [] (arm_dma_alloc+0x3c/0x48)
[] (arm_dma_alloc) from [] (dwc2_hsotg_ep_enable+0xec/0x46c)
[] (dwc2_hsotg_ep_enable) from [] (usb_ep_enable+0x2c/0x3c)
[] (usb_ep_enable) from [] (ecm_set_alt+0xa8/0x154)
[] (ecm_set_alt) from [] (composite_setup+0xd74/0x1540)
[] (composite_setup) from [] 
(dwc2_hsotg_complete_setup+0xb8/0x370)
[] (dwc2_hsotg_complete_setup) from [] 
(usb_gadget_giveback_request+0xc/0x10)
[] (usb_gadget_giveback_request) from [] 
(dwc2_hsotg_complete_request+0x78/0x128)
[] (dwc2_hsotg_complete_request) from [] 
(dwc2_hsotg_epint+0x69c/0x81c)
[] (dwc2_hsotg_epint) from [] (dwc2_hsotg_irq+0xfc/0x748)
[] (dwc2_hsotg_irq) from [] 
(__handle_irq_event_percpu+0x58/0x140)
[] (__handle_irq_event_percpu) from [] 
(handle_irq_event_percpu+0x1c/0x58)
[] (handle_irq_event_percpu) from [] 
(handle_irq_event+0x38/0x5c)
[] (handle_irq_event) from [] 
(handle_fasteoi_irq+0xc4/0x19c)
[] (handle_fasteoi_irq) from [] 
(generic_handle_irq+0x18/0x28)
[] (generic_handle_irq) from [] 
(__handle_domain_irq+0x6c/0xe4)
[] (__handle_domain_irq) from [] (gic_handle_irq+0x50/0x9c)
[] (gic_handle_irq) from [] (__irq_svc+0x6c/0xa8)

Fixes: 5f54c54b0ba83 ("usb: dwc2: gadget: Add DDMA chain pointers to
dwc2_hsotg_ep structure")
Signed-off-by: Marek Szyprowski 
---
 drivers/usb/dwc2/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index b95930f20d90..c55db4aa54d6 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3753,7 +3753,7 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
hs_ep->desc_list = dma_alloc_coherent(hsotg->dev,
MAX_DMA_DESC_NUM_GENERIC *
sizeof(struct dwc2_dma_desc),
-   _ep->desc_list_dma, GFP_KERNEL);
+   _ep->desc_list_dma, GFP_ATOMIC);
if (!hs_ep->desc_list) {
ret = -ENOMEM;
goto error2;
-- 
1.9.1

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

[GIT PULL] USB-serial updates for v4.10-rc1

[Johan Hovold]

Hi Greg,

Here are my updates for 4.10. Details below.

Thanks,
Johan


The following changes since commit 07d9a380680d1c0eb51ef87ff2eab5c994949e69:

  Linux 4.9-rc2 (2016-10-23 17:10:14 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git 
tags/usb-serial-4.10-rc1

for you to fetch changes up to 3c3dd1e058cb01e835dcade4b54a6f13ffaeaf7c:

  USB: serial: kl5kusb105: abort on open exception path (2016-11-30 10:54:30 
+0100)


USB-serial updates for v4.10-rc1

These updates include a new driver for Fintek F8153x devices, support
for the GPIO functionality on CP2105 devices, and improved support for
CH34X devices.

Included are also some clean ups and fixes for various minor issues.

Signed-off-by: Johan Hovold 


Aidan Thornton (4):
  USB: serial: ch341: add register and USB request definitions
  USB: serial: ch341: reinitialize chip on reconfiguration
  USB: serial: ch341: add support for parity, frame length, stop bits
  USB: serial: ch341: add debug output for chip version

Geert Uytterhoeven (1):
  USB: serial: cp210x: use tcflag_t to fix incompatible pointer type

Ji-Ze Hong (Peter Hong) (1):
  USB: serial: add Fintek F81532/534 driver

Johan Hovold (5):
  USB: serial: cp210x: clean up CSIZE handling
  USB: serial: cp210x: return -EIO on short control transfers
  USB: serial: cp210x: use bool for registered flag
  USB: serial: fix invalid user-pointer checks
  USB: serial: kl5kusb105: fix open error path

Martyn Welch (1):
  USB: serial: cp210x: Adding GPIO support for CP2105

Pan Bian (1):
  USB: serial: kl5kusb105: abort on open exception path

 drivers/usb/serial/Kconfig|   10 +
 drivers/usb/serial/Makefile   |1 +
 drivers/usb/serial/ch341.c|  113 ++-
 drivers/usb/serial/cp210x.c   |  411 +-
 drivers/usb/serial/f81534.c   | 1409 +
 drivers/usb/serial/ftdi_sio.c |5 -
 drivers/usb/serial/io_edgeport.c  |3 -
 drivers/usb/serial/io_ti.c|3 -
 drivers/usb/serial/kl5kusb105.c   |   35 +-
 drivers/usb/serial/mos7720.c  |3 -
 drivers/usb/serial/mos7840.c  |3 -
 drivers/usb/serial/opticon.c  |3 -
 drivers/usb/serial/quatech2.c |3 -
 drivers/usb/serial/ssu100.c   |3 -
 drivers/usb/serial/ti_usb_3410_5052.c |3 -
 drivers/usb/serial/usb_wwan.c |3 -
 16 files changed, 1914 insertions(+), 97 deletions(-)
 create mode 100644 drivers/usb/serial/f81534.c
--
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: gadget: udc: atmel: used managed kasprintf

[Nicolas Ferre]

Le 01/12/2016 à 11:26, Alexandre Belloni a écrit :
> Use devm_kasprintf instead of simple kasprintf to free the allocated memory
> when needed.
> 
> Suggested-by: Peter Rosin 
> Signed-off-by: Alexandre Belloni 

Acked-by: Nicolas Ferre 

> ---
>  drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
> b/drivers/usb/gadget/udc/atmel_usba_udc.c
> index 45bc997d0711..aec72fe8273c 100644
> --- a/drivers/usb/gadget/udc/atmel_usba_udc.c
> +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
> @@ -1978,7 +1978,8 @@ static struct usba_ep * atmel_udc_of_init(struct 
> platform_device *pdev,
>   dev_err(>dev, "of_probe: name error(%d)\n", ret);
>   goto err;
>   }
> - ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index);
> + ep->ep.name = devm_kasprintf(>dev, GFP_KERNEL, "ep%d",
> +  ep->index);
>  
>   ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
>   ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
> 


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

[PATCH] usb: gadget: udc: atmel: used managed kasprintf

[Alexandre Belloni]

Use devm_kasprintf instead of simple kasprintf to free the allocated memory
when needed.

Suggested-by: Peter Rosin 
Signed-off-by: Alexandre Belloni 
---
 drivers/usb/gadget/udc/atmel_usba_udc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c 
b/drivers/usb/gadget/udc/atmel_usba_udc.c
index 45bc997d0711..aec72fe8273c 100644
--- a/drivers/usb/gadget/udc/atmel_usba_udc.c
+++ b/drivers/usb/gadget/udc/atmel_usba_udc.c
@@ -1978,7 +1978,8 @@ static struct usba_ep * atmel_udc_of_init(struct 
platform_device *pdev,
dev_err(>dev, "of_probe: name error(%d)\n", ret);
goto err;
}
-   ep->ep.name = kasprintf(GFP_KERNEL, "ep%d", ep->index);
+   ep->ep.name = devm_kasprintf(>dev, GFP_KERNEL, "ep%d",
+ep->index);
 
ep->ep_regs = udc->regs + USBA_EPT_BASE(i);
ep->dma_regs = udc->regs + USBA_DMA_BASE(i);
-- 
2.10.2

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

Re: [PATCH 1/1] xhci: Put warning message on a single line

[Mathias Nyman]

On 01.12.2016 12:17, Alexander Stein wrote:

This allows someone to grep for the complete warning message as in;
xhci-hcd xhci-hcd.0.auto: USB core suspending device not in U0/U1/U2.

Signed-off-by: Alexander Stein 
---


Thanks, added to queue for 4.11

-Mathias

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

Re: [PATCH] usb: gadget: udc: core: fix return code of usb_gadget_probe_driver()

[Krzysztof Opasiak]

Hi,

Good catch but I have a few comments to commit message:

On 12/01/2016 12:24 AM, Felix Hädicke wrote:
> This fixes a regression which was introduced by commit f1bddbb, by
> reverting a small fragment of commit 855ed04.
> 

Not exactly. The regression has been introduced by commit 855ed04
itself. This commit replaced previous construction similar what you sent
now with simple if():

@@ -535,11 +556,7 @@ int usb_gadget_probe_driver(struct
usb_gadget_driver *driver)
if (!ret)
break;
}
-   if (ret)
-   ret = -ENODEV;
-   else if (udc->driver)
-   ret = -EBUSY;
-   else
+   if (!ret && !udc->driver)
goto found;
} else {
list_for_each_entry(udc, _list, list) {

After that commit, if UDC is busy, gadget is added to the list of
pending gadgets. Unfortunately this list is not rescanned in
usb_gadget_unregister_driver(). This means that such gadget is going to
stay on this list forever so it's a bug.

Commit f1bddbb make this bug more visible (as it causes NULL pointer
dereference) as gadget has not been added to the list of pending gadgets
and we try to remove it from there.

Summing up, in my personal opinion I think that you should add:

Fixes: 855ed04 ("usb: gadget: udc-core: independent registration of
gadgets and gadget drivers")

above your sign off and fix the commit message as this bug has been
introduced before f1bddbb, just symptoms were a little bit different.

After fixing this you may add:

Tested-by: Krzysztof Opasiak 

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
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] fs: configfs: don't return anything from drop_link

[Christoph Hellwig]

Thanks a lot Andrzej!

I've applied the patch, but I undid the reformatting of the nvmet
code to keep the patch as simple as possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/1] xhci: Put warning message on a single line

[Alexander Stein]

This allows someone to grep for the complete warning message as in;
xhci-hcd xhci-hcd.0.auto: USB core suspending device not in U0/U1/U2.

Signed-off-by: Alexander Stein 
---
 drivers/usb/host/xhci-hub.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0ef1690..2d154e2 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -990,8 +990,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 
wValue,
temp = readl(port_array[wIndex]);
if ((temp & PORT_PE) == 0 || (temp & PORT_RESET)
|| (temp & PORT_PLS_MASK) >= XDEV_U3) {
-   xhci_warn(xhci, "USB core suspending device "
- "not in U0/U1/U2.\n");
+   xhci_warn(xhci, "USB core suspending device not 
in U0/U1/U2.\n");
goto error;
}
 
-- 
2.7.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] usb: gadget: uvc: fix UVC_ATTR macro for UVCG_OPTS_ATTR

[Jassi Brar]

From: Jassi Brar 

Typo in commit 76e0da34c7cec5a7d introduced a bug that prevents
creation of streaming_{interval,maxpacket,maxburst} nodes for
invalid 'aname' node.

Signed-off-by: Jassi Brar 
---
 drivers/usb/gadget/function/uvc_configfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index ecb5614..158849e 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -2245,7 +2245,7 @@ end:  
\
return ret; \
 }  \
\
-UVC_ATTR(f_uvc_opts_, cname, aname)
+UVC_ATTR(f_uvc_opts_, cname, cname)
 
 #define identity_conv(x) (x)
 
-- 
2.7.4

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

Re: [PATCH v10 2/8] power: add power sequence library

[Peter Chen]

On Thu, Dec 01, 2016 at 10:57:24PM +0100, Rafael J. Wysocki wrote:
> On Tue, Nov 22, 2016 at 4:53 AM, Peter Chen  wrote:
> > On Tue, Nov 22, 2016 at 03:23:12AM +0100, Rafael J. Wysocki wrote:
> >> > @@ -0,0 +1,237 @@
> >> > +/*
> >> > + * core.c  power sequence core file
> >> > + *
> >> > + * Copyright (C) 2016 Freescale Semiconductor, Inc.
> >> > + * Author: Peter Chen 
> >> > + *
> >> > + * This program is free software: you can redistribute it and/or modify
> >> > + * it under the terms of the GNU General Public License version 2  of
> >> > + * the License as published by the Free Software Foundation.
> >> > + *
> >> > + * This program is distributed in the hope that it will be useful,
> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> > + * GNU General Public License for more details.
> >> > + *
> >> > + * You should have received a copy of the GNU General Public License
> >> > + * along with this program.  If not, see .
> >>
> >> The last paragraph is not necessary AFAICS.
> >
> > I just copy it from:
> >
> > https://www.gnu.org/licenses/gpl-howto.en.html
> >
> > If you are concerns about it, I can delete it.
> 
> It is redundant, so yes, please.

ok.

> 
> >> > +
> >> > +static struct pwrseq *pwrseq_find_available_instance(struct device_node 
> >> > *np)
> >> > +{
> >> > +   struct pwrseq *pwrseq;
> >> > +
> >> > +   list_for_each_entry(pwrseq, _list, node) {
> >> > +   if (pwrseq->used)
> >> > +   continue;
> >> > +
> >> > +   /* compare compatible string for pwrseq node */
> >> > +   if (of_match_node(pwrseq->pwrseq_of_match_table, np)) {
> >> > +   pwrseq->used = true;
> >> > +   return pwrseq;
> >> > +   }
> >> > +
> >> > +   /* return generic pwrseq instance */
> >> > +   if (!strcmp(pwrseq->pwrseq_of_match_table->compatible,
> >> > +   "generic")) {
> >> > +   pr_debug("using generic pwrseq instance for 
> >> > %s\n",
> >> > +   np->full_name);
> >> > +   pwrseq->used = true;
> >> > +   return pwrseq;
> >> > +   }
> >> > +   }
> >> > +   pr_warn("Can't find any pwrseq instances for %s\n", 
> >> > np->full_name);
> >>
> >> pr_debug() ?
> >
> > If there is no pwrseq instance for that node, the power sequence on routine 
> > will
> > return fail, so I think an warning message is useful for user.
> 
> Useful in what way?  How is the user supposed to know what happened
> from this message?

Ok, I will change it to debug message.

> >> > + */
> >> > +struct pwrseq *of_pwrseq_on(struct device_node *np)
> >> > +{
> >> > +   struct pwrseq *pwrseq;
> >> > +   int ret;
> >> > +
> >> > +   pwrseq = pwrseq_find_available_instance(np);
> >>
> >> What does guarantee the integrity of ths list at this point?
> >
> > Once the use selects the specific pwrseq library, the library will
> > create an empty one instance during the initialization, and it
> > will be called at postcore_initcall, the device driver has not
> > probed yet.
> 
> Which doesn't matter really, because the list is global and some other
> driver using it might have been probed already.
> 
> You have a mutex here and it is used for add/remove.  Why isn't it
> used for list browsing?

I will add mutex for it, thanks.

> >
> >> > + */
> >> > +int of_pwrseq_on_list(struct device_node *np, struct list_head *head)
> >> > +{
> >> > +   struct pwrseq *pwrseq;
> >> > +   struct pwrseq_list_per_dev *pwrseq_list_node;
> >> > +
> >> > +   pwrseq = of_pwrseq_on(np);
> >> > +   if (IS_ERR(pwrseq))
> >> > +   return PTR_ERR(pwrseq);
> >> > +
> >> > +   pwrseq_list_node = kzalloc(sizeof(*pwrseq_list_node), 
> >> > GFP_KERNEL);
> >>
> >> Why don't you allocate memory before turning the power sequence on?
> >>
> >
> > This list is only for power sequence on instance, if I allocate memory 
> > before
> > power sequence on, I need to free it if power sequence on is failed.
> 
> So why is that a problem?
> 

Not any problems, I will follow your comments.

> >> > +   if (!pwrseq_list_node) {
> >> > +   of_pwrseq_off(pwrseq);
> >> > +   return -ENOMEM;
> >> > +   }
> >> > +   pwrseq_list_node->pwrseq = pwrseq;
> >> > +   list_add(_list_node->list, head);
> >> > +
> >> > +   return 0;
> >> > +}
> >> > +EXPORT_SYMBOL_GPL(of_pwrseq_on_list);
> >>
> >> So the caller is supposed to provide a list head of the list to put
> >> the power sequence object into on success, right?
> >
> > Yes
> >
> >>
> >> Can you explain to me what the idea here is, please?
> >>
> >
> > Taking USB devices as an example, there is one power sequence on list
> > 

Re: [PATCH] usb: gadget: uvc: fix UVC_ATTR macro for UVCG_OPTS_ATTR

[Greg KH]

On Fri, Dec 02, 2016 at 11:52:02AM +0530, Jassi Brar wrote:
> From: Jassi Brar 
> 
> Typo in commit 76e0da34c7cec5a7d introduced a bug that prevents
> creation of streaming_{interval,maxpacket,maxburst} nodes for
> invalid 'aname' node.
> 
> Signed-off-by: Jassi Brar 

Fixes: tag?
cc: stable tag?

Come on, you know better than this...

--
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 05/12] usb: chipdata: Replace the extcon API

[Peter Chen]

On Wed, Nov 30, 2016 at 02:57:33PM +0900, Chanwoo Choi wrote:
> This patch uses the resource-managed extcon API for extcon_register_notifier()
> and replaces the deprecated extcon API as following:
> - extcon_get_cable_state_() -> extcon_get_state()
> 
> Signed-off-by: Chanwoo Choi 
> ---
>  drivers/usb/chipidea/core.c | 30 ++
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 69426e644d17..a5b44963eaea 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -742,7 +742,7 @@ static int ci_get_platdata(struct device *dev,
>   cable->edev = ext_vbus;
>  
>   if (!IS_ERR(ext_vbus)) {
> - ret = extcon_get_cable_state_(cable->edev, EXTCON_USB);
> + ret = extcon_get_state(cable->edev, EXTCON_USB);
>   if (ret)
>   cable->state = true;
>   else
> @@ -754,7 +754,7 @@ static int ci_get_platdata(struct device *dev,
>   cable->edev = ext_id;
>  
>   if (!IS_ERR(ext_id)) {
> - ret = extcon_get_cable_state_(cable->edev, EXTCON_USB_HOST);
> + ret = extcon_get_state(cable->edev, EXTCON_USB_HOST);
>   if (ret)
>   cable->state = false;
>   else
> @@ -771,8 +771,8 @@ static int ci_extcon_register(struct ci_hdrc *ci)
>   id = >platdata->id_extcon;
>   id->ci = ci;
>   if (!IS_ERR(id->edev)) {
> - ret = extcon_register_notifier(id->edev, EXTCON_USB_HOST,
> ->nb);
> + ret = devm_extcon_register_notifier(ci->dev, id->edev,
> + EXTCON_USB_HOST, >nb);
>   if (ret < 0) {
>   dev_err(ci->dev, "register ID failed\n");
>   return ret;
> @@ -782,11 +782,9 @@ static int ci_extcon_register(struct ci_hdrc *ci)
>   vbus = >platdata->vbus_extcon;
>   vbus->ci = ci;
>   if (!IS_ERR(vbus->edev)) {
> - ret = extcon_register_notifier(vbus->edev, EXTCON_USB,
> ->nb);
> + ret = devm_extcon_register_notifier(ci->dev, vbus->edev,
> + EXTCON_USB, >nb);
>   if (ret < 0) {
> - extcon_unregister_notifier(id->edev, EXTCON_USB_HOST,
> ->nb);
>   dev_err(ci->dev, "register VBUS failed\n");
>   return ret;
>   }
> @@ -795,20 +793,6 @@ static int ci_extcon_register(struct ci_hdrc *ci)
>   return 0;
>  }
>  
> -static void ci_extcon_unregister(struct ci_hdrc *ci)
> -{
> - struct ci_hdrc_cable *cable;
> -
> - cable = >platdata->id_extcon;
> - if (!IS_ERR(cable->edev))
> - extcon_unregister_notifier(cable->edev, EXTCON_USB_HOST,
> ->nb);
> -
> - cable = >platdata->vbus_extcon;
> - if (!IS_ERR(cable->edev))
> - extcon_unregister_notifier(cable->edev, EXTCON_USB, >nb);
> -}
> -
>  static DEFINE_IDA(ci_ida);
>  
>  struct platform_device *ci_hdrc_add_device(struct device *dev,
> @@ -1053,7 +1037,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>   if (!ret)
>   return 0;
>  
> - ci_extcon_unregister(ci);
>  stop:
>   ci_role_destroy(ci);
>  deinit_phy:
> @@ -1073,7 +1056,6 @@ static int ci_hdrc_remove(struct platform_device *pdev)
>   }
>  
>   dbg_remove_files(ci);
> - ci_extcon_unregister(ci);
>   ci_role_destroy(ci);
>   ci_hdrc_enter_lpm(ci, true);
>   ci_usb_phy_exit(ci);
> -- 

Acked-by: Peter Chen 

-- 

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

Re: [PATCH 1/1] usb: xhci: fix possible wild pointer

[Lu Baolu]

Hi,

On 12/02/2016 12:18 PM, Baolin Wang wrote:
> On 2 December 2016 at 10:29, Lu Baolu  wrote:
>> handle_cmd_completion() frees a command structure which might
>> be still referenced by xhci->current_cmd. This might cause
>> problem when xhci->current_cmd is accessed after that.
>>
>> A real-life case could be like this. The host takes a very long
>> time to respond to a command, and the command timer is fired at
>> the same time when the command completion event arrives. The
>> command completion handler frees xhci->current_cmd before the
>> timer function can grab xhci->lock. Afterward, timer function
>> grabs the lock and go ahead with checking and setting members
>> of xhci->current_cmd.
>>
>> Cc:  # v3.16+
>> Signed-off-by: Lu Baolu 
> Nice catch. I was also curious where set xhci->current_cmd to be NULL
> when current command is freed.

Below code does:

xhci->current_cmd = list_entry(cmd->cmd_list.next,
   struct xhci_command, cmd_list);
mod_timer(>cmd_timer, jiffies + XHCI_CMD_DEFAULT_TIMEOUT);
+   } else if (xhci->current_cmd == cmd) {
+   xhci->current_cmd = NULL;
+   } else {
+   xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
}


Best regards,
Lu Baolu

> Tested-by: Baolin Wang 
>
>> ---
>>  drivers/usb/host/xhci-ring.c | 16 +++-
>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index bdf6b13..13e05f6 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1267,14 +1267,16 @@ void xhci_handle_command_timeout(unsigned long data)
>> bool second_timeout = false;
>> xhci = (struct xhci_hcd *) data;
>>
>> -   /* mark this command to be cancelled */
>> spin_lock_irqsave(>lock, flags);
>> -   if (xhci->current_cmd) {
>> -   if (xhci->current_cmd->status == COMP_CMD_ABORT)
>> -   second_timeout = true;
>> -   xhci->current_cmd->status = COMP_CMD_ABORT;
>> +   if (!xhci->current_cmd) {
>> +   spin_unlock_irqrestore(>lock, flags);
>> +   return;
>> }
>>
>> +   if (xhci->current_cmd->status == COMP_CMD_ABORT)
>> +   second_timeout = true;
>> +   xhci->current_cmd->status = COMP_CMD_ABORT;
>> +
>> /* Make sure command ring is running before aborting it */
>> hw_ring_state = xhci_read_64(xhci, >op_regs->cmd_ring);
>> if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
>> @@ -1422,6 +1424,10 @@ static void handle_cmd_completion(struct xhci_hcd 
>> *xhci,
>> xhci->current_cmd = list_entry(cmd->cmd_list.next,
>>struct xhci_command, 
>> cmd_list);
>> mod_timer(>cmd_timer, jiffies + 
>> XHCI_CMD_DEFAULT_TIMEOUT);
>> +   } else if (xhci->current_cmd == cmd) {
>> +   xhci->current_cmd = NULL;
>> +   } else {
>> +   xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
>> }
>>
>>  event_handled:
>> --
>> 2.1.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

--
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/1] usb: xhci: fix possible wild pointer

[Baolin Wang]

On 2 December 2016 at 12:40, Lu Baolu  wrote:
> Hi,
>
> On 12/02/2016 12:18 PM, Baolin Wang wrote:
>> On 2 December 2016 at 10:29, Lu Baolu  wrote:
>>> handle_cmd_completion() frees a command structure which might
>>> be still referenced by xhci->current_cmd. This might cause
>>> problem when xhci->current_cmd is accessed after that.
>>>
>>> A real-life case could be like this. The host takes a very long
>>> time to respond to a command, and the command timer is fired at
>>> the same time when the command completion event arrives. The
>>> command completion handler frees xhci->current_cmd before the
>>> timer function can grab xhci->lock. Afterward, timer function
>>> grabs the lock and go ahead with checking and setting members
>>> of xhci->current_cmd.
>>>
>>> Cc:  # v3.16+
>>> Signed-off-by: Lu Baolu 
>> Nice catch. I was also curious where set xhci->current_cmd to be NULL
>> when current command is freed.
>
> Below code does:

Yes, I just means you did that, but the original code did not. Sorry
for confusing.

>
> xhci->current_cmd = list_entry(cmd->cmd_list.next,
>struct xhci_command, cmd_list);
> mod_timer(>cmd_timer, jiffies + 
> XHCI_CMD_DEFAULT_TIMEOUT);
> +   } else if (xhci->current_cmd == cmd) {
> +   xhci->current_cmd = NULL;
> +   } else {
> +   xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
> }
>
>
> Best regards,
> Lu Baolu
>
>> Tested-by: Baolin Wang 
>>
>>> ---
>>>  drivers/usb/host/xhci-ring.c | 16 +++-
>>>  1 file changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>>> index bdf6b13..13e05f6 100644
>>> --- a/drivers/usb/host/xhci-ring.c
>>> +++ b/drivers/usb/host/xhci-ring.c
>>> @@ -1267,14 +1267,16 @@ void xhci_handle_command_timeout(unsigned long data)
>>> bool second_timeout = false;
>>> xhci = (struct xhci_hcd *) data;
>>>
>>> -   /* mark this command to be cancelled */
>>> spin_lock_irqsave(>lock, flags);
>>> -   if (xhci->current_cmd) {
>>> -   if (xhci->current_cmd->status == COMP_CMD_ABORT)
>>> -   second_timeout = true;
>>> -   xhci->current_cmd->status = COMP_CMD_ABORT;
>>> +   if (!xhci->current_cmd) {
>>> +   spin_unlock_irqrestore(>lock, flags);
>>> +   return;
>>> }
>>>
>>> +   if (xhci->current_cmd->status == COMP_CMD_ABORT)
>>> +   second_timeout = true;
>>> +   xhci->current_cmd->status = COMP_CMD_ABORT;
>>> +
>>> /* Make sure command ring is running before aborting it */
>>> hw_ring_state = xhci_read_64(xhci, >op_regs->cmd_ring);
>>> if ((xhci->cmd_ring_state & CMD_RING_STATE_RUNNING) &&
>>> @@ -1422,6 +1424,10 @@ static void handle_cmd_completion(struct xhci_hcd 
>>> *xhci,
>>> xhci->current_cmd = list_entry(cmd->cmd_list.next,
>>>struct xhci_command, 
>>> cmd_list);
>>> mod_timer(>cmd_timer, jiffies + 
>>> XHCI_CMD_DEFAULT_TIMEOUT);
>>> +   } else if (xhci->current_cmd == cmd) {
>>> +   xhci->current_cmd = NULL;
>>> +   } else {
>>> +   xhci_warn(xhci, "WARN current_cmd doesn't match command\n");
>>> }
>>>
>>>  event_handled:
>>> --
>>> 2.1.4
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>>> the body of a message to majord...@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>



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

RE: [PATCH v13 01/10] usbip: exporting devices: modifications to network header

[fx IWATA NOBUO]

Hello,

> +   if (!error)
> +   reply.returncode = 0;
> +   else
> +   reply.returncode = -1;
> 
> It looks a little bit ugly to me that we change the value to be unsigned
> and then assign a signed number to it. In addition your compiler is going
> to complain if you use -Wconversion flag.

My mistake.

> >  struct op_unexport_reply {
> > -   int returncode;
> > +   uint32_t returncode;
> >  } __attribute__((packed));

*** I will remove 'returncode'.

It's enough by using op_common.status(ST_OK|ST_NA).

There 2 error case for each operation.
Import : not-found, export-error
Export: busy(no free), attach-error
Unexport : not-found, export-error

Existing, import, doesn't carry the reason.
It only uses op_common.status.
Currently, usbip's exit-status doesn't have the reason.
There's a president with no field: op_devlist_request.

Thank you for reviewing,

n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 4:11 AM
> To: fx IWATA NOBUO; valentina.mane...@gmail.com; shuah...@samsung.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO
> Subject: Re: [PATCH v13 01/10] usbip: exporting devices: modifications to
> network header
> 
> Hi,
> 
> On 11/22/2016 07:48 AM, Nobuo Iwata wrote:
> > Modification to export and un-export response in
> > tools/usb/usbip/src/usbip_network.h. It just changes return code type
> > from int to uint32_t as same as other responses.
> >
> > Signed-off-by: Nobuo Iwata 
> > ---
> >  tools/usb/usbip/src/usbip_network.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/usb/usbip/src/usbip_network.h
> > b/tools/usb/usbip/src/usbip_network.h
> > index c1e875c..e1ca86a 100644
> > --- a/tools/usb/usbip/src/usbip_network.h
> > +++ b/tools/usb/usbip/src/usbip_network.h
> > @@ -93,7 +93,7 @@ struct op_export_request {  }
> > __attribute__((packed));
> >
> >  struct op_export_reply {
> > -   int returncode;
> > +   uint32_t returncode;
> >  } __attribute__((packed));
> >
> >
> > @@ -115,7 +115,7 @@ struct op_unexport_request {  }
> > __attribute__((packed));
> >
> >  struct op_unexport_reply {
> > -   int returncode;
> > +   uint32_t returncode;
> >  } __attribute__((packed));
> >
> >  #define PACK_OP_UNEXPORT_REQUEST(pack, request)  do {\
> >
> 
> The field name is returncode but we have no suitable defines with "return
> codes". I ran through USBIP documentation but didn't find any list of allowed
> return codes. Is there any?
> 
> You change the value type to unsigned but later you use:
> 
> +   if (!error)
> +   reply.returncode = 0;
> +   else
> +   reply.returncode = -1;
> 
> It looks a little bit ugly to me that we change the value to be unsigned
> and then assign a signed number to it. In addition your compiler is going
> to complain if you use -Wconversion flag.
> 
> In my opinion we should have suitable defines for error codes and those
> code should be documented in protocol documentation.
> 
> Best regards
> --
> Krzysztof Opasiak
> Samsung R Institute Poland
> Samsung Electronics
--
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 v13 02/10] usbip: exporting devices: modifications to host side libraries

[fx IWATA NOBUO]

Hello,

> In my humble opinion the code itself is good. The problem is commit
> message. Current commit massage has nothing to do with patch content
> and should be totally changed.
> 
> Please elaborate that this function is unused, write that using
> indexes on a list is generally not a good idea and so on.

OK.
I will improve the commit message.

Thank you,

n.iwata
//
> -Original Message-
> From: Krzysztof Opasiak [mailto:k.opas...@samsung.com]
> Sent: Friday, December 02, 2016 4:57 AM
> To: fx IWATA NOBUO; shuah...@samsung.com; valentina.mane...@gmail.com;
> gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Cc: fx MICHIMURA TADAO; Shuah Khan
> Subject: Re: [PATCH v13 02/10] usbip: exporting devices: modifications to
> host side libraries
> 
> 
> 
> On 11/24/2016 06:59 AM, fx IWATA NOBUO wrote:
> > Hello,
> >
> >> This doesn't look like a simple change to rename and reuse an unused
> >> function. This patch does lot more and is changing the user interface.
> >> Looks like instead of taking an integer value for device lookup, you
> >> are changing it to char *.
> >>
> >> Any reason why you have to change the user interface to go to char
> >> *busid?
> >>
> >> I would like to see a good explanation why this user interface change
> >> is necessary.
> >
> > I can't figure out why the second argument was int because it was
> > unused.
> >
> 
> I also cannot figure out this but using busid here looks much more reasonable
> to me.
> 
> > usbip_get_device() is a stub-side (usb-host) function.
> > As you know, only port number in vhci-side can be int.
> > Others are bus-id.
> > Furthermore, the unused code searches list node position. It doesn't
> > make sense.
> >
> 
> Agree.
> 
> > Here, this patch set needs to find bound device with bus-id in
> > stub-side.
> 
> stub-side or vudc side. In case of vudc the busid is set to usbip-vudc.%d
> 
> >
> > I may add explanation above to change log.
> > Or I can change new function name like usbip_find_device() and leave
> > the unused function once.
> >
> > Please, let me know how should I do.
> 
> In my humble opinion the code itself is good. The problem is commit message.
> Current commit massage has nothing to do with patch content and should be
> totally changed.
> 
> Please elaborate that this function is unused, write that using indexes
> on a list is generally not a good idea and so on.
> 
> Best regards,
> --
> Krzysztof Opasiak
> Samsung R Institute Poland
> Samsung Electronics
--
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