jffs2 and unaligned access

jffs2 and unaligned access

[Sascha Hauer]

Hi,

memcpy_from/to_io() use word aligned accesses on the io side of memory.
The MPC5200 local plus bus where our flashes are connected does not
allow unaligned accesses, so we have to use the io versions of memcpy.

I have the suspicion that this is not the correct fix to our problem,
but maybe someone can have a look at it and point me in the right
direction.

Thanks,
  Sascha

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

---
 fs/jffs2/scan.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6/fs/jffs2/scan.c
===================================================================
--- linux-2.6.orig/fs/jffs2/scan.c
+++ linux-2.6/fs/jffs2/scan.c
@@ -16,6 +16,7 @@
 #include <linux/pagemap.h>
 #include <linux/crc32.h>
 #include <linux/compiler.h>
+#include <linux/io.h>
 #include "nodelist.h"
 #include "summary.h"
 #include "debug.h"
@@ -506,7 +507,7 @@ static int jffs2_scan_eraseblock (struct
 					sumptr = kmalloc(sumlen, GFP_KERNEL);
 					if (!sumptr)
 						return -ENOMEM;
-					memcpy(sumptr + sumlen - buf_len, buf + buf_size - buf_len, buf_len);
+					memcpy_fromio(sumptr + sumlen - buf_len, buf + buf_size - buf_len, buf_len);
 				}
 				if (buf_len < sumlen) {
 					/* Need to read more so that the entire summary node is present */
@@ -1036,7 +1037,7 @@ static int jffs2_scan_dirent_node(struct
 	if (!fd) {
 		return -ENOMEM;
 	}
-	memcpy(&fd->name, rd->name, checkedlen);
+	memcpy_fromio(&fd->name, rd->name, checkedlen);
 	fd->name[checkedlen] = 0;
 
 	crc = crc32(0, fd->name, rd->nsize);
-- 
Pengutronix e.K. - Linux Solutions for Science and Industry
-----------------------------------------------------------
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite -> http://www.pengutronix.de/impressum/ <-

Re: jffs2 and unaligned access

[David Woodhouse]

On Wed, 2008-05-07 at 12:27 +0200, Sascha Hauer wrote:
> memcpy_from/to_io() use word aligned accesses on the io side of memory.
> The MPC5200 local plus bus where our flashes are connected does not
> allow unaligned accesses, so we have to use the io versions of memcpy.

But this region of flash is marked as suitable for execute-in-place,
otherwise the point() function wouldn't be working to give a direct
pointer to it. It sounds like we shouldn't be allowing that.

Which in turn means that perhaps we should have a property in the
corresponding node in the device-tree which indicates that it's not
suitable for direct access?

-- 
dwmw2

Re: jffs2 and unaligned access

[Sascha Hauer]

On Wed, May 07, 2008 at 11:53:49AM +0100, David Woodhouse wrote:
> On Wed, 2008-05-07 at 12:27 +0200, Sascha Hauer wrote:
> > memcpy_from/to_io() use word aligned accesses on the io side of memory.
> > The MPC5200 local plus bus where our flashes are connected does not
> > allow unaligned accesses, so we have to use the io versions of memcpy.
> 
> But this region of flash is marked as suitable for execute-in-place,
> otherwise the point() function wouldn't be working to give a direct
> pointer to it. It sounds like we shouldn't be allowing that.

It actually is suitable for execute-in-place. It's the flash U-Boot
starts from. The compiler will generate a proper alignment for you.

> 
> Which in turn means that perhaps we should have a property in the
> corresponding node in the device-tree which indicates that it's not
> suitable for direct access?

So far we did not work with the device-tree flash binding but with the
physmap-flash driver, but ok, this is subject to change anyway.

I gave it a quick try to disable direct accesses. It works, but has a
1:10 performance impact on mounting a jffs2. At least it's a clean
solution.

Sascha


-- 
Pengutronix e.K. - Linux Solutions for Science and Industry
-----------------------------------------------------------
Kontakt-Informationen finden Sie im Header dieser Mail oder
auf der Webseite -> http://www.pengutronix.de/impressum/ <-

Re: jffs2 and unaligned access

[Segher Boessenkool]

>> memcpy_from/to_io() use word aligned accesses on the io side of 
>> memory.
>> The MPC5200 local plus bus where our flashes are connected does not
>> allow unaligned accesses, so we have to use the io versions of memcpy.
>
> But this region of flash is marked as suitable for execute-in-place,
> otherwise the point() function wouldn't be working to give a direct
> pointer to it. It sounds like we shouldn't be allowing that.
>
> Which in turn means that perhaps we should have a property in the
> corresponding node in the device-tree which indicates that it's not
> suitable for direct access?

This isn't usually a property of the flash device, but of the various
buses/controllers above the flash device.  The device tree should mimic
reality (and it does, it just seems the kernel doesn't use this 
information
yet?)


Segher

Re: jffs2 and unaligned access

[Detlev Zundel]

Hi,

>>> memcpy_from/to_io() use word aligned accesses on the io side of
>>> memory.
>>> The MPC5200 local plus bus where our flashes are connected does not
>>> allow unaligned accesses, so we have to use the io versions of memcpy.
>>
>> But this region of flash is marked as suitable for execute-in-place,
>> otherwise the point() function wouldn't be working to give a direct
>> pointer to it. It sounds like we shouldn't be allowing that.
>>
>> Which in turn means that perhaps we should have a property in the
>> corresponding node in the device-tree which indicates that it's not
>> suitable for direct access?
>
> This isn't usually a property of the flash device, but of the various
> buses/controllers above the flash device.  

I wholeheartedly agree.  After all, its the Local+ bus playing games
here.  And fixing that for JFFS2 only ignores that other devices can
*and will* be connected on this bus.  Unaligned accesses to such devices
will also fail (likely from mtd unrelated code) - and fail silently,
taking quite a while to figure out what is going wrong where.

> The device tree should mimic reality (and it does, it just seems the
> kernel doesn't use this information yet?)

How is this exactly supposed to work?

Cheers
  Detlev

-- 
In the topologic hell the beer is packed in Klein's bottles.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de

Re: jffs2 and unaligned access

[Jon Smirl]

On 5/7/08, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, May 07, 2008 at 11:53:49AM +0100, David Woodhouse wrote:
>  > On Wed, 2008-05-07 at 12:27 +0200, Sascha Hauer wrote:
>  > > memcpy_from/to_io() use word aligned accesses on the io side of memory.
>  > > The MPC5200 local plus bus where our flashes are connected does not
>  > > allow unaligned accesses, so we have to use the io versions of memcpy.
>  >
>  > But this region of flash is marked as suitable for execute-in-place,
>  > otherwise the point() function wouldn't be working to give a direct
>  > pointer to it. It sounds like we shouldn't be allowing that.
>
>
> It actually is suitable for execute-in-place. It's the flash U-Boot
>  starts from. The compiler will generate a proper alignment for you.
>
>
>  >
>  > Which in turn means that perhaps we should have a property in the
>  > corresponding node in the device-tree which indicates that it's not
>  > suitable for direct access?
>
>
> So far we did not work with the device-tree flash binding but with the
>  physmap-flash driver, but ok, this is subject to change anyway.
>
>  I gave it a quick try to disable direct accesses. It works, but has a
>  1:10 performance impact on mounting a jffs2. At least it's a clean
>  solution.

How did this get resolved? The thread died without any final solution
being proposed.


>
>
>  Sascha
>
>
>
>  --
>  Pengutronix e.K. - Linux Solutions for Science and Industry
>  -----------------------------------------------------------
>  Kontakt-Informationen finden Sie im Header dieser Mail oder
>  auf der Webseite -> http://www.pengutronix.de/impressum/ <-
>


-- 
Jon Smirl
jonsmirl@gmail.com