[bug report] Input: add driver for Hynitron cstxxx touchscreens

[bug report] Input: add driver for Hynitron cstxxx touchscreens

[Dan Carpenter]

[ I sent this a couple weeks back, but it turns out that mutt + msmtp
  has been silently eating my emails instead of sending them so I'm
  resending two weeks of email. -dan ]

Hello Chris Morgan,

The patch 66603243f528: "Input: add driver for Hynitron cstxxx
touchscreens" from Oct 28, 2022, leads to the following Smatch static
checker warning:

	drivers/input/touchscreen/hynitron_cstxxx.c:238 cst3xx_bootloader_enter()
	error: uninitialized symbol 'tmp'.

drivers/input/touchscreen/hynitron_cstxxx.c
    209 static int cst3xx_bootloader_enter(struct i2c_client *client)
    210 {
    211         int err;
    212         u8 retry;
    213         u32 tmp;
    214         unsigned char buf[3];
    215 
    216         for (retry = 0; retry < 5; retry++) {
    217                 hyn_reset_proc(client, (7 + retry));

I would have changed this to a while (retry--) { loop except the retry
value probably matters here.

    218                 /* set cmd to enter program mode */
    219                 put_unaligned_le24(CST3XX_BOOTLDR_PROG_CMD, buf);
    220                 err = cst3xx_i2c_write(client, buf, 3);
    221                 if (err)
    222                         continue;
    223 
    224                 usleep_range(2000, 2500);
    225 
    226                 /* check whether in program mode */
    227                 err = cst3xx_i2c_read_register(client,
    228                                                CST3XX_BOOTLDR_PROG_CHK_REG,
    229                                                buf, 1);
    230                 if (err)
    231                         continue;
    232 
    233                 tmp = get_unaligned(buf);
    234                 if (tmp == CST3XX_BOOTLDR_CHK_VAL)
    235                         break;
    236         }
    237 
--> 238         if (tmp != CST3XX_BOOTLDR_CHK_VAL) {

This is a genuine bug.  It should be checking if retry == 5 but maybe
with a define instead of a magic 5.

    239                 dev_err(&client->dev, "%s unable to enter bootloader mode\n",
    240                         __func__);
    241                 return -ENODEV;
    242         }
    243 
    244         hyn_reset_proc(client, 40);
    245 
    246         return 0;
    247 }

regards,
dan carpenter

Re: [bug report] Input: add driver for Hynitron cstxxx touchscreens

[Dan Carpenter]

On Tue, Nov 15, 2022 at 03:36:18PM +0000, Christopher Morgan wrote:
> On Tue, Nov 15, 2022 at 03:43:39PM +0300, Dan Carpenter wrote:
> > [ I sent this a couple weeks back, but it turns out that mutt + msmtp
> >   has been silently eating my emails instead of sending them so I'm
> >   resending two weeks of email. -dan ]
> >
> > Hello Chris Morgan,
> >
> > The patch 66603243f528: "Input: add driver for Hynitron cstxxx
> > touchscreens" from Oct 28, 2022, leads to the following Smatch static
> > checker warning:
> 
> Apologies, but I'm a surprisingly un-skilled programmer.

Heh.  Every several years I realize that, "Nope.  I still don't
understand pointers."

> I'll be happy to fix the bugs though I just want to make sure I
> understand the problem clearly first.
> 
> >
> >        drivers/input/touchscreen/hynitron_cstxxx.c:238 cst3xx_bootloader_enter()
> >        error: uninitialized symbol 'tmp'.
> >
> 
> Does this mean I need to set the inital value of the tmp variable to 0? Looking
> at the code more closely I'm assuming this is the issue because if it runs through
> the loop 5 times and errors each time it will exit the loop without setting a
> value for tmp.

Setting it to zero works.  Although I had to look up that
CST3XX_BOOTLDR_CHK_VAL is not zero...  But presumably the people who
know the code will understand that right away.

> 
> > drivers/input/touchscreen/hynitron_cstxxx.c
> >     209 static int cst3xx_bootloader_enter(struct i2c_client *client)
> >     210 {
> >     211         int err;
> >     212         u8 retry;
> >     213         u32 tmp;
> >     214         unsigned char buf[3];
> >     215
> >     216         for (retry = 0; retry < 5; retry++) {
> >     217                 hyn_reset_proc(client, (7 + retry));
> >
> > I would have changed this to a while (retry--) { loop except the retry
> > value probably matters here.
> 
> Is that personal prefrence or guidance? The BSP driver did the incremental
> loop 5 times, so that's why I did it here.
> 

Yeah.  Just a personal preference.  I was trying to avoid using the
number 5 twice...  Never mind about what I said here.  Just initialize
it to zero.

regards,
dan carpenter