[PATCH 0/2] bcache: Fix symbol visibility and type assignment

[PATCH 0/2] bcache: Fix symbol visibility and type assignment

[Gabriel Shahrouzi]

Patch 1 ensures bcache_is_reboot is properly declared along with the
other forward declarations.

Patch 2 removes incorrect cpu_to_leXX conversions for assignments.

Gabriel Shahrouzi (2):
  bcache: Fix undeclared symbol warning for bcache_is_reboot
  bcache: Fix warnings for incorrect type in assignments

 drivers/md/bcache/bcache.h |  1 +
 drivers/md/bcache/super.c  | 12 ++++++------
 drivers/md/bcache/sysfs.c  |  2 --
 3 files changed, 7 insertions(+), 8 deletions(-)

-- 
2.43.0

[PATCH 1/2] bcache: Fix undeclared symbol warning for bcache_is_reboot

[Gabriel Shahrouzi]

Add extern declaration for bcache_is_reboot to bcache.h. Ensure proper
visibility for use across multiple files (super.c, sysfs.c) and follow
the declaration pattern for other forward declarations.

Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
---
 drivers/md/bcache/bcache.h | 1 +
 drivers/md/bcache/sysfs.c  | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 785b0d9008fac..531933351b8b8 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -1007,6 +1007,7 @@ extern struct workqueue_struct *bch_journal_wq;
 extern struct workqueue_struct *bch_flush_wq;
 extern struct mutex bch_register_lock;
 extern struct list_head bch_cache_sets;
+extern bool bcache_is_reboot;
 
 extern const struct kobj_type bch_cached_dev_ktype;
 extern const struct kobj_type bch_flash_dev_ktype;
diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
index e8f696cb58c05..47ef0167b9d23 100644
--- a/drivers/md/bcache/sysfs.c
+++ b/drivers/md/bcache/sysfs.c
@@ -17,8 +17,6 @@
 #include <linux/sort.h>
 #include <linux/sched/clock.h>
 
