staging: r8188eu: fix and clean up some firmware code

PATCH Superseded Michael Straube 3 participants 14 messages Apr 14, 2022 Staging

Share & tools

Michael Straube

author

2022-04-14 08:39 UTC #0 lore raw

cover staging: r8188eu: fix and clean up some firmware code

This series fixes wrong size of struct rt_firmware_hdr in the first patch and does some cleanups in rtl8188e_firmware_download() in the other patches.

Tested on x86_64 with Inter-Tech DMG-02.

Michael Straube (7):
staging: r8188eu: fix struct rt_firmware_hdr
staging: r8188eu: clean up comments in struct rt_firmware_hdr
staging: r8188eu: rename fields of struct rt_firmware_hdr
staging: r8188eu: use sizeof instead of hardcoded firmware header size
staging: r8188eu: remove variables from rtl8188e_firmware_download()
staging: r8188eu: always log firmware info
staging: r8188eu: check firmware header existence before access

 drivers/staging/r8188eu/core/rtw_fw.c | 75 ++++++++++-----------------
 1 file changed, 26 insertions(+), 49 deletions(-)

Signature · 1 line

2.35.1

Michael Straube

author

2022-04-14 08:39 UTC #1 lore raw

PATCH 1/7 staging: r8188eu: fix struct rt_firmware_hdr

The structure rt_firmware_hdr is wrong, there are two issues.

The first issue is that the size of struct rt_firmware_hdr is 33 bytes but the header in the firmware file is 32 bytes long.

The hexdump of rtl8188eufw.bin shows that the field Rsvd1 of struct rt_firmware_hdr should be u8 instead of __le16.

OFFSET rtl8188eufw.bin
-----------------------------------------------------------

0x00000000  E1 88 10 00 0B 00 01 00 01 21 11 27 30 36 00 00
0x00000010  2D 07 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x00000020  02 45 4E 00 00 00 00 00 00 00 00 00 00 00 00 00

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

This was figured out by looking at struct rtlwifi_firmware_header in drivers/net/wireless/rtlwifi/wifi.h and the firmware file that the rtlwifi/rtl8188ee driver uses.

The second issue is that the u16 and u32 fields sould be __le16 and __le32.

Change the field Rsvd1 to u8 and the u16, u32 fileds to __le16, __le32.

Both issues had no effect because the header size is actually hardcoded to 32 where it is used in the code. Also the fields after Subversion are not used and the bytes of the u16 and u32 fields are all zero.

Fixes 7884fc0a1473 ("staging: r8188eu: introduce new include dir for RTL8188eu driver")

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_fw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +4 -4

index 8620f3c92b52..7cd08268f3b9 100644
@@ -29,7 +29,7 @@ struct rt_firmware_hdr {
 					 *  FW for different conditions */
 	__le16		Version;	/*  FW Version */
 	u8		Subversion;	/*  FW Subversion, default 0x00 */
-	u16		Rsvd1;
+	u8		Rsvd1;
 
 	/*  LONG WORD 1 ---- */
 	u8		Month;	/*  Release time Month field */

@@ -42,11 +42,11 @@ struct rt_firmware_hdr {
 
 	/*  LONG WORD 2 ---- */
 	__le32		SvnIdx;	/*  The SVN entry index */
-	u32		Rsvd3;
+	__le32		Rsvd3;
 
 	/*  LONG WORD 3 ---- */
-	u32		Rsvd4;
-	u32		Rsvd5;
+	__le32		Rsvd4;
+	__le32		Rsvd5;
 };
 
 static void fw_download_enable(struct adapter *padapter, bool enable)

Signature · 1 line

2.35.1

Larry Finger

2022-04-14 15:32 UTC #2 lore raw

Re: [PATCH 1/7] staging: r8188eu: fix struct rt_firmware_hdr

Quoted 4 lines · Michael Straube

The second issue is that the u16 and u32 fields sould be __le16 and __le32.
Change the field Rsvd1 to u8 and the u16, u32 fileds to __le16, __le32.

There is a typo here.

Note that the various RsvdN fields are just reserving space and will never be referenced, thus it does not matter if they are u32 or __le32. It is OK to change them, but it is not wrong to leave them the way they are.

Larry

Michael Straube

author

2022-04-14 08:39 UTC #3 lore raw

PATCH 2/7 staging: r8188eu: clean up comments in struct rt_firmware_hdr

In reply to Michael Straube

The comments in struct rt_firmware_hdr are not needed. Remove them.

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_fw.c | 37 ++++++++-------------------
 1 file changed, 11 insertions(+), 26 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +11 ~~-26~~

index 7cd08268f3b9..323e0c634c4e 100644
@@ -14,37 +14,22 @@
 	(le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x2300 ||	\
 	(le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x88E0)
 
-/*  This structure must be careful with byte-ordering */
-
 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 */
+	__le16		Signature;
+	u8		Category;
+	u8		Function;
+	__le16		Version;
+	u8		Subversion;
 	u8		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		Month;
+	u8		Date;
+	u8		Hour;
+	u8		Minute;
+	__le16		RamCodeSize;
 	u8		Foundry;
 	u8		Rsvd2;
-
-	/*  LONG WORD 2 ---- */
-	__le32		SvnIdx;	/*  The SVN entry index */
+	__le32		SvnIdx;
 	__le32		Rsvd3;
-
-	/*  LONG WORD 3 ---- */
 	__le32		Rsvd4;
 	__le32		Rsvd5;
 };

Signature · 1 line

2.35.1

Michael Straube

author

2022-04-14 08:39 UTC #4 lore raw

PATCH 3/7 staging: r8188eu: rename fields of struct rt_firmware_hdr

In reply to Michael Straube

Rename the fields of struct rt_firmware_hdr to avoid camel case.

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_fw.c | 48 +++++++++++++--------------
 1 file changed, 24 insertions(+), 24 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +24 ~~-24~~

index 323e0c634c4e..3da52a1ba23c 100644
@@ -9,29 +9,29 @@
 #define MAX_PAGE_SIZE		4096
 
 #define IS_FW_HEADER_EXIST(_fwhdr)				\
-	((le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x92C0 ||	\
-	(le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x88C0 ||	\
-	(le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x2300 ||	\
-	(le16_to_cpu(_fwhdr->Signature) & 0xFFF0) == 0x88E0)
+	((le16_to_cpu(_fwhdr->signature) & 0xFFF0) == 0x92C0 ||	\
+	(le16_to_cpu(_fwhdr->signature) & 0xFFF0) == 0x88C0 ||	\
+	(le16_to_cpu(_fwhdr->signature) & 0xFFF0) == 0x2300 ||	\
+	(le16_to_cpu(_fwhdr->signature) & 0xFFF0) == 0x88E0)
 
 struct rt_firmware_hdr {
-	__le16		Signature;
-	u8		Category;
-	u8		Function;
-	__le16		Version;
-	u8		Subversion;
-	u8		Rsvd1;
-	u8		Month;
-	u8		Date;
-	u8		Hour;
-	u8		Minute;
-	__le16		RamCodeSize;
-	u8		Foundry;
-	u8		Rsvd2;
-	__le32		SvnIdx;
-	__le32		Rsvd3;
-	__le32		Rsvd4;
-	__le32		Rsvd5;
+	__le16	signature;
+	u8	category;
+	u8	function;
+	__le16	version;
+	u8	subversion;
+	u8	rsvd1;
+	u8	month;
+	u8	date;
+	u8	hour;
+	u8	minute;
+	__le16	ramcodesize;
+	u8	foundry;
+	u8	rsvd2;
+	__le32	svnidx;
+	__le32	rsvd3;
+	__le32	rsvd4;
+	__le32	rsvd5;
 };
 
 static void fw_download_enable(struct adapter *padapter, bool enable)

@@ -254,9 +254,9 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	/*  To Check Fw header. Added by tynli. 2009.12.04. */
 	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
 
-	fw_version = le16_to_cpu(fwhdr->Version);
-	fw_subversion = fwhdr->Subversion;
-	fw_signature = le16_to_cpu(fwhdr->Signature);
+	fw_version = le16_to_cpu(fwhdr->version);
+	fw_subversion = fwhdr->subversion;
+	fw_signature = le16_to_cpu(fwhdr->signature);
 
 	if (!log_version++)
 		pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",

Signature · 1 line

2.35.1

Michael Straube

author

2022-04-14 08:39 UTC #5 lore raw

PATCH 4/7 staging: r8188eu: use sizeof instead of hardcoded firmware header size

In reply to Michael Straube

Use sizeof() instead of hardcoding the firmware header size.

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_fw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +2 -3

index 3da52a1ba23c..94526064f29b 100644
@@ -263,9 +263,8 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 			DRIVER_PREFIX, fw_version, fw_subversion, fw_signature);
 
 	if (IS_FW_HEADER_EXIST(fwhdr)) {
-		/*  Shift 32 bytes for FW header */
-		fw_data = fw_data + 32;
-		fw_size = fw_size - 32;
+		fw_data = fw_data + sizeof(struct rt_firmware_hdr);
+		fw_size = fw_size - sizeof(struct rt_firmware_hdr);
 	}
 
 	/*  Suggested by Filen. If 8051 is running in RAM code, driver should inform Fw to reset by itself, */

Signature · 1 line

2.35.1

Pavel Skripkin

2022-04-14 19:36 UTC #6 lore raw

Re: [PATCH 4/7] staging: r8188eu: use sizeof instead of hardcoded firmware header size

Hi Michael,

Quoted 12 lines · Michael Straube

Use sizeof() instead of hardcoding the firmware header size.
Signed-off-by Michael Straube <[email protected]>
---
  drivers/staging/r8188eu/core/rtw_fw.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +0 -0
index 3da52a1ba23c..94526064f29b 100644

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 267

Quoted 8 lines

drivers/staging/r8188eu/core/rtw_fw.c +2 -3

@@ -263,9 +263,8 @@ int rtl8188e_firmware_download(struct adapter *padapter)
  			DRIVER_PREFIX, fw_version, fw_subversion, fw_signature);
 
  	if (IS_FW_HEADER_EXIST(fwhdr)) {
-		/*  Shift 32 bytes for FW header */
-		fw_data = fw_data + 32;
-		fw_size = fw_size - 32;
+		fw_data = fw_data + sizeof(struct rt_firmware_hdr);
+		fw_size = fw_size - sizeof(struct rt_firmware_hdr);

What about BUILD_BUG_ON(sizeof(struct rt_firmware_hdr) != 32)? To be sure we won't face same bug in future

With regards,
Pavel Skripkin

Michael Straube

author

2022-04-14 08:39 UTC #7 lore raw

PATCH 5/7 staging: r8188eu: remove variables from rtl8188e_firmware_download()

In reply to Michael Straube

The local variables fw_version, fw_subversion, fw_signature in rtl8188e_firmware_download() are only used in one place. Use the assigned values directly and remove the variables to make the code shorter and cleaner.

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_fw.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +2 -6

index 94526064f29b..cbc4980bd938 100644
@@ -237,7 +237,6 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	struct dvobj_priv *dvobj = adapter_to_dvobj(padapter);
 	struct device *device = dvobj_to_dev(dvobj);
 	struct rt_firmware_hdr *fwhdr = NULL;
-	u16 fw_version, fw_subversion, fw_signature;
 	u8 *fw_data;
 	u32 fw_size;
 	static int log_version;

@@ -254,13 +253,10 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	/*  To Check Fw header. Added by tynli. 2009.12.04. */
 	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
 
-	fw_version = le16_to_cpu(fwhdr->version);
-	fw_subversion = fwhdr->subversion;
-	fw_signature = le16_to_cpu(fwhdr->signature);
-
 	if (!log_version++)
 		pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
-			DRIVER_PREFIX, fw_version, fw_subversion, fw_signature);
+			DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,
+			le16_to_cpu(fwhdr->signature));
 
 	if (IS_FW_HEADER_EXIST(fwhdr)) {
 		fw_data = fw_data + sizeof(struct rt_firmware_hdr);

Signature · 1 line

2.35.1

Michael Straube

author

2022-04-14 08:39 UTC #8 lore raw

PATCH 6/7 staging: r8188eu: always log firmware info

In reply to Michael Straube

The local static variable log_version prevents logging the firmware information more than once, e.g. when the device is unplugged and plugged again. That is not necessary and complicates the code. Remove it.

Signed-off-by Michael Straube <[email protected]>

---
 drivers/staging/r8188eu/core/rtw_fw.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +3 -5

index cbc4980bd938..64963507a346 100644
@@ -239,7 +239,6 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	struct rt_firmware_hdr *fwhdr = NULL;
 	u8 *fw_data;
 	u32 fw_size;
-	static int log_version;
 
 	if (!dvobj->firmware.data)
 		ret = load_firmware(&dvobj->firmware, device);

@@ -253,10 +252,9 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	/*  To Check Fw header. Added by tynli. 2009.12.04. */
 	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
 
-	if (!log_version++)
-		pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
-			DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,
-			le16_to_cpu(fwhdr->signature));
+	pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
+		DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,
+		le16_to_cpu(fwhdr->signature));
 
 	if (IS_FW_HEADER_EXIST(fwhdr)) {
 		fw_data = fw_data + sizeof(struct rt_firmware_hdr);

Signature · 1 line

2.35.1

Larry Finger

2022-04-14 15:36 UTC #9 lore raw

Re: [PATCH 6/7] staging: r8188eu: always log firmware info

Quoted 4 lines · Michael Straube

The local static variable log_version prevents logging the firmware information more than once, e.g. when the device is unplugged and plugged again. That is not necessary and complicates the code. Remove it.

I think the slight complication of the code, one static variable and one if statement, is worth not spamming the logs. My recollection is that there are other cases besides unplugging and replugging that lead to the firmware being reloaded.

Larry

Pavel Skripkin

2022-04-14 19:39 UTC #10 lore raw

Re: [PATCH 6/7] staging: r8188eu: always log firmware info

Hi Larry,

Quoted 11 lines · Larry Finger

Quoted 4 lines · Michael Straube
The local static variable log_version prevents logging the firmware information more than once, e.g. when the device is unplugged and plugged again. That is not necessary and complicates the code. Remove it.
I think the slight complication of the code, one static variable and one if statement, is worth not spamming the logs. My recollection is that there are other cases besides unplugging and replugging that lead to the firmware being reloaded.

What about pr_info_once? Anyway even with old code message printed only once per driver load

With regards,
Pavel Skripkin

Michael Straube

author

2022-04-14 08:39 UTC #11 lore raw

PATCH 7/7 staging: r8188eu: check firmware header existence before access

In reply to Michael Straube

We should access the fields of fwhdr only if the check for firmware header existence is true. Move the affected code into the if block that checks firmware header existence.

Signed-off-by Michael Straube <[email protected]>

---
Do we need the IS_FW_HEADER_EXIST(fwhdr) check at all? The header _does_ exist in rtl8188eufw.bin and it's very unlikely that it ever changes _and_ the header will be removed.

 drivers/staging/r8188eu/core/rtw_fw.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

drivers/staging/r8188eu/core/rtw_fw.c +4 -5

index 64963507a346..c58bce1a1856 100644
@@ -249,14 +249,13 @@ int rtl8188e_firmware_download(struct adapter *padapter)
 	fw_data = dvobj->firmware.data;
 	fw_size = dvobj->firmware.size;
 
-	/*  To Check Fw header. Added by tynli. 2009.12.04. */
 	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
 
-	pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
-		DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,
-		le16_to_cpu(fwhdr->signature));
-
 	if (IS_FW_HEADER_EXIST(fwhdr)) {
+		pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
+			DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,
+			le16_to_cpu(fwhdr->signature));
+
 		fw_data = fw_data + sizeof(struct rt_firmware_hdr);
 		fw_size = fw_size - sizeof(struct rt_firmware_hdr);
 	}

