[PATCH] r8169: skip DASH fw status checks when DASH is disabled

[PATCH] r8169: skip DASH fw status checks when DASH is disabled

[pseudoc]

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(+)

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

Re: [PATCH] r8169: skip DASH fw status checks when DASH is disabled

[Heiner Kallweit]

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);
>  }
>  

Re: [PATCH] r8169: skip DASH fw status checks when DASH is disabled

[Jiri Pirko]

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);
>>  }
>>  
>
>

[PATCH v2] r8169: skip DASH fw status checks when DASH is disabled

[pseudoc]

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(+)

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

Re: [PATCH v2] r8169: skip DASH fw status checks when DASH is disabled

[Jiri Pirko]

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

Sorry Jiri Pirko

[pseudoc]

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.

Sorry for the spam, ignore the previous email please

[pseudoc]

Bear with me, I'm new to git send-email and linux kernel development in general.

Re: [PATCH v2] r8169: skip DASH fw status checks when DASH is disabled

[Paolo Abeni]

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

DRY rules - extract into rtl_cond_loop_wait_high()

[Atlas Yu]

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.

Re: DRY rules - extract into rtl_cond_loop_wait_high()

[Heiner Kallweit]

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.

DRY rules - extract into inline helper functions

[Atlas Yu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

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?

Re: DRY rules - extract into inline helper functions

[Heiner Kallweit]

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.

Re: [PATCH] r8169: skip DASH fw status checks when DASH is disabled

[pseudoc]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1051 bytes --]

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

Re: [PATCH] r8169: skip DASH fw status checks when DASH is disabled

[Heiner Kallweit]

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

Re: Heiner Kallweit

[Atlas Yu]

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.

Re: r8169 DASH-related issue

[Heiner Kallweit]

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.