Follow the kernel type preferrences of using 's16' over 'int16_t'.
Signed-off-by: Shiva Kerdel <[email protected]>
Acked-by: Stuart Yoder <[email protected]>
---
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
Follow the kernel type preferrences of using 's32' over 'int32_t'.
Signed-off-by: Shiva Kerdel <[email protected]>
Acked-by: Stuart Yoder <[email protected]>
---
Changes for v2:
- None
Changes for v3:
- None
Changes for v4:
- corrected patch version number.
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
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 <[email protected]>
> Acked-by: Stuart Yoder <[email protected]>
> ---
> 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
On Fri, Nov 11, 2016 at 12:07:40PM +0100, Shiva Kerdel wrote:
> Follow the kernel type preferrences of using 's32' over 'int32_t'.
>
> Signed-off-by: Shiva Kerdel <[email protected]>
> Acked-by: Stuart Yoder <[email protected]>
> ---
> Changes for v2:
> - None
> Changes for v3:
> - None
> Changes for v4:
> - corrected patch version number.
>
> 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;
Just use "int".
regards,
dan carpenter
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Friday, November 11, 2016 5:23 AM
> To: Shiva Kerdel <[email protected]>
> Cc: Stuart Yoder <[email protected]>; [email protected]; [email protected];
> [email protected]; Nipun Gupta <[email protected]>; [email protected]; German
> Rivera <[email protected]>; [email protected]; [email protected]
> 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 <[email protected]>
> > Acked-by: Stuart Yoder <[email protected]>
> > ---
> > 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
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
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Monday, November 14, 2016 4:06 AM
> To: Stuart Yoder <[email protected]>
> Cc: Shiva Kerdel <[email protected]>; [email protected]; [email protected]; linux-
> [email protected]; Nipun Gupta <[email protected]>; [email protected]; Laurentiu Tudor
> <[email protected]>
> 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
>> -----Original Message-----
>> From: Dan Carpenter [mailto:[email protected]]
>> Sent: Monday, November 14, 2016 4:06 AM
>> To: Stuart Yoder <[email protected]>
>> Cc: Shiva Kerdel <[email protected]>; [email protected]; [email protected]; linux-
>> [email protected]; Nipun Gupta <[email protected]>; [email protected]; Laurentiu Tudor
>> <[email protected]>
>> 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