Signature · 1 line

2.35.1

Larry Finger

2022-04-14 15:40 UTC #12 lore raw

Re: [PATCH 7/7] staging: r8188eu: check firmware header existence before access

Quoted 27 lines · Michael Straube

We should access the fields of fwhdr only if the check for firmware header existence is true. Move the affected code into the if block that checks firmware header existence.
Signed-off-by Michael Straube <[email protected]>
---
Do we need the IS_FW_HEADER_EXIST(fwhdr) check at all? The header _does_ exist in rtl8188eufw.bin and it's very unlikely that it ever changes _and_ the header will be removed.
  drivers/staging/r8188eu/core/rtw_fw.c | 9 ++++-----
  1 file changed, 4 insertions(+), 5 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +0 -3
index 64963507a346..c58bce1a1856 100644
@@ -249,14 +249,13 @@ int rtl8188e_firmware_download(struct adapter *padapter)
  	fw_data = dvobj->firmware.data;
  	fw_size = dvobj->firmware.size;
 
-	/*  To Check Fw header. Added by tynli. 2009.12.04. */
  	fwhdr = (struct rt_firmware_hdr *)dvobj->firmware.data;
 
-	pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
-		DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,

drivers/staging/r8188eu/core/rtw_fw.c int rtl8188e_firmware_download(struct adapter *padapter) · line 261

