Re: [RFC v02 02/15] dmaengine: core: Move and merge the code paths using private_candidate

[Peter Ujfalusi]

On 11/30/2015 04:42 PM, Andy Shevchenko wrote:
> On Mon, Nov 30, 2015 at 3:45 PM, Peter Ujfalusi  wrote:
>> Channel matching with private_candidate() is used in two paths, the error
>> checking is slightly different in them and they are duplicating code also.
>> Move the code under dma_get_channel() to provide consistent execution and
>> going to allow us to reuse this mode of channel lookup later.
>>
>> Signed-off-by: Peter Ujfalusi 
>> ---
>>  drivers/dma/dmaengine.c | 81 
>> +
>>  1 file changed, 42 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>> index 52c3eee48e2e..1249165fb4b2 100644
>> --- a/drivers/dma/dmaengine.c
>> +++ b/drivers/dma/dmaengine.c
>> @@ -549,6 +549,42 @@ static struct dma_chan *private_candidate(const 
>> dma_cap_mask_t *mask,
>> return NULL;
>>  }
>>
>> +static struct dma_chan *dma_get_channel(struct dma_device *device,
> 
> Naming scheme inside dmaengine.c looks like a mess.

Yes, I agree.

> 
> Since it's static function that utilizes private_candidate() may I
> propose the name like find_candidate() ?

I had __* version as well, but the find_candidate() sounds better.

Thanks,
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC v02 02/15] dmaengine: core: Move and merge the code paths using private_candidate

[Peter Ujfalusi]

Channel matching with private_candidate() is used in two paths, the error
checking is slightly different in them and they are duplicating code also.
Move the code under dma_get_channel() to provide consistent execution and
going to allow us to reuse this mode of channel lookup later.

Signed-off-by: Peter Ujfalusi 
---
 drivers/dma/dmaengine.c | 81 +
 1 file changed, 42 insertions(+), 39 deletions(-)

diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
index 52c3eee48e2e..1249165fb4b2 100644
--- a/drivers/dma/dmaengine.c
+++ b/drivers/dma/dmaengine.c
@@ -549,6 +549,42 @@ static struct dma_chan *private_candidate(const 
dma_cap_mask_t *mask,
return NULL;
 }
 
+static struct dma_chan *dma_get_channel(struct dma_device *device,
+   const dma_cap_mask_t *mask,
+   dma_filter_fn fn, void *fn_param)
+{
+   struct dma_chan *chan = private_candidate(mask, device, fn, fn_param);
+   int err;
+
+   if (chan) {
+   /* Found a suitable channel, try to grab, prep, and return it.
+* We first set DMA_PRIVATE to disable balance_ref_count as this
+* channel will not be published in the general-purpose
+* allocator
+*/
+   dma_cap_set(DMA_PRIVATE, device->cap_mask);
+   device->privatecnt++;
+   err = dma_chan_get(chan);
+
+   if (err) {
+   if (err == -ENODEV) {
+   pr_debug("%s: %s module removed\n", __func__,
+dma_chan_name(chan));
+   list_del_rcu(>global_node);
+   } else
+   pr_debug("%s: failed to get %s: (%d)\n",
+__func__, dma_chan_name(chan), err);
+
+   if (--device->privatecnt == 0)
+   dma_cap_clear(DMA_PRIVATE, device->cap_mask);
+
+   chan = ERR_PTR(err);
+   }
+   }
+
+   return chan ? chan : ERR_PTR(-EPROBE_DEFER);
+}
+
 /**
  * dma_get_slave_channel - try to get specific channel exclusively
  * @chan: target channel
@@ -587,7 +623,6 @@ struct dma_chan *dma_get_any_slave_channel(struct 
dma_device *device)
 {
dma_cap_mask_t mask;
struct dma_chan *chan;
-   int err;
 
dma_cap_zero(mask);
dma_cap_set(DMA_SLAVE, mask);
@@ -595,23 +630,11 @@ struct dma_chan *dma_get_any_slave_channel(struct 
dma_device *device)
/* lock against __dma_request_channel */
mutex_lock(_list_mutex);
 
-   chan = private_candidate(, device, NULL, NULL);
-   if (chan) {
-   dma_cap_set(DMA_PRIVATE, device->cap_mask);
-   device->privatecnt++;
-   err = dma_chan_get(chan);
-   if (err) {
-   pr_debug("%s: failed to get %s: (%d)\n",
-   __func__, dma_chan_name(chan), err);
-   chan = NULL;
-   if (--device->privatecnt == 0)
-   dma_cap_clear(DMA_PRIVATE, device->cap_mask);
-   }
-   }
+   chan = dma_get_channel(device, , NULL, NULL);
 
mutex_unlock(_list_mutex);
 
-   return chan;
+   return IS_ERR(chan) ? NULL : chan;
 }
 EXPORT_SYMBOL_GPL(dma_get_any_slave_channel);
 
