Re: [PATCH v8 1/3] dmaengine: Add new device_{set,release}_slave callbacks
From: Marek Szyprowski
Date: Mon Feb 13 2017 - 06:48:58 EST
Hi Vinod,
On 2017-02-13 02:42, Vinod Koul wrote:
On Fri, Feb 10, 2017 at 01:07:41PM +0100, Marek Szyprowski wrote:
Hi Vinod,
On 2017-02-10 05:34, Vinod Koul wrote:
On Thu, Feb 09, 2017 at 03:22:49PM +0100, Marek Szyprowski wrote:
Add two new callbacks to DMA engine device. They will used to provide
access to slave device (the device which requested given DMA channel)
You mean access to client devices?
Yes. It looks that I was confused by the code, where the term 'slave'
appears a few times. 'Client' is a bit more appropriate then.
for DMA engine driver. Access to slave device might be useful for example
for implementing advanced runtime power management.
DMA slave channels are exclusive, so only one slave device can be set
for a given DMA slave channel.
That is not a right assumption and my worry here. With virt-dma we don't
really assume a hardware channel and exclusive. Certain implementation may
do that but from framework we cannot assume that.
Okay, I came to such conclusion basing one the dma engine code, but maybe
I missed something. However in such case such callback will be called for
each client device and it will be up to the driver to handle that.
Thats right, but the assumption that we will have once physical channel
maynot be true.
device_set_slave() will be called after the device_alloc_chan_resources()
and device_release_slave() before the device_free_chan_resources().
Okay, I had to relook at the series to get around this part. Sorry but we
can't call it set_slave, it is actually set_client/consumer
That's okay, the name of the callbacks should be changed.
In our context slaves means dmaengine slave devices aka provider.
Client would be the consumer and not slave.
I'm a new to the DMA engine framework, I'm sorry for using wrong terms.
That's fine :-) we all learn incrementally.
Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
---
drivers/dma/dmaengine.c | 27 ++++++++++++++++++++++++---
include/linux/dmaengine.h | 10 ++++++++++
2 files changed, 34 insertions(+), 3 deletions(-)
diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 24e0221fd66d..5b7089d8be4d 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -705,6 +705,7 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
{
struct dma_device *d, *_d;
struct dma_chan *chan = NULL;
+ int ret;
/* If device-tree is present get slave info from here */
if (dev->of_node)
@@ -715,8 +716,9 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
chan = acpi_dma_request_slave_chan_by_name(dev, name);
if (chan) {
- /* Valid channel found or requester need to be deferred */
- if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER)
+ if (!IS_ERR(chan))
+ goto found;
+ if (PTR_ERR(chan) == -EPROBE_DEFER)
return chan;
}
@@ -738,7 +740,21 @@ struct dma_chan *dma_request_chan(struct device *dev, const char *name)
}
mutex_unlock(&dma_list_mutex);
- return chan ? chan : ERR_PTR(-EPROBE_DEFER);
+ if (!chan)
+ return ERR_PTR(-EPROBE_DEFER);
+ if (IS_ERR(chan))
+ return chan;
+found:
+ if (chan->device->device_set_slave) {
+ chan->slave = dev;
+ ret = chan->device->device_set_slave(chan, dev);
+ if (ret) {
+ chan->slave = NULL;
+ dma_release_channel(chan);
+ chan = ERR_PTR(ret);
+ }
+ }
+ return chan;
}
EXPORT_SYMBOL_GPL(dma_request_chan);
@@ -786,6 +802,11 @@ void dma_release_channel(struct dma_chan *chan)
mutex_lock(&dma_list_mutex);
WARN_ONCE(chan->client_count != 1,
"chan reference count %d != 1\n", chan->client_count);
+ if (chan->slave) {
+ if (chan->device->device_release_slave)
+ chan->device->device_release_slave(chan);
+ chan->slave = NULL;
+ }
dma_chan_put(chan);
/* drop PRIVATE cap enabled by __dma_request_channel() */
if (--chan->device->privatecnt == 0)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 533680860865..d22299e37e69 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -277,6 +277,9 @@ struct dma_chan {
struct dma_router *router;
void *route_data;
+ /* Only for SLAVE channels */
+ struct device *slave;
so assuming you refer to consumer aka client here, why do we need set if we
store it here.
DMA engine driver might need to do something with it (like setting up a pm
link for example) before starting any operations. It would be great if the
pointer to client device is available in device_alloc_chan_resources(), but
propagating it there is not possible without significant changes. That's why
I came with this a separate callback.
But then it gets the client device using the callback as well. So if we
retain that, this should go away.
Yes, that it would be an alternative solution to set/clear_client().
Maybe the client device shouldn't be stored in the dma_chan structure at all
and left to the drivers to use or manage it if really needed. This will also
solve the issue with virt-dma you have mentioned.
In the previous version I managed to pass client device pointer to
device_alloc_chan_resources() via of_xlate callback (please take a look into
v7), but that approach was rejected by Lars-Peter Clausen.
I feel this is better approach, perhaps we don't need the client pointer
here..
Then this is exactly what was implemented in v7 of this patchset. Could
you then
take a look at it? Or do you want me to resend it as v9?
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland