Re: [PATCH v4 3/4] iio: hid-sensor-magn-3d: Scan for usage attributes before setting up iio channels

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

 



Hello Srinivas,

Thanks for testing this patch. I only have one PC with a
hid-sensor-hub and mine doesn't have the axis usage attribute only
north. I'll look into this.

On Mon, Jul 7, 2014 at 2:58 PM, Srinivas Pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> wrote:
> Hi Reyad,
>
> I see panic in the probe function. Can you review your logic?
> It is possible that platforms don't have all attributes, so looks
> like the probe is returnning with error.
>
>
> On 07/07/2014 09:44 AM, Jonathan Cameron wrote:
>>
>> On 30/06/14 03:58, Reyad Attiyat wrote:
>>>
>>> Scan for and count the HID usage attributes supported by the driver.
>>> This allows for the driver to only setup the IIO channels for the
>>> sensor usages present in the HID USB reports.
>>>
>>> Signed-off-by: Reyad Attiyat <reyad.attiyat@xxxxxxxxx>
>>> ---
>>
>> There should be an explanation here of what has changed from one version
>> to the
>> next.
>>
>> The patches should have been run through checkpatch.pl (at least one
>> place below
>> indicates that didn't happen).
>>
>> Mostly little bits left.  Now I would definitely like an ack or
>> reviewed-by
>> from Srinivas for this one if at all possible.
>
>
> [    7.653643] BUG: unable to handle kernel NULL pointer dereference at
> 0000000000000031
> [    7.653648] IP: [<ffffffff81242cee>] sysfs_remove_link+0xe/0x30
> [    7.653650] PGD 13d0fd067 PUD 147cbd067 PMD 0
> [    7.653652] Oops: 0000 [#1] SMP
> [    7.653676] Modules linked in: hid_sensor_magn_3d(+) hid_sensor_rotation
> hid_sensor_gyro_3d hid_sensor_trigger industrialio_triggered_buffer
> kfifo_buf industrialio asix(+) usb_storage hid_sensor_iio_common usbnet
> usbhid mii joydev usbserial(+) hid_rmi(+) intel_rapl x86_pkg_temp_thermal
> uvcvideo intel_powerclamp coretemp kvm_intel videobuf2_vmalloc
> videobuf2_memops videobuf2_core iwlwifi kvm v4l2_common videodev
> hid_multitouch hid_sensor_hub ppdev btusb crct10dif_pclmul cfg80211
> crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 lrw gf128mul
> glue_helper ablk_helper cryptd mei_me(+) mei serio_raw lpc_ich(+) i915(+)
> snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_controller
> snd_hda_codec snd_hwdep snd_pcm snd_seq_midi drm_kms_helper
> snd_seq_midi_event snd_rawmidi snd_seq drm snd_seq_device snd_timer snd
> soundcore i2c_algo_bit mac_hid nfsd i2c_hid hid auth_rpcgss nfs_acl rfcomm
> nfs bnep bluetooth lockd winbond_cir sunrpc rc_core parport_pc dw_dmac
> dw_dmac_core video i2c_designware_platform spi_pxa2xx_platform
> i2c_designware_core binfmt_misc 8250_dw fscache lp nls_iso8859_1 parport
> ahci libahci sdhci_acpi sdhci
> [    7.653691] CPU: 1 PID: 372 Comm: systemd-udevd Not tainted 3.16.0-rc4+
> #27
> [    7.653692] Hardware name: Intel Corporation Shark Bay Client
> platform/Harris Beach SDS, BIOS HSWLPTU1.86C.0133.R00.1309172123 09/17/2013
> [    7.653693] task: ffff880148584b60 ti: ffff880148914000 task.ti:
> ffff880148914000
> [    7.653696] RIP: 0010:[<ffffffff81242cee>]  [<ffffffff81242cee>]
> sysfs_remove_link+0xe/0x30
> [    7.653697] RSP: 0018:ffff880148917c30  EFLAGS: 00010202
> [    7.653698] RAX: ffffffffa08a1028 RBX: ffff880090bef810 RCX:
> 0000000000001043
> [    7.653698] RDX: 0000000000001042 RSI: 0000000000000000 RDI:
> 0000000000000001
> [    7.653699] RBP: ffff880148917c30 R08: 00000000000171c0 R09:
> ffff88014f2571c0
> [    7.653700] R10: ffffea0002ac20c0 R11: ffffffff814a48a9 R12:
> 00000000fffffff0
> [    7.653701] R13: ffffffffa08a1028 R14: 0000000000000025 R15:
> 0000000000000001
> [    7.653702] FS:  00007fd70e437880(0000) GS:ffff88014f240000(0000)
> knlGS:0000000000000000
> [    7.653703] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    7.653704] CR2: 0000000000000031 CR3: 000000013d05e000 CR4:
> 00000000001407e0
> [    7.653705] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [    7.653706] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [    7.653706] Stack:
> [    7.653708]  ffff880148917c48 ffffffff814a0626 ffff880090bef810
> ffff880148917c78
> [    7.653710]  ffffffff814a0c2e ffff880090bef810 ffffffffa08a1028
> ffff880090bef870
> [    7.653712]  0000000000000000 ffff880148917ca0 ffffffff814a1053
> 0000000000000000
> [    7.653712] Call Trace:
> [    7.653716]  [<ffffffff814a0626>] driver_sysfs_remove+0x26/0x40
> [    7.653719]  [<ffffffff814a0c2e>] driver_probe_device+0x8e/0x3e0
> [    7.653720]  [<ffffffff814a1053>] __driver_attach+0x93/0xa0
> [    7.653722]  [<ffffffff814a0fc0>] ? __device_attach+0x40/0x40
> [    7.653725]  [<ffffffff8149ec93>] bus_for_each_dev+0x63/0xa0
> [    7.653727]  [<ffffffff814a06ce>] driver_attach+0x1e/0x20
> [    7.653728]  [<ffffffff814a02e0>] bus_add_driver+0x180/0x250
> [    7.653731]  [<ffffffffa005b000>] ? 0xffffffffa005afff
> [    7.653733]  [<ffffffff814a1834>] driver_register+0x64/0xf0
> [    7.653735]  [<ffffffff814a2dca>] __platform_driver_register+0x4a/0x50
> [    7.653737]  [<ffffffffa005b017>]
> hid_magn_3d_platform_driver_init+0x17/0x1000 [hid_sensor_magn_3d]
> [    7.653741]  [<ffffffff8100212c>] do_one_initcall+0xbc/0x1f0
> [    7.653744]  [<ffffffff811b239d>] ? kfree+0xfd/0x140
> [    7.653747]  [<ffffffff811996f2>] ? __vunmap+0xb2/0x100
> [    7.653750]  [<ffffffff810eaf0c>] load_module+0x1cec/0x2700
> [    7.653752]  [<ffffffff810e6e10>] ? store_uevent+0x40/0x40
> [    7.653755]  [<ffffffff810e79f1>] ?
> copy_module_from_fd.isra.46+0x121/0x180
> [    7.653757]  [<ffffffff810eba96>] SyS_finit_module+0x86/0xb0
> [    7.653761]  [<ffffffff8173f73f>] tracesys+0xe1/0xe6
> [    7.653779] Code: 48 8b 35 8e 16 d5 00 48 8b 57 10 e8 0d de ff ff 5d c3
> 66 2e 0f 1f 84 00 00 00 00 00 90 0f 1f 44 00 00 55 48 85 ff 48 89 e5 74 12
> <48> 8b 7f 30 31 d2 e8 47 dd ff ff 5d c3 0f 1f 44 00 00 48 8b 3d
> [    7.653781] RIP  [<ffffffff81242cee>] sysfs_remove_link+0xe/0x30
> [    7.653782]  RSP <ffff880148917c30>
> [    7.653782] CR2: 0000000000000031
> [    7.653785] ---[ end trace 05dd86b8f35f8800 ]---
>
>
> Other changes, as suggested by Jontahan regarding format, one more
> on iio_val in the structure magn_3d_state.
>
>
>
>> Any tested-bys, particularly with parts that don't have the new channel
>> types, would also be good.
>>
>> Jonathan
>>>
>>>   drivers/iio/magnetometer/hid-sensor-magn-3d.c | 111
>>> +++++++++++++++++---------
>>>   1 file changed, 75 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> index 41cf29e..070d20e 100644
>>> --- a/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> +++ b/drivers/iio/magnetometer/hid-sensor-magn-3d.c
>>> @@ -42,7 +42,8 @@ struct magn_3d_state {
>>>       struct hid_sensor_hub_callbacks callbacks;
>>>       struct hid_sensor_common common_attributes;
>>>       struct hid_sensor_hub_attribute_info magn[MAGN_3D_CHANNEL_MAX];
>>> -    u32 magn_val[MAGN_3D_CHANNEL_MAX];
>>> +    u32 *magn_val_addr[MAGN_3D_CHANNEL_MAX];
>>> +    u32 *iio_val;
>
> I prefer iio_vals, as this stores all the values not a single value.
>
> Thanks,
> Srinivas
>
>>>       int scale_pre_decml;
>>>       int scale_post_decml;
>>>       int scale_precision;
>>> @@ -221,8 +222,8 @@ static int magn_3d_proc_event(struct
>>> hid_sensor_hub_device *hsdev,
>>>       dev_dbg(&indio_dev->dev, "magn_3d_proc_event\n");
>>>       if (atomic_read(&magn_state->common_attributes.data_ready))
>>>           hid_sensor_push_data(indio_dev,
>>> -                magn_state->magn_val,
>>> -                sizeof(magn_state->magn_val));
>>> +                magn_state->iio_val,
>>> +                sizeof(magn_state->iio_val));
>>>
>>>       return 0;
>>>   }
>>> @@ -236,52 +237,99 @@ static int magn_3d_capture_sample(struct
>>> hid_sensor_hub_device *hsdev,
>>>       struct iio_dev *indio_dev = platform_get_drvdata(priv);
>>>       struct magn_3d_state *magn_state = iio_priv(indio_dev);
>>>       int offset;
>>> -    int ret = -EINVAL;
>>> +    int ret = 0;
>>> +    u32 *iio_val = NULL;
>>
>> This has me a little confused.  You have an iio_val in your state
>> structure and in this function.  I can't actually find anywhere where
>> the elements of the one in the state structure are ever assigned to
>> anything.
>>
>>>
>>>       switch (usage_id) {
>>>       case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS:
>>>       case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Y_AXIS:
>>>       case HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_Z_AXIS:
>>>           offset = usage_id - HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS;
>>> -        magn_state->magn_val[CHANNEL_SCAN_INDEX_X + offset] =
>>> -                        *(u32 *)raw_data;
>>> -        ret = 0;
>>> +        iio_val = magn_state->magn_val_addr[CHANNEL_SCAN_INDEX_X +
>>> offset];
>>> +
>>>       break;
>>>       default:
>>> -        break;
>>> +        return -EINVAL;
>>>       }
>>>
>>> +    if(iio_val)
>>
>> white space after if please.
>>>
>>> +        *iio_val = *(u32 *)raw_data;
>>> +    else
>>> +        ret = -EINVAL;
>>> +
>>>       return ret;
>>>   }
>>>
>>>   /* Parse report which is specific to an usage id*/
>>>   static int magn_3d_parse_report(struct platform_device *pdev,
>>>                   struct hid_sensor_hub_device *hsdev,
>>> -                struct iio_chan_spec *channels,
>>> +                struct iio_chan_spec **channels,
>>> +                int *chan_count,
>>>                   unsigned usage_id,
>>>                   struct magn_3d_state *st)
>>>   {
>>> -    int ret;
>>> +    int ret = 0;
>>>       int i;
>>> +    int attr_count = 0;
>>> +
>>> +    /* Scan for each usage attribute supported */
>>> +    for (i = 0; i < MAGN_3D_CHANNEL_MAX; i++) {
>>> +        u32 address = magn_3d_addresses[i];
>>>
>>> -    for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
>>> +        /* Check if usage attribute exists in the sensor hub device */
>>>           ret = sensor_hub_input_get_attribute_info(hsdev,
>>> -                HID_INPUT_REPORT,
>>> -                usage_id,
>>> -                HID_USAGE_SENSOR_ORIENT_MAGN_FLUX_X_AXIS + i,
>>> -                &st->magn[CHANNEL_SCAN_INDEX_X + i]);
>>> -        if (ret < 0)
>>> -            break;
>>> -        magn_3d_adjust_channel_bit_mask(channels,
>>> -                CHANNEL_SCAN_INDEX_X + i,
>>> -                st->magn[CHANNEL_SCAN_INDEX_X + i].size);
>>
>> I would have preferred you kept the indenting the same as before. That
>> would
>> make it more obvious what changed.
>>>
>>> +            HID_INPUT_REPORT,
>>> +            usage_id,
>>> +            address,
>>> +            &(st->magn[i]));
>>> +        if (!ret)
>>> +            attr_count++;
>>>       }
>>> -    dev_dbg(&pdev->dev, "magn_3d %x:%x, %x:%x, %x:%x\n",
>>> +
>>> +    dev_dbg(&pdev->dev, "magn_3d Found %d usage attributes\n",
>>> +            attr_count);
>>> +    dev_dbg(&pdev->dev, "magn_3d X: %x:%x Y: %x:%x Z: %x:%x\n",
>>>               st->magn[0].index,
>>>               st->magn[0].report_id,
>>>               st->magn[1].index, st->magn[1].report_id,
>>>               st->magn[2].index, st->magn[2].report_id);
>>>
>>> +    if (attr_count > 0)
>>> +        ret = 0;
>>
>> This would suggest to me that you want to rename the ret above (used
>> to indicate if a channel is there) as something more specific so you
>> don't end up appearing to squash and error here...
>>>
>>> +    else
>>> +        return  -EINVAL;
>>
>> Again your spacing is messed up. Please fix.
>>>
>>> +
>>> +    /* Setup IIO channel array */
>>> +    *channels = devm_kcalloc(&pdev->dev, attr_count,
>>> +
>>
>> The resulting indenting here is a complete mess. Please fix.
>>                                  sizeof(struct iio_chan_spec),
>>>
>>> +                                GFP_KERNEL);
>>> +    if (!*channels) {
>>> +        dev_err(&pdev->dev, "failed to allocate space for iio
>>> channels\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    st->iio_val = devm_kcalloc(&pdev->dev, attr_count,
>>> +                                sizeof(u32),
>>> +                                GFP_KERNEL);
>>> +    if (!st->iio_val) {
>>> +        dev_err(&pdev->dev, "failed to allocate space for iio values
>>> array\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    for (i = 0, *chan_count = 0;
>>> +        i < MAGN_3D_CHANNEL_MAX && *chan_count < attr_count;
>>> +        i++)
>>> +    {
>>> +        if (st->magn[i].index >= 0) {
>>> +            /* Setup IIO channel struct */
>>> +            *channels[*chan_count] = magn_3d_channels[i];
>>> +
>>> +            st->magn_val_addr[i] = &(st->iio_val[*chan_count]);
>>> +            magn_3d_adjust_channel_bit_mask(*channels, *chan_count,
>>> st->magn[i].size);
>>
>> The above should be wrapped appropriately.
>>>
>>> +            (*chan_count)++;
>>> +        }
>>> +    }
>>> +
>>>       st->scale_precision = hid_sensor_format_scale(
>>>                   HID_USAGE_SENSOR_COMPASS_3D,
>>>                   &st->magn[CHANNEL_SCAN_INDEX_X],
>>> @@ -311,6 +359,7 @@ static int hid_magn_3d_probe(struct
>>> platform_device *pdev)
>>>       struct magn_3d_state *magn_state;
>>>       struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>>>       struct iio_chan_spec *channels;
>>> +    int chan_count = 0;
>>>
>>>       indio_dev = devm_iio_device_alloc(&pdev->dev,
>>>                         sizeof(struct magn_3d_state));
>>> @@ -331,22 +380,15 @@ static int hid_magn_3d_probe(struct
>>> platform_device *pdev)
>>>           return ret;
>>>       }
>>>
>>> -    channels = kmemdup(magn_3d_channels, sizeof(magn_3d_channels),
>>> -               GFP_KERNEL);
>>> -    if (!channels) {
>>> -        dev_err(&pdev->dev, "failed to duplicate channels\n");
>>> -        return -ENOMEM;
>>> -    }
>>> -
>>> -    ret = magn_3d_parse_report(pdev, hsdev, channels,
>>> -                HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>>> +    ret = magn_3d_parse_report(pdev, hsdev, &channels,
>>> +                &chan_count, HID_USAGE_SENSOR_COMPASS_3D, magn_state);
>>>       if (ret) {
>>>           dev_err(&pdev->dev, "failed to setup attributes\n");
>>> -        goto error_free_dev_mem;
>>> +        return ret;
>>>       }
>>>
>>>       indio_dev->channels = channels;
>>> -    indio_dev->num_channels = ARRAY_SIZE(magn_3d_channels);
>>> +    indio_dev->num_channels = chan_count;
>>>       indio_dev->dev.parent = &pdev->dev;
>>>       indio_dev->info = &magn_3d_info;
>>>       indio_dev->name = name;
>>> @@ -356,7 +398,7 @@ static int hid_magn_3d_probe(struct
>>> platform_device *pdev)
>>>           NULL, NULL);
>>>       if (ret) {
>>>           dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
>>> -        goto error_free_dev_mem;
>>> +        return ret;
>>>       }
>>>       atomic_set(&magn_state->common_attributes.data_ready, 0);
>>>       ret = hid_sensor_setup_trigger(indio_dev, name,
>>> @@ -390,8 +432,6 @@ error_remove_trigger:
>>>       hid_sensor_remove_trigger(&magn_state->common_attributes);
>>>   error_unreg_buffer_funcs:
>>>       iio_triggered_buffer_cleanup(indio_dev);
>>> -error_free_dev_mem:
>>> -    kfree(indio_dev->channels);
>>>       return ret;
>>>   }
>>>
>>> @@ -406,7 +446,6 @@ static int hid_magn_3d_remove(struct
>>> platform_device *pdev)
>>>       iio_device_unregister(indio_dev);
>>>       hid_sensor_remove_trigger(&magn_state->common_attributes);
>>>       iio_triggered_buffer_cleanup(indio_dev);
>>> -    kfree(indio_dev->channels);
>>>
>>>       return 0;
>>>   }
>>>
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[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