linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* staging: r8188eu: struct rt_firmware_header issues
@ 2022-04-13 16:27 Michael Straube
  2022-04-13 19:42 ` Michael Straube
  2022-04-13 20:58 ` Pavel Skripkin
  0 siblings, 2 replies; 9+ messages in thread
From: Michael Straube @ 2022-04-13 16:27 UTC (permalink / raw)
  To: Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List, straube.linux

Hi all,

I think the rt_firmware_hdr structure in rtw_fw.c has some issues.


struct rt_firmware_hdr {
	/*  8-byte alinment required */
	/*  LONG WORD 0 ---- */
	__le16		Signature;	/* 92C0: test chip; 92C,
					 * 88C0: test chip; 88C1: MP A-cut;
					 * 92C1: MP A-cut */
	u8		Category;	/*  AP/NIC and USB/PCI */
	u8		Function;	/*  Reserved for different FW function
					 *  indcation, for further use when
					 *  driver needs to download different
					 *  FW for different conditions */
	__le16		Version;	/*  FW Version */
	u8		Subversion;	/*  FW Subversion, default 0x00 */
	u16		Rsvd1;

	/*  LONG WORD 1 ---- */
	u8		Month;	/*  Release time Month field */
	u8		Date;	/*  Release time Date field */
	u8		Hour;	/*  Release time Hour field */
	u8		Minute;	/*  Release time Minute field */
	__le16		RamCodeSize;	/*  The size of RAM code */
	u8		Foundry;
	u8		Rsvd2;

	/*  LONG WORD 2 ---- */
	__le32		SvnIdx;	/*  The SVN entry index */
	u32		Rsvd3;

	/*  LONG WORD 3 ---- */
	u32		Rsvd4;
	u32		Rsvd5;
};


Then we have in rtl8188e_firmware_download():


	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;

	<snip>

	if (IS_FW_HEADER_EXIST(fwhdr)) {
		/*  Shift 32 bytes for FW header */
		fw_data = fw_data + 32;
		fw_size = fw_size - 32;
	}

We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
33 bytes. I noticed this when I wanted to replace:

		fw_data = fw_data + 32;
		fw_size = fw_size - 32;

with:
		fw_data = fw_data + sizeof(struct rt_firmware_hdr);
		fw_size = fw_size - sizeof(struct rt_firmware_hdr);;

To me it looks add/sub 32 is correct here but the struct is
wrong. I don't know if the firmware for this driver is so much different
from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
They use a struct of size 32.

Also, souldn't the u16 and u32 variables in the struct be __le16 and
__le32 ?

I wonder if we can just use the rtlwifi_firmware_header structure from
drivers/net/wireless/realtek/rtlwifi/wifi.h ?

Comments from people with better knowledge appreciated. :)

regards,
Michael



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: staging: r8188eu: struct rt_firmware_header issues
  2022-04-13 16:27 staging: r8188eu: struct rt_firmware_header issues Michael Straube
@ 2022-04-13 19:42 ` Michael Straube
  2022-04-13 21:12   ` Larry Finger
  2022-04-14  8:41   ` David Laight
  2022-04-13 20:58 ` Pavel Skripkin
  1 sibling, 2 replies; 9+ messages in thread
From: Michael Straube @ 2022-04-13 19:42 UTC (permalink / raw)
  To: Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

