Wrapper API for static bus_dma allocations

[John Baldwin]

The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it to get
the bus address. Similarly, freeing it also requires several steps. In
addition, some of the steps are a bit subtle (e.g. the need to check for an
error in the bus_dma callback instead of from bus_dmamap_load()) and not all
drivers get those correct.

To try to make this simpler I've written a little wrapper API that tries to
provide a single call to allocate a buffer and a single call to free a buffer.
Each buffer is described by a structure defined by the API, and if the call to
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the buffer.

In the interests of simplicity, this API does not allow the buffer to be quite
as fully configured as the underlying bus_dma API, instead it aims to handle
the most common cases that are used in most drivers. As such, it assumes that
the buffer must be one contiguous range of DMA address space, and the only
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and the
flags parameter that is normally passed to bus_dmamem_alloc(). I believe that
this should be sufficient to cover the vast majority of the drivers in our
tree.

I've included below a patch that contains the wrapper API along with a sample
conversion of the ndis driver (chosen at random). If folks like this idea I
will update the patch to include manpage changes as well.

--- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
+++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
@@ -186,7 +186,6 @@
static ndis_status NdisMAllocateMapRegisters(ndis_handle,
uint32_t, uint8_t, uint32_t, uint32_t);
static void NdisMFreeMapRegisters(ndis_handle);
-static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
uint8_t, void **, ndis_physaddr *);
static void ndis_asyncmem_complete(device_object *, void *);
@@ -1387,23 +1386,6 @@
bus_dma_tag_destroy(sc->ndis_mtag);
}

-static void
-ndis_mapshared_cb(arg, segs, nseg, error)
- void *arg;
- bus_dma_segment_t *segs;
- int nseg;
- int error;
-{
- ndis_physaddr *p;
-
- if (error || nseg > 1)
- return;
-
- p = arg;
-
- p->np_quad = segs[0].ds_addr;
-}
-
/*
* This maps to bus_dmamem_alloc().
*/
@@ -1443,35 +1425,17 @@
* than 1GB of physical memory.
*/

- error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
- 0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
- NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
- &sh->ndis_stag);
+ error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
+ NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO);

if (error) {
free(sh, M_DEVBUF);
return;
}

- error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
- BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
-
- if (error) {
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
+ *vaddr = sh->ndis_mem.dma_vaddr;
+ paddr->np_quad = sh->ndis_mem.dma_baddr;

- error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
- len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
-
- if (error) {
- bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
-
/*
* Save the physical address along with the source address.
* The AirGo MIMO driver will call NdisMFreeSharedMemory()
@@ -1482,8 +1446,6 @@
*/

NDIS_LOCK(sc);
- sh->ndis_paddr.np_quad = paddr->np_quad;
- sh->ndis_saddr = *vaddr;
InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
NDIS_UNLOCK(sc);
}
@@ -1581,13 +1543,13 @@
l = sc->ndis_shlist.nle_flink;
while (l != &sc->ndis_shlist) {
sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
- if (sh->ndis_saddr == vaddr)
+ if (sh->ndis_mem.dma_vaddr == vaddr)
break;
/*
* Check the physaddr too, just in case the driver lied
* about the virtual address.
*/
- if (sh->ndis_paddr.np_quad == paddr.np_quad)
+ if (sh->ndis_mem.dma_baddr == paddr.np_quad)
break;
l = l->nle_flink;
}
@@ -1604,9 +1566,7 @@

NDIS_UNLOCK(sc);

- bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
- bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
+ bus_dma_mem_free(&sh->ndis_mem);

free(sh, M_DEVBUF);
}
--- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
+++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
@@ -66,10 +66,7 @@

struct ndis_shmem {
list_entry ndis_list;
- bus_dma_tag_t ndis_stag;
- bus_dmamap_t ndis_smap;
- void *ndis_saddr;
- ndis_physaddr ndis_paddr;
+ struct bus_dmamem ndis_mem;
};

struct ndis_cfglist {
--- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
+++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
@@ -540,3 +540,66 @@

return (0);
}
+
+struct bus_dma_mem_cb_data {
+ struct bus_dmamem *mem;
+ int error;
+};
+
+static void
+bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
+{
+ struct bus_dma_mem_cb_data *d;
+
+ d = arg;
+ d->error = error;
+ if (error)
+ return;
+ d->mem->dma_baddr = segs[0].ds_addr;
+}
+
+int
+bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
+{
+ struct bus_dma_mem_cb_data d;
+ int error;
+
+ bzero(mem, sizeof(*mem));
+ error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
+ BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
+ &mem->dma_tag);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
+ &mem->dma_map);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ d.mem = mem;
+ error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
+ bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
+ if (error == 0)
+ error = d.error;
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ return (0);
+}
+
+void
+bus_dma_mem_free(struct bus_dmamem *mem)
+{
+
+ if (mem->dma_baddr != 0)
+ bus_dmamap_unload(mem->dma_tag, mem->dma_map);
+ if (mem->dma_vaddr != NULL)
+ bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map);
+ if (mem->dma_tag != NULL)
+ bus_dma_tag_destroy(mem->dma_tag);
+ bzero(mem, sizeof(*mem));
+}
--- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
+++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
@@ -337,4 +337,29 @@

#endif /* __sparc64__ */

+/*
+ * A wrapper API to simplify management of static mappings.
+ */
+
+struct bus_dmamem {
+ bus_dma_tag_t dma_tag;
+ bus_dmamap_t dma_map;
+ void *dma_vaddr;
+ bus_addr_t dma_baddr;
+};
+
+/*
+ * Allocate a mapping. On success, zero is returned and the 'dma_vaddr'
+ * and 'dma_baddr' fields are populated with the virtual and bus addresses,
+ * respectively, of the mapping.
+ */
+int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr,
+ bus_size_t len, int flags);
+
+/*
+ * Release a mapping created by bus_dma_mem_create().
+ */
+void bus_dma_mem_free(struct bus_dmamem *mem);
+
#endif /* _BUS_DMA_H_ */

[Poul-Henning Kamp]

--------

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

[John Baldwin]

Post by Poul-Henning Kamp
--------

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

Given the amount of oddball hardware out there I don't think there is a
lot you can cut out. The filter function might be something we can lose
(and losing it would simplify the implementation), but all the other
weird constraints are actually used by something AFAIK. I do think we
can provide some simpler wrappers for some of the more common cases, but
there will be some hardware for which those wrappers do not work.

One suggestion Scott has had is to at least make it easier to extend the
API by using getter/setter routines on the tag to work with tag
attributes instead of passing them all in bus_dma_tag_create().

[Konstantin Belousov]

Post by John Baldwin

Post by Poul-Henning Kamp
--------

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

Given the amount of oddball hardware out there I don't think there is a
lot you can cut out. The filter function might be something we can lose
(and losing it would simplify the implementation), but all the other
weird constraints are actually used by something AFAIK. I do think we
can provide some simpler wrappers for some of the more common cases, but
there will be some hardware for which those wrappers do not work.
One suggestion Scott has had is to at least make it easier to extend the
API by using getter/setter routines on the tag to work with tag
attributes instead of passing them all in bus_dma_tag_create().

BTW, filter function is useless. It can deny specific bus address from
being used, but it does not provide the busdma implementation even a hint
what other address should be (tried to) used. In dmar busdma, I simply
ignored it. And there is no real users of filter in the tree.

[John Baldwin]

Post by Konstantin Belousov

Post by John Baldwin

Post by Poul-Henning Kamp
--------

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

Given the amount of oddball hardware out there I don't think there is a
lot you can cut out. The filter function might be something we can lose
(and losing it would simplify the implementation), but all the other
weird constraints are actually used by something AFAIK. I do think we
can provide some simpler wrappers for some of the more common cases, but
there will be some hardware for which those wrappers do not work.
One suggestion Scott has had is to at least make it easier to extend the
API by using getter/setter routines on the tag to work with tag
attributes instead of passing them all in bus_dma_tag_create().

BTW, filter function is useless. It can deny specific bus address from
being used, but it does not provide the busdma implementation even a hint
what other address should be (tried to) used. In dmar busdma, I simply
ignored it. And there is no real users of filter in the tree.

Yes, it is very annoying. I think some old ISA SCSI HBA driver might have
used it to skip over some low-memory hole (i.e. there were two valid DMA
ranges and this was the kludge instead of having two sets of lowaddr/highaddr
exclusions). (That is one part of the API we could rototill is to just remove
the highaddr arg just use a single arg which is effectively lowaddr. I think
all drivers always set highaddr to BUS_SPACE_MAXADDR.)

[Warner Losh]

Post by John Baldwin

Post by Konstantin Belousov

Post by John Baldwin

Post by Poul-Henning Kamp
--------

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

Given the amount of oddball hardware out there I don't think there is a
lot you can cut out. The filter function might be something we can lose
(and losing it would simplify the implementation), but all the other
weird constraints are actually used by something AFAIK. I do think we
can provide some simpler wrappers for some of the more common cases, but
there will be some hardware for which those wrappers do not work.
One suggestion Scott has had is to at least make it easier to extend the
API by using getter/setter routines on the tag to work with tag
attributes instead of passing them all in bus_dma_tag_create().

BTW, filter function is useless. It can deny specific bus address from
being used, but it does not provide the busdma implementation even a hint
what other address should be (tried to) used. In dmar busdma, I simply
ignored it. And there is no real users of filter in the tree.

Yes, it is very annoying. I think some old ISA SCSI HBA driver might have
used it to skip over some low-memory hole (i.e. there were two valid DMA
ranges and this was the kludge instead of having two sets of lowaddr/highaddr
exclusions). (That is one part of the API we could rototill is to just remove
the highaddr arg just use a single arg which is effectively lowaddr. I think
all drivers always set highaddr to BUS_SPACE_MAXADDR.)

Not all. There’s some PCI cards that can’t do 64-bit cycles that pass in the 32-bit
value on 64-bit systems. There’s 386 instances of this in the tree. But that may be
lowaddr only. It’s hard to grep for this to be sure.

Wanrer

[John Baldwin]

Post by Warner Losh

Post by John Baldwin

Post by Konstantin Belousov

Post by John Baldwin

Post by Poul-Henning Kamp
--------

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse [...]

Isn't it time we take a good hard stare at all of the bus_dma API,
and refactor it into something a lot more compact ?