@@ -628,35 +651,15 @@ struct dma_chan *__dma_request_channel(const 
dma_cap_mask_t *mask,
 {
struct dma_device *device, *_d;
struct dma_chan *chan = NULL;
-   int err;
 
/* Find a channel */
mutex_lock(_list_mutex);
list_for_each_entry_safe(device, _d, _device_list, global_node) {
-   chan = private_candidate(mask, device, fn, fn_param);
-   if (chan) {
-   /* Found a suitable channel, try to grab, prep, and
-* return it.  We first set DMA_PRIVATE to disable
-* balance_ref_count as this channel will not be
-* published in the general-purpose allocator
-*/
-   dma_cap_set(DMA_PRIVATE, device->cap_mask);
-   device->privatecnt++;
-   err = dma_chan_get(chan);
+   chan = dma_get_channel(device, mask, fn, fn_param);
+   if (!IS_ERR(chan))
+   break;
 
-   if (err == -ENODEV) {
-   pr_debug("%s: %s module removed\n",
-__func__, dma_chan_name(chan));
-   list_del_rcu(>global_node);
-   } else if (err)
-   pr_debug("%s: failed to get %s: (%d)\n",
-__func__, 

Re: [RFC v02 02/15] dmaengine: core: Move and merge the code paths using private_candidate

[Andy Shevchenko]

On Mon, Nov 30, 2015 at 3:45 PM, Peter Ujfalusi  wrote:
> Channel matching with private_candidate() is used in two paths, the error
> checking is slightly different in them and they are duplicating code also.
> Move the code under dma_get_channel() to provide consistent execution and
> going to allow us to reuse this mode of channel lookup later.
>
> Signed-off-by: Peter Ujfalusi 
> ---
>  drivers/dma/dmaengine.c | 81 
> +
>  1 file changed, 42 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index 52c3eee48e2e..1249165fb4b2 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -549,6 +549,42 @@ static struct dma_chan *private_candidate(const 
> dma_cap_mask_t *mask,
> return NULL;
>  }
>
> +static struct dma_chan *dma_get_channel(struct dma_device *device,

Naming scheme inside dmaengine.c looks like a mess.

Since it's static function that utilizes private_candidate() may I
propose the name like find_candidate() ?

> +   const dma_cap_mask_t *mask,
> +   dma_filter_fn fn, void *fn_param)
> +{
> +   struct dma_chan *chan = private_candidate(mask, device, fn, fn_param);
> +   int err;
> +
> +   if (chan) {
> +   /* Found a suitable channel, try to grab, prep, and return it.
> +* We first set DMA_PRIVATE to disable balance_ref_count as 
> this
> +* channel will not be published in the general-purpose
> +* allocator
> +*/
> +   dma_cap_set(DMA_PRIVATE, device->cap_mask);
> +   device->privatecnt++;
> +   err = dma_chan_get(chan);
> +
> +   if (err) {
> +   if (err == -ENODEV) {
> +   pr_debug("%s: %s module removed\n", __func__,
> +dma_chan_name(chan));
> +   list_del_rcu(>global_node);
> +   } else
> +   pr_debug("%s: failed to get %s: (%d)\n",
> +__func__, dma_chan_name(chan), err);
> +
> +   if (--device->privatecnt == 0)
> +   dma_cap_clear(DMA_PRIVATE, device->cap_mask);
> +
> +   chan = ERR_PTR(err);
> +   }
> +   }
> +
> +   return chan ? chan : ERR_PTR(-EPROBE_DEFER);
> +}
> +
>  /**
>   * dma_get_slave_channel - try to get specific channel exclusively
>   * @chan: target channel
> @@ -587,7 +623,6 @@ struct dma_chan *dma_get_any_slave_channel(struct 
> dma_device *device)
>  {
> dma_cap_mask_t mask;
> struct dma_chan *chan;
> -   int err;
>
> dma_cap_zero(mask);
> dma_cap_set(DMA_SLAVE, mask);
> @@ -595,23 +630,11 @@ struct dma_chan *dma_get_any_slave_channel(struct 
> dma_device *device)
> /* lock against __dma_request_channel */
> mutex_lock(_list_mutex);
>
> -   chan = private_candidate(, device, NULL, NULL);
> -   if (chan) {
> -   dma_cap_set(DMA_PRIVATE, device->cap_mask);
> -   device->privatecnt++;
> -   err = dma_chan_get(chan);
> -   if (err) {
> -   pr_debug("%s: failed to get %s: (%d)\n",
> -   __func__, dma_chan_name(chan), err);
> -   chan = NULL;
> -   if (--device->privatecnt == 0)
> -   dma_cap_clear(DMA_PRIVATE, device->cap_mask);
> -   }
> -   }
> +   chan = dma_get_channel(device, , NULL, NULL);
>
> mutex_unlock(_list_mutex);
>
> -   return chan;
> +   return IS_ERR(chan) ? NULL : chan;
>  }
>  EXPORT_SYMBOL_GPL(dma_get_any_slave_channel);
>
> @@ -628,35 +651,15 @@ struct dma_chan *__dma_request_channel(const 
> dma_cap_mask_t *mask,
>  {
> struct dma_device *device, *_d;
> struct dma_chan *chan = NULL;
> -   int err;
>
> /* Find a channel */
> mutex_lock(_list_mutex);
> list_for_each_entry_safe(device, _d, _device_list, global_node) {
> -   chan = private_candidate(mask, device, fn, fn_param);
> -   if (chan) {
> -   /* Found a suitable channel, try to grab, prep, and
> -* return it.  We first set DMA_PRIVATE to disable
> -* balance_ref_count as this channel will not be
> -* published in the general-purpose allocator
> -*/
> -   dma_cap_set(DMA_PRIVATE, device->cap_mask);
> -   device->privatecnt++;
> -   err = dma_chan_get(chan);
> +   chan = dma_get_channel(device, mask, fn, fn_param);
> +   if