Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Shiva Kerdel]

> > -----Original Message-----
> > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx]
> > Sent: Monday, November 14, 2016 4:06 AM
> > To: Stuart Yoder <stuart.yoder@xxxxxxx>
> > Cc: Shiva Kerdel <shiva@xxxxxxxx>; devel@xxxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx; Nipun Gupta <nipun.gupta@xxxxxxx>; treding@xxxxxxxxxx; Laurentiu Tudor
> > <laurentiu.tudor@xxxxxxx>
> > 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
I will do that, thank you for the clarification of what I should do.

Thanks,
Shiva Kerdel