Re: [PATCH net-next 3/3] r8169: release firmware on application failure

[Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>]

On 2026-09-16 19:02, Andrew Lunn wrote:
> On Wed, Sep 16, 2026 at 06:15:00PM -0300, Matheus Alves de Almeida wrote:
>> > If the firmware cannot be written, is the device dead? Should this
>> > return an error, so the caller can abort the probe?
>> 
>> A timeout while applying firmware could indeed indicate a PHY access
>> problem. However, firmware loading failures are already non-fatal,
>> and the callers of r8169_apply_firmware() do not propagate errors
>> either. Making firmware application failures fatal would require
>> broader changes to several r8169 PHY initialization paths.
> 
> But you are making such changes, returning errors up the call chain.
> Why are you making these changes? We either assume nothing can fail,
> so we throw away the return code, or we should assume everything can
> fail, and propagate the errors.
> 
> If we assume error can happen, if there is an error in firmware
> download, isn't that fatal? Should we even care about care about PHY
> read/write errors if firmware download has failed? And since firmware
> download is probably the first thing to happen, if anything is likely
> to fair, i would expect firmware download is what is going to fail.
> 
>    Andrew

Looking at this more in depth, I am leaning towards keeping the old
non-fatal behavior.

Right now, firmware application failures are ignored, so the device
can keep going even if applying the firmware fails. The TODO also
specifically says to release the firmware on failure, which seems to
imply that continuing without it was the intended behavior, and that
releasing it was mainly meant to prevent retrying the same failed
firmware application later.

Making firmware application failures fatal also creates a state problem.
If we release tp->rtl_fw after a failure, later initialization attempts
will see no firmware and continue without retrying it. Avoiding that
would require tracking the failure separately or keeping the firmware
loaded, which would no longer follow what the TODO suggests.

At that point this becomes a larger behavior change and could possibly
break hardware where firmware application failures are currently
tolerated.

Because of that, I am leaning towards keeping the existing behavior:
detect the failure, release the firmware as the TODO says, and continue
without it.

What do you think?