linux-kernel - Re: [PATCH 1/3] DMAENGINE: generic slave channel control

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: <AANLkTilZ14AEPqhwfvPbmvuBOIeSZPlHa1u4DR2GuoEC@mail.gmail.com>
Date:	Tue, 20 Jul 2010 23:26:40 +0200
From:	Linus Walleij <linus.ml.walleij@...il.com>
To:	"Koul, Vinod" <vinod.koul@...el.com>
Cc:	"Williams, Dan J" <dan.j.williams@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Alan Cox <alan@...ux.intel.com>
Subject: Re: [PATCH 1/3] DMAENGINE: generic slave channel control

2010/7/20 Koul, Vinod <vinod.koul@...el.com>:

> /**
>  * struct intel_mid_dma_slave - DMA slave structure
>  *
>  * @dirn: DMA trf direction
>  * @src_width: tx register width
>  * @dst_width: rx register width
>  * @hs_mode: HW/SW handshaking mode
>  * @cfg_mode: DMA data transfer mode (per-per/mem-per/mem-mem)
>  * @src_msize: Source DMA burst size
>  * @dst_msize: Dst DMA burst size
>  * @device_instance: DMA peripheral device instance, we can have multiple
>  *                      peripheral device connected to single DMAC
>  */
> 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*/
>        unsigned int            device_instance; /*0, 1 for peripheral instance*/
> };
>
> Yes the structures have various common things and would be good to abstract
> generic stuff in this and move the rest to private_config.
> But since we are talking about a generic DMA slave capabilities structure, I
> would recommend changing few.
>
> I am not sure why do you want the I/O addr to be passed here, sorry I
> don't follow that logic. If you notice in intel_mid_dma driver the I/O address
> is passed by client driver in prep_memcpy in src and dst fields. Since the slave
> structure tell you the direction and mode you can interpret that src/dst address
> as IO address easily

? Are you using memcpy() to talk to slaves?

I was assuming all slave communication was to use sglists through
the .device_prep_slave_sg() call. This is currently the design constraint,
memcpy() will by design increase both source and destination address
and also always operate on the memory bus.

(If you need reconfiguration also for memcpy() I think will be a
different issue, I'm only looking at slaves now.)

With only the slave interface the dma_chan struct can be deferred
by the DMA engine into a local struct which has this address configured
from the platform, statically.

The runtime configuration API is exactly about being able to reconfigure
even the source/destination address at runtime. This is why
these are on the interface.

> Addr_width and Maxburst: talks about the I/O width and msize I guess, but what
> about mem-width, I have few configuration where we would like to have different
> mem width as well.

OK then we need that for both src and dst.

> And why limit the generic API

Just trying to see how small we can get it, less things can go wrong.

> and what about per-per transfers which
> intel_mid_dma driver would need to support in future.

Yep per2per will definately need that.

> I would recommend renaming this field to src_addr_width
> etc and add dst_addr_xxx
> pair.

OK if Dan agrees I'll make a try.

> Yes I can have them in private_config but then again driver has to do a simple
> check on which one is src/dst in slave and which one is in privae_config. Again
> easily doable but little confusing, I am okay either way

No if you're already foreseeing that they'll be needed, lets put
them in from the beginning...

Yours,
Linus Walleij
--
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/