Messages in this thread | | | Date | Wed, 7 Dec 2005 17:10:58 -0200 | From | Eduardo Pereira Habkost <> | Subject | Re: [PATCH 00/10] usb-serial: Switches from spin lock to atomic_t. |
| |
Hi, Greg,
On Wed, Dec 07, 2005 at 09:56:14AM -0800, Greg KH wrote: > On Wed, Dec 07, 2005 at 03:13:32PM -0200, Eduardo Pereira Habkost wrote: > > I have a small question: in my view, this patch series is a small > > step towards implementing the usb-serial drivers The Right Way, as it > > removes a a bit of duplicated code. > > It doesn't remove any "duplicated code", it only changes a spinlock to > an atomic_t for one single value (which I personally do not think is the > best thing to do, and based on the number of comments on this thread, I > think others also feel this way.)
Every usb-serial driver currently has:
spin_lock(port->lock); if (port->write_urb_busy) return; port->write_urb_busy = 1; spin_unlock(port->lock);
Having the same 5 lines on many usb-serial drivers looks like duplicated code to me. Maybe I am being too exigent. Anyway, this is unrelated to the atomic_t change, and we could do it without dropping ths spinlock.
But I would like to know: would you would accept such change (that encapsulate this write_urb_busy logic without necessarily dropping the spinlock)?
And, about the atomic_t: I've felt most people didn't liked the atomic_t approach for one of two reasons:
1. The existence of write_urb_busy looks like a hack, and we've make it explicit when we isolated the code that uses write_urb_busy in a set of functions (the point of Arjan in his "square wheel" comment) 2. The whole discussion about atomic_t vs. spinlock efficiency
I agree with (1), but I still don't see why using a spinlock to protect a single field is better than using atomic_t. Both in code readability and efficiency. Specially as the difference between each one (even which one is more efficient) is very arch-specific.
Thank you very much for your advice,
-- Eduardo [unhandled content-type:application/pgp-signature]
| |