[PATCH net-next 0/3] r8169: handle firmware application errors

[PATCH net-next 0/3] r8169: handle firmware application errors

[Matheus Alves de Almeida]

Propagate PHY read/write errors through the firmware interpreter and
release loaded firmware if application fails.

This resolves the existing TODO in r8169_apply_firmware().

Matheus Alves de Almeida (3):
  r8169: propagate errors from PHY write operations
  r8169: propagate firmware access errors
  r8169: release firmware on application failure

 drivers/net/ethernet/realtek/r8169_firmware.c | 15 ++++--
 drivers/net/ethernet/realtek/r8169_firmware.h |  4 +-
 drivers/net/ethernet/realtek/r8169_main.c     | 50 +++++++++++--------
 3 files changed, 43 insertions(+), 26 deletions(-)


base-commit: 87b80c2f6b05cad9f0ff9136709c62a0f59923e3
-- 
2.43.0

[PATCH net-next 1/3] r8169: propagate errors from PHY write operations

[Matheus Alves de Almeida]

Currently, hardware timeouts and other errors are ignored during
PHY write operations.

Return an error when these operations fail and propagate it through
rtl_writephy().

Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
---
 drivers/net/ethernet/realtek/r8169_firmware.h |  2 +-
 drivers/net/ethernet/realtek/r8169_main.c     | 44 +++++++++++--------
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_firmware.h b/drivers/net/ethernet/realtek/r8169_firmware.h
index 7dc348ed8..1285a1c28 100644
--- a/drivers/net/ethernet/realtek/r8169_firmware.h
+++ b/drivers/net/ethernet/realtek/r8169_firmware.h
@@ -12,7 +12,7 @@
 #include <linux/firmware.h>
 
 struct rtl8169_private;
-typedef void (*rtl_fw_write_t)(struct rtl8169_private *tp, int reg, int val);
+typedef int (*rtl_fw_write_t)(struct rtl8169_private *tp, int reg, int val);
 typedef int (*rtl_fw_read_t)(struct rtl8169_private *tp, int reg);
 
 #define RTL_VER_SIZE		32
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5415ff62a..3c37c5a6c 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -1145,14 +1145,15 @@ DECLARE_RTL_COND(rtl_ocp_gphy_cond)
 	return RTL_R32(tp, GPHY_OCP) & OCPAR_FLAG;
 }
 
-static void r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
+static int r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
 {
 	if (rtl_ocp_reg_failure(reg))
-		return;
+		return 0;
 
 	RTL_W32(tp, GPHY_OCP, OCPAR_FLAG | (reg << 15) | data);
 
-	rtl_loop_wait_low(tp, &rtl_ocp_gphy_cond, 25, 10);
+	return rtl_loop_wait_low(tp, &rtl_ocp_gphy_cond, 25, 10) ?
+		0 : -ETIMEDOUT;
 }
 
 static int r8168_phy_ocp_read(struct rtl8169_private *tp, u32 reg)
@@ -1337,11 +1338,11 @@ static void rtl8168g_phy_suspend_quirk(struct rtl8169_private *tp, int value)
 	}
 };
 
-static void r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
+static int r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
 {
 	if (reg == 0x1f) {
 		tp->ocp_base = value ? value << 4 : OCP_STD_PHY_BASE;
-		return;
+		return 0;
 	}
 
 	if (tp->ocp_base != OCP_STD_PHY_BASE)
@@ -1350,7 +1351,7 @@ static void r8168g_mdio_write(struct rtl8169_private *tp, int reg, int value)
 	if (tp->ocp_base == OCP_STD_PHY_BASE && reg == MII_BMCR)
 		rtl8168g_phy_suspend_quirk(tp, value);
 
-	r8168_phy_ocp_write(tp, tp->ocp_base + reg * 2, value);
+	return r8168_phy_ocp_write(tp, tp->ocp_base + reg * 2, value);
 }
 
 static int r8168g_mdio_read(struct rtl8169_private *tp, int reg)
@@ -1364,14 +1365,15 @@ static int r8168g_mdio_read(struct rtl8169_private *tp, int reg)
 	return r8168_phy_ocp_read(tp, tp->ocp_base + reg * 2);
 }
 
