From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753344AbdBJMHw (ORCPT ); Fri, 10 Feb 2017 07:07:52 -0500 Received: from mailout3.w1.samsung.com ([210.118.77.13]:14572 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752339AbdBJMHs (ORCPT ); Fri, 10 Feb 2017 07:07:48 -0500 X-AuditID: cbfec7f1-f793f6d000007796-e9-589dad0f378f Subject: Re: [PATCH v8 1/3] dmaengine: Add new device_{set,release}_slave callbacks To: Vinod Koul , Ulf Hansson Cc: 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: <05d42359-1e72-3ec1-7d86-86135a8f013e@samsung.com> Date: Fri, 10 Feb 2017 13:07:41 +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: <20170210043442.GM19244@localhost> Content-type: text/plain; charset=utf-8; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SaUgUYRjHeXd2dkdralw1H6yUlg5S80JhsJACPwzkh6JDkciWHFQ82VFJ C9wtvKY0WzVlPTLP8MBtXUzFStfwxltDEiOkzNJETPFAS3dW8NvveZ7/8/75vzwEJuvA7Ynw 6DhWGa2IlEssxU1dG4MXjtYXB7rnTzvT25ouKf22oAGna/O2cVrzNVtMDw3ppHRFziuc1s9O 4vRYa5GEXsn8hOiCoQ8ieqB/FKe76wPo+ayP4ssks7WpQUx527yI0ddkSJjpyTYJ01iRzFQs GHHGMJEmZrIMNYhZ0TtcswiyvBTCRoYnsEo333uWYaMbOixWe/6BpnpEqkKqUzyyIIDygsLf T5HAx2B4pkHCI0tCRlUi+FO1Zi5WEPSo+qX7G09SqqTCoArB5up3czGHoGWpVbynsqZuwVYJ j++xDXUVxterTSKMysVgaKDaJJJQHsAv8pI9JilfGGlNNbGYOgOrz6pNdrbUHcjUv8QEjRWs 58yYdi0oN9Bnz5j6GOUDP3ZScIEdobFuEdszA2pZCmmvc0Q8InaLk6Bvx4QIftCjzjPHsYZf 3QYzn4CM9A6RwM8RPE5xFrgAweAiKfBF6OweMXsdAU1TPiY8T0J6qkyQMGBQlZvbV0D3Pkj4 H5UI8lbWpNnIUXsgjfZAAu2BBKUIq0E2bDwXFcpynq6cIoqLjw51vR8TpUe7h9W/073cjJZ6 fIyIIpD8MPnzUVGgDFckcIlRRgQEJrchIyqLA2VkiCIxiVXGBCvjI1nOiI4TYrkd2VY6HiCj QhVxbATLxrLK/amIsLDfPY7r4TLbZr4woazPf+ed0X2YezORZPxc9s19dm4yX9vrH0ZW4HZT 6iabhfVOjOtw6Ux+uC65e2iabwnN80r2O9eo3sosVI/19vHtTvLMXk/ayuFvZZZnjO7F7YnI bO95PmH+9FnvOqM690twztSNYd9aQ8nN8FiXf4a2/PgCnVzMhSk8nDAlp/gPmxrmm1QDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrMIsWRmVeSWpSXmKPExsVy+t/xa7rRa+dGGKx/JWTxd9IxdouNM9az Wqye+pfVYtL9CSwW589vYLdYMnk+q8Wmx9dYLS7vmsNm8bn3CKPFjPP7mCzOnL7EanF8bbjF y779LA68Hr9/TWL0WLznJZPHplWdbB53ru1h89i8pN5jyZtDrB5brrazePRtWcXo8XmTXABn lJtNRmpiSmqRQmpecn5KZl66rVJoiJuuhZJCXmJuqq1ShK5vSJCSQlliTimQZ2SABhycA9yD lfTtEtwyLv3cwFwwS7Ni0vKL7A2MDYpdjJwcEgImEs2ty9ghbDGJC/fWs3UxcnEICSxhlJh3 dw8jhPOcUWLq1kPMIFXCAqESv+d1sYLYIgLeEld+LGeHKGpgktj+5SAziMMsMIVZYuKf92BV bAKGEl1vu9hAbF4BO4mLu9rAbBYBVYmvPcvBdosKxEjs7b/PBFEjKPFj8j0WEJtTQF9i04R7 YJuZBcwkvrw8zAphy0tsXvOWeQKjwCwkLbOQlM1CUraAkXkVo0hqaXFuem6xoV5xYm5xaV66 XnJ+7iZGYARvO/Zz8w7GSxuDDzEKcDAq8fBOqJoTIcSaWFZcmXuIUYKDWUmEN3vp3Agh3pTE yqrUovz4otKc1OJDjKZAT0xklhJNzgcml7ySeEMTQ3NLQyNjCwtzIyMlcd6SD1fChQTSE0tS s1NTC1KLYPqYODilGhj1RMSM73uZpa1cV7T+h2Xcucus3MZuyyPz/Tfu+zVhiSz3tRbupqNh hutOrfr/ehZTyB4250OHqmT1D1XLpHhXbV0fI7A/OcPWVGWFr1ZmBN929hv8QueWnq3K3ZXi Ps1IYvvdbb65N1zmZIV5Prm6p9DI8vVlDaNKQ8sNCy7unOPcWfKOp0GJpTgj0VCLuag4EQCo hs2J9gIAAA== X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20170210120743eucas1p192a3cca52d2b547d1904ec35a843c4fb X-Msg-Generator: CA X-Sender-IP: 182.198.249.179 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> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 -- Marek Szyprowski, PhD Samsung R&D Institute Poland