[PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Jheng-Jhong Wu]

For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
are necessary to address the correct page. The driver sets the address for
more than 16 bits, but it uses 16-bit arguments and variables (these are
page_id, block_id, row) to do address operations. Obviously, these
arguments and variables cannot deal with more than 16-bit address.

Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>
---
 drivers/staging/mt29f_spinand/mt29f_spinand.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/mt29f_spinand/mt29f_spinand.c b/drivers/staging/mt29f_spinand/mt29f_spinand.c
index 4484784..a0f4cbcb 100644
--- a/drivers/staging/mt29f_spinand/mt29f_spinand.c
+++ b/drivers/staging/mt29f_spinand/mt29f_spinand.c
@@ -308,10 +308,10 @@ static int spinand_write_enable(struct spi_device *spi_nand)
 	return spinand_cmd(spi_nand, &cmd);
 }
 
-static int spinand_read_page_to_cache(struct spi_device *spi_nand, u16 page_id)
+static int spinand_read_page_to_cache(struct spi_device *spi_nand, u32 page_id)
 {
 	struct spinand_cmd cmd = {0};
-	u16 row;
+	u32 row;
 
 	row = page_id;
 	cmd.cmd = CMD_READ;
@@ -331,7 +331,7 @@ static int spinand_read_page_to_cache(struct spi_device *spi_nand, u16 page_id)
  *   locations.
  *   No tRd delay.
  */
-static int spinand_read_from_cache(struct spi_device *spi_nand, u16 page_id,
+static int spinand_read_from_cache(struct spi_device *spi_nand, u32 page_id,
 				   u16 byte_id, u16 len, u8 *rbuf)
 {
 	struct spinand_cmd cmd = {0};
@@ -362,7 +362,7 @@ static int spinand_read_from_cache(struct spi_device *spi_nand, u16 page_id,
  *   The read includes two commands to the Nand - 0x13 and 0x03 commands
  *   Poll to read status to wait for tRD time.
  */
-static int spinand_read_page(struct spi_device *spi_nand, u16 page_id,
+static int spinand_read_page(struct spi_device *spi_nand, u32 page_id,
 			     u16 offset, u16 len, u8 *rbuf)
 {
 	int ret;
@@ -430,7 +430,7 @@ static int spinand_read_page(struct spi_device *spi_nand, u16 page_id,
  *   Since it is writing the data to cache, there is no tPROG time.
  */
 static int spinand_program_data_to_cache(struct spi_device *spi_nand,
-					 u16 page_id, u16 byte_id,
+					 u32 page_id, u16 byte_id,
 					 u16 len, u8 *wbuf)
 {
 	struct spinand_cmd cmd = {0};
@@ -457,10 +457,10 @@ static int spinand_program_data_to_cache(struct spi_device *spi_nand,
  *   the Nand array.
  *   Need to wait for tPROG time to finish the transaction.
  */
-static int spinand_program_execute(struct spi_device *spi_nand, u16 page_id)
+static int spinand_program_execute(struct spi_device *spi_nand, u32 page_id)
 {
 	struct spinand_cmd cmd = {0};
-	u16 row;
+	u32 row;
 
 	row = page_id;
 	cmd.cmd = CMD_PROG_PAGE_EXC;
@@ -486,7 +486,7 @@ static int spinand_program_execute(struct spi_device *spi_nand, u16 page_id)
  *   Poll to wait for the tPROG time to finish the transaction.
  */
 static int spinand_program_page(struct spi_device *spi_nand,
-				u16 page_id, u16 offset, u16 len, u8 *buf)
+				u32 page_id, u16 offset, u16 len, u8 *buf)
 {
 	int retval;
 	u8 status = 0;
@@ -573,10 +573,10 @@ static int spinand_program_page(struct spi_device *spi_nand,
  *   one block--64 pages
  *   Need to wait for tERS.
  */
-static int spinand_erase_block_erase(struct spi_device *spi_nand, u16 block_id)
+static int spinand_erase_block_erase(struct spi_device *spi_nand, u32 block_id)
 {
 	struct spinand_cmd cmd = {0};
-	u16 row;
+	u32 row;
 
 	row = block_id;
 	cmd.cmd = CMD_ERASE_BLK;
@@ -599,7 +599,7 @@ static int spinand_erase_block_erase(struct spi_device *spi_nand, u16 block_id)
  *   and then send the 0xd8 erase command
  *   Poll to wait for the tERS time to complete the tranaction.
  */
-static int spinand_erase_block(struct spi_device *spi_nand, u16 block_id)
+static int spinand_erase_block(struct spi_device *spi_nand, u32 block_id)
 {
 	int retval;
 	u8 status = 0;
-- 
2.7.4

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Boris Brezillon]

Hi Jheng-Jhong,

On Wed,  1 Aug 2018 11:24:19 +0800
Jheng-Jhong Wu <goodwater.wu@gmail.com> wrote:

> For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> are necessary to address the correct page. The driver sets the address for
> more than 16 bits, but it uses 16-bit arguments and variables (these are
> page_id, block_id, row) to do address operations. Obviously, these
> arguments and variables cannot deal with more than 16-bit address.

I plan to remove this driver soon (after 4.19-rc1 is out). It's now
been replaced by the SPI framework [1]. Can you check if your NAND SPI
NAND is supported, if it's not add support for it, and if you find a
bug report/fix it.

Thanks,

Boris

[1]https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/mtd/nand/spi?h=next-20180731

> 
> Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>
> ---
>  drivers/staging/mt29f_spinand/mt29f_spinand.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/mt29f_spinand/mt29f_spinand.c b/drivers/staging/mt29f_spinand/mt29f_spinand.c
> index 4484784..a0f4cbcb 100644
> --- a/drivers/staging/mt29f_spinand/mt29f_spinand.c
> +++ b/drivers/staging/mt29f_spinand/mt29f_spinand.c
> @@ -308,10 +308,10 @@ static int spinand_write_enable(struct spi_device *spi_nand)
>  	return spinand_cmd(spi_nand, &cmd);
>  }
>  
> -static int spinand_read_page_to_cache(struct spi_device *spi_nand, u16 page_id)
> +static int spinand_read_page_to_cache(struct spi_device *spi_nand, u32 page_id)
>  {
>  	struct spinand_cmd cmd = {0};
> -	u16 row;
> +	u32 row;
>  
>  	row = page_id;
>  	cmd.cmd = CMD_READ;
> @@ -331,7 +331,7 @@ static int spinand_read_page_to_cache(struct spi_device *spi_nand, u16 page_id)
>   *   locations.
>   *   No tRd delay.
>   */
> -static int spinand_read_from_cache(struct spi_device *spi_nand, u16 page_id,
> +static int spinand_read_from_cache(struct spi_device *spi_nand, u32 page_id,
>  				   u16 byte_id, u16 len, u8 *rbuf)
>  {
>  	struct spinand_cmd cmd = {0};
> @@ -362,7 +362,7 @@ static int spinand_read_from_cache(struct spi_device *spi_nand, u16 page_id,
>   *   The read includes two commands to the Nand - 0x13 and 0x03 commands
>   *   Poll to read status to wait for tRD time.
>   */
> -static int spinand_read_page(struct spi_device *spi_nand, u16 page_id,
> +static int spinand_read_page(struct spi_device *spi_nand, u32 page_id,
>  			     u16 offset, u16 len, u8 *rbuf)
>  {
>  	int ret;
> @@ -430,7 +430,7 @@ static int spinand_read_page(struct spi_device *spi_nand, u16 page_id,
>   *   Since it is writing the data to cache, there is no tPROG time.
>   */
>  static int spinand_program_data_to_cache(struct spi_device *spi_nand,
> -					 u16 page_id, u16 byte_id,
> +					 u32 page_id, u16 byte_id,
>  					 u16 len, u8 *wbuf)
>  {
>  	struct spinand_cmd cmd = {0};
> @@ -457,10 +457,10 @@ static int spinand_program_data_to_cache(struct spi_device *spi_nand,
>   *   the Nand array.
>   *   Need to wait for tPROG time to finish the transaction.
>   */
> -static int spinand_program_execute(struct spi_device *spi_nand, u16 page_id)
> +static int spinand_program_execute(struct spi_device *spi_nand, u32 page_id)
>  {
>  	struct spinand_cmd cmd = {0};
> -	u16 row;
> +	u32 row;
>  
>  	row = page_id;
>  	cmd.cmd = CMD_PROG_PAGE_EXC;
> @@ -486,7 +486,7 @@ static int spinand_program_execute(struct spi_device *spi_nand, u16 page_id)
>   *   Poll to wait for the tPROG time to finish the transaction.
>   */
>  static int spinand_program_page(struct spi_device *spi_nand,
> -				u16 page_id, u16 offset, u16 len, u8 *buf)
> +				u32 page_id, u16 offset, u16 len, u8 *buf)
>  {
>  	int retval;
>  	u8 status = 0;
> @@ -573,10 +573,10 @@ static int spinand_program_page(struct spi_device *spi_nand,
>   *   one block--64 pages
>   *   Need to wait for tERS.
>   */
> -static int spinand_erase_block_erase(struct spi_device *spi_nand, u16 block_id)
> +static int spinand_erase_block_erase(struct spi_device *spi_nand, u32 block_id)
>  {
>  	struct spinand_cmd cmd = {0};
> -	u16 row;
> +	u32 row;
>  
>  	row = block_id;
>  	cmd.cmd = CMD_ERASE_BLK;
> @@ -599,7 +599,7 @@ static int spinand_erase_block_erase(struct spi_device *spi_nand, u16 block_id)
>   *   and then send the 0xd8 erase command
>   *   Poll to wait for the tERS time to complete the tranaction.
>   */
> -static int spinand_erase_block(struct spi_device *spi_nand, u16 block_id)
> +static int spinand_erase_block(struct spi_device *spi_nand, u32 block_id)
>  {
>  	int retval;
>  	u8 status = 0;

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Dan Carpenter]

On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> are necessary to address the correct page. The driver sets the address for
> more than 16 bits, but it uses 16-bit arguments and variables (these are
> page_id, block_id, row) to do address operations. Obviously, these
> arguments and variables cannot deal with more than 16-bit address.
> 
> Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>

This seems reasonable...  It would be needed to make commit 6efb21d6d0e7
("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
addressing.") work.  It also fixes a static checker warning.

My only concern is that the mtd/nand code seems to use -1 as a magical
page_id.  For example:


  2069  /**
  2070   * nand_exit_status_op - Exit a STATUS operation
  2071   * @chip: The NAND chip
  2072   *
  2073   * This function sends a READ0 command to cancel the effect of the STATUS
  2074   * command to avoid reading only the status until a new read command is sent.
  2075   *
  2076   * This function does not select/unselect the CS line.
  2077   *
  2078   * Returns 0 on success, a negative error code otherwise.
  2079   */
  2080  int nand_exit_status_op(struct nand_chip *chip)
  2081  {
  2082          struct mtd_info *mtd = nand_to_mtd(chip);
  2083  
  2084          if (chip->exec_op) {
  2085                  struct nand_op_instr instrs[] = {
  2086                          NAND_OP_CMD(NAND_CMD_READ0, 0),
  2087                  };
  2088                  struct nand_operation op = NAND_OPERATION(instrs);
  2089  
  2090                  return nand_exec_op(chip, &op);
  2091          }
  2092  
  2093          chip->cmdfunc(mtd, NAND_CMD_READ0, -1, -1);
                                   ^^^^^^^^^^^^^^      ^^
  2094  
  2095          return 0;
  2096  }
  2097  EXPORT_SYMBOL_GPL(nand_exit_status_op);

I'm not sure if this affect spinand_read_page() etc.

regards,
dan carpenter

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Miquel Raynal]

Hi Dan,

Dan Carpenter <dan.carpenter@oracle.com> wrote on Wed, 1 Aug 2018
15:05:51 +0300:

> On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > are necessary to address the correct page. The driver sets the address for
> > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > page_id, block_id, row) to do address operations. Obviously, these
> > arguments and variables cannot deal with more than 16-bit address.
> > 
> > Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>  
> 
> This seems reasonable...  It would be needed to make commit 6efb21d6d0e7
> ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> addressing.") work.  It also fixes a static checker warning.
> 
> My only concern is that the mtd/nand code seems to use -1 as a magical
> page_id.  For example:

I guess you missed Boris' comment: this driver is very likely to be
removed. A SPI-NAND framework has been added. It does not use the raw
NAND framework anymore, which was wrong anyway.

Thanks,
Miquèl

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Dan Carpenter]

On Wed, Aug 01, 2018 at 03:55:30PM +0200, Miquel Raynal wrote:
> Hi Dan,
> 
> Dan Carpenter <dan.carpenter@oracle.com> wrote on Wed, 1 Aug 2018
> 15:05:51 +0300:
> 
> > On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > > are necessary to address the correct page. The driver sets the address for
> > > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > > page_id, block_id, row) to do address operations. Obviously, these
> > > arguments and variables cannot deal with more than 16-bit address.
> > > 
> > > Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>  
> > 
> > This seems reasonable...  It would be needed to make commit 6efb21d6d0e7
> > ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> > addressing.") work.  It also fixes a static checker warning.
> > 
> > My only concern is that the mtd/nand code seems to use -1 as a magical
> > page_id.  For example:
> 
> I guess you missed Boris' comment: this driver is very likely to be
> removed. A SPI-NAND framework has been added. It does not use the raw
> NAND framework anymore, which was wrong anyway.
> 

We should probably revert commit 6efb21d6d0e7 ("staging:mt29f_spinand:
MT29F2G failing as only 16 bits used for addressing.") in that case if
all it does is add static checker warnings...

regards,
dan carpenter

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Jheng-Jhong Wu]

So should I change this into a revert patch, or you will revert commit
6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16 bits
used for addressing.") by yourself?
Thanks.

Best Regards,
Jheng-Jhong Wu (Victor Wu)

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Dan Carpenter]

On Mon, Aug 06, 2018 at 09:49:01AM +0800, Jheng-Jhong Wu wrote:
> So should I change this into a revert patch, or you will revert commit
> 6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16 bits
> used for addressing.") by yourself?
> Thanks.
> 

Are you asking me?  I'm not going to revert it.

regards,
dan carpenter

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Boris Brezillon]

Hi Dan,

On Wed, 1 Aug 2018 15:05:51 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > are necessary to address the correct page. The driver sets the address for
> > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > page_id, block_id, row) to do address operations. Obviously, these
> > arguments and variables cannot deal with more than 16-bit address.
> > 
> > Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>  
> 
> This seems reasonable...  It would be needed to make commit 6efb21d6d0e7
> ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> addressing.") work.  It also fixes a static checker warning.
> 
> My only concern is that the mtd/nand code seems to use -1 as a magical
> page_id.  For example:

Yes, -1 means "don't issue the row/page address cycles", though I
don't think page can be -1 for NAND_CMD_READ{1,0} commands.

Anyway, if you want this patch merged to fix a static checker warning,
I'm fine with that. In any case, I still plan to send a patch removing
this driver for v4.20, so, anyone using this driver should start
testing the new SPI NAND driver (drivers/mtd/nand/spi) and tweak/fix
the new implementation if needed.

Regards,

Boris

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Dan Carpenter]

On Mon, Aug 06, 2018 at 01:46:48PM +0200, Boris Brezillon wrote:
> Hi Dan,
> 
> On Wed, 1 Aug 2018 15:05:51 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:
> > > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > > are necessary to address the correct page. The driver sets the address for
> > > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > > page_id, block_id, row) to do address operations. Obviously, these
> > > arguments and variables cannot deal with more than 16-bit address.
> > > 
> > > Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>  
> > 
> > This seems reasonable...  It would be needed to make commit 6efb21d6d0e7
> > ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> > addressing.") work.  It also fixes a static checker warning.
> > 
> > My only concern is that the mtd/nand code seems to use -1 as a magical
> > page_id.  For example:
> 
> Yes, -1 means "don't issue the row/page address cycles", though I
> don't think page can be -1 for NAND_CMD_READ{1,0} commands.
> 

It sure looks like it can be in nand_exit_status_op()...

> Anyway, if you want this patch merged to fix a static checker warning,
> I'm fine with that. In any case, I still plan to send a patch removing
> this driver for v4.20, so, anyone using this driver should start
> testing the new SPI NAND driver (drivers/mtd/nand/spi) and tweak/fix
> the new implementation if needed.

I don't think we should make the code more complicated than necessary
just to make static checkers happy.  When you say "this driver", you
mean the staging driver?  In that case, there is no need to revert
commit 6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16
bits used for addressing.").

regards,
dan carpenter

Re: [PATCH] staging:mt29f_spinand: MT29F2G failing as only 16-bit arguments and variables used for addressing.

[Boris Brezillon]

On Mon, 6 Aug 2018 15:01:37 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Aug 06, 2018 at 01:46:48PM +0200, Boris Brezillon wrote:
> > Hi Dan,
> > 
> > On Wed, 1 Aug 2018 15:05:51 +0300
> > Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >   
> > > On Wed, Aug 01, 2018 at 11:24:19AM +0800, Jheng-Jhong Wu wrote:  
> > > > For NAND flash chips with more than 1Gbit (e.g. MT29F2G) more than 16 bits
> > > > are necessary to address the correct page. The driver sets the address for
> > > > more than 16 bits, but it uses 16-bit arguments and variables (these are
> > > > page_id, block_id, row) to do address operations. Obviously, these
> > > > arguments and variables cannot deal with more than 16-bit address.
> > > > 
> > > > Signed-off-by: Jheng-Jhong Wu <goodwater.wu@gmail.com>    
> > > 
> > > This seems reasonable...  It would be needed to make commit 6efb21d6d0e7
> > > ("staging:mt29f_spinand: MT29F2G failing as only 16 bits used for
> > > addressing.") work.  It also fixes a static checker warning.
> > > 
> > > My only concern is that the mtd/nand code seems to use -1 as a magical
> > > page_id.  For example:  
> > 
> > Yes, -1 means "don't issue the row/page address cycles", though I
> > don't think page can be -1 for NAND_CMD_READ{1,0} commands.
> >   
> 
> It sure looks like it can be in nand_exit_status_op()...

True, but nand_exit_status_op() won't be called here. It's only used
when the NAND controller driver is implementing ->exec_op() and needs
to do status polling, and from the micron NAND code that deals with raw
NAND chips with on-die ECC (the mt29f driver is supposed to deal with
SPI NANDs).

> 
> > Anyway, if you want this patch merged to fix a static checker warning,
> > I'm fine with that. In any case, I still plan to send a patch removing
> > this driver for v4.20, so, anyone using this driver should start
> > testing the new SPI NAND driver (drivers/mtd/nand/spi) and tweak/fix
> > the new implementation if needed.  
> 
> I don't think we should make the code more complicated than necessary
> just to make static checkers happy.  When you say "this driver", you
> mean the staging driver?

Yes.

> In that case, there is no need to revert
> commit 6efb21d6d0e7 ("staging:mt29f_spinand: MT29F2G failing as only 16
> bits used for addressing.").

Okay.