[PATCH] UIO: only call pgprot_noncached if defined

[PATCH] UIO: only call pgprot_noncached if defined

[Mike Frysinger]

Not all arches support pgprot_noncached (like the Blackfin port).  Other
drivers seem to handle this by checking to see if it is defined, so let's
do that in the UIO driver as well.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 drivers/uio/uio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
index f9b4647..c43dbea 100644
--- a/drivers/uio/uio.c
+++ b/drivers/uio/uio.c
@@ -529,7 +529,9 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
 
 	vma->vm_flags |= VM_IO | VM_RESERVED;
 
+#ifdef pgprot_noncached
 	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+#endif
 
 	return remap_pfn_range(vma,
 			       vma->vm_start,
-- 
1.6.0.2

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Hans J. Koch]

On Wed, Oct 29, 2008 at 01:26:17PM -0400, Mike Frysinger wrote:
> Not all arches support pgprot_noncached (like the Blackfin port). 

These archs have it (just a quick grep, might be incomplete):
x86, sh, alpha, powerpc, ia64, sparc, sparc64, mips, avr32, arm, parisc.

That's 11 archs _with_ pgprot_noncached(). Might be a good idea to add
that macro to blackfin, too.

> Other
> drivers seem to handle this by checking to see if it is defined,

That statement made me curious. I greped around for a while, but the
only driver that does this check is drivers/char/mem.c, while there are
22 drivers that use pgprot_noncached() without checking.

> so let's do that in the UIO driver as well.

Well, your patch is surely OK and will work, but I've got some
difficulties to understand why you prefer patching an ugly #ifdef into up
to 22 drivers instead of just defining that simple macro in your arch,
like many of the others already did. But maybe I missed the point
here, could you please elaborate a bit?

Thanks,
Hans

> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
>  drivers/uio/uio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index f9b4647..c43dbea 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -529,7 +529,9 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
>  
>  	vma->vm_flags |= VM_IO | VM_RESERVED;
>  
> +#ifdef pgprot_noncached
>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +#endif
>  
>  	return remap_pfn_range(vma,
>  			       vma->vm_start,
> -- 
> 1.6.0.2

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Mike Frysinger]

On Wed, Oct 29, 2008 at 15:53, Hans J. Koch wrote:
> On Wed, Oct 29, 2008 at 01:26:17PM -0400, Mike Frysinger wrote:
>> Not all arches support pgprot_noncached (like the Blackfin port).
>
> These archs have it (just a quick grep, might be incomplete):
> x86, sh, alpha, powerpc, ia64, sparc, sparc64, mips, avr32, arm, parisc.
>
> That's 11 archs _with_ pgprot_noncached(). Might be a good idea to add
> that macro to blackfin, too.

when i looked i got the feeling that the arches that could support
this added it, and those that couldnt did not.  the Blackfin arch
cannot support per-page caching behavior (well, we can with the MPU
turned on, but the overhead of the MPU makes it unrealistic for
production deployment in most cases), so we do not have it defined.

there doesnt seem to be any real documentation on this function though ...

>> Other
>> drivers seem to handle this by checking to see if it is defined,
>
> That statement made me curious. I greped around for a while, but the
> only driver that does this check is drivers/char/mem.c, while there are
> 22 drivers that use pgprot_noncached() without checking.

i was ignoring the drivers that are not generic as i dont think
they're relevant.  that cuts out quite a few of those "22 drivers".

>> so let's do that in the UIO driver as well.
>
> Well, your patch is surely OK and will work, but I've got some
> difficulties to understand why you prefer patching an ugly #ifdef into up
> to 22 drivers instead of just defining that simple macro in your arch,
> like many of the others already did. But maybe I missed the point
> here, could you please elaborate a bit?

it doesnt make sense to ifdef a driver that only works on a single
arch or two and has hardware support for this function (and thus has
it defined).
-mike

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Hans J. Koch]

There seem to be archs that cannot easily implement a sensible
pgprot_noncached() function, so we should merge this patch. UIO doesn't
compile on these archs right now.

Thanks,
Hans

