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.carpen...@oracle.com]
Sent: Monday, November 14, 2016 4:06 AM
To: Stuart Yoder <stuart.yo...@nxp.com>
Cc: Shiva Kerdel <sh...@exdev.nl>; de...@driverdev.osuosl.org; 
gre...@linuxfoundation.org; linux-
ker...@vger.kernel.org; Nipun Gupta <nipun.gu...@nxp.com>; tred...@nvidia.com; 
Laurentiu Tudor
<laurentiu.tu...@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 +, 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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.carpen...@oracle.com]
> Sent: Monday, November 14, 2016 4:06 AM
> To: Stuart Yoder <stuart.yo...@nxp.com>
> Cc: Shiva Kerdel <sh...@exdev.nl>; de...@driverdev.osuosl.org; 
> gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; Nipun Gupta <nipun.gu...@nxp.com>; 
> tred...@nvidia.com; Laurentiu Tudor
> <laurentiu.tu...@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 +, 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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.carpen...@oracle.com]
> Sent: Friday, November 11, 2016 5:23 AM
> To: Shiva Kerdel <sh...@exdev.nl>
> Cc: Stuart Yoder <stuart.yo...@nxp.com>; de...@driverdev.osuosl.org; 
> german.riv...@freescale.com;
> gre...@linuxfoundation.org; Nipun Gupta <nipun.gu...@nxp.com>; 
> linux-ker...@vger.kernel.org; German
> Rivera <german.riv...@nxp.com>; tred...@nvidia.com; itai.k...@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 <sh...@exdev.nl>
> > Acked-by: Stuart Yoder <stuart.yo...@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




___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 
> Acked-by: Stuart Yoder 
> ---
> 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

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel