[PATCH RESEND 4/6] clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL

[PATCH RESEND 4/6] clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-media@vger.kernel.org
---
 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(-)

diff --git a/drivers/media/platform/s5p-tv/hdmi_drv.c b/drivers/media/platform/s5p-tv/hdmi_drv.c
index 8a9cf43..1c48ca5 100644
--- a/drivers/media/platform/s5p-tv/hdmi_drv.c
+++ b/drivers/media/platform/s5p-tv/hdmi_drv.c
@@ -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;
 	}
diff --git a/drivers/media/platform/s5p-tv/mixer_drv.c b/drivers/media/platform/s5p-tv/mixer_drv.c
index ca0f297..c1b2e0e 100644
--- a/drivers/media/platform/s5p-tv/mixer_drv.c
+++ b/drivers/media/platform/s5p-tv/mixer_drv.c
@@ -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;
 	}
diff --git a/drivers/media/platform/s5p-tv/sdo_drv.c b/drivers/media/platform/s5p-tv/sdo_drv.c
index ad68bbe..269d246 100644
--- a/drivers/media/platform/s5p-tv/sdo_drv.c
+++ b/drivers/media/platform/s5p-tv/sdo_drv.c
@@ -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

Re: [PATCH RESEND 4/6] clk: s5p-tv: Fix incorrect usage of IS_ERR_OR_NULL

[Sylwester Nawrocki]

On 12/18/2012 06:34 PM, Tony Prisk wrote:
> Resend to include mailing lists.

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

> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.
>
> Signed-off-by: Tony Prisk<linux@prisktech.co.nz>
> CC: Kyungmin Park<kyungmin.park@samsung.com>
> CC: Tomasz Stanislawski<t.stanislaws@samsung.com>
> CC: linux-media@vger.kernel.org
> ---
>   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(-)

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

[PATCH RESEND 5/6] clk: s5p-fimc: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-media@vger.kernel.org
---
 drivers/media/platform/s5p-fimc/fimc-mdevice.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-fimc/fimc-mdevice.c b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
index 0531ab7..3ac4da2 100644
--- a/drivers/media/platform/s5p-fimc/fimc-mdevice.c
+++ b/drivers/media/platform/s5p-fimc/fimc-mdevice.c
@@ -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

[PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

Resend to include mailing lists.

Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
CC: Kyungmin Park <kyungmin.park@samsung.com>
CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
CC: linux-media@vger.kernel.org
---
 drivers/media/platform/s5p-g2d/g2d.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-g2d/g2d.c b/drivers/media/platform/s5p-g2d/g2d.c
index 1bfbc32..dcd5335 100644
--- a/drivers/media/platform/s5p-g2d/g2d.c
+++ b/drivers/media/platform/s5p-g2d/g2d.c
@@ -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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Sergei Shtylyov]

Hello.

On 18-12-2012 21:34, Tony Prisk wrote:

> Resend to include mailing lists.

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

> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.

> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
> CC: Kyungmin Park <kyungmin.park@samsung.com>
> CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
> CC: linux-media@vger.kernel.org

WBR, Sergei

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Sylwester Nawrocki]

On 12/22/2012 10:53 PM, Sergei Shtylyov wrote:
> Hello.
>
> On 18-12-2012 21:34, Tony Prisk wrote:
>
>> Resend to include mailing lists.
>
> Such remarks should be placed under --- tear line, not in the changelog.
>
>> Replace IS_ERR_OR_NULL with IS_ERR on clk_get results.
>
>> Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
>> CC: Kyungmin Park <kyungmin.park@samsung.com>
>> CC: Tomasz Stanislawski <t.stanislaws@samsung.com>
>> CC: linux-media@vger.kernel.org

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Tony Prisk]

On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> 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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Julia Lawall]

On Wed, 2 Jan 2013, Tony Prisk wrote:

> On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
>> 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

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
--- /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
--- /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
--- /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
--- /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);
  	}

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Wed, Jan 02, 2013 at 08:29:57AM +0100, Julia Lawall wrote:
> There are dereferences to the result of clk_get a few times.  I tried the 
> following semantic patch:

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.

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> > 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
> 

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Julia Lawall]

On Thu, 3 Jan 2013, Dan Carpenter wrote:

> On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> > > 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
> >
>
> 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.

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 10:14:13AM +0100, Julia Lawall wrote:
> On Thu, 3 Jan 2013, Dan Carpenter wrote:
> 
> > On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> > > On Wed, 2013-01-02 at 08:10 +0300, Dan Carpenter wrote:
> > > > 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
> > >
> >
> > 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.
> 
> 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.

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 12:05:20PM +0300, Dan Carpenter wrote:
> On Wed, Jan 02, 2013 at 06:31:53PM +1300, Tony Prisk wrote:
> > 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
> > 
> 
> Ah.  Grand.  Thanks...
> 
> Btw. The documentation for clk_get() really should include some of
> this information.

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 02:10:40PM +0300, Dan Carpenter wrote:
> 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:

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.

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

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.

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Dan Carpenter]

On Thu, Jan 03, 2013 at 11:21:02AM +0000, Russell King - ARM Linux wrote:
> Maybe you don't realise, but IS_ERR(NULL) is false.  Therefore, this falls
> into category (2).

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Thu, Jan 03, 2013 at 04:45:54PM +0300, Dan Carpenter wrote:
> On Thu, Jan 03, 2013 at 11:21:02AM +0000, Russell King - ARM Linux wrote:
> > Maybe you don't realise, but IS_ERR(NULL) is false.  Therefore, this falls
> > into category (2).
> 
> No, obviously, I know the difference between IS_ERR() and
> IS_ERR_OR_NULL().  That's how we started this thread.

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> 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.

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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Julia Lawall]

On Wed, 2 Jan 2013, Russell King - ARM Linux wrote:

> On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> > 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.
>
> 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

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Russell King - ARM Linux]

On Wed, Jan 02, 2013 at 10:44:32AM +0100, Julia Lawall wrote:
> On Wed, 2 Jan 2013, Russell King - ARM Linux wrote:
> 
> > On Wed, Jan 02, 2013 at 08:10:36AM +0300, Dan Carpenter wrote:
> > > 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.
> >
> > 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?

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

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

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.

Re: [PATCH RESEND 6/6] clk: s5p-g2d: Fix incorrect usage of IS_ERR_OR_NULL

[Sylwester Nawrocki]

On 01/02/2013 06:10 AM, Dan Carpenter wrote:
> clk_get() returns NULL if CONFIG_HAVE_CLK is disabled.

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.

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

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