Messages in this thread | | | From | Stuart Yoder <> | Subject | RE: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t' | Date | Mon, 14 Nov 2016 14:55:06 +0000 |
| |
> -----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
| |