Date:	Fri, 22 Dec 2006 12:05:55 -0800
From:	David Brownell <david-b@...bell.net>
To:	Nicolas Ferre <nicolas.ferre@....atmel.com>
Cc:	Haavard Skinnemoen <hskinnemoen@...el.com>,
	Patrice Vilchez <patrice.vilchez@....atmel.com>,
	Linux Kernel list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] input/spi: add ads7843 support to ads7846 touchscreen driver

On Thursday 21 December 2006 6:40 am, Nicolas Ferre wrote:
> Nicolas Ferre a écrit :
> >>> As the SPI underlying code behaves quite differently from a 
> >>> controller driver to another whan not having a tx_buf filled,
> >>> I have add a zerroed buffer to give to the spi layer while
> >>> receiving data. 
> >>
> >> You must be working with a buggy controller driver then.  That part of
> >> this patch should never be needed.  It's expected that rx-only transfers
> >> will omit a tx buf; all controller drivers must handle that case.
> > 
> > I said that because it is true that most of spi controller drivers 
> > manage rx only transactions filling the tx buffer with zerros but the 
> > spi_s3c24xx.c driver seems to fill with ones (line 177 hw_txbyte())
> > 
> > Anyway, I will check in our controller driver to sort this out.
> 
> I dug a bit into this.

I always like to see followup on such issues.  :)


> Well, in the atmel_spi driver code, we use previous rx buffer if we do 
> not provide a tx_buf (as it is said that in struct spi_transfer 
> comments,  "If the transmit buffer is null, undefined data will be 
> shifted out while filling rx_buf").

That seems like a reasonable approach.  If it's undefined, the only
reasons I can think of to not use the rx buffer are that:

 (a) the cachelines might not be managed correctly during DMA ...
     i.e. to defend against a bug in the controller driver; and
 (b) that writing such _defined_ data would be a "covert channel"
     in the security sense.

Now, (a) is just a bug to fix, and in most cases (b) won't be an
issue since even if the system with that driver is being evaluated
at a level where such channels matter, the hardware hooked up via
SPI will probably already be trusted.  (Unless the system allows
thing like MMC or SD cards...)  However, see below.


> So, the touchscreen controller sees sometimes a "start" condition (bit 7 
> of a control byte). It then takes the control byte and sets trash bits 
> as a configuration.

Actually, now that I look at it I see that the docs for both the
ads7846 and the ads7843 show that DIN/MOSI should be zero/low
after the command is given, while reading DOUT/MISO.

Which means that the real issue here is that the driver was wrong
in the first place to not explicitly write zeroes while it's reading
back that data.

The testing I've done with it involved two controller drivers which
do happen to interpret "undefined" as "write zeros":  omap_uwire,
which I'm guessing does so in hardware (MicroWire is half duplex),
and pxa2xx_spi, which explicitly writes zeros (null_writer).


> I ran into those troubles and add a zerroed buffer as tx.
> 
> Do you think that shifting zerros out when a tx_buf is not specified is 
> the desired behavior ?

I'm leaning towards a "yes" there -- changing the spec for spi_transfer.

The alternative would seem to mean teaching the SPI interface about
two different kinds of "rx only" transfers, one with "must tx zero"
semantics and the other with "tx data doesn't matter" (and security
audits would define it anyway, to avoid covert channels).  And I can't
easily justify that.

For now, I'd suggest you update the atmel_spi driver so that it shifts
zeroes in that case; can't hurt (too much).  And I'll forward the issue
to the SPI list.  If nobody there objects, I'll send patches to update
the spec for spi_transfer, and the s3c driver.

- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists