[PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[Arseniy Krasnov]

Valid mask is 0x3FFF, without this patch the following problems were
found:

1) [    0.938914] Could not find a valid ONFI parameter page, trying
                  bit-wise majority to recover it
   [    0.947384] ONFI parameter recovery failed, aborting

2) Read with disabled ECC mode was broken.

Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
Cc: <Stable@vger.kernel.org>
Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
---
 drivers/mtd/nand/raw/meson_nand.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
index a28574c00900..074e14225c06 100644
--- a/drivers/mtd/nand/raw/meson_nand.c
+++ b/drivers/mtd/nand/raw/meson_nand.c
@@ -280,7 +280,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
 
 	if (raw) {
 		len = mtd->writesize + mtd->oobsize;
-		cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir);
+		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);
 		writel(cmd, nfc->reg_base + NFC_REG_CMD);
 		return;
 	}
@@ -544,7 +544,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
 	if (ret)
 		goto out;
 
-	cmd = NFC_CMD_N2M | (len & GENMASK(5, 0));
+	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
 	meson_nfc_drain_cmd(nfc);
@@ -568,7 +568,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
 	if (ret)
 		return ret;
 
-	cmd = NFC_CMD_M2N | (len & GENMASK(5, 0));
+	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
 	writel(cmd, nfc->reg_base + NFC_REG_CMD);
 
 	meson_nfc_drain_cmd(nfc);
-- 
2.35.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[Tudor Ambarus]

On 3/29/23 08:47, Arseniy Krasnov wrote:
> Valid mask is 0x3FFF, without this patch the following problems were
> found:
> 
> 1) [    0.938914] Could not find a valid ONFI parameter page, trying
>                   bit-wise majority to recover it
>    [    0.947384] ONFI parameter recovery failed, aborting
> 
> 2) Read with disabled ECC mode was broken.
> 
> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
> Cc: <Stable@vger.kernel.org>
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> ---
>  drivers/mtd/nand/raw/meson_nand.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> index a28574c00900..074e14225c06 100644
> --- a/drivers/mtd/nand/raw/meson_nand.c
> +++ b/drivers/mtd/nand/raw/meson_nand.c
> @@ -280,7 +280,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
>  
>  	if (raw) {
>  		len = mtd->writesize + mtd->oobsize;
> -		cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir);
> +		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);

What happens when len > GENMASK(13, 0)? Do you check this somewhere?

Please introduce a macro/field for GENMASK(13, 0), having such mask
scattered along the code looks hackish and doesn't help readability.
You'll get to use FIELD_PREP as well.

>  		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  		return;
>  	}
> @@ -544,7 +544,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
>  	if (ret)
>  		goto out;
>  
> -	cmd = NFC_CMD_N2M | (len & GENMASK(5, 0));
> +	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  
>  	meson_nfc_drain_cmd(nfc);
> @@ -568,7 +568,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
>  	if (ret)
>  		return ret;
>  
> -	cmd = NFC_CMD_M2N | (len & GENMASK(5, 0));
> +	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>  
>  	meson_nfc_drain_cmd(nfc);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[Arseniy Krasnov]

On 29.03.2023 11:09, Tudor Ambarus wrote:
> 
> 
> On 3/29/23 08:47, Arseniy Krasnov wrote:
>> Valid mask is 0x3FFF, without this patch the following problems were
>> found:
>>
>> 1) [    0.938914] Could not find a valid ONFI parameter page, trying
>>                   bit-wise majority to recover it
>>    [    0.947384] ONFI parameter recovery failed, aborting
>>
>> 2) Read with disabled ECC mode was broken.
>>
>> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
>> Cc: <Stable@vger.kernel.org>
>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>> ---
>>  drivers/mtd/nand/raw/meson_nand.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>> index a28574c00900..074e14225c06 100644
>> --- a/drivers/mtd/nand/raw/meson_nand.c
>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>> @@ -280,7 +280,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
>>  
>>  	if (raw) {
>>  		len = mtd->writesize + mtd->oobsize;
>> -		cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir);
>> +		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);
> 
> What happens when len > GENMASK(13, 0)? Do you check this somewhere?

