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
| ||
|
Date: Tue, 10 Dec 2013 12:19:43 -0800 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, dwmw2@...radead.org, angus.clark@...com, linus.walleij@...aro.org, linux-mtd@...ts.infradead.org Subject: Re: [PATCH v3 04/36] mtd: st_spi_fsm: Supply framework for device requests On Fri, Nov 29, 2013 at 12:18:53PM +0000, Lee Jones wrote: > --- a/drivers/mtd/devices/st_spi_fsm.c > +++ b/drivers/mtd/devices/st_spi_fsm.c > @@ -46,6 +51,36 @@ static void stfsm_clear_fifo(struct stfsm *fsm) > } > } > > +static inline void stfsm_load_seq(struct stfsm *fsm, > + const struct stfsm_seq *seq) > +{ > + void __iomem *dst = fsm->base + SPI_FAST_SEQ_TRANSFER_SIZE; > + const uint32_t *src = (const uint32_t *)seq; > + int words = STFSM_SEQ_SIZE / sizeof(uint32_t); I think this is clearer as: int words = sizeof(*seq) / sizeof(*src); > + > + BUG_ON(!stfsm_is_idle(fsm)); > + > + while (words--) { > + writel(*src, dst); > + src++; > + dst += 4; > + } > +} > + > +static void stfsm_wait_seq(struct stfsm *fsm) > +{ > + unsigned long timeo = jiffies + HZ; > + > + while (time_before(jiffies, timeo)) { > + if (stfsm_is_idle(fsm)) > + return; > + > + cond_resched(); > + } > + > + dev_err(fsm->dev, "timeout on sequence completion\n"); I believe the timeout logic is incorrect. What if we wait a "long time" to call stfsm_wait_seq() (due to scheduling, or otherwise)? Then the while loop might not even run once (time_before(x, y) is false). Or what if cond_resched() waits for a long time... So you need an extra check of stfsm_is_idle() after the while loop, before you declare a timeout. > +} > + > static int stfsm_set_mode(struct stfsm *fsm, uint32_t mode) > { > int ret, timeout = 10; > diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h > index 4e92e58..6164142 100644 > --- a/drivers/mtd/devices/st_spi_fsm.h > +++ b/drivers/mtd/devices/st_spi_fsm.h > @@ -199,4 +199,18 @@ struct stfsm { > uint32_t fifo_dir_delay; > }; > > +struct stfsm_seq { > + uint32_t data_size; > + uint32_t addr1; > + uint32_t addr2; > + uint32_t addr_cfg; > + uint32_t seq_opc[5]; > + uint32_t mode; > + uint32_t dummy; > + uint32_t status; > + uint8_t seq[16]; > + uint32_t seq_cfg; > +} __attribute__((__packed__, aligned(4))); I think checkpatch.pl prefers these attributes use the kernel macros: struct stfsm_seq { ... } __packed __aligned(4); > +#define STFSM_SEQ_SIZE sizeof(struct stfsm_seq) If you agree with my earlier comment, you won't need this macro. > + > #endif /* ST_SPI_FSM_H */ 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