Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753497AbcKNKIo (ORCPT ); Mon, 14 Nov 2016 05:08:44 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:25076 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752395AbcKNKHw (ORCPT ); Mon, 14 Nov 2016 05:07:52 -0500 Date: Mon, 14 Nov 2016 13:05:42 +0300 From: Dan Carpenter To: Stuart Yoder Cc: Shiva Kerdel , "devel@driverdev.osuosl.org" , "gregkh@linuxfoundation.org" , "linux-kernel@vger.kernel.org" , Nipun Gupta , "treding@nvidia.com" , Laurentiu Tudor Subject: Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t' Message-ID: <20161114100447.GJ28701@mwanda> References: <20161111110740.1926-1-shiva@exdev.nl> <20161111112326.GG28701@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2146 Lines: 57 On Fri, Nov 11, 2016 at 02:52:31PM +0000, Stuart Yoder wrote: > > > diff --git a/drivers/staging/fsl-mc/include/mc-bus.h b/drivers/staging/fsl-mc/include/mc-bus.h > > > index e915574..c7cad87 100644 > > > --- a/drivers/staging/fsl-mc/include/mc-bus.h > > > +++ b/drivers/staging/fsl-mc/include/mc-bus.h > > > @@ -42,8 +42,8 @@ struct msi_domain_info; > > > */ > > > struct fsl_mc_resource_pool { > > > enum fsl_mc_pool_type type; > > > - int16_t max_count; > > > - int16_t free_count; > > > + s16 max_count; > > > > My understanding is that this has to be signed because the design of > > this driver is that we keep adding devices until the the counter > > overflows. After that there are a couple tests for > > "if (WARN_ON(res_pool->max_count < 0)) " which prevent the driver from > > working again. > > > > This all seems pretty horrible. > > Can you elaborate? > > The resource pools managed by this driver are populated by hardware objects > discovered when the fsl-mc bus probes a DPRC/container. > > The number of potential objects discovered of a given type is in the hundreds, > so a signed 16-bit number is order of magnitudes larger than anything we will > ever encounter. > > Would you feel better about this if max_count was an int? Yeah. > > The max_count reflects the total number of objects discovered. If that is > exceeded we display a warning, because something is horribly wrong. Nothing > stops working, the allocator simply refuses to add anything else to the > free list. I didn't look at this carefully... Anyway we can't remove devices either. If we just had an upper bound instead of overflowing the s16 then we could still remove devices. > > The only reason max_count is there at all is as an internal check against > bugs and resource leaks. If the driver is being removed and a resource > pool is being freed, max_count must be zero...i.e. all objects should have > been removed. If not, there is a leak somewhere. So, it's a sanity check. > Just use a normal upper bound with a #define instead of an magic number hidden and then disguised as an integer overflow. regards, dan carpenter