[PATCH v4 0/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[PATCH v4 0/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

As pointed out by this bug report [1], buffered writes are now broken on
S29GL064N. This issue comes from a rework which switched from using chip_good()
to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
returned by chip_good(). One way to solve the issue is to revert the change
partially to use chip_ready for S29GL064N.

[1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/

Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: stable@vger.kernel.org

Tokunori Ikegami (3):
  mtd: cfi_cmdset_0002: Move and rename
    chip_check/chip_ready/chip_good_for_write
  mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N
  mtd: cfi_cmdset_0002: Add S29GL064N ID definition

 drivers/mtd/chips/cfi_cmdset_0002.c | 93 +++++++++++++++--------------
 1 file changed, 49 insertions(+), 44 deletions(-)

-- 
2.32.0

[PATCH v4 1/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write

[Tokunori Ikegami]

This is a preparation patch for the functional change in preparation to a change
expected to fix the buffered writes on S29GL064N.

Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 77 ++++++++++++-----------------
 1 file changed, 32 insertions(+), 45 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index a761134fd3be..e68ddf0f7fc0 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -801,22 +801,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
 	return NULL;
 }
 
-/*
- * Return true if the chip is ready.
- *
- * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
- * non-suspended sector) and is indicated by no toggle bits toggling.
- *
- * Note that anything more complicated than checking if no bits are toggling
- * (including checking DQ5 for an error status) is tricky to get working
- * correctly and is therefore not done	(particularly with interleaved chips
- * as each chip must be checked independently of the others).
- */
-static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
-			       unsigned long addr)
+static int __xipram chip_check(struct map_info *map, struct flchip *chip,
+			       unsigned long addr, map_word *expected)
 {
 	struct cfi_private *cfi = map->fldrv_priv;
-	map_word d, t;
+	map_word oldd, curd;
+	int ret;
 
 	if (cfi_use_status_reg(cfi)) {
 		map_word ready = CMD(CFI_SR_DRB);
@@ -826,17 +816,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
 		 */
 		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
 				 cfi->device_type, NULL);
-		d = map_read(map, addr);
+		curd = map_read(map, addr);
 
-		return map_word_andequal(map, d, ready, ready);
+		return map_word_andequal(map, curd, ready, ready);
 	}
 
-	d = map_read(map, addr);
-	t = map_read(map, addr);
+	oldd = map_read(map, addr);
+	curd = map_read(map, addr);
+
+	ret = map_word_equal(map, oldd, curd);
 
-	return map_word_equal(map, d, t);
+	if (!ret || !expected)
+		return ret;
+
+	return map_word_equal(map, curd, *expected);
 }
 
+/*
+ * Return true if the chip is ready.
+ *
+ * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
+ * non-suspended sector) and is indicated by no toggle bits toggling.
+ *
+ * Note that anything more complicated than checking if no bits are toggling
+ * (including checking DQ5 for an error status) is tricky to get working
+ * correctly and is therefore not done	(particularly with interleaved chips
+ * as each chip must be checked independently of the others).
+ */
+#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL)
+
 /*
  * Return true if the chip is ready and has the correct value.
  *
@@ -852,32 +860,11 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
  * as each chip must be checked independently of the others).
  *
  */
-static int __xipram chip_good(struct map_info *map, struct flchip *chip,
-			      unsigned long addr, map_word expected)
-{
-	struct cfi_private *cfi = map->fldrv_priv;
-	map_word oldd, curd;
-
-	if (cfi_use_status_reg(cfi)) {
-		map_word ready = CMD(CFI_SR_DRB);
-
-		/*
-		 * For chips that support status register, check device
-		 * ready bit
-		 */
-		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
-				 cfi->device_type, NULL);
-		curd = map_read(map, addr);
-
-		return map_word_andequal(map, curd, ready, ready);
-	}
-
-	oldd = map_read(map, addr);
-	curd = map_read(map, addr);
-
-	return	map_word_equal(map, oldd, curd) &&
-		map_word_equal(map, curd, expected);
-}
+#define chip_good(map, chip, addr, expected) \
+	({ \
+		map_word datum = expected; \
+		chip_check(map, chip, addr, &datum); \
+	})
 
 static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
 {
-- 
2.32.0

[PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

As pointed out by this bug report [1], buffered writes are now broken on
S29GL064N. This issue comes from a rework which switched from using chip_good()
to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
returned by chip_good(). One way to solve the issue is to revert the change
partially to use chip_ready for S29GL064N.

[1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/

Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Cc: stable@vger.kernel.org
---
 drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
index e68ddf0f7fc0..6c57f85e1b8e 100644
--- a/drivers/mtd/chips/cfi_cmdset_0002.c
+++ b/drivers/mtd/chips/cfi_cmdset_0002.c
@@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
 		chip_check(map, chip, addr, &datum); \
 	})
 
+static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
+{
+	struct cfi_private *cfi = map->fldrv_priv;
+
+	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
+}
+
+static int __xipram chip_good_for_write(struct map_info *map,
+					struct flchip *chip, unsigned long addr,
+					map_word expected)
+{
+	if (cfi_use_chip_ready_for_write(map))
+		return chip_ready(map, chip, addr);
+
+	return chip_good(map, chip, addr, expected);
+}
+
 static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
 {
 	DECLARE_WAITQUEUE(wait, current);
@@ -1686,7 +1703,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
 		 * "chip_good" to avoid the failure due to scheduling.
 		 */
 		if (time_after(jiffies, timeo) &&
-		    !chip_good(map, chip, adr, datum)) {
+		    !chip_good_for_write(map, chip, adr, datum)) {
 			xip_enable(map, chip, adr);
 			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
 			xip_disable(map, chip, adr);
@@ -1694,7 +1711,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
 			break;
 		}
 
-		if (chip_good(map, chip, adr, datum)) {
+		if (chip_good_for_write(map, chip, adr, datum)) {
 			if (cfi_check_err_status(map, chip, adr))
 				ret = -EIO;
 			break;
@@ -1966,14 +1983,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map,
 		 * "chip_good" to avoid the failure due to scheduling.
 		 */
 		if (time_after(jiffies, timeo) &&
-		    !chip_good(map, chip, adr, datum)) {
+		    !chip_good_for_write(map, chip, adr, datum)) {
 			pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
 			       __func__, adr);
 			ret = -EIO;
 			break;
 		}
 
-		if (chip_good(map, chip, adr, datum)) {
+		if (chip_good_for_write(map, chip, adr, datum)) {
 			if (cfi_check_err_status(map, chip, adr))
 				ret = -EIO;
 			break;
-- 
2.32.0

Re: [PATCH v4 1/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write

[Miquel Raynal]

Hi Tokunori,

ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:53 +0900:

> This is a preparation patch for the functional change in preparation to a change
> expected to fix the buffered writes on S29GL064N.

This is a preparation patch for the S29GL064N buffer writes fix. There
is no functional change.

> 
> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 77 ++++++++++++-----------------
>  1 file changed, 32 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index a761134fd3be..e68ddf0f7fc0 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -801,22 +801,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
>  	return NULL;
>  }
>  
> -/*
> - * Return true if the chip is ready.
> - *
> - * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
> - * non-suspended sector) and is indicated by no toggle bits toggling.
> - *
> - * Note that anything more complicated than checking if no bits are toggling
> - * (including checking DQ5 for an error status) is tricky to get working
> - * correctly and is therefore not done	(particularly with interleaved chips
> - * as each chip must be checked independently of the others).
> - */
> -static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
> -			       unsigned long addr)
> +static int __xipram chip_check(struct map_info *map, struct flchip *chip,
> +			       unsigned long addr, map_word *expected)
>  {
>  	struct cfi_private *cfi = map->fldrv_priv;
> -	map_word d, t;
> +	map_word oldd, curd;
> +	int ret;
>  
>  	if (cfi_use_status_reg(cfi)) {
>  		map_word ready = CMD(CFI_SR_DRB);
> @@ -826,17 +816,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>  		 */
>  		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>  				 cfi->device_type, NULL);
> -		d = map_read(map, addr);
> +		curd = map_read(map, addr);
>  
> -		return map_word_andequal(map, d, ready, ready);
> +		return map_word_andequal(map, curd, ready, ready);

A lot of the diff is just a rename. I am not against a rename if you
feel it's better, but in this order:
1: prepare the fix
2: fix
3: rename/define id's, whatever

>  	}
>  
> -	d = map_read(map, addr);
> -	t = map_read(map, addr);
> +	oldd = map_read(map, addr);
> +	curd = map_read(map, addr);
> +
> +	ret = map_word_equal(map, oldd, curd);
>  
> -	return map_word_equal(map, d, t);
> +	if (!ret || !expected)
> +		return ret;
> +
> +	return map_word_equal(map, curd, *expected);
>  }
>  
> +/*
> + * Return true if the chip is ready.
> + *
> + * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
> + * non-suspended sector) and is indicated by no toggle bits toggling.
> + *
> + * Note that anything more complicated than checking if no bits are toggling
> + * (including checking DQ5 for an error status) is tricky to get working
> + * correctly and is therefore not done	(particularly with interleaved chips
> + * as each chip must be checked independently of the others).
> + */
> +#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL)

I don't see the point of such a define. Just get rid of it.

> +
>  /*
>   * Return true if the chip is ready and has the correct value.
>   *
> @@ -852,32 +860,11 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>   * as each chip must be checked independently of the others).
>   *
>   */
> -static int __xipram chip_good(struct map_info *map, struct flchip *chip,
> -			      unsigned long addr, map_word expected)
> -{
> -	struct cfi_private *cfi = map->fldrv_priv;
> -	map_word oldd, curd;
> -
> -	if (cfi_use_status_reg(cfi)) {
> -		map_word ready = CMD(CFI_SR_DRB);
> -
> -		/*
> -		 * For chips that support status register, check device
> -		 * ready bit
> -		 */
> -		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
> -				 cfi->device_type, NULL);
> -		curd = map_read(map, addr);
> -
> -		return map_word_andequal(map, curd, ready, ready);
> -	}
> -
> -	oldd = map_read(map, addr);
> -	curd = map_read(map, addr);
> -
> -	return	map_word_equal(map, oldd, curd) &&
> -		map_word_equal(map, curd, expected);
> -}
> +#define chip_good(map, chip, addr, expected) \
> +	({ \
> +		map_word datum = expected; \
> +		chip_check(map, chip, addr, &datum); \
> +	})

What is this for? Same here, I don't see the point. Please get rid of
all these unnecessary helpers which do nothing, besides complicating
user's life.

>  static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
>  {


Thanks,
Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Miquel Raynal]

Hi Tokunori,

ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:

> As pointed out by this bug report [1], buffered writes are now broken on
> S29GL064N. This issue comes from a rework which switched from using chip_good()
> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
> returned by chip_good().

Vignesh, I believe you understand this issue better than I do, can you
propose an improved commit log?

> One way to solve the issue is to revert the change
> partially to use chip_ready for S29GL064N.
> 
> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> 
> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: stable@vger.kernel.org
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index e68ddf0f7fc0..6c57f85e1b8e 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>  		chip_check(map, chip, addr, &datum); \
>  	})
>  
> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)