'len' will be trimmed. I'm not sure that this case is possible here, because GENMASK(13, 0)
is hardware limit for this NAND controller, so 'writesize' and 'oobsize' will be initialized
to fit this value. Moreover GENMASK(13, 0) is 16Kb - i think it is big enough for single
read. Also i'm not sure that it is good approach to check 'len' here - we are in the middle
of NAND read processing.

> 
> Please introduce a macro/field for GENMASK(13, 0), having such mask
> scattered along the code looks hackish and doesn't help readability.
> You'll get to use FIELD_PREP as well.

Ack, i'll do it in v3

Thanks, Arseniy

> 
>>  		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>  		return;
>>  	}
>> @@ -544,7 +544,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
>>  	if (ret)
>>  		goto out;
>>  
>> -	cmd = NFC_CMD_N2M | (len & GENMASK(5, 0));
>> +	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>  
>>  	meson_nfc_drain_cmd(nfc);
>> @@ -568,7 +568,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
>>  	if (ret)
>>  		return ret;
>>  
>> -	cmd = NFC_CMD_M2N | (len & GENMASK(5, 0));
>> +	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>  
>>  	meson_nfc_drain_cmd(nfc);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[Miquel Raynal]

Hi Arseniy,

avkrasnov@sberdevices.ru wrote on Wed, 29 Mar 2023 11:33:38 +0300:

> On 29.03.2023 11:09, Tudor Ambarus wrote:
> > 
> > 
> > On 3/29/23 08:47, Arseniy Krasnov wrote:  
> >> Valid mask is 0x3FFF, without this patch the following problems were
> >> found:
> >>
> >> 1) [    0.938914] Could not find a valid ONFI parameter page, trying
> >>                   bit-wise majority to recover it
> >>    [    0.947384] ONFI parameter recovery failed, aborting
> >>
> >> 2) Read with disabled ECC mode was broken.
> >>
> >> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
> >> Cc: <Stable@vger.kernel.org>
> >> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
> >> ---
> >>  drivers/mtd/nand/raw/meson_nand.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >> index a28574c00900..074e14225c06 100644
> >> --- a/drivers/mtd/nand/raw/meson_nand.c
> >> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >> @@ -280,7 +280,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
> >>  
> >>  	if (raw) {
> >>  		len = mtd->writesize + mtd->oobsize;
> >> -		cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir);
> >> +		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);  
> > 
> > What happens when len > GENMASK(13, 0)? Do you check this somewhere?  
> 
> 'len' will be trimmed. I'm not sure that this case is possible here, because GENMASK(13, 0)
> is hardware limit for this NAND controller, so 'writesize' and 'oobsize' will be initialized
> to fit this value. Moreover GENMASK(13, 0) is 16Kb - i think it is big enough for single
> read. Also i'm not sure that it is good approach to check 'len' here - we are in the middle
> of NAND read processing.

No, you should check the page size will not exceed this limit in the
attach hook, likely.

You should also refuse exec_op operations with a data length bigger
than 16k (either with a manual check in your own parser or just by
providing the max size to the parser table, depending on what's used).

> 
> > 
> > Please introduce a macro/field for GENMASK(13, 0), having such mask
> > scattered along the code looks hackish and doesn't help readability.
> > You'll get to use FIELD_PREP as well.  
> 
> Ack, i'll do it in v3
> 
> Thanks, Arseniy
> 
> >   
> >>  		writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>  		return;
> >>  	}
> >> @@ -544,7 +544,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
> >>  	if (ret)
> >>  		goto out;
> >>  
> >> -	cmd = NFC_CMD_N2M | (len & GENMASK(5, 0));
> >> +	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
> >>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>  
> >>  	meson_nfc_drain_cmd(nfc);
> >> @@ -568,7 +568,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	cmd = NFC_CMD_M2N | (len & GENMASK(5, 0));
> >> +	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
> >>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
> >>  
> >>  	meson_nfc_drain_cmd(nfc);  


Thanks,
Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[Arseniy Krasnov]

