From: David Laight <david.laight.linux@gmail.com>

FIELD_GET needs to use __auto_type to get the value of the 'reg'
parameter, this can't be used with bifields.

FIELD_GET also want to verify the size of 'reg' so can't add zero
to force the type to int.

So add a zero here.

Signed-off-by: David Laight <david.laight.linux@gmail.com>
---
 drivers/thunderbolt/tb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index e96474f17067..7ca2b5a0f01e 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
  */
 static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
 {
-	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
+	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);
 }
 
 /**
-- 
2.39.5

$subject has typo: thunderblot -> thunderbolt ;-)

On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:
> From: David Laight <david.laight.linux@gmail.com>
> 
> FIELD_GET needs to use __auto_type to get the value of the 'reg'
> parameter, this can't be used with bifields.
> 
> FIELD_GET also want to verify the size of 'reg' so can't add zero
> to force the type to int.
> 
> So add a zero here.
> 
> Signed-off-by: David Laight <david.laight.linux@gmail.com>
> ---
>  drivers/thunderbolt/tb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index e96474f17067..7ca2b5a0f01e 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
>   */
>  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
>  {
> -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);

Can't this use a cast instead? If not then can you also add a comment here
because next someone will send a patch "fixing" the unnecessary addition.

>  }
>  
>  /**
> -- 
> 2.39.5

On Wed, 10 Dec 2025 06:56:17 +0100
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> $subject has typo: thunderblot -> thunderbolt ;-)
> 
> On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:
> > From: David Laight <david.laight.linux@gmail.com>
> > 
> > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > parameter, this can't be used with bifields.
> > 
> > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > to force the type to int.
> > 
> > So add a zero here.
> > 
> > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > ---
> >  drivers/thunderbolt/tb.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index e96474f17067..7ca2b5a0f01e 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> >   */
> >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> >  {
> > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);  
> 
> Can't this use a cast instead? If not then can you also add a comment here
> because next someone will send a patch "fixing" the unnecessary addition.

A cast can do other (possibly incorrect) conversions, adding zero is never going
to so any 'damage' - even if it looks a bit odd.

Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
just do:
	/* The major version is in the top 3 bits */
	return sw->config.thunderbolt_version > 5;

The only other uses of thunderbolt_version are debug prints (in decimal).

	David

> 
> >  }
> >  
> >  /**
> > -- 
> > 2.39.5

On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> On Wed, 10 Dec 2025 06:56:17 +0100
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > $subject has typo: thunderblot -> thunderbolt ;-)
> > 
> > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:
> > > From: David Laight <david.laight.linux@gmail.com>
> > > 
> > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > parameter, this can't be used with bifields.
> > > 
> > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > to force the type to int.
> > > 
> > > So add a zero here.
> > > 
> > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > ---
> > >  drivers/thunderbolt/tb.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > index e96474f17067..7ca2b5a0f01e 100644
> > > --- a/drivers/thunderbolt/tb.h
> > > +++ b/drivers/thunderbolt/tb.h
> > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > >   */
> > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > >  {
> > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);  
> > 
> > Can't this use a cast instead? If not then can you also add a comment here
> > because next someone will send a patch "fixing" the unnecessary addition.
> 
> A cast can do other (possibly incorrect) conversions, adding zero is never going
> to so any 'damage' - even if it looks a bit odd.
> 
> Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> just do:
> 	/* The major version is in the top 3 bits */
> 	return sw->config.thunderbolt_version > 5;

You mean 

	return sw->config.thunderbolt_version >> 5;

?

Yes that works but I prefer then:

	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;

> 
> The only other uses of thunderbolt_version are debug prints (in decimal).
> 
> 	David
> 
> > 
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.39.5

On Wed, 10 Dec 2025 10:41:02 +0100
Mika Westerberg <mika.westerberg@linux.intel.com> wrote:

> On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> > On Wed, 10 Dec 2025 06:56:17 +0100
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> >   
> > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > 
> > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:  
> > > > From: David Laight <david.laight.linux@gmail.com>
> > > > 
> > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > parameter, this can't be used with bifields.
> > > > 
> > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > to force the type to int.
> > > > 
> > > > So add a zero here.
> > > > 
> > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > ---
> > > >  drivers/thunderbolt/tb.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > --- a/drivers/thunderbolt/tb.h
> > > > +++ b/drivers/thunderbolt/tb.h
> > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > >   */
> > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > >  {
> > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);    
> > > 
> > > Can't this use a cast instead? If not then can you also add a comment here
> > > because next someone will send a patch "fixing" the unnecessary addition.  
> > 
> > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > to so any 'damage' - even if it looks a bit odd.
> > 
> > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > just do:
> > 	/* The major version is in the top 3 bits */
> > 	return sw->config.thunderbolt_version > 5;  
> 
> You mean 
> 
> 	return sw->config.thunderbolt_version >> 5;
> 
> ?
> 
> Yes that works but I prefer then:
> 
> 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;

I've put that in for the next version (without the comment line).

	David

