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

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:

quoted

On Thu, Jun 24, 2021 at 07:25:00PM +0800, Coiby Xu wrote:

quoted

On Tue, Jun 22, 2021 at 10:29:39AM +0300, Dan Carpenter wrote:

quoted

On Mon, Jun 21, 2021 at 09:48:59PM +0800, Coiby Xu wrote:

quoted

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