linux-kernel - Re: [PATCH] intel_mid: Add Mrst & Mfld DMA Drivers

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <AANLkTilvD6KKEY3GE2Q0q3z4gu6K6RwkMWbRaRPl9_pU@mail.gmail.com>
Date:	Tue, 22 Jun 2010 17:40:44 -0700
From:	Dan Williams <dan.j.williams@...el.com>
To:	Alan Cox <alan@...ux.intel.com>
Cc:	vinod.koul@...el.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] intel_mid: Add Mrst & Mfld DMA Drivers

On Fri, Jun 18, 2010 at 8:51 AM, Alan Cox <alan@...ux.intel.com> wrote:
> From: Vinod Koul <vinod.koul@...el.com>
>
> This patch add DMA drivers for DMA controllers in Langwell chipset
> of Intel(R) Moorestown platform and DMA controllers in Penwell of
> Intel(R) Medfield platfrom
>
> This patch adds support for Moorestown DMAC1 and DMAC2 controllers.
> It also add support for Medfiled GP DMA and DMAC1 controllers.
> These controllers supports memory to peripheral and peripheral to
> memory transfers
> It support only single block transfers
> This driver is based on Kernel DMA engine
> Anyone who wishes to use this controller should use DMA engine APIs
>
> This controller exposes DMA_SLAVE capabilities and notifies the client drivers
> of DMA transaction completion
>
> Signed-off-by: Vinod Koul <vinod.koul@...el.com>
> Signed-off-by: Alan Cox <alan@...ux.intel.com>
> ---
[..]
> ++ *    midc_do_start           begin a transaction
> ++ *    @midc: channel
> ++ *    @first: first descriptor of series
> ++ *
> ++ *    Load a transaction into the engine. This must be called with dwc->lock
> ++ *    held and bh disabled.
> ++ */
> +static void midc_dostart(struct intel_mid_dma_chan *midc, struct intel_mid_dma_desc

(nit) The function name does not match the kernel doc, and I don't
know if those extra "+" characters will throw off the kernel-doc
scripts.  They also show up on midc_put_desc, but not
midc_scan_descriptors.

> +/*check desc, mark as complete when tx is complete*/
> +static void
> +midc_scan_descriptors(struct middma_device *mid, struct intel_mid_dma_chan *midc)
> +{
> +       struct intel_mid_dma_desc *desc = NULL, *_desc = NULL;
> +/*     u32 status_xfer; */
> +
> +       dma_dbg("called \n");
> +/*     status_xfer = ioread32(midc->dma_base + RAW_TFR);
> +       status_xfer = (status_xfer >> midc->ch_id) & 0x1;
> +       dma_dbg("ch[%d]:  status_xfer %x \n", midc->ch_id, status_xfer);
> +       if (!status_xfer)
> +               return;
> +*/
> +       /*tx is complete*/
> +       list_for_each_entry_safe(desc, _desc, &midc->active_list, desc_node) {
> +               if (desc == NULL)
> +                       continue;

How do we ever get desc == NULL at this point?

> +               if (desc->status == DMA_IN_PROGRESS)  {
> +                       desc->status = DMA_SUCCESS;
> +                       midc_descriptor_complete(midc, desc);
> +               }
> +       }
> +       return;
> +}
> +
[..]
> +#define _dma_printk(level, format, arg...)  \
> +       printk(level "LNW_DMA: %s %d " format, __func__, __LINE__, ## arg)
> +
> +#ifdef CONFIG_LNW_DMA_DEBUG
> +#define dma_dbg(format, arg...) _dma_printk(KERN_DEBUG, "DBG " format , ## arg)
> +#else
> +#define dma_dbg(format, arg...) do {} while (0);
> +#endif

This makes us lose compile coverage of the debug statements so they
bitrot until someone needs to debug a problem.

[..]
> +struct intel_mid_dma_slave {
> +       enum dma_data_direction         dirn;
> +       enum intel_mid_dma_width        src_width; /*width of DMA src txn*/
> +       enum intel_mid_dma_width        dst_width; /*width of DMA dst txn*/
> +       enum intel_mid_dma_hs_mode      hs_mode;  /*handshaking*/
> +       enum intel_mid_dma_mode         cfg_mode; /*mode configuration*/
> +       enum intel_mid_dma_msize        src_msize; /*size if src burst*/
> +       enum intel_mid_dma_msize        dst_msize; /*size of dst burst*/
> +       dma_async_tx_callback           callback; /*callback function*/
> +       void                            *callback_param; /*param for callback*/

Any advantage to having a default callback in the slave configuration?
 Why not let the client specify the callback on each operation, is it
a callback that the client does not know about?
--
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/