| Message ID | ad6c3c705fbef7ba4a1b762b39b0b8e04c10b632.1549364323.git.nicolas.ferre@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] dmaengine: at_xdmac: remove BUG_ON macro in tasklet | expand |
On Tue, Feb 05, 2019 at 12:03:42PM +0100, Nicolas Ferre wrote: > Complement the identification of errors with stoping the channel and > dumping the descriptor that led to the error case. > > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > drivers/dma/at_xdmac.c | 43 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c > index 37a269420435..ec7a29d8e448 100644 > --- a/drivers/dma/at_xdmac.c > +++ b/drivers/dma/at_xdmac.c > @@ -1575,6 +1575,41 @@ static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan) > dmaengine_desc_get_callback_invoke(txd, NULL); > } > > +static void at_xdmac_handle_error(struct at_xdmac_chan *atchan) > +{ > + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); > + struct at_xdmac_desc *bad_desc; > + > + /* > + * The descriptor currently at the head of the active list is > + * broked. Since we don't have any way to report errors, we'll > + * just have to scream loudly and try to carry on. > + */ > + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) > + dev_err(chan2dev(&atchan->chan), "read bus error!!!"); > + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) > + dev_err(chan2dev(&atchan->chan), "write bus error!!!"); > + if (atchan->irq_status & AT_XDMAC_CIS_ROIS) > + dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); > + > + spin_lock_bh(&atchan->lock); > + /* Channel must be disabled first as it's not done automatically */ > + at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask); > + while (at_xdmac_read(atxdmac, AT_XDMAC_GS) & atchan->mask) > + cpu_relax(); > + bad_desc = list_first_entry(&atchan->xfers_list, > + struct at_xdmac_desc, > + xfer_node); > + spin_unlock_bh(&atchan->lock); > + /* Print bad descriptor's details if needed */ > + dev_dbg(chan2dev(&atchan->chan), > + "%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n", > + __func__, &bad_desc->lld.mbr_sa, &bad_desc->lld.mbr_da, > + bad_desc->lld.mbr_ubc); > + > + /* Then continue with usual descriptor management */ > +} > + > static void at_xdmac_tasklet(unsigned long data) > { > struct at_xdmac_chan *atchan = (struct at_xdmac_chan *)data; > @@ -1594,12 +1629,8 @@ static void at_xdmac_tasklet(unsigned long data) > || (atchan->irq_status & error_mask)) { > struct dma_async_tx_descriptor *txd; > > - if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) > - dev_err(chan2dev(&atchan->chan), "read bus error!!!"); > - if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) > - dev_err(chan2dev(&atchan->chan), "write bus error!!!"); > - if (atchan->irq_status & AT_XDMAC_CIS_ROIS) > - dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); > + if (atchan->irq_status & error_mask) > + at_xdmac_handle_error(atchan); > > spin_lock(&atchan->lock); > desc = list_first_entry(&atchan->xfers_list, > -- > 2.17.1 >
On 05-02-19, 12:03, Nicolas Ferre wrote: > Complement the identification of errors with stoping the channel and > dumping the descriptor that led to the error case. > > Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> > --- > drivers/dma/at_xdmac.c | 43 ++++++++++++++++++++++++++++++++++++------ > 1 file changed, 37 insertions(+), 6 deletions(-) > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c > index 37a269420435..ec7a29d8e448 100644 > --- a/drivers/dma/at_xdmac.c > +++ b/drivers/dma/at_xdmac.c > @@ -1575,6 +1575,41 @@ static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan) > dmaengine_desc_get_callback_invoke(txd, NULL); > } > > +static void at_xdmac_handle_error(struct at_xdmac_chan *atchan) > +{ > + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); > + struct at_xdmac_desc *bad_desc; > + > + /* > + * The descriptor currently at the head of the active list is > + * broked. Since we don't have any way to report errors, we'll You meant borked or broken... > + * just have to scream loudly and try to carry on. should we carry on or abort..? > + */ > + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) > + dev_err(chan2dev(&atchan->chan), "read bus error!!!"); > + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) > + dev_err(chan2dev(&atchan->chan), "write bus error!!!"); > + if (atchan->irq_status & AT_XDMAC_CIS_ROIS) > + dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); > + > + spin_lock_bh(&atchan->lock); > + /* Channel must be disabled first as it's not done automatically */ > + at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask); > + while (at_xdmac_read(atxdmac, AT_XDMAC_GS) & atchan->mask) > + cpu_relax(); > + bad_desc = list_first_entry(&atchan->xfers_list, > + struct at_xdmac_desc, > + xfer_node); > + spin_unlock_bh(&atchan->lock); > + /* Print bad descriptor's details if needed */ Well this is not great to look and read at, please do consider adding empty line before comments or logical blocks.. > + dev_dbg(chan2dev(&atchan->chan), > + "%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n", > + __func__, &bad_desc->lld.mbr_sa, &bad_desc->lld.mbr_da, > + bad_desc->lld.mbr_ubc); not dev_err? > + > + /* Then continue with usual descriptor management */ > +} > + > static void at_xdmac_tasklet(unsigned long data) > { > struct at_xdmac_chan *atchan = (struct at_xdmac_chan *)data; > @@ -1594,12 +1629,8 @@ static void at_xdmac_tasklet(unsigned long data) > || (atchan->irq_status & error_mask)) { > struct dma_async_tx_descriptor *txd; > > - if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) > - dev_err(chan2dev(&atchan->chan), "read bus error!!!"); > - if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) > - dev_err(chan2dev(&atchan->chan), "write bus error!!!"); > - if (atchan->irq_status & AT_XDMAC_CIS_ROIS) > - dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); > + if (atchan->irq_status & error_mask) > + at_xdmac_handle_error(atchan); > > spin_lock(&atchan->lock); > desc = list_first_entry(&atchan->xfers_list, > -- > 2.17.1
Vinod, Thanks for your review, I'm preparing v2. On 11/02/2019 at 12:58, Vinod Koul wrote: > On 05-02-19, 12:03, Nicolas Ferre wrote: >> Complement the identification of errors with stoping the channel and >> dumping the descriptor that led to the error case. >> >> Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> >> --- >> drivers/dma/at_xdmac.c | 43 ++++++++++++++++++++++++++++++++++++------ >> 1 file changed, 37 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c >> index 37a269420435..ec7a29d8e448 100644 >> --- a/drivers/dma/at_xdmac.c >> +++ b/drivers/dma/at_xdmac.c >> @@ -1575,6 +1575,41 @@ static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan) >> dmaengine_desc_get_callback_invoke(txd, NULL); >> } >> >> +static void at_xdmac_handle_error(struct at_xdmac_chan *atchan) >> +{ >> + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); >> + struct at_xdmac_desc *bad_desc; >> + >> + /* >> + * The descriptor currently at the head of the active list is >> + * broked. Since we don't have any way to report errors, we'll > > You meant borked or broken... Broken > >> + * just have to scream loudly and try to carry on. > > should we carry on or abort..? Changed in: * just have to scream loudly and try to continue with other * descriptors queued (if any). >> + */ >> + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) >> + dev_err(chan2dev(&atchan->chan), "read bus error!!!"); >> + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) >> + dev_err(chan2dev(&atchan->chan), "write bus error!!!"); >> + if (atchan->irq_status & AT_XDMAC_CIS_ROIS) >> + dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); >> + >> + spin_lock_bh(&atchan->lock); >> + /* Channel must be disabled first as it's not done automatically */ >> + at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask); >> + while (at_xdmac_read(atxdmac, AT_XDMAC_GS) & atchan->mask) >> + cpu_relax(); >> + bad_desc = list_first_entry(&atchan->xfers_list, >> + struct at_xdmac_desc, >> + xfer_node); >> + spin_unlock_bh(&atchan->lock); >> + /* Print bad descriptor's details if needed */ > > Well this is not great to look and read at, please do consider adding > empty line before comments or logical blocks.. True, indeed. >> + dev_dbg(chan2dev(&atchan->chan), >> + "%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n", >> + __func__, &bad_desc->lld.mbr_sa, &bad_desc->lld.mbr_da, >> + bad_desc->lld.mbr_ubc); > > not dev_err? Well, we have the dev_err at the beginning of the function, I think it's enough: this is really debugging information that needs to be activated to track the DMA configuration bug: it's not meant for production. >> + >> + /* Then continue with usual descriptor management */ >> +} >> + >> static void at_xdmac_tasklet(unsigned long data) >> { >> struct at_xdmac_chan *atchan = (struct at_xdmac_chan *)data; >> @@ -1594,12 +1629,8 @@ static void at_xdmac_tasklet(unsigned long data) >> || (atchan->irq_status & error_mask)) { >> struct dma_async_tx_descriptor *txd; >> >> - if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) >> - dev_err(chan2dev(&atchan->chan), "read bus error!!!"); >> - if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) >> - dev_err(chan2dev(&atchan->chan), "write bus error!!!"); >> - if (atchan->irq_status & AT_XDMAC_CIS_ROIS) >> - dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); >> + if (atchan->irq_status & error_mask) >> + at_xdmac_handle_error(atchan); >> >> spin_lock(&atchan->lock); >> desc = list_first_entry(&atchan->xfers_list, >> -- >> 2.17.1 >
diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c index 37a269420435..ec7a29d8e448 100644 --- a/drivers/dma/at_xdmac.c +++ b/drivers/dma/at_xdmac.c @@ -1575,6 +1575,41 @@ static void at_xdmac_handle_cyclic(struct at_xdmac_chan *atchan) dmaengine_desc_get_callback_invoke(txd, NULL); } +static void at_xdmac_handle_error(struct at_xdmac_chan *atchan) +{ + struct at_xdmac *atxdmac = to_at_xdmac(atchan->chan.device); + struct at_xdmac_desc *bad_desc; + + /* + * The descriptor currently at the head of the active list is + * broked. Since we don't have any way to report errors, we'll + * just have to scream loudly and try to carry on. + */ + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) + dev_err(chan2dev(&atchan->chan), "read bus error!!!"); + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) + dev_err(chan2dev(&atchan->chan), "write bus error!!!"); + if (atchan->irq_status & AT_XDMAC_CIS_ROIS) + dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); + + spin_lock_bh(&atchan->lock); + /* Channel must be disabled first as it's not done automatically */ + at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask); + while (at_xdmac_read(atxdmac, AT_XDMAC_GS) & atchan->mask) + cpu_relax(); + bad_desc = list_first_entry(&atchan->xfers_list, + struct at_xdmac_desc, + xfer_node); + spin_unlock_bh(&atchan->lock); + /* Print bad descriptor's details if needed */ + dev_dbg(chan2dev(&atchan->chan), + "%s: lld: mbr_sa=%pad, mbr_da=%pad, mbr_ubc=0x%08x\n", + __func__, &bad_desc->lld.mbr_sa, &bad_desc->lld.mbr_da, + bad_desc->lld.mbr_ubc); + + /* Then continue with usual descriptor management */ +} + static void at_xdmac_tasklet(unsigned long data) { struct at_xdmac_chan *atchan = (struct at_xdmac_chan *)data; @@ -1594,12 +1629,8 @@ static void at_xdmac_tasklet(unsigned long data) || (atchan->irq_status & error_mask)) { struct dma_async_tx_descriptor *txd; - if (atchan->irq_status & AT_XDMAC_CIS_RBEIS) - dev_err(chan2dev(&atchan->chan), "read bus error!!!"); - if (atchan->irq_status & AT_XDMAC_CIS_WBEIS) - dev_err(chan2dev(&atchan->chan), "write bus error!!!"); - if (atchan->irq_status & AT_XDMAC_CIS_ROIS) - dev_err(chan2dev(&atchan->chan), "request overflow error!!!"); + if (atchan->irq_status & error_mask) + at_xdmac_handle_error(atchan); spin_lock(&atchan->lock); desc = list_first_entry(&atchan->xfers_list,
Complement the identification of errors with stoping the channel and dumping the descriptor that led to the error case. Signed-off-by: Nicolas Ferre <nicolas.ferre@microchip.com> --- drivers/dma/at_xdmac.c | 43 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 37 insertions(+), 6 deletions(-)