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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zf0-cXhouMkgebDR@nanopsycho>
Date: Fri, 22 Mar 2024 09:16:49 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Heiner Kallweit <hkallweit1@...il.com>
Cc: pseudoc <atlas.yu@...onical.com>, nic_swsd@...ltek.com,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, ChunHao Lin <hau@...ltek.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH] r8169: skip DASH fw status checks when DASH is disabled

Fri, Mar 22, 2024 at 08:01:40AM CET, hkallweit1@...il.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);
>>  }
>>  
>
>