[RFC 00/19] Improve the qlge driver based on drivers/staging/qlge/TODO

[RFC 00/19] Improve the qlge driver based on drivers/staging/qlge/TODO

[Coiby Xu]

This patch set improves qlge driver based on drivers/staging/qlge/TODO
written by Benjamin.

For the testing, the kernel was build with KASAN, UBSAN, 
DEBUG_ATOMIC_SLEEP, PROVE_LOCKING and DEBUG_KMEMLEAK enabled on a
machine from Red Hat. The machine happened to have two NICs managed by 
this qlge driver. I put these two NICs into separate network namespaces
and no errors occurred for the following tests,
    - with default MTU
        - non TCP packet
          - ping the other NIC from one NIC with different packet size, e.g.
            200, 300, 2200
        - TCP packets
          - start a http server on one NIC
            $ ip netns exec ns1 python -m http.server 8000 --bind 192.168.1.209
          - download a file from the http server using the other NIC
            curl -X GET --interface enp94s0f0 http://192.168.1.209:8000/kernel-5.11.3-300.fc34.src.rpm  -L -O
    - do the same tests with jumbo frame enabled (ip link set enp94s0f0 mtu 9000)


[1] https://lore.kernel.org/netdev/20200816025717.GA28176@f3/T/


Coiby Xu (19):
  staging: qlge: fix incorrect truesize accounting
  staging: qlge: change LARGE_BUFFER_MAX_SIZE to 4096
  staging: qlge: alloc skb with only enough room for header when data is
    put in the fragments
  staging: qlge: add qlge_* prefix to avoid namespace clashes
  staging: qlge: rename rx to completion queue and seperate rx_ring from
    completion queue
  staging: qlge: disable flow control by default
  staging: qlge: remove the TODO item of unnecessary memset 0
  staging: qlge: reorder members of qlge_adapter for optimization
  staging: qlge: remove the TODO item of reorder struct
  staging: qlge: remove the TODO item of avoid legacy/deprecated apis
  staging: qlge: the number of pages to contain a buffer queue is
    constant
  staging: qlge: rewrite do while loops as for loops in
    qlge_start_rx_ring
  staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock
  staging: qlge: rewrite do while loop as for loop in qlge_refill_bq
  staging: qlge: remove the TODO item about rewriting while loops as
    simple for loops
  staging: qlge: remove deadcode in qlge_build_rx_skb
  staging: qlge: fix weird line wrapping
  staging: qlge: fix two indentation issues
  staging: qlge: remove TODO item of unnecessary runtime checks

 drivers/staging/qlge/TODO           |  31 --
 drivers/staging/qlge/qlge.h         |  88 +++--
 drivers/staging/qlge/qlge_dbg.c     |  36 +-
 drivers/staging/qlge/qlge_ethtool.c |  26 +-
 drivers/staging/qlge/qlge_main.c    | 586 ++++++++++++++--------------
 drivers/staging/qlge/qlge_mpi.c     |  11 +-
 6 files changed, 373 insertions(+), 405 deletions(-)

-- 
2.32.0

[RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
based on MTU") introduced page_chunk structure. We should add
qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO        |  2 --
 drivers/staging/qlge/qlge_main.c | 10 +++++-----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index c76394b9451b..449d7dca478b 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,8 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* truesize accounting is incorrect (ex: a 9000B frame has skb->truesize 10280
-  while containing two frags of order-1 allocations, ie. >16K)
 * while in that area, using two 8k buffers to store one 9k frame is a poor
   choice of buffer size.
 * in the "chain of large buffers" case, the driver uses an skb allocated with
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 19a02e958865..6dd69b689a58 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -1446,7 +1446,7 @@ static void qlge_process_mac_rx_gro_page(struct qlge_adapter *qdev,
 
 	skb->len += length;
 	skb->data_len += length;
-	skb->truesize += length;
+	skb->truesize += qdev->lbq_buf_size;
 	skb_shinfo(skb)->nr_frags++;
 
 	rx_ring->rx_packets++;
@@ -1507,7 +1507,7 @@ static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
 			   lbq_desc->p.pg_chunk.offset + hlen, length - hlen);
 	skb->len += length - hlen;
 	skb->data_len += length - hlen;
-	skb->truesize += length - hlen;
+	skb->truesize += qdev->lbq_buf_size;
 
 	rx_ring->rx_packets++;
 	rx_ring->rx_bytes += skb->len;
@@ -1757,7 +1757,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 					   lbq_desc->p.pg_chunk.offset, length);
 			skb->len += length;
 			skb->data_len += length;
-			skb->truesize += length;
+			skb->truesize += qdev->lbq_buf_size;
 		} else {
 			/*
 			 * The headers and data are in a single large buffer. We
@@ -1783,7 +1783,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 					   length);
 			skb->len += length;
 			skb->data_len += length;
-			skb->truesize += length;
+			skb->truesize += qdev->lbq_buf_size;
 			qlge_update_mac_hdr_len(qdev, ib_mac_rsp,
 						lbq_desc->p.pg_chunk.va,
 						&hlen);
@@ -1835,7 +1835,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 					   lbq_desc->p.pg_chunk.offset, size);
 			skb->len += size;
 			skb->data_len += size;
-			skb->truesize += size;
+			skb->truesize += qdev->lbq_buf_size;
 			length -= size;
 			i++;
 		} while (length > 0);
-- 
2.32.0

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
> Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
> based on MTU") introduced page_chunk structure. We should add
> qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
> 

Add a Fixes tag.

The runtime impact of this is just that ethtool will report things
incorrectly, right?  It's not 100% from the commit message.  Could you
please edit the commit message so that an ignoramous like myself can
understand it?

Why is this an RFC instead of just a normal patch which we can apply?

regards,
dan carpenter

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
> Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
> based on MTU") introduced page_chunk structure. We should add
> qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
> 

Add a Fixes tag.

The runtime impact of this is just that ethtool will report things
incorrectly, right?  It's not 100% from the commit message.  Could you
please edit the commit message so that an ignoramous like myself can
understand it?

Why is this an RFC instead of just a normal patch which we can apply?

regards,
dan carpenter

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
>> Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
>> based on MTU") introduced page_chunk structure. We should add
>> qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>>
>
>Add a Fixes tag.

I will fix it in next version, thanks!

>
>The runtime impact of this is just that ethtool will report things
>incorrectly, right?  It's not 100% from the commit message.  Could you
>please edit the commit message so that an ignoramous like myself can
>understand it?

I'm not sure how it would affect ethtool. But according to "git log
--grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
according to the definition of truesize which according to Linux Kernel
Network by Rami Rosen, it's defined as follows,
> The total memory allocated for the SKB (including the SKB structure itself 
> and the size of the allocated data block).

I'll edit the commit message to include it, thanks!

>
>Why is this an RFC instead of just a normal patch which we can apply?

After doing the tests mentioned in the cover letter, I found Red Hat's 
network QE team has quite a rigorous test suite. But I needed to return 
the machine before having the time to learn about the test suite and run 
it by myself. So I mark it as an RFC before I borrow the machine again to 
run the test suite.

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
>> Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
>> based on MTU") introduced page_chunk structure. We should add
>> qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>>
>
>Add a Fixes tag.

I will fix it in next version, thanks!

>
>The runtime impact of this is just that ethtool will report things
>incorrectly, right?  It's not 100% from the commit message.  Could you
>please edit the commit message so that an ignoramous like myself can
>understand it?

I'm not sure how it would affect ethtool. But according to "git log
--grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
according to the definition of truesize which according to Linux Kernel
Network by Rami Rosen, it's defined as follows,
> The total memory allocated for the SKB (including the SKB structure itself 
> and the size of the allocated data block).

I'll edit the commit message to include it, thanks!

>
>Why is this an RFC instead of just a normal patch which we can apply?

After doing the tests mentioned in the cover letter, I found Red Hat's 
network QE team has quite a rigorous test suite. But I needed to return 
the machine before having the time to learn about the test suite and run 
it by myself. So I mark it as an RFC before I borrow the machine again to 
run the test suite.

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Benjamin Poirier]

On 2021-06-22 19:36 +0800, Coiby Xu wrote:
> On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
> > > Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
> > > based on MTU") introduced page_chunk structure. We should add
> > > qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
> > > 
> > 
> > Add a Fixes tag.
> 
> I will fix it in next version, thanks!
> 
> > 
> > The runtime impact of this is just that ethtool will report things
> > incorrectly, right?  It's not 100% from the commit message.  Could you
> > please edit the commit message so that an ignoramous like myself can
> > understand it?

truesize is used in socket memory accounting, the stuff behind sysctl
net.core.rmem_max, SO_RCVBUF, ss -m, ...

Some helpful chap wrote a page about it a while ago:
http://vger.kernel.org/~davem/skb_sk.html

> 
> I'm not sure how it would affect ethtool. But according to "git log
> --grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
> according to the definition of truesize which according to Linux Kernel
> Network by Rami Rosen, it's defined as follows,
> > The total memory allocated for the SKB (including the SKB structure
> > itself and the size of the allocated data block).
> 
> I'll edit the commit message to include it, thanks!
> 
> > 
> > Why is this an RFC instead of just a normal patch which we can apply?
> 
> After doing the tests mentioned in the cover letter, I found Red Hat's
> network QE team has quite a rigorous test suite. But I needed to return the
> machine before having the time to learn about the test suite and run it by
> myself. So I mark it as an RFC before I borrow the machine again to run the
> test suite.

Interesting. Is this test suite based on a public project?

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Benjamin Poirier]

On 2021-06-22 19:36 +0800, Coiby Xu wrote:
> On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
> > > Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
> > > based on MTU") introduced page_chunk structure. We should add
> > > qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
> > > 
> > 
> > Add a Fixes tag.
> 
> I will fix it in next version, thanks!
> 
> > 
> > The runtime impact of this is just that ethtool will report things
> > incorrectly, right?  It's not 100% from the commit message.  Could you
> > please edit the commit message so that an ignoramous like myself can
> > understand it?

truesize is used in socket memory accounting, the stuff behind sysctl
net.core.rmem_max, SO_RCVBUF, ss -m, ...

Some helpful chap wrote a page about it a while ago:
http://vger.kernel.org/~davem/skb_sk.html

> 
> I'm not sure how it would affect ethtool. But according to "git log
> --grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
> according to the definition of truesize which according to Linux Kernel
> Network by Rami Rosen, it's defined as follows,
> > The total memory allocated for the SKB (including the SKB structure
> > itself and the size of the allocated data block).
> 
> I'll edit the commit message to include it, thanks!
> 
> > 
> > Why is this an RFC instead of just a normal patch which we can apply?
> 
> After doing the tests mentioned in the cover letter, I found Red Hat's
> network QE team has quite a rigorous test suite. But I needed to return the
> machine before having the time to learn about the test suite and run it by
> myself. So I mark it as an RFC before I borrow the machine again to run the
> test suite.

Interesting. Is this test suite based on a public project?

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

On Wed, Jun 23, 2021 at 01:55:15PM +0900, Benjamin Poirier wrote:
>On 2021-06-22 19:36 +0800, Coiby Xu wrote:
>> On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
>> > > Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
>> > > based on MTU") introduced page_chunk structure. We should add
>> > > qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>> > >
>> >
>> > Add a Fixes tag.
>>
>> I will fix it in next version, thanks!
>>
>> >
>> > The runtime impact of this is just that ethtool will report things
>> > incorrectly, right?  It's not 100% from the commit message.  Could you
>> > please edit the commit message so that an ignoramous like myself can
>> > understand it?
>
>truesize is used in socket memory accounting, the stuff behind sysctl
>net.core.rmem_max, SO_RCVBUF, ss -m, ...
>
>Some helpful chap wrote a page about it a while ago:
>http://vger.kernel.org/~davem/skb_sk.html

Thanks for the explanation and the reference!

>
>>
>> I'm not sure how it would affect ethtool. But according to "git log
>> --grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
>> according to the definition of truesize which according to Linux Kernel
>> Network by Rami Rosen, it's defined as follows,
>> > The total memory allocated for the SKB (including the SKB structure
>> > itself and the size of the allocated data block).
>>
>> I'll edit the commit message to include it, thanks!
>>
>> >
>> > Why is this an RFC instead of just a normal patch which we can apply?
>>
>> After doing the tests mentioned in the cover letter, I found Red Hat's
>> network QE team has quite a rigorous test suite. But I needed to return the
>> machine before having the time to learn about the test suite and run it by
>> myself. So I mark it as an RFC before I borrow the machine again to run the
>> test suite.
>
>Interesting. Is this test suite based on a public project?

The test suite is written for Beaker [1] but it seems it's not public.

[1] https://fedoraproject.org/wiki/QA/Beaker

-- 
Best regards,
Coiby

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

On Wed, Jun 23, 2021 at 01:55:15PM +0900, Benjamin Poirier wrote:
>On 2021-06-22 19:36 +0800, Coiby Xu wrote:
>> On Mon, Jun 21, 2021 at 05:10:27PM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:44PM +0800, Coiby Xu wrote:
>> > > Commit 7c734359d3504c869132166d159c7f0649f0ab34 ("qlge: Size RX buffers
>> > > based on MTU") introduced page_chunk structure. We should add
>> > > qdev->lbq_buf_size to skb->truesize after __skb_fill_page_desc.
>> > >
>> >
>> > Add a Fixes tag.
>>
>> I will fix it in next version, thanks!
>>
>> >
>> > The runtime impact of this is just that ethtool will report things
>> > incorrectly, right?  It's not 100% from the commit message.  Could you
>> > please edit the commit message so that an ignoramous like myself can
>> > understand it?
>
>truesize is used in socket memory accounting, the stuff behind sysctl
>net.core.rmem_max, SO_RCVBUF, ss -m, ...
>
>Some helpful chap wrote a page about it a while ago:
>http://vger.kernel.org/~davem/skb_sk.html

Thanks for the explanation and the reference!

>
>>
>> I'm not sure how it would affect ethtool. But according to "git log
>> --grep=truesize", it affects coalescing SKBs. Btw, I fixed the issue
>> according to the definition of truesize which according to Linux Kernel
>> Network by Rami Rosen, it's defined as follows,
>> > The total memory allocated for the SKB (including the SKB structure
>> > itself and the size of the allocated data block).
>>
>> I'll edit the commit message to include it, thanks!
>>
>> >
>> > Why is this an RFC instead of just a normal patch which we can apply?
>>
>> After doing the tests mentioned in the cover letter, I found Red Hat's
>> network QE team has quite a rigorous test suite. But I needed to return the
>> machine before having the time to learn about the test suite and run it by
>> myself. So I mark it as an RFC before I borrow the machine again to run the
>> test suite.
>
>Interesting. Is this test suite based on a public project?

The test suite is written for Beaker [1] but it seems it's not public.

[1] https://fedoraproject.org/wiki/QA/Beaker

-- 
Best regards,
Coiby

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

On Thu, Jun 24, 2021 at 07:47:05PM +0800, Coiby Xu wrote:
>On Wed, Jun 23, 2021 at 01:55:15PM +0900, Benjamin Poirier wrote:
[..]
>>>> Why is this an RFC instead of just a normal patch which we can apply?
>>>
>>>After doing the tests mentioned in the cover letter, I found Red Hat's
>>>network QE team has quite a rigorous test suite. But I needed to return the
>>>machine before having the time to learn about the test suite and run it by
>>>myself. So I mark it as an RFC before I borrow the machine again to run the
>>>test suite.
>>
>>Interesting. Is this test suite based on a public project?
>
>The test suite is written for Beaker [1] but it seems it's not public.
>
>[1] https://fedoraproject.org/wiki/QA/Beaker

FYI, the tier-1 tests are part of the CKI project and are public [2].

[2] https://gitlab.com/cki-project/kernel-tests/-/tree/main/networking

-- 
Best regards,
Coiby

Re: [RFC 01/19] staging: qlge: fix incorrect truesize accounting

[Coiby Xu]

On Thu, Jun 24, 2021 at 07:47:05PM +0800, Coiby Xu wrote:
>On Wed, Jun 23, 2021 at 01:55:15PM +0900, Benjamin Poirier wrote:
[..]
>>>> Why is this an RFC instead of just a normal patch which we can apply?
>>>
>>>After doing the tests mentioned in the cover letter, I found Red Hat's
>>>network QE team has quite a rigorous test suite. But I needed to return the
>>>machine before having the time to learn about the test suite and run it by
>>>myself. So I mark it as an RFC before I borrow the machine again to run the
>>>test suite.
>>
>>Interesting. Is this test suite based on a public project?
>
>The test suite is written for Beaker [1] but it seems it's not public.
>
>[1] https://fedoraproject.org/wiki/QA/Beaker

FYI, the tier-1 tests are part of the CKI project and are public [2].

[2] https://gitlab.com/cki-project/kernel-tests/-/tree/main/networking

-- 
Best regards,
Coiby

[RFC 02/19] staging: qlge: change LARGE_BUFFER_MAX_SIZE to 4096

[Coiby Xu]

LARGE_BUFFER_MAX_SIZE=4096 could make better use of memory. This choice
is consist with ixgbe and e1000 ,
   - ixgbe sets the rx buffer's page order to 0 unless FCoE is enabled
   - e1000 allocs a page for a jumbo receive buffer

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO   | 2 --
 drivers/staging/qlge/qlge.h | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 449d7dca478b..0e26fac1ddc5 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,8 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* while in that area, using two 8k buffers to store one 9k frame is a poor
-  choice of buffer size.
 * in the "chain of large buffers" case, the driver uses an skb allocated with
   head room but only puts data in the frags.
 * rename "rx" queues to "completion" queues. Calling tx completion queues "rx
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 55e0ad759250..f54d38606b78 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -52,7 +52,7 @@
 #define RX_RING_SHADOW_SPACE	(sizeof(u64) + \
 		MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64) + \
 		MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64))
-#define LARGE_BUFFER_MAX_SIZE 8192
+#define LARGE_BUFFER_MAX_SIZE 4096
 #define LARGE_BUFFER_MIN_SIZE 2048
 
 #define MAX_CQ 128
-- 
2.32.0

[RFC 03/19] staging: qlge: alloc skb with only enough room for header when data is put in the fragments

[Coiby Xu]

Data is put in the fragments. No need to alloc a skb with unnecessarily
large data buffer.

Suggested-by: Benjamin Poirier <benjamin.poirier@gmail.com>
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO        | 2 --
 drivers/staging/qlge/qlge_main.c | 4 ++--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 0e26fac1ddc5..49cb09fc2be4 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,8 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* in the "chain of large buffers" case, the driver uses an skb allocated with
-  head room but only puts data in the frags.
 * rename "rx" queues to "completion" queues. Calling tx completion queues "rx
   queues" is confusing.
 * struct rx_ring is used for rx and tx completions, with some members relevant
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 6dd69b689a58..c91969b01bd5 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -1471,7 +1471,7 @@ static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
 	struct napi_struct *napi = &rx_ring->napi;
 	size_t hlen = ETH_HLEN;
 
-	skb = netdev_alloc_skb(ndev, length);
+	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
 	if (!skb) {
 		rx_ring->rx_dropped++;
 		put_page(lbq_desc->p.pg_chunk.page);
@@ -1765,7 +1765,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 			 * jumbo mtu on a non-TCP/UDP frame.
 			 */
 			lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
-			skb = netdev_alloc_skb(qdev->ndev, length);
+			skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
 			if (!skb) {
 				netif_printk(qdev, probe, KERN_DEBUG, qdev->ndev,
 					     "No skb available, drop the packet.\n");
-- 
2.32.0

[RFC 04/19] staging: qlge: add qlge_* prefix to avoid namespace clashes

[Coiby Xu]

This patch extends commit f8c047be540197ec69cde33e00e82d23961459ea
("staging: qlge: use qlge_* prefix to avoid namespace clashes with other qlogic drivers")
to add qlge_ prefix to rx_ring and tx_ring related structures.

Suggested-by: Benjamin Poirier <benjamin.poirier@gmail.com>
Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge.h         |  40 ++++-----
 drivers/staging/qlge/qlge_ethtool.c |   4 +-
 drivers/staging/qlge/qlge_main.c    | 124 ++++++++++++++--------------
 3 files changed, 84 insertions(+), 84 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index f54d38606b78..09d5878b95f7 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -869,17 +869,17 @@ enum {
 };
 
 #ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
-#define SMALL_BUFFER_SIZE 256
-#define SMALL_BUF_MAP_SIZE SMALL_BUFFER_SIZE
+#define QLGE_SMALL_BUFFER_SIZE 256
+#define QLGE_SMALL_BUF_MAP_SIZE QLGE_SMALL_BUFFER_SIZE
 #define SPLT_SETTING  FSC_DBRST_1024
 #define SPLT_LEN 0
 #define QLGE_SB_PAD 0
 #else
-#define SMALL_BUFFER_SIZE 512
-#define SMALL_BUF_MAP_SIZE (SMALL_BUFFER_SIZE / 2)
+#define QLGE_SMALL_BUFFER_SIZE 512
+#define QLGE_SMALL_BUF_MAP_SIZE (QLGE_SMALL_BUFFER_SIZE / 2)
 #define SPLT_SETTING  FSC_SH
 #define SPLT_LEN (SPLT_HDR_EP | \
-	min(SMALL_BUF_MAP_SIZE, 1023))
+	min(QLGE_SMALL_BUF_MAP_SIZE, 1023))
 #define QLGE_SB_PAD 32
 #endif
 
@@ -1063,7 +1063,7 @@ struct tx_doorbell_context {
 };
 
 /* DATA STRUCTURES SHARED WITH HARDWARE. */
