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

Message-ID: <hgczrxw25a7jn6ubuwijga7yn7epek4yhtya2gnd77awsgxdgs@lv2oxey567hu>
Date: Thu, 12 Jun 2025 14:15:10 +0200
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.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

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.

--
Kind Regards
Mehdi Djait