On 4/13/22 18:27, Michael Straube wrote:
> Hi all,
> 
> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
> 
> 
> struct rt_firmware_hdr {
>      /*  8-byte alinment required */
>      /*  LONG WORD 0 ---- */
>      __le16        Signature;    /* 92C0: test chip; 92C,
>                       * 88C0: test chip; 88C1: MP A-cut;
>                       * 92C1: MP A-cut */
>      u8        Category;    /*  AP/NIC and USB/PCI */
>      u8        Function;    /*  Reserved for different FW function
>                       *  indcation, for further use when
>                       *  driver needs to download different
>                       *  FW for different conditions */
>      __le16        Version;    /*  FW Version */
>      u8        Subversion;    /*  FW Subversion, default 0x00 */
>      u16        Rsvd1;
> 
>      /*  LONG WORD 1 ---- */
>      u8        Month;    /*  Release time Month field */
>      u8        Date;    /*  Release time Date field */
>      u8        Hour;    /*  Release time Hour field */
>      u8        Minute;    /*  Release time Minute field */
>      __le16        RamCodeSize;    /*  The size of RAM code */
>      u8        Foundry;
>      u8        Rsvd2;
> 
>      /*  LONG WORD 2 ---- */
>      __le32        SvnIdx;    /*  The SVN entry index */
>      u32        Rsvd3;
> 
>      /*  LONG WORD 3 ---- */
>      u32        Rsvd4;
>      u32        Rsvd5;
> };
> 
> 
> Then we have in rtl8188e_firmware_download():
> 
> 
>      fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
> 
>      <snip>
> 
>      if (IS_FW_HEADER_EXIST(fwhdr)) {
>          /*  Shift 32 bytes for FW header */
>          fw_data = fw_data + 32;
>          fw_size = fw_size - 32;
>      }
> 
> We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
> 33 bytes. I noticed this when I wanted to replace:
> 
>          fw_data = fw_data + 32;
>          fw_size = fw_size - 32;
> 
> with:
>          fw_data = fw_data + sizeof(struct rt_firmware_hdr);
>          fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
> 
> To me it looks add/sub 32 is correct here but the struct is
> wrong. I don't know if the firmware for this driver is so much different
> from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
> They use a struct of size 32.
> 
> Also, souldn't the u16 and u32 variables in the struct be __le16 and
> __le32 ?
> 
> I wonder if we can just use the rtlwifi_firmware_header structure from
> drivers/net/wireless/realtek/rtlwifi/wifi.h ?
> 
> Comments from people with better knowledge appreciated. :)
> 
> regards,
> Michael
> 
> 

Ok, I figured it out by looking at the hexdumps of firmware files. The
field Rsvd1 should be u8 instead of u16. I'll prepare a patch for this.

