| 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
| ||
|
Message-ID: <eb6a1c2f-a773-4db5-812d-df7680e46f5b@linux.intel.com>
Date: Wed, 24 Jan 2024 22:01:31 -0800
From: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
To: Basavaraj Natikar <bnatikar@....com>, jikos@...nel.org, jic23@...nel.org,
lars@...afoo.de, Basavaraj.Natikar@....com
Cc: linux-input@...r.kernel.org, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/4] iio: hid-sensor-als: Assign channels dynamically
Hi Basavraj,
On 1/24/24 21:51, Basavaraj Natikar wrote:
> Hi Srinivas,
>
>
> On 1/9/2024 11:30 PM, Srinivas Pandruvada wrote:
>> Instead of assuming that every channel defined statically by
>> als_channels[] is present, assign dynamically based on presence of the
>> respective usage id in the descriptor. This will allow to register ALS
>> with limited channel support. Append the timestamp as the last channel.
>>
>> When not all usage ids are present, the scan index is adjusted to
>> exclude unsupported channels.
>>
>> There is no intentional function changes done.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
>> ---
>> v3:
>> Addressed comments from Jonthan:
>> - Remove channel allocation and move to iio_priv()
>> - Parse all usage IDs in a single loop and continue
>> for failure. This way the temperature and chromaticity
>> will not need any special processing to parse usage ids.
>> - Don't leave empty channel indexes
>>
>> v2:
>> New change
>>
>> drivers/iio/light/hid-sensor-als.c | 56 +++++++++++++++++++++---------
>> 1 file changed, 39 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iio/light/hid-sensor-als.c b/drivers/iio/light/hid-sensor-als.c
>> index 5cd27f04b45e..72a7c01c97f8 100644
>> --- a/drivers/iio/light/hid-sensor-als.c
>> +++ b/drivers/iio/light/hid-sensor-als.c
>> @@ -25,17 +25,26 @@ struct als_state {
>> struct hid_sensor_hub_callbacks callbacks;
>> struct hid_sensor_common common_attributes;
>> struct hid_sensor_hub_attribute_info als[CHANNEL_SCAN_INDEX_MAX];
>> + struct iio_chan_spec channels[CHANNEL_SCAN_INDEX_MAX + 1];
>> struct {
>> u32 illum[CHANNEL_SCAN_INDEX_MAX];
>> + u32 scan_index[CHANNEL_SCAN_INDEX_MAX];
>> u64 timestamp __aligned(8);
>> } scan;
>> int scale_pre_decml;
>> int scale_post_decml;
>> int scale_precision;
>> int value_offset;
>> + int num_channels;
>> s64 timestamp;
>> };
>>
>> +/* The order of usage ids must match scan index starting from CHANNEL_SCAN_INDEX_INTENSITY */
>> +static const u32 als_usage_ids[] = {
>> + HID_USAGE_SENSOR_LIGHT_ILLUM,
>> + HID_USAGE_SENSOR_LIGHT_ILLUM,
>> +};
>> +
>> static const u32 als_sensitivity_addresses[] = {
>> HID_USAGE_SENSOR_DATA_LIGHT,
>> HID_USAGE_SENSOR_LIGHT_ILLUM,
>> @@ -216,11 +225,14 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
>> struct als_state *als_state = iio_priv(indio_dev);
>> int ret = -EINVAL;
>> u32 sample_data = *(u32 *)raw_data;
>> + int scan_index;
>>
>> switch (usage_id) {
>> case HID_USAGE_SENSOR_LIGHT_ILLUM:
>> - als_state->scan.illum[CHANNEL_SCAN_INDEX_INTENSITY] = sample_data;
>> - als_state->scan.illum[CHANNEL_SCAN_INDEX_ILLUM] = sample_data;
>> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_INTENSITY];
>> + als_state->scan.illum[scan_index] = sample_data;
>> + scan_index = als_state->scan.scan_index[CHANNEL_SCAN_INDEX_ILLUM];
>> + als_state->scan.illum[scan_index] = sample_data;
> Could you please use original above change which works fine for some reason if
> we use this change als_state->scan.scan_index we are getting garbage value.
I think this is because of the issue which you found later for this patch.
>> ret = 0;
>> break;
>> case HID_USAGE_SENSOR_TIME_TIMESTAMP:
>> @@ -237,27 +249,39 @@ static int als_capture_sample(struct hid_sensor_hub_device *hsdev,
>> /* Parse report which is specific to an usage id*/
>> static int als_parse_report(struct platform_device *pdev,
>> struct hid_sensor_hub_device *hsdev,
>> - struct iio_chan_spec *channels,
>> unsigned usage_id,
>> struct als_state *st)
>> {
>> - int ret;
>> + struct iio_chan_spec *channels;
>> + int ret, index = 0;
>> int i;
>>
>> + channels = st->channels;
>> +
>> for (i = 0; i <= CHANNEL_SCAN_INDEX_ILLUM; ++i) {
> Could you please change CHANNEL_SCAN_INDEX_ILLUM to CHANNEL_SCAN_INDEX_MAX
> which helps remaining 2 patches in the series.
>
Makes sense.
Thanks,
Srinivas
>> ret = sensor_hub_input_get_attribute_info(hsdev,
>> HID_INPUT_REPORT,
>> usage_id,
>> - HID_USAGE_SENSOR_LIGHT_ILLUM,
>> + als_usage_ids[i],
>> &st->als[i]);
>> if (ret < 0)
>> - return ret;
>> - als_adjust_channel_bit_mask(channels, i, st->als[i].size);
>> + continue;
>> +
>> + channels[index] = als_channels[i];
>> + st->scan.scan_index[i] = index;
>> +
>> + als_adjust_channel_bit_mask(channels, index, st->als[i].size);
>> + ++index;
>>
>> dev_dbg(&pdev->dev, "als %x:%x\n", st->als[i].index,
>> st->als[i].report_id);
>> }
>>
>> + st->num_channels = index;
>> + /* Return success even if one usage id is present */
>> + if (index)
>> + ret = 0;
>> +
>> st->scale_precision = hid_sensor_format_scale(usage_id,
>> &st->als[CHANNEL_SCAN_INDEX_INTENSITY],
>> &st->scale_pre_decml, &st->scale_post_decml);
>> @@ -293,15 +317,7 @@ static int hid_als_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> - indio_dev->channels = devm_kmemdup(&pdev->dev, als_channels,
>> - sizeof(als_channels), GFP_KERNEL);
>> - if (!indio_dev->channels) {
>> - dev_err(&pdev->dev, "failed to duplicate channels\n");
>> - return -ENOMEM;
>> - }
>> -
>> ret = als_parse_report(pdev, hsdev,
>> - (struct iio_chan_spec *)indio_dev->channels,
>> hsdev->usage,
>> als_state);
>> if (ret) {
>> @@ -309,8 +325,14 @@ static int hid_als_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> - indio_dev->num_channels =
>> - ARRAY_SIZE(als_channels);
>> + /* Add timestamp channel */
>> + als_state->channels[als_state->num_channels] = als_channels[CHANNEL_SCAN_INDEX_TIMESTAMP];
>> + als_state->channels[als_state->num_channels].scan_index = als_state->num_channels;
>> +
>> + /* +1 for adding timestamp channel */
>> + indio_dev->num_channels = als_state->num_channels + 1;
>> +
>> + indio_dev->channels = als_state->channels;
>> indio_dev->info = &als_info;
>> indio_dev->name = name;
>> indio_dev->modes = INDIO_DIRECT_MODE;
Powered by blists - more mailing lists