linux-kernel - Re: [RFC PATCH v5] 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: <20250612213904.GC10542@pendragon.ideasonboard.com>
Date: Fri, 13 Jun 2025 00:39:04 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mehdi Djait <mehdi.djait@...ux.intel.com>
Cc: sakari.ailus@...ux.intel.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, arnd@...db.de,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v5] media: v4l2-common: Add a helper for obtaining
 the clock producer

On Thu, Jun 12, 2025 at 02:15:10PM +0200, Mehdi Djait wrote:
> Hi Laurent,
> 
> Thank you for the review!
> 
> A very small question below.
> 
> On Wed, May 21, 2025 at 01:09:44PM +0200, Laurent Pinchart wrote:
> > On Wed, May 21, 2025 at 12:52:08PM +0200, Mehdi Djait wrote:
> 
> > > > +struct clk *devm_v4l2_sensor_clk_get(struct device *dev, const char *id)
> > > > +{
> > > > +	const char *clk_id __free(kfree) = NULL;
> > > > +	struct clk_hw *clk_hw;
> > > > +	struct clk *clk;
> > > > +	u32 rate;
> > > > +	int ret;
> > > > +
> > > > +	clk = devm_clk_get_optional(dev, id);
> > > > +	ret = device_property_read_u32(dev, "clock-frequency", &rate);
> > > > +
> > > > +	if (clk) {
> > > > +		if (!ret) {
> > > > +			ret = clk_set_rate(clk, rate);
> > > > +			if (ret)
> > > > +				dev_warn(dev, "Failed to set clock rate: %u\n",
> > > > +					 rate);
> > 
> > I would return ERR_PTR(ret) here.
> > 
> > > > +		}
> > > > +
> > > > +		return clk;
> > > > +	}
> > > > +
> > > > +	if (ret)
> > > > +		return ERR_PTR(ret);
> > 
> > And here, return a fixed error code, maybe -ENOENT, as propagating the
> > device_property_read_u32() error could result in strange error code for
> > the user.
> 
> device_property_read_u32() returns the following:
> 
> Return: number of values if @val was %NULL,
>         %0 if the property was found (success),
>           %-EINVAL if given arguments are not valid,
>           %-ENODATA if the property does not have a value,
>           %-EPROTO if the property is not an array of numbers,
>           %-EOVERFLOW if the size of the property is not as expected.
>           %-ENXIO if no suitable firmware interface is present.
> 
> Don't you think it is better to keep the return value and not overshadow
> it ? The function is well documented and this may help understand where
> the problem comes from if getting the clk fails.

I don't mind too much either way. If we want to make debugging easier,
may an error message would be appropriate.

-- 
Regards,

Laurent Pinchart