On Mon, 7 May 2001, Steve Longerbeam wrote:
> I also added locks in the lookup routine dma_to_ed_td().
> It's always a fast lookup, and it's possible the list could
> change in the middle of the search either by interrupt code
> or another processor, although I don't think currently the
> add/free routines are called at interrupt time.
Hi Steve,
Thanks for your fast response! Your (second) patch is almost identical to
the one which I've tried myself meanwhile and which survived heavy testing
(40GB/10h load+consistency test). So there was already much effort to
break it again.
My patch also uses a global spinlock and lock_irqsave for all readers and
writers. And yes, I'm pretty sure we need the readers protected - at least
for me the issue happened on UP making writers called in interrupt the
only event to explain the corruption. The one common lock for all ED/TD
readers and writers has no detectable footprint on USB performance - it's
still 1.15MB/sec as before.
Besides naming, the only difference of our patches is when dma_to_ed_td()
returns scan->virt, I've prefered to copy it into a local variable before
releasing the lock. Just paranoia. The api provides no means of changing
the content of a hash chain entry which is already on the list. And if it
would, we would return a stale value anyway.
As said above with my patch applied the test run through without any
problem over >10h. Without the patch it used to run into the BUG()
within first 30 minutes or so. Unless you see any additional insight from
testing exactly your patch too, I would take it for proven it's right.
To more minor concerns remain for me:
- I don't like the idea of having the static spinlock_t definition in the
usb-ohci.h file - if somebody would ever split the .c and include it
into both, we would have to instances of the lock and it breakes again.
But to be honest, I don't have a better suggestion.
- If there was already another lock (usb_ed_lock for example) supposed to
protect the hash chains, there would be still an undisclosed latent
error which we just hide by the introduction of the new lock.
Below I include the patch exactly as it was tested so far.
Martin
------------------------
--- linux-2.4.4/drivers/usb.orig/usb-ohci.h Wed May 2 19:41:41 2001
+++ linux-2.4.4/drivers/usb/usb-ohci.h Mon May 7 13:34:47 2001
@@ -352,6 +352,9 @@
struct hash_t *next; // chaining for collision cases
};
+static spinlock_t ohci_hashlock = SPIN_LOCK_UNLOCKED;
+
+
/* List of TD/ED hash entries */
struct hash_list_t {
struct hash_t *head;
@@ -458,12 +461,19 @@
static inline void *
dma_to_ed_td (struct hash_list_t * entry, dma_addr_t dma)
{
- struct hash_t * scan = entry->head;
+ struct hash_t * scan;
+ unsigned long flags;
+ void *virt;
+
+ spin_lock_irqsave(&ohci_hashlock,flags);
+ scan = entry->head;
while (scan && scan->dma != dma)
scan = scan->next;
if (!scan)
BUG();
- return scan->virt;
+ virt = scan->virt;
+ spin_unlock_irqrestore(&ohci_hashlock,flags);
+ return virt;
}
static inline struct ed *
@@ -485,11 +495,13 @@
hash_add_ed_td(struct hash_list_t * entry, void * virt, dma_addr_t dma)
{
struct hash_t * scan;
+ unsigned long flags;
scan = (struct hash_t *)kmalloc(sizeof(struct hash_t), ALLOC_FLAGS);
if (!scan)
return 0;
+ spin_lock_irqsave(&ohci_hashlock,flags);
if (!entry->tail) {
entry->head = entry->tail = scan;
} else {
@@ -500,6 +512,7 @@
scan->virt = virt;
scan->dma = dma;
scan->next = NULL;
+ spin_unlock_irqrestore(&ohci_hashlock,flags);
return 1;
}
@@ -522,6 +535,9 @@
hash_free_ed_td (struct hash_list_t * entry, void * virt)
{
struct hash_t *scan, *prev;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ohci_hashlock,flags);
scan = prev = entry->head;
// Find and unlink hash entry
@@ -542,6 +558,7 @@
prev->next = scan->next;
kfree(scan);
}
+ spin_unlock_irqrestore(&ohci_hashlock,flags);
}
static inline void
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
http://lists.sourceforge.net/lists/listinfo/linux-usb-devel
