| Message ID | 20210405085033.686284735@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi! > However, these functions alone don't provide any guarantees at the > system level. Drivers need to ensure that the only a single consumer has > access to the reset at the same time. In order for the SOR to be able to > exclusively access its reset, it must therefore ensure that the SOR > power domain is not powered off by holding on to a runtime PM reference > to that power domain across the reset assert/deassert operation. Yeah, but it should not leak the PM reference in the error handling. Signed-off-by: Pavel Machek (CIP) <pavel@denx.de> Best regards, Pavel diff --git a/drivers/gpu/drm/tegra/sor.c b/drivers/gpu/drm/tegra/sor.c index 7b88261f57bb..b3b727b2a3c5 100644 --- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3125,21 +3125,21 @@ static int tegra_sor_init(struct host1x_client *client) if (err < 0) { dev_err(sor->dev, "failed to acquire SOR reset: %d\n", err); - return err; + goto maybe_put; } err = reset_control_assert(sor->rst); if (err < 0) { dev_err(sor->dev, "failed to assert SOR reset: %d\n", err); - return err; + goto maybe_put; } } err = clk_prepare_enable(sor->clk); if (err < 0) { dev_err(sor->dev, "failed to enable clock: %d\n", err); - return err; + goto maybe_put; } usleep_range(1000, 3000); @@ -3150,7 +3150,7 @@ static int tegra_sor_init(struct host1x_client *client) dev_err(sor->dev, "failed to deassert SOR reset: %d\n", err); clk_disable_unprepare(sor->clk); - return err; + goto maybe_put; } reset_control_release(sor->rst); @@ -3171,6 +3171,11 @@ static int tegra_sor_init(struct host1x_client *client) } return 0; + + maybe_put: + if (sor->rst) + pm_runtime_put(sor->dev); + return err; } static int tegra_sor_exit(struct host1x_client *client)
On Mon, Apr 05, 2021 at 05:42:21PM +0200, Pavel Machek wrote: > Hi! > > > However, these functions alone don't provide any guarantees at the > > system level. Drivers need to ensure that the only a single consumer has > > access to the reset at the same time. In order for the SOR to be able to > > exclusively access its reset, it must therefore ensure that the SOR > > power domain is not powered off by holding on to a runtime PM reference > > to that power domain across the reset assert/deassert operation. > > Yeah, but it should not leak the PM reference in the error handling. True. However the only reason why the code could realistically fail between pm_runtime_resume_and_get() and pm_runtime_put() is if we did not take the runtime PM reference (which would cause the call to reset_control_acquire() to fail). So the chances of this actually leaking are practically zero, so I didn't want to bloat this bugfix with what's essentially dead code. I can queue up your fix below for v5.14, though, since it's obviously more correct from a theoretical point of view. Thierry
Hi! > > > However, these functions alone don't provide any guarantees at the > > > system level. Drivers need to ensure that the only a single consumer has > > > access to the reset at the same time. In order for the SOR to be able to > > > exclusively access its reset, it must therefore ensure that the SOR > > > power domain is not powered off by holding on to a runtime PM reference > > > to that power domain across the reset assert/deassert operation. > > > > Yeah, but it should not leak the PM reference in the error handling. > > True. However the only reason why the code could realistically fail > between pm_runtime_resume_and_get() and pm_runtime_put() is if we did > not take the runtime PM reference (which would cause the call to > reset_control_acquire() to fail). > > So the chances of this actually leaking are practically zero, so I > didn't want to bloat this bugfix with what's essentially dead code. I > can queue up your fix below for v5.14, though, since it's obviously > more correct from a theoretical point of view. Yes, that sounds like good solution, thanks! Best regards, Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
--- a/drivers/gpu/drm/tegra/sor.c +++ b/drivers/gpu/drm/tegra/sor.c @@ -3115,6 +3115,12 @@ static int tegra_sor_init(struct host1x_ * kernel is possible. */ if (sor->rst) { + err = pm_runtime_resume_and_get(sor->dev); + if (err < 0) { + dev_err(sor->dev, "failed to get runtime PM: %d\n", err); + return err; + } + err = reset_control_acquire(sor->rst); if (err < 0) { dev_err(sor->dev, "failed to acquire SOR reset: %d\n", @@ -3148,6 +3154,7 @@ static int tegra_sor_init(struct host1x_ } reset_control_release(sor->rst); + pm_runtime_put(sor->dev); } err = clk_prepare_enable(sor->clk_safe);