Hi Vinod,
> From: Vinod Koul, Sent: Wednesday, June 17, 2020 1:56 AM
>
> On 21-05-20, 20:46, Yoshihiro Shimoda wrote:
> > This driver assumed that freed descriptors have "done_cookie".
> > But, after the commit 24461d9792c2 ("dmaengine: virt-dma: Fix
> > access after free in vchan_complete()"), since the desc is freed
> > after callback function was called, this driver could not
> > match any done_cookie when a client driver (renesas_usbhs driver)
> > calls dmaengine_tx_status() in the callback function.
>
> Hmmm, I am not sure about this, why should we try to match! cookie is
> monotonically increasing number so if you see that current cookie
> completed is > requested you should return DMA_COMPLETE
The reason is this hardware is possible to stop the transfer even if
all transfer length is not received. This is related to one of USB
specification which allows to stop when getting a short packet.
So, a client driver has to get residue even if DMA_COMPLETE.
> The below case of checking residue should not even get executed
I see...
So, I'm thinking the current implementation was a tricky because we didn't
have dma_async_tx_callback_result when I wrote this usb-dmac driver.
I'll try this to fix the issue.
Best regards,
Yoshihiro Shimoda
> > So, add to check both descriptor types (freed and got) to fix
> > the issue.
> >
> > Reported-by: Hien Dang <hien.dang.eb@xxxxxxxxxxx>
> > Fixes: 24461d9792c2 ("dmaengine: virt-dma: Fix access after free in vchan_complete()")
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > ---
> > drivers/dma/sh/usb-dmac.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/dma/sh/usb-dmac.c b/drivers/dma/sh/usb-dmac.c
> > index b218a01..c0adc1c8 100644
> > --- a/drivers/dma/sh/usb-dmac.c
> > +++ b/drivers/dma/sh/usb-dmac.c
> > @@ -488,16 +488,17 @@ static u32 usb_dmac_chan_get_residue_if_complete(struct usb_dmac_chan *chan,
> > dma_cookie_t cookie)
> > {
> > struct usb_dmac_desc *desc;
> > - u32 residue = 0;
> >
> > + list_for_each_entry_reverse(desc, &chan->desc_got, node) {
> > + if (desc->done_cookie == cookie)
> > + return desc->residue;
> > + }
> > list_for_each_entry_reverse(desc, &chan->desc_freed, node) {
> > - if (desc->done_cookie == cookie) {
> > - residue = desc->residue;
> > - break;
> > - }
> > + if (desc->done_cookie == cookie)
> > + return desc->residue;
> > }
> >
> > - return residue;
> > + return 0;
> > }
> >
> > static u32 usb_dmac_chan_get_residue(struct usb_dmac_chan *chan,
> > --
> > 2.7.4
>
> --
> ~Vinod
