| Message ID | 20180315150814.9412-17-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 15, 2018 at 08:08:14AM -0700, Bart Van Assche wrote: > This patch avoids that sparse complains about using integer types > incorrectly (u32, __le32, __be32, ...) by splitting struct cache_sb > into two different structures: > - struct cache_sb in which all integer members except csum have > CPU endianness. > - struct cache_sb_le in which all integer members except csum are > declared as little endian. Can you call this cache_sb_disk to name it after the purpose instead of the implementation? Except for that this looks like the right fix: Reviewed-by: Christoph Hellwig <hch@lst.de>
On 15/03/2018 11:08 PM, Bart Van Assche wrote: > This patch avoids that sparse complains about using integer types > incorrectly (u32, __le32, __be32, ...) by splitting struct cache_sb > into two different structures: > - struct cache_sb in which all integer members except csum have > CPU endianness. > - struct cache_sb_le in which all integer members except csum are > declared as little endian. > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Reviewed-by: Coly Li <colyli@suse.de> Hi Bart, This patch is good, and I like the idea of STRUCT_CACHE_SB(). How about letting me to pick this patch into my bcache s390x fixes? I will fix the structure name suggested by Christoph, and set you as the patch author. Thanks. Coly Li > --- > drivers/md/bcache/super.c | 10 ++-- > include/uapi/linux/bcache.h | 118 +++++++++++++++++++++++--------------------- > 2 files changed, 68 insertions(+), 60 deletions(-) > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index 31d700aecd56..ef659c7d72f9 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -63,20 +63,22 @@ struct workqueue_struct *bcache_wq; > /* limitation of bcache devices number on single system */ > #define BCACHE_DEVICE_IDX_MAX ((1U << MINORBITS)/BCACHE_MINORS) > > +struct cache_sb_le STRUCT_CACHE_SB(le); > + > /* Superblock */ > > static const char *read_super(struct cache_sb *sb, struct block_device *bdev, > struct page **res) > { > const char *err; > - struct cache_sb *s; > + struct cache_sb_le *s; > struct buffer_head *bh = __bread(bdev, 1, SB_SIZE); > unsigned i; > > if (!bh) > return "IO error"; > > - s = (struct cache_sb *) bh->b_data; > + s = (struct cache_sb_le *) bh->b_data; > > sb->offset = le64_to_cpu(s->offset); > sb->version = le64_to_cpu(s->version); > @@ -211,7 +213,7 @@ static void write_bdev_super_endio(struct bio *bio) > > static void __write_super(struct cache_sb *sb, struct bio *bio) > { > - struct cache_sb *out = page_address(bio_first_page_all(bio)); > + struct cache_sb_le *out = page_address(bio_first_page_all(bio)); > unsigned i; > > bio->bi_iter.bi_sector = SB_SECTOR; > @@ -959,7 +961,7 @@ void bch_cached_dev_detach(struct cached_dev *dc) > int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c, > uint8_t *set_uuid) > { > - uint32_t rtime = cpu_to_le32(get_seconds()); > + __le32 rtime = cpu_to_le32(get_seconds()); > struct uuid_entry *u; > char buf[BDEVNAME_SIZE]; > > diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h > index 821f71a2e48f..772787c47772 100644 > --- a/include/uapi/linux/bcache.h > +++ b/include/uapi/linux/bcache.h > @@ -154,57 +154,63 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned nr_keys) > > #define BDEV_DATA_START_DEFAULT 16 /* sectors */ > > -struct cache_sb { > - __u64 csum; > - __u64 offset; /* sector where this sb was written */ > - __u64 version; > - > - __u8 magic[16]; > - > - __u8 uuid[16]; > - union { > - __u8 set_uuid[16]; > - __u64 set_magic; > - }; > - __u8 label[SB_LABEL_SIZE]; > - > - __u64 flags; > - __u64 seq; > - __u64 pad[8]; > - > - union { > - struct { > - /* Cache devices */ > - __u64 nbuckets; /* device size */ > - > - __u16 block_size; /* sectors */ > - __u16 bucket_size; /* sectors */ > - > - __u16 nr_in_set; > - __u16 nr_this_dev; > - }; > - struct { > - /* Backing devices */ > - __u64 data_offset; > - > - /* > - * block_size from the cache device section is still used by > - * backing devices, so don't add anything here until we fix > - * things to not need it for backing devices anymore > - */ > - }; > - }; > - > - __u32 last_mount; /* time_t */ > - > - __u16 first_bucket; > - union { > - __u16 njournal_buckets; > - __u16 keys; > - }; > - __u64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ > +#define STRUCT_CACHE_SB(e) \ > +{ \ > + __u64 csum; \ > + /* sector where this sb was written */ \ > + __##e##64 offset; \ > + __##e##64 version; \ > + \ > + __u8 magic[16]; \ > + \ > + __u8 uuid[16]; \ > + union { \ > + __u8 set_uuid[16]; \ > + __##e##64 set_magic; \ > + }; \ > + __u8 label[SB_LABEL_SIZE]; \ > + \ > + __##e##64 flags; \ > + __##e##64 seq; \ > + __##e##64 pad[8]; \ > + \ > + union { \ > + struct { \ > + /* Cache devices */ \ > + __##e##64 nbuckets; /* device size */ \ > + \ > + __##e##16 block_size; /* sectors */ \ > + __##e##16 bucket_size; /* sectors */ \ > + \ > + __##e##16 nr_in_set; \ > + __##e##16 nr_this_dev; \ > + }; \ > + struct { \ > + /* Backing devices */ \ > + __##e##64 data_offset; \ > + \ > + /* \ > + * block_size from the cache device section is still \ > + * used by backing devices, so don't add anything here \ > + * until we fix things to not need it for backing \ > + * devices anymore \ > + */ \ > + }; \ > + }; \ > + \ > + __##e##32 last_mount; /* time_t */ \ > + \ > + __##e##16 first_bucket; \ > + union { \ > + __##e##16 njournal_buckets; \ > + __##e##16 keys; \ > + }; \ > + /* journal buckets */ \ > + __##e##64 d[SB_JOURNAL_BUCKETS]; \ > }; > > +struct cache_sb STRUCT_CACHE_SB(u); > + > static inline _Bool SB_IS_BDEV(const struct cache_sb *sb) > { > return sb->version == BCACHE_SB_VERSION_BDEV > @@ -306,7 +312,7 @@ struct prio_set { > __u64 next_bucket; > > struct bucket_disk { > - __u16 prio; > + __le16 prio; > __u8 gen; > } __attribute((packed)) data[]; > }; > @@ -318,9 +324,9 @@ struct uuid_entry { > struct { > __u8 uuid[16]; > __u8 label[32]; > - __u32 first_reg; > - __u32 last_reg; > - __u32 invalidated; > + __le32 first_reg; > + __le32 last_reg; > + __le32 invalidated; > > __u32 flags; > /* Size of flash only volumes */ > @@ -366,9 +372,9 @@ struct bset { > struct uuid_entry_v0 { > __u8 uuid[16]; > __u8 label[32]; > - __u32 first_reg; > - __u32 last_reg; > - __u32 invalidated; > + __le32 first_reg; > + __le32 last_reg; > + __le32 invalidated; > __u32 pad; > }; > >
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c index 31d700aecd56..ef659c7d72f9 100644 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -63,20 +63,22 @@ struct workqueue_struct *bcache_wq; /* limitation of bcache devices number on single system */ #define BCACHE_DEVICE_IDX_MAX ((1U << MINORBITS)/BCACHE_MINORS) +struct cache_sb_le STRUCT_CACHE_SB(le); + /* Superblock */ static const char *read_super(struct cache_sb *sb, struct block_device *bdev, struct page **res) { const char *err; - struct cache_sb *s; + struct cache_sb_le *s; struct buffer_head *bh = __bread(bdev, 1, SB_SIZE); unsigned i; if (!bh) return "IO error"; - s = (struct cache_sb *) bh->b_data; + s = (struct cache_sb_le *) bh->b_data; sb->offset = le64_to_cpu(s->offset); sb->version = le64_to_cpu(s->version); @@ -211,7 +213,7 @@ static void write_bdev_super_endio(struct bio *bio) static void __write_super(struct cache_sb *sb, struct bio *bio) { - struct cache_sb *out = page_address(bio_first_page_all(bio)); + struct cache_sb_le *out = page_address(bio_first_page_all(bio)); unsigned i; bio->bi_iter.bi_sector = SB_SECTOR; @@ -959,7 +961,7 @@ void bch_cached_dev_detach(struct cached_dev *dc) int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c, uint8_t *set_uuid) { - uint32_t rtime = cpu_to_le32(get_seconds()); + __le32 rtime = cpu_to_le32(get_seconds()); struct uuid_entry *u; char buf[BDEVNAME_SIZE]; diff --git a/include/uapi/linux/bcache.h b/include/uapi/linux/bcache.h index 821f71a2e48f..772787c47772 100644 --- a/include/uapi/linux/bcache.h +++ b/include/uapi/linux/bcache.h @@ -154,57 +154,63 @@ static inline struct bkey *bkey_idx(const struct bkey *k, unsigned nr_keys) #define BDEV_DATA_START_DEFAULT 16 /* sectors */ -struct cache_sb { - __u64 csum; - __u64 offset; /* sector where this sb was written */ - __u64 version; - - __u8 magic[16]; - - __u8 uuid[16]; - union { - __u8 set_uuid[16]; - __u64 set_magic; - }; - __u8 label[SB_LABEL_SIZE]; - - __u64 flags; - __u64 seq; - __u64 pad[8]; - - union { - struct { - /* Cache devices */ - __u64 nbuckets; /* device size */ - - __u16 block_size; /* sectors */ - __u16 bucket_size; /* sectors */ - - __u16 nr_in_set; - __u16 nr_this_dev; - }; - struct { - /* Backing devices */ - __u64 data_offset; - - /* - * block_size from the cache device section is still used by - * backing devices, so don't add anything here until we fix - * things to not need it for backing devices anymore - */ - }; - }; - - __u32 last_mount; /* time_t */ - - __u16 first_bucket; - union { - __u16 njournal_buckets; - __u16 keys; - }; - __u64 d[SB_JOURNAL_BUCKETS]; /* journal buckets */ +#define STRUCT_CACHE_SB(e) \ +{ \ + __u64 csum; \ + /* sector where this sb was written */ \ + __##e##64 offset; \ + __##e##64 version; \ + \ + __u8 magic[16]; \ + \ + __u8 uuid[16]; \ + union { \ + __u8 set_uuid[16]; \ + __##e##64 set_magic; \ + }; \ + __u8 label[SB_LABEL_SIZE]; \ + \ + __##e##64 flags; \ + __##e##64 seq; \ + __##e##64 pad[8]; \ + \ + union { \ + struct { \ + /* Cache devices */ \ + __##e##64 nbuckets; /* device size */ \ + \ + __##e##16 block_size; /* sectors */ \ + __##e##16 bucket_size; /* sectors */ \ + \ + __##e##16 nr_in_set; \ + __##e##16 nr_this_dev; \ + }; \ + struct { \ + /* Backing devices */ \ + __##e##64 data_offset; \ + \ + /* \ + * block_size from the cache device section is still \ + * used by backing devices, so don't add anything here \ + * until we fix things to not need it for backing \ + * devices anymore \ + */ \ + }; \ + }; \ + \ + __##e##32 last_mount; /* time_t */ \ + \ + __##e##16 first_bucket; \ + union { \ + __##e##16 njournal_buckets; \ + __##e##16 keys; \ + }; \ + /* journal buckets */ \ + __##e##64 d[SB_JOURNAL_BUCKETS]; \ }; +struct cache_sb STRUCT_CACHE_SB(u); + static inline _Bool SB_IS_BDEV(const struct cache_sb *sb) { return sb->version == BCACHE_SB_VERSION_BDEV @@ -306,7 +312,7 @@ struct prio_set { __u64 next_bucket; struct bucket_disk { - __u16 prio; + __le16 prio; __u8 gen; } __attribute((packed)) data[]; }; @@ -318,9 +324,9 @@ struct uuid_entry { struct { __u8 uuid[16]; __u8 label[32]; - __u32 first_reg; - __u32 last_reg; - __u32 invalidated; + __le32 first_reg; + __le32 last_reg; + __le32 invalidated; __u32 flags; /* Size of flash only volumes */ @@ -366,9 +372,9 @@ struct bset { struct uuid_entry_v0 { __u8 uuid[16]; __u8 label[32]; - __u32 first_reg; - __u32 last_reg; - __u32 invalidated; + __le32 first_reg; + __le32 last_reg; + __le32 invalidated; __u32 pad; };
This patch avoids that sparse complains about using integer types incorrectly (u32, __le32, __be32, ...) by splitting struct cache_sb into two different structures: - struct cache_sb in which all integer members except csum have CPU endianness. - struct cache_sb_le in which all integer members except csum are declared as little endian. Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> --- drivers/md/bcache/super.c | 10 ++-- include/uapi/linux/bcache.h | 118 +++++++++++++++++++++++--------------------- 2 files changed, 68 insertions(+), 60 deletions(-)