Given the amount of oddball hardware out there I don't think there is a
lot you can cut out. The filter function might be something we can lose
(and losing it would simplify the implementation), but all the other
weird constraints are actually used by something AFAIK. I do think we
can provide some simpler wrappers for some of the more common cases, but
there will be some hardware for which those wrappers do not work.
One suggestion Scott has had is to at least make it easier to extend the
API by using getter/setter routines on the tag to work with tag
attributes instead of passing them all in bus_dma_tag_create().

BTW, filter function is useless. It can deny specific bus address from
being used, but it does not provide the busdma implementation even a hint
what other address should be (tried to) used. In dmar busdma, I simply
ignored it. And there is no real users of filter in the tree.

Yes, it is very annoying. I think some old ISA SCSI HBA driver might have
used it to skip over some low-memory hole (i.e. there were two valid DMA
ranges and this was the kludge instead of having two sets of
lowaddr/highaddr exclusions). (That is one part of the API we could
rototill is to just remove the highaddr arg just use a single arg which
is effectively lowaddr. I think all drivers always set highaddr to
BUS_SPACE_MAXADDR.)

Not all. There’s some PCI cards that can’t do 64-bit cycles that pass in the
32-bit value on 64-bit systems. There’s 386 instances of this in the tree.
But that may be lowaddr only. It’s hard to grep for this to be sure.

That is lowaddr only, not the filter callback.

However, even if we remove the filter and highaddr arguments from tags, you
are still stuck with creating a tag, allocating memory, and loading it to get
a bus address (and tracking the associated pointers, etc.). I still think a
wrapper API for the common case (static DMA allocations) would be useful.

Orthogonally I can explore removing the filter along with highaddr (it is
always BUS_SPACE_MAXADDR).

[Ian Lepore]

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it to get
the bus address. Similarly, freeing it also requires several steps. In
addition, some of the steps are a bit subtle (e.g. the need to check for an
error in the bus_dma callback instead of from bus_dmamap_load()) and not all
drivers get those correct.
To try to make this simpler I've written a little wrapper API that tries to
provide a single call to allocate a buffer and a single call to free a buffer.
Each buffer is described by a structure defined by the API, and if the call to
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the buffer.
In the interests of simplicity, this API does not allow the buffer to be quite
as fully configured as the underlying bus_dma API, instead it aims to handle
the most common cases that are used in most drivers. As such, it assumes that
the buffer must be one contiguous range of DMA address space, and the only
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and the
flags parameter that is normally passed to bus_dmamem_alloc(). I believe that
this should be sufficient to cover the vast majority of the drivers in our
tree.
I've included below a patch that contains the wrapper API along with a sample
conversion of the ndis driver (chosen at random). If folks like this idea I
will update the patch to include manpage changes as well.
--- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
+++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c

[...]

Post by John Baldwin
+struct bus_dmamem {
+ bus_dma_tag_t dma_tag;
+ bus_dmamap_t dma_map;
+ void *dma_vaddr;
+ bus_addr_t dma_baddr;
+};
+
+/*
+ * Allocate a mapping. On success, zero is returned and the 'dma_vaddr'
+ * and 'dma_baddr' fields are populated with the virtual and bus addresses,
+ * respectively, of the mapping.
+ */
+int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr,
+ bus_size_t len, int flags);
+
+/*
+ * Release a mapping created by bus_dma_mem_create().
+ */
+void bus_dma_mem_free(struct bus_dmamem *mem);
+
#endif /* _BUS_DMA_H_ */

You introduce new functions named bus_dma_mem_xxxx(), and a new
structure bus_dmamem. It's already hard enough to remember the oddball
mix of underbars in busdma naming, can the new struct be bus_dma_mem?

Also, the combo of create/free is most unsatisfying. Typically
alloc/free come in pairs, as do create/destroy. These pairings are used
pretty consistantly in busdma now, it'd be nice to not have to remember
this pair is an exception. (I'm agnostic about whether the new
functionality should be alloc/delete or create/destroy.)

-- Ian

[Ian Lepore]

On Thu, 2015-01-29 at 15:37 -0500, John Baldwin wrote:
[...]

Post by John Baldwin
+int
+bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
+{
+ struct bus_dma_mem_cb_data d;
+ int error;
+
+ bzero(mem, sizeof(*mem));
+ error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
+ BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
+ &mem->dma_tag);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
+ &mem->dma_map);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ d.mem = mem;
+ error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
+ bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
+ if (error == 0)
+ error = d.error;
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ return (0);
+}
+[...]

A couple more thoughts on this...

It would be good to pass the flags to both bus_dma_tag_create() and
bus_dmamem_alloc(). While the manpage seems to indicate that certain
flags are specific to tag-create or mem-alloc, there is actually no
overlap in values, so passing flags to tag-create that a given arch only
uses in mem-alloc should be harmless. Implementations may be able
optimize things if they can see flags at tag-create time that normally
apply to allocation.

Hmmm, it just popped into my head that a new flag that means "this is
going to be a combined tag+map+buffer allocation" might allow for even
more optimizations. For example you can avoid allocating any resources
related to bouncing if you know that the only mapping that will ever be
created with that tag+map is one belonging to a buffer that the
implementation can carefully allocate in a way that avoids bouncing.

Another way to achieve that would be to allow the whole implementation
of bus_dma_mem_create() to be supplied by the arch, rather than having
it be generic in kern. The more I think about it, the more I like this.

