Fix the following coccicheck warning:
./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
Signed-off-by: Wan Jiabing <[email protected]>
---
Changelog:
v2:
- Use typeof(bytes_left) instead of u8.
---
drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
index 35579cf4283f..57586a2e6dec 100644
--- a/drivers/net/ethernet/intel/ice/ice_gnss.c
+++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
@@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
for (i = 0; i < data_len; i += bytes_read) {
u16 bytes_left = data_len - i;
- bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
- ICE_MAX_I2C_DATA_SIZE;
+ bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
--
2.35.1
From: Wan Jiabing
> Sent: 21 March 2022 14:00
>
> Fix the following coccicheck warning:
> ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
>
> Signed-off-by: Wan Jiabing <[email protected]>
> ---
> Changelog:
> v2:
> - Use typeof(bytes_left) instead of u8.
> ---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> index 35579cf4283f..57586a2e6dec 100644
> --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> @@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
> for (i = 0; i < data_len; i += bytes_read) {
> u16 bytes_left = data_len - i;
Oh FFS why is that u16?
Don't do arithmetic on anything smaller than 'int'
David
>
> - bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
> - ICE_MAX_I2C_DATA_SIZE;
> + bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
>
> err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> --
> 2.35.1
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Jakub Kicinski
> Sent: 22 March 2022 18:28
>
> On Tue, 22 Mar 2022 18:12:08 +0000 David Laight wrote:
> > > > Oh FFS why is that u16?
> > > > Don't do arithmetic on anything smaller than 'int'
> > >
> > > Any reasoning? I don't say it's good or bad, just want to hear your
> > > arguments (disasms, perf and object code measurements) etc.
> >
> > Look at the object code on anything except x86.
> > The compiler has to add instruction to mask the value
> > (which is in a full sized register) down to 16 bits
> > after every arithmetic operation.
>
> Isn't it also slower on some modern x86 CPUs?
> I could have sworn someone mentioned that in the past.
Not in the cpu clock count tables I've read.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Alexander Lobakin
> Sent: 22 March 2022 17:51
> From: David Laight <[email protected]>
> Date: Mon, 21 Mar 2022 16:02:20 +0000
>
> > From: Wan Jiabing
> > > Sent: 21 March 2022 14:00
> > >
> > > Fix the following coccicheck warning:
> > > ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
> > >
> > > Signed-off-by: Wan Jiabing <[email protected]>
> > > ---
> > > Changelog:
> > > v2:
> > > - Use typeof(bytes_left) instead of u8.
> > > ---
> > > drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > > index 35579cf4283f..57586a2e6dec 100644
> > > --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> > > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > > @@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
> > > for (i = 0; i < data_len; i += bytes_read) {
> > > u16 bytes_left = data_len - i;
> >
> > Oh FFS why is that u16?
> > Don't do arithmetic on anything smaller than 'int'
>
> Any reasoning? I don't say it's good or bad, just want to hear your
> arguments (disasms, perf and object code measurements) etc.
Look at the object code on anything except x86.
The compiler has to add instruction to mask the value
(which is in a full sized register) down to 16 bits
after every arithmetic operation.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Tue, 22 Mar 2022 18:12:08 +0000 David Laight wrote:
> > > Oh FFS why is that u16?
> > > Don't do arithmetic on anything smaller than 'int'
> >
> > Any reasoning? I don't say it's good or bad, just want to hear your
> > arguments (disasms, perf and object code measurements) etc.
>
> Look at the object code on anything except x86.
> The compiler has to add instruction to mask the value
> (which is in a full sized register) down to 16 bits
> after every arithmetic operation.
Isn't it also slower on some modern x86 CPUs?
I could have sworn someone mentioned that in the past.
From: David Laight <[email protected]>
Date: Mon, 21 Mar 2022 16:02:20 +0000
> From: Wan Jiabing
> > Sent: 21 March 2022 14:00
> >
> > Fix the following coccicheck warning:
> > ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity for min()
> >
> > Signed-off-by: Wan Jiabing <[email protected]>
> > ---
> > Changelog:
> > v2:
> > - Use typeof(bytes_left) instead of u8.
> > ---
> > drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice_gnss.c b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > index 35579cf4283f..57586a2e6dec 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_gnss.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_gnss.c
> > @@ -76,8 +76,7 @@ static void ice_gnss_read(struct kthread_work *work)
> > for (i = 0; i < data_len; i += bytes_read) {
> > u16 bytes_left = data_len - i;
>
> Oh FFS why is that u16?
> Don't do arithmetic on anything smaller than 'int'
Any reasoning? I don't say it's good or bad, just want to hear your
arguments (disasms, perf and object code measurements) etc.
>
> David
>
> >
> > - bytes_read = bytes_left < ICE_MAX_I2C_DATA_SIZE ? bytes_left :
> > - ICE_MAX_I2C_DATA_SIZE;
> > + bytes_read = min_t(typeof(bytes_left), bytes_left, ICE_MAX_I2C_DATA_SIZE);
> >
> > err = ice_aq_read_i2c(hw, link_topo, ICE_GNSS_UBX_I2C_BUS_ADDR,
> > cpu_to_le16(ICE_GNSS_UBX_EMPTY_DATA),
> > --
> > 2.35.1
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
Thanks,
Al
> -----Original Message-----
> From: Wan Jiabing <[email protected]>
> Sent: Monday, March 21, 2022 7:30 PM
> To: Brandeburg, Jesse <[email protected]>; Nguyen, Anthony L
> <[email protected]>; David S. Miller <[email protected]>;
> Jakub Kicinski <[email protected]>; Paolo Abeni <[email protected]>;
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: Lobakin, Alexandr <[email protected]>; Wan Jiabing
> <[email protected]>
> Subject: [PATCH v2] ice: use min_t() to make code cleaner in ice_gnss
>
> Fix the following coccicheck warning:
> ./drivers/net/ethernet/intel/ice/ice_gnss.c:79:26-27: WARNING opportunity
> for min()
>
> Signed-off-by: Wan Jiabing <[email protected]>
> ---
> Changelog:
> v2:
> - Use typeof(bytes_left) instead of u8.
> ---
> drivers/net/ethernet/intel/ice/ice_gnss.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Tested-by: Gurucharan <[email protected]> (A Contingent worker at Intel)