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

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

[Heiner Kallweit]

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;
-
 		switch (opcode) {
 		case PHY_READ:
 			predata = fw_read(tp, regno);
-- 
2.43.0

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

[Simon Horman]

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
> 

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

[Heiner Kallweit]

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

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

[Simon Horman]

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

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

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html