API change for bus_dma

[John Baldwin]

On 21-Jun-2003 Scott Long wrote:
> All,
> 
> As I work towards locking down storage drivers, I'm also preening their
> use of busdma.  A common theme among them is a misuse of
> bus_dmamap_load() and the associated callback mechanism.  For most, the
> consequence is harmless as long as the card can support the amount of
> physical memory in the system (systems with IOMMU's not withstanding).
> However, in cases such as PAE where busdma might have to use bounce
> buffers, most drivers don't handle the possibility of bus_dmamap_load()
> returning EINPROGRESS.  The consequence of this is twofold:
> bus_dmamap_load() returns without the callback being called, but the
> driver doesn't detect this and merrily goes on its way.  Later on the
> callback does get called, and any state that was shared with it gets
> corrupted.  This is a problem even for drivers that are under Giant.
> 
> The solution for this is mostly a mechanical cut-n-paste of the code
> dealing with the callback.  However, locking down the drivers presents
> a new problem with the callback.  Since the callback can be called
> asynchronously from an SWI, it needs some way to synchronize with the
> driver.  Adding code to each callback to conditionally grab the driver
> mutex incurs a penalty (albiet small) and requires more effort.  The
> better solution is to export the driver mutex to busdma and have the
> SWI that runs the callback lock the mutex before calling the callback.

Erm, what's wrong with this:

void
foo_function()
{
        mtx_assert(&mylock, MA_OWNED);
        ...
}

void
foo_callback()
{
        mtx_lock(&mylock);
        foo_function();
        mtx_unlock(&mylock);
}

?

Using this approach is more flexible in case there is a driver
that uses a sx lock or a (not yet implemented) reader-writer lock
or a critical section, or whatever.  This just means that the
callback uses a wrapper function but that really isn't that hard to
do and there are other cases (callouts in general) that need this.
To me this seems to be adding a special case to the API that won't
work for all situations anyways.  I also don't see wrapper functions
as being all that hard.

-- 

John Baldwin <jhb at FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve!"  -  http://www.FreeBSD.org/