* [PATCH] ubifs: endian handling fixes and annotations
@ 2008-10-24 17:52 Harvey Harrison
2008-10-25 10:57 ` Artem Bityutskiy
0 siblings, 1 reply; 14+ messages in thread
From: Harvey Harrison @ 2008-10-24 17:52 UTC (permalink / raw)
To: Adrian Hunter, Artem Bityutskiy; +Cc: Andrew Morton, LKML
Noticed by sparse:
fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer
fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer
fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer
This should be checked to ensure the ubifs_assert is working as
intended, I've done the suggested annotation in this patch.
fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
This looks like a bugfix as your tmp was a u32 so there was truncation in
the atime, mtime, ctime value, probably not intentional, add a tmp_le64
and use it here.
fs/ubifs/key.h:348:9: warning: cast to restricted __le32
fs/ubifs/key.h:348:9: warning: cast to restricted __le32
fs/ubifs/key.h:419:9: warning: cast to restricted __le32
Read from the annotated union member instead.
fs/ubifs/recovery.c:175:13: warning: incorrect type in assignment (different base types)
fs/ubifs/recovery.c:175:13: expected unsigned int [unsigned] [usertype] save_flags
fs/ubifs/recovery.c:175:13: got restricted __le32 [usertype] flags
fs/ubifs/recovery.c:186:13: warning: incorrect type in assignment (different base types)
fs/ubifs/recovery.c:186:13: expected restricted __le32 [usertype] flags
fs/ubifs/recovery.c:186:13: got unsigned int [unsigned] [usertype] save_flags
Do byteshifting at compile time of the flag value. Annotate the saved_flags
as le32.
fs/ubifs/debug.c:368:10: warning: cast to restricted __le32
fs/ubifs/debug.c:368:10: warning: cast from restricted __le64
Should be checked if the truncation was intentional, I've changed the
printk to print the full width.
Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
fs/ubifs/debug.c | 4 ++--
fs/ubifs/dir.c | 3 ++-
fs/ubifs/file.c | 4 ++--
fs/ubifs/key.h | 4 ++--
fs/ubifs/recovery.c | 4 ++--
fs/ubifs/sb.c | 9 +++++----
6 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 7186400..f9deccb 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -364,8 +364,8 @@ void dbg_dump_node(const struct ubifs_info *c, const void *node)
le32_to_cpu(mst->ihead_lnum));
printk(KERN_DEBUG "\tihead_offs %u\n",
le32_to_cpu(mst->ihead_offs));
- printk(KERN_DEBUG "\tindex_size %u\n",
- le32_to_cpu(mst->index_size));
+ printk(KERN_DEBUG "\tindex_size %llu\n",
+ (unsigned long long)le64_to_cpu(mst->index_size));
printk(KERN_DEBUG "\tlpt_lnum %u\n",
le32_to_cpu(mst->lpt_lnum));
printk(KERN_DEBUG "\tlpt_offs %u\n",
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 8561890..198cf3e 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -428,7 +428,8 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir)
dbg_gen("feed '%s', ino %llu, new f_pos %#x",
dent->name, (unsigned long long)le64_to_cpu(dent->inum),
key_hash_flash(c, &dent->key));
- ubifs_assert(dent->ch.sqnum > ubifs_inode(dir)->creat_sqnum);
+ ubifs_assert(le64_to_cpu(dent->ch.sqnum) >
+ ubifs_inode(dir)->creat_sqnum);
nm.len = le16_to_cpu(dent->nlen);
over = filldir(dirent, dent->name, nm.len, file->f_pos,
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 51cf511..9124eee 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -72,7 +72,7 @@ static int read_block(struct inode *inode, void *addr, unsigned int block,
return err;
}
- ubifs_assert(dn->ch.sqnum > ubifs_inode(inode)->creat_sqnum);
+ ubifs_assert(le64_to_cpu(dn->ch.sqnum) > ubifs_inode(inode)->creat_sqnum);
len = le32_to_cpu(dn->size);
if (len <= 0 || len > UBIFS_BLOCK_SIZE)
@@ -626,7 +626,7 @@ static int populate_page(struct ubifs_info *c, struct page *page,
dn = bu->buf + (bu->zbranch[nn].offs - offs);
- ubifs_assert(dn->ch.sqnum >
+ ubifs_assert(le64_to_cpu(dn->ch.sqnum) >
ubifs_inode(inode)->creat_sqnum);
len = le32_to_cpu(dn->size);
diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
index 9ee6508..3f1f16b 100644
--- a/fs/ubifs/key.h
+++ b/fs/ubifs/key.h
@@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
{
const union ubifs_key *key = k;
- return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
+ return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
}
/**
@@ -416,7 +416,7 @@ static inline unsigned int key_block_flash(const struct ubifs_info *c,
{
const union ubifs_key *key = k;
- return le32_to_cpu(key->u32[1]) & UBIFS_S_KEY_BLOCK_MASK;
+ return le32_to_cpu(key->j32[1]) & UBIFS_S_KEY_BLOCK_MASK;
}
/**
diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
index 77d26c1..bed9742 100644
--- a/fs/ubifs/recovery.c
+++ b/fs/ubifs/recovery.c
@@ -168,12 +168,12 @@ static int write_rcvrd_mst_node(struct ubifs_info *c,
struct ubifs_mst_node *mst)
{
int err = 0, lnum = UBIFS_MST_LNUM, sz = c->mst_node_alsz;
- uint32_t save_flags;
+ __le32 save_flags;
dbg_rcvry("recovery");
save_flags = mst->flags;
- mst->flags = cpu_to_le32(le32_to_cpu(mst->flags) | UBIFS_MST_RCVRY);
+ mst->flags |= cpu_to_le32(UBIFS_MST_RCVRY);
ubifs_prepare_node(c, mst, UBIFS_MST_NODE_SZ, 1);
err = ubi_leb_change(c->ubi, lnum, mst, sz, UBI_SHORTTERM);
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 2bf753b..0f39235 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -81,6 +81,7 @@ static int create_default_filesystem(struct ubifs_info *c)
int lpt_lebs, lpt_first, orph_lebs, big_lpt, ino_waste, sup_flags = 0;
int min_leb_cnt = UBIFS_MIN_LEB_CNT;
uint64_t tmp64, main_bytes;
+ __le64 tmp_le64;
/* Some functions called from here depend on the @c->key_len filed */
c->key_len = UBIFS_SK_LEN;
@@ -295,10 +296,10 @@ static int create_default_filesystem(struct ubifs_info *c)
ino->ch.node_type = UBIFS_INO_NODE;
ino->creat_sqnum = cpu_to_le64(++c->max_sqnum);
ino->nlink = cpu_to_le32(2);
- tmp = cpu_to_le64(CURRENT_TIME_SEC.tv_sec);
- ino->atime_sec = tmp;
- ino->ctime_sec = tmp;
- ino->mtime_sec = tmp;
+ tmp_le64 = cpu_to_le64(CURRENT_TIME_SEC.tv_sec);
+ ino->atime_sec = tmp_le64;
+ ino->ctime_sec = tmp_le64;
+ ino->mtime_sec = tmp_le64;
ino->atime_nsec = 0;
ino->ctime_nsec = 0;
ino->mtime_nsec = 0;
--
1.6.0.3.723.g757e
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] ubifs: endian handling fixes and annotations
2008-10-24 17:52 [PATCH] ubifs: endian handling fixes and annotations Harvey Harrison
@ 2008-10-25 10:57 ` Artem Bityutskiy
2008-10-25 18:52 ` Harvey Harrison
0 siblings, 1 reply; 14+ messages in thread
From: Artem Bityutskiy @ 2008-10-25 10:57 UTC (permalink / raw)
To: Harvey Harrison; +Cc: Adrian Hunter, Andrew Morton, LKML
Harvey,
On Fri, 2008-10-24 at 10:52 -0700, Harvey Harrison wrote:
> Noticed by sparse:
> fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer
> fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer
> fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer
>
> This should be checked to ensure the ubifs_assert is working as
> intended, I've done the suggested annotation in this patch.
>
> fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
> fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
> fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
> fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
> fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
> fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
> fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
> fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
> fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
> fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
> fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
> fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
... snip ...
thanks for the patch. It's shame we did not fix this ourselves. We did
run sparse before submitting UBIFS and did not see these warnings.
Probably sparse has been improved recently. Anyway, thank you, I'll look
closer at your patch and apply it to ubifs-2.6.git.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ubifs: endian handling fixes and annotations
2008-10-25 10:57 ` Artem Bityutskiy
@ 2008-10-25 18:52 ` Harvey Harrison
2008-10-26 10:12 ` Artem Bityutskiy
2008-10-26 13:22 ` Artem Bityutskiy
0 siblings, 2 replies; 14+ messages in thread
From: Harvey Harrison @ 2008-10-25 18:52 UTC (permalink / raw)
To: dedekind; +Cc: Adrian Hunter, Andrew Morton, LKML
On Sat, 2008-10-25 at 13:57 +0300, Artem Bityutskiy wrote:
> Harvey,
>
> On Fri, 2008-10-24 at 10:52 -0700, Harvey Harrison wrote:
> > Noticed by sparse:
> > fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer
> > fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer
> > fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer
> >
> > This should be checked to ensure the ubifs_assert is working as
> > intended, I've done the suggested annotation in this patch.
> >
> > fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
> > fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
> > fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
> > fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
> > fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
> > fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
> > fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
> > fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
> > fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
> > fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
> > fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
> > fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
>
> ... snip ...
>
> thanks for the patch. It's shame we did not fix this ourselves. We did
> run sparse before submitting UBIFS and did not see these warnings.
> Probably sparse has been improved recently. Anyway, thank you, I'll look
> closer at your patch and apply it to ubifs-2.6.git.
>
Run sparse with -D__CHECK_ENDIAN__ to see these warnings.
Harvey
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ubifs: endian handling fixes and annotations
2008-10-25 18:52 ` Harvey Harrison
@ 2008-10-26 10:12 ` Artem Bityutskiy
2008-10-26 13:22 ` Artem Bityutskiy
1 sibling, 0 replies; 14+ messages in thread
From: Artem Bityutskiy @ 2008-10-26 10:12 UTC (permalink / raw)
To: Harvey Harrison; +Cc: Adrian Hunter, Andrew Morton, LKML
On Sat, 2008-10-25 at 11:52 -0700, Harvey Harrison wrote:
> Run sparse with -D__CHECK_ENDIAN__ to see these warnings.
Thanks. Pushed your patch to ubifs-2.6.git.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ubifs: endian handling fixes and annotations
2008-10-25 18:52 ` Harvey Harrison
2008-10-26 10:12 ` Artem Bityutskiy
@ 2008-10-26 13:22 ` Artem Bityutskiy
2008-10-26 18:44 ` Harvey Harrison
1 sibling, 1 reply; 14+ messages in thread
From: Artem Bityutskiy @ 2008-10-26 13:22 UTC (permalink / raw)
To: Harvey Harrison; +Cc: Adrian Hunter, Andrew Morton, LKML
On Sat, 2008-10-25 at 11:52 -0700, Harvey Harrison wrote:
> > > fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
> > > fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
> > > fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
> > > fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
> > > fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
> > > fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
> >
> > ... snip ...
> >
> > thanks for the patch. It's shame we did not fix this ourselves. We did
> > run sparse before submitting UBIFS and did not see these warnings.
> > Probably sparse has been improved recently. Anyway, thank you, I'll look
> > closer at your patch and apply it to ubifs-2.6.git.
> >
>
> Run sparse with -D__CHECK_ENDIAN__ to see these warnings.
Any idea why this is not default?
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ubifs: endian handling fixes and annotations
2008-10-26 13:22 ` Artem Bityutskiy
@ 2008-10-26 18:44 ` Harvey Harrison
0 siblings, 0 replies; 14+ messages in thread
From: Harvey Harrison @ 2008-10-26 18:44 UTC (permalink / raw)
To: dedekind; +Cc: Adrian Hunter, Andrew Morton, LKML
On Sun, 2008-10-26 at 15:22 +0200, Artem Bityutskiy wrote:
> On Sat, 2008-10-25 at 11:52 -0700, Harvey Harrison wrote:
> > > > fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
> > > > fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
> > > > fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
> > > > fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
> > > > fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
> > > > fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
> > >
> > > ... snip ...
> > >
> > > thanks for the patch. It's shame we did not fix this ourselves. We did
> > > run sparse before submitting UBIFS and did not see these warnings.
> > > Probably sparse has been improved recently. Anyway, thank you, I'll look
> > > closer at your patch and apply it to ubifs-2.6.git.
> > >
> >
> > Run sparse with -D__CHECK_ENDIAN__ to see these warnings.
>
> Any idea why this is not default?
>
Currently the build gets a bit verbose with this turned on, I've been
working to get the noise down a bit, but perhaps still a ways off from
turning this on.
Harvey
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-22 19:27 ` Sebastian Andrzej Siewior
` (2 preceding siblings ...)
2008-11-24 14:19 ` Adrian Hunter
@ 2008-12-02 9:12 ` Artem Bityutskiy
3 siblings, 0 replies; 14+ messages in thread
From: Artem Bityutskiy @ 2008-12-02 9:12 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: linux-fsdevel, linux-kernel
On Sat, 2008-11-22 at 20:27 +0100, Sebastian Andrzej Siewior wrote:
> >index 9ee6508..3f1f16b 100644
> >--- a/fs/ubifs/key.h
> >+++ b/fs/ubifs/key.h
> >@@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
> > {
> > const union ubifs_key *key = k;
> >
> >- return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> >+ return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
>
> If you would change such references to something like
> |return le32_to_cpup(&key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> then on powerpc
>
> text data bss dec hex filename
> 155384 1284 24 156692 26414 ubifs-b4.ko
> 155372 1284 24 156680 26408 ubifs-after.ko
>
> because now it is possible to load the value as LE from memory instead
> of loading it BE and swapping it afterwads.
Well, I think stuff like this should be done by either GCC or by a PPC-specific
'le32_to_cpu()' implementation.
--
Best regards,
Artem Bityutskiy (Битюцкий Артём)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-24 14:19 ` Adrian Hunter
@ 2008-11-24 16:46 ` Harvey Harrison
0 siblings, 0 replies; 14+ messages in thread
From: Harvey Harrison @ 2008-11-24 16:46 UTC (permalink / raw)
To: Adrian Hunter
Cc: Sebastian Andrzej Siewior, Artem Bityutskiy, linux-fsdevel, linux-kernel
On Mon, 2008-11-24 at 16:19 +0200, Adrian Hunter wrote:
> Sebastian Andrzej Siewior wrote:
> > * Artem Bityutskiy | 2008-11-21 19:19:24 [+0200]:
> >
> >> index 9ee6508..3f1f16b 100644
> >> --- a/fs/ubifs/key.h
> >> +++ b/fs/ubifs/key.h
> >> @@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
> >> {
> >> const union ubifs_key *key = k;
> >>
> >> - return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> >> + return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> >
> > If you would change such references to something like
> > |return le32_to_cpup(&key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> > then on powerpc
> >
> > text data bss dec hex filename
> > 155384 1284 24 156692 26414 ubifs-b4.ko
> > 155372 1284 24 156680 26408 ubifs-after.ko
> >
> > because now it is possible to load the value as LE from memory instead
> > of loading it BE and swapping it afterwads.
>
> Wouldn't that be true for every le32_to_cpu of an lvalue? Shame you can't
> do:
>
> is_lvalue(x) ? le32_to_cpup(&(x)) : le32_to_cpu(x)
>
No, you wouldn't want to do the above if the lvalue was on the stack as most
of the time the extra code to setup a pointer to a stack variable ends up
being more expensive than just using cpu_to_le32.
Cheers,
Harvey
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-22 19:27 ` Sebastian Andrzej Siewior
2008-11-23 3:21 ` Harvey Harrison
2008-11-23 10:05 ` Jamie Lokier
@ 2008-11-24 14:19 ` Adrian Hunter
2008-11-24 16:46 ` Harvey Harrison
2008-12-02 9:12 ` Artem Bityutskiy
3 siblings, 1 reply; 14+ messages in thread
From: Adrian Hunter @ 2008-11-24 14:19 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: Artem Bityutskiy, linux-fsdevel, linux-kernel
Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2008-11-21 19:19:24 [+0200]:
>
>> index 9ee6508..3f1f16b 100644
>> --- a/fs/ubifs/key.h
>> +++ b/fs/ubifs/key.h
>> @@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
>> {
>> const union ubifs_key *key = k;
>>
>> - return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
>> + return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
>
> If you would change such references to something like
> |return le32_to_cpup(&key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> then on powerpc
>
> text data bss dec hex filename
> 155384 1284 24 156692 26414 ubifs-b4.ko
> 155372 1284 24 156680 26408 ubifs-after.ko
>
> because now it is possible to load the value as LE from memory instead
> of loading it BE and swapping it afterwads.
Wouldn't that be true for every le32_to_cpu of an lvalue? Shame you can't
do:
is_lvalue(x) ? le32_to_cpup(&(x)) : le32_to_cpu(x)
>> }
>>
>> /**
>> @@ -416,7 +416,7 @@ static inline unsigned int key_block_flash(const struct ubifs_info *c,
>> {
>> const union ubifs_key *key = k;
>>
>> - return le32_to_cpu(key->u32[1]) & UBIFS_S_KEY_BLOCK_MASK;
>> + return le32_to_cpu(key->j32[1]) & UBIFS_S_KEY_BLOCK_MASK;
>> }
>
> This and the previous change look like a bugfix for something that
> should trigger during recovery or something? Shouldn't I fail in
> ubifs_validate_entry() during recovery?
This is just about casting. key->u32[1] and key->j32[1] are the same object.
There is no "real" bug in these two cases - just compilation warnings.
>> /**
>> diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
>> index 77d26c1..bed9742 100644
>> --- a/fs/ubifs/recovery.c
>> +++ b/fs/ubifs/recovery.c
>> @@ -168,12 +168,12 @@ static int write_rcvrd_mst_node(struct ubifs_info *c,
>> struct ubifs_mst_node *mst)
>> {
>> int err = 0, lnum = UBIFS_MST_LNUM, sz = c->mst_node_alsz;
>> - uint32_t save_flags;
>> + __le32 save_flags;
>>
>> dbg_rcvry("recovery");
>>
>> save_flags = mst->flags;
>> - mst->flags = cpu_to_le32(le32_to_cpu(mst->flags) | UBIFS_MST_RCVRY);
>> + mst->flags |= cpu_to_le32(UBIFS_MST_RCVRY);
>
> another micro optimisation would be to use __constant_cpu_to_le32()
As per Harvey's reply.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-22 19:27 ` Sebastian Andrzej Siewior
2008-11-23 3:21 ` Harvey Harrison
@ 2008-11-23 10:05 ` Jamie Lokier
2008-11-24 14:19 ` Adrian Hunter
2008-12-02 9:12 ` Artem Bityutskiy
3 siblings, 0 replies; 14+ messages in thread
From: Jamie Lokier @ 2008-11-23 10:05 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: Artem Bityutskiy, linux-fsdevel, linux-kernel
Sebastian Andrzej Siewior wrote:
> >- return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> >+ return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
>
> If you would change such references to something like
> |return le32_to_cpup(&key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
> then on powerpc
>
> text data bss dec hex filename
> 155384 1284 24 156692 26414 ubifs-b4.ko
> 155372 1284 24 156680 26408 ubifs-after.ko
>
> because now it is possible to load the value as LE from memory instead
> of loading it BE and swapping it afterwads.
If le32_to_cpu used __builtin_bswap32 (a GCC builtin), wouldn't
GCC do this trivial optimisation itself?
-- Jamie
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-23 3:21 ` Harvey Harrison
@ 2008-11-23 9:28 ` Sebastian Andrzej Siewior
0 siblings, 0 replies; 14+ messages in thread
From: Sebastian Andrzej Siewior @ 2008-11-23 9:28 UTC (permalink / raw)
To: Harvey Harrison; +Cc: Artem Bityutskiy, linux-fsdevel, linux-kernel
* Harvey Harrison | 2008-11-22 19:21:29 [-0800]:
>Nope, no difference, cpu_to_le32 does constant-detection these days and
>so there ends up being no difference.
Indeed. __builtin_constant_p() does the trick. So I guess we could get
rid of __constant_cpu_to_XX32() and its
| grep -RE __constant_cpu_to_[bl]e32 . | wc -l
| 413
users.
>
>Harvey
>
Sebastian
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-22 19:27 ` Sebastian Andrzej Siewior
@ 2008-11-23 3:21 ` Harvey Harrison
2008-11-23 9:28 ` Sebastian Andrzej Siewior
2008-11-23 10:05 ` Jamie Lokier
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Harvey Harrison @ 2008-11-23 3:21 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: Artem Bityutskiy, linux-fsdevel, linux-kernel
On Sat, 2008-11-22 at 20:27 +0100, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2008-11-21 19:19:24 [+0200]:
>
> > save_flags = mst->flags;
> >- mst->flags = cpu_to_le32(le32_to_cpu(mst->flags) | UBIFS_MST_RCVRY);
> >+ mst->flags |= cpu_to_le32(UBIFS_MST_RCVRY);
>
> another micro optimisation would be to use __constant_cpu_to_le32()
>
Nope, no difference, cpu_to_le32 does constant-detection these days and
so there ends up being no difference.
Harvey
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] UBIFS: endian handling fixes and annotations
2008-11-21 17:19 ` [PATCH] UBIFS: endian handling fixes and annotations Artem Bityutskiy
@ 2008-11-22 19:27 ` Sebastian Andrzej Siewior
2008-11-23 3:21 ` Harvey Harrison
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Sebastian Andrzej Siewior @ 2008-11-22 19:27 UTC (permalink / raw)
To: Artem Bityutskiy; +Cc: linux-fsdevel, linux-kernel
* Artem Bityutskiy | 2008-11-21 19:19:24 [+0200]:
>index 9ee6508..3f1f16b 100644
>--- a/fs/ubifs/key.h
>+++ b/fs/ubifs/key.h
>@@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
> {
> const union ubifs_key *key = k;
>
>- return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
>+ return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
If you would change such references to something like
|return le32_to_cpup(&key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
then on powerpc
text data bss dec hex filename
155384 1284 24 156692 26414 ubifs-b4.ko
155372 1284 24 156680 26408 ubifs-after.ko
because now it is possible to load the value as LE from memory instead
of loading it BE and swapping it afterwads.
> }
>
> /**
>@@ -416,7 +416,7 @@ static inline unsigned int key_block_flash(const struct ubifs_info *c,
> {
> const union ubifs_key *key = k;
>
>- return le32_to_cpu(key->u32[1]) & UBIFS_S_KEY_BLOCK_MASK;
>+ return le32_to_cpu(key->j32[1]) & UBIFS_S_KEY_BLOCK_MASK;
> }
This and the previous change look like a bugfix for something that
should trigger during recovery or something? Shouldn't I fail in
ubifs_validate_entry() during recovery?
> /**
>diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
>index 77d26c1..bed9742 100644
>--- a/fs/ubifs/recovery.c
>+++ b/fs/ubifs/recovery.c
>@@ -168,12 +168,12 @@ static int write_rcvrd_mst_node(struct ubifs_info *c,
> struct ubifs_mst_node *mst)
> {
> int err = 0, lnum = UBIFS_MST_LNUM, sz = c->mst_node_alsz;
>- uint32_t save_flags;
>+ __le32 save_flags;
>
> dbg_rcvry("recovery");
>
> save_flags = mst->flags;
>- mst->flags = cpu_to_le32(le32_to_cpu(mst->flags) | UBIFS_MST_RCVRY);
>+ mst->flags |= cpu_to_le32(UBIFS_MST_RCVRY);
another micro optimisation would be to use __constant_cpu_to_le32()
Sebastian
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] UBIFS: endian handling fixes and annotations
2008-11-21 17:19 UBIFS updates for 2.6.28 Artem Bityutskiy
@ 2008-11-21 17:19 ` Artem Bityutskiy
2008-11-22 19:27 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 14+ messages in thread
From: Artem Bityutskiy @ 2008-11-21 17:19 UTC (permalink / raw)
To: linux-fsdevel; +Cc: linux-kernel
From: Harvey Harrison <harvey.harrison@gmail.com>
Noticed by sparse:
fs/ubifs/file.c:75:2: warning: restricted __le64 degrades to integer
fs/ubifs/file.c:629:4: warning: restricted __le64 degrades to integer
fs/ubifs/dir.c:431:3: warning: restricted __le64 degrades to integer
This should be checked to ensure the ubifs_assert is working as
intended, I've done the suggested annotation in this patch.
fs/ubifs/sb.c:298:6: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:298:6: expected int [signed] [assigned] tmp
fs/ubifs/sb.c:298:6: got restricted __le64 [usertype] <noident>
fs/ubifs/sb.c:299:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:299:19: expected restricted __le64 [usertype] atime_sec
fs/ubifs/sb.c:299:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:300:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:300:19: expected restricted __le64 [usertype] ctime_sec
fs/ubifs/sb.c:300:19: got int [signed] [assigned] tmp
fs/ubifs/sb.c:301:19: warning: incorrect type in assignment (different base types)
fs/ubifs/sb.c:301:19: expected restricted __le64 [usertype] mtime_sec
fs/ubifs/sb.c:301:19: got int [signed] [assigned] tmp
This looks like a bugfix as your tmp was a u32 so there was truncation in
the atime, mtime, ctime value, probably not intentional, add a tmp_le64
and use it here.
fs/ubifs/key.h:348:9: warning: cast to restricted __le32
fs/ubifs/key.h:348:9: warning: cast to restricted __le32
fs/ubifs/key.h:419:9: warning: cast to restricted __le32
Read from the annotated union member instead.
fs/ubifs/recovery.c:175:13: warning: incorrect type in assignment (different base types)
fs/ubifs/recovery.c:175:13: expected unsigned int [unsigned] [usertype] save_flags
fs/ubifs/recovery.c:175:13: got restricted __le32 [usertype] flags
fs/ubifs/recovery.c:186:13: warning: incorrect type in assignment (different base types)
fs/ubifs/recovery.c:186:13: expected restricted __le32 [usertype] flags
fs/ubifs/recovery.c:186:13: got unsigned int [unsigned] [usertype] save_flags
Do byteshifting at compile time of the flag value. Annotate the saved_flags
as le32.
fs/ubifs/debug.c:368:10: warning: cast to restricted __le32
fs/ubifs/debug.c:368:10: warning: cast from restricted __le64
Should be checked if the truncation was intentional, I've changed the
printk to print the full width.
Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
fs/ubifs/debug.c | 4 ++--
fs/ubifs/dir.c | 3 ++-
fs/ubifs/file.c | 4 ++--
fs/ubifs/key.h | 4 ++--
fs/ubifs/recovery.c | 4 ++--
fs/ubifs/sb.c | 9 +++++----
6 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 7186400..f9deccb 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -364,8 +364,8 @@ void dbg_dump_node(const struct ubifs_info *c, const void *node)
le32_to_cpu(mst->ihead_lnum));
printk(KERN_DEBUG "\tihead_offs %u\n",
le32_to_cpu(mst->ihead_offs));
- printk(KERN_DEBUG "\tindex_size %u\n",
- le32_to_cpu(mst->index_size));
+ printk(KERN_DEBUG "\tindex_size %llu\n",
+ (unsigned long long)le64_to_cpu(mst->index_size));
printk(KERN_DEBUG "\tlpt_lnum %u\n",
le32_to_cpu(mst->lpt_lnum));
printk(KERN_DEBUG "\tlpt_offs %u\n",
diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
index 526c01e..37a9e60 100644
--- a/fs/ubifs/dir.c
+++ b/fs/ubifs/dir.c
@@ -428,7 +428,8 @@ static int ubifs_readdir(struct file *file, void *dirent, filldir_t filldir)
dbg_gen("feed '%s', ino %llu, new f_pos %#x",
dent->name, (unsigned long long)le64_to_cpu(dent->inum),
key_hash_flash(c, &dent->key));
- ubifs_assert(dent->ch.sqnum > ubifs_inode(dir)->creat_sqnum);
+ ubifs_assert(le64_to_cpu(dent->ch.sqnum) >
+ ubifs_inode(dir)->creat_sqnum);
nm.len = le16_to_cpu(dent->nlen);
over = filldir(dirent, dent->name, nm.len, file->f_pos,
diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
index 51cf511..9124eee 100644
--- a/fs/ubifs/file.c
+++ b/fs/ubifs/file.c
@@ -72,7 +72,7 @@ static int read_block(struct inode *inode, void *addr, unsigned int block,
return err;
}
- ubifs_assert(dn->ch.sqnum > ubifs_inode(inode)->creat_sqnum);
+ ubifs_assert(le64_to_cpu(dn->ch.sqnum) > ubifs_inode(inode)->creat_sqnum);
len = le32_to_cpu(dn->size);
if (len <= 0 || len > UBIFS_BLOCK_SIZE)
@@ -626,7 +626,7 @@ static int populate_page(struct ubifs_info *c, struct page *page,
dn = bu->buf + (bu->zbranch[nn].offs - offs);
- ubifs_assert(dn->ch.sqnum >
+ ubifs_assert(le64_to_cpu(dn->ch.sqnum) >
ubifs_inode(inode)->creat_sqnum);
len = le32_to_cpu(dn->size);
diff --git a/fs/ubifs/key.h b/fs/ubifs/key.h
index 9ee6508..3f1f16b 100644
--- a/fs/ubifs/key.h
+++ b/fs/ubifs/key.h
@@ -345,7 +345,7 @@ static inline int key_type_flash(const struct ubifs_info *c, const void *k)
{
const union ubifs_key *key = k;
- return le32_to_cpu(key->u32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
+ return le32_to_cpu(key->j32[1]) >> UBIFS_S_KEY_BLOCK_BITS;
}
/**
@@ -416,7 +416,7 @@ static inline unsigned int key_block_flash(const struct ubifs_info *c,
{
const union ubifs_key *key = k;
- return le32_to_cpu(key->u32[1]) & UBIFS_S_KEY_BLOCK_MASK;
+ return le32_to_cpu(key->j32[1]) & UBIFS_S_KEY_BLOCK_MASK;
}
/**
diff --git a/fs/ubifs/recovery.c b/fs/ubifs/recovery.c
index 77d26c1..bed9742 100644
--- a/fs/ubifs/recovery.c
+++ b/fs/ubifs/recovery.c
@@ -168,12 +168,12 @@ static int write_rcvrd_mst_node(struct ubifs_info *c,
struct ubifs_mst_node *mst)
{
int err = 0, lnum = UBIFS_MST_LNUM, sz = c->mst_node_alsz;
- uint32_t save_flags;
+ __le32 save_flags;
dbg_rcvry("recovery");
save_flags = mst->flags;
- mst->flags = cpu_to_le32(le32_to_cpu(mst->flags) | UBIFS_MST_RCVRY);
+ mst->flags |= cpu_to_le32(UBIFS_MST_RCVRY);
ubifs_prepare_node(c, mst, UBIFS_MST_NODE_SZ, 1);
err = ubi_leb_change(c->ubi, lnum, mst, sz, UBI_SHORTTERM);
diff --git a/fs/ubifs/sb.c b/fs/ubifs/sb.c
index 2bf753b..0f39235 100644
--- a/fs/ubifs/sb.c
+++ b/fs/ubifs/sb.c
@@ -81,6 +81,7 @@ static int create_default_filesystem(struct ubifs_info *c)
int lpt_lebs, lpt_first, orph_lebs, big_lpt, ino_waste, sup_flags = 0;
int min_leb_cnt = UBIFS_MIN_LEB_CNT;
uint64_t tmp64, main_bytes;
+ __le64 tmp_le64;
/* Some functions called from here depend on the @c->key_len filed */
c->key_len = UBIFS_SK_LEN;
@@ -295,10 +296,10 @@ static int create_default_filesystem(struct ubifs_info *c)
ino->ch.node_type = UBIFS_INO_NODE;
ino->creat_sqnum = cpu_to_le64(++c->max_sqnum);
ino->nlink = cpu_to_le32(2);
- tmp = cpu_to_le64(CURRENT_TIME_SEC.tv_sec);
- ino->atime_sec = tmp;
- ino->ctime_sec = tmp;
- ino->mtime_sec = tmp;
+ tmp_le64 = cpu_to_le64(CURRENT_TIME_SEC.tv_sec);
+ ino->atime_sec = tmp_le64;
+ ino->ctime_sec = tmp_le64;
+ ino->mtime_sec = tmp_le64;
ino->atime_nsec = 0;
ino->ctime_nsec = 0;
ino->mtime_nsec = 0;
--
1.5.4.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2008-12-02 9:15 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-24 17:52 [PATCH] ubifs: endian handling fixes and annotations Harvey Harrison
2008-10-25 10:57 ` Artem Bityutskiy
2008-10-25 18:52 ` Harvey Harrison
2008-10-26 10:12 ` Artem Bityutskiy
2008-10-26 13:22 ` Artem Bityutskiy
2008-10-26 18:44 ` Harvey Harrison
2008-11-21 17:19 UBIFS updates for 2.6.28 Artem Bityutskiy
2008-11-21 17:19 ` [PATCH] UBIFS: endian handling fixes and annotations Artem Bityutskiy
2008-11-22 19:27 ` Sebastian Andrzej Siewior
2008-11-23 3:21 ` Harvey Harrison
2008-11-23 9:28 ` Sebastian Andrzej Siewior
2008-11-23 10:05 ` Jamie Lokier
2008-11-24 14:19 ` Adrian Hunter
2008-11-24 16:46 ` Harvey Harrison
2008-12-02 9:12 ` Artem Bityutskiy
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).