Re: crypto: stm32 - Revert broken pm_runtime_resume_and_get changes
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: 2021-12-01 14:47:19
Also in:
linux-pm
On Wed, Dec 1, 2021 at 7:30 AM Herbert Xu [off-list ref] wrote:
On Sat, Nov 27, 2021 at 02:39:52PM +0100, Heiner Kallweit wrote:quoted
When discussing whether pm_runtime_resume_and_get() should be annotated as __must_check, I became aware of RPM usage in crypto/stm32. Following two patches replace usage of pm_runtime_get_sync() with pm_runtime_resume_and_get() w/o checking the return code. 747bf30fd944 ("crypto: stm32/cryp - Fix PM reference leak on stm32-cryp.c") 1cb3ad701970 ("crypto: stm32/hash - Fix PM reference leak on stm32-hash.c") This results in RPM usage like the following in stm32_hash_export(): pm_runtime_resume_and_get(hdev->dev); ... pm_runtime_mark_last_busy(hdev->dev); pm_runtime_put_autosuspend(hdev->dev); This is broken. After pm_runtime_resume_and_get() the usage count may be incremented or not. If not, then the call to pm_runtime_put_autosuspend() results in exactly the imbalance that the patch claims to fix. Therefore I think both patches should be reverted, or the return code of pm_runtime_resume_and_get() has to be checked and properly handled in the driver logic.I agree. But we can't revert them completely because it does fix some genuine issues with the ones where we do check the error code. What about this patch? ---8<--- We should not call pm_runtime_resume_and_get where the reference count is expected to be incremented unconditionally. This patch reverts these calls to the original unconditional get_sync call.
Acked-by: Rafael J. Wysocki <redacted> The rule of thumb is to use pm_runtime_resume_and_get() if you want to check the return value (and act on it) and pm_runtime_get_sync() otherwise.
quoted hunk ↗ jump to hunk
Reported-by: Heiner Kallweit <hkallweit1@gmail.com> Fixes: 747bf30fd944 ("crypto: stm32/cryp - Fix PM reference leak...") Fixes: 1cb3ad701970 ("crypto: stm32/hash - Fix PM reference leak...") Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>diff --git a/drivers/crypto/stm32/stm32-cryp.c b/drivers/crypto/stm32/stm32-cryp.c index 7389a0536ff0..05087831e199 100644 --- a/drivers/crypto/stm32/stm32-cryp.c +++ b/drivers/crypto/stm32/stm32-cryp.c@@ -542,7 +542,7 @@ static int stm32_cryp_hw_init(struct stm32_cryp *cryp) int ret; u32 cfg, hw_mode; - pm_runtime_resume_and_get(cryp->dev); + pm_runtime_get_sync(cryp->dev); /* Disable interrupt */ stm32_cryp_write(cryp, CRYP_IMSCR, 0);diff --git a/drivers/crypto/stm32/stm32-hash.c b/drivers/crypto/stm32/stm32-hash.c index 389de9e3302d..d33006d43f76 100644 --- a/drivers/crypto/stm32/stm32-hash.c +++ b/drivers/crypto/stm32/stm32-hash.c@@ -813,7 +813,7 @@ static void stm32_hash_finish_req(struct ahash_request *req, int err) static int stm32_hash_hw_init(struct stm32_hash_dev *hdev, struct stm32_hash_request_ctx *rctx) { - pm_runtime_resume_and_get(hdev->dev); + pm_runtime_get_sync(hdev->dev); if (!(HASH_FLAGS_INIT & hdev->flags)) { stm32_hash_write(hdev, HASH_CR, HASH_CR_INIT);@@ -962,7 +962,7 @@ static int stm32_hash_export(struct ahash_request *req, void *out) u32 *preg; unsigned int i; - pm_runtime_resume_and_get(hdev->dev); + pm_runtime_get_sync(hdev->dev); while ((stm32_hash_read(hdev, HASH_SR) & HASH_SR_BUSY)) cpu_relax();@@ -1000,7 +1000,7 @@ static int stm32_hash_import(struct ahash_request *req, const void *in) preg = rctx->hw_context; - pm_runtime_resume_and_get(hdev->dev); + pm_runtime_get_sync(hdev->dev); stm32_hash_write(hdev, HASH_IMR, *preg++); stm32_hash_write(hdev, HASH_STR, *preg++); --Email: Herbert Xu [off-list ref] Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt