linux-kernel - Re: MCP251x SPI CAN controller on Cavium ThunderX

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]

Message-ID: <1feffea6-00de-6e73-309a-bd9619c19666@pengutronix.de>
Date:   Wed, 15 Nov 2017 14:31:45 +0100
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Jan Glauber <jan.glauber@...iumnetworks.com>
Cc:     Mark Brown <broonie@...nel.org>,
        Tim Harvey <tharvey@...eworks.com>, linux-spi@...r.kernel.org,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        linux-can <linux-can@...r.kernel.org>
Subject: Re: MCP251x SPI CAN controller on Cavium ThunderX

On 11/15/2017 01:40 PM, Marc Kleine-Budde wrote:
> mcp251x_spi_trans() is called with len=3,
> priv->spi_tx_buf and priv->spi_rx_buf point to previously allocared memory
> 
> priv->spi_tx_buf has been filled before calling mcp251x_spi_trans().

> #define OCTEON_SPI_MAX_BYTES 9

> static int octeon_spi_do_transfer(struct octeon_spi *p,
> 				  struct spi_message *msg,
> 				  struct spi_transfer *xfer,
> 				  bool last_xfer)
> {
> 	struct spi_device *spi = msg->spi;
> 	union cvmx_mpi_cfg mpi_cfg;
> 	union cvmx_mpi_tx mpi_tx;
> 	unsigned int clkdiv;
> 	int mode;
> 	bool cpha, cpol;
> 	const u8 *tx_buf;
> 	u8 *rx_buf;
> 	int len;
> 	int i;
> 
> 	mode = spi->mode;
> 	cpha = mode & SPI_CPHA;
> 	cpol = mode & SPI_CPOL;
> 
> 	clkdiv = p->sys_freq / (2 * xfer->speed_hz);
> 
> 	mpi_cfg.u64 = 0;
> 
> 	mpi_cfg.s.clkdiv = clkdiv;
> 	mpi_cfg.s.cshi = (mode & SPI_CS_HIGH) ? 1 : 0;
> 	mpi_cfg.s.lsbfirst = (mode & SPI_LSB_FIRST) ? 1 : 0;
> 	mpi_cfg.s.wireor = (mode & SPI_3WIRE) ? 1 : 0;
> 	mpi_cfg.s.idlelo = cpha != cpol;
> 	mpi_cfg.s.cslate = cpha ? 1 : 0;
> 	mpi_cfg.s.enable = 1;
> 
> 	if (spi->chip_select < 4)
> 		p->cs_enax |= 1ull << (12 + spi->chip_select);
> 	mpi_cfg.u64 |= p->cs_enax;
> 
> 	if (mpi_cfg.u64 != p->last_cfg) {
> 		p->last_cfg = mpi_cfg.u64;
> 		writeq(mpi_cfg.u64, p->register_base + OCTEON_SPI_CFG(p));
> 	}
> 	tx_buf = xfer->tx_buf;
> 	rx_buf = xfer->rx_buf;
> 	len = xfer->len;
> 	while (len > OCTEON_SPI_MAX_BYTES) {
> 		for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
> 			u8 d;
> 			if (tx_buf)
> 				d = *tx_buf++;
> 			else
> 				d = 0;
> 			writeq(d, p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 		}
> 		mpi_tx.u64 = 0;
> 		mpi_tx.s.csid = spi->chip_select;
> 		mpi_tx.s.leavecs = 1;
> 		mpi_tx.s.txnum = tx_buf ? OCTEON_SPI_MAX_BYTES : 0;

This looks fishy, OCTEON_SPI_MAX_BYTES is 9....

> 		mpi_tx.s.totnum = OCTEON_SPI_MAX_BYTES;
> 		writeq(mpi_tx.u64, p->register_base + OCTEON_SPI_TX(p));
> 
> 		octeon_spi_wait_ready(p);
> 		if (rx_buf)
> 			for (i = 0; i < OCTEON_SPI_MAX_BYTES; i++) {
> 				u64 v = readq(p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 				*rx_buf++ = (u8)v;
> 			}
> 		len -= OCTEON_SPI_MAX_BYTES;
> 	}
> 
> 	for (i = 0; i < len; i++) {
> 		u8 d;
> 		if (tx_buf)
> 			d = *tx_buf++;
> 		else
> 			d = 0;
> 		writeq(d, p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 	}
> 
> 	mpi_tx.u64 = 0;
> 	mpi_tx.s.csid = spi->chip_select;
> 	if (last_xfer)
> 		mpi_tx.s.leavecs = xfer->cs_change;
> 	else
> 		mpi_tx.s.leavecs = !xfer->cs_change;
> 	mpi_tx.s.txnum = tx_buf ? len : 0;
> 	mpi_tx.s.totnum = len;
> 	writeq(mpi_tx.u64, p->register_base + OCTEON_SPI_TX(p));
> 
> 	octeon_spi_wait_ready(p);
> 	if (rx_buf)
> 		for (i = 0; i < len; i++) {
> 			u64 v = readq(p->register_base + OCTEON_SPI_DAT0(p) + (8 * i));
> 			*rx_buf++ = (u8)v;
> 		}

Personally I'd fold this into the while loop, as there's quite some code
duplication. Of course your have to improve the "if (last_xfer)" a bit.

> 
> 	if (xfer->delay_usecs)
> 		udelay(xfer->delay_usecs);
> 
> 	return xfer->len;
> }

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |



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