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 <[email protected]>
---
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?
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
Hello,
On Wed, Jul 26, 2023 at 09:59:38AM +0200, Uwe Kleine-K?nig wrote:
> - dev_err(jrdev, "Device is busy\n");
> + dev_warn(jrdev, "Device is busy, fasten your seat belts, a crash is ahead.\n");
I intended to make this higher-priority than dev_err. So this should be
a dev_alert instead of dev_warn. I'll wait a bit before resending to
allow feedback. If you intend to apply it before, please fix-up
accordingly.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi
> -----Original Message-----
> From: Uwe Kleine-König <[email protected]>
> Sent: Wednesday, July 26, 2023 1:30 PM
> To: Horia Geanta <[email protected]>; Pankaj Gupta
> <[email protected]>; Gaurav Jain <[email protected]>; Herbert Xu
> <[email protected]>; David S. Miller <[email protected]>
> Cc: [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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?
>
> 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");
Changes are ok. Can you improve the error message or keep it same.
Regards
Gaurav
> + 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
Hi
> -----Original Message-----
> From: Uwe Kleine-König <[email protected]>
> Sent: Wednesday, July 26, 2023 1:30 PM
> To: Horia Geanta <[email protected]>; Pankaj Gupta
> <[email protected]>; Gaurav Jain <[email protected]>; Herbert Xu
> <[email protected]>; David S. Miller <[email protected]>
> Cc: [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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/[email protected]/
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
Hello Gaurav,
On Mon, Jul 31, 2023 at 09:56:22AM +0000, Gaurav Jain wrote:
> > -----Original Message-----
> > From: Uwe Kleine-K?nig <[email protected]>
> > Sent: Wednesday, July 26, 2023 1:30 PM
> > To: Horia Geanta <[email protected]>; Pankaj Gupta
> > <[email protected]>; Gaurav Jain <[email protected]>; Herbert Xu
> > <[email protected]>; David S. Miller <[email protected]>
> > Cc: [email protected]; [email protected]
> > 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 <[email protected]>
> > ---
> > 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?
> >
> > 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");
>
> Changes are ok. Can you improve the error message or keep it same.
What do you imagine here? "Device is busy" lacks urgency in my eyes and
is hard to rate by a log reader. Mentioning that something bad is about
to happen would be good. Do you want it expressed in a more serious way?
Something like:
Device is busy; consumers might start to crash
?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe
> -----Original Message-----
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Tuesday, August 1, 2023 1:44 PM
> To: Gaurav Jain <[email protected]>
> Cc: Horia Geanta <[email protected]>; Pankaj Gupta
> <[email protected]>; Herbert Xu <[email protected]>; David
> S. Miller <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [EXT] [PATCH] crypto: caam/jr - Convert to platform remove
> callback returning void
>
> Hello Gaurav,
>
> On Mon, Jul 31, 2023 at 09:56:22AM +0000, Gaurav Jain wrote:
> > > -----Original Message-----
> > > From: Uwe Kleine-K?nig <[email protected]>
> > > Sent: Wednesday, July 26, 2023 1:30 PM
> > > To: Horia Geanta <[email protected]>; Pankaj Gupta
> > > <[email protected]>; Gaurav Jain <[email protected]>; Herbert
> > > Xu <[email protected]>; David S. Miller
> > > <[email protected]>
> > > Cc: [email protected]; [email protected]
> > > 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 <[email protected]>
> > > ---
> > > 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?
> > >
> > > 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");
> >
> > Changes are ok. Can you improve the error message or keep it same.
>
> What do you imagine here? "Device is busy" lacks urgency in my eyes and is hard
> to rate by a log reader. Mentioning that something bad is about to happen
> would be good.
Agreed.
Do you want it expressed in a more serious way?
> Something like:
>
> Device is busy; consumers might start to crash
Yes, this is also fine. or
something like:
Device is busy; System might crash.
Regards
Gaurav Jain
>
> ?
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |