lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 23 Sep 2010 16:20:06 -0700 From: "Ira W. Snyder" <iws@...o.caltech.edu> To: Dan Williams <dan.j.williams@...el.com> Cc: linux-kernel@...r.kernel.org, Andrew Morton <akpm@...ux-foundation.org> Subject: Re: [PATCH] fsldma: move DMA_SLAVE support functions to the driver On Thu, Sep 23, 2010 at 02:58:06PM -0700, Dan Williams wrote: > On Thu, Sep 23, 2010 at 1:12 PM, Ira W. Snyder <iws@...o.caltech.edu> wrote: > > On Thu, Sep 23, 2010 at 11:31:11AM -0700, Dan Williams wrote: > >> On Tue, Sep 7, 2010 at 1:45 PM, Ira W. Snyder <iws@...o.caltech.edu> wrote: > >> > The DMA_SLAVE support functions all existed as static inlines in the > >> > driver specific header arch/powerpc/include/asm/fsldma.h. Move the body > >> > of the functions to the driver itself, and EXPORT_SYMBOL_GPL() them. > >> > > >> > At the same time, add the missing linux/list.h header. > >> > > >> > Signed-off-by: Ira W. Snyder <iws@...o.caltech.edu> > >> > --- > >> > > >> > Hello AKPM, Dan, > >> > >> Hi, I'm back from paternity leave. > >> > >> > > >> > At AKPM's request, I moved these static inline'd functions into the driver > >> > code itself, and then EXPORT_SYMBOL_GPL()'d them. I also added a gfp > >> > parameter to the fsl_dma_slave_alloc() function. And included the > >> > linux/list.h header. > >> > > >> > If you'd like three separate patches, I can do that. The changes were so > >> > trivial that I lumped all of them into a single patch. > >> > > >> > Thanks, > >> > Ira > >> > > >> > arch/powerpc/include/asm/fsldma.h | 70 +++------------------------------ > >> > drivers/dma/fsldma.c | 77 +++++++++++++++++++++++++++++++++++++ > >> > >> Remind me why we need this header file and these routines again? git > >> grep says nothing in-tree is using these? > >> > >> I also do not want to proliferate driver specific interfaces. It > >> defeats the purpose of having a common dmaengine structure. Along > >> these lines Linus W. added the new device_control() method (first > >> added in commit c3635c78, later tweaked in 05827630). Could that be > >> massaged for your needs? > >> > > > > These routines are used by a (currently out of tree) driver that I > > maintain. Please see the LKML and linuxppc-dev ML post titled: > > [PATCH RFCv2 0/5] CARMA Board Support > > > > The FPGA programming driver uses these interfaces. Past a few initial > > comments (which I promptly fixed!), it seems that nobody wants to look > > at this driver. I don't know how to move it towards mainline other than > > spam people, which I would prefer to avoid doing. > > > > The new device_control() method doesn't have enough information for this > > purpose. I also don't know of any other DMA controllers that have the > > feature I'm using (described below). > > > > The fsldma controller has the option of being controlled by an external > > device, via some pins. This is leveraged by our system controller to > > properly program some big data processing FPGAs. > > > > Basically, the fsldma controller must be told: > > 1) descriptors (src, dst, len tuples) > > 2) use the external-start feature (external control) > > 3) go > > > > Now #3's "go" doesn't actually start the transfer. It just starts > > listening on the external pins for the correct sequence for each > > transaction. > > > > Also, the descriptors must be set up such that they form a single chain, > > without interruptions between segments. Back at the beginning of the > > year, I did a lot of work on the fsldma driver. It *should* be able to > > chain DMA transfers appropriately for this mode using the existing > > device_prep_dma_memcpy() routine. > > > > This means that it would be possible to move some of the functionality > > of fsldma's device_prep_slave_sg() into core DMAEngine code, and > > implement something like dma_memcpy_sg_to_sg(). This would take two > > scatterlists and call device_prep_slave_sg() enough times to setup a DMA > > operation that will move all data appropriately. > > > > This is exactly what fsldma's device_prep_slave_sg() actually does. It > > was non-trivial to get the triply nested loops correct. > > > > As an example from my driver, I have this scenario: > > 1) a scatterlist of kernel memory pages (this is my data payload) > > 2) a fixed set of non-incrementing physical addresses I need to send that data to > > > > Other than the fsldma device_prep_slave_sg(), this is difficult to do. > > > > I hope that helps to explain the problem. > > Yes, thanks now I recall, and I remember saying something about how > this passes the "Voyager test". That said it seems odd to carry > symbol exports for something like this. Instead of exporting can we > exploit the fact that the fpga driver already knows it has a > fsldma_device pointer and do something like: > > struct fsldma_device *fdev = container_of(dma, typeof(fdev), common); > > fdev->slave_alloc(); > > This keeps it contained to the fsldma driver. > I agree that it sucks to have exports for this. I'm happy with the static inlines in a header, but AKPM didn't like them. I think the solution above would actually need to be a struct fsldma_chan instead of a struct fsldma_device, but otherwise would work. The caveat with your solution above is that (parts of) drivers/dma/fsldma.h need to be copied into arch/powerpc/include/asm/fsldma.h for the type information. It seems a bit awkward as well. I'd like to propose another possible solution: 1) add an external_control member to struct dma_slave_config 2) remove the list_head from struct fsl_dma_slave 3) remove all of the functions from arch/powerpc/include/asm/fsldma.h 4) add dma_async_memcpy_sg_to_sg() dma_cookie_t dma_async_memcpy_sg_to_sg(struct dma_chan *chan, struct *dst_sg, int dst_nents, struct *src_sg, int src_nents, dma_async_tx_callback cb, void *cb_param); This function would do most of the current work that the current fsldma device_prep_slave_sg() does: setting up a DMA chain to transfer between two scatterlists. I'm willing to do an ugly hack to make a fake scatterlist with my fixed non-incrementing hardware addresses for the dst_sg. Like this: sg_dma_address(sg) = MY_FIXED_ADDRESS_1; sg_dma_len(sg) = MY_FIXED_LENGTH_1; sg = sg_next(sg); sg_dma_address(sg) = MY_FIXED_ADDRESS_2; sg_dma_len(sg) = MY_FIXED_LENGTH_2; Now my driver would look like this: struct dma_slave_config dsc; dsc.external_control = true; /* allocate and fill my dst_sg with the fake data, like above */ /* DO NOT use dma_map_sg() on the fake scatterlist */ /* allocate and fill my src_sg with pages + data */ src_nents = dma_map_sg(..., src_sg, DMA_TO_DEVICE); /* sets up the DMA transfer, but doesn't start it */ cookie = dma_async_memcpy_sg_to_sg(chan, dst_sg, dst_nents, src_sg, src_nents, my_cb, my_cb_param); /* puts the DMA engine into external control mode */ ret = chan->device_control(chan, DMA_SLAVE_CONFIG, (unsigned long)dsc); /* "start" the transfer */ dma_async_memcpy_issue_pending(chan); /* * start the system controller FPGA programmer, which will toggle the * DMA engine's external control pins appropriately. * * Wait until my callback is called, which will wake up and continue * the function. Using wait_for_completion_timeout(), etc. Which is * exactly what I do now. */ The freescale DMA engine must have the external control bit set on every new transaction. It is automatically cleared after a transaction completes. Ira -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists