Re: [PATCH] dmaengine: cppi41: Fix oops in cppi41_runtime_resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 01/12/2017 05:04 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@xxxxxx> [170112 14:53]:
>>
>>
>> On 01/12/2017 04:19 PM, Tony Lindgren wrote:
>>> * Grygorii Strashko <grygorii.strashko@xxxxxx> [170112 13:54]:
>>>> On 01/12/2017 03:30 PM, Tony Lindgren wrote:
>>>>
>>>> Sry, but even if it looks better it might still race with PM runtime :(
>>>>
>>>>> -	if (likely(pm_runtime_active(cdd->ddev.dev)))
>>>>> +	if (likely(atomic_read(&cdd->active)))
>>>>>  		push_desc_queue(c);
>>>>>  	else
>>>>
>>>>
>>>> - CPU is here (-EINPROGRESS and active == 0) and then preempted 
>>>> - PM runtime will finish cppi41_runtime_resume and clean-up pending descs
>>>> - CPU return here and adds desc to the pending queue
>>>> - oops
>>>>
>>>> Am I wrong?
>>>
>>> We had cppi41_dma_issue_pending() getting called from atomic contex and
>>> cppi41_runtime_resume() getting preempted where cppi41_dma_issue_pending()
>>> would add to the queue.
>>
>> Again, I can be mistaken but cppi41_configure_channel() seems not atomic.
>> cppi41_configure_channel()->dma_async_issue_pending()
>> + documentation says "This function can be called in an interrupt context"
>>
>> And definitely it will be preemptive on RT :(
> 
> Hmm OK. So are you thinking we should add a spinlock around the
> test in cppi41_dma_issue_pending() and when modifying cdd->active?

in general yes.

> 
> Something like:
> 
> spin_lock_irqsave(&cdd->lock, flags);
> if (likely(cdd->active))
> 	push_desc_queue(c);
> else
> 	list_add_tail(&c->node, &cdd->pending);
> spin_unlock_irqrestore(&cdd->lock, flags);
> 
> Or do you have something better in mind?

I've come up with smth like below. *Not tested*
But may be it can be done more simpler.

diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
index d5ba43a..41a8768 100644
--- a/drivers/dma/cppi41.c
+++ b/drivers/dma/cppi41.c
@@ -153,6 +153,7 @@ struct cppi41_dd {
 
        /* context for suspend/resume */
        unsigned int dma_tdfdq;
+       int is_suspended;
 };
 
 #define FIST_COMPLETION_QUEUE  93
@@ -317,12 +318,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
 
                while (val) {
                        u32 desc, len;
-                       int error;
-
-                       error = pm_runtime_get(cdd->ddev.dev);
-                       if (error < 0)
-                               dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
-                                       __func__, error);
 
                        q_num = __fls(val);
                        val &= ~(1 << q_num);
@@ -343,9 +338,6 @@ static irqreturn_t cppi41_irq(int irq, void *data)
                        c->residue = pd_trans_len(c->desc->pd6) - len;
                        dma_cookie_complete(&c->txd);
                        dmaengine_desc_get_callback_invoke(&c->txd, NULL);
-
-                       pm_runtime_mark_last_busy(cdd->ddev.dev);
-                       pm_runtime_put_autosuspend(cdd->ddev.dev);
                }
        }
        return IRQ_HANDLED;
@@ -457,38 +449,36 @@ static void push_desc_queue(struct cppi41_channel *c)
        cppi_writel(reg, cdd->qmgr_mem + QMGR_QUEUE_D(c->q_num));
 }
 
-static void pending_desc(struct cppi41_channel *c)
+static void cppi41_dma_issue_pending(struct dma_chan *chan)
 {
+       struct cppi41_channel *c = to_cpp41_chan(chan);
        struct cppi41_dd *cdd = c->cdd;
+       int error;
+       struct cppi41_channel *_c;
        unsigned long flags;
 
        spin_lock_irqsave(&cdd->lock, flags);
        list_add_tail(&c->node, &cdd->pending);
-       spin_unlock_irqrestore(&cdd->lock, flags);
-}
-
-static void cppi41_dma_issue_pending(struct dma_chan *chan)
-{
-       struct cppi41_channel *c = to_cpp41_chan(chan);
-       struct cppi41_dd *cdd = c->cdd;
-       int error;
 
        error = pm_runtime_get(cdd->ddev.dev);
-       if ((error != -EINPROGRESS) && error < 0) {
+       if (error < 0) {
                pm_runtime_put_noidle(cdd->ddev.dev);
                dev_err(cdd->ddev.dev, "Failed to pm_runtime_get: %i\n",
                        error);
-
+               spin_unlock_irqrestore(&cdd->lock, flags);
                return;
        }
 
-       if (likely(pm_runtime_active(cdd->ddev.dev)))
-               push_desc_queue(c);
-       else
-               pending_desc(c);
+       if (!cdd->is_suspended) {
+               list_for_each_entry_safe(c, _c, &cdd->pending, node) {
+                       push_desc_queue(c);
+                       list_del(&c->node);
+               };
+       }
 
        pm_runtime_mark_last_busy(cdd->ddev.dev);
        pm_runtime_put_autosuspend(cdd->ddev.dev);
+       spin_lock_irqsave(&cdd->lock, flags);
 }
 
 static u32 get_host_pd0(u32 length)
@@ -1150,10 +1140,20 @@ static int __maybe_unused cppi41_resume(struct device *dev)
 static int __maybe_unused cppi41_runtime_suspend(struct device *dev)
 {
        struct cppi41_dd *cdd = dev_get_drvdata(dev);
+       int ret;
+       unsigned long flags;
 
-       WARN_ON(!list_empty(&cdd->pending));
+       spin_lock_irqsave(&cdd->lock, flags);
+       if (!list_empty(&cdd->pending)) {
+               WARN_ON(!list_empty(&cdd->pending));
+               ret = -EBUSY;
+       } else {
+               /* ... suspend the device ... */
+               cdd->is_suspended = 1;
+       }
+       spin_unlock_irqrestore(&cdd->lock, flags);
 
-       return 0;
+       return ret;
 }
 
 static int __maybe_unused cppi41_runtime_resume(struct device *dev)
@@ -1163,6 +1163,8 @@ static int __maybe_unused cppi41_runtime_resume(struct device *dev)
        unsigned long flags;
 
        spin_lock_irqsave(&cdd->lock, flags);
+       cdd->is_suspended = 0;
+
        list_for_each_entry_safe(c, _c, &cdd->pending, node) {
                push_desc_queue(c);
                list_del(&c->node);


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



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux