| Message ID | 20240322034617.23742-1-atlas.yu@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | r8169: skip DASH fw status checks when DASH is disabled | expand |
On 22.03.2024 04:46, pseudoc 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. > To me this seems to be somewhat in conflict with the commit message of the original change. There's a statement that DASH firmware may influence driver behavior even if DASH is disabled. I think we have to consider three cases in the driver: 1. DASH enabled (implies firmware is present) 2. DASH disabled (firmware present) 3. DASH disabled (no firmware) I assume your change is for case 3. Is there a way to detect firmware presence on driver load? > 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") SoB is missing > --- > drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c > index 5c879a5c86d7..a39520a3f41d 100644 > --- a/drivers/net/ethernet/realtek/r8169_main.c > +++ b/drivers/net/ethernet/realtek/r8169_main.c > @@ -1317,6 +1317,8 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private *tp) > static void rtl8168dp_driver_start(struct rtl8169_private *tp) > { > r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START); > + if (!tp->dash_enabled) > + return; > rtl_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); > } > > @@ -1324,6 +1326,8 @@ 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); > + if (!tp->dash_enabled) > + return; > rtl_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); > } > > @@ -1338,6 +1342,8 @@ 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); > + if (!tp->dash_enabled) > + return; > rtl_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); > } > > @@ -1346,6 +1352,8 @@ 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); > + if (!tp->dash_enabled) > + return; > rtl_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); > } >
Fri, Mar 22, 2024 at 08:01:40AM CET, hkallweit1@gmail.com wrote: >On 22.03.2024 04:46, pseudoc 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. >> > >To me this seems to be somewhat in conflict with the commit message of the >original change. There's a statement that DASH firmware may influence driver >behavior even if DASH is disabled. >I think we have to consider three cases in the driver: >1. DASH enabled (implies firmware is present) >2. DASH disabled (firmware present) >3. DASH disabled (no firmware) > >I assume your change is for case 3. > >Is there a way to detect firmware presence on driver load? > >> 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") > >SoB is missing Also, please fix the From email header to contain the same proper name and email address as SoB tag. Also, indicate the targetting tree. Please make sure you read again: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html?highlight=network#tl-dr pw-bot: cr > >> --- >> drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c >> index 5c879a5c86d7..a39520a3f41d 100644 >> --- a/drivers/net/ethernet/realtek/r8169_main.c >> +++ b/drivers/net/ethernet/realtek/r8169_main.c >> @@ -1317,6 +1317,8 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private *tp) >> static void rtl8168dp_driver_start(struct rtl8169_private *tp) >> { >> r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START); >> + if (!tp->dash_enabled) >> + return; >> rtl_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); >> } >> >> @@ -1324,6 +1326,8 @@ 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); >> + if (!tp->dash_enabled) >> + return; >> rtl_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); >> } >> >> @@ -1338,6 +1342,8 @@ 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); >> + if (!tp->dash_enabled) >> + return; >> rtl_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); >> } >> >> @@ -1346,6 +1352,8 @@ 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); >> + if (!tp->dash_enabled) >> + return; >> rtl_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); >> } >> > >
On Fri, Mar 22, 2024 at 3:01 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > To me this seems to be somewhat in conflict with the commit message of the > original change. There's a statement that DASH firmware may influence driver > behavior even if DASH is disabled. > I think we have to consider three cases in the driver: > 1. DASH enabled (implies firmware is present) > 2. DASH disabled (firmware present) > 3. DASH disabled (no firmware) > I assume your change is for case 3. I checked the r8168 driver[1], for both DP and EP DASH types, "rtl8168_wait_dash_fw_ready" will immediately return if DASH is disabled. So I think the firmware presence doesn't really matter. > Is there a way to detect firmware presence on driver load? By comparing r8168_n.c and r8169_main.c, I think "rtl_ep_ocp_read_cond" and "rtl_dp_ocp_read_cond" is checking that, which is redundant when DASH is disabled. [1] r8168 driver: https://www.realtek.com/en/component/zoo/category/network-interface-controllers-10-100-1000m-gigabit-ethernet-pci-express-software
On 22.03.2024 09:33, pseudoc wrote: > On Fri, Mar 22, 2024 at 3:01 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: >> To me this seems to be somewhat in conflict with the commit message of the >> original change. There's a statement that DASH firmware may influence driver >> behavior even if DASH is disabled. >> I think we have to consider three cases in the driver: >> 1. DASH enabled (implies firmware is present) >> 2. DASH disabled (firmware present) >> 3. DASH disabled (no firmware) > >> I assume your change is for case 3. > I checked the r8168 driver[1], for both DP and EP DASH types, > "rtl8168_wait_dash_fw_ready" will immediately return if DASH is disabled. > So I think the firmware presence doesn't really matter. > >> Is there a way to detect firmware presence on driver load? > By comparing r8168_n.c and r8169_main.c, I think "rtl_ep_ocp_read_cond" and > "rtl_dp_ocp_read_cond" is checking that, which is redundant when DASH is disabled. > No, this only checks whether DASH is enabled. I don't think is redundant, because the original change explicitly mentions that DASH fw may impact behavior even if DASH is disabled. I understand that on your test system DASH is disabled. But does your system have a DASH fw or not? My assumption is that the poll loop is relevant on systems with DASH fw, even if DASH is disabled. I'd appreciate if somebody from Realtek could comment on this. Hau? Including the question whether DASH fw presence can be detected, even if DASH is disabled. > [1] r8168 driver: https://www.realtek.com/en/component/zoo/category/network-interface-controllers-10-100-1000m-gigabit-ethernet-pci-express-software
On Fri, Mar 22, 2024 at 6:16 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > No, this only checks whether DASH is enabled. > I don't think is redundant, because the original change explicitly mentions that > DASH fw may impact behavior even if DASH is disabled. I see, thanks for the clarification. > I understand that on your test system DASH is disabled. But does your system have > a DASH fw or not? I am not familiar with DASH, my system's DASH type is "RTL_DASH_EP", and I have no idea if it has a DASH firmware or not. I am glad to check it if you tell me how. My patched r8169 driver and r8168 driver both work well on my system. > My assumption is that the poll loop is relevant on systems with DASH fw, even if > DASH is disabled. I know your concern, but in my case it is wasting 300ms on driver startup. Maybe we can find a way to avoid this together.
On 22.03.2024 11:49, Atlas Yu wrote: > On Fri, Mar 22, 2024 at 6:16 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> No, this only checks whether DASH is enabled. >> I don't think is redundant, because the original change explicitly mentions that >> DASH fw may impact behavior even if DASH is disabled. > > I see, thanks for the clarification. > >> I understand that on your test system DASH is disabled. But does your system have >> a DASH fw or not? > > I am not familiar with DASH, my system's DASH type is "RTL_DASH_EP", and I have no > idea if it has a DASH firmware or not. I am glad to check it if you tell me how. I don't have access to datasheets and can't tell. Therefore I asked Realtek to comment. > My patched r8169 driver and r8168 driver both work well on my system. > >> My assumption is that the poll loop is relevant on systems with DASH fw, even if >> DASH is disabled. > > I know your concern, but in my case it is wasting 300ms on driver startup. Maybe > we can find a way to avoid this together. Before applying the change I'd like to ensure that it doesn't break anything on systems with a different DASH setup. So let's see whether Realtek provides some more insight.
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c index 5c879a5c86d7..a39520a3f41d 100644 --- a/drivers/net/ethernet/realtek/r8169_main.c +++ b/drivers/net/ethernet/realtek/r8169_main.c @@ -1317,6 +1317,8 @@ static void rtl8168ep_stop_cmac(struct rtl8169_private *tp) static void rtl8168dp_driver_start(struct rtl8169_private *tp) { r8168dp_oob_notify(tp, OOB_CMD_DRIVER_START); + if (!tp->dash_enabled) + return; rtl_loop_wait_high(tp, &rtl_dp_ocp_read_cond, 10000, 10); } @@ -1324,6 +1326,8 @@ 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); + if (!tp->dash_enabled) + return; rtl_loop_wait_high(tp, &rtl_ep_ocp_read_cond, 10000, 30); } @@ -1338,6 +1342,8 @@ 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); + if (!tp->dash_enabled) + return; rtl_loop_wait_low(tp, &rtl_dp_ocp_read_cond, 10000, 10); } @@ -1346,6 +1352,8 @@ 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); + if (!tp->dash_enabled) + return; rtl_loop_wait_low(tp, &rtl_ep_ocp_read_cond, 10000, 10); }
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") --- drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++++ 1 file changed, 8 insertions(+)