[PATCH] nandsim: add id_bytes module parameter

[PATCH] nandsim: add id_bytes module parameter

[Akinobu Mita]

nandsim can simulate NAND Flash which returns the ID bytes specified
by first_id_byte, ..., fourth_id_byte module parameters.

In order to simulate NAND flash which returns more than four ID bytes,
this adds id_bytes module parameter which is specified by the array of
byte like this:

 # modprobe nandsim id_bytes=0x98,0xdc,0x90,0x26,0x76,0x15,0x01,0x08 bch=1

This doesn't add fifth_id_byte, ..., seventh_id_byte module parameters,
becuase they are redundant.  But the existing first_id_byte, ...,
fourth_id_byte module parameters are preserved.

Cc: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Brian Norris <computersforpeace@gmail.com>
Cc: linux-mtd@lists.infradead.org
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/mtd/nand/nandsim.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
index 4c0ada3..8b4a48e 100644
--- a/drivers/mtd/nand/nandsim.c
+++ b/drivers/mtd/nand/nandsim.c
@@ -89,10 +89,6 @@
 #define CONFIG_NANDSIM_MAX_PARTS  32
 #endif
 
-static uint first_id_byte  = CONFIG_NANDSIM_FIRST_ID_BYTE;
-static uint second_id_byte = CONFIG_NANDSIM_SECOND_ID_BYTE;
-static uint third_id_byte  = CONFIG_NANDSIM_THIRD_ID_BYTE;
-static uint fourth_id_byte = CONFIG_NANDSIM_FOURTH_ID_BYTE;
 static uint access_delay   = CONFIG_NANDSIM_ACCESS_DELAY;
 static uint programm_delay = CONFIG_NANDSIM_PROGRAMM_DELAY;
 static uint erase_delay    = CONFIG_NANDSIM_ERASE_DELAY;
@@ -113,11 +109,19 @@ static unsigned int overridesize = 0;
 static char *cache_file = NULL;
 static unsigned int bbt;
 static unsigned int bch;
+static u_char id_bytes[8] = {
+	[0] = CONFIG_NANDSIM_FIRST_ID_BYTE,
+	[1] = CONFIG_NANDSIM_SECOND_ID_BYTE,
+	[2] = CONFIG_NANDSIM_THIRD_ID_BYTE,
+	[3] = CONFIG_NANDSIM_FOURTH_ID_BYTE,
+	[4 ... 7] = 0xFF,
+};
 
-module_param(first_id_byte,  uint, 0400);
-module_param(second_id_byte, uint, 0400);
-module_param(third_id_byte,  uint, 0400);
-module_param(fourth_id_byte, uint, 0400);
+module_param_array(id_bytes, byte, NULL, 0400);
+module_param_cb(first_id_byte, &param_ops_byte, &id_bytes[0], 0400);
+module_param_cb(second_id_byte, &param_ops_byte, &id_bytes[1], 0400);
+module_param_cb(third_id_byte, &param_ops_byte, &id_bytes[2], 0400);
+module_param_cb(fourth_id_byte, &param_ops_byte, &id_bytes[3], 0400);
 module_param(access_delay,   uint, 0400);
 module_param(programm_delay, uint, 0400);
 module_param(erase_delay,    uint, 0400);
@@ -138,6 +142,7 @@ module_param(cache_file,     charp, 0400);
 module_param(bbt,	     uint, 0400);
 module_param(bch,	     uint, 0400);
 
+MODULE_PARM_DESC(id_bytes,       "The ID bytes returned by NAND Flash 'read ID' command");
 MODULE_PARM_DESC(first_id_byte,  "The first byte returned by NAND Flash 'read ID' command (manufacturer ID)");
 MODULE_PARM_DESC(second_id_byte, "The second byte returned by NAND Flash 'read ID' command (chip ID)");
 MODULE_PARM_DESC(third_id_byte,  "The third byte returned by NAND Flash 'read ID' command");
