Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755542AbcKOHyh (ORCPT ); Tue, 15 Nov 2016 02:54:37 -0500 Received: from web01.01d.eu ([5.200.27.195]:37568 "EHLO web01.01d.eu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751668AbcKOHyg (ORCPT ); Tue, 15 Nov 2016 02:54:36 -0500 Subject: Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t' To: Stuart Yoder , Dan Carpenter References: <20161111110740.1926-1-shiva@exdev.nl> <20161111112326.GG28701@mwanda> <20161114100447.GJ28701@mwanda> Cc: "devel@driverdev.osuosl.org" , "gregkh@linuxfoundation.org" , "linux-kernel@vger.kernel.org" , Nipun Gupta , "treding@nvidia.com" , Laurentiu Tudor From: Shiva Kerdel Message-ID: <9ff02314-4820-b270-7477-e67fbf41639d@exdev.nl> Date: Tue, 15 Nov 2016 08:54:16 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated-Id: shiva@exdev.nl Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3071 Lines: 70 >> -----Original Message----- >> From: Dan Carpenter [mailto:dan.carpenter@oracle.com] >> Sent: Monday, November 14, 2016 4:06 AM >> To: Stuart Yoder >> Cc: Shiva Kerdel ; devel@driverdev.osuosl.org; gregkh@linuxfoundation.org; linux- >> kernel@vger.kernel.org; Nipun Gupta ; treding@nvidia.com; Laurentiu Tudor >> >> 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