Re: [PATCH 3/3] iio: imu: bmi160: Remove potential undefined behavior in bmi160_config_pin()

[Nuno Sá]

On Mon, 2026-03-09 at 09:27 -0700, Josh Poimboeuf wrote:
> On Mon, Mar 09, 2026 at 04:16:16PM +0000, Nuno Sá wrote:
> > On Mon, 2026-03-09 at 09:03 -0700, Josh Poimboeuf wrote:
> > > If 'pin' is not one of its expected values, the value of
> > > 'int_out_ctrl_shift' is undefined.  With UBSAN enabled, this causes
> > > Clang to generate undefined behavior, resulting in the following
> > > warning:
> > > 
> > >   drivers/iio/imu/bmi160/bmi160_core.o: warning: objtool: bmi160_setup_irq() falls through to
> > > next
> > > function __cfi_bmi160_core_runtime_resume()
> > > 
> > > Prevent the UB and improve error handling by adding a BUG() if 'pin' has
> > > an unexpected value.
> > > 
> > > Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
> > > Cc: David Lechner <dlechner@xxxxxxxxxxxx>
> > > CC: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > > Cc: Andy Shevchenko <andy@xxxxxxxxxx>
> > > Fixes: 895bf81e6bbf ("iio:bmi160: add drdy interrupt support")
> > > Reported-by: Arnd Bergmann <arnd@xxxxxxxx>
> > > Closes: https://lore.kernel.org/a426d669-58bb-4be1-9eaa-6f3d83109e2d@xxxxxxxxxxxxxxxx
> > > Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > > ---
> > >  drivers/iio/imu/bmi160/bmi160_core.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> > > index 5f47708b4c5d..e5326df75e49 100644
> > > --- a/drivers/iio/imu/bmi160/bmi160_core.c
> > > +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> > > @@ -579,6 +579,8 @@ static int bmi160_config_pin(struct regmap *regmap, enum bmi160_int_pin
> > > pin,
> > >  		int_latch_mask = BMI160_INT2_LATCH_MASK;
> > >  		int_map_mask = BMI160_INT2_MAP_DRDY_EN;
> > >  		break;
> > > +	default:
> > > +		BUG();
> > >  	}
> > >  	int_out_ctrl_mask = BMI160_INT_OUT_CTRL_MASK << int_out_ctrl_shift;
> > >  
> > 
> > AFAIK, BUG() is not something we should use lightly so I wonder why having it rather that a
> > normal
> > 'return -EINVAL'?
> > 
> > At the very least, it could be WARN but I still think that's too much for a device .probe(). Any
> > special reason using BUG()?
> 
> Using BUG() for a default "invalid enum" is a common pattern, but indeed
> returning an error would also be valid here.  I can change that to
> return -EINVAL.
> 
> > Also seems like:
> > 
> > https://elixir.bootlin.com/linux/v7.0-rc2/source/drivers/iio/imu/bmi160/bmi160_core.c#L624
> > 
> > could be improved? Like setting 'pin_name' in the first switch() case.
> 
> Yeah, I assume you mean setting 'pin_name' to something like "invalid"
> for the default case?

Nope, I meant not duplicating the switch() case. The default should return an error so I would say
no need to bother in setting the name for that case.

- Nuno Sá