Messages in this thread | | | Subject | Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t' | From | Shiva Kerdel <> | Date | Tue, 15 Nov 2016 08:54:16 +0100 |
| |
>> -----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 I will do that, thank you for the clarification of what I should do.
Thanks, Shiva Kerdel
| |