[PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[Gil Fine]

DROM for USB4 host/device has a shorter header than Thunderbolt DROM
header. This patch addresses host/device with USB4 DROM (According to spec:
Universal Serial Bus 4 (USB4) Device ROM Specification, Rev 1.0, Feb-2021).

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/eeprom.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
index 46d0906a3070..f9d26bd4f486 100644
--- a/drivers/thunderbolt/eeprom.c
+++ b/drivers/thunderbolt/eeprom.c
@@ -214,7 +214,10 @@ static u32 tb_crc32(void *data, size_t len)
 	return ~__crc32c_le(~0, data, len);
 }
 
-#define TB_DROM_DATA_START 13
+#define TB_DROM_DATA_START		13
+#define TB_DROM_HEADER_LENGTH		22
+/* BYTES 16-21 - nonexistent in USB4 DROM */
+#define TB_DROM_USB4_HEADER_LENGTH	16
 struct tb_drom_header {
 	/* BYTE 0 */
 	u8 uid_crc8; /* checksum for uid */
@@ -224,9 +227,9 @@ struct tb_drom_header {
 	u32 data_crc32; /* checksum for data_len bytes starting at byte 13 */
 	/* BYTE 13 */
 	u8 device_rom_revision; /* should be <= 1 */
-	u16 data_len:10;
-	u8 __unknown1:6;
-	/* BYTES 16-21 */
+	u16 data_len:12;
+	u8 reserved:4;
+	/* BYTES 16-21 - Only for TBT DROM, nonexistent in USB4 DROM */
 	u16 vendor_id;
 	u16 model_id;
 	u8 model_rev;
@@ -401,10 +404,10 @@ static int tb_drom_parse_entry_port(struct tb_switch *sw,
  *
  * Drom must have been copied to sw->drom.
  */
-static int tb_drom_parse_entries(struct tb_switch *sw)
+static int tb_drom_parse_entries(struct tb_switch *sw, size_t header_length)
 {
 	struct tb_drom_header *header = (void *) sw->drom;
-	u16 pos = sizeof(*header);
+	u16 pos = header_length;
 	u16 drom_size = header->data_len + TB_DROM_DATA_START;
 	int res;
 
@@ -566,7 +569,7 @@ static int tb_drom_parse(struct tb_switch *sw)
 			header->data_crc32, crc);
 	}
 
-	return tb_drom_parse_entries(sw);
+	return tb_drom_parse_entries(sw, TB_DROM_HEADER_LENGTH);
 }
 
 static int usb4_drom_parse(struct tb_switch *sw)
@@ -583,7 +586,7 @@ static int usb4_drom_parse(struct tb_switch *sw)
 		return -EINVAL;
 	}
 
