Re: [PATCH net v2 1/2] r8169: add handling DASH when DASH is disabled

[Heiner Kallweit <hkallweit1@gmail.com>]

On 09.11.2023 12:49, Paolo Abeni wrote:
> On Thu, 2023-11-09 at 02:48 +0800, ChunHao Lin wrote:
>> For devices that support DASH, even DASH is disabled, there may still
>> exist a default firmware that will influence device behavior.
>> So driver needs to handle DASH for devices that support DASH, no
>> matter the DASH status is.
>>
>> This patch also prepare for "fix DASH deviceis network lost issue".
>>
>> Signed-off-by: ChunHao Lin <hau@realtek.com>
> 
> You should include the fixes tag you already added in v1 and your Sob
> should come as the last tag
> 
> The same applies to the next patch 
> 
>> Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> It's not clear where/when Heiner provided the above tag for this patch.
> I hope that was off-list.
> 
Right, so far I added my Rb for patch 2 only. Will have a look at patch 1
again once there's a version with your review comments having been addressed.

>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 35 ++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 0c76c162b8a9..108dc75050ba 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -624,6 +624,7 @@ struct rtl8169_private {
>>  
>>  	unsigned supports_gmii:1;
>>  	unsigned aspm_manageable:1;
>> +	unsigned dash_enabled:1;
>>  	dma_addr_t counters_phys_addr;
>>  	struct rtl8169_counters *counters;
>>  	struct rtl8169_tc_offsets tc_offset;
>> @@ -1253,14 +1254,26 @@ static bool r8168ep_check_dash(struct rtl8169_private *tp)
>>  	return r8168ep_ocp_read(tp, 0x128) & BIT(0);
>>  }
>>  
>> -static enum rtl_dash_type rtl_check_dash(struct rtl8169_private *tp)
>> +static bool rtl_dash_is_enabled(struct rtl8169_private *tp)
>> +{
>> +	switch (tp->dash_type) {
>> +	case RTL_DASH_DP:
>> +		return r8168dp_check_dash(tp);
>> +	case RTL_DASH_EP:
>> +		return r8168ep_check_dash(tp);
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static enum rtl_dash_type rtl_get_dash_type(struct rtl8169_private *tp)
>>  {
>>  	switch (tp->mac_version) {
>>  	case RTL_GIGA_MAC_VER_28:
>>  	case RTL_GIGA_MAC_VER_31:
>> -		return r8168dp_check_dash(tp) ? RTL_DASH_DP : RTL_DASH_NONE;
>> +		return RTL_DASH_DP;
>>  	case RTL_GIGA_MAC_VER_51 ... RTL_GIGA_MAC_VER_53:
>> -		return r8168ep_check_dash(tp) ? RTL_DASH_EP : RTL_DASH_NONE;
>> +		return RTL_DASH_EP;
>>  	default:
>>  		return RTL_DASH_NONE;
>>  	}
>> @@ -1453,7 +1466,7 @@ static void __rtl8169_set_wol(struct rtl8169_private *tp, u32 wolopts)
>>  
>>  	device_set_wakeup_enable(tp_to_dev(tp), wolopts);
>>  
>> -	if (tp->dash_type == RTL_DASH_NONE) {
>> +	if (!tp->dash_enabled) {
>>  		rtl_set_d3_pll_down(tp, !wolopts);
>>  		tp->dev->wol_enabled = wolopts ? 1 : 0;
>>  	}
>> @@ -2512,7 +2525,7 @@ static void rtl_wol_enable_rx(struct rtl8169_private *tp)
>>  
>>  static void rtl_prepare_power_down(struct rtl8169_private *tp)
>>  {
>> -	if (tp->dash_type != RTL_DASH_NONE)
>> +	if (tp->dash_enabled)
>>  		return;
>>  
>>  	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
>> @@ -4869,7 +4882,7 @@ static int rtl8169_runtime_idle(struct device *device)
>>  {
>>  	struct rtl8169_private *tp = dev_get_drvdata(device);
>>  
>> -	if (tp->dash_type != RTL_DASH_NONE)
>> +	if (tp->dash_enabled)
>>  		return -EBUSY;
>>  
>>  	if (!netif_running(tp->dev) || !netif_carrier_ok(tp->dev))
>> @@ -4896,7 +4909,7 @@ static void rtl_shutdown(struct pci_dev *pdev)
>>  	rtl_rar_set(tp, tp->dev->perm_addr);
>>  
>>  	if (system_state == SYSTEM_POWER_OFF &&
>> -	    tp->dash_type == RTL_DASH_NONE) {
>> +		!tp->dash_enabled) {
> 
> Since you have to repost, please maintain the correct indentation
> above:
> 
> 	if (system_state == SYSTEM_POWER_OFF &&
> 	    !tp->dash_enabled) {
> 
>         ^^^^
> spaces here.
> 
> 
> Cheers,
> 
> Paolo
>