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