-extern bool bcache_is_reboot;
-
 /* Default is 0 ("writethrough") */
 static const char * const bch_cache_modes[] = {
 	"writethrough",
-- 
2.43.0

Re: [PATCH 1/2] bcache: Fix undeclared symbol warning for bcache_is_reboot

[Coly Li]

On Mon, Apr 07, 2025 at 11:33:21PM +0800, Gabriel Shahrouzi wrote:
> Add extern declaration for bcache_is_reboot to bcache.h. Ensure proper
> visibility for use across multiple files (super.c, sysfs.c) and follow
> the declaration pattern for other forward declarations.
> 
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> ---
>  drivers/md/bcache/bcache.h | 1 +
>  drivers/md/bcache/sysfs.c  | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 785b0d9008fac..531933351b8b8 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -1007,6 +1007,7 @@ extern struct workqueue_struct *bch_journal_wq;
>  extern struct workqueue_struct *bch_flush_wq;
>  extern struct mutex bch_register_lock;
>  extern struct list_head bch_cache_sets;
> +extern bool bcache_is_reboot;
>  

NACK. It is uncessary to make more .c files to be aware of
bcache_is_reboot. Current code is in better form IMHO.


>  extern const struct kobj_type bch_cached_dev_ktype;
>  extern const struct kobj_type bch_flash_dev_ktype;
> diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> index e8f696cb58c05..47ef0167b9d23 100644
> --- a/drivers/md/bcache/sysfs.c
> +++ b/drivers/md/bcache/sysfs.c
> @@ -17,8 +17,6 @@
>  #include <linux/sort.h>
>  #include <linux/sched/clock.h>
>  
> -extern bool bcache_is_reboot;
> -
>  /* Default is 0 ("writethrough") */
>  static const char * const bch_cache_modes[] = {
>  	"writethrough",
> -- 
> 2.43.0
> 

-- 
Coly Li

Re: [PATCH 1/2] bcache: Fix undeclared symbol warning for bcache_is_reboot

[Gabriel Shahrouzi]

On Tue, Apr 8, 2025 at 12:55 AM Coly Li <colyli@kernel.org> wrote:
>
> On Mon, Apr 07, 2025 at 11:33:21PM +0800, Gabriel Shahrouzi wrote:
> > Add extern declaration for bcache_is_reboot to bcache.h. Ensure proper
> > visibility for use across multiple files (super.c, sysfs.c) and follow
> > the declaration pattern for other forward declarations.
> >
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > ---
> >  drivers/md/bcache/bcache.h | 1 +
> >  drivers/md/bcache/sysfs.c  | 2 --
> >  2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> > index 785b0d9008fac..531933351b8b8 100644
> > --- a/drivers/md/bcache/bcache.h
> > +++ b/drivers/md/bcache/bcache.h
> > @@ -1007,6 +1007,7 @@ extern struct workqueue_struct *bch_journal_wq;
> >  extern struct workqueue_struct *bch_flush_wq;
> >  extern struct mutex bch_register_lock;
> >  extern struct list_head bch_cache_sets;
> > +extern bool bcache_is_reboot;
> >
>
> NACK. It is uncessary to make more .c files to be aware of
> bcache_is_reboot. Current code is in better form IMHO.
Ah I see. It's only used in sysfs.c and super.c compared to the other
forward declarations. Limiting the scope to only those files makes the
most sense instead of including it in the header file.
>
>
> >  extern const struct kobj_type bch_cached_dev_ktype;
> >  extern const struct kobj_type bch_flash_dev_ktype;
> > diff --git a/drivers/md/bcache/sysfs.c b/drivers/md/bcache/sysfs.c
> > index e8f696cb58c05..47ef0167b9d23 100644
> > --- a/drivers/md/bcache/sysfs.c
> > +++ b/drivers/md/bcache/sysfs.c
> > @@ -17,8 +17,6 @@
> >  #include <linux/sort.h>
> >  #include <linux/sched/clock.h>
> >
> > -extern bool bcache_is_reboot;
> > -
> >  /* Default is 0 ("writethrough") */
> >  static const char * const bch_cache_modes[] = {
> >       "writethrough",
> > --
> > 2.43.0
> >
>
> --
> Coly Li

[PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

[Gabriel Shahrouzi]

Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
assigning values (priorities, timestamps) to native integer type
members. Prevent incorrect byte ordering for big-endian systems.

Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
---
 drivers/md/bcache/super.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index e42f1400cea9d..c4c5ca17fb600 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
 		for (b = ca->buckets + i * prios_per_bucket(ca);
 		     b < ca->buckets + ca->sb.nbuckets && d < end;
 		     b++, d++) {
-			d->prio = cpu_to_le16(b->prio);
+			d->prio = b->prio;
 			d->gen = b->gen;
 		}
 
@@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
 			d = p->data;
 		}
 
-		b->prio = le16_to_cpu(d->prio);
+		b->prio = d->prio;
 		b->gen = b->last_gc = d->gen;
 	}
 
@@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
 
 		SET_UUID_FLASH_ONLY(u, 0);
 		memcpy(u->uuid, invalid_uuid, 16);
-		u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
+		u->invalidated = (u32)ktime_get_real_seconds();
 		bch_uuid_write(d->c);
 	}
 
@@ -1188,7 +1188,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((u32)ktime_get_real_seconds());
+	uint32_t rtime = (u32)ktime_get_real_seconds();
 	struct uuid_entry *u;
 	struct cached_dev *exist_dc, *t;
 	int ret = 0;
@@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
 	    (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
 	     BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
 		memcpy(u->uuid, invalid_uuid, 16);
-		u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
+		u->invalidated = (u32)ktime_get_real_seconds();
 		u = NULL;
 	}
 
@@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
 
 	get_random_bytes(u->uuid, 16);
 	memset(u->label, 0, 32);
-	u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
+	u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
 
 	SET_UUID_FLASH_ONLY(u, 1);
 	u->sectors = size >> 9;
-- 
2.43.0

Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

[Coly Li]

On Mon, Apr 07, 2025 at 11:33:22PM +0800, Gabriel Shahrouzi wrote:
> Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
> assigning values (priorities, timestamps) to native integer type
> members. Prevent incorrect byte ordering for big-endian systems.
> 

Hmm, why do you feel the conversions are unncessary? Please explain
with details.

I don't mean the modification is correct or incorrect, just want to
see detailed analysis and help me understand in correct why as you
are.

BTW, did you have chance to test your patch on big-endian machine?

Thanks.


> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> ---
>  drivers/md/bcache/super.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index e42f1400cea9d..c4c5ca17fb600 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
>  		for (b = ca->buckets + i * prios_per_bucket(ca);
>  		     b < ca->buckets + ca->sb.nbuckets && d < end;
>  		     b++, d++) {
> -			d->prio = cpu_to_le16(b->prio);
> +			d->prio = b->prio;
>  			d->gen = b->gen;
>  		}
>  
> @@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
>  			d = p->data;
>  		}
>  
> -		b->prio = le16_to_cpu(d->prio);
> +		b->prio = d->prio;
>  		b->gen = b->last_gc = d->gen;
>  	}
>  
> @@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
>  
>  		SET_UUID_FLASH_ONLY(u, 0);
>  		memcpy(u->uuid, invalid_uuid, 16);
> -		u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> +		u->invalidated = (u32)ktime_get_real_seconds();
>  		bch_uuid_write(d->c);
>  	}
>  
> @@ -1188,7 +1188,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((u32)ktime_get_real_seconds());
> +	uint32_t rtime = (u32)ktime_get_real_seconds();
>  	struct uuid_entry *u;
>  	struct cached_dev *exist_dc, *t;
>  	int ret = 0;
> @@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
>  	    (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
>  	     BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
>  		memcpy(u->uuid, invalid_uuid, 16);
> -		u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> +		u->invalidated = (u32)ktime_get_real_seconds();
>  		u = NULL;
>  	}
>  
> @@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
>  
>  	get_random_bytes(u->uuid, 16);
>  	memset(u->label, 0, 32);
> -	u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
> +	u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
>  
>  	SET_UUID_FLASH_ONLY(u, 1);
>  	u->sectors = size >> 9;
> -- 
> 2.43.0
> 