On Wed, Oct 29, 2008 at 01:26:17PM -0400, Mike Frysinger wrote:
> Not all arches support pgprot_noncached (like the Blackfin port).  Other
> drivers seem to handle this by checking to see if it is defined, so let's
> do that in the UIO driver as well.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Signed-off-by: Hans J. Koch <hjk@linutronix.de>

> ---
>  drivers/uio/uio.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/uio/uio.c b/drivers/uio/uio.c
> index f9b4647..c43dbea 100644
> --- a/drivers/uio/uio.c
> +++ b/drivers/uio/uio.c
> @@ -529,7 +529,9 @@ static int uio_mmap_physical(struct vm_area_struct *vma)
>  
>  	vma->vm_flags |= VM_IO | VM_RESERVED;
>  
> +#ifdef pgprot_noncached
>  	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +#endif
>  
>  	return remap_pfn_range(vma,
>  			       vma->vm_start,
> -- 
> 1.6.0.2

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Greg KH]

On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
> There seem to be archs that cannot easily implement a sensible
> pgprot_noncached() function, so we should merge this patch. UIO doesn't
> compile on these archs right now.

No, we should fix those arches to have that function at least NULLed
out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
not something we really want to do if at all possible.

thanks,

greg k-h

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Hans J. Koch]

On Wed, Nov 05, 2008 at 08:33:34AM -0800, Greg KH wrote:
> On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
> > There seem to be archs that cannot easily implement a sensible
> > pgprot_noncached() function, so we should merge this patch. UIO doesn't
> > compile on these archs right now.
> 
> No, we should fix those arches to have that function at least NULLed
> out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
> not something we really want to do if at all possible.

Yep, 5 minutes after I sent the mail I regretted it. Blackfin could
easily implement an "empty" macro that just returns its parameter.

I hereby revoke my Signed-off-by, you're right.

Thanks,
Hans

> 
> thanks,
> 
> greg k-h

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Mike Frysinger]

On Wed, Nov 5, 2008 at 12:08, Hans J. Koch wrote:
> On Wed, Nov 05, 2008 at 08:33:34AM -0800, Greg KH wrote:
>> On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
>> > There seem to be archs that cannot easily implement a sensible
>> > pgprot_noncached() function, so we should merge this patch. UIO doesn't
>> > compile on these archs right now.
>>
>> No, we should fix those arches to have that function at least NULLed
>> out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
>> not something we really want to do if at all possible.
>
> Yep, 5 minutes after I sent the mail I regretted it. Blackfin could
> easily implement an "empty" macro that just returns its parameter.

just because it's easy to do doesnt mean it's correct.  i'm looking
for the correct answer here, not the quick & dirty & forget about it.

perhaps uio_mmap_physical() should look like:
static int uio_mmap_physical(struct vm_area_struct *vma)
{
#ifdef pgprot_noncached
    struct uio_device *idev = vma->vm_private_data;
    int mi = uio_find_mem_index(vma);
    if (mi < 0)
        return -EINVAL;

    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,
                   vma->vm_end - vma->vm_start,
                   vma->vm_page_prot);
#else
    return -EFAULT;
#endif
}

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Mike Frysinger]

On Wed, Nov 5, 2008 at 11:33, Greg KH wrote:
> On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
>> There seem to be archs that cannot easily implement a sensible
>> pgprot_noncached() function, so we should merge this patch. UIO doesn't
>> compile on these archs right now.
>
> No, we should fix those arches to have that function at least NULLed
> out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
> not something we really want to do if at all possible.

that was my question.  this function isnt documented.  if the hardware
doesnt support it, is the right thing really for the arch to lie to
drivers and not actually give back cached settings even though it
asked for non-cached ?
-mike

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Greg KH]

On Wed, Nov 05, 2008 at 12:33:37PM -0500, Mike Frysinger wrote:
> On Wed, Nov 5, 2008 at 11:33, Greg KH wrote:
> > On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
> >> There seem to be archs that cannot easily implement a sensible
> >> pgprot_noncached() function, so we should merge this patch. UIO doesn't
> >> compile on these archs right now.
> >
> > No, we should fix those arches to have that function at least NULLed
> > out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
> > not something we really want to do if at all possible.
> 
> that was my question.  this function isnt documented.  if the hardware
> doesnt support it, is the right thing really for the arch to lie to
> drivers and not actually give back cached settings even though it
> asked for non-cached ?

