Inconsistency in clk framework

Hi Mike,

In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.

This seems to contradict the kernel docs associated with the normal clk_get (when HAVE_CLK is defined) which states:

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

Wouldn't a return code of ERR_PTR(-ENOENT) make more sense and be inline with the empty of_ versions as well (which return -ENOENT when CONFIG_OF is undefined).

Also, I noticed that clk_get_sys() doesn't appear to be defined in clk.h when HAVE_CLK is undefined - is this correct?

Quoted 4 lines · Tony Prisk

Hi Mike,

In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.

That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working?

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Tony Prisk

Hi Mike,

In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.

That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working?

That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock.

Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct?

Quoted 14 lines · Tony Prisk

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Tony Prisk

Hi Mike,

In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.

That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working?

That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock.

Quoted 3 lines

Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct?

For the sake of clarity, I should rephrase:

If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.

Quoted 20 lines · Tony Prisk

Quoted 11 lines · Tony Prisk

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Tony Prisk

Hi Mike,

In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.

That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working?

Quoted 6 lines

That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock.

Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct?

For the sake of clarity, I should rephrase:

Quoted 2 lines

If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.

Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so.

Quoted 21 lines · Russell King - ARM Linux

Quoted 20 lines · Tony Prisk

Quoted 11 lines · Tony Prisk

Quoted 9 lines · Russell King - ARM Linux

Quoted 4 lines · Tony Prisk

Hi Mike,

In attempting to remove some IS_ERR_OR_NULL references, it was pointed out that clk_get() can return NULL if CONFIG_HAVE_CLK is not defined.

That is correct - but why is that a problem? As far as users are concerned, NULL is a valid clock. If HAVE_CLK is undefined, do you want all your drivers to suddenly stop working?

Quoted 6 lines

That will be where the misunderstanding has occurred - I didn't consider NULL to be a valid clock.

Given that NULL is a valid clock, I guess all tests against get_clk and of_get_clk results should be IS_ERR_OR_NULL. Correct?

For the sake of clarity, I should rephrase:

Quoted 8 lines

Quoted 2 lines

If the driver can't operate with a NULL clk, it should use a IS_ERR_OR_NULL test to test for failure, rather than IS_ERR.

Why should a _consumer_ of a clock care? It is _very_ important that people get this idea - to a consumer, the struct clk is just an opaque cookie. The fact that it appears to be a pointer does _not_ mean that the driver can do any kind of dereferencing on that pointer - it should never do so.

As a simple example:
We have a PWM module that requires a clock source to be enabled before registers can be read/written.

*pseudo code*
x = clk_get("pwm_clock")
if IS_ERR(x) then fail
err = clk_enable(x)
if (err != 0) then fail
start writing to module registers

Assuming HAVE_CLK is undefined:

x = clk_get("pwm_clock") (= NULL)
if IS_ERR(x) then fail (not an error)
err = clk_enable(x) (= 0)
if (err) then fail (not an error)
start writing to module registers
(register writes lock the bus because the clock wasn't really enabled, but no errors occurred enabling the clock)