linux-kernel - Re: [v6 08/14] iio: imu: add Bosch Sensortec BNO055 core driver

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]

Message-ID: <164f663acbba481a8ea8f45f185aaf83@iit.it>
Date:   Tue, 14 Jun 2022 09:11:24 +0000
From:   Andrea Merello <Andrea.Merello@....it>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Jonathan Cameron <jic23@...nel.org>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Rob Herring" <robh+dt@...nel.org>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        Alexandru Ardelean <ardeleanalex@...il.com>,
        jmondi <jacopo@...ndi.org>
Subject: Re: [v6 08/14] iio: imu: add Bosch Sensortec BNO055 core driver

Few inline comments, OK for the rest.

>...
>
>> +                                /* G:   2,    4,    8,    16 */
>
>Indentation of this comment is a bit off.
>
>> +static int bno055_acc_range_vals[] = {1962, 3924, 7848, 15696};
>
>Perhaps split this to 4 lines and put the comment on top of the third line?

Not sure what you mean here, sorry. May you elaborate or provide an example, please?

>...
>
>> +static void bno055_debugfs_init(struct iio_dev *iio_dev)
>> +{
>> +       struct bno055_priv *priv = iio_priv(iio_dev);
>> +
>> +       priv->debugfs = debugfs_create_file("firmware_version", 0400,
>> +                                           iio_get_debugfs_dentry(iio_dev),
>> +                                           priv, &bno055_fw_version_ops);
>
>> +       devm_add_action_or_reset(priv->dev, bno055_debugfs_remove, priv->debugfs);
>
>Shouldn't we report the potential error here? It's not directly
>related to debugfs, but something which is not directly related.

The error eventually comes out from something that has nothing to do with debugs per se (i.e. the devm stuff), but it will only affect debugfs indeed.

Assuming that we don't want to make the whole driver fail in case debugfs stuff fails (see last part of the comment above debugfs_create_file() implementation), and given that the devm_add_action_or_reset(), should indeed "reset" in case of failure (i.e. we should be in a clean situation anyway), I would say it should be OK not to propagate the error and let things go on.

However we can add a dev_warn() to report what happened.