linux-kernel - Re: [RFC PATCH v3] media: v4l2-common: Add a helper for obtaining the clock producer

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] [day] [month] [year] [list]

Message-ID: <2f73luvvsoxqikskvkxw4yadlhq4vl7ywh5givld7pivj2lyal@ncsyli2uoz74>
Date: Wed, 16 Apr 2025 18:31:30 +0200
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	tomi.valkeinen@...asonboard.com, jacopo.mondi@...asonboard.com, hverkuil@...all.nl, 
	kieran.bingham@...asonboard.com, naush@...pberrypi.com, mchehab@...nel.org, hdegoede@...hat.com, 
	dave.stevenson@...pberrypi.com, linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v3] media: v4l2-common: Add a helper for obtaining
 the clock producer

Hi Sakari, Laurent,

On Mon, Mar 24, 2025 at 07:37:57AM +0000, Sakari Ailus wrote:
> Hi Laurent, Mehdi,
> 
> On Fri, Mar 21, 2025 at 01:30:43PM +0100, Mehdi Djait wrote:
> > Hi Laurent,
> > 
> > thank you for the review!
> > 
> > On Fri, Mar 21, 2025 at 12:11:24PM +0200, Laurent Pinchart wrote:
> > > Hi Mehdi,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Fri, Mar 21, 2025 at 10:38:14AM +0100, Mehdi Djait wrote:
> > 
> > SNIP
> > 
> > > > +
> > > > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > > +{
> > > > +	struct clk_hw *clk_hw;
> > > > +	struct clk *clk;
> > > > +	u32 rate;
> > > > +	int ret;
> > > > +
> > > > +	clk = devm_clk_get_optional(dev, id);
> > > > +	if (clk)
> > > > +		return clk;
> > > > +
> > > > +#ifdef CONFIG_COMMON_CLK
> > > 
> > > This patch will cause warnings when CONFIG_COMMON_CLK is disabled. Could
> > > you use
> > > 
> > > 	if (IS_REACHABLE(CONFIG_COMMON_CLK)) {
> > > 		...
> > > 	}
> > > 
> > > instead ? It will also ensure that all code gets compile-tested, even
> > > when CONFIG_COMMON_CLK is disabled ?
> > > 
> > > If you want to minimize implementation, you could write
> > > 
> > > 	if (!IS_REACHABLE(CONFIG_COMMON_CLK))
> > > 		return ERR_PTR(-ENOENT);
> > > 
> > > and keep the code below as-is.
> > > 
> > 
> > this is indeed way better! I will change this in the v3
> 
> This would work at the moment if devm_clk_hw_register_fixed_rate() was
> always available but it evaluates to __clk_hw_register_fixed_rate() that
> depends on COMMON_CLK.
> 
> I think t he best option would be to add a stub for it for !COMMON_CLK
> case. It may take some time to get that merged and into the media tree. C99
> also allows declaring variables midst of a basic block so declaring rate
> and clkhw later should address the warning.

I don't fully understand the concern with the availability of the
function. It is used by drivers in multiple subsystems and any change to
it should take care of the API users.

Don't you think that adding a stub for !COMMON_CLK would make the series
take a much longer time to get merged and complicate the situation ?

--
Kind Regards
Mehdi Djait