| 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
| ||
|
Message-ID: <20140320072744.GB31517@norris-Latitude-E6410>
Date: Thu, 20 Mar 2014 00:27:44 -0700
From: Brian Norris <computersforpeace@...il.com>
To: Lee Jones <lee.jones@...aro.org>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
DCG_UPD_stlinux_kernel@...t.st.com, dwmw2@...radead.org,
linux-mtd@...ts.infradead.org, Angus.Clark@...com
Subject: Re: [PATCH 27/35] mtd: st_spi_fsm: Supply a busy wait for post-write
status
On Tue, Feb 18, 2014 at 02:55:54PM +0000, Lee Jones wrote:
> When we write data to the Serial Flash chip we'll wait a predetermined
> period of time before giving up. During that period of time we poll the
> status register until completion.
>
> Acked-by Angus Clark <angus.clark@...com>
> Signed-off-by: Lee Jones <lee.jones@...aro.org>
> ---
> drivers/mtd/devices/st_spi_fsm.c | 223 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 223 insertions(+)
>
> diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c
> index ea55548..0eacfbf 100644
> --- a/drivers/mtd/devices/st_spi_fsm.c
> +++ b/drivers/mtd/devices/st_spi_fsm.c
> @@ -238,8 +238,18 @@
> #define FLASH_CMD_READ4_1_1_4 0x6c
> #define FLASH_CMD_READ4_1_4_4 0xec
>
> +/* Status register */
> +#define FLASH_STATUS_BUSY 0x01
> +#define FLASH_STATUS_WEL 0x02
> +#define FLASH_STATUS_BP0 0x04
> +#define FLASH_STATUS_BP1 0x08
> +#define FLASH_STATUS_BP2 0x10
> +#define FLASH_STATUS_SRWP0 0x80
> +#define FLASH_STATUS_TIMEOUT 0xff
> +
> #define FLASH_PAGESIZE 256 /* In Bytes */
> #define FLASH_PAGESIZE_32 (FLASH_PAGESIZE / 4) /* In uint32_t */
> +#define FLASH_MAX_BUSY_WAIT (300 * HZ) /* Maximum 'CHIPERASE' time */
>
> /*
> * Flags to tweak operation of default read/write/erase routines
> @@ -519,6 +529,22 @@ static struct stfsm_seq stfsm_seq_read_jedec = {
> SEQ_CFG_STARTSEQ),
> };
>
> +static struct stfsm_seq stfsm_seq_read_status_fifo = {
> + .data_size = TRANSFER_SIZE(4),
> + .seq_opc[0] = (SEQ_OPC_PADS_1 |
> + SEQ_OPC_CYCLES(8) |
> + SEQ_OPC_OPCODE(FLASH_CMD_RDSR)),
> + .seq = {
> + STFSM_INST_CMD1,
> + STFSM_INST_DATA_READ,
> + STFSM_INST_STOP,
> + },
> + .seq_cfg = (SEQ_CFG_PADS_1 |
> + SEQ_CFG_READNOTWRITE |
> + SEQ_CFG_CSDEASSERT |
> + SEQ_CFG_STARTSEQ),
> +};
> +
> static struct stfsm_seq stfsm_seq_erase_sector = {
> /* 'addr_cfg' configured during initialisation */
> .seq_opc = {
> @@ -699,6 +725,53 @@ static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter)
> return 0;
> }
>
> +static uint8_t stfsm_wait_busy(struct stfsm *fsm)
> +{
> + struct stfsm_seq *seq = &stfsm_seq_read_status_fifo;
> + unsigned long deadline;
> + uint32_t status;
> + int timeout = 0;
> +
> + /* Use RDRS1 */
> + seq->seq_opc[0] = (SEQ_OPC_PADS_1 |
> + SEQ_OPC_CYCLES(8) |
> + SEQ_OPC_OPCODE(FLASH_CMD_RDSR));
> +
> + /* Load read_status sequence */
> + stfsm_load_seq(fsm, seq);
> +
> + /*
> + * Repeat until busy bit is deasserted, or timeout, or error (S25FLxxxS)
> + */
> + deadline = jiffies + FLASH_MAX_BUSY_WAIT;
> + while (!timeout) {
> + cond_resched();
Do you really want to give up the CPU right from the start? Shouldn't
you run through the loop once first? Maybe move this to the end of the
while loop.
> +
> + if (time_after_eq(jiffies, deadline))
> + timeout = 1;
> +
> + stfsm_wait_seq(fsm);
> +
> + stfsm_read_fifo(fsm, &status, 4);
> +
> + if ((status & FLASH_STATUS_BUSY) == 0)
> + return 0;
> +
> + if ((fsm->configuration & CFG_S25FL_CHECK_ERROR_FLAGS) &&
> + ((status & S25FL_STATUS_P_ERR) ||
> + (status & S25FL_STATUS_E_ERR)))
> + return (uint8_t)(status & 0xff);
> +
> + if (!timeout)
> + /* Restart */
> + writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG);
> + }
> +
> + dev_err(fsm->dev, "timeout on wait_busy\n");
> +
> + return FLASH_STATUS_TIMEOUT;
> +}
> +
> static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data)
> {
> struct stfsm_seq *seq = &stfsm_seq_wrvcr;
> @@ -1020,6 +1093,98 @@ static int stfsm_read(struct stfsm *fsm, uint8_t *buf, uint32_t size,
> return 0;
> }
>
> +static int stfsm_write(struct stfsm *fsm, const uint8_t *const buf,
> + const uint32_t size, const uint32_t offset)
I'm still not sure about addint the 'const' label to these local
parameters. It doesn't add any value to the callers (they are
passed-by-value anyway), nor are they very useful locally. I mentioned
this in my first review, and you never commented. I won't insist you
drop the 'const', if you really prefer it...
My suggestion:
const uint8_t *const buf => const uint8_t *buf
const uint32_t size => uint32_t size
const uint32_t offset => uint32_t offset
(For the first one: a pointer to constant data is a useful type
annotation; a constant pointer to data is not.)
Similar patterns apply elsewhere.
> +{
> + struct stfsm_seq *seq = &stfsm_seq_write;
> + uint32_t data_pads;
> + uint32_t write_mask;
> + uint32_t size_ub;
> + uint32_t size_lb;
> + uint32_t size_mop;
> + uint32_t tmp[4];
> + uint32_t page_buf[FLASH_PAGESIZE_32];
> + uint8_t *t = (uint8_t *)&tmp;
> + const uint8_t *p;
> + int ret;
> + int i;
> +
> + dev_dbg(fsm->dev, "writing %d bytes to 0x%08x\n", size, offset);
> +
> + /* Enter 32-bit address mode, if required */
> + if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR)
> + stfsm_enter_32bit_addr(fsm, 1);
> +
> + /* Must write in multiples of 32 cycles (or 32*pads/8 bytes) */
> + data_pads = ((seq->seq_cfg >> 16) & 0x3) + 1;
> + write_mask = (data_pads << 2) - 1;
> +
> + /* Handle non-aligned buf */
> + if ((uint32_t)buf & 0x3) {
> + memcpy(page_buf, buf, size);
> + p = (uint8_t *)page_buf;
> + } else {
> + p = buf;
> + }
> +
> + /* Handle non-aligned size */
> + size_ub = (size + write_mask) & ~write_mask;
> + size_lb = size & ~write_mask;
> + size_mop = size & write_mask;
> +
> + seq->data_size = TRANSFER_SIZE(size_ub);
> + seq->addr1 = (offset >> 16) & 0xffff;
> + seq->addr2 = offset & 0xffff;
> +
> + /* Need to set FIFO to write mode, before writing data to FIFO (see
> + * GNBvb79594)
> + */
> + writel(0x00040000, fsm->base + SPI_FAST_SEQ_CFG);
> +
> + /*
> + * Before writing data to the FIFO, apply a small delay to allow a
> + * potential change of FIFO direction to complete.
> + */
> + if (fsm->fifo_dir_delay == 0)
> + readl(fsm->base + SPI_FAST_SEQ_CFG);
> + else
> + udelay(fsm->fifo_dir_delay);
> +
> +
> + /* Write data to FIFO, before starting sequence (see GNBvd79593) */
> + if (size_lb) {
> + stfsm_write_fifo(fsm, (uint32_t *)p, size_lb);
> + p += size_lb;
> + }
> +
> + /* Handle non-aligned size */
> + if (size_mop) {
> + memset(t, 0xff, write_mask + 1); /* fill with 0xff's */
> + for (i = 0; i < size_mop; i++)
> + t[i] = *p++;
> +
> + stfsm_write_fifo(fsm, tmp, write_mask + 1);
> + }
> +
> + /* Start sequence */
> + stfsm_load_seq(fsm, seq);
> +
> + /* Wait for sequence to finish */
> + stfsm_wait_seq(fsm);
> +
> + /* Wait for completion */
> + ret = stfsm_wait_busy(fsm);
> +
> + /* Exit 32-bit address mode, if required */
> + if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR) {
> + stfsm_enter_32bit_addr(fsm, 0);
> + if (fsm->configuration & CFG_WRITE_EX_32BIT_ADDR_DELAY)
> + udelay(1);
> + }
> +
> + return 0;
> +}
> +
> /*
> * Read an address range from the flash chip. The address range
> * may be any size provided it is within the physical boundaries.
> @@ -1053,6 +1218,63 @@ static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
> return 0;
> }
>
> +/*
> + * Write an address range to the flash chip. Data must be written in
> + * FLASH_PAGESIZE chunks. The address range may be any size provided
> + * it is within the physical boundaries.
> + */
> +static int stfsm_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
> + size_t *retlen, const u_char *buf)
> +{
> + struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent);
> +
> + u32 page_offs;
> + u32 bytes;
> + uint8_t *b = (uint8_t *)buf;
> + int ret = 0;
> +
> + dev_dbg(fsm->dev, "%s to 0x%08x, len %zd\n", __func__, (u32)to, len);
> +
> + if (retlen)
> + *retlen = 0;
You're still duplicating the code from mtd_write(). Please kill the
above.
> +
> + if (!len)
> + return 0;
Ditto.
> +
> + if (to + len > mtd->size)
> + return -EINVAL;
Ditto.
> +
> + /* Offset within page */
> + page_offs = to % FLASH_PAGESIZE;
> +
> + mutex_lock(&fsm->lock);
> +
> + while (len) {
> + /* Write up to page boundary */
> + bytes = min(FLASH_PAGESIZE - page_offs, len);
> +
> + ret = stfsm_write(fsm, b, bytes, to);
> + if (ret)
> + goto out1;
> +
> + b += bytes;
> + len -= bytes;
> + to += bytes;
> +
> + /* We are now page-aligned */
> + page_offs = 0;
> +
> + if (retlen)
You can assume retlen is always non-NULL. Drop the 'if (retlen)'.
> + *retlen += bytes;
> +
> + }
> +
> +out1:
> + mutex_unlock(&fsm->lock);
> +
> + return ret;
> +}
> +
> static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec)
> {
> const struct stfsm_seq *seq = &stfsm_seq_read_jedec;
> @@ -1310,6 +1532,7 @@ static int stfsm_probe(struct platform_device *pdev)
> fsm->mtd.erasesize = info->sector_size;
>
> fsm->mtd._read = stfsm_mtd_read;
> + fsm->mtd._write = stfsm_mtd_write;
>
> dev_err(&pdev->dev,
> "Found serial flash device: %s\n"
Brian
--
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