diff mbox series

be2net: Fix some u16 fields appropriately

Message ID 1503818685-32068-1-git-send-email-yanhaishuang@cmss.chinamobile.com
State Changes Requested, archived
Delegated to: David Miller
Headers show
Series be2net: Fix some u16 fields appropriately | expand

Commit Message

Haishuang Yan Aug. 27, 2017, 7:24 a.m. UTC
In be_tx_compl_process, frag_index declared as u32, so it's better to
declare last_index as u32 also.

CC: Ajit Khaparde <ajit.khaparde@broadcom.com>
Fixes: b0fd2eb28bd4 ("be2net: Declare some u16 fields as u32 to improve
performance")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 drivers/net/ethernet/emulex/benet/be.h      | 2 +-
 drivers/net/ethernet/emulex/benet/be_main.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

David Miller Aug. 28, 2017, 11:19 p.m. UTC | #1
From: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
Date: Sun, 27 Aug 2017 15:24:45 +0800

> In be_tx_compl_process, frag_index declared as u32, so it's better to
> declare last_index as u32 also.
> 
> CC: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Fixes: b0fd2eb28bd4 ("be2net: Declare some u16 fields as u32 to improve
> performance")
> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>

That is not a legitimate reason for making this change.

> @@ -255,7 +255,7 @@ struct be_tx_stats {
>  /* Structure to hold some data of interest obtained from a TX CQE */
>  struct be_tx_compl_info {
>  	u8 status;		/* Completion status */
> -	u16 end_index;		/* Completed TXQ Index */
> +	u32 end_index;		/* Completed TXQ Index */
>  };
>  
>  struct be_tx_obj {

The ->end_index comes solely from:

	txcp->end_index = GET_TX_COMPL_BITS(wrb_index, compl);

Which is precisely a 16-bit value.

I'm not applying this, sorry.
Haishuang Yan Aug. 29, 2017, 1:04 a.m. UTC | #2
> On 2017年8月29日, at 上午7:19, David Miller <davem@davemloft.net> wrote:
> 
> From: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> Date: Sun, 27 Aug 2017 15:24:45 +0800
> 
>> In be_tx_compl_process, frag_index declared as u32, so it's better to
>> declare last_index as u32 also.
>> 
>> CC: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Fixes: b0fd2eb28bd4 ("be2net: Declare some u16 fields as u32 to improve
>> performance")
>> Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
> 
> That is not a legitimate reason for making this change.
> 
>> @@ -255,7 +255,7 @@ struct be_tx_stats {
>> /* Structure to hold some data of interest obtained from a TX CQE */
>> struct be_tx_compl_info {
>> 	u8 status;		/* Completion status */
>> -	u16 end_index;		/* Completed TXQ Index */
>> +	u32 end_index;		/* Completed TXQ Index */
>> };
>> 
>> struct be_tx_obj {
> 
> The ->end_index comes solely from:
> 
> 	txcp->end_index = GET_TX_COMPL_BITS(wrb_index, compl);
> 
> Which is precisely a 16-bit value.
> 
> I'm not applying this, sorry.
> 

Hi David,

The GET_TX_COMPL_BITS comes from amap_get which also returns a 32-bit value:

  6 static inline u32 amap_get(void *ptr, u32 dw_offset, u32 mask, u32 offset)
  5 {
  4     u32 *dw = (u32 *) ptr;
  3     return mask & (*(dw + dw_offset) >> offset);
  2 }
  1
869 #define AMAP_GET_BITS(_struct, field, ptr)              \
  1         amap_get(ptr,                       \
  2             offsetof(_struct, field)/32,            \
  3             amap_mask(sizeof(((_struct *)0)->field)),   \
  4             AMAP_BIT_OFFSET(_struct, field))
David Miller Aug. 29, 2017, 4:22 a.m. UTC | #3
From: 严海双 <yanhaishuang@cmss.chinamobile.com>

Date: Tue, 29 Aug 2017 09:04:57 +0800

> The GET_TX_COMPL_BITS comes from amap_get which also returns a 32-bit value:


It never returns a value with more than 16-bits of significance for
this specific call.

Please stop trying to be semantically clever when arguing about this
change.

It's not about types, it's about what range of values the struct
member can actually hold.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/emulex/benet/be.h b/drivers/net/ethernet/emulex/benet/be.h
index 674cf9d..2ba4d61 100644
--- a/drivers/net/ethernet/emulex/benet/be.h
+++ b/drivers/net/ethernet/emulex/benet/be.h
@@ -255,7 +255,7 @@  struct be_tx_stats {
 /* Structure to hold some data of interest obtained from a TX CQE */
 struct be_tx_compl_info {
 	u8 status;		/* Completion status */
-	u16 end_index;		/* Completed TXQ Index */
+	u32 end_index;		/* Completed TXQ Index */
 };
 
 struct be_tx_obj {
diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index 319eee3..3645344 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -2606,7 +2606,7 @@  static struct be_tx_compl_info *be_tx_compl_get(struct be_tx_obj *txo)
 }
 
 static u16 be_tx_compl_process(struct be_adapter *adapter,
-			       struct be_tx_obj *txo, u16 last_index)
+			       struct be_tx_obj *txo, u32 last_index)
 {
 	struct sk_buff **sent_skbs = txo->sent_skb_list;
 	struct be_queue_info *txq = &txo->q;