-- 
Coly Li

Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

[Gabriel Shahrouzi]

On Tue, Apr 8, 2025 at 12:58 AM Coly Li <colyli@kernel.org> wrote:
>
> On Mon, Apr 07, 2025 at 11:33:22PM +0800, Gabriel Shahrouzi wrote:
> > Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
> > assigning values (priorities, timestamps) to native integer type
> > members. Prevent incorrect byte ordering for big-endian systems.
> >
>
> Hmm, why do you feel the conversions are unncessary? Please explain
> with details.
I used Sparse for static analysis on bcache and it gave incorrect type
in assignment warnings.

For example:

u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());

ktime_get_real_seconds() returns back u64 and gets casted down to a
u32. u is of type struct uuid_entry whose member fields are either u8,
u32, or u64. A conversion here contradicts the type it should be
assigned.

From my understanding, this would not produce an unexpected result if
the value were to be read from or written to some location which seems
to be the case here. I believe it would only cause issues on
big-endian systems if the value were to be modified in some way.

Looking at the commit history for when the code for this specific
example was first introduced (12 years ago), it seems like this was
the author’s intent. It looks like the intention was to store the
value as little endian in uint32_t. Doing this, the author saves space
/ time. If the type was le32 instead, the conversion would have to be
applied each time it’s used. Alternatively, if another member variable
was defined but for the le32 version, then extra space is used up.

In the unlikely event that these specific files change drastically,
making sure the types are the same serves as a preventative measure
to make sure it’s not misused. On the other hand, making the change
most likely goes against the author’s original intent and could cause
something unintended.
>
> I don't mean the modification is correct or incorrect, just want to
> see detailed analysis and help me understand in correct why as you
> are.
>
> BTW, did you have chance to test your patch on big-endian machine?
I only analyzed the compilation warnings so far. I’ll look into trying
to test this on a big-endian machine.


