On 7/7/2020 1:50 AM, Peter Ujfalusi wrote:
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.
It is possible there's a race as well in addition to the issue that the patch
fixes. I'll take a look and see if there's an additional fix for the unbalanced
locking. Thanks for checking Peter.
Fixes: e81274cd6b52 ("dmaengine: add support to dynamic register/unregister of channels")
Reported-by: Swathi Kovvuri <swathi.kovvuri@xxxxxxxxx>
Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
Tested-by: Swathi Kovvuri <swathi.kovvuri@xxxxxxxxx>
---
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
