Re: Linux UIO driver cache problem in PowerPC (fix)

Re: Linux UIO driver cache problem in PowerPC (fix)

[Hans-Jürgen Koch]

Am Wed, 12 Mar 2008 15:22:59 -0400
schrieb "Jean-Samuel Chenard" <jsamch@gmail.com>:

> Hi,

Hi Jean-Samuel,
thanks for your report. Please CC LKML if you find bugs like this. I
also added Grant Likely (PPC Xilinx Virtex maintainer), as it seems to
be a platform specific problem.

> 
> Experimenting with your userspace I/O driver on a Xilinx FPGA (PowerPC
> 405), I was having problems when reading hardware device registers
> mapped on my platform using the user-space mmap() call.
> 
> After some investigation (looking at the driver/char/mspec.c file), I
> found that preventing caching of the page in uio_mmap_physical fixed
> my issues with userspace mmap() reading messed-up values.

Hm, we already mark the page with VM_IO. That's sufficient on x86 and
arm. I'm not sure whether this is a bug in PPC memory handling. Why do
they cache VM_IO pages? 

> 
> This patch is against your initial commit of the UIO driver in the
> mainline kernel tree.
> 
> ====
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index 865f32b..36e1123 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -22,6 +22,7 @@
>  #include <linux/string.h>
>  #include <linux/kobject.h>
>  #include <linux/uio_driver.h>
> +#include <asm/pgtable.h>
> 
>  #define UIO_MAX_DEVICES 255
> 
> @@ -447,6 +448,8 @@ static int uio_mmap_physical(struct
> vm_area_struct *vma)
> 
>         vma->vm_flags |= VM_IO | VM_RESERVED;
> 
> +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +
>         return remap_pfn_range(vma,
>                                vma->vm_start,
>                                idev->info->mem[mi].addr >> PAGE_SHIFT,
> ====
> 
> I am a bit unsure if this will break something in any of your uses of
> the UIO driver (on other platforms than ppc), but it really fixes
> things for me.  

It should be OK, but unneccesary on other platforms. I have no
objections, but I fear in 6 months we'll see a patch removing that
line again because it's not needed... 

> Let me know if you need more details on what was
> happening on my platform before I added this statement.

I'd like to hear the opinion of people really involved in PPC memory
handling.

> 
> Thanks for the great driver for user-space I/O, it will save me lots
> of time in my research.
> 
> Regards,
> 
> Jean-Samuel

Thanks,
Hans

Re: Linux UIO driver cache problem in PowerPC (fix)

[Grant Likely]

On Thu, Mar 13, 2008 at 1:19 AM, Hans-Jürgen Koch <hjk@linutronix.de> wrote:
> Am Wed, 12 Mar 2008 15:22:59 -0400
>  schrieb "Jean-Samuel Chenard" <jsamch@gmail.com>:
>
>  > Hi,
>
>  Hi Jean-Samuel,
>  thanks for your report. Please CC LKML if you find bugs like this. I
>  also added Grant Likely (PPC Xilinx Virtex maintainer), as it seems to
>  be a platform specific problem.

Hmmm.  I'm not a memory handling expert so I don't know what's going
on here.  Josh, does this look like anything that would be ppc405
related?

g.

>
>  >
>  > Experimenting with your userspace I/O driver on a Xilinx FPGA (PowerPC
>  > 405), I was having problems when reading hardware device registers
>  > mapped on my platform using the user-space mmap() call.
>  >
>  > After some investigation (looking at the driver/char/mspec.c file), I
>  > found that preventing caching of the page in uio_mmap_physical fixed
>  > my issues with userspace mmap() reading messed-up values.
>
>  Hm, we already mark the page with VM_IO. That's sufficient on x86 and
>  arm. I'm not sure whether this is a bug in PPC memory handling. Why do
>  they cache VM_IO pages?
>
>  >
>  > This patch is against your initial commit of the UIO driver in the
>  > mainline kernel tree.
>  >
>  > ====
>  > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
>  > index 865f32b..36e1123 100644
>  > --- a/drivers/uio/uio.c
>  > +++ b/drivers/uio/uio.c
>  > @@ -22,6 +22,7 @@
>  >  #include <linux/string.h>
>  >  #include <linux/kobject.h>
>  >  #include <linux/uio_driver.h>
>  > +#include <asm/pgtable.h>
>  >
>  >  #define UIO_MAX_DEVICES 255
>  >
>  > @@ -447,6 +448,8 @@ static int uio_mmap_physical(struct
>  > vm_area_struct *vma)
>  >
>  >         vma->vm_flags |= VM_IO | VM_RESERVED;
>  >
>  > +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
>  > +
>  >         return remap_pfn_range(vma,
>  >                                vma->vm_start,
>  >                                idev->info->mem[mi].addr >> PAGE_SHIFT,
>  > ====
>  >
>  > I am a bit unsure if this will break something in any of your uses of
>  > the UIO driver (on other platforms than ppc), but it really fixes
>  > things for me.
>
>  It should be OK, but unneccesary on other platforms. I have no
>  objections, but I fear in 6 months we'll see a patch removing that
>  line again because it's not needed...
>
>  > Let me know if you need more details on what was
>  > happening on my platform before I added this statement.
>
>  I'd like to hear the opinion of people really involved in PPC memory
>  handling.
>
>  >
>  > Thanks for the great driver for user-space I/O, it will save me lots
>  > of time in my research.
>  >
>  > Regards,
>  >
>  > Jean-Samuel
>
>  Thanks,
>  Hans
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: Linux UIO driver cache problem in PowerPC (fix)

[Juergen Beisert]

On Thursday 13 March 2008 22:53, Grant Likely wrote:
> On Thu, Mar 13, 2008 at 1:19 AM, Hans-Jürgen Koch <hjk@linutronix.de> wrote:
> > Am Wed, 12 Mar 2008 15:22:59 -0400
> >
> >  schrieb "Jean-Samuel Chenard" <jsamch@gmail.com>:
> >  > Hi,
> >
> >  Hi Jean-Samuel,
> >  thanks for your report. Please CC LKML if you find bugs like this. I
> >  also added Grant Likely (PPC Xilinx Virtex maintainer), as it seems to
> >  be a platform specific problem.
>
> Hmmm.  I'm not a memory handling expert so I don't know what's going
> on here.  Josh, does this look like anything that would be ppc405
> related?

The UIO also doesn't work on a MPC5200 CPU. With the additional 
pgprot_noncached() call now it works as expected.

Juergen

Re: Linux UIO driver cache problem in PowerPC (fix)

[Hans J. Koch]

Am Thu, 13 Mar 2008 08:19:32 +0100
schrieb Hans-Jürgen Koch <hjk@linutronix.de>:

> Am Wed, 12 Mar 2008 15:22:59 -0400
> schrieb "Jean-Samuel Chenard" <jsamch@gmail.com>:
> 
> > Hi,

Hi Jean-Samuel,
I investigated a bit and found your patch to be correct. I cleaned it
up a bit and sent it to Greg and LKML. I added you as author and also
took the freedom to add your Signed-off-by.

Juergen, Thomas, Grant, if you can confirm this, please add your
signature as well.

Thanks,
Hans

> 
> Hi Jean-Samuel,
> thanks for your report. Please CC LKML if you find bugs like this. I
> also added Grant Likely (PPC Xilinx Virtex maintainer), as it seems to
> be a platform specific problem.
> 
> > 
> > Experimenting with your userspace I/O driver on a Xilinx FPGA
> > (PowerPC 405), I was having problems when reading hardware device
> > registers mapped on my platform using the user-space mmap() call.
> > 
> > After some investigation (looking at the driver/char/mspec.c file),
> > I found that preventing caching of the page in uio_mmap_physical
> > fixed my issues with userspace mmap() reading messed-up values.
> 
> Hm, we already mark the page with VM_IO. That's sufficient on x86 and
> arm. I'm not sure whether this is a bug in PPC memory handling. Why do
> they cache VM_IO pages? 
> 
> > 
> > This patch is against your initial commit of the UIO driver in the
> > mainline kernel tree.
> > 
> > ====
> > diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> > index 865f32b..36e1123 100644
> > --- a/drivers/uio/uio.c
> > +++ b/drivers/uio/uio.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/string.h>
> >  #include <linux/kobject.h>
> >  #include <linux/uio_driver.h>
> > +#include <asm/pgtable.h>
> > 
> >  #define UIO_MAX_DEVICES 255
> > 
> > @@ -447,6 +448,8 @@ static int uio_mmap_physical(struct
> > vm_area_struct *vma)
> > 
> >         vma->vm_flags |= VM_IO | VM_RESERVED;
> > 
> > +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > +
> >         return remap_pfn_range(vma,
> >                                vma->vm_start,
> >                                idev->info->mem[mi].addr >>
> > PAGE_SHIFT, ====
> > 
> > I am a bit unsure if this will break something in any of your uses
> > of the UIO driver (on other platforms than ppc), but it really fixes
> > things for me.  
> 
> It should be OK, but unneccesary on other platforms. I have no
> objections, but I fear in 6 months we'll see a patch removing that
> line again because it's not needed... 
> 
> > Let me know if you need more details on what was
> > happening on my platform before I added this statement.
> 
> I'd like to hear the opinion of people really involved in PPC memory
> handling.
> 
> > 
> > Thanks for the great driver for user-space I/O, it will save me lots
> > of time in my research.
> > 
> > Regards,
> > 
> > Jean-Samuel
> 
> Thanks,
> Hans
> 
> --
> To unsubscribe from this list: send the line "unsubscribe
> linux-kernel" in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: Linux UIO driver cache problem in PowerPC (fix)

[Leon Woestenberg]

Hello,

On Fri, Mar 14, 2008 at 11:33 AM, Hans J. Koch <hjk@linutronix.de> wrote:
>  I investigated a bit and found your patch to be correct. I cleaned it
>  up a bit and sent it to Greg and LKML. I added you as author and also
>  took the freedom to add your Signed-off-by.
>
Could you elaborate why exactly this is needed, maybe even as a
comment in the patch?

It seemed non-trivial enough to rectify adding a comment.

Thanks,
-- 
Leon

Re: Linux UIO driver cache problem in PowerPC (fix)

[Hans J. Koch]

Am Sun, 16 Mar 2008 03:51:10 +0100
schrieb "Leon Woestenberg" <leon.woestenberg@gmail.com>:

> Hello,
> 
> On Fri, Mar 14, 2008 at 11:33 AM, Hans J. Koch <hjk@linutronix.de>
> wrote:
> >  I investigated a bit and found your patch to be correct. I cleaned
> > it up a bit and sent it to Greg and LKML. I added you as author
> > and also took the freedom to add your Signed-off-by.
> >
> Could you elaborate why exactly this is needed, maybe even as a
> comment in the patch?

As far as I understood, the VM_IO flag simply means that the page is
used for IO. On some architectures this automatically means that the
page is not cached, but this is not guaranteed. That's why
pgprot_noncached() exists.

If I misunderstood something or somebody can explain it better, please
let me know.

> 
> It seemed non-trivial enough to rectify adding a comment.

It _is_ trivial. It's just something you don't need every day, and
therefore it's not known to everybody (including me) or not obvious.
But if you look at it:

VM_IO means IO
pgprot_noncached means noncached

What could be more trivial?

Thanks,
Hans