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 <[email protected]>
---
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
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
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
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Thursday, September 28, 2017 3:49 PM
> To: Ruxandra Ioana Radulescu <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Roy Pledge <[email protected]>;
> [email protected]; [email protected]; Bogdan Purcareata
> <[email protected]>; Laurentiu Tudor
> <[email protected]>
> 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
On Thu, Sep 28, 2017 at 01:07:48PM +0000, Ruxandra Ioana Radulescu wrote:
> > -----Original Message-----
> > From: Dan Carpenter [mailto:[email protected]]
> > Sent: Thursday, September 28, 2017 3:49 PM
> > To: Ruxandra Ioana Radulescu <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; Roy Pledge <[email protected]>;
> > [email protected]; [email protected]; Bogdan Purcareata
> > <[email protected]>; Laurentiu Tudor
> > <[email protected]>
> > 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
> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Thursday, September 28, 2017 4:18 PM
> To: Ruxandra Ioana Radulescu <[email protected]>
> Cc: Dan Carpenter <[email protected]>;
> [email protected]; [email protected]; [email protected]; Roy
> Pledge <[email protected]>; [email protected];
> [email protected]; Bogdan Purcareata <[email protected]>;
> Laurentiu Tudor <[email protected]>
> 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:[email protected]]
> > > Sent: Thursday, September 28, 2017 3:49 PM
> > > To: Ruxandra Ioana Radulescu <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]; Roy Pledge
> <[email protected]>;
> > > [email protected]; [email protected]; Bogdan Purcareata
> > > <[email protected]>; Laurentiu Tudor
> > > <[email protected]>
> > > 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