On Mon, May 13, 2002, David Brownell <[EMAIL PROTECTED]> wrote:
> > > > Because I said reference counting and you went out and found one special
> > > > case which you think immediately proves your point.
> > >
> > > No, I was talking about device management. You're the one who
> > > keeps ignoring such issues, and wants to talk refcounting instead.
> > >
> > > If you're interested, your most annoying (to me) "debate" tactic in this
> > > whole damn thread is consistently ignoring or mis-representing what
> > > I've said in this specific area.
> >
> > To be honest, that sounds like the pot calling the kettle black.
>
> Remember that this particular subthread started with me making
> a point about how the device management issues are rather
> distinct from the memory management issues.
I agree that they are seperate and I very much am aware of the issues.
Take a look at uhci.c, it has the same problems.
This subthread started because you wanted to call ->deallocate() from
usb_disconnect. However, that's merely because you want to keep the code
in sohci_free_dev and not change it. In that case, ->deallocate() needs
to be called from an interrupt context because of the way that
sohci_free_dev works.
You originally wrote:
"The simple fix is to move the call to
dev->bus->deallocate(dev)
from the memory management stuff (usb_free_dev) to the
device management, at the very end of usb_disconnect().
Then dev->bus should be nulled."
I asked how you viewed memory management to be different from device
management:
"Could you explain to me the difference between memory management and
device management in the example of usb-ohci?"
You explained ohci-hcd like this:
""Device Management" is where state associated with the
device's current configuration needs get cleaned up. The
stuff coupled to the "real hardware" which just went away.
-> EDs for that device that are still linked ... need to
block in the driver until we know the HC isn't using
these EDS any longer
-> any TDs on such EDs (reflecting driver bugs FWIW,
they should all have been unlinked already) must be
scrubbed out.
-> then free that ED/TD memory once the HC is known
not to be using it any more."
My response was that you're focus for the driver was wrong. You should
be targeting for a working system, not a buggy one.
Since I didn't elborate on each of these points before, I'll do so now,
in order:
- ED's will have already been started being unlinked by now.
(sohci_unlink_urb -> ep_rm_ed). The only thing that's waiting on the
ED's to be finished being unlinked is the memory the ED's sit in.
(part of struct ohci_device)
- Irrelevant. We're coding for a working system, not a buggy one.
- This is the important part. This needs to come after the ED is done
being unlinked.
So, everything boils down to the ED's. They get allocated at device
connection time and are shared by the HC and the HCD.
Howver, that doesn't mean we can't do it asynchronously. In fact, it's
already done asynchronously. ep_rm_ed queues it up and dl_del_list does
the actual unlinking.
We can easily add a field to struct ochi_ed to point to the struct
ohci_device that it's a part of, or calculate it using the pointer
value. I'll assume we add the pointer for now.
When we start using an ED in ep_add_ed, we call usb_dev_get(dev), and
fill in ep->dev. When we finish removing the device in dl_del_list, we
call usb_dev_put(ep->dev) and then null out ep->dev.
You leave the pci_pool_free (the dev_free inline function actually) for
sohci_free_dev. It will only get called when all of the references are
finished (including all of the ED's for that device) so it's all safe.
You no longer require deallocate() to be called in a thread context and
we get rid of much of the code in sohci_free_dev.
You also said one thing of interest I originally overlooked (my
apologies):
"And before its device address can be reused on that bus, FWIW ... not
that anyone is likely to disable the round-robin addressing lately."
That is incorrect. You've already disabled the EP's and TD's, so
they're not being used anymore. You can have an old one around still
while a new gets added because the old one is disabled.
> That's much the same class of point the coding style document
> makes about locking and reference counting -- that they're two
> distinct issues. Likewise, refcounting and device management
> are two distinct issues. (Sort of like getting rid of locks held by
> the hardware itself.)
While they are seperate issues, they are related. Obviously memory
management has to be tied to device management or we'll just leak
memory when a device is released.
However, we don't have to release that memory immediately when a device
is removed.
We only need to release the memory when everyone is done with it.
> That's why I think a snide pot/kettle/black type of comment is
> especially inappropriate for in this case. All along, you were not
> being responsive to this particular issue.
Snide? It's very much true.
> Today is about the first time I've seen you actually seem to respond
> to it
Before today we were discussing if we should change reference counting.
Now that it's been decided that it will be changed, we're discussing
what else should be changed since some drivers have bugs as a result of
the change.
I spent some time trying to keep the conversations on topic and if that
meant focusing on the real topic at hand, then I did.
> although "do it like uhci instead" isn't exactly aiming for common ground.
I think think aiming for common ground is required. If there are 2
solutions and one is better, then it should be used without compromising
it.
I'm not even sure if compromising is always possible given 2 solutions.
> > Seriously tho, you keep ignoring what I've been saying, and I don't know
> > why.
>
> Could you give a point of yours that I've _ignored_ rather than
> just not agreeing with? (I know I've pointed out this one issue more
> than once, including your non-responses.)
This whole thread for instance. You keep arguing that sohci_free_dev
should be that complicated.
However I keep pointing to uhci.c since it has pretty much the same
problems. It's an example of how you can do this better.
I haven't seen very many arguments refuting it, you just keep saying
things like "QH != dev", which are patently obvious and don't actually
address the fact I've solved the problems with uhci.c.
You did just now say "Lots of implicit state stashed in the HCD. Not a
style I like." which actually has some substance.
Although I think it doesn't really apply since it's all of like 3 lines
more code, when you can remove some 40 lines in sohci_free_dev.
> One that's significant to the main issues, as opposed to one where
> a response would just make more clutter in mailboxes. And ideally
> one where it'd be clear I was "ignoring" the issue.
Like the one above?
> > So I'm going and fixing the ohci and ehci drivers. I'll send a patch
> > when I get the chance to test it (not just compile it).
>
> Don't bother until you can actually indicate some real problem in
> the 2.5.15 code. So far all you've persuaded me of is that you'll go
> to substantial lengths to disagree with me. (If you didn't want to
> come off like that, see above.)
The problem is that it's more complicated than it needs to be. It can be
very elegant while being 100% correct logic too all in less code I bet
too.
> Perhaps I should just fix the uhci code too, eh? I'm sure that'd
> be equallly well received.
If you can make it better, by all means, I'll apply it if I think it is.
Even if I don't and you're sure it's better, it's still worth the effort
since there are previous examples of a maintainer not liking a change and
someone higher up (like Greg, Linus, etc would be in this case) applied
it because it was better.
> > > The code to which that comment applies is way at the end of
> > > that routine, after all the hardware (device management) hooks
> > > (such as net->stop) I was talking about were called.
> >
> > I don't see where net->stop is, do you mean dev->uninit or something?
>
> See the very first function called by unregister_netdevice().
dev_close which calls dev->stop. It appears that dev->stop is intended
to stop the device, bring it to the "down" state.
I don't see where any comments or any code require it to cleanup
everything immediately however. In fact, the comments in
unregister_net_device seem to imply otherwise.
It looks like it serves the purpose of ->disconnect() in the USB code.
It just needs to start stopping the device. It can take a while if it
wants to or it can finish immediately and finish cleaning up
asynchronously.
It doesn't seem any way analogous to the callback the HCD's get when
devices are removed however.
I can't really think of anything analogous right now. Maybe the SCSI
layer?
Anyway, all of this boils down to, been there, done that. I've had the
same problems with uhci.c and I've solved them and it resulted in less
code.
JE
_______________________________________________________________
Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: [EMAIL PROTECTED]
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
