Re: Input: cyttsp4 - SPI driver for Cypress TMA4XX touchscreen devices

Re: Input: cyttsp4 - SPI driver for Cypress TMA4XX touchscreen devices

[Geert Uytterhoeven]

On Fri, Jul 5, 2013 at 1:51 AM, Linux Kernel Mailing List
<linux-kernel@vger.kernel.org> wrote:
> +++ b/drivers/input/touchscreen/cyttsp4_spi.c

> +static int cyttsp_spi_xfer(struct device *dev, u8 *xfer_buf,
> +                          u8 op, u8 reg, u8 *buf, int length)
> +{

> +       if (reg > 255)

As "reg" is "u8", this is never true:

drivers/input/touchscreen/cyttsp4_spi.c: In function ‘cyttsp_spi_xfer’:
drivers/input/touchscreen/cyttsp4_spi.c:66: warning: comparison is
always false due to limited range of data type

> +               wr_buf[0] = op + CY_SPI_A8_BIT;
> +       else
> +               wr_buf[0] = op;

Can the if-clause and the first branch just be removed, or is there a real bug
involved (e.g. wrong type for "reg")?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Input: cyttsp4 - SPI driver for Cypress TMA4XX touchscreen devices

[Ferruh Yigit]

On 07/07/2013 10:15 PM, Geert Uytterhoeven wrote:
> On Fri, Jul 5, 2013 at 1:51 AM, Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org> wrote:
>> +++ b/drivers/input/touchscreen/cyttsp4_spi.c
>
>> +static int cyttsp_spi_xfer(struct device *dev, u8 *xfer_buf,
>> +                          u8 op, u8 reg, u8 *buf, int length)
>> +{
>
>> +       if (reg > 255)
>
> As "reg" is "u8", this is never true:
>
> drivers/input/touchscreen/cyttsp4_spi.c: In function ‘cyttsp_spi_xfer’:
> drivers/input/touchscreen/cyttsp4_spi.c:66: warning: comparison is
> always false due to limited range of data type
>
>> +               wr_buf[0] = op + CY_SPI_A8_BIT;
>> +       else
>> +               wr_buf[0] = op;
>
> Can the if-clause and the first branch just be removed, or is there a real bug
> involved (e.g. wrong type for "reg")?

Yes there was a bug here, and already sent a patch for this, please
check https://patchwork.kernel.org/patch/2820561/

thanks,
ferruh


This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message.

re: Input: cyttsp4 - SPI driver for Cypress TMA4XX touchscreen devices

[Dan Carpenter]

Hello Ferruh Yigit,

The patch 67bf12ca50d5: "Input: cyttsp4 - SPI driver for Cypress
TMA4XX touchscreen devices" from Jun 30, 2013, leads to the following
Smatch warning:
"drivers/input/touchscreen/cyttsp4_spi.c:66 cyttsp_spi_xfer()
	 warn: impossible condition '(reg > 255) => (0-255 > 255)'"

drivers/input/touchscreen/cyttsp4_spi.c
    66          if (reg > 255)
                    ^^^^^^^^^
"reg" is unsigned char so 255 is the max it can be.
    67                  wr_buf[0] = op + CY_SPI_A8_BIT;
    68          else
    69                  wr_buf[0] = op;

regards,
dan carpenter

Re: Input: cyttsp4 - SPI driver for Cypress TMA4XX touchscreen devices

[Ferruh Yigit]

On 07/03/2013 12:47 AM, Dan Carpenter wrote:
> Hello Ferruh Yigit,
>
> The patch 67bf12ca50d5: "Input: cyttsp4 - SPI driver for Cypress
> TMA4XX touchscreen devices" from Jun 30, 2013, leads to the following
> Smatch warning:
> "drivers/input/touchscreen/cyttsp4_spi.c:66 cyttsp_spi_xfer()
>        warn: impossible condition '(reg > 255) => (0-255 > 255)'"
>
> drivers/input/touchscreen/cyttsp4_spi.c
>      66          if (reg > 255)
>                      ^^^^^^^^^
> "reg" is unsigned char so 255 is the max it can be.
>      67                  wr_buf[0] = op + CY_SPI_A8_BIT;
>      68          else
>      69                  wr_buf[0] = op;
>
> regards,
> dan carpenter
>
Hi Dan,

Shortly, check is correct and address should be 16 bit, parameter size
is wrong.

In TSG4, register map is 512bytes long and to access all of it, one bit
from address byte is used (which bit to use differs for I2C and SPI),
the code you marked is doing that check.

Since common code used TSG3 and TSG4 for I2C, this parameter wrongly
used as u8. TSG3 does not access beyond 255 bytes but TSG4 may.

I have a patch for this, but should I create it against previous patch I
have sent or against latest next tree?

--

Thanks,
ferruh

This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message.

Re: Input: cyttsp4 - SPI driver for Cypress TMA4XX touchscreen devices

[Dan Carpenter]

On Wed, Jul 03, 2013 at 04:24:18PM +0300, Ferruh Yigit wrote:
> I have a patch for this, but should I create it against previous patch I
> have sent or against latest next tree?
> 

Latest linux-next.

regards,
dan carpenter