| Message ID | 20190617074858.32467-10-bpoirier@suse.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,01/16] qlge: Remove irq_cnt | expand |
From: Benjamin Poirier <bpoirier@suse.com> Date: Mon, 17 Jun 2019 16:48:52 +0900 > Signed-off-by: Benjamin Poirier <bpoirier@suse.com> > --- > drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++ > drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 ++++++------------ > 2 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h > index 5a4b2520cd2a..0bb7ccdca6a7 100644 > --- a/drivers/net/ethernet/qlogic/qlge/qlge.h > +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h > @@ -77,6 +77,12 @@ > #define LSD(x) ((u32)((u64)(x))) > #define MSD(x) ((u32)((((u64)(x)) >> 32))) > > +#define QLGE_FIT16(value) \ > +({ \ > + typeof(value) _value = value; \ > + (_value) == 65536 ? 0 : (u16)(_value); \ > +}) > + "(u16) 65536" is zero and the range of these values is 0 -- 65536. This whole expression is way overdone.
From: David Miller <davem@davemloft.net> Date: Sun, 23 Jun 2019 10:59:35 -0700 (PDT) > "(u16) 65536" is zero and the range of these values is 0 -- 65536. > > This whole expression is way overdone. Also, when you post the next revision of this patch series, please provide a proper "[PATCH net-next 00/16]" header posting explaining what this patch series does logically at the high level, how it is doing it, and why it is doing it that way. Thank you.
On 2019/06/23 10:59, David Miller wrote: > From: Benjamin Poirier <bpoirier@suse.com> > Date: Mon, 17 Jun 2019 16:48:52 +0900 > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com> > > --- > > drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++ > > drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 ++++++------------ > > 2 files changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h > > index 5a4b2520cd2a..0bb7ccdca6a7 100644 > > --- a/drivers/net/ethernet/qlogic/qlge/qlge.h > > +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h > > @@ -77,6 +77,12 @@ > > #define LSD(x) ((u32)((u64)(x))) > > #define MSD(x) ((u32)((((u64)(x)) >> 32))) > > > > +#define QLGE_FIT16(value) \ > > +({ \ > > + typeof(value) _value = value; \ > > + (_value) == 65536 ? 0 : (u16)(_value); \ > > +}) > > + > > "(u16) 65536" is zero and the range of these values is 0 -- 65536. > > This whole expression is way overdone. Indeed, I missed that a simple cast is enough :/ What I inferred from the presence of that expression though is that in the places where it is used, the device interprets a value of 0 as 65536. Manish, can you confirm that? As David points out, the expression is useless. A comment might not be however.
> -----Original Message----- > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On > Behalf Of Benjamin Poirier > Sent: Monday, June 24, 2019 1:22 PM > To: David Miller <davem@davemloft.net> > Cc: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux- > NIC-Dev@marvell.com>; netdev@vger.kernel.org > Subject: Re: [PATCH net-next 10/16] qlge: Factor out duplicated expression > > On 2019/06/23 10:59, David Miller wrote: > > From: Benjamin Poirier <bpoirier@suse.com> > > Date: Mon, 17 Jun 2019 16:48:52 +0900 > > > > > Signed-off-by: Benjamin Poirier <bpoirier@suse.com> > > > --- > > > drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++ > > > drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 > > > ++++++------------ > > > 2 files changed, 12 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h > > > b/drivers/net/ethernet/qlogic/qlge/qlge.h > > > index 5a4b2520cd2a..0bb7ccdca6a7 100644 > > > --- a/drivers/net/ethernet/qlogic/qlge/qlge.h > > > +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h > > > @@ -77,6 +77,12 @@ > > > #define LSD(x) ((u32)((u64)(x))) > > > #define MSD(x) ((u32)((((u64)(x)) >> 32))) > > > > > > +#define QLGE_FIT16(value) \ > > > +({ \ > > > + typeof(value) _value = value; \ > > > + (_value) == 65536 ? 0 : (u16)(_value); \ > > > +}) > > > + > > > > "(u16) 65536" is zero and the range of these values is 0 -- 65536. > > > > This whole expression is way overdone. > > Indeed, I missed that a simple cast is enough :/ > > What I inferred from the presence of that expression though is that in the > places where it is used, the device interprets a value of 0 as 65536. Manish, > can you confirm that? As David points out, the expression is useless. A > comment might not be however. Yes, I think it could be simplified to simple cast just.
On 2019/06/25 18:32, Manish Chopra wrote: [...] > > > > What I inferred from the presence of that expression though is that in the > > places where it is used, the device interprets a value of 0 as 65536. Manish, > > can you confirm that? As David points out, the expression is useless. A > > comment might not be however. > > Yes, I think it could be simplified to simple cast just. > I checked and it seems like the device treats cqicq->lbq_buf_size = 0 more like 65536 than like 0. That is, I saw it write up to 9596B (running at 9000 mtu...) in a single lbq buffer.
diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h b/drivers/net/ethernet/qlogic/qlge/qlge.h index 5a4b2520cd2a..0bb7ccdca6a7 100644 --- a/drivers/net/ethernet/qlogic/qlge/qlge.h +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h @@ -77,6 +77,12 @@ #define LSD(x) ((u32)((u64)(x))) #define MSD(x) ((u32)((((u64)(x)) >> 32))) +#define QLGE_FIT16(value) \ +({ \ + typeof(value) _value = value; \ + (_value) == 65536 ? 0 : (u16)(_value); \ +}) + /* MPI test register definitions. This register * is used for determining alternate NIC function's * PCI->func number. diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c b/drivers/net/ethernet/qlogic/qlge/qlge_main.c index 733f3b87c3a5..a21c47c4b9bd 100644 --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c @@ -2974,7 +2974,6 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring) void __iomem *doorbell_area = qdev->doorbell_area + (DB_PAGE_SIZE * (128 + rx_ring->cq_id)); int err = 0; - u16 bq_len; u64 tmp; __le64 *base_indirect_ptr; int page_entries; @@ -3009,8 +3008,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring) memset((void *)cqicb, 0, sizeof(struct cqicb)); cqicb->msix_vect = rx_ring->irq; - bq_len = (rx_ring->cq_len == 65536) ? 0 : (u16) rx_ring->cq_len; - cqicb->len = cpu_to_le16(bq_len | LEN_V | LEN_CPP_CONT); + cqicb->len = cpu_to_le16(QLGE_FIT16(rx_ring->cq_len) | LEN_V | + LEN_CPP_CONT); cqicb->addr = cpu_to_le64(rx_ring->cq_base_dma); @@ -3034,12 +3033,9 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring) page_entries++; } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring->lbq.len)); cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma); - bq_len = qdev->lbq_buf_size == 65536 ? 0 : - (u16)qdev->lbq_buf_size; - cqicb->lbq_buf_size = cpu_to_le16(bq_len); - bq_len = (rx_ring->lbq.len == 65536) ? 0 : - (u16)rx_ring->lbq.len; - cqicb->lbq_len = cpu_to_le16(bq_len); + cqicb->lbq_buf_size = + cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size)); + cqicb->lbq_len = cpu_to_le16(QLGE_FIT16(rx_ring->lbq.len)); rx_ring->lbq.prod_idx = 0; rx_ring->lbq.curr_idx = 0; rx_ring->lbq.clean_idx = 0; @@ -3059,9 +3055,7 @@ static int ql_start_rx_ring(struct ql_adapter *qdev, struct rx_ring *rx_ring) cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma); cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE); - bq_len = (rx_ring->sbq.len == 65536) ? 0 : - (u16)rx_ring->sbq.len; - cqicb->sbq_len = cpu_to_le16(bq_len); + cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(rx_ring->sbq.len)); rx_ring->sbq.prod_idx = 0; rx_ring->sbq.curr_idx = 0; rx_ring->sbq.clean_idx = 0;
Signed-off-by: Benjamin Poirier <bpoirier@suse.com> --- drivers/net/ethernet/qlogic/qlge/qlge.h | 6 ++++++ drivers/net/ethernet/qlogic/qlge/qlge_main.c | 18 ++++++------------ 2 files changed, 12 insertions(+), 12 deletions(-)