[PATCH 0/4] Add missing IS_ERR test

[PATCH 0/4] Add missing IS_ERR test

[Julia Lawall]

These patches perform various adjustments to error handling code related to
the possibility of an ERR_PTR value.

[PATCH 1/4] fs/btrfs/inode.c: Add missing IS_ERR test

[Julia Lawall]

After the conditional that precedes the following code, inode may be an
ERR_PTR value.  This can eg result from a memory allocation failure via the
call to btrfs_iget, and thus does not imply that root is different than
sub_root.  Thus, an IS_ERR check is added to ensure that there is no
dereference of inode in this case.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier f;
@@
f(...) { ... return ERR_PTR(...); }

@@
identifier r.f, fld;
expression x;
statement S1,S2;
@@
 x = f(...)
 ... when != IS_ERR(x)
(
 if (IS_ERR(x) ||...) S1 else S2
|
*x->fld
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 fs/btrfs/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 160b55b..b322158 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4134,7 +4134,7 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry)
 	}
 	srcu_read_unlock(&root->fs_info->subvol_srcu, index);
 
-	if (root != sub_root) {
+	if (!IS_ERR(inode) && root != sub_root) {
 		down_read(&root->fs_info->cleanup_work_sem);
 		if (!(inode->i_sb->s_flags & MS_RDONLY))
 			btrfs_orphan_cleanup(sub_root);

[PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

Function clk_get, defined just below this code, returns ERR_PTR not NULL in
an error case.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier f;
@@
f(...) { ... return ERR_PTR(...); }

@@
identifier r.f, fld;
expression x;
statement S1,S2;
@@
 x = f(...)
 ... when != IS_ERR(x)
(
 if (IS_ERR(x) ||...) S1 else S2
|
*x->fld
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/arm/mach-at91/clock.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c
index 9113da6..c9ee6d0 100644
--- a/arch/arm/mach-at91/clock.c
+++ b/arch/arm/mach-at91/clock.c
@@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
 {
 	struct clk *clk = clk_get(NULL, id);
 
-	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
+	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
 		return;
 
 	clk->function = func;

[PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test

[Julia Lawall]

lcd_device_register may return ERR_PTR, so a check is added for this value
before the dereference.  All of the other changes reorganize the error
handling code in this function to avoid duplicating all of it in the added
case.

In the original code, in one case, the global variable fb_buffer was set to
NULL in error code that appears after this variable is initialized.  This
is done now in all error handling code that has this property.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier f;
@@
f(...) { ... return ERR_PTR(...); }

@@
identifier r.f, fld;
expression x;
statement S1,S2;
@@
 x = f(...)
 ... when != IS_ERR(x)
(
 if (IS_ERR(x) ||...) S1 else S2
|
*x->fld
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 drivers/video/bf537-lq035.c |   58 +++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 25 deletions(-)

diff --git a/drivers/video/bf537-lq035.c b/drivers/video/bf537-lq035.c
index 18c5078..47c21fb 100644
--- a/drivers/video/bf537-lq035.c
+++ b/drivers/video/bf537-lq035.c
@@ -696,6 +696,7 @@ static int __devinit bfin_lq035_probe(struct platform_device *pdev)
 {
 	struct backlight_properties props;
 	dma_addr_t dma_handle;
+	int ret;
 
 	if (request_dma(CH_PPI, KBUILD_MODNAME)) {
 		pr_err("couldn't request PPI DMA\n");
@@ -704,17 +705,16 @@ static int __devinit bfin_lq035_probe(struct platform_device *pdev)
 
 	if (request_ports()) {
 		pr_err("couldn't request gpio port\n");
-		free_dma(CH_PPI);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto out_ports;
 	}
 
 	fb_buffer = dma_alloc_coherent(NULL, TOTAL_VIDEO_MEM_SIZE,
 				       &dma_handle, GFP_KERNEL);
 	if (fb_buffer == NULL) {
 		pr_err("couldn't allocate dma buffer\n");
-		free_dma(CH_PPI);
-		free_ports();
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_dma_coherent;
 	}
 
 	if (L1_DATA_A_LENGTH)
@@ -725,10 +725,8 @@ static int __devinit bfin_lq035_probe(struct platform_device *pdev)
 
 	if (dma_desc_table == NULL) {
 		pr_err("couldn't allocate dma descriptor\n");
-		free_dma(CH_PPI);
-		free_ports();
-		dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_table;
 	}
 
 	bfin_lq035_fb.screen_base = (void *)fb_buffer;
@@ -771,31 +769,21 @@ static int __devinit bfin_lq035_probe(struct platform_device *pdev)
 	bfin_lq035_fb.pseudo_palette = kzalloc(sizeof(u32) * 16, GFP_KERNEL);
 	if (bfin_lq035_fb.pseudo_palette == NULL) {
 		pr_err("failed to allocate pseudo_palette\n");
-		free_dma(CH_PPI);
-		free_ports();
-		dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_palette;
 	}
 
 	if (fb_alloc_cmap(&bfin_lq035_fb.cmap, NBR_PALETTE, 0) < 0) {
 		pr_err("failed to allocate colormap (%d entries)\n",
 			NBR_PALETTE);
-		free_dma(CH_PPI);
-		free_ports();
-		dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0);
-		kfree(bfin_lq035_fb.pseudo_palette);
-		return -EFAULT;
+		ret = -EFAULT;
+		goto out_cmap;
 	}
 
 	if (register_framebuffer(&bfin_lq035_fb) < 0) {
 		pr_err("unable to register framebuffer\n");
-		free_dma(CH_PPI);
-		free_ports();
-		dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0);
-		fb_buffer = NULL;
-		kfree(bfin_lq035_fb.pseudo_palette);
-		fb_dealloc_cmap(&bfin_lq035_fb.cmap);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out_reg;
 	}
 
 	i2c_add_driver(&ad5280_driver);
@@ -807,11 +795,31 @@ static int __devinit bfin_lq035_probe(struct platform_device *pdev)
 
 	lcd_dev = lcd_device_register(KBUILD_MODNAME, &pdev->dev, NULL,
 				      &bfin_lcd_ops);
+	if (IS_ERR(lcd_dev)) {
+		pr_err("unable to register lcd\n");
+		ret = PTR_ERR(lcd_dev);
+		goto out_lcd;
+	}
 	lcd_dev->props.max_contrast = 255,
 
 	pr_info("initialized");
 
 	return 0;
+out_lcd:
+	unregister_framebuffer(&bfin_lq035_fb);
+out_reg:
+	fb_dealloc_cmap(&bfin_lq035_fb.cmap);
+out_cmap:
+	kfree(bfin_lq035_fb.pseudo_palette);
+out_palette:
+out_table:
+	dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0);
+	fb_buffer = NULL;
+out_dma_coherent:
+	free_ports();
+out_ports:
+	free_dma(CH_PPI);
+	return ret;
 }
 
 static int __devexit bfin_lq035_remove(struct platform_device *pdev)

[PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test

[Julia Lawall]

Function _sr_lookup, defined in the same file, returns ERR_PTR not NULL in
an error case.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@r@
identifier f;
@@
f(...) { ... return ERR_PTR(...); }

@@
identifier r.f, fld;
expression x;
statement S1,S2;
@@
 x = f(...)
 ... when != IS_ERR(x)
(
 if (IS_ERR(x) ||...) S1 else S2
|
*x->fld
)
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
 arch/arm/mach-omap2/smartreflex.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index 77ecebf..d7deadf 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -966,7 +966,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 	}
 
 	sr_info = _sr_lookup(pdata->voltdm);
-	if (!sr_info) {
+	if (IS_ERR(sr_info)) {
 		dev_warn(&pdev->dev, "%s: omap_sr struct not found\n",
 			__func__);
 		return -EINVAL;

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Ryan Mallon]

On 01/25/2011 08:55 AM, Julia Lawall wrote:
> Function clk_get, defined just below this code, returns ERR_PTR not NULL in
> an error case.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @r@
> identifier f;
> @@
> f(...) { ... return ERR_PTR(...); }
> 
> @@
> identifier r.f, fld;
> expression x;
> statement S1,S2;
> @@
>  x = f(...)
>  ... when != IS_ERR(x)
> (
>  if (IS_ERR(x) ||...) S1 else S2
> |
> *x->fld
> )
> // </smpl>

I'm always really impressed by this tool :-).

> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
>  arch/arm/mach-at91/clock.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c
> index 9113da6..c9ee6d0 100644
> --- a/arch/arm/mach-at91/clock.c
> +++ b/arch/arm/mach-at91/clock.c
> @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
>  {
>  	struct clk *clk = clk_get(NULL, id);
>  
> -	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
> +	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
>  		return;

I think we want:

	if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
		return;

Since it is valid to return a NULL clk, and we don't want to try and
dereference it if that is the case.

We could also probably drop the !IS_ERR(clk_get(dev, func)) check, since
as far as I can tell it is just checking to see if the clock is already
associated, but there is no harm in re-assigning the same values, and
the two assignments in at91_clock_associate are going to be much quicker
than the lookup in clk_get.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, Ryan Mallon wrote:

> On 01/25/2011 08:55 AM, Julia Lawall wrote:
> > Function clk_get, defined just below this code, returns ERR_PTR not NULL in
> > an error case.
> > 
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
> > 
> > // <smpl>
> > @r@
> > identifier f;
> > @@
> > f(...) { ... return ERR_PTR(...); }
> > 
> > @@
> > identifier r.f, fld;
> > expression x;
> > statement S1,S2;
> > @@
> >  x = f(...)
> >  ... when != IS_ERR(x)
> > (
> >  if (IS_ERR(x) ||...) S1 else S2
> > |
> > *x->fld
> > )
> > // </smpl>
> 
> I'm always really impressed by this tool :-).
> 
> > 
> > Signed-off-by: Julia Lawall <julia@diku.dk>
> > 
> > ---
> >  arch/arm/mach-at91/clock.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/mach-at91/clock.c b/arch/arm/mach-at91/clock.c
> > index 9113da6..c9ee6d0 100644
> > --- a/arch/arm/mach-at91/clock.c
> > +++ b/arch/arm/mach-at91/clock.c
> > @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
> >  {
> >  	struct clk *clk = clk_get(NULL, id);
> >  
> > -	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
> > +	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
> >  		return;
> 
> I think we want:
> 
> 	if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
> 		return;
> 
> Since it is valid to return a NULL clk, and we don't want to try and
> dereference it if that is the case.

Looking at the given defintion of clk_get, I can't see how that could 
happen:

struct clk *clk_get(struct device *dev, const char *id)
{
        struct clk *clk;

        list_for_each_entry(clk, &clocks, node) {
                if (strcmp(id, clk->name) == 0)
                        return clk;
                if (clk->function && (dev == clk->dev) && strcmp(id, clk->function) == 0)
                        return clk;
        }

        return ERR_PTR(-ENOENT);
}

Both paths to the non-ERR_PTR return dereference clk.

julia

> We could also probably drop the !IS_ERR(clk_get(dev, func)) check, since
> as far as I can tell it is just checking to see if the clock is already
> associated, but there is no harm in re-assigning the same values, and
> the two assignments in at91_clock_associate are going to be much quicker
> than the lookup in clk_get.
> 
> ~Ryan
> 
> -- 
> Bluewater Systems Ltd - ARM Technology Solution Centre
> 
> Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
> ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
> http://www.bluewatersys.com	New Zealand
> Phone: +64 3 3779127		Freecall: Australia 1800 148 751
> Fax:   +64 3 3779135			  USA 1800 261 2934
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Vasiliy Kulikov]

On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote:
> On Tue, 25 Jan 2011, Ryan Mallon wrote:
> 
> > On 01/25/2011 08:55 AM, Julia Lawall wrote:
> > > @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
> > >  {
> > >  	struct clk *clk = clk_get(NULL, id);
> > >  
> > > -	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
> > > +	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
> > >  		return;
> > 
> > I think we want:
> > 
> > 	if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
> > 		return;
> > 
> > Since it is valid to return a NULL clk, and we don't want to try and
> > dereference it if that is the case.
> 
> Looking at the given defintion of clk_get, I can't see how that could 
> happen:

clk_get() is defined per-architecture, sometimes it is NULL only.

> struct clk *clk_get(struct device *dev, const char *id)
> {
>         struct clk *clk;
> 
>         list_for_each_entry(clk, &clocks, node) {
>                 if (strcmp(id, clk->name) == 0)
>                         return clk;
>                 if (clk->function && (dev == clk->dev) && strcmp(id, clk->function) == 0)
>                         return clk;
>         }
> 
>         return ERR_PTR(-ENOENT);
> }
> 
> Both paths to the non-ERR_PTR return dereference clk.
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Vasiliy

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Mon, 24 Jan 2011, Vasiliy Kulikov wrote:

> On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote:
> > On Tue, 25 Jan 2011, Ryan Mallon wrote:
> > 
> > > On 01/25/2011 08:55 AM, Julia Lawall wrote:
> > > > @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
> > > >  {
> > > >  	struct clk *clk = clk_get(NULL, id);
> > > >  
> > > > -	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
> > > > +	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
> > > >  		return;
> > > 
> > > I think we want:
> > > 
> > > 	if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
> > > 		return;
> > > 
> > > Since it is valid to return a NULL clk, and we don't want to try and
> > > dereference it if that is the case.
> > 
> > Looking at the given defintion of clk_get, I can't see how that could 
> > happen:
> 
> clk_get() is defined per-architecture, sometimes it is NULL only.

In this case there is a definition in the same file, so it doesn't seem 
necessary to worry about possible other ones.  Unless there is some goal 
in the future to remove the local one?

julia

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Ryan Mallon]

On 01/25/2011 09:05 AM, Vasiliy Kulikov wrote:
> On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote:
>> On Tue, 25 Jan 2011, Ryan Mallon wrote:
>>
>>> On 01/25/2011 08:55 AM, Julia Lawall wrote:
>>>> @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
>>>>  {
>>>>  	struct clk *clk = clk_get(NULL, id);
>>>>  
>>>> -	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
>>>> +	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
>>>>  		return;
>>>
>>> I think we want:
>>>
>>> 	if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
>>> 		return;
>>>
>>> Since it is valid to return a NULL clk, and we don't want to try and
>>> dereference it if that is the case.
>>
>> Looking at the given defintion of clk_get, I can't see how that could 
>> happen:
> 
> clk_get() is defined per-architecture, sometimes it is NULL only.

Julia is correct. Some architectures can return NULL from clk_get, but I
didn't check the at91 before posting :-/. If we can't return NULL from
clk_get then we shouldn't bother checking for it. I do think we should
drop the !IS_ERR(clk_get(dev, func)) check though.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Vasiliy Kulikov]

On Mon, Jan 24, 2011 at 21:09 +0100, Julia Lawall wrote:
> On Mon, 24 Jan 2011, Vasiliy Kulikov wrote:
> > clk_get() is defined per-architecture, sometimes it is NULL only.
> 
> In this case there is a definition in the same file, so it doesn't seem 
> necessary to worry about possible other ones.  Unless there is some goal 
> in the future to remove the local one?

My oppinion is that even such architecture dependent code written in
architecure indepenent way is highly desirable.  Besides more clean code
it is probably review by smaller group of people, so it is potentially
more buggy.


Thanks,

-- 
Vasiliy

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

> Julia is correct. Some architectures can return NULL from clk_get, but I
> didn't check the at91 before posting :-/. If we can't return NULL from
> clk_get then we shouldn't bother checking for it. I do think we should
> drop the !IS_ERR(clk_get(dev, func)) check though.

It seems a bit subtle, because the clk manipulated by clk_get in the call 
of clk_get(dev, func) is not necessarily the same as the one in 
clock_associate.  But perhaps this is the only possibility in practice?

julia

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Ryan Mallon]

On 01/25/2011 09:28 AM, Julia Lawall wrote:
>> Julia is correct. Some architectures can return NULL from clk_get, but I
>> didn't check the at91 before posting :-/. If we can't return NULL from
>> clk_get then we shouldn't bother checking for it. I do think we should
>> drop the !IS_ERR(clk_get(dev, func)) check though.
> 
> It seems a bit subtle, because the clk manipulated by clk_get in the call 
> of clk_get(dev, func) is not necessarily the same as the one in 
> clock_associate.  But perhaps this is the only possibility in practice?

Not sure I follow. The at91 clk_get does not modify the clk. In
at91_clock_associate we have:

	clk->function = func;
	clk->dev = dev;

and in clk_get we have:

	if (clk->function && (dev == clk->dev) &&
		strcmp(id, clk->function) == 0)
            return clk;

So at91_clock_associate sets the function for a clock, and clk_get
returns clocks based on the function association if the name lookup
fails. The only caveat to this is that the the clock function name
(clk->function) is not the same as any others clock's clk->name.

The !IS_ERR(clk_get(dev, func)) check in at91_clock_associate just
appears to check that the clock is not already associated with the given
function. We don't really need this check because we are just making the
same assignment anyway.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 578 bytes --]

On Monday, January 24, 2011 14:55:21 Julia Lawall wrote:
> lcd_device_register may return ERR_PTR, so a check is added for this value
> before the dereference.  All of the other changes reorganize the error
> handling code in this function to avoid duplicating all of it in the added
> case.
> 
> In the original code, in one case, the global variable fb_buffer was set to
> NULL in error code that appears after this variable is initialized.  This
> is done now in all error handling code that has this property.

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, Ryan Mallon wrote:

> On 01/25/2011 09:28 AM, Julia Lawall wrote:
> >> Julia is correct. Some architectures can return NULL from clk_get, but I
> >> didn't check the at91 before posting :-/. If we can't return NULL from
> >> clk_get then we shouldn't bother checking for it. I do think we should
> >> drop the !IS_ERR(clk_get(dev, func)) check though.
> > 
> > It seems a bit subtle, because the clk manipulated by clk_get in the call 
> > of clk_get(dev, func) is not necessarily the same as the one in 
> > clock_associate.  But perhaps this is the only possibility in practice?
> 
> Not sure I follow. The at91 clk_get does not modify the clk. In
> at91_clock_associate we have:
> 
> 	clk->function = func;
> 	clk->dev = dev;
> 
> and in clk_get we have:
> 
> 	if (clk->function && (dev == clk->dev) &&
> 		strcmp(id, clk->function) == 0)
>             return clk;
> 
> So at91_clock_associate sets the function for a clock, and clk_get
> returns clocks based on the function association if the name lookup
> fails. The only caveat to this is that the the clock function name
> (clk->function) is not the same as any others clock's clk->name.

Right, that was what I was worried about.  That one would find the same 
information already present but somewhere else.  But perhaps it can't 
happen, or it doesn't matter if it does?

julia

> The !IS_ERR(clk_get(dev, func)) check in at91_clock_associate just
> appears to check that the clock is not already associated with the given
> function. We don't really need this check because we are just making the
> same assignment anyway.
> 
> ~Ryan
> 
> -- 
> Bluewater Systems Ltd - ARM Technology Solution Centre
> 
> Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
> ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
> http://www.bluewatersys.com	New Zealand
> Phone: +64 3 3779127		Freecall: Australia 1800 148 751
> Fax:   +64 3 3779135			  USA 1800 261 2934
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Ryan Mallon]

On 01/25/2011 10:01 AM, Julia Lawall wrote:
> On Tue, 25 Jan 2011, Ryan Mallon wrote:
> 
>> On 01/25/2011 09:28 AM, Julia Lawall wrote:
>>>> Julia is correct. Some architectures can return NULL from clk_get, but I
>>>> didn't check the at91 before posting :-/. If we can't return NULL from
>>>> clk_get then we shouldn't bother checking for it. I do think we should
>>>> drop the !IS_ERR(clk_get(dev, func)) check though.
>>>
>>> It seems a bit subtle, because the clk manipulated by clk_get in the call 
>>> of clk_get(dev, func) is not necessarily the same as the one in 
>>> clock_associate.  But perhaps this is the only possibility in practice?
>>
>> Not sure I follow. The at91 clk_get does not modify the clk. In
>> at91_clock_associate we have:
>>
>> 	clk->function = func;
>> 	clk->dev = dev;
>>
>> and in clk_get we have:
>>
>> 	if (clk->function && (dev == clk->dev) &&
>> 		strcmp(id, clk->function) == 0)
>>             return clk;
>>
>> So at91_clock_associate sets the function for a clock, and clk_get
>> returns clocks based on the function association if the name lookup
>> fails. The only caveat to this is that the the clock function name
>> (clk->function) is not the same as any others clock's clk->name.
> 
> Right, that was what I was worried about.  That one would find the same 
> information already present but somewhere else.  But perhaps it can't 
> happen, or it doesn't matter if it does?

I think that users are expected to ensure that clock names and clock
function names do not overlap.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test

[Kevin Hilman]

Julia Lawall <julia@diku.dk> writes:

> Function _sr_lookup, defined in the same file, returns ERR_PTR not NULL in
> an error case.

Thanks, will queue this via the OMAP tree for 2.6.38-rc.

Kevin

> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> identifier f;
> @@
> f(...) { ... return ERR_PTR(...); }
>
> @@
> identifier r.f, fld;
> expression x;
> statement S1,S2;
> @@
>  x = f(...)
>  ... when != IS_ERR(x)
> (
>  if (IS_ERR(x) ||...) S1 else S2
> |
> *x->fld
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
>
> ---
>  arch/arm/mach-omap2/smartreflex.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
> index 77ecebf..d7deadf 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -966,7 +966,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
>  	}
>  
>  	sr_info = _sr_lookup(pdata->voltdm);
> -	if (!sr_info) {
> +	if (IS_ERR(sr_info)) {
>  		dev_warn(&pdev->dev, "%s: omap_sr struct not found\n",
>  			__func__);
>  		return -EINVAL;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, Ryan Mallon wrote:

> On 01/25/2011 10:01 AM, Julia Lawall wrote:
> > On Tue, 25 Jan 2011, Ryan Mallon wrote:
> > 
> >> On 01/25/2011 09:28 AM, Julia Lawall wrote:
> >>>> Julia is correct. Some architectures can return NULL from clk_get, but I
> >>>> didn't check the at91 before posting :-/. If we can't return NULL from
> >>>> clk_get then we shouldn't bother checking for it. I do think we should
> >>>> drop the !IS_ERR(clk_get(dev, func)) check though.
> >>>
> >>> It seems a bit subtle, because the clk manipulated by clk_get in the call 
> >>> of clk_get(dev, func) is not necessarily the same as the one in 
> >>> clock_associate.  But perhaps this is the only possibility in practice?
> >>
> >> Not sure I follow. The at91 clk_get does not modify the clk. In
> >> at91_clock_associate we have:
> >>
> >> 	clk->function = func;
> >> 	clk->dev = dev;
> >>
> >> and in clk_get we have:
> >>
> >> 	if (clk->function && (dev == clk->dev) &&
> >> 		strcmp(id, clk->function) == 0)
> >>             return clk;
> >>
> >> So at91_clock_associate sets the function for a clock, and clk_get
> >> returns clocks based on the function association if the name lookup
> >> fails. The only caveat to this is that the the clock function name
> >> (clk->function) is not the same as any others clock's clk->name.
> > 
> > Right, that was what I was worried about.  That one would find the same 
> > information already present but somewhere else.  But perhaps it can't 
> > happen, or it doesn't matter if it does?
> 
> I think that users are expected to ensure that clock names and clock
> function names do not overlap.

One can't have a clock with a different name but the same device and 
function?

julia

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Ryan Mallon]

On 01/25/2011 10:31 AM, Julia Lawall wrote:
> On Tue, 25 Jan 2011, Ryan Mallon wrote:
> 
>> On 01/25/2011 10:01 AM, Julia Lawall wrote:
>>> On Tue, 25 Jan 2011, Ryan Mallon wrote:
>>>
>>>> On 01/25/2011 09:28 AM, Julia Lawall wrote:
>>>>>> Julia is correct. Some architectures can return NULL from clk_get, but I
>>>>>> didn't check the at91 before posting :-/. If we can't return NULL from
>>>>>> clk_get then we shouldn't bother checking for it. I do think we should
>>>>>> drop the !IS_ERR(clk_get(dev, func)) check though.
>>>>>
>>>>> It seems a bit subtle, because the clk manipulated by clk_get in the call 
>>>>> of clk_get(dev, func) is not necessarily the same as the one in 
>>>>> clock_associate.  But perhaps this is the only possibility in practice?
>>>>
>>>> Not sure I follow. The at91 clk_get does not modify the clk. In
>>>> at91_clock_associate we have:
>>>>
>>>> 	clk->function = func;
>>>> 	clk->dev = dev;
>>>>
>>>> and in clk_get we have:
>>>>
>>>> 	if (clk->function && (dev == clk->dev) &&
>>>> 		strcmp(id, clk->function) == 0)
>>>>             return clk;
>>>>
>>>> So at91_clock_associate sets the function for a clock, and clk_get
>>>> returns clocks based on the function association if the name lookup
>>>> fails. The only caveat to this is that the the clock function name
>>>> (clk->function) is not the same as any others clock's clk->name.
>>>
>>> Right, that was what I was worried about.  That one would find the same 
>>> information already present but somewhere else.  But perhaps it can't 
>>> happen, or it doesn't matter if it does?
>>
>> I think that users are expected to ensure that clock names and clock
>> function names do not overlap.
> 
> One can't have a clock with a different name but the same device and 
> function?

You could, but it would not be helpful. Clock associations are used so
that _different_ devices can have the same function and map to the
correct clock. This is used when there are multiple instances of a
single peripheral. For example, the uart clocks work like this:

	at91_clock_associate("usart1_clk", &pdev->dev, "usart");

so then you can do this in a driver:
	
	uart_clk = clk_get(&pdev->dev, "usart");

Rather than:

	uart_clk = clk_get(NULL, "usart1_clk");

The former will find the correct uart clock for the device. Because each
uart is a separate device the correct clock will be selected for each uart.
	
My point was that there should be no overlap between clk->name and
clk->function otherwise clk_get will not be able to return the correct
clock.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 10:51:38AM +1300, Ryan Mallon wrote:
> You could, but it would not be helpful. Clock associations are used so
> that _different_ devices can have the same function and map to the
> correct clock. This is used when there are multiple instances of a
> single peripheral. For example, the uart clocks work like this:
> 
> 	at91_clock_associate("usart1_clk", &pdev->dev, "usart");
> 
> so then you can do this in a driver:
> 	
> 	uart_clk = clk_get(&pdev->dev, "usart");
> 
> Rather than:
> 
> 	uart_clk = clk_get(NULL, "usart1_clk");
> 
> The former will find the correct uart clock for the device. Because each
> uart is a separate device the correct clock will be selected for each uart.
> 	
> My point was that there should be no overlap between clk->name and
> clk->function otherwise clk_get will not be able to return the correct
> clock.

It would be nice if AT91 could switch over to using clkdev at some
point, which greatly helps with associating struct clk's with their
device/function names - and reduces the amount of "different" code.

You can then use clk_add_alias() to create aliases of an existing
clock.

Looking at your clk_get(), I think one way to do this would be, in
clk_register():

	struct clk_lookup *cl;

	/* create function-only entry */
	cl = clkdev_alloc(clk, clk->name, NULL);
	if (!cl)
		return -ENOMEM;
	clkdev_add(cl);

	/* not sure if you need this? */
	if (clk->dev && clk->function) {
		cl = clkdev_alloc(clk, clk->function, "%s", dev_name(clk->dev));
		if (!cl)
			return -ENOMEM;
		clkdev_add(cl);
	}

at91_clock_associate() becomes:

	err = clk_add_alias(func, dev_name(dev), id, NULL);
	return err;

Although maybe in the long run see about removing at91_clock_associate()
entirely as its just a wrapper.

The only requirement there is that 'dev' in each case is a registered
device, otherwise dev_name() won't work.

You can then use the generic clk_get()/clk_put() which clkdev provides.

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:23 Mon 24 Jan     , Russell King - ARM Linux wrote:
> On Tue, Jan 25, 2011 at 10:51:38AM +1300, Ryan Mallon wrote:
> > You could, but it would not be helpful. Clock associations are used so
> > that _different_ devices can have the same function and map to the
> > correct clock. This is used when there are multiple instances of a
> > single peripheral. For example, the uart clocks work like this:
> > 
> > 	at91_clock_associate("usart1_clk", &pdev->dev, "usart");
> > 
> > so then you can do this in a driver:
> > 	
> > 	uart_clk = clk_get(&pdev->dev, "usart");
> > 
> > Rather than:
> > 
> > 	uart_clk = clk_get(NULL, "usart1_clk");
> > 
> > The former will find the correct uart clock for the device. Because each
> > uart is a separate device the correct clock will be selected for each uart.
> > 	
> > My point was that there should be no overlap between clk->name and
> > clk->function otherwise clk_get will not be able to return the correct
> > clock.
> 
> It would be nice if AT91 could switch over to using clkdev at some
> point, which greatly helps with associating struct clk's with their
> device/function names - and reduces the amount of "different" code.
I working on it that's was one of the reason I move clkdev to drivers
as AT91 and AVR32 need to switch

Best Regards,
J.

Re: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test

[Paul Mundt]

On Mon, Jan 24, 2011 at 03:43:17PM -0500, Mike Frysinger wrote:
> On Monday, January 24, 2011 14:55:21 Julia Lawall wrote:
> > lcd_device_register may return ERR_PTR, so a check is added for this value
> > before the dereference.  All of the other changes reorganize the error
> > handling code in this function to avoid duplicating all of it in the added
> > case.
> > 
> > In the original code, in one case, the global variable fb_buffer was set to
> > NULL in error code that appears after this variable is initialized.  This
> > is done now in all error handling code that has this property.
> 
> Acked-by: Mike Frysinger <vapier@gentoo.org>

Applied, thanks.

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, Jean-Christophe PLAGNIOL-VILLARD wrote:

> On 23:23 Mon 24 Jan     , Russell King - ARM Linux wrote:
> > On Tue, Jan 25, 2011 at 10:51:38AM +1300, Ryan Mallon wrote:
> > > You could, but it would not be helpful. Clock associations are used so
> > > that _different_ devices can have the same function and map to the
> > > correct clock. This is used when there are multiple instances of a
> > > single peripheral. For example, the uart clocks work like this:
> > > 
> > > 	at91_clock_associate("usart1_clk", &pdev->dev, "usart");
> > > 
> > > so then you can do this in a driver:
> > > 	
> > > 	uart_clk = clk_get(&pdev->dev, "usart");
> > > 
> > > Rather than:
> > > 
> > > 	uart_clk = clk_get(NULL, "usart1_clk");
> > > 
> > > The former will find the correct uart clock for the device. Because each
> > > uart is a separate device the correct clock will be selected for each uart.
> > > 	
> > > My point was that there should be no overlap between clk->name and
> > > clk->function otherwise clk_get will not be able to return the correct
> > > clock.
> > 
> > It would be nice if AT91 could switch over to using clkdev at some
> > point, which greatly helps with associating struct clk's with their
> > device/function names - and reduces the amount of "different" code.
> I working on it that's was one of the reason I move clkdev to drivers
> as AT91 and AVR32 need to switch

Shall I just leave the patch as is then?

julia

RE: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test

[Hennerich, Michael]

Julia Lawall wrote on 2011-01-24:
> lcd_device_register may return ERR_PTR, so a check is added for this
> value before the dereference.  All of the other changes reorganize the
> error handling code in this function to avoid duplicating all of it in
> the added case.
>
> In the original code, in one case, the global variable fb_buffer was
> set to NULL in error code that appears after this variable is
> initialized.  This is done now in all error handling code that has
> this property.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @r@
> identifier f;
> @@
> f(...) { ... return ERR_PTR(...); }
>
> @@
> identifier r.f, fld;
> expression x;
> statement S1,S2;
> @@
>  x = f(...) ... when != IS_ERR(x) ( if (IS_ERR(x) ||...) S1 else S2
> |
> *x->fld
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>

Looks good to me.

Acked-by: "Hennerich, Michael" <Michael.Hennerich@analog.com>

> ---
>  drivers/video/bf537-lq035.c |   58 +++++++++++++++++++++++++----------
>  --------- 1 file changed, 33 insertions(+), 25 deletions(-)
> diff --git a/drivers/video/bf537-lq035.c b/drivers/video/bf537-lq035.c
> index 18c5078..47c21fb 100644
> --- a/drivers/video/bf537-lq035.c
> +++ b/drivers/video/bf537-lq035.c
> @@ -696,6 +696,7 @@ static int __devinit bfin_lq035_probe(struct
> platform_device *pdev)  {
>       struct backlight_properties props;
>       dma_addr_t dma_handle;
> +     int ret;
>
>       if (request_dma(CH_PPI, KBUILD_MODNAME)) {
>               pr_err("couldn't request PPI DMA\n"); @@ -704,17 +705,16 @@ static
> int __devinit bfin_lq035_probe(struct platform_device *pdev)
>
>       if (request_ports()) {
>               pr_err("couldn't request gpio port\n");
> -             free_dma(CH_PPI);
> -             return -EFAULT;
> +             ret = -EFAULT;
> +             goto out_ports;
>       }
>
>       fb_buffer = dma_alloc_coherent(NULL, TOTAL_VIDEO_MEM_SIZE,
>                                      &dma_handle, GFP_KERNEL);
>       if (fb_buffer == NULL) {
>               pr_err("couldn't allocate dma buffer\n");
> -             free_dma(CH_PPI);
> -             free_ports();
> -             return -ENOMEM;
> +             ret = -ENOMEM;
> +             goto out_dma_coherent;
>       }
>
>       if (L1_DATA_A_LENGTH)
> @@ -725,10 +725,8 @@ static int __devinit bfin_lq035_probe(struct
> platform_device *pdev)
>
>       if (dma_desc_table == NULL) {
>               pr_err("couldn't allocate dma descriptor\n");
> -             free_dma(CH_PPI);
> -             free_ports();
> -             dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer,
> 0);
> -             return -ENOMEM;
> +             ret = -ENOMEM;
> +             goto out_table;
>       }
>
>       bfin_lq035_fb.screen_base = (void *)fb_buffer; @@ -771,31 +769,21 @@
>  static int __devinit bfin_lq035_probe(struct platform_device *pdev)
>       bfin_lq035_fb.pseudo_palette = kzalloc(sizeof(u32) * 16, GFP_KERNEL);
>       if (bfin_lq035_fb.pseudo_palette == NULL) {             pr_err("failed to
>  allocate pseudo_palette\n");
> -             free_dma(CH_PPI);
> -             free_ports();
> -             dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer,
> 0);
> -             return -ENOMEM;
> +             ret = -ENOMEM;
> +             goto out_palette;
>       }
>
>       if (fb_alloc_cmap(&bfin_lq035_fb.cmap, NBR_PALETTE, 0) < 0) {
>               pr_err("failed to allocate colormap (%d entries)\n",
>                       NBR_PALETTE);
> -             free_dma(CH_PPI);
> -             free_ports();
> -             dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer,
> 0);
> -             kfree(bfin_lq035_fb.pseudo_palette);
> -             return -EFAULT;
> +             ret = -EFAULT;
> +             goto out_cmap;
>       }
>
>       if (register_framebuffer(&bfin_lq035_fb) < 0) {
>               pr_err("unable to register framebuffer\n");
> -             free_dma(CH_PPI); -             free_ports(); -         dma_free_coherent(NULL,
> TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0); -                fb_buffer = NULL;
> -             kfree(bfin_lq035_fb.pseudo_palette);
> -             fb_dealloc_cmap(&bfin_lq035_fb.cmap); -         return -EINVAL; +               ret =
> -EINVAL; +            goto out_reg;
>       }
>
>       i2c_add_driver(&ad5280_driver);
> @@ -807,11 +795,31 @@ static int __devinit bfin_lq035_probe(struct
> platform_device *pdev)
>
>       lcd_dev = lcd_device_register(KBUILD_MODNAME, &pdev->dev, NULL,
>                                     &bfin_lcd_ops);
> +     if (IS_ERR(lcd_dev)) {
> +             pr_err("unable to register lcd\n");
> +             ret = PTR_ERR(lcd_dev);
> +             goto out_lcd;
> +     }
>       lcd_dev->props.max_contrast = 255,
>
>       pr_info("initialized");
>
>       return 0;
> +out_lcd:
> +     unregister_framebuffer(&bfin_lq035_fb);
> +out_reg:
> +     fb_dealloc_cmap(&bfin_lq035_fb.cmap);
> +out_cmap:
> +     kfree(bfin_lq035_fb.pseudo_palette);
> +out_palette:
> +out_table:
> +     dma_free_coherent(NULL, TOTAL_VIDEO_MEM_SIZE, fb_buffer, 0);
> +     fb_buffer = NULL;
> +out_dma_coherent:
> +     free_ports();
> +out_ports:
> +     free_dma(CH_PPI);
> +     return ret;
>  }
>
>  static int __devexit bfin_lq035_remove(struct platform_device *pdev)

Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368; Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin, Margaret Seif

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[walter harms]

Am 24.01.2011 21:09, schrieb Julia Lawall:
> On Mon, 24 Jan 2011, Vasiliy Kulikov wrote:
> 
>> On Mon, Jan 24, 2011 at 21:00 +0100, Julia Lawall wrote:
>>> On Tue, 25 Jan 2011, Ryan Mallon wrote:
>>>
>>>> On 01/25/2011 08:55 AM, Julia Lawall wrote:
>>>>> @@ -224,7 +224,7 @@ void __init at91_clock_associate(const char *id, struct device *dev, const char
>>>>>  {
>>>>>  	struct clk *clk = clk_get(NULL, id);
>>>>>  
>>>>> -	if (!dev || !clk || !IS_ERR(clk_get(dev, func)))
>>>>> +	if (!dev || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
>>>>>  		return;
>>>>
>>>> I think we want:
>>>>
>>>> 	if (!dev || !clk || IS_ERR(clk) || !IS_ERR(clk_get(dev, func)))
>>>> 		return;
>>>>
>>>> Since it is valid to return a NULL clk, and we don't want to try and
>>>> dereference it if that is the case.
>>>
>>> Looking at the given defintion of clk_get, I can't see how that could 
>>> happen:
>>
>> clk_get() is defined per-architecture, sometimes it is NULL only.
> 
> In this case there is a definition in the same file, so it doesn't seem 
> necessary to worry about possible other ones.  Unless there is some goal 
> in the future to remove the local one?
> 

Would it be more easy to return NULL in the error case of clk_get() instead
of ERR_PTR(-ENOENT) ?

So the default could be return NULL and an architecture depending solution
replacing that.

re,
 wh

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 11:33:16AM +0100, walter harms wrote:
> Would it be more easy to return NULL in the error case of clk_get() instead
> of ERR_PTR(-ENOENT) ?
> 
> So the default could be return NULL and an architecture depending solution
> replacing that.

That's not how the API is defined.  The API defines error-pointers to be
errors, everything should be considered valid.  Please don't go down the
route of doing something architecturally different from that.

What if, say, you couldn't return the struct clk because maybe it could
only be controlled by one user?  Returning an EBUSY error pointer would
indicate this condition.  What if the module providing the struct clk
hasn't finished initializing - that's another reason for EBUSY rather
than ENOENT.

Error codes are useful to describe why something failed.  NULL pointers
can't do that.

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[walter harms]

Am 25.01.2011 11:43, schrieb Russell King - ARM Linux:
> On Tue, Jan 25, 2011 at 11:33:16AM +0100, walter harms wrote:
>> Would it be more easy to return NULL in the error case of clk_get() instead
>> of ERR_PTR(-ENOENT) ?
>>
>> So the default could be return NULL and an architecture depending solution
>> replacing that.
> 
> That's not how the API is defined.  The API defines error-pointers to be
> errors, everything should be considered valid.  Please don't go down the
> route of doing something architecturally different from that.
> 
> What if, say, you couldn't return the struct clk because maybe it could
> only be controlled by one user?  Returning an EBUSY error pointer would
> indicate this condition.  What if the module providing the struct clk
> hasn't finished initializing - that's another reason for EBUSY rather
> than ENOENT.
> 
> Error codes are useful to describe why something failed.  NULL pointers
> can't do that.
> 

On Mon, 24 Jan 2011, Vasiliy Kulikov wrote:
> 
...
> clk_get() is defined per-architecture, sometimes it is NULL only.
>

So these is a bug ? They should return -ENOENT ?

The interessting question is: what to do with an error ?

Obviously some architecture can live with NULL, so it is not an critical
error. An the patch shows a code that is simply a return, not even the
user is informed that something did not work as expected.

>From that point of view i would like question if it is useful to have
a "detailed" error instead of just returning NULL.

just my 2 cents,
re,
 wh

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 12:12:45PM +0100, walter harms wrote:
> Am 25.01.2011 11:43, schrieb Russell King - ARM Linux:
> > On Tue, Jan 25, 2011 at 11:33:16AM +0100, walter harms wrote:
> >> Would it be more easy to return NULL in the error case of clk_get() instead
> >> of ERR_PTR(-ENOENT) ?
> >>
> >> So the default could be return NULL and an architecture depending solution
> >> replacing that.
> > 
> > That's not how the API is defined.  The API defines error-pointers to be
> > errors, everything should be considered valid.  Please don't go down the
> > route of doing something architecturally different from that.
> > 
> > What if, say, you couldn't return the struct clk because maybe it could
> > only be controlled by one user?  Returning an EBUSY error pointer would
> > indicate this condition.  What if the module providing the struct clk
> > hasn't finished initializing - that's another reason for EBUSY rather
> > than ENOENT.
> > 
> > Error codes are useful to describe why something failed.  NULL pointers
> > can't do that.
> > 
> 
> On Mon, 24 Jan 2011, Vasiliy Kulikov wrote:
> > 
> ...
> > clk_get() is defined per-architecture, sometimes it is NULL only.
> >
> 
> So these is a bug ? They should return -ENOENT ?

You're stuck in "NULL is an error" mode.  struct clk * is a cookie which
only the clk API implementation understands.  It must not be treated as
anything different by drivers.  The only defined values for drivers are
error pointer codes which translate to errors.  Everything else, drivers
must assume is valid.  Drivers are not allowed to dereference struct clk.

It is for the clk API to interpret the struct clk * cookie as it sees fit.

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, walter harms wrote:

> 
> 
> Am 25.01.2011 11:43, schrieb Russell King - ARM Linux:
> > On Tue, Jan 25, 2011 at 11:33:16AM +0100, walter harms wrote:
> >> Would it be more easy to return NULL in the error case of clk_get() instead
> >> of ERR_PTR(-ENOENT) ?
> >>
> >> So the default could be return NULL and an architecture depending solution
> >> replacing that.
> > 
> > That's not how the API is defined.  The API defines error-pointers to be
> > errors, everything should be considered valid.  Please don't go down the
> > route of doing something architecturally different from that.
> > 
> > What if, say, you couldn't return the struct clk because maybe it could
> > only be controlled by one user?  Returning an EBUSY error pointer would
> > indicate this condition.  What if the module providing the struct clk
> > hasn't finished initializing - that's another reason for EBUSY rather
> > than ENOENT.
> > 
> > Error codes are useful to describe why something failed.  NULL pointers
> > can't do that.
> > 
> 
> On Mon, 24 Jan 2011, Vasiliy Kulikov wrote:
> > 
> ...
> > clk_get() is defined per-architecture, sometimes it is NULL only.
> >
> 
> So these is a bug ? They should return -ENOENT ?
> 
> The interessting question is: what to do with an error ?
> 
> Obviously some architecture can live with NULL, so it is not an critical
> error. An the patch shows a code that is simply a return, not even the
> user is informed that something did not work as expected.
> 
> From that point of view i would like question if it is useful to have
> a "detailed" error instead of just returning NULL.

Somewhat unrelatedly, I often run into code where error handling code is 
needed, but not present, and the function returns void, so nothing is 
provided for propagating the error further.  I generally consider these 
cases to be beyond my expertise to fix...

julia

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Russell King - ARM Linux]

On Tue, Jan 25, 2011 at 12:18:40PM +0100, Julia Lawall wrote:
> On Tue, 25 Jan 2011, walter harms wrote:
> > So these is a bug ? They should return -ENOENT ?
> > 
> > The interessting question is: what to do with an error ?
> > 
> > Obviously some architecture can live with NULL, so it is not an critical
> > error. An the patch shows a code that is simply a return, not even the
> > user is informed that something did not work as expected.
> > 
> > From that point of view i would like question if it is useful to have
> > a "detailed" error instead of just returning NULL.
> 
> Somewhat unrelatedly, I often run into code where error handling code is 
> needed, but not present, and the function returns void, so nothing is 
> provided for propagating the error further.  I generally consider these 
> cases to be beyond my expertise to fix...

That is a pain, but so is returning NULL in error conditions.  If you've
got several layers of nesting, and every level returns NULL on error,
it's an awful lot of debugging to find out _why_ a failure happened.

With error codes, it narrows down the number of places which could have
returned that error code, and as error codes can be descriptive, it
turns it into an "oh, I forgot about doing X" or "it's failing *there*"
rather than a puzzle.

The only place where it really makes sense to return NULL is with memory
allocators.  NULL is an accepted value for meaning "I couldn't allocate
memory" as its not a useful pointer value.

The alternative is to have an API like:

	struct clk *clk_get(int *error, ...)
or
	int clk_get(struct clk **, ...)

but that then leads to _additional_ errors made by driver authors and by
implementations - you can no longer guarantee that *error will always be
initialized, and this is why the whole ERR_PTR/PTR_ERR/IS_ERR stuff was
implemented.  The kernel used to have such things in it and they were
buggy.

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Julia Lawall]

On Tue, 25 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 25, 2011 at 12:18:40PM +0100, Julia Lawall wrote:
> > On Tue, 25 Jan 2011, walter harms wrote:
> > > So these is a bug ? They should return -ENOENT ?
> > > 
> > > The interessting question is: what to do with an error ?
> > > 
> > > Obviously some architecture can live with NULL, so it is not an critical
> > > error. An the patch shows a code that is simply a return, not even the
> > > user is informed that something did not work as expected.
> > > 
> > > From that point of view i would like question if it is useful to have
> > > a "detailed" error instead of just returning NULL.
> > 
> > Somewhat unrelatedly, I often run into code where error handling code is 
> > needed, but not present, and the function returns void, so nothing is 
> > provided for propagating the error further.  I generally consider these 
> > cases to be beyond my expertise to fix...
> 
> That is a pain, but so is returning NULL in error conditions.  If you've
> got several layers of nesting, and every level returns NULL on error,
> it's an awful lot of debugging to find out _why_ a failure happened.
> 
> With error codes, it narrows down the number of places which could have
> returned that error code, and as error codes can be descriptive, it
> turns it into an "oh, I forgot about doing X" or "it's failing *there*"
> rather than a puzzle.
> 
> The only place where it really makes sense to return NULL is with memory
> allocators.  NULL is an accepted value for meaning "I couldn't allocate
> memory" as its not a useful pointer value.
> 
> The alternative is to have an API like:
> 
> 	struct clk *clk_get(int *error, ...)
> or
> 	int clk_get(struct clk **, ...)
> 
> but that then leads to _additional_ errors made by driver authors and by
> implementations - you can no longer guarantee that *error will always be
> initialized, and this is why the whole ERR_PTR/PTR_ERR/IS_ERR stuff was
> implemented.  The kernel used to have such things in it and they were
> buggy.

I agree that error codes are very useful.  The problem is rather how to 
propagate any sort of error indicator, whether ERR_PTR, NULL, an negative 
integer, etc.

julia

Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test

[Jean-Christophe PLAGNIOL-VILLARD]

On 07:12 Tue 25 Jan     , Julia Lawall wrote:
> On Tue, 25 Jan 2011, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > On 23:23 Mon 24 Jan     , Russell King - ARM Linux wrote:
> > > On Tue, Jan 25, 2011 at 10:51:38AM +1300, Ryan Mallon wrote:
> > > > You could, but it would not be helpful. Clock associations are used so
> > > > that _different_ devices can have the same function and map to the
> > > > correct clock. This is used when there are multiple instances of a
> > > > single peripheral. For example, the uart clocks work like this:
> > > > 
> > > > 	at91_clock_associate("usart1_clk", &pdev->dev, "usart");
> > > > 
> > > > so then you can do this in a driver:
> > > > 	
> > > > 	uart_clk = clk_get(&pdev->dev, "usart");
> > > > 
> > > > Rather than:
> > > > 
> > > > 	uart_clk = clk_get(NULL, "usart1_clk");
> > > > 
> > > > The former will find the correct uart clock for the device. Because each
> > > > uart is a separate device the correct clock will be selected for each uart.
> > > > 	
> > > > My point was that there should be no overlap between clk->name and
> > > > clk->function otherwise clk_get will not be able to return the correct
> > > > clock.
> > > 
> > > It would be nice if AT91 could switch over to using clkdev at some
> > > point, which greatly helps with associating struct clk's with their
> > > device/function names - and reduces the amount of "different" code.
> > I working on it that's was one of the reason I move clkdev to drivers
> > as AT91 and AVR32 need to switch
> 
> Shall I just leave the patch as is then?
I'll try to send a patch before chinese new year

Best Regards,
J.