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

[Jiri Pirko <jiri@resnulli.us>]

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