regards,
Michael

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: staging: r8188eu: struct rt_firmware_header issues
  2022-04-13 16:27 staging: r8188eu: struct rt_firmware_header issues Michael Straube
  2022-04-13 19:42 ` Michael Straube
@ 2022-04-13 20:58 ` Pavel Skripkin
  2022-04-13 21:26   ` Michael Straube
  1 sibling, 1 reply; 9+ messages in thread
From: Pavel Skripkin @ 2022-04-13 20:58 UTC (permalink / raw)
  To: Michael Straube, Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

Hi Michael,

On 4/13/22 19:27, Michael Straube wrote:
> Hi all,
> 
> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
> 
> 
> struct rt_firmware_hdr {
> 	/*  8-byte alinment required */
> 	/*  LONG WORD 0 ---- */
> 	__le16		Signature;	/* 92C0: test chip; 92C,
> 					 * 88C0: test chip; 88C1: MP A-cut;
> 					 * 92C1: MP A-cut */
> 	u8		Category;	/*  AP/NIC and USB/PCI */
> 	u8		Function;	/*  Reserved for different FW function
> 					 *  indcation, for further use when
> 					 *  driver needs to download different
> 					 *  FW for different conditions */
> 	__le16		Version;	/*  FW Version */
> 	u8		Subversion;	/*  FW Subversion, default 0x00 */
> 	u16		Rsvd1;
> 
> 	/*  LONG WORD 1 ---- */
> 	u8		Month;	/*  Release time Month field */
> 	u8		Date;	/*  Release time Date field */
> 	u8		Hour;	/*  Release time Hour field */
> 	u8		Minute;	/*  Release time Minute field */
> 	__le16		RamCodeSize;	/*  The size of RAM code */
> 	u8		Foundry;
> 	u8		Rsvd2;
> 
> 	/*  LONG WORD 2 ---- */
> 	__le32		SvnIdx;	/*  The SVN entry index */
> 	u32		Rsvd3;
> 
> 	/*  LONG WORD 3 ---- */
> 	u32		Rsvd4;
> 	u32		Rsvd5;
> };
> 
> 
> Then we have in rtl8188e_firmware_download():
> 
> 
> 	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
> 
> 	<snip>
> 
> 	if (IS_FW_HEADER_EXIST(fwhdr)) {
> 		/*  Shift 32 bytes for FW header */
> 		fw_data = fw_data + 32;
> 		fw_size = fw_size - 32;
> 	}
> 
> We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
> 33 bytes. I noticed this when I wanted to replace:
> 

Looks like size of that structure is 36

└──$ pahole -C rt_firmware_hdr r8188eu.ko
struct rt_firmware_hdr {
	__le16                     Signature;            /*     0     2 */
	u8                         Category;             /*     2     1 */
	u8                         Function;             /*     3     1 */
	__le16                     Version;              /*     4     2 */
  	u8                         Subversion;           /*     6     1 */

  	/* XXX 1 byte hole, try to pack */

  	u16                        Rsvd1;                /*     8     2 */
  	u8                         Month;                /*    10     1 */
  	u8                         Date;                 /*    11     1 */
  	u8                         Hour;                 /*    12     1 */
  	u8                         Minute;               /*    13     1 */
  	__le16                     RamCodeSize;          /*    14     2 */
  	u8                         Foundry;              /*    16     1 */
  	u8                         Rsvd2;                /*    17     1 */

  	/* XXX 2 bytes hole, try to pack */

  	__le32                     SvnIdx;               /*    20     4 */
  	u32                        Rsvd3;                /*    24     4 */
  	u32                        Rsvd4;                /*    28     4 */
  	u32                        Rsvd5;                /*    32     4 */

  	/* size: 36, cachelines: 1, members: 17 */
  	/* sum members: 33, holes: 2, sum holes: 3 */
  	/* last cacheline: 36 bytes */
  };


And these comments like 'LONG WORD 1' looks misleading, maybe worth 
removing.


> 		fw_data = fw_data + 32;
> 		fw_size = fw_size - 32;
> 
> with:
> 		fw_data = fw_data + sizeof(struct rt_firmware_hdr);
> 		fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
> 
> To me it looks add/sub 32 is correct here but the struct is
> wrong. I don't know if the firmware for this driver is so much different
> from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
> They use a struct of size 32.
> 

I am not sure about 32... Why non-packed structures are passed via 
wires? Maybe `__packed` is missing? Is realtek fw open-source?

Just thoughts.

> Also, souldn't the u16 and u32 variables in the struct be __le16 and
> __le32 ?
> 
> I wonder if we can just use the rtlwifi_firmware_header structure from
> drivers/net/wireless/realtek/rtlwifi/wifi.h ?
> 

We _should_ use it, since its size is indeed 32 bytes.

└──$ pahole -s vmlinux | rg rtlwifi_firmware_header
rtlwifi_firmware_header	32	0


Looks like you have found a real bug


With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: staging: r8188eu: struct rt_firmware_header issues
  2022-04-13 19:42 ` Michael Straube
@ 2022-04-13 21:12   ` Larry Finger
  2022-04-14  8:41   ` David Laight
  1 sibling, 0 replies; 9+ messages in thread
From: Larry Finger @ 2022-04-13 21:12 UTC (permalink / raw)
  To: Michael Straube
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

