-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx]
Sent: Monday, November 14, 2016 4:06 AM
To: Stuart Yoder <stuart.yoder@xxxxxxx>
Cc: Shiva Kerdel <shiva@xxxxxxxx>; devel@xxxxxxxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
kernel@xxxxxxxxxxxxxxx; Nipun Gupta <nipun.gupta@xxxxxxx>; treding@xxxxxxxxxx; Laurentiu Tudor
<laurentiu.tudor@xxxxxxx>
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
