[PATCH 17/50] USB: serial: sierra: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Johan Hovold jhov...@gmail.com Signed-off-by: Ming Lei ming@canonical.com --- drivers/usb/serial/sierra.c |9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c index de958c5..e79b6ad 100644 --- a/drivers/usb/serial/sierra.c +++ b/drivers/usb/serial/sierra.c @@ -433,6 +433,7 @@ static void sierra_outdat_callback(struct urb *urb) struct sierra_port_private *portdata = usb_get_serial_port_data(port); struct sierra_intf_private *intfdata; int status = urb-status; + unsigned long flags; intfdata = port-serial-private; @@ -443,12 +444,12 @@ static void sierra_outdat_callback(struct urb *urb) dev_dbg(port-dev, %s - nonzero write bulk status received: %d\n, __func__, status); - spin_lock(portdata-lock); + spin_lock_irqsave(portdata-lock, flags); --portdata-outstanding_urbs; - spin_unlock(portdata-lock); - spin_lock(intfdata-susp_lock); + spin_unlock_irqrestore(portdata-lock, flags); + spin_lock_irqsave(intfdata-susp_lock, flags); --intfdata-in_flight; - spin_unlock(intfdata-susp_lock); + spin_unlock_irqrestore(intfdata-susp_lock, flags); usb_serial_port_softint(port); } -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 14/50] USB: serial: mos7720: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/mos7720.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/mos7720.c b/drivers/usb/serial/mos7720.c
index 51da424..9b8c866 100644
--- a/drivers/usb/serial/mos7720.c
+++ b/drivers/usb/serial/mos7720.c
@@ -338,14 +338,15 @@ static void async_complete(struct urb *urb)
{
struct urbtracker *urbtrack = urb-context;
int status = urb-status;
+ unsigned long flags;
if (unlikely(status))
dev_dbg(urb-dev-dev, %s - nonzero urb status received:
%d\n, __func__, status);
/* remove the urbtracker from the active_urbs list */
- spin_lock(urbtrack-mos_parport-listlock);
+ spin_lock_irqsave(urbtrack-mos_parport-listlock, flags);
list_del(urbtrack-urblist_entry);
- spin_unlock(urbtrack-mos_parport-listlock);
+ spin_unlock_irqrestore(urbtrack-mos_parport-listlock, flags);
kref_put(urbtrack-ref_count, destroy_urbtracker);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 24/50] input: cm109: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Dmitry Torokhov dmitry.torok...@gmail.com Cc: linux-in...@vger.kernel.org Signed-off-by: Ming Lei ming@canonical.com --- drivers/input/misc/cm109.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c index 082684e..cac4e37 100644 --- a/drivers/input/misc/cm109.c +++ b/drivers/input/misc/cm109.c @@ -340,6 +340,7 @@ static void cm109_urb_irq_callback(struct urb *urb) struct cm109_dev *dev = urb-context; const int status = urb-status; int error; + unsigned long flags; dev_dbg(dev-intf-dev, ### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x\n, dev-irq_data-byte[0], @@ -379,7 +380,7 @@ static void cm109_urb_irq_callback(struct urb *urb) out: - spin_lock(dev-ctl_submit_lock); + spin_lock_irqsave(dev-ctl_submit_lock, flags); dev-irq_urb_pending = 0; @@ -403,7 +404,7 @@ static void cm109_urb_irq_callback(struct urb *urb) __func__, error); } - spin_unlock(dev-ctl_submit_lock); + spin_unlock_irqrestore(dev-ctl_submit_lock, flags); } static void cm109_urb_ctl_callback(struct urb *urb) @@ -411,6 +412,7 @@ static void cm109_urb_ctl_callback(struct urb *urb) struct cm109_dev *dev = urb-context; const int status = urb-status; int error; + unsigned long flags; dev_dbg(dev-intf-dev, ### URB CTL: [0x%02x 0x%02x 0x%02x 0x%02x]\n, dev-ctl_data-byte[0], @@ -421,7 +423,7 @@ static void cm109_urb_ctl_callback(struct urb *urb) if (status) dev_err(dev-intf-dev, %s: urb status %d\n, __func__, status); - spin_lock(dev-ctl_submit_lock); + spin_lock_irqsave(dev-ctl_submit_lock, flags); dev-ctl_urb_pending = 0; @@ -442,7 +444,7 @@ static void cm109_urb_ctl_callback(struct urb *urb) } } - spin_unlock(dev-ctl_submit_lock); + spin_unlock_irqrestore(dev-ctl_submit_lock, flags); } static void cm109_toggle_buzzer_async(struct cm109_dev *dev) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 30/50] wireless: ath9k: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Luis R. Rodriguez mcg...@qca.qualcomm.com
Cc: John W. Linville linvi...@tuxdriver.com
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/wireless/ath/ath9k/hif_usb.c | 29 ++---
drivers/net/wireless/ath/ath9k/htc_drv_txrx.c |9
drivers/net/wireless/ath/ath9k/wmi.c | 11 +-
3 files changed, 27 insertions(+), 22 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c
b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 9e582e1..9d5e15a 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -136,6 +136,7 @@ static void hif_usb_mgmt_cb(struct urb *urb)
struct cmd_buf *cmd = (struct cmd_buf *)urb-context;
struct hif_device_usb *hif_dev;
bool txok = true;
+ unsigned long flags;
if (!cmd || !cmd-skb || !cmd-hif_dev)
return;
@@ -155,14 +156,14 @@ static void hif_usb_mgmt_cb(struct urb *urb)
* If the URBs are being flushed, no need to complete
* this packet.
*/
- spin_lock(hif_dev-tx.tx_lock);
+ spin_lock_irqsave(hif_dev-tx.tx_lock, flags);
if (hif_dev-tx.flags HIF_USB_TX_FLUSH) {
- spin_unlock(hif_dev-tx.tx_lock);
+ spin_unlock_irqrestore(hif_dev-tx.tx_lock, flags);
dev_kfree_skb_any(cmd-skb);
kfree(cmd);
return;
}
- spin_unlock(hif_dev-tx.tx_lock);
+ spin_unlock_irqrestore(hif_dev-tx.tx_lock, flags);
break;
default:
@@ -253,6 +254,7 @@ static void hif_usb_tx_cb(struct urb *urb)
struct tx_buf *tx_buf = (struct tx_buf *) urb-context;
struct hif_device_usb *hif_dev;
bool txok = true;
+ unsigned long flags;
if (!tx_buf || !tx_buf-hif_dev)
return;
@@ -272,13 +274,13 @@ static void hif_usb_tx_cb(struct urb *urb)
* If the URBs are being flushed, no need to add this
* URB to the free list.
*/
- spin_lock(hif_dev-tx.tx_lock);
+ spin_lock_irqsave(hif_dev-tx.tx_lock, flags);
if (hif_dev-tx.flags HIF_USB_TX_FLUSH) {
- spin_unlock(hif_dev-tx.tx_lock);
+ spin_unlock_irqrestore(hif_dev-tx.tx_lock, flags);
ath9k_skb_queue_purge(hif_dev, tx_buf-skb_queue);
return;
}
- spin_unlock(hif_dev-tx.tx_lock);
+ spin_unlock_irqrestore(hif_dev-tx.tx_lock, flags);
break;
default:
@@ -293,13 +295,13 @@ static void hif_usb_tx_cb(struct urb *urb)
__skb_queue_head_init(tx_buf-skb_queue);
/* Add this TX buffer to the free list */
- spin_lock(hif_dev-tx.tx_lock);
+ spin_lock_irqsave(hif_dev-tx.tx_lock, flags);
list_move_tail(tx_buf-list, hif_dev-tx.tx_buf);
hif_dev-tx.tx_buf_cnt++;
if (!(hif_dev-tx.flags HIF_USB_TX_STOP))
__hif_usb_tx(hif_dev); /* Check for pending SKBs */
TX_STAT_INC(buf_completed);
- spin_unlock(hif_dev-tx.tx_lock);
+ spin_unlock_irqrestore(hif_dev-tx.tx_lock, flags);
}
/* TX lock has to be taken */
@@ -530,8 +532,9 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb
*hif_dev,
int rx_remain_len, rx_pkt_len;
u16 pool_index = 0;
u8 *ptr;
+ unsigned long flags;
- spin_lock(hif_dev-rx_lock);
+ spin_lock_irqsave(hif_dev-rx_lock, flags);
rx_remain_len = hif_dev-rx_remain_len;
rx_pkt_len = hif_dev-rx_transfer_len;
@@ -559,7 +562,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb
*hif_dev,
}
}
- spin_unlock(hif_dev-rx_lock);
+ spin_unlock_irqrestore(hif_dev-rx_lock, flags);
while (index len) {
u16 pkt_len;
@@ -585,7 +588,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb
*hif_dev,
index = index + 4 + pkt_len + pad_len;
if (index MAX_RX_BUF_SIZE) {
- spin_lock(hif_dev-rx_lock);
+ spin_lock_irqsave(hif_dev-rx_lock, flags);
hif_dev-rx_remain_len = index - MAX_RX_BUF_SIZE;
hif_dev-rx_transfer_len =
MAX_RX_BUF_SIZE - chk_idx - 4;
@@ -595,7 +598,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb
*hif_dev,
if (!nskb) {
dev_err(hif_dev-udev-dev,
ath9k_htc: RX memory allocation
error\n
[PATCH 21/50] hid: usbhid: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Jiri Kosina jkos...@suse.cz
Cc: linux-in...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/hid/usbhid/hid-core.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 9941828..e1d8518 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -489,8 +489,9 @@ static void hid_ctrl(struct urb *urb)
struct hid_device *hid = urb-context;
struct usbhid_device *usbhid = hid-driver_data;
int unplug = 0, status = urb-status;
+ unsigned long flags;
- spin_lock(usbhid-lock);
+ spin_lock_irqsave(usbhid-lock, flags);
switch (status) {
case 0: /* success */
@@ -525,7 +526,7 @@ static void hid_ctrl(struct urb *urb)
}
clear_bit(HID_CTRL_RUNNING, usbhid-iofl);
- spin_unlock(usbhid-lock);
+ spin_unlock_irqrestore(usbhid-lock, flags);
usb_autopm_put_interface_async(usbhid-intf);
wake_up(usbhid-wait);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 20/50] USB: serial: usb_wwan: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/usb_wwan.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/usb_wwan.c b/drivers/usb/serial/usb_wwan.c
index 8257d30..c807d65 100644
--- a/drivers/usb/serial/usb_wwan.c
+++ b/drivers/usb/serial/usb_wwan.c
@@ -312,6 +312,7 @@ static void usb_wwan_outdat_callback(struct urb *urb)
struct usb_wwan_port_private *portdata;
struct usb_wwan_intf_private *intfdata;
int i;
+ unsigned long flags;
port = urb-context;
intfdata = port-serial-private;
@@ -319,9 +320,9 @@ static void usb_wwan_outdat_callback(struct urb *urb)
usb_serial_port_softint(port);
usb_autopm_put_interface_async(port-serial-interface);
portdata = usb_get_serial_port_data(port);
- spin_lock(intfdata-susp_lock);
+ spin_lock_irqsave(intfdata-susp_lock, flags);
intfdata-in_flight--;
- spin_unlock(intfdata-susp_lock);
+ spin_unlock_irqrestore(intfdata-susp_lock, flags);
for (i = 0; i N_OUT_URB; ++i) {
if (portdata-out_urbs[i] == urb) {
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 16/50] USB: serial: quatech2: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/quatech2.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/quatech2.c b/drivers/usb/serial/quatech2.c
index d997432..95e5dbf 100644
--- a/drivers/usb/serial/quatech2.c
+++ b/drivers/usb/serial/quatech2.c
@@ -630,16 +630,17 @@ static void qt2_write_bulk_callback(struct urb *urb)
{
struct usb_serial_port *port;
struct qt2_port_private *port_priv;
+ unsigned long flags;
port = urb-context;
port_priv = usb_get_serial_port_data(port);
- spin_lock(port_priv-urb_lock);
+ spin_lock_irqsave(port_priv-urb_lock, flags);
port_priv-urb_in_use = false;
usb_serial_port_softint(port);
- spin_unlock(port_priv-urb_lock);
+ spin_unlock_irqrestore(port_priv-urb_lock, flags);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/50] USB: serial: digi_acceleportldusb: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Peter Berger pber...@brimson.com
Cc: Al Borchers alborch...@steinerpoint.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/digi_acceleport.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/serial/digi_acceleport.c
b/drivers/usb/serial/digi_acceleport.c
index 19b467f..95b1959 100644
--- a/drivers/usb/serial/digi_acceleport.c
+++ b/drivers/usb/serial/digi_acceleport.c
@@ -988,6 +988,7 @@ static void digi_write_bulk_callback(struct urb *urb)
struct digi_serial *serial_priv;
int ret = 0;
int status = urb-status;
+ unsigned long flags;
/* port and serial sanity check */
if (port == NULL || (priv = usb_get_serial_port_data(port)) == NULL) {
@@ -1006,15 +1007,15 @@ static void digi_write_bulk_callback(struct urb *urb)
/* handle oob callback */
if (priv-dp_port_num == serial_priv-ds_oob_port_num) {
dev_dbg(port-dev, digi_write_bulk_callback: oob callback\n);
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
priv-dp_write_urb_in_use = 0;
wake_up_interruptible(port-write_wait);
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
return;
}
/* try to send any buffered data on this port */
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
priv-dp_write_urb_in_use = 0;
if (priv-dp_out_buf_len 0) {
*((unsigned char *)(port-write_urb-transfer_buffer))
@@ -1037,7 +1038,7 @@ static void digi_write_bulk_callback(struct urb *urb)
/* lost the race in write_chan(). */
schedule_work(priv-dp_wakeup_work);
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
if (ret ret != -EPERM)
dev_err_console(port,
%s: usb_submit_urb failed, ret=%d, port=%d\n,
@@ -1388,6 +1389,7 @@ static int digi_read_inb_callback(struct urb *urb)
unsigned char *data = ((unsigned char *)urb-transfer_buffer) + 3;
int flag, throttled;
int status = urb-status;
+ unsigned long flags;
/* do not process callbacks on closed ports */
/* but do continue the read chain */
@@ -1404,7 +1406,7 @@ static int digi_read_inb_callback(struct urb *urb)
return -1;
}
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
/* check for throttle; if set, do not resubmit read urb */
/* indicate the read chain needs to be restarted on unthrottle */
@@ -1438,7 +1440,7 @@ static int digi_read_inb_callback(struct urb *urb)
tty_flip_buffer_push(port-port);
}
}
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
if (opcode == DIGI_CMD_RECEIVE_DISABLE)
dev_dbg(port-dev, %s: got RECEIVE_DISABLE\n, __func__);
@@ -1469,6 +1471,7 @@ static int digi_read_oob_callback(struct urb *urb)
int opcode, line, status, val;
int i;
unsigned int rts;
+ unsigned long flags;
/* handle each oob command */
for (i = 0; i urb-actual_length - 3;) {
@@ -1496,7 +1499,7 @@ static int digi_read_oob_callback(struct urb *urb)
rts = tty-termios.c_cflag CRTSCTS;
if (tty opcode == DIGI_CMD_READ_INPUT_SIGNALS) {
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
/* convert from digi flags to termiox flags */
if (val DIGI_READ_INPUT_SIGNALS_CTS) {
priv-dp_modem_signals |= TIOCM_CTS;
@@ -1524,12 +1527,12 @@ static int digi_read_oob_callback(struct urb *urb)
else
priv-dp_modem_signals = ~TIOCM_CD;
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
} else if (opcode == DIGI_CMD_TRANSMIT_IDLE) {
- spin_lock(priv-dp_port_lock);
+ spin_lock_irqsave(priv-dp_port_lock, flags);
priv-dp_transmit_idle = 1;
wake_up_interruptible(priv-dp_transmit_idle_wait);
- spin_unlock(priv-dp_port_lock);
+ spin_unlock_irqrestore(priv-dp_port_lock, flags);
} else if (opcode == DIGI_CMD_IFLUSH_FIFO) {
wake_up_interruptible(priv-dp_flush_wait);
}
--
1.7.9.5
--
To unsubscribe from this list: send
[PATCH 19/50] USB: serial: ti_usb_3410_5052: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/ti_usb_3410_5052.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
b/drivers/usb/serial/ti_usb_3410_5052.c
index 7182bb7..4b984e9 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -1081,6 +1081,7 @@ static void ti_bulk_in_callback(struct urb *urb)
struct device *dev = urb-dev-dev;
int status = urb-status;
int retval = 0;
+ unsigned long flags;
switch (status) {
case 0:
@@ -1116,20 +1117,20 @@ static void ti_bulk_in_callback(struct urb *urb)
__func__);
else
ti_recv(port, urb-transfer_buffer, urb-actual_length);
- spin_lock(tport-tp_lock);
+ spin_lock_irqsave(tport-tp_lock, flags);
port-icount.rx += urb-actual_length;
- spin_unlock(tport-tp_lock);
+ spin_unlock_irqrestore(tport-tp_lock, flags);
}
exit:
/* continue to read unless stopping */
- spin_lock(tport-tp_lock);
+ spin_lock_irqsave(tport-tp_lock, flags);
if (tport-tp_read_urb_state == TI_READ_URB_RUNNING)
retval = usb_submit_urb(urb, GFP_ATOMIC);
else if (tport-tp_read_urb_state == TI_READ_URB_STOPPING)
tport-tp_read_urb_state = TI_READ_URB_STOPPED;
- spin_unlock(tport-tp_lock);
+ spin_unlock_irqrestore(tport-tp_lock, flags);
if (retval)
dev_err(dev, %s - resubmit read urb failed, %d\n,
__func__, retval);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 18/50] USB: serial: symbolserial: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/symbolserial.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/symbolserial.c
b/drivers/usb/serial/symbolserial.c
index 9b16489..b4f5cbe 100644
--- a/drivers/usb/serial/symbolserial.c
+++ b/drivers/usb/serial/symbolserial.c
@@ -41,6 +41,7 @@ static void symbol_int_callback(struct urb *urb)
int status = urb-status;
int result;
int data_length;
+ unsigned long flags;
switch (status) {
case 0:
@@ -81,7 +82,7 @@ static void symbol_int_callback(struct urb *urb)
}
exit:
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
/* Continue trying to always read if we should */
if (!priv-throttled) {
@@ -92,7 +93,7 @@ exit:
__func__, result);
} else
priv-actually_throttled = true;
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
}
static int symbol_open(struct tty_struct *tty, struct usb_serial_port *port)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 42/50] media: usb: tlg2300: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so disable local interrupt before holding a global lock which is held without irqsave. Cc: Mauro Carvalho Chehab mche...@redhat.com Cc: linux-me...@vger.kernel.org Signed-off-by: Ming Lei ming@canonical.com --- drivers/media/usb/tlg2300/pd-alsa.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/media/usb/tlg2300/pd-alsa.c b/drivers/media/usb/tlg2300/pd-alsa.c index 3f3e141..cbccc96 100644 --- a/drivers/media/usb/tlg2300/pd-alsa.c +++ b/drivers/media/usb/tlg2300/pd-alsa.c @@ -141,6 +141,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed) int len = urb-actual_length / stride; unsigned char *cp = urb-transfer_buffer; unsigned int oldptr = pa-rcv_position; + unsigned long flags; if (urb-actual_length == AUDIO_BUF_SIZE - 4) len -= (AUDIO_TRAILER_SIZE / stride); @@ -156,6 +157,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed) memcpy(runtime-dma_area + oldptr * stride, cp, len * stride); /* update the statas */ + local_irq_save(flags); snd_pcm_stream_lock(pa-capture_pcm_substream); pa-rcv_position+= len; if (pa-rcv_position = runtime-buffer_size) @@ -167,6 +169,7 @@ static inline void handle_audio_data(struct urb *urb, int *period_elapsed) *period_elapsed = 1; } snd_pcm_stream_unlock(pa-capture_pcm_substream); + local_irq_restore(flags); } static void complete_handler_audio(struct urb *urb) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 40/50] media: dvb-core: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
These functions may be called inside URB-complete(), so use
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/media/dvb-core/dvb_demux.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/media/dvb-core/dvb_demux.c
b/drivers/media/dvb-core/dvb_demux.c
index 3485655..58de441 100644
--- a/drivers/media/dvb-core/dvb_demux.c
+++ b/drivers/media/dvb-core/dvb_demux.c
@@ -476,7 +476,9 @@ static void dvb_dmx_swfilter_packet(struct dvb_demux
*demux, const u8 *buf)
void dvb_dmx_swfilter_packets(struct dvb_demux *demux, const u8 *buf,
size_t count)
{
- spin_lock(demux-lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(demux-lock, flags);
while (count--) {
if (buf[0] == 0x47)
@@ -484,7 +486,7 @@ void dvb_dmx_swfilter_packets(struct dvb_demux *demux,
const u8 *buf,
buf += 188;
}
- spin_unlock(demux-lock);
+ spin_unlock_irqrestore(demux-lock, flags);
}
EXPORT_SYMBOL(dvb_dmx_swfilter_packets);
@@ -519,8 +521,9 @@ static inline void _dvb_dmx_swfilter(struct dvb_demux
*demux, const u8 *buf,
{
int p = 0, i, j;
const u8 *q;
+ unsigned long flags;
- spin_lock(demux-lock);
+ spin_lock_irqsave(demux-lock, flags);
if (demux-tsbufp) { /* tsbuf[0] is now 0x47. */
i = demux-tsbufp;
@@ -564,7 +567,7 @@ static inline void _dvb_dmx_swfilter(struct dvb_demux
*demux, const u8 *buf,
}
bailout:
- spin_unlock(demux-lock);
+ spin_unlock_irqrestore(demux-lock, flags);
}
void dvb_dmx_swfilter(struct dvb_demux *demux, const u8 *buf, size_t count)
@@ -581,11 +584,13 @@ EXPORT_SYMBOL(dvb_dmx_swfilter_204);
void dvb_dmx_swfilter_raw(struct dvb_demux *demux, const u8 *buf, size_t count)
{
- spin_lock(demux-lock);
+ unsigned long flags;
+
+ spin_lock_irqsave(demux-lock, flags);
demux-feed-cb.ts(buf, count, NULL, 0, demux-feed-feed.ts, DMX_OK);
- spin_unlock(demux-lock);
+ spin_unlock_irqrestore(demux-lock, flags);
}
EXPORT_SYMBOL(dvb_dmx_swfilter_raw);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 33/50] wireless: libertas: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: John W. Linville linvi...@tuxdriver.com
Cc: libertas-...@lists.infradead.org
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/wireless/libertas/if_usb.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/libertas/if_usb.c
b/drivers/net/wireless/libertas/if_usb.c
index 2798077..f6a8396 100644
--- a/drivers/net/wireless/libertas/if_usb.c
+++ b/drivers/net/wireless/libertas/if_usb.c
@@ -626,6 +626,7 @@ static inline void process_cmdrequest(int recvlength,
uint8_t *recvbuff,
struct lbs_private *priv)
{
u8 i;
+ unsigned long flags;
if (recvlength LBS_CMD_BUFFER_SIZE) {
lbs_deb_usbd(cardp-udev-dev,
@@ -636,7 +637,7 @@ static inline void process_cmdrequest(int recvlength,
uint8_t *recvbuff,
BUG_ON(!in_interrupt());
- spin_lock(priv-driver_lock);
+ spin_lock_irqsave(priv-driver_lock, flags);
i = (priv-resp_idx == 0) ? 1 : 0;
BUG_ON(priv-resp_len[i]);
@@ -646,7 +647,7 @@ static inline void process_cmdrequest(int recvlength,
uint8_t *recvbuff,
kfree_skb(skb);
lbs_notify_command_response(priv, i);
- spin_unlock(priv-driver_lock);
+ spin_unlock_irqrestore(priv-driver_lock, flags);
lbs_deb_usbd(cardp-udev-dev,
Wake up main thread to handle cmd response\n);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 48/50] staging: bcm: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: de...@driverdev.osuosl.org Signed-off-by: Ming Lei ming@canonical.com --- drivers/staging/bcm/InterfaceRx.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/bcm/InterfaceRx.c b/drivers/staging/bcm/InterfaceRx.c index 26f5bc7..00af901 100644 --- a/drivers/staging/bcm/InterfaceRx.c +++ b/drivers/staging/bcm/InterfaceRx.c @@ -47,6 +47,7 @@ static void read_bulk_callback(struct urb *urb) struct bcm_interface_adapter *psIntfAdapter = pRcb-psIntfAdapter; struct bcm_mini_adapter *Adapter = psIntfAdapter-psAdapter; struct bcm_leader *pLeader = urb-transfer_buffer; + unsigned long flags; if (unlikely(netif_msg_rx_status(Adapter))) pr_info(PFX %s: rx urb status %d length %d\n, @@ -129,9 +130,9 @@ static void read_bulk_callback(struct urb *urb) (sizeof(struct bcm_leader)), pLeader-PLength); skb-len = pLeader-PLength + sizeof(USHORT); - spin_lock(Adapter-control_queue_lock); + spin_lock_irqsave(Adapter-control_queue_lock, flags); ENQUEUEPACKET(Adapter-RxControlHead,Adapter-RxControlTail,skb); - spin_unlock(Adapter-control_queue_lock); + spin_unlock_irqretore(Adapter-control_queue_lock, flags); atomic_inc(Adapter-cntrlpktCnt); wake_up(Adapter-process_rx_cntrlpkt); -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 25/50] ISDN: hfcsusb: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Karsten Keil i...@linux-pingi.de
Cc: David S. Miller da...@davemloft.net
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/isdn/hardware/mISDN/hfcsusb.c | 36 ++---
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/drivers/isdn/hardware/mISDN/hfcsusb.c
b/drivers/isdn/hardware/mISDN/hfcsusb.c
index 114f3bc..082f9e0 100644
--- a/drivers/isdn/hardware/mISDN/hfcsusb.c
+++ b/drivers/isdn/hardware/mISDN/hfcsusb.c
@@ -819,6 +819,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
int fifon = fifo-fifonum;
int i;
int hdlc = 0;
+ unsigned long flags;
if (debug DBG_HFC_CALL_TRACE)
printk(KERN_DEBUG %s: %s: fifo(%i) len(%i)
@@ -835,7 +836,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
return;
}
- spin_lock(hw-lock);
+ spin_lock_irqsave(hw-lock, flags);
if (fifo-dch) {
rx_skb = fifo-dch-rx_skb;
maxlen = fifo-dch-maxlen;
@@ -844,7 +845,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
if (fifo-bch) {
if (test_bit(FLG_RX_OFF, fifo-bch-Flags)) {
fifo-bch-dropcnt += len;
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
return;
}
maxlen = bchannel_get_rxbuf(fifo-bch, len);
@@ -854,7 +855,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
skb_trim(rx_skb, 0);
pr_warning(%s.B%d: No bufferspace for %d bytes\n,
hw-name, fifo-bch-nr, len);
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
return;
}
maxlen = fifo-bch-maxlen;
@@ -878,7 +879,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
} else {
printk(KERN_DEBUG %s: %s: No mem for rx_skb\n,
hw-name, __func__);
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
return;
}
}
@@ -888,7 +889,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
for fifo(%d) HFCUSB_D_RX\n,
hw-name, __func__, fifon);
skb_trim(rx_skb, 0);
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
return;
}
}
@@ -942,7 +943,7 @@ hfcsusb_rx_frame(struct usb_fifo *fifo, __u8 *data,
unsigned int len,
/* deliver transparent data to layer2 */
recv_Bchannel(fifo-bch, MISDN_ID_ANY, false);
}
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
}
static void
@@ -979,18 +980,19 @@ rx_iso_complete(struct urb *urb)
__u8 *buf;
static __u8 eof[8];
__u8 s0_state;
+ unsigned long flags;
fifon = fifo-fifonum;
status = urb-status;
- spin_lock(hw-lock);
+ spin_lock_irqsave(hw-lock, flags);
if (fifo-stop_gracefull) {
fifo-stop_gracefull = 0;
fifo-active = 0;
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
return;
}
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
/*
* ISO transfer only partially completed,
@@ -1096,15 +1098,16 @@ rx_int_complete(struct urb *urb)
struct usb_fifo *fifo = (struct usb_fifo *) urb-context;
struct hfcsusb *hw = fifo-hw;
static __u8 eof[8];
+ unsigned long flags;
- spin_lock(hw-lock);
+ spin_lock_irqsave(hw-lock, flags);
if (fifo-stop_gracefull) {
fifo-stop_gracefull = 0;
fifo-active = 0;
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
return;
}
- spin_unlock(hw-lock);
+ spin_unlock_irqrestore(hw-lock, flags);
fifon = fifo-fifonum;
if ((!fifo-active) || (urb-status)) {
@@ -1172,12 +1175,13 @@ tx_iso_complete(struct urb *urb)
int *tx_idx;
int frame_complete, fifon, status, fillempty = 0;
__u8 threshbit, *p;
+ unsigned long flags;
- spin_lock(hw-lock);
+ spin_lock_irqsave(hw-lock, flags);
if (fifo
[PATCH 09/50] USB: usbtest: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/usbtest.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 8b4ca1c..5c73df5 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -804,11 +804,12 @@ static void ctrl_complete(struct urb *urb)
struct usb_ctrlrequest *reqp;
struct subcase *subcase;
int status = urb-status;
+ unsigned long flags;
reqp = (struct usb_ctrlrequest *)urb-setup_packet;
subcase = container_of(reqp, struct subcase, setup);
- spin_lock(ctx-lock);
+ spin_lock_irqsave(ctx-lock, flags);
ctx-count--;
ctx-pending--;
@@ -907,7 +908,7 @@ error:
/* signal completion when nothing's queued */
if (ctx-pending == 0)
complete(ctx-complete);
- spin_unlock(ctx-lock);
+ spin_unlock_irqrestore(ctx-lock, flags);
}
static int
@@ -1552,8 +1553,9 @@ struct iso_context {
static void iso_callback(struct urb *urb)
{
struct iso_context *ctx = urb-context;
+ unsigned long flags;
- spin_lock(ctx-lock);
+ spin_lock_irqsave(ctx-lock, flags);
ctx-count--;
ctx-packet_count += urb-number_of_packets;
@@ -1593,7 +1595,7 @@ static void iso_callback(struct urb *urb)
complete(ctx-done);
}
done:
- spin_unlock(ctx-lock);
+ spin_unlock_irqrestore(ctx-lock, flags);
}
static struct urb *iso_alloc_urb(
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 50/50] staging: vt6656: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: de...@driverdev.osuosl.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/staging/vt6656/usbpipe.c |9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/vt6656/usbpipe.c b/drivers/staging/vt6656/usbpipe.c
index 098be60..0282f2e 100644
--- a/drivers/staging/vt6656/usbpipe.c
+++ b/drivers/staging/vt6656/usbpipe.c
@@ -485,6 +485,7 @@ static void s_nsBulkInUsbIoCompleteRead(struct urb *urb)
int bIndicateReceive = false;
int bReAllocSkb = false;
int status;
+ unsigned long flags;
DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFOs_nsBulkInUsbIoCompleteRead\n);
status = urb-status;
@@ -515,18 +516,18 @@ static void s_nsBulkInUsbIoCompleteRead(struct urb *urb)
STAvUpdateUSBCounter(pDevice-scStatistic.USB_BulkInStat, status);
if (bIndicateReceive) {
-spin_lock(pDevice-lock);
+spin_lock_irqsave(pDevice-lock, flags);
if (RXbBulkInProcessData(pDevice, pRCB, bytesRead) == true)
bReAllocSkb = true;
-spin_unlock(pDevice-lock);
+spin_unlock_irqrestore(pDevice-lock, flags);
}
pRCB-Ref--;
if (pRCB-Ref == 0)
{
DBG_PRT(MSG_LEVEL_DEBUG, KERN_INFORxvFreeNormal %d
\n,pDevice-NumRecvFreeList);
-spin_lock(pDevice-lock);
+spin_lock_irqsave(pDevice-lock, flags);
RXvFreeRCB(pRCB, bReAllocSkb);
-spin_unlock(pDevice-lock);
+spin_unlock_irqrestore(pDevice-lock, flags);
}
return;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/50] USB: serial: io_ti: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/io_ti.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 60054e7..4943194 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -1615,6 +1615,7 @@ static void edge_bulk_in_callback(struct urb *urb)
int retval = 0;
int port_number;
int status = urb-status;
+ unsigned long flags;
switch (status) {
case 0:
@@ -1663,13 +1664,13 @@ static void edge_bulk_in_callback(struct urb *urb)
exit:
/* continue read unless stopped */
- spin_lock(edge_port-ep_lock);
+ spin_lock_irqsave(edge_port-ep_lock, flags);
if (edge_port-ep_read_urb_state == EDGE_READ_URB_RUNNING)
retval = usb_submit_urb(urb, GFP_ATOMIC);
else if (edge_port-ep_read_urb_state == EDGE_READ_URB_STOPPING)
edge_port-ep_read_urb_state = EDGE_READ_URB_STOPPED;
- spin_unlock(edge_port-ep_lock);
+ spin_unlock_irqrestore(edge_port-ep_lock, flags);
if (retval)
dev_err(dev, %s - usb_submit_urb failed with result %d\n,
__func__, retval);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 38/50] media: usb: tlg2300: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/media/usb/tlg2300/pd-video.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/tlg2300/pd-video.c
b/drivers/media/usb/tlg2300/pd-video.c
index 8df668d..4e5bd07 100644
--- a/drivers/media/usb/tlg2300/pd-video.c
+++ b/drivers/media/usb/tlg2300/pd-video.c
@@ -151,11 +151,12 @@ static void init_copy(struct video_data *video, bool
index)
static bool get_frame(struct front_face *front, int *need_init)
{
struct videobuf_buffer *vb = front-curr_frame;
+ unsigned long flags;
if (vb)
return true;
- spin_lock(front-queue_lock);
+ spin_lock_irqsave(front-queue_lock, flags);
if (!list_empty(front-active)) {
vb = list_entry(front-active.next,
struct videobuf_buffer, queue);
@@ -164,7 +165,7 @@ static bool get_frame(struct front_face *front, int
*need_init)
front-curr_frame = vb;
list_del_init(vb-queue);
}
- spin_unlock(front-queue_lock);
+ spin_unlock_irqrestore(front-queue_lock, flags);
return !!vb;
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 15/50] USB: serial: mos77840: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Johan Hovold jhov...@gmail.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/mos7840.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
index 0a818b2..f21dcd0 100644
--- a/drivers/usb/serial/mos7840.c
+++ b/drivers/usb/serial/mos7840.c
@@ -788,17 +788,18 @@ static void mos7840_bulk_out_data_callback(struct urb
*urb)
struct usb_serial_port *port;
int status = urb-status;
int i;
+ unsigned long flags;
mos7840_port = urb-context;
port = mos7840_port-port;
- spin_lock(mos7840_port-pool_lock);
+ spin_lock_irqsave(mos7840_port-pool_lock, flags);
for (i = 0; i NUM_URBS; i++) {
if (urb == mos7840_port-write_urb_pool[i]) {
mos7840_port-busy[i] = 0;
break;
}
}
- spin_unlock(mos7840_port-pool_lock);
+ spin_unlock_irqrestore(mos7840_port-pool_lock, flags);
if (status) {
dev_dbg(port-dev, nonzero write bulk status received:%d\n,
status);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 04/50] USB: adutux: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Lisa Nguyen l...@xenapiadmin.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/adutux.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index eb3c8c1..387c75e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -195,12 +195,13 @@ static void adu_interrupt_in_callback(struct urb *urb)
{
struct adu_device *dev = urb-context;
int status = urb-status;
+ unsigned long flags;
dbg(4, %s : enter, status %d, __func__, status);
adu_debug_data(5, __func__, urb-actual_length,
urb-transfer_buffer);
- spin_lock(dev-buflock);
+ spin_lock_irqsave(dev-buflock, flags);
if (status != 0) {
if ((status != -ENOENT) (status != -ECONNRESET)
@@ -229,7 +230,7 @@ static void adu_interrupt_in_callback(struct urb *urb)
exit:
dev-read_urb_finished = 1;
- spin_unlock(dev-buflock);
+ spin_unlock_irqrestore(dev-buflock, flags);
/* always wake up so we recover from errors */
wake_up_interruptible(dev-read_wait);
adu_debug_data(5, __func__, urb-actual_length,
@@ -241,6 +242,7 @@ static void adu_interrupt_out_callback(struct urb *urb)
{
struct adu_device *dev = urb-context;
int status = urb-status;
+ unsigned long flags;
dbg(4, %s : enter, status %d, __func__, status);
adu_debug_data(5, __func__, urb-actual_length, urb-transfer_buffer);
@@ -254,10 +256,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- spin_lock(dev-buflock);
+ spin_lock_irqsave(dev-buflock, flags);
dev-out_urb_finished = 1;
wake_up(dev-write_wait);
- spin_unlock(dev-buflock);
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
adu_debug_data(5, __func__, urb-actual_length,
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 10/50] USB: serial: cyberjack: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Matthias Bruestle and Harald Welte supp...@reiner-sct.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/serial/cyberjack.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/serial/cyberjack.c b/drivers/usb/serial/cyberjack.c
index 7814262..0ab0957 100644
--- a/drivers/usb/serial/cyberjack.c
+++ b/drivers/usb/serial/cyberjack.c
@@ -271,11 +271,12 @@ static void cyberjack_read_int_callback(struct urb *urb)
/* React only to interrupts signaling a bulk_in transfer */
if (urb-actual_length == 4 data[0] == 0x01) {
short old_rdtodo;
+ unsigned long flags;
/* This is a announcement of coming bulk_ins. */
unsigned short size = ((unsigned short)data[3]8)+data[2]+3;
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
old_rdtodo = priv-rdtodo;
@@ -290,7 +291,7 @@ static void cyberjack_read_int_callback(struct urb *urb)
dev_dbg(dev, %s - rdtodo: %d\n, __func__, priv-rdtodo);
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
if (!old_rdtodo) {
result = usb_submit_urb(port-read_urb, GFP_ATOMIC);
@@ -317,6 +318,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
short todo;
int result;
int status = urb-status;
+ unsigned long flags;
usb_serial_debug_data(dev, __func__, urb-actual_length, data);
if (status) {
@@ -330,7 +332,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
tty_flip_buffer_push(port-port);
}
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
/* Reduce urbs to do by one. */
priv-rdtodo -= urb-actual_length;
@@ -339,7 +341,7 @@ static void cyberjack_read_bulk_callback(struct urb *urb)
priv-rdtodo = 0;
todo = priv-rdtodo;
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
dev_dbg(dev, %s - rdtodo: %d\n, __func__, todo);
@@ -359,6 +361,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
struct cyberjack_private *priv = usb_get_serial_port_data(port);
struct device *dev = port-dev;
int status = urb-status;
+ unsigned long flags;
set_bit(0, port-write_urbs_free);
if (status) {
@@ -367,7 +370,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
return;
}
- spin_lock(priv-lock);
+ spin_lock_irqsave(priv-lock, flags);
/* only do something if we have more data to send */
if (priv-wrfilled) {
@@ -411,7 +414,7 @@ static void cyberjack_write_bulk_callback(struct urb *urb)
}
exit:
- spin_unlock(priv-lock);
+ spin_unlock_irqrestore(priv-lock, flags);
usb_serial_port_softint(port);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 27/50] USBNET: hso: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/usb/hso.c | 38 ++
1 file changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index cba1d46..c865441 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c
@@ -1008,6 +1008,7 @@ static void read_bulk_callback(struct urb *urb)
struct net_device *net;
int result;
int status = urb-status;
+ unsigned long flags;
/* is al ok? (Filip: Who's Al ?) */
if (status) {
@@ -1036,11 +1037,11 @@ static void read_bulk_callback(struct urb *urb)
if (urb-actual_length) {
/* Handle the IP stream, add header and push it onto network
* stack if the packet is complete. */
- spin_lock(odev-net_lock);
+ spin_lock_irqsave(odev-net_lock, flags);
packetizeRx(odev, urb-transfer_buffer, urb-actual_length,
(urb-transfer_buffer_length
urb-actual_length) ? 1 : 0);
- spin_unlock(odev-net_lock);
+ spin_unlock_irqrestore(odev-net_lock, flags);
}
/* We are done with this URB, resubmit it. Prep the USB to wait for
@@ -1201,6 +1202,7 @@ static void hso_std_serial_read_bulk_callback(struct urb
*urb)
{
struct hso_serial *serial = urb-context;
int status = urb-status;
+ unsigned long flags;
/* sanity check */
if (!serial) {
@@ -1223,17 +1225,17 @@ static void hso_std_serial_read_bulk_callback(struct
urb *urb)
if (serial-parent-port_spec HSO_INFO_CRC_BUG)
fix_crc_bug(urb, serial-in_endp-wMaxPacketSize);
/* Valid data, handle RX data */
- spin_lock(serial-serial_lock);
+ spin_lock_irqsave(serial-serial_lock, flags);
serial-rx_urb_filled[hso_urb_to_index(serial, urb)] = 1;
put_rxbuf_data_and_resubmit_bulk_urb(serial);
- spin_unlock(serial-serial_lock);
+ spin_unlock_irqrestore(serial-serial_lock, flags);
} else if (status == -ENOENT || status == -ECONNRESET) {
/* Unlinked - check for throttled port. */
D2(Port %d, successfully unlinked urb, serial-minor);
- spin_lock(serial-serial_lock);
+ spin_lock_irqsave(serial-serial_lock, flags);
serial-rx_urb_filled[hso_urb_to_index(serial, urb)] = 0;
hso_resubmit_rx_bulk_urb(serial, urb);
- spin_unlock(serial-serial_lock);
+ spin_unlock_irqrestore(serial-serial_lock, flags);
} else {
D2(Port %d, status = %d for read urb, serial-minor, status);
return;
@@ -1510,12 +1512,13 @@ static void tiocmget_intr_callback(struct urb *urb)
DUMP(serial_state_notification,
sizeof(struct hso_serial_state_notification));
} else {
+ unsigned long flags;
UART_state_bitmap = le16_to_cpu(serial_state_notification-
UART_state_bitmap);
prev_UART_state_bitmap = tiocmget-prev_UART_state_bitmap;
icount = tiocmget-icount;
- spin_lock(serial-serial_lock);
+ spin_lock_irqsave(serial-serial_lock, flags);
if ((UART_state_bitmap B_OVERRUN) !=
(prev_UART_state_bitmap B_OVERRUN))
icount-parity++;
@@ -1538,7 +1541,7 @@ static void tiocmget_intr_callback(struct urb *urb)
(prev_UART_state_bitmap B_RX_CARRIER))
icount-dcd++;
tiocmget-prev_UART_state_bitmap = UART_state_bitmap;
- spin_unlock(serial-serial_lock);
+ spin_unlock_irqrestore(serial-serial_lock, flags);
tiocmget-intr_completed = 1;
wake_up_interruptible(tiocmget-waitq);
}
@@ -1883,8 +1886,9 @@ static void intr_callback(struct urb *urb)
serial = get_serial_by_shared_int_and_type(shared_int,
(1 i));
if (serial != NULL) {
+ unsigned long flags;
D1(Pending read interrupt on port %d\n, i);
- spin_lock(serial-serial_lock);
+ spin_lock_irqsave(serial-serial_lock, flags);
if (serial-rx_state == RX_IDLE
serial-port.count 0) {
/* Setup and send a ctrl req read on
@@ -1898,7 +1902,7 @@ static void intr_callback(struct urb *urb
[PATCH 23/50] BT: bfusb: read_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
read_lock_irqsave().
Cc: Marcel Holtmann mar...@holtmann.org
Cc: Gustavo Padovan gust...@padovan.org
Cc: Johan Hedberg johan.hedb...@gmail.com
Cc: linux-blueto...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/bluetooth/bfusb.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/bfusb.c b/drivers/bluetooth/bfusb.c
index 995aee9..2e93501 100644
--- a/drivers/bluetooth/bfusb.c
+++ b/drivers/bluetooth/bfusb.c
@@ -186,6 +186,7 @@ static void bfusb_tx_complete(struct urb *urb)
{
struct sk_buff *skb = (struct sk_buff *) urb-context;
struct bfusb_data *data = (struct bfusb_data *) skb-dev;
+ unsigned long flags;
BT_DBG(bfusb %p urb %p skb %p len %d, data, urb, skb, skb-len);
@@ -199,14 +200,14 @@ static void bfusb_tx_complete(struct urb *urb)
else
data-hdev-stat.err_tx++;
- read_lock(data-lock);
+ read_lock_irqsave(data-lock, flags);
skb_unlink(skb, data-pending_q);
skb_queue_tail(data-completed_q, skb);
bfusb_tx_wakeup(data);
- read_unlock(data-lock);
+ read_unlock_irqrestore(data-lock, flags);
}
@@ -347,10 +348,11 @@ static void bfusb_rx_complete(struct urb *urb)
unsigned char *buf = urb-transfer_buffer;
int count = urb-actual_length;
int err, hdr, len;
+ unsigned long flags;
BT_DBG(bfusb %p urb %p skb %p len %d, data, urb, skb, skb-len);
- read_lock(data-lock);
+ read_lock_irqsave(data-lock, flags);
if (!test_bit(HCI_RUNNING, data-hdev-flags))
goto unlock;
@@ -392,7 +394,7 @@ static void bfusb_rx_complete(struct urb *urb)
bfusb_rx_submit(data, urb);
- read_unlock(data-lock);
+ read_unlock_irqrestore(data-lock, flags);
return;
@@ -406,7 +408,7 @@ resubmit:
}
unlock:
- read_unlock(data-lock);
+ read_unlock_irqrestore(data-lock, flags);
}
static int bfusb_open(struct hci_dev *hdev)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/50] USB: misc: uss720: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/uss720.c |6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/misc/uss720.c b/drivers/usb/misc/uss720.c
index e129cf6..f7d15e8 100644
--- a/drivers/usb/misc/uss720.c
+++ b/drivers/usb/misc/uss720.c
@@ -121,6 +121,7 @@ static void async_complete(struct urb *urb)
dev_err(urb-dev-dev, async_complete: urb error %d\n,
status);
} else if (rq-dr.bRequest == 3) {
+ unsigned long flags;
memcpy(priv-reg, rq-reg, sizeof(priv-reg));
#if 0
dev_dbg(priv-usbdev-dev,
@@ -131,8 +132,11 @@ static void async_complete(struct urb *urb)
(unsigned int)priv-reg[6]);
#endif
/* if nAck interrupts are enabled and we have an interrupt,
call the interrupt procedure */
- if (rq-reg[2] rq-reg[1] 0x10 pp)
+ if (rq-reg[2] rq-reg[1] 0x10 pp) {
+ local_irq_save(flags);
parport_generic_irq(pp);
+ local_irq_restore(flags);
+ }
}
complete(rq-compl);
kref_put(rq-ref_count, destroy_async);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 34/50] wireless: libertas_tf: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: John W. Linville linvi...@tuxdriver.com
Cc: libertas-...@lists.infradead.org
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/wireless/libertas_tf/if_usb.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/libertas_tf/if_usb.c
b/drivers/net/wireless/libertas_tf/if_usb.c
index d576dd6..0e9e972 100644
--- a/drivers/net/wireless/libertas_tf/if_usb.c
+++ b/drivers/net/wireless/libertas_tf/if_usb.c
@@ -610,6 +610,8 @@ static inline void process_cmdrequest(int recvlength,
uint8_t *recvbuff,
struct if_usb_card *cardp,
struct lbtf_private *priv)
{
+ unsigned long flags;
+
if (recvlength LBS_CMD_BUFFER_SIZE) {
lbtf_deb_usbd(cardp-udev-dev,
The receive buffer is too large\n);
@@ -619,12 +621,12 @@ static inline void process_cmdrequest(int recvlength,
uint8_t *recvbuff,
BUG_ON(!in_interrupt());
- spin_lock(priv-driver_lock);
+ spin_lock_irqsave(priv-driver_lock, flags);
memcpy(priv-cmd_resp_buff, recvbuff + MESSAGE_HEADER_LEN,
recvlength - MESSAGE_HEADER_LEN);
kfree_skb(skb);
lbtf_cmd_response_rx(priv);
- spin_unlock(priv-driver_lock);
+ spin_unlock_irqrestore(priv-driver_lock, flags);
}
/**
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 31/50] wireless: zd1211rw: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Daniel Drake d...@gentoo.org
Cc: Ulrich Kunitz k...@deine-taler.de
Cc: John W. Linville linvi...@tuxdriver.com
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/wireless/zd1211rw/zd_usb.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/zd1211rw/zd_usb.c
b/drivers/net/wireless/zd1211rw/zd_usb.c
index 7ef0b4a..8169ee0 100644
--- a/drivers/net/wireless/zd1211rw/zd_usb.c
+++ b/drivers/net/wireless/zd1211rw/zd_usb.c
@@ -372,14 +372,15 @@ static inline void handle_regs_int_override(struct urb
*urb)
{
struct zd_usb *usb = urb-context;
struct zd_usb_interrupt *intr = usb-intr;
+ unsigned long flags;
- spin_lock(intr-lock);
+ spin_lock_irqsave(intr-lock, flags);
if (atomic_read(intr-read_regs_enabled)) {
atomic_set(intr-read_regs_enabled, 0);
intr-read_regs_int_overridden = 1;
complete(intr-read_regs.completion);
}
- spin_unlock(intr-lock);
+ spin_unlock_irqrestore(intr-lock, flags);
}
static inline void handle_regs_int(struct urb *urb)
@@ -388,9 +389,10 @@ static inline void handle_regs_int(struct urb *urb)
struct zd_usb_interrupt *intr = usb-intr;
int len;
u16 int_num;
+ unsigned long flags;
ZD_ASSERT(in_interrupt());
- spin_lock(intr-lock);
+ spin_lock_irqsave(intr-lock, flags);
int_num = le16_to_cpu(*(__le16 *)(urb-transfer_buffer+2));
if (int_num == CR_INTERRUPT) {
@@ -426,7 +428,7 @@ static inline void handle_regs_int(struct urb *urb)
}
out:
- spin_unlock(intr-lock);
+ spin_unlock_irqrestore(intr-lock, flags);
/* CR_INTERRUPT might override read_reg too. */
if (int_num == CR_INTERRUPT atomic_read(intr-read_regs_enabled))
@@ -666,6 +668,7 @@ static void rx_urb_complete(struct urb *urb)
struct zd_usb_rx *rx;
const u8 *buffer;
unsigned int length;
+ unsigned long flags;
switch (urb-status) {
case 0:
@@ -694,14 +697,14 @@ static void rx_urb_complete(struct urb *urb)
/* If there is an old first fragment, we don't care. */
dev_dbg_f(urb_dev(urb), *** first fragment ***\n);
ZD_ASSERT(length = ARRAY_SIZE(rx-fragment));
- spin_lock(rx-lock);
+ spin_lock_irqsave(rx-lock, flags);
memcpy(rx-fragment, buffer, length);
rx-fragment_length = length;
- spin_unlock(rx-lock);
+ spin_unlock_irqrestore(rx-lock, flags);
goto resubmit;
}
- spin_lock(rx-lock);
+ spin_lock_irqsave(rx-lock, flags);
if (rx-fragment_length 0) {
/* We are on a second fragment, we believe */
ZD_ASSERT(length + rx-fragment_length =
@@ -711,9 +714,9 @@ static void rx_urb_complete(struct urb *urb)
handle_rx_packet(usb, rx-fragment,
rx-fragment_length + length);
rx-fragment_length = 0;
- spin_unlock(rx-lock);
+ spin_unlock_irqrestore(rx-lock, flags);
} else {
- spin_unlock(rx-lock);
+ spin_unlock_irqrestore(rx-lock, flags);
handle_rx_packet(usb, buffer, length);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 37/50] media: usb: sn9x102: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/media/usb/sn9c102/sn9c102_core.c |7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/sn9c102/sn9c102_core.c
b/drivers/media/usb/sn9c102/sn9c102_core.c
index 2cb44de..33dc595 100644
--- a/drivers/media/usb/sn9c102/sn9c102_core.c
+++ b/drivers/media/usb/sn9c102/sn9c102_core.c
@@ -784,12 +784,14 @@ end_of_frame:
cam-sensor.pix_format.pixelformat ==
V4L2_PIX_FMT_JPEG) eof)) {
u32 b;
+ unsigned long flags;
b = (*f)-buf.bytesused;
(*f)-state = F_DONE;
(*f)-buf.sequence= ++cam-frame_count;
- spin_lock(cam-queue_lock);
+ spin_lock_irqsave(cam-queue_lock,
+ flags);
list_move_tail((*f)-frame,
cam-outqueue);
if (!list_empty(cam-inqueue))
@@ -799,7 +801,8 @@ end_of_frame:
frame );
else
(*f) = NULL;
- spin_unlock(cam-queue_lock);
+ spin_unlock_irqrestore(cam-queue_lock,
+ flags);
memcpy(cam-sysfs.frame_header,
cam-sof.header, soflen);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 47/50] staging: btmtk_usb: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: de...@driverdev.osuosl.org Signed-off-by: Ming Lei ming@canonical.com --- drivers/staging/btmtk_usb/btmtk_usb.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/btmtk_usb/btmtk_usb.c b/drivers/staging/btmtk_usb/btmtk_usb.c index 0e783e8..ea10d4f 100644 --- a/drivers/staging/btmtk_usb/btmtk_usb.c +++ b/drivers/staging/btmtk_usb/btmtk_usb.c @@ -1218,6 +1218,7 @@ static void btmtk_usb_tx_complete(struct urb *urb) struct sk_buff *skb = urb-context; struct hci_dev *hdev = (struct hci_dev *)skb-dev; struct btmtk_usb_data *data = hci_get_drvdata(hdev); + unsigned long flags; BT_DBG(%s: %s urb %p status %d count %d\n, __func__, hdev-name, urb, urb-status, urb-actual_length); @@ -1231,9 +1232,9 @@ static void btmtk_usb_tx_complete(struct urb *urb) hdev-stat.err_tx++; done: - spin_lock(data-txlock); + spin_lock_irqsave(data-txlock, flags); data-tx_in_flight--; - spin_unlock(data-txlock); + spin_unlock_irqrestore(data-txlock, flags); kfree(urb-setup_packet); -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 36/50] media: usb: em28xx: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/media/usb/em28xx/em28xx-core.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/em28xx/em28xx-core.c
b/drivers/media/usb/em28xx/em28xx-core.c
index fc157af..0d698f9 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -941,6 +941,7 @@ static void em28xx_irq_callback(struct urb *urb)
{
struct em28xx *dev = urb-context;
int i;
+ unsigned long flags;
switch (urb-status) {
case 0: /* success */
@@ -956,9 +957,9 @@ static void em28xx_irq_callback(struct urb *urb)
}
/* Copy data from URB */
- spin_lock(dev-slock);
+ spin_lock_irqsave(dev-slock, flags);
dev-usb_ctl.urb_data_copy(dev, urb);
- spin_unlock(dev-slock);
+ spin_unlock_irqrestore(dev-slock, flags);
/* Reset urb buffers */
for (i = 0; i urb-number_of_packets; i++) {
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber starb...@users.sourceforge.net
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/legousbtower.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c
index 8089479..4044989 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
struct lego_usb_tower *dev = urb-context;
int status = urb-status;
int retval;
+ unsigned long flags;
dbg(4, %s: enter, status %d, __func__, status);
@@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
}
if (urb-actual_length 0) {
- spin_lock (dev-read_buffer_lock);
+ spin_lock_irqsave (dev-read_buffer_lock, flags);
if (dev-read_buffer_length + urb-actual_length
read_buffer_size) {
memcpy (dev-read_buffer + dev-read_buffer_length,
dev-interrupt_in_buffer,
@@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb *urb)
} else {
printk(KERN_WARNING %s: read_buffer overflow, %d bytes
dropped, __func__, urb-actual_length);
}
- spin_unlock (dev-read_buffer_lock);
+ spin_unlock_irqrestore (dev-read_buffer_lock, flags);
}
resubmit:
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 29/50] USBNET: rtl8150: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: net...@vger.kernel.org Signed-off-by: Ming Lei ming@canonical.com --- drivers/net/usb/rtl8150.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c index 6cbdac6..199e0fb 100644 --- a/drivers/net/usb/rtl8150.c +++ b/drivers/net/usb/rtl8150.c @@ -372,6 +372,7 @@ static void read_bulk_callback(struct urb *urb) u16 rx_stat; int status = urb-status; int result; + unsigned long flags; dev = urb-context; if (!dev) @@ -413,9 +414,9 @@ static void read_bulk_callback(struct urb *urb) netdev-stats.rx_packets++; netdev-stats.rx_bytes += pkt_len; - spin_lock(dev-rx_pool_lock); + spin_lock_irqsave(dev-rx_pool_lock, flags); skb = pull_skb(dev); - spin_unlock(dev-rx_pool_lock); + spin_unlock_irqrestore(dev-rx_pool_lock, flags); if (!skb) goto resched; -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 26/50] USBNET: cdc-phonet: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/usb/cdc-phonet.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/usb/cdc-phonet.c b/drivers/net/usb/cdc-phonet.c
index 7d78669..413ec32 100644
--- a/drivers/net/usb/cdc-phonet.c
+++ b/drivers/net/usb/cdc-phonet.c
@@ -99,6 +99,7 @@ static void tx_complete(struct urb *req)
struct net_device *dev = skb-dev;
struct usbpn_dev *pnd = netdev_priv(dev);
int status = req-status;
+ unsigned long flags;
switch (status) {
case 0:
@@ -115,10 +116,10 @@ static void tx_complete(struct urb *req)
}
dev-stats.tx_packets++;
- spin_lock(pnd-tx_lock);
+ spin_lock_irqsave(pnd-tx_lock, flags);
pnd-tx_queue--;
netif_wake_queue(dev);
- spin_unlock(pnd-tx_lock);
+ spin_unlock_irqrestore(pnd-tx_lock, flags);
dev_kfree_skb_any(skb);
usb_free_urb(req);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 22/50] BT: btusb: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Marcel Holtmann mar...@holtmann.org Cc: Gustavo Padovan gust...@padovan.org Cc: Johan Hedberg johan.hedb...@gmail.com Cc: linux-blueto...@vger.kernel.org Signed-off-by: Ming Lei ming@canonical.com --- drivers/bluetooth/btusb.c |5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index ea63958..018b8b0 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -573,6 +573,7 @@ static void btusb_tx_complete(struct urb *urb) struct sk_buff *skb = urb-context; struct hci_dev *hdev = (struct hci_dev *) skb-dev; struct btusb_data *data = hci_get_drvdata(hdev); + unsigned long flags; BT_DBG(%s urb %p status %d count %d, hdev-name, urb, urb-status, urb-actual_length); @@ -586,9 +587,9 @@ static void btusb_tx_complete(struct urb *urb) hdev-stat.err_tx++; done: - spin_lock(data-txlock); + spin_lock_irqsave(data-txlock, flags); data-tx_in_flight--; - spin_unlock(data-txlock); + spin_unlock_irqrestore(data-txlock, flags); kfree(urb-setup_packet); -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 44/50] sound: usb: caiaq: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Daniel Mack zon...@gmail.com
Cc: Jaroslav Kysela pe...@perex.cz
Cc: Takashi Iwai ti...@suse.de
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei ming@canonical.com
---
sound/usb/caiaq/audio.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 7103b09..e5675ab 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -672,10 +672,11 @@ static void read_completed(struct urb *urb)
offset += len;
if (len 0) {
- spin_lock(cdev-spinlock);
+ unsigned long flags;
+ spin_lock_irqsave(cdev-spinlock, flags);
fill_out_urb(cdev, out, out-iso_frame_desc[outframe]);
read_in_urb(cdev, urb, urb-iso_frame_desc[frame]);
- spin_unlock(cdev-spinlock);
+ spin_unlock_irqrestore(cdev-spinlock, flags);
check_for_elapsed_periods(cdev, cdev-sub_playback);
check_for_elapsed_periods(cdev, cdev-sub_capture);
send_it = 1;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Jaroslav Kysela pe...@perex.cz
Cc: Takashi Iwai ti...@suse.de
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei ming@canonical.com
---
sound/usb/usx2y/usbusx2yaudio.c |4
1 file changed, 4 insertions(+)
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 4967fe9..e2ee893 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
struct snd_usX2Y_substream *subs = usX2Y-subs[s];
if (subs) {
if (atomic_read(subs-state) = state_PRERUNNING) {
+ unsigned long flags;
+
+ local_irq_save(flags);
snd_pcm_stop(subs-pcm_substream,
SNDRV_PCM_STATE_XRUN);
+ local_irq_restore(flags);
}
for (u = 0; u NRURBS; u++) {
struct urb *urb = subs-urb[u];
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 39/50] media: usb: tm6000: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Mauro Carvalho Chehab mche...@redhat.com
Cc: linux-me...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/media/usb/tm6000/tm6000-video.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/tm6000/tm6000-video.c
b/drivers/media/usb/tm6000/tm6000-video.c
index cc1aa14..8bb440f 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -434,6 +434,7 @@ static void tm6000_irq_callback(struct urb *urb)
struct tm6000_dmaqueue *dma_q = urb-context;
struct tm6000_core *dev = container_of(dma_q, struct tm6000_core, vidq);
int i;
+ unsigned long flags;
switch (urb-status) {
case 0:
@@ -450,9 +451,9 @@ static void tm6000_irq_callback(struct urb *urb)
break;
}
- spin_lock(dev-slock);
+ spin_lock_irqsave(dev-slock, flags);
tm6000_isoc_copy(urb);
- spin_unlock(dev-slock);
+ spin_unlock_irqrestore(dev-slock, flags);
/* Reset urb buffers */
for (i = 0; i urb-number_of_packets; i++) {
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 49/50] staging: ced1401: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: de...@driverdev.osuosl.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/staging/ced1401/usb1401.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/staging/ced1401/usb1401.c
b/drivers/staging/ced1401/usb1401.c
index 97c55f9..70d2f43 100644
--- a/drivers/staging/ced1401/usb1401.c
+++ b/drivers/staging/ced1401/usb1401.c
@@ -265,6 +265,7 @@ static void ced_writechar_callback(struct urb *pUrb)
{
DEVICE_EXTENSION *pdx = pUrb-context;
int nGot = pUrb-actual_length; /* what we transferred */
+ unsigned long flags;
if (pUrb-status) { /* sync/async unlink faults aren't errors */
if (!
@@ -275,24 +276,24 @@ static void ced_writechar_callback(struct urb *pUrb)
__func__, pUrb-status);
}
- spin_lock(pdx-err_lock);
+ spin_lock_irqsave(pdx-err_lock, flags);
pdx-errors = pUrb-status;
- spin_unlock(pdx-err_lock);
+ spin_unlock_irqrestore(pdx-err_lock, flags);
nGot = 0; /* and tidy up again if so */
- spin_lock(pdx-charOutLock); /* already at irq level */
+ spin_lock_irqsave(pdx-charOutLock, flags);/* already at
irq level */
pdx-dwOutBuffGet = 0; /* Reset the output buffer */
pdx-dwOutBuffPut = 0;
pdx-dwNumOutput = 0; /* Clear the char count */
pdx-bPipeError[0] = 1; /* Flag an error for later */
pdx-bSendCharsPending = false; /* Allow other threads again */
- spin_unlock(pdx-charOutLock); /* already at irq level */
+ spin_unlock_irqrestore(pdx-charOutLock, flags); /*
already at irq level */
dev_dbg(pdx-interface-dev,
%s - char out done, 0 chars sent, __func__);
} else {
dev_dbg(pdx-interface-dev,
%s - char out done, %d chars sent, __func__, nGot);
- spin_lock(pdx-charOutLock); /* already at irq level */
+ spin_lock_irqsave(pdx-charOutLock, flags);/* already at
irq level */
pdx-dwNumOutput -= nGot; /* Now adjust the char send
buffer */
pdx-dwOutBuffGet += nGot; /* to match what we did */
if (pdx-dwOutBuffGet = OUTBUF_SZ) /* Can't do this any
earlier as data could be overwritten */
@@ -305,7 +306,7 @@ static void ced_writechar_callback(struct urb *pUrb)
unsigned int dwCount = pdx-dwNumOutput;/*
maximum to send */
if ((pdx-dwOutBuffGet + dwCount) OUTBUF_SZ) /*
does it cross buffer end? */
dwCount = OUTBUF_SZ - pdx-dwOutBuffGet;
- spin_unlock(pdx-charOutLock); /* we are done with
stuff that changes */
+ spin_unlock_irqrestore(pdx-charOutLock, flags);
/* we are done with stuff that changes */
memcpy(pdx-pCoherCharOut, pDat, dwCount); /*
copy output data to the buffer */
usb_fill_bulk_urb(pdx-pUrbCharOut, pdx-udev,
usb_sndbulkpipe(pdx-udev,
@@ -318,7 +319,7 @@ static void ced_writechar_callback(struct urb *pUrb)
iReturn = usb_submit_urb(pdx-pUrbCharOut, GFP_ATOMIC);
dev_dbg(pdx-interface-dev, %s n=%d%s, __func__,
dwCount, pDat);
- spin_lock(pdx-charOutLock); /* grab lock for
errors */
+ spin_lock_irqsave(pdx-charOutLock, flags);/*
grab lock for errors */
if (iReturn) {
pdx-bPipeError[nPipe] = 1; /* Flag an
error to be handled later */
pdx-bSendCharsPending = false; /* Allow other
threads again */
@@ -329,7 +330,7 @@ static void ced_writechar_callback(struct urb *pUrb)
}
} else
pdx-bSendCharsPending = false; /* Allow other threads
again */
- spin_unlock(pdx-charOutLock); /* already at irq level */
+ spin_unlock_irqrestore(pdx-charOutLock, flags); /*
already at irq level */
}
}
@@ -505,8 +506,9 @@ static void staged_callback(struct urb *pUrb)
unsigned int nGot = pUrb-actual_length;/* what we transferred
*/
bool bCancel = false;
bool bRestartCharInput; /* used at the end */
+ unsigned long flags;
- spin_lock(pdx-stagedLock);/* stop ReadWriteMem() action while
this routine is running */
+ spin_lock_irqsave(pdx-stagedLock, flags); /* stop ReadWriteMem()
action while this routine
[PATCH 43/50] sound: usb: midi: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Jaroslav Kysela pe...@perex.cz
Cc: Takashi Iwai ti...@suse.de
Cc: Clemens Ladisch clem...@ladisch.de
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei ming@canonical.com
---
sound/usb/midi.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index b901f46..86af276 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -279,15 +279,16 @@ static void snd_usbmidi_out_urb_complete(struct urb* urb)
struct out_urb_context *context = urb-context;
struct snd_usb_midi_out_endpoint* ep = context-ep;
unsigned int urb_index;
+ unsigned long flags;
- spin_lock(ep-buffer_lock);
+ spin_lock_irqsave(ep-buffer_lock, flags);
urb_index = context - ep-urbs;
ep-active_urbs = ~(1 urb_index);
if (unlikely(ep-drain_urbs)) {
ep-drain_urbs = ~(1 urb_index);
wake_up(ep-drain_wait);
}
- spin_unlock(ep-buffer_lock);
+ spin_unlock_irqrestore(ep-buffer_lock, flags);
if (urb-status 0) {
int err = snd_usbmidi_urb_error(urb-status);
if (err 0) {
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 06/50] USB: iowarrior: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/iowarrior.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/iowarrior.c b/drivers/usb/misc/iowarrior.c
index d36f34e..010ed6d 100644
--- a/drivers/usb/misc/iowarrior.c
+++ b/drivers/usb/misc/iowarrior.c
@@ -162,6 +162,7 @@ static void iowarrior_callback(struct urb *urb)
int offset;
int status = urb-status;
int retval;
+ unsigned long flags;
switch (status) {
case 0:
@@ -175,7 +176,7 @@ static void iowarrior_callback(struct urb *urb)
goto exit;
}
- spin_lock(dev-intr_idx_lock);
+ spin_lock_irqsave(dev-intr_idx_lock, flags);
intr_idx = atomic_read(dev-intr_idx);
/* aux_idx become previous intr_idx */
aux_idx = (intr_idx == 0) ? (MAX_INTERRUPT_BUFFER - 1) : (intr_idx - 1);
@@ -211,7 +212,7 @@ static void iowarrior_callback(struct urb *urb)
*(dev-read_queue + offset + (dev-report_size)) = dev-serial_number++;
atomic_set(dev-intr_idx, aux_idx);
- spin_unlock(dev-intr_idx_lock);
+ spin_unlock_irqrestore(dev-intr_idx_lock, flags);
/* tell the blocking read about the new data */
wake_up_interruptible(dev-read_wait);
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 32/50] wireless: ath: carl9170: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Christian Lamparter chunk...@googlemail.com
Cc: John W. Linville linvi...@tuxdriver.com
Cc: linux-wirel...@vger.kernel.org
Cc: net...@vger.kernel.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/net/wireless/ath/carl9170/rx.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/carl9170/rx.c
b/drivers/net/wireless/ath/carl9170/rx.c
index 4684dd9..61f62a6 100644
--- a/drivers/net/wireless/ath/carl9170/rx.c
+++ b/drivers/net/wireless/ath/carl9170/rx.c
@@ -129,6 +129,7 @@ static int carl9170_check_sequence(struct ar9170 *ar,
unsigned int seq)
static void carl9170_cmd_callback(struct ar9170 *ar, u32 len, void *buffer)
{
+ unsigned long flags;
/*
* Some commands may have a variable response length
* and we cannot predict the correct length in advance.
@@ -148,7 +149,7 @@ static void carl9170_cmd_callback(struct ar9170 *ar, u32
len, void *buffer)
carl9170_restart(ar, CARL9170_RR_INVALID_RSP);
}
- spin_lock(ar-cmd_lock);
+ spin_lock_irqsave(ar-cmd_lock, flags);
if (ar-readbuf) {
if (len = 4)
memcpy(ar-readbuf, buffer + 4, len - 4);
@@ -156,7 +157,7 @@ static void carl9170_cmd_callback(struct ar9170 *ar, u32
len, void *buffer)
ar-readbuf = NULL;
}
complete(ar-cmd_wait);
- spin_unlock(ar-cmd_lock);
+ spin_unlock_irqrestore(ar-cmd_lock, flags);
}
void carl9170_handle_command_response(struct ar9170 *ar, void *buf, u32 len)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/50] USB: usblp: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Pete Zaitcev zait...@redhat.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/class/usblp.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/class/usblp.c b/drivers/usb/class/usblp.c
index d4c47d5..04163d8 100644
--- a/drivers/usb/class/usblp.c
+++ b/drivers/usb/class/usblp.c
@@ -297,6 +297,7 @@ static void usblp_bulk_read(struct urb *urb)
{
struct usblp *usblp = urb-context;
int status = urb-status;
+ unsigned long flags;
if (usblp-present usblp-used) {
if (status)
@@ -304,14 +305,14 @@ static void usblp_bulk_read(struct urb *urb)
nonzero read bulk status received: %d\n,
usblp-minor, status);
}
- spin_lock(usblp-lock);
+ spin_lock_irqsave(usblp-lock, flags);
if (status 0)
usblp-rstatus = status;
else
usblp-rstatus = urb-actual_length;
usblp-rcomplete = 1;
wake_up(usblp-rwait);
- spin_unlock(usblp-lock);
+ spin_unlock_irqrestore(usblp-lock, flags);
usb_free_urb(urb);
}
@@ -320,6 +321,7 @@ static void usblp_bulk_write(struct urb *urb)
{
struct usblp *usblp = urb-context;
int status = urb-status;
+ unsigned long flags;
if (usblp-present usblp-used) {
if (status)
@@ -327,7 +329,7 @@ static void usblp_bulk_write(struct urb *urb)
nonzero write bulk status received: %d\n,
usblp-minor, status);
}
- spin_lock(usblp-lock);
+ spin_lock_irqsave(usblp-lock, flags);
if (status 0)
usblp-wstatus = status;
else
@@ -335,7 +337,7 @@ static void usblp_bulk_write(struct urb *urb)
usblp-no_paper = 0;
usblp-wcomplete = 1;
wake_up(usblp-wwait);
- spin_unlock(usblp-lock);
+ spin_unlock_irqrestore(usblp-lock, flags);
usb_free_urb(urb);
}
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/50] USB: cdc-wdm: spin_lock in complete() cleanup
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Oliver Neukum oli...@neukum.org
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/class/cdc-wdm.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/class/cdc-wdm.c b/drivers/usb/class/cdc-wdm.c
index 8a230f0..5f78d18 100644
--- a/drivers/usb/class/cdc-wdm.c
+++ b/drivers/usb/class/cdc-wdm.c
@@ -143,10 +143,12 @@ found:
static void wdm_out_callback(struct urb *urb)
{
struct wdm_device *desc;
+ unsigned long flags;
+
desc = urb-context;
- spin_lock(desc-iuspin);
+ spin_lock_irqsave(desc-iuspin, flags);
desc-werr = urb-status;
- spin_unlock(desc-iuspin);
+ spin_unlock_irqrestore(desc-iuspin, flags);
kfree(desc-outbuf);
desc-outbuf = NULL;
clear_bit(WDM_IN_USE, desc-flags);
@@ -158,8 +160,9 @@ static void wdm_in_callback(struct urb *urb)
struct wdm_device *desc = urb-context;
int status = urb-status;
int length = urb-actual_length;
+ unsigned long flags;
- spin_lock(desc-iuspin);
+ spin_lock_irqsave(desc-iuspin, flags);
clear_bit(WDM_RESPONDING, desc-flags);
if (status) {
@@ -203,7 +206,7 @@ skip_error:
wake_up(desc-wait);
set_bit(WDM_READ, desc-flags);
- spin_unlock(desc-iuspin);
+ spin_unlock_irqrestore(desc-iuspin, flags);
}
static void wdm_int_callback(struct urb *urb)
@@ -212,6 +215,7 @@ static void wdm_int_callback(struct urb *urb)
int status = urb-status;
struct wdm_device *desc;
struct usb_cdc_notification *dr;
+ unsigned long flags;
desc = urb-context;
dr = (struct usb_cdc_notification *)desc-sbuf;
@@ -260,7 +264,7 @@ static void wdm_int_callback(struct urb *urb)
goto exit;
}
- spin_lock(desc-iuspin);
+ spin_lock_irqsave(desc-iuspin, flags);
clear_bit(WDM_READ, desc-flags);
set_bit(WDM_RESPONDING, desc-flags);
if (!test_bit(WDM_DISCONNECTING, desc-flags)
@@ -269,7 +273,7 @@ static void wdm_int_callback(struct urb *urb)
dev_dbg(desc-intf-dev, %s: usb_submit_urb %d,
__func__, rv);
}
- spin_unlock(desc-iuspin);
+ spin_unlock_irqrestore(desc-iuspin, flags);
if (rv 0) {
clear_bit(WDM_RESPONDING, desc-flags);
if (rv == -EPERM)
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/50] USB: legousbtower: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 8:18 PM, Sergei Shtylyov
sergei.shtyl...@cogentembedded.com wrote:
Hello.
On 11-07-2013 13:05, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Cc: Juergen Stuber starb...@users.sourceforge.net
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/misc/legousbtower.c |5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/legousbtower.c
b/drivers/usb/misc/legousbtower.c
index 8089479..4044989 100644
--- a/drivers/usb/misc/legousbtower.c
+++ b/drivers/usb/misc/legousbtower.c
@@ -771,6 +771,7 @@ static void tower_interrupt_in_callback (struct urb
*urb)
struct lego_usb_tower *dev = urb-context;
int status = urb-status;
int retval;
+ unsigned long flags;
dbg(4, %s: enter, status %d, __func__, status);
@@ -788,7 +789,7 @@ static void tower_interrupt_in_callback (struct urb
*urb)
}
if (urb-actual_length 0) {
- spin_lock (dev-read_buffer_lock);
+ spin_lock_irqsave (dev-read_buffer_lock, flags);
if (dev-read_buffer_length + urb-actual_length
read_buffer_size) {
memcpy (dev-read_buffer +
dev-read_buffer_length,
dev-interrupt_in_buffer,
@@ -799,7 +800,7 @@ static void tower_interrupt_in_callback (struct urb
*urb)
} else {
printk(KERN_WARNING %s: read_buffer overflow, %d
bytes dropped, __func__, urb-actual_length);
}
- spin_unlock (dev-read_buffer_lock);
+ spin_unlock_irqrestore (dev-read_buffer_lock, flags);
}
I don't think this patch passes checkpatch.pl.
No errors reported from checkpatch.pl, only warnings which isn't introduced
by this patch.
Thanks,
--
Ming Lei
--
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 17/50] USB: serial: sierra: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 9:02 PM, Sergei Shtylyov sergei.shtyl...@cogentembedded.com wrote: Hello. On 11-07-2013 13:05, Ming Lei wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Johan Hovold jhov...@gmail.com Signed-off-by: Ming Lei ming@canonical.com --- drivers/usb/serial/sierra.c |9 + 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/usb/serial/sierra.c b/drivers/usb/serial/sierra.c index de958c5..e79b6ad 100644 --- a/drivers/usb/serial/sierra.c +++ b/drivers/usb/serial/sierra.c @@ -433,6 +433,7 @@ static void sierra_outdat_callback(struct urb *urb) struct sierra_port_private *portdata = usb_get_serial_port_data(port); struct sierra_intf_private *intfdata; int status = urb-status; + unsigned long flags; intfdata = port-serial-private; @@ -443,12 +444,12 @@ static void sierra_outdat_callback(struct urb *urb) dev_dbg(port-dev, %s - nonzero write bulk status received: %d\n, __func__, status); - spin_lock(portdata-lock); + spin_lock_irqsave(portdata-lock, flags); --portdata-outstanding_urbs; - spin_unlock(portdata-lock); - spin_lock(intfdata-susp_lock); + spin_unlock_irqrestore(portdata-lock, flags); + spin_lock_irqsave(intfdata-susp_lock, flags); You are allowing an interrupt enabled window where previously it wasn't possible. Why notleave these 2 lines as is? It isn't a big deal because no lock is held when interrupt is enabled. Thanks, -- Ming Lei -- 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 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai ti...@suse.de wrote:
At Thu, 11 Jul 2013 17:08:30 +0400,
Sergei Shtylyov wrote:
On 11-07-2013 13:06, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Changelog doesn't match the patch.
Yep, but moreover...
Cc: Jaroslav Kysela pe...@perex.cz
Cc: Takashi Iwai ti...@suse.de
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei ming@canonical.com
---
sound/usb/usx2y/usbusx2yaudio.c |4
1 file changed, 4 insertions(+)
diff --git a/sound/usb/usx2y/usbusx2yaudio.c
b/sound/usb/usx2y/usbusx2yaudio.c
index 4967fe9..e2ee893 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev *usX2Y)
struct snd_usX2Y_substream *subs = usX2Y-subs[s];
if (subs) {
if (atomic_read(subs-state) = state_PRERUNNING) {
+ unsigned long flags;
+
+ local_irq_save(flags);
snd_pcm_stop(subs-pcm_substream,
SNDRV_PCM_STATE_XRUN);
+ local_irq_restore(flags);
}
... actually this snd_pcm_stop() call should have been covered by
snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
together with the change, i.e. wrapping with
snd_pcm_stream_lock_irqsave().
Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
out similar patch later, :-)
I'll prepare the patch for 3.11 independently from your patch series,
so please drop this one.
OK, thanks for dealing with that.
BTW, the word cleanup in the subject is inappropriate. This is
rather a fix together with the core change.
It is a cleanup since the patchset only addresses lock problem which
is caused by the tasklet conversion.
And, I wonder whether we can take a safer approach. When the caller
condition changed, we often introduced either a different ops
(e.g. ioctl case) or a flag for the new condition, at least during the
transition period.
Interrupt is't enabled until all current drivers are cleaned up, so it is really
safe, please see patch [2].
Last but not least, is a conversion to tasklet really preferred?
tasklet is rather an obsoleted infrastructure nowadays, and people
don't recommend to use it any longer, AFAIK.
We discussed the problem in the below link previously[1], Steven
and Thomas suggested to use threaded irq handler, but which
may degrade USB mass storage performance, so we have to
take tasklet now until we rewrite transport part of USB mass storage
driver.
Also the conversion[2] has avoided the tasklet spin lock problem
already.
[1], http://marc.info/?t=13707911921r=1w=2
[2], http://marc.info/?l=linux-usbm=137286326726326w=2
Thanks,
--
Ming Lei
--
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 45/50] sound: usb: usx2y: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 10:34 PM, Takashi Iwai ti...@suse.de wrote:
At Thu, 11 Jul 2013 22:13:35 +0800,
Ming Lei wrote:
On Thu, Jul 11, 2013 at 9:50 PM, Takashi Iwai ti...@suse.de wrote:
At Thu, 11 Jul 2013 17:08:30 +0400,
Sergei Shtylyov wrote:
On 11-07-2013 13:06, Ming Lei wrote:
Complete() will be run with interrupt enabled, so change to
spin_lock_irqsave().
Changelog doesn't match the patch.
Yep, but moreover...
Cc: Jaroslav Kysela pe...@perex.cz
Cc: Takashi Iwai ti...@suse.de
Cc: alsa-de...@alsa-project.org
Signed-off-by: Ming Lei ming@canonical.com
---
sound/usb/usx2y/usbusx2yaudio.c |4
1 file changed, 4 insertions(+)
diff --git a/sound/usb/usx2y/usbusx2yaudio.c
b/sound/usb/usx2y/usbusx2yaudio.c
index 4967fe9..e2ee893 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -273,7 +273,11 @@ static void usX2Y_clients_stop(struct usX2Ydev
*usX2Y)
struct snd_usX2Y_substream *subs = usX2Y-subs[s];
if (subs) {
if (atomic_read(subs-state) = state_PRERUNNING) {
+ unsigned long flags;
+
+ local_irq_save(flags);
snd_pcm_stop(subs-pcm_substream,
SNDRV_PCM_STATE_XRUN);
+ local_irq_restore(flags);
}
... actually this snd_pcm_stop() call should have been covered by
snd_pcm_stream_lock(). Maybe it'd be enough to have a single patch
together with the change, i.e. wrapping with
snd_pcm_stream_lock_irqsave().
Please use snd_pcm_stream_lock_irqsave() so that I can avoid sending
out similar patch later, :-)
I'll prepare the patch for 3.11 independently from your patch series,
so please drop this one.
OK, thanks for dealing with that.
BTW, the word cleanup in the subject is inappropriate. This is
rather a fix together with the core change.
It is a cleanup since the patchset only addresses lock problem which
is caused by the tasklet conversion.
Well, the conversion to irqsave() is needed for the future drop of
irq_save() in the caller, right? Then this isn't a cleanup but a
preparation for movement ahead.
Sounds more accurate, and I will change the title in next round, :-)
Thanks,
--
Ming Lei
--
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 03/50] USB: usblp: spin_lock in complete() cleanup
On Thu, Jul 11, 2013 at 11:33 PM, Pete Zaitcev zait...@kotori.zaitcev.us wrote: On Thu, 11 Jul 2013 17:05:26 +0800 Ming Lei ming@canonical.com wrote: Complete() will be run with interrupt enabled, so change to spin_lock_irqsave(). Cc: Pete Zaitcev zait...@redhat.com Signed-off-by: Ming Lei ming@canonical.com Signed-off-by: Pete Zaitcev zait...@kotori.zaitcev.us Thank you. Good luck with that, Ming. I think the spin_lock_irqsave thing should've been done back in days of uhci-hcd. But we always had some super annoying edge cases when we considered it previously. I see Alan Stern signed off the HCD changes at the beginning of July, so you're on the right track. BTW, perfect formatting in usblp.c, too bad for Sergei's heartburn ^_^ -- Pete P.S. Is it just me, or you forgot to fix usb-skeleton.c in your 50 patches? It is my script which ignored the top directory of drivers/usb, :-) and I will add it in the next round, thanks for pointing it out. Thanks, -- Ming Lei -- 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: Video corruption varies by system load
On Mon, Jul 8, 2013 at 9:39 AM, Devin Heitmueller dheitmuel...@kernellabs.com wrote: Hi Alan, Thanks for taking the time to provide feedback. I'm just noticing now that I left off the subject line, which all the more reason makes me thankful that you bothered to read an email with as uninteresting a subject line as is possible. :-) On Tue, Jul 2, 2013 at 11:21 AM, Alan Stern st...@rowland.harvard.edu wrote: That's weird. But it might not be the scheduler so much; it could be related more to the total CPU load. Sorry, I didn't mean to suggest the scheduler itself was at fault - just that the high context switching may be changing the timing in a way that exposes a missing spinlock in usb core or increases the likelihood that a call that is sleepable is taking the path of the context switch. The problem has been seen on both the stock EHCI driver (on x86) as well as the musb driver used on the TI Davinci platform (ARM). The transfer buffer itself is being allocated using usb_alloc_coherent(), and I've seen it when allocating with vmalloc() as well. Do you mean kmalloc()? Memory allocated with vmalloc() is generally not suitable for DMA mapping. Yeah, typo. Sorry. This feels like some sort of DMA or cache related issue, since the behavior of the URB completion handler itself appears completely consistent regardless of the system load. I'm seeing the issue on 3.10-rc6 all the way back to 2.6.31 (the earliest I can go on my Ubuntu box given some udev related dependencies). I've done plenty of work on USB drivers under Linux over the years, but haven't dug too much into the USB core. Anybody who has any suggestions on how to debug such a timing problem, such suggestions would be very welcomed. This is an interesting problem, but I don't think you'll get much insight from looking at the USB side of things. You could try asking the people in charge of the DMA- and cache-related parts of the kernel. I finally dug out my Beagle 480 USB, so I will get that hooked up this week, write a decoder to reassemble the video frames based on the USB trace, and know once and for all whether the device is delivering correct video or not. If the video being delivered by the device has no corruption, then we're talking about some sort of memory consistency or DMA issue (or perhaps some sort of problem with the USB core populating the finished URBs before calling the completion handler). If the video coming down the bus is corrupted, then we're probably talking about some sort of timing problem with the URB submission (combined with the FIFO on the chip poorly handling the incorrect timing). One suggestion: maybe it is better to verify if there is such problem on x86-ehci first because x86 is a memory coherent arch, which means there shouldn't have the DMA issue on x86 generally. Thanks, -- Ming Lei -- 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: inconsistent {IN-HARDIRQ-W} - {HARDIRQ-ON-W} usage with hcd_urb_list_lock
On Sun, Jul 7, 2013 at 4:48 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Sat, 6 Jul 2013, Maarten Lankhorst wrote:
I didn't even know I still had lockdep on.
The following lockdep splat happened when I plugged in a usb bluetooth
dongle, using
the pre-rc1 3.11 kernel at HEAD b2c311075db
=
[ INFO: inconsistent lock state ]
3.10.0+ #106 Not tainted
-
inconsistent {IN-HARDIRQ-W} - {HARDIRQ-ON-W} usage.
irq/42-xhci_hcd/97 [HC0[0]:SC0[2]:HE1:SE0] takes:
(hcd_urb_list_lock){?.}, at: [8149440e]
usb_hcd_unlink_urb_from_ep+0x28/0x4e
stack backtrace:
CPU: 1 PID: 97 Comm: irq/42-xhci_hcd Not tainted 3.10.0+ #106
Maarten, I am wondering you are running kernel from linus tree since
looks xhci threaded irq is enabled from the log.
If yes, please don't do that because USB host controller driver still
doesn't support threaded irq now.
Hardware name: Acer Aspire M3985/Aspire M3985, BIOS P01-A1 03/12/2012
8210c150 88040834da48 81691af4 0007
8804082e20b0 88040834daa8 8168cb10 0002
88040001 8804 8100f4f7 88040834dac4
Call Trace:
[81691af4] dump_stack+0x4f/0x84
[8168cb10] print_usage_bug+0x1f5/0x206
[8100f4f7] ? save_stack_trace+0x2f/0x50
[810af30c] mark_lock+0x276/0x2cf
[810ae8cc] ? check_usage_forwards+0x12f/0x12f
[810af925] __lock_acquire+0x5c0/0x1c2e
[810b1e04] ? mark_held_locks+0x6d/0x117
[8168e8d4] ? __slab_free+0x1c7/0x2ed
[810b1f5a] ? trace_hardirqs_on_caller+0xac/0x1bb
[810b2076] ? trace_hardirqs_on+0xd/0xf
[8149440e] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
[810b1556] lock_acquire+0x87/0x139
[8149440e] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
[81698a1a] _raw_spin_lock+0x3b/0x4a
[8149440e] ? usb_hcd_unlink_urb_from_ep+0x28/0x4e
[8149440e] usb_hcd_unlink_urb_from_ep+0x28/0x4e
[814bf55a] xhci_irq+0x5ac/0x143d
[81699171] ? _raw_spin_unlock_irq+0x3b/0x5d
[8108386d] ? finish_task_switch+0x7c/0x101
[81083830] ? finish_task_switch+0x3f/0x101
[81697060] ? __schedule+0x42a/0x885
[810d7fdb] ? irq_thread_fn+0x48/0x48
[814c03fc] xhci_msi_irq+0x11/0x15
It looks like xhci_msi_irq() needs to call local_irq_save() and
local_irq_restore().
Looks it isn't needed.
Thanks,
--
Ming Lei
--
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 v5 0/5] USB: HCD/EHCI: giveback of URB in tasklet context
Hi, The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler(IRQs disabled time) can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb(). The patchset enables the mechanism on EHCI HCD now. In the commit log of patch 5/5, detailed test result on three machines (ARM A9/A15 dual core, X86) are provided about transfer performance and ehci irq handling time. From the result, basically no transfer performance loss is found and ehci irq handling time drops much with the patchset. V5: - 4/5: don't remove one blank line V4: - 3/5: introduced to speedup periodic qh unlink in ehci_endpoint_disable - 3/5: at the same time, improve ehci_endpoint_disable - 4/5: unlink qh immediately if qh_completions() returns fault V3: - 1/4: don't check HCD_BH when initializing tasklet - 1/4: don't save flags for spin_lock_irq in tasklet - 1/4: use true/false for bool variable - 1/4: other trivial changes(patch style, vairable name, ...) - 3/4: reorganize code to make output of diff friendly - 3/4: don't cancel hrtimer to make change simple(mark it as TODO) - 4/4: add worst case test data on reading mass storage device as required by Oliver V2: - 1/4: always run URB complete() of root-hub in tasklet - 1/4: store urb status in urb-unlinked - 1/4: don't allocate 'struct giveback_urb_bh' dynamically - 1/4: other minor changes - 2/4: descript changes simply - 3/4: don't use QH_STATE_UNLINK_WAIT to implement intr qh unlink wait - 3/4: cancel unlink wait change - 4/4: merge HCD private lock changes - rebase on 3.10-rc7-next20130624 V1: - change percput tasklet into tasklet in HCD to avoid out of order of URB-complete() for same endpoint - disable local IRQs when calling complete() from tasklet to avoid deadlock which is caused by holding lock via spin_lock and the same lock might be acquired in hard irq context - document coming change about calling complete() with irq enabled so that we can start to clean up USB drivers which call spin_lock() in complete() Documentation/usb/URB.txt | 21 +++--- drivers/usb/core/hcd.c| 147 + drivers/usb/host/ehci-fsl.c |2 +- drivers/usb/host/ehci-grlib.c |2 +- drivers/usb/host/ehci-hcd.c | 19 ++--- drivers/usb/host/ehci-hub.c |1 + drivers/usb/host/ehci-mem.c |1 + drivers/usb/host/ehci-mv.c|2 +- drivers/usb/host/ehci-octeon.c|2 +- drivers/usb/host/ehci-pmcmsp.c|2 +- drivers/usb/host/ehci-ppc-of.c|2 +- drivers/usb/host/ehci-ps3.c |2 +- drivers/usb/host/ehci-q.c |5 -- drivers/usb/host/ehci-sched.c | 47 +++- drivers/usb/host/ehci-sead3.c |2 +- drivers/usb/host/ehci-sh.c|2 +- drivers/usb/host/ehci-tilegx.c|2 +- drivers/usb/host/ehci-timer.c | 34 - drivers/usb/host/ehci-w90x900.c |2 +- drivers/usb/host/ehci-xilinx-of.c |2 +- drivers/usb/host/ehci.h |3 + include/linux/usb/hcd.h | 17 + 22 files changed, 247 insertions(+), 72 deletions(-) Thanks, -- Ming Lei -- 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 v5 1/5] USB: HCD: support giveback of URB in tasklet context
This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.
Motivations:
1), on some arch(such as ARM), DMA mapping/unmapping is a bit
time-consuming, for example: when accessing usb mass storage
via EHCI on pandaboard, the common length of transfer buffer is 120KB,
the time consumed on DMA unmapping may reach hundreds of microseconds;
even on A15 based box, the time is still about scores of microseconds
2), on some arch, reading DMA coherent memoery is very time-consuming,
the most common example is usb video class driver[1]
3), driver's complete() callback may do much things which is driver
specific, so the time is consumed unnecessarily in hardware irq context.
4), running driver's complete() callback in hardware irq context causes
that host controller driver has to release its lock in interrupt handler,
so reacquiring the lock after return may busy wait a while and increase
interrupt handling time. More seriously, releasing the HCD lock makes
HCD becoming quite complicated to deal with introduced races.
So the patch proposes to run giveback of URB in tasklet context, then
time consumed in HCD irq handling doesn't depend on drivers' complete and
DMA mapping/unmapping any more, also we can simplify HCD since the HCD
lock isn't needed to be released during irq handling.
The patch should be reasonable and doable:
1), for drivers, they don't care if the complete() is called in hard irq
context or softirq context
2), the biggest change is the situation in which usb_submit_urb() is called
in complete() callback, so the introduced tasklet schedule delay might be a
con, but it shouldn't be a big deal:
- control/bulk asynchronous transfer isn't sensitive to schedule
delay
- the patch schedules giveback of periodic URBs using
tasklet_hi_schedule, so the introduced delay should be very
small
- for ISOC transfer, generally, drivers submit several URBs
concurrently to avoid interrupt delay, so it is OK with the
little schedule delay.
- for interrupt transfer, generally, drivers only submit one URB
at the same time, but interrupt transfer is often used in event
report, polling, ... situations, and a little delay should be OK.
Considered that HCDs may optimize on submitting URB in complete(), the
patch may cause the optimization not working, so introduces one flag to mark
if the HCD supports to run giveback URB in tasklet context. When all HCDs
are ready, the flag can be removed.
[1], http://marc.info/?t=136438111600010r=1w=2
Cc: Oliver Neukum oli...@neukum.org
Acked-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/hcd.c | 147 +--
include/linux/usb/hcd.h | 17 ++
2 files changed, 134 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..26471cd 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -694,15 +694,7 @@ error:
/* any errors get returned through the urb completion */
spin_lock_irq(hcd_root_hub_lock);
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- /* This peculiar use of spinlocks echoes what real HC drivers do.
-* Avoiding calls to local_irq_disable/enable makes the code
-* RT-friendly.
-*/
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
-
spin_unlock_irq(hcd_root_hub_lock);
return 0;
}
@@ -742,9 +734,7 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
memcpy(urb-transfer_buffer, buffer, length);
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, 0);
- spin_lock(hcd_root_hub_lock);
} else {
length = 0;
set_bit(HCD_FLAG_POLL_PENDING, hcd-flags);
@@ -834,10 +824,7 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
if (urb == hcd-status_urb) {
hcd-status_urb = NULL;
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
}
}
done:
@@ -1648,6 +1635,72 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
/*-*/
+static void __usb_hcd_giveback_urb(struct urb *urb)
+{
+ struct usb_hcd *hcd = bus_to_hcd(urb-dev-bus);
+ int status = urb
[PATCH v5 3/5] USB: EHCI: improve ehci_endpoint_disable
The patch does the below improvement:
- think QH_STATE_COMPLETING as unlinking state since all URBs on the
endpoint should be in unlinking or unlinked when doing endpoint_disable()
- add WARN_ON(!list_empty(qh-qtd_list)); if qh-qh_state is
QH_STATE_LINKED because there shouldn't be any active transfer in qh
- when qh-qh_state is QH_STATE_LINKED, the QH(async or periodic)
should be in its corresponding list, so the search through the async
list isn't necessary.
- unlink periodic QH to speed up unlinking if the QH is in linked
state
Basically, only the last one is related with this patchset because
the assumption of periodic qh self-unlinks on empty isn't true
any more when we introduce unlink-wait for periodic qh.
Acked-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7abf1ce..387cedf 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -942,7 +942,7 @@ ehci_endpoint_disable (struct usb_hcd *hcd, struct
usb_host_endpoint *ep)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
unsigned long flags;
- struct ehci_qh *qh, *tmp;
+ struct ehci_qh *qh;
/* ASSERT: any requests/urbs are being unlinked */
/* ASSERT: nobody can be submitting urbs for this any more */
@@ -972,17 +972,13 @@ rescan:
qh-qh_state = QH_STATE_IDLE;
switch (qh-qh_state) {
case QH_STATE_LINKED:
- case QH_STATE_COMPLETING:
- for (tmp = ehci-async-qh_next.qh;
- tmp tmp != qh;
- tmp = tmp-qh_next.qh)
- continue;
- /* periodic qh self-unlinks on empty, and a COMPLETING qh
-* may already be unlinked.
-*/
- if (tmp)
+ WARN_ON(!list_empty(qh-qtd_list));
+ if (usb_endpoint_type(ep-desc) != USB_ENDPOINT_XFER_INT)
start_unlink_async(ehci, qh);
+ else
+ start_unlink_intr(ehci, qh);
/* FALL THROUGH */
+ case QH_STATE_COMPLETING: /* already in unlinking */
case QH_STATE_UNLINK: /* wait for hw to finish? */
case QH_STATE_UNLINK_WAIT:
idle_timeout:
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] USB: URB documentation: claim complete() will be run with IRQs enabled
There is no good reason to run complete() in hard interrupt disabled context. After switch to run complete() in tasklet, we will enable local IRQs when calling complete() since we can do it at that time. Even though we still disable IRQs now when calling complete() in tasklet, the URB documentation is updated to claim complete() will be run in tasklet context and local IRQs will be enabled, so that USB drivers can know the change and avoid one deadlock caused by: assume IRQs disabled in complete() and call spin_lock() to hold lock which might be acquired in interrupt context. Current spin_lock() usages in drivers' complete() will be cleaned up at the same time, and once the cleanup is finished, local IRQs will be enabled when calling complete() in tasklet. Also fix description about type of usb_complete_t, and remove the advice of running completion handler in tasklet for decreasing system latency. Cc: Oliver Neukum oli...@neukum.org Acked-by: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- Documentation/usb/URB.txt | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt index 00d2c64..50da0d4 100644 --- a/Documentation/usb/URB.txt +++ b/Documentation/usb/URB.txt @@ -195,13 +195,12 @@ by the completion handler. The handler is of the following type: - typedef void (*usb_complete_t)(struct urb *, struct pt_regs *) + typedef void (*usb_complete_t)(struct urb *) -I.e., it gets the URB that caused the completion call, plus the -register values at the time of the corresponding interrupt (if any). -In the completion handler, you should have a look at urb-status to -detect any USB errors. Since the context parameter is included in the URB, -you can pass information to the completion handler. +I.e., it gets the URB that caused the completion call. In the completion +handler, you should have a look at urb-status to detect any USB errors. +Since the context parameter is included in the URB, you can pass +information to the completion handler. Note that even when an error (or unlink) is reported, data may have been transferred. That's because USB transfers are packetized; it might take @@ -210,12 +209,12 @@ have transferred successfully before the completion was called. NOTE: * WARNING * -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called -during hardware interrupt processing. If you can, defer substantial -work to a tasklet (bottom half) to keep system latencies low. You'll -probably need to use spinlocks to protect data structures you manipulate -in completion handlers. +NEVER SLEEP IN A COMPLETION HANDLER. These are often called in atomic +context. +In the current kernel, completion handlers run with local interrupts +disabled, but in the future this will be changed, so don't assume that +local IRQs are always disabled inside completion handlers. 1.8. How to do isochronous (ISO) transfers? -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/5] USB: EHCI: improve interrupt qh unlink
ehci-hcd currently unlinks an interrupt QH when it becomes empty, that
is, after its last URB completes. This works well because in almost
all cases, the completion handler for an interrupt URB resubmits the
URB; therefore the QH doesn't become empty and doesn't get unlinked.
When we start using tasklets for URB completion, this scheme won't work
as well. The resubmission won't occur until the tasklet runs, which
will be some time after the completion is queued with the tasklet.
During that delay, the QH will be empty and so will be unlinked
unnecessarily.
To prevent this problem, this patch adds a 5-ms time delay before empty
interrupt QHs are unlinked. Most often, during that time the interrupt
URB will be resubmitted and thus we can avoid unlinking the QH.
Signed-off-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c |1 +
drivers/usb/host/ehci-hub.c |1 +
drivers/usb/host/ehci-mem.c |1 +
drivers/usb/host/ehci-sched.c | 47 +++--
drivers/usb/host/ehci-timer.c | 34 -
drivers/usb/host/ehci.h |3 +++
6 files changed, 84 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 387cedf..7ad9ef8 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -487,6 +487,7 @@ static int ehci_init(struct usb_hcd *hcd)
ehci-periodic_size = DEFAULT_I_TDPS;
INIT_LIST_HEAD(ehci-async_unlink);
INIT_LIST_HEAD(ehci-async_idle);
+ INIT_LIST_HEAD(ehci-intr_unlink_wait);
INIT_LIST_HEAD(ehci-intr_unlink);
INIT_LIST_HEAD(ehci-intr_qh_list);
INIT_LIST_HEAD(ehci-cached_itd_list);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 2b70277..6037f84 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
end_unlink_async(ehci);
unlink_empty_async_suspended(ehci);
+ ehci_handle_start_intr_unlinks(ehci);
ehci_handle_intr_unlinks(ehci);
end_free_itds(ehci);
diff --git a/drivers/usb/host/ehci-mem.c b/drivers/usb/host/ehci-mem.c
index ef2c3a1..52a7773 100644
--- a/drivers/usb/host/ehci-mem.c
+++ b/drivers/usb/host/ehci-mem.c
@@ -93,6 +93,7 @@ static struct ehci_qh *ehci_qh_alloc (struct ehci_hcd *ehci,
gfp_t flags)
qh-qh_dma = dma;
// INIT_LIST_HEAD (qh-qh_list);
INIT_LIST_HEAD (qh-qtd_list);
+ INIT_LIST_HEAD(qh-unlink_node);
/* dummy td enables safe urb queuing */
qh-dummy = ehci_qtd_alloc (ehci, flags);
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..9438873 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,12 +601,29 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del(qh-intr_node);
}
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+ if (qh-qh_state != QH_STATE_LINKED ||
+ list_empty(qh-unlink_node))
+ return;
+
+ list_del_init(qh-unlink_node);
+
+ /*
+* TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
+* avoiding unnecessary CPU wakeup
+*/
+}
+
static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
/* If the QH isn't linked then there's nothing we can do. */
if (qh-qh_state != QH_STATE_LINKED)
return;
+ /* if the qh is waiting for unlink, cancel it now */
+ cancel_unlink_wait_intr(ehci, qh);
+
qh_unlink_periodic (ehci, qh);
/* Make sure the unlinks are visible before starting the timer */
@@ -632,6 +649,27 @@ static void start_unlink_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
}
}
+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void start_unlink_intr_wait(struct ehci_hcd *ehci,
+ struct ehci_qh *qh)
+{
+ qh-unlink_cycle = ehci-intr_unlink_wait_cycle;
+
+ /* New entries go at the end of the intr_unlink_wait list */
+ list_add_tail(qh-unlink_node, ehci-intr_unlink_wait);
+
+ if (ehci-rh_state EHCI_RH_RUNNING)
+ ehci_handle_start_intr_unlinks(ehci);
+ else if (ehci-intr_unlink_wait.next == qh-unlink_node) {
+ ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+ ++ehci-intr_unlink_wait_cycle;
+ }
+}
+
static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
struct ehci_qh_hw *hw = qh-hw;
@@ -889,6 +927,9 @@ static int intr_submit (
if (qh-qh_state == QH_STATE_IDLE) {
qh_refresh(ehci, qh
[PATCH v5 5/5] USB: EHCI: support running URB giveback in tasklet context
, max:140)
-
* On T410, sometimes read ehci status register in ehci_irq takes more
than 100us, and the problem has been reported on the link:
http://marc.info/?t=13706586731r=1w=2
Acked-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-fsl.c |2 +-
drivers/usb/host/ehci-grlib.c |2 +-
drivers/usb/host/ehci-hcd.c |2 +-
drivers/usb/host/ehci-mv.c|2 +-
drivers/usb/host/ehci-octeon.c|2 +-
drivers/usb/host/ehci-pmcmsp.c|2 +-
drivers/usb/host/ehci-ppc-of.c|2 +-
drivers/usb/host/ehci-ps3.c |2 +-
drivers/usb/host/ehci-q.c |5 -
drivers/usb/host/ehci-sead3.c |2 +-
drivers/usb/host/ehci-sh.c|2 +-
drivers/usb/host/ehci-tilegx.c|2 +-
drivers/usb/host/ehci-w90x900.c |2 +-
drivers/usb/host/ehci-xilinx-of.c |2 +-
14 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index bd831ec..330274a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -669,7 +669,7 @@ static const struct hc_driver ehci_fsl_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = HCD_USB2 | HCD_MEMORY,
+ .flags = HCD_USB2 | HCD_MEMORY | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-grlib.c b/drivers/usb/host/ehci-grlib.c
index a77bd8d..2905004 100644
--- a/drivers/usb/host/ehci-grlib.c
+++ b/drivers/usb/host/ehci-grlib.c
@@ -43,7 +43,7 @@ static const struct hc_driver ehci_grlib_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7ad9ef8..73c7299 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1166,7 +1166,7 @@ static const struct hc_driver ehci_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags =HCD_MEMORY | HCD_USB2,
+ .flags =HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 915c2db..ce18a36 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c
@@ -96,7 +96,7 @@ static const struct hc_driver mv_ehci_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-octeon.c b/drivers/usb/host/ehci-octeon.c
index 45cc001..ab0397e 100644
--- a/drivers/usb/host/ehci-octeon.c
+++ b/drivers/usb/host/ehci-octeon.c
@@ -51,7 +51,7 @@ static const struct hc_driver ehci_octeon_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-pmcmsp.c b/drivers/usb/host/ehci-pmcmsp.c
index 601e208..893b707 100644
--- a/drivers/usb/host/ehci-pmcmsp.c
+++ b/drivers/usb/host/ehci-pmcmsp.c
@@ -286,7 +286,7 @@ static const struct hc_driver ehci_msp_hc_driver = {
#else
.irq = ehci_irq,
#endif
- .flags =HCD_MEMORY | HCD_USB2,
+ .flags =HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ppc-of.c b/drivers/usb/host/ehci-ppc-of.c
index 86da09c..014d37b 100644
--- a/drivers/usb/host/ehci-ppc-of.c
+++ b/drivers/usb/host/ehci-ppc-of.c
@@ -28,7 +28,7 @@ static const struct hc_driver ehci_ppc_of_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
index fd98377..8188542 100644
--- a/drivers/usb/host/ehci-ps3.c
+++ b/drivers/usb/host/ehci-ps3.c
@@ -71,7 +71,7 @@ static const struct hc_driver ps3_ehci_hc_driver = {
.product_desc = PS3 EHCI Host Controller,
.hcd_priv_size = sizeof(struct ehci_hcd),
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY
Re: How should we handle isochronous underruns?
On Mon, Jul 1, 2013 at 10:48 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 1 Jul 2013, Ming Lei wrote: The fact that the delay is small doesn't matter -- what's important is that it will be 0 whereas now there is no delay between completion and resubmission (in fact, the resubmission occurs before the completion ends). Yes, that is the change tasklet is bringing, so looks we need to find a way to distinguish streaming-on from underrun when stream-td_list becomes empty in iso_stream_schedule(). I don't think the introduced tasklet delay is a problem for EHCI since per EHCI spec the maximum rate at which the host controller will issue interrupts is one microframe(125us), which means isochronous transfer completion can be reported to CPU with about 125us delay in hardware level. Irrelevant. Besides, the delay can be longer than 125 us if another interrupt is being handled. Thus, for example, even if the pipeline contains only a single URB, with the current code it will not become empty. But with the new code it will. If the load on the system is too high, the pipeline could empty out even if it normally contains two or more URBs. Single URB may not work well too when running complete() in hard irq context. It could work very well indeed, if the interval between URBs was larger than 1 ms. It is too fragile so that single isoc URB is seldom used, isn't it? Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/5] USB: EHCI: improve interrupt qh unlink
On Wed, Jul 3, 2013 at 1:46 AM, Alan Stern st...@rowland.harvard.edu wrote:
-
/* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
static void ehci_poll_ASS(struct ehci_hcd *ehci)
{
This blank line should remain intact. Apart from that, the patch is
okay.
Sorry for missing this, will fix it in v5.
BTW, I saw you commented that Patches 1, 2, 4 all look okay in v3
review, so could I add your Ack in other 3 patches in v5?
Thanks,
--
Ming Lei
--
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: How should we handle isochronous underruns?
On Sun, Jun 30, 2013 at 11:02 PM, Alan Stern st...@rowland.harvard.edu wrote: Naturally, under normal circumstances this won't matter, because underruns shouldn't occur. But I know from experience that people try to push the latency as far down as they can, which increases the likelihood of underruns. I understand the latency is effected by packet count in one URB, and it shouldn't be related with URB count, so looks we still can ease the problem by submitting more URBs concurrently, and at the same time make less packets per URB if guys care latency. Correct me if it is wrong... On Mon, Jul 1, 2013 at 7:36 PM, Clemens Ladisch clem...@ladisch.de wrote: The audio drivers uses at least two URBs. (The actual number and length of URBs depends on how the application configures its buffer.) Clemens, thanks for your input. Also, another important data about the problem is that how much time one isoc URB may span, which depends on the endpoint interval and packet number of URB. Thanks, -- Ming Lei -- 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: How should we handle isochronous underruns?
On Mon, Jul 1, 2013 at 9:06 PM, Clemens Ladisch clem...@ladisch.de wrote: Ming Lei wrote: On Sun, Jun 30, 2013 at 11:02 PM, Alan Stern st...@rowland.harvard.edu wrote: Naturally, under normal circumstances this won't matter, because underruns shouldn't occur. But I know from experience that people try to push the latency as far down as they can, which increases the likelihood of underruns. I understand the latency is effected by packet count in one URB, and it shouldn't be related with URB count, This is true only when capturing. For playback, the latency is the length of the entire pipeline. For playback, every URB submitted is added into hw table immediately, then the data will be played to speaker. I don't understand why the latency is the entire pipeline. If you submitted a bit more URBs, underruns shouldn't happen at all. Could you explain it in a bit detail? Thanks, -- Ming Lei -- 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: How should we handle isochronous underruns?
On Mon, Jul 1, 2013 at 9:27 PM, Clemens Ladisch clem...@ladisch.de wrote: Ming Lei wrote: On Mon, Jul 1, 2013 at 9:06 PM, Clemens Ladisch clem...@ladisch.de wrote: Ming Lei wrote: I understand the latency is effected by packet count in one URB, and it shouldn't be related with URB count, This is true only when capturing. For playback, the latency is the length of the entire pipeline. For playback, every URB submitted is added into hw table immediately, then the data will be played to speaker. I don't understand why the latency is the entire pipeline. A submitted packet will be transmitted only after all the other packets in the pipeline have been transmitted. Yes, that is always true since EHCI HW will send out data(packet) to device one by one at the scheduled frame/uframe according to the order of URB submitting , so the USB audio driver can submit URBs in advance, can't it? Also could you provide the typical time one URB in audio driver may span? Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Mon, Jul 1, 2013 at 10:44 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 1 Jul 2013, Ming Lei wrote: Currently, URB might be probably submitted to HCD too even after usb_hcd_flush_endpoint() completes since both accesses to dev-ep_in[epnum] and ep-enabled aren't protected by effective locks. The urb_list_lock in hcd.c serves to synchronize changes to ep-enabled. An URB might be submitted after usb_hcd_flush_endpoint() completes, but the submission will fail in usb_hcd_link_urb_to_ep(). But the lock isn't held when setting and clearing ep-enabled in usb_enable_endpoint and usb_disable_endpoint, is it? No. But it is held in usb_hcd_flush_endpoint() and usb_hcd_link_urb_to_ep(). Therefore, if a thread clears ep-enabled and then flushes the endpoint, further submissions will fail. I am wondering if holding the lock in usb_hcd_flush_endpoint() can avoid the race completely. Suppose usb_hcd_link_urb_to_ep() in submit path runs on CPU1 just when usb_hcd_flush_endpoint()(called from usb_disable_endpoint()) completes on CPU0, there is no any guarantee that CPU1 can see the up-to-date value of ep-enabled, so the urb might be submitted to HCD successfully. However, this does still leave one possible race: On the first submission to an isochronous endpoint, itd_submit() and sitd_submit() will allocate a new ehci_iso_stream before calling usb_hcd_link_urb_to_ep(). If the endpoint has been flushed and disabled, the enqueue will fail but the new iso_stream will not be destroyed -- it will leak. But this is an old failure mode, not related to the new changes. So how about not adding the WARN_ON(!list_empty(qh-qtd_list) now when qh-qh_state is QH_STATE_LINKED? I think we should add the WARN_ON. Or jump to the default case in that switch statement -- maybe the error message there should include a WARN_ON. How about just adding WARN_ON(!list_empty(qh-qtd_list) but continue unlinking the qh like current code, which should be harmless and avoid the QH leak? I think the end result would be the same. It wouldn't hurt and it wouldn't help. We'd still end up at the default case. It may help to avoid one QH leak, even though the problem is caused by usbcore, so I plan to do that if you have no objection. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Tue, Jul 2, 2013 at 12:02 AM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 1 Jul 2013, Ming Lei wrote: On Mon, Jul 1, 2013 at 10:44 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 1 Jul 2013, Ming Lei wrote: Currently, URB might be probably submitted to HCD too even after usb_hcd_flush_endpoint() completes since both accesses to dev-ep_in[epnum] and ep-enabled aren't protected by effective locks. The urb_list_lock in hcd.c serves to synchronize changes to ep-enabled. An URB might be submitted after usb_hcd_flush_endpoint() completes, but the submission will fail in usb_hcd_link_urb_to_ep(). But the lock isn't held when setting and clearing ep-enabled in usb_enable_endpoint and usb_disable_endpoint, is it? No. But it is held in usb_hcd_flush_endpoint() and usb_hcd_link_urb_to_ep(). Therefore, if a thread clears ep-enabled and then flushes the endpoint, further submissions will fail. I am wondering if holding the lock in usb_hcd_flush_endpoint() can avoid the race completely. Suppose usb_hcd_link_urb_to_ep() in submit path runs on CPU1 just when usb_hcd_flush_endpoint()(called from usb_disable_endpoint()) completes on CPU0, there is no any guarantee that CPU1 can see the up-to-date value of ep-enabled, so the urb might be submitted to HCD successfully. There is indeed such a guarantee, provided by the spinlock. CPU1 acquires urb_list_lock after CPU0 releases it, right? Therefore, all stores to memory performed by CPU0 before releasing the lock will be visible to CPU1 after it acquires the lock. Therefore CPU1 will see that ep-enabled is 0. OK, I got it, but looks it is a bit tricky and maybe some comment is helpful, thanks for your explanation. How about just adding WARN_ON(!list_empty(qh-qtd_list) but continue unlinking the qh like current code, which should be harmless and avoid the QH leak? I think the end result would be the same. It wouldn't hurt and it wouldn't help. We'd still end up at the default case. It may help to avoid one QH leak, even though the problem is caused by usbcore, so I plan to do that if you have no objection. I don't mind -- but I don't see how it will help. Alan Stern Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Mon, Jul 1, 2013 at 10:44 PM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 1 Jul 2013, Ming Lei wrote: However, this does still leave one possible race: On the first submission to an isochronous endpoint, itd_submit() and sitd_submit() will allocate a new ehci_iso_stream before calling usb_hcd_link_urb_to_ep(). If the endpoint has been flushed and disabled, the enqueue will fail but the new iso_stream will not be destroyed -- it will leak. But this is an old failure mode, not related to the new changes. Yes, not only the stream, but also ehci_iso_sched instance. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/5] USB: HCD/EHCI: giveback of URB in tasklet context
Hi, The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler(IRQs disabled time) can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb(). The patchset enables the mechanism on EHCI HCD now. In the commit log of patch 5/5, detailed test result on three machines (ARM A9/A15 dual core, X86) are provided about transfer performance and ehci irq handling time. From the result, basically no transfer performance loss is found and ehci irq handling time drops much with the patchset. V4: - 3/5: introduced to speedup periodic qh unlink in ehci_endpoint_disable - 3/5: at the same time, improve ehci_endpoint_disable - 4/5: unlink qh immediately if qh_completions() returns fault V3: - 1/4: don't check HCD_BH when initializing tasklet - 1/4: don't save flags for spin_lock_irq in tasklet - 1/4: use true/false for bool variable - 1/4: other trivial changes(patch style, vairable name, ...) - 3/4: reorganize code to make output of diff friendly - 3/4: don't cancel hrtimer to make change simple(mark it as TODO) - 4/4: add worst case test data on reading mass storage device as required by Oliver V2: - 1/4: always run URB complete() of root-hub in tasklet - 1/4: store urb status in urb-unlinked - 1/4: don't allocate 'struct giveback_urb_bh' dynamically - 1/4: other minor changes - 2/4: descript changes simply - 3/4: don't use QH_STATE_UNLINK_WAIT to implement intr qh unlink wait - 3/4: cancel unlink wait change - 4/4: merge HCD private lock changes - rebase on 3.10-rc7-next20130624 V1: - change percput tasklet into tasklet in HCD to avoid out of order of URB-complete() for same endpoint - disable local IRQs when calling complete() from tasklet to avoid deadlock which is caused by holding lock via spin_lock and the same lock might be acquired in hard irq context - document coming change about calling complete() with irq enabled so that we can start to clean up USB drivers which call spin_lock() in complete() Documentation/usb/URB.txt | 21 +++--- drivers/usb/core/hcd.c| 147 + drivers/usb/host/ehci-fsl.c |2 +- drivers/usb/host/ehci-grlib.c |2 +- drivers/usb/host/ehci-hcd.c | 19 ++--- drivers/usb/host/ehci-hub.c |1 + drivers/usb/host/ehci-mem.c |1 + drivers/usb/host/ehci-mv.c|2 +- drivers/usb/host/ehci-octeon.c|2 +- drivers/usb/host/ehci-pmcmsp.c|2 +- drivers/usb/host/ehci-ppc-of.c|2 +- drivers/usb/host/ehci-ps3.c |2 +- drivers/usb/host/ehci-q.c |5 -- drivers/usb/host/ehci-sched.c | 47 +++- drivers/usb/host/ehci-sead3.c |2 +- drivers/usb/host/ehci-sh.c|2 +- drivers/usb/host/ehci-tilegx.c|2 +- drivers/usb/host/ehci-timer.c | 35 - drivers/usb/host/ehci-w90x900.c |2 +- drivers/usb/host/ehci-xilinx-of.c |2 +- drivers/usb/host/ehci.h |3 + include/linux/usb/hcd.h | 17 + 22 files changed, 247 insertions(+), 73 deletions(-) Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 1/5] USB: HCD: support giveback of URB in tasklet context
This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.
Motivations:
1), on some arch(such as ARM), DMA mapping/unmapping is a bit
time-consuming, for example: when accessing usb mass storage
via EHCI on pandaboard, the common length of transfer buffer is 120KB,
the time consumed on DMA unmapping may reach hundreds of microseconds;
even on A15 based box, the time is still about scores of microseconds
2), on some arch, reading DMA coherent memoery is very time-consuming,
the most common example is usb video class driver[1]
3), driver's complete() callback may do much things which is driver
specific, so the time is consumed unnecessarily in hardware irq context.
4), running driver's complete() callback in hardware irq context causes
that host controller driver has to release its lock in interrupt handler,
so reacquiring the lock after return may busy wait a while and increase
interrupt handling time. More seriously, releasing the HCD lock makes
HCD becoming quite complicated to deal with introduced races.
So the patch proposes to run giveback of URB in tasklet context, then
time consumed in HCD irq handling doesn't depend on drivers' complete and
DMA mapping/unmapping any more, also we can simplify HCD since the HCD
lock isn't needed to be released during irq handling.
The patch should be reasonable and doable:
1), for drivers, they don't care if the complete() is called in hard irq
context or softirq context
2), the biggest change is the situation in which usb_submit_urb() is called
in complete() callback, so the introduced tasklet schedule delay might be a
con, but it shouldn't be a big deal:
- control/bulk asynchronous transfer isn't sensitive to schedule
delay
- the patch schedules giveback of periodic URBs using
tasklet_hi_schedule, so the introduced delay should be very
small
- for ISOC transfer, generally, drivers submit several URBs
concurrently to avoid interrupt delay, so it is OK with the
little schedule delay.
- for interrupt transfer, generally, drivers only submit one URB
at the same time, but interrupt transfer is often used in event
report, polling, ... situations, and a little delay should be OK.
Considered that HCDs may optimize on submitting URB in complete(), the
patch may cause the optimization not working, so introduces one flag to mark
if the HCD supports to run giveback URB in tasklet context. When all HCDs
are ready, the flag can be removed.
[1], http://marc.info/?t=136438111600010r=1w=2
Cc: Oliver Neukum oli...@neukum.org
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/hcd.c | 147 +--
include/linux/usb/hcd.h | 17 ++
2 files changed, 134 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..26471cd 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -694,15 +694,7 @@ error:
/* any errors get returned through the urb completion */
spin_lock_irq(hcd_root_hub_lock);
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- /* This peculiar use of spinlocks echoes what real HC drivers do.
-* Avoiding calls to local_irq_disable/enable makes the code
-* RT-friendly.
-*/
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
-
spin_unlock_irq(hcd_root_hub_lock);
return 0;
}
@@ -742,9 +734,7 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
memcpy(urb-transfer_buffer, buffer, length);
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, 0);
- spin_lock(hcd_root_hub_lock);
} else {
length = 0;
set_bit(HCD_FLAG_POLL_PENDING, hcd-flags);
@@ -834,10 +824,7 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
if (urb == hcd-status_urb) {
hcd-status_urb = NULL;
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
}
}
done:
@@ -1648,6 +1635,72 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
/*-*/
+static void __usb_hcd_giveback_urb(struct urb *urb)
+{
+ struct usb_hcd *hcd = bus_to_hcd(urb-dev-bus);
+ int status = urb-unlinked
[PATCH v4 2/5] USB: URB documentation: claim complete() will be run with IRQs enabled
There is no good reason to run complete() in hard interrupt disabled context. After switch to run complete() in tasklet, we will enable local IRQs when calling complete() since we can do it at that time. Even though we still disable IRQs now when calling complete() in tasklet, the URB documentation is updated to claim complete() will be run in tasklet context and local IRQs will be enabled, so that USB drivers can know the change and avoid one deadlock caused by: assume IRQs disabled in complete() and call spin_lock() to hold lock which might be acquired in interrupt context. Current spin_lock() usages in drivers' complete() will be cleaned up at the same time, and once the cleanup is finished, local IRQs will be enabled when calling complete() in tasklet. Also fix description about type of usb_complete_t, and remove the advice of running completion handler in tasklet for decreasing system latency. Cc: Oliver Neukum oli...@neukum.org Acked-by: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- Documentation/usb/URB.txt | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt index 00d2c64..50da0d4 100644 --- a/Documentation/usb/URB.txt +++ b/Documentation/usb/URB.txt @@ -195,13 +195,12 @@ by the completion handler. The handler is of the following type: - typedef void (*usb_complete_t)(struct urb *, struct pt_regs *) + typedef void (*usb_complete_t)(struct urb *) -I.e., it gets the URB that caused the completion call, plus the -register values at the time of the corresponding interrupt (if any). -In the completion handler, you should have a look at urb-status to -detect any USB errors. Since the context parameter is included in the URB, -you can pass information to the completion handler. +I.e., it gets the URB that caused the completion call. In the completion +handler, you should have a look at urb-status to detect any USB errors. +Since the context parameter is included in the URB, you can pass +information to the completion handler. Note that even when an error (or unlink) is reported, data may have been transferred. That's because USB transfers are packetized; it might take @@ -210,12 +209,12 @@ have transferred successfully before the completion was called. NOTE: * WARNING * -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called -during hardware interrupt processing. If you can, defer substantial -work to a tasklet (bottom half) to keep system latencies low. You'll -probably need to use spinlocks to protect data structures you manipulate -in completion handlers. +NEVER SLEEP IN A COMPLETION HANDLER. These are often called in atomic +context. +In the current kernel, completion handlers run with local interrupts +disabled, but in the future this will be changed, so don't assume that +local IRQs are always disabled inside completion handlers. 1.8. How to do isochronous (ISO) transfers? -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/5] USB: EHCI: improve ehci_endpoint_disable
The patch does the below improvement:
- think QH_STATE_COMPLETING as unlinking state since all URBs on the
endpoint should be in unlinking or unlinked when doing endpoint_disable()
- add WARN_ON(!list_empty(qh-qtd_list)); if qh-qh_state is
QH_STATE_LINKED because there shouldn't be any active transfer in qh
- when qh-qh_state is QH_STATE_LINKED, the QH(async or periodic)
should be in its corresponding list, so the search through the async
list isn't necessary.
- unlink periodic QH to speed up unlinking if the QH is in linked
state
Basically, only the last one is related with this patchset because
the assumption of periodic qh self-unlinks on empty isn't true
any more when we introduce unlink-wait for periodic qh.
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c | 16 ++--
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7abf1ce..387cedf 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -942,7 +942,7 @@ ehci_endpoint_disable (struct usb_hcd *hcd, struct
usb_host_endpoint *ep)
{
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
unsigned long flags;
- struct ehci_qh *qh, *tmp;
+ struct ehci_qh *qh;
/* ASSERT: any requests/urbs are being unlinked */
/* ASSERT: nobody can be submitting urbs for this any more */
@@ -972,17 +972,13 @@ rescan:
qh-qh_state = QH_STATE_IDLE;
switch (qh-qh_state) {
case QH_STATE_LINKED:
- case QH_STATE_COMPLETING:
- for (tmp = ehci-async-qh_next.qh;
- tmp tmp != qh;
- tmp = tmp-qh_next.qh)
- continue;
- /* periodic qh self-unlinks on empty, and a COMPLETING qh
-* may already be unlinked.
-*/
- if (tmp)
+ WARN_ON(!list_empty(qh-qtd_list));
+ if (usb_endpoint_type(ep-desc) != USB_ENDPOINT_XFER_INT)
start_unlink_async(ehci, qh);
+ else
+ start_unlink_intr(ehci, qh);
/* FALL THROUGH */
+ case QH_STATE_COMPLETING: /* already in unlinking */
case QH_STATE_UNLINK: /* wait for hw to finish? */
case QH_STATE_UNLINK_WAIT:
idle_timeout:
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 4/5] USB: EHCI: improve interrupt qh unlink
ehci-hcd currently unlinks an interrupt QH when it becomes empty, that
is, after its last URB completes. This works well because in almost
all cases, the completion handler for an interrupt URB resubmits the
URB; therefore the QH doesn't become empty and doesn't get unlinked.
When we start using tasklets for URB completion, this scheme won't work
as well. The resubmission won't occur until the tasklet runs, which
will be some time after the completion is queued with the tasklet.
During that delay, the QH will be empty and so will be unlinked
unnecessarily.
To prevent this problem, this patch adds a 5-ms time delay before empty
interrupt QHs are unlinked. Most often, during that time the interrupt
URB will be resubmitted and thus we can avoid unlinking the QH.
Signed-off-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c |1 +
drivers/usb/host/ehci-hub.c |1 +
drivers/usb/host/ehci-mem.c |1 +
drivers/usb/host/ehci-sched.c | 47 +++--
drivers/usb/host/ehci-timer.c | 35 --
drivers/usb/host/ehci.h |3 +++
6 files changed, 84 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 387cedf..7ad9ef8 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -487,6 +487,7 @@ static int ehci_init(struct usb_hcd *hcd)
ehci-periodic_size = DEFAULT_I_TDPS;
INIT_LIST_HEAD(ehci-async_unlink);
INIT_LIST_HEAD(ehci-async_idle);
+ INIT_LIST_HEAD(ehci-intr_unlink_wait);
INIT_LIST_HEAD(ehci-intr_unlink);
INIT_LIST_HEAD(ehci-intr_qh_list);
INIT_LIST_HEAD(ehci-cached_itd_list);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 2b70277..6037f84 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
end_unlink_async(ehci);
unlink_empty_async_suspended(ehci);
+ ehci_handle_start_intr_unlinks(ehci);
ehci_handle_intr_unlinks(ehci);
end_free_itds(ehci);
diff --git a/drivers/usb/host/ehci-mem.c b/drivers/usb/host/ehci-mem.c
index ef2c3a1..52a7773 100644
--- a/drivers/usb/host/ehci-mem.c
+++ b/drivers/usb/host/ehci-mem.c
@@ -93,6 +93,7 @@ static struct ehci_qh *ehci_qh_alloc (struct ehci_hcd *ehci,
gfp_t flags)
qh-qh_dma = dma;
// INIT_LIST_HEAD (qh-qh_list);
INIT_LIST_HEAD (qh-qtd_list);
+ INIT_LIST_HEAD(qh-unlink_node);
/* dummy td enables safe urb queuing */
qh-dummy = ehci_qtd_alloc (ehci, flags);
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..9438873 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,12 +601,29 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del(qh-intr_node);
}
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+ if (qh-qh_state != QH_STATE_LINKED ||
+ list_empty(qh-unlink_node))
+ return;
+
+ list_del_init(qh-unlink_node);
+
+ /*
+* TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
+* avoiding unnecessary CPU wakeup
+*/
+}
+
static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
/* If the QH isn't linked then there's nothing we can do. */
if (qh-qh_state != QH_STATE_LINKED)
return;
+ /* if the qh is waiting for unlink, cancel it now */
+ cancel_unlink_wait_intr(ehci, qh);
+
qh_unlink_periodic (ehci, qh);
/* Make sure the unlinks are visible before starting the timer */
@@ -632,6 +649,27 @@ static void start_unlink_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
}
}
+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void start_unlink_intr_wait(struct ehci_hcd *ehci,
+ struct ehci_qh *qh)
+{
+ qh-unlink_cycle = ehci-intr_unlink_wait_cycle;
+
+ /* New entries go at the end of the intr_unlink_wait list */
+ list_add_tail(qh-unlink_node, ehci-intr_unlink_wait);
+
+ if (ehci-rh_state EHCI_RH_RUNNING)
+ ehci_handle_start_intr_unlinks(ehci);
+ else if (ehci-intr_unlink_wait.next == qh-unlink_node) {
+ ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+ ++ehci-intr_unlink_wait_cycle;
+ }
+}
+
static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
struct ehci_qh_hw *hw = qh-hw;
@@ -889,6 +927,9 @@ static int intr_submit (
if (qh-qh_state == QH_STATE_IDLE) {
qh_refresh(ehci
[PATCH v4 5/5] USB: EHCI: support running URB giveback in tasklet context
, max:140)
-
* On T410, sometimes read ehci status register in ehci_irq takes more
than 100us, and the problem has been reported on the link:
http://marc.info/?t=13706586731r=1w=2
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-fsl.c |2 +-
drivers/usb/host/ehci-grlib.c |2 +-
drivers/usb/host/ehci-hcd.c |2 +-
drivers/usb/host/ehci-mv.c|2 +-
drivers/usb/host/ehci-octeon.c|2 +-
drivers/usb/host/ehci-pmcmsp.c|2 +-
drivers/usb/host/ehci-ppc-of.c|2 +-
drivers/usb/host/ehci-ps3.c |2 +-
drivers/usb/host/ehci-q.c |5 -
drivers/usb/host/ehci-sead3.c |2 +-
drivers/usb/host/ehci-sh.c|2 +-
drivers/usb/host/ehci-tilegx.c|2 +-
drivers/usb/host/ehci-w90x900.c |2 +-
drivers/usb/host/ehci-xilinx-of.c |2 +-
14 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index bd831ec..330274a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -669,7 +669,7 @@ static const struct hc_driver ehci_fsl_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = HCD_USB2 | HCD_MEMORY,
+ .flags = HCD_USB2 | HCD_MEMORY | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-grlib.c b/drivers/usb/host/ehci-grlib.c
index a77bd8d..2905004 100644
--- a/drivers/usb/host/ehci-grlib.c
+++ b/drivers/usb/host/ehci-grlib.c
@@ -43,7 +43,7 @@ static const struct hc_driver ehci_grlib_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7ad9ef8..73c7299 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1166,7 +1166,7 @@ static const struct hc_driver ehci_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags =HCD_MEMORY | HCD_USB2,
+ .flags =HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 915c2db..ce18a36 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c
@@ -96,7 +96,7 @@ static const struct hc_driver mv_ehci_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-octeon.c b/drivers/usb/host/ehci-octeon.c
index 45cc001..ab0397e 100644
--- a/drivers/usb/host/ehci-octeon.c
+++ b/drivers/usb/host/ehci-octeon.c
@@ -51,7 +51,7 @@ static const struct hc_driver ehci_octeon_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-pmcmsp.c b/drivers/usb/host/ehci-pmcmsp.c
index 601e208..893b707 100644
--- a/drivers/usb/host/ehci-pmcmsp.c
+++ b/drivers/usb/host/ehci-pmcmsp.c
@@ -286,7 +286,7 @@ static const struct hc_driver ehci_msp_hc_driver = {
#else
.irq = ehci_irq,
#endif
- .flags =HCD_MEMORY | HCD_USB2,
+ .flags =HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ppc-of.c b/drivers/usb/host/ehci-ppc-of.c
index 86da09c..014d37b 100644
--- a/drivers/usb/host/ehci-ppc-of.c
+++ b/drivers/usb/host/ehci-ppc-of.c
@@ -28,7 +28,7 @@ static const struct hc_driver ehci_ppc_of_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
index fd98377..8188542 100644
--- a/drivers/usb/host/ehci-ps3.c
+++ b/drivers/usb/host/ehci-ps3.c
@@ -71,7 +71,7 @@ static const struct hc_driver ps3_ehci_hc_driver = {
.product_desc = PS3 EHCI Host Controller,
.hcd_priv_size = sizeof(struct ehci_hcd),
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Sun, Jun 30, 2013 at 2:35 AM, Alan Stern st...@rowland.harvard.edu wrote: On Sat, 29 Jun 2013, Ming Lei wrote: The ehci_endpoint_disable() routine can be improved. In the QH_STATE_LINNKED or QH_STATE_COMPLETING case, we now need to handle interrupt QHs -- the comment about periodic qh self-unlinks on empty isn't entirely correct any more, because now the unlink doesn't occur until the QH has been empty for 5 ms. Actually, almost all interrupt URBs are resubmitted in its completion handler, that means they are basically stopped by unlinking before disabling endpoint, so I am wondering if we need to consider improving ehci_endpoint_disable() on interrupt endpoint. Come to think of it, we never should see an endpoint being disabled while there are active URBs. It might be a better idea to put a Right. WARN_ON there and simply return, without unlinking anything. If this ever triggers, it means there's a bug in usbcore. But that isn't always true, see below case: - the last URB in the endpoint completes without resubmitting in its completion handler - then the corresponding qh is kept in linked state, but wait for being unlinked - usb_disable_endpoint() is called before the qh is put into unlinked state, and no URBs are unlinked in usb_hcd_flush_endpoint() - ehci_endpoint_disable() still see qh in QH_STATE_LINKED state. So I think we should unlink here to speed up the procedure as suggested in your previous email. But I suggest to do the above on in another patch because most of the change is not much related with this patch. Okay. @@ -921,7 +966,7 @@ static void scan_intr(struct ehci_hcd *ehci) temp = qh_completions(ehci, qh); if (unlikely(temp || (list_empty(qh-qtd_list) qh-qh_state == QH_STATE_LINKED))) - start_unlink_intr(ehci, qh); + start_unlink_intr_wait(ehci, qh); This is not quite right. If temp is nonzero then we want to unlink the QH right away (because a fault occurred), so we should call It might depend on the fault type, looks we need to unlink qh immediately on SHUTDOWN and qh-dequeue_during_giveback, and Yes. for other non-unlink faults, drivers may not treat it as fault and continue to resubmit URB, such as hub_irq(). No. All faults have to cause the QH to be unlinked. That's another invariant of the driver. This is partly related to the silicon quirk present in some controllers (they perform the overlay even when the Halt bit is set in the qTD). In any case, whenever the QH is halted, the only safe way to recover is to unlink it and then relink. OK, got it, so we have to unlink the QH here as before to handle the fault. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Sun, Jun 30, 2013 at 10:05 PM, Alan Stern st...@rowland.harvard.edu wrote: On Sun, 30 Jun 2013, Ming Lei wrote: Come to think of it, we never should see an endpoint being disabled while there are active URBs. It might be a better idea to put a Right. WARN_ON there and simply return, without unlinking anything. If this ever triggers, it means there's a bug in usbcore. But that isn't always true, see below case: - the last URB in the endpoint completes without resubmitting in its completion handler - then the corresponding qh is kept in linked state, but wait for being unlinked - usb_disable_endpoint() is called before the qh is put into unlinked state, and no URBs are unlinked in usb_hcd_flush_endpoint() - ehci_endpoint_disable() still see qh in QH_STATE_LINKED state. So I think we should unlink here to speed up the procedure as suggested in your previous email. You are right. If the QH's qtd_list isn't empty then we should WARN_ON and return without doing anything -- just leak the QH. Otherwise, Yes, we can add the WARN_ON() because caller should have unlinked all requests, but looks we can handle the unlink here too without obvious side-effect. Currently, URB might be probably submitted to HCD too even after usb_hcd_flush_endpoint() completes since both accesses to dev-ep_in[epnum] and ep-enabled aren't protected by effective locks. So how about not adding the WARN_ON(!list_empty(qh-qtd_list) now when qh-qh_state is QH_STATE_LINKED? Also I am not sure if we should return without doing anything under the situation, at least when the QH is in IDLE and QH's qtd_list isn't empty, ep-hcpriv is cleared, then return. start the unlink immediately. (By the way, in your suggested patch, I would not define the eptype variable. Simply calculate the value where it is used. Also, you removed all usages of tmp, so the declaration should be removed as well.) OK, I will follow your suggest. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Mon, Jul 1, 2013 at 1:35 AM, Alan Stern st...@rowland.harvard.edu wrote: On Mon, 1 Jul 2013, Ming Lei wrote: So I think we should unlink here to speed up the procedure as suggested in your previous email. You are right. If the QH's qtd_list isn't empty then we should WARN_ON and return without doing anything -- just leak the QH. Otherwise, Yes, we can add the WARN_ON() because caller should have unlinked all requests, but looks we can handle the unlink here too without obvious side-effect. Currently, URB might be probably submitted to HCD too even after usb_hcd_flush_endpoint() completes since both accesses to dev-ep_in[epnum] and ep-enabled aren't protected by effective locks. The urb_list_lock in hcd.c serves to synchronize changes to ep-enabled. An URB might be submitted after usb_hcd_flush_endpoint() completes, but the submission will fail in usb_hcd_link_urb_to_ep(). But the lock isn't held when setting and clearing ep-enabled in usb_enable_endpoint and usb_disable_endpoint, is it? Also looks there is the similar problem about 'dev-can_submit' usage too. So how about not adding the WARN_ON(!list_empty(qh-qtd_list) now when qh-qh_state is QH_STATE_LINKED? I think we should add the WARN_ON. Or jump to the default case in that switch statement -- maybe the error message there should include a WARN_ON. How about just adding WARN_ON(!list_empty(qh-qtd_list) but continue unlinking the qh like current code, which should be harmless and avoid the QH leak? Also I am not sure if we should return without doing anything under the situation, at least when the QH is in IDLE and QH's qtd_list isn't empty, ep-hcpriv is cleared, then return. The only way for a QH to be in the IDLE state with a non-empty qtd_list is if qh-clearing_tt is set. The code already checks for that. OK. Thanks, -- Ming Lei -- 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: How should we handle isochronous underruns?
On Sun, Jun 30, 2013 at 11:02 PM, Alan Stern st...@rowland.harvard.edu wrote: Clement and Laurent: The two of you seem to be the people who make the most use of isochronous USB transfers. Since the ehci-hcd driver is being changed to use a tasklet for URB completion callbacks, it looks like I will need to reconsider how isochronous underruns get handled. Without using tasklet, the hard interrupt handler still can be delayed for some time, and switching to tasklet doesn't change the fact of the probable delay of URB handling. The basic prolem is simple enough: What should the HCD do when an URB is submitted after an isochronous pipeline has emptied out? I think some time-related data should be helpful for the discussion, for example, how long the isochronous pipeline may become empty in current audio/video driver(application) without any URB resubmit during the period? The problem will be more acute than in the past, because URB resubmissions will no longer be synchronous with URB completions, thanks to the tasklet. That is, in the current code, if the completion handler resubmits the URB, the resubmission occurs before the HCD finishes the completion callback. But in the new code, the HCD will simply hand the URB over to the tasklet, and the resubmission won't occur until some time later (when the tasklet wakes up and invokes the completion handler). As far as the HCD is concerned, the completion The delay should be very small(generally several microseconds) since isochronous URBs are completed in high priority tasklet. I don't think the introduced tasklet delay is a problem for EHCI since per EHCI spec the maximum rate at which the host controller will issue interrupts is one microframe(125us), which means isochronous transfer completion can be reported to CPU with about 125us delay in hardware level. will already be finished. Thus, for example, even if the pipeline contains only a single URB, with the current code it will not become empty. But with the new code it will. If the load on the system is too high, the pipeline could empty out even if it normally contains two or more URBs. Single URB may not work well too when running complete() in hard irq context. In UVC driver, looks it may submit at most 5 URBs which can include max 32 packets, so it will take about 5*32*125us(20ms) to make isoc pipeline empty suppose the interval is 1uF. But I don't know how USB audio driver uses URBs, and could it submit only one isoc URB to playback/record audio data? This means that the HCD will have trouble telling the difference between an underrun and a normal restart of a stopped I/O stream. In both cases it will see an URB being submitted to an empty queue. IMO, we should try to avoid the situation, and in UVC cases looks it is impossible to see an URB being submitted to an empty queue except for at the moment of starting streaming or interrupt disabled for extremely long. Here's where the URB_ISO_ASAP flag will be useful; if that flag is set then the URB is restarting a stopped stream, but if it isn't set then the pipeline experienced an underrun. Naturally, under normal circumstances this won't matter, because underruns shouldn't occur. But I know from experience that people try to push the latency as far down as they can, which increases the likelihood of underruns. There are several possible things the HCD could do when an underrun occurs: It could schedule the URB for the first time slot known to be available, even if that means skipping some time slots which the hardware might (or might not) be able to use. It could try to schedule the URB for the next time slot after the last one used by the preceding URB, even if that time slot has already expired. Something in between... What would be best for your purposes? Or do you have any different suggestions? 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 Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
On Sat, Jun 29, 2013 at 1:36 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Fri, 28 Jun 2013, Ming Lei wrote:
ehci-hcd currently unlinks an interrupt QH when it becomes empty, that
is, after its last URB completes. This works well because in almost
all cases, the completion handler for an interrupt URB resubmits the
URB; therefore the QH doesn't become empty and doesn't get unlinked.
When we start using tasklets for URB completion, this scheme won't work
as well. The resubmission won't occur until the tasklet runs, which
will be some time after the completion is queued with the tasklet.
During that delay, the QH will be empty and so will be unlinked
unnecessarily.
To prevent this problem, this patch adds a 5-ms time delay before empty
interrupt QHs are unlinked. Most often, during that time the interrupt
URB will be resubmitted and thus we can avoid unlinking the QH.
There a few, relatively minor issues...
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..e2fccc0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -484,6 +484,7 @@ static int ehci_init(struct usb_hcd *hcd)
ehci-periodic_size = DEFAULT_I_TDPS;
INIT_LIST_HEAD(ehci-async_unlink);
INIT_LIST_HEAD(ehci-async_idle);
+ INIT_LIST_HEAD(ehci-intr_unlink_wait);
INIT_LIST_HEAD(ehci-intr_unlink);
INIT_LIST_HEAD(ehci-intr_qh_list);
INIT_LIST_HEAD(ehci-cached_itd_list);
The ehci_endpoint_disable() routine can be improved. In the
QH_STATE_LINNKED or QH_STATE_COMPLETING case, we now need to handle
interrupt QHs -- the comment about periodic qh self-unlinks on empty
isn't entirely correct any more, because now the unlink doesn't occur
until the QH has been empty for 5 ms.
Actually, almost all interrupt URBs are resubmitted in its completion handler,
that means they are basically stopped by unlinking before disabling endpoint,
so I am wondering if we need to consider improving ehci_endpoint_disable()
on interrupt endpoint.
To start with, I think the QH_STATE_COMPLETING case label can be moved
to the QH_STATE_UNLINK section. That case should never happen anyway;
endpoints aren't supposed to be disabled while they are in use.
Yes, since all URBs on the endpoint should be in unlinking or unlinked when
doing endpoint_disable().
Next, the search through the async list can be removed. If an async QH
is in the LINKED state then it must be on the async list. Likewise, if
an intr QH is in the LINKED state then it must be on the intr_qh_list.
Right.
But I suggest to do the above on in another patch because most of the
change is not much related with this patch.
So all we have to do is figure out whether the QH is async or intr. If
it is async, call start_unlink_async(), otherwise call
start_unlink_intr(). We shouldn't have to wait 5 ms for an interrupt
QH to be unlinked.
OK, how about the below change?
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index fe27038..0cdaf7f 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -944,6 +944,7 @@ ehci_endpoint_disable (struct usb_hcd *hcd, struct
usb_host_endpoint *ep)
struct ehci_hcd *ehci = hcd_to_ehci (hcd);
unsigned long flags;
struct ehci_qh *qh, *tmp;
+ int eptype = usb_endpoint_type(ep-desc);
/* ASSERT: any requests/urbs are being unlinked */
/* ASSERT: nobody can be submitting urbs for this any more */
@@ -973,17 +974,12 @@ rescan:
qh-qh_state = QH_STATE_IDLE;
switch (qh-qh_state) {
case QH_STATE_LINKED:
- case QH_STATE_COMPLETING:
- for (tmp = ehci-async-qh_next.qh;
- tmp tmp != qh;
- tmp = tmp-qh_next.qh)
- continue;
- /* periodic qh self-unlinks on empty, and a COMPLETING qh
-* may already be unlinked.
-*/
- if (tmp)
+ if (eptype != USB_ENDPOINT_XFER_INT)
start_unlink_async(ehci, qh);
+ else
+ start_unlink_intr(ehci, qh);
/* FALL THROUGH */
+ case QH_STATE_COMPLETING: /* already in unlinking */
case QH_STATE_UNLINK: /* wait for hw to finish? */
case QH_STATE_UNLINK_WAIT:
idle_timeout:
@@ -632,6 +649,31 @@ static void start_unlink_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
}
}
+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void start_unlink_intr_wait(struct ehci_hcd *ehci,
+struct ehci_qh *qh)
+{
+ /* If the QH isn't linked then there's nothing we can do. */
+ if (qh-qh_state
[PATCH v3 3/4] USB: EHCI: improve interrupt qh unlink
ehci-hcd currently unlinks an interrupt QH when it becomes empty, that
is, after its last URB completes. This works well because in almost
all cases, the completion handler for an interrupt URB resubmits the
URB; therefore the QH doesn't become empty and doesn't get unlinked.
When we start using tasklets for URB completion, this scheme won't work
as well. The resubmission won't occur until the tasklet runs, which
will be some time after the completion is queued with the tasklet.
During that delay, the QH will be empty and so will be unlinked
unnecessarily.
To prevent this problem, this patch adds a 5-ms time delay before empty
interrupt QHs are unlinked. Most often, during that time the interrupt
URB will be resubmitted and thus we can avoid unlinking the QH.
Signed-off-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c |1 +
drivers/usb/host/ehci-hub.c |1 +
drivers/usb/host/ehci-mem.c |1 +
drivers/usb/host/ehci-sched.c | 47 -
drivers/usb/host/ehci-timer.c | 35 --
drivers/usb/host/ehci.h |3 +++
6 files changed, 85 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 246e124..e2fccc0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -484,6 +484,7 @@ static int ehci_init(struct usb_hcd *hcd)
ehci-periodic_size = DEFAULT_I_TDPS;
INIT_LIST_HEAD(ehci-async_unlink);
INIT_LIST_HEAD(ehci-async_idle);
+ INIT_LIST_HEAD(ehci-intr_unlink_wait);
INIT_LIST_HEAD(ehci-intr_unlink);
INIT_LIST_HEAD(ehci-intr_qh_list);
INIT_LIST_HEAD(ehci-cached_itd_list);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index b2f6450..4104c66 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
end_unlink_async(ehci);
unlink_empty_async_suspended(ehci);
+ ehci_handle_start_intr_unlinks(ehci);
ehci_handle_intr_unlinks(ehci);
end_free_itds(ehci);
diff --git a/drivers/usb/host/ehci-mem.c b/drivers/usb/host/ehci-mem.c
index ef2c3a1..52a7773 100644
--- a/drivers/usb/host/ehci-mem.c
+++ b/drivers/usb/host/ehci-mem.c
@@ -93,6 +93,7 @@ static struct ehci_qh *ehci_qh_alloc (struct ehci_hcd *ehci,
gfp_t flags)
qh-qh_dma = dma;
// INIT_LIST_HEAD (qh-qh_list);
INIT_LIST_HEAD (qh-qtd_list);
+ INIT_LIST_HEAD(qh-unlink_node);
/* dummy td enables safe urb queuing */
qh-dummy = ehci_qtd_alloc (ehci, flags);
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index acff5b8..101aea6 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,12 +601,29 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del(qh-intr_node);
}
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+ if (qh-qh_state != QH_STATE_LINKED ||
+ list_empty(qh-unlink_node))
+ return;
+
+ list_del_init(qh-unlink_node);
+
+ /*
+* TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
+* avoiding unnecessary CPU wakeup
+*/
+}
+
static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
/* If the QH isn't linked then there's nothing we can do. */
if (qh-qh_state != QH_STATE_LINKED)
return;
+ /* if the qh is waiting for unlink, cancel it now */
+ cancel_unlink_wait_intr(ehci, qh);
+
qh_unlink_periodic (ehci, qh);
/* Make sure the unlinks are visible before starting the timer */
@@ -632,6 +649,31 @@ static void start_unlink_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
}
}
+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void start_unlink_intr_wait(struct ehci_hcd *ehci,
+ struct ehci_qh *qh)
+{
+ /* If the QH isn't linked then there's nothing we can do. */
+ if (qh-qh_state != QH_STATE_LINKED)
+ return;
+
+ qh-unlink_cycle = ehci-intr_unlink_wait_cycle;
+
+ /* New entries go at the end of the intr_unlink_wait list */
+ list_add_tail(qh-unlink_node, ehci-intr_unlink_wait);
+
+ if (ehci-rh_state EHCI_RH_RUNNING)
+ ehci_handle_start_intr_unlinks(ehci);
+ else if (ehci-intr_unlink_wait.next == qh-unlink_node) {
+ ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+ ++ehci-intr_unlink_wait_cycle;
+ }
+}
+
static void end_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
struct ehci_qh_hw
[PATCH v3 1/4] USB: HCD: support giveback of URB in tasklet context
This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.
Motivations:
1), on some arch(such as ARM), DMA mapping/unmapping is a bit
time-consuming, for example: when accessing usb mass storage
via EHCI on pandaboard, the common length of transfer buffer is 120KB,
the time consumed on DMA unmapping may reach hundreds of microseconds;
even on A15 based box, the time is still about scores of microseconds
2), on some arch, reading DMA coherent memoery is very time-consuming,
the most common example is usb video class driver[1]
3), driver's complete() callback may do much things which is driver
specific, so the time is consumed unnecessarily in hardware irq context.
4), running driver's complete() callback in hardware irq context causes
that host controller driver has to release its lock in interrupt handler,
so reacquiring the lock after return may busy wait a while and increase
interrupt handling time. More seriously, releasing the HCD lock makes
HCD becoming quite complicated to deal with introduced races.
So the patch proposes to run giveback of URB in tasklet context, then
time consumed in HCD irq handling doesn't depend on drivers' complete and
DMA mapping/unmapping any more, also we can simplify HCD since the HCD
lock isn't needed to be released during irq handling.
The patch should be reasonable and doable:
1), for drivers, they don't care if the complete() is called in hard irq
context or softirq context
2), the biggest change is the situation in which usb_submit_urb() is called
in complete() callback, so the introduced tasklet schedule delay might be a
con, but it shouldn't be a big deal:
- control/bulk asynchronous transfer isn't sensitive to schedule
delay
- the patch schedules giveback of periodic URBs using
tasklet_hi_schedule, so the introduced delay should be very
small
- for ISOC transfer, generally, drivers submit several URBs
concurrently to avoid interrupt delay, so it is OK with the
little schedule delay.
- for interrupt transfer, generally, drivers only submit one URB
at the same time, but interrupt transfer is often used in event
report, polling, ... situations, and a little delay should be OK.
Considered that HCDs may optimize on submitting URB in complete(), the
patch may cause the optimization not working, so introduces one flag to mark
if the HCD supports to run giveback URB in tasklet context. When all HCDs
are ready, the flag can be removed.
[1], http://marc.info/?t=136438111600010r=1w=2
Cc: Oliver Neukum oli...@neukum.org
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/hcd.c | 147 +--
include/linux/usb/hcd.h | 17 ++
2 files changed, 134 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d53547d..b851c88 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -669,15 +669,7 @@ error:
/* any errors get returned through the urb completion */
spin_lock_irq(hcd_root_hub_lock);
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- /* This peculiar use of spinlocks echoes what real HC drivers do.
-* Avoiding calls to local_irq_disable/enable makes the code
-* RT-friendly.
-*/
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
-
spin_unlock_irq(hcd_root_hub_lock);
return 0;
}
@@ -717,9 +709,7 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
memcpy(urb-transfer_buffer, buffer, length);
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, 0);
- spin_lock(hcd_root_hub_lock);
} else {
length = 0;
set_bit(HCD_FLAG_POLL_PENDING, hcd-flags);
@@ -809,10 +799,7 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
if (urb == hcd-status_urb) {
hcd-status_urb = NULL;
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
}
}
done:
@@ -1623,6 +1610,72 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
/*-*/
+static void __usb_hcd_giveback_urb(struct urb *urb)
+{
+ struct usb_hcd *hcd = bus_to_hcd(urb-dev-bus);
+ int status = urb-unlinked
[PATCH v3 2/4] USB: URB documentation: claim complete() will be run with IRQs enabled
There is no good reason to run complete() in hard interrupt disabled context. After switch to run complete() in tasklet, we will enable local IRQs when calling complete() since we can do it at that time. Even though we still disable IRQs now when calling complete() in tasklet, the URB documentation is updated to claim complete() will be run in tasklet context and local IRQs will be enabled, so that USB drivers can know the change and avoid one deadlock caused by: assume IRQs disabled in complete() and call spin_lock() to hold lock which might be acquired in interrupt context. Current spin_lock() usages in drivers' complete() will be cleaned up at the same time, and once the cleanup is finished, local IRQs will be enabled when calling complete() in tasklet. Also fix description about type of usb_complete_t, and remove the advice of running completion handler in tasklet for decreasing system latency. Cc: Oliver Neukum oli...@neukum.org Acked-by: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- Documentation/usb/URB.txt | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt index 00d2c64..50da0d4 100644 --- a/Documentation/usb/URB.txt +++ b/Documentation/usb/URB.txt @@ -195,13 +195,12 @@ by the completion handler. The handler is of the following type: - typedef void (*usb_complete_t)(struct urb *, struct pt_regs *) + typedef void (*usb_complete_t)(struct urb *) -I.e., it gets the URB that caused the completion call, plus the -register values at the time of the corresponding interrupt (if any). -In the completion handler, you should have a look at urb-status to -detect any USB errors. Since the context parameter is included in the URB, -you can pass information to the completion handler. +I.e., it gets the URB that caused the completion call. In the completion +handler, you should have a look at urb-status to detect any USB errors. +Since the context parameter is included in the URB, you can pass +information to the completion handler. Note that even when an error (or unlink) is reported, data may have been transferred. That's because USB transfers are packetized; it might take @@ -210,12 +209,12 @@ have transferred successfully before the completion was called. NOTE: * WARNING * -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called -during hardware interrupt processing. If you can, defer substantial -work to a tasklet (bottom half) to keep system latencies low. You'll -probably need to use spinlocks to protect data structures you manipulate -in completion handlers. +NEVER SLEEP IN A COMPLETION HANDLER. These are often called in atomic +context. +In the current kernel, completion handlers run with local interrupts +disabled, but in the future this will be changed, so don't assume that +local IRQs are always disabled inside completion handlers. 1.8. How to do isochronous (ISO) transfers? -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler(IRQs disabled time) can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb(). The patchset enables the mechanism on EHCI HCD now. In the commit log of patch 4/4, detailed test result on three machines (ARM A9/A15 dual core, X86) are provided about transfer performance and ehci irq handling time. From the result, basically no transfer performance loss is found and ehci irq handling time drops much with the patchset. V3: - 1/4: don't check HCD_BH when initializing tasklet - 1/4: don't save flags for spin_lock_irq in tasklet - 1/4: use true/false for bool variable - 1/4: other trivial changes(patch style, vairable name, ...) - 3/4: reorganize code to make output of diff friendly - 3/4: don't cancel hrtimer to make change simple(mark it as TODO) - 4/4: add worst case test data on reading mass storage device as required by Oliver V2: - 1/4: always run URB complete() of root-hub in tasklet - 1/4: store urb status in urb-unlinked - 1/4: don't allocate 'struct giveback_urb_bh' dynamically - 1/4: other minor changes - 2/4: descript changes simply - 3/4: don't use QH_STATE_UNLINK_WAIT to implement intr qh unlink wait - 3/4: cancel unlink wait change - 4/4: merge HCD private lock changes - rebase on 3.10-rc7-next20130624 V1: - change percput tasklet into tasklet in HCD to avoid out of order of URB-complete() for same endpoint - disable local IRQs when calling complete() from tasklet to avoid deadlock which is caused by holding lock via spin_lock and the same lock might be acquired in hard irq context - document coming change about calling complete() with irq enabled so that we can start to clean up USB drivers which call spin_lock() in complete() Documentation/usb/URB.txt | 21 +++--- drivers/usb/core/hcd.c| 147 + drivers/usb/host/ehci-fsl.c |2 +- drivers/usb/host/ehci-grlib.c |2 +- drivers/usb/host/ehci-hcd.c |3 +- drivers/usb/host/ehci-hub.c |1 + drivers/usb/host/ehci-mem.c |1 + drivers/usb/host/ehci-mv.c|2 +- drivers/usb/host/ehci-octeon.c|2 +- drivers/usb/host/ehci-pmcmsp.c|2 +- drivers/usb/host/ehci-ppc-of.c|2 +- drivers/usb/host/ehci-ps3.c |2 +- drivers/usb/host/ehci-q.c |5 -- drivers/usb/host/ehci-sched.c | 47 +++- drivers/usb/host/ehci-sead3.c |2 +- drivers/usb/host/ehci-sh.c|2 +- drivers/usb/host/ehci-tilegx.c|2 +- drivers/usb/host/ehci-timer.c | 35 - drivers/usb/host/ehci-w90x900.c |2 +- drivers/usb/host/ehci-xilinx-of.c |2 +- drivers/usb/host/ehci.h |3 + include/linux/usb/hcd.h | 17 + 22 files changed, 242 insertions(+), 62 deletions(-) Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] USB: EHCI: support running URB giveback in tasklet context
, max:140)
-
* On T410, sometimes read ehci status register in ehci_irq takes more
than 100us, and the problem has been reported on the link:
http://marc.info/?t=13706586731r=1w=2
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-fsl.c |2 +-
drivers/usb/host/ehci-grlib.c |2 +-
drivers/usb/host/ehci-hcd.c |2 +-
drivers/usb/host/ehci-mv.c|2 +-
drivers/usb/host/ehci-octeon.c|2 +-
drivers/usb/host/ehci-pmcmsp.c|2 +-
drivers/usb/host/ehci-ppc-of.c|2 +-
drivers/usb/host/ehci-ps3.c |2 +-
drivers/usb/host/ehci-q.c |5 -
drivers/usb/host/ehci-sead3.c |2 +-
drivers/usb/host/ehci-sh.c|2 +-
drivers/usb/host/ehci-tilegx.c|2 +-
drivers/usb/host/ehci-w90x900.c |2 +-
drivers/usb/host/ehci-xilinx-of.c |2 +-
14 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index bd831ec..330274a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -669,7 +669,7 @@ static const struct hc_driver ehci_fsl_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = HCD_USB2 | HCD_MEMORY,
+ .flags = HCD_USB2 | HCD_MEMORY | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-grlib.c b/drivers/usb/host/ehci-grlib.c
index 5d75de9..c6048ee 100644
--- a/drivers/usb/host/ehci-grlib.c
+++ b/drivers/usb/host/ehci-grlib.c
@@ -43,7 +43,7 @@ static const struct hc_driver ehci_grlib_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index e2fccc0..a0c27fd 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1167,7 +1167,7 @@ static const struct hc_driver ehci_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags =HCD_MEMORY | HCD_USB2,
+ .flags =HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
index 915c2db..ce18a36 100644
--- a/drivers/usb/host/ehci-mv.c
+++ b/drivers/usb/host/ehci-mv.c
@@ -96,7 +96,7 @@ static const struct hc_driver mv_ehci_hc_driver = {
* generic hardware linkage
*/
.irq = ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-octeon.c b/drivers/usb/host/ehci-octeon.c
index 45cc001..ab0397e 100644
--- a/drivers/usb/host/ehci-octeon.c
+++ b/drivers/usb/host/ehci-octeon.c
@@ -51,7 +51,7 @@ static const struct hc_driver ehci_octeon_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-pmcmsp.c b/drivers/usb/host/ehci-pmcmsp.c
index 363890e..3ab32a2 100644
--- a/drivers/usb/host/ehci-pmcmsp.c
+++ b/drivers/usb/host/ehci-pmcmsp.c
@@ -286,7 +286,7 @@ static const struct hc_driver ehci_msp_hc_driver = {
#else
.irq = ehci_irq,
#endif
- .flags =HCD_MEMORY | HCD_USB2,
+ .flags =HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ppc-of.c b/drivers/usb/host/ehci-ppc-of.c
index 56dc732..da95a31 100644
--- a/drivers/usb/host/ehci-ppc-of.c
+++ b/drivers/usb/host/ehci-ppc-of.c
@@ -28,7 +28,7 @@ static const struct hc_driver ehci_ppc_of_hc_driver = {
* generic hardware linkage
*/
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2 | HCD_BH,
/*
* basic lifecycle operations
diff --git a/drivers/usb/host/ehci-ps3.c b/drivers/usb/host/ehci-ps3.c
index fd98377..8188542 100644
--- a/drivers/usb/host/ehci-ps3.c
+++ b/drivers/usb/host/ehci-ps3.c
@@ -71,7 +71,7 @@ static const struct hc_driver ps3_ehci_hc_driver = {
.product_desc = PS3 EHCI Host Controller,
.hcd_priv_size = sizeof(struct ehci_hcd),
.irq= ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
+ .flags = HCD_MEMORY | HCD_USB2
[PATCH v3] USB: check sg buffer size in usb_submit_urb
USB spec stats that short packet can only appear at the end
of transfer. Because lost of HC(EHCI/UHCI/OHCI/...) can't
build a full packet from discontinuous buffers, we introduce
the limit in usb_submit_urb() to avoid such kind of bad sg buffers
coming from driver.
The limit might be a bit strict:
- platform has iommu to do sg list mapping
- some host controllers may support to build full packet from
discontinuous buffers.
But considered that most of HCs don't support that, and driver
need work well or keep consistent on different HCs and ARCHs, we
have to introduce the limit.
Currently, only usbtest is reported to pass such sg buffers to HC,
and other users(mass storage, usbfs) don't have the problem.
We don't check it on USB wireless device, because:
- wireless devices can't be attached to common USB
bus(EHCI/UHCI/OHCI/...)
- the max packet size of endpoint may be odd, and often can't
devide 4KB which is a typical usage in usb mass storage application
Reported-by: Konstantin Filatov kfila...@parallels.com
Reported-by: Denis V. Lunev d...@openvz.org
Cc: Felipe Balbi ba...@ti.com
Acked-by: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
V3:
- save one level of indentation required by Alan Sergei
V2:
- don't check sg buffer size on wireless device
V1:
- avoid checking the last element in if(...) as Alan suggested
- rewrite doc according to Alan's suggestion
drivers/usb/core/urb.c |8
include/linux/usb.h|4 +++-
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 16927fa..e75115a 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -7,6 +7,7 @@
#include linux/usb.h
#include linux/wait.h
#include linux/usb/hcd.h
+#include linux/scatterlist.h
#define to_urb(d) container_of(d, struct urb, kref)
@@ -413,6 +414,13 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
urb-iso_frame_desc[n].status = -EXDEV;
urb-iso_frame_desc[n].actual_length = 0;
}
+ } else if (dev-speed != USB_SPEED_WIRELESS urb-num_sgs) {
+ struct scatterlist *sg;
+ int i;
+
+ for_each_sg(urb-sg, sg, urb-num_sgs - 1, i)
+ if (sg-length % max)
+ return -EINVAL;
}
/* the I/O buffer must be mapped/unmapped, except when length=0 */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a232b7e..e99b2a1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1258,7 +1258,9 @@ typedef void (*usb_complete_t)(struct urb *);
* the device driver is saying that it provided this DMA address,
* which the host controller driver should use in preference to the
* transfer_buffer.
- * @sg: scatter gather buffer list
+ * @sg: scatter gather buffer list, the buffer size of each element in
+ * the list (except the last) must be divisible by the endpoint's
+ * max packet size
* @num_mapped_sgs: (internal) number of mapped sg entries
* @num_sgs: number of entries in the sg list
* @transfer_buffer_length: How big is transfer_buffer. The transfer may
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] USB: check sg buffer size in usb_submit_urb
USB spec stats that short packet can only appear at the end
of transfer. Because lost of HC(EHCI/UHCI/OHCI/...) can't
build a full packet from discontinuous buffers, we introduce
the limit in usb_submit_urb() to avoid such kind of bad sg buffers
coming from driver.
The limit might be a bit strict:
- platform has iommu to do sg list mapping
- some host controllers may support to build full packet from
discontinuous buffers.
But considered that most of HCs don't support that, and driver
need work well or keep consistent on different HCs and ARCHs, we
have to introduce the limit.
Currently, only usbtest is reported to pass such sg buffers to HC,
and other users(mass storage, usbfs) don't have the problem.
We don't check it on USB wireless device, because:
- wireless devices can't be attached to common USB
bus(EHCI/UHCI/OHCI/...)
- the max packet size of endpoint may be odd, and often can't
devide 4KB which is a typical usage in usb mass storage application
Reported-by: Konstantin Filatov kfila...@parallels.com
Reported-by: Denis V. Lunev d...@openvz.org
Cc: Alan Stern st...@rowland.harvard.edu
Cc: Felipe Balbi ba...@ti.com
Signed-off-by: Ming Lei ming@canonical.com
---
V2:
- don't check sg buffer size on wireless device
V1:
- avoid checking the last element in if(...) as Alan suggested
- rewrite doc according to Alan's suggestion
drivers/usb/core/urb.c | 11 +++
include/linux/usb.h|4 +++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 16927fa..c76b7df 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -7,6 +7,7 @@
#include linux/usb.h
#include linux/wait.h
#include linux/usb/hcd.h
+#include linux/scatterlist.h
#define to_urb(d) container_of(d, struct urb, kref)
@@ -413,6 +414,16 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
urb-iso_frame_desc[n].status = -EXDEV;
urb-iso_frame_desc[n].actual_length = 0;
}
+ } else {
+ /* check sg buffer size */
+ if (dev-speed != USB_SPEED_WIRELESS urb-num_sgs) {
+ struct scatterlist *sg;
+ int i;
+
+ for_each_sg(urb-sg, sg, urb-num_sgs - 1, i)
+ if (sg-length % max)
+ return -EINVAL;
+ }
}
/* the I/O buffer must be mapped/unmapped, except when length=0 */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a232b7e..e99b2a1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1258,7 +1258,9 @@ typedef void (*usb_complete_t)(struct urb *);
* the device driver is saying that it provided this DMA address,
* which the host controller driver should use in preference to the
* transfer_buffer.
- * @sg: scatter gather buffer list
+ * @sg: scatter gather buffer list, the buffer size of each element in
+ * the list (except the last) must be divisible by the endpoint's
+ * max packet size
* @num_mapped_sgs: (internal) number of mapped sg entries
* @num_sgs: number of entries in the sg list
* @transfer_buffer_length: How big is transfer_buffer. The transfer may
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] USB: HCD: support giveback of URB in tasklet context
On Mon, Jun 24, 2013 at 11:17 PM, Alan Stern st...@rowland.harvard.edu wrote:
On Mon, 24 Jun 2013, Ming Lei wrote:
This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.
Changes from v1 to v2?
The change log is put in 0/4.
+static void usb_giveback_urb_bh(unsigned long param)
+{
+ struct giveback_urb_bh *bh = (struct giveback_urb_bh *)param;
+ unsigned long flags;
+ struct list_head local_list;
+
+ spin_lock_irqsave(bh-lock, flags);
+ bh-running = 1;
+ restart:
+ list_replace_init(bh-head, local_list);
+ spin_unlock_irqrestore(bh-lock, flags);
Tasklet routines are always called with interrupts enabled, right?
Therefore you don't need to use the flags argument here or below.
Good catch.
@@ -1667,25 +1721,40 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
*/
void usb_hcd_giveback_urb(struct usb_hcd *hcd, struct urb *urb, int status)
{
- urb-hcpriv = NULL;
- if (unlikely(urb-unlinked))
- status = urb-unlinked;
- else if (unlikely((urb-transfer_flags URB_SHORT_NOT_OK)
- urb-actual_length urb-transfer_buffer_length
- !status))
- status = -EREMOTEIO;
+ struct giveback_urb_bh *bh;
+ bool sched, high_prio_bh;
- unmap_urb_for_dma(hcd, urb);
- usbmon_urb_complete(hcd-self, urb, status);
- usb_unanchor_urb(urb);
+ /* pass status to tasklet via unlinked */
+ if (likely(!urb-unlinked))
+ urb-unlinked = status;
- /* pass ownership to the completion handler */
- urb-status = status;
- urb-complete (urb);
- atomic_dec (urb-use_count);
- if (unlikely(atomic_read(urb-reject)))
- wake_up (usb_kill_urb_queue);
- usb_put_urb (urb);
+ if (!hcd_giveback_urb_in_bh(hcd) !is_root_hub(urb-dev)) {
+ __usb_hcd_giveback_urb(urb);
+ return;
+ }
+
+ if (usb_pipeisoc(urb-pipe) || usb_pipeint(urb-pipe)) {
+ bh = hcd-high_prio_bh;
+ high_prio_bh = 1;
+ } else {
+ bh = hcd-low_prio_bh;
+ high_prio_bh = 0;
+ }
Bool values should be assigned true or false, not 1 or 0.
Right.
+
+ spin_lock(bh-lock);
+ list_add_tail(urb-urb_list, bh-head);
+ if (bh-running)
+ sched = 0;
+ else
+ sched = 1;
+ spin_unlock(bh-lock);
How about calling this variable running instead of sched? Then you
could just say:
running = bh-running;
with no if statement.
OK, even we can do this below without name change:
sched = !bh-running;
+
+ if (!sched)
+ ;
+ else if (high_prio_bh)
+ tasklet_hi_schedule(bh-bh);
+ else
+ tasklet_schedule(bh-bh);
}
EXPORT_SYMBOL_GPL(usb_hcd_giveback_urb);
@@ -2307,6 +2376,42 @@ EXPORT_SYMBOL_GPL (usb_hc_died);
/*-*/
+static void __init_giveback_urb_bh(struct giveback_urb_bh *bh)
+{
+
+ spin_lock_init(bh-lock);
+ INIT_LIST_HEAD(bh-head);
+ tasklet_init(bh-bh, usb_giveback_urb_bh, (unsigned long)bh);
+}
+
+static void init_giveback_urb_bh(struct usb_hcd *hcd)
+{
+ if (!hcd_giveback_urb_in_bh(hcd))
+ return;
As you mentioned, there's not much point in skipping this code when
it's not needed. In fact, you could just put the two lines below
directly into usb_create_shared_hcd(), and get rid of the __ from the
beginning of the name.
OK.
+
+ __init_giveback_urb_bh(hcd-high_prio_bh);
+ __init_giveback_urb_bh(hcd-low_prio_bh);
+}
+
+static void exit_giveback_urb_bh(struct usb_hcd *hcd)
+{
+ if (!hcd_giveback_urb_in_bh(hcd))
+ return;
+
+ /*
+ * tasklet_kill() isn't needed here because:
+ * - driver's disconnec() called from usb_disconnect() should
Misspelled disconnect.
+ * make sure its URBs are completed during the disconnect()
+ * callback
+ *
+ * - it is too late to run complete() here since driver may have
+ * been removed already now
+ *
+ * Using tasklet to run URB's complete() doesn't change this
+ * behavior of usbcore.
+ */
+}
Why have a separate subroutine for doing nothing? Simply put this
comment directly into usb_remove_hcd(). (And you can remove the last
two lines of the comment; they don't make sense in this context.)
Looks it is a bit clean to put the comment in the function, but it is
OK to add them in usb_remove_hcd() too.
Thanks again for your review.
Thanks,
--
Ming Lei
--
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
Re: [PATCH v2 3/4] USB: EHCI: improve interrupt qh unlink
On Tue, Jun 25, 2013 at 2:53 AM, Alan Stern st...@rowland.harvard.edu wrote:
On Mon, 24 Jun 2013, Ming Lei wrote:
Given interrupt URB will be resubmitted from tasklet context which
is scheduled by ehci hardware interrupt handler, and commonly only
one interrupt URB is scheduled on qh, so the qh may be unlinked
immediately once qh_completions() returns from ehci_irq(), then
the intr URB to be resubmitted in complete() can only be scheduled
and linked to hardware until the qh unlink is completed.
This patch improves this above situation, and the qh will wait for 5
milliseconds before being unlinked from hardware, if one URB is submitted
during the period, the qh is move out of unlink wait list and the
interrupt transfer can be scheduled immediately, not like before: the
transfer is only linked to hardware until previous unlink is completed.
This description is very hard to understand. I suggest rewriting it,
something like this:
ehci-hcd currently unlinks an interrupt QH when it becomes empty, that
is, after its last URB completes. This works well because in almost
all cases, the completion handler for an interrupt URB resubmits the
URB; therefore the QH doesn't become empty and doesn't get unlinked.
When we start using tasklets for URB completion, this scheme won't work
as well. The resubmission won't occur until the tasklet runs, which
will be some time after the completion is queued with the tasklet.
During that delay, the QH will be empty and so will be unlinked
unnecessarily.
To prevent this problem, this patch adds a 5-ms time delay before empty
interrupt QHs are unlinked. Most often, during that time the interrupt
URB will be resubmitted and thus we can avoid unlinking the QH.
Excellent description about the change, and I will add it in V3,
also care to add your signed-off in the patch for the change log part?
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..5bf67e2 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,12 +601,24 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del(qh-intr_node);
}
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+/* must be called with holding ehci-lock */
The comment should be:
/* caller must hold ehci-lock */
But in fact you can leave it out. Almost all the code in this file
runs with the lock held.
OK.
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh
*qh)
{
- /* If the QH isn't linked then there's nothing we can do. */
- if (qh-qh_state != QH_STATE_LINKED)
+ if (qh-qh_state != QH_STATE_LINKED || list_empty(qh-unlink_node))
return;
+ list_del_init(qh-unlink_node);
+
+ /* avoid unnecessary CPU wakeup */
+ if (list_empty(ehci-intr_unlink_wait))
+ ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
If you don't mind, can we leave out ehci_disable_event() for now and
add it after the rest of this series is merged? It will keeps things a
little simpler, and then we'll be able to use ehci_disable_event() for
the IAA watchdog timer event as well as using it here.
I am fine, so this patch will become simpler and has less change.
+}
+
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+ /* if the qh is waitting for unlink, cancel it now */
s/waitt/wait/
+ cancel_unlink_wait_intr(ehci, qh);
+
qh_unlink_periodic (ehci, qh);
/* Make sure the unlinks are visible before starting the timer */
@@ -632,6 +644,45 @@ static void start_unlink_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
}
}
+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void __start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+ bool wait)
+{
+ /* If the QH isn't linked then there's nothing we can do. */
+ if (qh-qh_state != QH_STATE_LINKED)
+ return;
+
+ if (!wait)
+ return start_do_unlink_intr(ehci, qh);
+
+ qh-unlink_cycle = ehci-intr_unlink_wait_cycle;
+
+ /* New entries go at the end of the intr_unlink_wait list */
+ list_add_tail(qh-unlink_node, ehci-intr_unlink_wait);
+
+ if (ehci-rh_state EHCI_RH_RUNNING)
+ ehci_handle_start_intr_unlinks(ehci);
+ else if (ehci-intr_unlink_wait.next == qh-unlink_node) {
+ ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+ ++ehci-intr_unlink_wait_cycle;
+ }
+}
+
+static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+ __start_unlink_intr(ehci, qh, false);
+}
+
+static void start_unlink_intr_wait(struct ehci_hcd *ehci,
+struct
Re: [v4] usb: UHCI: fix pkt size in TD for a sg element
On Tue, Jun 25, 2013 at 12:26 AM, Alan Stern st...@rowland.harvard.edu wrote: Other than usbtest, the only driver using SG that I know of is usb-storage, and it does make that assumption. It works because the Another example is usbfs driver, which sets the SG size as 16KB and also makes the assumption. block layer packages the I/O up into groups of pages and the bulk maxpacket values always divide 4096 (or whatever the page size happens to be) -- except for wireless USB. Thanks, -- Ming Lei -- 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] USB: check sg buffer size in usb_submit_urb
USB spec stats that short packet can only appear at the end
of transfer. Because lost of HC(EHCI/UHCI/OHCI/...) can't
build a full packet from discontinuous buffers, we introduce
the limit in usb_submit_urb() to avoid such kind of bad sg
coming from driver.
The limit might be a bit strict:
- platform has iommu to do sg list mapping
- some host controllers may support to build full packet from
discontinuous buffers.
But considered that most of HCs don't support that, and driver
need work well or keep consistent on different HCs or ARCHs, we
have to introduce the limit.
Currently, only usbtest is reported to pass such sg buffers to HC,
and other users(mass storage, usbfs) don't have the problem.
Reported-by: Konstantin Filatov kfila...@parallels.com
Reported-by: Denis V. Lunev d...@openvz.org
Cc: Alan Stern st...@rowland.harvard.edu
Cc: Felipe Balbi ba...@ti.com
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/urb.c | 11 +++
include/linux/usb.h|3 ++-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 16927fa..ad6717a 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -7,6 +7,7 @@
#include linux/usb.h
#include linux/wait.h
#include linux/usb/hcd.h
+#include linux/scatterlist.h
#define to_urb(d) container_of(d, struct urb, kref)
@@ -413,6 +414,16 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
urb-iso_frame_desc[n].status = -EXDEV;
urb-iso_frame_desc[n].actual_length = 0;
}
+ } else {
+ /* check sg buffer size */
+ if (urb-num_sgs) {
+ struct scatterlist *sg;
+ int i;
+
+ for_each_sg(urb-sg, sg, urb-num_sgs, i)
+ if (i urb-num_sgs - 1 (sg-length % max))
+ return -EINVAL;
+ }
}
/* the I/O buffer must be mapped/unmapped, except when length=0 */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a232b7e..d21a025 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1258,7 +1258,8 @@ typedef void (*usb_complete_t)(struct urb *);
* the device driver is saying that it provided this DMA address,
* which the host controller driver should use in preference to the
* transfer_buffer.
- * @sg: scatter gather buffer list
+ * @sg: scatter gather buffer list, except for the last sg, buffer size
+ * of all sg must be divided by the endpoint's max packet size
* @num_mapped_sgs: (internal) number of mapped sg entries
* @num_sgs: number of entries in the sg list
* @transfer_buffer_length: How big is transfer_buffer. The transfer may
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V1] USB: check sg buffer size in usb_submit_urb
USB spec stats that short packet can only appear at the end
of transfer. Because lost of HC(EHCI/UHCI/OHCI/...) can't
build a full packet from discontinuous buffers, we introduce
the limit in usb_submit_urb() to avoid such kind of bad sg
coming from driver.
The limit might be a bit strict:
- platform has iommu to do sg list mapping
- some host controllers may support to build full packet from
discontinuous buffers.
But considered that most of HCs don't support that, and driver
need work well or keep consistent on different HCs or ARCHs, we
have to introduce the limit.
Currently, only usbtest is reported to pass such sg buffers to HC,
and other users(mass storage, usbfs) don't have the problem.
Reported-by: Konstantin Filatov kfila...@parallels.com
Reported-by: Denis V. Lunev d...@openvz.org
Cc: Alan Stern st...@rowland.harvard.edu
Cc: Felipe Balbi ba...@ti.com
Signed-off-by: Ming Lei ming@canonical.com
---
V1:
- avoid checking the last element in if(...) as Alan suggested
- rewrite doc according to Alan's suggestion
Thomas, please let us know if you think wireless USB needn't the check.
drivers/usb/core/urb.c | 11 +++
include/linux/usb.h|4 +++-
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 16927fa..3b95821 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -7,6 +7,7 @@
#include linux/usb.h
#include linux/wait.h
#include linux/usb/hcd.h
+#include linux/scatterlist.h
#define to_urb(d) container_of(d, struct urb, kref)
@@ -413,6 +414,16 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
urb-iso_frame_desc[n].status = -EXDEV;
urb-iso_frame_desc[n].actual_length = 0;
}
+ } else {
+ /* check sg buffer size */
+ if (urb-num_sgs) {
+ struct scatterlist *sg;
+ int i;
+
+ for_each_sg(urb-sg, sg, urb-num_sgs - 1, i)
+ if (sg-length % max)
+ return -EINVAL;
+ }
}
/* the I/O buffer must be mapped/unmapped, except when length=0 */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index a232b7e..e99b2a1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1258,7 +1258,9 @@ typedef void (*usb_complete_t)(struct urb *);
* the device driver is saying that it provided this DMA address,
* which the host controller driver should use in preference to the
* transfer_buffer.
- * @sg: scatter gather buffer list
+ * @sg: scatter gather buffer list, the buffer size of each element in
+ * the list (except the last) must be divisible by the endpoint's
+ * max packet size
* @num_mapped_sgs: (internal) number of mapped sg entries
* @num_sgs: number of entries in the sg list
* @transfer_buffer_length: How big is transfer_buffer. The transfer may
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V1] USB: check sg buffer size in usb_submit_urb
On Wed, Jun 26, 2013 at 10:30 AM, Thomas Pugliese thomas.pugli...@gmail.com wrote: Hi Ming, If the target device is wireless, this check should not be necessary. Both types of WUSB host controllers (HWA and WHCI) support building a packet from a buffer that crosses SG boundaries. Thomas, thanks for your clarification. I will rule out the check on wireless devices by using 'dev-speed' in v2, but not only because WUSB HC supports building a packet from discontinous buffers, but also below: - max packet size of wireless device's endpoint might be changed runtime - wireless devices can't be used in common USB(1.1/2.0/3.0) bus. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] USB: HCD/EHCI: giveback of URB in tasklet context
Hi, The patchset supports to run giveback of URB in tasklet context, so that DMA unmapping/mapping on transfer buffer and compelte() callback can be run with interrupt enabled, then time of HCD interrupt handler(IRQs disabled time) can be saved much. Also this approach may simplify HCD since HCD lock needn't be released any more before calling usb_hcd_giveback_urb(). The patchset enables the mechanism on EHCI HCD now. In the commit log of patch 4/4, detailed test result on three machines (ARM A9/A15 dual core, X86) are provided about transfer performance and ehci irq handling time. From the result, no transfer performance loss is found and ehci irq handling time drops much with the patchset. V2: - 1/4: always run URB complete() of root-hub in tasklet - 1/4: store urb status in urb-unlinked - 1/4: don't allocate 'struct giveback_urb_bh' dynamically - 1/4: other minor changes - 2/4: descript changes simply - 3/4: don't use QH_STATE_UNLINK_WAIT to implement intr qh unlink wait - 3/4: cancel unlink wait change - 4/4: merge HCD private lock changes - rebase on 3.10-rc7-next20130624 V1: - change percput tasklet into tasklet in HCD to avoid out of order of URB-complete() for same endpoint - disable local IRQs when calling complete() from tasklet to avoid deadlock which is caused by holding lock via spin_lock and the same lock might be acquired in hard irq context - document coming change about calling complete() with irq enabled so that we can start to clean up USB drivers which call spin_lock() in complete() Documentation/usb/URB.txt | 21 +++-- drivers/usb/core/hcd.c| 169 ++--- drivers/usb/host/ehci-fsl.c |2 +- drivers/usb/host/ehci-grlib.c |2 +- drivers/usb/host/ehci-hcd.c |3 +- drivers/usb/host/ehci-hub.c |1 + drivers/usb/host/ehci-mem.c |1 + drivers/usb/host/ehci-mv.c|2 +- drivers/usb/host/ehci-octeon.c|2 +- drivers/usb/host/ehci-pmcmsp.c|2 +- drivers/usb/host/ehci-ppc-of.c|2 +- drivers/usb/host/ehci-ps3.c |2 +- drivers/usb/host/ehci-q.c |5 -- drivers/usb/host/ehci-sched.c | 62 +- drivers/usb/host/ehci-sead3.c |2 +- drivers/usb/host/ehci-sh.c|2 +- drivers/usb/host/ehci-tilegx.c|2 +- drivers/usb/host/ehci-timer.c | 45 +- drivers/usb/host/ehci-w90x900.c |2 +- drivers/usb/host/ehci-xilinx-of.c |2 +- drivers/usb/host/ehci.h |3 + include/linux/usb/hcd.h | 17 22 files changed, 287 insertions(+), 64 deletions(-) Thanks, -- Ming Lei -- 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 1/4] USB: HCD: support giveback of URB in tasklet context
This patch implements the mechanism of giveback of URB in
tasklet context, so that hardware interrupt handling time for
usb host controller can be saved much, and HCD interrupt handling
can be simplified.
Motivations:
1), on some arch(such as ARM), DMA mapping/unmapping is a bit
time-consuming, for example: when accessing usb mass storage
via EHCI on pandaboard, the common length of transfer buffer is 120KB,
the time consumed on DMA unmapping may reach hundreds of microseconds;
even on A15 based box, the time is still about scores of microseconds
2), on some arch, reading DMA coherent memoery is very time-consuming,
the most common example is usb video class driver[1]
3), driver's complete() callback may do much things which is driver
specific, so the time is consumed unnecessarily in hardware irq context.
4), running driver's complete() callback in hardware irq context causes
that host controller driver has to release its lock in interrupt handler,
so reacquiring the lock after return may busy wait a while and increase
interrupt handling time. More seriously, releasing the HCD lock makes
HCD becoming quite complicated to deal with introduced races.
So the patch proposes to run giveback of URB in tasklet context, then
time consumed in HCD irq handling doesn't depend on drivers' complete and
DMA mapping/unmapping any more, also we can simplify HCD since the HCD
lock isn't needed to be released during irq handling.
The patch should be reasonable and doable:
1), for drivers, they don't care if the complete() is called in hard irq
context or softirq context
2), the biggest change is the situation in which usb_submit_urb() is called
in complete() callback, so the introduced tasklet schedule delay might be a
con, but it shouldn't be a big deal:
- control/bulk asynchronous transfer isn't sensitive to schedule
delay
- the patch schedules giveback of periodic URBs using
tasklet_hi_schedule, so the introduced delay should be very
small
- for ISOC transfer, generally, drivers submit several URBs
concurrently to avoid interrupt delay, so it is OK with the
little schedule delay.
- for interrupt transfer, generally, drivers only submit one URB
at the same time, but interrupt transfer is often used in event
report, polling, ... situations, and a little delay should be OK.
Considered that HCDs may optimize on submitting URB in complete(), the
patch may cause the optimization not working, so introduces one flag to mark
if the HCD supports to run giveback URB in tasklet context. When all HCDs
are ready, the flag can be removed.
[1], http://marc.info/?t=136438111600010r=1w=2
Cc: Oliver Neukum oli...@neukum.org
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/core/hcd.c | 169 ++-
include/linux/usb/hcd.h | 17 +
2 files changed, 156 insertions(+), 30 deletions(-)
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 014dc99..1fbd193 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -694,15 +694,7 @@ error:
/* any errors get returned through the urb completion */
spin_lock_irq(hcd_root_hub_lock);
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- /* This peculiar use of spinlocks echoes what real HC drivers do.
-* Avoiding calls to local_irq_disable/enable makes the code
-* RT-friendly.
-*/
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
-
spin_unlock_irq(hcd_root_hub_lock);
return 0;
}
@@ -742,9 +734,7 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
memcpy(urb-transfer_buffer, buffer, length);
usb_hcd_unlink_urb_from_ep(hcd, urb);
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, 0);
- spin_lock(hcd_root_hub_lock);
} else {
length = 0;
set_bit(HCD_FLAG_POLL_PENDING, hcd-flags);
@@ -834,10 +824,7 @@ static int usb_rh_urb_dequeue(struct usb_hcd *hcd, struct
urb *urb, int status)
if (urb == hcd-status_urb) {
hcd-status_urb = NULL;
usb_hcd_unlink_urb_from_ep(hcd, urb);
-
- spin_unlock(hcd_root_hub_lock);
usb_hcd_giveback_urb(hcd, urb, status);
- spin_lock(hcd_root_hub_lock);
}
}
done:
@@ -1648,6 +1635,73 @@ int usb_hcd_unlink_urb (struct urb *urb, int status)
/*-*/
+static void __usb_hcd_giveback_urb(struct urb *urb)
+{
+ struct usb_hcd *hcd = bus_to_hcd(urb-dev-bus);
+ int status = urb-unlinked
[PATCH v2 4/4] USB: EHCI: support running URB giveback in tasklet context
All 4 transfer types can work well on EHCI HCD after switching to run URB giveback in tasklet context, so mark all HCD drivers to support it. At the same time, don't release ehci-lock during URB giveback, and remove the check on HCD_BH in ehci_disable_event(). From below test results on 3 machines(2 ARM and one x86), time consumed by EHCI interrupt handler droped much without performance loss. 1 test description 1.1 mass storage performance test: - run below command 10 times and compute the average performance dd if=/dev/sdN iflag=direct of=/dev/null bs=200M count=1 - two usb mass storage device: A: sandisk extreme USB 3.0 16G(used in test case 1 case 2) B: kingston DataTraveler G2 4GB(only used in test case 2) 1.2 uvc function test: - run one simple capture program in the below link http://kernel.ubuntu.com/~ming/up/capture.c - capture format 640*480 and results in High Bandwidth mode on the uvc device: Z-Star 0x0ac8/0x3450 - on T410(x86) laptop, also use guvcview to watch video capture/playback 1.3 about test2 and test4 - both two devices involved are tested concurrently by above test items 1.4 how to compute irq time(the time consumed by ehci_irq) - use trace points of irq:irq_handler_entry and irq:irq_handler_exit 1.5 kernel 3.10.0-rc3-next-20130528 1.6 test machines Pandaboard A1: ARM CortexA9 dural core Arndale board: ARM CortexA15 dural core T410: i5 CPU 2.67GHz quad core 2 test result 2.1 test case1: single mass storage device performance test upstream| patched perf(MB/s)+irq time(us) | perf(MB/s)+irq time(us) Pandaboard A1: 25.280(avg:145,max:772) | 25.540(avg:14, max:75) Arndale board: 29.700(avg:33, max:129) | 29.700(avg:10, max:50) T410: 34.430(avg:17, max:154*)| 34.660(avg:12, max:155) - 2.2 test case2: two mass storage devices' performance test upstream| patched perf(MB/s)+irq time(us) | perf(MB/s)+irq time(us) Pandaboard A1: 15.840/15.580(avg:158,max:1216) | 16.500/16.160(avg:15,max:139) Arndale board: 17.370/16.220(avg:33 max:234) | 17.480/16.200(avg:11, max:91) T410: 21.180/19.820(avg:18 max:160) | 21.220/19.880(avg:11, max:149) - 2.3 test case3: one uvc streaming test - uvc device works well(on x86, luvcview can be used too and has same result with uvc capture) upstream| patched irq time(us)| irq time(us) Pandaboard A1: (avg:445, max:873) | (avg:33, max:44) Arndale board: (avg:316, max:630) | (avg:20, max:27) T410: (avg:39, max:107) | (avg:10, max:65) - 2.4 test case4: one uvc streaming plus one mass storage device test upstream| patched perf(MB/s)+irq time(us) | perf(MB/s)+irq time(us) Pandaboard A1: 20.340(avg:259,max:1704)| 20.390(avg:24, max:101) Arndale board: 23.460(avg:124,max:726) | 23.370(avg:15, max:52) T410: 28.520(avg:27, max:169) | 28.630(avg:13, max:160) - * On T410, sometimes read ehci status register in ehci_irq takes more than 100us, and the problem has been reported on the link: http://marc.info/?t=13706586731r=1w=2 Cc: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- drivers/usb/host/ehci-fsl.c |2 +- drivers/usb/host/ehci-grlib.c |2 +- drivers/usb/host/ehci-hcd.c |2 +- drivers/usb/host/ehci-mv.c|2 +- drivers/usb/host/ehci-octeon.c|2 +- drivers/usb/host/ehci-pmcmsp.c|2 +- drivers/usb/host/ehci-ppc-of.c|2 +- drivers/usb/host/ehci-ps3.c |2 +- drivers/usb/host/ehci-q.c |5 - drivers/usb/host/ehci-sead3.c |2 +- drivers/usb/host/ehci-sh.c|2 +- drivers/usb/host/ehci-tilegx.c|2 +- drivers/usb/host/ehci-timer.c |3 --- drivers/usb/host/ehci-w90x900.c |2 +- drivers/usb/host/ehci-xilinx-of.c |2 +- 15 files changed, 13 insertions(+), 21 deletions(-) diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c index bd831ec..330274a 100644 --- a/drivers
[PATCH v2 3/4] USB: EHCI: improve interrupt qh unlink
Given interrupt URB will be resubmitted from tasklet context which
is scheduled by ehci hardware interrupt handler, and commonly only
one interrupt URB is scheduled on qh, so the qh may be unlinked
immediately once qh_completions() returns from ehci_irq(), then
the intr URB to be resubmitted in complete() can only be scheduled
and linked to hardware until the qh unlink is completed.
This patch improves this above situation, and the qh will wait for 5
milliseconds before being unlinked from hardware, if one URB is submitted
during the period, the qh is move out of unlink wait list and the
interrupt transfer can be scheduled immediately, not like before: the
transfer is only linked to hardware until previous unlink is completed.
Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
drivers/usb/host/ehci-hcd.c |1 +
drivers/usb/host/ehci-hub.c |1 +
drivers/usb/host/ehci-mem.c |1 +
drivers/usb/host/ehci-sched.c | 62 ++---
drivers/usb/host/ehci-timer.c | 48 ++-
drivers/usb/host/ehci.h |3 ++
6 files changed, 111 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 7abf1ce..35f1372 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -487,6 +487,7 @@ static int ehci_init(struct usb_hcd *hcd)
ehci-periodic_size = DEFAULT_I_TDPS;
INIT_LIST_HEAD(ehci-async_unlink);
INIT_LIST_HEAD(ehci-async_idle);
+ INIT_LIST_HEAD(ehci-intr_unlink_wait);
INIT_LIST_HEAD(ehci-intr_unlink);
INIT_LIST_HEAD(ehci-intr_qh_list);
INIT_LIST_HEAD(ehci-cached_itd_list);
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 2b70277..6037f84 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -345,6 +345,7 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
end_unlink_async(ehci);
unlink_empty_async_suspended(ehci);
+ ehci_handle_start_intr_unlinks(ehci);
ehci_handle_intr_unlinks(ehci);
end_free_itds(ehci);
diff --git a/drivers/usb/host/ehci-mem.c b/drivers/usb/host/ehci-mem.c
index ef2c3a1..52a7773 100644
--- a/drivers/usb/host/ehci-mem.c
+++ b/drivers/usb/host/ehci-mem.c
@@ -93,6 +93,7 @@ static struct ehci_qh *ehci_qh_alloc (struct ehci_hcd *ehci,
gfp_t flags)
qh-qh_dma = dma;
// INIT_LIST_HEAD (qh-qh_list);
INIT_LIST_HEAD (qh-qtd_list);
+ INIT_LIST_HEAD(qh-unlink_node);
/* dummy td enables safe urb queuing */
qh-dummy = ehci_qtd_alloc (ehci, flags);
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index f80d033..5bf67e2 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -601,12 +601,24 @@ static void qh_unlink_periodic(struct ehci_hcd *ehci,
struct ehci_qh *qh)
list_del(qh-intr_node);
}
-static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+/* must be called with holding ehci-lock */
+static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
{
- /* If the QH isn't linked then there's nothing we can do. */
- if (qh-qh_state != QH_STATE_LINKED)
+ if (qh-qh_state != QH_STATE_LINKED || list_empty(qh-unlink_node))
return;
+ list_del_init(qh-unlink_node);
+
+ /* avoid unnecessary CPU wakeup */
+ if (list_empty(ehci-intr_unlink_wait))
+ ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
+}
+
+static void start_do_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
+{
+ /* if the qh is waitting for unlink, cancel it now */
+ cancel_unlink_wait_intr(ehci, qh);
+
qh_unlink_periodic (ehci, qh);
/* Make sure the unlinks are visible before starting the timer */
@@ -632,6 +644,45 @@ static void start_unlink_intr(struct ehci_hcd *ehci,
struct ehci_qh *qh)
}
}
+/*
+ * It is common only one intr URB is scheduled on one qh, and
+ * given complete() is run in tasklet context, introduce a bit
+ * delay to avoid unlink qh too early.
+ */
+static void __start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh,
+ bool wait)
+{
+ /* If the QH isn't linked then there's nothing we can do. */
+ if (qh-qh_state != QH_STATE_LINKED)
+ return;
+
+ if (!wait)
+ return start_do_unlink_intr(ehci, qh);
+
+ qh-unlink_cycle = ehci-intr_unlink_wait_cycle;
+
+ /* New entries go at the end of the intr_unlink_wait list */
+ list_add_tail(qh-unlink_node, ehci-intr_unlink_wait);
+
+ if (ehci-rh_state EHCI_RH_RUNNING)
+ ehci_handle_start_intr_unlinks(ehci);
+ else if (ehci-intr_unlink_wait.next == qh-unlink_node) {
+ ehci_enable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR, true);
+ ++ehci
[PATCH v2 2/4] USB: URB documentation: claim complete() will be run with IRQs enabled
There is no good reason to run complete() in hard interrupt disabled context. After switch to run complete() in tasklet, we will enable local IRQs when calling complete() since we can do it at that time. Even though we still disable IRQs now when calling complete() in tasklet, the URB documentation is updated to claim complete() will be run in tasklet context and local IRQs will be enabled, so that USB drivers can know the change and avoid one deadlock caused by: assume IRQs disabled in complete() and call spin_lock() to hold lock which might be acquired in interrupt context. Current spin_lock() usages in drivers' complete() will be cleaned up at the same time, and once the cleanup is finished, local IRQs will be enabled when calling complete() in tasklet. Also fix description about type of usb_complete_t, and remove the advice of running completion handler in tasklet for decreasing system latency. Cc: Oliver Neukum oli...@neukum.org Cc: Alan Stern st...@rowland.harvard.edu Signed-off-by: Ming Lei ming@canonical.com --- Documentation/usb/URB.txt | 21 ++--- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/Documentation/usb/URB.txt b/Documentation/usb/URB.txt index 00d2c64..50da0d4 100644 --- a/Documentation/usb/URB.txt +++ b/Documentation/usb/URB.txt @@ -195,13 +195,12 @@ by the completion handler. The handler is of the following type: - typedef void (*usb_complete_t)(struct urb *, struct pt_regs *) + typedef void (*usb_complete_t)(struct urb *) -I.e., it gets the URB that caused the completion call, plus the -register values at the time of the corresponding interrupt (if any). -In the completion handler, you should have a look at urb-status to -detect any USB errors. Since the context parameter is included in the URB, -you can pass information to the completion handler. +I.e., it gets the URB that caused the completion call. In the completion +handler, you should have a look at urb-status to detect any USB errors. +Since the context parameter is included in the URB, you can pass +information to the completion handler. Note that even when an error (or unlink) is reported, data may have been transferred. That's because USB transfers are packetized; it might take @@ -210,12 +209,12 @@ have transferred successfully before the completion was called. NOTE: * WARNING * -NEVER SLEEP IN A COMPLETION HANDLER. These are normally called -during hardware interrupt processing. If you can, defer substantial -work to a tasklet (bottom half) to keep system latencies low. You'll -probably need to use spinlocks to protect data structures you manipulate -in completion handlers. +NEVER SLEEP IN A COMPLETION HANDLER. These are often called in atomic +context. +In the current kernel, completion handlers run with local interrupts +disabled, but in the future this will be changed, so don't assume that +local IRQs are always disabled inside completion handlers. 1.8. How to do isochronous (ISO) transfers? -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] USB: EHCI: improve interrupt qh unlink
On Mon, Jun 24, 2013 at 6:28 PM, Oliver Neukum oli...@neukum.org wrote: On Monday 24 June 2013 17:42:04 Ming Lei wrote: This patch improves this above situation, and the qh will wait for 5 milliseconds before being unlinked from hardware, if one URB is submitted during the period, the qh is move out of unlink wait list and the interrupt transfer can be scheduled immediately, not like before: the transfer is only linked to hardware until previous unlink is completed. It seems to me that this logic should be used only if the URB completed without error. The current completed URB with error doesn't mean the later submitted URB will complete with error, so I don't think the logic is related with URB completion error. Thanks, -- Ming Lei -- 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: [v2] usb: UHCI: fix pkt size in TD for a non-aligned sg element
On Mon, Jun 24, 2013 at 6:10 PM, Konstantin Filatov kfila...@parallels.com wrote: On 06/23/2013 07:05 PM, Alan Stern wrote: That's why, if the check is checked, I feel it should be added to each HCD driver separately. Maybe I'm wrong. But before doing anything, you should check with Thomas Pugliese. He recently added SG support to the wireless USB driver. Suppose wireless USB is one exception, it should the only one, so we can rule it out easily by using hcd-wireless, right? Yes, that's true. Alan Stern This test passed for many years. It became to fail only for the recent kernels. So this event looks like a broken thing. It's not a good idea to force a higher driver to care about the max pkt size in a dynamically selected usb configuration. Note that a sg-list could be The constraint is from USB spec(1.1/2.0/3.0), and a short packet simply means the end of one transfer, I think we can see it as a API about sg list passed to usb_submit_urb(). created even in another subsystem. Considered that there isn't much such kind of usages now, we need to consider the problem, and introducing check in usb_submit_urb() is necessary, so we can avoid and kill such mistaken usages at early stage. Otherwise, we have to introduce buffer debounce in usbcore or hcd to work around the problem when many users start to do that. If we decided to treat such sg-lists as acceptable, then we have only two options: 1. Send a shorter packet between two full packets of a TD list. 2. Introduce a bounce buffer similarly in lib/swiotlb.c Option 2 is very costed. If the option 1 is acceptable, then we should choose it. No, option 1 isn't accepted, and option 2 isn't necessary since there aren't much such usage. In fact, except for usbtest, I don't know there are other drivers which may pass such kind of sg list. If anyone knows, please share it. But ehci does resolve this issue in such way already, so the option 1 is acceptable. No, ehci only doesn't cause memory access error but the way is wrong, and it does violate USB spec. From my point of view this choice is unrelated to the wireless USB requirements. We just want to confirm if wireless USB need the same check. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] USB: HCD: support giveback of URB in tasklet context
On Mon, Jun 24, 2013 at 5:42 PM, Ming Lei ming@canonical.com wrote:
+
+static void init_giveback_urb_bh(struct usb_hcd *hcd)
+{
+ if (!hcd_giveback_urb_in_bh(hcd))
+ return;
Sorry, the above check isn't needed, and the below check isn't
needed too. I will fix it in V3.
+
+ __init_giveback_urb_bh(hcd-high_prio_bh);
+ __init_giveback_urb_bh(hcd-low_prio_bh);
+}
+
+static void exit_giveback_urb_bh(struct usb_hcd *hcd)
+{
+ if (!hcd_giveback_urb_in_bh(hcd))
+ return;
+
+ /*
+* tasklet_kill() isn't needed here because:
+* - driver's disconnec() called from usb_disconnect() should
+* make sure its URBs are completed during the disconnect()
+* callback
+*
+* - it is too late to run complete() here since driver may have
+* been removed already now
+*
+* Using tasklet to run URB's complete() doesn't change this
+* behavior of usbcore.
+*/
+}
+
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] USB: EHCI: support running URB giveback in tasklet context
On Mon, Jun 24, 2013 at 6:24 PM, Oliver Neukum oli...@neukum.org wrote: On Monday 24 June 2013 17:42:05 Ming Lei wrote: All 4 transfer types can work well on EHCI HCD after switching to run URB giveback in tasklet context, so mark all HCD drivers to support it. At the same time, don't release ehci-lock during URB giveback, and remove the check on HCD_BH in ehci_disable_event(). From below test results on 3 machines(2 ARM and one x86), time consumed by EHCI interrupt handler droped much without performance loss. 1 test description 1.1 mass storage performance test: - run below command 10 times and compute the average performance dd if=/dev/sdN iflag=direct of=/dev/null bs=200M count=1 It would be nice to get worst case numbers. How bad does it get if you reduce the sg size in usb-storage from 120K to 4K? A quick test on one arm A15 box shows that the average speed over 10 times 'dd' becomes 8.0MB/sec from 8.160MB/sec when 'bs' parameter of 'dd' changes to 4K, so there is ~1.9% performance loss with the patch under the worst case. Same test on my T410(x86), the speed difference is only 40K. I will collect the worst case numbers and include it in the commit log of V3. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] USB: EHCI: support running URB giveback in tasklet context
On Mon, Jun 24, 2013 at 9:06 PM, Oliver Neukum oli...@neukum.org wrote:
On Monday 24 June 2013 20:58:26 Ming Lei wrote:
On Mon, Jun 24, 2013 at 6:24 PM, Oliver Neukum oli...@neukum.org wrote:
On Monday 24 June 2013 17:42:05 Ming Lei wrote:
All 4 transfer types can work well on EHCI HCD after switching to run
URB giveback in tasklet context, so mark all HCD drivers to support
it.
At the same time, don't release ehci-lock during URB giveback,
and remove the check on HCD_BH in ehci_disable_event().
From below test results on 3 machines(2 ARM and one x86), time
consumed by EHCI interrupt handler droped much without performance
loss.
1 test description
1.1 mass storage performance test:
- run below command 10 times and compute the average performance
dd if=/dev/sdN iflag=direct of=/dev/null bs=200M count=1
It would be nice to get worst case numbers. How bad does it get
if you reduce the sg size in usb-storage from 120K to 4K?
A quick test on one arm A15 box shows that the average speed over
10 times 'dd' becomes 8.0MB/sec from 8.160MB/sec when 'bs'
parameter of 'dd' changes to 4K, so there is ~1.9% performance
loss with the patch under the worst case.
Same test on my T410(x86), the speed difference is only 40K.
I will collect the worst case numbers and include it in the commit
log of V3.
Sorry,
I was referring to scsiglue.c
struct scsi_host_template usb_stor_host_template = {
/* basic userland interface stuff */
.name = usb-storage,
.proc_name =usb-storage,
.proc_info =proc_info,
.info = host_info,
/* command interface -- queued only */
.queuecommand = queuecommand,
/* error and abort handlers */
.eh_abort_handler = command_abort,
.eh_device_reset_handler = device_reset,
.eh_bus_reset_handler = bus_reset,
/* queue commands only, only one command per LUN */
.can_queue =1,
.cmd_per_lun = 1,
/* unknown initiator id */
.this_id = -1,
.slave_alloc = slave_alloc,
.slave_configure = slave_configure,
/* lots of sg segments can be handled */
.sg_tablesize = SCSI_MAX_SG_CHAIN_SEGMENTS,
/* limit the total size of a transfer to 120 KB */
.max_sectors = 240,
If you go to 8 sectors here, you should get the absolute worst case.
If you check trace of usbmon, 'dd if=/dev/sda iflag=direct bs=4k count=xxx'
generates the 4k data stage per transfer(cbw/data/csw).
So there is no difference between them.
Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v4] usb: UHCI: fix pkt size in TD for a sg element
On Mon, Jun 24, 2013 at 10:33 PM, Denis V. Lunev d...@openvz.org wrote: From: Konstantin Filatov kfila...@parallels.com This patch shortens TD's packet not only for the last TD in sg list, but also for the last TD in sg element. Signed-off-by: Konstantin Filatov kfila...@parallels.com Signed-off-by: Denis V. Lunev d...@openvz.org Considered that: - the change violates USB spec(1.1/2.0/3.0) - the problem should be avoided in usbcore since it isn't a uhci specific problem - this patch only hides problem, doesn't help to fix real problem. so NAK. I will cook a patch to prevent such bad sg list from reaching HCDs tomorrow. Thanks, -- Ming Lei -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v4] usb: UHCI: fix pkt size in TD for a sg element
On Mon, Jun 24, 2013 at 11:04 PM, Felipe Balbi ba...@ti.com wrote: On Mon, Jun 24, 2013 at 10:56:30PM +0800, Ming Lei wrote: On Mon, Jun 24, 2013 at 10:33 PM, Denis V. Lunev d...@openvz.org wrote: From: Konstantin Filatov kfila...@parallels.com This patch shortens TD's packet not only for the last TD in sg list, but also for the last TD in sg element. Signed-off-by: Konstantin Filatov kfila...@parallels.com Signed-off-by: Denis V. Lunev d...@openvz.org Considered that: - the change violates USB spec(1.1/2.0/3.0) I can't see how this would violate USB spec. USB specifications have no knowledge of scatter-gather. Per USB spec, short packet can only be the last packet of one transfer. In our linux USB implementation, we think the sg list passed to URB or one URB as one single transfer, so the middle short packet will cause the URB to be understood as two or more transfers by device. It really doesn't matter how the data gets into the HW's FIFO, as long as it *does* get there. IOW an SG table like below: sg[0].length = 512 sg[1].length = 512 sg[2].length = 20 is no different than: sg[0].length = 502 sg[1].length = 512 sg[2].length = 30 from the USB perspective, all is sees is 1044 bytes being shifted through the data lines. Suggest to see the discussion in the below link: http://marc.info/?t=13718225823r=1w=2 - the problem should be avoided in usbcore since it isn't a uhci specific problem to this I agree. - this patch only hides problem, doesn't help to fix real problem. what is the problem then ? Are you saying that it's wrong to have an sg which is not aligned to wMaxPacketSize somewhere in the middle of an sg-table ? How so ? What does that have to do with USB at all ? The problem is that short packet can only be the last packet of one transfer. Thanks, -- Ming Lei -- 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
