Messages in this thread | | | Subject | Re: [PATCH v8 1/3] dmaengine: Add new device_{set,release}_slave callbacks | From | Marek Szyprowski <> | Date | Mon, 13 Feb 2017 12:48:45 +0100 |
| |
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
| |