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@nxp.com>
>
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.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
>