* [PATCH 0/4] Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: open list; +Cc: kernel-janitors These patches perform various adjustments to error handling code related to the possibility of an ERR_PTR value. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 0/4] Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: open list; +Cc: kernel-janitors These patches perform various adjustments to error handling code related to the possibility of an ERR_PTR value. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 1/4] fs/btrfs/inode.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall @ 2011-01-24 19:55 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: Chris Mason; +Cc: kernel-janitors, linux-btrfs, linux-kernel 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); ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 1/4] fs/btrfs/inode.c: Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: Chris Mason; +Cc: kernel-janitors, linux-btrfs, linux-kernel 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); ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall (?) @ 2011-01-24 19:55 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: Russell King Cc: kernel-janitors, Nicolas Ferre, Ryan Mallon, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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; ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall (?) @ 2011-01-24 19:56 ` Ryan Mallon -1 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 19:56 UTC (permalink / raw) To: Julia Lawall Cc: Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 19:56 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 19:56 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 19:56 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 19:56 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 19:56 ` Ryan Mallon (?) @ 2011-01-24 20:00 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:00 UTC (permalink / raw) To: Ryan Mallon Cc: Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:00 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:00 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:00 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:00 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:00 ` Julia Lawall (?) @ 2011-01-24 20:05 ` Vasiliy Kulikov -1 siblings, 0 replies; 90+ messages in thread From: Vasiliy Kulikov @ 2011-01-24 20:05 UTC (permalink / raw) To: Julia Lawall Cc: Ryan Mallon, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:05 ` Vasiliy Kulikov 0 siblings, 0 replies; 90+ messages in thread From: Vasiliy Kulikov @ 2011-01-24 20:05 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:05 ` Vasiliy Kulikov 0 siblings, 0 replies; 90+ messages in thread From: Vasiliy Kulikov @ 2011-01-24 20:05 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:05 ` Vasiliy Kulikov (?) @ 2011-01-24 20:09 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:09 UTC (permalink / raw) To: Vasiliy Kulikov Cc: Ryan Mallon, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:09 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:09 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:09 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:09 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:09 ` Julia Lawall (?) @ 2011-01-24 20:14 ` Vasiliy Kulikov -1 siblings, 0 replies; 90+ messages in thread From: Vasiliy Kulikov @ 2011-01-24 20:14 UTC (permalink / raw) To: Julia Lawall Cc: Ryan Mallon, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:14 ` Vasiliy Kulikov 0 siblings, 0 replies; 90+ messages in thread From: Vasiliy Kulikov @ 2011-01-24 20:14 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:14 ` Vasiliy Kulikov 0 siblings, 0 replies; 90+ messages in thread From: Vasiliy Kulikov @ 2011-01-24 20:14 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:09 ` Julia Lawall (?) @ 2011-01-25 10:33 ` walter harms -1 siblings, 0 replies; 90+ messages in thread From: walter harms @ 2011-01-25 10:33 UTC (permalink / raw) To: Julia Lawall Cc: Vasiliy Kulikov, Ryan Mallon, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 10:33 ` walter harms 0 siblings, 0 replies; 90+ messages in thread From: walter harms @ 2011-01-25 10:33 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 10:33 ` walter harms 0 siblings, 0 replies; 90+ messages in thread From: walter harms @ 2011-01-25 10:33 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 10:33 ` walter harms (?) @ 2011-01-25 10:43 ` Russell King - ARM Linux -1 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 10:43 UTC (permalink / raw) To: walter harms Cc: Julia Lawall, Vasiliy Kulikov, Ryan Mallon, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 10:43 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 10:43 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 10:43 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 10:43 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 10:43 ` Russell King - ARM Linux (?) @ 2011-01-25 11:12 ` walter harms -1 siblings, 0 replies; 90+ messages in thread From: walter harms @ 2011-01-25 11:12 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Julia Lawall, Vasiliy Kulikov, Ryan Mallon, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:12 ` walter harms 0 siblings, 0 replies; 90+ messages in thread From: walter harms @ 2011-01-25 11:12 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:12 ` walter harms 0 siblings, 0 replies; 90+ messages in thread From: walter harms @ 2011-01-25 11:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 11:12 ` walter harms (?) @ 2011-01-25 11:17 ` Russell King - ARM Linux -1 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 11:17 UTC (permalink / raw) To: walter harms Cc: Julia Lawall, Vasiliy Kulikov, Ryan Mallon, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:17 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 11:17 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:17 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 11:17 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 11:12 ` walter harms (?) @ 2011-01-25 11:18 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 11:18 UTC (permalink / raw) To: walter harms Cc: Russell King - ARM Linux, Vasiliy Kulikov, Ryan Mallon, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:18 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 11:18 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:18 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 11:18 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 11:18 ` Julia Lawall (?) @ 2011-01-25 11:26 ` Russell King - ARM Linux -1 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 11:26 UTC (permalink / raw) To: Julia Lawall Cc: walter harms, Vasiliy Kulikov, Ryan Mallon, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:26 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 11:26 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:26 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-25 11:26 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 11:26 ` Russell King - ARM Linux (?) @ 2011-01-25 11:31 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 11:31 UTC (permalink / raw) To: Russell King - ARM Linux Cc: walter harms, Vasiliy Kulikov, Ryan Mallon, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:31 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 11:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 11:31 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 11:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:05 ` Vasiliy Kulikov (?) @ 2011-01-24 20:11 ` Ryan Mallon -1 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 20:11 UTC (permalink / raw) To: Vasiliy Kulikov Cc: Julia Lawall, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:11 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 20:11 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:11 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 20:11 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:11 ` Ryan Mallon (?) @ 2011-01-24 20:28 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:28 UTC (permalink / raw) To: Ryan Mallon Cc: Vasiliy Kulikov, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel > 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:28 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:28 UTC (permalink / raw) To: linux-arm-kernel > 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:28 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 20:28 UTC (permalink / raw) To: linux-arm-kernel > 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:28 ` Julia Lawall (?) @ 2011-01-24 20:38 ` Ryan Mallon -1 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 20:38 UTC (permalink / raw) To: Julia Lawall Cc: Vasiliy Kulikov, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:38 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 20:38 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 20:38 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 20:38 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 20:38 ` Ryan Mallon (?) @ 2011-01-24 21:01 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 21:01 UTC (permalink / raw) To: Ryan Mallon Cc: Vasiliy Kulikov, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 > ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:01 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 21:01 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:01 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 21:01 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 21:01 ` Julia Lawall (?) @ 2011-01-24 21:06 ` Ryan Mallon -1 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 21:06 UTC (permalink / raw) To: Julia Lawall Cc: Vasiliy Kulikov, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:06 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 21:06 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:06 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 21:06 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 21:06 ` Ryan Mallon (?) @ 2011-01-24 21:31 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 21:31 UTC (permalink / raw) To: Ryan Mallon Cc: Vasiliy Kulikov, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:31 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 21:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:31 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 21:31 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 21:31 ` Julia Lawall (?) @ 2011-01-24 21:51 ` Ryan Mallon -1 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 21:51 UTC (permalink / raw) To: Julia Lawall Cc: Vasiliy Kulikov, Russell King, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:51 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 21:51 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 21:51 ` Ryan Mallon 0 siblings, 0 replies; 90+ messages in thread From: Ryan Mallon @ 2011-01-24 21:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 21:51 ` Ryan Mallon (?) @ 2011-01-24 23:23 ` Russell King - ARM Linux -1 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-24 23:23 UTC (permalink / raw) To: Ryan Mallon Cc: Julia Lawall, Vasiliy Kulikov, kernel-janitors, Nicolas Ferre, Jean-Christophe PLAGNIOL-VILLARD, Andrew Victor, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 23:23 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-24 23:23 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-24 23:23 ` Russell King - ARM Linux 0 siblings, 0 replies; 90+ messages in thread From: Russell King - ARM Linux @ 2011-01-24 23:23 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-24 23:23 ` Russell King - ARM Linux (?) @ 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD -1 siblings, 0 replies; 90+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-01-25 1:44 UTC (permalink / raw) To: Russell King - ARM Linux Cc: Ryan Mallon, Julia Lawall, Vasiliy Kulikov, kernel-janitors, Nicolas Ferre, Andrew Victor, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 90+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-01-25 1:44 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 90+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-01-25 1:44 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD (?) @ 2011-01-25 6:12 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 6:12 UTC (permalink / raw) To: Jean-Christophe PLAGNIOL-VILLARD Cc: Russell King - ARM Linux, Ryan Mallon, Vasiliy Kulikov, kernel-janitors, Nicolas Ferre, Andrew Victor, linux-arm-kernel, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 6:12 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 6:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 6:12 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-25 6:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test 2011-01-25 6:12 ` Julia Lawall (?) @ 2011-01-25 17:23 ` Jean-Christophe PLAGNIOL-VILLARD -1 siblings, 0 replies; 90+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-01-25 17:23 UTC (permalink / raw) To: Julia Lawall Cc: Russell King - ARM Linux, Ryan Mallon, Vasiliy Kulikov, kernel-janitors, Nicolas Ferre, Andrew Victor, linux-arm-kernel, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 17:23 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 90+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-01-25 17:23 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 2/4] arch/arm/mach-at91/clock.c: Add missing IS_ERR test @ 2011-01-25 17:23 ` Jean-Christophe PLAGNIOL-VILLARD 0 siblings, 0 replies; 90+ messages in thread From: Jean-Christophe PLAGNIOL-VILLARD @ 2011-01-25 17:23 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall @ 2011-01-24 19:55 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: Paul Mundt Cc: kernel-janitors, Mike Frysinger, Michael Hennerich, Bryan Wu, linux-fbdev, linux-kernel 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) ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: Paul Mundt Cc: kernel-janitors, Mike Frysinger, Michael Hennerich, Bryan Wu, linux-fbdev, linux-kernel 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) ^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall @ 2011-01-24 20:43 ` Mike Frysinger -1 siblings, 0 replies; 90+ messages in thread From: Mike Frysinger @ 2011-01-24 20:43 UTC (permalink / raw) To: Julia Lawall Cc: Paul Mundt, kernel-janitors, Michael Hennerich, Bryan Wu, linux-fbdev, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test @ 2011-01-24 20:43 ` Mike Frysinger 0 siblings, 0 replies; 90+ messages in thread From: Mike Frysinger @ 2011-01-24 20:43 UTC (permalink / raw) To: Julia Lawall Cc: Paul Mundt, kernel-janitors, Michael Hennerich, Bryan Wu, linux-fbdev, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test 2011-01-24 20:43 ` Mike Frysinger @ 2011-01-25 6:12 ` Paul Mundt -1 siblings, 0 replies; 90+ messages in thread From: Paul Mundt @ 2011-01-25 6:12 UTC (permalink / raw) To: Mike Frysinger Cc: Julia Lawall, kernel-janitors, Michael Hennerich, Bryan Wu, linux-fbdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test @ 2011-01-25 6:12 ` Paul Mundt 0 siblings, 0 replies; 90+ messages in thread From: Paul Mundt @ 2011-01-25 6:12 UTC (permalink / raw) To: Mike Frysinger Cc: Julia Lawall, kernel-janitors, Michael Hennerich, Bryan Wu, linux-fbdev, linux-kernel 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. ^ permalink raw reply [flat|nested] 90+ messages in thread
* RE: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall @ 2011-01-25 8:36 ` Hennerich, Michael -1 siblings, 0 replies; 90+ messages in thread From: Hennerich, Michael @ 2011-01-25 8:36 UTC (permalink / raw) To: Julia Lawall, Paul Mundt Cc: kernel-janitors, Mike Frysinger, Bryan Wu, linux-fbdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* RE: [PATCH 3/4] drivers/video/bf537-lq035.c: Add missing IS_ERR test @ 2011-01-25 8:36 ` Hennerich, Michael 0 siblings, 0 replies; 90+ messages in thread From: Hennerich, Michael @ 2011-01-25 8:36 UTC (permalink / raw) To: Julia Lawall, Paul Mundt Cc: kernel-janitors, Mike Frysinger, Bryan Wu, linux-fbdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall (?) @ 2011-01-24 19:55 ` Julia Lawall -1 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: Tony Lindgren Cc: kernel-janitors, Russell King, linux-omap, linux-arm-kernel, linux-kernel 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; ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 90+ messages in thread
* [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test @ 2011-01-24 19:55 ` Julia Lawall 0 siblings, 0 replies; 90+ messages in thread From: Julia Lawall @ 2011-01-24 19:55 UTC (permalink / raw) To: linux-arm-kernel 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; ^ permalink raw reply related [flat|nested] 90+ messages in thread
* Re: [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test 2011-01-24 19:55 ` Julia Lawall (?) @ 2011-01-24 21:24 ` Kevin Hilman -1 siblings, 0 replies; 90+ messages in thread From: Kevin Hilman @ 2011-01-24 21:24 UTC (permalink / raw) To: Julia Lawall Cc: Tony Lindgren, kernel-janitors, Russell King, linux-omap, linux-arm-kernel, linux-kernel 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/ ^ permalink raw reply [flat|nested] 90+ messages in thread
* [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test @ 2011-01-24 21:24 ` Kevin Hilman 0 siblings, 0 replies; 90+ messages in thread From: Kevin Hilman @ 2011-01-24 21:24 UTC (permalink / raw) To: linux-arm-kernel 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 90+ messages in thread
* Re: [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: Add missing IS_ERR test @ 2011-01-24 21:24 ` Kevin Hilman 0 siblings, 0 replies; 90+ messages in thread From: Kevin Hilman @ 2011-01-24 21:24 UTC (permalink / raw) To: linux-arm-kernel 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/ ^ permalink raw reply [flat|nested] 90+ messages in thread
end of thread, other threads:[~2011-01-25 17:25 UTC | newest] Thread overview: 90+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-24 19:55 [PATCH 0/4] Add missing IS_ERR test Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 19:55 ` [PATCH 1/4] fs/btrfs/inode.c: " Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 19:55 ` [PATCH 2/4] arch/arm/mach-at91/clock.c: " Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 19:56 ` Ryan Mallon 2011-01-24 19:56 ` Ryan Mallon 2011-01-24 19:56 ` Ryan Mallon 2011-01-24 20:00 ` Julia Lawall 2011-01-24 20:00 ` Julia Lawall 2011-01-24 20:00 ` Julia Lawall 2011-01-24 20:05 ` Vasiliy Kulikov 2011-01-24 20:05 ` Vasiliy Kulikov 2011-01-24 20:05 ` Vasiliy Kulikov 2011-01-24 20:09 ` Julia Lawall 2011-01-24 20:09 ` Julia Lawall 2011-01-24 20:09 ` Julia Lawall 2011-01-24 20:14 ` Vasiliy Kulikov 2011-01-24 20:14 ` Vasiliy Kulikov 2011-01-24 20:14 ` Vasiliy Kulikov 2011-01-25 10:33 ` walter harms 2011-01-25 10:33 ` walter harms 2011-01-25 10:33 ` walter harms 2011-01-25 10:43 ` Russell King - ARM Linux 2011-01-25 10:43 ` Russell King - ARM Linux 2011-01-25 10:43 ` Russell King - ARM Linux 2011-01-25 11:12 ` walter harms 2011-01-25 11:12 ` walter harms 2011-01-25 11:12 ` walter harms 2011-01-25 11:17 ` Russell King - ARM Linux 2011-01-25 11:17 ` Russell King - ARM Linux 2011-01-25 11:17 ` Russell King - ARM Linux 2011-01-25 11:18 ` Julia Lawall 2011-01-25 11:18 ` Julia Lawall 2011-01-25 11:18 ` Julia Lawall 2011-01-25 11:26 ` Russell King - ARM Linux 2011-01-25 11:26 ` Russell King - ARM Linux 2011-01-25 11:26 ` Russell King - ARM Linux 2011-01-25 11:31 ` Julia Lawall 2011-01-25 11:31 ` Julia Lawall 2011-01-25 11:31 ` Julia Lawall 2011-01-24 20:11 ` Ryan Mallon 2011-01-24 20:11 ` Ryan Mallon 2011-01-24 20:11 ` Ryan Mallon 2011-01-24 20:28 ` Julia Lawall 2011-01-24 20:28 ` Julia Lawall 2011-01-24 20:28 ` Julia Lawall 2011-01-24 20:38 ` Ryan Mallon 2011-01-24 20:38 ` Ryan Mallon 2011-01-24 20:38 ` Ryan Mallon 2011-01-24 21:01 ` Julia Lawall 2011-01-24 21:01 ` Julia Lawall 2011-01-24 21:01 ` Julia Lawall 2011-01-24 21:06 ` Ryan Mallon 2011-01-24 21:06 ` Ryan Mallon 2011-01-24 21:06 ` Ryan Mallon 2011-01-24 21:31 ` Julia Lawall 2011-01-24 21:31 ` Julia Lawall 2011-01-24 21:31 ` Julia Lawall 2011-01-24 21:51 ` Ryan Mallon 2011-01-24 21:51 ` Ryan Mallon 2011-01-24 21:51 ` Ryan Mallon 2011-01-24 23:23 ` Russell King - ARM Linux 2011-01-24 23:23 ` Russell King - ARM Linux 2011-01-24 23:23 ` Russell King - ARM Linux 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD 2011-01-25 1:44 ` Jean-Christophe PLAGNIOL-VILLARD 2011-01-25 6:12 ` Julia Lawall 2011-01-25 6:12 ` Julia Lawall 2011-01-25 6:12 ` Julia Lawall 2011-01-25 17:23 ` Jean-Christophe PLAGNIOL-VILLARD 2011-01-25 17:23 ` Jean-Christophe PLAGNIOL-VILLARD 2011-01-25 17:23 ` Jean-Christophe PLAGNIOL-VILLARD 2011-01-24 19:55 ` [PATCH 3/4] drivers/video/bf537-lq035.c: " Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 20:43 ` Mike Frysinger 2011-01-24 20:43 ` Mike Frysinger 2011-01-25 6:12 ` Paul Mundt 2011-01-25 6:12 ` Paul Mundt 2011-01-25 8:36 ` Hennerich, Michael 2011-01-25 8:36 ` Hennerich, Michael 2011-01-24 19:55 ` [PATCH 4/4] arch/arm/mach-omap2/smartreflex.c: " Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 19:55 ` Julia Lawall 2011-01-24 21:24 ` Kevin Hilman 2011-01-24 21:24 ` Kevin Hilman 2011-01-24 21:24 ` Kevin Hilman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.