Remove incorrect usage of IS_ERR_OR_NULL on clk_get

Resend to include mailing lists.

As per the kernel docs, clk_get() cannot return NULL.

* 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.

This patch set replaces instances of IS_ERR_OR_NULL tests againt clk_get() results with IS_ERR tests.

Tony Prisk (6):
clk: omap: Fix incorrect usage of IS_ERR_OR_NULL
clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL
clk: exynos: Fix incorrect usage of IS_ERR_OR_NULL
clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL
clk: s5p-fimc: Fix incorrect usage of IS_ERR_OR_NULL
clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

 arch/arm/plat-omap/dmtimer.c                   |    6 +++---
 drivers/gpu/drm/exynos/exynos_hdmi.c           |   10 +++++-----
 drivers/gpu/drm/exynos/exynos_mixer.c          |   14 +++++++-------
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |    2 +-
 drivers/media/platform/s5p-g2d/g2d.c           |    4 ++--
 drivers/media/platform/s5p-tv/hdmi_drv.c       |   10 +++++-----
 drivers/media/platform/s5p-tv/mixer_drv.c      |   10 +++++-----
 drivers/media/platform/s5p-tv/sdo_drv.c        |   10 +++++-----
 8 files changed, 33 insertions(+), 33 deletions(-)

1.7.9.5

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

---
 arch/arm/plat-omap/dmtimer.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 938b50a..e5e150d 100644
