diff mbox series

[net-next] r8169: remove not needed check in rtl_fw_write_firmware

Message ID 52f09685-47ba-4cfe-8933-bf641c3d1b1d@gmail.com (mailing list archive)
State Accepted
Commit 3a767b482cacd9bfeac786837fcac419af315995
Delegated to: Netdev Maintainers
Headers show
Series [net-next] r8169: remove not needed check in rtl_fw_write_firmware | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/codegen success Generated files up to date
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1127 this patch: 1127
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1154 this patch: 1154
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1154 this patch: 1154
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 9 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Heiner Kallweit Nov. 23, 2023, 9:53 a.m. UTC
This check can never be true for a firmware file with a correct format.
Existing checks in rtl_fw_data_ok() are sufficient, no problems with
invalid firmware files are known.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_firmware.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Simon Horman Nov. 23, 2023, 2:54 p.m. UTC | #1
On Thu, Nov 23, 2023 at 10:53:26AM +0100, Heiner Kallweit wrote:
> This check can never be true for a firmware file with a correct format.
> Existing checks in rtl_fw_data_ok() are sufficient, no problems with
> invalid firmware files are known.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169_firmware.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_firmware.c b/drivers/net/ethernet/realtek/r8169_firmware.c
> index cbc6b846d..ed6e721b1 100644
> --- a/drivers/net/ethernet/realtek/r8169_firmware.c
> +++ b/drivers/net/ethernet/realtek/r8169_firmware.c
> @@ -151,9 +151,6 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
>  		u32 regno = (action & 0x0fff0000) >> 16;
>  		enum rtl_fw_opcode opcode = action >> 28;
>  
> -		if (!action)
> -			break;
> -

Hi Heiner,

I could well be wrong, but this does seem to guard against the following case:

1. data = 0
2. regno = 0
3. opcode = 0 (PHY_READ)

Which does not seem to be checked in rtl_fw_data_ok().

It's unclear to me if there is any value in this guard.

