[PATCH net] net: libwx: fix to set pause param

[PATCH net] net: libwx: fix to set pause param

[Jiawen Wu]

For some interfaces like PHY_INTERFACE_MODE_10GBASER, no link events
occur when the manual pause modes are changed. So add extra judgment to
configure the MAC.

Fixes: 2fe2ca09da95 ("net: wangxun: add flow control support")
Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
---
 drivers/net/ethernet/wangxun/libwx/wx_ethtool.c | 11 ++++++++++-
 drivers/net/ethernet/wangxun/libwx/wx_hw.c      |  3 +++
 drivers/net/ethernet/wangxun/libwx/wx_type.h    |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
index 43019ec9329c..1b4f97ecee4a 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
@@ -266,11 +266,20 @@ int wx_set_pauseparam(struct net_device *netdev,
 		      struct ethtool_pauseparam *pause)
 {
 	struct wx *wx = netdev_priv(netdev);
+	int err;
 
 	if (wx->mac.type == wx_mac_aml)
 		return -EOPNOTSUPP;
 
-	return phylink_ethtool_set_pauseparam(wx->phylink, pause);
+	err = phylink_ethtool_set_pauseparam(wx->phylink, pause);
+	if (err)
+		return err;
+
+	if (wx->fc.rx_pause != pause->rx_pause ||
+	    wx->fc.tx_pause != pause->tx_pause)
+		return wx_fc_enable(wx, pause->tx_pause, pause->rx_pause);
+
+	return 0;
 }
 EXPORT_SYMBOL(wx_set_pauseparam);
 
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
index aed45abafb1b..2a56c9fdb3e8 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
+++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
@@ -2450,6 +2450,9 @@ int wx_fc_enable(struct wx *wx, bool tx_pause, bool rx_pause)
 			wx_disable_rx_drop(wx, wx->rx_ring[i]);
 	}
 
+	wx->fc.rx_pause = rx_pause;
+	wx->fc.tx_pause = tx_pause;
+
 	return 0;
 }
 EXPORT_SYMBOL(wx_fc_enable);
diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
index 4c545b2aa997..b1c6483c485a 100644
--- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
+++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
@@ -1067,6 +1067,8 @@ enum wx_isb_idx {
 struct wx_fc_info {
 	u32 high_water; /* Flow Ctrl High-water */
 	u32 low_water; /* Flow Ctrl Low-water */
+	bool rx_pause;
+	bool tx_pause;
 };
 
 /* Statistics counters collected by the MAC */
-- 
2.48.1

Re: [PATCH net] net: libwx: fix to set pause param

[Russell King (Oracle)]

On Fri, Apr 25, 2025 at 03:09:42PM +0800, Jiawen Wu wrote:
> @@ -266,11 +266,20 @@ int wx_set_pauseparam(struct net_device *netdev,
>  		      struct ethtool_pauseparam *pause)
>  {
>  	struct wx *wx = netdev_priv(netdev);
> +	int err;
>  
>  	if (wx->mac.type == wx_mac_aml)
>  		return -EOPNOTSUPP;
>  
> -	return phylink_ethtool_set_pauseparam(wx->phylink, pause);
> +	err = phylink_ethtool_set_pauseparam(wx->phylink, pause);
> +	if (err)
> +		return err;
> +
> +	if (wx->fc.rx_pause != pause->rx_pause ||
> +	    wx->fc.tx_pause != pause->tx_pause)
> +		return wx_fc_enable(wx, pause->tx_pause, pause->rx_pause);

Why? phylink_ethtool_set_pauseparam() will cause mac_link_down() +
mac_link_up() to be called with the new parameters.

One of the points of phylink is to stop drivers implementing stuff
buggily - which is exactly what the above is.

->rx_pause and ->tx_pause do not set the pause enables unconditionally.
Please read the documentation in include/uapi/linux/ethtool.h which
states how these two flags are interpreted, specifically the last
paragraph of the struct's documentation.

I'm guessing your change comes from a misunderstanding how the
interface is supposed to work and you believe that phylink isn't
implementing 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] net: libwx: fix to set pause param

[Jiawen Wu]

On Fri, Apr 25, 2025 3:38 PM, Russell King (Oracle) wrote:
> On Fri, Apr 25, 2025 at 03:09:42PM +0800, Jiawen Wu wrote:
> > @@ -266,11 +266,20 @@ int wx_set_pauseparam(struct net_device *netdev,
> >  		      struct ethtool_pauseparam *pause)
> >  {
> >  	struct wx *wx = netdev_priv(netdev);
> > +	int err;
> >
> >  	if (wx->mac.type == wx_mac_aml)
> >  		return -EOPNOTSUPP;
> >
> > -	return phylink_ethtool_set_pauseparam(wx->phylink, pause);
> > +	err = phylink_ethtool_set_pauseparam(wx->phylink, pause);
> > +	if (err)
> > +		return err;
> > +
> > +	if (wx->fc.rx_pause != pause->rx_pause ||
> > +	    wx->fc.tx_pause != pause->tx_pause)
> > +		return wx_fc_enable(wx, pause->tx_pause, pause->rx_pause);
> 
> Why? phylink_ethtool_set_pauseparam() will cause mac_link_down() +
> mac_link_up() to be called with the new parameters.
> 
> One of the points of phylink is to stop drivers implementing stuff
> buggily - which is exactly what the above is.
> 
> ->rx_pause and ->tx_pause do not set the pause enables unconditionally.
> Please read the documentation in include/uapi/linux/ethtool.h which
> states how these two flags are interpreted, specifically the last
> paragraph of the struct's documentation.
> 
> I'm guessing your change comes from a misunderstanding how the
> interface is supposed to work and you believe that phylink isn't
> implementing it correctly.

You are right.
I should set autoneg off first, although there has no autoneg bit in this link mode.

Re: [PATCH net] net: libwx: fix to set pause param

[Russell King (Oracle)]

On Fri, Apr 25, 2025 at 05:20:53PM +0800, Jiawen Wu wrote:
> On Fri, Apr 25, 2025 3:38 PM, Russell King (Oracle) wrote:
> > On Fri, Apr 25, 2025 at 03:09:42PM +0800, Jiawen Wu wrote:
> > > @@ -266,11 +266,20 @@ int wx_set_pauseparam(struct net_device *netdev,
> > >  		      struct ethtool_pauseparam *pause)
> > >  {
> > >  	struct wx *wx = netdev_priv(netdev);
> > > +	int err;
> > >
> > >  	if (wx->mac.type == wx_mac_aml)
> > >  		return -EOPNOTSUPP;
> > >
> > > -	return phylink_ethtool_set_pauseparam(wx->phylink, pause);
> > > +	err = phylink_ethtool_set_pauseparam(wx->phylink, pause);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (wx->fc.rx_pause != pause->rx_pause ||
> > > +	    wx->fc.tx_pause != pause->tx_pause)
> > > +		return wx_fc_enable(wx, pause->tx_pause, pause->rx_pause);
> > 
> > Why? phylink_ethtool_set_pauseparam() will cause mac_link_down() +
> > mac_link_up() to be called with the new parameters.
> > 
> > One of the points of phylink is to stop drivers implementing stuff
> > buggily - which is exactly what the above is.
> > 
> > ->rx_pause and ->tx_pause do not set the pause enables unconditionally.
> > Please read the documentation in include/uapi/linux/ethtool.h which
> > states how these two flags are interpreted, specifically the last
> > paragraph of the struct's documentation.
> > 
> > I'm guessing your change comes from a misunderstanding how the
> > interface is supposed to work and you believe that phylink isn't
> > implementing it correctly.
> 
> You are right.
> I should set autoneg off first, although there has no autoneg bit in this link mode.

Yes, "autoneg" in the pause API selects between using the result of
autonegotiation if enabled, or using the values from tx/rx in the
pause API.

If autonegotiation (as in the control in SLINKSETTINGS) is disabled
or autonegotiation is unsupported, then "autoneg" set in the pause
parameters results in no pause. The same is incidentally true of EEE
settings as well.

So, "autoneg" in SLINKSETTINGS is like the big switch allowing or
preventing all autonegotiation over the link. The other "autoneg"s
control whether the result of autonegotiation is used.

There is one thing in the ethtool_pauseparam documentation that should
be removed:

 * Drivers should reject a non-zero setting of @autoneg when
 * autoneogotiation is disabled (or not supported) for the link.

Let's think about what that means.

- I have a 100baseT/FD link for example, and it used autoneg, and has
  pause enabled.
- I decide to disable autoneg, instead selecting fixed-link mode
  through SLINKSETTINGS.
- Reading the pause parameter settings returns the original state of
  "autoneg" which is set.
- Writing that back results in "rejection" if the above statement is
  followed - which is non-sensical. Let's say it's forced to zero.
- I later re-enable autoneg via SLINKSETTINGS
- I now have to remember to modify the pause mode parameters to
  re-enable pause autoneg.

Things get worse if instead of the above, disabling SLINKSETTINGS
autoneg results in the pause param autoneg being immediately disabled
without API changes - we then end up with one API making magic changes
to settings in another API, and I don't think that is what anyone
would reasonably expect to happen.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!