| Message ID | 20170116233329.GF7403@atomide.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
* Tony Lindgren <tony@atomide.com> [170116 15:36]: > * Tony Lindgren <tony@atomide.com> [170113 14:00]: > > * Grygorii Strashko <grygorii.strashko@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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tony, On Mon, Jan 16, 2017 at 03:54:29PM -0800, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [170116 15:36]: > > * Tony Lindgren <tony@atomide.com> [170113 14:00]: > > > * Grygorii Strashko <grygorii.strashko@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. Giving that cppi is a submodule inside the usb subsysytem and it does't have separate power rail or clock, what is the benefit to adding runtime PM in the cppi driver? Thanks, -Bin. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Bin Liu <b-liu@ti.com> [170117 05:00]: > On Mon, Jan 16, 2017 at 03:54:29PM -0800, Tony Lindgren wrote: > > 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. > > Giving that cppi is a submodule inside the usb subsysytem and it does't > have separate power rail or clock, what is the benefit to adding runtime > PM in the cppi driver? Good question. We need at least minimal support to enable things for probe and then idle cppi41 properly if only cppi41.ko is loaded with no USB modules. But yeah now that musb does runtime PM based on the cable detection, we pretty much guarantee that cppi41 is always enabled when USB is in use. And if there are no other devices using cppi41 dma on davinci, we can simplify the PM runtime a bit for cppi41. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 17, 2017 at 08:11:39AM -0800, Tony Lindgren wrote: > * Bin Liu <b-liu@ti.com> [170117 05:00]: > > On Mon, Jan 16, 2017 at 03:54:29PM -0800, Tony Lindgren wrote: > > > 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. > > > > Giving that cppi is a submodule inside the usb subsysytem and it does't > > have separate power rail or clock, what is the benefit to adding runtime > > PM in the cppi driver? > > Good question. We need at least minimal support to enable things for > probe and then idle cppi41 properly if only cppi41.ko is loaded with no > USB modules. > > But yeah now that musb does runtime PM based on the cable detection, we > pretty much guarantee that cppi41 is always enabled when USB is in use. > > And if there are no other devices using cppi41 dma on davinci, we can > simplify the PM runtime a bit for cppi41. This might be a good idea. I didn't have much time to play with this cppi41 runtime PM, but it seems I am having more issues than you and others seeing. Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Bin Liu <b-liu@ti.com> [170117 08:22]: > On Tue, Jan 17, 2017 at 08:11:39AM -0800, Tony Lindgren wrote: > > * Bin Liu <b-liu@ti.com> [170117 05:00]: > > > On Mon, Jan 16, 2017 at 03:54:29PM -0800, Tony Lindgren wrote: > > > > 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. > > > > > > Giving that cppi is a submodule inside the usb subsysytem and it does't > > > have separate power rail or clock, what is the benefit to adding runtime > > > PM in the cppi driver? > > > > Good question. We need at least minimal support to enable things for > > probe and then idle cppi41 properly if only cppi41.ko is loaded with no > > USB modules. > > > > But yeah now that musb does runtime PM based on the cable detection, we > > pretty much guarantee that cppi41 is always enabled when USB is in use. > > > > And if there are no other devices using cppi41 dma on davinci, we can > > simplify the PM runtime a bit for cppi41. > > This might be a good idea. I didn't have much time to play with this > cppi41 runtime PM, but it seems I am having more issues than you and > others seeing. What kind of additional issues are you seeing not described in the $subject patch? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 17, 2017 at 08:31:03AM -0800, Tony Lindgren wrote: > * Bin Liu <b-liu@ti.com> [170117 08:22]: > > On Tue, Jan 17, 2017 at 08:11:39AM -0800, Tony Lindgren wrote: > > > * Bin Liu <b-liu@ti.com> [170117 05:00]: > > > > On Mon, Jan 16, 2017 at 03:54:29PM -0800, Tony Lindgren wrote: > > > > > 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. > > > > > > > > Giving that cppi is a submodule inside the usb subsysytem and it does't > > > > have separate power rail or clock, what is the benefit to adding runtime > > > > PM in the cppi driver? > > > > > > Good question. We need at least minimal support to enable things for > > > probe and then idle cppi41 properly if only cppi41.ko is loaded with no > > > USB modules. > > > > > > But yeah now that musb does runtime PM based on the cable detection, we > > > pretty much guarantee that cppi41 is always enabled when USB is in use. > > > > > > And if there are no other devices using cppi41 dma on davinci, we can > > > simplify the PM runtime a bit for cppi41. > > > > This might be a good idea. I didn't have much time to play with this > > cppi41 runtime PM, but it seems I am having more issues than you and > > others seeing. > > What kind of additional issues are you seeing not described in the $subject > patch? I didn't take a note and don't remember if those are in the $subject patch. But - enumeration begining with a reset that the device doesn't accept address X, error code -71; or - console fooding with cppi error code -115 after thumb drive enumeration. Again, I only tried for a few minutes and didn't take a note, since I don't have time to look at this ATM. Regards, -Bin. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;