Patchwork [01/21] mtd: nand: pxa3xx: Allocate data buffer on detected flash size

login
register
mail settings
Submitter Ezequiel Garcia
Date Sept. 19, 2013, 4:01 p.m.
Message ID <1379606505-2529-2-git-send-email-ezequiel.garcia@free-electrons.com>
Download mbox | patch
Permalink /patch/275982/
State Accepted
Commit 62e8b851783138a11da63285be0fbf69530ff73d
Headers show

Comments

Ezequiel Garcia - Sept. 19, 2013, 4:01 p.m.
This commit replaces the currently hardcoded buffer size, by a
dynamic detection scheme. First a small 256 bytes buffer is allocated
so the device can be detected (using READID and friends commands).

After detection, this buffer is released and a new buffer is allocated
to acommodate the page size plus out-of-band size.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/nand/pxa3xx_nand.c | 45 ++++++++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 15 deletions(-)
Brian Norris - Sept. 26, 2013, 8:10 p.m.
On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> This commit replaces the currently hardcoded buffer size, by a
> dynamic detection scheme. First a small 256 bytes buffer is allocated
> so the device can be detected (using READID and friends commands).
> 
> After detection, this buffer is released and a new buffer is allocated
> to acommodate the page size plus out-of-band size.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

This is the patch that breaks Daniel's DMA setup, right? It looks a bit
off. I'll wait to comment on it much until v2.

BTW, there is a similar issue with at least one other driver (denali.c,
maybe others) where the driver uses some hard assumptions about the
maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
kind of ugly and not very sustainable/maintainable, since these
dimensions keep increasing. I appreciate your effort to avoid simply
kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
drivers, but I don't know if we'll have much testing opportunities for
the ancient ones...

Also, it seems like your driver has a few potential leaks right now. If
alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
(e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
should address this soon, even if not in this patch series.

Brian
Ezequiel Garcia - Sept. 30, 2013, 12:37 p.m.
On Thu, Sep 26, 2013 at 01:10:32PM -0700, Brian Norris wrote:
> On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> > This commit replaces the currently hardcoded buffer size, by a
> > dynamic detection scheme. First a small 256 bytes buffer is allocated
> > so the device can be detected (using READID and friends commands).
> > 
> > After detection, this buffer is released and a new buffer is allocated
> > to acommodate the page size plus out-of-band size.
> > 
> > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> 
> This is the patch that breaks Daniel's DMA setup, right? It looks a bit
> off.

What do you mean by 'a bit off'?

> I'll wait to comment on it much until v2.
> 

Ok... let me re-work it then and prepare the v2.

> BTW, there is a similar issue with at least one other driver (denali.c,
> maybe others) where the driver uses some hard assumptions about the
> maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
> kind of ugly and not very sustainable/maintainable, since these
> dimensions keep increasing. I appreciate your effort to avoid simply
> kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
> drivers, but I don't know if we'll have much testing opportunities for
> the ancient ones...
> 
> Also, it seems like your driver has a few potential leaks right now. If
> alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
> (e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
> should address this soon, even if not in this patch series.
> 

Hm.. are you sure about that? AFAICS, there's no leak at all.

If alloc_nand_resource() succeeds, the only leakable resources allocated
are the ones allocated at pxa3xx_nand_init_buff() and the NAND base
stuff.

If pxa3xx_nand_probe() later fails to complete, it calls pxa3xx_nand_remove()
in this part:

	if (!probe_success) {
		pxa3xx_nand_remove(pdev);
		return -ENODEV;
	}

Which takes care of cleaning both the buffers and the NAND base stuff.

Or am I missing something?

Feel free to ask more questions and thanks a lot for the feedback.
Brian Norris - Oct. 2, 2013, 9:14 p.m.
On Mon, Sep 30, 2013 at 09:37:57AM -0300, Ezequiel Garcia wrote:
> On Thu, Sep 26, 2013 at 01:10:32PM -0700, Brian Norris wrote:
> > On Thu, Sep 19, 2013 at 01:01:25PM -0300, Ezequiel Garcia wrote:
> > > This commit replaces the currently hardcoded buffer size, by a
> > > dynamic detection scheme. First a small 256 bytes buffer is allocated
> > > so the device can be detected (using READID and friends commands).
> > > 
> > > After detection, this buffer is released and a new buffer is allocated
> > > to acommodate the page size plus out-of-band size.
> > > 
> > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> > 
> > This is the patch that breaks Daniel's DMA setup, right? It looks a bit
> > off.
> 
> What do you mean by 'a bit off'?

The existing code allocates data_buff with kmalloc() or
dma_alloc_coherent() depending on ARCH_HAS_DMA, but your patch does the
initial fixed-size allocation only with kmalloc(). It's just what you
and Daniel already identified: that this causes problems for the DMA
case.

> > BTW, there is a similar issue with at least one other driver (denali.c,
> > maybe others) where the driver uses some hard assumptions about the
> > maximum page/OOB sizes (NAND_MAX_PAGESIZE and NAND_MAX_OOBSIZE). This is
> > kind of ugly and not very sustainable/maintainable, since these
> > dimensions keep increasing. I appreciate your effort to avoid simply
> > kludging in a larger MAX_BUFF_SIZE :) I had similar plans for the other
> > drivers, but I don't know if we'll have much testing opportunities for
> > the ancient ones...
> > 
> > Also, it seems like your driver has a few potential leaks right now. If
> > alloc_nand_resource() succeeds but pxa3xx_nand_probe() later fails
> > (e.g., in pxa3xx_nand_scan()), you don't clean up after yourself. You
> > should address this soon, even if not in this patch series.
> > 
> 
> Hm.. are you sure about that? AFAICS, there's no leak at all.

No, I'm not sure :)

> If alloc_nand_resource() succeeds, the only leakable resources allocated
> are the ones allocated at pxa3xx_nand_init_buff() and the NAND base
> stuff.
> 
> If pxa3xx_nand_probe() later fails to complete, it calls pxa3xx_nand_remove()
> in this part:
> 
> 	if (!probe_success) {
> 		pxa3xx_nand_remove(pdev);
> 		return -ENODEV;
> 	}
> 
> Which takes care of cleaning both the buffers and the NAND base stuff.
> 
> Or am I missing something?

Nope, you were correct. I just missed this point when reading.

Brian

Patch

diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
index dd03dfd..b4f3784 100644
--- a/drivers/mtd/nand/pxa3xx_nand.c
+++ b/drivers/mtd/nand/pxa3xx_nand.c
@@ -39,6 +39,13 @@ 
 #define NAND_STOP_DELAY		(2 * HZ/50)
 #define PAGE_CHUNK_SIZE		(2048)
 
+/*
+ * Define a buffer size for the initial command that detects the flash device:
+ * STATUS, READID and PARAM. The largest of these is the PARAM command,
+ * needing 256 bytes.
+ */
+#define INIT_BUFFER_SIZE	256
+
 /* registers and bit definitions */
 #define NDCR		(0x00) /* Control register */
 #define NDTR0CS0	(0x04) /* Timing Parameter 0 for CS0 */
@@ -164,6 +171,7 @@  struct pxa3xx_nand_info {
 
 	unsigned int 		buf_start;
 	unsigned int		buf_count;
+	unsigned int		buf_size;
 
 	/* DMA information */
 	int			drcmr_dat;
@@ -912,26 +920,20 @@  static int pxa3xx_nand_detect_config(struct pxa3xx_nand_info *info)
 	return 0;
 }
 
-/* the maximum possible buffer size for large page with OOB data
- * is: 2048 + 64 = 2112 bytes, allocate a page here for both the
- * data buffer and the DMA descriptor
- */
-#define MAX_BUFF_SIZE	PAGE_SIZE
-
 #ifdef ARCH_HAS_DMA
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
 	struct platform_device *pdev = info->pdev;
-	int data_desc_offset = MAX_BUFF_SIZE - sizeof(struct pxa_dma_desc);
+	int data_desc_offset = info->buf_size - sizeof(struct pxa_dma_desc);
 
 	if (use_dma == 0) {
-		info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+		info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
 		if (info->data_buff == NULL)
 			return -ENOMEM;
 		return 0;
 	}
 
-	info->data_buff = dma_alloc_coherent(&pdev->dev, MAX_BUFF_SIZE,
+	info->data_buff = dma_alloc_coherent(&pdev->dev, info->buf_size,
 				&info->data_buff_phys, GFP_KERNEL);
 	if (info->data_buff == NULL) {
 		dev_err(&pdev->dev, "failed to allocate dma buffer\n");
@@ -945,7 +947,7 @@  static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 				pxa3xx_nand_data_dma_irq, info);
 	if (info->data_dma_ch < 0) {
 		dev_err(&pdev->dev, "failed to request data dma\n");
-		dma_free_coherent(&pdev->dev, MAX_BUFF_SIZE,
+		dma_free_coherent(&pdev->dev, info->buf_size,
 				info->data_buff, info->data_buff_phys);
 		return info->data_dma_ch;
 	}
@@ -958,7 +960,7 @@  static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 	struct platform_device *pdev = info->pdev;
 	if (use_dma) {
 		pxa_free_dma(info->data_dma_ch);
-		dma_free_coherent(&pdev->dev, MAX_BUFF_SIZE,
+		dma_free_coherent(&pdev->dev, info->buf_size,
 				  info->data_buff, info->data_buff_phys);
 	} else {
 		kfree(info->data_buff);
@@ -967,7 +969,7 @@  static void pxa3xx_nand_free_buff(struct pxa3xx_nand_info *info)
 #else
 static int pxa3xx_nand_init_buff(struct pxa3xx_nand_info *info)
 {
-	info->data_buff = kmalloc(MAX_BUFF_SIZE, GFP_KERNEL);
+	info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
 	if (info->data_buff == NULL)
 		return -ENOMEM;
 	return 0;
@@ -1081,7 +1083,16 @@  KEEP_CONFIG:
 	else
 		host->col_addr_cycles = 1;
 
+	/* release the initial buffer */
+	kfree(info->data_buff);
+
+	/* allocate the real data + oob buffer */
+	info->buf_size = mtd->writesize + mtd->oobsize;
+	ret = pxa3xx_nand_init_buff(info);
+	if (ret)
+		return ret;
 	info->oob_buff = info->data_buff + mtd->writesize;
+
 	if ((mtd->size >> chip->page_shift) > 65536)
 		host->row_addr_cycles = 3;
 	else
@@ -1187,9 +1198,13 @@  static int alloc_nand_resource(struct platform_device *pdev)
 	}
 	info->mmio_phys = r->start;
 
-	ret = pxa3xx_nand_init_buff(info);
-	if (ret)
+	/* Allocate a buffer to allow flash detection */
+	info->buf_size = INIT_BUFFER_SIZE;
+	info->data_buff = kmalloc(info->buf_size, GFP_KERNEL);
+	if (info->data_buff == NULL) {
+		ret = -ENOMEM;
 		goto fail_disable_clk;
+	}
 
 	/* initialize all interrupts to be disabled */
 	disable_int(info, NDSR_MASK);
@@ -1207,7 +1222,7 @@  static int alloc_nand_resource(struct platform_device *pdev)
 
 fail_free_buf:
 	free_irq(irq, info);
-	pxa3xx_nand_free_buff(info);
+	kfree(info->data_buff);
 fail_disable_clk:
 	clk_disable_unprepare(info->clk);
 	return ret;