On Wed, Feb 19, 2003, David Brownell <[EMAIL PROTECTED]> wrote:
>
> >>>>but the hcd drivers should have already canceled them, right? Hm, but
> >>>
> >>>Nope, the only way an hcd can know the device is gone is by
> >>>calling the misnamed bus->op->deallocate() routine. And
> >>>because of the mess stemming from that misnaming, that code
> >>>path doesn't even try to do that any more.
> >>
> >>Huh? Due to a change I just made? I don't see that. It seems to be
>
> You didn't change that, right -- that's what I was saying.
>
> >>that way for a while. And Johannes just stated that uhci doesn't even
> >>implement disconnect() so there's gotta be some way it determines that a
> >>device isn't there anymore. Or it probably just doesn't care anymore,
> >>which seems to be a valid thing to do.
>
> Having looked at the history of that misnamed API:
>
> - Originally provided in 2.2.10 or so, I think OHCI was the only
> user. It was misnamed at that time, given that it was used
> only to handshake "device is gone".
>
> - Sometime in the 2.3 series, someone made a change to make the
> usage in usbcore match that misnaming: just free memory. But
> whoever that was did not change that single implementation of
> the method (ohci) to match the changed semantics.
>
> - So that 2.3.x change created a bug where none existed before.
>
> It would only appear with OHCI *and* with a device driver that
> was buggy and didn't disconnect() or refcount correctly.
I made that change a long time ago when I implemented the reference
counting code (IIRC). I may have missed the change to OHCI, but that
doesn't mean it's misnamed.
It just means that OHCI hasn't been updated to match what it does now.
I've attached the patch I made for 2.4 at the end. See the comments at
the end about it.
> Of course the APIs were in such flux that long ago that it's very
> understandable how a misnamed API would cause that kind of trouble.
>
> Particularly in areas like this, where different HC hardware (or
> at least their drivers) has different expectations.
I think the change is compatible with the expectations of the hardware.
It apparentely wasn't compatible with the implementation of that
hardware.
It seems to me that the fix here is update the implementation,
usb-ohci.o
But I've done that now, so it's moot.
> > UHCI doesn't care since there is no per device state that is tracked.
> > The other HCDs do have some per device state and use the functionality.
> > Think of UHCI as the exception.
> >
> > The deallocate() callback is exactly what is needed. When all of the
> > references are gone, let the HCD free any private state for that device.
>
> The deallocate() callback only did "exactly what is needed" for a
> short while before that 2.3.x semantics change ... but that change
> rather signifcantly broke the only real user of that API.
The concept of the deallocate() callback is still correct today.
The implementation in usb-ohci.c isn't correct for that concept.
> > The HCDs shouldn't be unlinking any URBs (I don't know if that was
> > brought up, but has been brought up in the past). The driver that
> > submitted the URB should be the only code that unlinks them.
>
> Ignore the URB unlink problem ... think instead about QH unlinking.
>
> I know that for UHCI, URB == QH. (Not that it needs to be, but
> that's certainly a workable implementation approach.) But for the
> other controllers, even after every URB for a device is unlinked,
> one or more QHs can still be linked ... since the QHs are there
> for endpoints, not for URBs. Even after every URB is unlinked,
> the QH may still be in use by the hardware.
>
> So it's natural, coming from a UHCI perspective, to think that the
> only issue is making sure the URBs are unlinked. Not so, and that
> misnamed deallocate() routine was solving a different problem from
> the very beginning.
How so? The deallocate() routine is there for the HCD to free EDs and
QHs (in EHCI's version of QH).
> >>I'm still thinking through the urb module reference count stuff, and am
> >>not entirely convinced that things are currently safe without it, but
> >>other things have been distracting me...
> >
> > The URB reference counting may not be necessary. There's only 2 possible
> > pieces of code that can use it: the driver and the HCD.
> >
> > As long as the rules about who owns the URB when and where are clearly
> > defined (and implemented), then reference counting may not be necessary.
>
> That's pretty much where I'm coming from, FWIW. Not that it hurts
> to have a refcount, just minor overhead; and it can certainly help
> guard agains some classes of error. But "not necessary".
Yeah, I can't think of any reason why the URB reference count is
necessary right now.
Back to the patch I've attached. It's against 2.4.19-pre3 (I'll update
to Greg's CVS tree) and hasn't had the level of testing necessary to
call it good. I also want to recheck that I caught all of the necessary
paths.
While not ready to be included, I wanted to let everyone see it.
One benefit is that it results in a net code reduction.
usb-ohci.c | 72 +++++++++++--------------------------------------------------
usb-ohci.h | 58 +++++++++++++++++++++++++++++++------------------
2 files changed, 50 insertions(+), 80 deletions(-)
FWIW, we were hitting the BUG() call in sohci_free_dev().
Interestingly enough, the comment before sohci_free_dev is correct, but
the implementation doesn't follow those rules.
JE
Index: usb-ohci.c
===================================================================
retrieving revision 1.1
retrieving revision 1.2
diff -u -u -r1.1 -r1.2
--- usb-ohci.c 24 Jul 2002 21:29:04 -0000 1.1
+++ usb-ohci.c 12 Feb 2003 22:20:03 -0000 1.2
@@ -832,7 +832,7 @@
static int sohci_free_dev (struct usb_device * usb_dev)
{
unsigned long flags;
- int i, cnt = 0;
+ int i;
ed_t * ed;
struct ohci_device * dev = usb_to_ohci (usb_dev);
ohci_t * ohci = usb_dev->bus->hcpriv;
@@ -848,7 +848,7 @@
*/
spin_lock_irqsave (&usb_ed_lock, flags);
for(i = 0; i < NUM_EDS; i++) {
- ed = &(dev->ed[i]);
+ ed = dev->ed[i];
if (ed->state != ED_NEW) {
if (ed->state == ED_OPER) {
/* driver on that interface didn't unlink an
urb */
@@ -858,53 +858,14 @@
}
ep_rm_ed (usb_dev, ed);
ed->state = ED_DEL;
- cnt++;
- }
+ } else {
+ pci_pool_free(ohci->ed_cache, ed, ed->dma);
+ dev->ed[i] = NULL;
+ }
}
spin_unlock_irqrestore (&usb_ed_lock, flags);
-
- /* if the controller is running, tds for those unlinked
- * urbs get freed by dl_del_list at the next SF interrupt
- */
- if (cnt > 0) {
-
- if (ohci->disabled) {
- /* FIXME: Something like this should kick in,
- * though it's currently an exotic case ...
- * the controller won't ever be touching
- * these lists again!!
- dl_del_list (ohci,
- le16_to_cpu (ohci->hcca->frame_no) & 1);
- */
- warn ("TD leak, %d", cnt);
-
- } else if (!in_interrupt ()) {
- DECLARE_WAIT_QUEUE_HEAD (freedev_wakeup);
- DECLARE_WAITQUEUE (wait, current);
- int timeout = OHCI_UNLINK_TIMEOUT;
-
- /* SF interrupt handler calls dl_del_list */
- add_wait_queue (&freedev_wakeup, &wait);
- dev->wait = &freedev_wakeup;
- set_current_state(TASK_UNINTERRUPTIBLE);
- while (timeout && dev->ed_cnt)
- timeout = schedule_timeout (timeout);
- set_current_state(TASK_RUNNING);
- remove_wait_queue (&freedev_wakeup, &wait);
- if (dev->ed_cnt) {
- err ("free device %d timeout",
usb_dev->devnum);
- return -ETIMEDOUT;
- }
- } else {
- /* likely some interface's driver has a refcount bug */
- err ("bus %s devnum %d deletion in interrupt",
- ohci->ohci_dev->slot_name, usb_dev->devnum);
- BUG ();
- }
- }
}
- /* free device, and associated EDs */
dev_free (ohci, dev);
return 0;
@@ -1200,8 +1161,8 @@
spin_lock_irqsave (&usb_ed_lock, flags);
- ed = ed_ret = &(usb_to_ohci (usb_dev)->ed[(usb_pipeendpoint (pipe) << 1) |
- (usb_pipecontrol (pipe)? 0: usb_pipeout (pipe))]);
+ ed = ed_ret = usb_to_ohci (usb_dev)->ed[(usb_pipeendpoint (pipe) << 1) |
+ (usb_pipecontrol (pipe)? 0: usb_pipeout (pipe))];
if ((ed->state & ED_DEL) || (ed->state & ED_URB_DEL)) {
/* pending delete request */
@@ -1225,7 +1186,6 @@
ed->hwHeadP = ed->hwTailP;
ed->state = ED_UNLINK;
ed->type = usb_pipetype (pipe);
- usb_to_ohci (usb_dev)->ed_cnt++;
}
ohci->dev[usb_pipedevice (pipe)] = usb_dev;
@@ -1580,9 +1540,11 @@
spin_lock_irqsave (&usb_ed_lock, flags);
for (ed = ohci->ed_rm_list[frame]; ed != NULL; ed = ed->ed_rm_list) {
+ unsigned char type;
tdTailP = dma_to_td (ohci, le32_to_cpup (&ed->hwTailP) & 0xfffffff0);
tdHeadP = dma_to_td (ohci, le32_to_cpup (&ed->hwHeadP) & 0xfffffff0);
+
edINFO = le32_to_cpup (&ed->hwINFO);
td_p = &ed->hwHeadP;
@@ -1605,20 +1567,13 @@
}
}
+ type = ed->type;
if (ed->state & ED_DEL) { /* set by sohci_free_dev */
- struct ohci_device * dev = usb_to_ohci (ohci->dev[edINFO &
0x7F]);
td_free (ohci, tdTailP); /* free dummy td */
ed->hwINFO = cpu_to_le32 (OHCI_ED_SKIP);
ed->state = ED_NEW;
hash_free_ed(ohci, ed);
- /* if all eds are removed wake up sohci_free_dev */
- if (!--dev->ed_cnt) {
- wait_queue_head_t *wait_head = dev->wait;
-
- dev->wait = 0;
- if (wait_head)
- wake_up (wait_head);
- }
+ pci_pool_free(ohci->ed_cache, ed, ed->dma);
} else {
ed->state &= ~ED_URB_DEL;
tdHeadP = dma_to_td (ohci, le32_to_cpup (&ed->hwHeadP) &
0xfffffff0);
@@ -1630,12 +1585,11 @@
ed->hwINFO = cpu_to_le32 (OHCI_ED_SKIP);
ed->state = ED_NEW;
hash_free_ed(ohci, ed);
- --(usb_to_ohci (ohci->dev[edINFO & 0x7F]))->ed_cnt;
} else
ed->hwINFO &= ~cpu_to_le32 (OHCI_ED_SKIP);
}
- switch (ed->type) {
+ switch (type) {
case PIPE_CONTROL:
ctrl = 1;
break;
Index: usb-ohci.h
===================================================================
retrieving revision 1.1
retrieving revision 1.2
diff -u -u -r1.1 -r1.2
--- usb-ohci.h 24 Jul 2002 21:29:03 -0000 1.1
+++ usb-ohci.h 12 Feb 2003 22:20:04 -0000 1.2
@@ -405,7 +405,7 @@
struct pci_dev *ohci_dev;
u8 pci_latency;
struct pci_pool *td_cache;
- struct pci_pool *dev_cache;
+ struct pci_pool *ed_cache;
struct hash_list_t td_hash[TD_HASH_SIZE];
struct hash_list_t ed_hash[ED_HASH_SIZE];
@@ -414,10 +414,7 @@
#define NUM_EDS 32 /* num of preallocated endpoint descriptors */
struct ohci_device {
- ed_t ed[NUM_EDS];
- dma_addr_t dma;
- int ed_cnt;
- wait_queue_head_t * wait;
+ struct ed *ed[NUM_EDS];
};
// #define ohci_to_usb(ohci) ((ohci)->usb)
@@ -565,12 +562,12 @@
GFP_KERNEL | OHCI_MEM_FLAGS);
if (!ohci->td_cache)
return -ENOMEM;
- ohci->dev_cache = pci_pool_create ("ohci_dev", ohci->ohci_dev,
- sizeof (struct ohci_device),
+ ohci->ed_cache = pci_pool_create ("ohci_ed", ohci->ohci_dev,
+ sizeof (struct ed),
32 /* byte alignment */,
0 /* no page-crossing issues */,
GFP_KERNEL | OHCI_MEM_FLAGS);
- if (!ohci->dev_cache)
+ if (!ohci->ed_cache)
return -ENOMEM;
return 0;
}
@@ -581,9 +578,9 @@
pci_pool_destroy (ohci->td_cache);
ohci->td_cache = 0;
}
- if (ohci->dev_cache) {
- pci_pool_destroy (ohci->dev_cache);
- ohci->dev_cache = 0;
+ if (ohci->ed_cache) {
+ pci_pool_destroy (ohci->ed_cache);
+ ohci->ed_cache = 0;
}
}
@@ -621,23 +618,42 @@
{
dma_addr_t dma;
struct ohci_device *dev;
- int i, offset;
+ int i;
- dev = pci_pool_alloc (hc->dev_cache, mem_flags, &dma);
- if (dev) {
- memset (dev, 0, sizeof (*dev));
- dev->dma = dma;
- offset = ((char *)&dev->ed) - ((char *)dev);
- for (i = 0; i < NUM_EDS; i++, offset += sizeof dev->ed [0])
- dev->ed [i].dma = dma + offset;
- /* add to hashtable if used */
+ dev = kmalloc(sizeof(*dev), mem_flags);
+ if (!dev)
+ return NULL;
+ memset (dev, 0, sizeof (*dev));
+
+ for (i = 0; i < NUM_EDS; i++) {
+ struct ed *ed;
+
+ ed = pci_pool_alloc(hc->ed_cache, mem_flags, &dma);
+ if (!ed)
+ goto err;
+
+ dev->ed[i] = ed;
+
+ memset(ed, 0, sizeof(*ed));
+ ed->dma = dma;
}
+
return dev;
+
+err:
+ for (i = 0; i < NUM_EDS; i++) {
+ if (dev->ed[i]) {
+ pci_pool_free(hc->ed_cache, dev->ed[i], dev->ed[i]->dma);
+ dev->ed[i] = NULL;
+ }
+ }
+
+ return NULL;
}
static inline void
dev_free (struct ohci *hc, struct ohci_device *dev)
{
- pci_pool_free (hc->dev_cache, dev, dev->dma);
+ kfree(dev);
}
-------------------------------------------------------
This SF.net email is sponsored by: SlickEdit Inc. Develop an edge.
The most comprehensive and flexible code editor you can use.
Code faster. C/C++, C#, Java, HTML, XML, many more. FREE 30-Day Trial.
www.slickedit.com/sourceforge
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