-	return tb_drom_parse_entries(sw);
+	return tb_drom_parse_entries(sw, TB_DROM_USB4_HEADER_LENGTH);
 }
 
 /**
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[Greg KH]

On Mon, Jun 14, 2021 at 04:52:10PM +0300, Gil Fine wrote:
> DROM for USB4 host/device has a shorter header than Thunderbolt DROM
> header. This patch addresses host/device with USB4 DROM (According to spec:
> Universal Serial Bus 4 (USB4) Device ROM Specification, Rev 1.0, Feb-2021).
> 
> Signed-off-by: Gil Fine <gil.fine@intel.com>
> ---
>  drivers/thunderbolt/eeprom.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> index 46d0906a3070..f9d26bd4f486 100644
> --- a/drivers/thunderbolt/eeprom.c
> +++ b/drivers/thunderbolt/eeprom.c
> @@ -214,7 +214,10 @@ static u32 tb_crc32(void *data, size_t len)
>  	return ~__crc32c_le(~0, data, len);
>  }
>  
> -#define TB_DROM_DATA_START 13
> +#define TB_DROM_DATA_START		13
> +#define TB_DROM_HEADER_LENGTH		22
> +/* BYTES 16-21 - nonexistent in USB4 DROM */
> +#define TB_DROM_USB4_HEADER_LENGTH	16
>  struct tb_drom_header {
>  	/* BYTE 0 */
>  	u8 uid_crc8; /* checksum for uid */
> @@ -224,9 +227,9 @@ struct tb_drom_header {
>  	u32 data_crc32; /* checksum for data_len bytes starting at byte 13 */
>  	/* BYTE 13 */
>  	u8 device_rom_revision; /* should be <= 1 */
> -	u16 data_len:10;
> -	u8 __unknown1:6;
> -	/* BYTES 16-21 */
> +	u16 data_len:12;
> +	u8 reserved:4;
> +	/* BYTES 16-21 - Only for TBT DROM, nonexistent in USB4 DROM */

What is the odds the above does not work properly for big endian
systems?

And why put the comment after the area and not before?

thanks,

greg k-h

Re: [PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[Mika Westerberg]

Hi Greg,

On Mon, Jun 14, 2021 at 04:06:30PM +0200, Greg KH wrote:
> On Mon, Jun 14, 2021 at 04:52:10PM +0300, Gil Fine wrote:
> > DROM for USB4 host/device has a shorter header than Thunderbolt DROM
> > header. This patch addresses host/device with USB4 DROM (According to spec:
> > Universal Serial Bus 4 (USB4) Device ROM Specification, Rev 1.0, Feb-2021).
> > 
> > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > ---
> >  drivers/thunderbolt/eeprom.c | 19 +++++++++++--------
> >  1 file changed, 11 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> > index 46d0906a3070..f9d26bd4f486 100644
> > --- a/drivers/thunderbolt/eeprom.c
> > +++ b/drivers/thunderbolt/eeprom.c
> > @@ -214,7 +214,10 @@ static u32 tb_crc32(void *data, size_t len)
> >  	return ~__crc32c_le(~0, data, len);
> >  }
> >  
> > -#define TB_DROM_DATA_START 13
> > +#define TB_DROM_DATA_START		13
> > +#define TB_DROM_HEADER_LENGTH		22
> > +/* BYTES 16-21 - nonexistent in USB4 DROM */
> > +#define TB_DROM_USB4_HEADER_LENGTH	16
> >  struct tb_drom_header {
> >  	/* BYTE 0 */
> >  	u8 uid_crc8; /* checksum for uid */
> > @@ -224,9 +227,9 @@ struct tb_drom_header {
> >  	u32 data_crc32; /* checksum for data_len bytes starting at byte 13 */
> >  	/* BYTE 13 */
> >  	u8 device_rom_revision; /* should be <= 1 */
> > -	u16 data_len:10;
> > -	u8 __unknown1:6;
> > -	/* BYTES 16-21 */
> > +	u16 data_len:12;
> > +	u8 reserved:4;
> > +	/* BYTES 16-21 - Only for TBT DROM, nonexistent in USB4 DROM */
> 
> What is the odds the above does not work properly for big endian
> systems?

If you mean the bitfields, we have been trying to get rid of them. Any
new code is expected not to introduce new structures like this but it
has been OK for existing structures (for now).

> And why put the comment after the area and not before?

The gap is there after the "reserved" field.

Thanks!

Re: [PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[Greg KH]

On Mon, Jun 14, 2021 at 05:59:35PM +0300, Mika Westerberg wrote:
> Hi Greg,
> 
> On Mon, Jun 14, 2021 at 04:06:30PM +0200, Greg KH wrote:
> > On Mon, Jun 14, 2021 at 04:52:10PM +0300, Gil Fine wrote:
> > > DROM for USB4 host/device has a shorter header than Thunderbolt DROM
> > > header. This patch addresses host/device with USB4 DROM (According to spec:
> > > Universal Serial Bus 4 (USB4) Device ROM Specification, Rev 1.0, Feb-2021).
> > > 
> > > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > > ---
> > >  drivers/thunderbolt/eeprom.c | 19 +++++++++++--------
> > >  1 file changed, 11 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> > > index 46d0906a3070..f9d26bd4f486 100644
> > > --- a/drivers/thunderbolt/eeprom.c
> > > +++ b/drivers/thunderbolt/eeprom.c
> > > @@ -214,7 +214,10 @@ static u32 tb_crc32(void *data, size_t len)
> > >  	return ~__crc32c_le(~0, data, len);
> > >  }
> > >  
> > > -#define TB_DROM_DATA_START 13
> > > +#define TB_DROM_DATA_START		13
> > > +#define TB_DROM_HEADER_LENGTH		22
> > > +/* BYTES 16-21 - nonexistent in USB4 DROM */
> > > +#define TB_DROM_USB4_HEADER_LENGTH	16
> > >  struct tb_drom_header {
> > >  	/* BYTE 0 */
> > >  	u8 uid_crc8; /* checksum for uid */
> > > @@ -224,9 +227,9 @@ struct tb_drom_header {
> > >  	u32 data_crc32; /* checksum for data_len bytes starting at byte 13 */
> > >  	/* BYTE 13 */
> > >  	u8 device_rom_revision; /* should be <= 1 */
> > > -	u16 data_len:10;
> > > -	u8 __unknown1:6;
> > > -	/* BYTES 16-21 */
> > > +	u16 data_len:12;
> > > +	u8 reserved:4;
> > > +	/* BYTES 16-21 - Only for TBT DROM, nonexistent in USB4 DROM */
> > 
> > What is the odds the above does not work properly for big endian
> > systems?
> 
> If you mean the bitfields, we have been trying to get rid of them. Any
> new code is expected not to introduce new structures like this but it
> has been OK for existing structures (for now).

Ok, as long as you all realize this is broken :)

> > And why put the comment after the area and not before?
> 
> The gap is there after the "reserved" field.

Ah, I read the patch wrong, sorry, my fault.

greg k-h

Re: [PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[Mika Westerberg]

On Mon, Jun 14, 2021 at 05:21:06PM +0200, Greg KH wrote:
> On Mon, Jun 14, 2021 at 05:59:35PM +0300, Mika Westerberg wrote:
> > Hi Greg,
> > 
> > On Mon, Jun 14, 2021 at 04:06:30PM +0200, Greg KH wrote:
> > > On Mon, Jun 14, 2021 at 04:52:10PM +0300, Gil Fine wrote:
> > > > DROM for USB4 host/device has a shorter header than Thunderbolt DROM
> > > > header. This patch addresses host/device with USB4 DROM (According to spec:
> > > > Universal Serial Bus 4 (USB4) Device ROM Specification, Rev 1.0, Feb-2021).
> > > > 
> > > > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > > > ---
> > > >  drivers/thunderbolt/eeprom.c | 19 +++++++++++--------
> > > >  1 file changed, 11 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/thunderbolt/eeprom.c b/drivers/thunderbolt/eeprom.c
> > > > index 46d0906a3070..f9d26bd4f486 100644
> > > > --- a/drivers/thunderbolt/eeprom.c
> > > > +++ b/drivers/thunderbolt/eeprom.c
> > > > @@ -214,7 +214,10 @@ static u32 tb_crc32(void *data, size_t len)
> > > >  	return ~__crc32c_le(~0, data, len);
> > > >  }
> > > >  
> > > > -#define TB_DROM_DATA_START 13
> > > > +#define TB_DROM_DATA_START		13
> > > > +#define TB_DROM_HEADER_LENGTH		22
> > > > +/* BYTES 16-21 - nonexistent in USB4 DROM */
> > > > +#define TB_DROM_USB4_HEADER_LENGTH	16
> > > >  struct tb_drom_header {
> > > >  	/* BYTE 0 */
> > > >  	u8 uid_crc8; /* checksum for uid */
> > > > @@ -224,9 +227,9 @@ struct tb_drom_header {
> > > >  	u32 data_crc32; /* checksum for data_len bytes starting at byte 13 */
> > > >  	/* BYTE 13 */
> > > >  	u8 device_rom_revision; /* should be <= 1 */
> > > > -	u16 data_len:10;
> > > > -	u8 __unknown1:6;
> > > > -	/* BYTES 16-21 */
> > > > +	u16 data_len:12;
> > > > +	u8 reserved:4;
> > > > +	/* BYTES 16-21 - Only for TBT DROM, nonexistent in USB4 DROM */
> > > 
> > > What is the odds the above does not work properly for big endian
> > > systems?
> > 
> > If you mean the bitfields, we have been trying to get rid of them. Any
> > new code is expected not to introduce new structures like this but it
> > has been OK for existing structures (for now).
> 
> Ok, as long as you all realize this is broken :)

Yes we do :) This is one of the things that need to be sorted out once
we have all the "features" in place.

Re: [PATCH] thunderbolt: Fix DROM handling for USB4 DROM

[Mika Westerberg]

On Mon, Jun 14, 2021 at 04:52:10PM +0300, Gil Fine wrote:
> DROM for USB4 host/device has a shorter header than Thunderbolt DROM
> header. This patch addresses host/device with USB4 DROM (According to spec:
> Universal Serial Bus 4 (USB4) Device ROM Specification, Rev 1.0, Feb-2021).
> 
> Signed-off-by: Gil Fine <gil.fine@intel.com>

I did some minor tweaks and applied to thunderbolt.git/next, thanks!