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

[Marek Szyprowski <m.szyprowski@samsung.com>]

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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland