linux-kernel - Re: [PATCH v3 0/1] iio/scmi: Add reading "raw" attribute.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 5 Oct 2021 15:51:57 +0300
From:   Vasyl Vavrychuk <vasyl.vavrychuk@...nsynergy.com>
To:     Jyoti Bhayana <jbhayana@...gle.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jonathan Cameron <jic23@...nel.org>,
        Andriy Tryshnivskyy <andriy.tryshnivskyy@...nsynergy.com>
Subject: Re: [PATCH v3 0/1] iio/scmi: Add reading "raw" attribute.

Hi, Jyoti,

> In the code below, why is the logic of enabling and disabling the
> sensor in this function?  Generally the function to read the sensor
> value is just used for the code to read the sensor values ? and not
> enable/disable the sensor

But to read sensor value we have to enable it first. Other way to enable 
sensor we found is, for example:

   echo 1 > /sys/bus/iio/devices/.../scan_elements/in_anglvel_x_en

But, this command is related to IIO buffers use.

Other sensors drivers enable/disable sensor in read raw too, for 
example, drivers/iio/accel/kxcjk-1013.c has:

   case IIO_CHAN_INFO_RAW:
           mutex_lock(&data->mutex);
           if (iio_buffer_enabled(indio_dev))
                   ret = -EBUSY;
           else {
                   ret = kxcjk1013_set_power_state(data, true);
                   ... reading ...
                   ret = kxcjk1013_set_power_state(data, false);
           }
           mutex_unlock(&data->mutex);

But, after looking on this code I have some questions:

1. Should we block reading raw attribute and IIO buffer enabled, for for 
SCMI sensor it can coexist?
2. Should we wrap reading raw attribute implementation in iio_dev->mlock 
mutex?

>>     case IIO_CHAN_INFO_RAW:
>> +               sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
>> +                                          SCMI_SENS_CFG_SENSOR_ENABLE);
>> +               err = sensor->sensor_ops->config_set(
>> +                       sensor->ph, sensor->sensor_info->id, sensor_config);
>> +               if (err) {
>> +                       dev_err(&iio_dev->dev,
>> +                               "Error in enabling sensor %s err %d",
>> +                               sensor->sensor_info->name, err);
>> +                       return err;
>> +               }
>> +
>> +               err = sensor->sensor_ops->reading_get_timestamped(
>> +                       sensor->ph, sensor->sensor_info->id,
>> +                       sensor->sensor_info->num_axis, readings);
>> +               if (err) {
>> +                       dev_err(&iio_dev->dev,
>> +                               "Error in reading raw attribute for sensor %s err %d",
>> +                               sensor->sensor_info->name, err);
>> +                       return err;
>> +               }
>> +
>> +               sensor_config = FIELD_PREP(SCMI_SENS_CFG_SENSOR_ENABLED_MASK,
>> +                                          SCMI_SENS_CFG_SENSOR_DISABLE);
>> +               err = sensor->sensor_ops->config_set(
>> +                       sensor->ph, sensor->sensor_info->id, sensor_config);
>> +               if (err) {
>> +                       dev_err(&iio_dev->dev,
>> +                               "Error in enabling sensor %s err %d",
>> +                               sensor->sensor_info->name, err);
>> +                       return err;
>> +               }
>> +               /* Check if raw value fits 32 bits */
>> +               if (readings[ch->scan_index].value < INT_MIN ||
>> +                   readings[ch->scan_index].value > INT_MAX)
>> +                       return -ERANGE;
>> +               /* Use 32-bit value, since practically there is no need in 64 bits */
>> +               *val = (int)readings[ch->scan_index].value;
>>
>> +               return IIO_VAL_INT;