[PATCH] staging: vme_user: Change slot number type from int to u32

[PATCH] staging: vme_user: Change slot number type from int to u32

[Riyan Dhiman]

Change the type used for VME slot numbers from int to u32 throughout vme
driver. This modification more accurately represents the nature of slot
numbers which are always non-negative.

The changes include
- Updating variable declarations
- Modifying function signatures and return types

This change imporves type safety, prevents potential issues with sign conversion.

Signed-off-by: Riyan Dhiman <riyandhiman14@gmail.com>
---
 drivers/staging/vme_user/vme.c        | 2 +-
 drivers/staging/vme_user/vme.h        | 4 ++--
 drivers/staging/vme_user/vme_bridge.h | 2 +-
 drivers/staging/vme_user/vme_fake.c   | 2 +-
 drivers/staging/vme_user/vme_tsi148.c | 4 ++--
 drivers/staging/vme_user/vme_user.c   | 2 +-
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vme_user/vme.c b/drivers/staging/vme_user/vme.c
index 42304c9f83a2..aa3be2d7248d 100644
--- a/drivers/staging/vme_user/vme.c
+++ b/drivers/staging/vme_user/vme.c
@@ -1696,7 +1696,7 @@ EXPORT_SYMBOL(vme_lm_free);
  *         or the function is not supported. Hardware specific errors may also
  *         be returned.
  */
