linux-kernel - Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300 accelerometer

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: <b9381886-35a8-8e5d-02b9-5d229439d11e@vaisala.com>
Date:   Tue, 20 Apr 2021 14:36:11 +0300
From:   Tomas Melin <tomas.melin@...sala.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/2] iio: accel: Add driver for Murata SCA3300
 accelerometer


On 4/20/21 1:47 PM, Andy Shevchenko wrote:
> On Tue, Apr 20, 2021 at 11:50 AM Tomas Melin <tomas.melin@...sala.com> wrote:
>
>>>> +       sca_data->txbuf[0] = 0x0 | (SCA3300_REG_STATUS << 2);
>>> Seems you ignored my comment. What is this 0x0? What is the meaning of it?
>>> Same for all the rest magic numbers in the code.
>> Sorry, not ignored but will remove this redundant 0x0 for next round.
> Maybe it's not redundant after all (I noticed other magic numbers in
> the same position)? Please, comment your intention case-by-case.

0x0 is for read operation, but since it's just or'd, end result should

be the same. It was there in v1 for clarity (also #defined). Basically

thinking perhaps it's cleaner to just leave it out.

Other magics should be cleaned up now.


> ...
>
>>>> +       for_each_set_bit(bit, indio_dev->active_scan_mask,
>>>> +                        indio_dev->masklength) {
>>>> +               ret = sca3300_read_reg(data, sca3300_channels[bit].address,
>>>> +                                      &val);
>>>> +               if (ret) {
>>>> +                       dev_err(&data->spi->dev,
>>>> +                               "failed to read register, error: %d\n", ret);
>>>> +                       goto out;
>>> Does it mean interrupt is handled in this case?
>>> Perhaps a comment why it's okay to consider so?
>> IRQ_HANDLED seemed more correct than IRQ_NONE.
> Why? Care to explain?

Thinking that IRQ was for the device and it was indeed handled. There 
were errors when handling

it, but it was handled as much as possible.

>
>>   Or did You have some
>> other option in mind?
>>
>> How about something like:
>>
>>       /* handled with errors */
> But what if this is the very first interrupt (bit in the loop) that
> failed? What about the rest?

Aah, right. Other option could be to simply continue loop and set 'val' 
to e.g. 0 for

readings with errors. But perhaps it is after all better to bail out, 
and only for cases

when _all_ data is reliable, it is pushed to buffers(?)

Comes to mind that perhaps better to have error message in this irq 
handler as

dev_err_ratelimited(), to avoid possible flooding.


So to conclude, proposing:

*change to dev_err_ratelimited()

* comment goto:

     /* handled, but bailing out this round due to errors */


Would this be OK?

Thanks,

Tomas




>
>>       goto out;
>>
>>>> +               }
>>>> +               data->scan.channels[i++] = val;
>>>> +       }