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] [day] [month] [year] [list]

Date:   Tue, 20 Apr 2021 15:12:42 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Tomas Melin <tomas.melin@...sala.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 Tue, Apr 20, 2021 at 2:36 PM Tomas Melin <tomas.melin@...sala.com> wrote:
> 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:

...

> >>>> +       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?

Sounds like a plan!

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

-- 
With Best Regards,
Andy Shevchenko