-int vme_slot_num(struct vme_dev *vdev)
+u32 vme_slot_num(struct vme_dev *vdev)
 {
 	struct vme_bridge *bridge;
 
diff --git a/drivers/staging/vme_user/vme.h b/drivers/staging/vme_user/vme.h
index 7753e736f9fd..66388767b6c7 100644
--- a/drivers/staging/vme_user/vme.h
+++ b/drivers/staging/vme_user/vme.h
@@ -102,7 +102,7 @@ extern const struct bus_type vme_bus_type;
  * @bridge_list: List of devices (per bridge)
  */
 struct vme_dev {
-	int num;
+	u32 num;
 	struct vme_bridge *bridge;
 	struct device dev;
 	struct list_head drv_list;
@@ -179,7 +179,7 @@ int vme_lm_attach(struct vme_resource *, int, void (*callback)(void *), void *);
 int vme_lm_detach(struct vme_resource *, int);
 void vme_lm_free(struct vme_resource *);
 
-int vme_slot_num(struct vme_dev *);
+u32 vme_slot_num(struct vme_dev *);
 int vme_bus_num(struct vme_dev *);
 
 int vme_register_driver(struct vme_driver *, unsigned int);
diff --git a/drivers/staging/vme_user/vme_bridge.h b/drivers/staging/vme_user/vme_bridge.h
index 9bdc41bb6602..6778447eadfb 100644
--- a/drivers/staging/vme_user/vme_bridge.h
+++ b/drivers/staging/vme_user/vme_bridge.h
@@ -160,7 +160,7 @@ struct vme_bridge {
 	int (*lm_detach)(struct vme_lm_resource *, int);
 
 	/* CR/CSR space functions */
-	int (*slot_get)(struct vme_bridge *);
+	u32 (*slot_get)(struct vme_bridge *);
 
 	/* Bridge parent interface */
 	void *(*alloc_consistent)(struct device *dev, size_t size, dma_addr_t *dma);
diff --git a/drivers/staging/vme_user/vme_fake.c b/drivers/staging/vme_user/vme_fake.c
index 7f84d1c86f29..81601bfa4155 100644
--- a/drivers/staging/vme_user/vme_fake.c
+++ b/drivers/staging/vme_user/vme_fake.c
@@ -987,7 +987,7 @@ static int fake_lm_detach(struct vme_lm_resource *lm, int monitor)
 /*
  * Determine Geographical Addressing
  */
-static int fake_slot_get(struct vme_bridge *fake_bridge)
+static u32 fake_slot_get(struct vme_bridge *fake_bridge)
 {
 	return geoid;
 }
diff --git a/drivers/staging/vme_user/vme_tsi148.c b/drivers/staging/vme_user/vme_tsi148.c
index d81be8e4ceba..65237fb12466 100644
--- a/drivers/staging/vme_user/vme_tsi148.c
+++ b/drivers/staging/vme_user/vme_tsi148.c
@@ -2109,7 +2109,7 @@ static int tsi148_lm_detach(struct vme_lm_resource *lm, int monitor)
 /*
  * Determine Geographical Addressing
  */
-static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
+static u32 tsi148_slot_get(struct vme_bridge *tsi148_bridge)
 {
 	u32 slot = 0;
 	struct tsi148_driver *bridge;
@@ -2123,7 +2123,7 @@ static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
 		slot = geoid;
 	}
 
-	return (int)slot;
+	return slot;
 }
 
 static void *tsi148_alloc_consistent(struct device *parent, size_t size,
diff --git a/drivers/staging/vme_user/vme_user.c b/drivers/staging/vme_user/vme_user.c
index 5829a4141561..63f8efc19324 100644
--- a/drivers/staging/vme_user/vme_user.c
+++ b/drivers/staging/vme_user/vme_user.c
@@ -506,7 +506,7 @@ static int vme_user_match(struct vme_dev *vdev)
 	int i;
 
 	int cur_bus = vme_bus_num(vdev);
-	int cur_slot = vme_slot_num(vdev);
+	u32 cur_slot = vme_slot_num(vdev);
 
 	for (i = 0; i < bus_num; i++)
 		if ((cur_bus == bus[i]) && (cur_slot == vdev->num))
-- 
2.46.0

Re: [PATCH] staging: vme_user: Change slot number type from int to u32

[Nam Cao]

On Sun, Aug 25, 2024 at 12:59:55PM +0530, Riyan Dhiman wrote:
> Change the type used for VME slot numbers from int to u32 throughout vme
> driver. This modification more accurately represents the nature of slot
> numbers which are always non-negative.
> 
> The changes include
> - Updating variable declarations
> - Modifying function signatures and return types
> 
> This change imporves type safety, prevents potential issues with sign conversion.

Signed integer may be used to encode both non-negative valid values and
negative error code.

Are you sure none of the changed functions ever return a negative error
code?

Best regards,
Nam

Re: [PATCH] staging: vme_user: Change slot number type from int to u32

[Nam Cao]

On Sun, Aug 25, 2024 at 02:20:34PM +0530, Riyan Dhiman wrote:
> > Are you sure none of the changed functions ever return a negative error
> > code?
> 
> Yes, I have checked all the functions and variables for it.
> the slot function is defined like this

What about vme_slot_num()? It may return -EINVAL, right?

> static u32 tsi148_slot_get(struct vme_bridge *tsi148_bridge)
> {
> u32 slot = 0;
> struct tsi148_driver *bridge;
> 
> bridge = tsi148_bridge->driver_priv;
> 
> if (!geoid) {
> slot = ioread32be(bridge->base + TSI148_LCSR_VSTAT);
> slot = slot & TSI148_LCSR_VSTAT_GA_M;
> } else {
> slot = geoid;
> }
> 
> return slot;
> }
> 
> Here, slot is defined as u32, and the value read from ioread32be is also
> u32.
> In the else case, it is assigned to geoid, which is an int global variable.
> This global variable is a module parameter defined as follows:
> 
> MODULE_PARM_DESC(geoid, "Override geographical addressing");
> module_param(geoid, int, 0);
> 
> which I assume will always be non-negative.

If this should always be non-negative, then perhaps we should prevent users
from specifying negatives values. But that's a separate issue.

Best regards,
Nam

Re: [PATCH] staging: vme_user: Change slot number type from int to u32

[Dan Carpenter]

This patch introduces bugs.  I wrote a blog entry about how to choose data
types.
https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/

regards,
dan carpenter

Re: [PATCH] staging: vme_user: Change slot number type from int to u32

[Dan Carpenter]

On Sun, Aug 25, 2024 at 12:59:55PM +0530, Riyan Dhiman wrote:
> Change the type used for VME slot numbers from int to u32 throughout vme
> driver. This modification more accurately represents the nature of slot
> numbers which are always non-negative.
> 
> The changes include
> - Updating variable declarations
> - Modifying function signatures and return types
> 
> This change imporves type safety, prevents potential issues with sign conversion.

How type promotion works is that if we have if (a < b) { we first cast
everything to int.  Then we look at the types of a and b and if one of them has
more than 31 positive bits,  which ever has the most positive bits then we cast
both sides to that.  The danger is that a negative value will be cast to a high
unsigned value.

In a way you could look at it like the unsigned types are what is making the
code more dangerous.  If we didn't have unsigned types, nothing would change the
negatives into unsigned values.  Sure we'd have to always check for negatives,
but you'd just get used to that and do it.  This is how high level languages
like python work.  They don't have any kind of nonsense about if you're
comparing a define and a number -5 and the define is defined as another define
and you have to dig through five different header files and then the define
eventually becomes a sizeof() and so that means -5 is now 18446744073709551611.
In python -5 is just -5.

Of course, there is a place for unsigned types in C but it's so subtle and
complicated to explain.  I think people wish that there was a way to make C
safer when there really isn't.  There is no easy answer like just declare
everything as u32.  It's a false hope.

Here is a blog with more ranting.
https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

regards,
dan carpenter

Re: [PATCH] staging: vme_user: Change slot number type from int to u32

[Dan Carpenter]

On Mon, Aug 26, 2024 at 05:13:25PM +0530, Riyan Dhiman wrote:
> >
> >
> > Of course, there is a place for unsigned types in C but it's so subtle and
> > complicated to explain.  I think people wish that there was a way to make C
> > safer when there really isn't.  There is no easy answer like just declare
> > everything as u32.  It's a false hope.
> >
> 
> The reason I changed int to u32 is that the slot function was declared this
> way.
> 
> static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
> {
> u32 slot = 0;
> struct tsi148_driver *bridge;
> 
> bridge = tsi148_bridge->driver_priv;
> 
> if (!geoid) {
> slot = ioread32be(bridge->base + TSI148_LCSR_VSTAT);
> slot = slot & TSI148_LCSR_VSTAT_GA_M;
> } else {
> slot = geoid;
> }
> 
> return (int)slot;
> }
> 
> The slot is taken from the ioread32be function, which returns a u32.
> However, it gets cast to an int on return.
> If the value exceeds the int range, it could result in a strange large
> value. I didn't realize the potential bugs
> this could cause. How should cases like this be handled? Should we include
> an if condition to check if the
> slot is within the int range, something like this?
> 
> if (slot > INT_MAX) {
>     return -ERANGE;
> }
> return (int)slot;
> 
> Also, since slot will always return u32 isn't it better to declare it as
> u32 rather than int?
> 

slot is "slot = slot & TSI148_LCSR_VSTAT_GA_M;" which means it is in the 0-31
range.  The "geoid" variable is a module parameter.  These days we frown on
module parameters, and say it should be configured via sysfs instead.
It doesn't make sense for geiod to be negative or >= VME_MAX_SLOTS.  We should
check that in the probe() function.

I would declare geoid as u32, to communicate that negatives are not allowed.

Remove the cast from tsi148_slot_get() but leave the function as returning int.
No need to over engineer it.  Plus that way it matches the type of
vme_slot_num() which returns negative error codes so int is the correct type.

regards,
dan carpenter

Re: [PATCH] staging: vme_user: Change slot number type from int to u32

[Dan Carpenter]

On Mon, Aug 26, 2024 at 05:18:57PM +0530, Riyan Dhiman wrote:
> >
> > Of course, there is a place for unsigned types in C but it's so subtle and
> > complicated to explain.  I think people wish that there was a way to make C
> > safer when there really isn't.  There is no easy answer like just declare
> > everything as u32.  It's a false hope.
> >
> 
> There is a function in the vme_user driver (vme.c:L715)
> 
> unsigned int vme_master_rmw(struct vme_resource *resource, unsigned int
> mask,
>    unsigned int compare, unsigned int swap, loff_t offset)
> {
> struct vme_bridge *bridge = find_bridge(resource);
> struct vme_master_resource *image;
> 
> if (!bridge->master_rmw) {
> dev_warn(bridge->parent, "Writing to resource not supported\n");
> return -EINVAL;
> }
> 
> if (resource->type != VME_MASTER) {
> dev_err(bridge->parent, "Not a master resource\n");
> return -EINVAL;
> }
> 
> image = list_entry(resource->entry, struct vme_master_resource, list);
> 
> return bridge->master_rmw(image, mask, compare, swap, offset);
> }
> EXPORT_SYMBOL(vme_master_rmw);
> 
> It is declared as unsigned everywhere but returns negative error codes.
> This is an issue,
>  right? How should it be fixed? Do we need to change all the function
> declarations to int,
> or just in this function, since bridge->master_rmw returns an unsigned int?

Easy answer: Just delete vme_master_rmw() because nothing uses it.

Complicated answer: So long as the caller is saving it to int then it will work
okay.  If it saves it to any other type then it will cause problems.

regards,
dan carpenter