From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752923AbcGGPv6 (ORCPT ); Thu, 7 Jul 2016 11:51:58 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:48643 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751117AbcGGPvt (ORCPT ); Thu, 7 Jul 2016 11:51:49 -0400 Date: Thu, 7 Jul 2016 08:51:02 -0700 From: Guenter Roeck To: Arvind Yadav Cc: scottwood@freescale.com, qiang.zhao@freescale.com, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: Remove lots of IS_ERR_VALUE abuses and compilation warning. Message-ID: <20160707155102.GA5107@roeck-us.net> References: <1467905249-3990-1-git-send-email-arvind.yadav.cs@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1467905249-3990-1-git-send-email-arvind.yadav.cs@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 07, 2016 at 08:57:29PM +0530, Arvind Yadav wrote: > Passing value in IS_ERR_VALUE() is wrong, as they > pass an 'int' into a function that takes an 'unsigned long' > argument. This happens to work because the type is sign-extended > on 64-bit architectures before it gets converted into an > unsigned type. > > Passing an 'unsigned short' or 'unsigned int' > argument into IS_ERR_VALUE() is guaranteed to be broken, as are > 8-bit integers and types that are wider than 'unsigned long'. > > Any user will get compilation warning for that do not pass an > 'unsigned long' argument. > > Commit '287980e49f; - This change is alreday fixes lots of other > worst abusers. > Couple of generic comments: - Your patch subject lines don't include the affected drivers/modules. As such, most of them will be ignored because maintainers won't realize that you are talking with them. Some may ask you to resubmit with proper subject lines. Commit 287980e49f is different; it addresses the problem in several drivers in a single commit. - If you patch a single file, I think it would be better to adjust the description accordingly. In this patch, the offending variable type is u32. The patch description is therefore misleading; the code here simply does not work. - When you resubmit a patch, you don't include a version, not a change log. This means additional work for maintainers, who have to figure out which patch to apply. Specific comment: The allocator in question returns -ENOMEM in an unsigned long. This is assigned to u32. A proper fix would be to assign the return value to an unsigned long and to use IS_ER_VALUE() to check if it reports an error, and to only assign it to ucc_fast_rx_virtual_fifo_base_offset if there was no error. Also, unless I am missing something - since ucc_fast_rx_virtual_fifo_base_offset is defined as u32, it is somewhat unlikely that it is ever < 0. Guenter > Signed-off-by: Arvind Yadav > --- > drivers/soc/fsl/qe/ucc_fast.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/fsl/qe/ucc_fast.c b/drivers/soc/fsl/qe/ucc_fast.c > index a768931..7cc783c 100644 > --- a/drivers/soc/fsl/qe/ucc_fast.c > +++ b/drivers/soc/fsl/qe/ucc_fast.c > @@ -268,7 +268,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc > /* Allocate memory for Tx Virtual Fifo */ > uccf->ucc_fast_tx_virtual_fifo_base_offset = > qe_muram_alloc(uf_info->utfs, UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT); > - if (IS_ERR_VALUE(uccf->ucc_fast_tx_virtual_fifo_base_offset)) { > + if (uccf->ucc_fast_tx_virtual_fifo_base_offset < 0) { > printk(KERN_ERR "%s: cannot allocate MURAM for TX FIFO\n", > __func__); > uccf->ucc_fast_tx_virtual_fifo_base_offset = 0; > @@ -281,7 +281,7 @@ int ucc_fast_init(struct ucc_fast_info * uf_info, struct ucc_fast_private ** ucc > qe_muram_alloc(uf_info->urfs + > UCC_FAST_RECEIVE_VIRTUAL_FIFO_SIZE_FUDGE_FACTOR, > UCC_FAST_VIRT_FIFO_REGS_ALIGNMENT); > - if (IS_ERR_VALUE(uccf->ucc_fast_rx_virtual_fifo_base_offset)) { > + if (uccf->ucc_fast_rx_virtual_fifo_base_offset < 0) { > printk(KERN_ERR "%s: cannot allocate MURAM for RX FIFO\n", > __func__); > uccf->ucc_fast_rx_virtual_fifo_base_offset = 0;