On 4/13/22 14:42, Michael Straube wrote:
> On 4/13/22 18:27, Michael Straube wrote:
>> Hi all,
>>
>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>
>>
>> struct rt_firmware_hdr {
>>      /*  8-byte alinment required */
>>      /*  LONG WORD 0 ---- */
>>      __le16        Signature;    /* 92C0: test chip; 92C,
>>                       * 88C0: test chip; 88C1: MP A-cut;
>>                       * 92C1: MP A-cut */
>>      u8        Category;    /*  AP/NIC and USB/PCI */
>>      u8        Function;    /*  Reserved for different FW function
>>                       *  indcation, for further use when
>>                       *  driver needs to download different
>>                       *  FW for different conditions */
>>      __le16        Version;    /*  FW Version */
>>      u8        Subversion;    /*  FW Subversion, default 0x00 */
>>      u16        Rsvd1;
>>
>>      /*  LONG WORD 1 ---- */
>>      u8        Month;    /*  Release time Month field */
>>      u8        Date;    /*  Release time Date field */
>>      u8        Hour;    /*  Release time Hour field */
>>      u8        Minute;    /*  Release time Minute field */
>>      __le16        RamCodeSize;    /*  The size of RAM code */
>>      u8        Foundry;
>>      u8        Rsvd2;
>>
>>      /*  LONG WORD 2 ---- */
>>      __le32        SvnIdx;    /*  The SVN entry index */
>>      u32        Rsvd3;
>>
>>      /*  LONG WORD 3 ---- */
>>      u32        Rsvd4;
>>      u32        Rsvd5;
>> };
>>
>>
>> Then we have in rtl8188e_firmware_download():
>>
>>
>>      fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
>>
>>      <snip>
>>
>>      if (IS_FW_HEADER_EXIST(fwhdr)) {
>>          /*  Shift 32 bytes for FW header */
>>          fw_data = fw_data + 32;
>>          fw_size = fw_size - 32;
>>      }
>>
>> We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
>> 33 bytes. I noticed this when I wanted to replace:
>>
>>          fw_data = fw_data + 32;
>>          fw_size = fw_size - 32;
>>
>> with:
>>          fw_data = fw_data + sizeof(struct rt_firmware_hdr);
>>          fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
>>
>> To me it looks add/sub 32 is correct here but the struct is
>> wrong. I don't know if the firmware for this driver is so much different
>> from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
>> They use a struct of size 32.
>>
>> Also, souldn't the u16 and u32 variables in the struct be __le16 and
>> __le32 ?
>>
>> I wonder if we can just use the rtlwifi_firmware_header structure from
>> drivers/net/wireless/realtek/rtlwifi/wifi.h ?
>>
>> Comments from people with better knowledge appreciated. :)
>>
>> regards,
>> Michael
>>
>>
> 
> Ok, I figured it out by looking at the hexdumps of firmware files. The
> field Rsvd1 should be u8 instead of u16. I'll prepare a patch for this.

I agree. I had prepared an analysis, but did not get it mailed before I got this 
one.

Larry


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: staging: r8188eu: struct rt_firmware_header issues
  2022-04-13 20:58 ` Pavel Skripkin
@ 2022-04-13 21:26   ` Michael Straube
  0 siblings, 0 replies; 9+ messages in thread
From: Michael Straube @ 2022-04-13 21:26 UTC (permalink / raw)
  To: Pavel Skripkin, Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

