Re: [PATCH 3/4] fsldma: remove DMA_SLAVE support

2010-09-29 Thread Dan Williams
On Mon, Sep 27, 2010 at 3:57 PM, Ira W. Snyder i...@ovro.caltech.edu wrote:
 Now that the generic DMAEngine API has support for scatterlist to
 scatterlist copying, this implementation of the DMA_SLAVE API is no
 longer necessary.

 In order to let device_control() continue to function, a stub
 device_prep_slave_sg() function is provided. This allows custom device
 configuration, such as enabling external control.


 +       case DMA_SLAVE_CONFIG:
 +
 +               cfg = (struct fsldma_slave_config *)arg;

Now that I actually see someone trying to use the recommended
extension model it comes across as unsafe, what guarantees that arg is
pointing to a fsldma_slave_config.  At at minimum you could ensure
that this channel has been claimed for private usage which loosely
implies that the client knows that it is talking to an fsldma channel.
 Even safer is to just assign you a one-off dma_ctrl_cmd
(FSLDMA_EXTERNAL_START) for this purpose.  Otherwise this and the
other patches look good.

--
Dan
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


Re: [PATCH 3/4] fsldma: remove DMA_SLAVE support

2010-09-29 Thread Ira W. Snyder
On Wed, Sep 29, 2010 at 02:52:16PM -0700, Dan Williams wrote:
 On Mon, Sep 27, 2010 at 3:57 PM, Ira W. Snyder i...@ovro.caltech.edu wrote:
  Now that the generic DMAEngine API has support for scatterlist to
  scatterlist copying, this implementation of the DMA_SLAVE API is no
  longer necessary.
 
  In order to let device_control() continue to function, a stub
  device_prep_slave_sg() function is provided. This allows custom device
  configuration, such as enabling external control.
 
 
  +       case DMA_SLAVE_CONFIG:
  +
  +               cfg = (struct fsldma_slave_config *)arg;
 
 Now that I actually see someone trying to use the recommended
 extension model it comes across as unsafe, what guarantees that arg is
 pointing to a fsldma_slave_config.  At at minimum you could ensure
 that this channel has been claimed for private usage which loosely
 implies that the client knows that it is talking to an fsldma channel.
  Even safer is to just assign you a one-off dma_ctrl_cmd
 (FSLDMA_EXTERNAL_START) for this purpose.  Otherwise this and the
 other patches look good.
 

I agree, it is a very unsafe model.

I'll take your suggestion, and do that instead. A new patch will be
forthcoming shortly.

Thanks,
Ira
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


[PATCH 3/4] fsldma: remove DMA_SLAVE support

2010-09-27 Thread Ira W. Snyder
Now that the generic DMAEngine API has support for scatterlist to
scatterlist copying, this implementation of the DMA_SLAVE API is no
longer necessary.

In order to let device_control() continue to function, a stub
device_prep_slave_sg() function is provided. This allows custom device
configuration, such as enabling external control.

Signed-off-by: Ira W. Snyder i...@ovro.caltech.edu
---
 arch/powerpc/include/asm/fsldma.h |  115 ++--
 drivers/dma/fsldma.c  |  219 +++--
 2 files changed, 48 insertions(+), 286 deletions(-)

