| Message ID | 20180207160002.GA9292@embeddedgus (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes: > Add suffix ULL to constant 9 in order to give the compiler complete > information about the proper arithmetic to use. Notice that this > constant is used in a context that expects an expression of type > unsigned long long (64 bits, unsigned). > > The expression tfr->len * 9 * 1000000 is currently being evaluated > using 32-bit arithmetic. > > Addresses-Coverity-ID: 1339619 > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> The effect looks like it would be that we would have chosen polling mode instead of waiting for interrupts for some transfers >477 seconds. Seems like a good fix for an unlikely bug. Reviewed-by: Eric Anholt <eric@anholt.net>
On 7 February 2018 at 16:00, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: > Add suffix ULL to constant 9 in order to give the compiler complete > information about the proper arithmetic to use. Notice that this > constant is used in a context that expects an expression of type > unsigned long long (64 bits, unsigned). > > The expression tfr->len * 9 * 1000000 is currently being evaluated > using 32-bit arithmetic. > > Addresses-Coverity-ID: 1339619 What does this number mean? If it is an index into some internal database, please remove it. > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/spi/spi-bcm2835aux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c > index 7428091..a768c23 100644 > --- a/drivers/spi/spi-bcm2835aux.c > +++ b/drivers/spi/spi-bcm2835aux.c > @@ -363,7 +363,7 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master, > * chunk getting transferred - in our case the chunk size > * is 3 bytes, so we approximate this by 9 bits/byte > */ > - xfer_time_us = tfr->len * 9 * 1000000; > + xfer_time_us = tfr->len * 9ULL * 1000000; > do_div(xfer_time_us, spi_used_hz); > > /* run in polling mode for short transfers */ > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Eric, On 02/08/2018 02:22 AM, Eric Anholt wrote: > "Gustavo A. R. Silva" <gustavo@embeddedor.com> writes: > >> Add suffix ULL to constant 9 in order to give the compiler complete >> information about the proper arithmetic to use. Notice that this >> constant is used in a context that expects an expression of type >> unsigned long long (64 bits, unsigned). >> >> The expression tfr->len * 9 * 1000000 is currently being evaluated >> using 32-bit arithmetic. >> >> Addresses-Coverity-ID: 1339619 >> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > > The effect looks like it would be that we would have chosen polling mode > instead of waiting for interrupts for some transfers >477 seconds. > Seems like a good fix for an unlikely bug. > > Reviewed-by: Eric Anholt <eric@anholt.net> > Thank you for your review. -- Gustavo
Hi Ard, On 02/08/2018 03:54 AM, Ard Biesheuvel wrote: > On 7 February 2018 at 16:00, Gustavo A. R. Silva <gustavo@embeddedor.com> wrote: >> Add suffix ULL to constant 9 in order to give the compiler complete >> information about the proper arithmetic to use. Notice that this >> constant is used in a context that expects an expression of type >> unsigned long long (64 bits, unsigned). >> >> The expression tfr->len * 9 * 1000000 is currently being evaluated >> using 32-bit arithmetic. >> >> Addresses-Coverity-ID: 1339619 > > What does this number mean? If it is an index into some internal > database, please remove it. > This is a unique Coverity identifier. We want to keep information like public Bugzilla IDs and tools like Coverity on the commit message. Thanks -- Gustavo
On 12 February 2018 at 18:04, Gustavo A. R. Silva <garsilva@embeddedor.com> wrote: > Hi Ard, > > On 02/08/2018 03:54 AM, Ard Biesheuvel wrote: >> >> On 7 February 2018 at 16:00, Gustavo A. R. Silva <gustavo@embeddedor.com> >> wrote: >>> >>> Add suffix ULL to constant 9 in order to give the compiler complete >>> information about the proper arithmetic to use. Notice that this >>> constant is used in a context that expects an expression of type >>> unsigned long long (64 bits, unsigned). >>> >>> The expression tfr->len * 9 * 1000000 is currently being evaluated >>> using 32-bit arithmetic. >>> >>> Addresses-Coverity-ID: 1339619 >> >> >> What does this number mean? If it is an index into some internal >> database, please remove it. >> > > This is a unique Coverity identifier. We want to keep information like > public Bugzilla IDs and tools like Coverity on the commit message. > Who is 'we' in this case? And how is this id to any benefit of other people that have been excluded from 'we'? If you add identifiers like this, make sure that they don't only make sense to the in-crowd. For instance, you could replace this with a http link to the database entry if you really must.
On 02/12/2018 10:45 AM, Ard Biesheuvel wrote: > On 12 February 2018 at 18:04, Gustavo A. R. Silva > <garsilva@embeddedor.com> wrote: >> Hi Ard, >> >> On 02/08/2018 03:54 AM, Ard Biesheuvel wrote: >>> >>> On 7 February 2018 at 16:00, Gustavo A. R. Silva <gustavo@embeddedor.com> >>> wrote: >>>> >>>> Add suffix ULL to constant 9 in order to give the compiler complete >>>> information about the proper arithmetic to use. Notice that this >>>> constant is used in a context that expects an expression of type >>>> unsigned long long (64 bits, unsigned). >>>> >>>> The expression tfr->len * 9 * 1000000 is currently being evaluated >>>> using 32-bit arithmetic. >>>> >>>> Addresses-Coverity-ID: 1339619 >>> >>> >>> What does this number mean? If it is an index into some internal >>> database, please remove it. >>> >> >> This is a unique Coverity identifier. We want to keep information like >> public Bugzilla IDs and tools like Coverity on the commit message. >> > > Who is 'we' in this case? And how is this id to any benefit of other > people that have been excluded from 'we'? We is probably the greater Linux community here. > > If you add identifiers like this, make sure that they don't only make > sense to the in-crowd. For instance, you could replace this with a > http link to the database entry if you really must. I don't think it is that easy to extract good URLs from the public Linux coverity instance which is why referring to coverity IDs is being done AFAICT.
On 12 February 2018 at 19:10, Florian Fainelli <f.fainelli@gmail.com> wrote: > On 02/12/2018 10:45 AM, Ard Biesheuvel wrote: >> On 12 February 2018 at 18:04, Gustavo A. R. Silva >> <garsilva@embeddedor.com> wrote: >>> Hi Ard, >>> >>> On 02/08/2018 03:54 AM, Ard Biesheuvel wrote: >>>> >>>> On 7 February 2018 at 16:00, Gustavo A. R. Silva <gustavo@embeddedor.com> >>>> wrote: >>>>> >>>>> Add suffix ULL to constant 9 in order to give the compiler complete >>>>> information about the proper arithmetic to use. Notice that this >>>>> constant is used in a context that expects an expression of type >>>>> unsigned long long (64 bits, unsigned). >>>>> >>>>> The expression tfr->len * 9 * 1000000 is currently being evaluated >>>>> using 32-bit arithmetic. >>>>> >>>>> Addresses-Coverity-ID: 1339619 >>>> >>>> >>>> What does this number mean? If it is an index into some internal >>>> database, please remove it. >>>> >>> >>> This is a unique Coverity identifier. We want to keep information like >>> public Bugzilla IDs and tools like Coverity on the commit message. >>> >> >> Who is 'we' in this case? And how is this id to any benefit of other >> people that have been excluded from 'we'? > > We is probably the greater Linux community here. > >> >> If you add identifiers like this, make sure that they don't only make >> sense to the in-crowd. For instance, you could replace this with a >> http link to the database entry if you really must. > > I don't think it is that easy to extract good URLs from the public Linux > coverity instance which is why referring to coverity IDs is being done > AFAICT. Ah ok, pardon my ignorance then. I wasn't aware there is a public Linux coverity instance. Got a link?
On 02/12/2018 11:11 AM, Ard Biesheuvel wrote: > On 12 February 2018 at 19:10, Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 02/12/2018 10:45 AM, Ard Biesheuvel wrote: >>> On 12 February 2018 at 18:04, Gustavo A. R. Silva >>> <garsilva@embeddedor.com> wrote: >>>> Hi Ard, >>>> >>>> On 02/08/2018 03:54 AM, Ard Biesheuvel wrote: >>>>> >>>>> On 7 February 2018 at 16:00, Gustavo A. R. Silva <gustavo@embeddedor.com> >>>>> wrote: >>>>>> >>>>>> Add suffix ULL to constant 9 in order to give the compiler complete >>>>>> information about the proper arithmetic to use. Notice that this >>>>>> constant is used in a context that expects an expression of type >>>>>> unsigned long long (64 bits, unsigned). >>>>>> >>>>>> The expression tfr->len * 9 * 1000000 is currently being evaluated >>>>>> using 32-bit arithmetic. >>>>>> >>>>>> Addresses-Coverity-ID: 1339619 >>>>> >>>>> >>>>> What does this number mean? If it is an index into some internal >>>>> database, please remove it. >>>>> >>>> >>>> This is a unique Coverity identifier. We want to keep information like >>>> public Bugzilla IDs and tools like Coverity on the commit message. >>>> >>> >>> Who is 'we' in this case? And how is this id to any benefit of other >>> people that have been excluded from 'we'? >> >> We is probably the greater Linux community here. >> >>> >>> If you add identifiers like this, make sure that they don't only make >>> sense to the in-crowd. For instance, you could replace this with a >>> http link to the database entry if you really must. >> >> I don't think it is that easy to extract good URLs from the public Linux >> coverity instance which is why referring to coverity IDs is being done >> AFAICT. > > Ah ok, pardon my ignorance then. I wasn't aware there is a public > Linux coverity instance. Got a link? It requires you to sign up to get notifications and have a dashboard, does this link work for you? https://scan.coverity.com/projects/linux?tab=overview
On Wed, 2018-02-07 at 10:00 -0600, Gustavo A. R. Silva wrote: > Add suffix ULL to constant 9 in order to give the compiler complete > information about the proper arithmetic to use. Notice that this > constant is used in a context that expects an expression of type > unsigned long long (64 bits, unsigned). > > The expression tfr->len * 9 * 1000000 is currently being evaluated > using 32-bit arithmetic. > > Addresses-Coverity-ID: 1339619 > Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> > --- > drivers/spi/spi-bcm2835aux.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c > index 7428091..a768c23 100644 > --- a/drivers/spi/spi-bcm2835aux.c > +++ b/drivers/spi/spi-bcm2835aux.c > @@ -363,7 +363,7 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master, > * chunk getting transferred - in our case the chunk size > * is 3 bytes, so we approximate this by 9 bits/byte > */ > - xfer_time_us = tfr->len * 9 * 1000000; > + xfer_time_us = tfr->len * 9ULL * 1000000; > do_div(xfer_time_us, spi_used_hz); > > /* run in polling mode for short transfers */ if (xfer_time_us < BCM2835_AUX_SPI_POLLING_LIMIT_US) If one looks at the logic as a whole... tfr->len * 9ULL * 100000 / spi_used_hz < 30 A little algebra can change that into: tfr->len < spi_used_hz / 300000 So rather than add more 64-bit math, it can be removed entirely. Both the length and speed are 32 bits and there's no need to multiply them into something larger than might need 64 bits.
diff --git a/drivers/spi/spi-bcm2835aux.c b/drivers/spi/spi-bcm2835aux.c index 7428091..a768c23 100644 --- a/drivers/spi/spi-bcm2835aux.c +++ b/drivers/spi/spi-bcm2835aux.c @@ -363,7 +363,7 @@ static int bcm2835aux_spi_transfer_one(struct spi_master *master, * chunk getting transferred - in our case the chunk size * is 3 bytes, so we approximate this by 9 bits/byte */ - xfer_time_us = tfr->len * 9 * 1000000; + xfer_time_us = tfr->len * 9ULL * 1000000; do_div(xfer_time_us, spi_used_hz); /* run in polling mode for short transfers */
Add suffix ULL to constant 9 in order to give the compiler complete information about the proper arithmetic to use. Notice that this constant is used in a context that expects an expression of type unsigned long long (64 bits, unsigned). The expression tfr->len * 9 * 1000000 is currently being evaluated using 32-bit arithmetic. Addresses-Coverity-ID: 1339619 Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com> --- drivers/spi/spi-bcm2835aux.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)