linux-kernel - Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Sep 2022 10:36:46 +0000
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Jacopo Mondi <jacopo@...ndi.org>
Cc:     Marco Felsch <m.felsch@...gutronix.de>,
        Stephen Boyd <sboyd@...nel.org>, linux-clk@...r.kernel.org,
        mchehab@...nel.org, laurent.pinchart+renesas@...asonboard.com,
        akinobu.mita@...il.com, jacopo+renesas@...ndi.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] media: mt9m111: add V4L2_CID_LINK_FREQ support

Hi Jacopo,

On Tue, Sep 20, 2022 at 11:19:33AM +0200, Jacopo Mondi wrote:
> Hello
> 
> On Tue, Sep 20, 2022 at 10:56:17AM +0200, Marco Felsch wrote:
> > Hi Sakari,
> >
> > On 22-09-19, Sakari Ailus wrote:
> >
> > ...
> >
> > > > > > +	ret = clk_prepare_enable(mt9m111->clk);
> > > > > > +	if (ret < 0)
> > > > > > +		return ret;
> > > > > > +
> > > > > > +	extclk_rate = clk_get_rate(mt9m111->clk);
> > > > > > +	clk_disable_unprepare(mt9m111->clk);
> > > > >
> > > > > I don't think you'll need to enable a clock to just get its frequency.
> > > >
> > > > The official API states that you need to turn on the clk before
> > > > requesting it and it makes sense. Also there is a new helper
> > > > devm_clk_get_enabled() which addresses simple clk usage since most of
> > > > drivers don't enable it before requesting the rate.
> 
> Had the same question on v1 and Marco pointed me to the clk_get_rate()
> documentation
> https://elixir.bootlin.com/linux/v6.0-rc1/source/include/linux/clk.h#L682
> 
> which indeed specifies
> "This is only valid once the clock source has been enabled."
> 
> However none (or very few) of the linux-media i2c drivers actually do
> that.

I'm not aware of any. That's indeed what the documentation says. Also
clk_enable() documentation says that "If the clock can not be
enabled/disabled, this should return success". So I wonder how much can
you trust it. ;-)

> 
> I have added in cc the clk framework maintainer to see if he can help
> shed some light on this

Thanks.

But yes, to make this work in general case, one would need a way to ensure
the frequency is the one assigned in DT and that it won't change.

Getting the frequency (in an unreliable way?) isn't perfect but better than
nothing. So far I haven't heard of issues in practice though.

-- 
Regards,

Sakari Ailus