At the very least I would call this function:
cfi_use_chip_ready_for_writes()

Yet, I still don't fully get what chip_ready is versus chip_good.

> +{
> +	struct cfi_private *cfi = map->fldrv_priv;
> +
> +	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
> +}
> +
> +static int __xipram chip_good_for_write(struct map_info *map,
> +					struct flchip *chip, unsigned long addr,
> +					map_word expected)
> +{
> +	if (cfi_use_chip_ready_for_write(map))
> +		return chip_ready(map, chip, addr);

If possible and not too invasive I would definitely add a "quirks" flag
somewhere instead of this cfi_use_chip_ready_for_write() check.

Anyway, I would move this to the chip_good() implementation directly so
we partially hide the quirks complexity from the core.

> +
> +	return chip_good(map, chip, addr, expected);
> +}
> +
>  static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
>  {
>  	DECLARE_WAITQUEUE(wait, current);
> @@ -1686,7 +1703,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
>  		 * "chip_good" to avoid the failure due to scheduling.
>  		 */
>  		if (time_after(jiffies, timeo) &&
> -		    !chip_good(map, chip, adr, datum)) {
> +		    !chip_good_for_write(map, chip, adr, datum)) {
>  			xip_enable(map, chip, adr);
>  			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
>  			xip_disable(map, chip, adr);
> @@ -1694,7 +1711,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
>  			break;
>  		}
>  
> -		if (chip_good(map, chip, adr, datum)) {
> +		if (chip_good_for_write(map, chip, adr, datum)) {
>  			if (cfi_check_err_status(map, chip, adr))
>  				ret = -EIO;
>  			break;
> @@ -1966,14 +1983,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map,
>  		 * "chip_good" to avoid the failure due to scheduling.
>  		 */
>  		if (time_after(jiffies, timeo) &&
> -		    !chip_good(map, chip, adr, datum)) {
> +		    !chip_good_for_write(map, chip, adr, datum)) {
>  			pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
>  			       __func__, adr);
>  			ret = -EIO;
>  			break;
>  		}
>  
> -		if (chip_good(map, chip, adr, datum)) {
> +		if (chip_good_for_write(map, chip, adr, datum)) {
>  			if (cfi_check_err_status(map, chip, adr))
>  				ret = -EIO;
>  			break;


Thanks,
Miquèl

Re: [PATCH v4 0/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Miquel Raynal]

Hi Tokunori,

ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:52 +0900:

> As pointed out by this bug report [1], buffered writes are now broken on
> S29GL064N. This issue comes from a rework which switched from using chip_good()
> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
> returned by chip_good(). One way to solve the issue is to revert the change
> partially to use chip_ready for S29GL064N.

Thanks for your quick iterations but overall I seriously don't feel
this series is ready. At least right now I am still not satisfied by
its shape.

> 
> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> 
> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")

... and this Fixes tag points to a v4.18 kernel, so if this is true, we
should have a backport-able patch. This also means that this is not as
critical than I thought if people already lived 4 years with the bug.

Vignesh, I let you continue iterating with Tokunori and ack the series
when you feel it's ready.

> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> Cc: stable@vger.kernel.org
> 
> Tokunori Ikegami (3):
>   mtd: cfi_cmdset_0002: Move and rename
>     chip_check/chip_ready/chip_good_for_write
>   mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N
>   mtd: cfi_cmdset_0002: Add S29GL064N ID definition
> 
>  drivers/mtd/chips/cfi_cmdset_0002.c | 93 +++++++++++++++--------------
>  1 file changed, 49 insertions(+), 44 deletions(-)
> 

Thanks,
Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Vignesh Raghavendra]

On 16/03/22 10:51 pm, Miquel Raynal wrote:
> Hi Tokunori,
> 
> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
> 
>> As pointed out by this bug report [1], buffered writes are now broken on
>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>> returned by chip_good().
> 
> Vignesh, I believe you understand this issue better than I do, can you
> propose an improved commit log?

How about:

Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
check correct value") buffered writes fail on S29GL064N. This is
because, on S29GL064N, reads return 0xFF at the end of DQ polling for
write completion, where as, chip_good() check expects actual data
written to the last location to be returned post DQ polling completion.
Fix is to revert to using chip_good() for S29GL064N which only checks
for DQ lines to settle down to determine write completion.

> 
>> One way to solve the issue is to revert the change
>> partially to use chip_ready for S29GL064N.
>>
>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>
>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>  		chip_check(map, chip, addr, &datum); \
>>  	})
>>  
>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
> 
> At the very least I would call this function:
> cfi_use_chip_ready_for_writes()
> 
> Yet, I still don't fully get what chip_ready is versus chip_good.
> 
>> +{
>> +	struct cfi_private *cfi = map->fldrv_priv;
>> +
>> +	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>> +}
>> +
>> +static int __xipram chip_good_for_write(struct map_info *map,
>> +					struct flchip *chip, unsigned long addr,
>> +					map_word expected)
>> +{
>> +	if (cfi_use_chip_ready_for_write(map))
>> +		return chip_ready(map, chip, addr);
> 
> If possible and not too invasive I would definitely add a "quirks" flag
> somewhere instead of this cfi_use_chip_ready_for_write() check.
> 
> Anyway, I would move this to the chip_good() implementation directly so
> we partially hide the quirks complexity from the core.

Yeah, unfortunately this driver does not use quirk flags and tends to
hide quirks behind bool functions like above

Regards
Vignesh

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Ahmad Fatoum]

Hello Vignesh,

On 17.03.22 11:01, Vignesh Raghavendra wrote:
> 
> 
> On 16/03/22 10:51 pm, Miquel Raynal wrote:
>> Hi Tokunori,
>>
>> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
>>
>>> As pointed out by this bug report [1], buffered writes are now broken on
>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>> returned by chip_good().
>>
>> Vignesh, I believe you understand this issue better than I do, can you
>> propose an improved commit log?
> 
> How about:
> 
> Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
> check correct value") buffered writes fail on S29GL064N. This is
> because, on S29GL064N, reads return 0xFF at the end of DQ polling for
> write completion, where as, chip_good() check expects actual data
> written to the last location to be returned post DQ polling completion.
> Fix is to revert to using chip_good() for S29GL064N which only checks
> for DQ lines to settle down to determine write completion.

Message sounds good to me with one remark: The issue is independent of
whether buffered writes are used or not. It's just because buffered writes
are the default, that it was broken by dfeae1073583 ("mtd: cfi_cmdset_0002:
Change write buffer to check correct value"). The word write case was broken
by 37c673ade35c ("mtd: cfi_cmdset_0002: Use chip_good() to retry in
do_write_oneword()"), so the commit message should probably reference
both. as this commit indeed fixes both FORCE_WORD_WRITE == 0 and == 1.

Thanks,
Ahmad


> 
>>
>>> One way to solve the issue is to revert the change
>>> partially to use chip_ready for S29GL064N.
>>>
>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>
>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>  drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>>  1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>>  		chip_check(map, chip, addr, &datum); \
>>>  	})
>>>  
>>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
>>
>> At the very least I would call this function:
>> cfi_use_chip_ready_for_writes()
>>
>> Yet, I still don't fully get what chip_ready is versus chip_good.
>>
>>> +{
>>> +	struct cfi_private *cfi = map->fldrv_priv;
>>> +
>>> +	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>>> +}
>>> +
>>> +static int __xipram chip_good_for_write(struct map_info *map,
>>> +					struct flchip *chip, unsigned long addr,
>>> +					map_word expected)
>>> +{
>>> +	if (cfi_use_chip_ready_for_write(map))
>>> +		return chip_ready(map, chip, addr);
>>
>> If possible and not too invasive I would definitely add a "quirks" flag
>> somewhere instead of this cfi_use_chip_ready_for_write() check.
>>
>> Anyway, I would move this to the chip_good() implementation directly so
>> we partially hide the quirks complexity from the core.
> 
> Yeah, unfortunately this driver does not use quirk flags and tends to
> hide quirks behind bool functions like above
> 
> Regards
> Vignesh
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Thorsten Leemhuis]

On 16.03.22 16:54, Tokunori Ikegami wrote:
> As pointed out by this bug report [1], buffered writes are now broken on
> S29GL064N. This issue comes from a rework which switched from using chip_good()
> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
> returned by chip_good(). One way to solve the issue is to revert the change
> partially to use chip_ready for S29GL064N.
> 
> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/

Why did you switch from the documented format for links you added on my
request (see
https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/

) to v2 to something else that is not recognized by tools and scripts
that rely on proper link tags? You are making my and maybe other peoples
life unnecessary hard. :-((

FWIW, the proper style should support footnote style like this:

Link:
https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
 [1]

Ciao, Thorsten

#regzbot ^backmonitor:
https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Miquel Raynal]

Hi Thorsten,

regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:

> On 16.03.22 16:54, Tokunori Ikegami wrote:
> > As pointed out by this bug report [1], buffered writes are now broken on
> > S29GL064N. This issue comes from a rework which switched from using chip_good()
> > to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
> > returned by chip_good(). One way to solve the issue is to revert the change
> > partially to use chip_ready for S29GL064N.
> > 
> > [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/  
> 
> Why did you switch from the documented format for links you added on my
> request (see
> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
> 
> ) to v2 to something else that is not recognized by tools and scripts
> that rely on proper link tags? You are making my and maybe other peoples
> life unnecessary hard. :-((
> 
> FWIW, the proper style should support footnote style like this:
> 
> Link:
> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>  [1]
> 
> Ciao, Thorsten
> 
> #regzbot ^backmonitor:
> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> 

Because today's requirement from maintainers is to provide a Link
tag that points to the mail discussion of the patch being applied. I
then asked to use the above form instead to point to the bug report
because I don't see the point of having a "Link" tag for it?

Thanks,
Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Thorsten Leemhuis]

On 21.03.22 13:35, Miquel Raynal wrote:
>
> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:
> 
>> On 16.03.22 16:54, Tokunori Ikegami wrote:
>>> As pointed out by this bug report [1], buffered writes are now broken on
>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>> returned by chip_good(). One way to solve the issue is to revert the change
>>> partially to use chip_ready for S29GL064N.
>>>
>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/  
>>
>> Why did you switch from the documented format for links you added on my
>> request (see
>> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
>>
>> ) to v2 to something else that is not recognized by tools and scripts
>> that rely on proper link tags? You are making my and maybe other peoples
>> life unnecessary hard. :-((
>>
>> FWIW, the proper style should support footnote style like this:
>>
>> Link:
>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>  [1]
>>
>> Ciao, Thorsten
>>
>> #regzbot ^backmonitor:
>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>
> 
> Because today's requirement from maintainers is to provide a Link
> tag that points to the mail discussion of the patch being applied.

That can be an additional Link tag, that is done all the time.

> I
> then asked to use the above form instead to point to the bug report
> because I don't see the point of having a "Link" tag for it?

But it's not your own project, we are all working with thousands of
people together on this project on various different fronts. That needs
coordination, as some things otherwise become hard or impossible. That's
why we have documentation that explains how to do some things. Not
following it just because you don't like it is not helpful and in this
case makes my life as a volunteer a lot harder.

If you don't like the approach explained by the documentation, submit a
patch adjusting the documentation and then we can talk about this. But
until that is applied please stick to the format explained by the
documentation.

Ciao, Thorsten

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Miquel Raynal]

Thorsten,

regressions@leemhuis.info wrote on Mon, 21 Mar 2022 13:51:10 +0100:

> On 21.03.22 13:35, Miquel Raynal wrote:
> >
> > regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:
> > 
> >> On 16.03.22 16:54, Tokunori Ikegami wrote:
> >>> As pointed out by this bug report [1], buffered writes are now broken on
> >>> S29GL064N. This issue comes from a rework which switched from using chip_good()
> >>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
> >>> returned by chip_good(). One way to solve the issue is to revert the change
> >>> partially to use chip_ready for S29GL064N.
> >>>
> >>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/  
> >>
> >> Why did you switch from the documented format for links you added on my
> >> request (see
> >> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
> >>
> >> ) to v2 to something else that is not recognized by tools and scripts
> >> that rely on proper link tags? You are making my and maybe other peoples
> >> life unnecessary hard. :-((
> >>
> >> FWIW, the proper style should support footnote style like this:
> >>
> >> Link:
> >> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> >>  [1]
> >>
> >> Ciao, Thorsten
> >>
> >> #regzbot ^backmonitor:
> >> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> >>
> > 
> > Because today's requirement from maintainers is to provide a Link
> > tag that points to the mail discussion of the patch being applied.
> 
> That can be an additional Link tag, that is done all the time.
> 
> > I
> > then asked to use the above form instead to point to the bug report
> > because I don't see the point of having a "Link" tag for it?

Perhaps I should emphasize that I don't remember your initial request
regarding the use of a Link tag and my original idea was to help this
contributor, not kill your tools which I actually know very little
about.

> But it's not your own project, we are all working with thousands of
> people together on this project on various different fronts. That needs
> coordination, as some things otherwise become hard or impossible. That's
> why we have documentation that explains how to do some things. Not
> following it just because you don't like it is not helpful and in this
> case makes my life as a volunteer a lot harder.

Let's be honest, you are referring to a Documentation patch that *you*
wrote and was merged into Linus' tree mid January. How often do you
think people used to the contribution workflow monitor these files?

I am totally fine enforcing the use of Link: tags if this is what has
been decided, just don't expect everybody to switch to a style rather
than another over a night.

> If you don't like the approach explained by the documentation, submit a
> patch adjusting the documentation and then we can talk about this. But
> until that is applied please stick to the format explained by the
> documentation.

This is uselessly condescending.

Thanks,
Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Thorsten Leemhuis]

On 21.03.22 14:41, Miquel Raynal wrote:
> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 13:51:10 +0100:
>> On 21.03.22 13:35, Miquel Raynal wrote:
>>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:
>>>
>>>> On 16.03.22 16:54, Tokunori Ikegami wrote:
>>>>> As pointed out by this bug report [1], buffered writes are now broken on
>>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>>>> returned by chip_good(). One way to solve the issue is to revert the change
>>>>> partially to use chip_ready for S29GL064N.
>>>>>
>>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/  
>>>>
>>>> Why did you switch from the documented format for links you added on my
>>>> request (see
>>>> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
>>>>
>>>> ) to v2 to something else that is not recognized by tools and scripts
>>>> that rely on proper link tags? You are making my and maybe other peoples
>>>> life unnecessary hard. :-((
>>>>
>>>> FWIW, the proper style should support footnote style like this:
>>>>
>>>> Link:
>>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>  [1]
>>>>
>>>> Ciao, Thorsten
>>>>
>>>> #regzbot ^backmonitor:
>>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>
>>>
>>> Because today's requirement from maintainers is to provide a Link
>>> tag that points to the mail discussion of the patch being applied.
>>
>> That can be an additional Link tag, that is done all the time.
>>
>>> I
>>> then asked to use the above form instead to point to the bug report
>>> because I don't see the point of having a "Link" tag for it?
> 
> Perhaps I should emphasize that I don't remember your initial request
> regarding the use of a Link tag

Happen, no worries.

> and my original idea was to help this
> contributor, not kill your tools which I actually know very little
> about.
>>> But it's not your own project, we are all working with thousands of
>> people together on this project on various different fronts. That needs
>> coordination, as some things otherwise become hard or impossible. That's
>> why we have documentation that explains how to do some things. Not
>> following it just because you don't like it is not helpful and in this
>> case makes my life as a volunteer a lot harder.
> 
> Let's be honest, you are referring to a Documentation patch that *you*
> wrote

Correct, but in case of submitting-patches it was just a clarification
how to place links; why the whole aspect was missing in the other is
kinda odd and likely lost in history...

> and was merged into Linus' tree mid January. How often do you
> think people used to the contribution workflow monitor these files?

Not often, that's why I have no problem pointing it out, even if that's
slightly annoying. But you can imagine that it felt kinda odd on my side
when asking someone to set the links (with references to the docs
explaining how to set them) and seeing them added then in v2, just so
see they vanished again in v3 of the same patch. :-/

> I am totally fine enforcing the use of Link: tags if this is what has
> been decided, just don't expect everybody to switch to a style rather
> than another over a night.

I don't.

>> If you don't like the approach explained by the documentation, submit a
>> patch adjusting the documentation and then we can talk about this. But
>> until that is applied please stick to the format explained by the
>> documentation.
> This is uselessly condescending.

I apologize, it wasn't meant that way. I had to many discussions already
where people were not setting any links and it seems the topic is slowly
hitting a nerve here. Sorry.

Ciao, Thorsten

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Miquel Raynal]

Hi Thorsten,

regressions@leemhuis.info wrote on Mon, 21 Mar 2022 15:17:50 +0100:

> On 21.03.22 14:41, Miquel Raynal wrote:
> > regressions@leemhuis.info wrote on Mon, 21 Mar 2022 13:51:10 +0100:  
> >> On 21.03.22 13:35, Miquel Raynal wrote:  
> >>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:
> >>>  
> >>>> On 16.03.22 16:54, Tokunori Ikegami wrote:  
> >>>>> As pointed out by this bug report [1], buffered writes are now broken on
> >>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
> >>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
> >>>>> returned by chip_good(). One way to solve the issue is to revert the change
> >>>>> partially to use chip_ready for S29GL064N.
> >>>>>
> >>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/    
> >>>>
> >>>> Why did you switch from the documented format for links you added on my
> >>>> request (see
> >>>> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
> >>>>
> >>>> ) to v2 to something else that is not recognized by tools and scripts
> >>>> that rely on proper link tags? You are making my and maybe other peoples
> >>>> life unnecessary hard. :-((
> >>>>
> >>>> FWIW, the proper style should support footnote style like this:
> >>>>
> >>>> Link:
> >>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> >>>>  [1]
> >>>>
> >>>> Ciao, Thorsten
> >>>>
> >>>> #regzbot ^backmonitor:
> >>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
> >>>>  
> >>>
> >>> Because today's requirement from maintainers is to provide a Link
> >>> tag that points to the mail discussion of the patch being applied.  
> >>
> >> That can be an additional Link tag, that is done all the time.
> >>  
> >>> I
> >>> then asked to use the above form instead to point to the bug report
> >>> because I don't see the point of having a "Link" tag for it?  
> > 
> > Perhaps I should emphasize that I don't remember your initial request
> > regarding the use of a Link tag  
> 
> Happen, no worries.
> 
> > and my original idea was to help this
> > contributor, not kill your tools which I actually know very little
> > about.  
> >>> But it's not your own project, we are all working with thousands of  
> >> people together on this project on various different fronts. That needs
> >> coordination, as some things otherwise become hard or impossible. That's
> >> why we have documentation that explains how to do some things. Not
> >> following it just because you don't like it is not helpful and in this
> >> case makes my life as a volunteer a lot harder.  
> > 
> > Let's be honest, you are referring to a Documentation patch that *you*
> > wrote  
> 
> Correct, but in case of submitting-patches it was just a clarification
> how to place links; why the whole aspect was missing in the other is
> kinda odd and likely lost in history...
> 
> > and was merged into Linus' tree mid January. How often do you
> > think people used to the contribution workflow monitor these files?  
> 
> Not often, that's why I have no problem pointing it out, even if that's
> slightly annoying. But you can imagine that it felt kinda odd on my side
> when asking someone to set the links (with references to the docs
> explaining how to set them) and seeing them added then in v2, just so
> see they vanished again in v3 of the same patch. :-/

I fully understand. I actually learned that these tags had to be used
for this purpose, so I will actually enforce their use in my next
reviews.

Just a side question, should the Documentation also mention how
to refer to links for people not used to it? Something like
[5.Posting.rst]:

	"Link: <link> [1]
	 Link: <link> [2]"

My original point was that maintainers would almost always add
a Link tag at the end, containing the mailing-list thread about the
patch being applied. Just saying in the commit log "see the link below"
then becomes misleading.

> > I am totally fine enforcing the use of Link: tags if this is what has
> > been decided, just don't expect everybody to switch to a style rather
> > than another over a night.  
> 
> I don't.
> 
> >> If you don't like the approach explained by the documentation, submit a
> >> patch adjusting the documentation and then we can talk about this. But
> >> until that is applied please stick to the format explained by the
> >> documentation.  
> > This is uselessly condescending.  
> 
> I apologize, it wasn't meant that way.

No worries, thanks :-)

> I had to many discussions already
> where people were not setting any links and it seems the topic is slowly
> hitting a nerve here. Sorry.

I also feel like I am repeating myself sometimes. And then I remember
Rob and the ton of e-mails where he has to repeat himself hundreds of
times a day and I feel slightly better :-p

Cheers, Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Thorsten Leemhuis]

On 21.03.22 15:56, Miquel Raynal wrote:
> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 15:17:50 +0100:
>> On 21.03.22 14:41, Miquel Raynal wrote:
>>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 13:51:10 +0100:  
>>>> On 21.03.22 13:35, Miquel Raynal wrote:  
>>>>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:
>>>>>  
>>>>>> On 16.03.22 16:54, Tokunori Ikegami wrote:  
>>>>>>> As pointed out by this bug report [1], buffered writes are now broken on
>>>>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>>>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>>>>>> returned by chip_good(). One way to solve the issue is to revert the change
>>>>>>> partially to use chip_ready for S29GL064N.
>>>>>>>
>>>>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/    
>>>>>>
>>>>>> Why did you switch from the documented format for links you added on my
>>>>>> request (see
>>>>>> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
>>>>>>
>>>>>> ) to v2 to something else that is not recognized by tools and scripts
>>>>>> that rely on proper link tags? You are making my and maybe other peoples
>>>>>> life unnecessary hard. :-((
>>>>>>
>>>>>> FWIW, the proper style should support footnote style like this:
>>>>>>
>>>>>> Link:
>>>>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>>  [1]
>>>>>>
>>>>>> Ciao, Thorsten
>>>>>>
>>>>>> #regzbot ^backmonitor:
>>>>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>>  
>>>>>
>>>>> Because today's requirement from maintainers is to provide a Link
>>>>> tag that points to the mail discussion of the patch being applied.  
>>>>
>>>> That can be an additional Link tag, that is done all the time.
>>>>  
>>>>> I
>>>>> then asked to use the above form instead to point to the bug report
>>>>> because I don't see the point of having a "Link" tag for it?  
>>>
>>> Perhaps I should emphasize that I don't remember your initial request
>>> regarding the use of a Link tag  
>>
>> Happen, no worries.
>>
>>> and my original idea was to help this
>>> contributor, not kill your tools which I actually know very little
>>> about.  
>>>>> But it's not your own project, we are all working with thousands of  
>>>> people together on this project on various different fronts. That needs
>>>> coordination, as some things otherwise become hard or impossible. That's
>>>> why we have documentation that explains how to do some things. Not
>>>> following it just because you don't like it is not helpful and in this
>>>> case makes my life as a volunteer a lot harder.  
>>>
>>> Let's be honest, you are referring to a Documentation patch that *you*
>>> wrote  
>>
>> Correct, but in case of submitting-patches it was just a clarification
>> how to place links; why the whole aspect was missing in the other is
>> kinda odd and likely lost in history...
>>
>>> and was merged into Linus' tree mid January. How often do you
>>> think people used to the contribution workflow monitor these files?  
>>
>> Not often, that's why I have no problem pointing it out, even if that's
>> slightly annoying. But you can imagine that it felt kinda odd on my side
>> when asking someone to set the links (with references to the docs
>> explaining how to set them) and seeing them added then in v2, just so
>> see they vanished again in v3 of the same patch. :-/
> 
> I fully understand. I actually learned that these tags had to be used
> for this purpose, so I will actually enforce their use in my next
> reviews.
> 
> Just a side question, should the Documentation also mention how
> to refer to links for people not used to it? Something like
> [5.Posting.rst]:
> 
> 	"Link: <link> [1]
> 	 Link: <link> [2]"

Maybe. But I think the better approach would be: introduce more specify
tags like "Reported:" (and maybe drop "Reported-by" at the same time?)
or "BugLink" (some people use that already!) would be better -- and then
maybe "Posted:", "Reviewposting", or something like that for the link to
the patch that is being applied; and leave "Link" for the rest. I
proposed that a while ago, but that didn't get any traction.

> My original point was that maintainers would almost always add
> a Link tag at the end, containing the mailing-list thread about the
> patch being applied. Just saying in the commit log "see the link below"
> then becomes misleading.

Maybe, but OTOH that link is normally at the end, which kinda makes it
obvious.

> [...]

Ciao, Thorsten

Re: [PATCH v4 1/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write

[Tokunori Ikegami]

Hi Miquèl-san,

On 2022/03/17 2:15, Miquel Raynal wrote:
> Hi Tokunori,
>
> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:53 +0900:
>
>> This is a preparation patch for the functional change in preparation to a change
>> expected to fix the buffered writes on S29GL064N.
> This is a preparation patch for the S29GL064N buffer writes fix. There
> is no functional change.
Fixed this by the version 5 patch.
>
>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/mtd/chips/cfi_cmdset_0002.c | 77 ++++++++++++-----------------
>>   1 file changed, 32 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>> index a761134fd3be..e68ddf0f7fc0 100644
>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>> @@ -801,22 +801,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
>>   	return NULL;
>>   }
>>   
>> -/*
>> - * Return true if the chip is ready.
>> - *
>> - * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
>> - * non-suspended sector) and is indicated by no toggle bits toggling.
>> - *
>> - * Note that anything more complicated than checking if no bits are toggling
>> - * (including checking DQ5 for an error status) is tricky to get working
>> - * correctly and is therefore not done	(particularly with interleaved chips
>> - * as each chip must be checked independently of the others).
>> - */
>> -static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>> -			       unsigned long addr)
>> +static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>> +			       unsigned long addr, map_word *expected)
>>   {
>>   	struct cfi_private *cfi = map->fldrv_priv;
>> -	map_word d, t;
>> +	map_word oldd, curd;
>> +	int ret;
>>   
>>   	if (cfi_use_status_reg(cfi)) {
>>   		map_word ready = CMD(CFI_SR_DRB);
>> @@ -826,17 +816,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>>   		 */
>>   		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>>   				 cfi->device_type, NULL);
>> -		d = map_read(map, addr);
>> +		curd = map_read(map, addr);
>>   
>> -		return map_word_andequal(map, d, ready, ready);
>> +		return map_word_andequal(map, curd, ready, ready);
> A lot of the diff is just a rename. I am not against a rename if you
> feel it's better, but in this order:
> 1: prepare the fix
> 2: fix
> 3: rename/define id's, whatever
This is also fixed as same.
>
>>   	}
>>   
>> -	d = map_read(map, addr);
>> -	t = map_read(map, addr);
>> +	oldd = map_read(map, addr);
>> +	curd = map_read(map, addr);
>> +
>> +	ret = map_word_equal(map, oldd, curd);
>>   
>> -	return map_word_equal(map, d, t);
>> +	if (!ret || !expected)
>> +		return ret;
>> +
>> +	return map_word_equal(map, curd, *expected);
>>   }
>>   
>> +/*
>> + * Return true if the chip is ready.
>> + *
>> + * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
>> + * non-suspended sector) and is indicated by no toggle bits toggling.
>> + *
>> + * Note that anything more complicated than checking if no bits are toggling
>> + * (including checking DQ5 for an error status) is tricky to get working
>> + * correctly and is therefore not done	(particularly with interleaved chips
>> + * as each chip must be checked independently of the others).
>> + */
>> +#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL)
> I don't see the point of such a define. Just get rid of it.
Yes deleted the macro as changed the name chip_check to chip_ready and 
to use only chip_ready without the macro.
>
>> +
>>   /*
>>    * Return true if the chip is ready and has the correct value.
>>    *
>> @@ -852,32 +860,11 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>>    * as each chip must be checked independently of the others).
>>    *
>>    */
>> -static int __xipram chip_good(struct map_info *map, struct flchip *chip,
>> -			      unsigned long addr, map_word expected)
>> -{
>> -	struct cfi_private *cfi = map->fldrv_priv;
>> -	map_word oldd, curd;
>> -
>> -	if (cfi_use_status_reg(cfi)) {
>> -		map_word ready = CMD(CFI_SR_DRB);
>> -
>> -		/*
>> -		 * For chips that support status register, check device
>> -		 * ready bit
>> -		 */
>> -		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>> -				 cfi->device_type, NULL);
>> -		curd = map_read(map, addr);
>> -
>> -		return map_word_andequal(map, curd, ready, ready);
>> -	}
>> -
>> -	oldd = map_read(map, addr);
>> -	curd = map_read(map, addr);
>> -
>> -	return	map_word_equal(map, oldd, curd) &&
>> -		map_word_equal(map, curd, expected);
>> -}
>> +#define chip_good(map, chip, addr, expected) \
>> +	({ \
>> +		map_word datum = expected; \
>> +		chip_check(map, chip, addr, &datum); \
>> +	})
> What is this for? Same here, I don't see the point. Please get rid of
> all these unnecessary helpers which do nothing, besides complicating
> user's life.

Fixed as same.

Regards,
Ikegami

>
>>   static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
>>   {
>
> Thanks,
> Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

Hi Miquèl-san,

On 2022/03/17 2:21, Miquel Raynal wrote:
> Hi Tokunori,
>
> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
>
>> As pointed out by this bug report [1], buffered writes are now broken on
>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>> returned by chip_good().
> Vignesh, I believe you understand this issue better than I do, can you
> propose an improved commit log?
>
>> One way to solve the issue is to revert the change
>> partially to use chip_ready for S29GL064N.
>>
>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>
>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> Cc: stable@vger.kernel.org
>> ---
>>   drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>   		chip_check(map, chip, addr, &datum); \
>>   	})
>>   
>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
> At the very least I would call this function:
> cfi_use_chip_ready_for_writes()
>
> Yet, I still don't fully get what chip_ready is versus chip_good.
This was deleted as to use the quirks flag instead.
>
>> +{
>> +	struct cfi_private *cfi = map->fldrv_priv;
>> +
>> +	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>> +}
>> +
>> +static int __xipram chip_good_for_write(struct map_info *map,
>> +					struct flchip *chip, unsigned long addr,
>> +					map_word expected)
>> +{
>> +	if (cfi_use_chip_ready_for_write(map))
>> +		return chip_ready(map, chip, addr);
> If possible and not too invasive I would definitely add a "quirks" flag
> somewhere instead of this cfi_use_chip_ready_for_write() check.
Added the quirks flag by the version 5 patch.
>
> Anyway, I would move this to the chip_good() implementation directly so
> we partially hide the quirks complexity from the core.

Yes also added the chip_good to check the quirks flag.

Regards,
Ikegami

>
>> +
>> +	return chip_good(map, chip, addr, expected);
>> +}
>> +
>>   static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
>>   {
>>   	DECLARE_WAITQUEUE(wait, current);
>> @@ -1686,7 +1703,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
>>   		 * "chip_good" to avoid the failure due to scheduling.
>>   		 */
>>   		if (time_after(jiffies, timeo) &&
>> -		    !chip_good(map, chip, adr, datum)) {
>> +		    !chip_good_for_write(map, chip, adr, datum)) {
>>   			xip_enable(map, chip, adr);
>>   			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
>>   			xip_disable(map, chip, adr);
>> @@ -1694,7 +1711,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
>>   			break;
>>   		}
>>   
>> -		if (chip_good(map, chip, adr, datum)) {
>> +		if (chip_good_for_write(map, chip, adr, datum)) {
>>   			if (cfi_check_err_status(map, chip, adr))
>>   				ret = -EIO;
>>   			break;
>> @@ -1966,14 +1983,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map,
>>   		 * "chip_good" to avoid the failure due to scheduling.
>>   		 */
>>   		if (time_after(jiffies, timeo) &&
>> -		    !chip_good(map, chip, adr, datum)) {
>> +		    !chip_good_for_write(map, chip, adr, datum)) {
>>   			pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
>>   			       __func__, adr);
>>   			ret = -EIO;
>>   			break;
>>   		}
>>   
>> -		if (chip_good(map, chip, adr, datum)) {
>> +		if (chip_good_for_write(map, chip, adr, datum)) {
>>   			if (cfi_check_err_status(map, chip, adr))
>>   				ret = -EIO;
>>   			break;
>
> Thanks,
> Miquèl

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

Hi,

On 2022/03/17 19:01, Vignesh Raghavendra wrote:
>
> On 16/03/22 10:51 pm, Miquel Raynal wrote:
>> Hi Tokunori,
>>
>> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
>>
>>> As pointed out by this bug report [1], buffered writes are now broken on
>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>> returned by chip_good().
>> Vignesh, I believe you understand this issue better than I do, can you
>> propose an improved commit log?
> How about:
>
> Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
> check correct value") buffered writes fail on S29GL064N. This is
> because, on S29GL064N, reads return 0xFF at the end of DQ polling for
> write completion, where as, chip_good() check expects actual data
> written to the last location to be returned post DQ polling completion.
> Fix is to revert to using chip_good() for S29GL064N which only checks
> for DQ lines to settle down to determine write completion.

Fixed the commit message as suggested by the version 5 patch.

>>> One way to solve the issue is to revert the change
>>> partially to use chip_ready for S29GL064N.
>>>
>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>
>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>   drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>>   		chip_check(map, chip, addr, &datum); \
>>>   	})
>>>   
>>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
>> At the very least I would call this function:
>> cfi_use_chip_ready_for_writes()
>>
>> Yet, I still don't fully get what chip_ready is versus chip_good.
>>
>>> +{
>>> +	struct cfi_private *cfi = map->fldrv_priv;
>>> +
>>> +	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>>> +}
>>> +
>>> +static int __xipram chip_good_for_write(struct map_info *map,
>>> +					struct flchip *chip, unsigned long addr,
>>> +					map_word expected)
>>> +{
>>> +	if (cfi_use_chip_ready_for_write(map))
>>> +		return chip_ready(map, chip, addr);
>> If possible and not too invasive I would definitely add a "quirks" flag
>> somewhere instead of this cfi_use_chip_ready_for_write() check.
>>
>> Anyway, I would move this to the chip_good() implementation directly so
>> we partially hide the quirks complexity from the core.
> Yeah, unfortunately this driver does not use quirk flags and tends to
> hide quirks behind bool functions like above

Added the quirks flag as mentioned.

Regards,
Ikegami

>
> Regards
> Vignesh

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

Hi Ahmad-san,

On 2022/03/17 23:16, Ahmad Fatoum wrote:
> Hello Vignesh,
>
> On 17.03.22 11:01, Vignesh Raghavendra wrote:
>>
>> On 16/03/22 10:51 pm, Miquel Raynal wrote:
>>> Hi Tokunori,
>>>
>>> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
>>>
>>>> As pointed out by this bug report [1], buffered writes are now broken on
>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>>> returned by chip_good().
>>> Vignesh, I believe you understand this issue better than I do, can you
>>> propose an improved commit log?
>> How about:
>>
>> Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
>> check correct value") buffered writes fail on S29GL064N. This is
>> because, on S29GL064N, reads return 0xFF at the end of DQ polling for
>> write completion, where as, chip_good() check expects actual data
>> written to the last location to be returned post DQ polling completion.
>> Fix is to revert to using chip_good() for S29GL064N which only checks
>> for DQ lines to settle down to determine write completion.
> Message sounds good to me with one remark: The issue is independent of
> whether buffered writes are used or not. It's just because buffered writes
> are the default, that it was broken by dfeae1073583 ("mtd: cfi_cmdset_0002:
> Change write buffer to check correct value"). The word write case was broken
> by 37c673ade35c ("mtd: cfi_cmdset_0002: Use chip_good() to retry in
> do_write_oneword()"), so the commit message should probably reference
> both. as this commit indeed fixes both FORCE_WORD_WRITE == 0 and == 1.

Is this really caused the error on do_write_oneword by the changed?
Actually it was changed to use chip_good instead of chip_ready.
But before the change still do_write_oneword uses both chip_ready and 
chip_good.
So it seems that it is possible to be caused the error before the change 
also.

By the way could you please try to test the version 5 patches again?

Regards,
Ikegami

>
> Thanks,
> Ahmad
>
>
>>>> One way to solve the issue is to revert the change
>>>> partially to use chip_ready for S29GL064N.
>>>>
>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>
>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>> Cc: stable@vger.kernel.org
>>>> ---
>>>>   drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>>>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>>>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>>>   		chip_check(map, chip, addr, &datum); \
>>>>   	})
>>>>   
>>>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
>>> At the very least I would call this function:
>>> cfi_use_chip_ready_for_writes()
>>>
>>> Yet, I still don't fully get what chip_ready is versus chip_good.
>>>
>>>> +{
>>>> +	struct cfi_private *cfi = map->fldrv_priv;
>>>> +
>>>> +	return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>>>> +}
>>>> +
>>>> +static int __xipram chip_good_for_write(struct map_info *map,
>>>> +					struct flchip *chip, unsigned long addr,
>>>> +					map_word expected)
>>>> +{
>>>> +	if (cfi_use_chip_ready_for_write(map))
>>>> +		return chip_ready(map, chip, addr);
>>> If possible and not too invasive I would definitely add a "quirks" flag
>>> somewhere instead of this cfi_use_chip_ready_for_write() check.
>>>
>>> Anyway, I would move this to the chip_good() implementation directly so
>>> we partially hide the quirks complexity from the core.
>> Yeah, unfortunately this driver does not use quirk flags and tends to
>> hide quirks behind bool functions like above
>>
>> Regards
>> Vignesh
>>
>

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

Hi,

On 2022/03/22 0:16, Thorsten Leemhuis wrote:
> On 21.03.22 15:56, Miquel Raynal wrote:
>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 15:17:50 +0100:
>>> On 21.03.22 14:41, Miquel Raynal wrote:
>>>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 13:51:10 +0100:
>>>>> On 21.03.22 13:35, Miquel Raynal wrote:
>>>>>> regressions@leemhuis.info wrote on Mon, 21 Mar 2022 12:48:11 +0100:
>>>>>>   
>>>>>>> On 16.03.22 16:54, Tokunori Ikegami wrote:
>>>>>>>> As pointed out by this bug report [1], buffered writes are now broken on
>>>>>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>>>>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>>>>>>> returned by chip_good(). One way to solve the issue is to revert the change
>>>>>>>> partially to use chip_ready for S29GL064N.
>>>>>>>>
>>>>>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>>> Why did you switch from the documented format for links you added on my
>>>>>>> request (see
>>>>>>> https://lore.kernel.org/stable/f1b44e87-e457-7783-d46e-0d577cea3b72@leemhuis.info/
>>>>>>>
>>>>>>> ) to v2 to something else that is not recognized by tools and scripts
>>>>>>> that rely on proper link tags? You are making my and maybe other peoples
>>>>>>> life unnecessary hard. :-((
>>>>>>>
>>>>>>> FWIW, the proper style should support footnote style like this:
>>>>>>>
>>>>>>> Link:
>>>>>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>>>   [1]
>>>>>>>
>>>>>>> Ciao, Thorsten
>>>>>>>
>>>>>>> #regzbot ^backmonitor:
>>>>>>> https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>>>   
>>>>>> Because today's requirement from maintainers is to provide a Link
>>>>>> tag that points to the mail discussion of the patch being applied.
>>>>> That can be an additional Link tag, that is done all the time.
>>>>>   
>>>>>> I
>>>>>> then asked to use the above form instead to point to the bug report
>>>>>> because I don't see the point of having a "Link" tag for it?
>>>> Perhaps I should emphasize that I don't remember your initial request
>>>> regarding the use of a Link tag
>>> Happen, no worries.
>>>
>>>> and my original idea was to help this
>>>> contributor, not kill your tools which I actually know very little
>>>> about.
>>>>>> But it's not your own project, we are all working with thousands of
>>>>> people together on this project on various different fronts. That needs
>>>>> coordination, as some things otherwise become hard or impossible. That's
>>>>> why we have documentation that explains how to do some things. Not
>>>>> following it just because you don't like it is not helpful and in this
>>>>> case makes my life as a volunteer a lot harder.
>>>> Let's be honest, you are referring to a Documentation patch that *you*
>>>> wrote
>>> Correct, but in case of submitting-patches it was just a clarification
>>> how to place links; why the whole aspect was missing in the other is
>>> kinda odd and likely lost in history...
>>>
>>>> and was merged into Linus' tree mid January. How often do you
>>>> think people used to the contribution workflow monitor these files?
>>> Not often, that's why I have no problem pointing it out, even if that's
>>> slightly annoying. But you can imagine that it felt kinda odd on my side
>>> when asking someone to set the links (with references to the docs
>>> explaining how to set them) and seeing them added then in v2, just so
>>> see they vanished again in v3 of the same patch. :-/
>> I fully understand. I actually learned that these tags had to be used
>> for this purpose, so I will actually enforce their use in my next
>> reviews.
>>
>> Just a side question, should the Documentation also mention how
>> to refer to links for people not used to it? Something like
>> [5.Posting.rst]:
>>
>> 	"Link: <link> [1]
>> 	 Link: <link> [2]"
> Maybe. But I think the better approach would be: introduce more specify
> tags like "Reported:" (and maybe drop "Reported-by" at the same time?)
> or "BugLink" (some people use that already!) would be better -- and then
> maybe "Posted:", "Reviewposting", or something like that for the link to
> the patch that is being applied; and leave "Link" for the rest. I
> proposed that a while ago, but that didn't get any traction.

Fixed to use Link tag as before by the version 5 patches instead of [1].

Regards,
Ikegami

>
>> My original point was that maintainers would almost always add
>> a Link tag at the end, containing the mailing-list thread about the
>> patch being applied. Just saying in the commit log "see the link below"
>> then becomes misleading.
> Maybe, but OTOH that link is normally at the end, which kinda makes it
> obvious.
>
>> [...]
> Ciao, Thorsten

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Ahmad Fatoum]

On 22.03.22 03:49, Tokunori Ikegami wrote:
> Hi Ahmad-san,
> 
> On 2022/03/17 23:16, Ahmad Fatoum wrote:
>> Hello Vignesh,
>>
>> On 17.03.22 11:01, Vignesh Raghavendra wrote:
>>>
>>> On 16/03/22 10:51 pm, Miquel Raynal wrote:
>>>> Hi Tokunori,
>>>>
>>>> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
>>>>
>>>>> As pointed out by this bug report [1], buffered writes are now broken on
>>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>>>> returned by chip_good().
>>>> Vignesh, I believe you understand this issue better than I do, can you
>>>> propose an improved commit log?
>>> How about:
>>>
>>> Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
>>> check correct value") buffered writes fail on S29GL064N. This is
>>> because, on S29GL064N, reads return 0xFF at the end of DQ polling for
>>> write completion, where as, chip_good() check expects actual data
>>> written to the last location to be returned post DQ polling completion.
>>> Fix is to revert to using chip_good() for S29GL064N which only checks
>>> for DQ lines to settle down to determine write completion.
>> Message sounds good to me with one remark: The issue is independent of
>> whether buffered writes are used or not. It's just because buffered writes
>> are the default, that it was broken by dfeae1073583 ("mtd: cfi_cmdset_0002:
>> Change write buffer to check correct value"). The word write case was broken
>> by 37c673ade35c ("mtd: cfi_cmdset_0002: Use chip_good() to retry in
>> do_write_oneword()"), so the commit message should probably reference
>> both. as this commit indeed fixes both FORCE_WORD_WRITE == 0 and == 1.
> 
> Is this really caused the error on do_write_oneword by the changed?
> Actually it was changed to use chip_good instead of chip_ready.
> But before the change still do_write_oneword uses both chip_ready and chip_good.
> So it seems that it is possible to be caused the error before the change also.

Oh, I think you're right. Disregard my suggestion for the other
Fixes: entry then.

> By the way could you please try to test the version 5 patches again?

Just did so for v7. Sorry for the delay.

Cheers,
Ahmad

> 
> Regards,
> Ikegami
> 
>>
>> Thanks,
>> Ahmad
>>
>>
>>>>> One way to solve the issue is to revert the change
>>>>> partially to use chip_ready for S29GL064N.
>>>>>
>>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>
>>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>>>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>>>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>>> Cc: stable@vger.kernel.org
>>>>> ---
>>>>>   drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>>>>   1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>>>>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>>>>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>>>>           chip_check(map, chip, addr, &datum); \
>>>>>       })
>>>>>   +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
>>>> At the very least I would call this function:
>>>> cfi_use_chip_ready_for_writes()
>>>>
>>>> Yet, I still don't fully get what chip_ready is versus chip_good.
>>>>
>>>>> +{
>>>>> +    struct cfi_private *cfi = map->fldrv_priv;
>>>>> +
>>>>> +    return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>>>>> +}
>>>>> +
>>>>> +static int __xipram chip_good_for_write(struct map_info *map,
>>>>> +                    struct flchip *chip, unsigned long addr,
>>>>> +                    map_word expected)
>>>>> +{
>>>>> +    if (cfi_use_chip_ready_for_write(map))
>>>>> +        return chip_ready(map, chip, addr);
>>>> If possible and not too invasive I would definitely add a "quirks" flag
>>>> somewhere instead of this cfi_use_chip_ready_for_write() check.
>>>>
>>>> Anyway, I would move this to the chip_good() implementation directly so
>>>> we partially hide the quirks complexity from the core.
>>> Yeah, unfortunately this driver does not use quirk flags and tends to
>>> hide quirks behind bool functions like above
>>>
>>> Regards
>>> Vignesh
>>>
>>
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami]

Hi Ahmad-san,

On 2022/03/28 19:49, Ahmad Fatoum wrote:
> On 22.03.22 03:49, Tokunori Ikegami wrote:
>> Hi Ahmad-san,
>>
>> On 2022/03/17 23:16, Ahmad Fatoum wrote:
>>> Hello Vignesh,
>>>
>>> On 17.03.22 11:01, Vignesh Raghavendra wrote:
>>>> On 16/03/22 10:51 pm, Miquel Raynal wrote:
>>>>> Hi Tokunori,
>>>>>
>>>>> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900:
>>>>>
>>>>>> As pointed out by this bug report [1], buffered writes are now broken on
>>>>>> S29GL064N. This issue comes from a rework which switched from using chip_good()
>>>>>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error
>>>>>> returned by chip_good().
>>>>> Vignesh, I believe you understand this issue better than I do, can you
>>>>> propose an improved commit log?
>>>> How about:
>>>>
>>>> Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to
>>>> check correct value") buffered writes fail on S29GL064N. This is
>>>> because, on S29GL064N, reads return 0xFF at the end of DQ polling for
>>>> write completion, where as, chip_good() check expects actual data
>>>> written to the last location to be returned post DQ polling completion.
>>>> Fix is to revert to using chip_good() for S29GL064N which only checks
>>>> for DQ lines to settle down to determine write completion.
>>> Message sounds good to me with one remark: The issue is independent of
>>> whether buffered writes are used or not. It's just because buffered writes
>>> are the default, that it was broken by dfeae1073583 ("mtd: cfi_cmdset_0002:
>>> Change write buffer to check correct value"). The word write case was broken
>>> by 37c673ade35c ("mtd: cfi_cmdset_0002: Use chip_good() to retry in
>>> do_write_oneword()"), so the commit message should probably reference
>>> both. as this commit indeed fixes both FORCE_WORD_WRITE == 0 and == 1.
>> Is this really caused the error on do_write_oneword by the changed?
>> Actually it was changed to use chip_good instead of chip_ready.
>> But before the change still do_write_oneword uses both chip_ready and chip_good.
>> So it seems that it is possible to be caused the error before the change also.
> Oh, I think you're right. Disregard my suggestion for the other
> Fixes: entry then.
Thanks for the confirmation.
>
>> By the way could you please try to test the version 5 patches again?
> Just did so for v7. Sorry for the delay.

Thank you so much for your help. Sorry I missed to cc on version 5 to 
version 7 patches.

Regards,
Ikegami

>
> Cheers,
> Ahmad
>
>> Regards,
>> Ikegami
>>
>>> Thanks,
>>> Ahmad
>>>
>>>
>>>>>> One way to solve the issue is to revert the change
>>>>>> partially to use chip_ready for S29GL064N.
>>>>>>
>>>>>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/
>>>>>>
>>>>>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
>>>>>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com>
>>>>>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>>>> Cc: stable@vger.kernel.org
>>>>>> ---
>>>>>>    drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++----
>>>>>>    1 file changed, 21 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
>>>>>> index e68ddf0f7fc0..6c57f85e1b8e 100644
>>>>>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
>>>>>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
>>>>>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip,
>>>>>>            chip_check(map, chip, addr, &datum); \
>>>>>>        })
>>>>>>    +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map)
>>>>> At the very least I would call this function:
>>>>> cfi_use_chip_ready_for_writes()
>>>>>
>>>>> Yet, I still don't fully get what chip_ready is versus chip_good.
>>>>>
>>>>>> +{
>>>>>> +    struct cfi_private *cfi = map->fldrv_priv;
>>>>>> +
>>>>>> +    return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01;
>>>>>> +}
>>>>>> +
>>>>>> +static int __xipram chip_good_for_write(struct map_info *map,
>>>>>> +                    struct flchip *chip, unsigned long addr,
>>>>>> +                    map_word expected)
>>>>>> +{
>>>>>> +    if (cfi_use_chip_ready_for_write(map))
>>>>>> +        return chip_ready(map, chip, addr);
>>>>> If possible and not too invasive I would definitely add a "quirks" flag
>>>>> somewhere instead of this cfi_use_chip_ready_for_write() check.
>>>>>
>>>>> Anyway, I would move this to the chip_good() implementation directly so
>>>>> we partially hide the quirks complexity from the core.
>>>> Yeah, unfortunately this driver does not use quirk flags and tends to
>>>> hide quirks behind bool functions like above
>>>>
>>>> Regards
>>>> Vignesh
>>>>
>