>
> Thanks.
>
>
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > ---
> >  drivers/md/bcache/super.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > index e42f1400cea9d..c4c5ca17fb600 100644
> > --- a/drivers/md/bcache/super.c
> > +++ b/drivers/md/bcache/super.c
> > @@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
> >               for (b = ca->buckets + i * prios_per_bucket(ca);
> >                    b < ca->buckets + ca->sb.nbuckets && d < end;
> >                    b++, d++) {
> > -                     d->prio = cpu_to_le16(b->prio);
> > +                     d->prio = b->prio;
> >                       d->gen = b->gen;
> >               }
> >
> > @@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
> >                       d = p->data;
> >               }
> >
> > -             b->prio = le16_to_cpu(d->prio);
> > +             b->prio = d->prio;
> >               b->gen = b->last_gc = d->gen;
> >       }
> >
> > @@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
> >
> >               SET_UUID_FLASH_ONLY(u, 0);
> >               memcpy(u->uuid, invalid_uuid, 16);
> > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > +             u->invalidated = (u32)ktime_get_real_seconds();
> >               bch_uuid_write(d->c);
> >       }
> >
> > @@ -1188,7 +1188,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((u32)ktime_get_real_seconds());
> > +     uint32_t rtime = (u32)ktime_get_real_seconds();
> >       struct uuid_entry *u;
> >       struct cached_dev *exist_dc, *t;
> >       int ret = 0;
> > @@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> >           (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
> >            BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
> >               memcpy(u->uuid, invalid_uuid, 16);
> > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > +             u->invalidated = (u32)ktime_get_real_seconds();
> >               u = NULL;
> >       }
> >
> > @@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
> >
> >       get_random_bytes(u->uuid, 16);
> >       memset(u->label, 0, 32);
> > -     u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
> > +     u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
> >
> >       SET_UUID_FLASH_ONLY(u, 1);
> >       u->sectors = size >> 9;
> > --
> > 2.43.0
> >
>
> --
> Coly Li

Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

[Coly Li]

On Tue, Apr 08, 2025 at 03:15:00AM +0800, Gabriel Shahrouzi wrote:
> On Tue, Apr 8, 2025 at 12:58 AM Coly Li <colyli@kernel.org> wrote:
> >
> > On Mon, Apr 07, 2025 at 11:33:22PM +0800, Gabriel Shahrouzi wrote:
> > > Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
> > > assigning values (priorities, timestamps) to native integer type
> > > members. Prevent incorrect byte ordering for big-endian systems.
> > >
> >
> > Hmm, why do you feel the conversions are unncessary? Please explain
> > with details.
> I used Sparse for static analysis on bcache and it gave incorrect type
> in assignment warnings.
> 
> For example:
> 
> u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> 
> ktime_get_real_seconds() returns back u64 and gets casted down to a
> u32. u is of type struct uuid_entry whose member fields are either u8,
> u32, or u64. A conversion here contradicts the type it should be
> assigned.
> 
> From my understanding, this would not produce an unexpected result if
> the value were to be read from or written to some location which seems
> to be the case here. I believe it would only cause issues on
> big-endian systems if the value were to be modified in some way.
> 

Yes you are right, and I agree with you.


> Looking at the commit history for when the code for this specific
> example was first introduced (12 years ago), it seems like this was
> the author’s intent. It looks like the intention was to store the
> value as little endian in uint32_t. Doing this, the author saves space
> / time. If the type was le32 instead, the conversion would have to be
> applied each time it’s used. Alternatively, if another member variable
> was defined but for the le32 version, then extra space is used up.
> 

This is kind of convention that on-media values are stored in little
endian, for portablity purpose. But bcache is special, current
implementation and usage don't require/support portability on different
byte order machines. So cpu_to/from_le** routines are almostly
unnecessary indeed.

*BUT* the cast (u32) works as expected on big endian machine as well,
same result generated as little indian machine does. The out-of-order
issue on big endian machine for the code you mentioned won't happen. 

> In the unlikely event that these specific files change drastically,
> making sure the types are the same serves as a preventative measure
> to make sure it’s not misused. On the other hand, making the change
> most likely goes against the author’s original intent and could cause
> something unintended.
> >
> > I don't mean the modification is correct or incorrect, just want to
> > see detailed analysis and help me understand in correct why as you
> > are.
> >
> > BTW, did you have chance to test your patch on big-endian machine?
> I only analyzed the compilation warnings so far. I’ll look into trying
> to test this on a big-endian machine.
> 
> 

You may have a try and verify my statement.

And for the change in bch_prio_write(), this is something out of your
orignal scope of this patch. The prio width is 16bits, byte order and
length truncation issue doesn't apply here.

After all, no mather the cpu_to_le*() or le*_to_cpu() routines are used
or not, the code works well. Because bcache cache device dosn't port
between big and little endian machines.

I don't want to unify the code to all use cpu_to_le*() routines or
remove all these routines, both sides make sense and resonable.
IMHO they are just changes for changing. So I intend to keep it as what
Kent orignally wrote it.

Thanks.

> > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > > ---
> > >  drivers/md/bcache/super.c | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > > index e42f1400cea9d..c4c5ca17fb600 100644
> > > --- a/drivers/md/bcache/super.c
> > > +++ b/drivers/md/bcache/super.c
> > > @@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
> > >               for (b = ca->buckets + i * prios_per_bucket(ca);
> > >                    b < ca->buckets + ca->sb.nbuckets && d < end;
> > >                    b++, d++) {
> > > -                     d->prio = cpu_to_le16(b->prio);
> > > +                     d->prio = b->prio;
> > >                       d->gen = b->gen;
> > >               }
> > >
> > > @@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
> > >                       d = p->data;
> > >               }
> > >
> > > -             b->prio = le16_to_cpu(d->prio);
> > > +             b->prio = d->prio;
> > >               b->gen = b->last_gc = d->gen;
> > >       }
> > >
> > > @@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
> > >
> > >               SET_UUID_FLASH_ONLY(u, 0);
> > >               memcpy(u->uuid, invalid_uuid, 16);
> > > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > > +             u->invalidated = (u32)ktime_get_real_seconds();
> > >               bch_uuid_write(d->c);
> > >       }
> > >
> > > @@ -1188,7 +1188,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((u32)ktime_get_real_seconds());
> > > +     uint32_t rtime = (u32)ktime_get_real_seconds();
> > >       struct uuid_entry *u;
> > >       struct cached_dev *exist_dc, *t;
> > >       int ret = 0;
> > > @@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> > >           (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
> > >            BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
> > >               memcpy(u->uuid, invalid_uuid, 16);
> > > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > > +             u->invalidated = (u32)ktime_get_real_seconds();
> > >               u = NULL;
> > >       }
> > >
> > > @@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
> > >
> > >       get_random_bytes(u->uuid, 16);
> > >       memset(u->label, 0, 32);
> > > -     u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
> > > +     u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
> > >
> > >       SET_UUID_FLASH_ONLY(u, 1);
> > >       u->sectors = size >> 9;
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Coly Li
> 

-- 
Coly Li

Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

[Gabriel Shahrouzi]

