Re: [PATCH 5.4 160/344] dmaengine: imx-sdma: Fix memory leak

[Greg Kroah-Hartman]

On Mon, Feb 24, 2020 at 01:24:04PM +0000, Andreas Tobler wrote:
> Hi all,
> 
> On 21.02.20 08:39, Greg Kroah-Hartman wrote:
> > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > 
> > [ Upstream commit 02939cd167095f16328a1bd5cab5a90b550606df ]
> > 
> > The current descriptor is not on any list of the virtual DMA channel.
> > Once sdma_terminate_all() is called when a descriptor is currently
> > in flight then this one is forgotten to be freed. We have to call
> > vchan_terminate_vdesc() on this descriptor to re-add it to the lists.
> > Now that we also free the currently running descriptor we can (and
> > actually have to) remove the current descriptor from its list also
> > for the cyclic case.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > Reviewed-by: Robin Gong <yibin.gong@xxxxxxx>
> > Tested-by: Robin Gong <yibin.gong@xxxxxxx>
> > Link: https://lore.kernel.org/r/20191216105328.15198-10-s.hauer@xxxxxxxxxxxxxx
> > Signed-off-by: Vinod Koul <vkoul@xxxxxxxxxx>
> > Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> > ---
> >   drivers/dma/imx-sdma.c | 19 +++++++++++--------
> >   1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index c27e206a764c3..66f1b2ac5cde4 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -760,12 +760,8 @@ static void sdma_start_desc(struct sdma_channel *sdmac)
> >   		return;
> >   	}
> >   	sdmac->desc = desc = to_sdma_desc(&vd->tx);
> > -	/*
> > -	 * Do not delete the node in desc_issued list in cyclic mode, otherwise
> > -	 * the desc allocated will never be freed in vchan_dma_desc_free_list
> > -	 */
> > -	if (!(sdmac->flags & IMX_DMA_SG_LOOP))
> > -		list_del(&vd->node);
> > +
> > +	list_del(&vd->node);
> >   
> >   	sdma->channel_control[channel].base_bd_ptr = desc->bd_phys;
> >   	sdma->channel_control[channel].current_bd_ptr = desc->bd_phys;
> > @@ -1071,7 +1067,6 @@ static void sdma_channel_terminate_work(struct work_struct *work)
> >   
> >   	spin_lock_irqsave(&sdmac->vc.lock, flags);
> >   	vchan_get_all_descriptors(&sdmac->vc, &head);
> > -	sdmac->desc = NULL;
> >   	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> >   	vchan_dma_desc_free_list(&sdmac->vc, &head);
> >   	sdmac->context_loaded = false;
> > @@ -1080,11 +1075,19 @@ static void sdma_channel_terminate_work(struct work_struct *work)
> >   static int sdma_disable_channel_async(struct dma_chan *chan)
> >   {
> >   	struct sdma_channel *sdmac = to_sdma_chan(chan);
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&sdmac->vc.lock, flags);
> >   
> >   	sdma_disable_channel(chan);
> >   
> > -	if (sdmac->desc)
> > +	if (sdmac->desc) {
> > +		vchan_terminate_vdesc(&sdmac->desc->vd);
> > +		sdmac->desc = NULL;
> >   		schedule_work(&sdmac->terminate_worker);
> > +	}
> > +
> > +	spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> >   
> >   	return 0;
> >   }
> > 
> 
> This patch breaks our imx6 board with the attached trace.  Reverting the 
> patch makes it boot again.
> I tried also 5.6-rc3 and it booted too. A closer look into imx-sdma.c 
> from 5.6-rc3 showed me some details which might have to be backported as 
> well to make this patch work.
> I tried a1ff6a07f5a3951fcac84f064a76d1ad79c10e40 and was somehow 
> successful. I still have one trace but the board boots now.
> 
> Any insights from the experts?

I've now reverted it from all of the 3 stable trees it endeded up in.

thanks,

greg k-h