| Message ID | 20210621134902.83587-17-coiby.xu@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | Improve the qlge driver based on drivers/staging/qlge/TODO | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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
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 >
*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 >
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
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(-)