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
