Message ID | CAOMZO5BzXhrbDtkFMPmYPnQp+X2=gSEJGc+3U8H+QrGdgRgCxw@mail.gmail.com |
---|---|
State | New, archived |
Headers | show |
于 2013年11月12日 11:44, Fabio Estevam 写道: > On Tue, Nov 12, 2013 at 12:47 AM, Huang Shijie<b32955@freescale.com> wrote: >> 于 2013年11月12日 02:23, Brian Norris 写道: >> For imx23, the work flow is like this: >> [1] first check the fingerprint, if we can find it, we will return >> immediately. >> [2] if [1] failed, such as you erase all the partitions, the gpmi will call >> mx23_write_transcription_stamp() to write the fingerprint. >> >> So the @chip->buffer is not only used by the >> mx23_check_transcription_stamp(), >> but _also_ used by the mx23_write_transcription_stamp() when the gpmi >> can not find any >> fingerprint in the NAND page. >> >> >> That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both >> the mx23_check_transcription_stamp() >> and mx23_write_transcription_stamp(). > Understood. > > What if we just allocate the 4-byte buffer once on probe? > > Like this: > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index a9830ff..647da1b 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > @@ -1342,7 +1342,6 @@ static int mx23_check_transcription_stamp(struct > gpmi_nand_data *this) > unsigned int search_area_size_in_strides; > unsigned int stride; > unsigned int page; > - uint8_t *buffer = chip->buffers->databuf; > int saved_chip_number; > int found_an_ncb_fingerprint = false; > > @@ -1368,10 +1367,10 @@ static int > mx23_check_transcription_stamp(struct gpmi_nand_data *this) > * and starts in the 12th byte of the page. > */ > chip->cmdfunc(mtd, NAND_CMD_READ0, 12, page); > - chip->read_buf(mtd, buffer, strlen(fingerprint)); > + chip->read_buf(mtd, this->buffer, strlen(fingerprint)); > > /* Look for the fingerprint. */ > - if (!memcmp(buffer, fingerprint, strlen(fingerprint))) { > + if (!memcmp(this->buffer, fingerprint, strlen(fingerprint))) { > found_an_ncb_fingerprint = true; > break; > } > @@ -1401,7 +1400,6 @@ static int mx23_write_transcription_stamp(struct > gpmi_nand_data *this) > unsigned int block; > unsigned int stride; > unsigned int page; > - uint8_t *buffer = chip->buffers->databuf; > int saved_chip_number; > int status; > > @@ -1442,9 +1440,9 @@ static int mx23_write_transcription_stamp(struct > gpmi_nand_data *this) > } > > /* Write the NCB fingerprint into the page buffer. */ > - memset(buffer, ~0, mtd->writesize); > + memset(this->buffer, ~0, mtd->writesize); > memset(chip->oob_poi, ~0, mtd->oobsize); NULL pointer here, since chip->oob_poi is NULL. > - memcpy(buffer + 12, fingerprint, strlen(fingerprint)); > + memcpy(this->buffer + 12, fingerprint, strlen(fingerprint)); > > /* Loop through the first search area, writing NCB fingerprints. */ > dev_dbg(dev, "Writing NCB fingerprints...\n"); > @@ -1455,7 +1453,7 @@ static int mx23_write_transcription_stamp(struct > gpmi_nand_data *this) > /* Write the first page of the current stride. */ > dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page); > chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); > - chip->ecc.write_page_raw(mtd, chip, buffer, 0); > + chip->ecc.write_page_raw(mtd, chip, this->buffer, 0); NULL pointer here. thanks Huang Shijie
On Mon, Nov 11, 2013 at 7:44 PM, Fabio Estevam <festevam@gmail.com> wrote: > On Tue, Nov 12, 2013 at 12:47 AM, Huang Shijie <b32955@freescale.com> wrote: >> 于 2013年11月12日 02:23, Brian Norris 写道: > >> For imx23, the work flow is like this: >> [1] first check the fingerprint, if we can find it, we will return >> immediately. >> [2] if [1] failed, such as you erase all the partitions, the gpmi will call >> mx23_write_transcription_stamp() to write the fingerprint. >> >> So the @chip->buffer is not only used by the >> mx23_check_transcription_stamp(), >> but _also_ used by the mx23_write_transcription_stamp() when the gpmi >> can not find any >> fingerprint in the NAND page. >> >> >> That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both >> the mx23_check_transcription_stamp() >> and mx23_write_transcription_stamp(). > > Understood. > > What if we just allocate the 4-byte buffer once on probe? > > Like this: > > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > index a9830ff..647da1b 100644 > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c ... > @@ -1728,6 +1726,10 @@ static int gpmi_nand_probe(struct platform_device *pdev) > return -ENOMEM; > } > > + this->buffer = devm_kzalloc(&pdev->dev, strlen(fingerprint), GFP_KERNEL); > + if (!this->buffer) > + return -ENOMEM; > + > platform_set_drvdata(pdev, this); > this->pdev = pdev; > this->dev = &pdev->dev; ... As Huang noted, this approach is still very fragile. I think it's best if we rework GPMI's init sequence so that we don't have to do these kind of fragile dodges any more; just do all the BBT scanning after nand_scan_tail(). Let's wait for Huang's version of my suggestion. Brian
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c index a9830ff..647da1b 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c @@ -1342,7 +1342,6 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this) unsigned int search_area_size_in_strides; unsigned int stride; unsigned int page; - uint8_t *buffer = chip->buffers->databuf; int saved_chip_number; int found_an_ncb_fingerprint = false; @@ -1368,10 +1367,10 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this) * and starts in the 12th byte of the page. */ chip->cmdfunc(mtd, NAND_CMD_READ0, 12, page); - chip->read_buf(mtd, buffer, strlen(fingerprint)); + chip->read_buf(mtd, this->buffer, strlen(fingerprint)); /* Look for the fingerprint. */ - if (!memcmp(buffer, fingerprint, strlen(fingerprint))) { + if (!memcmp(this->buffer, fingerprint, strlen(fingerprint))) { found_an_ncb_fingerprint = true; break; } @@ -1401,7 +1400,6 @@ static int mx23_write_transcription_stamp(struct gpmi_nand_data *this) unsigned int block; unsigned int stride; unsigned int page; - uint8_t *buffer = chip->buffers->databuf; int saved_chip_number; int status; @@ -1442,9 +1440,9 @@ static int mx23_write_transcription_stamp(struct gpmi_nand_data *this) } /* Write the NCB fingerprint into the page buffer. */ - memset(buffer, ~0, mtd->writesize); + memset(this->buffer, ~0, mtd->writesize); memset(chip->oob_poi, ~0, mtd->oobsize); - memcpy(buffer + 12, fingerprint, strlen(fingerprint)); + memcpy(this->buffer + 12, fingerprint, strlen(fingerprint)); /* Loop through the first search area, writing NCB fingerprints. */ dev_dbg(dev, "Writing NCB fingerprints...\n"); @@ -1455,7 +1453,7 @@ static int mx23_write_transcription_stamp(struct gpmi_nand_data *this) /* Write the first page of the current stride. */ dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page); chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page); - chip->ecc.write_page_raw(mtd, chip, buffer, 0); + chip->ecc.write_page_raw(mtd, chip, this->buffer, 0); chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1); /* Wait for the write to finish. */ @@ -1728,6 +1726,10 @@ static int gpmi_nand_probe(struct platform_device *pdev) return -ENOMEM; } + this->buffer = devm_kzalloc(&pdev->dev, strlen(fingerprint), GFP_KERNEL); + if (!this->buffer) + return -ENOMEM; + platform_set_drvdata(pdev, this); this->pdev = pdev; this->dev = &pdev->dev; diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h index a7685e3..e88114c 100644 --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h @@ -186,6 +186,7 @@ struct gpmi_nand_data { /* private */ void *private; + uint8_t *buffer; }; ______________________________________________________