Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 24 Jun 2021 15:49:26 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     Coiby Xu <coiby.xu@gmail.com>
Cc:     linux-staging@lists.linux.dev, netdev@vger.kernel.org,
        Benjamin Poirier <benjamin.poirier@gmail.com>,
        Shung-Hsi Yu <shung-hsi.yu@suse.com>,
        Manish Chopra <manishc@marvell.com>,
        "supporter:QLOGIC QLGE 10Gb ETHERNET DRIVER" 
        <GR-Linux-NIC-Dev@marvell.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [RFC 16/19] staging: qlge: remove deadcode in qlge_build_rx_skb
Message-ID: <20210624124926.GI1983@kadam>
References: <20210621134902.83587-1-coiby.xu@gmail.com>
 <20210621134902.83587-17-coiby.xu@gmail.com>
 <20210622072939.GL1861@kadam>
 <20210624112500.rhtqp7j3odq6b6bq@Rk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210624112500.rhtqp7j3odq6b6bq@Rk>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

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