Re: [PATCH] ARM: allow, but warn, when issuing ioremap() on RAM

[Catalin Marinas]

On Sat, 2010-10-09 at 03:41 +0100, Nicolas Pitre wrote:
> On Sat, 9 Oct 2010, Russell King - ARM Linux wrote:
> 
> > On Fri, Oct 08, 2010 at 05:00:46PM -0700, Greg KH wrote:
> > > But you can't expect that you make this change, and not fix up the
> > > drivers, and people would be happy, right?  The rule for API changes
> > > like this, or anything, is that the person making the change fixes the
> > > other drivers, and that seems to be the issue here.
> >
> > Let's entirely revert the change and wait for people's data to be
> > corrupted then.  I don't have the time nor the motivation to work
> > through crap driver code to fix up these unreliable games which are
> > already illegal on platforms such as x86.
> >
> > If people want their system to be unpredictable, then let's carry on
> > giving them the rope to hang themselves in that manner.
> >
> > > Any pointers to patches where people have fixed up the drivers?
> >
> > Despite the discussion, I'm unaware of anyone really taking the issue
> > seriously and producing any patches during the last six months.
> >
> > So, I say sod it, let's revert the change.
> 
> I think that the real issue here is to avoid breaking those drivers
> which were using this legitimately.  So what about this compromize
> instead:
> 
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 99627d3..4f071e4 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -201,6 +201,15 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>         if (pfn >= 0x100000 && (__pfn_to_phys(pfn) & ~SUPERSECTION_MASK))
>                 return NULL;
> 
> +       /*
> +        * Warn if RAM is mapped to discourage this usage. Let's forbid it
> +        * outright on ARMv6+ where this became architecturally undefined
> +        * in theory and causes memory corruption in practice.
> +        */
> +       if (WARN_ON(pfn_valid(pfn)))
> +               if (__LINUX_ARM_ARCH__ >= 6)
> +                       return NULL;
> +
>         type = get_mem_type(mtype);
>         if (!type)
>                 return NULL;

We could be a bit more flexible as a temporary solution. But that's a
hack and doesn't guarantee the attributes that the driver requested. If
the driver would use writel/readl, that's not too bad since we pushed
explicit barriers in these macros.

It would need to be improved to invalidate the corresponding cache lines
(using the DMA API?) but it would look even worse.


diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 6bdf42c..082bbd0 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -204,10 +204,13 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 #endif
 
 	/*
-	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
+	 * Don't allow RAM to be mapped as Device memory - this causes
+	 * problems with ARMv6+
 	 */
 	if (WARN_ON(pfn_valid(pfn)))
-		return NULL;
+		if (__LINUX_ARM_ARCH__ >= 6 &&
+		    (mtype != MT_DEVICE_CACHED || mtype != MT_DEVICE_WC))
+			mtype = MT_DEVICE_WC;
 
 	type = get_mem_type(mtype);
 	if (!type)



-- 
Catalin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/