linux-kernel - Re: [PATCH v5 1/3] spi-nor: Add support for Intel SPI serial flash 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] [day] [month] [year] [list]
Message-ID: <75195621-aeec-3d83-46ed-74ab1660fd5f@gmail.com>
Date:   Fri, 25 Nov 2016 18:26:10 +0100
From:   Marek Vasut <marek.vasut@...il.com>
To:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        linux-mtd@...ts.infradead.org
Cc:     Cyrille Pitchen <cyrille.pitchen@...el.com>,
        Boris Brezillon <boris.brezillon@...e-electrons.com>,
        Richard Weinberger <richard@....at>,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Lee Jones <lee.jones@...aro.org>,
        Peter Tyser <ptyser@...-inc.com>, key.seong.lim@...el.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/3] spi-nor: Add support for Intel SPI serial flash
 controller

On 11/14/2016 11:24 AM, Mika Westerberg wrote:
> Add support for the SPI serial flash host controller found on many Intel
> CPUs including Baytrail and Braswell. The SPI serial flash controller is
> used to access BIOS and other platform specific information. By default the
> driver exposes a single read-only MTD device but with a module parameter
> "writeable=1" the MTD device can be made read-write which makes it possible
> to upgrade BIOS directly from Linux.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@...ux.intel.com>

Only minor nits below. Sorry I took so long with the review.

[...]

> +static int intel_spi_wait_hw_busy(struct intel_spi *ispi)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(INTEL_SPI_TIMEOUT);
> +	u32 hwstat;

Won't some include/linux/iopoll.h macro help here ?

> +	while (!time_after(jiffies, timeout)) {
> +		hwstat = readl(ispi->base + HSFSTS_CTL);
> +		if (!(hwstat & HSFSTS_CTL_SCIP))
> +			return 0;
> +		cond_resched();
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int intel_spi_wait_sw_busy(struct intel_spi *ispi)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(INTEL_SPI_TIMEOUT);
> +	u32 swstat;
> +
> +	while (!time_after(jiffies, timeout)) {

DTTO

> +		swstat = readl(ispi->sregs + SSFSTS_CTL);
> +		if (!(swstat & SSFSTS_CTL_SCIP))
> +			return 0;
> +		cond_resched();
> +	}
> +
> +	return -ETIMEDOUT;
> +}

[...]

> +static ssize_t intel_spi_read(struct spi_nor *nor, loff_t from, size_t len,
> +			      u_char *read_buf)
> +{
> +	struct intel_spi *ispi = nor->priv;
> +	size_t block_size, retlen = 0;
> +	u32 val, status;
> +	ssize_t ret;
> +
> +	switch (nor->read_opcode) {
> +	case SPINOR_OP_READ:
> +	case SPINOR_OP_READ_FAST:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	while (len > 0) {
> +		block_size = min_t(size_t, len, INTEL_SPI_FIFO_SZ);
> +
> +		writel(from, ispi->base + FADDR);
> +
> +		val = readl(ispi->base + HSFSTS_CTL);
> +		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> +		val &= ~HSFSTS_CTL_FDBC_MASK;
> +		val |= (block_size - 1) << HSFSTS_CTL_FDBC_SHIFT;
> +		val &= ~HSFSTS_CTL_FCYCLE_MASK;
> +		val |= HSFSTS_CTL_FCYCLE_READ;
> +		val |= HSFSTS_CTL_FGO;

Sort those bit clears and bit sets so you first clear and then set bits.

> +		writel(val, ispi->base + HSFSTS_CTL);
> +
> +		ret = intel_spi_wait_hw_busy(ispi);
> +		if (ret)
> +			return ret;
> +
> +		status = readl(ispi->base + HSFSTS_CTL);
> +		if (status & HSFSTS_CTL_FCERR)
> +			ret = -EIO;
> +		else if (status & HSFSTS_CTL_AEL)
> +			ret = -EACCES;
> +
> +		if (ret < 0) {
> +			dev_err(ispi->dev, "read error: %llx: %#x\n", from,
> +				status);
> +			return ret;
> +		}
> +
> +		ret = intel_spi_read_block(ispi, read_buf, block_size);
> +		if (ret)
> +			return ret;
> +
> +		len -= block_size;
> +		from += block_size;
> +		retlen += block_size;
> +		read_buf += block_size;
> +	}
> +
> +	return retlen;
> +}
> +
> +static ssize_t intel_spi_write(struct spi_nor *nor, loff_t to, size_t len,
> +			       const u_char *write_buf)
> +{
> +	struct intel_spi *ispi = nor->priv;
> +	size_t block_size, retlen = 0;
> +	u32 val, status;
> +	ssize_t ret;
> +
> +	while (len > 0) {
> +		block_size = min_t(size_t, len, INTEL_SPI_FIFO_SZ);
> +
> +		writel(to, ispi->base + FADDR);
> +
> +		val = readl(ispi->base + HSFSTS_CTL);
> +		val |= HSFSTS_CTL_AEL | HSFSTS_CTL_FCERR | HSFSTS_CTL_FDONE;
> +		val &= ~HSFSTS_CTL_FDBC_MASK;
> +		val |= (block_size - 1) << HSFSTS_CTL_FDBC_SHIFT;
> +		val &= ~HSFSTS_CTL_FCYCLE_MASK;
> +		val |= HSFSTS_CTL_FCYCLE_WRITE;

DTTO here.

> +		/* Write enable */
> +		if (ispi->preopcodes[1] == SPINOR_OP_WREN)
> +			val |= SSFSTS_CTL_SPOP;
> +		val |= SSFSTS_CTL_ACS;
> +		writel(val, ispi->base + HSFSTS_CTL);
> +
> +		ret = intel_spi_write_block(ispi, write_buf, block_size);
> +		if (ret) {
> +			dev_err(ispi->dev, "failed to write block\n");
> +			return ret;
> +		}
> +
> +		/* Start the write now */
> +		val = readl(ispi->base + HSFSTS_CTL);
> +		writel(val | HSFSTS_CTL_FGO, ispi->base + HSFSTS_CTL);
> +
> +		ret = intel_spi_wait_hw_busy(ispi);
> +		if (ret) {
> +			dev_err(ispi->dev, "timeout\n");
> +			return ret;
> +		}
> +
> +		status = readl(ispi->base + HSFSTS_CTL);
> +		if (status & HSFSTS_CTL_FCERR)
> +			ret = -EIO;
> +		else if (status & HSFSTS_CTL_AEL)
> +			ret = -EACCES;
> +
> +		if (ret < 0) {
> +			dev_err(ispi->dev, "write error: %llx: %#x\n", to,
> +				status);
> +			return ret;
> +		}
> +
> +		len -= block_size;
> +		to += block_size;
> +		retlen += block_size;
> +		write_buf += block_size;
> +	}
> +
> +	return retlen;
> +}

[...]

-- 
Best regards,
Marek Vasut