| Message ID | 20200128110102.11522-4-martin@kaiser.cx (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | imx-rngc - several small fixes | expand |
Hi Martin, On Tue, 28 Jan 2020 at 16:31, Martin Kaiser <martin@kaiser.cx> wrote: > > Use devres to register the rngc with the hwrng core. Drop the explicit > deregistration. > > Signed-off-by: Martin Kaiser <martin@kaiser.cx> > --- > drivers/char/hw_random/imx-rngc.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c > index 903894518c8d..1381ddd5b891 100644 > --- a/drivers/char/hw_random/imx-rngc.c > +++ b/drivers/char/hw_random/imx-rngc.c > @@ -263,7 +263,7 @@ static int imx_rngc_probe(struct platform_device *pdev) > } > } > > - ret = hwrng_register(&rngc->rng); > + ret = devm_hwrng_register(&pdev->dev, &rngc->rng); > if (ret) { > dev_err(&pdev->dev, "FSL RNGC registering failed (%d)\n", ret); > goto err; > @@ -282,8 +282,6 @@ static int __exit imx_rngc_remove(struct platform_device *pdev) > { > struct imx_rngc *rngc = platform_get_drvdata(pdev); > > - hwrng_unregister(&rngc->rng); > - > clk_disable_unprepare(rngc->clk); > > return 0; > -- > 2.20.1 > After imx_rngc_remove function hwrng_unregister will get called. This leaves a window where the clock to rng hardware block is disabled but still user space can access it via /dev/hwrng. This does not look right, please revisit the patch. Regards, PrasannaKumar
Hi PrasannaKumar, Thus wrote PrasannaKumar Muralidharan (prasannatsmkumar@gmail.com): > After imx_rngc_remove function hwrng_unregister will get called. This > leaves a window where the clock to rng hardware block is disabled but > still user space can access it via /dev/hwrng. thanks for spotting this issue. I see that in __device_release_driver, the driver's remove function is called before the devres cleanup. > This does not look right, please revisit the patch. I checked again how other hwrng drivers use devres. Some don't have to disable a clock and need no remove function at all. Others enable the clock in the hwrng init routine and disable it in the cleanup routine. Both of these approaches don't work here. I should disable the clock eventually and I need it in the probe function to run the selftest before hwrng init is called. Therefore, I suggest to drop this patch, at least for the moment. Herbert, should I resend the series without this patch or is it ok for you to take the remaining patches as-is? BTW, 3e75241be808 ("hwrng: drivers - Use device-managed registration API") makes the same change that I proposed here for a couple of other hwrng drivers and seems to introduce the same race condition in som drivers e.g. drivers/char/hw_random/exynos-trng.c. Should we try to fix this? Thanks, Martin
diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c index 903894518c8d..1381ddd5b891 100644 --- a/drivers/char/hw_random/imx-rngc.c +++ b/drivers/char/hw_random/imx-rngc.c @@ -263,7 +263,7 @@ static int imx_rngc_probe(struct platform_device *pdev) } } - ret = hwrng_register(&rngc->rng); + ret = devm_hwrng_register(&pdev->dev, &rngc->rng); if (ret) { dev_err(&pdev->dev, "FSL RNGC registering failed (%d)\n", ret); goto err; @@ -282,8 +282,6 @@ static int __exit imx_rngc_remove(struct platform_device *pdev) { struct imx_rngc *rngc = platform_get_drvdata(pdev); - hwrng_unregister(&rngc->rng); - clk_disable_unprepare(rngc->clk); return 0;
Use devres to register the rngc with the hwrng core. Drop the explicit deregistration. Signed-off-by: Martin Kaiser <martin@kaiser.cx> --- drivers/char/hw_random/imx-rngc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)