| Message ID | 1441032373-16992-5-git-send-email-maxime.ripard@free-electrons.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
On Mon, Aug 31, 2015 at 04:46:04PM +0200, Maxime Ripard wrote: > To check the alignment of the image blocks to the storage blocks, the > current code uses a convoluted syntax, while a simple mod also does the > work. > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > common/aboot.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/common/aboot.c b/common/aboot.c > index 65e633acfcb9..c8556d9b23f4 100644 > --- a/common/aboot.c > +++ b/common/aboot.c > @@ -269,8 +269,7 @@ void write_sparse_image(block_dev_desc_t *dev_desc, > } > > /* verify sparse_header->blk_sz is an exact multiple of info->blksz */ > - if (sparse_header->blk_sz != > - (sparse_header->blk_sz & ~(info->blksz - 1))) { > + if (sparse_header->blk_sz % info->blksz) { So, sometimes we have convoluted syntax like this to avoid what ends up as floating point math on 32bit platforms. Maybe this needs to be one of the helpers in include/linux/math64.h ? Or is this really just convoluted syntax? Thanks!
On Fri, Sep 04, 2015 at 01:20:38PM -0400, Tom Rini wrote: > On Mon, Aug 31, 2015 at 04:46:04PM +0200, Maxime Ripard wrote: > > > To check the alignment of the image blocks to the storage blocks, the > > current code uses a convoluted syntax, while a simple mod also does the > > work. > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > common/aboot.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/common/aboot.c b/common/aboot.c > > index 65e633acfcb9..c8556d9b23f4 100644 > > --- a/common/aboot.c > > +++ b/common/aboot.c > > @@ -269,8 +269,7 @@ void write_sparse_image(block_dev_desc_t *dev_desc, > > } > > > > /* verify sparse_header->blk_sz is an exact multiple of info->blksz */ > > - if (sparse_header->blk_sz != > > - (sparse_header->blk_sz & ~(info->blksz - 1))) { > > + if (sparse_header->blk_sz % info->blksz) { > > So, sometimes we have convoluted syntax like this to avoid what ends up > as floating point math on 32bit platforms. Now that you speak of this, we did have some compilers on some platforms that ended up generating floating point related failures on our branch, but I didn't have the time to look into it. However, I don't really know how we can end up with floating point math with a mod, is this some optimisations done by gcc? > Maybe this needs to be one of the helpers in include/linux/math64.h? Probably then, yes. Thanks! Maxime
On Sun, Sep 06, 2015 at 01:27:20PM +0200, Maxime Ripard wrote: > On Fri, Sep 04, 2015 at 01:20:38PM -0400, Tom Rini wrote: > > On Mon, Aug 31, 2015 at 04:46:04PM +0200, Maxime Ripard wrote: > > > > > To check the alignment of the image blocks to the storage blocks, the > > > current code uses a convoluted syntax, while a simple mod also does the > > > work. > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > --- > > > common/aboot.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/common/aboot.c b/common/aboot.c > > > index 65e633acfcb9..c8556d9b23f4 100644 > > > --- a/common/aboot.c > > > +++ b/common/aboot.c > > > @@ -269,8 +269,7 @@ void write_sparse_image(block_dev_desc_t *dev_desc, > > > } > > > > > > /* verify sparse_header->blk_sz is an exact multiple of info->blksz */ > > > - if (sparse_header->blk_sz != > > > - (sparse_header->blk_sz & ~(info->blksz - 1))) { > > > + if (sparse_header->blk_sz % info->blksz) { > > > > So, sometimes we have convoluted syntax like this to avoid what ends up > > as floating point math on 32bit platforms. > > Now that you speak of this, we did have some compilers on some > platforms that ended up generating floating point related failures on > our branch, but I didn't have the time to look into it. > > However, I don't really know how we can end up with floating point > math with a mod, is this some optimisations done by gcc? My fuzz recollection is that GCC on 32bit ARM when it defaults to a hardfloat ABI "knows" that it is "safe" to use the floating point registers to do 64bit division rather than shifting and so forth. > > Maybe this needs to be one of the helpers in include/linux/math64.h? > > Probably then, yes. Yeah, this is it. My first guess is that if you look at 9e374e7b729dc9f68be89cd3e3b1d4d48c768ecf you'll find an example that matches the problem here and what the right helper/code chagnes are.
On Sun, Sep 06, 2015 at 03:28:34PM -0400, Tom Rini wrote: > On Sun, Sep 06, 2015 at 01:27:20PM +0200, Maxime Ripard wrote: > > On Fri, Sep 04, 2015 at 01:20:38PM -0400, Tom Rini wrote: > > > On Mon, Aug 31, 2015 at 04:46:04PM +0200, Maxime Ripard wrote: > > > > > > > To check the alignment of the image blocks to the storage blocks, the > > > > current code uses a convoluted syntax, while a simple mod also does the > > > > work. > > > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > > > --- > > > > common/aboot.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/common/aboot.c b/common/aboot.c > > > > index 65e633acfcb9..c8556d9b23f4 100644 > > > > --- a/common/aboot.c > > > > +++ b/common/aboot.c > > > > @@ -269,8 +269,7 @@ void write_sparse_image(block_dev_desc_t *dev_desc, > > > > } > > > > > > > > /* verify sparse_header->blk_sz is an exact multiple of info->blksz */ > > > > - if (sparse_header->blk_sz != > > > > - (sparse_header->blk_sz & ~(info->blksz - 1))) { > > > > + if (sparse_header->blk_sz % info->blksz) { > > > > > > So, sometimes we have convoluted syntax like this to avoid what ends up > > > as floating point math on 32bit platforms. > > > > Now that you speak of this, we did have some compilers on some > > platforms that ended up generating floating point related failures on > > our branch, but I didn't have the time to look into it. > > > > However, I don't really know how we can end up with floating point > > math with a mod, is this some optimisations done by gcc? > > My fuzz recollection is that GCC on 32bit ARM when it defaults to a > hardfloat ABI "knows" that it is "safe" to use the floating point > registers to do 64bit division rather than shifting and so forth. Hmm, ok, I see. > > > Maybe this needs to be one of the helpers in include/linux/math64.h? > > > > Probably then, yes. > > Yeah, this is it. My first guess is that if you look at > 9e374e7b729dc9f68be89cd3e3b1d4d48c768ecf you'll find an example that > matches the problem here and what the right helper/code chagnes are. I'll look at this and use it then. Thanks! Maxime
diff --git a/common/aboot.c b/common/aboot.c index 65e633acfcb9..c8556d9b23f4 100644 --- a/common/aboot.c +++ b/common/aboot.c @@ -269,8 +269,7 @@ void write_sparse_image(block_dev_desc_t *dev_desc, } /* verify sparse_header->blk_sz is an exact multiple of info->blksz */ - if (sparse_header->blk_sz != - (sparse_header->blk_sz & ~(info->blksz - 1))) { + if (sparse_header->blk_sz % info->blksz) { printf("%s: Sparse image block size issue [%u]\n", __func__, sparse_header->blk_sz); fastboot_fail("sparse image block size issue");
To check the alignment of the image blocks to the storage blocks, the current code uses a convoluted syntax, while a simple mod also does the work. Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> --- common/aboot.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)