| Message ID | 20240322082628.46272-1-atlas.yu@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] r8169: skip DASH fw status checks when DASH is disabled | expand |
Fri, Mar 22, 2024 at 09:26:28AM CET, atlas.yu@canonical.com 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: pseudoc <atlas.yu@canonical.com> Please use proper name here and in the "From:" email header. "pseudoc" certainly is not one. Also, when you submit a patch, please to that in new thread, never reply the old one. Also, please honour the 24h timeout before you send next patch version. Also, indicate the target tree in the [patch] brackets. Could you please read: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html?highlight=network#tl-dr Thanks! >--- > 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); > } > >-- >2.40.1 > >
Bear with me, I'm new to git send-email and linux kernel development in general.
I am sorry Jiri Pirko, I had sent the [PATCH v2] before I saw your reply. I will send it as [PATCH v2 net] tomorrow with a proper name in the "From:" and "Signed-off-by:" fields. Bear with me, I am new to this. Sorry for the inconvenience and thanks for your patience.
On Fri, 2024-03-22 at 16:26 +0800, pseudoc wrote: > 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); You are replicating this chunk several times. It would probably be better to create a new helper - say rtl_cond_loop_wait_high() or something similar - and use it where needed. Cheers, Paolo
On Tue, Mar 26, 2024 at 5:09 PM Paolo Abeni <pabeni@redhat.com> wrote: > > 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); > > You are replicating this chunk several times. It would probably be > better to create a new helper - say rtl_cond_loop_wait_high() or > something similar - and use it where needed. Sure, will do, thanks for the suggestion.
On 26.03.2024 11:08, Atlas Yu wrote: > On Tue, Mar 26, 2024 at 5:09 PM Paolo Abeni <pabeni@redhat.com> wrote: > >>> 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); >> >> You are replicating this chunk several times. It would probably be >> better to create a new helper - say rtl_cond_loop_wait_high() or >> something similar - and use it where needed. > > Sure, will do, thanks for the suggestion. cond like conditional would be a little too generic here IMO. Something like rtl_dash_loop_wait_high()/low() would make clear that the poll loop is relevant only if DASH is enabled.
On Wed, Mar 27, 2024 at 4:29 AM Heiner Kallweit <hkallweit1@gmail.com> wrote: > cond like conditional would be a little too generic here IMO. > Something like rtl_dash_loop_wait_high()/low() would make clear > that the poll loop is relevant only if DASH is enabled. I don't know if cond might be reused later somewhere, so I am thinking of creating both dash_loop_wait and cond_loop_wait. And specifying them to be inline functions explicitly. What do you think?
On 27.03.2024 03:15, Atlas Yu wrote: > On Wed, Mar 27, 2024 at 4:29 AM Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> cond like conditional would be a little too generic here IMO. >> Something like rtl_dash_loop_wait_high()/low() would make clear >> that the poll loop is relevant only if DASH is enabled. > > I don't know if cond might be reused later somewhere, so I am thinking of > creating both dash_loop_wait and cond_loop_wait. And specifying them to be > inline functions explicitly. What do you think? It's about replacing a very simple check in 6 places. So we shouldn't over-engineer the helpers. It's discouraged to use inline in source files. Kernel standard is to let the compiler decide.
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") Signed-off-by: pseudoc <atlas.yu@canonical.com> --- drivers/net/ethernet/realtek/r8169_main.c | 8 ++++++++ 1 file changed, 8 insertions(+)