Re: [PATCH 1/2] drm/meson: dw-hdmi: Disable clocks on driver teardown

[Marc Zyngier]

On 2020-11-23 14:03, Jerome Brunet wrote:
> On Fri 20 Nov 2020 at 10:42, Marc Zyngier <maz@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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...