linux-kernel - Re: [RESEND] spi/tegra114: Correct support for cs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5241BEEC.9000508@nvidia.com>
Date:	Tue, 24 Sep 2013 12:33:48 -0400
From:	Rhyland Klein <rklein@...dia.com>
To:	Trent Piepho <tpiepho@...il.com>
CC:	Stephen Warren <swarren@...dotorg.org>,
	Simon Glass <sjg@...omium.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Olof Johansson <olof@...om.net>,
	Laxman Dewangan <ldewangan@...dia.com>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"spi-devel-general@...ts.sourceforge.net" 
	<spi-devel-general@...ts.sourceforge.net>
Subject: Re: [RESEND] spi/tegra114: Correct support for cs_change

On 9/23/2013 7:08 PM, Trent Piepho wrote:
> On Mon, Sep 23, 2013 at 2:14 PM, Stephen Warren <swarren@...dotorg.org> wrote:
>>
>> That sounds broken. Normally, shouldn't CS assert before a transaction,
>> stay asserted during a transaction, then deassert after the transaction?
>> It shouldn't rise and fall very quickly in between parts of the transaction.
> 
> That is normal, where a transaction is a spi_message made up of
> multiple spi_transfers.  The cs_change bit for a transfer will insert
> a de-asserted pulse after a transfer or leave CS de-asserted after the
> last transfer.
> 
>>>>> need to generate a falling-edge to trigger the beginning of a SPI
>>>>> transaction. Doing this write with the default value of SPI_COMMAND1
>>>>> causes a brief rise and fall of CS, giving us our falling-edge.
> 
> I wonder, is the real problem that the spi layer allows CS to possibly
> remain asserted between transactions to the same device?  Normally you
> would expect it to be de-asserted at the end of a spi_message, but I
> believe the Linux spi semantics are that it may or may not actually be
> de-asserted at that time.  It only guarantees that is will be
> de-asserted before a message to a different device starts.
> 
> I guess this is supposed to be an optimization.  Some drivers, like
> gpio bit-banging, probably have a cost associated with any CS change.
> Usually many messages in a row are to the same device.  Most devices
> don't care if CS pulses between messages.  Thus not pulsing CS between
> each message is faster.
> 
>>>
>>> Otherwise, this logic allows us to skip the spi of COMMAND1 which would
>>> normally be used to create the falling edge to start a new transaction,
>>> leaving the previous one open for more transfers.
>>
>> This sounds like something the SPI core should be managing. If a driver
>> is using the SPI bus to communicate with a device in a way that needs CS
>> left active while outside a transaction, it shouldn't be possible for
>> another driver to come along and communicate with another device before
>> the first transaction is all complete. The bus should be locked.
>> Allowing anything else could corrupt the protocol that required specific
>> CS states outside the transaction.
> 
> If the transaction is one message, which can be multiple transfers and
> multiple CS pulses, then the spi core always does it atomically.  The
> limitation is the driver can't get the result of the transaction until
> the entire transaction is finished.
> 
> If a driver needs to get part of a transaction to complete the rest,
> e.g. read a 16-bit length from the device and then read that many
> bytes, it can still be done.  It doesn't seem to be documented in
> spi-summary, but the way to do this is with spi_bus_(un)lock() and
> spi_(a)sync_locked() calls.  The driver must lock the bus, used the
> _locked versions to issue spi_messages, then unlock when done.  This
> should prevent another device on the bus from getting a messages, and
> thus CS pulses, in the middle of the transaction.

I suppose I can should reword my comment then on the code which checked:

+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);

While this does do exactly what I said, ending a previous "on-going"
transaction in favor of a new one, this shouldn't be expected to be the
way for clients to guarantee that they have locked a bus. This is more
of a way internally to the Tegra SPI driver to clear its state.

-rhyland
-- 
nvpublic
--
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/