On Tue, Apr 8, 2025 at 6:39 AM Coly Li <colyli@kernel.org> wrote:
>
> On Tue, Apr 08, 2025 at 03:15:00AM +0800, Gabriel Shahrouzi wrote:
> > On Tue, Apr 8, 2025 at 12:58 AM Coly Li <colyli@kernel.org> wrote:
> > >
> > > On Mon, Apr 07, 2025 at 11:33:22PM +0800, Gabriel Shahrouzi wrote:
> > > > Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
> > > > assigning values (priorities, timestamps) to native integer type
> > > > members. Prevent incorrect byte ordering for big-endian systems.
> > > >
> > >
> > > Hmm, why do you feel the conversions are unncessary? Please explain
> > > with details.
> > I used Sparse for static analysis on bcache and it gave incorrect type
> > in assignment warnings.
> >
> > For example:
> >
> > u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> >
> > ktime_get_real_seconds() returns back u64 and gets casted down to a
> > u32. u is of type struct uuid_entry whose member fields are either u8,
> > u32, or u64. A conversion here contradicts the type it should be
> > assigned.
> >
> > From my understanding, this would not produce an unexpected result if
> > the value were to be read from or written to some location which seems
> > to be the case here. I believe it would only cause issues on
> > big-endian systems if the value were to be modified in some way.
> >
>
> Yes you are right, and I agree with you.
>
>
> > Looking at the commit history for when the code for this specific
> > example was first introduced (12 years ago), it seems like this was
> > the author’s intent. It looks like the intention was to store the
> > value as little endian in uint32_t. Doing this, the author saves space
> > / time. If the type was le32 instead, the conversion would have to be
> > applied each time it’s used. Alternatively, if another member variable
> > was defined but for the le32 version, then extra space is used up.
> >
>
> This is kind of convention that on-media values are stored in little
> endian, for portablity purpose. But bcache is special, current
> implementation and usage don't require/support portability on different
> byte order machines. So cpu_to/from_le** routines are almostly
> unnecessary indeed.
>
> *BUT* the cast (u32) works as expected on big endian machine as well,
> same result generated as little indian machine does. The out-of-order
> issue on big endian machine for the code you mentioned won't happen.
Got it.
>
> > In the unlikely event that these specific files change drastically,
> > making sure the types are the same serves as a preventative measure
> > to make sure it’s not misused. On the other hand, making the change
> > most likely goes against the author’s original intent and could cause
> > something unintended.
> > >
> > > I don't mean the modification is correct or incorrect, just want to
> > > see detailed analysis and help me understand in correct why as you
> > > are.
> > >
> > > BTW, did you have chance to test your patch on big-endian machine?
> > I only analyzed the compilation warnings so far. I’ll look into trying
> > to test this on a big-endian machine.
> >
> >
>
> You may have a try and verify my statement.
>
> And for the change in bch_prio_write(), this is something out of your
> orignal scope of this patch. The prio width is 16bits, byte order and
> length truncation issue doesn't apply here.
Ah I should have been more clear about this when explaining. My main
concern was with the endian conversion which is what prompted me to
group them together.
>
> After all, no mather the cpu_to_le*() or le*_to_cpu() routines are used
> or not, the code works well. Because bcache cache device dosn't port
> between big and little endian machines.
Ah ok this makes sense when considering the use case of bcache.
>
> I don't want to unify the code to all use cpu_to_le*() routines or
> remove all these routines, both sides make sense and resonable.
> IMHO they are just changes for changing. So I intend to keep it as what
> Kent orignally wrote it.
Makes sense.
>
> Thanks.
>
> > > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > > > ---
> > > >  drivers/md/bcache/super.c | 12 ++++++------
> > > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > > > index e42f1400cea9d..c4c5ca17fb600 100644
> > > > --- a/drivers/md/bcache/super.c
> > > > +++ b/drivers/md/bcache/super.c
> > > > @@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
> > > >               for (b = ca->buckets + i * prios_per_bucket(ca);
> > > >                    b < ca->buckets + ca->sb.nbuckets && d < end;
> > > >                    b++, d++) {
> > > > -                     d->prio = cpu_to_le16(b->prio);
> > > > +                     d->prio = b->prio;
> > > >                       d->gen = b->gen;
> > > >               }
> > > >
> > > > @@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
> > > >                       d = p->data;
> > > >               }
> > > >
> > > > -             b->prio = le16_to_cpu(d->prio);
> > > > +             b->prio = d->prio;
> > > >               b->gen = b->last_gc = d->gen;
> > > >       }
> > > >
> > > > @@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
> > > >
> > > >               SET_UUID_FLASH_ONLY(u, 0);
> > > >               memcpy(u->uuid, invalid_uuid, 16);
> > > > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > > > +             u->invalidated = (u32)ktime_get_real_seconds();
> > > >               bch_uuid_write(d->c);
> > > >       }
> > > >
> > > > @@ -1188,7 +1188,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((u32)ktime_get_real_seconds());
> > > > +     uint32_t rtime = (u32)ktime_get_real_seconds();
> > > >       struct uuid_entry *u;
> > > >       struct cached_dev *exist_dc, *t;
> > > >       int ret = 0;
> > > > @@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> > > >           (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
> > > >            BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
> > > >               memcpy(u->uuid, invalid_uuid, 16);
> > > > -             u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > > > +             u->invalidated = (u32)ktime_get_real_seconds();
> > > >               u = NULL;
> > > >       }
> > > >
> > > > @@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
> > > >
> > > >       get_random_bytes(u->uuid, 16);
> > > >       memset(u->label, 0, 32);
> > > > -     u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
> > > > +     u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
> > > >
> > > >       SET_UUID_FLASH_ONLY(u, 1);
> > > >       u->sectors = size >> 9;
> > > > --
> > > > 2.43.0
> > > >
> > >
> > > --
> > > Coly Li
> >
>
> --
> Coly Li