Re: [PATCH v2] dmaengine: check device and channel list for empty

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 07/07/2020 9.05, Jiri Slaby wrote:
> On 26. 06. 20, 20:09, Dave Jiang wrote:
>> Check dma device list and channel list for empty before iterate as the
>> iteration function assume the list to be not empty. With devices and
>> channels now being hot pluggable this is a condition that needs to be
>> checked. Otherwise it can cause the iterator to spin forever.
> 
> Could you be a little bit more specific how this can spin forever? I.e.
> can you attach a stacktrace of such a behaviour?
> 
> As in the empty case, "&pos->member" is "head" (look into
> list_for_each_entry) and the for loop should loop exactly zero times.

This is my understanding as well.

Isn't it more plausible that you have race between
dma_async_device_register() / dma_async_device_unregister() /
dma_async_device_channel_register() /
dma_async_device_channel_unregister() ?

It looks like that there is unbalanced locking between
dma_async_device_channel_register() and
dma_async_device_channel_unregister().

The later locks the dma_list_mutex for a short while, while the former
does not.
Both device_register/unregister locks the same dma_list_mutex in some point.

>> Fixes: e81274cd6b52 ("dmaengine: add support to dynamic register/unregister of channels")
>> Reported-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> Tested-by: Swathi Kovvuri <swathi.kovvuri@intel.com>
>> ---
>>
>> Rebased to dmaengine next tree
>>
>>  drivers/dma/dmaengine.c |  119 +++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 94 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
>> index 2b06a7a8629d..0d6529eff66f 100644
>> --- a/drivers/dma/dmaengine.c>> +++ b/drivers/dma/dmaengine.c

...

>> +static int dma_channel_enumeration(struct dma_device *device)
>> +{
>> +	struct dma_chan *chan;
>> +	int rc;
>> +
>> +	if (list_empty(&device->channels))
>> +		return 0;
>> +
>> +	/* represent channels in sysfs. Probably want devs too */
>> +	list_for_each_entry(chan, &device->channels, device_node) {
>> +		rc = __dma_async_device_channel_register(device, chan);
>> +		if (rc < 0)
>> +			return rc;
>> +	}
>> +
>> +	/* take references on public channels */
>> +	if (dmaengine_ref_count && !dma_has_cap(DMA_PRIVATE, device->cap_mask))
>> +		list_for_each_entry(chan, &device->channels, device_node) {
>> +			/* if clients are already waiting for channels we need
>> +			 * to take references on their behalf
>> +			 */
>> +			if (dma_chan_get(chan) == -ENODEV) {
>> +				/* note we can only get here for the first
>> +				 * channel as the remaining channels are
>> +				 * guaranteed to get a reference
>> +				 */
>> +				return -ENODEV;
>> +			}
>> +		}
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * dma_async_device_register - registers DMA devices found
>>   * @device:	pointer to &struct dma_device
>> @@ -1247,33 +1330,15 @@ int dma_async_device_register(struct dma_device *device)
>>  	if (rc != 0)
>>  		return rc;
>>  
>> +	mutex_lock(&dma_list_mutex);
>>  	mutex_init(&device->chan_mutex);
>>  	ida_init(&device->chan_ida);
>> -
>> -	/* represent channels in sysfs. Probably want devs too */
>> -	list_for_each_entry(chan, &device->channels, device_node) {
>> -		rc = __dma_async_device_channel_register(device, chan);
>> -		if (rc < 0)
>> -			goto err_out;
>>
>> +	rc = dma_channel_enumeration(device);
>> +	if (rc < 0) {
>> +		mutex_unlock(&dma_list_mutex);
>> +		goto err_out;
>>  	}

Here you effectively moved the __dma_async_device_channel_register()
under dma_list_mutex.


>>  
>> -	mutex_lock(&dma_list_mutex);
>> -	/* take references on public channels */
>> -	if (dmaengine_ref_count && !dma_has_cap(DMA_PRIVATE, device->cap_mask))
>> -		list_for_each_entry(chan, &device->channels, device_node) {
>> -			/* if clients are already waiting for channels we need
>> -			 * to take references on their behalf
>> -			 */
>> -			if (dma_chan_get(chan) == -ENODEV) {
>> -				/* note we can only get here for the first
>> -				 * channel as the remaining channels are
>> -				 * guaranteed to get a reference
>> -				 */
>> -				rc = -ENODEV;
>> -				mutex_unlock(&dma_list_mutex);
>> -				goto err_out;
>> -			}
>> -		}
>>  	list_add_tail_rcu(&device->global_node, &dma_device_list);
>>  	if (dma_has_cap(DMA_PRIVATE, device->cap_mask))
>>  		device->privatecnt++;	/* Always private */
>> @@ -1291,6 +1356,9 @@ int dma_async_device_register(struct dma_device *device)
>>  		return rc;
>>  	}
>>  
>> +	if (list_empty(&device->channels))
>> +		return rc;
>> +
>>  	list_for_each_entry(chan, &device->channels, device_node) {
>>  		if (chan->local == NULL)
>>  			continue;
>> @@ -1317,8 +1385,9 @@ void dma_async_device_unregister(struct dma_device *device)
>>  
>>  	dmaengine_debug_unregister(device);
>>  
>> -	list_for_each_entry_safe(chan, n, &device->channels, device_node)
>> -		__dma_async_device_channel_unregister(device, chan);
>> +	if (!list_empty(&device->channels))
>> +		list_for_each_entry_safe(chan, n, &device->channels, device_node)
>> +			__dma_async_device_channel_unregister(device, chan);
>>  
>>  	mutex_lock(&dma_list_mutex);
>>  	/*
> 
> 
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki