From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752760AbdBMLsy (ORCPT ); Mon, 13 Feb 2017 06:48:54 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:9648 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751652AbdBMLsv (ORCPT ); Mon, 13 Feb 2017 06:48:51 -0500 X-AuditID: cbfec7ef-f79d26d00000420c-0c-58a19d2153e0 Subject: Re: [PATCH v8 1/3] dmaengine: Add new device_{set,release}_slave callbacks To: Vinod Koul Cc: Ulf Hansson , linux-samsung-soc@vger.kernel.org, dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, Krzysztof Kozlowski , Bartlomiej Zolnierkiewicz , "Rafael J. Wysocki" , Lars-Peter Clausen , Arnd Bergmann , Inki Dae From: Marek Szyprowski Message-id: Date: Mon, 13 Feb 2017 12:48:45 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-version: 1.0 In-reply-to: <20170213014229.GG2843@localhost> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrNKsWRmVeSWpSXmKPExsWy7djPc7qKcxdGGDRstbH4O+kYu8XGGetZ LVZP/ctqMen+BBaL8+c3sFssmTyf1WLT42usFpd3zWGz+Nx7hNFixvl9TBZnTl9itTi+Ntzi Zd9+Fgdej9+/JjF6LN7zkslj06pONo871/aweWxeUu+x5M0hVo8tV9tZPPq2rGL0+LxJLoAz issmJTUnsyy1SN8ugSujacF79oKf+hUHZ+1ibWB8odbFyMkhIWAisWLKXmYIW0ziwr31bCC2 kMAyRonzLQFdjFxA9mdGie1fJwAVcYA19H4KhKtZ+lccwn7OKLH1awyILSwQKvF7XhcriC0i oCqx5WcHI8gcZoE9zBJdLw+CLWMTMJToetvFBjKTV8BO4vl9H5AwC1D92/52dhBbVCBGonfT NLByXgFBiR+T77GA2JwCehLrr81hBLGZBawknv1rZYWw5SU2r3kL9ctXdonr1zMhTpaV2HQA Kuwi0bnsJJQtLPHq+BZ2CFtG4vLkbhYIu59RoqlVG8KewShx7i0vhG0tcfj4RahVfBKTtk2H hgivREebEESJh8SWhsVQYUeJDXujIAF4k0ni1u0m1gmM8rOQPDMLyQOzkDywgJF5FaNIamlx bnpqsaFecWJucWleul5yfu4mRmCSOv3v+PsdjE+bQw4xCnAwKvHwNrQtiBBiTSwrrsw9xCjB wawkwusye2GEEG9KYmVValF+fFFpTmrxIUZpDhYlcd69C66ECwmkJ5akZqemFqQWwWSZODil GhizP85n+nZ9zZx4S/3kNyxR69SvC0jEbCxqOuk1/8SUbqYvRR/7SxLc0l4HHWv86S5Xlhe9 tSEgQi14flrpigddXZtFr/iYRbPnz59xS0o2b1+ob9CR48tSuLOfKK//8lNa7d7tn39LFbb/ lD7Z8+xkcW/F+eoJX70vWYYc+3E2REldLF5lz00lluKMREMt5qLiRABn3j+7TgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrIIsWRmVeSWpSXmKPExsVy+t/xq7qKcxdGGDzcZGHxd9IxdouNM9az Wqye+pfVYtL9CSwW589vYLdYMnk+q8Wmx9dYLS7vmsNm8bn3CKPFjPP7mCzOnL7EanF8bbjF y779LA68Hr9/TWL0WLznJZPHplWdbB53ru1h89i8pN5jyZtDrB5brrazePRtWcXo8XmTXABn lJtNRmpiSmqRQmpecn5KZl66rVJoiJuuhZJCXmJuqq1ShK5vSJCSQlliTimQZ2SABhycA9yD lfTtEtwymha8Zy/4qV9xcNYu1gbGF2pdjBwcEgImEr2fArsYOYFMMYkL99azdTFycQgJLGGU mDHnLxOE85xRYvv0D6wgVcICoRK/53WB2SICqhJbfnYwQhTdZJL4u2w6K4jDLLCHWeLzp/2M IFVsAoYSXW+72EDW8QrYSTy/7wMSZgFqftvfzg5iiwrESOztv88EYvMKCEr8mHyPBcTmFNCT WH9tDtgYZgEziS8vD7NC2PISm9e8ZZ7AKDALScssJGWzkJQtYGRexSiSWlqcm55bbKRXnJhb XJqXrpecn7uJERi924793LKDsetd8CFGAQ5GJR7ehrYFEUKsiWXFlbmHGCU4mJVEeF1mL4wQ 4k1JrKxKLcqPLyrNSS0+xGgK9MREZinR5HxgYskriTc0MTS3NDQytrAwNzJSEued+uFKuJBA emJJanZqakFqEUwfEwenVANjmGXmIVOW+RpafvvdeIyFTu7QML1guz0rzaSeZcHFiI3L7hT9 EjfR8e9vUZ3kHZC+4eqK9dYZ31I/fP5qErT51jZ7v9d66kE7UpddZQ6TZEiYJ7Eo9YaRXSLX 5evf7actFPn5O+DdPc0HV+uMfzJb72yLUNy5u+6iKA9Hx38TfRsR7ZplRipKLMUZiYZazEXF iQDm14/J9AIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170213114846eucas1p106db985d7b76eec2743ab08211a4266d X-Msg-Generator: CA X-Sender-IP: 182.198.249.180 X-Local-Sender: =?UTF-8?B?TWFyZWsgU3p5cHJvd3NraRtTUlBPTC1LZXJuZWwgKFRQKRs=?= =?UTF-8?B?7IK87ISx7KCE7J6QG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?TWFyZWsgU3p5cHJvd3NraRtTUlBPTC1LZXJuZWwgKFRQKRtT?= =?UTF-8?B?YW1zdW5nIEVsZWN0cm9uaWNzG1NlbmlvciBTb2Z0d2FyZSBFbmdpbmVlcg==?= X-Sender-Code: =?UTF-8?B?QzEwG0VIURtDMTBDRDAyQ0QwMjczOTI=?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20170209142307eucas1p180323d005f524760913b8d04ac966423 X-RootMTR: 20170209142307eucas1p180323d005f524760913b8d04ac966423 References: <1486650171-20598-1-git-send-email-m.szyprowski@samsung.com> <1486650171-20598-2-git-send-email-m.szyprowski@samsung.com> <20170210043442.GM19244@localhost> <05d42359-1e72-3ec1-7d86-86135a8f013e@samsung.com> <20170213014229.GG2843@localhost> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> --- >>>> 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