> 
> > 
> > The only other uses of thunderbolt_version are debug prints (in decimal).
> > 
> > 	David
> >   
> > >   
> > > >  }
> > > >  
> > > >  /**
> > > > -- 
> > > > 2.39.5    
>

On Wed, Dec 10, 2025 at 10:18:42AM +0000, David Laight wrote:
> On Wed, 10 Dec 2025 10:41:02 +0100
> Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> 
> > On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:
> > > On Wed, 10 Dec 2025 06:56:17 +0100
> > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > >   
> > > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > > 
> > > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:  
> > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > 
> > > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > > parameter, this can't be used with bifields.
> > > > > 
> > > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > > to force the type to int.
> > > > > 
> > > > > So add a zero here.
> > > > > 
> > > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > > ---
> > > > >  drivers/thunderbolt/tb.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > > --- a/drivers/thunderbolt/tb.h
> > > > > +++ b/drivers/thunderbolt/tb.h
> > > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > > >   */
> > > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > > >  {
> > > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);    
> > > > 
> > > > Can't this use a cast instead? If not then can you also add a comment here
> > > > because next someone will send a patch "fixing" the unnecessary addition.  
> > > 
> > > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > > to so any 'damage' - even if it looks a bit odd.
> > > 
> > > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > > just do:
> > > 	/* The major version is in the top 3 bits */
> > > 	return sw->config.thunderbolt_version > 5;  
> > 
> > You mean 
> > 
> > 	return sw->config.thunderbolt_version >> 5;
> > 
> > ?
> > 
> > Yes that works but I prefer then:
> > 
> > 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;
> 
> I've put that in for the next version (without the comment line).

FIELD_GET() is here exactly to let people to not opencode this
error-prone bit manipulation. So, let's continue using it.

David, can you explain in details why this code needs to be fixed? Why
and when typecast wouldn't work so that you have to use an ugly '+0'
hack, or even drop the FIELD_GET().

My current understanding is that the existing FIELD_GET()
implementation works well with any data types, including bitfields,
and what you suggested in this series - does not.

If it's correct, I don't think that switching to your version is
well-justified.

Thanks,
Yury

On Wed, 10 Dec 2025 13:13:55 -0500
Yury Norov <yury.norov@gmail.com> wrote:

> On Wed, Dec 10, 2025 at 10:18:42AM +0000, David Laight wrote:
> > On Wed, 10 Dec 2025 10:41:02 +0100
> > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> >   
> > > On Wed, Dec 10, 2025 at 09:34:03AM +0000, David Laight wrote:  
> > > > On Wed, 10 Dec 2025 06:56:17 +0100
> > > > Mika Westerberg <mika.westerberg@linux.intel.com> wrote:
> > > >     
> > > > > $subject has typo: thunderblot -> thunderbolt ;-)
> > > > > 
> > > > > On Tue, Dec 09, 2025 at 10:03:06AM +0000, david.laight.linux@gmail.com wrote:    
> > > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > > 
> > > > > > FIELD_GET needs to use __auto_type to get the value of the 'reg'
> > > > > > parameter, this can't be used with bifields.
> > > > > > 
> > > > > > FIELD_GET also want to verify the size of 'reg' so can't add zero
> > > > > > to force the type to int.
> > > > > > 
> > > > > > So add a zero here.
> > > > > > 
> > > > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > > > ---
> > > > > >  drivers/thunderbolt/tb.h | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > > > > > index e96474f17067..7ca2b5a0f01e 100644
> > > > > > --- a/drivers/thunderbolt/tb.h
> > > > > > +++ b/drivers/thunderbolt/tb.h
> > > > > > @@ -1307,7 +1307,7 @@ static inline struct tb_retimer *tb_to_retimer(struct device *dev)
> > > > > >   */
> > > > > >  static inline unsigned int usb4_switch_version(const struct tb_switch *sw)
> > > > > >  {
> > > > > > -	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version);
> > > > > > +	return FIELD_GET(USB4_VERSION_MAJOR_MASK, sw->config.thunderbolt_version + 0);      
> > > > > 
> > > > > Can't this use a cast instead? If not then can you also add a comment here
> > > > > because next someone will send a patch "fixing" the unnecessary addition.    
> > > > 
> > > > A cast can do other (possibly incorrect) conversions, adding zero is never going
> > > > to so any 'damage' - even if it looks a bit odd.
> > > > 
> > > > Actually, I suspect the best thing here is to delete USB4_VERSION_MAJOR_MASK and
> > > > just do:
> > > > 	/* The major version is in the top 3 bits */
> > > > 	return sw->config.thunderbolt_version > 5;    
> > > 
> > > You mean 
> > > 
> > > 	return sw->config.thunderbolt_version >> 5;
> > > 
> > > ?
> > > 
> > > Yes that works but I prefer then:
> > > 
> > > 	return sw->config.thunderbolt_version >> USB4_VERSION_MAJOR_SHIFT;  
> > 
> > I've put that in for the next version (without the comment line).  
> 
> FIELD_GET() is here exactly to let people to not opencode this
> error-prone bit manipulation. So, let's continue using it.
> 
> David, can you explain in details why this code needs to be fixed? Why
> and when typecast wouldn't work so that you have to use an ugly '+0'
> hack, or even drop the FIELD_GET().
> 
> My current understanding is that the existing FIELD_GET()
> implementation works well with any data types, including bitfields,
> and what you suggested in this series - does not.

The underlying issue is that FIELD_GET() does a check that the supplied 'reg'
field is large enough for the mask.
In this case the 'reg' is a bitfield and you can't use sizeof(), typeof() or
__auto_type on a bitfield.

I don't want to (for example) add zero inside FIELD_GET() (as is done
for 'val') because that would promote u8/u16 to 32 bits - making the
size check less useful.

Ok, the current version relies on how the compiler happens to treat:
	_Generic(foo->bitfield, ...)
clang seems to treat a bitfield as 'int' (so sizeof() the result is 4)
regardless of the width.
OTOH gcc treats 'u32 bits:8' as 'unsigned char', but bits:7 selects
the 'default' and then __unsigned_scaler_typeof() fails.

I don't know what the standard says - but at least one of the compilers
is buggy.

So the current code doesn't really work with bitfields at all.

An alternate change would be to make the 4 fields in DWORD (sic) 4
just u8 instead of u32 xxx:8.
This would be similar to the two u16 at the top.

Perhaps that is the best fix.

	David

> 
> If it's correct, I don't think that switching to your version is
> well-justified.
> 
> Thanks,
> Yury