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 <[email protected]> >> --- >> 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 > -- Nicolas Ferre