Quoted 10 lines

drivers/staging/r8188eu/core/rtw_fw.c +4 -2

-		le16_to_cpu(fwhdr->signature));
-
  	if (IS_FW_HEADER_EXIST(fwhdr)) {
+		pr_info("%sFirmware Version %d, SubVersion %d, Signature 0x%x\n",
+			DRIVER_PREFIX, le16_to_cpu(fwhdr->version), fwhdr->subversion,
+			le16_to_cpu(fwhdr->signature));
+
  		fw_data = fw_data + sizeof(struct rt_firmware_hdr);
  		fw_size = fw_size - sizeof(struct rt_firmware_hdr);
  	}

You can probably remove the IS_FW_HEADER_EXIST macro, but please restore the guard against logging the version more than once.

Larry

Larry Finger

Acked-by on the v1 series

2022-04-14 15:43 UTC #13 lore raw

In reply to Michael Straube

Quoted 6 lines · Michael Straube

This series fixes wrong size of struct rt_firmware_hdr in the first patch and does some cleanups in rtl8188e_firmware_download() in the other patches.
Tested on x86_64 with Inter-Tech DMG-02.

Quoted 8 lines

Michael Straube (7):
staging: r8188eu: fix struct rt_firmware_hdr
staging: r8188eu: clean up comments in struct rt_firmware_hdr
staging: r8188eu: rename fields of struct rt_firmware_hdr
staging: r8188eu: use sizeof instead of hardcoded firmware header size
staging: r8188eu: remove variables from rtl8188e_firmware_download()
staging: r8188eu: always log firmware info
staging: r8188eu: check firmware header existence before access

Acked-by Larry Finger <[email protected]> for 2/7 through 5/7. Patch 1/7 has a typo in the commit message, and I have objection to the logging changes in 6/7 and 7/7.

Larry