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 <dmitry.torokhov@xxxxxxxxx>
> 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 <kjlu@xxxxxxx>
> > > ---
> > > 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(&ts->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(&client->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 <timeout> 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
