Hello,
this patch series converts the stm32-hash driver to use .remove_new. The
first patch is a fix that could be backported to stable, but it's not
very urgent. It's only problematic if such a device is unbound (which
happens rarely) and then clk_prepare_enable() fails. Up to you if you
want to tag it as stable material.
The overall goal is to reduce the number of drivers using the irritating
.remove() callback by one. See the commit log of the third patch for the
reasoning.
Best regards
Uwe
Uwe Kleine-König (3):
crypto: stm32/hash - Properly handle pm_runtime_get failing
crypto: stm32/hash - Drop if block with always false condition
crypto: stm32/hash - Convert to platform remove callback returning
void
drivers/crypto/stm32/stm32-hash.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
base-commit: 3de0152bf26ff0c5083ef831ba7676fc4c92e63a
--
2.39.2
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 ignored (apart
from emitting a warning) 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. Eventually after all drivers
are converted, .remove_new() is renamed to .remove().
Trivially convert this driver from always returning zero in the remove
callback to the void returning variant.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/crypto/stm32/stm32-hash.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index b10243035584..68c52eeaa6b1 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -2112,7 +2112,7 @@ static int stm32_hash_probe(struct platform_device *pdev)
return ret;
}
-static int stm32_hash_remove(struct platform_device *pdev)
+static void stm32_hash_remove(struct platform_device *pdev)
{
struct stm32_hash_dev *hdev = platform_get_drvdata(pdev);
int ret;
@@ -2135,8 +2135,6 @@ static int stm32_hash_remove(struct platform_device *pdev)
if (ret >= 0)
clk_disable_unprepare(hdev->clk);
-
- return 0;
}
#ifdef CONFIG_PM
@@ -2173,7 +2171,7 @@ static const struct dev_pm_ops stm32_hash_pm_ops = {
static struct platform_driver stm32_hash_driver = {
.probe = stm32_hash_probe,
- .remove = stm32_hash_remove,
+ .remove_new = stm32_hash_remove,
.driver = {
.name = "stm32-hash",
.pm = &stm32_hash_pm_ops,
--
2.39.2
stm32_hash_remove() is only called after stm32_hash_probe() succeeded. In
this case platform_set_drvdata() was called with a non-NULL data patameter.
The check for hdev being non-NULL can be dropped because hdev is never NULL
(or something bad like memory corruption happened and then the check
doesn't help any more either).
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/crypto/stm32/stm32-hash.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 75d281edae2a..b10243035584 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -2114,13 +2114,9 @@ static int stm32_hash_probe(struct platform_device *pdev)
static int stm32_hash_remove(struct platform_device *pdev)
{
- struct stm32_hash_dev *hdev;
+ struct stm32_hash_dev *hdev = platform_get_drvdata(pdev);
int ret;
- hdev = platform_get_drvdata(pdev);
- if (!hdev)
- return -ENODEV;
-
ret = pm_runtime_get_sync(hdev->dev);
stm32_hash_unregister_algs(hdev);
--
2.39.2
If pm_runtime_get() (disguised as pm_runtime_resume_and_get()) fails, this
means the clk wasn't prepared and enabled. Returning early in this case
however is wrong as then the following resource frees are skipped and this
is never catched up. So do all the cleanups but clk_disable_unprepare().
Also don't emit a warning, as stm32_hash_runtime_resume() already emitted
one.
Note that the return value of stm32_hash_remove() is mostly ignored by
the device core. The only effect of returning zero instead of an error
value is to suppress another warning in platform_remove(). So return 0
even if pm_runtime_resume_and_get() failed.
Fixes: 8b4d566de6a5 ("crypto: stm32/hash - Add power management support")
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/crypto/stm32/stm32-hash.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
index 88a186c3dd78..75d281edae2a 100644
--- a/drivers/crypto/stm32/stm32-hash.c
+++ b/drivers/crypto/stm32/stm32-hash.c
@@ -2121,9 +2121,7 @@ static int stm32_hash_remove(struct platform_device *pdev)
if (!hdev)
return -ENODEV;
- ret = pm_runtime_resume_and_get(hdev->dev);
- if (ret < 0)
- return ret;
+ ret = pm_runtime_get_sync(hdev->dev);
stm32_hash_unregister_algs(hdev);
@@ -2139,7 +2137,8 @@ static int stm32_hash_remove(struct platform_device *pdev)
pm_runtime_disable(hdev->dev);
pm_runtime_put_noidle(hdev->dev);
- clk_disable_unprepare(hdev->clk);
+ if (ret >= 0)
+ clk_disable_unprepare(hdev->clk);
return 0;
}
--
2.39.2
Hello,
Thanks for the modification.
This should be applied for fixes/stable.
Please add Cc: [email protected] in your commit message.
Best regards,
Thomas
On 7/31/23 18:54, Uwe Kleine-König wrote:
> If pm_runtime_get() (disguised as pm_runtime_resume_and_get()) fails, this
> means the clk wasn't prepared and enabled. Returning early in this case
> however is wrong as then the following resource frees are skipped and this
> is never catched up. So do all the cleanups but clk_disable_unprepare().
>
> Also don't emit a warning, as stm32_hash_runtime_resume() already emitted
> one.
>
> Note that the return value of stm32_hash_remove() is mostly ignored by
> the device core. The only effect of returning zero instead of an error
> value is to suppress another warning in platform_remove(). So return 0
> even if pm_runtime_resume_and_get() failed.
>
> Fixes: 8b4d566de6a5 ("crypto: stm32/hash - Add power management support")
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/crypto/stm32/stm32-hash.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c
> index 88a186c3dd78..75d281edae2a 100644
> --- a/drivers/crypto/stm32/stm32-hash.c
> +++ b/drivers/crypto/stm32/stm32-hash.c
> @@ -2121,9 +2121,7 @@ static int stm32_hash_remove(struct platform_device *pdev)
> if (!hdev)
> return -ENODEV;
>
> - ret = pm_runtime_resume_and_get(hdev->dev);
> - if (ret < 0)
> - return ret;
> + ret = pm_runtime_get_sync(hdev->dev);
>
> stm32_hash_unregister_algs(hdev);
>
> @@ -2139,7 +2137,8 @@ static int stm32_hash_remove(struct platform_device *pdev)
> pm_runtime_disable(hdev->dev);
> pm_runtime_put_noidle(hdev->dev);
>
> - clk_disable_unprepare(hdev->clk);
> + if (ret >= 0)
> + clk_disable_unprepare(hdev->clk);
>
> return 0;
> }
Hello,
On Wed, Aug 09, 2023 at 09:44:30AM +0200, Thomas BOURGOIN wrote:
> Thanks for the modification.
> This should be applied for fixes/stable.
> Please add Cc: [email protected] in your commit message.
I usually let maintainers decide if they want this Cc line and in
practise the Fixes: line seems to be enough for the stable team to pick
up a commit for backporting.
If your mail means I should resend the patch just to add the Cc: line,
please tell me again. Should I resent patches 2 and 3 then, too?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello,
On 8/9/23 12:58, Uwe Kleine-König wrote:
> I usually let maintainers decide if they want this Cc line and in
> practise the Fixes: line seems to be enough for the stable team to pick
> up a commit for backporting.
Ok, I thought this was required to apply the patch correctly on previous
stable releases. (Someone asked me to do it on one of my previous patch)
> If your mail means I should resend the patch just to add the Cc: line,
> please tell me again. Should I resent patches 2 and 3 then, too?
No, patch 3 will break the driver on previous version because remove_new
does not exist.
I don't think it's mandatory for patch 2, it's an optimisation it does
not fix something broken. The driver works as intended with the
condition so no need to remove it.
Thomas
On Mon, Jul 31, 2023 at 06:54:53PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
>
> this patch series converts the stm32-hash driver to use .remove_new. The
> first patch is a fix that could be backported to stable, but it's not
> very urgent. It's only problematic if such a device is unbound (which
> happens rarely) and then clk_prepare_enable() fails. Up to you if you
> want to tag it as stable material.
>
> The overall goal is to reduce the number of drivers using the irritating
> .remove() callback by one. See the commit log of the third patch for the
> reasoning.
>
> Best regards
> Uwe
>
> Uwe Kleine-K?nig (3):
> crypto: stm32/hash - Properly handle pm_runtime_get failing
> crypto: stm32/hash - Drop if block with always false condition
> crypto: stm32/hash - Convert to platform remove callback returning
> void
>
> drivers/crypto/stm32/stm32-hash.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> base-commit: 3de0152bf26ff0c5083ef831ba7676fc4c92e63a
> --
> 2.39.2
All applied. Thanks.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt