| Message ID | 20240827115848.3908369-1-yanzhen@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] ethernet: marvell: Use min macro | expand |
On Tue, Aug 27, 2024 at 07:58:48PM +0800, Yan Zhen wrote: > Using the real macro is usually more intuitive and readable, > When the original file is guaranteed to contain the minmax.h header file > and compile correctly. > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > --- > drivers/net/ethernet/marvell/mvneta.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > index d72b2d5f96db..415d2b9e63f9 100644 > --- a/drivers/net/ethernet/marvell/mvneta.c > +++ b/drivers/net/ethernet/marvell/mvneta.c > @@ -4750,8 +4750,7 @@ mvneta_ethtool_set_ringparam(struct net_device *dev, > > if ((ring->rx_pending == 0) || (ring->tx_pending == 0)) > return -EINVAL; > - pp->rx_ring_size = ring->rx_pending < MVNETA_MAX_RXD ? > - ring->rx_pending : MVNETA_MAX_RXD; > + pp->rx_ring_size = min(ring->rx_pending, MVNETA_MAX_RXD); Given that the type of ring->rx_pending is __32, and MVNETA_MAX_RXD is a positive value. See: 80fcac55385c ("minmax: add umin(a, b) and umax(a, b)") https://git.kernel.org/torvalds/c/80fcac55385c > > pp->tx_ring_size = clamp_t(u16, ring->tx_pending, > MVNETA_MAX_SKB_DESCS * 2, MVNETA_MAX_TXD); > -- > 2.34.1 > >
On Tue, Aug 27, 2024 at 06:54:08PM +0100, Simon Horman wrote: > On Tue, Aug 27, 2024 at 07:58:48PM +0800, Yan Zhen wrote: > > Using the real macro is usually more intuitive and readable, > > When the original file is guaranteed to contain the minmax.h header file > > and compile correctly. > > > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > > --- > > drivers/net/ethernet/marvell/mvneta.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > index d72b2d5f96db..415d2b9e63f9 100644 > > --- a/drivers/net/ethernet/marvell/mvneta.c > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > @@ -4750,8 +4750,7 @@ mvneta_ethtool_set_ringparam(struct net_device *dev, > > > > if ((ring->rx_pending == 0) || (ring->tx_pending == 0)) > > return -EINVAL; > > - pp->rx_ring_size = ring->rx_pending < MVNETA_MAX_RXD ? > > - ring->rx_pending : MVNETA_MAX_RXD; > > + pp->rx_ring_size = min(ring->rx_pending, MVNETA_MAX_RXD); > > Given that the type of ring->rx_pending is __32, and MVNETA_MAX_RXD is > a positive value. Sorry, I hit send to soon. What I wanted to say is: I think that it is appropriate to use umin() here. Because: 1) As I understand things, the type of MVNETA_MAX_RXD is signed, but it always holds a positive value 2) ring->rx_pending is unsigned > See: 80fcac55385c ("minmax: add umin(a, b) and umax(a, b)") > https://git.kernel.org/torvalds/c/80fcac55385c
From: Simon Horman > Sent: 27 August 2024 18:58 > > On Tue, Aug 27, 2024 at 06:54:08PM +0100, Simon Horman wrote: > > On Tue, Aug 27, 2024 at 07:58:48PM +0800, Yan Zhen wrote: > > > Using the real macro is usually more intuitive and readable, > > > When the original file is guaranteed to contain the minmax.h header file > > > and compile correctly. > > > > > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > > > --- > > > drivers/net/ethernet/marvell/mvneta.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > > index d72b2d5f96db..415d2b9e63f9 100644 > > > --- a/drivers/net/ethernet/marvell/mvneta.c > > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > > @@ -4750,8 +4750,7 @@ mvneta_ethtool_set_ringparam(struct net_device *dev, > > > > > > if ((ring->rx_pending == 0) || (ring->tx_pending == 0)) > > > return -EINVAL; > > > - pp->rx_ring_size = ring->rx_pending < MVNETA_MAX_RXD ? > > > - ring->rx_pending : MVNETA_MAX_RXD; > > > + pp->rx_ring_size = min(ring->rx_pending, MVNETA_MAX_RXD); > > > > Given that the type of ring->rx_pending is __32, and MVNETA_MAX_RXD is > > a positive value. > > Sorry, I hit send to soon. What I wanted to say is: > > I think that it is appropriate to use umin() here. > Because: > 1) As I understand things, the type of MVNETA_MAX_RXD is signed, > but it always holds a positive value > 2) ring->rx_pending is unsigned Provided MVNETA_MAX_RXD is constant it is fine. umin() is only needed for signed variables that can only contain non-negative values. You only need to use it is the compiler bleats... umin(x, y) is safer than min_t(unsigned_type, x, y) because you can't get the type wrong. If will also generate better code since it never sign extends a 32bit value to 64bits (expensive on 32bit). David > > > See: 80fcac55385c ("minmax: add umin(a, b) and umax(a, b)") > > https://git.kernel.org/torvalds/c/80fcac55385c - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Sat, Aug 31, 2024 at 12:39:28PM +0000, David Laight wrote: > From: Simon Horman > > Sent: 27 August 2024 18:58 > > > > On Tue, Aug 27, 2024 at 06:54:08PM +0100, Simon Horman wrote: > > > On Tue, Aug 27, 2024 at 07:58:48PM +0800, Yan Zhen wrote: > > > > Using the real macro is usually more intuitive and readable, > > > > When the original file is guaranteed to contain the minmax.h header file > > > > and compile correctly. > > > > > > > > Signed-off-by: Yan Zhen <yanzhen@vivo.com> > > > > --- > > > > drivers/net/ethernet/marvell/mvneta.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c > > > > index d72b2d5f96db..415d2b9e63f9 100644 > > > > --- a/drivers/net/ethernet/marvell/mvneta.c > > > > +++ b/drivers/net/ethernet/marvell/mvneta.c > > > > @@ -4750,8 +4750,7 @@ mvneta_ethtool_set_ringparam(struct net_device *dev, > > > > > > > > if ((ring->rx_pending == 0) || (ring->tx_pending == 0)) > > > > return -EINVAL; > > > > - pp->rx_ring_size = ring->rx_pending < MVNETA_MAX_RXD ? > > > > - ring->rx_pending : MVNETA_MAX_RXD; > > > > + pp->rx_ring_size = min(ring->rx_pending, MVNETA_MAX_RXD); > > > > > > Given that the type of ring->rx_pending is __32, and MVNETA_MAX_RXD is > > > a positive value. > > > > Sorry, I hit send to soon. What I wanted to say is: > > > > I think that it is appropriate to use umin() here. > > Because: > > 1) As I understand things, the type of MVNETA_MAX_RXD is signed, > > but it always holds a positive value > > 2) ring->rx_pending is unsigned > > Provided MVNETA_MAX_RXD is constant it is fine. > umin() is only needed for signed variables that can only contain > non-negative values. > > You only need to use it is the compiler bleats... > > umin(x, y) is safer than min_t(unsigned_type, x, y) because you can't > get the type wrong. > If will also generate better code since it never sign extends a > 32bit value to 64bits (expensive on 32bit). Hi David, My understanding of umin() was a bit off - I thought it was also relevant when one of the arguments is a constant. Thanks for setting me straight.
diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index d72b2d5f96db..415d2b9e63f9 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -4750,8 +4750,7 @@ mvneta_ethtool_set_ringparam(struct net_device *dev, if ((ring->rx_pending == 0) || (ring->tx_pending == 0)) return -EINVAL; - pp->rx_ring_size = ring->rx_pending < MVNETA_MAX_RXD ? - ring->rx_pending : MVNETA_MAX_RXD; + pp->rx_ring_size = min(ring->rx_pending, MVNETA_MAX_RXD); pp->tx_ring_size = clamp_t(u16, ring->tx_pending, MVNETA_MAX_SKB_DESCS * 2, MVNETA_MAX_TXD);
Using the real macro is usually more intuitive and readable, When the original file is guaranteed to contain the minmax.h header file and compile correctly. Signed-off-by: Yan Zhen <yanzhen@vivo.com> --- drivers/net/ethernet/marvell/mvneta.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)