RE: [PATCH 01/11] dmaengine: add context parameter toprep_slave_sg and prep_dma_cyclic

From: Bounine, Alexandre
Date: Fri Feb 03 2012 - 10:33:05 EST


On Thu, Feb 02, 2012 at 4:44 PM, Russell King wrote:
>
> On Thu, Feb 02, 2012 at 04:32:11PM -0500, Alexandre Bounine wrote:
> > Add context parameter to device_prep_slave_sg() and
device_prep_dma_cyclic()
> > interfaces to allow passing client/target specific information
associated
> > with the data transfer.
>
> I'm slightly concerned about having this as a vague 'void *' pointer.
> What that means is that the data being passed through it is entirely
> unspecified, and quite specific to the DMA engine.
>
> That's rather worrying when we (on ARM) are moving towards a model
> where the same peripheral IP can be backed by multiple different DMA
> engines. We already have peripheral drivers (clients of DMA engine)
> which can be attached to completely different DMA engine drivers.
>
> If DMA engines continue to operate conventionally with this parameter
> NULL then that's fine for current stuff. I would suggest that it's
> made to be something a little better defined though, as the 'void *'
> approach encourages each driver writer to invent their own way to
> specify, eg, an interleaved transfer. We'll then end up with N
> different ways to specify the same thing.
>
> Not only that, but peripheral drivers won't know what kind of data to
> pass there (they would have to have additional knowledge of the DMA
> engine which they're attached to.)
>
> Basically, allowing a DMA engine specific blob of data to be passed
> destroys the idea of having a consistent API for peripheral drivers
> to use, because they can then no longer be independent of their DMA
> engine.
>
I agree with your concern about "void *" being quite wide-open
definition.
But it may be practically impossible to come up with better definition
that may be equally suitable for different systems or HW combinations
(e.g. definition that is good for ARM peripherals may not fit into the
RapidIO model).

Some level of protection should be expected from filter callbacks
invoked
by dma_request_channel(). This way, for example, RapidIO client will not
acquire DMA channel which does not support RapidIO data transfers.

After saying all above to defend the changes, I have to admit that your
comment made me to look harder for an alternative way to pass
per-transfer
parameter. As a result, I am considering a new idea when per-transfer
configuration data will be passed as the first element of the scatter
list.
Something like a packet header data but it will be used to setup
buffer descriptors instead of being part of data stream.

This new approach will preserve existing DMA engine interface and be
easier
for backporting of RapidIO drivers. Yes, there is a system dependency in
scatter
list format but it is completely protected by channel filtering
mechanism.

Because of that it may be a good idea to consider this set of patches
being "on hold" unless there are other parties interested in proposed
interface changes.

AB.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/