On Thu, 15 Sep 2022 17:27:43 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> Hello Jagath Jog J,
>
> The patch 961db2da159d: "iio: accel: bma400: Add support for single
> and double tap events" from Aug 31, 2022, leads to the following
> Smatch static checker warning:
>
> drivers/iio/accel/bma400_core.c:1287 bma400_tap_event_en()
> error: uninitialized symbol 'field_value'.
>
> drivers/iio/accel/bma400_core.c
> 1242 static int bma400_tap_event_en(struct bma400_data *data,
> 1243 enum iio_event_direction dir, int state)
> 1244 {
> 1245 unsigned int mask, field_value;
> ^^^^^^^^^^^^
> This is uninitialized.
>
> 1246 int ret;
> 1247
> 1248 /*
> 1249 * Tap interrupts can be configured only in normal mode.
> 1250 * See table in section 4.3 "Power modes - performance modes" of
> 1251 * datasheet v1.2.
> 1252 */
> 1253 if (data->power_mode != POWER_MODE_NORMAL)
> 1254 return -EINVAL;
> 1255
> 1256 /*
> 1257 * Tap interrupts are operating with a data rate of 200Hz.
> 1258 * See section 4.7 "Tap sensing interrupt" in datasheet v1.2.
> 1259 */
> 1260 if (data->sample_freq.hz != 200 && state) {
> 1261 dev_err(data->dev, "Invalid data rate for tap interrupts.\n");
> 1262 return -EINVAL;
> 1263 }
> 1264
> 1265 ret = regmap_update_bits(data->regmap, BMA400_INT12_MAP_REG,
> 1266 BMA400_S_TAP_MSK,
> 1267 FIELD_PREP(BMA400_S_TAP_MSK, state));
> 1268 if (ret)
> 1269 return ret;
> 1270
> 1271 switch (dir) {
> 1272 case IIO_EV_DIR_SINGLETAP:
> 1273 mask = BMA400_S_TAP_MSK;
> 1274 set_mask_bits(&field_value, BMA400_S_TAP_MSK,
> ^^^^^^^^^^^^
> Smatch ought to print a warning here but these macros use a bunch of
> *(&(*(&field_value))) permutations that confuse Smatch. Smatch does
> not print a warning if you're just taking the address of a variable.
>
> This initializes BIT(3)? I believe that KMSan will detect this as a bug
> an issue a warning here.
>
> 1275 FIELD_PREP(BMA400_S_TAP_MSK, state));
> 1276 break;
> 1277 case IIO_EV_DIR_DOUBLETAP:
> 1278 mask = BMA400_D_TAP_MSK;
> 1279 set_mask_bits(&field_value, BMA400_D_TAP_MSK,
> 1280 FIELD_PREP(BMA400_D_TAP_MSK, state));
> 1281 break;
> 1282 default:
> 1283 return -EINVAL;
> 1284 }
> 1285
> 1286 ret = regmap_update_bits(data->regmap, BMA400_INT_CONFIG1_REG, mask,
> --> 1287 field_value);
>
> This uses BIT(3). The function only cares about BIT(3) but the other
> bits are uninitialized.
>
> 1288 if (ret)
> 1289 return ret;
> 1290
> 1291 set_mask_bits(&data->tap_event_en_bitmask, mask, field_value);
> 1292
> 1293 return 0;
> 1294 }
>
> regards,
> dan carpenter
Agreed. It's not a bug as such because the other bits are never used, but
it certainly hard for checkers to reason about.
Obvious 'fix' is just set the initial value to 0.
I'll send a patch.
