Re: [PATCH] touchscreen: elants: fix a missing check of return values
On Fri, Dec 21, 2018 at 03:05:29PM -0600, Kangjie Lu wrote:
> Hi Dmitry,
>
> Thanks for the feedback.
>
> On Fri, Dec 21, 2018 at 2:27 AM Dmitry Torokhov
> wrote:
>
> > Hi Kangjie,
> >
> > On Fri, Dec 21, 2018 at 12:59:16AM -0600, Kangjie Lu wrote:
> > > elants_i2c_send() may fail, let's check its return values. The fix does
> > > the check and reports an error message upon the failure.
> > >
> > > Signed-off-by: Kangjie Lu
> > > ---
> > > drivers/input/touchscreen/elants_i2c.c | 10 --
> > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/input/touchscreen/elants_i2c.c
> > b/drivers/input/touchscreen/elants_i2c.c
> > > index f2cb23121833..cb3c1470bb68 100644
> > > --- a/drivers/input/touchscreen/elants_i2c.c
> > > +++ b/drivers/input/touchscreen/elants_i2c.c
> > > @@ -245,8 +245,14 @@ static int elants_i2c_calibrate(struct elants_data
> > *ts)
> > > ts->state = ELAN_WAIT_RECALIBRATION;
> > > reinit_completion(>cmd_done);
> > >
> > > - elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
> > > - elants_i2c_send(client, rek, sizeof(rek));
> > > + error = elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
> > > + error |= elants_i2c_send(client, rek, sizeof(rek));
> >
> > I dislike this kind of error handling as this may result in invalid
> > error code being reported, in case 2 commands produce different results.
> >
>
> I will fix this.
>
>
> > > + if (error) {
> > > + dev_err(>dev,
> > > + "error in sending I2C messages for
> > calibration: %d\n",
> > > + error);
> > > + return error;
> >
> > If we just return like you do it here, interrupts will stay disabled and
> > touchscreen will be completely dead. With the old code we'd report
> > timeout on calibration, and touchscreen would have chance of working. We
> > would also be able to retry calibration.
> >
>
> How about this: we print out the error message but still continue the
> following execution?
Yes, we could do dev_warn() here, but elants_i2c_send() already logs
errors, so I do not see much benefit from doing this.
> Also, if elants_i2c_send() fails,
> would wait_for_completion_interruptible_timeout() always capture a timeout?
Well, if controller does not get the calibration command(s) it will not
do anything and at worst in time
wait_for_completion_interruptible_timeout() will return and we will
properly report this condition.
Another option is to rearrange the code to ensure we are not leaving
interrupts disabled on error.
Thanks.
--
Dmitry
Re: [PATCH] touchscreen: elants: fix a missing check of return values
Hi Kangjie,
On Fri, Dec 21, 2018 at 12:59:16AM -0600, Kangjie Lu wrote:
> elants_i2c_send() may fail, let's check its return values. The fix does
> the check and reports an error message upon the failure.
>
> Signed-off-by: Kangjie Lu
> ---
> drivers/input/touchscreen/elants_i2c.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/touchscreen/elants_i2c.c
> b/drivers/input/touchscreen/elants_i2c.c
> index f2cb23121833..cb3c1470bb68 100644
> --- a/drivers/input/touchscreen/elants_i2c.c
> +++ b/drivers/input/touchscreen/elants_i2c.c
> @@ -245,8 +245,14 @@ static int elants_i2c_calibrate(struct elants_data *ts)
> ts->state = ELAN_WAIT_RECALIBRATION;
> reinit_completion(>cmd_done);
>
> - elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
> - elants_i2c_send(client, rek, sizeof(rek));
> + error = elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
> + error |= elants_i2c_send(client, rek, sizeof(rek));
I dislike this kind of error handling as this may result in invalid
error code being reported, in case 2 commands produce different results.
> + if (error) {
> + dev_err(>dev,
> + "error in sending I2C messages for calibration:
> %d\n",
> + error);
> + return error;
If we just return like you do it here, interrupts will stay disabled and
touchscreen will be completely dead. With the old code we'd report
timeout on calibration, and touchscreen would have chance of working. We
would also be able to retry calibration.
> + }
>
> enable_irq(client->irq);
>
> --
> 2.17.2 (Apple Git-113)
>
Thanks.
--
Dmitry
[PATCH] touchscreen: elants: fix a missing check of return values
elants_i2c_send() may fail, let's check its return values. The fix does
the check and reports an error message upon the failure.
Signed-off-by: Kangjie Lu
---
drivers/input/touchscreen/elants_i2c.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/input/touchscreen/elants_i2c.c
b/drivers/input/touchscreen/elants_i2c.c
index f2cb23121833..cb3c1470bb68 100644
--- a/drivers/input/touchscreen/elants_i2c.c
+++ b/drivers/input/touchscreen/elants_i2c.c
@@ -245,8 +245,14 @@ static int elants_i2c_calibrate(struct elants_data *ts)
ts->state = ELAN_WAIT_RECALIBRATION;
reinit_completion(>cmd_done);
- elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
- elants_i2c_send(client, rek, sizeof(rek));
+ error = elants_i2c_send(client, w_flashkey, sizeof(w_flashkey));
+ error |= elants_i2c_send(client, rek, sizeof(rek));
+ if (error) {
+ dev_err(>dev,
+ "error in sending I2C messages for calibration:
%d\n",
+ error);
+ return error;
+ }
enable_irq(client->irq);
--
2.17.2 (Apple Git-113)
