[PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Bean Huo 霍斌斌 (beanhuo)]

This patch used to modify the method of spi_nor_scan overwrite platform ID point.

If type of platform data match with the name of spi_nor_ids set,
and JEDEC ID also match with INFO ID of spi_nor_ids set,spi device
ID point(this is before probed according to device name) shouldn't be
overwrote.Otherwise,this point will be overwrote by new spi_nor_ids
set point that probed according to JEDEC ID.

Signed-off-by: bean huo <beanhuo@micron.com>
---
v1-v2:
	add extended ID match judgment.

 drivers/mtd/spi-nor/spi-nor.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b5ad6be..3bb8077 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -963,16 +963,23 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 			return PTR_ERR(jid);
 		} else if (jid != id) {
 			/*
-			 * JEDEC knows better, so overwrite platform ID. We
-			 * can't trust partitions any longer, but we'll let
-			 * mtd apply them anyway, since some partitions may be
-			 * marked read-only, and we don't want to lose that
-			 * information, even if it's not 100% accurate.
+			 * If type of platform data match with the name of
+			 * spi_nor_ids set,and JEDEC ID also match with
+			 * INFO ID of spi_nor_ids set,shouldn't overwrite
+			 * spi device info point.Otherwise,will overwrite
+			 * it.
 			 */
-			dev_warn(dev, "found %s, expected %s\n",
-				 jid->name, id->name);
-			id = jid;
-			info = (void *)jid->driver_data;
+			struct flash_info *tmpinfo;
+
+			tmpinfo = (void *)jid->driver_data;
+			if (tmpinfo->jedec_id != info->jedec_id ||
+					(info->ext_id != 0 &&
+					 tmpinfo->ext_id != info->ext_id)) {
+				dev_warn(dev, "found %s, expected %s\n",
+						jid->name, id->name);
+				id = jid;
+				info = (void *)jid->driver_data;
+			}
 		}
 	}
 
-- 
1.7.9.5

Re: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Marek Vasut]

On Tuesday, October 14, 2014 at 03:26:49 AM, Bean Huo 霍斌斌 (beanhuo) wrote:
[...]

> -			dev_warn(dev, "found %s, expected %s\n",
> -				 jid->name, id->name);
> -			id = jid;
> -			info = (void *)jid->driver_data;
> +			struct flash_info *tmpinfo;
> +
> +			tmpinfo = (void *)jid->driver_data;
> +			if (tmpinfo->jedec_id != info->jedec_id ||
> +					(info->ext_id != 0 &&
> +					 tmpinfo->ext_id != info->ext_id)) {
> +				dev_warn(dev, "found %s, expected %s\n",
> +						jid->name, id->name);
> +				id = jid;
> +				info = (void *)jid->driver_data;
> +			}

Won't $info contain an undefined value in case the newly added condition isn't 
met ? The old code initialized $info to a certain value always, the new code 
does not do that.

Best regards,
Marek Vasut

RE: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[bpqw]

>Won't $info contain an undefined value in case the newly added condition isn't met ? The old code initialized $info to a certain value always, the new code does not do that.

Hi,Marek Vasut
 Thanks.the $info has been defined before as below:

 info = (void *)id->driver_data;
  
Unless id has not been initialized before.If having the following situation,
current linux codes will not properly match platform device data:

For example:

const struct spi_device_id spi_nor_ids[] = {
......
......
......
	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },//former right platform point will be overwrote by this data info.
	{ "n25q128a13",  INFO(0x20bb18, 0x1234, 64 * 1024,  512, SECT_4K) },//this is the right platform data,I want to match this data info.
	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
......
......
......
};
EXPORT_SYMBOL_GPL(spi_nor_ids);

Maybe my patch should take into account that ID is NULL,
I want to change my patch following, could you please give me some suggestions:

	struct flash_info *tmpinfo;

	tmpinfo = (void *)jid->driver_data;
	if (id == NULL || tmpinfo->jedec_id != info->jedec_id ||
		 (info->ext_id != 0 && tmpinfo->ext_id != info->ext_id)) {
			dev_warn(dev, "found %s, expected %s\n",
				jid->name, id->name);
			id = jid;
			info = (void *)jid->driver_data;
				}

Re: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Rafał Miłecki]

On 14 October 2014 03:26, Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote:
> This patch used to modify the method of spi_nor_scan overwrite platform ID point.
>
> If type of platform data match with the name of spi_nor_ids set,
> and JEDEC ID also match with INFO ID of spi_nor_ids set,spi device
> ID point(this is before probed according to device name) shouldn't be
> overwrote.Otherwise,this point will be overwrote by new spi_nor_ids
> set point that probed according to JEDEC ID.

There are a lot of changes happening/requested around this code. I
also proposed some patch touching this code, see
https://patchwork.ozlabs.org/patch/377917/

Right now there is a slow ongoing work on fixing some m25p80
regression, which also may touch the code you change. So I'll suggest
postponing this patch until we get a regression fixed and then work on
this hacky code again.

