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 <[email protected]>
>> Signed-off-by: Dave Jiang <[email protected]>
>> Tested-by: Swathi Kovvuri <[email protected]>
>> ---
>>
>> 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
