| Message ID | 20181221065919.60129-1-kjlu@umn.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | touchscreen: elants: fix a missing check of return values | expand |
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@umn.edu> > --- > 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. > + 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. > + } > > enable_irq(client->irq); > > -- > 2.17.2 (Apple Git-113) > Thanks.
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@gmail.com> > 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@umn.edu> > > > --- > > > 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.
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)); + if (error) { + dev_err(&client->dev, + "error in sending I2C messages for calibration: %d\n", + error); + return error; + } enable_irq(client->irq);
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@umn.edu> --- drivers/input/touchscreen/elants_i2c.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)