| Message ID | 20240328055152.18443-1-atlas.yu@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5e864d90b20803edf6bd44a99fb9afa7171785f2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] r8169: skip DASH fw status checks when DASH is disabled | expand |
On 28.03.2024 06:51, Atlas Yu wrote: > On devices that support DASH, the current code in the "rtl_loop_wait" function > raises false alarms when DASH is disabled. This occurs because the function > attempts to wait for the DASH firmware to be ready, even though it's not > relevant in this case. > > r8169 0000:0c:00.0 eth0: RTL8168ep/8111ep, 38:7c:76:49:08:d9, XID 502, IRQ 86 > r8169 0000:0c:00.0 eth0: jumbo features [frames: 9194 bytes, tx checksumming: ko] > r8169 0000:0c:00.0 eth0: DASH disabled > ... > r8169 0000:0c:00.0 eth0: rtl_ep_ocp_read_cond == 0 (loop: 30, delay: 10000). > > This patch modifies the driver start/stop functions to skip checking the DASH > firmware status when DASH is explicitly disabled. This prevents unnecessary > delays and false alarms. > > The patch has been tested on several ThinkStation P8/PX workstations. > > Fixes: 0ab0c45d8aae ("r8169: add handling DASH when DASH is disabled") > Signed-off-by: Atlas Yu <atlas.yu@canonical.com> You sent a v2 already, so I think this is v3. And the change log is missing. But as the change is more or less trivial, no need to resubmit IMO. > --- > drivers/net/ethernet/realtek/r8169_main.c | 31 ++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 5c879a5c86d7..4ac444eb269f 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -1314,17 +1314,40 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private *tp) > RTL_W8(tp, IBCR0, RTL_R8(tp, IBCR0) & ~0x01); > } > > +static void rtl_dash_loop_wait(struct rtl8169_private *tp, > + const struct rtl_cond *c, > + unsigned long usecs, int n, bool high) > +{ > + if (!tp->dash_enabled) > + return; > + rtl_loop_wait(tp, c, usecs, n, high); > +} > + > +static void rtl_dash_loop_wait_high(struct rtl8169_private *tp, > + const struct rtl_cond *c, > + unsigned long d, int n) > +{ > + rtl_dash_loop_wait(tp, c, d, n, true); > +} > + > +static void rtl_dash_loop_wait_low(struct rtl8169_private *tp, > + const struct rtl_cond *c, > + unsigned long d, int n) > +{ > + rtl_dash_loop_wait(tp, c, d, n, false); > +} > + > static void rtl8168dp_driver_start(struct rtl8169_private *tp) > { > r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START); > - rtl_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); > + rtl_dash_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); > } > > static void rtl8168ep_driver_start(struct rtl8169_private *tp) > { > r8168ep_ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START); > r8168ep_ocp_write(tp, 0x01, 0x30, r8168ep_ocp_read(tp, 0x30) | 0x01); > - rtl_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); > + rtl_dash_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); > } > > static void rtl8168_driver_start(struct rtl8169_private *tp) > @@ -1338,7 +1361,7 @@ static void rtl8168_driver_start(struct rtl8169_private *tp) > static void rtl8168dp_driver_stop(struct rtl8169_private *tp) > { > r8168dp_oob_notify(tp, OOB_CMD_DRIVER_STOP); > - rtl_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); > + rtl_dash_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); > } > > static void rtl8168ep_driver_stop(struct rtl8169_private *tp) > @@ -1346,7 +1369,7 @@ static void rtl8168ep_driver_stop(struct rtl8169_private *tp) > rtl8168ep_stop_cmac(tp); > r8168ep_ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_STOP); > r8168ep_ocp_write(tp, 0x01, 0x30, r8168ep_ocp_read(tp, 0x30) | 0x01); > - rtl_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); > + rtl_dash_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); > } > > static void rtl8168_driver_stop(struct rtl8169_private *tp) Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>
On Thu, Mar 28, 2024 at 2:49 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > You sent a v2 already, so I think this is v3. And the change log is missing. > But as the change is more or less trivial, no need to resubmit IMO. I'm sorry for the confusion. I throught that v2 does not count, since I sent that within 24 hours and instead of creating a new thread, I just replied to the original thread. I will make sure to include the change log in the next patch submission. Let me check other contributors' patches to see how they format their change log.
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Thu, 28 Mar 2024 13:51:52 +0800 you wrote: > On devices that support DASH, the current code in the "rtl_loop_wait" function > raises false alarms when DASH is disabled. This occurs because the function > attempts to wait for the DASH firmware to be ready, even though it's not > relevant in this case. > > r8169 0000:0c:00.0 eth0: RTL8168ep/8111ep, 38:7c:76:49:08:d9, XID 502, IRQ 86 > r8169 0000:0c:00.0 eth0: jumbo features [frames: 9194 bytes, tx checksumming: ko] > r8169 0000:0c:00.0 eth0: DASH disabled > ... > r8169 0000:0c:00.0 eth0: rtl_ep_ocp_read_cond == 0 (loop: 30, delay: 10000). > > [...] Here is the summary with links: - [net,v2] r8169: skip DASH fw status checks when DASH is disabled https://git.kernel.org/netdev/net/c/5e864d90b208 You are awesome, thank you!
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 5c879a5c86d7..4ac444eb269f 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -1314,17 +1314,40 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private *tp) RTL_W8(tp, IBCR0, RTL_R8(tp, IBCR0) & ~0x01); } +static void rtl_dash_loop_wait(struct rtl8169_private *tp, + const struct rtl_cond *c, + unsigned long usecs, int n, bool high) +{ + if (!tp->dash_enabled) + return; + rtl_loop_wait(tp, c, usecs, n, high); +} + +static void rtl_dash_loop_wait_high(struct rtl8169_private *tp, + const struct rtl_cond *c, + unsigned long d, int n) +{ + rtl_dash_loop_wait(tp, c, d, n, true); +} + +static void rtl_dash_loop_wait_low(struct rtl8169_private *tp, + const struct rtl_cond *c, + unsigned long d, int n) +{ + rtl_dash_loop_wait(tp, c, d, n, false); +} + static void rtl8168dp_driver_start(struct rtl8169_private *tp) { r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START); - rtl_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); + rtl_dash_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); } static void rtl8168ep_driver_start(struct rtl8169_private *tp) { r8168ep_ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_START); r8168ep_ocp_write(tp, 0x01, 0x30, r8168ep_ocp_read(tp, 0x30) | 0x01); - rtl_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); + rtl_dash_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); } static void rtl8168_driver_start(struct rtl8169_private *tp) @@ -1338,7 +1361,7 @@ static void rtl8168_driver_start(struct rtl8169_private *tp) static void rtl8168dp_driver_stop(struct rtl8169_private *tp) { r8168dp_oob_notify(tp, OOB_CMD_DRIVER_STOP); - rtl_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); + rtl_dash_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); } static void rtl8168ep_driver_stop(struct rtl8169_private *tp) @@ -1346,7 +1369,7 @@ static void rtl8168ep_driver_stop(struct rtl8169_private *tp) rtl8168ep_stop_cmac(tp); r8168ep_ocp_write(tp, 0x01, 0x180, OOB_CMD_DRIVER_STOP); r8168ep_ocp_write(tp, 0x01, 0x30, r8168ep_ocp_read(tp, 0x30) | 0x01); - rtl_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); + rtl_dash_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); } static void rtl8168_driver_stop(struct rtl8169_private *tp)
On devices that support DASH, the current code in the "rtl_loop_wait" function raises false alarms when DASH is disabled. This occurs because the function attempts to wait for the DASH firmware to be ready, even though it's not relevant in this case. r8169 0000:0c:00.0 eth0: RTL8168ep/8111ep, 38:7c:76:49:08:d9, XID 502, IRQ 86 r8169 0000:0c:00.0 eth0: jumbo features [frames: 9194 bytes, tx checksumming: ko] r8169 0000:0c:00.0 eth0: DASH disabled ... r8169 0000:0c:00.0 eth0: rtl_ep_ocp_read_cond == 0 (loop: 30, delay: 10000). This patch modifies the driver start/stop functions to skip checking the DASH firmware status when DASH is explicitly disabled. This prevents unnecessary delays and false alarms. The patch has been tested on several ThinkStation P8/PX workstations. Fixes: 0ab0c45d8aae ("r8169: add handling DASH when DASH is disabled") Signed-off-by: Atlas Yu <atlas.yu@canonical.com> --- drivers/net/ethernet/realtek/r8169_main.c | 31 ++++++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-)