linux-kernel - Re: [PATCH v8 1/3] dmaengine: Add new device_{set,release}

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 10 Feb 2017 10:04:42 +0530
From:   Vinod Koul <vinod.koul@...el.com>
To:     Marek Szyprowski <m.szyprowski@...sung.com>
Cc:     linux-samsung-soc@...r.kernel.org, dmaengine@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Arnd Bergmann <arnd@...db.de>, Inki Dae <inki.dae@...sung.com>
Subject: Re: [PATCH v8 1/3] dmaengine: Add new device_{set,release}_slave
 callbacks

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

-- 
~Vinod