Staging: fsl-mc: include: mc: Kernel type 's16' preferred over 'int16_t'

PATCH In review Shiva Kerdel 3 participants 8 messages Nov 11, 2016 – Nov 15, 2016 Staging

lore

Share & tools

lore

Shiva Kerdel

author

2016-11-11 11:08 UTC #0 lore raw

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

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

drivers/staging/fsl-mc/include/mc-bus.h +2 -2

index e915574..c7cad87 100644
@@ -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;

Signature · 1 line

2.10.2

Shiva Kerdel

author

2016-11-11 11:08 UTC #1 lore raw

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

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

drivers/staging/fsl-mc/include/mc.h +1 -1

index a781a36..1c46c0c 100644
@@ -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;

Signature · 1 line

2.10.2

Dan Carpenter

2016-11-11 11:24 UTC #3 lore raw

In reply to Shiva Kerdel

Quoted 20 lines · Shiva Kerdel

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(-)
drivers/staging/fsl-mc/include/mc-bus.h +0 -0
index e915574..c7cad87 100644

drivers/staging/fsl-mc/include/mc-bus.h struct msi_domain_info; · line 45

Quoted 6 lines

drivers/staging/fsl-mc/include/mc-bus.h +1 -2

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

Stuart Yoder

2016-11-11 16:26 UTC #4 lore raw

Quoted 45 lines

-----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'
Quoted 20 lines · Shiva Kerdel
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(-)
drivers/staging/fsl-mc/include/mc-bus.h +0 -0
index e915574..c7cad87 100644
drivers/staging/fsl-mc/include/mc-bus.h struct msi_domain_info; · line 45
Quoted 6 lines
drivers/staging/fsl-mc/include/mc-bus.h +1 -2
@@ -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.

Quoted 1 line

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

Dan Carpenter

2016-11-14 10:08 UTC #5 lore raw

Quoted 22 lines · Stuart Yoder

Quoted 18 lines
drivers/staging/fsl-mc/include/mc-bus.h struct msi_domain_info; · line 45
Quoted 11 lines
drivers/staging/fsl-mc/include/mc-bus.h +1 -2
index e915574..c7cad87 100644
@@ -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.
Quoted 1 line
This all seems pretty horrible.
Can you elaborate?

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

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

Stuart Yoder

2016-11-14 15:11 UTC #6 lore raw

Quoted 56 lines

-----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'
Quoted 22 lines · Stuart Yoder
Quoted 18 lines
drivers/staging/fsl-mc/include/mc-bus.h struct msi_domain_info; · line 45
Quoted 11 lines
drivers/staging/fsl-mc/include/mc-bus.h +1 -2
index e915574..c7cad87 100644
@@ -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.
Quoted 1 line
This all seems pretty horrible.
Can you elaborate?
Quoted 8 lines
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.
Quoted 5 lines
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.
Quoted 1 line

Quoted 8 lines

Quoted 5 lines
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

Shiva Kerdel

author

2016-11-15 07:54 UTC #7 lore raw

Quoted 58 lines

Quoted 49 lines
-----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'
Quoted 20 lines · Stuart Yoder
Quoted 17 lines
drivers/staging/fsl-mc/include/mc-bus.h struct msi_domain_info; · line 45
Quoted 11 lines
drivers/staging/fsl-mc/include/mc-bus.h +1 -2
index e915574..c7cad87 100644
@@ -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.
Quoted 1 line
This all seems pretty horrible.
Can you elaborate?
Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
Quoted 5 lines
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.

Quoted 8 lines

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