linux-kernel - Re: [PATCH 10/11] dmaengine: imx-sdma: drop remove callback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aLhbjvVytJnOgkvL@lizhi-Precision-Tower-5810>
Date: Wed, 3 Sep 2025 11:15:26 -0400
From: Frank Li <Frank.li@....com>
To: Marco Felsch <m.felsch@...gutronix.de>
Cc: Vinod Koul <vkoul@...nel.org>, Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>,
	Jiada Wang <jiada_wang@...tor.com>, dmaengine@...r.kernel.org,
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/11] dmaengine: imx-sdma: drop remove callback

On Wed, Sep 03, 2025 at 03:06:18PM +0200, Marco Felsch wrote:
> The whole driver was converted to the devm APIs except for this last
> for-loop. This loop is buggy due to three reasons:
>  1) It removes the channels without removing the users first. This can
>     lead to very bad situations.
>  2) The loop starts at 0 and which is channel0 which is a special
>     control channel not registered via vchan_init(). Therefore the
>     remove() always Oops because of NULL pointer exception.
>  3) sdma_free_chan_resources() disable the clks unconditional without
>     checking if the clks are enabled. This is done for all
>     MAX_DMA_CHANNELS which hang the system if there is at least one unused
>     channel.
>
> Since the dmaengine core supports devlinks we already addressed the
> first issue.
>
> The devlink support also addresses the third issue because during the
> consumer teardown phase each requested channel is dropped accordingly so
> the dmaengine driver doesn't need to this.
>
> The second issue is fixed by not doing anything on channel0.
>
> To sum-up, all issues are fixed by dropping the .remove() callback and
> let the frameworks do their job.

I not realize devlink have this beanfit also. devlink may need more review,
which change some default behavior. I suggest put dmaengin dmalink at this
patch to new serial.

It is easily omited by other dmaengine owner if mixed it to cleanup serial.

Reviewed-by: Frank Li <Frank.Li@....com>
>
> Signed-off-by: Marco Felsch <m.felsch@...gutronix.de>
> ---
>  drivers/dma/imx-sdma.c | 14 --------------
>  1 file changed, 14 deletions(-)
>
> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> index 6c6d38b202dd2deffc36b1bd27bc7c60de3d7403..c31785977351163d6fddf4d8b2f90dfebb508400 100644
> --- a/drivers/dma/imx-sdma.c
> +++ b/drivers/dma/imx-sdma.c
> @@ -2405,25 +2405,11 @@ static int sdma_probe(struct platform_device *pdev)
>  	return 0;
>  }
>
> -static void sdma_remove(struct platform_device *pdev)
> -{
> -	struct sdma_engine *sdma = platform_get_drvdata(pdev);
> -	int i;
> -
> -	/* Kill the tasklet */
> -	for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> -		struct sdma_channel *sdmac = &sdma->channel[i];
> -
> -		sdma_free_chan_resources(&sdmac->vc.chan);
> -	}
> -}
> -
>  static struct platform_driver sdma_driver = {
>  	.driver		= {
>  		.name	= "imx-sdma",
>  		.of_match_table = sdma_dt_ids,
>  	},
> -	.remove		= sdma_remove,
>  	.probe		= sdma_probe,
>  };
>
>
> --
> 2.47.2
>