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 <[email protected]>
---
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
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
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
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
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
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
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
*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
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