-struct tx_buf_desc {
+struct qlge_tx_buf_desc {
 	__le64 addr;
 	__le32 len;
 #define TX_DESC_LEN_MASK	0x000fffff
@@ -1101,7 +1101,7 @@ struct qlge_ob_mac_iocb_req {
 	__le32 reserved3;
 	__le16 vlan_tci;
 	__le16 reserved4;
-	struct tx_buf_desc tbd[TX_DESC_PER_IOCB];
+	struct qlge_tx_buf_desc tbd[TX_DESC_PER_IOCB];
 } __packed;
 
 struct qlge_ob_mac_iocb_rsp {
@@ -1146,7 +1146,7 @@ struct qlge_ob_mac_tso_iocb_req {
 #define OB_MAC_TRANSPORT_HDR_SHIFT 6
 	__le16 vlan_tci;
 	__le16 mss;
-	struct tx_buf_desc tbd[TX_DESC_PER_IOCB];
+	struct qlge_tx_buf_desc tbd[TX_DESC_PER_IOCB];
 } __packed;
 
 struct qlge_ob_mac_tso_iocb_rsp {
@@ -1347,7 +1347,7 @@ struct ricb {
 /* SOFTWARE/DRIVER DATA STRUCTURES. */
 
 struct qlge_oal {
-	struct tx_buf_desc oal[TX_DESC_PER_OAL];
+	struct qlge_tx_buf_desc oal[TX_DESC_PER_OAL];
 };
 
 struct map_list {
@@ -1355,19 +1355,19 @@ struct map_list {
 	DEFINE_DMA_UNMAP_LEN(maplen);
 };
 
-struct tx_ring_desc {
+struct qlge_tx_ring_desc {
 	struct sk_buff *skb;
 	struct qlge_ob_mac_iocb_req *queue_entry;
 	u32 index;
 	struct qlge_oal oal;
 	struct map_list map[MAX_SKB_FRAGS + 2];
 	int map_cnt;
-	struct tx_ring_desc *next;
+	struct qlge_tx_ring_desc *next;
 };
 
 #define QL_TXQ_IDX(qdev, skb) (smp_processor_id() % (qdev->tx_ring_count))
 
-struct tx_ring {
+struct qlge_tx_ring {
 	/*
 	 * queue info.
 	 */
@@ -1384,7 +1384,7 @@ struct tx_ring {
 	u16 cq_id;		/* completion (rx) queue for tx completions */
 	u8 wq_id;		/* queue id for this entry */
 	u8 reserved1[3];
-	struct tx_ring_desc *q;	/* descriptor list for the queue */
+	struct qlge_tx_ring_desc *q;	/* descriptor list for the queue */
 	spinlock_t lock;
 	atomic_t tx_count;	/* counts down for every outstanding IO */
 	struct delayed_work tx_work;
@@ -1437,9 +1437,9 @@ struct qlge_bq {
 #define QLGE_BQ_CONTAINER(bq) \
 ({ \
 	typeof(bq) _bq = bq; \
-	(struct rx_ring *)((char *)_bq - (_bq->type == QLGE_SB ? \
-					  offsetof(struct rx_ring, sbq) : \
-					  offsetof(struct rx_ring, lbq))); \
+	(struct qlge_rx_ring *)((char *)_bq - (_bq->type == QLGE_SB ? \
+					  offsetof(struct qlge_rx_ring, sbq) : \
+					  offsetof(struct qlge_rx_ring, lbq))); \
 })
 
 /* Experience shows that the device ignores the low 4 bits of the tail index.
@@ -1456,7 +1456,7 @@ struct qlge_bq {
 		     (_bq)->next_to_clean); \
 })
 
-struct rx_ring {
+struct qlge_rx_ring {
 	struct cqicb cqicb;	/* The chip's completion queue init control block. */
 
 	/* Completion queue elements. */
@@ -2135,8 +2135,8 @@ struct qlge_adapter {
 	int ring_mem_size;
 	void *ring_mem;
 
-	struct rx_ring rx_ring[MAX_RX_RINGS];
-	struct tx_ring tx_ring[MAX_TX_RINGS];
+	struct qlge_rx_ring rx_ring[MAX_RX_RINGS];
+	struct qlge_tx_ring tx_ring[MAX_TX_RINGS];
 	unsigned int lbq_buf_order;
 	u32 lbq_buf_size;
 
@@ -2287,6 +2287,6 @@ void qlge_get_dump(struct qlge_adapter *qdev, void *buff);
 netdev_tx_t qlge_lb_send(struct sk_buff *skb, struct net_device *ndev);
 void qlge_check_lb_frame(struct qlge_adapter *qdev, struct sk_buff *skb);
 int qlge_own_firmware(struct qlge_adapter *qdev);
-int qlge_clean_lb_rx_ring(struct rx_ring *rx_ring, int budget);
+int qlge_clean_lb_rx_ring(struct qlge_rx_ring *rx_ring, int budget);
 
 #endif /* _QLGE_H_ */
diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
index b70570b7b467..22c27b97a908 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -186,7 +186,7 @@ static const char qlge_gstrings_test[][ETH_GSTRING_LEN] = {
 static int qlge_update_ring_coalescing(struct qlge_adapter *qdev)
 {
 	int i, status = 0;
-	struct rx_ring *rx_ring;
+	struct qlge_rx_ring *rx_ring;
 	struct cqicb *cqicb;
 
 	if (!netif_running(qdev->ndev))
@@ -537,7 +537,7 @@ static int qlge_run_loopback_test(struct qlge_adapter *qdev)
 	int i;
 	netdev_tx_t rc;
 	struct sk_buff *skb;
-	unsigned int size = SMALL_BUF_MAP_SIZE;
+	unsigned int size = QLGE_SMALL_BUF_MAP_SIZE;
 
 	for (i = 0; i < 64; i++) {
 		skb = netdev_alloc_skb(qdev->ndev, size);
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index c91969b01bd5..77c71ae698ab 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -964,7 +964,7 @@ static struct qlge_bq_desc *qlge_get_curr_buf(struct qlge_bq *bq)
 }
 
 static struct qlge_bq_desc *qlge_get_curr_lchunk(struct qlge_adapter *qdev,
-						 struct rx_ring *rx_ring)
+						 struct qlge_rx_ring *rx_ring)
 {
 	struct qlge_bq_desc *lbq_desc = qlge_get_curr_buf(&rx_ring->lbq);
 
@@ -982,7 +982,7 @@ static struct qlge_bq_desc *qlge_get_curr_lchunk(struct qlge_adapter *qdev,
 }
 
 /* Update an rx ring index. */
-static void qlge_update_cq(struct rx_ring *rx_ring)
+static void qlge_update_cq(struct qlge_rx_ring *rx_ring)
 {
 	rx_ring->cnsmr_idx++;
 	rx_ring->curr_entry++;
@@ -992,7 +992,7 @@ static void qlge_update_cq(struct rx_ring *rx_ring)
 	}
 }
 
-static void qlge_write_cq_idx(struct rx_ring *rx_ring)
+static void qlge_write_cq_idx(struct qlge_rx_ring *rx_ring)
 {
 	qlge_write_db_reg(rx_ring->cnsmr_idx, rx_ring->cnsmr_idx_db_reg);
 }
@@ -1003,7 +1003,7 @@ static const char * const bq_type_name[] = {
 };
 
 /* return 0 or negative error */
-static int qlge_refill_sb(struct rx_ring *rx_ring,
+static int qlge_refill_sb(struct qlge_rx_ring *rx_ring,
 			  struct qlge_bq_desc *sbq_desc, gfp_t gfp)
 {
 	struct qlge_adapter *qdev = rx_ring->qdev;
@@ -1016,13 +1016,13 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
 		     "ring %u sbq: getting new skb for index %d.\n",
 		     rx_ring->cq_id, sbq_desc->index);
 
-	skb = __netdev_alloc_skb(qdev->ndev, SMALL_BUFFER_SIZE, gfp);
+	skb = __netdev_alloc_skb(qdev->ndev, QLGE_SMALL_BUFFER_SIZE, gfp);
 	if (!skb)
 		return -ENOMEM;
 	skb_reserve(skb, QLGE_SB_PAD);
 
 	sbq_desc->dma_addr = dma_map_single(&qdev->pdev->dev, skb->data,
-					    SMALL_BUF_MAP_SIZE,
+					    QLGE_SMALL_BUF_MAP_SIZE,
 					    DMA_FROM_DEVICE);
 	if (dma_mapping_error(&qdev->pdev->dev, sbq_desc->dma_addr)) {
 		netif_err(qdev, ifup, qdev->ndev, "PCI mapping failed.\n");
@@ -1036,7 +1036,7 @@ static int qlge_refill_sb(struct rx_ring *rx_ring,
 }
 
 /* return 0 or negative error */
-static int qlge_refill_lb(struct rx_ring *rx_ring,
+static int qlge_refill_lb(struct qlge_rx_ring *rx_ring,
 			  struct qlge_bq_desc *lbq_desc, gfp_t gfp)
 {
 	struct qlge_adapter *qdev = rx_ring->qdev;
@@ -1086,7 +1086,7 @@ static int qlge_refill_lb(struct rx_ring *rx_ring,
 /* return 0 or negative error */
 static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 {
-	struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
+	struct qlge_rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
 	struct qlge_adapter *qdev = rx_ring->qdev;
 	struct qlge_bq_desc *bq_desc;
 	int refill_count;
@@ -1141,7 +1141,7 @@ static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 	return retval;
 }
 
-static void qlge_update_buffer_queues(struct rx_ring *rx_ring, gfp_t gfp,
+static void qlge_update_buffer_queues(struct qlge_rx_ring *rx_ring, gfp_t gfp,
 				      unsigned long delay)
 {
 	bool sbq_fail, lbq_fail;
@@ -1168,7 +1168,7 @@ static void qlge_update_buffer_queues(struct rx_ring *rx_ring, gfp_t gfp,
 
 static void qlge_slow_refill(struct work_struct *work)
 {
-	struct rx_ring *rx_ring = container_of(work, struct rx_ring,
+	struct qlge_rx_ring *rx_ring = container_of(work, struct qlge_rx_ring,
 					       refill_work.work);
 	struct napi_struct *napi = &rx_ring->napi;
 
@@ -1189,7 +1189,7 @@ static void qlge_slow_refill(struct work_struct *work)
  * fails at some stage, or from the interrupt when a tx completes.
  */
 static void qlge_unmap_send(struct qlge_adapter *qdev,
-			    struct tx_ring_desc *tx_ring_desc, int mapped)
+			    struct qlge_tx_ring_desc *tx_ring_desc, int mapped)
 {
 	int i;
 
@@ -1232,12 +1232,12 @@ static void qlge_unmap_send(struct qlge_adapter *qdev,
  */
 static int qlge_map_send(struct qlge_adapter *qdev,
 			 struct qlge_ob_mac_iocb_req *mac_iocb_ptr,
-			 struct sk_buff *skb, struct tx_ring_desc *tx_ring_desc)
+			 struct sk_buff *skb, struct qlge_tx_ring_desc *tx_ring_desc)
 {
 	int len = skb_headlen(skb);
 	dma_addr_t map;
 	int frag_idx, err, map_idx = 0;
-	struct tx_buf_desc *tbd = mac_iocb_ptr->tbd;
+	struct qlge_tx_buf_desc *tbd = mac_iocb_ptr->tbd;
 	int frag_cnt = skb_shinfo(skb)->nr_frags;
 
 	if (frag_cnt) {
@@ -1312,13 +1312,13 @@ static int qlge_map_send(struct qlge_adapter *qdev,
 			 * of our sglist (OAL).
 			 */
 			tbd->len =
-			    cpu_to_le32((sizeof(struct tx_buf_desc) *
+			    cpu_to_le32((sizeof(struct qlge_tx_buf_desc) *
 					 (frag_cnt - frag_idx)) | TX_DESC_C);
 			dma_unmap_addr_set(&tx_ring_desc->map[map_idx], mapaddr,
 					   map);
 			dma_unmap_len_set(&tx_ring_desc->map[map_idx], maplen,
 					  sizeof(struct qlge_oal));
-			tbd = (struct tx_buf_desc *)&tx_ring_desc->oal;
+			tbd = (struct qlge_tx_buf_desc *)&tx_ring_desc->oal;
 			map_idx++;
 		}
 
@@ -1358,7 +1358,7 @@ static int qlge_map_send(struct qlge_adapter *qdev,
 
 /* Categorizing receive firmware frame errors */
 static void qlge_categorize_rx_err(struct qlge_adapter *qdev, u8 rx_err,
-				   struct rx_ring *rx_ring)
+				   struct qlge_rx_ring *rx_ring)
 {
 	struct nic_stats *stats = &qdev->nic_stats;
 
@@ -1414,7 +1414,7 @@ static void qlge_update_mac_hdr_len(struct qlge_adapter *qdev,
 
 /* Process an inbound completion from an rx ring. */
 static void qlge_process_mac_rx_gro_page(struct qlge_adapter *qdev,
-					 struct rx_ring *rx_ring,
+					 struct qlge_rx_ring *rx_ring,
 					 struct qlge_ib_mac_iocb_rsp *ib_mac_rsp,
 					 u32 length, u16 vlan_id)
 {
@@ -1460,7 +1460,7 @@ static void qlge_process_mac_rx_gro_page(struct qlge_adapter *qdev,
 
 /* Process an inbound completion from an rx ring. */
 static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
-				     struct rx_ring *rx_ring,
+				     struct qlge_rx_ring *rx_ring,
 				     struct qlge_ib_mac_iocb_rsp *ib_mac_rsp,
 				     u32 length, u16 vlan_id)
 {
@@ -1471,7 +1471,7 @@ static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
 	struct napi_struct *napi = &rx_ring->napi;
 	size_t hlen = ETH_HLEN;
 
-	skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
+	skb = napi_alloc_skb(&rx_ring->napi, QLGE_SMALL_BUFFER_SIZE);
 	if (!skb) {
 		rx_ring->rx_dropped++;
 		put_page(lbq_desc->p.pg_chunk.page);
@@ -1551,7 +1551,7 @@ static void qlge_process_mac_rx_page(struct qlge_adapter *qdev,
 
 /* Process an inbound completion from an rx ring. */
 static void qlge_process_mac_rx_skb(struct qlge_adapter *qdev,
-				    struct rx_ring *rx_ring,
+				    struct qlge_rx_ring *rx_ring,
 				    struct qlge_ib_mac_iocb_rsp *ib_mac_rsp,
 				    u32 length, u16 vlan_id)
 {
@@ -1569,7 +1569,7 @@ static void qlge_process_mac_rx_skb(struct qlge_adapter *qdev,
 	skb_reserve(new_skb, NET_IP_ALIGN);
 
 	dma_sync_single_for_cpu(&qdev->pdev->dev, sbq_desc->dma_addr,
-				SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
+				QLGE_SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
 
 	skb_put_data(new_skb, skb->data, length);
 
@@ -1671,7 +1671,7 @@ static void qlge_realign_skb(struct sk_buff *skb, int len)
  * future, but for not it works well.
  */
 static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
-					 struct rx_ring *rx_ring,
+					 struct qlge_rx_ring *rx_ring,
 					 struct qlge_ib_mac_iocb_rsp *ib_mac_rsp)
 {
 	u32 length = le32_to_cpu(ib_mac_rsp->data_len);
@@ -1692,7 +1692,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 		 */
 		sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
 		dma_unmap_single(&qdev->pdev->dev, sbq_desc->dma_addr,
-				 SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
+				 QLGE_SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
 		skb = sbq_desc->p.skb;
 		qlge_realign_skb(skb, hdr_len);
 		skb_put(skb, hdr_len);
@@ -1723,7 +1723,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 			sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
 			dma_sync_single_for_cpu(&qdev->pdev->dev,
 						sbq_desc->dma_addr,
-						SMALL_BUF_MAP_SIZE,
+						QLGE_SMALL_BUF_MAP_SIZE,
 						DMA_FROM_DEVICE);
 			skb_put_data(skb, sbq_desc->p.skb->data, length);
 		} else {
@@ -1735,7 +1735,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 			qlge_realign_skb(skb, length);
 			skb_put(skb, length);
 			dma_unmap_single(&qdev->pdev->dev, sbq_desc->dma_addr,
-					 SMALL_BUF_MAP_SIZE,
+					 QLGE_SMALL_BUF_MAP_SIZE,
 					 DMA_FROM_DEVICE);
 			sbq_desc->p.skb = NULL;
 		}
@@ -1765,7 +1765,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 			 * jumbo mtu on a non-TCP/UDP frame.
 			 */
 			lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
-			skb = napi_alloc_skb(&rx_ring->napi, SMALL_BUFFER_SIZE);
+			skb = napi_alloc_skb(&rx_ring->napi, QLGE_SMALL_BUFFER_SIZE);
 			if (!skb) {
 				netif_printk(qdev, probe, KERN_DEBUG, qdev->ndev,
 					     "No skb available, drop the packet.\n");
@@ -1805,7 +1805,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 
 		sbq_desc = qlge_get_curr_buf(&rx_ring->sbq);
 		dma_unmap_single(&qdev->pdev->dev, sbq_desc->dma_addr,
-				 SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
+				 QLGE_SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
 		if (!(ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS)) {
 			/*
 			 * This is an non TCP/UDP IP frame, so
@@ -1848,7 +1848,7 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 
 /* Process an inbound completion from an rx ring. */
 static void qlge_process_mac_split_rx_intr(struct qlge_adapter *qdev,
-					   struct rx_ring *rx_ring,
+					   struct qlge_rx_ring *rx_ring,
 					   struct qlge_ib_mac_iocb_rsp *ib_mac_rsp,
 					   u16 vlan_id)
 {
@@ -1942,7 +1942,7 @@ static void qlge_process_mac_split_rx_intr(struct qlge_adapter *qdev,
 
 /* Process an inbound completion from an rx ring. */
 static unsigned long qlge_process_mac_rx_intr(struct qlge_adapter *qdev,
-					      struct rx_ring *rx_ring,
+					      struct qlge_rx_ring *rx_ring,
 					      struct qlge_ib_mac_iocb_rsp *ib_mac_rsp)
 {
 	u32 length = le32_to_cpu(ib_mac_rsp->data_len);
@@ -1993,8 +1993,8 @@ static unsigned long qlge_process_mac_rx_intr(struct qlge_adapter *qdev,
 static void qlge_process_mac_tx_intr(struct qlge_adapter *qdev,
 				     struct qlge_ob_mac_iocb_rsp *mac_rsp)
 {
-	struct tx_ring *tx_ring;
-	struct tx_ring_desc *tx_ring_desc;
+	struct qlge_tx_ring *tx_ring;
+	struct qlge_tx_ring_desc *tx_ring_desc;
 
 	tx_ring = &qdev->tx_ring[mac_rsp->txq_idx];
 	tx_ring_desc = &tx_ring->q[mac_rsp->tid];
@@ -2087,14 +2087,14 @@ static void qlge_process_chip_ae_intr(struct qlge_adapter *qdev,
 	}
 }
 
-static int qlge_clean_outbound_rx_ring(struct rx_ring *rx_ring)
+static int qlge_clean_outbound_rx_ring(struct qlge_rx_ring *rx_ring)
 {
 	struct qlge_adapter *qdev = rx_ring->qdev;
 	u32 prod = qlge_read_sh_reg(rx_ring->prod_idx_sh_reg);
 	struct qlge_ob_mac_iocb_rsp *net_rsp = NULL;
 	int count = 0;
 
-	struct tx_ring *tx_ring;
+	struct qlge_tx_ring *tx_ring;
 	/* While there are entries in the completion queue. */
 	while (prod != rx_ring->cnsmr_idx) {
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
@@ -2133,7 +2133,7 @@ static int qlge_clean_outbound_rx_ring(struct rx_ring *rx_ring)
 	return count;
 }
 
-static int qlge_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
+static int qlge_clean_inbound_rx_ring(struct qlge_rx_ring *rx_ring, int budget)
 {
 	struct qlge_adapter *qdev = rx_ring->qdev;
 	u32 prod = qlge_read_sh_reg(rx_ring->prod_idx_sh_reg);
@@ -2178,9 +2178,9 @@ static int qlge_clean_inbound_rx_ring(struct rx_ring *rx_ring, int budget)
 
 static int qlge_napi_poll_msix(struct napi_struct *napi, int budget)
 {
-	struct rx_ring *rx_ring = container_of(napi, struct rx_ring, napi);
+	struct qlge_rx_ring *rx_ring = container_of(napi, struct qlge_rx_ring, napi);
 	struct qlge_adapter *qdev = rx_ring->qdev;
-	struct rx_ring *trx_ring;
+	struct qlge_rx_ring *trx_ring;
 	int i, work_done = 0;
 	struct intr_context *ctx = &qdev->intr_context[rx_ring->cq_id];
 
@@ -2368,7 +2368,7 @@ static void qlge_restore_vlan(struct qlge_adapter *qdev)
 /* MSI-X Multiple Vector Interrupt Handler for inbound completions. */
 static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id)
 {
-	struct rx_ring *rx_ring = dev_id;
+	struct qlge_rx_ring *rx_ring = dev_id;
 
 	napi_schedule(&rx_ring->napi);
 	return IRQ_HANDLED;
@@ -2381,7 +2381,7 @@ static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id)
  */
 static irqreturn_t qlge_isr(int irq, void *dev_id)
 {
-	struct rx_ring *rx_ring = dev_id;
+	struct qlge_rx_ring *rx_ring = dev_id;
 	struct qlge_adapter *qdev = rx_ring->qdev;
 	struct intr_context *intr_context = &qdev->intr_context[0];
 	u32 var;
@@ -2529,9 +2529,9 @@ static netdev_tx_t qlge_send(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct qlge_adapter *qdev = netdev_to_qdev(ndev);
 	struct qlge_ob_mac_iocb_req *mac_iocb_ptr;
-	struct tx_ring_desc *tx_ring_desc;
+	struct qlge_tx_ring_desc *tx_ring_desc;
 	int tso;
-	struct tx_ring *tx_ring;
+	struct qlge_tx_ring *tx_ring;
 	u32 tx_ring_idx = (u32)skb->queue_mapping;
 
 	tx_ring = &qdev->tx_ring[tx_ring_idx];
@@ -2654,9 +2654,9 @@ static int qlge_alloc_shadow_space(struct qlge_adapter *qdev)
 	return -ENOMEM;
 }
 
-static void qlge_init_tx_ring(struct qlge_adapter *qdev, struct tx_ring *tx_ring)
+static void qlge_init_tx_ring(struct qlge_adapter *qdev, struct qlge_tx_ring *tx_ring)
 {
-	struct tx_ring_desc *tx_ring_desc;
+	struct qlge_tx_ring_desc *tx_ring_desc;
 	int i;
 	struct qlge_ob_mac_iocb_req *mac_iocb_ptr;
 
@@ -2673,7 +2673,7 @@ static void qlge_init_tx_ring(struct qlge_adapter *qdev, struct tx_ring *tx_ring
 }
 
 static void qlge_free_tx_resources(struct qlge_adapter *qdev,
-				   struct tx_ring *tx_ring)
+				   struct qlge_tx_ring *tx_ring)
 {
 	if (tx_ring->wq_base) {
 		dma_free_coherent(&qdev->pdev->dev, tx_ring->wq_size,
@@ -2685,7 +2685,7 @@ static void qlge_free_tx_resources(struct qlge_adapter *qdev,
 }
 
 static int qlge_alloc_tx_resources(struct qlge_adapter *qdev,
-				   struct tx_ring *tx_ring)
+				   struct qlge_tx_ring *tx_ring)
 {
 	tx_ring->wq_base =
 		dma_alloc_coherent(&qdev->pdev->dev, tx_ring->wq_size,
@@ -2696,7 +2696,7 @@ static int qlge_alloc_tx_resources(struct qlge_adapter *qdev,
 		goto pci_alloc_err;
 
 	tx_ring->q =
-		kmalloc_array(tx_ring->wq_len, sizeof(struct tx_ring_desc),
+		kmalloc_array(tx_ring->wq_len, sizeof(struct qlge_tx_ring_desc),
 			      GFP_KERNEL);
 	if (!tx_ring->q)
 		goto err;
@@ -2711,7 +2711,7 @@ static int qlge_alloc_tx_resources(struct qlge_adapter *qdev,
 	return -ENOMEM;
 }
 
-static void qlge_free_lbq_buffers(struct qlge_adapter *qdev, struct rx_ring *rx_ring)
+static void qlge_free_lbq_buffers(struct qlge_adapter *qdev, struct qlge_rx_ring *rx_ring)
 {
 	struct qlge_bq *lbq = &rx_ring->lbq;
 	unsigned int last_offset;
@@ -2738,7 +2738,7 @@ static void qlge_free_lbq_buffers(struct qlge_adapter *qdev, struct rx_ring *rx_
 	}
 }
 
-static void qlge_free_sbq_buffers(struct qlge_adapter *qdev, struct rx_ring *rx_ring)
+static void qlge_free_sbq_buffers(struct qlge_adapter *qdev, struct qlge_rx_ring *rx_ring)
 {
 	int i;
 
@@ -2752,7 +2752,7 @@ static void qlge_free_sbq_buffers(struct qlge_adapter *qdev, struct rx_ring *rx_
 		}
 		if (sbq_desc->p.skb) {
 			dma_unmap_single(&qdev->pdev->dev, sbq_desc->dma_addr,
-					 SMALL_BUF_MAP_SIZE,
+					 QLGE_SMALL_BUF_MAP_SIZE,
 					 DMA_FROM_DEVICE);
 			dev_kfree_skb(sbq_desc->p.skb);
 			sbq_desc->p.skb = NULL;
@@ -2768,7 +2768,7 @@ static void qlge_free_rx_buffers(struct qlge_adapter *qdev)
 	int i;
 
 	for (i = 0; i < qdev->rx_ring_count; i++) {
-		struct rx_ring *rx_ring = &qdev->rx_ring[i];
+		struct qlge_rx_ring *rx_ring = &qdev->rx_ring[i];
 
 		if (rx_ring->lbq.queue)
 			qlge_free_lbq_buffers(qdev, rx_ring);
@@ -2788,7 +2788,7 @@ static void qlge_alloc_rx_buffers(struct qlge_adapter *qdev)
 
 static int qlge_init_bq(struct qlge_bq *bq)
 {
-	struct rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
+	struct qlge_rx_ring *rx_ring = QLGE_BQ_CONTAINER(bq);
 	struct qlge_adapter *qdev = rx_ring->qdev;
 	struct qlge_bq_desc *bq_desc;
 	__le64 *buf_ptr;
@@ -2816,7 +2816,7 @@ static int qlge_init_bq(struct qlge_bq *bq)
 }
 
 static void qlge_free_rx_resources(struct qlge_adapter *qdev,
-				   struct rx_ring *rx_ring)
+				   struct qlge_rx_ring *rx_ring)
 {
 	/* Free the small buffer queue. */
 	if (rx_ring->sbq.base) {
@@ -2853,7 +2853,7 @@ static void qlge_free_rx_resources(struct qlge_adapter *qdev,
  * on the values in the parameter structure.
  */
 static int qlge_alloc_rx_resources(struct qlge_adapter *qdev,
-				   struct rx_ring *rx_ring)
+				   struct qlge_rx_ring *rx_ring)
 {
 	/*
 	 * Allocate the completion queue for this rx_ring.
@@ -2878,8 +2878,8 @@ static int qlge_alloc_rx_resources(struct qlge_adapter *qdev,
 
 static void qlge_tx_ring_clean(struct qlge_adapter *qdev)
 {
-	struct tx_ring *tx_ring;
-	struct tx_ring_desc *tx_ring_desc;
+	struct qlge_tx_ring *tx_ring;
+	struct qlge_tx_ring_desc *tx_ring_desc;
 	int i, j;
 
 	/*
@@ -2949,7 +2949,7 @@ static int qlge_alloc_mem_resources(struct qlge_adapter *qdev)
  * The control block is defined as
  * "Completion Queue Initialization Control Block", or cqicb.
  */
-static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring)
+static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct qlge_rx_ring *rx_ring)
 {
 	struct cqicb *cqicb = &rx_ring->cqicb;
 	void *shadow_reg = qdev->rx_ring_shadow_reg_area +
@@ -3036,7 +3036,7 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
 		} while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
 		cqicb->sbq_addr =
 			cpu_to_le64(rx_ring->sbq.base_indirect_dma);
-		cqicb->sbq_buf_size = cpu_to_le16(SMALL_BUFFER_SIZE);
+		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
 		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
 		rx_ring->sbq.next_to_use = 0;
 		rx_ring->sbq.next_to_clean = 0;
@@ -3062,7 +3062,7 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct rx_ring *rx_ring
 	return err;
 }
 
-static int qlge_start_tx_ring(struct qlge_adapter *qdev, struct tx_ring *tx_ring)
+static int qlge_start_tx_ring(struct qlge_adapter *qdev, struct qlge_tx_ring *tx_ring)
 {
 	struct wqicb *wqicb = (struct wqicb *)tx_ring;
 	void __iomem *doorbell_area =
@@ -3917,8 +3917,8 @@ static void qlge_set_lb_size(struct qlge_adapter *qdev)
 static int qlge_configure_rings(struct qlge_adapter *qdev)
 {
 	int i;
-	struct rx_ring *rx_ring;
-	struct tx_ring *tx_ring;
+	struct qlge_rx_ring *rx_ring;
+	struct qlge_tx_ring *tx_ring;
 	int cpu_cnt = min_t(int, MAX_CPUS, num_online_cpus());
 
 	/* In a perfect world we have one RSS ring for each CPU
@@ -4083,8 +4083,8 @@ static struct net_device_stats *qlge_get_stats(struct net_device
 					       *ndev)
 {
 	struct qlge_adapter *qdev = netdev_to_qdev(ndev);
-	struct rx_ring *rx_ring = &qdev->rx_ring[0];
-	struct tx_ring *tx_ring = &qdev->tx_ring[0];
+	struct qlge_rx_ring *rx_ring = &qdev->rx_ring[0];
+	struct qlge_tx_ring *tx_ring = &qdev->tx_ring[0];
 	unsigned long pkts, mcast, dropped, errors, bytes;
 	int i;
 
@@ -4648,7 +4648,7 @@ netdev_tx_t qlge_lb_send(struct sk_buff *skb, struct net_device *ndev)
 	return qlge_send(skb, ndev);
 }
 
-int qlge_clean_lb_rx_ring(struct rx_ring *rx_ring, int budget)
+int qlge_clean_lb_rx_ring(struct qlge_rx_ring *rx_ring, int budget)
 {
 	return qlge_clean_inbound_rx_ring(rx_ring, budget);
 }
-- 
2.32.0

Re: [RFC 04/19] staging: qlge: add qlge_* prefix to avoid namespace clashes

[Benjamin Poirier]

On 2021-06-21 21:48 +0800, Coiby Xu wrote:
> This patch extends commit f8c047be540197ec69cde33e00e82d23961459ea
> ("staging: qlge: use qlge_* prefix to avoid namespace clashes with other qlogic drivers")
> to add qlge_ prefix to rx_ring and tx_ring related structures.

There are still many struct, defines and enums in qlge.h which don't
have a prefix or mix ql_ and qlge_, some of which conflict with other
instances elsewhere in the kernel.
ex: QL_ADAPTER_UP

I think they should all be changed, not just the ones have a conflict
today.

Re: [RFC 04/19] staging: qlge: add qlge_* prefix to avoid namespace clashes

[Benjamin Poirier]

On 2021-06-21 21:48 +0800, Coiby Xu wrote:
> This patch extends commit f8c047be540197ec69cde33e00e82d23961459ea
> ("staging: qlge: use qlge_* prefix to avoid namespace clashes with other qlogic drivers")
> to add qlge_ prefix to rx_ring and tx_ring related structures.

There are still many struct, defines and enums in qlge.h which don't
have a prefix or mix ql_ and qlge_, some of which conflict with other
instances elsewhere in the kernel.
ex: QL_ADAPTER_UP

I think they should all be changed, not just the ones have a conflict
today.

Re: [RFC 04/19] staging: qlge: add qlge_* prefix to avoid namespace clashes

[Coiby Xu]

On Tue, Jun 22, 2021 at 04:55:42PM +0900, Benjamin Poirier wrote:
>On 2021-06-21 21:48 +0800, Coiby Xu wrote:
>> This patch extends commit f8c047be540197ec69cde33e00e82d23961459ea
>> ("staging: qlge: use qlge_* prefix to avoid namespace clashes with other qlogic drivers")
>> to add qlge_ prefix to rx_ring and tx_ring related structures.
>
>There are still many struct, defines and enums in qlge.h which don't
>have a prefix or mix ql_ and qlge_, some of which conflict with other
>instances elsewhere in the kernel.
>ex: QL_ADAPTER_UP
>
>I think they should all be changed, not just the ones have a conflict
>today.

I'll fix them in next version. Thanks for the reminder!

-- 
Best regards,
Coiby

Re: [RFC 04/19] staging: qlge: add qlge_* prefix to avoid namespace clashes

[Coiby Xu]

On Tue, Jun 22, 2021 at 04:55:42PM +0900, Benjamin Poirier wrote:
>On 2021-06-21 21:48 +0800, Coiby Xu wrote:
>> This patch extends commit f8c047be540197ec69cde33e00e82d23961459ea
>> ("staging: qlge: use qlge_* prefix to avoid namespace clashes with other qlogic drivers")
>> to add qlge_ prefix to rx_ring and tx_ring related structures.
>
>There are still many struct, defines and enums in qlge.h which don't
>have a prefix or mix ql_ and qlge_, some of which conflict with other
>instances elsewhere in the kernel.
>ex: QL_ADAPTER_UP
>
>I think they should all be changed, not just the ones have a conflict
>today.

I'll fix them in next version. Thanks for the reminder!

-- 
Best regards,
Coiby

[RFC 05/19] staging: qlge: rename rx to completion queue and seperate rx_ring from completion queue

[Coiby Xu]

This patch addresses the following TODO items,
> * rename "rx" queues to "completion" queues. Calling tx completion queues "rx
>   queues" is confusing.
> * struct rx_ring is used for rx and tx completions, with some members relevant
>   to one case only

The first part of the completion queue (index range: [0, qdev->rss_ring_count))
is for inbound completions and the remaining part (index range:
[qdev->rss_ring_count, qdev->cq_count)) is for outbound completions.

Note the structure filed name and reserved is unused, remove it to
reduce holes.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO           |   4 -
 drivers/staging/qlge/qlge.h         |  30 +--
 drivers/staging/qlge/qlge_dbg.c     |   6 +-
 drivers/staging/qlge/qlge_ethtool.c |  24 +--
 drivers/staging/qlge/qlge_main.c    | 291 +++++++++++++++-------------
 5 files changed, 188 insertions(+), 167 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 49cb09fc2be4..b7a60425fcd2 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,10 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* rename "rx" queues to "completion" queues. Calling tx completion queues "rx
-  queues" is confusing.
-* struct rx_ring is used for rx and tx completions, with some members relevant
-  to one case only
 * the flow control implementation in firmware is buggy (sends a flood of pause
   frames, resets the link, device and driver buffer queues become
   desynchronized), disable it by default
diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 09d5878b95f7..926af25b14fa 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -1456,15 +1456,15 @@ struct qlge_bq {
 		     (_bq)->next_to_clean); \
 })
 
-struct qlge_rx_ring {
+struct qlge_cq {
 	struct cqicb cqicb;	/* The chip's completion queue init control block. */
 
 	/* Completion queue elements. */
+	u16 cq_id;
 	void *cq_base;
 	dma_addr_t cq_base_dma;
 	u32 cq_size;
 	u32 cq_len;
-	u16 cq_id;
 	__le32 *prod_idx_sh_reg;	/* Shadowed producer register. */
 	dma_addr_t prod_idx_sh_reg_dma;
 	void __iomem *cnsmr_idx_db_reg;	/* PCI doorbell mem area + 0 */
@@ -1472,6 +1472,13 @@ struct qlge_rx_ring {
 	struct qlge_net_rsp_iocb *curr_entry;	/* next entry on queue */
 	void __iomem *valid_db_reg;	/* PCI doorbell mem area + 0x04 */
 
+	/* Misc. handler elements. */
+	u32 irq;		/* Which vector this ring is assigned. */
+	u32 cpu;		/* Which CPU this should run on. */
+	struct qlge_adapter *qdev;
+};
+
+struct qlge_rx_ring {
 	/* Large buffer queue elements. */
 	struct qlge_bq lbq;
 	struct qlge_page_chunk master_chunk;
@@ -1480,19 +1487,15 @@ struct qlge_rx_ring {
 	/* Small buffer queue elements. */
 	struct qlge_bq sbq;
 
-	/* Misc. handler elements. */
-	u32 irq;		/* Which vector this ring is assigned. */
-	u32 cpu;		/* Which CPU this should run on. */
-	struct delayed_work refill_work;
-	char name[IFNAMSIZ + 5];
 	struct napi_struct napi;
-	u8 reserved;
-	struct qlge_adapter *qdev;
+	struct delayed_work refill_work;
 	u64 rx_packets;
 	u64 rx_multicast;
 	u64 rx_bytes;
 	u64 rx_dropped;
 	u64 rx_errors;
+	struct qlge_adapter *qdev;
+	u16 cq_id;
 };
 
 /*
@@ -1753,7 +1756,7 @@ enum {
 #define SHADOW_REG_SHIFT	20
 
 struct qlge_nic_misc {
-	u32 rx_ring_count;
+	u32 cq_count;
 	u32 tx_ring_count;
 	u32 intr_count;
 	u32 function;
@@ -2127,14 +2130,15 @@ struct qlge_adapter {
 	int tx_ring_count;	/* One per online CPU. */
 	u32 rss_ring_count;	/* One per irq vector.  */
 	/*
-	 * rx_ring_count =
+	 * cq_count =
 	 *  (CPU count * outbound completion rx_ring) +
 	 *  (irq_vector_cnt * inbound (RSS) completion rx_ring)
 	 */
-	int rx_ring_count;
+	int cq_count;
 	int ring_mem_size;
 	void *ring_mem;
 
+	struct qlge_cq cq[MAX_RX_RINGS];
 	struct qlge_rx_ring rx_ring[MAX_RX_RINGS];
 	struct qlge_tx_ring tx_ring[MAX_TX_RINGS];
 	unsigned int lbq_buf_order;
@@ -2287,6 +2291,6 @@ void qlge_get_dump(struct qlge_adapter *qdev, void *buff);
 netdev_tx_t qlge_lb_send(struct sk_buff *skb, struct net_device *ndev);
 void qlge_check_lb_frame(struct qlge_adapter *qdev, struct sk_buff *skb);
 int qlge_own_firmware(struct qlge_adapter *qdev);
-int qlge_clean_lb_rx_ring(struct qlge_rx_ring *rx_ring, int budget);
+int qlge_clean_lb_cq(struct qlge_cq *cq, int budget);
 
 #endif /* _QLGE_H_ */
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 37e593f0fd82..d093e6c9f19c 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -403,7 +403,7 @@ static void qlge_get_intr_states(struct qlge_adapter *qdev, u32 *buf)
 {
 	int i;
 
-	for (i = 0; i < qdev->rx_ring_count; i++, buf++) {
+	for (i = 0; i < qdev->cq_count; i++, buf++) {
 		qlge_write32(qdev, INTR_EN,
 			     qdev->intr_context[i].intr_read_mask);
 		*buf = qlge_read32(qdev, INTR_EN);
@@ -1074,7 +1074,7 @@ int qlge_core_dump(struct qlge_adapter *qdev, struct qlge_mpi_coredump *mpi_core
 				       sizeof(struct mpi_coredump_segment_header)
 				       + sizeof(mpi_coredump->misc_nic_info),
 				       "MISC NIC INFO");
-	mpi_coredump->misc_nic_info.rx_ring_count = qdev->rx_ring_count;
+	mpi_coredump->misc_nic_info.cq_count = qdev->cq_count;
 	mpi_coredump->misc_nic_info.tx_ring_count = qdev->tx_ring_count;
 	mpi_coredump->misc_nic_info.intr_count = qdev->intr_count;
 	mpi_coredump->misc_nic_info.function = qdev->func;
@@ -1237,7 +1237,7 @@ static void qlge_gen_reg_dump(struct qlge_adapter *qdev,
 				       sizeof(struct mpi_coredump_segment_header)
 				       + sizeof(mpi_coredump->misc_nic_info),
 				       "MISC NIC INFO");
-	mpi_coredump->misc_nic_info.rx_ring_count = qdev->rx_ring_count;
+	mpi_coredump->misc_nic_info.cq_count = qdev->cq_count;
 	mpi_coredump->misc_nic_info.tx_ring_count = qdev->tx_ring_count;
 	mpi_coredump->misc_nic_info.intr_count = qdev->intr_count;
 	mpi_coredump->misc_nic_info.function = qdev->func;
diff --git a/drivers/staging/qlge/qlge_ethtool.c b/drivers/staging/qlge/qlge_ethtool.c
index 22c27b97a908..7f77f99cc047 100644
--- a/drivers/staging/qlge/qlge_ethtool.c
+++ b/drivers/staging/qlge/qlge_ethtool.c
@@ -186,7 +186,7 @@ static const char qlge_gstrings_test[][ETH_GSTRING_LEN] = {
 static int qlge_update_ring_coalescing(struct qlge_adapter *qdev)
 {
 	int i, status = 0;
-	struct qlge_rx_ring *rx_ring;
+	struct qlge_cq *cq;
 	struct cqicb *cqicb;
 
 	if (!netif_running(qdev->ndev))
@@ -195,18 +195,18 @@ static int qlge_update_ring_coalescing(struct qlge_adapter *qdev)
 	/* Skip the default queue, and update the outbound handler
 	 * queues if they changed.
 	 */
-	cqicb = (struct cqicb *)&qdev->rx_ring[qdev->rss_ring_count];
+	cqicb = (struct cqicb *)&qdev->cq[qdev->rss_ring_count];
 	if (le16_to_cpu(cqicb->irq_delay) != qdev->tx_coalesce_usecs ||
 	    le16_to_cpu(cqicb->pkt_delay) != qdev->tx_max_coalesced_frames) {
-		for (i = qdev->rss_ring_count; i < qdev->rx_ring_count; i++) {
-			rx_ring = &qdev->rx_ring[i];
-			cqicb = (struct cqicb *)rx_ring;
+		for (i = qdev->rss_ring_count; i < qdev->cq_count; i++) {
+			cq = &qdev->cq[i];
+			cqicb = (struct cqicb *)cq;
 			cqicb->irq_delay = cpu_to_le16(qdev->tx_coalesce_usecs);
 			cqicb->pkt_delay =
 				cpu_to_le16(qdev->tx_max_coalesced_frames);
 			cqicb->flags = FLAGS_LI;
 			status = qlge_write_cfg(qdev, cqicb, sizeof(*cqicb),
-						CFG_LCQ, rx_ring->cq_id);
+						CFG_LCQ, cq->cq_id);
 			if (status) {
 				netif_err(qdev, ifup, qdev->ndev,
 					  "Failed to load CQICB.\n");
@@ -216,18 +216,18 @@ static int qlge_update_ring_coalescing(struct qlge_adapter *qdev)
 	}
 
 	/* Update the inbound (RSS) handler queues if they changed. */
-	cqicb = (struct cqicb *)&qdev->rx_ring[0];
+	cqicb = (struct cqicb *)&qdev->cq[0];
 	if (le16_to_cpu(cqicb->irq_delay) != qdev->rx_coalesce_usecs ||
 	    le16_to_cpu(cqicb->pkt_delay) != qdev->rx_max_coalesced_frames) {
-		for (i = 0; i < qdev->rss_ring_count; i++, rx_ring++) {
-			rx_ring = &qdev->rx_ring[i];
-			cqicb = (struct cqicb *)rx_ring;
+		for (i = 0; i < qdev->rss_ring_count; i++, cq++) {
+			cq = &qdev->cq[i];
+			cqicb = (struct cqicb *)cq;
 			cqicb->irq_delay = cpu_to_le16(qdev->rx_coalesce_usecs);
 			cqicb->pkt_delay =
 				cpu_to_le16(qdev->rx_max_coalesced_frames);
 			cqicb->flags = FLAGS_LI;
 			status = qlge_write_cfg(qdev, cqicb, sizeof(*cqicb),
-						CFG_LCQ, rx_ring->cq_id);
+						CFG_LCQ, cq->cq_id);
 			if (status) {
 				netif_err(qdev, ifup, qdev->ndev,
 					  "Failed to load CQICB.\n");
@@ -554,7 +554,7 @@ static int qlge_run_loopback_test(struct qlge_adapter *qdev)
 	}
 	/* Give queue time to settle before testing results. */
 	msleep(2);
-	qlge_clean_lb_rx_ring(&qdev->rx_ring[0], 128);
+	qlge_clean_lb_cq(&qdev->cq[0], 128);
 	return atomic_read(&qdev->lb_count) ? -EIO : 0;
 }
 
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 77c71ae698ab..94853b182608 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -982,19 +982,19 @@ static struct qlge_bq_desc *qlge_get_curr_lchunk(struct qlge_adapter *qdev,
 }
 
 /* Update an rx ring index. */
-static void qlge_update_cq(struct qlge_rx_ring *rx_ring)
+static void qlge_update_cq(struct qlge_cq *cq)
 {
-	rx_ring->cnsmr_idx++;
-	rx_ring->curr_entry++;
-	if (unlikely(rx_ring->cnsmr_idx == rx_ring->cq_len)) {
-		rx_ring->cnsmr_idx = 0;
-		rx_ring->curr_entry = rx_ring->cq_base;
+	cq->cnsmr_idx++;
+	cq->curr_entry++;
+	if (unlikely(cq->cnsmr_idx == cq->cq_len)) {
+		cq->cnsmr_idx = 0;
+		cq->curr_entry = cq->cq_base;
 	}
 }
 
-static void qlge_write_cq_idx(struct qlge_rx_ring *rx_ring)
+static void qlge_write_cq_idx(struct qlge_cq *cq)
 {
-	qlge_write_db_reg(rx_ring->cnsmr_idx, rx_ring->cnsmr_idx_db_reg);
+	qlge_write_db_reg(cq->cnsmr_idx, cq->cnsmr_idx_db_reg);
 }
 
 static const char * const bq_type_name[] = {
@@ -2087,21 +2087,21 @@ static void qlge_process_chip_ae_intr(struct qlge_adapter *qdev,
 	}
 }
 
-static int qlge_clean_outbound_rx_ring(struct qlge_rx_ring *rx_ring)
+static int qlge_clean_outbound_cq(struct qlge_cq *cq)
 {
-	struct qlge_adapter *qdev = rx_ring->qdev;
-	u32 prod = qlge_read_sh_reg(rx_ring->prod_idx_sh_reg);
+	struct qlge_adapter *qdev = cq->qdev;
+	u32 prod = qlge_read_sh_reg(cq->prod_idx_sh_reg);
 	struct qlge_ob_mac_iocb_rsp *net_rsp = NULL;
 	int count = 0;
 
 	struct qlge_tx_ring *tx_ring;
 	/* While there are entries in the completion queue. */
-	while (prod != rx_ring->cnsmr_idx) {
+	while (prod != cq->cnsmr_idx) {
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 			     "cq_id = %d, prod = %d, cnsmr = %d\n",
-			     rx_ring->cq_id, prod, rx_ring->cnsmr_idx);
+			     cq->cq_id, prod, cq->cnsmr_idx);
 
-		net_rsp = (struct qlge_ob_mac_iocb_rsp *)rx_ring->curr_entry;
+		net_rsp = (struct qlge_ob_mac_iocb_rsp *)cq->curr_entry;
 		rmb();
 		switch (net_rsp->opcode) {
 		case OPCODE_OB_MAC_TSO_IOCB:
@@ -2114,12 +2114,12 @@ static int qlge_clean_outbound_rx_ring(struct qlge_rx_ring *rx_ring)
 				     net_rsp->opcode);
 		}
 		count++;
-		qlge_update_cq(rx_ring);
-		prod = qlge_read_sh_reg(rx_ring->prod_idx_sh_reg);
+		qlge_update_cq(cq);
+		prod = qlge_read_sh_reg(cq->prod_idx_sh_reg);
 	}
 	if (!net_rsp)
 		return 0;
-	qlge_write_cq_idx(rx_ring);
+	qlge_write_cq_idx(cq);
 	tx_ring = &qdev->tx_ring[net_rsp->txq_idx];
 	if (__netif_subqueue_stopped(qdev->ndev, tx_ring->wq_id)) {
 		if ((atomic_read(&tx_ring->tx_count) > (tx_ring->wq_len / 4)))
@@ -2133,20 +2133,21 @@ static int qlge_clean_outbound_rx_ring(struct qlge_rx_ring *rx_ring)
 	return count;
 }
 
-static int qlge_clean_inbound_rx_ring(struct qlge_rx_ring *rx_ring, int budget)
+static int qlge_clean_inbound_cq(struct qlge_cq *cq, int budget)
 {
-	struct qlge_adapter *qdev = rx_ring->qdev;
-	u32 prod = qlge_read_sh_reg(rx_ring->prod_idx_sh_reg);
+	struct qlge_adapter *qdev = cq->qdev;
+	u32 prod = qlge_read_sh_reg(cq->prod_idx_sh_reg);
+	struct qlge_rx_ring *rx_ring = &qdev->rx_ring[cq->cq_id];
 	struct qlge_net_rsp_iocb *net_rsp;
 	int count = 0;
 
 	/* While there are entries in the completion queue. */
-	while (prod != rx_ring->cnsmr_idx) {
+	while (prod != cq->cnsmr_idx) {
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 			     "cq_id = %d, prod = %d, cnsmr = %d\n",
-			     rx_ring->cq_id, prod, rx_ring->cnsmr_idx);
+			     cq->cq_id, prod, cq->cnsmr_idx);
 
-		net_rsp = rx_ring->curr_entry;
+		net_rsp = cq->curr_entry;
 		rmb();
 		switch (net_rsp->opcode) {
 		case OPCODE_IB_MAC_IOCB:
@@ -2166,13 +2167,13 @@ static int qlge_clean_inbound_rx_ring(struct qlge_rx_ring *rx_ring, int budget)
 			break;
 		}
 		count++;
-		qlge_update_cq(rx_ring);
-		prod = qlge_read_sh_reg(rx_ring->prod_idx_sh_reg);
+		qlge_update_cq(cq);
+		prod = qlge_read_sh_reg(cq->prod_idx_sh_reg);
 		if (count == budget)
 			break;
 	}
 	qlge_update_buffer_queues(rx_ring, GFP_ATOMIC, 0);
-	qlge_write_cq_idx(rx_ring);
+	qlge_write_cq_idx(cq);
 	return count;
 }
 
@@ -2180,45 +2181,46 @@ static int qlge_napi_poll_msix(struct napi_struct *napi, int budget)
 {
 	struct qlge_rx_ring *rx_ring = container_of(napi, struct qlge_rx_ring, napi);
 	struct qlge_adapter *qdev = rx_ring->qdev;
-	struct qlge_rx_ring *trx_ring;
+	struct qlge_cq *tcq, *rcq;
 	int i, work_done = 0;
 	struct intr_context *ctx = &qdev->intr_context[rx_ring->cq_id];
 
+	rcq =  &qdev->cq[rx_ring->cq_id];
+
 	netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 		     "Enter, NAPI POLL cq_id = %d.\n", rx_ring->cq_id);
 
 	/* Service the TX rings first.  They start
 	 * right after the RSS rings.
 	 */
-	for (i = qdev->rss_ring_count; i < qdev->rx_ring_count; i++) {
-		trx_ring = &qdev->rx_ring[i];
+	for (i = qdev->rss_ring_count; i < qdev->cq_count; i++) {
+		tcq = &qdev->cq[i];
 		/* If this TX completion ring belongs to this vector and
 		 * it's not empty then service it.
 		 */
-		if ((ctx->irq_mask & (1 << trx_ring->cq_id)) &&
-		    (qlge_read_sh_reg(trx_ring->prod_idx_sh_reg) !=
-		     trx_ring->cnsmr_idx)) {
+		if ((ctx->irq_mask & (1 << tcq->cq_id)) &&
+		    (qlge_read_sh_reg(tcq->prod_idx_sh_reg) !=
+		     tcq->cnsmr_idx)) {
 			netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
 				     "%s: Servicing TX completion ring %d.\n",
-				     __func__, trx_ring->cq_id);
-			qlge_clean_outbound_rx_ring(trx_ring);
+				     __func__, tcq->cq_id);
+			qlge_clean_outbound_cq(tcq);
 		}
 	}
 
 	/*
 	 * Now service the RSS ring if it's active.
 	 */
-	if (qlge_read_sh_reg(rx_ring->prod_idx_sh_reg) !=
-	    rx_ring->cnsmr_idx) {
+	if (qlge_read_sh_reg(rcq->prod_idx_sh_reg) != rcq->cnsmr_idx) {
 		netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
 			     "%s: Servicing RX completion ring %d.\n",
-			     __func__, rx_ring->cq_id);
-		work_done = qlge_clean_inbound_rx_ring(rx_ring, budget);
+			     __func__, rcq->cq_id);
+		work_done = qlge_clean_inbound_cq(rcq, budget);
 	}
 
 	if (work_done < budget) {
 		napi_complete_done(napi, work_done);
-		qlge_enable_completion_interrupt(qdev, rx_ring->irq);
+		qlge_enable_completion_interrupt(qdev, rcq->irq);
 	}
 	return work_done;
 }
@@ -2368,8 +2370,10 @@ static void qlge_restore_vlan(struct qlge_adapter *qdev)
 /* MSI-X Multiple Vector Interrupt Handler for inbound completions. */
 static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id)
 {
-	struct qlge_rx_ring *rx_ring = dev_id;
+	struct qlge_cq *cq = dev_id;
+	struct qlge_rx_ring *rx_ring;
 
+	rx_ring = &cq->qdev->rx_ring[cq->cq_id];
 	napi_schedule(&rx_ring->napi);
 	return IRQ_HANDLED;
 }
@@ -2381,11 +2385,16 @@ static irqreturn_t qlge_msix_rx_isr(int irq, void *dev_id)
  */
 static irqreturn_t qlge_isr(int irq, void *dev_id)
 {
-	struct qlge_rx_ring *rx_ring = dev_id;
-	struct qlge_adapter *qdev = rx_ring->qdev;
-	struct intr_context *intr_context = &qdev->intr_context[0];
-	u32 var;
+	struct intr_context *intr_context;
+	struct qlge_rx_ring *rx_ring;
+	struct qlge_cq *cq = dev_id;
+	struct qlge_adapter *qdev;
 	int work_done = 0;
+	u32 var;
+
+	qdev = cq->qdev;
+	rx_ring = &qdev->rx_ring[cq->cq_id];
+	intr_context = &qdev->intr_context[0];
 
 	/* Experience shows that when using INTx interrupts, interrupts must
 	 * be masked manually.
@@ -2767,7 +2776,7 @@ static void qlge_free_rx_buffers(struct qlge_adapter *qdev)
 {
 	int i;
 
-	for (i = 0; i < qdev->rx_ring_count; i++) {
+	for (i = 0; i < qdev->rss_ring_count; i++) {
 		struct qlge_rx_ring *rx_ring = &qdev->rx_ring[i];
 
 		if (rx_ring->lbq.queue)
@@ -2815,9 +2824,12 @@ static int qlge_init_bq(struct qlge_bq *bq)
 	return 0;
 }
 
-static void qlge_free_rx_resources(struct qlge_adapter *qdev,
-				   struct qlge_rx_ring *rx_ring)
+static void qlge_free_cq_resources(struct qlge_adapter *qdev,
+				   struct qlge_cq *cq)
 {
+	struct qlge_rx_ring *rx_ring;
+
+	rx_ring = &qdev->rx_ring[cq->cq_id];
 	/* Free the small buffer queue. */
 	if (rx_ring->sbq.base) {
 		dma_free_coherent(&qdev->pdev->dev, QLGE_BQ_SIZE,
@@ -2836,40 +2848,43 @@ static void qlge_free_rx_resources(struct qlge_adapter *qdev,
 		rx_ring->lbq.base = NULL;
 	}
 
+	rx_ring = &qdev->rx_ring[cq->cq_id];
 	/* Free the large buffer queue control blocks. */
 	kfree(rx_ring->lbq.queue);
 	rx_ring->lbq.queue = NULL;
 
-	/* Free the rx queue. */
-	if (rx_ring->cq_base) {
+	/* Free the completion queue. */
+	if (cq->cq_base) {
 		dma_free_coherent(&qdev->pdev->dev,
-				  rx_ring->cq_size,
-				  rx_ring->cq_base, rx_ring->cq_base_dma);
-		rx_ring->cq_base = NULL;
+				  cq->cq_size,
+				  cq->cq_base, cq->cq_base_dma);
+		cq->cq_base = NULL;
 	}
 }
 
 /* Allocate queues and buffers for this completions queue based
  * on the values in the parameter structure.
  */
-static int qlge_alloc_rx_resources(struct qlge_adapter *qdev,
-				   struct qlge_rx_ring *rx_ring)
+static int qlge_alloc_cq_resources(struct qlge_adapter *qdev,
+				   struct qlge_cq *cq)
 {
+	struct qlge_rx_ring *rx_ring;
 	/*
 	 * Allocate the completion queue for this rx_ring.
 	 */
-	rx_ring->cq_base =
-		dma_alloc_coherent(&qdev->pdev->dev, rx_ring->cq_size,
-				   &rx_ring->cq_base_dma, GFP_ATOMIC);
+	cq->cq_base =
+		dma_alloc_coherent(&qdev->pdev->dev, cq->cq_size,
+				   &cq->cq_base_dma, GFP_ATOMIC);
 
-	if (!rx_ring->cq_base) {
-		netif_err(qdev, ifup, qdev->ndev, "rx_ring alloc failed.\n");
+	if (!cq->cq_base) {
+		netif_err(qdev, ifup, qdev->ndev, "cq alloc failed.\n");
 		return -ENOMEM;
 	}
 
-	if (rx_ring->cq_id < qdev->rss_ring_count &&
+	rx_ring = &qdev->rx_ring[cq->cq_id];
+	if (cq->cq_id < qdev->rss_ring_count &&
 	    (qlge_init_bq(&rx_ring->sbq) || qlge_init_bq(&rx_ring->lbq))) {
-		qlge_free_rx_resources(qdev, rx_ring);
+		qlge_free_cq_resources(qdev, cq);
 		return -ENOMEM;
 	}
 
@@ -2910,8 +2925,8 @@ static void qlge_free_mem_resources(struct qlge_adapter *qdev)
 
 	for (i = 0; i < qdev->tx_ring_count; i++)
 		qlge_free_tx_resources(qdev, &qdev->tx_ring[i]);
-	for (i = 0; i < qdev->rx_ring_count; i++)
-		qlge_free_rx_resources(qdev, &qdev->rx_ring[i]);
+	for (i = 0; i < qdev->cq_count; i++)
+		qlge_free_cq_resources(qdev, &qdev->cq[i]);
 	qlge_free_shadow_space(qdev);
 }
 
@@ -2923,8 +2938,8 @@ static int qlge_alloc_mem_resources(struct qlge_adapter *qdev)
 	if (qlge_alloc_shadow_space(qdev))
 		return -ENOMEM;
 
-	for (i = 0; i < qdev->rx_ring_count; i++) {
-		if (qlge_alloc_rx_resources(qdev, &qdev->rx_ring[i]) != 0) {
+	for (i = 0; i < qdev->cq_count; i++) {
+		if (qlge_alloc_cq_resources(qdev, &qdev->cq[i]) != 0) {
 			netif_err(qdev, ifup, qdev->ndev,
 				  "RX resource allocation failed.\n");
 			goto err_mem;
@@ -2949,56 +2964,43 @@ static int qlge_alloc_mem_resources(struct qlge_adapter *qdev)
  * The control block is defined as
  * "Completion Queue Initialization Control Block", or cqicb.
  */
-static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct qlge_rx_ring *rx_ring)
+static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
 {
-	struct cqicb *cqicb = &rx_ring->cqicb;
+	struct cqicb *cqicb = &cq->cqicb;
 	void *shadow_reg = qdev->rx_ring_shadow_reg_area +
-		(rx_ring->cq_id * RX_RING_SHADOW_SPACE);
+		(cq->cq_id * RX_RING_SHADOW_SPACE);
 	u64 shadow_reg_dma = qdev->rx_ring_shadow_reg_dma +
-		(rx_ring->cq_id * RX_RING_SHADOW_SPACE);
+		(cq->cq_id * RX_RING_SHADOW_SPACE);
 	void __iomem *doorbell_area =
-		qdev->doorbell_area + (DB_PAGE_SIZE * (128 + rx_ring->cq_id));
+		qdev->doorbell_area + (DB_PAGE_SIZE * (128 + cq->cq_id));
+	struct qlge_rx_ring *rx_ring;
 	int err = 0;
 	u64 tmp;
 	__le64 *base_indirect_ptr;
 	int page_entries;
 
-	/* Set up the shadow registers for this ring. */
-	rx_ring->prod_idx_sh_reg = shadow_reg;
-	rx_ring->prod_idx_sh_reg_dma = shadow_reg_dma;
-	*rx_ring->prod_idx_sh_reg = 0;
-	shadow_reg += sizeof(u64);
-	shadow_reg_dma += sizeof(u64);
-	rx_ring->lbq.base_indirect = shadow_reg;
-	rx_ring->lbq.base_indirect_dma = shadow_reg_dma;
-	shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
-	shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
-	rx_ring->sbq.base_indirect = shadow_reg;
-	rx_ring->sbq.base_indirect_dma = shadow_reg_dma;
+	/* Set up the shadow registers for this cq. */
+	cq->prod_idx_sh_reg = shadow_reg;
+	cq->prod_idx_sh_reg_dma = shadow_reg_dma;
+	*cq->prod_idx_sh_reg = 0;
 
 	/* PCI doorbell mem area + 0x00 for consumer index register */
-	rx_ring->cnsmr_idx_db_reg = (u32 __iomem *)doorbell_area;
-	rx_ring->cnsmr_idx = 0;
-	rx_ring->curr_entry = rx_ring->cq_base;
+	cq->cnsmr_idx_db_reg = (u32 __iomem *)doorbell_area;
+	cq->cnsmr_idx = 0;
+	cq->curr_entry = cq->cq_base;
 
 	/* PCI doorbell mem area + 0x04 for valid register */
-	rx_ring->valid_db_reg = doorbell_area + 0x04;
-
-	/* PCI doorbell mem area + 0x18 for large buffer consumer */
-	rx_ring->lbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x18);
-
-	/* PCI doorbell mem area + 0x1c */
-	rx_ring->sbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x1c);
+	cq->valid_db_reg = doorbell_area + 0x04;
 
 	memset((void *)cqicb, 0, sizeof(struct cqicb));
-	cqicb->msix_vect = rx_ring->irq;
+	cqicb->msix_vect = cq->irq;
 
-	cqicb->len = cpu_to_le16(QLGE_FIT16(rx_ring->cq_len) | LEN_V |
+	cqicb->len = cpu_to_le16(QLGE_FIT16(cq->cq_len) | LEN_V |
 				 LEN_CPP_CONT);
 
-	cqicb->addr = cpu_to_le64(rx_ring->cq_base_dma);
+	cqicb->addr = cpu_to_le64(cq->cq_base_dma);
 
-	cqicb->prod_idx_addr = cpu_to_le64(rx_ring->prod_idx_sh_reg_dma);
+	cqicb->prod_idx_addr = cpu_to_le64(cq->prod_idx_sh_reg_dma);
 
 	/*
 	 * Set up the control block load flags.
@@ -3006,7 +3008,23 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct qlge_rx_ring *rx
 	cqicb->flags = FLAGS_LC |	/* Load queue base address */
 		FLAGS_LV |		/* Load MSI-X vector */
 		FLAGS_LI;		/* Load irq delay values */
-	if (rx_ring->cq_id < qdev->rss_ring_count) {
+
+	if (cq->cq_id < qdev->rss_ring_count) {
+		rx_ring = &qdev->rx_ring[cq->cq_id];
+		shadow_reg += sizeof(u64);
+		shadow_reg_dma += sizeof(u64);
+		rx_ring->lbq.base_indirect = shadow_reg;
+		rx_ring->lbq.base_indirect_dma = shadow_reg_dma;
+		shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		rx_ring->sbq.base_indirect = shadow_reg;
+		rx_ring->sbq.base_indirect_dma = shadow_reg_dma;
+		/* PCI doorbell mem area + 0x18 for large buffer consumer */
+		rx_ring->lbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x18);
+
+		/* PCI doorbell mem area + 0x1c */
+		rx_ring->sbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x1c);
+
 		cqicb->flags |= FLAGS_LL;	/* Load lbq values */
 		tmp = (u64)rx_ring->lbq.base_dma;
 		base_indirect_ptr = rx_ring->lbq.base_indirect;
@@ -3034,14 +3052,12 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct qlge_rx_ring *rx
 			base_indirect_ptr++;
 			page_entries++;
 		} while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
-		cqicb->sbq_addr =
-			cpu_to_le64(rx_ring->sbq.base_indirect_dma);
+		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
 		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
 		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
 		rx_ring->sbq.next_to_use = 0;
 		rx_ring->sbq.next_to_clean = 0;
-	}
-	if (rx_ring->cq_id < qdev->rss_ring_count) {
+
 		/* Inbound completion handling rx_rings run in
 		 * separate NAPI contexts.
 		 */
@@ -3054,7 +3070,7 @@ static int qlge_start_rx_ring(struct qlge_adapter *qdev, struct qlge_rx_ring *rx
 		cqicb->pkt_delay = cpu_to_le16(qdev->tx_max_coalesced_frames);
 	}
 	err = qlge_write_cfg(qdev, cqicb, sizeof(struct cqicb),
-			     CFG_LCQ, rx_ring->cq_id);
+			     CFG_LCQ, cq->cq_id);
 	if (err) {
 		netif_err(qdev, ifup, qdev->ndev, "Failed to load CQICB.\n");
 		return err;
@@ -3195,20 +3211,20 @@ static void qlge_set_tx_vect(struct qlge_adapter *qdev)
 	if (likely(test_bit(QL_MSIX_ENABLED, &qdev->flags))) {
 		/* Assign irq vectors to TX rx_rings.*/
 		for (vect = 0, j = 0, i = qdev->rss_ring_count;
-		     i < qdev->rx_ring_count; i++) {
+		     i < qdev->cq_count; i++) {
 			if (j == tx_rings_per_vector) {
 				vect++;
 				j = 0;
 			}
-			qdev->rx_ring[i].irq = vect;
+			qdev->cq[i].irq = vect;
 			j++;
 		}
 	} else {
 		/* For single vector all rings have an irq
 		 * of zero.
 		 */
-		for (i = 0; i < qdev->rx_ring_count; i++)
-			qdev->rx_ring[i].irq = 0;
+		for (i = 0; i < qdev->cq_count; i++)
+			qdev->cq[i].irq = 0;
 	}
 }
 
@@ -3226,21 +3242,21 @@ static void qlge_set_irq_mask(struct qlge_adapter *qdev, struct intr_context *ct
 		/* Add the RSS ring serviced by this vector
 		 * to the mask.
 		 */
-		ctx->irq_mask = (1 << qdev->rx_ring[vect].cq_id);
+		ctx->irq_mask = (1 << qdev->cq[vect].cq_id);
 		/* Add the TX ring(s) serviced by this vector
 		 * to the mask.
 		 */
 		for (j = 0; j < tx_rings_per_vector; j++) {
 			ctx->irq_mask |=
-				(1 << qdev->rx_ring[qdev->rss_ring_count +
+				(1 << qdev->cq[qdev->rss_ring_count +
 				 (vect * tx_rings_per_vector) + j].cq_id);
 		}
 	} else {
 		/* For single vector we just shift each queue's
 		 * ID into the mask.
 		 */
-		for (j = 0; j < qdev->rx_ring_count; j++)
-			ctx->irq_mask |= (1 << qdev->rx_ring[j].cq_id);
+		for (j = 0; j < qdev->cq_count; j++)
+			ctx->irq_mask |= (1 << qdev->cq[j].cq_id);
 	}
 }
 
@@ -3261,7 +3277,7 @@ static void qlge_resolve_queues_to_irqs(struct qlge_adapter *qdev)
 		 * vectors for each queue.
 		 */
 		for (i = 0; i < qdev->intr_count; i++, intr_context++) {
-			qdev->rx_ring[i].irq = i;
+			qdev->cq[i].irq = i;
 			intr_context->intr = i;
 			intr_context->qdev = qdev;
 			/* Set up this vector's bit-mask that indicates
@@ -3357,9 +3373,9 @@ static void qlge_free_irq(struct qlge_adapter *qdev)
 		if (intr_context->hooked) {
 			if (test_bit(QL_MSIX_ENABLED, &qdev->flags)) {
 				free_irq(qdev->msi_x_entry[i].vector,
-					 &qdev->rx_ring[i]);
+					 &qdev->cq[i]);
 			} else {
-				free_irq(qdev->pdev->irq, &qdev->rx_ring[0]);
+				free_irq(qdev->pdev->irq, &qdev->cq[0]);
 			}
 		}
 	}
@@ -3381,7 +3397,7 @@ static int qlge_request_irq(struct qlge_adapter *qdev)
 					     intr_context->handler,
 					     0,
 					     intr_context->name,
-					     &qdev->rx_ring[i]);
+					     &qdev->cq[i]);
 			if (status) {
 				netif_err(qdev, ifup, qdev->ndev,
 					  "Failed request for MSIX interrupt %d.\n",
@@ -3398,13 +3414,13 @@ static int qlge_request_irq(struct qlge_adapter *qdev)
 				     intr_context->name);
 			netif_printk(qdev, ifup, KERN_DEBUG, qdev->ndev,
 				     "%s: dev_id = 0x%p.\n", __func__,
-				     &qdev->rx_ring[0]);
+				     &qdev->cq[0]);
 			status =
 				request_irq(pdev->irq, qlge_isr,
 					    test_bit(QL_MSI_ENABLED, &qdev->flags)
 					    ? 0
 					    : IRQF_SHARED,
-					    intr_context->name, &qdev->rx_ring[0]);
+					    intr_context->name, &qdev->cq[0]);
 			if (status)
 				goto err_irq;
 
@@ -3620,8 +3636,8 @@ static int qlge_adapter_initialize(struct qlge_adapter *qdev)
 		qdev->wol = WAKE_MAGIC;
 
 	/* Start up the rx queues. */
-	for (i = 0; i < qdev->rx_ring_count; i++) {
-		status = qlge_start_rx_ring(qdev, &qdev->rx_ring[i]);
+	for (i = 0; i < qdev->cq_count; i++) {
+		status = qlge_start_cq(qdev, &qdev->cq[i]);
 		if (status) {
 			netif_err(qdev, ifup, qdev->ndev,
 				  "Failed to start rx ring[%d].\n", i);
@@ -3916,10 +3932,11 @@ static void qlge_set_lb_size(struct qlge_adapter *qdev)
 
 static int qlge_configure_rings(struct qlge_adapter *qdev)
 {
-	int i;
+	int cpu_cnt = min_t(int, MAX_CPUS, num_online_cpus());
 	struct qlge_rx_ring *rx_ring;
 	struct qlge_tx_ring *tx_ring;
-	int cpu_cnt = min_t(int, MAX_CPUS, num_online_cpus());
+	struct qlge_cq *cq;
+	int i;
 
 	/* In a perfect world we have one RSS ring for each CPU
 	 * and each has it's own vector.  To do that we ask for
@@ -3933,7 +3950,7 @@ static int qlge_configure_rings(struct qlge_adapter *qdev)
 	/* Adjust the RSS ring count to the actual vector count. */
 	qdev->rss_ring_count = qdev->intr_count;
 	qdev->tx_ring_count = cpu_cnt;
-	qdev->rx_ring_count = qdev->tx_ring_count + qdev->rss_ring_count;
+	qdev->cq_count = qdev->tx_ring_count + qdev->rss_ring_count;
 
 	for (i = 0; i < qdev->tx_ring_count; i++) {
 		tx_ring = &qdev->tx_ring[i];
@@ -3951,31 +3968,35 @@ static int qlge_configure_rings(struct qlge_adapter *qdev)
 		tx_ring->cq_id = qdev->rss_ring_count + i;
 	}
 
-	for (i = 0; i < qdev->rx_ring_count; i++) {
-		rx_ring = &qdev->rx_ring[i];
-		memset((void *)rx_ring, 0, sizeof(*rx_ring));
-		rx_ring->qdev = qdev;
-		rx_ring->cq_id = i;
-		rx_ring->cpu = i % cpu_cnt;	/* CPU to run handler on. */
+	for (i = 0; i < qdev->cq_count; i++) {
+		cq = &qdev->cq[i];
+		memset((void *)cq, 0, sizeof(*cq));
+		cq->qdev = qdev;
+		cq->cq_id = i;
+		cq->cpu = i % cpu_cnt;	/* CPU to run handler on. */
 		if (i < qdev->rss_ring_count) {
 			/*
 			 * Inbound (RSS) queues.
 			 */
-			rx_ring->cq_len = qdev->rx_ring_size;
-			rx_ring->cq_size =
-				rx_ring->cq_len * sizeof(struct qlge_net_rsp_iocb);
+			cq->cq_len = qdev->rx_ring_size;
+			cq->cq_size =
+				cq->cq_len * sizeof(struct qlge_net_rsp_iocb);
+			rx_ring = &qdev->rx_ring[i];
+			memset((void *)rx_ring, 0, sizeof(*rx_ring));
 			rx_ring->lbq.type = QLGE_LB;
+			rx_ring->cq_id = i;
 			rx_ring->sbq.type = QLGE_SB;
 			INIT_DELAYED_WORK(&rx_ring->refill_work,
 					  &qlge_slow_refill);
+			rx_ring->qdev = qdev;
 		} else {
 			/*
 			 * Outbound queue handles outbound completions only.
 			 */
 			/* outbound cq is same size as tx_ring it services. */
-			rx_ring->cq_len = qdev->tx_ring_size;
-			rx_ring->cq_size =
-				rx_ring->cq_len * sizeof(struct qlge_net_rsp_iocb);
+			cq->cq_len = qdev->tx_ring_size;
+			cq->cq_size =
+				cq->cq_len * sizeof(struct qlge_net_rsp_iocb);
 		}
 	}
 	return 0;
@@ -4648,9 +4669,9 @@ netdev_tx_t qlge_lb_send(struct sk_buff *skb, struct net_device *ndev)
 	return qlge_send(skb, ndev);
 }
 
-int qlge_clean_lb_rx_ring(struct qlge_rx_ring *rx_ring, int budget)
+int qlge_clean_lb_cq(struct qlge_cq *cq, int budget)
 {
-	return qlge_clean_inbound_rx_ring(rx_ring, budget);
+	return qlge_clean_inbound_cq(cq, budget);
 }
 
 static void qlge_remove(struct pci_dev *pdev)
-- 
2.32.0

[RFC 06/19] staging: qlge: disable flow control by default

[Coiby Xu]

According to the TODO item,
> * the flow control implementation in firmware is buggy (sends a flood of pause
>   frames, resets the link, device and driver buffer queues become
>   desynchronized), disable it by default

Currently, qlge_mpi_port_cfg_work calls qlge_mb_get_port_cfg which gets
the link config from the firmware and saves it to qdev->link_config. By
default, flow control is enabled. This commit writes the
save the pause parameter of qdev->link_config and don't let it
overwritten by link settings of current port. Since qdev->link_config=0
when qdev is initialized, this could disable flow control by default and
the pause parameter value could also survive MPI resetting,
    $ ethtool -a enp94s0f0
    Pause parameters for enp94s0f0:
    Autonegotiate:  off
    RX:             off
    TX:             off

The follow control can be enabled manually,

    $ ethtool -A enp94s0f0 rx on tx on
    $ ethtool -a enp94s0f0
    Pause parameters for enp94s0f0:
    Autonegotiate:  off
    RX:             on
    TX:             on

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO       |  3 ---
 drivers/staging/qlge/qlge_mpi.c | 11 ++++++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index b7a60425fcd2..8c84160b5993 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,9 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* the flow control implementation in firmware is buggy (sends a flood of pause
-  frames, resets the link, device and driver buffer queues become
-  desynchronized), disable it by default
 * some structures are initialized redundantly (ex. memset 0 after
   alloc_etherdev())
 * the driver has a habit of using runtime checks where compile time checks are
diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 2630ebf50341..0f1c7da80413 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c
@@ -806,6 +806,7 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
 {
 	struct mbox_params mbc;
 	struct mbox_params *mbcp = &mbc;
+	u32 saved_pause_link_config = 0;
 	int status = 0;
 
 	memset(mbcp, 0, sizeof(struct mbox_params));
@@ -826,7 +827,15 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
 	} else	{
 		netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
 			     "Passed Get Port Configuration.\n");
-		qdev->link_config = mbcp->mbox_out[1];
+		/*
+		 * Don't let the pause parameter be overwritten by
+		 *
+		 * In this way, follow control can be disabled by default
+		 * and the setting could also survive the MPI reset
+		 */
+		saved_pause_link_config = qdev->link_config & CFG_PAUSE_STD;
+		qdev->link_config = ~CFG_PAUSE_STD & mbcp->mbox_out[1];
+		qdev->link_config |= saved_pause_link_config;
 		qdev->max_frame_size = mbcp->mbox_out[2];
 	}
 	return status;
-- 
2.32.0

Re: [RFC 06/19] staging: qlge: disable flow control by default

[Benjamin Poirier]

On 2021-06-21 21:48 +0800, Coiby Xu wrote:
> According to the TODO item,
> > * the flow control implementation in firmware is buggy (sends a flood of pause
> >   frames, resets the link, device and driver buffer queues become
> >   desynchronized), disable it by default
> 
> Currently, qlge_mpi_port_cfg_work calls qlge_mb_get_port_cfg which gets
> the link config from the firmware and saves it to qdev->link_config. By
> default, flow control is enabled. This commit writes the
> save the pause parameter of qdev->link_config and don't let it
> overwritten by link settings of current port. Since qdev->link_config=0
> when qdev is initialized, this could disable flow control by default and
> the pause parameter value could also survive MPI resetting,
>     $ ethtool -a enp94s0f0
>     Pause parameters for enp94s0f0:
>     Autonegotiate:  off
>     RX:             off
>     TX:             off
> 
> The follow control can be enabled manually,
> 
>     $ ethtool -A enp94s0f0 rx on tx on
>     $ ethtool -a enp94s0f0
>     Pause parameters for enp94s0f0:
>     Autonegotiate:  off
>     RX:             on
>     TX:             on
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/TODO       |  3 ---
>  drivers/staging/qlge/qlge_mpi.c | 11 ++++++++++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
> index b7a60425fcd2..8c84160b5993 100644
> --- a/drivers/staging/qlge/TODO
> +++ b/drivers/staging/qlge/TODO
> @@ -4,9 +4,6 @@
>    ql_build_rx_skb(). That function is now used exclusively to handle packets
>    that underwent header splitting but it still contains code to handle non
>    split cases.
> -* the flow control implementation in firmware is buggy (sends a flood of pause
> -  frames, resets the link, device and driver buffer queues become
> -  desynchronized), disable it by default
>  * some structures are initialized redundantly (ex. memset 0 after
>    alloc_etherdev())
>  * the driver has a habit of using runtime checks where compile time checks are
> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> index 2630ebf50341..0f1c7da80413 100644
> --- a/drivers/staging/qlge/qlge_mpi.c
> +++ b/drivers/staging/qlge/qlge_mpi.c
> @@ -806,6 +806,7 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>  {
>  	struct mbox_params mbc;
>  	struct mbox_params *mbcp = &mbc;
> +	u32 saved_pause_link_config = 0;

Initialization is not needed given the code below, in fact the
declaration can be moved to the block below.

>  	int status = 0;
>  
>  	memset(mbcp, 0, sizeof(struct mbox_params));
> @@ -826,7 +827,15 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>  	} else	{
>  		netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
>  			     "Passed Get Port Configuration.\n");
> -		qdev->link_config = mbcp->mbox_out[1];
> +		/*
> +		 * Don't let the pause parameter be overwritten by
> +		 *
> +		 * In this way, follow control can be disabled by default
> +		 * and the setting could also survive the MPI reset
> +		 */

It seems this comment is incomplete. Also, it's "flow control", not
"follow control".

> +		saved_pause_link_config = qdev->link_config & CFG_PAUSE_STD;
> +		qdev->link_config = ~CFG_PAUSE_STD & mbcp->mbox_out[1];
> +		qdev->link_config |= saved_pause_link_config;
>  		qdev->max_frame_size = mbcp->mbox_out[2];
>  	}
>  	return status;
> -- 
> 2.32.0
> 

Re: [RFC 06/19] staging: qlge: disable flow control by default

[Benjamin Poirier]

On 2021-06-21 21:48 +0800, Coiby Xu wrote:
> According to the TODO item,
> > * the flow control implementation in firmware is buggy (sends a flood of pause
> >   frames, resets the link, device and driver buffer queues become
> >   desynchronized), disable it by default
> 
> Currently, qlge_mpi_port_cfg_work calls qlge_mb_get_port_cfg which gets
> the link config from the firmware and saves it to qdev->link_config. By
> default, flow control is enabled. This commit writes the
> save the pause parameter of qdev->link_config and don't let it
> overwritten by link settings of current port. Since qdev->link_config=0
> when qdev is initialized, this could disable flow control by default and
> the pause parameter value could also survive MPI resetting,
>     $ ethtool -a enp94s0f0
>     Pause parameters for enp94s0f0:
>     Autonegotiate:  off
>     RX:             off
>     TX:             off
> 
> The follow control can be enabled manually,
> 
>     $ ethtool -A enp94s0f0 rx on tx on
>     $ ethtool -a enp94s0f0
>     Pause parameters for enp94s0f0:
>     Autonegotiate:  off
>     RX:             on
>     TX:             on
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/TODO       |  3 ---
>  drivers/staging/qlge/qlge_mpi.c | 11 ++++++++++-
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
> index b7a60425fcd2..8c84160b5993 100644
> --- a/drivers/staging/qlge/TODO
> +++ b/drivers/staging/qlge/TODO
> @@ -4,9 +4,6 @@
>    ql_build_rx_skb(). That function is now used exclusively to handle packets
>    that underwent header splitting but it still contains code to handle non
>    split cases.
> -* the flow control implementation in firmware is buggy (sends a flood of pause
> -  frames, resets the link, device and driver buffer queues become
> -  desynchronized), disable it by default
>  * some structures are initialized redundantly (ex. memset 0 after
>    alloc_etherdev())
>  * the driver has a habit of using runtime checks where compile time checks are
> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
> index 2630ebf50341..0f1c7da80413 100644
> --- a/drivers/staging/qlge/qlge_mpi.c
> +++ b/drivers/staging/qlge/qlge_mpi.c
> @@ -806,6 +806,7 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>  {
>  	struct mbox_params mbc;
>  	struct mbox_params *mbcp = &mbc;
> +	u32 saved_pause_link_config = 0;

Initialization is not needed given the code below, in fact the
declaration can be moved to the block below.

>  	int status = 0;
>  
>  	memset(mbcp, 0, sizeof(struct mbox_params));
> @@ -826,7 +827,15 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>  	} else	{
>  		netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
>  			     "Passed Get Port Configuration.\n");
> -		qdev->link_config = mbcp->mbox_out[1];
> +		/*
> +		 * Don't let the pause parameter be overwritten by
> +		 *
> +		 * In this way, follow control can be disabled by default
> +		 * and the setting could also survive the MPI reset
> +		 */

It seems this comment is incomplete. Also, it's "flow control", not
"follow control".

> +		saved_pause_link_config = qdev->link_config & CFG_PAUSE_STD;
> +		qdev->link_config = ~CFG_PAUSE_STD & mbcp->mbox_out[1];
> +		qdev->link_config |= saved_pause_link_config;
>  		qdev->max_frame_size = mbcp->mbox_out[2];
>  	}
>  	return status;
> -- 
> 2.32.0
> 

Re: [RFC 06/19] staging: qlge: disable flow control by default

[Coiby Xu]

On Tue, Jun 22, 2021 at 04:49:51PM +0900, Benjamin Poirier wrote:
>On 2021-06-21 21:48 +0800, Coiby Xu wrote:
>> According to the TODO item,
>> > * the flow control implementation in firmware is buggy (sends a flood of pause
>> >   frames, resets the link, device and driver buffer queues become
>> >   desynchronized), disable it by default
>>
>> Currently, qlge_mpi_port_cfg_work calls qlge_mb_get_port_cfg which gets
>> the link config from the firmware and saves it to qdev->link_config. By
>> default, flow control is enabled. This commit writes the
>> save the pause parameter of qdev->link_config and don't let it
>> overwritten by link settings of current port. Since qdev->link_config=0
>> when qdev is initialized, this could disable flow control by default and
>> the pause parameter value could also survive MPI resetting,
>>     $ ethtool -a enp94s0f0
>>     Pause parameters for enp94s0f0:
>>     Autonegotiate:  off
>>     RX:             off
>>     TX:             off
>>
>> The follow control can be enabled manually,
>>
>>     $ ethtool -A enp94s0f0 rx on tx on
>>     $ ethtool -a enp94s0f0
>>     Pause parameters for enp94s0f0:
>>     Autonegotiate:  off
>>     RX:             on
>>     TX:             on
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/TODO       |  3 ---
>>  drivers/staging/qlge/qlge_mpi.c | 11 ++++++++++-
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
>> index b7a60425fcd2..8c84160b5993 100644
>> --- a/drivers/staging/qlge/TODO
>> +++ b/drivers/staging/qlge/TODO
>> @@ -4,9 +4,6 @@
>>    ql_build_rx_skb(). That function is now used exclusively to handle packets
>>    that underwent header splitting but it still contains code to handle non
>>    split cases.
>> -* the flow control implementation in firmware is buggy (sends a flood of pause
>> -  frames, resets the link, device and driver buffer queues become
>> -  desynchronized), disable it by default
>>  * some structures are initialized redundantly (ex. memset 0 after
>>    alloc_etherdev())
>>  * the driver has a habit of using runtime checks where compile time checks are
>> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
>> index 2630ebf50341..0f1c7da80413 100644
>> --- a/drivers/staging/qlge/qlge_mpi.c
>> +++ b/drivers/staging/qlge/qlge_mpi.c
>> @@ -806,6 +806,7 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>>  {
>>  	struct mbox_params mbc;
>>  	struct mbox_params *mbcp = &mbc;
>> +	u32 saved_pause_link_config = 0;
>
>Initialization is not needed given the code below, 

Thanks for the spotting this issue!

> in fact the
>declaration can be moved to the block below.

I thought I need to put the declaration in the beginning of the
function. But it seems Linux kernel coding style doesn't require it.
I'll move it to the else block below then.

>
>>  	int status = 0;
>>
>>  	memset(mbcp, 0, sizeof(struct mbox_params));
>> @@ -826,7 +827,15 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>>  	} else	{
>>  		netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
>>  			     "Passed Get Port Configuration.\n");
>> -		qdev->link_config = mbcp->mbox_out[1];
>> +		/*
>> +		 * Don't let the pause parameter be overwritten by
>> +		 *
>> +		 * In this way, follow control can be disabled by default
>> +		 * and the setting could also survive the MPI reset
>> +		 */
>
>It seems this comment is incomplete. Also, it's "flow control", not
>"follow control".

Ah, yes. I should state it as "Don't let the pause parameter be 
overwritten by be overwritten by the firmware.". And thanks for
correcting the typo.
>
>> +		saved_pause_link_config = qdev->link_config & CFG_PAUSE_STD;
>> +		qdev->link_config = ~CFG_PAUSE_STD & mbcp->mbox_out[1];
>> +		qdev->link_config |= saved_pause_link_config;
>>  		qdev->max_frame_size = mbcp->mbox_out[2];
>>  	}
>>  	return status;
>> --
>> 2.32.0
>>

-- 
Best regards,
Coiby

Re: [RFC 06/19] staging: qlge: disable flow control by default

[Coiby Xu]

On Tue, Jun 22, 2021 at 04:49:51PM +0900, Benjamin Poirier wrote:
>On 2021-06-21 21:48 +0800, Coiby Xu wrote:
>> According to the TODO item,
>> > * the flow control implementation in firmware is buggy (sends a flood of pause
>> >   frames, resets the link, device and driver buffer queues become
>> >   desynchronized), disable it by default
>>
>> Currently, qlge_mpi_port_cfg_work calls qlge_mb_get_port_cfg which gets
>> the link config from the firmware and saves it to qdev->link_config. By
>> default, flow control is enabled. This commit writes the
>> save the pause parameter of qdev->link_config and don't let it
>> overwritten by link settings of current port. Since qdev->link_config=0
>> when qdev is initialized, this could disable flow control by default and
>> the pause parameter value could also survive MPI resetting,
>>     $ ethtool -a enp94s0f0
>>     Pause parameters for enp94s0f0:
>>     Autonegotiate:  off
>>     RX:             off
>>     TX:             off
>>
>> The follow control can be enabled manually,
>>
>>     $ ethtool -A enp94s0f0 rx on tx on
>>     $ ethtool -a enp94s0f0
>>     Pause parameters for enp94s0f0:
>>     Autonegotiate:  off
>>     RX:             on
>>     TX:             on
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/TODO       |  3 ---
>>  drivers/staging/qlge/qlge_mpi.c | 11 ++++++++++-
>>  2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
>> index b7a60425fcd2..8c84160b5993 100644
>> --- a/drivers/staging/qlge/TODO
>> +++ b/drivers/staging/qlge/TODO
>> @@ -4,9 +4,6 @@
>>    ql_build_rx_skb(). That function is now used exclusively to handle packets
>>    that underwent header splitting but it still contains code to handle non
>>    split cases.
>> -* the flow control implementation in firmware is buggy (sends a flood of pause
>> -  frames, resets the link, device and driver buffer queues become
>> -  desynchronized), disable it by default
>>  * some structures are initialized redundantly (ex. memset 0 after
>>    alloc_etherdev())
>>  * the driver has a habit of using runtime checks where compile time checks are
>> diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
>> index 2630ebf50341..0f1c7da80413 100644
>> --- a/drivers/staging/qlge/qlge_mpi.c
>> +++ b/drivers/staging/qlge/qlge_mpi.c
>> @@ -806,6 +806,7 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>>  {
>>  	struct mbox_params mbc;
>>  	struct mbox_params *mbcp = &mbc;
>> +	u32 saved_pause_link_config = 0;
>
>Initialization is not needed given the code below, 

Thanks for the spotting this issue!

> in fact the
>declaration can be moved to the block below.

I thought I need to put the declaration in the beginning of the
function. But it seems Linux kernel coding style doesn't require it.
I'll move it to the else block below then.

>
>>  	int status = 0;
>>
>>  	memset(mbcp, 0, sizeof(struct mbox_params));
>> @@ -826,7 +827,15 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
>>  	} else	{
>>  		netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
>>  			     "Passed Get Port Configuration.\n");
>> -		qdev->link_config = mbcp->mbox_out[1];
>> +		/*
>> +		 * Don't let the pause parameter be overwritten by
>> +		 *
>> +		 * In this way, follow control can be disabled by default
>> +		 * and the setting could also survive the MPI reset
>> +		 */
>
>It seems this comment is incomplete. Also, it's "flow control", not
>"follow control".

Ah, yes. I should state it as "Don't let the pause parameter be 
overwritten by be overwritten by the firmware.". And thanks for
correcting the typo.
>
>> +		saved_pause_link_config = qdev->link_config & CFG_PAUSE_STD;
>> +		qdev->link_config = ~CFG_PAUSE_STD & mbcp->mbox_out[1];
>> +		qdev->link_config |= saved_pause_link_config;
>>  		qdev->max_frame_size = mbcp->mbox_out[2];
>>  	}
>>  	return status;
>> --
>> 2.32.0
>>

-- 
Best regards,
Coiby

[RFC 07/19] staging: qlge: remove the TODO item of unnecessary memset 0

[Coiby Xu]

commit 953b94009377419f28fd0153f91fcd5b5a347608 ("staging: qlge:
Initialize devlink health dump framework") removed the unnecessary
memset 0 after alloc_etherdev_mq. Delete this TODO item.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 8c84160b5993..cc5f8cf7608d 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -4,8 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* some structures are initialized redundantly (ex. memset 0 after
-  alloc_etherdev())
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
 * reorder struct members to avoid holes if it doesn't impact performance
-- 
2.32.0

[RFC 08/19] staging: qlge: reorder members of qlge_adapter for optimization

[Coiby Xu]

Before reordering, pahole shows,
    /* size: 21168, cachelines: 331, members: 69 */
    /* sum members: 21144, holes: 4, sum holes: 18 */
    /* padding: 6 */
    /* paddings: 6, sum paddings: 24 */
    /* forced alignments: 1 */
    /* last cacheline: 48 bytes */

After reordering following pahole's suggestion,
    /* size: 21152, cachelines: 331, members: 69 */
    /* sum members: 21144, holes: 1, sum holes: 2 */
    /* padding: 6 */
    /* paddings: 6, sum paddings: 24 */
    /* forced alignments: 1 */
    /* last cacheline: 32 bytes */

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 926af25b14fa..9177baa9f022 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -2081,8 +2081,8 @@ struct qlge_adapter *netdev_to_qdev(struct net_device *ndev)
  */
 struct qlge_adapter {
 	struct ricb ricb;
-	unsigned long flags;
 	u32 wol;
+	unsigned long flags;
 
 	struct nic_stats nic_stats;
 
@@ -2103,6 +2103,8 @@ struct qlge_adapter {
 	spinlock_t adapter_lock;
 	spinlock_t stats_lock;
 
+	u32 intr_count;
+
 	/* PCI Bus Relative Register Addresses */
 	void __iomem *reg_base;
 	void __iomem *doorbell_area;
@@ -2123,7 +2125,6 @@ struct qlge_adapter {
 
 	int tx_ring_size;
 	int rx_ring_size;
-	u32 intr_count;
 	struct msix_entry *msi_x_entry;
 	struct intr_context intr_context[MAX_RX_RINGS];
 
@@ -2162,6 +2163,7 @@ struct qlge_adapter {
 	u32 max_frame_size;
 
 	union flash_params flash;
+	u16 device_id;
 
 	struct workqueue_struct *workqueue;
 	struct delayed_work asic_reset_work;
@@ -2171,7 +2173,6 @@ struct qlge_adapter {
 	struct delayed_work mpi_idc_work;
 	struct completion ide_completion;
 	const struct nic_operations *nic_ops;
-	u16 device_id;
 	struct timer_list timer;
 	atomic_t lb_count;
 	/* Keep local copy of current mac address. */
-- 
2.32.0

[RFC 09/19] staging: qlge: remove the TODO item of reorder struct

[Coiby Xu]

struct qlge_cq has one hole, but for the sake of readability (irq is one
of the Misc. handler elements, don't move it from after 'irq' to after
'cnsmr_idx' ), keep it untouched. Then, there is no struct that need
reordering according to pahole.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index cc5f8cf7608d..2c4cc586a4bf 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -6,7 +6,6 @@
   split cases.
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
-* reorder struct members to avoid holes if it doesn't impact performance
 * avoid legacy/deprecated apis (ex. replace pci_dma_*, replace pci_enable_msi,
   use pci_iomap)
 * some "while" loops could be rewritten with simple "for", ex.
-- 
2.32.0

[RFC 10/19] staging: qlge: remove the TODO item of avoid legacy/deprecated apis

[Coiby Xu]

The following commits have finished the job,
- commit e955a071b9b3e6b634b7ceda64025bfbd6529dcc  ("staging: qlge:
  replace deprecated apis pci_dma_*")
- commit 50b483a1457abd6fe27117f0507297e107ef42b2 ("qlge: Use
  pci_enable_msix_range() instead of pci_enable_msix()")

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 2c4cc586a4bf..8bb6779a5bb4 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -6,8 +6,6 @@
   split cases.
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
-* avoid legacy/deprecated apis (ex. replace pci_dma_*, replace pci_enable_msi,
-  use pci_iomap)
 * some "while" loops could be rewritten with simple "for", ex.
   ql_wait_reg_rdy(), ql_start_rx_ring())
 * remove duplicate and useless comments
-- 
2.32.0

[RFC 11/19] staging: qlge: the number of pages to contain a buffer queue is constant

[Coiby Xu]

This patch is extented work of commit ec705b983b46b8e2d3cafd40c188458bf4241f11
("staging: qlge: Remove qlge_bq.len & size"). Since the same len is used
for both sbq (small buffer queue) and lbq (large buffer queue), the
number of pages to contain a buffer queue is also known at compile time.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge.h      | 13 ++++++-------
 drivers/staging/qlge/qlge_main.c |  8 ++++----
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/qlge/qlge.h b/drivers/staging/qlge/qlge.h
index 9177baa9f022..32755b0e2fb7 100644
--- a/drivers/staging/qlge/qlge.h
+++ b/drivers/staging/qlge/qlge.h
@@ -42,16 +42,15 @@
 
 #define DB_PAGE_SIZE 4096
 
-/* Calculate the number of (4k) pages required to
- * contain a buffer queue of the given length.
+/*
+ * The number of (4k) pages required to contain a buffer queue.
  */
-#define MAX_DB_PAGES_PER_BQ(x) \
-		(((x * sizeof(u64)) / DB_PAGE_SIZE) + \
-		(((x * sizeof(u64)) % DB_PAGE_SIZE) ? 1 : 0))
+#define MAX_DB_PAGES_PER_BQ \
+		(((QLGE_BQ_LEN * sizeof(u64)) / DB_PAGE_SIZE) + \
+		(((QLGE_BQ_LEN * sizeof(u64)) % DB_PAGE_SIZE) ? 1 : 0))
 
 #define RX_RING_SHADOW_SPACE	(sizeof(u64) + \
-		MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64) + \
-		MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN) * sizeof(u64))
+		MAX_DB_PAGES_PER_BQ * sizeof(u64) * 2)
 #define LARGE_BUFFER_MAX_SIZE 4096
 #define LARGE_BUFFER_MIN_SIZE 2048
 
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 94853b182608..7aee9e904097 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3015,8 +3015,8 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
 		shadow_reg_dma += sizeof(u64);
 		rx_ring->lbq.base_indirect = shadow_reg;
 		rx_ring->lbq.base_indirect_dma = shadow_reg_dma;
-		shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
-		shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		shadow_reg += (sizeof(u64) * MAX_DB_PAGES_PER_BQ);
+		shadow_reg_dma += (sizeof(u64) * MAX_DB_PAGES_PER_BQ);
 		rx_ring->sbq.base_indirect = shadow_reg;
 		rx_ring->sbq.base_indirect_dma = shadow_reg_dma;
 		/* PCI doorbell mem area + 0x18 for large buffer consumer */
@@ -3034,7 +3034,7 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
 			tmp += DB_PAGE_SIZE;
 			base_indirect_ptr++;
 			page_entries++;
-		} while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		} while (page_entries < MAX_DB_PAGES_PER_BQ);
 		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
 		cqicb->lbq_buf_size =
 			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
@@ -3051,7 +3051,7 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
 			tmp += DB_PAGE_SIZE;
 			base_indirect_ptr++;
 			page_entries++;
-		} while (page_entries < MAX_DB_PAGES_PER_BQ(QLGE_BQ_LEN));
+		} while (page_entries < MAX_DB_PAGES_PER_BQ);
 		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
 		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
 		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
-- 
2.32.0

[RFC 12/19] staging: qlge: rewrite do while loops as for loops in qlge_start_rx_ring

[Coiby Xu]

Since MAX_DB_PAGES_PER_BQ > 0, the for loop is equivalent to do while
loop.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_main.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 7aee9e904097..c5e161595b1f 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -3029,12 +3029,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
 		tmp = (u64)rx_ring->lbq.base_dma;
 		base_indirect_ptr = rx_ring->lbq.base_indirect;
 		page_entries = 0;
-		do {
+		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
 			*base_indirect_ptr = cpu_to_le64(tmp);
 			tmp += DB_PAGE_SIZE;
 			base_indirect_ptr++;
-			page_entries++;
-		} while (page_entries < MAX_DB_PAGES_PER_BQ);
+		}
 		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
 		cqicb->lbq_buf_size =
 			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
@@ -3046,12 +3045,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
 		tmp = (u64)rx_ring->sbq.base_dma;
 		base_indirect_ptr = rx_ring->sbq.base_indirect;
 		page_entries = 0;
-		do {
+		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
 			*base_indirect_ptr = cpu_to_le64(tmp);
 			tmp += DB_PAGE_SIZE;
 			base_indirect_ptr++;
-			page_entries++;
-		} while (page_entries < MAX_DB_PAGES_PER_BQ);
+		}
 		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
 		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
 		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
-- 
2.32.0

Re: [RFC 12/19] staging: qlge: rewrite do while loops as for loops in qlge_start_rx_ring

[Benjamin Poirier]

On 2021-06-21 21:48 +0800, Coiby Xu wrote:
> Since MAX_DB_PAGES_PER_BQ > 0, the for loop is equivalent to do while
> loop.
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 7aee9e904097..c5e161595b1f 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -3029,12 +3029,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>  		tmp = (u64)rx_ring->lbq.base_dma;
>  		base_indirect_ptr = rx_ring->lbq.base_indirect;
>  		page_entries = 0;

This initialization can be removed now. Same thing below.

> -		do {
> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>  			*base_indirect_ptr = cpu_to_le64(tmp);
>  			tmp += DB_PAGE_SIZE;
>  			base_indirect_ptr++;
> -			page_entries++;
> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
> +		}
>  		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
>  		cqicb->lbq_buf_size =
>  			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
> @@ -3046,12 +3045,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>  		tmp = (u64)rx_ring->sbq.base_dma;
>  		base_indirect_ptr = rx_ring->sbq.base_indirect;
>  		page_entries = 0;
> -		do {
> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>  			*base_indirect_ptr = cpu_to_le64(tmp);
>  			tmp += DB_PAGE_SIZE;
>  			base_indirect_ptr++;
> -			page_entries++;
> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
> +		}
>  		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
>  		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
>  		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
> -- 
> 2.32.0
> 

Re: [RFC 12/19] staging: qlge: rewrite do while loops as for loops in qlge_start_rx_ring

[Benjamin Poirier]

On 2021-06-21 21:48 +0800, Coiby Xu wrote:
> Since MAX_DB_PAGES_PER_BQ > 0, the for loop is equivalent to do while
> loop.
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index 7aee9e904097..c5e161595b1f 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -3029,12 +3029,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>  		tmp = (u64)rx_ring->lbq.base_dma;
>  		base_indirect_ptr = rx_ring->lbq.base_indirect;
>  		page_entries = 0;

This initialization can be removed now. Same thing below.

> -		do {
> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>  			*base_indirect_ptr = cpu_to_le64(tmp);
>  			tmp += DB_PAGE_SIZE;
>  			base_indirect_ptr++;
> -			page_entries++;
> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
> +		}
>  		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
>  		cqicb->lbq_buf_size =
>  			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
> @@ -3046,12 +3045,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>  		tmp = (u64)rx_ring->sbq.base_dma;
>  		base_indirect_ptr = rx_ring->sbq.base_indirect;
>  		page_entries = 0;
> -		do {
> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>  			*base_indirect_ptr = cpu_to_le64(tmp);
>  			tmp += DB_PAGE_SIZE;
>  			base_indirect_ptr++;
> -			page_entries++;
> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
> +		}
>  		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
>  		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
>  		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
> -- 
> 2.32.0
> 

Re: [RFC 12/19] staging: qlge: rewrite do while loops as for loops in qlge_start_rx_ring

[Coiby Xu]

On Tue, Jun 22, 2021 at 04:45:22PM +0900, Benjamin Poirier wrote:
>On 2021-06-21 21:48 +0800, Coiby Xu wrote:
>> Since MAX_DB_PAGES_PER_BQ > 0, the for loop is equivalent to do while
>> loop.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/qlge_main.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> index 7aee9e904097..c5e161595b1f 100644
>> --- a/drivers/staging/qlge/qlge_main.c
>> +++ b/drivers/staging/qlge/qlge_main.c
>> @@ -3029,12 +3029,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>>  		tmp = (u64)rx_ring->lbq.base_dma;
>>  		base_indirect_ptr = rx_ring->lbq.base_indirect;
>>  		page_entries = 0;
>
>This initialization can be removed now. Same thing below.

Yes, thanks for the suggestion!

>
>> -		do {
>> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>>  			*base_indirect_ptr = cpu_to_le64(tmp);
>>  			tmp += DB_PAGE_SIZE;
>>  			base_indirect_ptr++;
>> -			page_entries++;
>> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
>> +		}
>>  		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
>>  		cqicb->lbq_buf_size =
>>  			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
>> @@ -3046,12 +3045,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>>  		tmp = (u64)rx_ring->sbq.base_dma;
>>  		base_indirect_ptr = rx_ring->sbq.base_indirect;
>>  		page_entries = 0;
>> -		do {
>> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>>  			*base_indirect_ptr = cpu_to_le64(tmp);
>>  			tmp += DB_PAGE_SIZE;
>>  			base_indirect_ptr++;
>> -			page_entries++;
>> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
>> +		}
>>  		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
>>  		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
>>  		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
>> --
>> 2.32.0
>>

-- 
Best regards,
Coiby

Re: [RFC 12/19] staging: qlge: rewrite do while loops as for loops in qlge_start_rx_ring

[Coiby Xu]

On Tue, Jun 22, 2021 at 04:45:22PM +0900, Benjamin Poirier wrote:
>On 2021-06-21 21:48 +0800, Coiby Xu wrote:
>> Since MAX_DB_PAGES_PER_BQ > 0, the for loop is equivalent to do while
>> loop.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/qlge_main.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> index 7aee9e904097..c5e161595b1f 100644
>> --- a/drivers/staging/qlge/qlge_main.c
>> +++ b/drivers/staging/qlge/qlge_main.c
>> @@ -3029,12 +3029,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>>  		tmp = (u64)rx_ring->lbq.base_dma;
>>  		base_indirect_ptr = rx_ring->lbq.base_indirect;
>>  		page_entries = 0;
>
>This initialization can be removed now. Same thing below.

Yes, thanks for the suggestion!

>
>> -		do {
>> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>>  			*base_indirect_ptr = cpu_to_le64(tmp);
>>  			tmp += DB_PAGE_SIZE;
>>  			base_indirect_ptr++;
>> -			page_entries++;
>> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
>> +		}
>>  		cqicb->lbq_addr = cpu_to_le64(rx_ring->lbq.base_indirect_dma);
>>  		cqicb->lbq_buf_size =
>>  			cpu_to_le16(QLGE_FIT16(qdev->lbq_buf_size));
>> @@ -3046,12 +3045,11 @@ static int qlge_start_cq(struct qlge_adapter *qdev, struct qlge_cq *cq)
>>  		tmp = (u64)rx_ring->sbq.base_dma;
>>  		base_indirect_ptr = rx_ring->sbq.base_indirect;
>>  		page_entries = 0;
>> -		do {
>> +		for (page_entries = 0; page_entries < MAX_DB_PAGES_PER_BQ; page_entries++) {
>>  			*base_indirect_ptr = cpu_to_le64(tmp);
>>  			tmp += DB_PAGE_SIZE;
>>  			base_indirect_ptr++;
>> -			page_entries++;
>> -		} while (page_entries < MAX_DB_PAGES_PER_BQ);
>> +		}
>>  		cqicb->sbq_addr = cpu_to_le64(rx_ring->sbq.base_indirect_dma);
>>  		cqicb->sbq_buf_size = cpu_to_le16(QLGE_SMALL_BUFFER_SIZE);
>>  		cqicb->sbq_len = cpu_to_le16(QLGE_FIT16(QLGE_BQ_LEN));
>> --
>> 2.32.0
>>

-- 
Best regards,
Coiby

[RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

Since wait_count=30 > 0, the for loop is equivalent to do while
loop. This commit also replaces 100 with UDELAY_DELAY.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index c5e161595b1f..2d2405be38f5 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
 int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
 {
 	unsigned int wait_count = 30;
+	int count;
 
-	do {
+	for (count = 0; count < wait_count; count++) {
 		if (!qlge_sem_trylock(qdev, sem_mask))
 			return 0;
-		udelay(100);
-	} while (--wait_count);
+		udelay(UDELAY_DELAY);
+	}
 	return -ETIMEDOUT;
 }
 
-- 
2.32.0

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> Since wait_count=30 > 0, the for loop is equivalent to do while
> loop. This commit also replaces 100 with UDELAY_DELAY.
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index c5e161595b1f..2d2405be38f5 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>  {
>  	unsigned int wait_count = 30;
> +	int count;
>  
> -	do {
> +	for (count = 0; count < wait_count; count++) {
>  		if (!qlge_sem_trylock(qdev, sem_mask))
>  			return 0;
> -		udelay(100);
> -	} while (--wait_count);
> +		udelay(UDELAY_DELAY);

This is an interesting way to silence the checkpatch udelay warning.  ;)

regards,
dan carpenter

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> Since wait_count=30 > 0, the for loop is equivalent to do while
> loop. This commit also replaces 100 with UDELAY_DELAY.
> 
> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
> ---
>  drivers/staging/qlge/qlge_main.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> index c5e161595b1f..2d2405be38f5 100644
> --- a/drivers/staging/qlge/qlge_main.c
> +++ b/drivers/staging/qlge/qlge_main.c
> @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>  {
>  	unsigned int wait_count = 30;
> +	int count;
>  
> -	do {
> +	for (count = 0; count < wait_count; count++) {
>  		if (!qlge_sem_trylock(qdev, sem_mask))
>  			return 0;
> -		udelay(100);
> -	} while (--wait_count);
> +		udelay(UDELAY_DELAY);

This is an interesting way to silence the checkpatch udelay warning.  ;)

regards,
dan carpenter

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> Since wait_count=30 > 0, the for loop is equivalent to do while
>> loop. This commit also replaces 100 with UDELAY_DELAY.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/qlge_main.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> index c5e161595b1f..2d2405be38f5 100644
>> --- a/drivers/staging/qlge/qlge_main.c
>> +++ b/drivers/staging/qlge/qlge_main.c
>> @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>>  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>>  {
>>  	unsigned int wait_count = 30;
>> +	int count;
>>
>> -	do {
>> +	for (count = 0; count < wait_count; count++) {
>>  		if (!qlge_sem_trylock(qdev, sem_mask))
>>  			return 0;
>> -		udelay(100);
>> -	} while (--wait_count);
>> +		udelay(UDELAY_DELAY);
>
>This is an interesting way to silence the checkpatch udelay warning.  ;)

I didn't know this could silence the warning :)

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> Since wait_count=30 > 0, the for loop is equivalent to do while
>> loop. This commit also replaces 100 with UDELAY_DELAY.
>>
>> Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
>> ---
>>  drivers/staging/qlge/qlge_main.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> index c5e161595b1f..2d2405be38f5 100644
>> --- a/drivers/staging/qlge/qlge_main.c
>> +++ b/drivers/staging/qlge/qlge_main.c
>> @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>>  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>>  {
>>  	unsigned int wait_count = 30;
>> +	int count;
>>
>> -	do {
>> +	for (count = 0; count < wait_count; count++) {
>>  		if (!qlge_sem_trylock(qdev, sem_mask))
>>  			return 0;
>> -		udelay(100);
>> -	} while (--wait_count);
>> +		udelay(UDELAY_DELAY);
>
>This is an interesting way to silence the checkpatch udelay warning.  ;)

I didn't know this could silence the warning :)

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Joe Perches]

On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
> On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> > > Since wait_count=30 > 0, the for loop is equivalent to do while
> > > loop. This commit also replaces 100 with UDELAY_DELAY.
[]
> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
[]
> > > @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
> > >  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
> > >  {
> > >  	unsigned int wait_count = 30;
> > > +	int count;
> > > 
> > > -	do {
> > > +	for (count = 0; count < wait_count; count++) {
> > >  		if (!qlge_sem_trylock(qdev, sem_mask))
> > >  			return 0;
> > > -		udelay(100);
> > > -	} while (--wait_count);
> > > +		udelay(UDELAY_DELAY);
> > 
> > This is an interesting way to silence the checkpatch udelay warning.  ;)
> 
> I didn't know this could silence the warning :)

It also seems odd to have unsigned int wait_count and int count.

Maybe just use 30 in the loop without using wait_count at all.

I also think using UDELAY_DELAY is silly and essentially misleading
as it's also used as an argument value for mdelay

$ git grep -w UDELAY_DELAY
drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Joe Perches]

On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
> On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> > > Since wait_count=30 > 0, the for loop is equivalent to do while
> > > loop. This commit also replaces 100 with UDELAY_DELAY.
[]
> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
[]
> > > @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
> > >  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
> > >  {
> > >  	unsigned int wait_count = 30;
> > > +	int count;
> > > 
> > > -	do {
> > > +	for (count = 0; count < wait_count; count++) {
> > >  		if (!qlge_sem_trylock(qdev, sem_mask))
> > >  			return 0;
> > > -		udelay(100);
> > > -	} while (--wait_count);
> > > +		udelay(UDELAY_DELAY);
> > 
> > This is an interesting way to silence the checkpatch udelay warning.  ;)
> 
> I didn't know this could silence the warning :)

It also seems odd to have unsigned int wait_count and int count.

Maybe just use 30 in the loop without using wait_count at all.

I also think using UDELAY_DELAY is silly and essentially misleading
as it's also used as an argument value for mdelay

$ git grep -w UDELAY_DELAY
drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
>On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
>> On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> > > Since wait_count=30 > 0, the for loop is equivalent to do while
>> > > loop. This commit also replaces 100 with UDELAY_DELAY.
>[]
>> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>[]
>> > > @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>> > >  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>> > >  {
>> > >  	unsigned int wait_count = 30;
>> > > +	int count;
>> > >
>> > > -	do {
>> > > +	for (count = 0; count < wait_count; count++) {
>> > >  		if (!qlge_sem_trylock(qdev, sem_mask))
>> > >  			return 0;
>> > > -		udelay(100);
>> > > -	} while (--wait_count);
>> > > +		udelay(UDELAY_DELAY);
>> >
>> > This is an interesting way to silence the checkpatch udelay warning.  ;)
>>
>> I didn't know this could silence the warning :)
>
>It also seems odd to have unsigned int wait_count and int count.
>
>Maybe just use 30 in the loop without using wait_count at all.

Thanks for the suggestion. I will apply it to v1.
>
>I also think using UDELAY_DELAY is silly and essentially misleading
>as it's also used as an argument value for mdelay
>
>$ git grep -w UDELAY_DELAY
>drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
>drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */

Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
mdelay?

>
>

-- 
Best regards,
Coiby

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
>On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
>> On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> > > Since wait_count=30 > 0, the for loop is equivalent to do while
>> > > loop. This commit also replaces 100 with UDELAY_DELAY.
>[]
>> > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>[]
>> > > @@ -140,12 +140,13 @@ static int qlge_sem_trylock(struct qlge_adapter *qdev, u32 sem_mask)
>> > >  int qlge_sem_spinlock(struct qlge_adapter *qdev, u32 sem_mask)
>> > >  {
>> > >  	unsigned int wait_count = 30;
>> > > +	int count;
>> > >
>> > > -	do {
>> > > +	for (count = 0; count < wait_count; count++) {
>> > >  		if (!qlge_sem_trylock(qdev, sem_mask))
>> > >  			return 0;
>> > > -		udelay(100);
>> > > -	} while (--wait_count);
>> > > +		udelay(UDELAY_DELAY);
>> >
>> > This is an interesting way to silence the checkpatch udelay warning.  ;)
>>
>> I didn't know this could silence the warning :)
>
>It also seems odd to have unsigned int wait_count and int count.
>
>Maybe just use 30 in the loop without using wait_count at all.

Thanks for the suggestion. I will apply it to v1.
>
>I also think using UDELAY_DELAY is silly and essentially misleading
>as it's also used as an argument value for mdelay
>
>$ git grep -w UDELAY_DELAY
>drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
>drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */

Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
mdelay?

>
>

-- 
Best regards,
Coiby

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Joe Perches]

On Thu, 2021-07-01 at 07:33 +0800, Coiby Xu wrote:
> On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
> > On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
> > > On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
> > > > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> > > > > Since wait_count=30 > 0, the for loop is equivalent to do while
> > > > > loop. This commit also replaces 100 with UDELAY_DELAY.
> > []
> > > > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> > []
> > I also think using UDELAY_DELAY is silly and essentially misleading
> > as it's also used as an argument value for mdelay
> > 
> > $ git grep -w UDELAY_DELAY
> > drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */
> 
> Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
> mdelay?

I think the define is pointless and it'd be more readable
to just use 100 in all the cases.

IMO: There really aren't enough cases to justify using defines.

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Joe Perches]

On Thu, 2021-07-01 at 07:33 +0800, Coiby Xu wrote:
> On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
> > On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
> > > On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
> > > > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
> > > > > Since wait_count=30 > 0, the for loop is equivalent to do while
> > > > > loop. This commit also replaces 100 with UDELAY_DELAY.
> > []
> > > > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
> > []
> > I also think using UDELAY_DELAY is silly and essentially misleading
> > as it's also used as an argument value for mdelay
> > 
> > $ git grep -w UDELAY_DELAY
> > drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */
> 
> Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
> mdelay?

I think the define is pointless and it'd be more readable
to just use 100 in all the cases.

IMO: There really aren't enough cases to justify using defines.

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

On Wed, Jun 30, 2021 at 09:35:31PM -0700, Joe Perches wrote:
>On Thu, 2021-07-01 at 07:33 +0800, Coiby Xu wrote:
>> On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
>> > On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
>> > > On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>> > > > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> > > > > Since wait_count=30 > 0, the for loop is equivalent to do while
>> > > > > loop. This commit also replaces 100 with UDELAY_DELAY.
>> > []
>> > > > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> > []
>> > I also think using UDELAY_DELAY is silly and essentially misleading
>> > as it's also used as an argument value for mdelay
>> >
>> > $ git grep -w UDELAY_DELAY
>> > drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
>> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */
>>
>> Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
>> mdelay?
>
>I think the define is pointless and it'd be more readable
>to just use 100 in all the cases.
>
>IMO: There really aren't enough cases to justify using defines.

I thought magic number should be avoided if possible. This case is new
to me. Thanks for the explanation!

>
>

-- 
Best regards,
Coiby

Re: [RFC 13/19] staging: qlge: rewrite do while loop as for loop in qlge_sem_spinlock

[Coiby Xu]

On Wed, Jun 30, 2021 at 09:35:31PM -0700, Joe Perches wrote:
>On Thu, 2021-07-01 at 07:33 +0800, Coiby Xu wrote:
>> On Wed, Jun 30, 2021 at 03:58:06AM -0700, Joe Perches wrote:
>> > On Thu, 2021-06-24 at 19:22 +0800, Coiby Xu wrote:
>> > > On Tue, Jun 22, 2021 at 10:20:36AM +0300, Dan Carpenter wrote:
>> > > > On Mon, Jun 21, 2021 at 09:48:56PM +0800, Coiby Xu wrote:
>> > > > > Since wait_count=30 > 0, the for loop is equivalent to do while
>> > > > > loop. This commit also replaces 100 with UDELAY_DELAY.
>> > []
>> > > > > diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
>> > []
>> > I also think using UDELAY_DELAY is silly and essentially misleading
>> > as it's also used as an argument value for mdelay
>> >
>> > $ git grep -w UDELAY_DELAY
>> > drivers/staging/qlge/qlge.h:#define UDELAY_DELAY 100
>> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_main.c:               udelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY);
>> > drivers/staging/qlge/qlge_mpi.c:                mdelay(UDELAY_DELAY); /* 100ms */
>>
>> Thanks for spotting this issue! How about "#define MDELAY_DELAY 100" for
>> mdelay?
>
>I think the define is pointless and it'd be more readable
>to just use 100 in all the cases.
>
>IMO: There really aren't enough cases to justify using defines.

I thought magic number should be avoided if possible. This case is new
to me. Thanks for the explanation!

>
>

-- 
Best regards,
Coiby

[RFC 14/19] staging: qlge: rewrite do while loop as for loop in qlge_refill_bq

[Coiby Xu]

Since refill_count > 0, the for loop is equivalent to do while
loop.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/qlge_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 2d2405be38f5..904dba7aaee5 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -1092,6 +1092,7 @@ static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 	struct qlge_bq_desc *bq_desc;
 	int refill_count;
 	int retval;
+	int count;
 	int i;
 
 	refill_count = QLGE_BQ_WRAP(QLGE_BQ_ALIGN(bq->next_to_clean - 1) -
@@ -1102,7 +1103,7 @@ static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 	i = bq->next_to_use;
 	bq_desc = &bq->queue[i];
 	i -= QLGE_BQ_LEN;
-	do {
+	for (count = 0; count < refill_count; count++) {
 		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
 			     "ring %u %s: try cleaning idx %d\n",
 			     rx_ring->cq_id, bq_type_name[bq->type], i);
@@ -1124,8 +1125,7 @@ static int qlge_refill_bq(struct qlge_bq *bq, gfp_t gfp)
 			bq_desc = &bq->queue[0];
 			i -= QLGE_BQ_LEN;
 		}
-		refill_count--;
-	} while (refill_count);
+	}
 	i += QLGE_BQ_LEN;
 
 	if (bq->next_to_use != i) {
-- 
2.32.0

[RFC 15/19] staging: qlge: remove the TODO item about rewriting while loops as simple for loops

[Coiby Xu]

Since all while loops that could be written as simple for loops have
been converted, remove the TODO item.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 8bb6779a5bb4..4575f35114bf 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -6,8 +6,6 @@
   split cases.
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
-* some "while" loops could be rewritten with simple "for", ex.
-  ql_wait_reg_rdy(), ql_start_rx_ring())
 * remove duplicate and useless comments
 * fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
   qlge_set_multicast_list()).
-- 
2.32.0

[RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

This part of code is for the case that "the headers and data are in
a single large buffer". However, qlge_process_mac_split_rx_intr is for
handling packets that packets underwent head splitting. In reality, with
jumbo frame enabled, the part of code couldn't be reached regardless of
the packet size when ping the NIC.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO        |  6 ---
 drivers/staging/qlge/qlge_main.c | 66 ++++++++------------------------
 2 files changed, 17 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 4575f35114bf..0f96186ed77c 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -1,9 +1,3 @@
-* commit 7c734359d350 ("qlge: Size RX buffers based on MTU.", v2.6.33-rc1)
-  introduced dead code in the receive routines, which should be rewritten
-  anyways by the admission of the author himself, see the comment above
-  ql_build_rx_skb(). That function is now used exclusively to handle packets
-  that underwent header splitting but it still contains code to handle non
-  split cases.
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
 * remove duplicate and useless comments
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 904dba7aaee5..e560006225ca 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -1741,55 +1741,23 @@ static struct sk_buff *qlge_build_rx_skb(struct qlge_adapter *qdev,
 			sbq_desc->p.skb = NULL;
 		}
 	} else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL) {
-		if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
-			netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
-				     "Header in small, %d bytes in large. Chain large to small!\n",
-				     length);
-			/*
-			 * The data is in a single large buffer.  We
-			 * chain it to the header buffer's skb and let
-			 * it rip.
-			 */
-			lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
-			netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
-				     "Chaining page at offset = %d, for %d bytes  to skb.\n",
-				     lbq_desc->p.pg_chunk.offset, length);
-			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
-					   lbq_desc->p.pg_chunk.offset, length);
-			skb->len += length;
-			skb->data_len += length;
-			skb->truesize += qdev->lbq_buf_size;
-		} else {
-			/*
-			 * The headers and data are in a single large buffer. We
-			 * copy it to a new skb and let it go. This can happen with
-			 * jumbo mtu on a non-TCP/UDP frame.
-			 */
-			lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
-			skb = napi_alloc_skb(&rx_ring->napi, QLGE_SMALL_BUFFER_SIZE);
-			if (!skb) {
-				netif_printk(qdev, probe, KERN_DEBUG, qdev->ndev,
-					     "No skb available, drop the packet.\n");
-				return NULL;
-			}
-			dma_unmap_page(&qdev->pdev->dev, lbq_desc->dma_addr,
-				       qdev->lbq_buf_size,
-				       DMA_FROM_DEVICE);
-			skb_reserve(skb, NET_IP_ALIGN);
-			netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
-				     "%d bytes of headers and data in large. Chain page to new skb and pull tail.\n",
-				     length);
-			skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
-					   lbq_desc->p.pg_chunk.offset,
-					   length);
-			skb->len += length;
-			skb->data_len += length;
-			skb->truesize += qdev->lbq_buf_size;
-			qlge_update_mac_hdr_len(qdev, ib_mac_rsp,
-						lbq_desc->p.pg_chunk.va,
-						&hlen);
-			__pskb_pull_tail(skb, hlen);
-		}
+		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
+			     "Header in small, %d bytes in large. Chain large to small!\n",
+			     length);
+		/*
+		 * The data is in a single large buffer.  We
+		 * chain it to the header buffer's skb and let
+		 * it rip.
+		 */
+		lbq_desc = qlge_get_curr_lchunk(qdev, rx_ring);
+		netif_printk(qdev, rx_status, KERN_DEBUG, qdev->ndev,
+			     "Chaining page at offset = %d, for %d bytes  to skb.\n",
+			     lbq_desc->p.pg_chunk.offset, length);
+		skb_fill_page_desc(skb, 0, lbq_desc->p.pg_chunk.page,
+				   lbq_desc->p.pg_chunk.offset, length);
+		skb->len += length;
+		skb->data_len += length;
+		skb->truesize += qdev->lbq_buf_size;
 	} else {
 		/*
 		 * The data is in a chain of large buffers
-- 
2.32.0

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
> This part of code is for the case that "the headers and data are in
> a single large buffer". However, qlge_process_mac_split_rx_intr is for
> handling packets that packets underwent head splitting. In reality, with
> jumbo frame enabled, the part of code couldn't be reached regardless of
> the packet size when ping the NIC.
> 

This commit message is a bit confusing.  We're just deleting the else
statement.  Once I knew that then it was easy enough to review
qlge_process_mac_rx_intr() and see that if if
ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
> This part of code is for the case that "the headers and data are in
> a single large buffer". However, qlge_process_mac_split_rx_intr is for
> handling packets that packets underwent head splitting. In reality, with
> jumbo frame enabled, the part of code couldn't be reached regardless of
> the packet size when ping the NIC.
> 

This commit message is a bit confusing.  We're just deleting the else
statement.  Once I knew that then it was easy enough to review
qlge_process_mac_rx_intr() and see that if if
ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
>> This part of code is for the case that "the headers and data are in
>> a single large buffer". However, qlge_process_mac_split_rx_intr is for
>> handling packets that packets underwent head splitting. In reality, with
>> jumbo frame enabled, the part of code couldn't be reached regardless of
>> the packet size when ping the NIC.
>>
>
>This commit message is a bit confusing.  We're just deleting the else
>statement.  Once I knew that then it was easy enough to review
>qlge_process_mac_rx_intr() and see that if if
>ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
>ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.

Do you suggest moving to upper if, i.e.

         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {

and then deleting the else statement?

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
>> This part of code is for the case that "the headers and data are in
>> a single large buffer". However, qlge_process_mac_split_rx_intr is for
>> handling packets that packets underwent head splitting. In reality, with
>> jumbo frame enabled, the part of code couldn't be reached regardless of
>> the packet size when ping the NIC.
>>
>
>This commit message is a bit confusing.  We're just deleting the else
>statement.  Once I knew that then it was easy enough to review
>qlge_process_mac_rx_intr() and see that if if
>ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
>ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.

Do you suggest moving to upper if, i.e.

         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {

and then deleting the else statement?

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
> On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
> > > This part of code is for the case that "the headers and data are in
> > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
> > > handling packets that packets underwent head splitting. In reality, with
> > > jumbo frame enabled, the part of code couldn't be reached regardless of
> > > the packet size when ping the NIC.
> > > 
> > 
> > This commit message is a bit confusing.  We're just deleting the else
> > statement.  Once I knew that then it was easy enough to review
> > qlge_process_mac_rx_intr() and see that if if
> > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
> > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
> 
> Do you suggest moving to upper if, i.e.
> 
>         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
> 
> and then deleting the else statement?
> 

I have a rule that when people whinge about commit messages they should
write a better one themselves, but I have violated my own rule.  Sorry.
Here is my suggestion:

    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
    true as well.  Thus, we can remove that condition and delete the
    else statement which is dead code.

    (Originally this code was for the case that "the headers and data are
    in a single large buffer". However, qlge_process_mac_split_rx_intr
    is for handling packets that packets underwent head splitting).

TBH, I don't know the code well enough to understand the second
paragraph but the first paragraph is straight forward.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
> On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
> > > This part of code is for the case that "the headers and data are in
> > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
> > > handling packets that packets underwent head splitting. In reality, with
> > > jumbo frame enabled, the part of code couldn't be reached regardless of
> > > the packet size when ping the NIC.
> > > 
> > 
> > This commit message is a bit confusing.  We're just deleting the else
> > statement.  Once I knew that then it was easy enough to review
> > qlge_process_mac_rx_intr() and see that if if
> > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
> > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
> 
> Do you suggest moving to upper if, i.e.
> 
>         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
> 
> and then deleting the else statement?
> 

I have a rule that when people whinge about commit messages they should
write a better one themselves, but I have violated my own rule.  Sorry.
Here is my suggestion:

    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
    true as well.  Thus, we can remove that condition and delete the
    else statement which is dead code.

    (Originally this code was for the case that "the headers and data are
    in a single large buffer". However, qlge_process_mac_split_rx_intr
    is for handling packets that packets underwent head splitting).

TBH, I don't know the code well enough to understand the second
paragraph but the first paragraph is straight forward.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Thu, Jun 24, 2021 at 03:49:26PM +0300, Dan Carpenter wrote:
>On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
>> On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
>> > > This part of code is for the case that "the headers and data are in
>> > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
>> > > handling packets that packets underwent head splitting. In reality, with
>> > > jumbo frame enabled, the part of code couldn't be reached regardless of
>> > > the packet size when ping the NIC.
>> > >
>> >
>> > This commit message is a bit confusing.  We're just deleting the else
>> > statement.  Once I knew that then it was easy enough to review
>> > qlge_process_mac_rx_intr() and see that if if
>> > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
>> > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
>>
>> Do you suggest moving to upper if, i.e.
>>
>>         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
>>
>> and then deleting the else statement?
>>
>
>I have a rule that when people whinge about commit messages they should
>write a better one themselves, but I have violated my own rule.  Sorry.
>Here is my suggestion:
>
>    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
>    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
>    true as well.  Thus, we can remove that condition and delete the
>    else statement which is dead code.
>
>    (Originally this code was for the case that "the headers and data are
>    in a single large buffer". However, qlge_process_mac_split_rx_intr
>    is for handling packets that packets underwent head splitting).

Thanks for sharing your commit message! Now I see what you mean. But I'm
not sure if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" is true when  
"ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" is true. We only know that
the head splitting case is exclusively dealt with by the function 
qlge_process_mac_split_rx_intr,
     /* Process an inbound completion from an rx ring. */
     static unsigned long qlge_process_mac_rx_intr(struct qlge_adapter *qdev,
     					      struct rx_ring *rx_ring,
     					      struct qlge_ib_mac_iocb_rsp *ib_mac_rsp)
     {
         ...   
     	if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV) {
     		/* The data and headers are split into
     		 * separate buffers.
     		 */
     		qlge_process_mac_split_rx_intr(qdev, rx_ring, ib_mac_rsp,
     					       vlan_id);
     	} else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DS) {
		

And skb_build_skb is only called by qlge_build_rx_skb. So this part of
code that deals with the packets that don't go through head splitting
must be dead code. And the test that ping the NIC with packets of
different sizes could also confirm it.

>
>TBH, I don't know the code well enough to understand the second
>paragraph but the first paragraph is straight forward.
>
>regards,
>dan carpenter

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Thu, Jun 24, 2021 at 03:49:26PM +0300, Dan Carpenter wrote:
>On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
>> On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
>> > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
>> > > This part of code is for the case that "the headers and data are in
>> > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
>> > > handling packets that packets underwent head splitting. In reality, with
>> > > jumbo frame enabled, the part of code couldn't be reached regardless of
>> > > the packet size when ping the NIC.
>> > >
>> >
>> > This commit message is a bit confusing.  We're just deleting the else
>> > statement.  Once I knew that then it was easy enough to review
>> > qlge_process_mac_rx_intr() and see that if if
>> > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
>> > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
>>
>> Do you suggest moving to upper if, i.e.
>>
>>         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
>>
>> and then deleting the else statement?
>>
>
>I have a rule that when people whinge about commit messages they should
>write a better one themselves, but I have violated my own rule.  Sorry.
>Here is my suggestion:
>
>    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
>    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
>    true as well.  Thus, we can remove that condition and delete the
>    else statement which is dead code.
>
>    (Originally this code was for the case that "the headers and data are
>    in a single large buffer". However, qlge_process_mac_split_rx_intr
>    is for handling packets that packets underwent head splitting).

Thanks for sharing your commit message! Now I see what you mean. But I'm
not sure if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" is true when  
"ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" is true. We only know that
the head splitting case is exclusively dealt with by the function 
qlge_process_mac_split_rx_intr,
     /* Process an inbound completion from an rx ring. */
     static unsigned long qlge_process_mac_rx_intr(struct qlge_adapter *qdev,
     					      struct rx_ring *rx_ring,
     					      struct qlge_ib_mac_iocb_rsp *ib_mac_rsp)
     {
         ...   
     	if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV) {
     		/* The data and headers are split into
     		 * separate buffers.
     		 */
     		qlge_process_mac_split_rx_intr(qdev, rx_ring, ib_mac_rsp,
     					       vlan_id);
     	} else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DS) {
		

And skb_build_skb is only called by qlge_build_rx_skb. So this part of
code that deals with the packets that don't go through head splitting
must be dead code. And the test that ping the NIC with packets of
different sizes could also confirm it.

>
>TBH, I don't know the code well enough to understand the second
>paragraph but the first paragraph is straight forward.
>
>regards,
>dan carpenter

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

On Sun, Jun 27, 2021 at 06:53:49PM +0800, Coiby Xu wrote:
> On Thu, Jun 24, 2021 at 03:49:26PM +0300, Dan Carpenter wrote:
> > On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
> > > On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
> > > > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
> > > > > This part of code is for the case that "the headers and data are in
> > > > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
> > > > > handling packets that packets underwent head splitting. In reality, with
> > > > > jumbo frame enabled, the part of code couldn't be reached regardless of
> > > > > the packet size when ping the NIC.
> > > > >
> > > >
> > > > This commit message is a bit confusing.  We're just deleting the else
> > > > statement.  Once I knew that then it was easy enough to review
> > > > qlge_process_mac_rx_intr() and see that if if
> > > > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
> > > > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
> > > 
> > > Do you suggest moving to upper if, i.e.
> > > 
> > >         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
> > > 
> > > and then deleting the else statement?
> > > 
> > 
> > I have a rule that when people whinge about commit messages they should
> > write a better one themselves, but I have violated my own rule.  Sorry.
> > Here is my suggestion:
> > 
> >    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
> >    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
> >    true as well.  Thus, we can remove that condition and delete the
> >    else statement which is dead code.
> > 
> >    (Originally this code was for the case that "the headers and data are
> >    in a single large buffer". However, qlge_process_mac_split_rx_intr
> >    is for handling packets that packets underwent head splitting).
> 
> Thanks for sharing your commit message! Now I see what you mean. But I'm
> not sure if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" is true when
> "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" is true.

Well... It is true.  qlge_process_mac_split_rx_intr() is only called
when "->flags4 & IB_MAC_IOCB_RSP_HS" is true or when
"->flags3 & IB_MAC_IOCB_RSP_DL" is false.

To me the fact that it's clearly dead code, helps me to verify that the
patch doesn't change behavior.  Anyway, "this part of code" was a bit
vague and it took me a while to figure out the patch deletes the else
statement.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

On Sun, Jun 27, 2021 at 06:53:49PM +0800, Coiby Xu wrote:
> On Thu, Jun 24, 2021 at 03:49:26PM +0300, Dan Carpenter wrote:
> > On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
> > > On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
> > > > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
> > > > > This part of code is for the case that "the headers and data are in
> > > > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
> > > > > handling packets that packets underwent head splitting. In reality, with
> > > > > jumbo frame enabled, the part of code couldn't be reached regardless of
> > > > > the packet size when ping the NIC.
> > > > >
> > > >
> > > > This commit message is a bit confusing.  We're just deleting the else
> > > > statement.  Once I knew that then it was easy enough to review
> > > > qlge_process_mac_rx_intr() and see that if if
> > > > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
> > > > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
> > > 
> > > Do you suggest moving to upper if, i.e.
> > > 
> > >         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
> > > 
> > > and then deleting the else statement?
> > > 
> > 
> > I have a rule that when people whinge about commit messages they should
> > write a better one themselves, but I have violated my own rule.  Sorry.
> > Here is my suggestion:
> > 
> >    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
> >    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
> >    true as well.  Thus, we can remove that condition and delete the
> >    else statement which is dead code.
> > 
> >    (Originally this code was for the case that "the headers and data are
> >    in a single large buffer". However, qlge_process_mac_split_rx_intr
> >    is for handling packets that packets underwent head splitting).
> 
> Thanks for sharing your commit message! Now I see what you mean. But I'm
> not sure if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" is true when
> "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" is true.

Well... It is true.  qlge_process_mac_split_rx_intr() is only called
when "->flags4 & IB_MAC_IOCB_RSP_HS" is true or when
"->flags3 & IB_MAC_IOCB_RSP_DL" is false.

To me the fact that it's clearly dead code, helps me to verify that the
patch doesn't change behavior.  Anyway, "this part of code" was a bit
vague and it took me a while to figure out the patch deletes the else
statement.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Mon, Jun 28, 2021 at 09:46:45AM +0300, Dan Carpenter wrote:
>On Sun, Jun 27, 2021 at 06:53:49PM +0800, Coiby Xu wrote:
>> On Thu, Jun 24, 2021 at 03:49:26PM +0300, Dan Carpenter wrote:
>> > On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
>> > > On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
>> > > > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
>> > > > > This part of code is for the case that "the headers and data are in
>> > > > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
>> > > > > handling packets that packets underwent head splitting. In reality, with
>> > > > > jumbo frame enabled, the part of code couldn't be reached regardless of
>> > > > > the packet size when ping the NIC.
>> > > > >
>> > > >
>> > > > This commit message is a bit confusing.  We're just deleting the else
>> > > > statement.  Once I knew that then it was easy enough to review
>> > > > qlge_process_mac_rx_intr() and see that if if
>> > > > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
>> > > > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
>> > >
>> > > Do you suggest moving to upper if, i.e.
>> > >
>> > >         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
>> > >
>> > > and then deleting the else statement?
>> > >
>> >
>> > I have a rule that when people whinge about commit messages they should
>> > write a better one themselves, but I have violated my own rule.  Sorry.
>> > Here is my suggestion:
>> >
>> >    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
>> >    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
>> >    true as well.  Thus, we can remove that condition and delete the
>> >    else statement which is dead code.
>> >
>> >    (Originally this code was for the case that "the headers and data are
>> >    in a single large buffer". However, qlge_process_mac_split_rx_intr
>> >    is for handling packets that packets underwent head splitting).
>>
>> Thanks for sharing your commit message! Now I see what you mean. But I'm
>> not sure if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" is true when
>> "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" is true.
>
>Well... It is true.  qlge_process_mac_split_rx_intr() is only called
>when "->flags4 & IB_MAC_IOCB_RSP_HS" is true or when
>"->flags3 & IB_MAC_IOCB_RSP_DL" is false.

Actually qlge_process_mac_rx_intr calls qlge_process_mac_split_rx_intr when 
"ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV" is true or in the last else,

     /* Process an inbound completion from an rx ring. */
     static unsigned long qlge_process_mac_rx_intr(struct qlge_adapter *qdev,
     					      struct rx_ring *rx_ring,
     					      struct qlge_ib_mac_iocb_rsp *ib_mac_rsp)
     {
         ...
     	if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV) {
     		/* The data and headers are split into
     		 * separate buffers.
     		 */
     		qlge_process_mac_split_rx_intr(qdev, rx_ring, ib_mac_rsp,
     					       vlan_id);
     	} else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DS) {
             ...
     	} else {
     		/* Non-TCP/UDP large frames that span multiple buffers
     		 * can be processed corrrectly by the split frame logic.
     		 */
     		qlge_process_mac_split_rx_intr(qdev, rx_ring, ib_mac_rsp,
     					       vlan_id);
     	}

So I think we can't say that if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV" 
is true,  then "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be true. And 
I don't know how to reach the conclusion that the last else means 
"->flags3 & IB_MAC_IOCB_RSP_DL" is false.

>
>To me the fact that it's clearly dead code, helps me to verify that the
>patch doesn't change behavior.  Anyway, "this part of code" was a bit
>vague and it took me a while to figure out the patch deletes the else
>statement.
>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Mon, Jun 28, 2021 at 09:46:45AM +0300, Dan Carpenter wrote:
>On Sun, Jun 27, 2021 at 06:53:49PM +0800, Coiby Xu wrote:
>> On Thu, Jun 24, 2021 at 03:49:26PM +0300, Dan Carpenter wrote:
>> > On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:
>> > > On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:
>> > > > On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:
>> > > > > This part of code is for the case that "the headers and data are in
>> > > > > a single large buffer". However, qlge_process_mac_split_rx_intr is for
>> > > > > handling packets that packets underwent head splitting. In reality, with
>> > > > > jumbo frame enabled, the part of code couldn't be reached regardless of
>> > > > > the packet size when ping the NIC.
>> > > > >
>> > > >
>> > > > This commit message is a bit confusing.  We're just deleting the else
>> > > > statement.  Once I knew that then it was easy enough to review
>> > > > qlge_process_mac_rx_intr() and see that if if
>> > > > ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL is set then
>> > > > ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV must be set.
>> > >
>> > > Do you suggest moving to upper if, i.e.
>> > >
>> > >         } else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL && ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS) {
>> > >
>> > > and then deleting the else statement?
>> > >
>> >
>> > I have a rule that when people whinge about commit messages they should
>> > write a better one themselves, but I have violated my own rule.  Sorry.
>> > Here is my suggestion:
>> >
>> >    If the "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" condition is true
>> >    then we know that "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be
>> >    true as well.  Thus, we can remove that condition and delete the
>> >    else statement which is dead code.
>> >
>> >    (Originally this code was for the case that "the headers and data are
>> >    in a single large buffer". However, qlge_process_mac_split_rx_intr
>> >    is for handling packets that packets underwent head splitting).
>>
>> Thanks for sharing your commit message! Now I see what you mean. But I'm
>> not sure if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" is true when
>> "ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DL" is true.
>
>Well... It is true.  qlge_process_mac_split_rx_intr() is only called
>when "->flags4 & IB_MAC_IOCB_RSP_HS" is true or when
>"->flags3 & IB_MAC_IOCB_RSP_DL" is false.

Actually qlge_process_mac_rx_intr calls qlge_process_mac_split_rx_intr when 
"ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV" is true or in the last else,

     /* Process an inbound completion from an rx ring. */
     static unsigned long qlge_process_mac_rx_intr(struct qlge_adapter *qdev,
     					      struct rx_ring *rx_ring,
     					      struct qlge_ib_mac_iocb_rsp *ib_mac_rsp)
     {
         ...
     	if (ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV) {
     		/* The data and headers are split into
     		 * separate buffers.
     		 */
     		qlge_process_mac_split_rx_intr(qdev, rx_ring, ib_mac_rsp,
     					       vlan_id);
     	} else if (ib_mac_rsp->flags3 & IB_MAC_IOCB_RSP_DS) {
             ...
     	} else {
     		/* Non-TCP/UDP large frames that span multiple buffers
     		 * can be processed corrrectly by the split frame logic.
     		 */
     		qlge_process_mac_split_rx_intr(qdev, rx_ring, ib_mac_rsp,
     					       vlan_id);
     	}

So I think we can't say that if "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HV" 
is true,  then "ib_mac_rsp->flags4 & IB_MAC_IOCB_RSP_HS" must be true. And 
I don't know how to reach the conclusion that the last else means 
"->flags3 & IB_MAC_IOCB_RSP_DL" is false.

>
>To me the fact that it's clearly dead code, helps me to verify that the
>patch doesn't change behavior.  Anyway, "this part of code" was a bit
>vague and it took me a while to figure out the patch deletes the else
>statement.
>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

*sigh*

You're right.  Sorry.

I misread IB_MAC_IOCB_RSP_HV as IB_MAC_IOCB_RSP_HS.  In my defense, it's
a five word name and only one letter is different.  It's like trying to
find Waldo.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Dan Carpenter]

*sigh*

You're right.  Sorry.

I misread IB_MAC_IOCB_RSP_HV as IB_MAC_IOCB_RSP_HS.  In my defense, it's
a five word name and only one letter is different.  It's like trying to
find Waldo.

regards,
dan carpenter

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Tue, Jun 29, 2021 at 05:22:02PM +0300, Dan Carpenter wrote:
>*sigh*
>
>You're right.  Sorry.
>
>I misread IB_MAC_IOCB_RSP_HV as IB_MAC_IOCB_RSP_HS.  In my defense, it's
>a five word name and only one letter is different.  It's like trying to
>find Waldo.

That's fine. Thanks for reviewing the patch and prompting me to write a 
more comprehensible commit message!

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb

[Coiby Xu]

On Tue, Jun 29, 2021 at 05:22:02PM +0300, Dan Carpenter wrote:
>*sigh*
>
>You're right.  Sorry.
>
>I misread IB_MAC_IOCB_RSP_HV as IB_MAC_IOCB_RSP_HS.  In my defense, it's
>a five word name and only one letter is different.  It's like trying to
>find Waldo.

That's fine. Thanks for reviewing the patch and prompting me to write a 
more comprehensible commit message!

>
>regards,
>dan carpenter
>

-- 
Best regards,
Coiby

[RFC 17/19] staging: qlge: fix weird line wrapping

[Coiby Xu]

This commits fix weird line wrapping based on "clang-format
drivers/staging/qlge/qlge_main.c"

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO        |   2 -
 drivers/staging/qlge/qlge_main.c | 106 +++++++++++++++----------------
 2 files changed, 52 insertions(+), 56 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 0f96186ed77c..b8def0c70614 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -1,7 +1,5 @@
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
 * remove duplicate and useless comments
-* fix weird line wrapping (all over, ex. the ql_set_routing_reg() calls in
-  qlge_set_multicast_list()).
 * fix weird indentation (all over, ex. the for loops in qlge_get_stats())
 * fix checkpatch issues
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index e560006225ca..21fb942c2595 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -442,8 +442,7 @@ static int qlge_set_mac_addr(struct qlge_adapter *qdev, int set)
 	status = qlge_sem_spinlock(qdev, SEM_MAC_ADDR_MASK);
 	if (status)
 		return status;
-	status = qlge_set_mac_addr_reg(qdev, (u8 *)addr,
-				       MAC_ADDR_TYPE_CAM_MAC,
+	status = qlge_set_mac_addr_reg(qdev, (u8 *)addr, MAC_ADDR_TYPE_CAM_MAC,
 				       qdev->func * MAX_CQ);
 	qlge_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
 	if (status)
@@ -524,8 +523,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
 		{
 			value = RT_IDX_DST_DFLT_Q | /* dest */
 				RT_IDX_TYPE_NICQ | /* type */
-				(RT_IDX_IP_CSUM_ERR_SLOT <<
-				RT_IDX_IDX_SHIFT); /* index */
+			(RT_IDX_IP_CSUM_ERR_SLOT
+			 << RT_IDX_IDX_SHIFT); /* index */
 			break;
 		}
 	case RT_IDX_TU_CSUM_ERR: /* Pass up TCP/UDP CSUM error frames. */
@@ -554,7 +553,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
 		{
 			value = RT_IDX_DST_DFLT_Q |	/* dest */
 			    RT_IDX_TYPE_NICQ |	/* type */
-			    (RT_IDX_MCAST_MATCH_SLOT << RT_IDX_IDX_SHIFT);/* index */
+			(RT_IDX_MCAST_MATCH_SLOT
+			 << RT_IDX_IDX_SHIFT); /* index */
 			break;
 		}
 	case RT_IDX_RSS_MATCH:	/* Pass up matched RSS frames. */
@@ -648,15 +648,15 @@ static int qlge_read_flash_word(struct qlge_adapter *qdev, int offset, __le32 *d
 {
 	int status = 0;
 	/* wait for reg to come ready */
-	status = qlge_wait_reg_rdy(qdev,
-				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
+	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
+				   FLASH_ADDR_ERR);
 	if (status)
 		goto exit;
 	/* set up for reg read */
 	qlge_write32(qdev, FLASH_ADDR, FLASH_ADDR_R | offset);
 	/* wait for reg to come ready */
-	status = qlge_wait_reg_rdy(qdev,
-				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
+	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
+				   FLASH_ADDR_ERR);
 	if (status)
 		goto exit;
 	/* This data is stored on flash as an array of
@@ -792,8 +792,8 @@ static int qlge_write_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 data)
 {
 	int status;
 	/* wait for reg to come ready */
-	status = qlge_wait_reg_rdy(qdev,
-				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
+	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
+				   XGMAC_ADDR_XME);
 	if (status)
 		return status;
 	/* write the data to the data reg */
@@ -811,15 +811,15 @@ int qlge_read_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 *data)
 {
 	int status = 0;
 	/* wait for reg to come ready */
-	status = qlge_wait_reg_rdy(qdev,
-				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
+	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
+				   XGMAC_ADDR_XME);
 	if (status)
 		goto exit;
 	/* set up for reg read */
 	qlge_write32(qdev, XGMAC_ADDR, reg | XGMAC_ADDR_R);
 	/* wait for reg to come ready */
-	status = qlge_wait_reg_rdy(qdev,
-				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
+	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
+				   XGMAC_ADDR_XME);
 	if (status)
 		goto exit;
 	/* get the data */
@@ -1067,8 +1067,8 @@ static int qlge_refill_lb(struct qlge_rx_ring *rx_ring,
 
 	lbq_desc->p.pg_chunk = *master_chunk;
 	lbq_desc->dma_addr = rx_ring->chunk_dma_addr;
-	*lbq_desc->buf_ptr = cpu_to_le64(lbq_desc->dma_addr +
-					 lbq_desc->p.pg_chunk.offset);
+	*lbq_desc->buf_ptr =
+		cpu_to_le64(lbq_desc->dma_addr + lbq_desc->p.pg_chunk.offset);
 
 	/* Adjust the master page chunk for next
 	 * buffer get.
@@ -1233,7 +1233,8 @@ static void qlge_unmap_send(struct qlge_adapter *qdev,
  */
 static int qlge_map_send(struct qlge_adapter *qdev,
 			 struct qlge_ob_mac_iocb_req *mac_iocb_ptr,
-			 struct sk_buff *skb, struct qlge_tx_ring_desc *tx_ring_desc)
+			 struct sk_buff *skb,
+			 struct qlge_tx_ring_desc *tx_ring_desc)
 {
 	int len = skb_headlen(skb);
 	dma_addr_t map;
@@ -1295,7 +1296,8 @@ static int qlge_map_send(struct qlge_adapter *qdev,
 			 *      etc...
 			 */
 			/* Tack on the OAL in the eighth segment of IOCB. */
-			map = dma_map_single(&qdev->pdev->dev, &tx_ring_desc->oal,
+			map = dma_map_single(&qdev->pdev->dev,
+					     &tx_ring_desc->oal,
 					     sizeof(struct qlge_oal),
 					     DMA_TO_DEVICE);
 			err = dma_mapping_error(&qdev->pdev->dev, map);
@@ -1405,8 +1407,7 @@ static void qlge_update_mac_hdr_len(struct qlge_adapter *qdev,
 	if (ib_mac_rsp->flags2 & IB_MAC_IOCB_RSP_V) {
 		tags = (u16 *)page;
 		/* Look for stacked vlan tags in ethertype field */
-		if (tags[6] == ETH_P_8021Q &&
-		    tags[8] == ETH_P_8021Q)
+		if (tags[6] == ETH_P_8021Q && tags[8] == ETH_P_8021Q)
 			*len += 2 * VLAN_HLEN;
 		else
 			*len += VLAN_HLEN;
@@ -1442,8 +1443,7 @@ static void qlge_process_mac_rx_gro_page(struct qlge_adapter *qdev,
 	prefetch(lbq_desc->p.pg_chunk.va);
 	__skb_fill_page_desc(skb, skb_shinfo(skb)->nr_frags,
 			     lbq_desc->p.pg_chunk.page,
-			     lbq_desc->p.pg_chunk.offset,
-			     length);
+			     lbq_desc->p.pg_chunk.offset, length);
 
 	skb->len += length;
 	skb->data_len += length;
@@ -2264,8 +2264,8 @@ static int __qlge_vlan_rx_add_vid(struct qlge_adapter *qdev, u16 vid)
 	u32 enable_bit = MAC_ADDR_E;
 	int err;
 
-	err = qlge_set_mac_addr_reg(qdev, (u8 *)&enable_bit,
-				    MAC_ADDR_TYPE_VLAN, vid);
+	err = qlge_set_mac_addr_reg(qdev, (u8 *)&enable_bit, MAC_ADDR_TYPE_VLAN,
+				    vid);
 	if (err)
 		netif_err(qdev, ifup, qdev->ndev,
 			  "Failed to init vlan address.\n");
@@ -2295,8 +2295,8 @@ static int __qlge_vlan_rx_kill_vid(struct qlge_adapter *qdev, u16 vid)
 	u32 enable_bit = 0;
 	int err;
 
-	err = qlge_set_mac_addr_reg(qdev, (u8 *)&enable_bit,
-				    MAC_ADDR_TYPE_VLAN, vid);
+	err = qlge_set_mac_addr_reg(qdev, (u8 *)&enable_bit, MAC_ADDR_TYPE_VLAN,
+				    vid);
 	if (err)
 		netif_err(qdev, ifup, qdev->ndev,
 			  "Failed to clear vlan address.\n");
@@ -2400,8 +2400,8 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
 		netif_err(qdev, intr, qdev->ndev,
 			  "Got MPI processor interrupt.\n");
 		qlge_write32(qdev, INTR_MASK, (INTR_MASK_PI << 16));
-		queue_delayed_work_on(smp_processor_id(),
-				      qdev->workqueue, &qdev->mpi_work, 0);
+		queue_delayed_work_on(smp_processor_id(), qdev->workqueue,
+				      &qdev->mpi_work, 0);
 		work_done++;
 	}
 
@@ -2730,8 +2730,7 @@ static void qlge_free_sbq_buffers(struct qlge_adapter *qdev, struct qlge_rx_ring
 		}
 		if (sbq_desc->p.skb) {
 			dma_unmap_single(&qdev->pdev->dev, sbq_desc->dma_addr,
-					 QLGE_SMALL_BUF_MAP_SIZE,
-					 DMA_FROM_DEVICE);
+					 QLGE_SMALL_BUF_MAP_SIZE, DMA_FROM_DEVICE);
 			dev_kfree_skb(sbq_desc->p.skb);
 			sbq_desc->p.skb = NULL;
 		}
@@ -2824,9 +2823,8 @@ static void qlge_free_cq_resources(struct qlge_adapter *qdev,
 
 	/* Free the completion queue. */
 	if (cq->cq_base) {
-		dma_free_coherent(&qdev->pdev->dev,
-				  cq->cq_size,
-				  cq->cq_base, cq->cq_base_dma);
+		dma_free_coherent(&qdev->pdev->dev, cq->cq_size, cq->cq_base,
+				  cq->cq_base_dma);
 		cq->cq_base = NULL;
 	}
 }
@@ -3128,8 +3126,8 @@ static void qlge_enable_msix(struct qlge_adapter *qdev)
 		for (i = 0; i < qdev->intr_count; i++)
 			qdev->msi_x_entry[i].entry = i;
 
-		err = pci_enable_msix_range(qdev->pdev, qdev->msi_x_entry,
-					    1, qdev->intr_count);
+		err = pci_enable_msix_range(qdev->pdev, qdev->msi_x_entry, 1,
+					    qdev->intr_count);
 		if (err < 0) {
 			kfree(qdev->msi_x_entry);
 			qdev->msi_x_entry = NULL;
@@ -3509,8 +3507,8 @@ static int qlge_route_initialize(struct qlge_adapter *qdev)
 		}
 	}
 
-	status = qlge_set_routing_reg(qdev, RT_IDX_CAM_HIT_SLOT,
-				      RT_IDX_CAM_HIT, 1);
+	status = qlge_set_routing_reg(qdev, RT_IDX_CAM_HIT_SLOT, RT_IDX_CAM_HIT,
+				      1);
 	if (status)
 		netif_err(qdev, ifup, qdev->ndev,
 			  "Failed to init routing register for CAM packets.\n");
@@ -3713,8 +3711,8 @@ static void qlge_display_dev_info(struct net_device *ndev)
 		   qdev->chip_rev_id >> 4 & 0x0000000f,
 		   qdev->chip_rev_id >> 8 & 0x0000000f,
 		   qdev->chip_rev_id >> 12 & 0x0000000f);
-	netif_info(qdev, probe, qdev->ndev,
-		   "MAC address %pM\n", ndev->dev_addr);
+	netif_info(qdev, probe, qdev->ndev, "MAC address %pM\n",
+		   ndev->dev_addr);
 }
 
 static int qlge_wol(struct qlge_adapter *qdev)
@@ -4119,8 +4117,8 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 	 */
 	if (ndev->flags & IFF_PROMISC) {
 		if (!test_bit(QL_PROMISCUOUS, &qdev->flags)) {
-			if (qlge_set_routing_reg
-			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 1)) {
+			if (qlge_set_routing_reg(qdev, RT_IDX_PROMISCUOUS_SLOT,
+						 RT_IDX_VALID, 1)) {
 				netif_err(qdev, hw, qdev->ndev,
 					  "Failed to set promiscuous mode.\n");
 			} else {
@@ -4129,8 +4127,8 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 		}
 	} else {
 		if (test_bit(QL_PROMISCUOUS, &qdev->flags)) {
-			if (qlge_set_routing_reg
-			    (qdev, RT_IDX_PROMISCUOUS_SLOT, RT_IDX_VALID, 0)) {
+			if (qlge_set_routing_reg(qdev, RT_IDX_PROMISCUOUS_SLOT,
+						 RT_IDX_VALID, 0)) {
 				netif_err(qdev, hw, qdev->ndev,
 					  "Failed to clear promiscuous mode.\n");
 			} else {
@@ -4146,8 +4144,8 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 	if ((ndev->flags & IFF_ALLMULTI) ||
 	    (netdev_mc_count(ndev) > MAX_MULTICAST_ENTRIES)) {
 		if (!test_bit(QL_ALLMULTI, &qdev->flags)) {
-			if (qlge_set_routing_reg
-			    (qdev, RT_IDX_ALLMULTI_SLOT, RT_IDX_MCAST, 1)) {
+			if (qlge_set_routing_reg(qdev, RT_IDX_ALLMULTI_SLOT,
+						 RT_IDX_MCAST, 1)) {
 				netif_err(qdev, hw, qdev->ndev,
 					  "Failed to set all-multi mode.\n");
 			} else {
@@ -4156,8 +4154,8 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 		}
 	} else {
 		if (test_bit(QL_ALLMULTI, &qdev->flags)) {
-			if (qlge_set_routing_reg
-			    (qdev, RT_IDX_ALLMULTI_SLOT, RT_IDX_MCAST, 0)) {
+			if (qlge_set_routing_reg(qdev, RT_IDX_ALLMULTI_SLOT,
+						 RT_IDX_MCAST, 0)) {
 				netif_err(qdev, hw, qdev->ndev,
 					  "Failed to clear all-multi mode.\n");
 			} else {
@@ -4182,8 +4180,8 @@ static void qlge_set_multicast_list(struct net_device *ndev)
 			i++;
 		}
 		qlge_sem_unlock(qdev, SEM_MAC_ADDR_MASK);
-		if (qlge_set_routing_reg
-		    (qdev, RT_IDX_MCAST_MATCH_SLOT, RT_IDX_MCAST_MATCH, 1)) {
+		if (qlge_set_routing_reg(qdev, RT_IDX_MCAST_MATCH_SLOT,
+					 RT_IDX_MCAST_MATCH, 1)) {
 			netif_err(qdev, hw, qdev->ndev,
 				  "Failed to set multicast match mode.\n");
 		} else {
@@ -4458,8 +4456,8 @@ static int qlge_init_device(struct pci_dev *pdev, struct qlge_adapter *qdev,
 	/*
 	 * Set up the operating parameters.
 	 */
-	qdev->workqueue = alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM,
-						  ndev->name);
+	qdev->workqueue =
+		alloc_ordered_workqueue("%s", WQ_MEM_RECLAIM, ndev->name);
 	if (!qdev->workqueue) {
 		err = -ENOMEM;
 		goto err_free_mpi_coredump;
@@ -4702,8 +4700,8 @@ static pci_ers_result_t qlge_io_error_detected(struct pci_dev *pdev,
 		pci_disable_device(pdev);
 		return PCI_ERS_RESULT_NEED_RESET;
 	case pci_channel_io_perm_failure:
-		dev_err(&pdev->dev,
-			"%s: pci_channel_io_perm_failure.\n", __func__);
+		dev_err(&pdev->dev, "%s: pci_channel_io_perm_failure.\n",
+			__func__);
 		del_timer_sync(&qdev->timer);
 		qlge_eeh_close(ndev);
 		set_bit(QL_EEH_FATAL, &qdev->flags);
-- 
2.32.0

Re: [RFC 17/19] staging: qlge: fix weird line wrapping

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:49:00PM +0800, Coiby Xu wrote:
> @@ -524,8 +523,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>  		{
>  			value = RT_IDX_DST_DFLT_Q | /* dest */
>  				RT_IDX_TYPE_NICQ | /* type */
> -				(RT_IDX_IP_CSUM_ERR_SLOT <<
> -				RT_IDX_IDX_SHIFT); /* index */
> +			(RT_IDX_IP_CSUM_ERR_SLOT
> +			 << RT_IDX_IDX_SHIFT); /* index */

The original is not great but the new indenting is definitely worse.
It might look nicer with the comments moved in the front?  Why does
RT_IDX_IDX_SHIFT have two IDX strings?

			/* value = dest | type | index; */
			value = RT_IDX_DST_DFLT_Q |
				RT_IDX_TYPE_NICQ  |
				(RT_IDX_IP_CSUM_ERR_SLOT << RT_IDX_IDX_SHIFT);


>  			break;
>  		}
>  	case RT_IDX_TU_CSUM_ERR: /* Pass up TCP/UDP CSUM error frames. */
> @@ -554,7 +553,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>  		{
>  			value = RT_IDX_DST_DFLT_Q |	/* dest */
>  			    RT_IDX_TYPE_NICQ |	/* type */
> -			    (RT_IDX_MCAST_MATCH_SLOT << RT_IDX_IDX_SHIFT);/* index */
> +			(RT_IDX_MCAST_MATCH_SLOT
> +			 << RT_IDX_IDX_SHIFT); /* index */

Original is better.

>  			break;
>  		}
>  	case RT_IDX_RSS_MATCH:	/* Pass up matched RSS frames. */
> @@ -648,15 +648,15 @@ static int qlge_read_flash_word(struct qlge_adapter *qdev, int offset, __le32 *d
>  {
>  	int status = 0;
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
> +				   FLASH_ADDR_ERR);
>  	if (status)
>  		goto exit;
>  	/* set up for reg read */
>  	qlge_write32(qdev, FLASH_ADDR, FLASH_ADDR_R | offset);
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
> +				   FLASH_ADDR_ERR);
>  	if (status)
>  		goto exit;
>  	/* This data is stored on flash as an array of
> @@ -792,8 +792,8 @@ static int qlge_write_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 data)
>  {
>  	int status;
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> +				   XGMAC_ADDR_XME);
>  	if (status)
>  		return status;
>  	/* write the data to the data reg */
> @@ -811,15 +811,15 @@ int qlge_read_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 *data)
>  {
>  	int status = 0;
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> +				   XGMAC_ADDR_XME);

Need a blank line after the declaration block.

>  	if (status)
>  		goto exit;
>  	/* set up for reg read */
>  	qlge_write32(qdev, XGMAC_ADDR, reg | XGMAC_ADDR_R);
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> +				   XGMAC_ADDR_XME);
>  	if (status)
>  		goto exit;
>  	/* get the data */

regards,
dan carpenter

Re: [RFC 17/19] staging: qlge: fix weird line wrapping

[Dan Carpenter]

On Mon, Jun 21, 2021 at 09:49:00PM +0800, Coiby Xu wrote:
> @@ -524,8 +523,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>  		{
>  			value = RT_IDX_DST_DFLT_Q | /* dest */
>  				RT_IDX_TYPE_NICQ | /* type */
> -				(RT_IDX_IP_CSUM_ERR_SLOT <<
> -				RT_IDX_IDX_SHIFT); /* index */
> +			(RT_IDX_IP_CSUM_ERR_SLOT
> +			 << RT_IDX_IDX_SHIFT); /* index */

The original is not great but the new indenting is definitely worse.
It might look nicer with the comments moved in the front?  Why does
RT_IDX_IDX_SHIFT have two IDX strings?

			/* value = dest | type | index; */
			value = RT_IDX_DST_DFLT_Q |
				RT_IDX_TYPE_NICQ  |
				(RT_IDX_IP_CSUM_ERR_SLOT << RT_IDX_IDX_SHIFT);


>  			break;
>  		}
>  	case RT_IDX_TU_CSUM_ERR: /* Pass up TCP/UDP CSUM error frames. */
> @@ -554,7 +553,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>  		{
>  			value = RT_IDX_DST_DFLT_Q |	/* dest */
>  			    RT_IDX_TYPE_NICQ |	/* type */
> -			    (RT_IDX_MCAST_MATCH_SLOT << RT_IDX_IDX_SHIFT);/* index */
> +			(RT_IDX_MCAST_MATCH_SLOT
> +			 << RT_IDX_IDX_SHIFT); /* index */

Original is better.

>  			break;
>  		}
>  	case RT_IDX_RSS_MATCH:	/* Pass up matched RSS frames. */
> @@ -648,15 +648,15 @@ static int qlge_read_flash_word(struct qlge_adapter *qdev, int offset, __le32 *d
>  {
>  	int status = 0;
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
> +				   FLASH_ADDR_ERR);
>  	if (status)
>  		goto exit;
>  	/* set up for reg read */
>  	qlge_write32(qdev, FLASH_ADDR, FLASH_ADDR_R | offset);
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
> +				   FLASH_ADDR_ERR);
>  	if (status)
>  		goto exit;
>  	/* This data is stored on flash as an array of
> @@ -792,8 +792,8 @@ static int qlge_write_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 data)
>  {
>  	int status;
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> +				   XGMAC_ADDR_XME);
>  	if (status)
>  		return status;
>  	/* write the data to the data reg */
> @@ -811,15 +811,15 @@ int qlge_read_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 *data)
>  {
>  	int status = 0;
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> +				   XGMAC_ADDR_XME);

Need a blank line after the declaration block.

>  	if (status)
>  		goto exit;
>  	/* set up for reg read */
>  	qlge_write32(qdev, XGMAC_ADDR, reg | XGMAC_ADDR_R);
>  	/* wait for reg to come ready */
> -	status = qlge_wait_reg_rdy(qdev,
> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
> +				   XGMAC_ADDR_XME);
>  	if (status)
>  		goto exit;
>  	/* get the data */

regards,
dan carpenter

Re: [RFC 17/19] staging: qlge: fix weird line wrapping

[Coiby Xu]

On Tue, Jun 22, 2021 at 11:46:11AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:49:00PM +0800, Coiby Xu wrote:
>> @@ -524,8 +523,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>>  		{
>>  			value = RT_IDX_DST_DFLT_Q | /* dest */
>>  				RT_IDX_TYPE_NICQ | /* type */
>> -				(RT_IDX_IP_CSUM_ERR_SLOT <<
>> -				RT_IDX_IDX_SHIFT); /* index */
>> +			(RT_IDX_IP_CSUM_ERR_SLOT
>> +			 << RT_IDX_IDX_SHIFT); /* index */
>
>The original is not great but the new indenting is definitely worse.
>It might look nicer with the comments moved in the front?  Why does
>RT_IDX_IDX_SHIFT have two IDX strings?

I'm not sure about it. Two IDX strings seems to be a typo.
>
>			/* value = dest | type | index; */
>			value = RT_IDX_DST_DFLT_Q |
>				RT_IDX_TYPE_NICQ  |
>				(RT_IDX_IP_CSUM_ERR_SLOT << RT_IDX_IDX_SHIFT);
>

This looks better! Thanks!

>
>>  			break;
>>  		}
>>  	case RT_IDX_TU_CSUM_ERR: /* Pass up TCP/UDP CSUM error frames. */
>> @@ -554,7 +553,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>>  		{
>>  			value = RT_IDX_DST_DFLT_Q |	/* dest */
>>  			    RT_IDX_TYPE_NICQ |	/* type */
>> -			    (RT_IDX_MCAST_MATCH_SLOT << RT_IDX_IDX_SHIFT);/* index */
>> +			(RT_IDX_MCAST_MATCH_SLOT
>> +			 << RT_IDX_IDX_SHIFT); /* index */
>
>Original is better.

I'll also move the comments in the front.
>
>>  			break;
>>  		}
>>  	case RT_IDX_RSS_MATCH:	/* Pass up matched RSS frames. */
>> @@ -648,15 +648,15 @@ static int qlge_read_flash_word(struct qlge_adapter *qdev, int offset, __le32 *d
>>  {
>>  	int status = 0;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
>> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
>> +				   FLASH_ADDR_ERR);
>>  	if (status)
>>  		goto exit;
>>  	/* set up for reg read */
>>  	qlge_write32(qdev, FLASH_ADDR, FLASH_ADDR_R | offset);
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
>> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
>> +				   FLASH_ADDR_ERR);
>>  	if (status)
>>  		goto exit;
>>  	/* This data is stored on flash as an array of
>> @@ -792,8 +792,8 @@ static int qlge_write_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 data)
>>  {
>>  	int status;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>>  	if (status)
>>  		return status;
>>  	/* write the data to the data reg */
>> @@ -811,15 +811,15 @@ int qlge_read_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 *data)
>>  {
>>  	int status = 0;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>
>Need a blank line after the declaration block.

Sure, I will fix it in next version.

>
>>  	if (status)
>>  		goto exit;
>>  	/* set up for reg read */
>>  	qlge_write32(qdev, XGMAC_ADDR, reg | XGMAC_ADDR_R);
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>>  	if (status)
>>  		goto exit;
>>  	/* get the data */
>
>regards,
>dan carpenter

-- 
Best regards,
Coiby

Re: [RFC 17/19] staging: qlge: fix weird line wrapping

[Coiby Xu]

On Tue, Jun 22, 2021 at 11:46:11AM +0300, Dan Carpenter wrote:
>On Mon, Jun 21, 2021 at 09:49:00PM +0800, Coiby Xu wrote:
>> @@ -524,8 +523,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>>  		{
>>  			value = RT_IDX_DST_DFLT_Q | /* dest */
>>  				RT_IDX_TYPE_NICQ | /* type */
>> -				(RT_IDX_IP_CSUM_ERR_SLOT <<
>> -				RT_IDX_IDX_SHIFT); /* index */
>> +			(RT_IDX_IP_CSUM_ERR_SLOT
>> +			 << RT_IDX_IDX_SHIFT); /* index */
>
>The original is not great but the new indenting is definitely worse.
>It might look nicer with the comments moved in the front?  Why does
>RT_IDX_IDX_SHIFT have two IDX strings?

I'm not sure about it. Two IDX strings seems to be a typo.
>
>			/* value = dest | type | index; */
>			value = RT_IDX_DST_DFLT_Q |
>				RT_IDX_TYPE_NICQ  |
>				(RT_IDX_IP_CSUM_ERR_SLOT << RT_IDX_IDX_SHIFT);
>

This looks better! Thanks!

>
>>  			break;
>>  		}
>>  	case RT_IDX_TU_CSUM_ERR: /* Pass up TCP/UDP CSUM error frames. */
>> @@ -554,7 +553,8 @@ static int qlge_set_routing_reg(struct qlge_adapter *qdev, u32 index, u32 mask,
>>  		{
>>  			value = RT_IDX_DST_DFLT_Q |	/* dest */
>>  			    RT_IDX_TYPE_NICQ |	/* type */
>> -			    (RT_IDX_MCAST_MATCH_SLOT << RT_IDX_IDX_SHIFT);/* index */
>> +			(RT_IDX_MCAST_MATCH_SLOT
>> +			 << RT_IDX_IDX_SHIFT); /* index */
>
>Original is better.

I'll also move the comments in the front.
>
>>  			break;
>>  		}
>>  	case RT_IDX_RSS_MATCH:	/* Pass up matched RSS frames. */
>> @@ -648,15 +648,15 @@ static int qlge_read_flash_word(struct qlge_adapter *qdev, int offset, __le32 *d
>>  {
>>  	int status = 0;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
>> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
>> +				   FLASH_ADDR_ERR);
>>  	if (status)
>>  		goto exit;
>>  	/* set up for reg read */
>>  	qlge_write32(qdev, FLASH_ADDR, FLASH_ADDR_R | offset);
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   FLASH_ADDR, FLASH_ADDR_RDY, FLASH_ADDR_ERR);
>> +	status = qlge_wait_reg_rdy(qdev, FLASH_ADDR, FLASH_ADDR_RDY,
>> +				   FLASH_ADDR_ERR);
>>  	if (status)
>>  		goto exit;
>>  	/* This data is stored on flash as an array of
>> @@ -792,8 +792,8 @@ static int qlge_write_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 data)
>>  {
>>  	int status;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>>  	if (status)
>>  		return status;
>>  	/* write the data to the data reg */
>> @@ -811,15 +811,15 @@ int qlge_read_xgmac_reg(struct qlge_adapter *qdev, u32 reg, u32 *data)
>>  {
>>  	int status = 0;
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>
>Need a blank line after the declaration block.

Sure, I will fix it in next version.

>
>>  	if (status)
>>  		goto exit;
>>  	/* set up for reg read */
>>  	qlge_write32(qdev, XGMAC_ADDR, reg | XGMAC_ADDR_R);
>>  	/* wait for reg to come ready */
>> -	status = qlge_wait_reg_rdy(qdev,
>> -				   XGMAC_ADDR, XGMAC_ADDR_RDY, XGMAC_ADDR_XME);
>> +	status = qlge_wait_reg_rdy(qdev, XGMAC_ADDR, XGMAC_ADDR_RDY,
>> +				   XGMAC_ADDR_XME);
>>  	if (status)
>>  		goto exit;
>>  	/* get the data */
>
>regards,
>dan carpenter

-- 
Best regards,
Coiby

[RFC 18/19] staging: qlge: fix two indentation issues

[Coiby Xu]

Fix two indentation issues.

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO        |  1 -
 drivers/staging/qlge/qlge_dbg.c  | 30 +++++++++++++++---------------
 drivers/staging/qlge/qlge_main.c |  4 ++--
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index b8def0c70614..7e466a0f7771 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -1,5 +1,4 @@
 * the driver has a habit of using runtime checks where compile time checks are
   possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
 * remove duplicate and useless comments
-* fix weird indentation (all over, ex. the for loops in qlge_get_stats())
 * fix checkpatch issues
diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index d093e6c9f19c..d4d486f99549 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -353,21 +353,21 @@ static int qlge_get_xgmac_regs(struct qlge_adapter *qdev, u32 *buf,
 		 */
 		if ((i == 0x00000114) ||
 		    (i == 0x00000118) ||
-			(i == 0x0000013c) ||
-			(i == 0x00000140) ||
-			(i > 0x00000150 && i < 0x000001fc) ||
-			(i > 0x00000278 && i < 0x000002a0) ||
-			(i > 0x000002c0 && i < 0x000002cf) ||
-			(i > 0x000002dc && i < 0x000002f0) ||
-			(i > 0x000003c8 && i < 0x00000400) ||
-			(i > 0x00000400 && i < 0x00000410) ||
-			(i > 0x00000410 && i < 0x00000420) ||
-			(i > 0x00000420 && i < 0x00000430) ||
-			(i > 0x00000430 && i < 0x00000440) ||
-			(i > 0x00000440 && i < 0x00000450) ||
-			(i > 0x00000450 && i < 0x00000500) ||
-			(i > 0x0000054c && i < 0x00000568) ||
-			(i > 0x000005c8 && i < 0x00000600)) {
+		    (i == 0x0000013c) ||
+		    (i == 0x00000140) ||
+		    (i > 0x00000150 && i < 0x000001fc) ||
+		    (i > 0x00000278 && i < 0x000002a0) ||
+		    (i > 0x000002c0 && i < 0x000002cf) ||
+		    (i > 0x000002dc && i < 0x000002f0) ||
+		    (i > 0x000003c8 && i < 0x00000400) ||
+		    (i > 0x00000400 && i < 0x00000410) ||
+		    (i > 0x00000410 && i < 0x00000420) ||
+		    (i > 0x00000420 && i < 0x00000430) ||
+		    (i > 0x00000430 && i < 0x00000440) ||
+		    (i > 0x00000440 && i < 0x00000450) ||
+		    (i > 0x00000450 && i < 0x00000500) ||
+		    (i > 0x0000054c && i < 0x00000568) ||
+		    (i > 0x000005c8 && i < 0x00000600)) {
 			if (other_function)
 				status =
 				qlge_read_other_func_xgmac_reg(qdev, i, buf);
diff --git a/drivers/staging/qlge/qlge_main.c b/drivers/staging/qlge/qlge_main.c
index 21fb942c2595..7cec2d6c3fea 100644
--- a/drivers/staging/qlge/qlge_main.c
+++ b/drivers/staging/qlge/qlge_main.c
@@ -699,8 +699,8 @@ static int qlge_get_8000_flash_params(struct qlge_adapter *qdev)
 
 	status = qlge_validate_flash(qdev,
 				     sizeof(struct flash_params_8000) /
-				   sizeof(u16),
-				   "8000");
+				       sizeof(u16),
+				     "8000");
 	if (status) {
 		netif_err(qdev, ifup, qdev->ndev, "Invalid flash.\n");
 		status = -EINVAL;
-- 
2.32.0

[RFC 19/19] staging: qlge: remove TODO item of unnecessary runtime checks

[Coiby Xu]

The following commits [1],

- e4c911a73c89 ("staging: qlge: Remove rx_ring.type")
- a68a5b2fd3a2 ("staging: qlge: Remove bq_desc.maplen")
- 16714d98bf63 ("staging: qlge: Remove rx_ring.sbq_buf_size")
- ec705b983b46 ("staging: qlge: Remove qlge_bq.len & size")

and recent "commit a0e57b58d35d3d6808187bb10ee9e5030ff87618
("staging: qlge: the number of pages to contain a buffer queue is
 constant") has fixed issue. Thus remove the TODO item.

[1] https://lore.kernel.org/netdev/YJeUZo+zoNZmFuKs@f3/

Signed-off-by: Coiby Xu <coiby.xu@gmail.com>
---
 drivers/staging/qlge/TODO | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index 7e466a0f7771..0e349ffc630e 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO
@@ -1,4 +1,2 @@
-* the driver has a habit of using runtime checks where compile time checks are
-  possible (ex. ql_free_rx_buffers(), ql_alloc_rx_buffers())
 * remove duplicate and useless comments
 * fix checkpatch issues
-- 
2.32.0