| Message ID | 20190613065815.GF16334@mwanda (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 0530ef6b41e80c5cc979e0e50682302161edb6b7 |
| Headers | show |
| Series | coresight: potential uninitialized variable in probe() | expand |
Hi Dan, On Wed, Jun 12, 2019 at 11:58:15PM -0700, Dan Carpenter wrote: > The "drvdata->atclk" clock is optional, but if it gets set to an error > pointer then we're accidentally return an uninitialized variable instead > of success. You are right, thanks a lot for pointing out. I'd like to initialize 'ret = 0' at the head of function, so we can has the same fashion with other CoreSight drivers (e.g. replicator). static int funnel_probe(struct device *dev, struct resource *res) { - int ret; + int ret = 0; If you agree, could you send a new patch for this? Thanks, Leo Yan > Fixes: 78e6427b4e7b ("coresight: funnel: Support static funnel") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/hwtracing/coresight/coresight-funnel.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c > index 5867fcb4503b..fa97cb9ab4f9 100644 > --- a/drivers/hwtracing/coresight/coresight-funnel.c > +++ b/drivers/hwtracing/coresight/coresight-funnel.c > @@ -244,6 +244,7 @@ static int funnel_probe(struct device *dev, struct resource *res) > } > > pm_runtime_put(dev); > + ret = 0; > > out_disable_clk: > if (ret && !IS_ERR_OR_NULL(drvdata->atclk)) > -- > 2.20.1 >
On Thu, Jun 13, 2019 at 03:49:22PM +0800, Leo Yan wrote: > Hi Dan, > > On Wed, Jun 12, 2019 at 11:58:15PM -0700, Dan Carpenter wrote: > > The "drvdata->atclk" clock is optional, but if it gets set to an error > > pointer then we're accidentally return an uninitialized variable instead > > of success. > > You are right, thanks a lot for pointing out. > > I'd like to initialize 'ret = 0' at the head of function, so we can > has the same fashion with other CoreSight drivers (e.g. replicator). > > static int funnel_probe(struct device *dev, struct resource *res) > { > - int ret; > + int ret = 0; > > If you agree, could you send a new patch for this? Obviously that's an option I considered... The reason I didn't go with that is that a common bug that I see is: int ret = 0; p = kmalloc(); if (!p) goto free_whatever; In my experience it's better to initialize the return as late as possible so that you get static checker warnings when you forget to set the error code. Also I think my way is more readable. I like to make the success path as explicit as possible. I hate when people do things like: if (!ret) return ret; About 10% of the time when you see this it is a bug, but it's hard to tell because it's not readable like it would be if people did: if (!ret) return 0; Or sometimes you see things like: if (corner_case) goto free; /* success path */ Without the "/* success path */ comment explaining why we're returning zero most readers will assume it's a mistake. regards, dan carpenter
Hi Dan, On Thu, Jun 13, 2019 at 11:14:19AM +0300, Dan Carpenter wrote: > On Thu, Jun 13, 2019 at 03:49:22PM +0800, Leo Yan wrote: > > Hi Dan, > > > > On Wed, Jun 12, 2019 at 11:58:15PM -0700, Dan Carpenter wrote: > > > The "drvdata->atclk" clock is optional, but if it gets set to an error > > > pointer then we're accidentally return an uninitialized variable instead > > > of success. > > > > You are right, thanks a lot for pointing out. > > > > I'd like to initialize 'ret = 0' at the head of function, so we can > > has the same fashion with other CoreSight drivers (e.g. replicator). > > > > static int funnel_probe(struct device *dev, struct resource *res) > > { > > - int ret; > > + int ret = 0; > > > > If you agree, could you send a new patch for this? > > Obviously that's an option I considered... The reason I didn't go with > that is that a common bug that I see is: > > int ret = 0; > > p = kmalloc(); > if (!p) > goto free_whatever; > > In my experience it's better to initialize the return as late as > possible so that you get static checker warnings when you forget to set > the error code. Just want to check one thing, which static checker you are using? I use sparse but it doesn't report this issue (I learned coccinelle also can do this but it outputs verbose logs). To be honest, I didn't often run static checker when committed patches, but hope later can improve for this. > Also I think my way is more readable. I like to make the success path > as explicit as possible. I hate when people do things like: > > if (!ret) > return ret; > > About 10% of the time when you see this it is a bug, but it's hard to > tell because it's not readable like it would be if people did: > > if (!ret) > return 0; > > Or sometimes you see things like: > > if (corner_case) > goto free; /* success path */ > > Without the "/* success path */ comment explaining why we're returning > zero most readers will assume it's a mistake. Thanks for sharing much knowledge; your change is okay for me. I think the point is the good habit can avoid pitfall and traps :) [1] Thanks, Leo Yan [1] https://www.amazon.com/C-Traps-Pitfalls-Andrew-Koenig/dp/0201179288
On Thu, Jun 13, 2019 at 05:56:37PM +0800, Leo Yan wrote: [...] > > In my experience it's better to initialize the return as late as > > possible so that you get static checker warnings when you forget to set > > the error code. > > Just want to check one thing, which static checker you are using? Okay, I think it's smatch :) Thanks, Leo Yan
On Thu, 13 Jun 2019 at 00:59, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > The "drvdata->atclk" clock is optional, but if it gets set to an error > pointer then we're accidentally return an uninitialized variable instead > of success. > > Fixes: 78e6427b4e7b ("coresight: funnel: Support static funnel") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/hwtracing/coresight/coresight-funnel.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c > index 5867fcb4503b..fa97cb9ab4f9 100644 > --- a/drivers/hwtracing/coresight/coresight-funnel.c > +++ b/drivers/hwtracing/coresight/coresight-funnel.c > @@ -244,6 +244,7 @@ static int funnel_probe(struct device *dev, struct resource *res) > } > > pm_runtime_put(dev); > + ret = 0; Applied - thanks. > > out_disable_clk: > if (ret && !IS_ERR_OR_NULL(drvdata->atclk)) > -- > 2.20.1 >
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c index 5867fcb4503b..fa97cb9ab4f9 100644 --- a/drivers/hwtracing/coresight/coresight-funnel.c +++ b/drivers/hwtracing/coresight/coresight-funnel.c @@ -244,6 +244,7 @@ static int funnel_probe(struct device *dev, struct resource *res) } pm_runtime_put(dev); + ret = 0; out_disable_clk: if (ret && !IS_ERR_OR_NULL(drvdata->atclk))
The "drvdata->atclk" clock is optional, but if it gets set to an error pointer then we're accidentally return an uninitialized variable instead of success. Fixes: 78e6427b4e7b ("coresight: funnel: Support static funnel") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/hwtracing/coresight/coresight-funnel.c | 1 + 1 file changed, 1 insertion(+)