>  		switch (opcode) {
>  		case PHY_READ:
>  			predata = fw_read(tp, regno);
> -- 
> 2.43.0
>
Heiner Kallweit Nov. 23, 2023, 3:12 p.m. UTC | #2
On 23.11.2023 15:54, Simon Horman wrote:
> On Thu, Nov 23, 2023 at 10:53:26AM +0100, Heiner Kallweit wrote:
>> This check can never be true for a firmware file with a correct format.
>> Existing checks in rtl_fw_data_ok() are sufficient, no problems with
>> invalid firmware files are known.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/ethernet/realtek/r8169_firmware.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_firmware.c b/drivers/net/ethernet/realtek/r8169_firmware.c
>> index cbc6b846d..ed6e721b1 100644
>> --- a/drivers/net/ethernet/realtek/r8169_firmware.c
>> +++ b/drivers/net/ethernet/realtek/r8169_firmware.c
>> @@ -151,9 +151,6 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
>>  		u32 regno = (action & 0x0fff0000) >> 16;
>>  		enum rtl_fw_opcode opcode = action >> 28;
>>  
>> -		if (!action)
>> -			break;
>> -
> 
> Hi Heiner,
> 
> I could well be wrong, but this does seem to guard against the following case:
> 
> 1. data = 0
> 2. regno = 0
> 3. opcode = 0 (PHY_READ)
> 
> Which does not seem to be checked in rtl_fw_data_ok().
> 
> It's unclear to me if there is any value in this guard.
> 
Value 0 is used with a special meaning in two places:
1. Newer firmwares with some meta data before the actual firmware
   have first dword 0 to be able to differentiate old and new fw format.
2. Typically (not always) fw files in new format have a trailing dword 0.

A potential problem (as you mention) is that value 0 isn't really a
sentinel value because reading PHY register 0 is a valid command.
It's just never used in their firmwares.

There's no need to guard from reading PHY reg 0. It does no harm.
I *think* they once added this check to detect end of file.
But that's not needed because the actual firmware length is
part of the meta data. Therefore reading data from the firmware
will stop before reaching the training zero(s).

>>  		switch (opcode) {
>>  		case PHY_READ:
>>  			predata = fw_read(tp, regno);
>> -- 
>> 2.43.0
>>
patchwork-bot+netdevbpf@kernel.org Nov. 24, 2023, 3:30 p.m. UTC | #3
Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 23 Nov 2023 10:53:26 +0100 you wrote:
> This check can never be true for a firmware file with a correct format.
> Existing checks in rtl_fw_data_ok() are sufficient, no problems with
> invalid firmware files are known.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/ethernet/realtek/r8169_firmware.c | 3 ---
>  1 file changed, 3 deletions(-)

Here is the summary with links:
  - [net-next] r8169: remove not needed check in rtl_fw_write_firmware
    https://git.kernel.org/netdev/net-next/c/3a767b482cac

You are awesome, thank you!
Simon Horman Nov. 24, 2023, 9:51 p.m. UTC | #4
On Thu, Nov 23, 2023 at 04:12:59PM +0100, Heiner Kallweit wrote:
> On 23.11.2023 15:54, Simon Horman wrote:
> > On Thu, Nov 23, 2023 at 10:53:26AM +0100, Heiner Kallweit wrote:
> >> This check can never be true for a firmware file with a correct format.
> >> Existing checks in rtl_fw_data_ok() are sufficient, no problems with
> >> invalid firmware files are known.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/net/ethernet/realtek/r8169_firmware.c | 3 ---
> >>  1 file changed, 3 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/realtek/r8169_firmware.c b/drivers/net/ethernet/realtek/r8169_firmware.c
> >> index cbc6b846d..ed6e721b1 100644
> >> --- a/drivers/net/ethernet/realtek/r8169_firmware.c
> >> +++ b/drivers/net/ethernet/realtek/r8169_firmware.c
> >> @@ -151,9 +151,6 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
> >>  		u32 regno = (action & 0x0fff0000) >> 16;
> >>  		enum rtl_fw_opcode opcode = action >> 28;
> >>  
> >> -		if (!action)
> >> -			break;
> >> -
> > 
> > Hi Heiner,
> > 
> > I could well be wrong, but this does seem to guard against the following case:
> > 
> > 1. data = 0
> > 2. regno = 0
> > 3. opcode = 0 (PHY_READ)
> > 
> > Which does not seem to be checked in rtl_fw_data_ok().
> > 
> > It's unclear to me if there is any value in this guard.
> > 
> Value 0 is used with a special meaning in two places:
> 1. Newer firmwares with some meta data before the actual firmware
>    have first dword 0 to be able to differentiate old and new fw format.
> 2. Typically (not always) fw files in new format have a trailing dword 0.
> 
> A potential problem (as you mention) is that value 0 isn't really a
> sentinel value because reading PHY register 0 is a valid command.
> It's just never used in their firmwares.
> 
> There's no need to guard from reading PHY reg 0. It does no harm.
> I *think* they once added this check to detect end of file.
> But that's not needed because the actual firmware length is
> part of the meta data. Therefore reading data from the firmware
> will stop before reaching the training zero(s).

Thanks for the clarification.
I am happy with this patch (which is now in net-next).

> 
> >>  		switch (opcode) {
> >>  		case PHY_READ:
> >>  			predata = fw_read(tp, regno);
> >> -- 
> >> 2.43.0
> >>
>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/realtek/r8169_firmware.c b/drivers/net/ethernet/realtek/r8169_firmware.c
index cbc6b846d..ed6e721b1 100644
--- a/drivers/net/ethernet/realtek/r8169_firmware.c
+++ b/drivers/net/ethernet/realtek/r8169_firmware.c
@@ -151,9 +151,6 @@  void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
 		u32 regno = (action & 0x0fff0000) >> 16;
 		enum rtl_fw_opcode opcode = action >> 28;
 
-		if (!action)
-			break;
-
 		switch (opcode) {
 		case PHY_READ:
 			predata = fw_read(tp, regno);