Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 21 Jul 2019 11:12:56 +0200
From:   Stefano Brivio <sbrivio@redhat.com>
To:     Hariprasad Kelam <hariprasad.kelam@gmail.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rat_cs: Remove duplicate code
Message-ID: <20190721111256.708f4cfa@elisabeth>
In-Reply-To: <20190720174613.GA31062@hari-Inspiron-1545>
References: <20190720174613.GA31062@hari-Inspiron-1545>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, 20 Jul 2019 23:16:47 +0530
Hariprasad Kelam <hariprasad.kelam@gmail.com> wrote:

> Code is same if translate is true/false in case invalid packet is
> received.So remove else part.
> 
> Issue identified with coccicheck
> 
> Signed-off-by: Hariprasad Kelam <hariprasad.kelam@gmail.com>
> ---
>  drivers/net/wireless/ray_cs.c | 29 ++++++++---------------------
>  1 file changed, 8 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/wireless/ray_cs.c b/drivers/net/wireless/ray_cs.c
> index cf37268..a51bbe7 100644
> --- a/drivers/net/wireless/ray_cs.c
> +++ b/drivers/net/wireless/ray_cs.c
> @@ -2108,29 +2108,16 @@ static void rx_data(struct net_device *dev, struct rcs __iomem *prcs,
>  #endif
>  
>  	if (!sniffer) {
> -		if (translate) {
>  /* TBD length needs fixing for translated header */
> -			if (rx_len < (ETH_HLEN + RX_MAC_HEADER_LENGTH) ||
> -			    rx_len >
> -			    (dev->mtu + RX_MAC_HEADER_LENGTH + ETH_HLEN +
> -			     FCS_LEN)) {
> -				pr_debug(
> -				      "ray_cs invalid packet length %d received\n",
> -				      rx_len);
> -				return;
> -			}
> -		} else { /* encapsulated ethernet */
> -
> -			if (rx_len < (ETH_HLEN + RX_MAC_HEADER_LENGTH) ||
> -			    rx_len >
> -			    (dev->mtu + RX_MAC_HEADER_LENGTH + ETH_HLEN +
> -			     FCS_LEN)) {
> -				pr_debug(
> -				      "ray_cs invalid packet length %d received\n",
> -				      rx_len);
> -				return;
> +		if (rx_len < (ETH_HLEN + RX_MAC_HEADER_LENGTH) ||
> +		    rx_len >
> +		    (dev->mtu + RX_MAC_HEADER_LENGTH + ETH_HLEN +
> +		     FCS_LEN)) {
> +			pr_debug(
> +			      "ray_cs invalid packet length %d received\n",
> +			      rx_len);
> +			return;
>  			}
> -		}

NACK. The TBD comment makes no sense anymore if you remove one of the
branches. Believe me or not, I have one of those cards, a (yes, 22
years old) Buslink Raytheon model 24020. That check needed (for sure)
and needs (maybe) to be fixed.

Besides, patch subject and resulting coding style are also wrong.

-- 
Stefano