diff mbox

[v8,1/3] dmaengine: Add new device_{set,release}_slave callbacks

Message ID 1486650171-20598-2-git-send-email-m.szyprowski@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Marek Szyprowski Feb. 9, 2017, 2:22 p.m. UTC
Add two new callbacks to DMA engine device. They will used to provide
access to slave device (the device which requested given DMA channel)
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.

device_set_slave() will be called after the device_alloc_chan_resources()
and device_release_slave() before the device_free_chan_resources().

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/dma/dmaengine.c   | 27 ++++++++++++++++++++++++---
 include/linux/dmaengine.h | 10 ++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

Comments

Vinod Koul Feb. 10, 2017, 4:34 a.m. UTC | #1
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?

> 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.

> 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

In our context slaves means dmaengine slave devices aka provider.
Client would be the consumer and not slave.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  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.

> +
>  	void *private;
>  };
>  
> @@ -686,6 +689,10 @@ struct dma_filter {
>   * @device_alloc_chan_resources: allocate resources and return the
>   *	number of allocated descriptors
>   * @device_free_chan_resources: release DMA channel's resources
> + * @device_set_slave: provide access to the slave device, which requested
> + *	given DMA channel, called after @device_alloc_chan_resources
> + * @device_release_slave: finishes access to the slave device, called
> + *	before @device_free_chan_resources
>   * @device_prep_dma_memcpy: prepares a memcpy operation
>   * @device_prep_dma_xor: prepares a xor operation
>   * @device_prep_dma_xor_val: prepares a xor validation operation
> @@ -746,6 +753,9 @@ struct dma_device {
>  	int (*device_alloc_chan_resources)(struct dma_chan *chan);
>  	void (*device_free_chan_resources)(struct dma_chan *chan);
>  
> +	int (*device_set_slave)(struct dma_chan *chan, struct device *slave);
> +	void (*device_release_slave)(struct dma_chan *chan);
> +
>  	struct dma_async_tx_descriptor *(*device_prep_dma_memcpy)(
>  		struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
>  		size_t len, unsigned long flags);
> -- 
> 1.9.1
>
Marek Szyprowski Feb. 10, 2017, 12:07 p.m. UTC | #2
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.

>> 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.

>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   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.

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.

 > ...

Best regards
Vinod Koul Feb. 13, 2017, 1:42 a.m. UTC | #3
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@samsung.com>
> >>---
> >>  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.

> 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..
Marek Szyprowski Feb. 13, 2017, 11:48 a.m. UTC | #4
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@samsung.com>
>>>> ---
>>>>   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
diff mbox

Patch

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;
+
 	void *private;
 };
 
@@ -686,6 +689,10 @@  struct dma_filter {
  * @device_alloc_chan_resources: allocate resources and return the
  *	number of allocated descriptors
  * @device_free_chan_resources: release DMA channel's resources
+ * @device_set_slave: provide access to the slave device, which requested
+ *	given DMA channel, called after @device_alloc_chan_resources
+ * @device_release_slave: finishes access to the slave device, called
+ *	before @device_free_chan_resources
  * @device_prep_dma_memcpy: prepares a memcpy operation
  * @device_prep_dma_xor: prepares a xor operation
  * @device_prep_dma_xor_val: prepares a xor validation operation
@@ -746,6 +753,9 @@  struct dma_device {
 	int (*device_alloc_chan_resources)(struct dma_chan *chan);
 	void (*device_free_chan_resources)(struct dma_chan *chan);
 
+	int (*device_set_slave)(struct dma_chan *chan, struct device *slave);
+	void (*device_release_slave)(struct dma_chan *chan);
+
 	struct dma_async_tx_descriptor *(*device_prep_dma_memcpy)(
 		struct dma_chan *chan, dma_addr_t dst, dma_addr_t src,
 		size_t len, unsigned long flags);