linux-kernel - Re: [PATCH] spi: tegra: add spi driver for SLINK controller

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] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 23 Oct 2012 10:17:13 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Laxman Dewangan <ldewangan@...dia.com>, grant.likely@...retlab.ca,
	rob.herring@...xeda.com, spi-devel-general@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH] spi: tegra: add spi driver for SLINK controller

On Mon, Oct 22, 2012 at 02:02:47PM -0600, Stephen Warren wrote:
> On 10/18/2012 04:47 AM, Laxman Dewangan wrote:

> > +	/* Read back register to make sure that register writes completed */
> > +	if (reg != SLINK_TX_FIFO)
> > +		readl(tspi->base + SLINK_MAS_DATA);

> Is that really necessary on every single write, or only for certain
> specific operations? This seems rather heavy-weight.

I saw that myself - I figured that since SPI is (relatively) slow itself
the simplicity gained from doing the flush unconditionally was
reasonable, further optimisation can always be done later.

> > +	val = tegra_slink_readl(tspi, SLINK_STATUS);

> > +	/* Write 1 to clear status register */
> > +	val_write = SLINK_RDY;
> > +	val_write |= (val & SLINK_FIFO_ERROR);

> Why not just always set SLINK_FIFO_ERROR; does it have to be set in the
> write only if the status was previously asserted? If that is true, how
> do you avoid a race condition where the bit gets set in SLINK_STATUS
> after you read it but before you write to clear it?

The whole point with this sort of interrupt scheme is usually that the
interrupt is level triggered and will just continue to be asserted if
some source within the interrupt isn't acked; the race is usually the
other way around where you may ack a status you didn't see.

> > +static unsigned int tegra_slink_read_rx_fifo_to_client_rxbuf(
> 
> > +		bits_per_word = t->bits_per_word ? t->bits_per_word :
> > +						tspi->cur_spi->bits_per_word;

> That logic is repeated a few times. Shouldn't there be a macro to avoid
> cut/paste. Perhaps even in the SPI core rather than this driver.

Yes, should be in the SPI core as this pattern exists for all SPI
drivers.

> Doesn't this function need to parse all the other standardized
> properties from spi-bus.txt, or does the SPI core handle that?

No, it doesn't.  But perhaps it should (see the thing above about the
per-transfer max frequency too...).  In general SPI hasn't done much
about factoring code out of drivers until very recently.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)