From owner-freebsd-arch@freebsd.org Sat Feb 27 05:39:29 2016 Return-Path: Delivered-To: freebsd-arch@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9C01AAB570F for ; Sat, 27 Feb 2016 05:39:29 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qg0-x236.google.com (mail-qg0-x236.google.com [IPv6:2607:f8b0:400d:c04::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6136DF63 for ; Sat, 27 Feb 2016 05:39:29 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qg0-x236.google.com with SMTP id d32so25495877qgd.0 for ; Fri, 26 Feb 2016 21:39:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc; bh=bgDBvfDtcl0nSd0QdhX4BoKMaHKrxlMImdvvfThGs3U=; b=cH70FmuLKyXY47CG2JmD+oTEygOHrhusUKsxyKNIab2GINwy1dwD1JUKzvh0F5kOSC uuYgydI4HO1pnFHdBwO6vaOD+/DqR14gXVyg8mbhVi0MQn4hNNYcHX81LRVsCaA5l7TC 0PkgqUwt17ieDEdb/vf3YdXtvD/Tg3WSI3LUzlCiW0sR76lv4Jc8uW1qPHIkyZzIKdSa soyRW1CuwTOaKRVBxgajFYXO1XoVkXcl5dXdtvDWZGAcCPF7b9bW2a8kIKbEdntcP8GK 4yLV7v7LmAYA0JZWhRaA9tgagm3yooxXLU+Q56bbV+kcT+ASp9XxTWEAnx+HvV3ypw+j iY9Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:date :message-id:subject:from:to:cc; bh=bgDBvfDtcl0nSd0QdhX4BoKMaHKrxlMImdvvfThGs3U=; b=JWGqsNnCUNPk4W0sSs+oQUwf7VpdJ/1NM4vnMljH9w42OhsUeOQiU95nuY3p/2fteF fwUHSG8DvStchn8nMOHtFuqr5UttBlbi9A+LIWntOGhjOOKCfyO0athvspTS4Z3ot56r TI8TIYVbeoANRyKc7nh1rrHOEEQyMW1ysNCeVjRv805ASRemhz6O9/o9NTmLLNv4tEee NjXfCJU+6R9pz9NwXyWINzX2vnKO5KD/kkebihsiLZcrJbjVCcpotInhqk2EafHqXb/3 M8R6rVJH7IhJFN61L03HEEbiL2G6pjDw5OohGHxqpVwEN5UA9cFTA7jyY9dzDgV4HsdH gR7Q== X-Gm-Message-State: AD7BkJJjAZMELUyLsc4POdnlFD+ALJr/25aebXtdKoex5U8jk6gs1djI5zfiLQBNss7MLncpjcBBOCwcqmYaIA== MIME-Version: 1.0 X-Received: by 10.140.175.136 with SMTP id v130mr6994243qhv.74.1456551568387; Fri, 26 Feb 2016 21:39:28 -0800 (PST) Sender: wlosh@bsdimp.com Received: by 10.140.30.166 with HTTP; Fri, 26 Feb 2016 21:39:28 -0800 (PST) X-Originating-IP: [50.253.99.174] In-Reply-To: <2856669.mkVhDvxH7k@ralph.baldwin.cx> References: <2800970.jY4xzTy9Hz@ralph.baldwin.cx> <2856669.mkVhDvxH7k@ralph.baldwin.cx> Date: Fri, 26 Feb 2016 22:39:28 -0700 X-Google-Sender-Auth: KuKnKPHjdmJBfPMaq9OMrRlVqfc Message-ID: Subject: Re: Wrapper API for static bus_dma allocations From: Warner Losh To: John Baldwin Cc: "freebsd-arch@freebsd.org" Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Feb 2016 05:39:29 -0000 On Fri, Feb 26, 2016 at 6:10 PM, John Baldwin wrote: > On Thursday, January 29, 2015 03:37:19 PM John Baldwin wrote: > > 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 I have no objections to this. Warner