diff --git a/arch/powerpc/include/asm/fsldma.h 
b/arch/powerpc/include/asm/fsldma.h
index debc5ed..dc0bd27 100644
--- a/arch/powerpc/include/asm/fsldma.h
+++ b/arch/powerpc/include/asm/fsldma.h
@@ -1,7 +1,7 @@
 /*
  * Freescale MPC83XX / MPC85XX DMA Controller
  *
- * Copyright (c) 2009 Ira W. Snyder i...@ovro.caltech.edu
+ * Copyright (c) 2009-2010 Ira W. Snyder i...@ovro.caltech.edu
  *
  * This file is licensed under the terms of the GNU General Public License
  * version 2. This program is licensed as is without any warranty of any
@@ -11,127 +11,32 @@
 #ifndef __ARCH_POWERPC_ASM_FSLDMA_H__
 #define __ARCH_POWERPC_ASM_FSLDMA_H__
 
-#include linux/slab.h
 #include linux/dmaengine.h
 
 /*
- * Definitions for the Freescale DMA controller's DMA_SLAVE implemention
+ * The Freescale DMA controller has several features that are not accomodated
+ * in the Linux DMAEngine API. Therefore, the generic structure is expanded
+ * to allow drivers to use these features.
  *
- * The Freescale DMA_SLAVE implementation was designed to handle many-to-many
- * transfers. An example usage would be an accelerated copy between two
- * scatterlists. Another example use would be an accelerated copy from
- * multiple non-contiguous device buffers into a single scatterlist.
+ * This structure should be passed into the DMAEngine routine device_control()
+ * as in this example:
  *
- * A DMA_SLAVE transaction is defined by a struct fsl_dma_slave. This
- * structure contains a list of hardware addresses that should be copied
- * to/from the scatterlist passed into device_prep_slave_sg(). The structure
- * also has some fields to enable hardware-specific features.
+ * chan-device-device_control(chan, DMA_SLAVE_CONFIG, (unsigned long)cfg);
  */
 
 /**
- * struct fsl_dma_hw_addr
- * @entry: linked list entry
- * @address: the hardware address
- * @length: length to transfer
- *
- * Holds a single physical hardware address / length pair for use
- * with the DMAEngine DMA_SLAVE API.
- */
-struct fsl_dma_hw_addr {
-   struct list_head entry;
-
-   dma_addr_t address;
-   size_t length;
-};
-
-/**
  * struct fsl_dma_slave
- * @addresses: a linked list of struct fsl_dma_hw_addr structures
+ * @config: the standard Linux DMAEngine API DMA_SLAVE configuration
  * @request_count: value for DMA request count
- * @src_loop_size: setup and enable constant source-address DMA transfers
- * @dst_loop_size: setup and enable constant destination address DMA transfers
  * @external_start: enable externally started DMA transfers
  * @external_pause: enable externally paused DMA transfers
- *
- * Holds a list of address / length pairs for use with the DMAEngine
- * DMA_SLAVE API implementation for the Freescale DMA controller.
  */
-struct fsl_dma_slave {
+struct fsldma_slave_config {
+   struct dma_slave_config config;
 
-   /* List of hardware address/length pairs */
-   struct list_head addresses;
-
-   /* Support for extra controller features */
unsigned int request_count;
-   unsigned int src_loop_size;
-   unsigned int dst_loop_size;
bool external_start;
bool external_pause;
 };
 
-/**
- * fsl_dma_slave_append - add an address/length pair to a struct fsl_dma_slave
- * @slave: the struct fsl_dma_slave to add to
- * @address: the hardware address to add
- * @length: the length of bytes to transfer from @address
- *
- * Add a hardware address/length pair to a struct fsl_dma_slave. Returns 0 on
- * success, -ERRNO otherwise.
- */
-static inline int fsl_dma_slave_append(struct fsl_dma_slave *slave,
-  dma_addr_t address, size_t length)
-{
-   struct fsl_dma_hw_addr *addr;
-
-   addr = kzalloc(sizeof(*addr), GFP_ATOMIC);
-   if (!addr)
-   return -ENOMEM;
-
-   INIT_LIST_HEAD(addr-entry);
-   addr-address = address;
-   addr-length = length;
-
-   list_add_tail(addr-entry, slave-addresses);
-   return 0;
-}
-
-/**
- * fsl_dma_slave_free - free a struct fsl_dma_slave
- * @slave: the struct fsl_dma_slave to free
- *
- * Free a struct fsl_dma_slave and all associated address/length pairs
- */
-static inline void fsl_dma_slave_free(struct fsl_dma_slave *slave)
-{
-   struct fsl_dma_hw_addr *addr, *tmp;
-
-   if (slave) {
-   list_for_each_entry_safe(addr, tmp, slave-addresses, entry) {
-