[PATCH net 0/2] r8169: fix DASH deviceis network lost issue

[PATCH net 0/2] r8169: fix DASH deviceis network lost issue

[ChunHao Lin]

This series are used to fix network lost issue on systems that support
DASH.

ChunHao Lin (2):
  r8169: add handling DASH when DASH is disabled
  r8169: fix network lost after resume on DASH systems

 drivers/net/ethernet/realtek/r8169_main.c | 43 +++++++++++++++++------
 1 file changed, 33 insertions(+), 10 deletions(-)

-- 
2.39.2

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

[ChunHao Lin]

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.

Fixes: ee7a1beb9759 ("r8169:call "rtl8168_driver_start" "rtl8168_driver_stop" only when hardware dash function is enabled")
Signed-off-by: ChunHao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 37 +++++++++++++++++------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 4b8251cdb436..8cbd7c96d9e1 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_enable: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_check_dash(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 0;
+	}
+}
+
+static enum rtl_dash_type rtl_check_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_enable) {
 		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_enable)
 		return;
 
 	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
@@ -4867,7 +4880,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_enable)
 		return -EBUSY;
 
 	if (!netif_running(tp->dev) || !netif_carrier_ok(tp->dev))
@@ -4894,7 +4907,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_enable) {
 		pci_wake_from_d3(pdev, tp->saved_wolopts);
 		pci_set_power_state(pdev, PCI_D3hot);
 	}
@@ -5252,7 +5265,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
 	tp->aspm_manageable = !rc;
 
-	tp->dash_type = rtl_check_dash(tp);
+	tp->dash_type = rtl_check_dash_type(tp);
+	tp->dash_enable = rtl_check_dash(tp);
 
 	tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
 
@@ -5323,7 +5337,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* configure chip for default features */
 	rtl8169_set_features(dev, dev->features);
 
-	if (tp->dash_type == RTL_DASH_NONE) {
+	if (!tp->dash_enable) {
 		rtl_set_d3_pll_down(tp, true);
 	} else {
 		rtl_set_d3_pll_down(tp, false);
@@ -5363,7 +5377,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 			    "ok" : "ko");
 
 	if (tp->dash_type != RTL_DASH_NONE) {
-		netdev_info(dev, "DASH enabled\n");
+		if (tp->dash_enable)
+			netdev_info(dev, "DASH enabled\n");
+		else
+			netdev_info(dev, "DASH disabled\n");
 		rtl8168_driver_start(tp);
 	}
 
-- 
2.39.2

[PATCH net 2/2] r8169: fix network lost after resume on DASH systems

[ChunHao Lin]

Device that support DASH may be reseted or powered off during suspend.
So driver needs to handle DASH during system suspend and resume. Or
DASH firmware will influence device behavior and causes network lost.

Fixes: b646d90053f8 ("r8169: magic.")
Signed-off-by: ChunHao Lin <hau@realtek.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 8cbd7c96d9e1..cf32993992bc 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -4659,10 +4659,16 @@ static void rtl8169_down(struct rtl8169_private *tp)
 	rtl8169_cleanup(tp);
 	rtl_disable_exit_l1(tp);
 	rtl_prepare_power_down(tp);
+
+	if (tp->dash_type != RTL_DASH_NONE)
+		rtl8168_driver_stop(tp);
 }
 
 static void rtl8169_up(struct rtl8169_private *tp)
 {
+	if (tp->dash_type != RTL_DASH_NONE)
+		rtl8168_driver_start(tp);
+
 	pci_set_master(tp->pci_dev);
 	phy_init_hw(tp->phydev);
 	phy_resume(tp->phydev);
-- 
2.39.2

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

[Heiner Kallweit]

On 06.11.2023 16:11, 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.
> 
AFAICS there's no functional change in patch 1 (except the dash disabled
message). It just prepares patch 2. That's worth mentioning.

> Fixes: ee7a1beb9759 ("r8169:call "rtl8168_driver_start" "rtl8168_driver_stop" only when hardware dash function is enabled")
> Signed-off-by: ChunHao Lin <hau@realtek.com>

stable should be cc'ed

> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 37 +++++++++++++++++------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 4b8251cdb436..8cbd7c96d9e1 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_enable:1;

better: dash_enabled

>  	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_check_dash(struct rtl8169_private *tp)

maybe better: rtl_dash_is_enabled()

