* [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
@ 2017-09-27 17:57 Ioana Radulescu
2017-09-28 12:48 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Ioana Radulescu @ 2017-09-27 17:57 UTC (permalink / raw)
To: gregkh
Cc: devel, linux-kernel, agraf, arnd, bogdan.purcareata, stuyoder,
laurentiu.tudor, ruxandra.radulescu, roy.pledge
For some dpio functions, a negative cpu id parameter value is
valid and means "any". But when trying to validate this param
value against an upper limit, in this case num_possible_cpus(),
we risk obtaining the wrong result due to an implicit cast
to unsigned.
Avoid an incorrect check result when cpu id is negative, by
explicitly stating the comparison is between signed values.
Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com>
---
drivers/staging/fsl-mc/bus/dpio/dpio-service.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
index f809682..26922fc 100644
--- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
+++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
@@ -76,7 +76,7 @@ static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,
if (d)
return d;
- if (unlikely(cpu >= num_possible_cpus()))
+ if (unlikely(cpu >= (int)num_possible_cpus()))
return NULL;
/*
@@ -121,7 +121,7 @@ struct dpaa2_io *dpaa2_io_create(const struct dpaa2_io_desc *desc)
return NULL;
/* check if CPU is out of range (-1 means any cpu) */
- if (desc->cpu >= num_possible_cpus()) {
+ if (desc->cpu >= (int)num_possible_cpus()) {
kfree(obj);
return NULL;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
2017-09-27 17:57 [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison Ioana Radulescu
@ 2017-09-28 12:48 ` Dan Carpenter
2017-09-28 12:53 ` Dan Carpenter
2017-09-28 13:07 ` Ruxandra Ioana Radulescu
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-09-28 12:48 UTC (permalink / raw)
To: Ioana Radulescu
Cc: gregkh, devel, arnd, stuyoder, roy.pledge, linux-kernel, agraf,
bogdan.purcareata, laurentiu.tudor
On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> index f809682..26922fc 100644
> --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> @@ -76,7 +76,7 @@ static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,
> if (d)
> return d;
>
> - if (unlikely(cpu >= num_possible_cpus()))
> + if (unlikely(cpu >= (int)num_possible_cpus()))
Drivers shouldn't use likely/unlikley. Please write it more explicitly
like this:
if (cpu != -1 && cpu >= num_possible_cpus())
return NULL;
Same for the other one as well.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
2017-09-28 12:48 ` Dan Carpenter
@ 2017-09-28 12:53 ` Dan Carpenter
2017-09-28 13:07 ` Ruxandra Ioana Radulescu
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-09-28 12:53 UTC (permalink / raw)
To: Ioana Radulescu
Cc: devel, arnd, stuyoder, gregkh, roy.pledge, linux-kernel, agraf,
bogdan.purcareata, laurentiu.tudor
On Thu, Sep 28, 2017 at 03:48:36PM +0300, Dan Carpenter wrote:
> On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > index f809682..26922fc 100644
> > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > @@ -76,7 +76,7 @@ static inline struct dpaa2_io *service_select_by_cpu(struct dpaa2_io *d,
> > if (d)
> > return d;
> >
> > - if (unlikely(cpu >= num_possible_cpus()))
> > + if (unlikely(cpu >= (int)num_possible_cpus()))
>
>
> Drivers shouldn't use likely/unlikley. Please write it more explicitly
> like this:
>
> if (cpu != -1 && cpu >= num_possible_cpus())
This would probably be more readable as a define.
if (cpu != DPAA_ANY_CPU && cpu >= num_possible_cpus())
return NULL;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
2017-09-28 12:48 ` Dan Carpenter
2017-09-28 12:53 ` Dan Carpenter
@ 2017-09-28 13:07 ` Ruxandra Ioana Radulescu
2017-09-28 13:17 ` gregkh
1 sibling, 1 reply; 6+ messages in thread
From: Ruxandra Ioana Radulescu @ 2017-09-28 13:07 UTC (permalink / raw)
To: Dan Carpenter
Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org,
arnd@arndb.de, stuyoder@gmail.com, Roy Pledge,
linux-kernel@vger.kernel.org, agraf@suse.de, Bogdan Purcareata,
Laurentiu Tudor
> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, September 28, 2017 3:49 PM
> To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org;
> arnd@arndb.de; stuyoder@gmail.com; Roy Pledge <roy.pledge@nxp.com>;
> linux-kernel@vger.kernel.org; agraf@suse.de; Bogdan Purcareata
> <bogdan.purcareata@nxp.com>; Laurentiu Tudor
> <laurentiu.tudor@nxp.com>
> Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
>
> On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > index f809682..26922fc 100644
> > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > @@ -76,7 +76,7 @@ static inline struct dpaa2_io
> *service_select_by_cpu(struct dpaa2_io *d,
> > if (d)
> > return d;
> >
> > - if (unlikely(cpu >= num_possible_cpus()))
> > + if (unlikely(cpu >= (int)num_possible_cpus()))
>
>
> Drivers shouldn't use likely/unlikley.
I was under the impression it's ok to use them on hotpath
(and while not entirely obvious, this function is called on
other drivers' hotpath).
> Please write it more explicitly like this:
>
> if (cpu != -1 && cpu >= num_possible_cpus())
> return NULL;
>
> Same for the other one as well.
Will rewrite as you suggested in the second email and send a v2.
Thanks,
Ioana
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
2017-09-28 13:07 ` Ruxandra Ioana Radulescu
@ 2017-09-28 13:17 ` gregkh
2017-09-28 13:23 ` Ruxandra Ioana Radulescu
0 siblings, 1 reply; 6+ messages in thread
From: gregkh @ 2017-09-28 13:17 UTC (permalink / raw)
To: Ruxandra Ioana Radulescu
Cc: Dan Carpenter, devel@driverdev.osuosl.org, arnd@arndb.de,
stuyoder@gmail.com, Roy Pledge, linux-kernel@vger.kernel.org,
agraf@suse.de, Bogdan Purcareata, Laurentiu Tudor
On Thu, Sep 28, 2017 at 01:07:48PM +0000, Ruxandra Ioana Radulescu wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > Sent: Thursday, September 28, 2017 3:49 PM
> > To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> > Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org;
> > arnd@arndb.de; stuyoder@gmail.com; Roy Pledge <roy.pledge@nxp.com>;
> > linux-kernel@vger.kernel.org; agraf@suse.de; Bogdan Purcareata
> > <bogdan.purcareata@nxp.com>; Laurentiu Tudor
> > <laurentiu.tudor@nxp.com>
> > Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
> >
> > On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > index f809682..26922fc 100644
> > > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > @@ -76,7 +76,7 @@ static inline struct dpaa2_io
> > *service_select_by_cpu(struct dpaa2_io *d,
> > > if (d)
> > > return d;
> > >
> > > - if (unlikely(cpu >= num_possible_cpus()))
> > > + if (unlikely(cpu >= (int)num_possible_cpus()))
> >
> >
> > Drivers shouldn't use likely/unlikley.
>
> I was under the impression it's ok to use them on hotpath
> (and while not entirely obvious, this function is called on
> other drivers' hotpath).
Only use it if you can measure the difference. If you can not, then do
not use it as the compiler and the CPU will guess it better than you
will.
This has been proven many times, something like 80% of our
likely/unlikely usage in the kernel is wrong because of this, see the
work from Andi Kleen many years ago in this area.
So please remove it. Unless you can prove it matters, and if so,
document that.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
2017-09-28 13:17 ` gregkh
@ 2017-09-28 13:23 ` Ruxandra Ioana Radulescu
0 siblings, 0 replies; 6+ messages in thread
From: Ruxandra Ioana Radulescu @ 2017-09-28 13:23 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: Dan Carpenter, devel@driverdev.osuosl.org, arnd@arndb.de,
stuyoder@gmail.com, Roy Pledge, linux-kernel@vger.kernel.org,
agraf@suse.de, Bogdan Purcareata, Laurentiu Tudor
> -----Original Message-----
> From: gregkh@linuxfoundation.org [mailto:gregkh@linuxfoundation.org]
> Sent: Thursday, September 28, 2017 4:18 PM
> To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> Cc: Dan Carpenter <dan.carpenter@oracle.com>;
> devel@driverdev.osuosl.org; arnd@arndb.de; stuyoder@gmail.com; Roy
> Pledge <roy.pledge@nxp.com>; linux-kernel@vger.kernel.org;
> agraf@suse.de; Bogdan Purcareata <bogdan.purcareata@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>
> Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
>
> On Thu, Sep 28, 2017 at 01:07:48PM +0000, Ruxandra Ioana Radulescu wrote:
> > > -----Original Message-----
> > > From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> > > Sent: Thursday, September 28, 2017 3:49 PM
> > > To: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>
> > > Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org;
> > > arnd@arndb.de; stuyoder@gmail.com; Roy Pledge
> <roy.pledge@nxp.com>;
> > > linux-kernel@vger.kernel.org; agraf@suse.de; Bogdan Purcareata
> > > <bogdan.purcareata@nxp.com>; Laurentiu Tudor
> > > <laurentiu.tudor@nxp.com>
> > > Subject: Re: [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison
> > >
> > > On Wed, Sep 27, 2017 at 12:57:28PM -0500, Ioana Radulescu wrote:
> > > > diff --git a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > > index f809682..26922fc 100644
> > > > --- a/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > > +++ b/drivers/staging/fsl-mc/bus/dpio/dpio-service.c
> > > > @@ -76,7 +76,7 @@ static inline struct dpaa2_io
> > > *service_select_by_cpu(struct dpaa2_io *d,
> > > > if (d)
> > > > return d;
> > > >
> > > > - if (unlikely(cpu >= num_possible_cpus()))
> > > > + if (unlikely(cpu >= (int)num_possible_cpus()))
> > >
> > >
> > > Drivers shouldn't use likely/unlikley.
> >
> > I was under the impression it's ok to use them on hotpath
> > (and while not entirely obvious, this function is called on
> > other drivers' hotpath).
>
> Only use it if you can measure the difference. If you can not, then do
> not use it as the compiler and the CPU will guess it better than you
> will.
>
> This has been proven many times, something like 80% of our
> likely/unlikely usage in the kernel is wrong because of this, see the
> work from Andi Kleen many years ago in this area.
>
> So please remove it. Unless you can prove it matters, and if so,
> document that.
Greg, thanks for the explanation. Will remove it in v2.
Ioana
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-28 13:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-27 17:57 [PATCH] staging: fsl-mc/dpio: Fix incorrect comparison Ioana Radulescu
2017-09-28 12:48 ` Dan Carpenter
2017-09-28 12:53 ` Dan Carpenter
2017-09-28 13:07 ` Ruxandra Ioana Radulescu
2017-09-28 13:17 ` gregkh
2017-09-28 13:23 ` Ruxandra Ioana Radulescu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox