Hi > -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > Sent: Wednesday, July 26, 2023 1:30 PM > To: Horia Geanta <horia.geanta@xxxxxxx>; Pankaj Gupta > <pankaj.gupta@xxxxxxx>; Gaurav Jain <gaurav.jain@xxxxxxx>; Herbert Xu > <herbert@xxxxxxxxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx> > Cc: linux-crypto@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx > Subject: [EXT] [PATCH] crypto: caam/jr - Convert to platform remove callback > returning void > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report this > email' button > > > The .remove() callback for a platform driver returns an int which makes many > driver authors wrongly assume it's possible to do error handling by returning an > error code. However the value returned is (mostly) ignored and this typically > results in resource leaks. To improve here there is a quest to make the remove > callback return void. In the first step of this quest all drivers are converted > to .remove_new() which already returns void. > > The driver adapted here suffers from this wrong assumption. Returning -EBUSY > if there are still users results in resource leaks and probably a crash. Also further > down passing the error code of caam_jr_shutdown() to the caller only results in > another error message and has no further consequences compared to returning > zero. > > Still convert the driver to return no value in the remove callback. This also allows > to drop caam_jr_platform_shutdown() as the only function called by it now has > the same prototype. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > --- > Hello, > > note that the problems described above and in the extended comment isn't > introduced by this patch. It's as old as > 313ea293e9c4d1eabcaddd2c0800f083b03c2a2e at least. > > Also orthogonal to this patch I wonder about the use of a shutdown callback. > What makes this driver special to require extra handling at shutdown time? This is introduced for kexec boot. See this https://patchwork.kernel.org/project/linux-crypto/patch/20230316060734.818549-1-meenakshi.aggarwal@xxxxxxx/ Regards Gaurav Jain > > Best regards > Uwe > > drivers/crypto/caam/jr.c | 22 +++++++++------------- > 1 file changed, 9 insertions(+), 13 deletions(-) > > diff --git a/drivers/crypto/caam/jr.c b/drivers/crypto/caam/jr.c index > 96dea5304d22..66b1eb3eb4a4 100644 > --- a/drivers/crypto/caam/jr.c > +++ b/drivers/crypto/caam/jr.c > @@ -162,7 +162,7 @@ static int caam_jr_shutdown(struct device *dev) > return ret; > } > > -static int caam_jr_remove(struct platform_device *pdev) > +static void caam_jr_remove(struct platform_device *pdev) > { > int ret; > struct device *jrdev; > @@ -175,11 +175,14 @@ static int caam_jr_remove(struct platform_device > *pdev) > caam_rng_exit(jrdev->parent); > > /* > - * Return EBUSY if job ring already allocated. > + * If a job ring is still allocated there is trouble ahead. Once > + * caam_jr_remove() returned, jrpriv will be freed and the registers > + * will get unmapped. So any user of such a job ring will probably > + * crash. > */ > if (atomic_read(&jrpriv->tfm_count)) { > - dev_err(jrdev, "Device is busy\n"); > - return -EBUSY; > + dev_warn(jrdev, "Device is busy, fasten your seat belts, a crash is > ahead.\n"); > + return; > } > > /* Unregister JR-based RNG & crypto algorithms */ @@ -194,13 +197,6 > @@ static int caam_jr_remove(struct platform_device *pdev) > ret = caam_jr_shutdown(jrdev); > if (ret) > dev_err(jrdev, "Failed to shut down job ring\n"); > - > - return ret; > -} > - > -static void caam_jr_platform_shutdown(struct platform_device *pdev) -{ > - caam_jr_remove(pdev); > } > > /* Main per-ring interrupt handler */ > @@ -657,8 +653,8 @@ static struct platform_driver caam_jr_driver = { > .of_match_table = caam_jr_match, > }, > .probe = caam_jr_probe, > - .remove = caam_jr_remove, > - .shutdown = caam_jr_platform_shutdown, > + .remove_new = caam_jr_remove, > + .shutdown = caam_jr_remove, > }; > > static int __init jr_driver_init(void) > > base-commit: dd105461ad15ea930d88aec1e4fcfc1f3186da43 > -- > 2.39.2
