[PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[DongCV]

From: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>

This patch fixes the output warning logs and data loss when
performing mount/umount then remount the device with jffs2 format.

This is the warning logs when performing mount/umount then remount the device with jffs2 format:
"root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
[ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
[ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
[ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
[ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
[ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
[ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
[ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
[ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
[ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
[ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
[ 3840.212900] jffs2: Further such events for this erase block will not be printed
[ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
:~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...

Signed-off-by: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
---
 drivers/spi/spi-rspi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 9daf500..2ee1301 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
 			ret = rspi_pio_transfer(rspi, NULL, rx, n);
 			if (ret < 0)
 				return ret;
-			*rx++ = ret;
 		}
 		n -= len;
 	}
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Geert Uytterhoeven]

Hi Dong,

On Mon, Feb 6, 2017 at 2:02 AM, DongCV <cv-dong@jinso.co.jp> wrote:
> From: Dong <cv-dong@jinso.co.jp>
>
> This patch fixes the output warning logs and data loss when
> performing mount/umount then remount the device with jffs2 format.
>
> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
> [ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
> [ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
> [ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
> [ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
> [ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
> [ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
> [ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
> [ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
> [ 3840.212900] jffs2: Further such events for this erase block will not be printed
> [ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
> root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
> :~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...
>
> Signed-off-by: Dong <cv-dong@jinso.co.jp>
> ---
>  drivers/spi/spi-rspi.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 9daf500..2ee1301 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
>                         ret = rspi_pio_transfer(rspi, NULL, rx, n);
>                         if (ret < 0)
>                                 return ret;
> -                       *rx++ = ret;
>                 }
>                 n -= len;
>         }

Apart from sending patches inline, my comments from
https://www.spinics.net/lists/linux-spi/msg09753.html are still valid.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[DongCV]

Dear Geert,

Thank you for your reply.

>> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
>> index 9daf500..2ee1301 100644
>> --- a/drivers/spi/spi-rspi.c
>> +++ b/drivers/spi/spi-rspi.c
>> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
>>                        ret = rspi_pio_transfer(rspi, NULL, rx, n);
>>                        if (ret < 0)
>>                                return ret;
>> -                     *rx++ = ret;
> Storing the success code (0) in the receive buffer is indeed wrong.
>
> However, there are other bugs in that code:
>
> rspi_pio_transfer(rspi, NULL, rx, n) transfers n bytes instead of len,
> while n is decreased by len (which is <= n).
> Furthermore rx is not incremented.
> Hence if len < n, n will still be non-zero, and a new iteration of the
> loop will be started, trying to receive more data, and overwriting the
> just filled buffer.
>
> The same bug is present in qspi_transfer_out().
>
>>                }
>>                n -= len;
>>        }
>> --
>> 1.9.1
>>
(nip)
> Apart from sending patches inline, my comments from
> https://www.spinics.net/lists/linux-spi/msg09753.html are still valid.

Sorry I might not understand your explanation correctly. Could you 
please explain it more details?
(Because I've tried to understand your explanation then analyzed the 
source code again as below:
https://patchwork.kernel.org/patch/9541629/)

Thank you.
Dong



On 02/06/2017 08:02 PM, Geert Uytterhoeven wrote:
> Hi Dong,
>
> On Mon, Feb 6, 2017 at 2:02 AM, DongCV <cv-dong@jinso.co.jp> wrote:
>> From: Dong <cv-dong@jinso.co.jp>
>>
>> This patch fixes the output warning logs and data loss when
>> performing mount/umount then remount the device with jffs2 format.
>>
>> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
>> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
>> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
>> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
>> [ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
>> [ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
>> [ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
>> [ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
>> [ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
>> [ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
>> [ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
>> [ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
>> [ 3840.212900] jffs2: Further such events for this erase block will not be printed
>> [ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
>> root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
>> :~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...
>>
>> Signed-off-by: Dong <cv-dong@jinso.co.jp>
>> ---
>>   drivers/spi/spi-rspi.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
>> index 9daf500..2ee1301 100644
>> --- a/drivers/spi/spi-rspi.c
>> +++ b/drivers/spi/spi-rspi.c
>> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
>>                          ret = rspi_pio_transfer(rspi, NULL, rx, n);
>>                          if (ret < 0)
>>                                  return ret;
>> -                       *rx++ = ret;
>>                  }
>>                  n -= len;
>>          }
> Apart from sending patches inline, my comments from
> https://www.spinics.net/lists/linux-spi/msg09753.html are still valid.
>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
>

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Geert Uytterhoeven]

Hi Dong,

On Tue, Feb 7, 2017 at 11:25 AM, DongCV <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org> wrote:
>>> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
>>> index 9daf500..2ee1301 100644
>>> --- a/drivers/spi/spi-rspi.c
>>> +++ b/drivers/spi/spi-rspi.c
>>> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi,
>>> struct spi_transfer *xfer)
>>>                        ret = rspi_pio_transfer(rspi, NULL, rx, n);
>>>                        if (ret < 0)
>>>                                return ret;
>>> -                     *rx++ = ret;
>>
>> Storing the success code (0) in the receive buffer is indeed wrong.
>>
>> However, there are other bugs in that code:
>>
>> rspi_pio_transfer(rspi, NULL, rx, n) transfers n bytes instead of len,
>> while n is decreased by len (which is <= n).
>> Furthermore rx is not incremented.
>> Hence if len < n, n will still be non-zero, and a new iteration of the
>> loop will be started, trying to receive more data, and overwriting the
>> just filled buffer.
>>
>> The same bug is present in qspi_transfer_out().
>>
>>>                }
>>>                n -= len;
>>>        }
>>> --
>>> 1.9.1
>>>
> (nip)
>>
>> Apart from sending patches inline, my comments from
>> https://www.spinics.net/lists/linux-spi/msg09753.html are still valid.
>
>
> Sorry I might not understand your explanation correctly. Could you please
> explain it more details?
> (Because I've tried to understand your explanation then analyzed the source
> code again as below:
> https://patchwork.kernel.org/patch/9541629/)

qspi_transfer_in() does:

        while (n > 0) {
                len = qspi_set_receive_trigger(rspi, n);
                // len will be <= n
                if (len == QSPI_BUFFER_SIZE) {
                        // receive blocks of len bytes
                        ...
                } else {
                        // receive n (not len) bytes
                        ret = rspi_pio_transfer(rspi, NULL, rx, n);
                        //
                        if (ret < 0)
                                return ret;
                        // bogus write (which your patch removes: OK)
                        *rx++ = ret;
                        // here we should also return (see below why)
                        // (in qspi_transfer_out() we should "break")
                }
                // Either we received a block of len bytes
                // or we received n bytes, and the below is wrong if len < n!
                n -= len;
                // If len was < n, n will be non-zero, and we will receive more
                // bytes in the next iteration
        }

        return 0;
}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Hiep Cao Minh]

Hi Geert,

Sorry to bother you!
> qspi_transfer_in() does:
>
>          while (n > 0) {
>                  len = qspi_set_receive_trigger(rspi, n);
>                  // len will be <= n
I agree. This is len = min value of n and 32.
>                  if (len == QSPI_BUFFER_SIZE) {
>                          // receive blocks of len bytes
Yes, This is len == 32 bytes
>                          ...
>                  } else {
>                          // receive n (not len) bytes
This is always n == len ( This case, len or n is always < 32)
Because this is the last n bytes should be received.

>                          ret = rspi_pio_transfer(rspi, NULL, rx, n);
>                          //
>                          if (ret < 0)
>                                  return ret;
>                          // bogus write (which your patch removes: OK)
>                          *rx++ = ret;
I agree. This code needs to be removed.

>                          // here we should also return (see below why)
>                          // (in qspi_transfer_out() we should "break")
>                  }
>                  // Either we received a block of len bytes
>                  // or we received n bytes, and the below is wrong if len < n!
>                  n -= len;
>                  // If len was < n, n will be non-zero, and we will receive more
>                  // bytes in the next iteration
I am sorry, I don't understand your opinion here also.
The following is my opinion:

In case of receiving n bytes data > 32 bytes (Ex: n= 50bytes)
The first loop, n= 50,and  len = 32 bytes by getting min value of 50 and 
32 from qspi_set_receive_trigger()
(this case was n < len).
Then it receives 32 bytes in "if (len == QSPI_BUFFER_SIZE)" statement.
After received 32bytes of data, n -= len is implemented. It means n =(n 
- len) = (50-32)= 18 bytes.
The first loop finished.

The second loop, n=18 bytes, and len = 18 bytes by getting min value of 
32 and 18 from qspi_set_receive_trigger().
This time, 'else' statement would be implemented. *rx pointer was 
increased into rspi_pio_transfer()
After received the last 18 bytes of data into 'else' statement, n -=len 
was implemented again.
It means n = (n - len) = (18 -18) = 0 byte.
The second loop finished.

This time n = 0, Completed receiving data.

In case of receiving n bytes data < 32 bytes  (Ex: n= 20 bytes).
It's the same with the second loop above.

Thank you.
Hiep.

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Geert Uytterhoeven]

Hi Hiep,

On Mon, Feb 13, 2017 at 7:50 AM, Hiep Cao Minh <cm-hiep-HEF513clHfp3+QwDJ9on6Q@public.gmane.org> wrote:
> Sorry to bother you!

No, thank you for finally make me see my wrong reasoning!

>> qspi_transfer_in() does:
>>
>>          while (n > 0) {
>>                  len = qspi_set_receive_trigger(rspi, n);
>>                  // len will be <= n
>
> I agree. This is len = min value of n and 32.
>>
>>                  if (len == QSPI_BUFFER_SIZE) {
>>                          // receive blocks of len bytes
>
> Yes, This is len == 32 bytes
>>
>>                          ...
>>                  } else {
>>                          // receive n (not len) bytes
>
> This is always n == len ( This case, len or n is always < 32)
> Because this is the last n bytes should be received.

Right.
Doh, I must have missed that if "len < n", the first branch is always taken.

>>                          ret = rspi_pio_transfer(rspi, NULL, rx, n);

It would have been clearer to use "len" here instead of "n".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Hiep Cao Minh]

Hi Geert,

Sorry for the delay.

Thanks for your reply.


> On Mon, Feb 13, 2017 at 7:50 AM, Hiep Cao Minh <cm-hiep@jinso.co.jp> wrote:
>> Sorry to bother you!
> No, thank you for finally make me see my wrong reasoning!
>
>>> qspi_transfer_in() does:
>>>
>>>           while (n > 0) {
>>>                   len = qspi_set_receive_trigger(rspi, n);
>>>                   // len will be <= n
>> I agree. This is len = min value of n and 32.
>>>                   if (len == QSPI_BUFFER_SIZE) {
>>>                           // receive blocks of len bytes
>> Yes, This is len == 32 bytes
>>>                           ...
>>>                   } else {
>>>                           // receive n (not len) bytes
>> This is always n == len ( This case, len or n is always < 32)
>> Because this is the last n bytes should be received.
> Right.
> Doh, I must have missed that if "len < n", the first branch is always taken.
>
>>>                           ret = rspi_pio_transfer(rspi, NULL, rx, n);
> It would have been clearer to use "len" here instead of "n".
I agree, Dong will update your suggestion at the patch.

Thank you.
Hiep.

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On Mon, Feb 06, 2017 at 10:02:13AM +0900, DongCV wrote:
> From: Dong <cv-dong@jinso.co.jp>
> 
> This patch fixes the output warning logs and data loss when
> performing mount/umount then remount the device with jffs2 format.

This is not a good changelog since it does not describe what the problem
with the driver is or how the change fixes it - it just says that there
is a problem.

> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead

Please think hard before including long log messages in upstream
reports, they often contain almost no useful information relative to
their size so often obscure the relevant content in your message.  Some
subset is usually much better (eg, "produces lots of errors like X"
here).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Geert Uytterhoeven]

On Mon, Feb 6, 2017 at 1:46 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Feb 06, 2017 at 10:02:13AM +0900, DongCV wrote:
>> From: Dong <cv-dong@jinso.co.jp>
>>
>> This patch fixes the output warning logs and data loss when
>> performing mount/umount then remount the device with jffs2 format.
>
> This is not a good changelog since it does not describe what the problem
> with the driver is or how the change fixes it - it just says that there
> is a problem.
>
>> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
>> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
>> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
>> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
>
> Please think hard before including long log messages in upstream
> reports, they often contain almost no useful information relative to
> their size so often obscure the relevant content in your message.  Some
> subset is usually much better (eg, "produces lots of errors like X"
> here).

May I suggest the following:

    spi: rspi: Fix bogus received byte in qspi_transfer_in()

    When there are less than QSPI_BUFFER_SIZE remaining bytes to be received,
    qspi_transfer_in() writes one bogus byte in the receive buffer, possibly
    leading to a buffer overflow.

    This can be reproduced by mounting, unmounting, and remounting a
    jffs2-formatted device, causing lots of warnings like:

        jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b40000: 0x1900 instead

    Remove the bogus write to fix this.

It's also a good idea to add a Fixes tag:

    Fixes: 3be09bec42a800d4 ("spi: rspi: supports 32bytes buffer for
DUAL and QUAD")

(the code was moved afterwards, but both the origin and the move were
 integrated in v4.10-rc1).

Finally:

    Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Applied "spi: rspi: Fixes bogus received byte in qspi_transfer_in()" to the spi tree

[Mark Brown]

The patch

   spi: rspi: Fixes bogus received byte in qspi_transfer_in()

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7264abc7000d601726aefb05189ea524ee3995ba Mon Sep 17 00:00:00 2001
From: DongCV <cv-dong@jinso.co.jp>
Date: Wed, 15 Feb 2017 19:50:51 +0900
Subject: [PATCH] spi: rspi: Fixes bogus received byte in qspi_transfer_in()

In qspi_transfer_in(), when receiving the last n (or len) bytes of data,
one bogus byte was written in the receive buffer.
This code leads to a buffer overflow.

"jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b40000: 0x1900 instead
jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b40004: 0x000c instead"

The error message above happens when trying to mount, unmount,
and remount a jffs2-formatted device.
This patch removed the bogus write to fixes: 3be09bec42a800d4
"spi: rspi: supports 32bytes buffer for DUAL and QUAD"

And here is Geert's comment:

"spi: rspi: Fix bogus received byte in qspi_transfer_in()
When there are less than QSPI_BUFFER_SIZE remaining bytes to be received,
qspi_transfer_in() writes one bogus byte in the receive buffer, possibly
leading to a buffer overflow.
This can be reproduced by mounting, unmounting, and remounting a
jffs2-formatted device, causing lots of warnings like:

"jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
at 0x03b40000: 0x1900 instead"

Remove the bogus write to fix this. "

Signed-off-by: DongCV <cv-dong@jinso.co.jp>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-rspi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 9daf50031737..2ee130138066 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
 			ret = rspi_pio_transfer(rspi, NULL, rx, n);
 			if (ret < 0)
 				return ret;
-			*rx++ = ret;
 		}
 		n -= len;
 	}
-- 
2.11.0

[PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[DongCV]

[-- Attachment #1: Type: text/plain, Size: 290 bytes --]

Dear Mark,
cc:Geert

This patch fixes the output warning logs and data loss when performing 
mount/umount then remount the device with jffs2 format.
This patch is tested on Lager board at Linux uptream v4.10-rc5.
Please consider the patch for Renesas-soc Gen2

Best regards,
Dong.

------


[-- Attachment #2: 0001-spi-rspi-fix-the-bug-related-to-mount-remount-jffs2.patch --]
[-- Type: text/x-patch, Size: 2535 bytes --]

>From 5513c6a02c911fcf9f99d5188fa46256aa1e41b4 Mon Sep 17 00:00:00 2001
From: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
Date: Fri, 27 Jan 2017 16:29:18 +0900
Subject: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

This patch fixes the output warning logs and data loss when
performing mount/umount then remount the device with jffs2 format.

This is the warning logs when performing mount/umount then remount the device with jffs2 format:
"root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
[ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
[ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
[ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
[ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
[ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
[ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
[ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
[ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
[ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
[ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
[ 3840.212900] jffs2: Further such events for this erase block will not be printed
[ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
:~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...

Signed-off-by: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
---
 drivers/spi/spi-rspi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
index 9daf500..2ee1301 100644
--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
 			ret = rspi_pio_transfer(rspi, NULL, rx, n);
 			if (ret < 0)
 				return ret;
-			*rx++ = ret;
 		}
 		n -= len;
 	}
-- 
1.9.1

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Geert Uytterhoeven]

Hi Dong,

On Fri, Jan 27, 2017 at 9:12 AM, DongCV <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org> wrote:
> This patch fixes the output warning logs and data loss when performing
> mount/umount then remount the device with jffs2 format.
> This patch is tested on Lager board at Linux uptream v4.10-rc5.
> Please consider the patch for Renesas-soc Gen2

Please send patches inline.

> From 5513c6a02c911fcf9f99d5188fa46256aa1e41b4 Mon Sep 17 00:00:00 2001
> From: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
> Date: Fri, 27 Jan 2017 16:29:18 +0900
> Subject: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2
>
> This patch fixes the output warning logs and data loss when
> performing mount/umount then remount the device with jffs2 format.
>
> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
> [ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
> [ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
> [ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
> [ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
> [ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
> [ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
> [ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
> [ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
> [ 3840.212900] jffs2: Further such events for this erase block will not be printed
> [ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
> root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
> :~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...
>
> Signed-off-by: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
> ---
>  drivers/spi/spi-rspi.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
> index 9daf500..2ee1301 100644
> --- a/drivers/spi/spi-rspi.c
> +++ b/drivers/spi/spi-rspi.c
> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
>                       ret = rspi_pio_transfer(rspi, NULL, rx, n);
>                       if (ret < 0)
>                               return ret;
> -                     *rx++ = ret;

Storing the success code (0) in the receive buffer is indeed wrong.

However, there are other bugs in that code:

rspi_pio_transfer(rspi, NULL, rx, n) transfers n bytes instead of len,
while n is decreased by len (which is <= n).
Furthermore rx is not incremented.
Hence if len < n, n will still be non-zero, and a new iteration of the
loop will be started, trying to receive more data, and overwriting the
just filled buffer.

The same bug is present in qspi_transfer_out().

>               }
>               n -= len;
>       }
> --
> 1.9.1
>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[Geert Uytterhoeven]

CC correct linux-renesas-soc

On Fri, Jan 27, 2017 at 10:00 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> Hi Dong,
>
> On Fri, Jan 27, 2017 at 9:12 AM, DongCV <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org> wrote:
>> This patch fixes the output warning logs and data loss when performing
>> mount/umount then remount the device with jffs2 format.
>> This patch is tested on Lager board at Linux uptream v4.10-rc5.
>> Please consider the patch for Renesas-soc Gen2
>
> Please send patches inline.
>
>> From 5513c6a02c911fcf9f99d5188fa46256aa1e41b4 Mon Sep 17 00:00:00 2001
>> From: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
>> Date: Fri, 27 Jan 2017 16:29:18 +0900
>> Subject: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2
>>
>> This patch fixes the output warning logs and data loss when
>> performing mount/umount then remount the device with jffs2 format.
>>
>> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
>> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
>> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
>> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
>> [ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
>> [ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
>> [ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
>> [ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
>> [ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
>> [ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
>> [ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
>> [ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
>> [ 3840.212900] jffs2: Further such events for this erase block will not be printed
>> [ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
>> root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
>> :~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...
>>
>> Signed-off-by: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
>> ---
>>  drivers/spi/spi-rspi.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
>> index 9daf500..2ee1301 100644
>> --- a/drivers/spi/spi-rspi.c
>> +++ b/drivers/spi/spi-rspi.c
>> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
>>                       ret = rspi_pio_transfer(rspi, NULL, rx, n);
>>                       if (ret < 0)
>>                               return ret;
>> -                     *rx++ = ret;
>
> Storing the success code (0) in the receive buffer is indeed wrong.
>
> However, there are other bugs in that code:
>
> rspi_pio_transfer(rspi, NULL, rx, n) transfers n bytes instead of len,
> while n is decreased by len (which is <= n).
> Furthermore rx is not incremented.
> Hence if len < n, n will still be non-zero, and a new iteration of the
> loop will be started, trying to receive more data, and overwriting the
> just filled buffer.
>
> The same bug is present in qspi_transfer_out().
>
>>               }
>>               n -= len;
>>       }
>> --
>> 1.9.1

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2

[DongCV]

Dear Geert,

I apologize for the delayed response.
I have reviewed the code. I saw that rx and tx are incremented in the 
rspi_pio_transfer() function.
" if (rx) {
       int ret = rspi_data_in(rspi);
       if (ret < 0)
           return ret;
       *rx++ = ret;
   }"

The same case for tx.
" if (tx) {
      int ret = rspi_data_out(rspi, *tx++);
      if (ret < 0)
         return ret;
   }"

I've retested on Lager(H2) with v4.10-rc2 afterI added the patch "spi: 
rspi: fix the bug related to mount/remount jffs2".
I have tried to read/write a data from/to device, then using cmp command 
to compare destination data and source data.
Result is the destination data is same to the source data.


Regards,
Dong


On 01/27/2017 06:00 PM, Geert Uytterhoeven wrote:
> Hi Dong,
>
> On Fri, Jan 27, 2017 at 9:12 AM, DongCV <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org> wrote:
>> This patch fixes the output warning logs and data loss when performing
>> mount/umount then remount the device with jffs2 format.
>> This patch is tested on Lager board at Linux uptream v4.10-rc5.
>> Please consider the patch for Renesas-soc Gen2
> Please send patches inline.
>
>>  From 5513c6a02c911fcf9f99d5188fa46256aa1e41b4 Mon Sep 17 00:00:00 2001
>> From: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
>> Date: Fri, 27 Jan 2017 16:29:18 +0900
>> Subject: [PATCH] spi: rspi: fix the bug related to mount/remount jffs2
>>
>> This patch fixes the output warning logs and data loss when
>> performing mount/umount then remount the device with jffs2 format.
>>
>> This is the warning logs when performing mount/umount then remount the device with jffs2 format:
>> "root@linaro-naro:~# mount -t jffs2 /dev/mtdblock2 /mnt/media
>> [ 3839.928013] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40000: 0x1900 instead
>> [ 3839.956515] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40004: 0x000c instead
>> [ 3839.985009] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40008: 0xb0b1 instead
>> [ 3840.013591] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4000c: 0x1900 instead
>> [ 3840.042087] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40010: 0x0044 instead
>> [ 3840.070566] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40014: 0xfb1d instead
>> [ 3840.099159] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40018: 0x0002 instead
>> [ 3840.127604] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4001c: 0x0001 instead
>> [ 3840.156043] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b40020: 0x81a4 instead
>> [ 3840.184472] jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found at 0x03b4002c: 0x6529 instead
>> [ 3840.212900] jffs2: Further such events for this erase block will not be printed
>> [ 3840.322915] jffs2: notice: (2008) read_dnode: node CRC failed on dnode at 0x3b40080: read 0xc40b5dfc, calculated 0x264be003
>> root@linaro-naro[ 3840.356857] jffs2: warning: (2008) jffs2_do_read_inode_internal: no data nodes found for ino #2
>> :~# [ 3840.386659] jffs2: Returned error for crccheck of ino #2. Expect badness...
>>
>> Signed-off-by: Dong <cv-dong-HEF513clHfp3+QwDJ9on6Q@public.gmane.org>
>> ---
>>   drivers/spi/spi-rspi.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c
>> index 9daf500..2ee1301 100644
>> --- a/drivers/spi/spi-rspi.c
>> +++ b/drivers/spi/spi-rspi.c
>> @@ -848,7 +848,6 @@ static int qspi_transfer_in(struct rspi_data *rspi, struct spi_transfer *xfer)
>>                        ret = rspi_pio_transfer(rspi, NULL, rx, n);
>>                        if (ret < 0)
>>                                return ret;
>> -                     *rx++ = ret;
> Storing the success code (0) in the receive buffer is indeed wrong.
>
> However, there are other bugs in that code:
>
> rspi_pio_transfer(rspi, NULL, rx, n) transfers n bytes instead of len,
> while n is decreased by len (which is <= n).
> Furthermore rx is not incremented.
> Hence if len < n, n will still be non-zero, and a new iteration of the
> loop will be started, trying to receive more data, and overwriting the
> just filled buffer.
>
> The same bug is present in qspi_transfer_out().
>
>>                }
>>                n -= len;
>>        }
>> --
>> 1.9.1
>>
> Gr{oetje,eeting}s,
>
>                          Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
>

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html