Probably not, it sounds like the arch needs to be fixed :)

thanks,

greg k-h

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Mike Frysinger]

On Wed, Nov 5, 2008 at 12:42, Greg KH wrote:
> On Wed, Nov 05, 2008 at 12:33:37PM -0500, Mike Frysinger wrote:
>> On Wed, Nov 5, 2008 at 11:33, Greg KH wrote:
>> > On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
>> >> There seem to be archs that cannot easily implement a sensible
>> >> pgprot_noncached() function, so we should merge this patch. UIO doesn't
>> >> compile on these archs right now.
>> >
>> > No, we should fix those arches to have that function at least NULLed
>> > out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
>> > not something we really want to do if at all possible.
>>
>> that was my question.  this function isnt documented.  if the hardware
>> doesnt support it, is the right thing really for the arch to lie to
>> drivers and not actually give back cached settings even though it
>> asked for non-cached ?
>
> Probably not, it sounds like the arch needs to be fixed :)

i'd agree, but we're dealing with reality here: we're talking no-mmu
and we can not do caching on a per-page basis while maintaining
anything resembling usable performance.
-mike

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Hans J. Koch]

On Wed, Nov 05, 2008 at 12:33:37PM -0500, Mike Frysinger wrote:
> On Wed, Nov 5, 2008 at 11:33, Greg KH wrote:
> > On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
> >> There seem to be archs that cannot easily implement a sensible
> >> pgprot_noncached() function, so we should merge this patch. UIO doesn't
> >> compile on these archs right now.
> >
> > No, we should fix those arches to have that function at least NULLed
> > out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
> > not something we really want to do if at all possible.
> 
> that was my question.  this function isnt documented.  if the hardware
> doesnt support it, is the right thing really for the arch to lie to
> drivers and not actually give back cached settings even though it
> asked for non-cached ?

Well, if there's no possibility to map device memory in a non-cached
way, it's very likely you don't want to use UIO at all. If there's a few
seconds between a write and the value actually appearing in the hardware
register, you will certainly don't see the results you expect.

If Blackfin can't mmap non-cached memory, we should make UIO depend on
!BLACKFIN.

Hans

Re: [PATCH] UIO: only call pgprot_noncached if defined

[Mike Frysinger]

On Wed, Nov 5, 2008 at 13:27, Hans J. Koch wrote:
> On Wed, Nov 05, 2008 at 12:33:37PM -0500, Mike Frysinger wrote:
>> On Wed, Nov 5, 2008 at 11:33, Greg KH wrote:
>> > On Wed, Nov 05, 2008 at 12:36:11PM +0100, Hans J. Koch wrote:
>> >> There seem to be archs that cannot easily implement a sensible
>> >> pgprot_noncached() function, so we should merge this patch. UIO doesn't
>> >> compile on these archs right now.
>> >
>> > No, we should fix those arches to have that function at least NULLed
>> > out.  Isn't there only one, Blackfin?  Putting #ifdefs in .c files is
>> > not something we really want to do if at all possible.
>>
>> that was my question.  this function isnt documented.  if the hardware
>> doesnt support it, is the right thing really for the arch to lie to
>> drivers and not actually give back cached settings even though it
>> asked for non-cached ?
>
> Well, if there's no possibility to map device memory in a non-cached
> way, it's very likely you don't want to use UIO at all. If there's a few
> seconds between a write and the value actually appearing in the hardware
> register, you will certainly don't see the results you expect.
>
> If Blackfin can't mmap non-cached memory, we should make UIO depend on
> !BLACKFIN.

if it wasnt usable on the Blackfin arch, i wouldnt have been looking
at it in the first place.  there is some usage where merely hooking up
an interrupt is the only thing that is needed by userspace.

the only memory regions where a device would be hooked up are never
marked as cachable.  so if this function is only for mapping addresses
where device registers would live, then a stub pgprot_noncached()
would be fine.  but we cant do non-cached access on external memory
(e.g. SDRAM / DDR) if the device wanted coherent memory for dma.
-mike