Re: [PATCH v2 1/2] sc16is7xx: Prevent TX buffer overrun, prevent crash

[Florian Achleitner <achleitner.florian@xxxxxxxxxxx>]

Hi Jakub!

On Saturday 03 October 2015 19:52:57 Jakub Kiciński wrote:
> Hi Florian!
> 
> Thanks a lot for the submission.  Would you mind digging a little
> deeper into this problem?  I think the root cause is that the SPI
> read fails for some reason and our driver does not handle that
> properly, i.e. we don't read 255, it's just a random/failure value.
> Unfortunatelly I don't have any boards with this chip any more to
> do any tests or development myself, so it's on you ;)

No problem, the board is still on the desk :)

> This driver initially supported only I2C and in I2C regmap code if
> the read fails we will always get a zero value.  Therefore we felt
> free to ignore in sc16is7xx_port_read() the return value of
> regmap_read().
> 
> Could you please run your tests again and see if perhaps the read is
> failing?  In that case we should zero the return value from
> sc16is7xx_port_read().

I added a dev_err in *_port_read,write,update and *_fifo_read,write. No regmap 
function returned an error in my test, but the 255 read value is still there.

> I think from this thread:
> https://lkml.org/lkml/2008/2/20/271
> we can assume that zero-length writes are a valid use-case for SPI
> and if so could you please test the driver for your SPI controller?
> Perhaps the zero-length check should be placed in the SPI controller
> driver?

I digged a little deeper and did some measurements to support my idea.
I think the reason for the 255 read is that the chip does not support the zero 
length write. 

The chip's SPI interface defines two sorts of frames. One for normal register 
access, which is essentially an address followed by one byte of data, either 
read or written.

The second type is for accessing the fifo. It has an address and two bytes of 
data, by definition. 

If the master now issues a zero length write, it sends only the address byte, 
but the chip will expect two following data bytes, which do not arrive. 
Instead it will consume the following frame. When this frame is the tx fifo 
level read, the chip will not drive its SO line (still expecting a fifo 
write), and the master reads 255. Now two bytes were clocked, and they are 
back in sync. However, the value is crap.

If my theorie is true, we would also have to make sure, that fifo access is 
always two bytes to keep it synced. I will check this, and craft another 
patch, if neccessary.

> 
> I am OK with adding the sanity checks but lets first get to the bottom
> of this!

I agree. I split the patch in two parts, because I would vote for patch 1/2 
unconditionally, to prevent buffer overruns, whatever happens. 
And then, use patch 2/2 only if it is the right solution.
> 
> Thanks!

Thanks,
Florian

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html