Re: [PATCH v2 1/2] Input: mms114 - use smbus functions whenever possible

[Andi Shyti <andi@etezian.org>]

Hi Stephan,

> Not sure if you saw my comment regarding your patch [1],
> so I'll just repeat it properly inline here:
> 
> [1]: https://patchwork.kernel.org/patch/11178515/#22927311
> 
> On Sun, Oct 20, 2019 at 11:28:55PM +0300, Andi Shyti wrote:
> > The exchange of data to and from the mms114 touchscreen never
> > exceeds 256 bytes. In the worst case it goes up to 80 bytes in
> > the interrupt handler while reading the events.
> > 
> 
> i2c_smbus_read_i2c_block_data() is actually limited to
> I2C_SMBUS_BLOCK_MAX = 32.

oh sorry, I don't know how I slipped on this.

But this means that the i2c in the kernel is wrong (or outdated),
smbus specifies 256 bytes of data[*]. I might have relied on the
specification more than the code.

I guess SMBUS needs some update.

> > diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> > index a5ab774da4cc..170dcb5312b9 100644
> > --- a/drivers/input/touchscreen/mms114.c
> > +++ b/drivers/input/touchscreen/mms114.c
> > @@ -204,14 +204,15 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
> >  	}
> >  	mutex_unlock(&input_dev->mutex);
> >  
> > -	packet_size = mms114_read_reg(data, MMS114_PACKET_SIZE);
> > +	packet_size = i2c_smbus_read_byte_data(data->client,
> > +					       MMS114_PACKET_SIZE);
> >  	if (packet_size <= 0)
> >  		goto out;
> >  
> >  	touch_size = packet_size / MMS114_PACKET_NUM;
> >  
> > -	error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
> > -			(u8 *)touch);
> > +	error = i2c_smbus_read_i2c_block_data(data->client, MMS114_INFORMATION,
> > +					      packet_size, (u8 *)touch);
> 
> ... and here we try to read up to 80 bytes, as you mentioned.
> 
> i2c_smbus_read_i2c_block_data() will silently fall back to reading only
> 32 bytes. Therefore, if we try to read more than 32 bytes here we will
> later read uninitialized data.
> 
> With this change, if you use more than 4 fingers you can easily trigger
> a situation where one of the fingers gets "stuck", together with:
>   mms114 4-0048: Wrong touch type (0)

yes, it can be that for this there might be some delays or miss
reads.

> So we still need the custom functions here, or maybe avoid the problem
> by using regmap instead.

This was what Dmitry was proposing almost a couple of years ago,
but then I stopped working on this.

> > +		error = i2c_smbus_read_i2c_block_data(data->client,
> > +						      MMS152_FW_REV, 3, buf);
> >  		if (error)
> 
> i2c_smbus_read_i2c_block_data() returns the number of read bytes,
> therefore this check will always fail.
> 
> It should be: if (error < 0)

> > +		error = i2c_smbus_read_i2c_block_data(data->client,
> > +						      MMS114_TSP_REV, 6, buf);
> >  		if (error)
> 
> As above.

Yes, an oversight in these two cases. Thanks!

Well, I guess some more rework needs to be done in this patch...
at the end Dmitry was right :)

Thanks,
Andi

[*] http://www.smbus.org/specs/