[PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage

Choong Yong Liang posted 2 patches 1 week ago
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
drivers/net/phy/phy-c45.c                         | 11 +++++------
drivers/net/phy/phy_device.c                      |  6 +++---
include/linux/phy.h                               |  5 ++---
4 files changed, 13 insertions(+), 12 deletions(-)
[PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Choong Yong Liang 1 week ago
From: Choong Yong Liang <yong.liang.choong@intel.com>

When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
the driver side is disabled. If we try to enable EEE through
'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
matches the setting required to enable EEE in ethnl_set_eee().

This patch series will remove phydev->eee_enabled and replace it with
eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
will follow the master configuration to have software and hardware in sync,
allowing 'ethtool --show-eee' to display the correct value during the
initial stage.

v2 changes:
 - Implement the prototype suggested by Russell
 - Check EEE before calling phy_support_eee()

Thanks to Russell for the proposed prototype in [1].

Reference:
[1] https://patchwork.kernel.org/comment/26121323/

Choong Yong Liang (2):
  net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
  net: stmmac: set initial EEE policy configuration

 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
 drivers/net/phy/phy-c45.c                         | 11 +++++------
 drivers/net/phy/phy_device.c                      |  6 +++---
 include/linux/phy.h                               |  5 ++---
 4 files changed, 13 insertions(+), 12 deletions(-)

-- 
2.34.1
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Heiner Kallweit 1 week ago
On 15.11.2024 12:11, Choong Yong Liang wrote:
> From: Choong Yong Liang <yong.liang.choong@intel.com>
> 
> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> the driver side is disabled. If we try to enable EEE through
> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> matches the setting required to enable EEE in ethnl_set_eee().
> 
> This patch series will remove phydev->eee_enabled and replace it with
> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> will follow the master configuration to have software and hardware in sync,
> allowing 'ethtool --show-eee' to display the correct value during the
> initial stage.
> 
> v2 changes:
>  - Implement the prototype suggested by Russell
>  - Check EEE before calling phy_support_eee()
> 
> Thanks to Russell for the proposed prototype in [1].
> 
> Reference:
> [1] https://patchwork.kernel.org/comment/26121323/
> 
> Choong Yong Liang (2):
>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>   net: stmmac: set initial EEE policy configuration
> 
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
>  drivers/net/phy/phy-c45.c                         | 11 +++++------
>  drivers/net/phy/phy_device.c                      |  6 +++---
>  include/linux/phy.h                               |  5 ++---
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 

Russell submitted the proposed patch already:
https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
So there's no need for your patch 1.
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Russell King (Oracle) 1 week ago
On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
> On 15.11.2024 12:11, Choong Yong Liang wrote:
> > From: Choong Yong Liang <yong.liang.choong@intel.com>
> > 
> > When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> > the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> > the driver side is disabled. If we try to enable EEE through
> > 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> > matches the setting required to enable EEE in ethnl_set_eee().
> > 
> > This patch series will remove phydev->eee_enabled and replace it with
> > eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> > will follow the master configuration to have software and hardware in sync,
> > allowing 'ethtool --show-eee' to display the correct value during the
> > initial stage.
> > 
> > v2 changes:
> >  - Implement the prototype suggested by Russell
> >  - Check EEE before calling phy_support_eee()
> > 
> > Thanks to Russell for the proposed prototype in [1].
> > 
> > Reference:
> > [1] https://patchwork.kernel.org/comment/26121323/
> > 
> > Choong Yong Liang (2):
> >   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
> >   net: stmmac: set initial EEE policy configuration
> > 
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
> >  drivers/net/phy/phy-c45.c                         | 11 +++++------
> >  drivers/net/phy/phy_device.c                      |  6 +++---
> >  include/linux/phy.h                               |  5 ++---
> >  4 files changed, 13 insertions(+), 12 deletions(-)
> > 
> 
> Russell submitted the proposed patch already:
> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
> So there's no need for your patch 1.

Patch 1 is an updated version of that patch, minus my authorship and of
course no sign-off. I've already marked this series as requiring changes
in patchwork (hopefully, if I did it correctly.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Heiner Kallweit 6 days, 20 hours ago
On 15.11.2024 15:12, Russell King (Oracle) wrote:
> On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
>> On 15.11.2024 12:11, Choong Yong Liang wrote:
>>> From: Choong Yong Liang <yong.liang.choong@intel.com>
>>>
>>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
>>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
>>> the driver side is disabled. If we try to enable EEE through
>>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
>>> matches the setting required to enable EEE in ethnl_set_eee().
>>>
>>> This patch series will remove phydev->eee_enabled and replace it with
>>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
>>> will follow the master configuration to have software and hardware in sync,
>>> allowing 'ethtool --show-eee' to display the correct value during the
>>> initial stage.
>>>
>>> v2 changes:
>>>  - Implement the prototype suggested by Russell
>>>  - Check EEE before calling phy_support_eee()
>>>
>>> Thanks to Russell for the proposed prototype in [1].
>>>
>>> Reference:
>>> [1] https://patchwork.kernel.org/comment/26121323/
>>>
>>> Choong Yong Liang (2):
>>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>>>   net: stmmac: set initial EEE policy configuration
>>>
>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
>>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
>>>  drivers/net/phy/phy_device.c                      |  6 +++---
>>>  include/linux/phy.h                               |  5 ++---
>>>  4 files changed, 13 insertions(+), 12 deletions(-)
>>>
>>
>> Russell submitted the proposed patch already:
>> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
>> So there's no need for your patch 1.
> 
> Patch 1 is an updated version of that patch, minus my authorship and of
> course no sign-off. I've already marked this series as requiring changes
> in patchwork (hopefully, if I did it correctly.)
> 

The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
and I wonder whether we can do better, as this results in several calls
with the same argument. The following is an alternative, to be applied
on top of your original patch. I don't have a clear preference, though.

---
 drivers/net/phy/phy.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8876f3673..22c9bbebb 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  * configuration.
  */
 static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
-				      struct ethtool_keee *data)
+				      struct ethtool_keee *old_cfg)
 {
-	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
-	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
-		eee_to_eeecfg(&phydev->eee_cfg, data);
+	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
+	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
 		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
 		if (phydev->link) {
 			phydev->link = false;
@@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
 {
+	struct eee_config old_cfg;
 	int ret;
 
 	if (!phydev->drv)
 		return -EIO;
 
+	old_cfg = phydev->eee_cfg;
+	eee_to_eeecfg(&phydev->eee_cfg, data);
+
 	mutex_lock(&phydev->lock);
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
-	if (ret >= 0) {
-		if (ret == 0)
-			phy_ethtool_set_eee_noneg(phydev, data);
-		eee_to_eeecfg(&phydev->eee_cfg, data);
-	}
+	if (ret == 0)
+		phy_ethtool_set_eee_noneg(phydev, data);
 	mutex_unlock(&phydev->lock);
 
-	return ret < 0 ? ret : 0;
+	if (ret < 0) {
+		phydev->eee_cfg = old_cfg;
+		return ret;
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(phy_ethtool_set_eee);
 
-- 
2.47.0
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Russell King (Oracle) 6 days, 1 hour ago
On Fri, Nov 15, 2024 at 09:35:25PM +0100, Heiner Kallweit wrote:
> On 15.11.2024 15:12, Russell King (Oracle) wrote:
> > On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
> >> On 15.11.2024 12:11, Choong Yong Liang wrote:
> >>> From: Choong Yong Liang <yong.liang.choong@intel.com>
> >>>
> >>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
> >>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
> >>> the driver side is disabled. If we try to enable EEE through
> >>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
> >>> matches the setting required to enable EEE in ethnl_set_eee().
> >>>
> >>> This patch series will remove phydev->eee_enabled and replace it with
> >>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
> >>> will follow the master configuration to have software and hardware in sync,
> >>> allowing 'ethtool --show-eee' to display the correct value during the
> >>> initial stage.
> >>>
> >>> v2 changes:
> >>>  - Implement the prototype suggested by Russell
> >>>  - Check EEE before calling phy_support_eee()
> >>>
> >>> Thanks to Russell for the proposed prototype in [1].
> >>>
> >>> Reference:
> >>> [1] https://patchwork.kernel.org/comment/26121323/
> >>>
> >>> Choong Yong Liang (2):
> >>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
> >>>   net: stmmac: set initial EEE policy configuration
> >>>
> >>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
> >>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
> >>>  drivers/net/phy/phy_device.c                      |  6 +++---
> >>>  include/linux/phy.h                               |  5 ++---
> >>>  4 files changed, 13 insertions(+), 12 deletions(-)
> >>>
> >>
> >> Russell submitted the proposed patch already:
> >> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
> >> So there's no need for your patch 1.
> > 
> > Patch 1 is an updated version of that patch, minus my authorship and of
> > course no sign-off. I've already marked this series as requiring changes
> > in patchwork (hopefully, if I did it correctly.)
> > 
> 
> The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
> and I wonder whether we can do better, as this results in several calls
> with the same argument. The following is an alternative, to be applied
> on top of your original patch. I don't have a clear preference, though.
> 
> ---
>  drivers/net/phy/phy.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 8876f3673..22c9bbebb 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>   * configuration.
>   */
>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
> -				      struct ethtool_keee *data)
> +				      struct ethtool_keee *old_cfg)
>  {
> -	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
> -	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
> -		eee_to_eeecfg(&phydev->eee_cfg, data);
> +	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
> +	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
>  		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>  		if (phydev->link) {
>  			phydev->link = false;
> @@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>   */
>  int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
>  {
> +	struct eee_config old_cfg;
>  	int ret;
>  
>  	if (!phydev->drv)
>  		return -EIO;
>  
> +	old_cfg = phydev->eee_cfg;
> +	eee_to_eeecfg(&phydev->eee_cfg, data);
> +

Hmm, don't we want to do this under phydev->lock, because network
drivers and phylib may be reading from phydev->eee_cfg? If we
update it outside the lock, and then revert, there's a chance that
the phylib state machine / network driver may see the changes
which then get reverted on failure, potentially leading to
inconsistent state.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Heiner Kallweit 5 days, 23 hours ago
On 16.11.2024 16:34, Russell King (Oracle) wrote:
> On Fri, Nov 15, 2024 at 09:35:25PM +0100, Heiner Kallweit wrote:
>> On 15.11.2024 15:12, Russell King (Oracle) wrote:
>>> On Fri, Nov 15, 2024 at 02:41:54PM +0100, Heiner Kallweit wrote:
>>>> On 15.11.2024 12:11, Choong Yong Liang wrote:
>>>>> From: Choong Yong Liang <yong.liang.choong@intel.com>
>>>>>
>>>>> When the MAC boots up with a Marvell PHY and phy_support_eee() is implemented,
>>>>> the 'ethtool --show-eee' command shows that EEE is enabled, but in actuality,
>>>>> the driver side is disabled. If we try to enable EEE through
>>>>> 'ethtool --set-eee' for a Marvell PHY, nothing happens because the eee_cfg
>>>>> matches the setting required to enable EEE in ethnl_set_eee().
>>>>>
>>>>> This patch series will remove phydev->eee_enabled and replace it with
>>>>> eee_cfg.eee_enabled. When performing genphy_c45_an_config_eee_aneg(), it
>>>>> will follow the master configuration to have software and hardware in sync,
>>>>> allowing 'ethtool --show-eee' to display the correct value during the
>>>>> initial stage.
>>>>>
>>>>> v2 changes:
>>>>>  - Implement the prototype suggested by Russell
>>>>>  - Check EEE before calling phy_support_eee()
>>>>>
>>>>> Thanks to Russell for the proposed prototype in [1].
>>>>>
>>>>> Reference:
>>>>> [1] https://patchwork.kernel.org/comment/26121323/
>>>>>
>>>>> Choong Yong Liang (2):
>>>>>   net: phy: replace phydev->eee_enabled with eee_cfg.eee_enabled
>>>>>   net: stmmac: set initial EEE policy configuration
>>>>>
>>>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |  3 +++
>>>>>  drivers/net/phy/phy-c45.c                         | 11 +++++------
>>>>>  drivers/net/phy/phy_device.c                      |  6 +++---
>>>>>  include/linux/phy.h                               |  5 ++---
>>>>>  4 files changed, 13 insertions(+), 12 deletions(-)
>>>>>
>>>>
>>>> Russell submitted the proposed patch already:
>>>> https://patchwork.kernel.org/project/netdevbpf/patch/E1tBXAF-00341F-EQ@rmk-PC.armlinux.org.uk/
>>>> So there's no need for your patch 1.
>>>
>>> Patch 1 is an updated version of that patch, minus my authorship and of
>>> course no sign-off. I've already marked this series as requiring changes
>>> in patchwork (hopefully, if I did it correctly.)
>>>
>>
>> The updated version adds an argument to genphy_c45_an_config_eee_aneg(),
>> and I wonder whether we can do better, as this results in several calls
>> with the same argument. The following is an alternative, to be applied
>> on top of your original patch. I don't have a clear preference, though.
>>
>> ---
>>  drivers/net/phy/phy.c | 25 +++++++++++++++----------
>>  1 file changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 8876f3673..22c9bbebb 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>>   * configuration.
>>   */
>>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>> -				      struct ethtool_keee *data)
>> +				      struct ethtool_keee *old_cfg)
>>  {
>> -	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
>> -	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
>> -		eee_to_eeecfg(&phydev->eee_cfg, data);
>> +	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
>> +	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
>>  		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
>>  		if (phydev->link) {
>>  			phydev->link = false;
>> @@ -1706,21 +1705,27 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>>   */
>>  int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
>>  {
>> +	struct eee_config old_cfg;
>>  	int ret;
>>  
>>  	if (!phydev->drv)
>>  		return -EIO;
>>  
>> +	old_cfg = phydev->eee_cfg;
>> +	eee_to_eeecfg(&phydev->eee_cfg, data);
>> +
> 
> Hmm, don't we want to do this under phydev->lock, because network
> drivers and phylib may be reading from phydev->eee_cfg? If we
> update it outside the lock, and then revert, there's a chance that
> the phylib state machine / network driver may see the changes
> which then get reverted on failure, potentially leading to
> inconsistent state.
> 

Good point, then the patch would look like this.
BTW: Saw that Jakub applied your patch already.

---
 drivers/net/phy/phy.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8876f3673..7f6594a66 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1682,11 +1682,10 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
  * configuration.
  */
 static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
-				      struct ethtool_keee *data)
+				      struct ethtool_keee *old_cfg)
 {
-	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
-	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
-		eee_to_eeecfg(&phydev->eee_cfg, data);
+	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
+	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
 		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
 		if (phydev->link) {
 			phydev->link = false;
@@ -1706,18 +1705,23 @@ static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
  */
 int phy_ethtool_set_eee(struct phy_device *phydev, struct ethtool_keee *data)
 {
+	struct eee_config old_cfg;
 	int ret;
 
 	if (!phydev->drv)
 		return -EIO;
 
 	mutex_lock(&phydev->lock);
+
+	old_cfg = phydev->eee_cfg;
+	eee_to_eeecfg(&phydev->eee_cfg, data);
+
 	ret = genphy_c45_ethtool_set_eee(phydev, data);
-	if (ret >= 0) {
-		if (ret == 0)
-			phy_ethtool_set_eee_noneg(phydev, data);
-		eee_to_eeecfg(&phydev->eee_cfg, data);
-	}
+	if (ret == 0)
+		phy_ethtool_set_eee_noneg(phydev, data);
+	else if (ret < 0)
+		phydev->eee_cfg = old_cfg;
+
 	mutex_unlock(&phydev->lock);
 
 	return ret < 0 ? ret : 0;
-- 
2.47.0
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Russell King (Oracle) 5 days, 23 hours ago
On Sat, Nov 16, 2024 at 06:41:13PM +0100, Heiner Kallweit wrote:
> On 16.11.2024 16:34, Russell King (Oracle) wrote:
> > Hmm, don't we want to do this under phydev->lock, because network
> > drivers and phylib may be reading from phydev->eee_cfg? If we
> > update it outside the lock, and then revert, there's a chance that
> > the phylib state machine / network driver may see the changes
> > which then get reverted on failure, potentially leading to
> > inconsistent state.
> 
> Good point, then the patch would look like this.
> BTW: Saw that Jakub applied your patch already.

Yes indeed, so I hope Jakub will apply your follow-up patch soon!
This LGTM.

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH net v2 0/2] Fix 'ethtool --show-eee' during initial stage
Posted by Heiner Kallweit 5 days, 22 hours ago
On 16.11.2024 18:44, Russell King (Oracle) wrote:
> On Sat, Nov 16, 2024 at 06:41:13PM +0100, Heiner Kallweit wrote:
>> On 16.11.2024 16:34, Russell King (Oracle) wrote:
>>> Hmm, don't we want to do this under phydev->lock, because network
>>> drivers and phylib may be reading from phydev->eee_cfg? If we
>>> update it outside the lock, and then revert, there's a chance that
>>> the phylib state machine / network driver may see the changes
>>> which then get reverted on failure, potentially leading to
>>> inconsistent state.
>>
>> Good point, then the patch would look like this.
>> BTW: Saw that Jakub applied your patch already.
> 
> Yes indeed, so I hope Jakub will apply your follow-up patch soon!
> This LGTM.
> 
OK, then I'll submit the followup.

> Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Thanks!
>