@@ -306,7 +311,7 @@ struct nandsim {
 	unsigned int nbparts;
 
 	uint busw;              /* flash chip bus width (8 or 16) */
-	u_char ids[4];          /* chip's ID bytes */
+	u_char ids[8];          /* chip's ID bytes */
 	uint32_t options;       /* chip's characteristic bits */
 	uint32_t state;         /* current chip state */
 	uint32_t nxstate;       /* next expected state */
@@ -2214,17 +2219,18 @@ static int __init ns_init_module(void)
 	 * Perform minimum nandsim structure initialization to handle
 	 * the initial ID read command correctly
 	 */
-	if (third_id_byte != 0xFF || fourth_id_byte != 0xFF)
+	if (id_bytes[6] != 0xFF || id_bytes[7] != 0xFF)
+		nand->geom.idbytes = 8;
+	else if (id_bytes[4] != 0xFF || id_bytes[5] != 0xFF)
+		nand->geom.idbytes = 6;
+	else if (id_bytes[2] != 0xFF || id_bytes[3] != 0xFF)
 		nand->geom.idbytes = 4;
 	else
 		nand->geom.idbytes = 2;
 	nand->regs.status = NS_STATUS_OK(nand);
 	nand->nxstate = STATE_UNKNOWN;
 	nand->options |= OPT_PAGE512; /* temporary value */
-	nand->ids[0] = first_id_byte;
-	nand->ids[1] = second_id_byte;
-	nand->ids[2] = third_id_byte;
-	nand->ids[3] = fourth_id_byte;
+	memcpy(nand->ids, id_bytes, sizeof(nand->ids));
 	if (bus_width == 16) {
 		nand->busw = 16;
 		chip->options |= NAND_BUSWIDTH_16;
-- 
1.9.1

Re: [PATCH] nandsim: add id_bytes module parameter

[Artem Bityutskiy]

On Mon, 2014-08-11 at 08:29 +0900, Akinobu Mita wrote:
> nandsim can simulate NAND Flash which returns the ID bytes specified
> by first_id_byte, ..., fourth_id_byte module parameters.
> 
> In order to simulate NAND flash which returns more than four ID bytes,
> this adds id_bytes module parameter which is specified by the array of
> byte like this:
> 
>  # modprobe nandsim id_bytes=0x98,0xdc,0x90,0x26,0x76,0x15,0x01,0x08 bch=1
> 
> This doesn't add fifth_id_byte, ..., seventh_id_byte module parameters,
> becuase they are redundant.  But the existing first_id_byte, ...,
> fourth_id_byte module parameters are preserved.

Hi, I missed this patch, sorry. It looks good to me, I'll take it to my
tree.

-- 
Best Regards,
Artem Bityutskiy

Re: [PATCH] nandsim: add id_bytes module parameter

[Artem Bityutskiy]

On Mon, 2014-10-13 at 16:48 +0300, Artem Bityutskiy wrote:
> On Mon, 2014-08-11 at 08:29 +0900, Akinobu Mita wrote:
> > nandsim can simulate NAND Flash which returns the ID bytes specified
> > by first_id_byte, ..., fourth_id_byte module parameters.
> > 
> > In order to simulate NAND flash which returns more than four ID bytes,
> > this adds id_bytes module parameter which is specified by the array of
> > byte like this:
> > 
> >  # modprobe nandsim id_bytes=0x98,0xdc,0x90,0x26,0x76,0x15,0x01,0x08 bch=1
> > 
> > This doesn't add fifth_id_byte, ..., seventh_id_byte module parameters,
> > becuase they are redundant.  But the existing first_id_byte, ...,
> > fourth_id_byte module parameters are preserved.
> 
> Hi, I missed this patch, sorry. It looks good to me, I'll take it to my
> tree.

Actually, let's merge this via the l2-mtd.git tree. Brian, what do you
think about this patch? It looks good for me in general, but I did not
review it line-by-line. The only thing is that the 'modinfo nandsim' may
look confusing for the user, who sees so many ID-related parameters, so
I'd add an "(obsolete)" marker to the string describing the old
parameters. But this is a minor thing, I did not want to ask Akinobu
about this because the patch was already waiting for very long time, I'd
do this myself while merging.

Artem.

Re: [PATCH] nandsim: add id_bytes module parameter

[Brian Norris]

On Mon, Oct 13, 2014 at 06:40:45PM +0300, Artem Bityutskiy wrote:
> On Mon, 2014-10-13 at 16:48 +0300, Artem Bityutskiy wrote:
> > On Mon, 2014-08-11 at 08:29 +0900, Akinobu Mita wrote:
> > > nandsim can simulate NAND Flash which returns the ID bytes specified
> > > by first_id_byte, ..., fourth_id_byte module parameters.
> > > 
> > > In order to simulate NAND flash which returns more than four ID bytes,
> > > this adds id_bytes module parameter which is specified by the array of
> > > byte like this:
> > > 
> > >  # modprobe nandsim id_bytes=0x98,0xdc,0x90,0x26,0x76,0x15,0x01,0x08 bch=1
> > > 
> > > This doesn't add fifth_id_byte, ..., seventh_id_byte module parameters,
> > > becuase they are redundant.  But the existing first_id_byte, ...,
> > > fourth_id_byte module parameters are preserved.
> > 
> > Hi, I missed this patch, sorry. It looks good to me, I'll take it to my
> > tree.
> 
> Actually, let's merge this via the l2-mtd.git tree. Brian, what do you
> think about this patch? It looks good for me in general, but I did not
> review it line-by-line. The only thing is that the 'modinfo nandsim' may
> look confusing for the user, who sees so many ID-related parameters, so
> I'd add an "(obsolete)" marker to the string describing the old
> parameters. But this is a minor thing, I did not want to ask Akinobu
> about this because the patch was already waiting for very long time, I'd
> do this myself while merging.

Sorry, I really don't have the time to review every single patch these
days. I had briefly looked at this patch previously and thought it was a
good idea, but like you, I didn't look through all the code.

I'll try to queue this up after the merge window, unless I discover
something that really must be improved.

Brian

Re: [PATCH] nandsim: add id_bytes module parameter

[Akinobu Mita]

2014-10-16 8:04 GMT+09:00 Brian Norris <computersforpeace@gmail.com>:
> On Mon, Oct 13, 2014 at 06:40:45PM +0300, Artem Bityutskiy wrote:
>> On Mon, 2014-10-13 at 16:48 +0300, Artem Bityutskiy wrote:
>> > On Mon, 2014-08-11 at 08:29 +0900, Akinobu Mita wrote:
>> > > nandsim can simulate NAND Flash which returns the ID bytes specified
>> > > by first_id_byte, ..., fourth_id_byte module parameters.
>> > >
>> > > In order to simulate NAND flash which returns more than four ID bytes,
>> > > this adds id_bytes module parameter which is specified by the array of
>> > > byte like this:
>> > >
>> > >  # modprobe nandsim id_bytes=0x98,0xdc,0x90,0x26,0x76,0x15,0x01,0x08 bch=1
>> > >
>> > > This doesn't add fifth_id_byte, ..., seventh_id_byte module parameters,
>> > > becuase they are redundant.  But the existing first_id_byte, ...,
>> > > fourth_id_byte module parameters are preserved.
>> >
>> > Hi, I missed this patch, sorry. It looks good to me, I'll take it to my
>> > tree.
>>
>> Actually, let's merge this via the l2-mtd.git tree. Brian, what do you
>> think about this patch? It looks good for me in general, but I did not
>> review it line-by-line. The only thing is that the 'modinfo nandsim' may
>> look confusing for the user, who sees so many ID-related parameters, so
>> I'd add an "(obsolete)" marker to the string describing the old
>> parameters. But this is a minor thing, I did not want to ask Akinobu
>> about this because the patch was already waiting for very long time, I'd
>> do this myself while merging.
>
> Sorry, I really don't have the time to review every single patch these
> days. I had briefly looked at this patch previously and thought it was a
> good idea, but like you, I didn't look through all the code.
>
> I'll try to queue this up after the merge window, unless I discover
> something that really must be improved.

Thanks for considering this patch.

After reading this again, I realized that module_param_named() should
be used instead of module_param_cb() for existing parameters.  So I'll
send updated patch with adding "(obsolete)" to module description as
Artem suggested.