Re: [bug report] iio: accel: bma400: Add support for single and double tap events

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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. 




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux