On Sat, 12 Jun 2010, Sergei Shtylyov wrote: > > error_create_attr_group: > > + if (HC_IS_RUNNING(hcd->state)) > > + hcd->state = HC_STATE_QUIESCING; > > + spin_lock_irq(&hcd_root_hub_lock); > > + hcd->rh_registered = 0; > > + spin_unlock_irq(&hcd_root_hub_lock); > > What's the point of taking a spinlock and disabling interrupts around an > atomic write? That's a good question. I have stumbled upon exactly the same issue in the past. Well, in fact it's really two questions. The more trivial one is why disable interrupts? That's easy -- since the hcd_root_hub_lock spinlock can be acquired by code that runs in_interrupt, you have to make sure that interrupts are disabled whenever you acquire the spinlock. If you didn't, and an interrupt occurred while you were holding the spinlock, and the interrupt handler wanted to lock the spinlock, you'd be in trouble. The second, more interesting question is why use the spinlock at all? Answer: It's not for protecting the flag; it's for synchronization. To begin with, you should be aware that setting rh_registered is _not_ an atomic operation. Although some CPUs may be able to access bitflags using atomic operations, this certainly isn't guaranteed for all CPUs. Remember, the rh_registered field is defined by: unsigned rh_registered:1; and not by: unsigned rh_registered; To change its value, a CPU might need to load the entire word containing that field into a register, then alter one bit in the register, and then write the register back to memory. That's why you'll sometimes see code use set_bit(), clear_bit(), and test_bit() instead of simply using bitflags -- these operations _are_ guaranteed to be atomic on all CPUs. In fact, I've got another patch, not yet submitted, which changes some of the bitflags in usb_hcd to make them atomic. However it wouldn't have made any difference if rh_registered _were_ atomic; the spinlock would still be needed. In fact, this code could have been written as: hcd->rh_registered = 0; spin_lock_irq(&hcd_root_hub_lock); spin_unlock_irq(&hcd_root_hub_lock); although that would look pretty strange. The reason, as I said above, is for synchronization. hcd->rh_registered is tested in two places: usb_hcd_resume_root_hub() and usb_hc_died(). The reason, obviously, is because we don't want to autoresume a root hub that isn't registered or is about to be unregistered, and likewise for reporting that a root hub has died. The one thing we don't want to happen is that one CPU sets hcd->rh_registered to 0, while at the same time or a little later, another CPU tests hcd->rh_registered and still sees the old value of 1. The way to prevent this is to use a synchronization primitive: a lock, mutex, or memory barrier. Since some of the code runs in_interrupt I couldn't use a mutex, and memory barriers should be used only in special circumstances. Thus a spinlock was the appropriate solution. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
