linux-kernel - Re: [PATCH v8 2/2] iio: pressure: dps310: Reset chip after timeout

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: <20220916162535.00000cf6@huawei.com>
Date:   Fri, 16 Sep 2022 16:25:35 +0100
From:   Jonathan Cameron <Jonathan.Cameron@...wei.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Eddie James <eajames@...ux.ibm.com>, <jic23@...nel.org>,
        <lars@...afoo.de>, <linux-iio@...r.kernel.org>, <joel@....id.au>,
        <linux-kernel@...r.kernel.org>, <stable@...r.kernel.org>
Subject: Re: [PATCH v8 2/2] iio: pressure: dps310: Reset chip after timeout

On Fri, 16 Sep 2022 08:51:13 +0300
Andy Shevchenko <andy.shevchenko@...il.com> wrote:

> On Thu, Sep 15, 2022 at 10:57 PM Eddie James <eajames@...ux.ibm.com> wrote:
> >
> > The DPS310 chip has been observed to get "stuck" such that pressure
> > and temperature measurements are never indicated as "ready" in the
> > MEAS_CFG register. The only solution is to reset the device and try
> > again. In order to avoid continual failures, use a boolean flag to
> > only try the reset after timeout once if errors persist.  
> 
> ...
> 
> > +static int dps310_ready(struct dps310_data *data, int ready_bit, int timeout)
> > +{
> > +       int rc;
> > +
> > +       rc = dps310_ready_status(data, ready_bit, timeout);
> > +       if (rc) {  
> 
> > +               if (rc == -ETIMEDOUT && !data->timeout_recovery_failed) {  
> 
> Here you compare rc with a certain error code...
> 
> > +                       /* Reset and reinitialize the chip. */
> > +                       if (dps310_reset_reinit(data)) {
> > +                               data->timeout_recovery_failed = true;
> > +                       } else {
> > +                               /* Try again to get sensor ready status. */  
> 
> > +                               if (dps310_ready_status(data, ready_bit, timeout))  
> 
> ...but here you assume that the only error code is -ETIMEDOUT. It's
> kinda inconsistent (if you rely on internals of ready_status, then why
> to check the certain error code, or otherwise why not to return a real
> second error code). That's why I asked about re-using rc here.

Hmm.

1st time around, any other error code would result in us just returning directly
which is fine.
2nd time around I'm not sure we care about what the error code is.  Even if
something else went wrong we failed to recover and the first error
that lead to that was -ETIMEDOUT.

So I think this is correct as is, be it a little unusual!

Jonathan

> 
> In any case I don't think this justifies a v9, let's wait for your
> answer and Jonathan's opinion.
> 
> > +                                       data->timeout_recovery_failed = true;
> > +                               else
> > +                                       return 0;
> > +                       }
> > +               }
> > +
> > +               return rc;
> > +       }
> > +
> > +       data->timeout_recovery_failed = false;
> > +       return 0;
> > +}  
>