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(-)
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
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.
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!
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
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!
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
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!
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! >
© 2016 - 2026 Red Hat, Inc.