diff mbox series

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

Message ID 20231108184849.2925-2-hau@realtek.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series r8169: fix DASH devices network lost issue | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1312 this patch: 1312
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 1340 this patch: 1340
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1340 this patch: 1340
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 94 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

ChunHao Lin Nov. 8, 2023, 6:48 p.m. UTC 
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>
Reviewed-by: Heiner Kallweit <hkallweit1@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/net/ethernet/realtek/r8169_main.c | 35 ++++++++++++++++-------
 1 file changed, 25 insertions(+), 10 deletions(-)

Comments

Paolo Abeni Nov. 9, 2023, 11:49 a.m. UTC | #1 
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.

> 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
ChunHao Lin Nov. 9, 2023, 4:20 p.m. UTC | #2 
> 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
> 
I forget to add Fixes tag. I will add it back.

> > 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.
I may misunderstanding what he means. I will not add this tag in my next patch.

> 
> > 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.
I will correct the indentation. Thanks.
Heiner Kallweit Nov. 9, 2023, 4:37 p.m. UTC | #3 
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
>
diff mbox series

Patch

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) {
 		pci_wake_from_d3(pdev, tp->saved_wolopts);
 		pci_set_power_state(pdev, PCI_D3hot);
 	}
@@ -5254,7 +5267,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_get_dash_type(tp);
+	tp->dash_enabled = rtl_dash_is_enabled(tp);
 
 	tp->cp_cmd = RTL_R16(tp, CPlusCmd) & CPCMD_MASK;
 
@@ -5325,7 +5339,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_enabled) {
 		rtl_set_d3_pll_down(tp, true);
 	} else {
 		rtl_set_d3_pll_down(tp, false);
@@ -5365,7 +5379,8 @@  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");
+		netdev_info(dev, "DASH %s\n",
+			    tp->dash_enabled ? "enabled" : "disabled");
 		rtl8168_driver_start(tp);
 	}