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: <Z-EL1Y9Fh_0Z8KBt@kekkonen.localdomain>
Date: Mon, 24 Mar 2025 07:37:57 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Mehdi Djait <mehdi.djait@...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 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.

> 
> > > +	if (!is_acpi_node(dev_fwnode(dev)))
> > > +		return ERR_PTR(-ENOENT);
> > > +
> > > +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	if (!id) {
> > > +		id = devm_kasprintf(dev, GFP_KERNEL, "clk-%s", dev_name(dev));
> > 
> > As far as I understand, the name doesn't need to stay valid after
> > devm_clk_hw_register_fixed_rate() returns. You can call kasprintf here,
> > and call kfree after devm_clk_hw_register_fixed_rate(). You could use
> > __free to manage the memory life time:
> > 
> > 	const char *clk_id __free(kfree) = NULL;
> > 
> > 	if (!id) {
> > 		clk_id = kasprintf(GFP_KERNEL, "clk-%s", dev_name(dev));
> > 		if (!clk_id)
> > 			return ERR_PTR(-ENOMEM);
> > 		id = clk_id;
> > 	}
> > 
> 
> Ack.
> 

-- 
Kind regards,

Sakari Ailus