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

[Dan Carpenter <dan.carpenter@linaro.org>]

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