Could we eliminate the alignment arg and instead document that the
alignment of the allocated buffer will be at least the same as the size
argument up to PAGE_SIZE, and will be PAGE_SIZE for anything 1 page or
larger? There is a standard uma-based allocator available in
kern/subr_busdma_buffalloc.c that implements this behavior. It's
currently used only by arm, but should be trivial to use more widely.
When I investigated alignment usage a couple years ago, I found nearly
all existing uses of alignment specify a value <= the size in the tag.

-- Ian

[John Baldwin]

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it to get
the bus address. Similarly, freeing it also requires several steps. In
addition, some of the steps are a bit subtle (e.g. the need to check for an
error in the bus_dma callback instead of from bus_dmamap_load()) and not all
drivers get those correct.
To try to make this simpler I've written a little wrapper API that tries to
provide a single call to allocate a buffer and a single call to free a buffer.
Each buffer is described by a structure defined by the API, and if the call to
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the buffer.
In the interests of simplicity, this API does not allow the buffer to be quite
as fully configured as the underlying bus_dma API, instead it aims to handle
the most common cases that are used in most drivers. As such, it assumes that
the buffer must be one contiguous range of DMA address space, and the only
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and the
flags parameter that is normally passed to bus_dmamem_alloc(). I believe that
this should be sufficient to cover the vast majority of the drivers in our
tree.
I've included below a patch that contains the wrapper API along with a sample
conversion of the ndis driver (chosen at random). If folks like this idea I
will update the patch to include manpage changes as well.
--- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
+++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
@@ -186,7 +186,6 @@
static ndis_status NdisMAllocateMapRegisters(ndis_handle,
uint32_t, uint8_t, uint32_t, uint32_t);
static void NdisMFreeMapRegisters(ndis_handle);
-static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
uint8_t, void **, ndis_physaddr *);
static void ndis_asyncmem_complete(device_object *, void *);
@@ -1387,23 +1386,6 @@
bus_dma_tag_destroy(sc->ndis_mtag);
}
-static void
-ndis_mapshared_cb(arg, segs, nseg, error)
- void *arg;
- bus_dma_segment_t *segs;
- int nseg;
- int error;
-{
- ndis_physaddr *p;
-
- if (error || nseg > 1)
- return;
-
- p = arg;
-
- p->np_quad = segs[0].ds_addr;
-}
-
/*
* This maps to bus_dmamem_alloc().
*/
@@ -1443,35 +1425,17 @@
* than 1GB of physical memory.
*/
- error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
- 0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
- NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
- &sh->ndis_stag);
+ error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
+ NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO);
if (error) {
free(sh, M_DEVBUF);
return;
}
- error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
- BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
-
- if (error) {
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
+ *vaddr = sh->ndis_mem.dma_vaddr;
+ paddr->np_quad = sh->ndis_mem.dma_baddr;
- error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
- len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
-
- if (error) {
- bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
-
/*
* Save the physical address along with the source address.
* The AirGo MIMO driver will call NdisMFreeSharedMemory()
@@ -1482,8 +1446,6 @@
*/
NDIS_LOCK(sc);
- sh->ndis_paddr.np_quad = paddr->np_quad;
- sh->ndis_saddr = *vaddr;
InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
NDIS_UNLOCK(sc);
}
@@ -1581,13 +1543,13 @@
l = sc->ndis_shlist.nle_flink;
while (l != &sc->ndis_shlist) {
sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
- if (sh->ndis_saddr == vaddr)
+ if (sh->ndis_mem.dma_vaddr == vaddr)
break;
/*
* Check the physaddr too, just in case the driver lied
* about the virtual address.
*/
- if (sh->ndis_paddr.np_quad == paddr.np_quad)
+ if (sh->ndis_mem.dma_baddr == paddr.np_quad)
break;
l = l->nle_flink;
}
@@ -1604,9 +1566,7 @@
NDIS_UNLOCK(sc);
- bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
- bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
+ bus_dma_mem_free(&sh->ndis_mem);
free(sh, M_DEVBUF);
}
--- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
+++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
@@ -66,10 +66,7 @@
struct ndis_shmem {
list_entry ndis_list;
- bus_dma_tag_t ndis_stag;
- bus_dmamap_t ndis_smap;
- void *ndis_saddr;
- ndis_physaddr ndis_paddr;
+ struct bus_dmamem ndis_mem;
};
struct ndis_cfglist {
--- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
+++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
@@ -540,3 +540,66 @@
return (0);
}
+
+struct bus_dma_mem_cb_data {
+ struct bus_dmamem *mem;
+ int error;
+};
+
+static void
+bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
+{
+ struct bus_dma_mem_cb_data *d;
+
+ d = arg;
+ d->error = error;
+ if (error)
+ return;
+ d->mem->dma_baddr = segs[0].ds_addr;
+}
+
+int
+bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
+{
+ struct bus_dma_mem_cb_data d;
+ int error;
+
+ bzero(mem, sizeof(*mem));
+ error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
+ BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
+ &mem->dma_tag);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
+ &mem->dma_map);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ d.mem = mem;
+ error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
+ bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
+ if (error == 0)
+ error = d.error;
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ return (0);
+}
+
+void
+bus_dma_mem_free(struct bus_dmamem *mem)
+{
+
+ if (mem->dma_baddr != 0)
+ bus_dmamap_unload(mem->dma_tag, mem->dma_map);
+ if (mem->dma_vaddr != NULL)
+ bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map);
+ if (mem->dma_tag != NULL)
+ bus_dma_tag_destroy(mem->dma_tag);
+ bzero(mem, sizeof(*mem));
+}
--- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
+++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
@@ -337,4 +337,29 @@
#endif /* __sparc64__ */
+/*
+ * A wrapper API to simplify management of static mappings.
+ */
+
+struct bus_dmamem {
+ bus_dma_tag_t dma_tag;
+ bus_dmamap_t dma_map;
+ void *dma_vaddr;
+ bus_addr_t dma_baddr;
+};
+
+/*
+ * Allocate a mapping. On success, zero is returned and the 'dma_vaddr'
+ * and 'dma_baddr' fields are populated with the virtual and bus addresses,
+ * respectively, of the mapping.
+ */
+int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr,
+ bus_size_t len, int flags);
+
+/*
+ * Release a mapping created by bus_dma_mem_create().
+ */
+void bus_dma_mem_free(struct bus_dmamem *mem);
+
#endif /* _BUS_DMA_H_ */

Ping. The last time I brought this up we ended up off in the weeds a bit
about changes to the bus dma API in general. However, I think that even if
we reduce the number of args to bus_dma_create_tag you are still left with
managing a tag per static allocation etc. I'm working on porting a driver
today where I basically just copied this into the driver directly rather
than having to implement it all by hand for the umpteenth time. Are folks
seriously opposed to having a single function you can call to allocate a
static DMA mapping?

[Warner Losh]

Post by John Baldwin

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it

to get

Post by John Baldwin
the bus address. Similarly, freeing it also requires several steps. In
addition, some of the steps are a bit subtle (e.g. the need to check for

an

Post by John Baldwin
error in the bus_dma callback instead of from bus_dmamap_load()) and not

all

Post by John Baldwin
drivers get those correct.
To try to make this simpler I've written a little wrapper API that tries

to

Post by John Baldwin
provide a single call to allocate a buffer and a single call to free a

buffer.

Post by John Baldwin
Each buffer is described by a structure defined by the API, and if the

call to

Post by John Baldwin
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the

buffer.

Post by John Baldwin
In the interests of simplicity, this API does not allow the buffer to be

quite

Post by John Baldwin
as fully configured as the underlying bus_dma API, instead it aims to

handle

Post by John Baldwin
the most common cases that are used in most drivers. As such, it

assumes that

Post by John Baldwin
the buffer must be one contiguous range of DMA address space, and the

only

Post by John Baldwin
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and

the

Post by John Baldwin
flags parameter that is normally passed to bus_dmamem_alloc(). I

believe that

Post by John Baldwin
this should be sufficient to cover the vast majority of the drivers in

our

Post by John Baldwin
tree.
I've included below a patch that contains the wrapper API along with a

sample

Post by John Baldwin
conversion of the ndis driver (chosen at random). If folks like this

idea I

Post by John Baldwin
will update the patch to include manpage changes as well.
--- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
+++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
@@ -186,7 +186,6 @@
static ndis_status NdisMAllocateMapRegisters(ndis_handle,
uint32_t, uint8_t, uint32_t, uint32_t);
static void NdisMFreeMapRegisters(ndis_handle);
-static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
uint8_t, void **, ndis_physaddr *);
static void ndis_asyncmem_complete(device_object *, void *);
@@ -1387,23 +1386,6 @@
bus_dma_tag_destroy(sc->ndis_mtag);
}
-static void
-ndis_mapshared_cb(arg, segs, nseg, error)
- void *arg;
- bus_dma_segment_t *segs;
- int nseg;
- int error;
-{
- ndis_physaddr *p;
-
- if (error || nseg > 1)
- return;
-
- p = arg;
-
- p->np_quad = segs[0].ds_addr;
-}
-
/*
* This maps to bus_dmamem_alloc().
*/
@@ -1443,35 +1425,17 @@
* than 1GB of physical memory.
*/
- error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
- 0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
- NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
- &sh->ndis_stag);
+ error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
+ NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT |

BUS_DMA_ZERO);

