[PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Wolfram Sang]

clk_get functions return an ERR_PTR and not NULL in the error case. Make
that consistent for the dummy functions when HAVE_CLK is not enabled.
Otherwise unexpected codepaths might be trying to use a NULL pointer.

Signed-off-by: Wolfram Sang <wsa@the-dreams.de>
---
 include/linux/clk.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index c7f258a81761..37a286b69943 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -340,12 +340,12 @@ struct clk *clk_get_sys(const char *dev_id, const char *con_id);
 
 static inline struct clk *clk_get(struct device *dev, const char *id)
 {
-	return NULL;
+	return ERR_PTR(-ENOENT);
 }
 
 static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-	return NULL;
+	return ERR_PTR(-ENOENT);
 }
 
 static inline void clk_put(struct clk *clk) {}
-- 
2.1.4

Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Russell King - ARM Linux]

On Thu, Feb 05, 2015 at 08:09:22PM +0100, Wolfram Sang wrote:
> clk_get functions return an ERR_PTR and not NULL in the error case. Make
> that consistent for the dummy functions when HAVE_CLK is not enabled.
> Otherwise unexpected codepaths might be trying to use a NULL pointer.

NAK.

There are some drivers which rely on this behaviour (take a driver,
such as some PXA IP) which uses the clk API but is also reused on
x86 which doesn't.

Remember, for any clk API implementation, it is well defined that
the clk API should safely eat any struct clk that clk_get() may
return which isn't a pointer-error.

So, if an implementation returns the NULL pointer, then the clk API
functions must accept it without crashing the kernel.

Also, clock consumer drivers are _not_ allowed to dereference
struct clk.  Absolutely not permitted.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

On Thu, Feb 05, 2015 at 11:40:40PM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 05, 2015 at 08:09:22PM +0100, Wolfram Sang wrote:
> > clk_get functions return an ERR_PTR and not NULL in the error case. Make
> > that consistent for the dummy functions when HAVE_CLK is not enabled.
> > Otherwise unexpected codepaths might be trying to use a NULL pointer.
> 
> NAK.
> 
> There are some drivers which rely on this behaviour (take a driver,
> such as some PXA IP) which uses the clk API but is also reused on
> x86 which doesn't.

Okay, thanks for the explanation. Let me recap, there are three cases
that clk_get can return:

a) pointer to a clk
b) ERR_PTR meaning something went wrong when trying to get a clk
c) NULL meaning there is no clk to get

So, if this patch from i2c/for-next [1] wants to add optional clk
support, it should fallback to the old handling not by checking for
(!IS_ERR()) but for (!IS_ERR_OR_NULL()).

Correct?

[1] https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=e961a094afe04c6c8ca1adac50c8d16513f31b93


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Russell King - ARM Linux]

On Sat, Feb 07, 2015 at 05:42:09PM +0100, Wolfram Sang wrote:
> On Thu, Feb 05, 2015 at 11:40:40PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Feb 05, 2015 at 08:09:22PM +0100, Wolfram Sang wrote:
> > > clk_get functions return an ERR_PTR and not NULL in the error case. Make
> > > that consistent for the dummy functions when HAVE_CLK is not enabled.
> > > Otherwise unexpected codepaths might be trying to use a NULL pointer.
> > 
> > NAK.
> > 
> > There are some drivers which rely on this behaviour (take a driver,
> > such as some PXA IP) which uses the clk API but is also reused on
> > x86 which doesn't.
> 
> Okay, thanks for the explanation. Let me recap, there are three cases
> that clk_get can return:
> 
> a) pointer to a clk
> b) ERR_PTR meaning something went wrong when trying to get a clk
> c) NULL meaning there is no clk to get
> 
> So, if this patch from i2c/for-next [1] wants to add optional clk
> support, it should fallback to the old handling not by checking for
> (!IS_ERR()) but for (!IS_ERR_OR_NULL()).
> 
> Correct?
> 
> [1] https://git.kernel.org/cgit/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=e961a094afe04c6c8ca1adac50c8d16513f31b93

Not really.

/**
 * clk_get - lookup and obtain a reference to a clock producer.
 * @dev: device for clock "consumer"
 * @id: clock consumer ID
 *
 * Returns a struct clk corresponding to the clock producer, or
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 * valid IS_ERR() condition containing errno.  The implementation
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That description applies to users of clk_get() and the rest of the clk
API, and according to that definition:

	clk = clk_get();
	if (IS_ERR(clk)) {
		/*
		 * clk represents an error or a deferred probe
		 */
		error = PTR_ERR(clk);
	} else {
		/*
		 * the clk cookie is valid and can be passed into
		 * every other clk function.
		 */
	}

Now, as far as specific errors go, clk_get() (and devm_clk_get()) will
behave as follows with DT (I think - the inventers of this code really
need to get better documentation - and it's worth pointing out that
there's a lot of redundant error checking going on):

1. the specified clk is found
2. -EPROBE_DEFER error pointer if the clk is found in DT, but is not
   yet available
3. -ENOENT error pointer if the clk is not found

The NULL value is basically undefined for drivers, and according to the
definition above, drivers should treat it as "success".  It can be
passed into the rest of the clk API harmlessly, which may or may not
return an error (it's defined that the clk API shall be safe for any
value returned by clk_get() which is not an error-pointer.)

It was a concious decision to have the !CONFIG_HAVE_CLK cause the API
to return success when finding a clock - the reason for it is to allow
drivers (such as PXA) which require a struct clk to be re-used on x86
without requiring x86 to implement the clk API (which they objected to.)

The problem comes is that you seem to want something different to that.
This is for opencores, right?  It's in much the same position as
everything else - you don't want the driver to fail when we have
!CONFIG_HAVE_CLK, but you need the clock frequency internally.

I would suggest something like:

	i2c->clk = devm_clk_get(&pdev->dev, NULL);
	if (!IS_ERR(i2c->clk)) {
		int ret = clk_prepare_enable(i2c->clk);
		if (ret) {
			dev_err(&pdev->dev,
				"clk_prepare_enable failed: %d\n", ret);
			return ret;
		}
		i2c->ip_clock_khz = clk_get_rate(i2c->clk) / 1000;
	}

	if (i2c->ip_clock_khz == 0) {
		/*
		 * We don't have a valid clock rate from the clk API, try
		 * to get the clock frequency from DT.
		 */
		if (of_property_read_u32(np, "opencores,ip-clock-frequency",
					 &val)) {
			if (!clock_frequency_present) {
				dev_err(&pdev->dev,
					"Missing required parameter 'opencores,ip-clock-frequency'\n");
				/* presumably this is a probe failure */
			}
		}
	} else {
		if (clock_frequency_present)
			i2c->bus_clock_khz = clock_frequency / 1000;
	}

I'm not sure what you do with clock_frequency_present and bus_clock_khz
so that bit of the above is a guess (I'm not sure why you have it in the
if (!IS_ERR()) { } block.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

Russell,

thanks for the details again!

>  * Returns a struct clk corresponding to the clock producer, or
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>  * valid IS_ERR() condition containing errno.  The implementation
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Yes, I read that...

> The NULL value is basically undefined for drivers, and according to the
> definition above, drivers should treat it as "success".

... and this is what I stumbled over. For me, the NULL pointer meant
failure. But given your background information, especially about the
intentional and defined usage of the pointers returned by clk_get, I
get a better picture of the NULL return value.

> The problem comes is that you seem to want something different to that.
> This is for opencores, right?  It's in much the same position as
> everything else - you don't want the driver to fail when we have
> !CONFIG_HAVE_CLK, but you need the clock frequency internally.

Yes, but it is a bit more complicated...

> I'm not sure what you do with clock_frequency_present and bus_clock_khz
> so that bit of the above is a guess (I'm not sure why you have it in the
> if (!IS_ERR()) { } block.)

... which has to do with this. Historically, "clock-frequency" in DT
means the i2c bus speed. This driver used the binding wrong to specify
the IP core clock speed instead and also used a fixed I2C bus speed
then. Now, when wanting to support a) flexible I2C bus speeds and b)
clock information from standard DT clock bindings, we also need to
ensure that we support the now deprecated and wrong binding as a
fallback. And even after having learnt something about the clk API now,
I still think the best fix is to replace all instances of 'if
(!IS_ERR(dev->clk))' with (!IS_ERR_OR_NULL()). Because in our case, NULL
is not success. We should skip any clk API interaction then and use the
fallback mechanisms.

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Russell King - ARM Linux]

On Mon, Feb 09, 2015 at 03:29:54PM +0100, Wolfram Sang wrote:
> ... which has to do with this. Historically, "clock-frequency" in DT
> means the i2c bus speed. This driver used the binding wrong to specify
> the IP core clock speed instead and also used a fixed I2C bus speed
> then. Now, when wanting to support a) flexible I2C bus speeds and b)
> clock information from standard DT clock bindings, we also need to
> ensure that we support the now deprecated and wrong binding as a
> fallback. And even after having learnt something about the clk API now,
> I still think the best fix is to replace all instances of 'if
> (!IS_ERR(dev->clk))' with (!IS_ERR_OR_NULL()). Because in our case, NULL
> is not success. We should skip any clk API interaction then and use the
> fallback mechanisms.

I disagree; I also utterly _hate_ IS_ERR_OR_NULL - this macro has been
responsible for a /lot/ of error handling bugs, and I wish the macro
would die.

The "test-for-zero-clock-rate" method I showed in my previous email
will work for you - and will avoid using this hateful macro while
still giving the behaviour you desire, and you won't be caring about
the detail of the clk API implementation either.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] clk: return proper ERR_PTR for clk_get when !HAVE_CLK

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]


Back from a HDD crash...

> The "test-for-zero-clock-rate" method I showed in my previous email
> will work for you - and will avoid using this hateful macro while
> still giving the behaviour you desire, and you won't be caring about
> the detail of the clk API implementation either.

The resume path needed fixing, too. I'll send the patch right away.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]