RE: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Bean Huo 霍斌斌 (beanhuo)]

>There are a lot of changes happening/requested around this code. I also proposed some patch touching this code, see https://patchwork.ozlabs.org/patch/377917/

>Right now there is a slow ongoing work on fixing some m25p80 regression, which also may touch the code you change. So I'll suggest postponing this patch until we get a regression fixed and then work on this hacky code again.


Thanks for your concerns.But for the same deivce id with the different extended id and
the same device name with the different device id issue, in your patch,this doesn't take
into account. I have encountered this situation. for example:


first condition (for the same deivce id with the different extended id):

	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
	{ "n25ml064",    INFO(0x20ba17, 0x1234, 64 * 1024,  256, 0) },//right device data
	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },

second condition(the same device id with the different device name):

 	{ "n25tl28",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
	{ "n25ml028",     INFO(0x20ba17, 0, 64 * 1024,  256, 0) },//right device data
	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },

Re: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Rafał Miłecki]

On 16 October 2014 10:00, Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote:
>>There are a lot of changes happening/requested around this code. I also proposed some patch touching this code, see https://patchwork.ozlabs.org/patch/377917/
>
>>Right now there is a slow ongoing work on fixing some m25p80 regression, which also may touch the code you change. So I'll suggest postponing this patch until we get a regression fixed and then work on this hacky code again.
>
>
> Thanks for your concerns.But for the same deivce id with the different extended id and
> the same device name with the different device id issue, in your patch,this doesn't take
> into account. I have encountered this situation. for example:

I really wasn't looking into details yet and I'm aware my patch does
something else. I just say we should first fix the regression and then
base next patches on top of that regression fix. I'm not NACKing your
changes :)

RE: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[bpqw]

>I really wasn't looking into details yet and I'm aware my patch does something else. I just say we should first fix the regression and then base next patches on top of that regression fix. I'm not NACKing your changes :)

I will take into account for your patch,and will cover all these isse in my next version patch.

By the way ,I still don't understand why you specify id NULL in you patch:

+	if (id) {
+		info = (void *)id->driver_data;
+		if (info->jedec_id) {
+			dev_warn(dev,
+				 "passed SPI device ID (%s) contains JEDEC, ignoring it, driver should be fixed!\n",
+				 id->name);
+			id = NULL;
 		}
 	}
 
+	if (!id) {
+		id = nor->read_id(nor);
+		if (IS_ERR(id))
+			return PTR_ERR(id);
+	}
+	info = (void *)id->driver_data;
+

Re: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Rafał Miłecki]

On 16 October 2014 10:41, bpqw <bpqw@micron.com> wrote:
> By the way ,I still don't understand why you specify id NULL in you patch:

I guess I needed it for my Broadcom SPI driver, see:
mtd: bcm53xxspiflash: new driver for SPI flahes on Broadcom ARM SoCs
https://patchwork.ozlabs.org/patch/381902/

But I'll need to re-think that and update my patch.

Re: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Rafał Miłecki]

On 16 October 2014 02:52, bpqw <bpqw@micron.com> wrote:
> For example:
>
> const struct spi_device_id spi_nor_ids[] = {
> ......
> ......
> ......
>         { "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
>         { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },//former right platform point will be overwrote by this data info.
>         { "n25q128a13",  INFO(0x20bb18, 0x1234, 64 * 1024,  512, SECT_4K) },//this is the right platform data,I want to match this data info.
>         { "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
>         { "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },

Your initial commit was really missing some example indeed. It was
trying to understand it for 10 minutes with no luck (until seeing
above case) ;)

I see problem in spi-nor that your patch reveals:
ISSUE: spi-nor accepts any entry from spi_nor_ids with no ext_id
specified as long as the jedec_id matches
I guess reason behind this is to support chips with the same
parameters but different extended IDs. That simplifies the driver and
support for extra chips but may also cause problems like in your case.

So AFAIU your patch tries to force using specific flash info, because
JEDEC based detection fails. In general this is what we try to avoid.
We want to auto-detect flash chips and to relieve platform code / DT.

Could we drop this patch and modify spi-nor to detect your chip
automatically? That would be much cleaner solution to me.

RE: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[bpqw]

Hi,rafal

Maybe this patch is not very reasonable.But for fix this case,I will develop a new  patch that
is just used to add extended ID for micron spi nor in the spi_nor_ids[].
Thanks for your review my patch.

Re: [PATCH 1/1 v2] driver:mtd:spi-nor:fix spi_nor_scan overwrite platform ID point

[Rafał Miłecki]

On 27 October 2014 09:44, bpqw <bpqw@micron.com> wrote:
> Maybe this patch is not very reasonable.But for fix this case,I will develop a new  patch that
> is just used to add extended ID for micron spi nor in the spi_nor_ids[].

Great, I hope it will work on top of the
[PATCH] mtd: spi-nor: prefer more specific entries from chips database
, good luck :)

-- 
Rafał