On 4/13/22 22:58, Pavel Skripkin wrote:
> Hi Michael,
> 
> On 4/13/22 19:27, Michael Straube wrote:
>> Hi all,
>>
>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>
>>
>> struct rt_firmware_hdr {
>>     /*  8-byte alinment required */
>>     /*  LONG WORD 0 ---- */
>>     __le16        Signature;    /* 92C0: test chip; 92C,
>>                      * 88C0: test chip; 88C1: MP A-cut;
>>                      * 92C1: MP A-cut */
>>     u8        Category;    /*  AP/NIC and USB/PCI */
>>     u8        Function;    /*  Reserved for different FW function
>>                      *  indcation, for further use when
>>                      *  driver needs to download different
>>                      *  FW for different conditions */
>>     __le16        Version;    /*  FW Version */
>>     u8        Subversion;    /*  FW Subversion, default 0x00 */
>>     u16        Rsvd1;
>>
>>     /*  LONG WORD 1 ---- */
>>     u8        Month;    /*  Release time Month field */
>>     u8        Date;    /*  Release time Date field */
>>     u8        Hour;    /*  Release time Hour field */
>>     u8        Minute;    /*  Release time Minute field */
>>     __le16        RamCodeSize;    /*  The size of RAM code */
>>     u8        Foundry;
>>     u8        Rsvd2;
>>
>>     /*  LONG WORD 2 ---- */
>>     __le32        SvnIdx;    /*  The SVN entry index */
>>     u32        Rsvd3;
>>
>>     /*  LONG WORD 3 ---- */
>>     u32        Rsvd4;
>>     u32        Rsvd5;
>> };
>>
>>
>> Then we have in rtl8188e_firmware_download():
>>
>>
>>     fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
>>
>>     <snip>
>>
>>     if (IS_FW_HEADER_EXIST(fwhdr)) {
>>         /*  Shift 32 bytes for FW header */
>>         fw_data = fw_data + 32;
>>         fw_size = fw_size - 32;
>>     }
>>
>> We add/sub 32 bytes but the size of struct rt_firmware_hdr is actually
>> 33 bytes. I noticed this when I wanted to replace:
>>
> 
> Looks like size of that structure is 36
> 
> └──$ pahole -C rt_firmware_hdr r8188eu.ko
> struct rt_firmware_hdr {
>      __le16                     Signature;            /*     0     2 */
>      u8                         Category;             /*     2     1 */
>      u8                         Function;             /*     3     1 */
>      __le16                     Version;              /*     4     2 */
>       u8                         Subversion;           /*     6     1 */
> 
>       /* XXX 1 byte hole, try to pack */
> 
>       u16                        Rsvd1;                /*     8     2 */
>       u8                         Month;                /*    10     1 */
>       u8                         Date;                 /*    11     1 */
>       u8                         Hour;                 /*    12     1 */
>       u8                         Minute;               /*    13     1 */
>       __le16                     RamCodeSize;          /*    14     2 */
>       u8                         Foundry;              /*    16     1 */
>       u8                         Rsvd2;                /*    17     1 */
> 
>       /* XXX 2 bytes hole, try to pack */
> 
>       __le32                     SvnIdx;               /*    20     4 */
>       u32                        Rsvd3;                /*    24     4 */
>       u32                        Rsvd4;                /*    28     4 */
>       u32                        Rsvd5;                /*    32     4 */
> 
>       /* size: 36, cachelines: 1, members: 17 */
>       /* sum members: 33, holes: 2, sum holes: 3 */
>       /* last cacheline: 36 bytes */
>   };
> 
> 
> And these comments like 'LONG WORD 1' looks misleading, maybe worth 
> removing.
> 
> 
>>         fw_data = fw_data + 32;
>>         fw_size = fw_size - 32;
>>
>> with:
>>         fw_data = fw_data + sizeof(struct rt_firmware_hdr);
>>         fw_size = fw_size - sizeof(struct rt_firmware_hdr);;
>>
>> To me it looks add/sub 32 is correct here but the struct is
>> wrong. I don't know if the firmware for this driver is so much different
>> from firmware for the drivers in drivers/net/wireless/realtek/rtlwifi.
>> They use a struct of size 32.
>>
> 
> I am not sure about 32... Why non-packed structures are passed via 
> wires? Maybe `__packed` is missing? Is realtek fw open-source?
> 
> Just thoughts.
> 
>> Also, souldn't the u16 and u32 variables in the struct be __le16 and
>> __le32 ?
>>
>> I wonder if we can just use the rtlwifi_firmware_header structure from
>> drivers/net/wireless/realtek/rtlwifi/wifi.h ?
>>
> 
> We _should_ use it, since its size is indeed 32 bytes.
> 
> └──$ pahole -s vmlinux | rg rtlwifi_firmware_header
> rtlwifi_firmware_header    32    0
> 
> 
> Looks like you have found a real bug
> 
> 

Hi Pavel,

I looked at the hexdump of rtl8188eufw.bin and compared it with
rtl8188efw.bin and rtlwifi_firmware_header.

The comparsion showed that Rsvd1 should be u8 instead of u16:

OFFSET      rtl8188eufw.bin
     -----------------------------------------------------------
     0x00000000  E1 88 10 00 0B 00 01  00     01     21    11    27 30 
36 00 00
                                   ^   ^      ^      ^     ^     ^
                          Subversion   Rsvd1  Month  Date  Hour  Minute

