RE: [PATCH 07/13] dmaengine: introduce dma_request_channel andprivate channels

[Sosnowski, Maciej]

Williams, Dan J wrote:
> This interface is primarily for device-to-memory clients which need to
> search for dma channels with platform-specific characteristics.  The
> prototype is:
> 
> struct dma_chan *dma_request_channel(dma_cap_mask_t mask,
>                                      dma_filter_fn filter_fn,
>                                      void *filter_param);
> 
> When the optional 'filter_fn' parameter is set to NULL
> dma_request_channel simply returns the first channel that satisfies
> the 
> capability mask.  Otherwise, when the mask parameter is insufficient
> for 
> specifying the necessary channel, the filter_fn routine can be used to
> disposition the available channels in the system. The filter_fn
> routine 
> is called once for each free channel in the system.  Upon seeing a
> suitable channel filter_fn returns DMA_ACK which flags that channel to
> be the return value from dma_request_channel.  A channel allocated via
> this interface is exclusive to the caller, until dma_release_channel()
> is called.
> 
> To ensure that all channels are not consumed by the general-purpose
> allocator the DMA_PRIVATE capability is provided to exclude a
> dma_device 
> from general-purpose (memory-to-memory) consideration.
> 
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> 
>  drivers/dma/dmaengine.c   |  161
>  ++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/dmaengine.h |   16 ++++ 2 files changed, 159
> insertions(+), 18 deletions(-) 
> 
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index ec483cc..46fd5fa 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -190,7 +190,7 @@ static int dma_chan_get(struct dma_chan *chan)
>  			chan->client_count = 0;
>  			module_put(owner);
>  			err = -ENOMEM;
> -		} else
> +		} else if (!dma_has_cap(DMA_PRIVATE, chan->device->cap_mask))
>  			balance_ref_count(chan);
>  	}
> 
> @@ -221,6 +221,8 @@ static void dma_client_chan_alloc(struct
> dma_client *client) 
> 
>  	/* Find a channel */
>  	list_for_each_entry(device, &dma_device_list, global_node) {
> +		if (dma_has_cap(DMA_PRIVATE, device->cap_mask))
> +			continue;
>  		/* Does the client require a specific DMA controller? */
>  		if (client->slave && client->slave->dma_dev
>  				&& client->slave->dma_dev != device->dev)
> @@ -313,6 +315,7 @@ static int __init dma_channel_table_init(void)
>  	 * channel_table
>  	 */
>  	clear_bit(DMA_INTERRUPT, dma_cap_mask_all.bits);
> +	clear_bit(DMA_PRIVATE, dma_cap_mask_all.bits);
>  	clear_bit(DMA_SLAVE, dma_cap_mask_all.bits);

The comment above should be updated according to this change:
-/* 'interrupt' and 'slave' are channel capabilities, but are not
+/* 'interrupt', 'private' and 'slave' are channel capabilities, but are not

> +static struct dma_chan *private_candidate(dma_cap_mask_t *mask,
> struct dma_device *dev) +{
> +	struct dma_chan *chan;
> +	struct dma_chan *ret = NULL;
> +
> +	/* devices with multiple channels need special handling as we need
> to +	 * ensure that all channels are either private or public.
> +	 */
> +	if (dev->chancnt > 1 && !dma_has_cap(DMA_PRIVATE, dev->cap_mask))
> +		list_for_each_entry(chan, &dev->channels, device_node) {
> +			/* some channels are already publicly allocated */
> +			if (chan->client_count)
> +				return NULL;
> +		}

Isn't it a dangerous approach to let clients consume for their exclusive usage channels
meant for general-purpose ("pubilc" ones)?
Why not to limit private_candidate to devices with DMA_PRIVATE capability only?

> +
> +	list_for_each_entry(chan, &dev->channels, device_node) {
> +		if (!__dma_chan_satisfies_mask(chan, mask)) {
> +			pr_debug("%s: %s wrong capabilities\n",
> +				 __func__, dev_name(&chan->dev));
> +			continue;
> +		}

As capabilities are per device, this check could be performed just once
before list_for_each_entry(chan, &dev->channels, device_node).

Regards,
Maciej--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/