[PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Shiva Kerdel]

Follow the kernel type preferrences of using 's32' over 'int16_t'.

Signed-off-by: Shiva Kerdel <shiva@exdev.nl>
Acked-by: Stuart Yoder <stuart.yoder@nxp.com>
---
 drivers/staging/fsl-mc/include/mc-bus.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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;
+	s16 free_count;
 	struct mutex mutex;	/* serializes access to free_list */
 	struct list_head free_list;
 	struct fsl_mc_bus *mc_bus;
-- 
2.10.2

[PATCH 2/2] Staging: fsl-mc: include: mc: Kernel type 's32' preferred over 'int32_t'

[Shiva Kerdel]

Follow the kernel type preferrences of using 's32' over 'int32_t'.

Signed-off-by: Shiva Kerdel <shiva@exdev.nl>
Acked-by: Stuart Yoder <stuart.yoder@nxp.com>
---
 drivers/staging/fsl-mc/include/mc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-mc/include/mc.h b/drivers/staging/fsl-mc/include/mc.h
index a781a36..1c46c0c 100644
--- a/drivers/staging/fsl-mc/include/mc.h
+++ b/drivers/staging/fsl-mc/include/mc.h
@@ -81,7 +81,7 @@ enum fsl_mc_pool_type {
  */
 struct fsl_mc_resource {
 	enum fsl_mc_pool_type type;
-	int32_t id;
+	s32 id;
 	void *data;
 	struct fsl_mc_resource_pool *parent_pool;
 	struct list_head node;
-- 
2.10.2

Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Shiva Kerdel]

>> -----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

RE: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Stuart Yoder]

> -----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

Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Dan Carpenter]

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

RE: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Stuart Yoder]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Friday, November 11, 2016 5:23 AM
> To: Shiva Kerdel <shiva@exdev.nl>
> Cc: Stuart Yoder <stuart.yoder@nxp.com>; devel@driverdev.osuosl.org; German.Rivera@freescale.com;
> gregkh@linuxfoundation.org; Nipun Gupta <nipun.gupta@nxp.com>; linux-kernel@vger.kernel.org; German
> Rivera <german.rivera@nxp.com>; treding@nvidia.com; itai.katz@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 12:07:39PM +0100, Shiva Kerdel wrote:
> > Follow the kernel type preferrences of using 's16' over 'int16_t'.
> >
> > Signed-off-by: Shiva Kerdel <shiva@exdev.nl>
> > Acked-by: Stuart Yoder <stuart.yoder@nxp.com>
> > ---
> > Changes for v2:
> >     - corrected an error in the log message, wrote 's32' instead of 's16'.
> > Changes for v3:
> >     - added the missing annotates.
> > Changes for v4:
> >     - corrected patch subject to version 4.
> >
> >  drivers/staging/fsl-mc/include/mc-bus.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > 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?

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.

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.

Thanks,
Stuart

Re: [PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Dan Carpenter]

On Fri, Nov 11, 2016 at 12:07:39PM +0100, Shiva Kerdel wrote:
> Follow the kernel type preferrences of using 's16' over 'int16_t'.
> 
> Signed-off-by: Shiva Kerdel <shiva@exdev.nl>
> Acked-by: Stuart Yoder <stuart.yoder@nxp.com>
> ---
> Changes for v2:
>     - corrected an error in the log message, wrote 's32' instead of 's16'.
> Changes for v3:
>     - added the missing annotates.
> Changes for v4:
>     - corrected patch subject to version 4.
> 
>  drivers/staging/fsl-mc/include/mc-bus.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 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.

regards,
dan carpenter

[PATCH 1/2] Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

[Shiva Kerdel]

Follow the kernel type preferrences of using 's16' over 'int16_t'.

Signed-off-by: Shiva Kerdel <shiva@exdev.nl>
Acked-by: Stuart Yoder <stuart.yoder@nxp.com>
---
Changes for v2:
    - corrected an error in the log message, wrote 's32' instead of 's16'.
Changes for v3:
    - added the missing annotates.
Changes for v4:
    - corrected patch subject to version 4.

 drivers/staging/fsl-mc/include/mc-bus.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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;
+	s16 free_count;
 	struct mutex mutex;	/* serializes access to free_list */
 	struct list_head free_list;
 	struct fsl_mc_bus *mc_bus;
-- 
2.10.2