-static void mac_mcu_write(struct rtl8169_private *tp, int reg, int value)
+static int mac_mcu_write(struct rtl8169_private *tp, int reg, int value)
 {
 	if (reg == 0x1f) {
 		tp->ocp_base = value << 4;
-		return;
+		return 0;
 	}
 
 	r8168_mac_ocp_write(tp, tp->ocp_base + reg, value);
+	return 0;
 }
 
 static bool rtl_is_8116af(struct rtl8169_private *tp)
@@ -1393,16 +1395,19 @@ DECLARE_RTL_COND(rtl_phyar_cond)
 	return RTL_R32(tp, PHYAR) & 0x80000000;
 }
 
-static void r8169_mdio_write(struct rtl8169_private *tp, int reg, int value)
+static int r8169_mdio_write(struct rtl8169_private *tp, int reg, int value)
 {
 	RTL_W32(tp, PHYAR, 0x80000000 | (reg & 0x1f) << 16 | (value & 0xffff));
 
-	rtl_loop_wait_low(tp, &rtl_phyar_cond, 25, 20);
+	if (!rtl_loop_wait_low(tp, &rtl_phyar_cond, 25, 20))
+		return -ETIMEDOUT;
 	/*
 	 * According to hardware specs a 20us delay is required after write
 	 * complete indication, but before sending next command.
 	 */
 	udelay(20);
+
+	return 0;
 }
 
 static int r8169_mdio_read(struct rtl8169_private *tp, int reg)
@@ -1440,13 +1445,17 @@ static void r8168dp_2_mdio_stop(struct rtl8169_private *tp)
 	RTL_W32(tp, 0xd0, RTL_R32(tp, 0xd0) | R8168DP_1_MDIO_ACCESS_BIT);
 }
 
-static void r8168dp_2_mdio_write(struct rtl8169_private *tp, int reg, int value)
+static int r8168dp_2_mdio_write(struct rtl8169_private *tp, int reg, int value)
 {
+	int rc;
+
 	r8168dp_2_mdio_start(tp);
 
-	r8169_mdio_write(tp, reg, value);
+	rc = r8169_mdio_write(tp, reg, value);
 
 	r8168dp_2_mdio_stop(tp);
+
+	return rc;
 }
 
 static int r8168dp_2_mdio_read(struct rtl8169_private *tp, int reg)
@@ -1466,19 +1475,16 @@ static int r8168dp_2_mdio_read(struct rtl8169_private *tp, int reg)
 	return value;
 }
 
-static void rtl_writephy(struct rtl8169_private *tp, int location, int val)
+static int rtl_writephy(struct rtl8169_private *tp, int location, int val)
 {
 	switch (tp->mac_version) {
 	case RTL_GIGA_MAC_VER_28:
 	case RTL_GIGA_MAC_VER_31:
-		r8168dp_2_mdio_write(tp, location, val);
-		break;
+		return r8168dp_2_mdio_write(tp, location, val);
 	case RTL_GIGA_MAC_VER_40 ... RTL_GIGA_MAC_VER_LAST:
-		r8168g_mdio_write(tp, location, val);
-		break;
+		return r8168g_mdio_write(tp, location, val);
 	default:
-		r8169_mdio_write(tp, location, val);
-		break;
+		return r8169_mdio_write(tp, location, val);
 	}
 }
 
-- 
2.43.0

[PATCH net-next 2/3] r8169: propagate firmware access errors

[Matheus Alves de Almeida]

rtl_fw_write_firmware() does not check errors returned from  fw_write()
and fw_read(), nor does it report failures to its caller.

Change rtl_fw_write_firmware() to return an int and propagate errors
from fw_write() and fw_read().

Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
---
 drivers/net/ethernet/realtek/r8169_firmware.c | 15 ++++++++++++---
 drivers/net/ethernet/realtek/r8169_firmware.h |  2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_firmware.c b/drivers/net/ethernet/realtek/r8169_firmware.c
index 6dff3d947..645d510e6 100644
--- a/drivers/net/ethernet/realtek/r8169_firmware.c
+++ b/drivers/net/ethernet/realtek/r8169_firmware.c
@@ -137,7 +137,7 @@ static bool rtl_fw_data_ok(struct rtl_fw *rtl_fw)
 	return false;
 }
 