So rt_firmware_hdr is wrong, but it had no effect because 32 is
hardcoded here:

	/*  Shift 32 bytes for FW header */
	fw_data = fw_data + 32;
	fw_size = fw_size - 32;

and all fields after Subversion are not used.

regards,
Michael

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: staging: r8188eu: struct rt_firmware_header issues
  2022-04-13 19:42 ` Michael Straube
  2022-04-13 21:12   ` Larry Finger
@ 2022-04-14  8:41   ` David Laight
  2022-04-14 10:08     ` Michael Straube
  1 sibling, 1 reply; 9+ messages in thread
From: David Laight @ 2022-04-14  8:41 UTC (permalink / raw)
  To: 'Michael Straube', Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

From: Michael Straube
> Sent: 13 April 2022 20:42
> 
> On 4/13/22 18:27, Michael Straube wrote:
> > Hi all,
> >
> > I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
> >
> >
> > struct rt_firmware_hdr {
> >      /*  8-byte alinment required */

Probably need an __aligned(8) at the bottom then?

> >      /*  LONG WORD 0 ---- */
> >      __le16        Signature;    /* 92C0: test chip; 92C,
> >                       * 88C0: test chip; 88C1: MP A-cut;
> >                       * 92C1: MP A-cut */
> >      u8        Category;    /*  AP/NIC and USB/PCI */
> >      u8        Function;    /*  Reserved for different FW function
> >                       *  indcation, for further use when
> >                       *  driver needs to download different
> >                       *  FW for different conditions */
> >      __le16        Version;    /*  FW Version */
> >      u8        Subversion;    /*  FW Subversion, default 0x00 */
> >      u16        Rsvd1;
> >
> >      /*  LONG WORD 1 ---- */
> >      u8        Month;    /*  Release time Month field */
> >      u8        Date;    /*  Release time Date field */
> >      u8        Hour;    /*  Release time Hour field */
> >      u8        Minute;    /*  Release time Minute field */
> >      __le16        RamCodeSize;    /*  The size of RAM code */
> >      u8        Foundry;
> >      u8        Rsvd2;
> >
> >      /*  LONG WORD 2 ---- */
> >      __le32        SvnIdx;    /*  The SVN entry index */
> >      u32        Rsvd3;
> >
> >      /*  LONG WORD 3 ---- */
> >      u32        Rsvd4;
> >      u32        Rsvd5;
> > };
...
> 
> Ok, I figured it out by looking at the hexdumps of firmware files. The
> field Rsvd1 should be u8 instead of u16. I'll prepare a patch for this.

I'd also add a compile-time assert on the size.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: staging: r8188eu: struct rt_firmware_header issues
  2022-04-14  8:41   ` David Laight
@ 2022-04-14 10:08     ` Michael Straube
  2022-04-14 10:14       ` David Laight
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Straube @ 2022-04-14 10:08 UTC (permalink / raw)
  To: David Laight, Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

On 4/14/22 10:41, David Laight wrote:
> From: Michael Straube
>> Sent: 13 April 2022 20:42
>>
>> On 4/13/22 18:27, Michael Straube wrote:
>>> Hi all,
>>>
>>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>>
>>>
>>> struct rt_firmware_hdr {
>>>       /*  8-byte alinment required */
> 
> Probably need an __aligned(8) at the bottom then?

I don't see any reason why this is needed. Do I miss something?

>>>       /*  LONG WORD 0 ---- */
>>>       __le16        Signature;    /* 92C0: test chip; 92C,
>>>                        * 88C0: test chip; 88C1: MP A-cut;
>>>                        * 92C1: MP A-cut */
>>>       u8        Category;    /*  AP/NIC and USB/PCI */
>>>       u8        Function;    /*  Reserved for different FW function
>>>                        *  indcation, for further use when
>>>                        *  driver needs to download different
>>>                        *  FW for different conditions */
>>>       __le16        Version;    /*  FW Version */
>>>       u8        Subversion;    /*  FW Subversion, default 0x00 */
>>>       u16        Rsvd1;
>>>
>>>       /*  LONG WORD 1 ---- */
>>>       u8        Month;    /*  Release time Month field */
>>>       u8        Date;    /*  Release time Date field */
>>>       u8        Hour;    /*  Release time Hour field */
>>>       u8        Minute;    /*  Release time Minute field */
>>>       __le16        RamCodeSize;    /*  The size of RAM code */
>>>       u8        Foundry;
>>>       u8        Rsvd2;
>>>
>>>       /*  LONG WORD 2 ---- */
>>>       __le32        SvnIdx;    /*  The SVN entry index */
>>>       u32        Rsvd3;
>>>
>>>       /*  LONG WORD 3 ---- */
>>>       u32        Rsvd4;
>>>       u32        Rsvd5;
>>> };
> ...
>>
>> Ok, I figured it out by looking at the hexdumps of firmware files. The
>> field Rsvd1 should be u8 instead of u16. I'll prepare a patch for this.
> 
> I'd also add a compile-time assert on the size.

I'll add a patch to the series I recently sent.

thanks,
Michael

^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: staging: r8188eu: struct rt_firmware_header issues
  2022-04-14 10:08     ` Michael Straube