On 29.03.2023 12:00, Miquel Raynal wrote:
> Hi Arseniy,
> 
> avkrasnov@sberdevices.ru wrote on Wed, 29 Mar 2023 11:33:38 +0300:
> 
>> On 29.03.2023 11:09, Tudor Ambarus wrote:
>>>
>>>
>>> On 3/29/23 08:47, Arseniy Krasnov wrote:  
>>>> Valid mask is 0x3FFF, without this patch the following problems were
>>>> found:
>>>>
>>>> 1) [    0.938914] Could not find a valid ONFI parameter page, trying
>>>>                   bit-wise majority to recover it
>>>>    [    0.947384] ONFI parameter recovery failed, aborting
>>>>
>>>> 2) Read with disabled ECC mode was broken.
>>>>
>>>> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
>>>> Cc: <Stable@vger.kernel.org>
>>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>
>>>> ---
>>>>  drivers/mtd/nand/raw/meson_nand.c | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>> index a28574c00900..074e14225c06 100644
>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>> @@ -280,7 +280,7 @@ static void meson_nfc_cmd_access(struct nand_chip *nand, int raw, bool dir,
>>>>  
>>>>  	if (raw) {
>>>>  		len = mtd->writesize + mtd->oobsize;
>>>> -		cmd = (len & GENMASK(5, 0)) | scrambler | DMA_DIR(dir);
>>>> +		cmd = (len & GENMASK(13, 0)) | scrambler | DMA_DIR(dir);  
>>>
>>> What happens when len > GENMASK(13, 0)? Do you check this somewhere?  
>>
>> 'len' will be trimmed. I'm not sure that this case is possible here, because GENMASK(13, 0)
>> is hardware limit for this NAND controller, so 'writesize' and 'oobsize' will be initialized
>> to fit this value. Moreover GENMASK(13, 0) is 16Kb - i think it is big enough for single
>> read. Also i'm not sure that it is good approach to check 'len' here - we are in the middle
>> of NAND read processing.
> 
> No, you should check the page size will not exceed this limit in the
> attach hook, likely.
> 
> You should also refuse exec_op operations with a data length bigger
> than 16k (either with a manual check in your own parser or just by
> providing the max size to the parser table, depending on what's used).
>

 
Ok, I see.

Thanks, Arseniy

>>
>>>
>>> Please introduce a macro/field for GENMASK(13, 0), having such mask
>>> scattered along the code looks hackish and doesn't help readability.
>>> You'll get to use FIELD_PREP as well.  
>>
>> Ack, i'll do it in v3
>>
>> Thanks, Arseniy
>>
>>>   
>>>>  		writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>  		return;
>>>>  	}
>>>> @@ -544,7 +544,7 @@ static int meson_nfc_read_buf(struct nand_chip *nand, u8 *buf, int len)
>>>>  	if (ret)
>>>>  		goto out;
>>>>  
>>>> -	cmd = NFC_CMD_N2M | (len & GENMASK(5, 0));
>>>> +	cmd = NFC_CMD_N2M | (len & GENMASK(13, 0));
>>>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>  
>>>>  	meson_nfc_drain_cmd(nfc);
>>>> @@ -568,7 +568,7 @@ static int meson_nfc_write_buf(struct nand_chip *nand, u8 *buf, int len)
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	cmd = NFC_CMD_M2N | (len & GENMASK(5, 0));
>>>> +	cmd = NFC_CMD_M2N | (len & GENMASK(13, 0));
>>>>  	writel(cmd, nfc->reg_base + NFC_REG_CMD);
>>>>  
>>>>  	meson_nfc_drain_cmd(nfc);  
> 
> 
> Thanks,
> Miquèl

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] mtd: rawnand: meson: fix bitmask for length in command word

[Miquel Raynal]

On Wed, 2023-03-29 at 07:47:26 UTC, Arseniy Krasnov wrote:
> Valid mask is 0x3FFF, without this patch the following problems were
> found:
> 
> 1) [    0.938914] Could not find a valid ONFI parameter page, trying
>                   bit-wise majority to recover it
>    [    0.947384] ONFI parameter recovery failed, aborting
> 
> 2) Read with disabled ECC mode was broken.
> 
> Fixes: 8fae856c5350 ("mtd: rawnand: meson: add support for Amlogic NAND flash controller")
> Cc: <Stable@vger.kernel.org>
> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/fixes, thanks.

Miquel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel