[linux-sunxi] Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.
On Fri, May 22, 2015 at 12:25:05AM -0700, Brian Norris wrote: > On Fri, May 22, 2015 at 12:17:27AM -0700, Brian Norris wrote: > > Admittedly, as he's using an out-of-tree driver, I'm not > > sure I know exactly what failure modes he is hitting yet. > Sorry, I realized I misread here. He's using spi-sunxi. Given that... > ... is this code even valid? > static int sun6i_spi_transfer_one(struct spi_master *master, > struct spi_device *spi, > struct spi_transfer *tfr) > { > ... > /* We don't support transfer larger than the FIFO */ > if (tfr->len > SUN6I_FIFO_DEPTH) > return -EINVAL; > Seems like it should be looping over the transfer in multiple chunks > instead. Well, it's not ideal. Like I say I think that logic probably belongs in the core rather than individual drivers then we minimise the problems, if I remember correctly there was the suggestion that the DMA code was going to follow soon and be used for larger transfers when the original driver was merged. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
[linux-sunxi] Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.
(trimming CC a little this time, though it's still a bit large) On Fri, May 22, 2015 at 12:17:27AM -0700, Brian Norris wrote: > Admittedly, as he's using an out-of-tree driver, I'm not > sure I know exactly what failure modes he is hitting yet. Sorry, I realized I misread here. He's using spi-sunxi. Given that... ... is this code even valid? static int sun6i_spi_transfer_one(struct spi_master *master, struct spi_device *spi, struct spi_transfer *tfr) { ... /* We don't support transfer larger than the FIFO */ if (tfr->len > SUN6I_FIFO_DEPTH) return -EINVAL; Seems like it should be looping over the transfer in multiple chunks instead. Brian -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.
On Thu, May 21, 2015 at 11:28:02AM +0100, Mark Brown wrote: > On Thu, May 21, 2015 at 10:39:13AM +0200, Michal Suchanek wrote: > > On 21 May 2015 at 01:38, Brian Norris wrote: > > Why is this thread about SPI error handling CCed to quite so many > people? I can't really speak for the original patch author, who created the CC list. I added you for comment on the SPI API (thanks BTW). This is part of a series that included an ill-conceived DT binding for recording a "max transfer length" property in the flash node. That's one step that easily blows up the CC list for the series, as there are 5 DT maintainers and a mailing list. Others are contributors to the m25p80 / spi-nor drivers. (All in all, you can probably trace this back to scripts/get_maintainer.pl.) > > >> Check the amount of data actually written by the driver. > > > > I'm not sure if we should just reactively use the retlen, or if we > > > should be communicating such a limitation via the SPI API. Thoughts? > > > Is there any driver that would break if the SPI master truncated > > writes when the message is too long rather than returning an error an > > refusing the transfer? > > Any such driver is buggy, the actual length transferred has always been > a part of the SPI API. OK, so m25p80.c currently checks the retlen (spi_message::actual_length), but it doesn't actually handle it well if the SPI master can't write a full page-size chunk in one go. It looks like we'd leave holes in the programmed data right now. So that qualifies as buggy, and that's part of what Michal is trying to fix, I think. Admittedly, as he's using an out-of-tree driver, I'm not sure I know exactly what failure modes he is hitting yet. > We should probably expose limitations so clients > can query in advance (we don't at the minute) but it'd be a while before > clients could rely on that information being useful and it's still > possible things can be truncated by error. That might help in the long run. In this case, I think we might be able to get by (for a properly-functioning SPI driver with a limited transfer size) with the current API, and handling the 'return length < message length' case better. BTW, one extra note for Michal regarding your SPI controller/driver transfer length limitation: you would do well to try as much as possible to allow full NOR pages to be programmed in one SPINOR_OP_PP sequence. I know of some SPI NOR that, though they are byte addressable, actually have opaque internal ECC which is encoded on page boundaries, so you get better flash lifetime if you program on page boundaries, rather than on whatever smaller chunk size your SPI driver supports. So that might require a little more work on your SPI driver. Brian -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.
On Thu, May 21, 2015 at 10:39:13AM +0200, Michal Suchanek wrote: > On 21 May 2015 at 01:38, Brian Norris wrote: Why is this thread about SPI error handling CCed to quite so many people? > >> Check the amount of data actually written by the driver. > > I'm not sure if we should just reactively use the retlen, or if we > > should be communicating such a limitation via the SPI API. Thoughts? > Is there any driver that would break if the SPI master truncated > writes when the message is too long rather than returning an error an > refusing the transfer? Any such driver is buggy, the actual length transferred has always been a part of the SPI API. We should probably expose limitations so clients can query in advance (we don't at the minute) but it'd be a while before clients could rely on that information being useful and it's still possible things can be truncated by error. With modern drivers using transfer_one() where we have control of the chip select we do also have the ability to add support to the core for chopping large transfers up into smaller ones that the hardware can handle transparently. That won't for everything though since it depends on us being able to manually control the chip select which not all hardware can do. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: Digital signature
[linux-sunxi] Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.
On 21 May 2015 at 01:38, Brian Norris wrote: > + linux-spi, Mark > > On Thu, Apr 30, 2015 at 03:38:50PM +0200, Michal Suchanek wrote: >> My SPI controller driver does not support DMA so writes are truncated to >> FIFO size. > > Which SPI master driver? I am using sunxi SPI driver. The dmaengine support for sunxi is not yet in mainline kernel so the SPI master functionality is limited. > >> Check the amount of data actually written by the driver. > > I'm not sure if we should just reactively use the retlen, or if we > should be communicating such a limitation via the SPI API. Thoughts? > Is there any driver that would break if the SPI master truncated writes when the message is too long rather than returning an error an refusing the transfer? m25p80 as is would because it assumes all data is always written. Some display driver I tried earlier has an option to limit transfer size actively in the driver. Thanks Michal -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH 2/3] MTD: spi-nor: check for short writes in spi_nor_write.
+ linux-spi, Mark On Thu, Apr 30, 2015 at 03:38:50PM +0200, Michal Suchanek wrote: > My SPI controller driver does not support DMA so writes are truncated to > FIFO size. Which SPI master driver? > Check the amount of data actually written by the driver. I'm not sure if we should just reactively use the retlen, or if we should be communicating such a limitation via the SPI API. Thoughts? Brian > Signed-off-by: Michal Suchanek > --- > drivers/mtd/spi-nor/spi-nor.c | 42 +- > 1 file changed, 17 insertions(+), 25 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c > index 14a5d23..75904b5 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -807,7 +807,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, > size_t len, > size_t *retlen, const u_char *buf) > { > struct spi_nor *nor = mtd_to_spi_nor(mtd); > - u32 page_offset, page_size, i; > + u32 page_offset, page_remainder, page_size, i; > int ret; > > dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > @@ -816,36 +816,28 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t > to, size_t len, > if (ret) > return ret; > > - write_enable(nor); > - > - page_offset = to & (nor->page_size - 1); > - > - /* do all the bytes fit onto one page? */ > - if (page_offset + len <= nor->page_size) { > - nor->write(nor, to, len, retlen, buf); > - } else { > - /* the size of data remaining on the first page */ > - page_size = nor->page_size - page_offset; > - nor->write(nor, to, page_size, retlen, buf); > + /* write everything in nor->page_size chunks */ > + /* check the size of data actually written */ > + for (i = 0; i < len; i += *retlen) { > + page_size = len - i; > + page_offset = (to + i) & (nor->page_size - 1); > + page_remainder = nor->page_size - page_offset; > + if (page_size > nor->page_size) > + page_size = nor->page_size; > + if (page_remainder && (page_size > page_remainder)) > + page_size = page_remainder; > > - /* write everything in nor->page_size chunks */ > - for (i = page_size; i < len; i += page_size) { > - page_size = len - i; > - if (page_size > nor->page_size) > - page_size = nor->page_size; > - > - ret = spi_nor_wait_till_ready(nor); > - if (ret) > - goto write_err; > + write_enable(nor); > > - write_enable(nor); > + nor->write(nor, to + i, page_size, retlen, buf + i); > > - nor->write(nor, to + i, page_size, retlen, buf + i); > - } > + ret = spi_nor_wait_till_ready(nor); > + if (ret) > + goto write_err; > } > > - ret = spi_nor_wait_till_ready(nor); > write_err: > + *retlen = i; > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE); > return ret; > } -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.