Post by John Baldwin
if (error) {
free(sh, M_DEVBUF);
return;
}
- error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
- BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
-
- if (error) {
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
+ *vaddr = sh->ndis_mem.dma_vaddr;
+ paddr->np_quad = sh->ndis_mem.dma_baddr;
- error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
- len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
-
- if (error) {
- bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
-
/*
* Save the physical address along with the source address.
* The AirGo MIMO driver will call NdisMFreeSharedMemory()
@@ -1482,8 +1446,6 @@
*/
NDIS_LOCK(sc);
- sh->ndis_paddr.np_quad = paddr->np_quad;
- sh->ndis_saddr = *vaddr;
InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
NDIS_UNLOCK(sc);
}
@@ -1581,13 +1543,13 @@
l = sc->ndis_shlist.nle_flink;
while (l != &sc->ndis_shlist) {
sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
- if (sh->ndis_saddr == vaddr)
+ if (sh->ndis_mem.dma_vaddr == vaddr)
break;
/*
* Check the physaddr too, just in case the driver lied
* about the virtual address.
*/
- if (sh->ndis_paddr.np_quad == paddr.np_quad)
+ if (sh->ndis_mem.dma_baddr == paddr.np_quad)
break;
l = l->nle_flink;
}
@@ -1604,9 +1566,7 @@
NDIS_UNLOCK(sc);
- bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
- bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
+ bus_dma_mem_free(&sh->ndis_mem);
free(sh, M_DEVBUF);
}
--- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
+++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
@@ -66,10 +66,7 @@
struct ndis_shmem {
list_entry ndis_list;
- bus_dma_tag_t ndis_stag;
- bus_dmamap_t ndis_smap;
- void *ndis_saddr;
- ndis_physaddr ndis_paddr;
+ struct bus_dmamem ndis_mem;
};
struct ndis_cfglist {
--- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
+++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
@@ -540,3 +540,66 @@
return (0);
}
+
+struct bus_dma_mem_cb_data {
+ struct bus_dmamem *mem;
+ int error;
+};
+
+static void
+bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
+{
+ struct bus_dma_mem_cb_data *d;
+
+ d = arg;
+ d->error = error;
+ if (error)
+ return;
+ d->mem->dma_baddr = segs[0].ds_addr;
+}
+
+int
+bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
+{
+ struct bus_dma_mem_cb_data d;
+ int error;
+
+ bzero(mem, sizeof(*mem));
+ error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
+ BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
+ &mem->dma_tag);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
+ &mem->dma_map);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ d.mem = mem;
+ error = bus_dmamap_load(mem->dma_tag, mem->dma_map,

mem->dma_vaddr, len,

Post by John Baldwin
+ bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
+ if (error == 0)
+ error = d.error;
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ return (0);
+}
+
+void
+bus_dma_mem_free(struct bus_dmamem *mem)
+{
+
+ if (mem->dma_baddr != 0)
+ bus_dmamap_unload(mem->dma_tag, mem->dma_map);
+ if (mem->dma_vaddr != NULL)
+ bus_dmamem_free(mem->dma_tag, mem->dma_vaddr,

mem->dma_map);

Post by John Baldwin
+ if (mem->dma_tag != NULL)
+ bus_dma_tag_destroy(mem->dma_tag);
+ bzero(mem, sizeof(*mem));
+}
--- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
+++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
@@ -337,4 +337,29 @@
#endif /* __sparc64__ */
+/*
+ * A wrapper API to simplify management of static mappings.
+ */
+
+struct bus_dmamem {
+ bus_dma_tag_t dma_tag;
+ bus_dmamap_t dma_map;
+ void *dma_vaddr;
+ bus_addr_t dma_baddr;
+};
+
+/*
+ * Allocate a mapping. On success, zero is returned and the 'dma_vaddr'
+ * and 'dma_baddr' fields are populated with the virtual and bus

addresses,

Post by John Baldwin
+ * respectively, of the mapping.
+ */
+int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr,
+ bus_size_t len, int flags);
+
+/*
+ * Release a mapping created by bus_dma_mem_create().
+ */
+void bus_dma_mem_free(struct bus_dmamem *mem);
+
#endif /* _BUS_DMA_H_ */

Ping. The last time I brought this up we ended up off in the weeds a bit
about changes to the bus dma API in general. However, I think that even if
we reduce the number of args to bus_dma_create_tag you are still left with
managing a tag per static allocation etc. I'm working on porting a driver
today where I basically just copied this into the driver directly rather
than having to implement it all by hand for the umpteenth time. Are folks
seriously opposed to having a single function you can call to allocate a
static DMA mapping

I have no objections to this.

Warner

[Marius Strobl]

