| Message ID | 20160906153857.5503-5-romain.perier@free-electrons.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Regards,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Mhhh, I think that I understood what you meant. The node /dev/hwrng is managed by hw_random core, a read might happen on this node while platform->remove is called... However, if hwrng_unregister is the first function called from platform->remove, the driver is unbinded from the hw_random core atomically (unregister and read cannot happen in the same time because there is a mutex), so the problem does not happen. I propose to remove this patch from the series. Thanks! Romain
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
On Thu, Sep 08, 2016 at 05:47:13PM +0200, Romain Perier wrote: > > 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 ? Right. remove can happen at any time and the driver needs to cope with it by returning an error from data_read if the hardware disappears in the middle of an operation. Cheers,
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);
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(-)