From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751224AbdFCQxl (ORCPT ); Sat, 3 Jun 2017 12:53:41 -0400 Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:5708 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750991AbdFCQxk (ORCPT ); Sat, 3 Jun 2017 12:53:40 -0400 X-IronPort-AV: E=Sophos;i="5.39,291,1493676000"; d="scan'208";a="227199083" Date: Sat, 3 Jun 2017 18:53:36 +0200 (CEST) From: Julia Lawall X-X-Sender: jll@hadrien To: Colin Ian King cc: Andy Shevchenko , Giuseppe Cavallaro , Alexandre Torgue , netdev , kernel-janitors@vger.kernel.org, "linux-kernel@vger.kernel.org" Subject: Re: [PATCH] net: stmmac: ensure jumbo_frm error return is correctly checked for -ve value In-Reply-To: <40331b09-5829-9041-f815-7f1426960bd8@canonical.com> Message-ID: References: <20170602145827.21151-1-colin.king@canonical.com> <40331b09-5829-9041-f815-7f1426960bd8@canonical.com> User-Agent: Alpine 2.20 (DEB 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 3 Jun 2017, Colin Ian King wrote: > On 03/06/17 16:55, Andy Shevchenko wrote: > > On Fri, Jun 2, 2017 at 5:58 PM, Colin King wrote: > >> The current comparison of entry < 0 will never be true since entry is an > >> unsigned integer. Cast entry to an int to ensure -ve error return values > >> from the call to jumbo_frm are correctly being caught. > > > >> if (unlikely(is_jumbo) && likely(priv->synopsys_id < > >> DWMAC_CORE_4_00)) { > >> entry = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion); > >> - if (unlikely(entry < 0)) > >> + if (unlikely((int)entry < 0)) > > > > It feels like a hiding some other issue. > > > > The alternative is: > > int rc = priv->hw->mode->jumbo_frm(tx_q, skb, csum_insertion); > if (unlikely(rc < 0)) > goto dma_map_err; > > entry = rc; > > however, that is effectively the same. The cast I'm using is a well used > idiom in the kernel, it used in almost a hundred similar cases. > > git grep "< 0" | grep "(int)" | wc -l > 95 Does entry really have to be unsigned? The jumbo_frm function returns an int, not an unsigned int, so it seems unpleasant to make it unsigned prematurely just to put a cast afterwards. The remaining computation seems to involve only small numbers. julia