* Tony Lindgren <t...@atomide.com> [170116 15:36]:
> * Tony Lindgren <t...@atomide.com> [170113 14:00]:
> > * Grygorii Strashko <grygorii.stras...@ti.com> [170113 13:37]:
> > > > Simplified diff with fix on top of your patch:
> > > >
> > > > diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> > > > index ce37a1a..9e9403a 100644
> > > > --- a/drivers/dma/cppi41.c
> > > > +++ b/drivers/dma/cppi41.c
> > > > @@ -319,12 +319,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 != -EINPROGRESS) && (error < 0))
> > > > - dev_err(cdd->ddev.dev, "%s pm runtime
> > > > get: %i\n",
> > > > - __func__, error);
> > > >
> > > > /* This warning should never trigger */
> > > > WARN_ON(cdd->is_suspended);
> > > > @@ -500,7 +494,6 @@ static void cppi41_dma_issue_pending(struct
> > > > dma_chan *chan)
> > > > spin_unlock_irqrestore(&cdd->lock, flags);
> > > >
> > > > pm_runtime_mark_last_busy(cdd->ddev.dev);
> > > > - pm_runtime_put_autosuspend(cdd->ddev.dev);
> > > > }
> > > >
> > > > static u32 get_host_pd0(u32 length)
> > > >
> > >
> > > Ok. this is incorrect in case of USB hub and will just hide the problem
> > > by incrementing PM runtime usage counter every time USB host is connected
> > > :(
> >
> > Yeah if anything changes in those two nested loops, the pm_runtime counts
> > get unbalanced.
> >
> > > Once USB hub is connected the PM runtime usage counter will became 1 and
> > > stay
> > > 1 after disconnection. Which means that some descriptor was pushed in Q,
> > > but IRQ
> > > was not triggered.
> > >
> > > Don't know how to proceed as I'm not so familiar with musb :(
> >
> > I'm now playing with saving the queue manager value and kicking the
> > PM runtime if we have transfers in progress. Looks like the dma
> > registers are zero while there are transfers in progress, or at least
> > I have not yet found any hardware registers that would tell that.
>
> Looks like drivers/usb/musb/musb_cppi41.c is not properly terminating
> dma transactions.. The following additional patch makes things behave
> without warnings for me.
>
> I'll fold the drivers/dma/cppi41.c changes to $subject patch and repost,
> then will post a separate musb fix for drivers/usb/musb/musb_cppi41.c
> that avoids the warning after some more investigating.
>
> Luckily the warning is harmless in this case as musb and cppi41 are
> in the same device so the common parent is powered and cppi41 is
> getting clocked.
Sorry actually it's after these fixes when we still need to implement
something for cppi41 PM runtime usecount that makes sense as the
calls will finally be paired. For testing, reverting 098de42ad670
("dmaengine: cppi41: Fix unpaired pm runtime when only a USB hub
is connected") can be done. But I don't like that as it's still
unpaired pm_runtime_calls potentially if something goes wrong.
Anyways, for the -rc series oops, we can just leave out the WARN_ON
parts for now until drivers/usb/musb/musb_cppi41.c is fixed too.
Regards,
Tony
> 8< ---------------------------
> diff --git a/drivers/dma/cppi41.c b/drivers/dma/cppi41.c
> --- a/drivers/dma/cppi41.c
> +++ b/drivers/dma/cppi41.c
> @@ -705,19 +705,31 @@ static int cppi41_stop_chan(struct dma_chan *chan)
> u32 desc_phys;
> int ret;
>
> + ret = pm_runtime_get(cdd->ddev.dev);
> + if ((ret != -EINPROGRESS) && (ret < 0))
> + dev_err(cdd->ddev.dev, "%s pm runtime get: %i\n",
> + __func__, ret);
> + WARN_ON(cdd->is_suspended);
> +
> desc_phys = lower_32_bits(c->desc_phys);
> desc_num = (desc_phys - cdd->descs_phys) / sizeof(struct cppi41_desc);
> - if (!cdd->chan_busy[desc_num])
> - return 0;
> + if (!cdd->chan_busy[desc_num]) {
> + ret = 0;
> + goto out;
> + }
>
> ret = cppi41_tear_down_chan(c);
> if (ret)
> - return ret;
> + goto out;
>
> WARN_ON(!cdd->chan_busy[desc_num]);
> cdd->chan_busy[desc_num] = NULL;
>
> - return 0;
> +out:
> + pm_runtime_mark_last_busy(cdd->ddev.dev);
> + pm_runtime_put_autosuspend(cdd->ddev.dev);
> +
> + return ret;
> }
>
> static void cleanup_chans(struct cppi41_dd *cdd)
> diff --git a/drivers/usb/musb/musb_cppi41.c b/drivers/usb/musb/musb_cppi41.c
> --- a/drivers/usb/musb/musb_cppi41.c
> +++ b/drivers/usb/musb/musb_cppi41.c
> @@ -445,9 +445,16 @@ static struct dma_channel
> *cppi41_dma_channel_allocate(struct dma_controller *c,
> static void cppi41_dma_channel_release(struct dma_channel *channel)
> {
> struct cppi41_dma_channel *cppi41_channel = channel->private_data;
> + struct musb_hw_ep *hw_ep = cppi41_channel->hw_ep;
> + int error;
>
> trace_musb_cppi41_free(cppi41_channel);
> if (cppi41_channel->is_allocated) {
> + error = dmaengine_terminate_all(cppi41_channel->dc);
> + if (error)
> + dev_err(hw_ep->musb->controller,
> + "dma terminate failed: %i\n",
> + error);
> cppi41_channel->is_allocated = 0;
> channel->status = MUSB_DMA_STATUS_FREE;
> channel->actual_len = 0;
> --
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
