* [PATCH] coresight: potential uninitialized variable in probe() @ 2019-06-13 6:58 Dan Carpenter 2019-06-13 7:49 ` Leo Yan 2019-06-17 19:27 ` Mathieu Poirier 0 siblings, 2 replies; 6+ messages in thread From: Dan Carpenter @ 2019-06-13 6:58 UTC (permalink / raw) To: Mathieu Poirier, Leo Yan Cc: Alexander Shishkin, kernel-janitors, linux-kernel, linux-arm-kernel, Suzuki K Poulose 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; out_disable_clk: if (ret && !IS_ERR_OR_NULL(drvdata->atclk)) -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: potential uninitialized variable in probe() 2019-06-13 6:58 [PATCH] coresight: potential uninitialized variable in probe() Dan Carpenter @ 2019-06-13 7:49 ` Leo Yan 2019-06-13 8:14 ` Dan Carpenter 2019-06-17 19:27 ` Mathieu Poirier 1 sibling, 1 reply; 6+ messages in thread From: Leo Yan @ 2019-06-13 7:49 UTC (permalink / raw) To: Dan Carpenter Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin, kernel-janitors, linux-kernel, linux-arm-kernel 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: potential uninitialized variable in probe() 2019-06-13 7:49 ` Leo Yan @ 2019-06-13 8:14 ` Dan Carpenter 2019-06-13 9:56 ` Leo Yan 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2019-06-13 8:14 UTC (permalink / raw) To: Leo Yan Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin, kernel-janitors, linux-kernel, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: potential uninitialized variable in probe() 2019-06-13 8:14 ` Dan Carpenter @ 2019-06-13 9:56 ` Leo Yan 2019-06-13 10:10 ` Leo Yan 0 siblings, 1 reply; 6+ messages in thread From: Leo Yan @ 2019-06-13 9:56 UTC (permalink / raw) To: Dan Carpenter Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin, kernel-janitors, linux-kernel, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: potential uninitialized variable in probe() 2019-06-13 9:56 ` Leo Yan @ 2019-06-13 10:10 ` Leo Yan 0 siblings, 0 replies; 6+ messages in thread From: Leo Yan @ 2019-06-13 10:10 UTC (permalink / raw) To: Dan Carpenter Cc: Mathieu Poirier, Suzuki K Poulose, Alexander Shishkin, kernel-janitors, linux-kernel, linux-arm-kernel 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] coresight: potential uninitialized variable in probe() 2019-06-13 6:58 [PATCH] coresight: potential uninitialized variable in probe() Dan Carpenter 2019-06-13 7:49 ` Leo Yan @ 2019-06-17 19:27 ` Mathieu Poirier 1 sibling, 0 replies; 6+ messages in thread From: Mathieu Poirier @ 2019-06-17 19:27 UTC (permalink / raw) To: Dan Carpenter Cc: Suzuki K Poulose, Alexander Shishkin, kernel-janitors, Linux Kernel Mailing List, Leo Yan, linux-arm-kernel 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-17 19:27 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-13 6:58 [PATCH] coresight: potential uninitialized variable in probe() Dan Carpenter 2019-06-13 7:49 ` Leo Yan 2019-06-13 8:14 ` Dan Carpenter 2019-06-13 9:56 ` Leo Yan 2019-06-13 10:10 ` Leo Yan 2019-06-17 19:27 ` Mathieu Poirier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).