Re: Cardbus woes
[[ This is drifting over into -arch, so I'm ccing there and asking
people to redirect there. ]]
[[ The context is what to do about certain kinds of locking with
pccard code raising the bigger issue of what to do with interrupts
and sleepers in a hostile environemnt ]]
In message [EMAIL PROTECTED] "Justin T. Gibbs" writes:
: I take that back. In pccbb_detach, you need a mechanism to ensure
: that you are no-longer executing in your interrupt handler prior to
: destroying the softc.
There are two sitautions where you need to worry about things.
The first one is where your ISR is interrupted and the thing that
interrupts it wants to blow away the device. I think that the current
pccard (eg oldcard) code queues an soft interrupt to deal with this so
it can't interrupt things, but maybe I'm misremembering. Or maybe it
does it badly enough to cause problems (I know this is different than
the pccbb case, since that's newcard).
The second is where you have sleepers that need to "drain" before you
can delete the device.
The first one strikes me as a special case of the second one, but
maybe it needs to be handled specially.
At the BSDcon removable device meeting (which I missed because I found
out about it too late), the idea was that the detach routine would
sleep until all the sleepers had woken up and exited. Then the detach
routine would complete. It was suggested that some provision be made
for telling the device that this was a forced detach, but I don't
think that was nailed down. I think that having the detach routine
call the bridge and ask it if the hardware is still there would come
close to answering this question. When the hardware is gone, detach
cannot return EBUSY and have that be meaningful. The hardware is
gone. Who cares if it is busy, it has to cope with it being ripped
untimely from the driver's grasp. Gone is gone :-).
One way to deal with this would be to have all devices that sleep do a
ref/deref on the device around the sleep. There would be an implicit
ref/deref in the ISR too. However, that strikes me as unworkable.
There are a lot of drivers that do a lot of sleeping and when they
wake up. This would require a massive pass through the entire device
driver tree (well, at least for the ones that can be removed). I also
don't like the idea of doing boatloads of locking/unlocking code on
every single interrupt, but maybe that's unavoidable.
One way around the interrupt context problem is to ensure that devices
are never removed from an interrupt context. Or are at least never
detached from an interrupt context. However, I'm not sure the concept
of an interrupt context still has meaning in the new threads for ISR.
Can an interrupt thread block and allow other non-interrupt threads to
run?
Warner
P.S. Here's two interfaces that I'd like to add to the bus interface
file. They try to implement part of the removable feature. They are
based on my talking to phk after the arch meeting I missed. I don't
see any other way to ensure that interrupts will terminate.
#
# Ask the bridge if the device is still there. If the bus doesn't
# support removal, then it should always return true. If the bus
# does support removal, this method must return the real status of
# the child. Generally this is done by asking the bridge chip to
# see if card is present or not.
#
# Drivers may call this routine from an interrupt context. Drivers
# may assume this call is relatively inexpensive. Bridge drivers
# that implement this call should endeavor to make this inexpensive.
#
METHOD int child_present {
device_tdev;
device_tchild;
} DEFAULT bus_generic_child_present;
#
# Ask the bridge if this instance of a device can be removed. Drivers
# may call this at any time. The bus code must guarantee that the
# value returned from this function is invariant on a per instance
# basis so drivers may cache the results.
#
# Drivers do not necessarily know that a bus supports device removal
# based on the attachment. PCI has both hot-plug and non-hot-plug
# variants, for example.
#
METHOD int is_removable {
device_tdev;
device_tchild;
} DEFAULT bus_generic_is_removable;
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
I was mostly trying to push the locks down so that they weren't held during attach and remove. I was using them to protect mostly the kthread state flag in the softc, as well as other variables in the softc. Comments? It seems to me that having a single thread per-softc gives you this protection without requiring any locks. Other than having to aquire Giant at thread starup (as most code below us is not thread safe yet), I don't see any other locking requirements. -- Justin To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
It seems to me that having a single thread per-softc gives you this protection without requiring any locks. Other than having to aquire Giant at thread starup (as most code below us is not thread safe yet), I don't see any other locking requirements. I take that back. In pccbb_detach, you need a mechanism to ensure that you are no-longer executing in your interrupt handler prior to destroying the softc. What you really want on detach (not only for pccbb, but in general), is a way to kill your ithread via bus_teardown_intr. Since we only provide a single ithread per shareable interrupt source, this wouldn't quite work, but you get the general idea. Can anyone think of another general "synchronization method" for a case where the second event may never happen again? Even if you check in the detach routine to see if an interrupt is pending in hardware for your device, this won't tell you if your interrupt thread has already been dispatched but the interrupt source has gone away (card was removed, so interrupt line is dead). Hmmm. -- Justin To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
On Thu, Dec 14, 2000 at 12:47:51PM -0800, John Baldwin wrote: In other news, while wandering through the cardbus code, I discovered that pccbb softc's have an internal mutex much to my surprise, and that they weren't quite being used properly AFAICT. In the pccb0 kthread, they were acquired/released without Giant. However, in the rest of the pccbb driver, they are acquired/released with Giant. This leads to lock order violations which could potentially deadlock the machine at some point. Also, the pccbb0 kthread holds the mutex across the entire card insertion and removal events, which is not quite right. Mutexes should only be held for short periods of time. As such, I've futzed around with the mutex operations in the pccbb driver some. It may not be completely correct, but I think it is an improvement. The patch for this can be found at http://www.FreeBSD.org/~jhb/patches/pccbb.patch. [I just now recalled your mentioning this a while back, so here's my much delayed response] Originally I had write the code with spl's, and at one point decided to convert them to mutex after finding spl's don't do anything. However, I don't claim to know a whole lot about mutexes or FreeBSD's implementation of it. Perhaps someone can point me to the Great Definitive Source? The first thing I'm unclear about is the deal with Giant. My understanding is that Giant is something one would acquire if he wanted to be really exclusive, or something like that. John - you mentioned I was acquiring/releasing with/without Giant - but I don't see how I'm doing things differently in and out of the kthread. The only place I mentioned Giant was right before kthread_exit, and that's necessary least kthread_exit calls panic() on me. On your patch, you acquire Giant at the start of the kthread - wouldn't that hold onto the lock until the thread exits, and make other things attempting to lock Giant block? As for the 'holding onto mutex for a long time' problem, I'm not sure if your solution is best. I originally put in the locks firstly because that's how NetBSD had it, and secondly so that card removal interrupts while card is still attaching wouldn't be lost. Your patched version allows for wakeup() to be called while insertion/removal is being processed. I've glanced through the code and didn't see anything that requires mutual exclusivity between the kthread the the interrupt handler, so perhaps a better solution would be to simply not use mutex at all? Comments? -- (o_ 1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2-1-2 _o) \\\_\Jonathan Chen [EMAIL PROTECTED] /_/// ) No electrons were harmed during production of this message ( ~ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
On 27-Dec-00 Jonathan Chen wrote: On Thu, Dec 14, 2000 at 12:47:51PM -0800, John Baldwin wrote: In other news, while wandering through the cardbus code, I discovered that pccbb softc's have an internal mutex much to my surprise, and that they weren't quite being used properly AFAICT. In the pccb0 kthread, they were acquired/released without Giant. However, in the rest of the pccbb driver, they are acquired/released with Giant. This leads to lock order violations which could potentially deadlock the machine at some point. Also, the pccbb0 kthread holds the mutex across the entire card insertion and removal events, which is not quite right. Mutexes should only be held for short periods of time. As such, I've futzed around with the mutex operations in the pccbb driver some. It may not be completely correct, but I think it is an improvement. The patch for this can be found at http://www.FreeBSD.org/~jhb/patches/pccbb.patch. [I just now recalled your mentioning this a while back, so here's my much delayed response] Originally I had write the code with spl's, and at one point decided to convert them to mutex after finding spl's don't do anything. However, I don't claim to know a whole lot about mutexes or FreeBSD's implementation of it. Perhaps someone can point me to the Great Definitive Source? There isn't one yet. We are still getting some of the internals done to make mutexes in drivers useful. :) For now all drivers still run under Giant. The first thing I'm unclear about is the deal with Giant. My understanding is that Giant is something one would acquire if he wanted to be really exclusive, or something like that. John - you mentioned I was acquiring/releasing with/without Giant - but I don't see how I'm doing things differently in and out of the kthread. The only place I mentioned Giant was right before kthread_exit, and that's necessary least kthread_exit calls panic() on me. On your patch, you acquire Giant at the start of the kthread - wouldn't that hold onto the lock until the thread exits, and make other things attempting to lock Giant block? Right now Giant is gotten on nearly every entry into the kernel (fast interrupt handlers don't grab it and INTR_MPSAFE interrupt threads don't either.) Basically, as a mutex it protects everything in the kernel. Thus, it is only safe to run w/o holding Giant if _all_ of the data structures you are touching are protected somehow. In the kthread, you didn't acquire Giant, so all of the data structures it touched (for example, in other driver attach and probes) weren't protected at all. Thus, cpu A could be running the pccbb thread doing an attach, and cpu B could be doing something else on the same driver and they would happily corrupt each other, crash teh system, etc. When your thread sleeps, it gives Giant up, so it won't block the system indefinitely. (Giant is special with regards to tsleep(9) in that case.) Other kthreads grab Giant when they first start up, see the aio daemon code for example. As for the 'holding onto mutex for a long time' problem, I'm not sure if your solution is best. I originally put in the locks firstly because that's how NetBSD had it, and secondly so that card removal interrupts while card is still attaching wouldn't be lost. Your patched version allows for wakeup() to be called while insertion/removal is being processed. I've glanced through the code and didn't see anything that requires mutual exclusivity between the kthread the the interrupt handler, so perhaps a better solution would be to simply not use mutex at all? I was mostly trying to push the locks down so that they weren't held during attach and remove. I was using them to protect mostly the kthread state flag in the softc, as well as other variables in the softc. Comments? -- John Baldwin [EMAIL PROTECTED] -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
: http://www.FreeBSD.org/~jhb/patches/pccbb.patch. I took a look a this patch, and saw nothing obviously wrong with it. Mike Smith has some uncommitted patches that might amke the resource issue better, but not completely if I'm reading them right (they are extensive, so I might not be). Justin Gibbs has also said that he's working on a rework of cardbus to factor things better. I don't think these patches will conflict with either of these efforts. Finally, can we talk about pccard/cardbus issues and changes in mobile@ rather than current@? Its charter covers this more closely. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
With the recent slaughter^Wrework of the PCI code, my hack to allocate resources for unattached PCI devices in pci_probe_nomatch() no longer works. The updated patch can be found at http://www.FreeBSD.org/~jhb/patches/cardbus.hack.patch. I have a set of working patches at http://ziplok.dyndns.org/msmith/pci.diff which handle resource reservation, making your hack unnecessary. They're not ready for commit yet, but they're known to work (assuming you get this message 8). -- ... every activity meets with opposition, everyone who acts has his rivals and unfortunately opponents also. But not because people want to be opponents, rather because the tasks and relationships force people to take different points of view. [Dr. Fritz Todt] V I C T O R Y N O T V E N G E A N C E To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
On 14-Dec-00 Mike Smith wrote: With the recent slaughter^Wrework of the PCI code, my hack to allocate resources for unattached PCI devices in pci_probe_nomatch() no longer works. The updated patch can be found at http://www.FreeBSD.org/~jhb/patches/cardbus.hack.patch. I have a set of working patches at http://ziplok.dyndns.org/msmith/pci.diff which handle resource reservation, making your hack unnecessary. They're not ready for commit yet, but they're known to work (assuming you get this message 8). Cool, my hack is all b0rked anyways, so I'll try this out. -- John Baldwin [EMAIL PROTECTED] -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
On 15-Dec-00 John Baldwin wrote: On 14-Dec-00 Mike Smith wrote: With the recent slaughter^Wrework of the PCI code, my hack to allocate resources for unattached PCI devices in pci_probe_nomatch() no longer works. The updated patch can be found at http://www.FreeBSD.org/~jhb/patches/cardbus.hack.patch. I have a set of working patches at http://ziplok.dyndns.org/msmith/pci.diff which handle resource reservation, making your hack unnecessary. They're not ready for commit yet, but they're known to work (assuming you get this message 8). Cool, my hack is all b0rked anyways, so I'll try this out. No go. :( With this, my soundcard no longer probes properly: pcm0: ESS Technology Maestro-2E port 0x1400-0x14ff irq 5 at device 8.0 on"pci0 pcm0: unable to map register space device_probe_and_attach: pcm0 attach returned 6 Also, the cardbus controller seems to not be getting interrupts, as it doesn't notice when a card is inserted and removed anymore. pccard seems to work, except that the beeping is gone (this may be due to pcm0 being hosed though). -- John Baldwin [EMAIL PROTECTED] -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
In message [EMAIL PROTECTED] John Baldwin writes: : Also, the cardbus controller seems to not be getting interrupts, as it doesn't : notice when a card is inserted and removed anymore. pccard seems to work, : except that the beeping is gone (this may be due to pcm0 being hosed though). beeping being gone has nothing to do with pcm0 as beeping doesn't use that. Warner To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
On 15-Dec-00 Warner Losh wrote: In message [EMAIL PROTECTED] John Baldwin writes: : Also, the cardbus controller seems to not be getting interrupts, as it : doesn't : notice when a card is inserted and removed anymore. pccard seems to work, : except that the beeping is gone (this may be due to pcm0 being hosed : though). beeping being gone has nothing to do with pcm0 as beeping doesn't use that. Well, except that the sound output for the little PC speaker is wired through my soundcard. If the soundcard is part way initialized and has the volume muted before it dies, then I won't hear any beeps from the PC speaker. Warner -- John Baldwin [EMAIL PROTECTED] -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.Baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Cardbus woes
I have a set of working patches at http://ziplok.dyndns.org/msmith/pci.diff which handle resource reservation, making your hack unnecessary. They're not ready for commit yet, but they're known to work (assuming you get this message 8). Cool, my hack is all b0rked anyways, so I'll try this out. No go. :( With this, my soundcard no longer probes properly: Verbose boot output, please? -- ... every activity meets with opposition, everyone who acts has his rivals and unfortunately opponents also. But not because people want to be opponents, rather because the tasks and relationships force people to take different points of view. [Dr. Fritz Todt] V I C T O R Y N O T V E N G E A N C E To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
