From: Stuart Yoder <stuart.yoder@nxp.com>
To: Dan Carpenter <dan.carpenter@oracle.com>,
        Shiva Kerdel <shiva@exdev.nl>
CC: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Nipun Gupta <nipun.gupta@nxp.com>,
        "treding@nvidia.com" <treding@nvidia.com>,
        "Laurentiu Tudor" <laurentiu.tudor@nxp.com>
Subject: RE: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16'
 preferred over 'int16_t'
Thread-Topic: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16'
 preferred over 'int16_t'
Thread-Index: AQHSPAvfdnmnRhb0FUi9htvgspyz2aDTo/IAgAA0wlCABGyFAIAATIMw
Date: Mon, 14 Nov 2016 14:55:06 +0000
Message-ID: <VI1PR0401MB2638835D7D17D746BBA0CD4A8DBC0@VI1PR0401MB2638.eurprd04.prod.outlook.com>
References: <20161111110740.1926-1-shiva@exdev.nl>
 <20161111112326.GG28701@mwanda>
 <VI1PR0401MB26383A9D15287D79B790BAD38DBB0@VI1PR0401MB2638.eurprd04.prod.outlook.com>
 <20161114100447.GJ28701@mwanda>
In-Reply-To: <20161114100447.GJ28701@mwanda>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Nov 2016 14:55:06.0897
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0401MB1856
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAEFBBLv020439
Content-Length: 3005
Lines: 77


> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Monday, November 14, 2016 4:06 AM
> To: Stuart Yoder <stuart.yoder@nxp.com>
> Cc: Shiva Kerdel <shiva@exdev.nl>; devel@driverdev.osuosl.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; Nipun Gupta <nipun.gupta@nxp.com>; treding@nvidia.com; Laurentiu Tudor
> <laurentiu.tudor@nxp.com>
> Subject: Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'
> 
> 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.

Ok, agree that it would be clearer like that.

Shiva, can you respin this patch and just make both max_count and free_count
to be of type "int".

I will get Dan's suggestion sent as a separate patch...to #define the upper bound
instead of relying on integer overflow.

Thanks,
Stuart