Re: [PATCH 4/9] hwrng: omap - Use the managed device resource API for registration

[Romain Perier <romain.perier@free-electrons.com>]

Le 07/09/2016 16:45, PrasannaKumar Muralidharan a écrit :
> On 7 September 2016 at 19:53, Romain Perier
> <romain.perier@free-electrons.com> wrote:
>> Hello,
>>
>>
>> Le 06/09/2016 18:31, PrasannaKumar Muralidharan a écrit :
>>>>
>>>> Use devm_hwrng_register instead of hwrng_register. It avoids the need
>>>> to handle unregistration explicitly from the remove function.
>>>>
>>>> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
>>>> ---
>>>>    drivers/char/hw_random/omap-rng.c | 4 +---
>>>>    1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/char/hw_random/omap-rng.c
>>>> b/drivers/char/hw_random/omap-rng.c
>>>> index d47b24d..171c3e8 100644
>>>> --- a/drivers/char/hw_random/omap-rng.c
>>>> +++ b/drivers/char/hw_random/omap-rng.c
>>>> @@ -381,7 +381,7 @@ static int omap_rng_probe(struct platform_device
>>>> *pdev)
>>>>           if (ret)
>>>>                   goto err_ioremap;
>>>>
>>>> -       ret = hwrng_register(&omap_rng_ops);
>>>> +       ret = devm_hwrng_register(dev, &omap_rng_ops);
>>>>           if (ret)
>>>>                   goto err_register;
>>>>
>>>> @@ -402,8 +402,6 @@ static int omap_rng_remove(struct platform_device
>>>> *pdev)
>>>>    {
>>>>           struct omap_rng_dev *priv = platform_get_drvdata(pdev);
>>>>
>>>> -       hwrng_unregister(&omap_rng_ops);
>>>> -
>>>>           priv->pdata->cleanup(priv);
>>>>
>>>>           pm_runtime_put_sync(&pdev->dev);
>>>> --
>>>
>>>
>>> If devm_hwrng_register is used hwrng_unregister will be called after
>>> pm_runtime_disable is called. If RNG device is in use calling
>>> omap_rng_remove may not work properly.
>>>
>>
>> The case where the remove function is called is if you unbind the driver by
>> hand or you call rmmod while the RNG device is used.
>> I don't think that the kernel will call platform->remove is the device is in
>> use (so /dev/hwrng). I mean the argument that the unregister function is
>> called after pm_runtime_disable is correct, but I don't think that the
>> remove function might be called while the device is in use. There is
>> necessarily a mutual exclusive case between "use the device" and "call the
>> remove function of the device". However, I am open to suggestions.
>
> The way you explained is good :D. Good point too. But the device is
> created by hw_random core (hwrng_modinit in core.c) so the device can
> be in use when omap-rng module is removed. Please feel free to correct
> me if I am wrong.
>
> Cheers,
> PrasannaKumar
>

Hi,

I was wondering something. hwrng_unregister does not check the kref 
reference counter of the object... so technically if the current 
rng_device is in use, with or without devm... calling platform->remove 
will break the driver anyway because hwrng_unregister will unbind the 
device from /dev/hwrng. What I mean is that I think that we had this 
issue even without devm_hwrng_register.

Herbert, could you confirm ?

Romain
-- 
Romain Perier, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com