Re: [PATCH net-next v2 03/17] net: stmmac: use correct type for tx_lpi_timer

[Simon Horman <horms@kernel.org>]

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.