> +{
> +	switch (tp->dash_type) {
> +	case RTL_DASH_DP:
> +		return r8168dp_check_dash(tp);
> +	case RTL_DASH_EP:
> +		return r8168ep_check_dash(tp);
> +	default:
> +		return 0;

false instead of 0

> +	}
> +}
> +
> +static enum rtl_dash_type rtl_check_dash_type(struct rtl8169_private *tp)
>  {

maybe better: rtl_get_dash_type()

>  	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_enable) {
>  		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_enable)
>  		return;
>  
>  	if (tp->mac_version == RTL_GIGA_MAC_VER_32 ||
> @@ -4867,7 +4880,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_enable)
>  		return -EBUSY;
>  
>  	if (!netif_running(tp->dev) || !netif_carrier_ok(tp->dev))
> @@ -4894,7 +4907,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_enable) {
>  		pci_wake_from_d3(pdev, tp->saved_wolopts);
>  		pci_set_power_state(pdev, PCI_D3hot);
>  	}
> @@ -5252,7 +5265,8 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  		rc = pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
>  	tp->aspm_manageable = !rc;
>  
> -	tp->dash_type = rtl_check_dash(tp);
> +	tp->dash_type = rtl_check_dash_type(tp);
> +	tp->dash_enable = rtl_check_dash(tp);
>  
>  	tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
>  
> @@ -5323,7 +5337,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	/* configure chip for default features */
>  	rtl8169_set_features(dev, dev->features);
>  
> -	if (tp->dash_type == RTL_DASH_NONE) {
> +	if (!tp->dash_enable) {
>  		rtl_set_d3_pll_down(tp, true);
>  	} else {
>  		rtl_set_d3_pll_down(tp, false);
> @@ -5363,7 +5377,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  			    "ok" : "ko");
>  
>  	if (tp->dash_type != RTL_DASH_NONE) {
> -		netdev_info(dev, "DASH enabled\n");
> +		if (tp->dash_enable)
> +			netdev_info(dev, "DASH enabled\n");
> +		else
> +			netdev_info(dev, "DASH disabled\n");

alternative:
netdev_info(dev, "DASH %s\n", tp->dash_enabled ? "enabled" : "disabled")

>  		rtl8168_driver_start(tp);
>  	}
>  

Re: [PATCH net 0/2] r8169: fix DASH deviceis network lost issue

[Heiner Kallweit]

On 06.11.2023 16:11, ChunHao Lin wrote:
> This series are used to fix network lost issue on systems that support
> DASH.
> 
As I have no he to test this: Can you add on which hw you tested this patch?

> ChunHao Lin (2):
>   r8169: add handling DASH when DASH is disabled
>   r8169: fix network lost after resume on DASH systems
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 43 +++++++++++++++++------
>  1 file changed, 33 insertions(+), 10 deletions(-)
> 

Re: [PATCH net 2/2] r8169: fix network lost after resume on DASH systems

[Heiner Kallweit]

On 06.11.2023 16:11, ChunHao Lin wrote:
> Device that support DASH may be reseted or powered off during suspend.
> So driver needs to handle DASH during system suspend and resume. Or
> DASH firmware will influence device behavior and causes network lost.
> 
> Fixes: b646d90053f8 ("r8169: magic.")
> Signed-off-by: ChunHao Lin <hau@realtek.com>

Also here: cc stable
With this:

Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Re: [PATCH net 0/2] r8169: fix DASH deviceis network lost issue

[Jakub Kicinski]

On Mon, 6 Nov 2023 23:11:22 +0800 ChunHao Lin wrote:
> This series are used to fix network lost issue on systems that support
> DASH.

Please use get_maintainer on the patch files to make sure you CC all
relevant people.

RE: [PATCH net 2/2] r8169: fix network lost after resume on DASH systems

[Hau]

> > Device that support DASH may be reseted or powered off during suspend.
> > So driver needs to handle DASH during system suspend and resume. Or
> > DASH firmware will influence device behavior and causes network lost.
> >
> > Fixes: b646d90053f8 ("r8169: magic.")
> > Signed-off-by: ChunHao Lin <hau@realtek.com>
> 
> Also here: cc stable
> With this:
> 
> Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>

Thanks for your advices. I will update the code and submit the patch again.

RE: [PATCH net 0/2] r8169: fix DASH deviceis network lost issue

[Hau]

> > This series are used to fix network lost issue on systems that support
> > DASH.
> 
> Please use get_maintainer on the patch files to make sure you CC all relevant
> people.

I will do this before submit patch. Thanks.