@ 2022-04-14 10:14       ` David Laight
  2022-04-14 15:46         ` Larry Finger
  0 siblings, 1 reply; 9+ messages in thread
From: David Laight @ 2022-04-14 10:14 UTC (permalink / raw)
  To: 'Michael Straube', Larry Finger
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

From: Michael Straube
> Sent: 14 April 2022 11:08
> 
> On 4/14/22 10:41, David Laight wrote:
> > From: Michael Straube
> >> Sent: 13 April 2022 20:42
> >>
> >> On 4/13/22 18:27, Michael Straube wrote:
> >>> Hi all,
> >>>
> >>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
> >>>
> >>>
> >>> struct rt_firmware_hdr {
> >>>       /*  8-byte alinment required */
> >
> > Probably need an __aligned(8) at the bottom then?
> 
> I don't see any reason why this is needed. Do I miss something?

Dunno, the comment might be wrong.

	david

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: staging: r8188eu: struct rt_firmware_header issues
  2022-04-14 10:14       ` David Laight
@ 2022-04-14 15:46         ` Larry Finger
  0 siblings, 0 replies; 9+ messages in thread
From: Larry Finger @ 2022-04-14 15:46 UTC (permalink / raw)
  To: David Laight, 'Michael Straube'
  Cc: Phillip Potter, Greg KH, open list:STAGING SUBSYSTEM,
	Linux Kernel Mailing List

On 4/14/22 05:14, David Laight wrote:
> From: Michael Straube
>> Sent: 14 April 2022 11:08
>>
>> On 4/14/22 10:41, David Laight wrote:
>>> From: Michael Straube
>>>> Sent: 13 April 2022 20:42
>>>>
>>>> On 4/13/22 18:27, Michael Straube wrote:
>>>>> Hi all,
>>>>>
>>>>> I think the rt_firmware_hdr structure in rtw_fw.c has some issues.
>>>>>
>>>>>
>>>>> struct rt_firmware_hdr {
>>>>>        /*  8-byte alinment required */
>>>
>>> Probably need an __aligned(8) at the bottom then?
>>
>> I don't see any reason why this is needed. Do I miss something?
> 
> Dunno, the comment might be wrong.

Actually, the le16_to_cpu() references require alignment 4 (I think I got that 
right).

Larry


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-04-14 16:07 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-13 16:27 staging: r8188eu: struct rt_firmware_header issues Michael Straube
2022-04-13 19:42 ` Michael Straube
2022-04-13 21:12   ` Larry Finger
2022-04-14  8:41   ` David Laight
2022-04-14 10:08     ` Michael Straube
2022-04-14 10:14       ` David Laight
2022-04-14 15:46         ` Larry Finger
2022-04-13 20:58 ` Pavel Skripkin
2022-04-13 21:26   ` Michael Straube

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).