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

[Simon Horman <horms@kernel.org>]

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