staging: r8188eu: fix and clean up some firmware code
v2 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.
v2:
Added a patch to check size of struct rt_firmware_hdr at compile time.
Michael Straube (8):
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
staging: r8188eu: check rt_firmware_hdr size at compile time
drivers/staging/r8188eu/core/rtw_fw.c | 77 ++++++++++----------------- 1 file changed, 28 insertions(+), 49 deletions(-)
Signature · 1 line
2.35.1
PATCH v2 1/8 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 MinuteThis 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.
--- drivers/staging/r8188eu/core/rtw_fw.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +4 -4
@@ -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
PATCH v2 2/8 staging: r8188eu: clean up comments in struct rt_firmware_hdr
In reply to Michael StraubeThe comments in struct rt_firmware_hdr are not needed. Remove them.
--- drivers/staging/r8188eu/core/rtw_fw.c | 37 ++++++++------------------- 1 file changed, 11 insertions(+), 26 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +11 -26
@@ -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
PATCH v2 3/8 staging: r8188eu: rename fields of struct rt_firmware_hdr
In reply to Michael StraubeRename the fields of struct rt_firmware_hdr to avoid camel case.
--- drivers/staging/r8188eu/core/rtw_fw.c | 48 +++++++++++++-------------- 1 file changed, 24 insertions(+), 24 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +24 -24
@@ -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
PATCH v2 4/8 staging: r8188eu: use sizeof instead of hardcoded firmware header size
In reply to Michael StraubeUse sizeof() instead of hardcoding the firmware header size.
--- drivers/staging/r8188eu/core/rtw_fw.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
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 ); }/* Suggested by Filen. If 8051 is running in RAM code, driver should inform Fw to reset by itself, */
Signature · 1 line
2.35.1
PATCH v2 5/8 staging: r8188eu: remove variables from rtl8188e_firmware_download()
In reply to Michael StraubeThe 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.
--- drivers/staging/r8188eu/core/rtw_fw.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +2 -6
@@ -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
PATCH v2 6/8 staging: r8188eu: always log firmware info
In reply to Michael StraubeThe 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.
--- drivers/staging/r8188eu/core/rtw_fw.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +3 -5
@@ -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
PATCH v2 7/8 staging: r8188eu: check firmware header existence before access
In reply to Michael StraubeWe 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.
--- drivers/staging/r8188eu/core/rtw_fw.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/staging/r8188eu/core/rtw_fw.c +4 -5
@@ -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
PATCH v2 8/8 staging: r8188eu: check rt_firmware_hdr size at compile time
In reply to Michael StraubeCheck the size of struct rt_firmware_hdr at compile time since we rely on the size to be 32 in rtl8188e_firmware_download().
--- drivers/staging/r8188eu/core/rtw_fw.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/staging/r8188eu/core/rtw_fw.c +2 -0
@@ -34,6 +34,8 @@ struct rt_firmware_hdr {__le32 rsvd5; }; +static_assert (sizeof (struct rt_firmware_hdr ) ==32 ); +static void fw_download_enable (struct adapter *padapter,bool enable) {u8 tmp;
Signature · 1 line
2.35.1
Larry Finger
NAKQuoted 8 lines · Michael Straube
This series fixes wrong size of struct
rt_firmware_hdrin the first patch and does some cleanups inrtl8188e_firmware_download()in the other patches.Tested on
x86_64with Inter-Tech DMG-02.v2:
Added a patch to check size of structrt_firmware_hdrat compile time.
Please wait a while before submitting new versions until those of us in time zones further west get a chance. My objections to the logging change still apply.
NACK
Larry