Messages in this thread | | | Date | Mon, 23 Nov 2020 14:15:45 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown |
| |
On 2020-11-23 14:03, Jerome Brunet wrote: > On Fri 20 Nov 2020 at 10:42, Marc Zyngier <maz@kernel.org> wrote: > >> The HDMI driver request clocks early, but never disable them, leaving >> the clocks on even when the driver is removed. >> >> Fix it by slightly refactoring the clock code, and register a devm >> action that will eventually disable/unprepare the enabled clocks. >> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> drivers/gpu/drm/meson/meson_dw_hdmi.c | 43 >> ++++++++++++++++++--------- >> 1 file changed, 29 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c >> b/drivers/gpu/drm/meson/meson_dw_hdmi.c >> index 7f8eea494147..29623b309cb1 100644 >> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c >> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c >> @@ -145,8 +145,6 @@ struct meson_dw_hdmi { >> struct reset_control *hdmitx_apb; >> struct reset_control *hdmitx_ctrl; >> struct reset_control *hdmitx_phy; >> - struct clk *hdmi_pclk; >> - struct clk *venci_clk; >> struct regulator *hdmi_supply; >> u32 irq_stat; >> struct dw_hdmi *hdmi; >> @@ -946,6 +944,29 @@ static void meson_disable_regulator(void *data) >> regulator_disable(data); >> } >> >> +static void meson_disable_clk(void *data) >> +{ >> + clk_disable_unprepare(data); >> +} >> + >> +static int meson_enable_clk(struct device *dev, char *name) >> +{ >> + struct clk *clk; >> + int ret; >> + >> + clk = devm_clk_get(dev, name); >> + if (IS_ERR(clk)) { >> + dev_err(dev, "Unable to get %s pclk\n", name); >> + return PTR_ERR(clk); >> + } >> + >> + ret = clk_prepare_enable(clk); >> + if (!ret) >> + ret = devm_add_action_or_reset(dev, meson_disable_clk, >> clk); > > Thanks for fixing this Marc. > > FYI, while it is fine to declare a function to disable the clocks, a > quick > cast may avoid it > > devm_add_action_or_reset(dev, (void(*)(void *))clk_disable_unprepare, > clk);
While this works for now, a change to the clk_disable_unprepare() prototype (such as adding a second argument) would now go completely unnoticed (after all, you've cast the function, it *must* be correct, right?), and someone would spend a few hours trying to track down memory corruption or some other interesting results.
Yes, casting C functions can be hilarious.
I can see a few uses of this hack in the tree, and I have my pop-corn ready.
Thanks,
M. -- Jazz is not dead. It just smells funny...
| |