@@ -143,7 +143,7 @@ int omap_dm_timer_prepare(struct omap_dm_timer *timer)
 	 */
 	if (!(timer->capability & OMAP_TIMER_NEEDS_RESET)) {
 		timer->fclk = clk_get(&timer->pdev->dev, "fck");
-		if (WARN_ON_ONCE(IS_ERR_OR_NULL(timer->fclk))) {
+		if (WARN_ON_ONCE(IS_ERR(timer->fclk))) {
 			timer->fclk = NULL;
 			dev_err(&timer->pdev->dev, ": No fclk handle.\n");
 			return -EINVAL;

@@ -418,7 +418,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 		return pdata->set_timer_src(timer->pdev, source);
 
 	fclk = clk_get(&timer->pdev->dev, "fck");
-	if (IS_ERR_OR_NULL(fclk)) {
+	if (IS_ERR(fclk)) {
 		pr_err("%s: fck not found\n", __func__);
 		return -EINVAL;
 	}

@@ -438,7 +438,7 @@ int omap_dm_timer_set_source(struct omap_dm_timer *timer, int source)
 	}
 
 	parent = clk_get(&timer->pdev->dev, parent_name);
-	if (IS_ERR_OR_NULL(parent)) {
+	if (IS_ERR(parent)) {
 		pr_err("%s: %s not found\n", __func__, parent_name);
 		ret = -EINVAL;
 		goto out;

1.7.9.5

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

In the fail: path of mixer_resources_init() and vp_resources_init() the first clk tested cannot be NULL either, so IS_ERR_OR_NULL is removed from these as well. Other clocks may still be NULL as they haven't been clk_get'd yet.

---
 drivers/gpu/drm/exynos/exynos_mixer.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

index e7fbb82..dbd97c0 100644
@@ -972,14 +972,14 @@ static int __devinit mixer_resources_init(struct exynos_drm_hdmi_context *ctx,
 	spin_lock_init(&mixer_res->reg_slock);
 
 	mixer_res->mixer = clk_get(dev, "mixer");
-	if (IS_ERR_OR_NULL(mixer_res->mixer)) {
+	if (IS_ERR(mixer_res->mixer)) {
 		dev_err(dev, "failed to get clock 'mixer'\n");
 		ret = -ENODEV;
 		goto fail;
 	}
 
 	mixer_res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(mixer_res->sclk_hdmi)) {
+	if (IS_ERR(mixer_res->sclk_hdmi)) {
 		dev_err(dev, "failed to get clock 'sclk_hdmi'\n");
 		ret = -ENODEV;
 		goto fail;

@@ -1019,7 +1019,7 @@ static int __devinit mixer_resources_init(struct exynos_drm_hdmi_context *ctx,
 fail:
 	if (!IS_ERR_OR_NULL(mixer_res->sclk_hdmi))
 		clk_put(mixer_res->sclk_hdmi);
-	if (!IS_ERR_OR_NULL(mixer_res->mixer))
+	if (!IS_ERR(mixer_res->mixer))
 		clk_put(mixer_res->mixer);
 	return ret;
 }

@@ -1034,19 +1034,19 @@ static int __devinit vp_resources_init(struct exynos_drm_hdmi_context *ctx,
 	int ret;
 
 	mixer_res->vp = clk_get(dev, "vp");
-	if (IS_ERR_OR_NULL(mixer_res->vp)) {
+	if (IS_ERR(mixer_res->vp)) {
 		dev_err(dev, "failed to get clock 'vp'\n");
 		ret = -ENODEV;
 		goto fail;
 	}
 	mixer_res->sclk_mixer = clk_get(dev, "sclk_mixer");
-	if (IS_ERR_OR_NULL(mixer_res->sclk_mixer)) {
+	if (IS_ERR(mixer_res->sclk_mixer)) {
 		dev_err(dev, "failed to get clock 'sclk_mixer'\n");
 		ret = -ENODEV;
 		goto fail;
 	}
 	mixer_res->sclk_dac = clk_get(dev, "sclk_dac");
-	if (IS_ERR_OR_NULL(mixer_res->sclk_dac)) {
+	if (IS_ERR(mixer_res->sclk_dac)) {
 		dev_err(dev, "failed to get clock 'sclk_dac'\n");
 		ret = -ENODEV;
 		goto fail;

@@ -1077,7 +1077,7 @@ fail:
 		clk_put(mixer_res->sclk_dac);
 	if (!IS_ERR_OR_NULL(mixer_res->sclk_mixer))
 		clk_put(mixer_res->sclk_mixer);
-	if (!IS_ERR_OR_NULL(mixer_res->vp))
+	if (!IS_ERR(mixer_res->vp))
 		clk_put(mixer_res->vp);
 	return ret;
 }

1.7.9.5

These kind of comments should go under the Signed of by line under a "---" line. They will be removed by git-am instead of being preserved in the git log.

Signed-off-by bla bla blah
---
Commments...

<patch>

regards,
dan carpenter

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

---
 drivers/gpu/drm/exynos/exynos_hdmi.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

index 2c115f8..557ef2f 100644
@@ -2167,27 +2167,27 @@ static int __devinit hdmi_resources_init(struct hdmi_context *hdata)
 
 	/* get clocks, power */
 	res->hdmi = clk_get(dev, "hdmi");
-	if (IS_ERR_OR_NULL(res->hdmi)) {
+	if (IS_ERR(res->hdmi)) {
 		DRM_ERROR("failed to get clock 'hdmi'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(res->sclk_hdmi)) {
+	if (IS_ERR(res->sclk_hdmi)) {
 		DRM_ERROR("failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_pixel = clk_get(dev, "sclk_pixel");
-	if (IS_ERR_OR_NULL(res->sclk_pixel)) {
+	if (IS_ERR(res->sclk_pixel)) {
 		DRM_ERROR("failed to get clock 'sclk_pixel'\n");
 		goto fail;
 	}
 	res->sclk_hdmiphy = clk_get(dev, "sclk_hdmiphy");
-	if (IS_ERR_OR_NULL(res->sclk_hdmiphy)) {
+	if (IS_ERR(res->sclk_hdmiphy)) {
 		DRM_ERROR("failed to get clock 'sclk_hdmiphy'\n");
 		goto fail;
 	}
 	res->hdmiphy = clk_get(dev, "hdmiphy");
-	if (IS_ERR_OR_NULL(res->hdmiphy)) {
+	if (IS_ERR(res->hdmiphy)) {
 		DRM_ERROR("failed to get clock 'hdmiphy'\n");
 		goto fail;
 	}

1.7.9.5

The original code is correct. clk_get() can return NULL depending on the .config.

regards,
dan carpenter

Thanks for than Dan,

Arguably that seems like an incorrect behaviour on the part of the clock subsystem given that the 'proper' function has kernel doc:

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

Therefore the 'empty' version should adhere to the same rules, and not return NULL.

I've cc'd Mike Turquette as well for his thoughts.

Regards
Tony P

I don't care either way, but being different from the documentation is less bad than crashing which is what your patch does. Please be more careful in the future.

regards,
dan carpenter

Critism accepted.

Given that the driver depends on (PLAT_SAMSUNG || ARCH_MULTIPLATFORM), and multiplatform select COMMON_CLK and PLAT_SAMSUNG is selected only by ARCH_S3C64XX which has HAVE_CLK I didn't see a problem here.

Your point is still valid and I will sort it out with Mike first.

Regards
Tony P

Actually, I was wrong here - PLAT_SAMSUNG is selectable via

PLAT_S3C24XX || ARCH_S3C64XX || PLAT_S5P

but all these options (or the options that select them) seem to select a clock system as well, hence still no reason for a crash.

Regards
Tony P

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

---
 drivers/media/platform/s5p-tv/hdmi_drv.c  |   10 +++++-----
 drivers/media/platform/s5p-tv/mixer_drv.c |   10 +++++-----
 drivers/media/platform/s5p-tv/sdo_drv.c   |   10 +++++-----
 3 files changed, 15 insertions(+), 15 deletions(-)

index 8a9cf43..1c48ca5 100644
@@ -781,27 +781,27 @@ static int hdmi_resources_init(struct hdmi_device *hdev)
 	/* get clocks, power */
 
 	res->hdmi = clk_get(dev, "hdmi");
-	if (IS_ERR_OR_NULL(res->hdmi)) {
+	if (IS_ERR(res->hdmi)) {
 		dev_err(dev, "failed to get clock 'hdmi'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(res->sclk_hdmi)) {
+	if (IS_ERR(res->sclk_hdmi)) {
 		dev_err(dev, "failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_pixel = clk_get(dev, "sclk_pixel");
-	if (IS_ERR_OR_NULL(res->sclk_pixel)) {
+	if (IS_ERR(res->sclk_pixel)) {
 		dev_err(dev, "failed to get clock 'sclk_pixel'\n");
 		goto fail;
 	}
 	res->sclk_hdmiphy = clk_get(dev, "sclk_hdmiphy");
-	if (IS_ERR_OR_NULL(res->sclk_hdmiphy)) {
+	if (IS_ERR(res->sclk_hdmiphy)) {
 		dev_err(dev, "failed to get clock 'sclk_hdmiphy'\n");
 		goto fail;
 	}
 	res->hdmiphy = clk_get(dev, "hdmiphy");
-	if (IS_ERR_OR_NULL(res->hdmiphy)) {
+	if (IS_ERR(res->hdmiphy)) {
 		dev_err(dev, "failed to get clock 'hdmiphy'\n");
 		goto fail;
 	}

index ca0f297..c1b2e0e 100644
@@ -240,27 +240,27 @@ static int mxr_acquire_clocks(struct mxr_device *mdev)
 	struct device *dev = mdev->dev;
 
 	res->mixer = clk_get(dev, "mixer");
-	if (IS_ERR_OR_NULL(res->mixer)) {
+	if (IS_ERR(res->mixer)) {
 		mxr_err(mdev, "failed to get clock 'mixer'\n");
 		goto fail;
 	}
 	res->vp = clk_get(dev, "vp");
-	if (IS_ERR_OR_NULL(res->vp)) {
+	if (IS_ERR(res->vp)) {
 		mxr_err(mdev, "failed to get clock 'vp'\n");
 		goto fail;
 	}
 	res->sclk_mixer = clk_get(dev, "sclk_mixer");
-	if (IS_ERR_OR_NULL(res->sclk_mixer)) {
+	if (IS_ERR(res->sclk_mixer)) {
 		mxr_err(mdev, "failed to get clock 'sclk_mixer'\n");
 		goto fail;
 	}
 	res->sclk_hdmi = clk_get(dev, "sclk_hdmi");
-	if (IS_ERR_OR_NULL(res->sclk_hdmi)) {
+	if (IS_ERR(res->sclk_hdmi)) {
 		mxr_err(mdev, "failed to get clock 'sclk_hdmi'\n");
 		goto fail;
 	}
 	res->sclk_dac = clk_get(dev, "sclk_dac");
-	if (IS_ERR_OR_NULL(res->sclk_dac)) {
+	if (IS_ERR(res->sclk_dac)) {
 		mxr_err(mdev, "failed to get clock 'sclk_dac'\n");
 		goto fail;
 	}

index ad68bbe..269d246 100644
@@ -341,25 +341,25 @@ static int __devinit sdo_probe(struct platform_device *pdev)
 
 	/* acquire clocks */
 	sdev->sclk_dac = clk_get(dev, "sclk_dac");
-	if (IS_ERR_OR_NULL(sdev->sclk_dac)) {
+	if (IS_ERR(sdev->sclk_dac)) {
 		dev_err(dev, "failed to get clock 'sclk_dac'\n");
 		ret = -ENXIO;
 		goto fail;
 	}
 	sdev->dac = clk_get(dev, "dac");
-	if (IS_ERR_OR_NULL(sdev->dac)) {
+	if (IS_ERR(sdev->dac)) {
 		dev_err(dev, "failed to get clock 'dac'\n");
 		ret = -ENXIO;
 		goto fail_sclk_dac;
 	}
 	sdev->dacphy = clk_get(dev, "dacphy");
-	if (IS_ERR_OR_NULL(sdev->dacphy)) {
+	if (IS_ERR(sdev->dacphy)) {
 		dev_err(dev, "failed to get clock 'dacphy'\n");
 		ret = -ENXIO;
 		goto fail_dac;
 	}
 	sclk_vpll = clk_get(dev, "sclk_vpll");
-	if (IS_ERR_OR_NULL(sclk_vpll)) {
+	if (IS_ERR(sclk_vpll)) {
 		dev_err(dev, "failed to get clock 'sclk_vpll'\n");
 		ret = -ENXIO;
 		goto fail_dacphy;

@@ -367,7 +367,7 @@ static int __devinit sdo_probe(struct platform_device *pdev)
 	clk_set_parent(sdev->sclk_dac, sclk_vpll);
 	clk_put(sclk_vpll);
 	sdev->fout_vpll = clk_get(dev, "fout_vpll");
-	if (IS_ERR_OR_NULL(sdev->fout_vpll)) {
+	if (IS_ERR(sdev->fout_vpll)) {
 		dev_err(dev, "failed to get clock 'fout_vpll'\n");
 		goto fail_dacphy;
 	}

1.7.9.5

In the future please put such comments after the --- tear line below.

Applied, after resolving conflict with other patch that addressed those issues in sdo_drv.c file. Thanks.

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

---
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 0531ab7..3ac4da2 100644
@@ -730,7 +730,7 @@ static int fimc_md_get_clocks(struct fimc_md *fmd)
 	for (i = 0; i < FIMC_MAX_CAMCLKS; i++) {
 		snprintf(clk_name, sizeof(clk_name), "sclk_cam%u", i);
 		clock = clk_get(NULL, clk_name);
-		if (IS_ERR_OR_NULL(clock)) {
+		if (IS_ERR(clock)) {
 			v4l2_err(&fmd->v4l2_dev, "Failed to get clock: %s",
 				  clk_name);
 			return -ENXIO;

1.7.9.5

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

---
 drivers/media/platform/s5p-g2d/g2d.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1bfbc32..dcd5335 100644
@@ -715,7 +715,7 @@ static int g2d_probe(struct platform_device *pdev)
 	}
 
 	dev->clk = clk_get(&pdev->dev, "sclk_fimg2d");
-	if (IS_ERR_OR_NULL(dev->clk)) {
+	if (IS_ERR(dev->clk)) {
 		dev_err(&pdev->dev, "failed to get g2d clock\n");
 		return -ENXIO;
 	}

@@ -727,7 +727,7 @@ static int g2d_probe(struct platform_device *pdev)
 	}
 
 	dev->gate = clk_get(&pdev->dev, "fimg2d");
-	if (IS_ERR_OR_NULL(dev->gate)) {
+	if (IS_ERR(dev->gate)) {
 		dev_err(&pdev->dev, "failed to get g2d clock gate\n");
 		ret = -ENXIO;
 		goto unprep_clk;

1.7.9.5

Hello.

Such remarks should be placed under --- tear line, not in the changelog.

WBR, Sergei

I've applied first version of this patch to my tree for 3.9, thanks.

clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.

I told Tony about this but everyone has been gone with end of year holidays so it hasn't been addressed.

Tony, please fix it so people don't apply these patches until clk_get() is updated to not return NULL. It sucks to have to revert patches.

regards,
dan carpenter

I posted the query to Mike Turquette, linux-kernel and linux-arm-kernel mailing lists, regarding the return of NULL when HAVE_CLK is undefined.

Short Answer: A return value of NULL is valid and not an error therefore we should be using IS_ERR, not IS_ERR_OR_NULL on clk_get results.

I see the obvious problem this creates, and asked this question:

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.

And Russell's answer:

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.

Thread can be viewed here:
https://lkml.org/lkml/2012/12/20/105

Regards
Tony Prisk

There are dereferences to the result of clk_get a few times. I tried the following semantic patch:

@@ expression E; identifier I; @@
* E = clk_get(...) ... E->I

It gives the results shown below (- marks matched lines, not lines to remove). I also tried with devm_clk_get instead of clk_get, but got nothing.

julia

diff -u -p /var/linuxes/linux-next/arch/sh/kernel/cpufreq.c /tmp/nothing/arch/sh/kernel/cpufreq.c
@@ -117,15 +117,11 @@ static int sh_cpufreq_cpu_init(struct cp
 
  	dev = get_cpu_device(cpu);
 
-	cpuclk = clk_get(dev, "cpu_clk");
-	if (IS_ERR(cpuclk)) {
  		dev_err(dev, "couldn't get CPU clk\n");
  		return PTR_ERR(cpuclk);
  	}
 
-	policy->cur = policy->min = policy->max = sh_cpufreq_get(cpu);
 
-	freq_table = cpuclk->nr_freqs ? cpuclk->freq_table : NULL;
  	if (freq_table) {
  		int result;
 

diff -u -p /var/linuxes/linux-next/arch/mips/kernel/cpufreq/loongson2_cpufreq.c /tmp/nothing/arch/mips/kernel/cpufreq/loongson2_cpufreq.c
@@ -111,13 +111,10 @@ static int loongson2_cpufreq_cpu_init(st
  	if (!cpu_online(policy->cpu))
  		return -ENODEV;
 
-	cpuclk = clk_get(NULL, "cpu_clk");
-	if (IS_ERR(cpuclk)) {
  		printk(KERN_ERR "cpufreq: couldn't get CPU clk\n");
  		return PTR_ERR(cpuclk);
  	}
 
-	cpuclk->rate = cpu_clock_freq / 1000;
  	if (!cpuclk->rate)
  		return -EINVAL;
 

diff -u -p /var/linuxes/linux-next/drivers/net/ethernet/ti/cpts.c /tmp/nothing/drivers/net/ethernet/ti/cpts.c
@@ -241,14 +241,10 @@ static void cpts_overflow_check(struct w
 
  static void cpts_clk_init(struct cpts *cpts)
  {
-	cpts->refclk = clk_get(NULL, CPTS_REF_CLOCK_NAME);
-	if (IS_ERR(cpts->refclk)) {
  		pr_err("Failed to clk_get %s\n", CPTS_REF_CLOCK_NAME);
  		cpts->refclk = NULL;
  		return;
  	}
-	clk_enable(cpts->refclk);
-	cpts->freq = cpts->refclk->recalc(cpts->refclk);
  }
 
  static void cpts_clk_release(struct cpts *cpts)

diff -u -p /var/linuxes/linux-next/drivers/i2c/busses/i2c-sh7760.c /tmp/nothing/drivers/i2c/busses/i2c-sh7760.c
@@ -397,11 +397,7 @@ static int calc_CCR(unsigned long scl_hz
  	signed char cdf, cdfm;
  	int scgd, scgdm, scgds;
 
-	mclk = clk_get(NULL, "peripheral_clk");
-	if (IS_ERR(mclk)) {
  		return PTR_ERR(mclk);
-	} else {
-		mck = mclk->rate;
  		clk_put(mclk);
  	}
 

And those are buggy; struct clk is _SUPPOSED_ to be an OPAQUE COOKIE and no one other than the clk code should be dereferencing it.

I guess this is the danger of the common clk API - we now have a struct clk that is exposed in a linux/*.h include which anyone can include, and now anyone can get a definition for a struct clk and dereference it, destroying opaqueness of this cookie.

Ah. Grand. Thanks...

Btw. The documentation for clk_get() really should include some of this information. I know Russell thinks that the driver authors are stupid and lazy, and it's probably true. But if everyone makes the same mistake over and over, then it probably means we could put a special note:

"Do not check this with IS_ERR_OR_NULL(). Null values are not an error. Drivers should treat the return value as an opaque cookie and they should not dereference it."

This is probably there in the file somewhere else, but I searched for "opaque", "cookie", and "dereference" and I didn't find anything. I'm not saying the documentation isn't perfect, just that driver authors are lazy and stupid but we can't kill them so we have to live with them.

regards,
dan carpenter

I still think it would also be helpful for the definition that returns NULL to have some documentation associated with it. Having a feature disabled and then trying to use the feature could reasonably considered to lead to a failure, so it is not obvious what the NULL represents.

julia

/**
* 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
* uses @dev and @id to determine the clock consumer, and thereby
* the clock producer. (IOW, @id may be identical strings, but
* clk_get may return different clock producers depending on @dev.) *
* Drivers must assume that the clock source is not enabled. *
* clk_get should not be called from within interrupt context. */

It *does* contain this information. The problem is that driver authors _ARE_ stupid, lazy morons who don't bother to read documentation.

/**
* 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
* uses @dev and @id to determine the clock consumer, and thereby
* the clock producer. (IOW, @id may be identical strings, but
* clk_get may return different clock producers depending on @dev.) *
* Drivers must assume that the clock source is not enabled. *
* clk_get should not be called from within interrupt context. */

Come on... Don't say we haven't read comment. Obviously, the first thing we did was read that comment. I've read it many times at this point and I still think we should add in a bit which says:

"NOTE: Drivers should treat the return value as an opaque cookie and not dereference it. NULL returns don't imply an error so don't use IS_ERR_OR_NULL() to check for errors."

regards,
dan carpenter

So where does it give you in that comment permission to treat NULL any differently to any other non-IS_ERR() return value?

It is very clear: values where IS_ERR() is true are considered errors. Everything else is considered valid.

No. The one thing I've learnt through maintaining www.arm.linux.org.uk is that the more of these kinds of "lets add to documentation" suggestions you get, the more _unclear_ the documentation becomes, and the more it is open to bad interpretation, and the more suggestions to add more words you receive.

Concise documentation is the only way to go. And what we have there today is concise and to the point. It specifies it very clearly:

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

That one sentence gives you all the information you need about it's return value. It gives you two choices. (1) a return value where IS_ERR() is true, which is an error, and (2) a return value where IS_ERR() is false, which is a valid cookie.

Maybe you don't realise, but IS_ERR(NULL) is false. Therefore, this falls into category (2).

You can't get clearer than that, unless you don't understand the IS_ERR() and associated macro.

Moreover, it tells you the function to use to check the return value for errors. IS_ERR(). It doesn't say IS_ERR_OR_NULL(), it says IS_ERR().

All it takes is for people to engage their grey cells and read the documentation as it stands, rather than trying to weasel their way around it and invent crap that it doesn't say.

No, obviously, I know the difference between IS_ERR() and IS_ERR_OR_NULL(). That's how we started this thread.

*shrug*.

regards,
dan carpenter

Well, if you know that, then how is the documentation anything _but_ clear?

How about people stop using IS_ERR_OR_NULL for stuff which it shouldn't be used for?

Perhaps the cases where clk_get returns NULL could have a comment indicating that NULL does not represent a failure?

In 3.7.1, it looks like it might have been possible for NULL to be returned by clk_get in arch/mips/loongson1/common/clock.c, but that definition seems to be gone in a recent linux-next. The remaining definitions look OK.

julia

No. More documentation is never the answer to people not reading existing documentation.

How about people just read the API and comply with it rather than doing their own thing all the time? We've already had at least one instance where someone has tried using IS_ERR() with the ioremap() return value.

Really, if you're going to program kernel space, it is very important that you *know* the interfaces that you're using and you comply with them. Otherwise, you have no business saying that you're a kernel programmer.

Yes, the odd mistake happens, but that's no excuse for the constant blatent mistakes with stuff like IS_ERR_OR_NULL() with clk_get() which just comes from total laziness on the part of the coder to understand the interfaces being used. Hell, it's even documented in linux/clk.h - that just shows how many people read the documentation which has been around since the clk API came about.

It's not a problem for this driver, as it never dereferences what's returned from clk_get(). It would have to include <plat/clock.h>, which it doesn't and which would have clearly indicated abuse of the clock API.

Moreover, this driver now depends on architectures that select HAVE_CLK, so it couldn't be build when CONFIG_HAVE_CLK is disabled.

As explained by Russell many times, the clock API users should not care whether the value returned from clk_get() is NULL or not. It should only be tested with IS_ERR(). The patches look fine to me, no need to do anything.

---

Regards,
Sylwester

It returns NULL if CONFIG_HAVE_CLK is disabled.

regards,
dan carpenter