-void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
+int rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
 {
 	struct rtl_fw_phy_action *pa = &rtl_fw->phy_action;
 	rtl_fw_write_t fw_write = rtl_fw->phy_write;
@@ -150,10 +150,13 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
 		u32 data = action & 0x0000ffff;
 		u32 regno = (action & 0x0fff0000) >> 16;
 		enum rtl_fw_opcode opcode = action >> 28;
+		int rc;
 
 		switch (opcode) {
 		case PHY_READ:
 			predata = fw_read(tp, regno);
+			if (predata < 0)
+				return predata;
 			count++;
 			break;
 		case PHY_DATA_OR:
@@ -179,7 +182,9 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
 			count = 0;
 			break;
 		case PHY_WRITE:
-			fw_write(tp, regno, data);
+			rc = fw_write(tp, regno, data);
+			if (rc < 0)
+				return rc;
 			break;
 		case PHY_READCOUNT_EQ_SKIP:
 			if (count == data)
@@ -194,7 +199,9 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
 				index += regno;
 			break;
 		case PHY_WRITE_PREVIOUS:
-			fw_write(tp, regno, predata);
+			rc = fw_write(tp, regno, predata);
+			if (rc < 0)
+				return rc;
 			break;
 		case PHY_SKIPN:
 			index += regno;
@@ -204,6 +211,8 @@ void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw)
 			break;
 		}
 	}
+
+	return 0;
 }
 
 void rtl_fw_release_firmware(struct rtl_fw *rtl_fw)
diff --git a/drivers/net/ethernet/realtek/r8169_firmware.h b/drivers/net/ethernet/realtek/r8169_firmware.h
index 1285a1c28..36c89bfda 100644
--- a/drivers/net/ethernet/realtek/r8169_firmware.h
+++ b/drivers/net/ethernet/realtek/r8169_firmware.h
@@ -36,4 +36,4 @@ struct rtl_fw {
 
 int rtl_fw_request_firmware(struct rtl_fw *rtl_fw);
 void rtl_fw_release_firmware(struct rtl_fw *rtl_fw);
-void rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw);
+int rtl_fw_write_firmware(struct rtl8169_private *tp, struct rtl_fw *rtl_fw);
-- 
2.43.0

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

[Matheus Alves de Almeida]

After an rtl_fw_write_firmware() failure, r8169_apply_firmware() does
not release the firmware. A TODO notes that it should be released
in this case.