Post by John Baldwin

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it to get
the bus address. Similarly, freeing it also requires several steps. In
addition, some of the steps are a bit subtle (e.g. the need to check for an
error in the bus_dma callback instead of from bus_dmamap_load()) and not all
drivers get those correct.
To try to make this simpler I've written a little wrapper API that tries to
provide a single call to allocate a buffer and a single call to free a buffer.
Each buffer is described by a structure defined by the API, and if the call to
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the buffer.
In the interests of simplicity, this API does not allow the buffer to be quite
as fully configured as the underlying bus_dma API, instead it aims to handle
the most common cases that are used in most drivers. As such, it assumes that
the buffer must be one contiguous range of DMA address space, and the only
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and the
flags parameter that is normally passed to bus_dmamem_alloc(). I believe that
this should be sufficient to cover the vast majority of the drivers in our
tree.
I've included below a patch that contains the wrapper API along with a sample
conversion of the ndis driver (chosen at random). If folks like this idea I
will update the patch to include manpage changes as well.
--- //depot/vendor/freebsd/src/sys/compat/ndis/subr_ndis.c
+++ //depot/user/jhb/cleanup/sys/compat/ndis/subr_ndis.c
@@ -186,7 +186,6 @@
static ndis_status NdisMAllocateMapRegisters(ndis_handle,
uint32_t, uint8_t, uint32_t, uint32_t);
static void NdisMFreeMapRegisters(ndis_handle);
-static void ndis_mapshared_cb(void *, bus_dma_segment_t *, int, int);
static void NdisMAllocateSharedMemory(ndis_handle, uint32_t,
uint8_t, void **, ndis_physaddr *);
static void ndis_asyncmem_complete(device_object *, void *);
@@ -1387,23 +1386,6 @@
bus_dma_tag_destroy(sc->ndis_mtag);
}
-static void
-ndis_mapshared_cb(arg, segs, nseg, error)
- void *arg;
- bus_dma_segment_t *segs;
- int nseg;
- int error;
-{
- ndis_physaddr *p;
-
- if (error || nseg > 1)
- return;
-
- p = arg;
-
- p->np_quad = segs[0].ds_addr;
-}
-
/*
* This maps to bus_dmamem_alloc().
*/
@@ -1443,35 +1425,17 @@
* than 1GB of physical memory.
*/
- error = bus_dma_tag_create(sc->ndis_parent_tag, 64,
- 0, NDIS_BUS_SPACE_SHARED_MAXADDR, BUS_SPACE_MAXADDR, NULL,
- NULL, len, 1, len, BUS_DMA_ALLOCNOW, NULL, NULL,
- &sh->ndis_stag);
+ error = bus_dma_mem_create(&sh->ndis_mem, sc->ndis_parent_tag, 64,
+ NDIS_BUS_SPACE_SHARED_MAXADDR, len, BUS_DMA_NOWAIT | BUS_DMA_ZERO);
if (error) {
free(sh, M_DEVBUF);
return;
}
- error = bus_dmamem_alloc(sh->ndis_stag, vaddr,
- BUS_DMA_NOWAIT | BUS_DMA_ZERO, &sh->ndis_smap);
-
- if (error) {
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
+ *vaddr = sh->ndis_mem.dma_vaddr;
+ paddr->np_quad = sh->ndis_mem.dma_baddr;
- error = bus_dmamap_load(sh->ndis_stag, sh->ndis_smap, *vaddr,
- len, ndis_mapshared_cb, (void *)paddr, BUS_DMA_NOWAIT);
-
- if (error) {
- bus_dmamem_free(sh->ndis_stag, *vaddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
- free(sh, M_DEVBUF);
- return;
- }
-
/*
* Save the physical address along with the source address.
* The AirGo MIMO driver will call NdisMFreeSharedMemory()
@@ -1482,8 +1446,6 @@
*/
NDIS_LOCK(sc);
- sh->ndis_paddr.np_quad = paddr->np_quad;
- sh->ndis_saddr = *vaddr;
InsertHeadList((&sc->ndis_shlist), (&sh->ndis_list));
NDIS_UNLOCK(sc);
}
@@ -1581,13 +1543,13 @@
l = sc->ndis_shlist.nle_flink;
while (l != &sc->ndis_shlist) {
sh = CONTAINING_RECORD(l, struct ndis_shmem, ndis_list);
- if (sh->ndis_saddr == vaddr)
+ if (sh->ndis_mem.dma_vaddr == vaddr)
break;
/*
* Check the physaddr too, just in case the driver lied
* about the virtual address.
*/
- if (sh->ndis_paddr.np_quad == paddr.np_quad)
+ if (sh->ndis_mem.dma_baddr == paddr.np_quad)
break;
l = l->nle_flink;
}
@@ -1604,9 +1566,7 @@
NDIS_UNLOCK(sc);
- bus_dmamap_unload(sh->ndis_stag, sh->ndis_smap);
- bus_dmamem_free(sh->ndis_stag, sh->ndis_saddr, sh->ndis_smap);
- bus_dma_tag_destroy(sh->ndis_stag);
+ bus_dma_mem_free(&sh->ndis_mem);
free(sh, M_DEVBUF);
}
--- //depot/vendor/freebsd/src/sys/dev/if_ndis/if_ndisvar.h
+++ //depot/user/jhb/cleanup/sys/dev/if_ndis/if_ndisvar.h
@@ -66,10 +66,7 @@
struct ndis_shmem {
list_entry ndis_list;
- bus_dma_tag_t ndis_stag;
- bus_dmamap_t ndis_smap;
- void *ndis_saddr;
- ndis_physaddr ndis_paddr;
+ struct bus_dmamem ndis_mem;
};
struct ndis_cfglist {
--- //depot/vendor/freebsd/src/sys/kern/subr_bus_dma.c
+++ //depot/user/jhb/cleanup/sys/kern/subr_bus_dma.c
@@ -540,3 +540,66 @@
return (0);
}
+
+struct bus_dma_mem_cb_data {
+ struct bus_dmamem *mem;
+ int error;
+};
+
+static void
+bus_dma_mem_cb(void *arg, bus_dma_segment_t *segs, int nseg, int error)
+{
+ struct bus_dma_mem_cb_data *d;
+
+ d = arg;
+ d->error = error;
+ if (error)
+ return;
+ d->mem->dma_baddr = segs[0].ds_addr;
+}
+
+int
+bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr, bus_size_t len, int flags)
+{
+ struct bus_dma_mem_cb_data d;
+ int error;
+
+ bzero(mem, sizeof(*mem));
+ error = bus_dma_tag_create(parent, alignment, 0, lowaddr,
+ BUS_SPACE_MAXADDR, NULL, NULL, len, 1, len, 0, NULL, NULL,
+ &mem->dma_tag);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ error = bus_dmamem_alloc(mem->dma_tag, &mem->dma_vaddr, flags,
+ &mem->dma_map);
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ d.mem = mem;
+ error = bus_dmamap_load(mem->dma_tag, mem->dma_map, mem->dma_vaddr, len,
+ bus_dma_mem_cb, &d, BUS_DMA_NOWAIT);
+ if (error == 0)
+ error = d.error;
+ if (error) {
+ bus_dma_mem_free(mem);
+ return (error);
+ }
+ return (0);
+}
+
+void
+bus_dma_mem_free(struct bus_dmamem *mem)
+{
+
+ if (mem->dma_baddr != 0)
+ bus_dmamap_unload(mem->dma_tag, mem->dma_map);
+ if (mem->dma_vaddr != NULL)
+ bus_dmamem_free(mem->dma_tag, mem->dma_vaddr, mem->dma_map);
+ if (mem->dma_tag != NULL)
+ bus_dma_tag_destroy(mem->dma_tag);
+ bzero(mem, sizeof(*mem));
+}
--- //depot/vendor/freebsd/src/sys/sys/bus_dma.h
+++ //depot/user/jhb/cleanup/sys/sys/bus_dma.h
@@ -337,4 +337,29 @@
#endif /* __sparc64__ */
+/*
+ * A wrapper API to simplify management of static mappings.
+ */
+
+struct bus_dmamem {
+ bus_dma_tag_t dma_tag;
+ bus_dmamap_t dma_map;
+ void *dma_vaddr;
+ bus_addr_t dma_baddr;
+};
+
+/*
+ * Allocate a mapping. On success, zero is returned and the 'dma_vaddr'
+ * and 'dma_baddr' fields are populated with the virtual and bus addresses,
+ * respectively, of the mapping.
+ */
+int bus_dma_mem_create(struct bus_dmamem *mem, bus_dma_tag_t parent,
+ bus_size_t alignment, bus_addr_t lowaddr,
+ bus_size_t len, int flags);
+
+/*
+ * Release a mapping created by bus_dma_mem_create().
+ */
+void bus_dma_mem_free(struct bus_dmamem *mem);
+
#endif /* _BUS_DMA_H_ */

