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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