Release the firmware on failure and remove the TODO.

Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
---
 drivers/net/ethernet/realtek/r8169_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 3c37c5a6c..c54423cd2 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -2596,12 +2596,14 @@ void r8169_apply_firmware(struct rtl8169_private *tp)
 {
 	int val;
 
-	/* TODO: release firmware if rtl_fw_write_firmware signals failure. */
 	if (tp->rtl_fw) {
-		rtl_fw_write_firmware(tp, tp->rtl_fw);
+		int rc = rtl_fw_write_firmware(tp, tp->rtl_fw);
 		/* At least one firmware doesn't reset tp->ocp_base. */
 		tp->ocp_base = OCP_STD_PHY_BASE;
 
+		if (rc < 0)
+			rtl_release_firmware(tp);
+
 		/* PHY soft reset may still be in progress */
 		if (tp->phydev)
 			phy_read_poll_timeout(tp->phydev, MII_BMCR, val,
-- 
2.43.0

Re: [PATCH net-next 1/3] r8169: propagate errors from PHY write operations

[Andrew Lunn]

> -static void r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
> +static int r8168_phy_ocp_write(struct rtl8169_private *tp, u32 reg, u32 data)
>  {
>  	if (rtl_ocp_reg_failure(reg))
> -		return;
> +		return 0;
>  
>  	RTL_W32(tp, GPHY_OCP, OCPAR_FLAG | (reg << 15) | data);
>  
> -	rtl_loop_wait_low(tp, &rtl_ocp_gphy_cond, 25, 10);
> +	return rtl_loop_wait_low(tp, &rtl_ocp_gphy_cond, 25, 10) ?
> +		0 : -ETIMEDOUT;

Maybe rewrite rtl_loop_wait_low(), _high() and rtl_loop_wait() to use
iopoll.h?

	Andrew

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

[Andrew Lunn]

On Wed, Sep 16, 2026 at 12:24:44PM -0300, Matheus Alves de Almeida wrote:
> After an rtl_fw_write_firmware() failure, r8169_apply_firmware() does
> not release the firmware. A TODO notes that it should be released
> in this case.
> 
> Release the firmware on failure and remove the TODO.
> 
> Signed-off-by: Matheus Alves de Almeida <matheus.aalmeida@inf.ufrgs.br>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 3c37c5a6c..c54423cd2 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -2596,12 +2596,14 @@ void r8169_apply_firmware(struct rtl8169_private *tp)
>  {
>  	int val;
>  
> -	/* TODO: release firmware if rtl_fw_write_firmware signals failure. */
>  	if (tp->rtl_fw) {
> -		rtl_fw_write_firmware(tp, tp->rtl_fw);
> +		int rc = rtl_fw_write_firmware(tp, tp->rtl_fw);
>  		/* At least one firmware doesn't reset tp->ocp_base. */
>  		tp->ocp_base = OCP_STD_PHY_BASE;
>  
> +		if (rc < 0)
> +			rtl_release_firmware(tp);
> +
>  		/* PHY soft reset may still be in progress */
>  		if (tp->phydev)
>  			phy_read_poll_timeout(tp->phydev, MII_BMCR, val,

If the firmware cannot be written, is the device dead? Should this
return an error, so the caller can abort the probe?

       Andrew

Re: [PATCH net-next 1/3] r8169: propagate errors from PHY write operations

[Matheus Alves de Almeida]

> Maybe rewrite rtl_loop_wait_low(), _high() and rtl_loop_wait() to use
> iopoll.h?

While I don’t think this fits the scope of this series, I’d be willing
to look into it as a follow-up.

Matheus Alves de Almeida

Re: [PATCH net-next 1/3] r8169: propagate errors from PHY write operations

[Andrew Lunn]

On Wed, Sep 16, 2026 at 06:10:00PM -0300, Matheus Alves de Almeida wrote:
> > Maybe rewrite rtl_loop_wait_low(), _high() and rtl_loop_wait() to use
> > iopoll.h?
> 
> While I don’t think this fits the scope of this series, I’d be willing
> to look into it as a follow-up.

You trimmed too much context.

The nice thing about iopoll.h is that they all return -ETIMEDOUT, or
some other error code on error. So your current patch looking at the
Boolean return value becomes redundant, you just follow the normal
pattern:

       ret = rtl_loop_wait_low();
       if (ret)
       	  return ret;

So i think this is in scope, otherwise you are going to rewrite 90% of
this patch when you do introduce it.

     Andrew

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

[Matheus Alves de Almeida]

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

Re: [PATCH net-next 1/3] r8169: propagate errors from PHY write operations

[Matheus Alves de Almeida]

On 2026-09-16 18:14, Andrew Lunn Wrote:
> On Wed, Sep 16, 2026 at 06:10:00PM -0300, Matheus Alves de Almeida wrote:
>> > Maybe rewrite rtl_loop_wait_low(), _high() and rtl_loop_wait() to use
>> > iopoll.h?
>> 
>> While I don’t think this fits the scope of this series, I’d be willing
>> to look into it as a follow-up.
> 
> You trimmed too much context.
> 
> The nice thing about iopoll.h is that they all return -ETIMEDOUT, or
> some other error code on error. So your current patch looking at the
> Boolean return value becomes redundant, you just follow the normal
> pattern:
> 
>        ret = rtl_loop_wait_low();
>        if (ret)
>        	  return ret;
> 
> So i think this is in scope, otherwise you are going to rewrite 90% of
> this patch when you do introduce it.
> 
>      Andrew

Sorry about trimming too much context. That makes sense. I was thinking
of the iopoll.h conversion primarily as cleanup. I’ll rework this for v2.

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

[Andrew Lunn]

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

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

[Matheus Alves de Almeida]

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

You're right. It is one of the first things that happen, so checking it
makes sense. I will make firmware application errors propagate up to
rtl_open() in v2.

Matheus

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

[Matheus Alves de Almeida]

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?

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

[Andrew Lunn]

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

I don't know this driver in detail, but fatal errors generally don't
cause state problem. The probe method fails, so the device is
destroyed. Is firmware download not performed in probe?

This driver does support a number of different devices. How many do
you have for testing? If you artificially cause firmware download to
fail, is the device usable?

	   Andrew