Ping. The last time I brought this up we ended up off in the weeds a bit
about changes to the bus dma API in general. However, I think that even if
we reduce the number of args to bus_dma_create_tag you are still left with
managing a tag per static allocation etc. I'm working on porting a driver
today where I basically just copied this into the driver directly rather
than having to implement it all by hand for the umpteenth time. Are folks
seriously opposed to having a single function you can call to allocate a
static DMA mapping?

No, I'm fine with your proposal.

Marius

[John Baldwin]

Post by John Baldwin
The bus_dma API to allocate a chunk of static DMA'able memory (e.g. for
descriptor rings) can be a bit obtuse to use in that it require a bit of
boilerplate to create a tag, allocate memory for the tag, then load it to get
the bus address. Similarly, freeing it also requires several steps. In
addition, some of the steps are a bit subtle (e.g. the need to check for an
error in the bus_dma callback instead of from bus_dmamap_load()) and not all
drivers get those correct.
To try to make this simpler I've written a little wrapper API that tries to
provide a single call to allocate a buffer and a single call to free a buffer.
Each buffer is described by a structure defined by the API, and if the call to
allocate a buffer succeeds, the structure contains both a pointer to the
buffer in the kernel's address space as well as a bus address of the buffer.
In the interests of simplicity, this API does not allow the buffer to be quite
as fully configured as the underlying bus_dma API, instead it aims to handle
the most common cases that are used in most drivers. As such, it assumes that
the buffer must be one contiguous range of DMA address space, and the only
parameters that can be specified are the parent tag, the alignment of the
buffer, the lowaddr parameter, the size of the buffer to allocate, and the
flags parameter that is normally passed to bus_dmamem_alloc(). I believe that
this should be sufficient to cover the vast majority of the drivers in our
tree.

After some more thinking, I've altered this API to include an 'args' struct
similar to the one Konstantin used for make_dev_s() to specify constraints.
This would permit most constraints to be specified on an as-needed basis
without requiring all of them each time. It does still assume 1 contiguous
region, but the majority of our drivers require that anyway.

I've forward ported this and converted a more typical driver (xl(4)) as a
demo of the new API. You can find the full diff here:

https://reviews.freebsd.org/D5704

(I've not yet written manpage updates)

Here's the new code in xl(4) to allocate the TX and RX rings. I think it
highlights the specific constraints (alignment, etc.) more clearly than the
previous version:

​ /*
​ * Now allocate a chunk of DMA-able memory for the DMA
​ * descriptor lists. All of our lists are allocated as a
​ * contiguous block of memory.
​ */
​ bus_dma_mem_args_init(&args);
​ args.dma_alignment = 8;
​ args.dma_lowaddr = BUS_SPACE_MAXADDR_32BIT;
​ error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_RX_LIST_SZ, 0, &args,
​ &sc->xl_ldata.xl_rx_ring);
​ if (error) {
​ device_printf(dev, "failed to allocate rx ring\n");
​ goto fail;
​ }
​ sc->xl_ldata.xl_rx_list = sc->xl_ldata.xl_rx_ring.dma_vaddr;
​
​ error = bus_dma_mem_alloc(bus_get_dma_tag(dev), XL_TX_LIST_SZ, 0, &args,
​ &sc->xl_ldata.xl_tx_ring);
​ if (error) {
​ device_printf(dev, "failed to allocate tx ring\n");
​ goto fail;
​ }
​ sc->xl_ldata.xl_tx_list = sc->xl_ldata.xl_tx_ring.dma_vaddr;
​