| Message ID | 20180803072016.21544-10-sjhuang@iluvatar.ai (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Use dmaenginem_async_device_register to simplify code | expand |
On 2018-08-03 10:19, Huang Shijie wrote: > Use dmaenginem_async_device_register to simplify the code: > remove dma_async_device_unregister > > Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> > --- > drivers/dma/ti/cppi41.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c > index 1497da367710..d2998a19ed2e 100644 > --- a/drivers/dma/ti/cppi41.c > +++ b/drivers/dma/ti/cppi41.c > @@ -1096,21 +1096,19 @@ static int cppi41_dma_probe(struct platform_device *pdev) > goto err_chans; > cdd->irq = irq; > > - ret = dma_async_device_register(&cdd->ddev); > + ret = dmaenginem_async_device_register(&cdd->ddev); > if (ret) > goto err_chans; > > ret = of_dma_controller_register(dev->of_node, > cppi41_dma_xlate, &cpp41_dma_info); > if (ret) > - goto err_of; > + goto err_chans; > > pm_runtime_mark_last_busy(dev); > pm_runtime_put_autosuspend(dev); > > return 0; > -err_of: > - dma_async_device_unregister(&cdd->ddev); > err_chans: > deinit_cppi41(dev, cdd); > err_init_cppi: > @@ -1132,7 +1130,6 @@ static int cppi41_dma_remove(struct platform_device *pdev) > dev_err(&pdev->dev, "%s could not pm_runtime_get: %i\n", > __func__, error); > of_dma_controller_free(pdev->dev.of_node); > - dma_async_device_unregister(&cdd->ddev); If I read the code right then this is not safe. We would have deinitalized cppi41 driver which is not functional, but we will still have the dma device registered and if a channel is requested we will have kernel crash. It falls in the same case as omap-dma, edma. > > devm_free_irq(&pdev->dev, cdd->irq, cdd); > deinit_cppi41(&pdev->dev, cdd); > - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 2018年08月03日 15:55, Peter Ujfalusi 写道: > > On 2018-08-03 10:19, Huang Shijie wrote: >> Use dmaenginem_async_device_register to simplify the code: >> remove dma_async_device_unregister >> >> Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> >> --- >> drivers/dma/ti/cppi41.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c >> index 1497da367710..d2998a19ed2e 100644 >> --- a/drivers/dma/ti/cppi41.c >> +++ b/drivers/dma/ti/cppi41.c >> @@ -1096,21 +1096,19 @@ static int cppi41_dma_probe(struct platform_device *pdev) >> goto err_chans; >> cdd->irq = irq; >> >> - ret = dma_async_device_register(&cdd->ddev); >> + ret = dmaenginem_async_device_register(&cdd->ddev); >> if (ret) >> goto err_chans; >> >> ret = of_dma_controller_register(dev->of_node, >> cppi41_dma_xlate, &cpp41_dma_info); >> if (ret) >> - goto err_of; >> + goto err_chans; >> >> pm_runtime_mark_last_busy(dev); >> pm_runtime_put_autosuspend(dev); >> >> return 0; >> -err_of: >> - dma_async_device_unregister(&cdd->ddev); >> err_chans: >> deinit_cppi41(dev, cdd); >> err_init_cppi: >> @@ -1132,7 +1130,6 @@ static int cppi41_dma_remove(struct platform_device *pdev) >> dev_err(&pdev->dev, "%s could not pm_runtime_get: %i\n", >> __func__, error); >> of_dma_controller_free(pdev->dev.of_node); >> - dma_async_device_unregister(&cdd->ddev); > If I read the code right then this is not safe. > We would have deinitalized cppi41 driver which is not functional, but we > will still have the dma device registered and if a channel is requested > we will have kernel crash. > > It falls in the same case as omap-dma, edma. Thanks for pointing this, we can drop the patches for omap-dma, edma. Huang Shijie -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 03, 2018 at 10:55:25AM +0300, Peter Ujfalusi wrote: > > > On 2018-08-03 10:19, Huang Shijie wrote: > > Use dmaenginem_async_device_register to simplify the code: > > remove dma_async_device_unregister > > > > Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> > > --- > > drivers/dma/ti/cppi41.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c > > index 1497da367710..d2998a19ed2e 100644 > > --- a/drivers/dma/ti/cppi41.c > > +++ b/drivers/dma/ti/cppi41.c > > @@ -1096,21 +1096,19 @@ static int cppi41_dma_probe(struct platform_device *pdev) > > goto err_chans; > > cdd->irq = irq; > > > > - ret = dma_async_device_register(&cdd->ddev); > > + ret = dmaenginem_async_device_register(&cdd->ddev); > > if (ret) > > goto err_chans; > > > > ret = of_dma_controller_register(dev->of_node, > > cppi41_dma_xlate, &cpp41_dma_info); > > if (ret) > > - goto err_of; > > + goto err_chans; > > > > pm_runtime_mark_last_busy(dev); > > pm_runtime_put_autosuspend(dev); > > > > return 0; > > -err_of: > > - dma_async_device_unregister(&cdd->ddev); > > err_chans: > > deinit_cppi41(dev, cdd); > > err_init_cppi: > > @@ -1132,7 +1130,6 @@ static int cppi41_dma_remove(struct platform_device *pdev) > > dev_err(&pdev->dev, "%s could not pm_runtime_get: %i\n", > > __func__, error); > > of_dma_controller_free(pdev->dev.of_node); > > - dma_async_device_unregister(&cdd->ddev); > > If I read the code right then this is not safe. I read the code again, and find it is okay. > We would have deinitalized cppi41 driver which is not functional, but we > will still have the dma device registered and if a channel is requested > we will have kernel crash. We cannot succeed to request a channel when the drv->remove() is called. Please see __device_release_driver: --------------------------------------------------------------------- if (dev->bus && dev->bus->remove) dev->bus->remove(dev); else if (drv->remove) drv->remove(dev); device_links_driver_cleanup(dev); dma_deconfigure(dev); devres_release_all(dev); ============> Devres release --------------------------------------------------------------------- For the DMA engine driver, there is only one case which will calls drv->remove(): Use the rmmod(or modprobe -r). We do not use the device_link_add API for DMA engines. And we not manually call the device_release_driver() for DMA engines. But when we use the rmmod, the module state will be MODULE_STATE_GOING. In the find_candidate(), dma_chan_get() will fail. And we cannot get a channel. Please correct me if I am wrong :) Thanks Huang Shijie -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 2018-08-06 06:28, Huang Shijie wrote: > On Fri, Aug 03, 2018 at 10:55:25AM +0300, Peter Ujfalusi wrote: >> >> >> On 2018-08-03 10:19, Huang Shijie wrote: >>> Use dmaenginem_async_device_register to simplify the code: >>> remove dma_async_device_unregister >>> >>> Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> >>> --- >>> drivers/dma/ti/cppi41.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c >>> index 1497da367710..d2998a19ed2e 100644 >>> --- a/drivers/dma/ti/cppi41.c >>> +++ b/drivers/dma/ti/cppi41.c >>> @@ -1096,21 +1096,19 @@ static int cppi41_dma_probe(struct platform_device *pdev) >>> goto err_chans; >>> cdd->irq = irq; >>> >>> - ret = dma_async_device_register(&cdd->ddev); >>> + ret = dmaenginem_async_device_register(&cdd->ddev); >>> if (ret) >>> goto err_chans; >>> >>> ret = of_dma_controller_register(dev->of_node, >>> cppi41_dma_xlate, &cpp41_dma_info); >>> if (ret) >>> - goto err_of; >>> + goto err_chans; >>> >>> pm_runtime_mark_last_busy(dev); >>> pm_runtime_put_autosuspend(dev); >>> >>> return 0; >>> -err_of: >>> - dma_async_device_unregister(&cdd->ddev); >>> err_chans: >>> deinit_cppi41(dev, cdd); >>> err_init_cppi: >>> @@ -1132,7 +1130,6 @@ static int cppi41_dma_remove(struct platform_device *pdev) >>> dev_err(&pdev->dev, "%s could not pm_runtime_get: %i\n", >>> __func__, error); >>> of_dma_controller_free(pdev->dev.of_node); >>> - dma_async_device_unregister(&cdd->ddev); >> >> If I read the code right then this is not safe. > I read the code again, and find it is okay. > >> We would have deinitalized cppi41 driver which is not functional, but we >> will still have the dma device registered and if a channel is requested >> we will have kernel crash. > We cannot succeed to request a channel when the drv->remove() is called. > > Please see __device_release_driver: > --------------------------------------------------------------------- > if (dev->bus && dev->bus->remove) > dev->bus->remove(dev); > else if (drv->remove) > drv->remove(dev); > > device_links_driver_cleanup(dev); > dma_deconfigure(dev); > > devres_release_all(dev); ============> Devres release > --------------------------------------------------------------------- > > For the DMA engine driver, there is only one case which will calls drv->remove(): > Use the rmmod(or modprobe -r). > > We do not use the device_link_add API for DMA engines. > And we not manually call the device_release_driver() for DMA engines. > > But when we use the rmmod, the module state will be MODULE_STATE_GOING. > In the find_candidate(), dma_chan_get() will fail. > And we cannot get a channel. > > Please correct me if I am wrong :) You are perfectly right. It might be only me, but I like to keep the resource teardown in a reverse order of their creation. If everything is devm then it is granted. In case of cppi4 it looks safe after reading in to the DMAengine core, module core and platform core code. But does the removed three lines worth over the clarity of how the module removal is proceeding? Alexandre and Tony put lots of effort to the cppi4 driver, I let them decide. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 07, 2018 at 10:01:47AM +0300, Peter Ujfalusi wrote: > Hi, > > On 2018-08-06 06:28, Huang Shijie wrote: > It might be only me, but I like to keep the resource teardown in a > reverse order of their creation. If everything is devm then it is granted. Yes. If everything is devm then it is granted.. > > In case of cppi4 it looks safe after reading in to the DMAengine core, > module core and platform core code. > > But does the removed three lines worth over the clarity of how the > module removal is proceeding? Please keep the driver as it is if you like the traditional way. :) The DMA driver's maintainer has the right to decide whether to use the dmaenginem_async_device_register or not. Thanks Huang Shijie -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Peter Ujfalusi <peter.ujfalusi@ti.com> [180807 07:05]: > But does the removed three lines worth over the clarity of how the > module removal is proceeding? > > Alexandre and Tony put lots of effort to the cppi4 driver, I let them > decide. Up to you guys as far as I'm concerned. Tony -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c index 1497da367710..d2998a19ed2e 100644 --- a/drivers/dma/ti/cppi41.c +++ b/drivers/dma/ti/cppi41.c @@ -1096,21 +1096,19 @@ static int cppi41_dma_probe(struct platform_device *pdev) goto err_chans; cdd->irq = irq; - ret = dma_async_device_register(&cdd->ddev); + ret = dmaenginem_async_device_register(&cdd->ddev); if (ret) goto err_chans; ret = of_dma_controller_register(dev->of_node, cppi41_dma_xlate, &cpp41_dma_info); if (ret) - goto err_of; + goto err_chans; pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); return 0; -err_of: - dma_async_device_unregister(&cdd->ddev); err_chans: deinit_cppi41(dev, cdd); err_init_cppi: @@ -1132,7 +1130,6 @@ static int cppi41_dma_remove(struct platform_device *pdev) dev_err(&pdev->dev, "%s could not pm_runtime_get: %i\n", __func__, error); of_dma_controller_free(pdev->dev.of_node); - dma_async_device_unregister(&cdd->ddev); devm_free_irq(&pdev->dev, cdd->irq, cdd); deinit_cppi41(&pdev->dev, cdd);
Use dmaenginem_async_device_register to simplify the code: remove dma_async_device_unregister Signed-off-by: Huang Shijie <sjhuang@iluvatar.ai> --- drivers/dma/ti/cppi41.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)