Re: [PATCH 09/46] dmaengine: cppi41: use dmaenginem_async_device_register to simplify the code

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

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