Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

USB: cdc-acm: remove unneeded spin_lock_irqsave/restore on write path

When writing data we were:
lock
do some work
unlock
call function
lock
do some work
unlock
return
return

It turns out, that "function" was only ever called in the one place, so
instead of locking/unlocking for no good reason, just inline the
function and only grab the lock once.

This has sped up the pathological case of sending 1 byte packets to a
loop-back cdc-acm device from 49600 bytes per second to 50100 bytes a
second on my workstation. A tiny increase yes, but noticable, and now
the spinlock isn't the hottest thing on the perf graph anymore. Yes, we
are still waiting for the hardware for the most part, but getting rid of
a spinlock_irq_save() call for every packet is still a good thing.

And we end up deleting lines of code, always a win overall.

This was found by using a Teensy 3.0 device and the test program and
firmware located at:
http://www.pjrc.com/teensy/benchmark_usb_serial_receive.html

Reported-by: Paul Stoffregen <paul@pjrc.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

+19 -33
+19 -33
drivers/usb/class/cdc-acm.c
··· 216 216 return rc; 217 217 } 218 218 219 - static int acm_write_start(struct acm *acm, int wbn) 220 - { 221 - unsigned long flags; 222 - struct acm_wb *wb = &acm->wb[wbn]; 223 - int rc; 224 - 225 - spin_lock_irqsave(&acm->write_lock, flags); 226 - if (!acm->dev) { 227 - wb->use = 0; 228 - spin_unlock_irqrestore(&acm->write_lock, flags); 229 - return -ENODEV; 230 - } 231 - 232 - dev_vdbg(&acm->data->dev, "%s - susp_count %d\n", __func__, 233 - acm->susp_count); 234 - usb_autopm_get_interface_async(acm->control); 235 - if (acm->susp_count) { 236 - if (!acm->delayed_wb) 237 - acm->delayed_wb = wb; 238 - else 239 - usb_autopm_put_interface_async(acm->control); 240 - spin_unlock_irqrestore(&acm->write_lock, flags); 241 - return 0; /* A white lie */ 242 - } 243 - usb_mark_last_busy(acm->dev); 244 - 245 - rc = acm_start_wb(acm, wb); 246 - spin_unlock_irqrestore(&acm->write_lock, flags); 247 - 248 - return rc; 249 - 250 - } 251 219 /* 252 220 * attributes exported through sysfs 253 221 */ ··· 621 653 } 622 654 wb = &acm->wb[wbn]; 623 655 656 + if (!acm->dev) { 657 + wb->use = 0; 658 + spin_unlock_irqrestore(&acm->write_lock, flags); 659 + return -ENODEV; 660 + } 661 + 624 662 count = (count > acm->writesize) ? acm->writesize : count; 625 663 dev_vdbg(&acm->data->dev, "%s - write %d\n", __func__, count); 626 664 memcpy(wb->buf, buf, count); 627 665 wb->len = count; 666 + 667 + usb_autopm_get_interface_async(acm->control); 668 + if (acm->susp_count) { 669 + if (!acm->delayed_wb) 670 + acm->delayed_wb = wb; 671 + else 672 + usb_autopm_put_interface_async(acm->control); 673 + spin_unlock_irqrestore(&acm->write_lock, flags); 674 + return count; /* A white lie */ 675 + } 676 + usb_mark_last_busy(acm->dev); 677 + 678 + stat = acm_start_wb(acm, wb); 628 679 spin_unlock_irqrestore(&acm->write_lock, flags); 629 680 630 - stat = acm_write_start(acm, wbn); 631 681 if (stat < 0) 632 682 return stat; 633 683 return count;