| Message ID | E1tUmAE-007VWv-UW@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: clean up and fix EEE implementation | expand |
On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > Use u32 to store this internally within stmmac rather than "int" > which could misinterpret large values. > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > should be unsigned to avoid a negative number being interpreted as a > very large positive number. > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > rather than int, which is derived from tx_lpi_timer, even though > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > value argument type for writel(). > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > Use u32 to store this internally within stmmac rather than "int" > which could misinterpret large values. > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > should be unsigned to avoid a negative number being interpreted as a > very large positive number. > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > rather than int, which is derived from tx_lpi_timer, even though > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > value argument type for writel(). > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> ... > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 9a9169ca7cd2..b0ef439b715b 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE | > NETIF_MSG_IFDOWN | NETIF_MSG_TIMER); > > #define STMMAC_DEFAULT_LPI_TIMER 1000 > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > -module_param(eee_timer, int, 0644); > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > +module_param(eee_timer, uint, 0644); > MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec"); > #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x)) > Hi Russell, now that eee_timer is unsigned the following check in stmmac_verify_args() can never be true. I guess it should be removed. if (eee_timer < 0) eee_timer = STMMAC_DEFAULT_LPI_TIMER; Flagged by Smatch. ...
On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote: > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > > Use u32 to store this internally within stmmac rather than "int" > > which could misinterpret large values. > > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > > should be unsigned to avoid a negative number being interpreted as a > > very large positive number. > > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > > rather than int, which is derived from tx_lpi_timer, even though > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > > value argument type for writel(). > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > ... > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 9a9169ca7cd2..b0ef439b715b 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE | > > NETIF_MSG_IFDOWN | NETIF_MSG_TIMER); > > > > #define STMMAC_DEFAULT_LPI_TIMER 1000 > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > -module_param(eee_timer, int, 0644); > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > +module_param(eee_timer, uint, 0644); > > MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec"); > > #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x)) > > > > Hi Russell, > > now that eee_timer is unsigned the following check in stmmac_verify_args() > can never be true. I guess it should be removed. > > if (eee_timer < 0) > eee_timer = STMMAC_DEFAULT_LPI_TIMER; > Thanks for finding that. The parameter description doesn't seem to detail whether this is intentional behaviour or not, or whether it is "because someone used int and we shouldn't have negative values here". I can't see why someone would pass a negative number for eee_timer given that it already defaults to STMMAC_DEFAULT_LPI_TIMER. So, I'm tempted to remove this.
On Tue, Jan 07, 2025 at 11:57:12AM +0000, Russell King (Oracle) wrote: > On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote: > > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > > > Use u32 to store this internally within stmmac rather than "int" > > > which could misinterpret large values. > > > > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > > > should be unsigned to avoid a negative number being interpreted as a > > > very large positive number. > > > > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > > > rather than int, which is derived from tx_lpi_timer, even though > > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > > > value argument type for writel(). > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > ... > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > index 9a9169ca7cd2..b0ef439b715b 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE | > > > NETIF_MSG_IFDOWN | NETIF_MSG_TIMER); > > > > > > #define STMMAC_DEFAULT_LPI_TIMER 1000 > > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > -module_param(eee_timer, int, 0644); > > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > +module_param(eee_timer, uint, 0644); > > > MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec"); > > > #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x)) > > > > > > > Hi Russell, > > > > now that eee_timer is unsigned the following check in stmmac_verify_args() > > can never be true. I guess it should be removed. > > > > if (eee_timer < 0) > > eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > Thanks for finding that. The parameter description doesn't seem to > detail whether this is intentional behaviour or not, or whether it is > "because someone used int and we shouldn't have negative values here". > > I can't see why someone would pass a negative number for eee_timer > given that it already defaults to STMMAC_DEFAULT_LPI_TIMER. > > So, I'm tempted to remove this. I'm not sure either. It did cross my mind that the check could be changed, to disallow illegal values (if the range of legal values is not all possible unsigned integer values). But it was just an idea without any inspection of the code or thought about it's practicality. And my first instinct was the same as yours: remove the check.
On Tue, Jan 07, 2025 at 02:41:03PM +0000, Simon Horman wrote: > On Tue, Jan 07, 2025 at 11:57:12AM +0000, Russell King (Oracle) wrote: > > On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote: > > > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > > > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > > > > Use u32 to store this internally within stmmac rather than "int" > > > > which could misinterpret large values. > > > > > > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > > > > should be unsigned to avoid a negative number being interpreted as a > > > > very large positive number. > > > > > > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > > > > rather than int, which is derived from tx_lpi_timer, even though > > > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > > > > value argument type for writel(). > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > ... > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > index 9a9169ca7cd2..b0ef439b715b 100644 > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE | > > > > NETIF_MSG_IFDOWN | NETIF_MSG_TIMER); > > > > > > > > #define STMMAC_DEFAULT_LPI_TIMER 1000 > > > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > -module_param(eee_timer, int, 0644); > > > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > +module_param(eee_timer, uint, 0644); > > > > MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec"); > > > > #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x)) > > > > > > > > > > Hi Russell, > > > > > > now that eee_timer is unsigned the following check in stmmac_verify_args() > > > can never be true. I guess it should be removed. > > > > > > if (eee_timer < 0) > > > eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > > > > Thanks for finding that. The parameter description doesn't seem to > > detail whether this is intentional behaviour or not, or whether it is > > "because someone used int and we shouldn't have negative values here". > > > > I can't see why someone would pass a negative number for eee_timer > > given that it already defaults to STMMAC_DEFAULT_LPI_TIMER. > > > > So, I'm tempted to remove this. > > I'm not sure either. It did cross my mind that the check could be changed, > to disallow illegal values (if the range of legal values is not all > possible unsigned integer values). But it was just an idea without any > inspection of the code or thought about it's practicality. And my first > instinct was the same as yours: remove the check. My reasoning is as follows: In the existing code with the module paramter is a signed int, then it take a value up to INT_MAX. Provided sizeof(int) == sizeof(u32), then this can be reported through the ethtool API. However, ethtool can set the timer to U32_MAX which can exceed INT_MAX in this case. The driver doesn't stop this, and uses a software based timer for any delay greater than the capabilities of the hardware timer (if any.) So, through ethtool one can set the LPI delay to anything between 0 and U32_MAX, whereas through the module parameter it's between 0 and INT_MAX. values between INT_MIN and -1 inclusive result in the default being used. It is, of course, absurd to have a negative delay, or even a delay anywhere near INT_MAX or U32_MAX. I'll separate out the change to eee_timer so if necessary, that can be reverted without reverting the entire patch. Oh goodo, an extra patch for a patchset which already exceeds netdev's 15 patches...
On Mon, Jan 06, 2025 at 05:45:37PM +0100, Andrew Lunn wrote: > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > > Use u32 to store this internally within stmmac rather than "int" > > which could misinterpret large values. > > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > > should be unsigned to avoid a negative number being interpreted as a > > very large positive number. > > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > > rather than int, which is derived from tx_lpi_timer, even though > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > > value argument type for writel(). > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> FYI, because of splitting this patch, I've dropped your r-b when posting v3.
On Tue, Jan 07, 2025 at 03:26:06PM +0000, Russell King (Oracle) wrote: > On Tue, Jan 07, 2025 at 02:41:03PM +0000, Simon Horman wrote: > > On Tue, Jan 07, 2025 at 11:57:12AM +0000, Russell King (Oracle) wrote: > > > On Tue, Jan 07, 2025 at 11:28:51AM +0000, Simon Horman wrote: > > > > On Mon, Jan 06, 2025 at 12:24:58PM +0000, Russell King (Oracle) wrote: > > > > > The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. > > > > > Use u32 to store this internally within stmmac rather than "int" > > > > > which could misinterpret large values. > > > > > > > > > > Since eee_timer is used to initialise priv->tx_lpi_timer, this also > > > > > should be unsigned to avoid a negative number being interpreted as a > > > > > very large positive number. > > > > > > > > > > Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 > > > > > rather than int, which is derived from tx_lpi_timer, even though > > > > > masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the > > > > > value argument type for writel(). > > > > > > > > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > > > > > > > > ... > > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > index 9a9169ca7cd2..b0ef439b715b 100644 > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > > > > @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE | > > > > > NETIF_MSG_IFDOWN | NETIF_MSG_TIMER); > > > > > > > > > > #define STMMAC_DEFAULT_LPI_TIMER 1000 > > > > > -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > > -module_param(eee_timer, int, 0644); > > > > > +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > > +module_param(eee_timer, uint, 0644); > > > > > MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec"); > > > > > #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x)) > > > > > > > > > > > > > Hi Russell, > > > > > > > > now that eee_timer is unsigned the following check in stmmac_verify_args() > > > > can never be true. I guess it should be removed. > > > > > > > > if (eee_timer < 0) > > > > eee_timer = STMMAC_DEFAULT_LPI_TIMER; > > > > > > > > > > Thanks for finding that. The parameter description doesn't seem to > > > detail whether this is intentional behaviour or not, or whether it is > > > "because someone used int and we shouldn't have negative values here". > > > > > > I can't see why someone would pass a negative number for eee_timer > > > given that it already defaults to STMMAC_DEFAULT_LPI_TIMER. > > > > > > So, I'm tempted to remove this. > > > > I'm not sure either. It did cross my mind that the check could be changed, > > to disallow illegal values (if the range of legal values is not all > > possible unsigned integer values). But it was just an idea without any > > inspection of the code or thought about it's practicality. And my first > > instinct was the same as yours: remove the check. > > My reasoning is as follows: > > In the existing code with the module paramter is a signed int, then it > take a value up to INT_MAX. Provided sizeof(int) == sizeof(u32), then > this can be reported through the ethtool API. However, ethtool can set > the timer to U32_MAX which can exceed INT_MAX in this case. The driver > doesn't stop this, and uses a software based timer for any delay greater > than the capabilities of the hardware timer (if any.) > > So, through ethtool one can set the LPI delay to anything between 0 and > U32_MAX, whereas through the module parameter it's between 0 and > INT_MAX. values between INT_MIN and -1 inclusive result in the default > being used. > > It is, of course, absurd to have a negative delay, or even a delay > anywhere near INT_MAX or U32_MAX. > > I'll separate out the change to eee_timer so if necessary, that can be > reverted without reverting the entire patch. Oh goodo, an extra patch > for a patchset which already exceeds netdev's 15 patches... Thanks Russell, FWIIW this sounds entirely reasonable to me.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c index c36f90a782c5..9ed8620580a8 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c @@ -420,10 +420,10 @@ static void dwmac4_set_eee_pls(struct mac_device_info *hw, int link) writel(value, ioaddr + GMAC4_LPI_CTRL_STATUS); } -static void dwmac4_set_eee_lpi_entry_timer(struct mac_device_info *hw, int et) +static void dwmac4_set_eee_lpi_entry_timer(struct mac_device_info *hw, u32 et) { void __iomem *ioaddr = hw->pcsr; - int value = et & STMMAC_ET_MAX; + u32 value = et & STMMAC_ET_MAX; int regval; /* Program LPI entry timer value into register */ diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h index 2f7295b6c1c5..0f200b72c225 100644 --- a/drivers/net/ethernet/stmicro/stmmac/hwif.h +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h @@ -363,7 +363,7 @@ struct stmmac_ops { void (*set_eee_mode)(struct mac_device_info *hw, bool en_tx_lpi_clockgating); void (*reset_eee_mode)(struct mac_device_info *hw); - void (*set_eee_lpi_entry_timer)(struct mac_device_info *hw, int et); + void (*set_eee_lpi_entry_timer)(struct mac_device_info *hw, u32 et); void (*set_eee_timer)(struct mac_device_info *hw, int ls, int tw); void (*set_eee_pls)(struct mac_device_info *hw, int link); void (*debug)(struct stmmac_priv *priv, void __iomem *ioaddr, diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index df386fc948ec..984e708d019f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -307,7 +307,7 @@ struct stmmac_priv { int lpi_irq; int eee_enabled; int eee_active; - int tx_lpi_timer; + u32 tx_lpi_timer; int tx_lpi_enabled; int eee_tw_timer; bool eee_sw_timer_en; diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 9a9169ca7cd2..b0ef439b715b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -111,8 +111,8 @@ static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFDOWN | NETIF_MSG_TIMER); #define STMMAC_DEFAULT_LPI_TIMER 1000 -static int eee_timer = STMMAC_DEFAULT_LPI_TIMER; -module_param(eee_timer, int, 0644); +static unsigned int eee_timer = STMMAC_DEFAULT_LPI_TIMER; +module_param(eee_timer, uint, 0644); MODULE_PARM_DESC(eee_timer, "LPI tx expiration time in msec"); #define STMMAC_LPI_T(x) (jiffies + usecs_to_jiffies(x)) @@ -394,11 +394,11 @@ static inline u32 stmmac_rx_dirty(struct stmmac_priv *priv, u32 queue) static void stmmac_lpi_entry_timer_config(struct stmmac_priv *priv, bool en) { - int tx_lpi_timer; + u32 tx_lpi_timer; /* Clear/set the SW EEE timer flag based on LPI ET enablement */ priv->eee_sw_timer_en = en ? 0 : 1; - tx_lpi_timer = en ? priv->tx_lpi_timer : 0; + tx_lpi_timer = en ? priv->tx_lpi_timer : 0; stmmac_set_eee_lpi_timer(priv, priv->hw, tx_lpi_timer); }
The ethtool interface uses u32 for tx_lpi_timer, and so does phylib. Use u32 to store this internally within stmmac rather than "int" which could misinterpret large values. Since eee_timer is used to initialise priv->tx_lpi_timer, this also should be unsigned to avoid a negative number being interpreted as a very large positive number. Also correct "value" in dwmac4_set_eee_lpi_entry_timer() to use u32 rather than int, which is derived from tx_lpi_timer, even though masking with STMMAC_ET_MAX will truncate the sign bits. u32 is the value argument type for writel(). Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 4 ++-- drivers/net/ethernet/stmicro/stmmac/hwif.h | 2 +- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-)