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

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

[Felipe Contreras]

Many drivers are broken, and there's no alternative in sight. Such a big
change should stay as a warning for now, and only later should it
actually fail.

The drivers are not doing something correct, we get it, but for now it's
better to allow them to work (they do 99% of the time anyway) rather
than to force everyone to revert this patch in their internal trees
until there's a solution. A slightly broken functionality is better than
no functionality at all.

A warning lets people know that what they are doing is not right, and
they should fix it.

Cc: Han Jonghun <jonghun79.han@gmail.com>
Cc: Hemant Pedanekar <hemantp@ti.com>
Cc: Arnd Hannemann <arnd@arndnet.de>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mm/ioremap.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

For issues related to this:
http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
http://article.gmane.org/gmane.linux.ports.sh.devel/8560
http://www.spinics.net/lists/linux-fbdev/msg01745.html
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/22271

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index ab50627..ccff7da 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -204,8 +204,7 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 	/*
 	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
 	 */
-	if (WARN_ON(pfn_valid(pfn)))
-		return NULL;
+	WARN_ON(pfn_valid(pfn));
 
 	type = get_mem_type(mtype);
 	if (!type)
-- 
1.7.3.1.2.g7fe2b

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

[Baruch Siach]

Hi Felipe,

On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
> Many drivers are broken, and there's no alternative in sight. Such a big
> change should stay as a warning for now, and only later should it
> actually fail.
> 
> The drivers are not doing something correct, we get it, but for now it's
> better to allow them to work (they do 99% of the time anyway) rather
> than to force everyone to revert this patch in their internal trees
> until there's a solution. A slightly broken functionality is better than
> no functionality at all.
> 
> A warning lets people know that what they are doing is not right, and
> they should fix it.

[snip]

> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -204,8 +204,7 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>  	/*
>  	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
>  	 */

Please change the comment to match the (new) code.

> -	if (WARN_ON(pfn_valid(pfn)))
> -		return NULL;
> +	WARN_ON(pfn_valid(pfn));
>  
>  	type = get_mem_type(mtype);
>  	if (!type)

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

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

[Felipe Contreras]

Many drivers are broken, and there's no alternative in sight. Such a big
change should stay as a warning for now, and only later should it
actually fail.

The drivers are not doing something correct, we get it, but for now it's
better to allow them to work (they do 99% of the time anyway) rather
than to force everyone to revert this patch in their internal trees
until there's a solution. A slightly broken functionality is better than
no functionality at all.

A warning lets people know that what they are doing is not right, and
they should fix it.

Cc: Han Jonghun <jonghun79.han@gmail.com>
Cc: Hemant Pedanekar <hemantp@ti.com>
Cc: Arnd Hannemann <arnd@arndnet.de>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Omar Ramirez Luna <omar.ramirez@ti.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 arch/arm/mm/ioremap.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

For issues related to this:
http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
http://article.gmane.org/gmane.linux.ports.sh.devel/8560
http://www.spinics.net/lists/linux-fbdev/msg01745.html
http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/22271

diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index ab50627..a168673 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -202,10 +202,9 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
 		return NULL;
 
 	/*
-	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
+	 * This causes problems with ARMv6+. Will be disallowed soon.
 	 */
-	if (WARN_ON(pfn_valid(pfn)))
-		return NULL;
+	WARN_ON(pfn_valid(pfn));
 
 	type = get_mem_type(mtype);
 	if (!type)
-- 
1.7.3.1.2.g7fe2b

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

[Uwe Kleine-König]

Hello,

On Thu, Oct 07, 2010 at 03:29:09PM +0300, Felipe Contreras wrote:
> Many drivers are broken, and there's no alternative in sight. Such a big
> change should stay as a warning for now, and only later should it
> actually fail.
> 
> The drivers are not doing something correct, we get it, but for now it's
> better to allow them to work (they do 99% of the time anyway) rather
> than to force everyone to revert this patch in their internal trees
> until there's a solution. A slightly broken functionality is better than
> no functionality at all.
> 
> A warning lets people know that what they are doing is not right, and
> they should fix it.
> 
> Cc: Han Jonghun <jonghun79.han@gmail.com>
> Cc: Hemant Pedanekar <hemantp@ti.com>
> Cc: Arnd Hannemann <arnd@arndnet.de>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Omar Ramirez Luna <omar.ramirez@ti.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  arch/arm/mm/ioremap.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> For issues related to this:
> http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
> http://article.gmane.org/gmane.linux.ports.sh.devel/8560
> http://www.spinics.net/lists/linux-fbdev/msg01745.html
> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/22271
> 
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index ab50627..a168673 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -202,10 +202,9 @@ void __iomem * __arm_ioremap_pfn_caller(unsigned long pfn,
>  		return NULL;
>  
>  	/*
> -	 * Don't allow RAM to be mapped - this causes problems with ARMv6+
> +	 * This causes problems with ARMv6+. Will be disallowed soon.
maybe specify soon as "before 2.6.37"?

Other than that, Acked-by: Uwe Kleine-König
<u.kleine-koenig@pengutronix.de>
because there is no other solution to make it into the kernel before
.36.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Russell King - ARM Linux]

On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
> Many drivers are broken, and there's no alternative in sight. Such a big
> change should stay as a warning for now, and only later should it
> actually fail.
> 
> The drivers are not doing something correct, we get it, but for now it's
> better to allow them to work (they do 99% of the time anyway) rather
> than to force everyone to revert this patch in their internal trees
> until there's a solution. A slightly broken functionality is better than
> no functionality at all.
> 
> A warning lets people know that what they are doing is not right, and
> they should fix it.

So what are _you_ going to do to fix these drivers?  Continue reverting
this patch?  Or are you just going to ignore the issue entirely?

Unless people can come up with a plan to fix their drivers using ioremap
on system RAM thereby violating the architecture specification, I'm
_not_ going to apply this patch.

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

[Felipe Contreras]

On Thu, Oct 7, 2010 at 10:22 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
>> Many drivers are broken, and there's no alternative in sight. Such a big
>> change should stay as a warning for now, and only later should it
>> actually fail.
>>
>> The drivers are not doing something correct, we get it, but for now it's
>> better to allow them to work (they do 99% of the time anyway) rather
>> than to force everyone to revert this patch in their internal trees
>> until there's a solution. A slightly broken functionality is better than
>> no functionality at all.
>>
>> A warning lets people know that what they are doing is not right, and
>> they should fix it.
>
> So what are _you_ going to do to fix these drivers?  Continue reverting
> this patch?  Or are you just going to ignore the issue entirely?

_I_ don't maintain any of these drivers.

I think when _you_ remove functionality from the architecture, you
should provide a mechanism that drivers can migrate to. Since there's
nothing like that, not even a guideline, you are breaking the drivers
willingly, and expecting other people to fix a difficult problem that
you yourself have no idea how to fix properly. At least for 2.6.36,
and probably 2.6.37, I think it's clear what people will do; revert
that patch in their trees, and think twice before switching to a newer
version.

> Unless people can come up with a plan to fix their drivers using ioremap
> on system RAM thereby violating the architecture specification, I'm
> _not_ going to apply this patch.

And then people wonder why companies don't try to work in upstream as
often as they should. If you, the ARM maintainer doesn't care about
breaking drivers with no warning whatsoever, I wonder why would driver
writers would feel compelled to fix the ARM architecture ASAP (if they
are even capable of doing that), specially if they already missed the
deadline, which had a grace period of between .36-rc1 and .36.

Other projects remove functionality only when there's an alternative
in place, and provide a grace period for the migration. I thought that
was sensible.

-- 
Felipe Contreras

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

[Russell King - ARM Linux]

On Fri, Oct 08, 2010 at 12:32:35PM +0300, Felipe Contreras wrote:
> I think when _you_ remove functionality from the architecture, you
> should provide a mechanism that drivers can migrate to. Since there's
> nothing like that, not even a guideline, you are breaking the drivers
> willingly, and expecting other people to fix a difficult problem that
> you yourself have no idea how to fix properly.

We can either wait for people to complain about silent data corruption
or we can be compliant with the architecture specification.  Which is
better - to avoid data corruption and be correct, or allow a system to
become flakey and corrupt people's data.

What I care about is system correctness and people's data - having
multiple mappings with different attributes is documented in very clear
terms as being 'unpredictable' and therefore it isn't permissible to
allow the practice that worked with previous processors (inherently
due to their cache architecture) to continue forward onto processors
with a different cache architecture.

As already discussed, it's nigh on impossible to unmap the existing
direct mapped region (read the previous discussions about why this is)
- which is precisely why there is no direct alternative solution.

The only possible solution is to exclude some memory at boot time from
the system direct map so that it never appears in the direct map, and
use ioremap on _that_.  Another possible alternative is to use highmem,
obtain highmem pages (making sure that it doesn't fall back to lowmem)
and remap them using interfaces such as vmap.

So there are solutions to the problem, but it seems that _no one_ is
willing to discuss it other than "we want our old way back".

If you want the old way back, apply pressure to silicon vendors and
ARM Ltd to change the architecture to lift this restriction - which
will probably mean doing away with aggressive speculative prefetching
so that it's possible to predict what will be in the cache at any
point in time.

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

[Felipe Contreras]

On Fri, Oct 8, 2010 at 8:53 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Oct 08, 2010 at 12:32:35PM +0300, Felipe Contreras wrote:
>> I think when _you_ remove functionality from the architecture, you
>> should provide a mechanism that drivers can migrate to. Since there's
>> nothing like that, not even a guideline, you are breaking the drivers
>> willingly, and expecting other people to fix a difficult problem that
>> you yourself have no idea how to fix properly.
>
> We can either wait for people to complain about silent data corruption
> or we can be compliant with the architecture specification.  Which is
> better - to avoid data corruption and be correct, or allow a system to
> become flakey and corrupt people's data.

The data corruption would happen only on the memory areas that are
doubly mapped, right? So the misbehavior would only be visible on the
driver. Then what is better? A driver that works 99% of the time, or a
driver that doesn't work at all?

Besides, there are many ARMv6 and ARMv7 devices that are already
shipping with this "wrong" ioremap() and I don't see them blowing up,
so I presume whatever issue is caused by this cannot be so drastic as
to not allow anybody doing this ever from the exact point you found
the issue.

Do you have a test that provides numbers on how often do issues popup,
and under which situations?

> What I care about is system correctness and people's data - having
> multiple mappings with different attributes is documented in very clear
> terms as being 'unpredictable' and therefore it isn't permissible to
> allow the practice that worked with previous processors (inherently
> due to their cache architecture) to continue forward onto processors
> with a different cache architecture.

Of course, it shouldn't be permissible, but you shouldn't just disable
features from one release to the next, each time you find out they are
not proper; there should be some grace period, specially if there's no
alternative.

> As already discussed, it's nigh on impossible to unmap the existing
> direct mapped region (read the previous discussions about why this is)
> - which is precisely why there is no direct alternative solution.
>
> The only possible solution is to exclude some memory at boot time from
> the system direct map so that it never appears in the direct map, and
> use ioremap on _that_.  Another possible alternative is to use highmem,
> obtain highmem pages (making sure that it doesn't fall back to lowmem)
> and remap them using interfaces such as vmap.
>
> So there are solutions to the problem, but it seems that _no one_ is
> willing to discuss it other than "we want our old way back".

People have been discussing them, but you can't expect a perfect
solution to pop up within one release cycle, specially when people
have real issues to deal with.

Be realistic, what is going to happen is that people are going to give
up on having their drivers working on .26, revert the patch, and since
the drivers are already not working, the fix can wait, right? Maybe
after .27, or some other time, a seasoned developer would have time to
get this fixed properly, or they are going to come with workarounds
like manually reserving memory with mem= bootarg:
http://article.gmane.org/gmane.linux.ports.arm.omap/44516

> If you want the old way back, apply pressure to silicon vendors and
> ARM Ltd to change the architecture to lift this restriction - which
> will probably mean doing away with aggressive speculative prefetching
> so that it's possible to predict what will be in the cache at any
> point in time.

Nobody is asking for the old way back, what is being asked is a *grace
period*, have a warning right now, then disable completely later on.

-- 
Felipe Contreras

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

[Andrew Morton]

On Thu, 7 Oct 2010 20:22:45 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
> > Many drivers are broken, and there's no alternative in sight. Such a big
> > change should stay as a warning for now, and only later should it
> > actually fail.
> > 
> > The drivers are not doing something correct, we get it, but for now it's
> > better to allow them to work (they do 99% of the time anyway) rather
> > than to force everyone to revert this patch in their internal trees
> > until there's a solution. A slightly broken functionality is better than
> > no functionality at all.
> > 
> > A warning lets people know that what they are doing is not right, and
> > they should fix it.
> 
> So what are _you_ going to do to fix these drivers?  Continue reverting
> this patch?  Or are you just going to ignore the issue entirely?
> 
> Unless people can come up with a plan to fix their drivers using ioremap
> on system RAM thereby violating the architecture specification, I'm
> _not_ going to apply this patch.

We *do* have a plan: as of 2.6.36, the kernel will emit a WARN_ON trace
when a driver does this.  Offending code will be discovered, developers
will get bug reports from worried users, etc.  This is usually pretty
effective.

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

[Russell King - ARM Linux]

On Fri, Oct 08, 2010 at 10:37:10PM +0300, Felipe Contreras wrote:
> People have been discussing them, but you can't expect a perfect
> solution to pop up within one release cycle, specially when people
> have real issues to deal with.

Two release cycles.  It was queued for the previous release, was posted
to the mailing list, was de-queued, and then re-added for the last merge
window.

That's not no warning - that's almost six months.

http://lists.arm.linux.org.uk/lurker/message/20100408.094818.d6854bd5.en.html

So what you're telling me is that in six months, not one driver has been
touched to address this issue?  So, if no one in that time has done any
work on this, then what use is it going to be making the kernel issue
a warning instead?

So, since this has been known about for six months to the day, I completely
fail to see how making this a warning instead will create the necessary
motivation for the issue to be addressed.

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

[Greg KH]

On Fri, Oct 08, 2010 at 06:53:08PM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 08, 2010 at 12:32:35PM +0300, Felipe Contreras wrote:
> > I think when _you_ remove functionality from the architecture, you
> > should provide a mechanism that drivers can migrate to. Since there's
> > nothing like that, not even a guideline, you are breaking the drivers
> > willingly, and expecting other people to fix a difficult problem that
> > you yourself have no idea how to fix properly.
> 
> We can either wait for people to complain about silent data corruption
> or we can be compliant with the architecture specification.  Which is
> better - to avoid data corruption and be correct, or allow a system to
> become flakey and corrupt people's data.
> 
> What I care about is system correctness and people's data - having
> multiple mappings with different attributes is documented in very clear
> terms as being 'unpredictable' and therefore it isn't permissible to
> allow the practice that worked with previous processors (inherently
> due to their cache architecture) to continue forward onto processors
> with a different cache architecture.
> 
> As already discussed, it's nigh on impossible to unmap the existing
> direct mapped region (read the previous discussions about why this is)
> - which is precisely why there is no direct alternative solution.

Wait, let me get this straight:
  - drivers used to work on 2.6.35
  - some ARM core code changed in .36-rc to fix this iomem problem that
    you found
  - no drivers are notified of the api change as it's a run-time change,
    so the build doesn't break.
  - drivers break when run as the api stops returning valid addresses
  - no known way is around to fix the broken drivers

Um, this doesn't sound like a valid thing to be doing, how do you expect
people to fix their code if they:
	- don't realize it as the api change doesn't break the build
	- there is no way to fix their code

This sounds like a huge regression that should be reverted, or am I
missing something here?

confused,

greg k-h

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

[Greg KH]

On Sat, Oct 09, 2010 at 12:04:51AM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 08, 2010 at 10:37:10PM +0300, Felipe Contreras wrote:
> > People have been discussing them, but you can't expect a perfect
> > solution to pop up within one release cycle, specially when people
> > have real issues to deal with.
> 
> Two release cycles.  It was queued for the previous release, was posted
> to the mailing list, was de-queued, and then re-added for the last merge
> window.
> 
> That's not no warning - that's almost six months.
> 
> http://lists.arm.linux.org.uk/lurker/message/20100408.094818.d6854bd5.en.html
> 
> So what you're telling me is that in six months, not one driver has been
> touched to address this issue?  So, if no one in that time has done any
> work on this, then what use is it going to be making the kernel issue
> a warning instead?
> 
> So, since this has been known about for six months to the day, I completely
> fail to see how making this a warning instead will create the necessary
> motivation for the issue to be addressed.

I doubt that people even noticed that this was a problem.

Unless you throw a run-time warning at them, or even better, break the
build of their drivers, they will not notice.

Also, how can we fix this in a driver, what is the proper steps to do
so?

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Fri, Oct 08, 2010 at 04:25:39PM -0700, Greg KH wrote:
> I doubt that people even noticed that this was a problem.

So what you're telling me is that ARM SoC people don't read the
ARM kernel mailing list?  I might as well stop posting patches
if no one's paying attention then! :-P

Reality is that it was discussed, and I did propose solutions at
the time.

> Unless you throw a run-time warning at them, or even better, break the
> build of their drivers, they will not notice.
> 
> Also, how can we fix this in a driver, what is the proper steps to do
> so?

On April 23rd:
| I think a viable safe solution is to set aside some RAM at boot (which
| the kernel doesn't manage at all) and then use ioremap on that; that
| approach will still work with this patch in place.

On April 30th when discussing the omapfb driver:
| There's really one option for this - in the machine's fixup handler, you
| can walk the meminfo array and kick out some memory from that.  This will
| prevent the kernel mapping that memory and make pfn_valid() fail for that
| memory range.  Another advantage of this is that it won't break when we
| switch to LMB.

So, as I say, there's been six months.  It was discussed.  But still
we find that the drivers haven't been touched and now we're complaining
that drivers are breaking.

If it hadn't been discussed, if solutions hadn't been proposed, then
yes, it would be right to revert it out-right.  But that's not what
happened.

If we have to have another three months (or so), this time with a warning,
then so be it, but let's make it plainly clear that it _will_ _definitely_
be changing, and that drivers _will_ break unless they are fixed.

Unfortunately, what I fear is that nothing will happen because people
want the ioremap-on-system-RAM to just work, and then we'll hit this
exact same issue again in three months time.

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

[Greg KH]

On Sat, Oct 09, 2010 at 12:44:48AM +0100, Russell King - ARM Linux wrote:
> On Fri, Oct 08, 2010 at 04:25:39PM -0700, Greg KH wrote:
> > I doubt that people even noticed that this was a problem.
> 
> So what you're telling me is that ARM SoC people don't read the
> ARM kernel mailing list?  I might as well stop posting patches
> if no one's paying attention then! :-P
> 
> Reality is that it was discussed, and I did propose solutions at
> the time.

Heh, fair enough

> > Unless you throw a run-time warning at them, or even better, break the
> > build of their drivers, they will not notice.
> > 
> > Also, how can we fix this in a driver, what is the proper steps to do
> > so?
> 
> On April 23rd:
> | I think a viable safe solution is to set aside some RAM at boot (which
> | the kernel doesn't manage at all) and then use ioremap on that; that
> | approach will still work with this patch in place.
> 
> On April 30th when discussing the omapfb driver:
> | There's really one option for this - in the machine's fixup handler, you
> | can walk the meminfo array and kick out some memory from that.  This will
> | prevent the kernel mapping that memory and make pfn_valid() fail for that
> | memory range.  Another advantage of this is that it won't break when we
> | switch to LMB.

Ah, but no drivers have been fixed in any of these ways yet?

> So, as I say, there's been six months.  It was discussed.  But still
> we find that the drivers haven't been touched and now we're complaining
> that drivers are breaking.
> 
> If it hadn't been discussed, if solutions hadn't been proposed, then
> yes, it would be right to revert it out-right.  But that's not what
> happened.
> 
> If we have to have another three months (or so), this time with a warning,
> then so be it, but let's make it plainly clear that it _will_ _definitely_
> be changing, and that drivers _will_ break unless they are fixed.
> 
> Unfortunately, what I fear is that nothing will happen because people
> want the ioremap-on-system-RAM to just work, and then we'll hit this
> exact same issue again in three months time.

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.

Any pointers to patches where people have fixed up the drivers?

thanks,

greg k-h

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

[Russell King - ARM Linux]

On Sat, Oct 09, 2010 at 12:44:48AM +0100, Russell King - ARM Linux wrote:
> So, as I say, there's been six months.  It was discussed.  But still
> we find that the drivers haven't been touched and now we're complaining
> that drivers are breaking.
> 
> If it hadn't been discussed, if solutions hadn't been proposed, then
> yes, it would be right to revert it out-right.  But that's not what
> happened.
> 
> If we have to have another three months (or so), this time with a warning,
> then so be it, but let's make it plainly clear that it _will_ _definitely_
> be changing, and that drivers _will_ break unless they are fixed.
> 
> Unfortunately, what I fear is that nothing will happen because people
> want the ioremap-on-system-RAM to just work, and then we'll hit this
> exact same issue again in three months time.

There is another solution to this which will be architecturally compliant
- as we can detect system ram ioremaps, we can force them to have the
same memory type, sharability and cache attributes as the existing
mappings rather than merely failing them.  But this is not what drivers
want.

The reason they play the ioremap-system-RAM game is to get around the
DMA coherence issues - rather than using the DMA API, because that
suffers from being incapable of dealing with large contiguous chunks
of memory.

Anyway, to fill other observers in, the issue here is:
- ARMv6 and above have weak memory ordering models.

- ARMv6 and above can speculatively prefetch from any region which has
  a 'normal memory' type.  As further hardware revisions are released,
  the speculative prefetch becomes progressively more aggressive.

- multiple mappings of the same physical address region with differing
  memory type (strongly ordered, device, normal memory) becomes
  unpredictable.  The memory type partly defines which reads/writes are
  allowed to bypass other reads/writes.  Unpredictable here means that
  there is no guarantee whether the access performed via the mapping
  you've created will be done as per the memory type specified in that
  mapping.

- multiple mappings of the same physical address region with differing
  sharability attributes have been observed to cause systems to crash/hang,
  but fall under the 'unpredictable' behaviour - which basically means
  you don't know if the coherence hardware will be involved in the
  access.

- multiple mappings of the same physical address region with differing
  cache attributes is also unpredictable - you can't guarantee whether
  the access will be performed using the cache attributes through the
  mapping you're performing the access through.

In the case of system memory, this is normally mapped as 'normal memory'
with write-back cache.  On uniprocessor systems, this is mapped as
non-shared memory.

ioremap() creates 'device' type mappings, which are marked as shared
(some devices, the shared-ness is used as another address bit!)

So, permitting ioremap of system RAM violates all three - which means
there is no guarantee of ordering, sharedness or cache behaviour via
mappings which alias with differing attributes.

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

[Russell King - ARM Linux]

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.

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

[Felipe Contreras]

2010/10/9 Greg KH <greg@kroah.com>:
> On Sat, Oct 09, 2010 at 12:04:51AM +0100, Russell King - ARM Linux wrote:
>> So, since this has been known about for six months to the day, I completely
>> fail to see how making this a warning instead will create the necessary
>> motivation for the issue to be addressed.
>
> I doubt that people even noticed that this was a problem.
>
> Unless you throw a run-time warning at them, or even better, break the
> build of their drivers, they will not notice.

The run-time warning is there, and at the same time the ioremap()
fails, but this has never been into any release (certainly not there
in .35).

IMO the vast majority of people only try to run final releases, and
they will only see the warning on .36. I don't think _all_ ARM users
are expected to follow every patch sent on the linux-arm-kernel
mailing list, nor to try the -rc series. In fact, I doubt many try the
final releases that closely, maybe one yes, one no.

-- 
Felipe Contreras

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

[Greg KH]

On Sat, Oct 09, 2010 at 01:25:47AM +0100, 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.

Well, how about throw a big WARN() to let people know that their driver
needs to be fixed?  That will give them the chance, right?

thanks,

greg k-h

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

[Felipe Contreras]

2010/10/9 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> On Fri, Oct 08, 2010 at 04:25:39PM -0700, Greg KH wrote:
>> Also, how can we fix this in a driver, what is the proper steps to do
>> so?
>
> On April 23rd:
> | I think a viable safe solution is to set aside some RAM at boot (which
> | the kernel doesn't manage at all) and then use ioremap on that; that
> | approach will still work with this patch in place.
>
> On April 30th when discussing the omapfb driver:
> | There's really one option for this - in the machine's fixup handler, you
> | can walk the meminfo array and kick out some memory from that.  This will
> | prevent the kernel mapping that memory and make pfn_valid() fail for that
> | memory range.  Another advantage of this is that it won't break when we
> | switch to LMB.

These are not solutions, these are pointers to find the solutions.

How do you set aside some RAM at boot for that? Is there anything like
memblock to do that? Or do you expect to set aside some memory with
mem=X, and manually specify the address in the .config?

> Unfortunately, what I fear is that nothing will happen because people
> want the ioremap-on-system-RAM to just work, and then we'll hit this
> exact same issue again in three months time.

Nobody has ever said, or suggested anything like that.

What people want is:
1) A solution in place
2) Time to implement that solution in their drivers

And since you did, let me say what I fear: that many more people will
find drivers totally broken on 2.6.36.

-- 
Felipe Contreras

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

[Nicolas Pitre]

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;

Nicolas

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

[Greg KH]

On Fri, Oct 08, 2010 at 10:41:42PM -0400, 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;
> 

That looks good to me, anyone else object to this?

Now we also need a way to fix the drivers for real on ARMv6+ now...

thanks,

greg k-h

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

[Nicolas Pitre]

On Fri, 8 Oct 2010, Greg KH wrote:

> On Fri, Oct 08, 2010 at 06:53:08PM +0100, Russell King - ARM Linux wrote:
> > On Fri, Oct 08, 2010 at 12:32:35PM +0300, Felipe Contreras wrote:
> > > I think when _you_ remove functionality from the architecture, you
> > > should provide a mechanism that drivers can migrate to. Since there's
> > > nothing like that, not even a guideline, you are breaking the drivers
> > > willingly, and expecting other people to fix a difficult problem that
> > > you yourself have no idea how to fix properly.
> > 
> > We can either wait for people to complain about silent data corruption
> > or we can be compliant with the architecture specification.  Which is
> > better - to avoid data corruption and be correct, or allow a system to
> > become flakey and corrupt people's data.
> > 
> > What I care about is system correctness and people's data - having
> > multiple mappings with different attributes is documented in very clear
> > terms as being 'unpredictable' and therefore it isn't permissible to
> > allow the practice that worked with previous processors (inherently
> > due to their cache architecture) to continue forward onto processors
> > with a different cache architecture.
> > 
> > As already discussed, it's nigh on impossible to unmap the existing
> > direct mapped region (read the previous discussions about why this is)
> > - which is precisely why there is no direct alternative solution.
> 
> Wait, let me get this straight:
>   - drivers used to work on 2.6.35

Possibly on pre ARMv6 only.  On ARMv6 and above, the discussed behavior 
_never_ worked.  It is fundamental to the CPU design.

>   - some ARM core code changed in .36-rc to fix this iomem problem that
>     you found

Nothing was fixed.  There is nothing to fix.  Simply that a wrong driver 
assumption about the hardware capability was prohibited by the API.

>   - no drivers are notified of the api change as it's a run-time change,
>     so the build doesn't break.

There is no API change.  Simply an API misuse.  Granted, on pre ARMv6 
this usage used to work, but on ARMv6 and above this never was supported 
at all by the hardware.  The fact that the software has let it through 
was a gross mistake, period.

>   - drivers break when run as the api stops returning valid addresses

Such drivers on ARMv6 should *never* have worked in the first place.  
When they did "work", they corrupted memory.

>   - no known way is around to fix the broken drivers

That's B/s.  As Russell said, there are known solutions that were 
proposed at least six months ago.

> Um, this doesn't sound like a valid thing to be doing,

Excuse me, but preventing data corruption is a damn valid thing to do.  
The real problem is that people kept on abusing the API for something 
that the hardware simply can't support, and the mere fact that things 
_appears_ to just work by accident in 98% of the cases encourage people 
to simply put their head in the sand and ignore the issue.  Sorry folks, 
but that's not how computing works.  We're not amateurs for damn sake, 
and when the ARM bible says this operation is UNSUPPORTED, it is 
UNSUPPORTED.  Seeing people trying to fsck around the issue because of 
laziness is revolting!

> how do you expect
> people to fix their code if they:
> 	- don't realize it as the api change doesn't break the build

The API should have complained the very first time they attempted the 
operation in question.  It should never have worked.  I'm sorry but the 
mere fact that the broken driver didn't receive an error before is no 
excuse to *knowingly* let memory corruption go on.

> 	- there is no way to fix their code

Wrong.  See above.

> This sounds like a huge regression that should be reverted, or am I
> missing something here?

I'm afraid you are totally missing the point.

The patch Russell introduced simply prevented an abuse of the API for 
which there are documented evidences that such abuse is a source of 
memory corruption.  This must not be allowed any longer, not even for a 
warning period.  There is no regression what so ever as those driver 
were always broken.  And I much prefer to have brokenness in the form of 
driver initialization error rather than silent and random memory 
corruptions.

So folks please get real and understand that Russell was right.

The only compromise that may be made is to allow the API abuse on pre 
ARMv6 implementations where this has no adverse effect, which is the 
patch I just posted.  Granted, on pre ARMv6, the ioremap of system RAM 
was fine and people legitimately relied on it in their drivers.  But 
this just can't be allowed anymore on ARMv6.


Nicolas

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

[Russell King - ARM Linux]

On Sat, Oct 09, 2010 at 03:45:16AM +0300, Felipe Contreras wrote:
> 2010/10/9 Greg KH <greg@kroah.com>:
> > On Sat, Oct 09, 2010 at 12:04:51AM +0100, Russell King - ARM Linux wrote:
> >> So, since this has been known about for six months to the day, I completely
> >> fail to see how making this a warning instead will create the necessary
> >> motivation for the issue to be addressed.
> >
> > I doubt that people even noticed that this was a problem.
> >
> > Unless you throw a run-time warning at them, or even better, break the
> > build of their drivers, they will not notice.
> 
> The run-time warning is there, and at the same time the ioremap()
> fails, but this has never been into any release (certainly not there
> in .35).

I've already described the background behind the lifecycle of this
change.

> IMO the vast majority of people only try to run final releases, and
> they will only see the warning on .36. I don't think _all_ ARM users
> are expected to follow every patch sent on the linux-arm-kernel
> mailing list, nor to try the -rc series. In fact, I doubt many try the
> final releases that closely, maybe one yes, one no.

If you have only just noticed it, you have tried zero -rc releases
during this cycle.  What's the point of having -rc releases if no
one in the ARM community tests them?

We might as well push our development stuff out just before Linus
releases a final release if the majority of ARM folk think that -rc
releases are not something they should be testing.

Quite the opposite, -rc releases are _important_ to test to ensure that
the quality of the final kernel is reasonable.  You can't rely on me
being able to test every damned ARM platform, or Tony to test every OMAP
platform.  As a developer, that's _your_ job to ensure that regressions
are found on the setup _you_ have.

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

[Russell King - ARM Linux]

On Sat, Oct 09, 2010 at 03:56:23AM +0300, Felipe Contreras wrote:
> 2010/10/9 Russell King - ARM Linux <linux@arm.linux.org.uk>:
> > On Fri, Oct 08, 2010 at 04:25:39PM -0700, Greg KH wrote:
> >> Also, how can we fix this in a driver, what is the proper steps to do
> >> so?
> >
> > On April 23rd:
> > | I think a viable safe solution is to set aside some RAM at boot (which
> > | the kernel doesn't manage at all) and then use ioremap on that; that
> > | approach will still work with this patch in place.
> >
> > On April 30th when discussing the omapfb driver:
> > | There's really one option for this - in the machine's fixup handler, you
> > | can walk the meminfo array and kick out some memory from that.  This will
> > | prevent the kernel mapping that memory and make pfn_valid() fail for that
> > | memory range.  Another advantage of this is that it won't break when we
> > | switch to LMB.
> 
> These are not solutions, these are pointers to find the solutions.
> 
> How do you set aside some RAM at boot for that? Is there anything like
> memblock to do that? Or do you expect to set aside some memory with
> mem=X, and manually specify the address in the .config?

Which bit of "walk the meminfo array and kick out some memory from that"
says "use memblock" or "use mem=X" ?

static unsigned long reserve_mem(struct meminfo *mi, unsigned long size)
{
	unsigned long addr = ~0;
	int i;

	for (i = mi->nr_banks - 1; i >= 0; i--)
		if (mi->bank[i].size >= size) {
			mi->bank[i].size -= size;
			addr = mi->bank[i].start + mi->bank[i].size;
			break;
		}

	return addr;
}

static void __init my_fixup(struct machine_desc *desc, struct tag *tags,
			    char **cmdline, struct meminfo *mi)
{
	omapfb_buffer_phys = reserve_mem(mi, 32*1048576);
	if (omapfb_buffer_phys == ~0)
		pr_warn("Unable to allocate omapfb buffer\n");
}

Then later:

	omapfb_buffer = ioremap(omapfb_buffer_phys, 32*1048576);

That's a damned simple and direct implementation of exactly what I
described, and results in something which is much more architecturally
correct than what's going on today.

> > Unfortunately, what I fear is that nothing will happen because people
> > want the ioremap-on-system-RAM to just work, and then we'll hit this
> > exact same issue again in three months time.
> 
> Nobody has ever said, or suggested anything like that.
> 
> What people want is:
> 1) A solution in place
> 2) Time to implement that solution in their drivers

Six months, and by your own emails, everyone reverting the change rather
than fixing producing their drivers.  That says it all about what kind
of attitude driver writers have towards architectural issues.

If they've been reverting the change, then they _do_ know about the issue,
so why have there been _zero_ patches from these people who are reverting
the change?  Maybe you should be asking these people why they haven't done
any work to fix the drivers when one of the solutions starts off as about
15 lines of code.

> And since you did, let me say what I fear: that many more people will
> find drivers totally broken on 2.6.36.

I've reverted the change, so now you can go back to violating the
requirements of the architecture and have your data silently corrupted.

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 6:04 AM, Greg KH <greg@kroah.com> wrote:
> On Fri, Oct 08, 2010 at 10:41:42PM -0400, Nicolas Pitre wrote:
>> 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;
>>
>
> That looks good to me, anyone else object to this?

I object; this is still breaking drivers.

The decision was to don't allow ioremap() on RAM for all ARM, I agree
with that decision, there's no point in an API to works differently on
different ARM chips. Moreover, this would be more in line with x86's
ioremap().

All that is needed IMO is a grace period... Before this is activated
for real, the warning should happen on all drivers.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 6:36 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Fri, 8 Oct 2010, Greg KH wrote:
>> Wait, let me get this straight:
>>   - drivers used to work on 2.6.35
>
> Possibly on pre ARMv6 only.  On ARMv6 and above, the discussed behavior
> _never_ worked.  It is fundamental to the CPU design.

That's not true, drivers on ARMv6 and above do work. I still wonder
how exactly to trigger the issue, and how often does this happen,
because I've never seen it.

I trust the experts that it is an issue, but I don't agree that
drivers that are currently working on ARMv6+ should be broken on
purpose right away.

>>   - some ARM core code changed in .36-rc to fix this iomem problem that
>>     you found
>
> Nothing was fixed.  There is nothing to fix.  Simply that a wrong driver
> assumption about the hardware capability was prohibited by the API.

Not exactly. There is supposedly a problem on ARMv6+, there must be a
mechanism to avoid this problem, all drivers should migrate to that,
or a different on, so that we have a sane API.

>>   - no drivers are notified of the api change as it's a run-time change,
>>     so the build doesn't break.
>
> There is no API change.  Simply an API misuse.  Granted, on pre ARMv6
> this usage used to work, but on ARMv6 and above this never was supported
> at all by the hardware.  The fact that the software has let it through
> was a gross mistake, period.

No. It worked just fine, now it's doesn't, that's a change in API
behavior, which is also part of the API. It might not be ideal, or
proper, but it does work.

>>   - drivers break when run as the api stops returning valid addresses
>
> Such drivers on ARMv6 should *never* have worked in the first place.
> When they did "work", they corrupted memory.

They corrupted their own memory, right? And on _very_ rare occasions I
presume, since I've never seen it. If so, there's no damage in letting
them work at least one more release (2.6.36).

>>   - no known way is around to fix the broken drivers
>
> That's B/s.  As Russell said, there are known solutions that were
> proposed at least six months ago.

You seriously think most people out there know how to "set aside some
RAM at boot time"? I don't, otherwise the drivers would be fixed by
now.

>> Um, this doesn't sound like a valid thing to be doing,
>
> Excuse me, but preventing data corruption is a damn valid thing to do.
> The real problem is that people kept on abusing the API for something
> that the hardware simply can't support, and the mere fact that things
> _appears_ to just work by accident in 98% of the cases encourage people
> to simply put their head in the sand and ignore the issue.  Sorry folks,
> but that's not how computing works.  We're not amateurs for damn sake,
> and when the ARM bible says this operation is UNSUPPORTED, it is
> UNSUPPORTED.  Seeing people trying to fsck around the issue because of
> laziness is revolting!

Have you ever seen this corruption? How often does this happen? So, a
driver would generate corruption in it's own data and your solution is
to make the driver stop working? That's going from 98% to 0% in one
release.

There's many people out there that have realized these issues, and the
only thing they would see is their driver broken on .36. How about
instead we just warn for now? What's wrong with that? There have been
many releases with the "wrong" ioremap(), one more would not hurt
anybody.

>> how do you expect
>> people to fix their code if they:
>>       - don't realize it as the api change doesn't break the build
>
> The API should have complained the very first time they attempted the
> operation in question.  It should never have worked.  I'm sorry but the
> mere fact that the broken driver didn't receive an error before is no
> excuse to *knowingly* let memory corruption go on.

Yes, it should have, but it didn't. Now people are relying on that
behavior, so, first you deprecate that usage with WARN_ON, and later
you remove that functionality.

>>       - there is no way to fix their code
>
> Wrong.  See above.
>
>> This sounds like a huge regression that should be reverted, or am I
>> missing something here?
>
> I'm afraid you are totally missing the point.
>
> The patch Russell introduced simply prevented an abuse of the API for
> which there are documented evidences that such abuse is a source of
> memory corruption.  This must not be allowed any longer, not even for a
> warning period.  There is no regression what so ever as those driver
> were always broken.  And I much prefer to have brokenness in the form of
> driver initialization error rather than silent and random memory
> corruptions.

People have different definitions of "broken"; working 99% of the time
is good enough for many people, specially compared to 0%.

So you found an issue on ARMv6+, great, let's warn the users so that
they know the issue exists, hopefully a generic mechanism to fix this
will come about for .37 so that some drivers can migrate to that, and
then truly disable the functionality.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 12:21 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Oct 09, 2010 at 03:56:23AM +0300, Felipe Contreras wrote:
>> 2010/10/9 Russell King - ARM Linux <linux@arm.linux.org.uk>:
>> > On Fri, Oct 08, 2010 at 04:25:39PM -0700, Greg KH wrote:
>> >> Also, how can we fix this in a driver, what is the proper steps to do
>> >> so?
>> >
>> > On April 23rd:
>> > | I think a viable safe solution is to set aside some RAM at boot (which
>> > | the kernel doesn't manage at all) and then use ioremap on that; that
>> > | approach will still work with this patch in place.
>> >
>> > On April 30th when discussing the omapfb driver:
>> > | There's really one option for this - in the machine's fixup handler, you
>> > | can walk the meminfo array and kick out some memory from that.  This will
>> > | prevent the kernel mapping that memory and make pfn_valid() fail for that
>> > | memory range.  Another advantage of this is that it won't break when we
>> > | switch to LMB.
>>
>> These are not solutions, these are pointers to find the solutions.
>>
>> How do you set aside some RAM at boot for that? Is there anything like
>> memblock to do that? Or do you expect to set aside some memory with
>> mem=X, and manually specify the address in the .config?
>
> Which bit of "walk the meminfo array and kick out some memory from that"
> says "use memblock" or "use mem=X" ?
>
> static unsigned long reserve_mem(struct meminfo *mi, unsigned long size)
> {
>        unsigned long addr = ~0;
>        int i;
>
>        for (i = mi->nr_banks - 1; i >= 0; i--)
>                if (mi->bank[i].size >= size) {
>                        mi->bank[i].size -= size;
>                        addr = mi->bank[i].start + mi->bank[i].size;
>                        break;
>                }
>
>        return addr;
> }
>
> static void __init my_fixup(struct machine_desc *desc, struct tag *tags,
>                            char **cmdline, struct meminfo *mi)
> {
>        omapfb_buffer_phys = reserve_mem(mi, 32*1048576);
>        if (omapfb_buffer_phys == ~0)
>                pr_warn("Unable to allocate omapfb buffer\n");
> }
>
> Then later:
>
>        omapfb_buffer = ioremap(omapfb_buffer_phys, 32*1048576);
>
> That's a damned simple and direct implementation of exactly what I
> described, and results in something which is much more architecturally
> correct than what's going on today.

This is only for omapfb, isn't it? Or is this a generic solution?

If it's a generic solution can we put this into arch/arm/mm? What is
simple and direct for you is something that would take a long long
time for other people to come with (if ever).

>> > Unfortunately, what I fear is that nothing will happen because people
>> > want the ioremap-on-system-RAM to just work, and then we'll hit this
>> > exact same issue again in three months time.
>>
>> Nobody has ever said, or suggested anything like that.
>>
>> What people want is:
>> 1) A solution in place
>> 2) Time to implement that solution in their drivers
>
> Six months, and by your own emails, everyone reverting the change rather
> than fixing producing their drivers.  That says it all about what kind
> of attitude driver writers have towards architectural issues.
>
> If they've been reverting the change, then they _do_ know about the issue,
> so why have there been _zero_ patches from these people who are reverting
> the change?  Maybe you should be asking these people why they haven't done
> any work to fix the drivers when one of the solutions starts off as about
> 15 lines of code.

No, I said people _will_ revert the patch. I'm pretty sure the vast
majority of people haven't even realized this change yet.

What you should have done IMO is only warn first. This would have had
no problem getting into 2.6.35, as it doesn't break anything, then
most people would have noticed this by now.

Also, what do you think is the right attitude? Do you think *all*
driver writers should follow each and every patch on the mailing list
or always try release candidates? Even if they all did (some do),
there's no generic solution, so they all are scratching their heads
about how to solve this. It might be crystal clear for you how such
generic solution could be implemented, or perhaps how some of these
drivers can be fixed individually, if so, why don't you come with a
proposal to mitigate the pain of fixing such drivers.

If you don't have time to write such a generic solution, that's fine,
that's why my proposal is to only warn for now.

I think saying "hey, it turns out you were using the API wrongly, I am
officially totally breaking you all, and I'm not bothering with
providing an example of what you should be doing instead, also, you
should follow all the patches in linux-arm-kernel, and try the release
candidates, because you should expect to get broken at any time" is
the wrong attitude.

>> And since you did, let me say what I fear: that many more people will
>> find drivers totally broken on 2.6.36.
>
> I've reverted the change, so now you can go back to violating the
> requirements of the architecture and have your data silently corrupted.

This is unreasonable, we need the warning at least on one release.

-- 
Felipe Contreras

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

[Arnd Bergmann]

On Saturday 09 October 2010 12:28:19 Felipe Contreras wrote:
> Also, what do you think is the right attitude? Do you think all
> driver writers should follow each and every patch on the mailing list
> or always try release candidates? Even if they all did (some do),
> there's no generic solution, so they all are scratching their heads
> about how to solve this. It might be crystal clear for you how such
> generic solution could be implemented, or perhaps how some of these
> drivers can be fixed individually, if so, why don't you come with a
> proposal to mitigate the pain of fixing such drivers.

Please put this into perspective, it's not like this was ever a
normal thing to do, and drivers doing an ioremap on kernel memory
would typically not make it through a review. I've looked through
the ARM specific drivers that do ioremap and practically all of
them just map their MMIO registers as they should.

The few drivers that may be hit by this are typically in drivers/staging
exactly because issues like this have not been fixed yet.

We should probably just fix the non-staging drivers that are hit by
this now and declare the issue done.

When you say that "many drivers broken", can you list the ones you know
about? It would probably help resolve this the right way.

	Arnd

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

[Dave Airlie]

>
> The few drivers that may be hit by this are typically in drivers/staging
> exactly because issues like this have not been fixed yet.
>
> We should probably just fix the non-staging drivers that are hit by
> this now and declare the issue done.
>
> When you say that "many drivers broken", can you list the ones you know
> about? It would probably help resolve this the right way.

I'm guessing some closed source graphics drivers for ARM do all kinds
of wrong crap, and Nokia use them a lot, and nobody wants to tell the
closed vendors to change their drivers because it costs money.

Dave.

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

[Uwe Kleine-König]

Hello,

On Sat, Oct 09, 2010 at 01:11:26PM +0200, Arnd Bergmann wrote:
> When you say that "many drivers broken", can you list the ones you know
> about? It would probably help resolve this the right way.
I know about the camera stuff on mx3/pcm037.  See
pcm037_camera_alloc_dma in arch/arm/mach-mx3/mach-pcm037.c.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Christoph Hellwig]

On Sat, Oct 09, 2010 at 09:43:50PM +1000, Dave Airlie wrote:
> I'm guessing some closed source graphics drivers for ARM do all kinds
> of wrong crap, and Nokia use them a lot, and nobody wants to tell the
> closed vendors to change their drivers because it costs money.

In which case Russell's message to tell them to sodd off is exactly the
right thing to do.  I've never figured out why Felipe cars so much, but
if he's one of the users of utterly buggy binary drivers that would
explain his weird actions here.

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

[Felipe Contreras]

2010/10/9 Arnd Bergmann <arnd@arndb.de>:
> On Saturday 09 October 2010 12:28:19 Felipe Contreras wrote:
>> Also, what do you think is the right attitude? Do you think all
>> driver writers should follow each and every patch on the mailing list
>> or always try release candidates? Even if they all did (some do),
>> there's no generic solution, so they all are scratching their heads
>> about how to solve this. It might be crystal clear for you how such
>> generic solution could be implemented, or perhaps how some of these
>> drivers can be fixed individually, if so, why don't you come with a
>> proposal to mitigate the pain of fixing such drivers.
>
> Please put this into perspective, it's not like this was ever a
> normal thing to do, and drivers doing an ioremap on kernel memory
> would typically not make it through a review. I've looked through
> the ARM specific drivers that do ioremap and practically all of
> them just map their MMIO registers as they should.
>
> The few drivers that may be hit by this are typically in drivers/staging
> exactly because issues like this have not been fixed yet.
>
> We should probably just fix the non-staging drivers that are hit by
> this now and declare the issue done.
>
> When you say that "many drivers broken", can you list the ones you know
> about? It would probably help resolve this the right way.

I know of tidspbridge, in my original mail I listed some links, one
which mentions: sh_mobile_ceu_camera, maybe i.MX31 users of the
mx3_camera driver: pcm037 and mx31moboard.

http://article.gmane.org/gmane.linux.ports.sh.devel/8560

>From the looks of it, PowerVR SGX seems like it will be broken too:
http://meego.gitorious.org/meego-os-base/kernel-source/trees/master/patches

Other people have been asking questions without mentioning specific drivers too:
http://www.spinics.net/lists/linux-fbdev/msg01745.html

But my guess is that most of the issues would be found after releasing
.36, and of course the current issues cannot possibly be fixed in that
time-frame.

Anyway, Russell said he already reverted the change, which I don't
think is ideal, we need the warning, then I think many more will pop
up.

-- 
Felipe Contreras

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

[Russell King - ARM Linux]

On Sat, Oct 09, 2010 at 01:44:44PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Sat, Oct 09, 2010 at 01:11:26PM +0200, Arnd Bergmann wrote:
> > When you say that "many drivers broken", can you list the ones you know
> > about? It would probably help resolve this the right way.
> I know about the camera stuff on mx3/pcm037.  See
> pcm037_camera_alloc_dma in arch/arm/mach-mx3/mach-pcm037.c.

dma_alloc_coherent, stuffing that into dma_declare_coherent_memory,
which then ioremaps the memory obtained from dma_alloc_coherent, and
is then handed out via a subsequent dma_alloc_coherent call.

So what we end up with is the kernel mapping (normal memory, cacheable),
the DMA coherent mapping (normal memory, non-cacheable) but with the
same shared-ness as the kernel mapping, and finally the ioremap mapping
(device, shared).  So three aliasing mappings all with different
attributes.

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 2:43 PM, Dave Airlie <airlied@gmail.com> wrote:
>> The few drivers that may be hit by this are typically in drivers/staging
>> exactly because issues like this have not been fixed yet.
>>
>> We should probably just fix the non-staging drivers that are hit by
>> this now and declare the issue done.
>>
>> When you say that "many drivers broken", can you list the ones you know
>> about? It would probably help resolve this the right way.
>
> I'm guessing some closed source graphics drivers for ARM do all kinds
> of wrong crap, and Nokia use them a lot, and nobody wants to tell the
> closed vendors to change their drivers because it costs money.

You are over simplifying things.

 1) The code is open[1]
 2) the decision comes from TI, Nokia can only do so much before shipping
 3) I know many people in Nokia have pushed very hard, and cleaned up
the driver a lot themselves
 4) There are other companies, Intel is using SGX too

Anyway, there are other examples, like tidspbridge, which Nokia has
spent a lot of time cleaning up, and TI made a great effort to push
into staging. And others already in mainline, like
sh_mobile_ceu_camera.

[1] http://meego.gitorious.org/meego-os-base/kernel-source/trees/master/patches

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 2:55 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Sat, Oct 09, 2010 at 09:43:50PM +1000, Dave Airlie wrote:
>> I'm guessing some closed source graphics drivers for ARM do all kinds
>> of wrong crap, and Nokia use them a lot, and nobody wants to tell the
>> closed vendors to change their drivers because it costs money.
>
> In which case Russell's message to tell them to sodd off is exactly the
> right thing to do.  I've never figured out why Felipe cars so much, but
> if he's one of the users of utterly buggy binary drivers that would
> explain his weird actions here.

If you mean PVR SGX, no, I don't use it myself, and it's not a binary
driver, the code is open:
http://meego.gitorious.org/meego-os-base/kernel-source/trees/master/patches

The one I use is tidspbridge which is in staging.

But that's not relevant, what is relevant is that is that the API was
broken without a warning grace period in final releases. "Good"
drivers are broken, I suspect many that haven't made into mainline yet
will receive a bad surprise. Making this only a warning on .36 doesn't
hurt anybody, does it?

-- 
Felipe Contreras

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

[Russell King - ARM Linux]

On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
> For issues related to this:
> http://article.gmane.org/gmane.linux.ports.arm.kernel/84454

This one nicely shows some of the problems which can occur with the
memory type attributes - and this is not attributable to ioremap().

ioremap() is used to map devices.  It creates device memory type mappings.
If what you're mapping doesn't support device memory type mappings, then
accesses via an ioremap()'d region isn't going to work - as this guy is
observing.

That's not because ioremap() is doing something wrong.  It's doing what
it's meant to do.  The use is wrong, and is completely unrelated to the
issue you've raised.

> http://article.gmane.org/gmane.linux.ports.sh.devel/8560

This one we know about, and as I've already said, it ends up with three
aliasing mappings each with different attributes thusly:

	cpu = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
	dma_declare_coherent_memory(dev, dma, dma, size, DMA_MEMORY_MAP);
		==> ioremap(dma, size);
	...
	dma_alloc_coherent(dev, ...);

This wasn't spotted in the review of sh-mobile code because it's not part
of the sh-mobile code base, but some of the generic sh architecture code.
sh-mobile went into the kernel on March 12th, so it does pre-date the
change to ioremap, and is therefore technically a regression.

However, as can be seen from the link above, it's been known about since
8th August - two months ago.  The problem has been discussed, and we had
a good solution which would work.  But then an oar got thrown in which
basically resulted in that solution being rejected - on the basis that
"it's an established API and it must work".

Well, this usage of the API doesn't work on x86!

The result - progress on the issue hit a brick wall and is unable to
proceed because of personal viewpoints conflicting with reality.

> http://www.spinics.net/lists/linux-fbdev/msg01745.html

External user?  Unreviewed code?  You can't seriously be suggesting
that we should care about code we haven't seen which is sitting
externally to the kernel tree, and this is a valid reason to hold
off on changes to the kernel.

> http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/22271

"No file".

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

[Russell King - ARM Linux]

On Sat, Oct 09, 2010 at 03:10:57PM +0300, Felipe Contreras wrote:
> On Sat, Oct 9, 2010 at 2:43 PM, Dave Airlie <airlied@gmail.com> wrote:
> >> The few drivers that may be hit by this are typically in drivers/staging
> >> exactly because issues like this have not been fixed yet.
> >>
> >> We should probably just fix the non-staging drivers that are hit by
> >> this now and declare the issue done.
> >>
> >> When you say that "many drivers broken", can you list the ones you know
> >> about? It would probably help resolve this the right way.
> >
> > I'm guessing some closed source graphics drivers for ARM do all kinds
> > of wrong crap, and Nokia use them a lot, and nobody wants to tell the
> > closed vendors to change their drivers because it costs money.
> 
> You are over simplifying things.
> 
>  1) The code is open[1]
>  2) the decision comes from TI, Nokia can only do so much before shipping

Well -

1. TI had been complaining last December about vmalloc() creating mappings
   with different shared-ness attributes causing problems with OMAP.

2. May 10, TI on the three-weekly conference call were warned about this
   change to ioremap with system RAM - and this was noted in the minutes
   and circulated.  It was brought up again on June 21st, and yet again
   on August 2nd.

So, the major players in the OMAP community were repeatedly made plainly
aware of this change and some were aware of the problems caused by
violating these kinds of restrictions.

You tell me - if I've raised the issue on the mailing list to which OMAP
people are aware of (and note that there was a reply from OMAP folk in
the inital thread), if I've raised it several times with TI in conference
calls which include the major players, and still OMAP drivers are doing
things wrongly.

You can lead the horse to water but you can not make it drink.  At some
point it either drinks or dies.  In this case, it seems death is the
favoured option as the warnings have been repeatedly ignored.

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

[Arnd Bergmann]

On Saturday 09 October 2010 13:59:45 Felipe Contreras wrote:
> I know of tidspbridge, in my original mail I listed some links, one
> which mentions: sh_mobile_ceu_camera, maybe i.MX31 users of the
> mx3_camera driver: pcm037 and mx31moboard.
>
> http://article.gmane.org/gmane.linux.ports.sh.devel/8560

sh_mobile_ceu_camera looks fine on mainline. It only remaps
the mmio registers, so no problem there. No arm platform even
declares the device, so the report must be with an external tree.

The mx3_camera driver also looks fine, there is only one ioremap
in there, which is completely sane.
 
> From the looks of it, PowerVR SGX seems like it will be broken too:
> http://meego.gitorious.org/meego-os-base/kernel-source/trees/master/patches

Not our problem, they already have a forked kernel tree with broken
drivers, they can fix it themselves or submit an acceptable driver
upstream if they want people to care about not breaking it.

> But my guess is that most of the issues would be found after releasing
> .36, and of course the current issues cannot possibly be fixed in that
> time-frame.

My guess is that tidspbridge is the only driver that someone (i.e. you)
cares about being broken more than it already is in the mainline kernel. :-)

How about the patch below?

	Arnd
---
staging/tidspbridge: add memory consistency to TODO list

This driver uses ioremap on regular memory to get an uncached mapping,
which causes problems on ARMv6 and higher due to aliasing with the
cached linar kernel mapping.

Make sure this gets fixed before the driver graduates from staging.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/drivers/staging/tidspbridge/TODO b/drivers/staging/tidspbridge/TODO
index 54f4a29..187363f 100644
--- a/drivers/staging/tidspbridge/TODO
+++ b/drivers/staging/tidspbridge/TODO
@@ -13,6 +13,7 @@
 * Audit and clean up header files folder
 * Use kernel coding style
 * checkpatch.pl fixes
+* allocate ext_mem_pool from consistent memory instead of using ioremap
 
 Please send any patches to Greg Kroah-Hartman <greg@kroah.com>
 and Omar Ramirez Luna <omar.ramirez@ti.com>.

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 4:52 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
>> For issues related to this:
>> http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
>
> This one nicely shows some of the problems which can occur with the
> memory type attributes - and this is not attributable to ioremap().
>
> ioremap() is used to map devices.  It creates device memory type mappings.
> If what you're mapping doesn't support device memory type mappings, then
> accesses via an ioremap()'d region isn't going to work - as this guy is
> observing.
>
> That's not because ioremap() is doing something wrong.  It's doing what
> it's meant to do.  The use is wrong, and is completely unrelated to the
> issue you've raised.

Ok, I was confused by Catalin's comment which does point to ioremap()
on normal RAM:
http://article.gmane.org/gmane.linux.ports.arm.kernel/84504

>> http://article.gmane.org/gmane.linux.ports.sh.devel/8560
>
> This one we know about, and as I've already said, it ends up with three
> aliasing mappings each with different attributes thusly:
>
>        cpu = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
>        dma_declare_coherent_memory(dev, dma, dma, size, DMA_MEMORY_MAP);
>                ==> ioremap(dma, size);
>        ...
>        dma_alloc_coherent(dev, ...);
>
> This wasn't spotted in the review of sh-mobile code because it's not part
> of the sh-mobile code base, but some of the generic sh architecture code.
> sh-mobile went into the kernel on March 12th, so it does pre-date the
> change to ioremap, and is therefore technically a regression.
>
> However, as can be seen from the link above, it's been known about since
> 8th August - two months ago.  The problem has been discussed, and we had
> a good solution which would work.  But then an oar got thrown in which
> basically resulted in that solution being rejected - on the basis that
> "it's an established API and it must work".

I don't see anyone rejecting any solution there. Where is anybody
saying "it's an established API and it must work"?

> Well, this usage of the API doesn't work on x86!
>
> The result - progress on the issue hit a brick wall and is unable to
> proceed because of personal viewpoints conflicting with reality.

Can you concentrate on the patch at hand?
http://article.gmane.org/gmane.linux.kernel/1045670

This doesn't break anything, nor prevents anyone coming up with solutions.

>> http://www.spinics.net/lists/linux-fbdev/msg01745.html
>
> External user?  Unreviewed code?  You can't seriously be suggesting
> that we should care about code we haven't seen which is sitting
> externally to the kernel tree, and this is a valid reason to hold
> off on changes to the kernel.

It's a plus, not the main reason to apply this patch. However,
progress is not held by any means, people can still fix their drivers,
generic solutions can still be proposed and worked on... This patch
doesn't hurt anybody.

You haven't answered my question: what is so horrible about warning
only on .36, and disallowing on .37?

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 5:37 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Oct 09, 2010 at 03:10:57PM +0300, Felipe Contreras wrote:
>> On Sat, Oct 9, 2010 at 2:43 PM, Dave Airlie <airlied@gmail.com> wrote:
>> >> The few drivers that may be hit by this are typically in drivers/staging
>> >> exactly because issues like this have not been fixed yet.
>> >>
>> >> We should probably just fix the non-staging drivers that are hit by
>> >> this now and declare the issue done.
>> >>
>> >> When you say that "many drivers broken", can you list the ones you know
>> >> about? It would probably help resolve this the right way.
>> >
>> > I'm guessing some closed source graphics drivers for ARM do all kinds
>> > of wrong crap, and Nokia use them a lot, and nobody wants to tell the
>> > closed vendors to change their drivers because it costs money.
>>
>> You are over simplifying things.
>>
>>  1) The code is open[1]
>>  2) the decision comes from TI, Nokia can only do so much before shipping
>
> Well -
>
> 1. TI had been complaining last December about vmalloc() creating mappings
>   with different shared-ness attributes causing problems with OMAP.
>
> 2. May 10, TI on the three-weekly conference call were warned about this
>   change to ioremap with system RAM - and this was noted in the minutes
>   and circulated.  It was brought up again on June 21st, and yet again
>   on August 2nd.
>
> So, the major players in the OMAP community were repeatedly made plainly
> aware of this change and some were aware of the problems caused by
> violating these kinds of restrictions.

My point was that Nokia was not the main entity to blame.

But anyway, I'm not too fond of secret meetings, the issues should be
discussed publicly.

> You tell me - if I've raised the issue on the mailing list to which OMAP
> people are aware of (and note that there was a reply from OMAP folk in
> the inital thread), if I've raised it several times with TI in conference
> calls which include the major players, and still OMAP drivers are doing
> things wrongly.
>
> You can lead the horse to water but you can not make it drink.  At some
> point it either drinks or dies.  In this case, it seems death is the
> favoured option as the warnings have been repeatedly ignored.

TI is not the owner of their drivers; the community is. I don't see
any mail from you on the linux-omap mailing list warning about
ioremap() changing behavior, nor do I think that would have been
enough warning, for OMAP, SH, or anybody else that might be affected.

All you are being asked is to make *one* release where the warning is
enabled but the behavior is not changed.

-- 
Felipe Contreras

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

[Russell King - ARM Linux]

On Sat, Oct 09, 2010 at 07:07:01PM +0300, Felipe Contreras wrote:
> On Sat, Oct 9, 2010 at 4:52 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
> >> For issues related to this:
> >> http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
> >
> > This one nicely shows some of the problems which can occur with the
> > memory type attributes - and this is not attributable to ioremap().
> >
> > ioremap() is used to map devices.  It creates device memory type mappings.
> > If what you're mapping doesn't support device memory type mappings, then
> > accesses via an ioremap()'d region isn't going to work - as this guy is
> > observing.
> >
> > That's not because ioremap() is doing something wrong.  It's doing what
> > it's meant to do.  The use is wrong, and is completely unrelated to the
> > issue you've raised.
> 
> Ok, I was confused by Catalin's comment which does point to ioremap()
> on normal RAM:
> http://article.gmane.org/gmane.linux.ports.arm.kernel/84504

Me too - it doesn't appear to relate to the specified problem.  You
don't want to map RAM as device nor strongly ordered, and we still
don't know what this "MMR" is.

> >> http://article.gmane.org/gmane.linux.ports.sh.devel/8560
> >
> > This one we know about, and as I've already said, it ends up with three
> > aliasing mappings each with different attributes thusly:
> >
> >        cpu = dma_alloc_coherent(dev, size, &dma, GFP_KERNEL);
> >        dma_declare_coherent_memory(dev, dma, dma, size, DMA_MEMORY_MAP);
> >                ==> ioremap(dma, size);
> >        ...
> >        dma_alloc_coherent(dev, ...);
> >
> > This wasn't spotted in the review of sh-mobile code because it's not part
> > of the sh-mobile code base, but some of the generic sh architecture code.
> > sh-mobile went into the kernel on March 12th, so it does pre-date the
> > change to ioremap, and is therefore technically a regression.
> >
> > However, as can be seen from the link above, it's been known about since
> > 8th August - two months ago.  The problem has been discussed, and we had
> > a good solution which would work.  But then an oar got thrown in which
> > basically resulted in that solution being rejected - on the basis that
> > "it's an established API and it must work".
> 
> I don't see anyone rejecting any solution there. Where is anybody
> saying "it's an established API and it must work"?

In a follow-on thread.

http://lists.arm.linux.org.uk/lurker/thread/20100820.200530.9bd67b3a.en.html

> > Well, this usage of the API doesn't work on x86!
> >
> > The result - progress on the issue hit a brick wall and is unable to
> > proceed because of personal viewpoints conflicting with reality.
> 
> Can you concentrate on the patch at hand?
> http://article.gmane.org/gmane.linux.kernel/1045670
> 
> This doesn't break anything, nor prevents anyone coming up with solutions.

and provides no motivation to fix anything either.

> >> http://www.spinics.net/lists/linux-fbdev/msg01745.html
> >
> > External user?  Unreviewed code?  You can't seriously be suggesting
> > that we should care about code we haven't seen which is sitting
> > externally to the kernel tree, and this is a valid reason to hold
> > off on changes to the kernel.
> 
> It's a plus, not the main reason to apply this patch. However,
> progress is not held by any means, people can still fix their drivers,
> generic solutions can still be proposed and worked on... This patch
> doesn't hurt anybody.
> 
> You haven't answered my question: what is so horrible about warning
> only on .36, and disallowing on .37?

My objection is one of methodology.

It's been known for six months that this change was going to be made.
It was discussed at the time it was proposed, and omapfb was raised as
a concern by OMAP people.

Most of the relevant parties (except sh-mobile as it wasn't obvious)
had been told.  The patch went in about three months ago.  Only now is
a major fuss being made over it.

If precisely nothing has happened in six months, inspite of my (repeated)
warnings that this change will hit mainline, then what hope is there that
giving a further three month grace in any form will be respected to get
drivers into shape?  If people can't get the idea with six months of
warning, then what is the use of adding such a restriction when no one
takes any notice of it anyway?

There's two ways to deal with no progress inspite of repeated warnings -
you either drop the issue, forget it ever existed and let people find the
resulting problems, or you force the issue.

I chose the latter, and yes, I expected that people will complain.  It
gets the issue _far_ higher in peoples sights than adding silly WARN_ON
stuff which all too easily gets ignored.  If email messages and verbal
discussions get ignored, what hope is there for a one-line kernel message
amongst 75 lines of kernel spew?

I doubt that a WARN_ON will result in any progress on the issue.

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

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4663 bytes --]

On Sat, 9 Oct 2010, Felipe Contreras wrote:

> On Sat, Oct 9, 2010 at 6:36 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Fri, 8 Oct 2010, Greg KH wrote:
> >> Wait, let me get this straight:
> >>   - drivers used to work on 2.6.35
> >
> > Possibly on pre ARMv6 only.  On ARMv6 and above, the discussed behavior
> > _never_ worked.  It is fundamental to the CPU design.
> 
> That's not true, drivers on ARMv6 and above do work.

So you want to fool yourself?  Fine.  This is your problem.

> I still wonder how exactly to trigger the issue, and how often does 
> this happen, because I've never seen it.

It's not because you don't see it right away that this is not happening.

> I trust the experts that it is an issue, but I don't agree that
> drivers that are currently working on ARMv6+ should be broken on
> purpose right away.

They are not working *reliably*.  Get that notion out of your mind.

> >>   - some ARM core code changed in .36-rc to fix this iomem problem that
> >>     you found
> >
> > Nothing was fixed.  There is nothing to fix.  Simply that a wrong driver
> > assumption about the hardware capability was prohibited by the API.
> 
> Not exactly. There is supposedly a problem on ARMv6+, there must be a
> mechanism to avoid this problem, all drivers should migrate to that,
> or a different on, so that we have a sane API.

Exact.  So please put the necessary effort to 1) understand the issue 
which was clearly explained in details by RMK already, or simply read 
the ARM Architecture Reference Manual for yourself, so that you 
understand the issue and stop asking for unreasonable things, and 2) do 
invest some time to fix your code that never worked properly in the 
first place.

> > There is no API change.  Simply an API misuse.  Granted, on pre ARMv6
> > this usage used to work, but on ARMv6 and above this never was supported
> > at all by the hardware.  The fact that the software has let it through
> > was a gross mistake, period.
> 
> No. It worked just fine, now it's doesn't, that's a change in API
> behavior, which is also part of the API. It might not be ideal, or
> proper, but it does work.

Look, if you can't be brought to reason and common sense this will be my 
last reply to you.

What would you say if someone suggested to turn the spinlocks into 
no-ops in a SMP kernel with full preemption configured in?  Let's say 
that person has done that because the lock validator was complaining and 
removing the locks made the warnings as well as the deadlock go away?  
And that person is claiming that the driver now works just fine (or 
appears to work) and no issue what so ever has been observed?  Because 
that's exactly what you're asking for right now.  Except that in this 
case you are willing to ignore a hardware race instead of a software 
one.

> >>   - drivers break when run as the api stops returning valid addresses
> >
> > Such drivers on ARMv6 should *never* have worked in the first place.
> > When they did "work", they corrupted memory.
> 
> They corrupted their own memory, right? And on _very_ rare occasions I
> presume, since I've never seen it.

Not their own memory, but *ANY* random memory.  You could corrupt your 
filesystem, have bugs in totally unrelated drivers, and so in ways that 
are totally unpredictable and unreproducible.  Is that the kind of 
system you want to debug?  If you have customers, is that the kind of 
products you want to sell them?

> If so, there's no damage in letting
> them work at least one more release (2.6.36).

On what basis can you dare make such a foolish claim?  Only on your 
limited testing and own observations?  Do you have some insider 
knowledge of the ARM architecture that would give us confidence in 
ignoring written documentation about this very issue?  What about those 
people besides you who did experience problems before?

If you want that, then do it in your own tree, oh and let me remember to 
ignore any future request for help from you.  But the mainline tree has 
to be proper regardless of the inconvenience that may cause you, 
_especially_ when data integrity is at stake.

> >>   - no known way is around to fix the broken drivers
> >
> > That's B/s.  As Russell said, there are known solutions that were
> > proposed at least six months ago.
> 
> You seriously think most people out there know how to "set aside some
> RAM at boot time"?

Indeed I do.  That is not rocket science.  You can easily figure it 
yourself too.

> I don't, otherwise the drivers would be fixed by now.

And that's why you want me to accept your argument for restoring a 
broken behavior?

Go do your homework and fix your code.


Nicolas

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

[Guennadi Liakhovetski]

On Sat, 9 Oct 2010, Arnd Bergmann wrote:

> On Saturday 09 October 2010 13:59:45 Felipe Contreras wrote:
> > I know of tidspbridge, in my original mail I listed some links, one
> > which mentions: sh_mobile_ceu_camera, maybe i.MX31 users of the
> > mx3_camera driver: pcm037 and mx31moboard.
> >
> > http://article.gmane.org/gmane.linux.ports.sh.devel/8560
> 
> sh_mobile_ceu_camera looks fine on mainline. It only remaps
> the mmio registers, so no problem there. No arm platform even
> declares the device, so the report must be with an external tree.
> 
> The mx3_camera driver also looks fine, there is only one ioremap
> in there, which is completely sane.

Arnd, the drivers are fine, they just use the videobuf-dma-contig.c 
driver, which calls dma_alloc_coherent(). But the platforms, that use 
those drivers reserve that memory from system RAM dma_alloc_coherent(NULL, 
...) and dma_declare_coherent_memory(), and this is what breaks currently. 
As Felipe and Uwe in another mail in this thread mention, there are 
currently two platforms in the mainline, that do this. mx1, mx2, omap1 and 
mach-shmobile platforms are potentially affected too.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 7:45 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Oct 09, 2010 at 07:07:01PM +0300, Felipe Contreras wrote:
>> On Sat, Oct 9, 2010 at 4:52 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:

>> >> http://article.gmane.org/gmane.linux.ports.sh.devel/8560

[...]

>> > However, as can be seen from the link above, it's been known about since
>> > 8th August - two months ago.  The problem has been discussed, and we had
>> > a good solution which would work.  But then an oar got thrown in which
>> > basically resulted in that solution being rejected - on the basis that
>> > "it's an established API and it must work".
>>
>> I don't see anyone rejecting any solution there. Where is anybody
>> saying "it's an established API and it must work"?
>
> In a follow-on thread.
>
> http://lists.arm.linux.org.uk/lurker/thread/20100820.200530.9bd67b3a.en.html

I read that thread, I don't see anyone saying "it must work". The
strongest opinion is to revert the patch until videobuf-dma-contig.c
is fixed, so that's "temporarily it must work", which is very similar
to my proposal.

>> Can you concentrate on the patch at hand?
>> http://article.gmane.org/gmane.linux.kernel/1045670
>>
>> This doesn't break anything, nor prevents anyone coming up with solutions.
>
> and provides no motivation to fix anything either.

It does, for the people that haven't found this issue yet.

>> You haven't answered my question: what is so horrible about warning
>> only on .36, and disallowing on .37?
>
> My objection is one of methodology.
>
> It's been known for six months that this change was going to be made.
> It was discussed at the time it was proposed, and omapfb was raised as
> a concern by OMAP people.
>
> Most of the relevant parties (except sh-mobile as it wasn't obvious)
> had been told.  The patch went in about three months ago.  Only now is
> a major fuss being made over it.
>
> If precisely nothing has happened in six months, inspite of my (repeated)
> warnings that this change will hit mainline, then what hope is there that
> giving a further three month grace in any form will be respected to get
> drivers into shape?  If people can't get the idea with six months of
> warning, then what is the use of adding such a restriction when no one
> takes any notice of it anyway?
>
> There's two ways to deal with no progress inspite of repeated warnings -
> you either drop the issue, forget it ever existed and let people find the
> resulting problems, or you force the issue.
>
> I chose the latter, and yes, I expected that people will complain.  It
> gets the issue _far_ higher in peoples sights than adding silly WARN_ON
> stuff which all too easily gets ignored.  If email messages and verbal
> discussions get ignored, what hope is there for a one-line kernel message
> amongst 75 lines of kernel spew?
>
> I doubt that a WARN_ON will result in any progress on the issue.

Let's remember that driver users != driver developers. This is not a
company where people are assigned drivers, and they are responsible of
heeding memos and fix issues in a timely manner. This is a community,
and there could be some developer out there who does have a life, but
might try .36 just for fun, see the warning, and fix the driver.

It's not about whether you think this is likely or not, it's about
following a sensible process where the potential for contributions is
maximized, and the breakage is minimized; that is, deprecate, release,
wait, obsolete. It's understandable that from your point of view this
has been deprecated since 6 months ago, and nagging the relevant
parties didn't achieve anything, so now they should be punished. But
what about the not so obviously relevant parties? Just make a release
with the warning to give them a chance, if nothing happens, _then_
obsolete, it's not so much to ask. Next time, deprecate right away,
then the warning would have been on .35, and by .36 it would have been
harder to argue that the warning alone is achieving anything, and you
could happily obsolete.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 8:38 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Sat, 9 Oct 2010, Felipe Contreras wrote:
>> On Sat, Oct 9, 2010 at 6:36 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
>> > On Fri, 8 Oct 2010, Greg KH wrote:
>> >> Wait, let me get this straight:
>> >>   - drivers used to work on 2.6.35
>> >
>> > Possibly on pre ARMv6 only.  On ARMv6 and above, the discussed behavior
>> > _never_ worked.  It is fundamental to the CPU design.
>>
>> That's not true, drivers on ARMv6 and above do work.
>
> So you want to fool yourself?  Fine.  This is your problem.

You are playing with the definition of "work". For me, and millions of
people out there, the drivers do work.

I'm not saying they are working flawlessly, nor that they shouldn't be
fixed, just that they mostly work, at least better than in 2.6.36-rc6.

>> I still wonder how exactly to trigger the issue, and how often does
>> this happen, because I've never seen it.
>
> It's not because you don't see it right away that this is not happening.

I know that, but still, I've asked for a test to trigger this bad
behavior, but I haven't seen any. So how often, and how bad this issue
really is remains a mystery to me.

Again, I'm not saying the issue is not there.

>> >>   - some ARM core code changed in .36-rc to fix this iomem problem that
>> >>     you found
>> >
>> > Nothing was fixed.  There is nothing to fix.  Simply that a wrong driver
>> > assumption about the hardware capability was prohibited by the API.
>>
>> Not exactly. There is supposedly a problem on ARMv6+, there must be a
>> mechanism to avoid this problem, all drivers should migrate to that,
>> or a different on, so that we have a sane API.
>
> Exact.  So please put the necessary effort to 1) understand the issue
> which was clearly explained in details by RMK already, or simply read
> the ARM Architecture Reference Manual for yourself, so that you
> understand the issue and stop asking for unreasonable things, and 2) do
> invest some time to fix your code that never worked properly in the
> first place.

_I_ don't have any code that uses ioremap() that way.

However, I'm looking at code that uses memblock. Wouldn't extending
memblock to specify the mapping of the memory areas be a sensible
generic mechanism?

>> > There is no API change.  Simply an API misuse.  Granted, on pre ARMv6
>> > this usage used to work, but on ARMv6 and above this never was supported
>> > at all by the hardware.  The fact that the software has let it through
>> > was a gross mistake, period.
>>
>> No. It worked just fine, now it's doesn't, that's a change in API
>> behavior, which is also part of the API. It might not be ideal, or
>> proper, but it does work.
>
> Look, if you can't be brought to reason and common sense this will be my
> last reply to you.
>
> What would you say if someone suggested to turn the spinlocks into
> no-ops in a SMP kernel with full preemption configured in?  Let's say
> that person has done that because the lock validator was complaining and
> removing the locks made the warnings as well as the deadlock go away?
> And that person is claiming that the driver now works just fine (or
> appears to work) and no issue what so ever has been observed?  Because
> that's exactly what you're asking for right now.  Except that in this
> case you are willing to ignore a hardware race instead of a software
> one.

Nonsense. That is turning good behavior into bad behavior. Find a
better example that turns bad behavior into good behavior, breaks the
API at run-time, and there isn't a single release that shows a warning
to alert users of the impending breakage.

>> >>   - drivers break when run as the api stops returning valid addresses
>> >
>> > Such drivers on ARMv6 should *never* have worked in the first place.
>> > When they did "work", they corrupted memory.
>>
>> They corrupted their own memory, right? And on _very_ rare occasions I
>> presume, since I've never seen it.
>
> Not their own memory, but *ANY* random memory.  You could corrupt your
> filesystem, have bugs in totally unrelated drivers, and so in ways that
> are totally unpredictable and unreproducible.  Is that the kind of
> system you want to debug?  If you have customers, is that the kind of
> products you want to sell them?

I haven't seen anyone claim that. I guess i'll have to read the TRM,
but of course, a test that reproduces this would help.

>> If so, there's no damage in letting
>> them work at least one more release (2.6.36).
>
> On what basis can you dare make such a foolish claim?  Only on your
> limited testing and own observations?  Do you have some insider
> knowledge of the ARM architecture that would give us confidence in
> ignoring written documentation about this very issue?  What about those
> people besides you who did experience problems before?

Machines don't explode with .35, right?

If you think this behavior is horrible, then this change should be
pushed to all the stable trees, shouldn't it?

> If you want that, then do it in your own tree, oh and let me remember to
> ignore any future request for help from you.  But the mainline tree has
> to be proper regardless of the inconvenience that may cause you,
> _especially_ when data integrity is at stake.

Perhaps, if the issue is as critical as you say.

>> >>   - no known way is around to fix the broken drivers
>> >
>> > That's B/s.  As Russell said, there are known solutions that were
>> > proposed at least six months ago.
>>
>> You seriously think most people out there know how to "set aside some
>> RAM at boot time"?
>
> Indeed I do.  That is not rocket science.  You can easily figure it
> yourself too.

That's exactly what I'm trying right now.

>> I don't, otherwise the drivers would be fixed by now.
>
> And that's why you want me to accept your argument for restoring a
> broken behavior?

*Temporarily*, at least one release.

> Go do your homework and fix your code.

Not _my_ code.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Sat, Oct 9, 2010 at 12:21 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> static unsigned long reserve_mem(struct meminfo *mi, unsigned long size)
> {
>        unsigned long addr = ~0;
>        int i;
>
>        for (i = mi->nr_banks - 1; i >= 0; i--)
>                if (mi->bank[i].size >= size) {
>                        mi->bank[i].size -= size;
>                        addr = mi->bank[i].start + mi->bank[i].size;
>                        break;
>                }
>
>        return addr;
> }
>
> static void __init my_fixup(struct machine_desc *desc, struct tag *tags,
>                            char **cmdline, struct meminfo *mi)
> {
>        omapfb_buffer_phys = reserve_mem(mi, 32*1048576);
>        if (omapfb_buffer_phys == ~0)
>                pr_warn("Unable to allocate omapfb buffer\n");
> }
>
> Then later:
>
>        omapfb_buffer = ioremap(omapfb_buffer_phys, 32*1048576);
>
> That's a damned simple and direct implementation of exactly what I
> described, and results in something which is much more architecturally
> correct than what's going on today.

I tried this, it doesn't work.

At the time 'fixup' is called, 'meminfo' is empty; the tags haven't
been parsed. So my solution is to move the memblock_add() after
'reserve', and pass 'meminfo' as an argument:

--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -272,8 +272,6 @@ void __init arm_memblock_init(struct meminfo *mi,
struct machine_desc *mdesc)
        int i;

        memblock_init();
-       for (i = 0; i < mi->nr_banks; i++)
-               memblock_add(mi->bank[i].start, mi->bank[i].size);

        /* Register the kernel text, kernel data and initrd with memblock. */
 #ifdef CONFIG_XIP_KERNEL
@@ -295,7 +293,10 @@ void __init arm_memblock_init(struct meminfo *mi,
struct machine_desc *mdesc)

        /* reserve any platform specific memblock areas */
        if (mdesc->reserve)
-               mdesc->reserve();
+               mdesc->reserve(mi);
+
+       for (i = 0; i < mi->nr_banks; i++)
+               memblock_add(mi->bank[i].start, mi->bank[i].size);

        memblock_analyze();
        memblock_dump_all();

-- 
Felipe Contreras

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

[Pedanekar, Hemant]

Russell King - ARM Linux wrote on Saturday, October 09, 2010 10:15 PM:

> On Sat, Oct 09, 2010 at 07:07:01PM +0300, Felipe Contreras wrote:
>> On Sat, Oct 9, 2010 at 4:52 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>>> On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
>>>> For issues related to this:
>>>> http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
>>> 
>>> This one nicely shows some of the problems which can occur with the
>>> memory type attributes - and this is not attributable to ioremap().
>>> 
>>> ioremap() is used to map devices.  It creates device memory type mappings.
>>> If what you're mapping doesn't support device memory type mappings, then
>>> accesses via an ioremap()'d region isn't going to work - as this guy is
>>> observing. 
>>> 
>>> That's not because ioremap() is doing something wrong. It's doing what
>>> it's meant to do.  The use is wrong, and is completely unrelated to the
>>> issue you've raised.
>> 
>> Ok, I was confused by Catalin's comment which does point to ioremap() on
>> normal RAM: http://article.gmane.org/gmane.linux.ports.arm.kernel/84504
> 
> Me too - it doesn't appear to relate to the specified problem.  You
> don't want to map RAM as device nor strongly ordered, and we still
> don't know what this "MMR" is.
> 

Just to add, the issue I faced was a different issue and more to do with
specific access requirement for particular memory mapped region and what memory
types could be used for defining mappings of the region (or passed as ioremap
parameter). To summarize, I needed to use type=MT_STRONGLY_ORDERED to make access
to the peripheral registers work, which, unfortunately is not available in the
kernel.

(BTW, I had referred MMR as memory mapped registers of the concerned peripheral.)

-
Hemant

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

[Uwe Kleine-König]

Hello,
 
On Sun, Oct 10, 2010 at 04:52:36AM +0300, Felipe Contreras wrote:
> On Sat, Oct 9, 2010 at 12:21 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > static unsigned long reserve_mem(struct meminfo *mi, unsigned long size)
> > {
> >        unsigned long addr = ~0;
> >        int i;
> >
> >        for (i = mi->nr_banks - 1; i >= 0; i--)
> >                if (mi->bank[i].size >= size) {
> >                        mi->bank[i].size -= size;
> >                        addr = mi->bank[i].start + mi->bank[i].size;
> >                        break;
> >                }
> >
> >        return addr;
> > }
> >
> > static void __init my_fixup(struct machine_desc *desc, struct tag *tags,
> >                            char **cmdline, struct meminfo *mi)
> > {
> >        omapfb_buffer_phys = reserve_mem(mi, 32*1048576);
> >        if (omapfb_buffer_phys == ~0)
> >                pr_warn("Unable to allocate omapfb buffer\n");
> > }
> >
> > Then later:
> >
> >        omapfb_buffer = ioremap(omapfb_buffer_phys, 32*1048576);
> >
> > That's a damned simple and direct implementation of exactly what I
> > described, and results in something which is much more architecturally
> > correct than what's going on today.
> 
> I tried this, it doesn't work.
Ack, didn't work.  I tried it for arch/arm/mach-mx3/mach-pcm037.c.

> 
> At the time 'fixup' is called, 'meminfo' is empty; the tags haven't
> been parsed. So my solution is to move the memblock_add() after
> 'reserve', and pass 'meminfo' as an argument:
> 
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -272,8 +272,6 @@ void __init arm_memblock_init(struct meminfo *mi,
> struct machine_desc *mdesc)
>         int i;
> 
>         memblock_init();
> -       for (i = 0; i < mi->nr_banks; i++)
> -               memblock_add(mi->bank[i].start, mi->bank[i].size);
> 
>         /* Register the kernel text, kernel data and initrd with memblock. */
>  #ifdef CONFIG_XIP_KERNEL
> @@ -295,7 +293,10 @@ void __init arm_memblock_init(struct meminfo *mi,
> struct machine_desc *mdesc)
> 
>         /* reserve any platform specific memblock areas */
>         if (mdesc->reserve)
> -               mdesc->reserve();
> +               mdesc->reserve(mi);
> +
> +       for (i = 0; i < mi->nr_banks; i++)
> +               memblock_add(mi->bank[i].start, mi->bank[i].size);
> 
>         memblock_analyze();
>         memblock_dump_all();
Alternatively when calling fixup only after parse_tags in setup_arch()
it worked.  But I guess this change is not for discussion.
OTOH I wonder why fixup gets passed meminfo.  It's not very useful if
it's not yet filled.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Russell King - ARM Linux]

On Mon, Oct 11, 2010 at 10:35:08AM +0200, Uwe Kleine-König wrote:
> Alternatively when calling fixup only after parse_tags in setup_arch()
> it worked.  But I guess this change is not for discussion.
> OTOH I wonder why fixup gets passed meminfo.  It's not very useful if
> it's not yet filled.

Because, rather than pass in ATAGs or set the memory on the command line,
they instead wanted to hard-code it in C - something which still happens
today:

arch/arm/mach-msm/board-halibut.c
arch/arm/mach-msm/board-mahimahi.c
arch/arm/mach-msm/board-sapphire.c
arch/arm/mach-msm/board-trout.c
arch/arm/mach-pxa/eseries.c
arch/arm/mach-pxa/spitz.c
arch/arm/mach-s3c2412/mach-smdk2413.c
arch/arm/mach-s3c2412/mach-vstms.c
arch/arm/mach-tegra/board-harmony.c

This is part of the on-going pain of poor quality boot loaders.

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

[Uwe Kleine-König]

On Mon, Oct 11, 2010 at 10:02:12AM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 11, 2010 at 10:35:08AM +0200, Uwe Kleine-König wrote:
> > Alternatively when calling fixup only after parse_tags in setup_arch()
> > it worked.  But I guess this change is not for discussion.
> > OTOH I wonder why fixup gets passed meminfo.  It's not very useful if
> > it's not yet filled.
> 
> Because, rather than pass in ATAGs or set the memory on the command line,
> they instead wanted to hard-code it in C - something which still happens
> today:
> 
> arch/arm/mach-msm/board-halibut.c
> arch/arm/mach-msm/board-mahimahi.c
> arch/arm/mach-msm/board-sapphire.c
> arch/arm/mach-msm/board-trout.c
> arch/arm/mach-pxa/eseries.c
> arch/arm/mach-pxa/spitz.c
> arch/arm/mach-s3c2412/mach-smdk2413.c
> arch/arm/mach-s3c2412/mach-vstms.c
> arch/arm/mach-tegra/board-harmony.c
> 
> This is part of the on-going pain of poor quality boot loaders.
Yep, so (at least regarding the memory layout) the name "fixup" is
wrong, it's more "initializ"ish.

So what now?  Do the memory reservation in mdesc->reserve as Felipe
suggested?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Catalin Marinas]

On Sat, 2010-10-09 at 17:45 +0100, Russell King - ARM Linux wrote:
> On Sat, Oct 09, 2010 at 07:07:01PM +0300, Felipe Contreras wrote:
> > On Sat, Oct 9, 2010 at 4:52 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> > > On Thu, Oct 07, 2010 at 12:44:22PM +0300, Felipe Contreras wrote:
> > >> For issues related to this:
> > >> http://article.gmane.org/gmane.linux.ports.arm.kernel/84454
> > >
> > > This one nicely shows some of the problems which can occur with the
> > > memory type attributes - and this is not attributable to ioremap().
> > >
> > > ioremap() is used to map devices.  It creates device memory type mappings.
> > > If what you're mapping doesn't support device memory type mappings, then
> > > accesses via an ioremap()'d region isn't going to work - as this guy is
> > > observing.
> > >
> > > That's not because ioremap() is doing something wrong.  It's doing what
> > > it's meant to do.  The use is wrong, and is completely unrelated to the
> > > issue you've raised.
> >
> > Ok, I was confused by Catalin's comment which does point to ioremap()
> > on normal RAM:
> > http://article.gmane.org/gmane.linux.ports.arm.kernel/84504
> 
> Me too - it doesn't appear to relate to the specified problem.  You
> don't want to map RAM as device nor strongly ordered, and we still
> don't know what this "MMR" is.

I don't know what MMR is either. But my comment is misleading, I think I
was thinking about dma_alloc_coherent().

We could to allow ioremap_wc() or ioremap_cached() to normal RAM but
that's probably outside the scope of ioremap* (and it would require
initial cache maintenance to remove potential dirty cache lines).

-- 
Catalin

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

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

[Felipe Contreras]

2010/10/11 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> On Mon, Oct 11, 2010 at 10:02:12AM +0100, Russell King - ARM Linux wrote:
>> On Mon, Oct 11, 2010 at 10:35:08AM +0200, Uwe Kleine-König wrote:
>> > Alternatively when calling fixup only after parse_tags in setup_arch()
>> > it worked.  But I guess this change is not for discussion.
>> > OTOH I wonder why fixup gets passed meminfo.  It's not very useful if
>> > it's not yet filled.
>>
>> Because, rather than pass in ATAGs or set the memory on the command line,
>> they instead wanted to hard-code it in C - something which still happens
>> today:
>>
>> arch/arm/mach-msm/board-halibut.c
>> arch/arm/mach-msm/board-mahimahi.c
>> arch/arm/mach-msm/board-sapphire.c
>> arch/arm/mach-msm/board-trout.c
>> arch/arm/mach-pxa/eseries.c
>> arch/arm/mach-pxa/spitz.c
>> arch/arm/mach-s3c2412/mach-smdk2413.c
>> arch/arm/mach-s3c2412/mach-vstms.c
>> arch/arm/mach-tegra/board-harmony.c
>>
>> This is part of the on-going pain of poor quality boot loaders.
> Yep, so (at least regarding the memory layout) the name "fixup" is
> wrong, it's more "initializ"ish.
>
> So what now?  Do the memory reservation in mdesc->reserve as Felipe
> suggested?

I read some fixups(), some boards seem to fill meminfo, so I thought
moving after parsing the tags would cause problems.

My proposal is also not correct because as Russell said, reserve()
callbacks expect memblock to already have memory configured.

My original patch had a reserve_mem() instead, that was passed with
'meminfo'. That would not conflict with any existing behavior.

A bit independently to this is the fact that I couldn't
memblock_reserve() the areas I kicked out of meminfo (couldn't see why
due to my environment), which I think is not ideal.


So, coming back to the arguments. I think Russell should have tried
this and provided an example of what to do, and how. I think it should
be clear by now that there's not much known affected drivers can do
right now.

And yet, I feel there would be more affected drivers we don't know
about, and doing WARN_ON on .36 and prohibit only afterwards can only
help.

-- 
Felipe Contreras

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

[Russell King - ARM Linux]

On Mon, Oct 11, 2010 at 01:08:29PM +0300, Felipe Contreras wrote:
> So, coming back to the arguments. I think Russell should have tried
> this and provided an example of what to do, and how. I think it should
> be clear by now that there's not much known affected drivers can do
> right now.

Those people who have stupid platforms that require major chunks of
memory should have been paying attention and evaluated the patch six
months ago.

They partly did that, and asked some questions about how to solve it.
They got answers, and then nothing happened.  And it's somehow my
problem that they did nothing?

*They* should have done more to help instead of ignoring the issue,
then we'd know where things were _before_ the patch was originally
merged.

Not my problem.

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

[Felipe Contreras]

On Mon, Oct 11, 2010 at 1:05 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> 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)

I will try that, but from what I can see this might still break some
drivers, right? I still think just having the warning for now should
be enough.

Anyway, I'm reading the TRM and I can't find any mention of this.
Catalin, can you point out where is this mentioned, and also, can you
confirm if this would affect only the memory that has the double
mapping, or it can corrupt other memory as well?

Cheers.

-- 
Felipe Contreras

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

[Russell King - ARM Linux]

On Mon, Oct 11, 2010 at 01:39:13PM +0300, Felipe Contreras wrote:
> Anyway, I'm reading the TRM and I can't find any mention of this.
> Catalin, can you point out where is this mentioned, and also, can you
> confirm if this would affect only the memory that has the double
> mapping, or it can corrupt other memory as well?

Page A3-31:

  If the same memory locations are marked as having different cacheability
  attributes, for example by the use of aliases in a virtual to physical
  address mapping, behavior is UNPREDICTABLE.

Page A3-36:

  Behavior is UNPREDICTABLE if the same memory location:
   ■ is marked as Shareable Normal and Non-shareable Normal
   ■ is marked as having different memory types (Normal, Device, or Strongly-
     ordered)
   ■ is marked as having different cacheability attributes
   ■ is marked as being Shareable Device and Non-shareable Device memory.

  Such memory marking contradictions can occur, for example, by the use of
  aliases in a virtual to physical address mapping.

And from the glossary:

  UNPREDICTABLE Means the behavior cannot be relied upon. UNPREDICTABLE
  behavior must not represent security holes. UNPREDICTABLE behavior must
  not halt or hang the processor, or any parts of the system. UNPREDICTABLE
  behavior must not be documented or promoted as having a defined effect.

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

[Pawel Moll]

Hello,

> +		    (mtype != MT_DEVICE_CACHED || mtype != MT_DEVICE_WC))

Em, am I wrong or this expression _always_ evaluates as 1? ;-)

Cheers!

Paweł

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

[Catalin Marinas]

On Mon, 2010-10-11 at 12:01 +0100, Pawel Moll wrote:
> > +                 (mtype != MT_DEVICE_CACHED || mtype != MT_DEVICE_WC))
> 
> Em, am I wrong or this expression _always_ evaluates as 1? ;-)

Should be '&&'. Thanks.

-- 
Catalin

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

[Catalin Marinas]

On Mon, 2010-10-11 at 11:39 +0100, Felipe Contreras wrote:
> On Mon, Oct 11, 2010 at 1:05 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > 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)
> 
> I will try that, but from what I can see this might still break some
> drivers, right? 

Probably, it depends on how drivers use the memory returned by ioremap
and the assumptions they make about the memory type (strict ordering,
write buffers, speculation). I think such memory should be used via the
I/O accessors and not directly, so you get the benefit of explicit
memory barriers. If not, you have to cope with the loss of attributes
because of the WC mapping.

But as it has been said (and it's my opinion as well), the drivers are
broken and are misusing the ioremap API.

> Anyway, I'm reading the TRM and I can't find any mention of this.
> Catalin, can you point out where is this mentioned, and also, can you
> confirm if this would affect only the memory that has the double
> mapping, or it can corrupt other memory as well?

That's in the ARM ARM (Architecture Reference Manual), not a TRM.

You never know what "unpredictable" means and this is specific to the
processor implementation, not the ARM ARM. You should not mix different
memory types for the same page.

Back in 2006 there was a discussion for a set of palliative measures in
hardware (initially until the OSes are fixed) and these allow the same
memory type but with different cacheability attributes (e.g. Normal
Non-cacheable vs Normal Cacheable) given that care is taken in software
wrt stale cache lines and speculation (but at least you don't get random
corruption somewhere else). They would need to have the same
shareability attributes though.

With the hack above, MT_DEVICE_WC is actually Normal Non-cacheable
memory. It needs cache flushing but it's not worse than the original
ioremap allowing this. The driver may not work as expected though.

-- 
Catalin

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

[Felipe Contreras]

On Mon, Oct 11, 2010 at 2:23 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Mon, 2010-10-11 at 11:39 +0100, Felipe Contreras wrote:
>> On Mon, Oct 11, 2010 at 1:05 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > 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)
>>
>> I will try that, but from what I can see this might still break some
>> drivers, right?
>
> Probably, it depends on how drivers use the memory returned by ioremap
> and the assumptions they make about the memory type (strict ordering,
> write buffers, speculation). I think such memory should be used via the
> I/O accessors and not directly, so you get the benefit of explicit
> memory barriers. If not, you have to cope with the loss of attributes
> because of the WC mapping.
>
> But as it has been said (and it's my opinion as well), the drivers are
> broken and are misusing the ioremap API.

Indeed, that is understood by everyone.

But what I think should happen on .36 is that the warning is on, and
the behavior doesn't change compared to .35. Your patch is changing
the behavior as you say in hacky way, which might have unintended
consequences in the affected drivers, I don't think that's ideal,
although it's better than breaking them completely.

>> Anyway, I'm reading the TRM and I can't find any mention of this.
>> Catalin, can you point out where is this mentioned, and also, can you
>> confirm if this would affect only the memory that has the double
>> mapping, or it can corrupt other memory as well?
>
> That's in the ARM ARM (Architecture Reference Manual), not a TRM.
>
> You never know what "unpredictable" means and this is specific to the
> processor implementation, not the ARM ARM. You should not mix different
> memory types for the same page.

Certainly, but saying corruption _will_ happen elsewhere is not
correct, there is no empirical evidence for that, although
theoretically it might happen.

Considering that, and the fact that .35 is not considered to be
horribly broken (or any of the previous releases), then having one
more releases with this broken behavior as a grace period wouldn't
hurt anybody.

> Back in 2006 there was a discussion for a set of palliative measures in
> hardware (initially until the OSes are fixed) and these allow the same
> memory type but with different cacheability attributes (e.g. Normal
> Non-cacheable vs Normal Cacheable) given that care is taken in software
> wrt stale cache lines and speculation (but at least you don't get random
> corruption somewhere else). They would need to have the same
> shareability attributes though.
>
> With the hack above, MT_DEVICE_WC is actually Normal Non-cacheable
> memory. It needs cache flushing but it's not worse than the original
> ioremap allowing this. The driver may not work as expected though.

I see, but are you saying that if the shareability is not the same,
then corruption elsewhere might actually happen? Or it would happen
most likely on the affected region? If it's the former, then I agree
that your patch should be used instead.

-- 
Felipe Contreras

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

[Catalin Marinas]

On Mon, 2010-10-11 at 13:03 +0100, Felipe Contreras wrote:
> On Mon, Oct 11, 2010 at 2:23 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Mon, 2010-10-11 at 11:39 +0100, Felipe Contreras wrote:
> >> I will try that, but from what I can see this might still break some
> >> drivers, right?
> >
> > Probably, it depends on how drivers use the memory returned by ioremap
> > and the assumptions they make about the memory type (strict ordering,
> > write buffers, speculation). I think such memory should be used via the
> > I/O accessors and not directly, so you get the benefit of explicit
> > memory barriers. If not, you have to cope with the loss of attributes
> > because of the WC mapping.
> >
> > But as it has been said (and it's my opinion as well), the drivers are
> > broken and are misusing the ioremap API.
> 
> Indeed, that is understood by everyone.
> 
> But what I think should happen on .36 is that the warning is on, and
> the behavior doesn't change compared to .35. Your patch is changing
> the behavior as you say in hacky way, which might have unintended
> consequences in the affected drivers, I don't think that's ideal,
> although it's better than breaking them completely.

Well, even if you allow Device memory via ioremap in Linux and the
hardware behaviour is not "unpredictable", it is not guaranteed to act
as a Device memory. You get speculative accesses via the Normal alias
and you may also get dirty cache lines evicted at random from the Normal
mapping (unless you invalidated the cache before). The dirty eviction is
valid even for pre-ARMv6 processors.

> >> Anyway, I'm reading the TRM and I can't find any mention of this.
> >> Catalin, can you point out where is this mentioned, and also, can you
> >> confirm if this would affect only the memory that has the double
> >> mapping, or it can corrupt other memory as well?
> >
> > That's in the ARM ARM (Architecture Reference Manual), not a TRM.
> >
> > You never know what "unpredictable" means and this is specific to the
> > processor implementation, not the ARM ARM. You should not mix different
> > memory types for the same page.
> 
> Certainly, but saying corruption _will_ happen elsewhere is not
> correct, there is no empirical evidence for that, although
> theoretically it might happen.

It may be just theoretical but architecture licensees implement the
hardware according to the ARM ARM. If it says "unpredictable", they
don't need to care much about this scenario as it is not allowed in
software and the hardware can be further optimised (well, I think
"unpredictable" doesn't allow hardware deadlocking or security
implications, so the memory corruption cannot be that random, possibly
just restricted to that memory range). It is probably ok on current
hardware but I cannot guarantee, you would have to check on a
case-by-case basis (CPU implementation).

> > Back in 2006 there was a discussion for a set of palliative measures in
> > hardware (initially until the OSes are fixed) and these allow the same
> > memory type but with different cacheability attributes (e.g. Normal
> > Non-cacheable vs Normal Cacheable) given that care is taken in software
> > wrt stale cache lines and speculation (but at least you don't get random
> > corruption somewhere else). They would need to have the same
> > shareability attributes though.
> >
> > With the hack above, MT_DEVICE_WC is actually Normal Non-cacheable
> > memory. It needs cache flushing but it's not worse than the original
> > ioremap allowing this. The driver may not work as expected though.
> 
> I see, but are you saying that if the shareability is not the same,
> then corruption elsewhere might actually happen? Or it would happen
> most likely on the affected region? If it's the former, then I agree
> that your patch should be used instead.

Pretty much same answer as above. But why do you need different
shareability attributes?

-- 
Catalin

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

[Russell King - ARM Linux]

On Sun, Oct 10, 2010 at 04:52:36AM +0300, Felipe Contreras wrote:
> At the time 'fixup' is called, 'meminfo' is empty; the tags haven't
> been parsed. So my solution is to move the memblock_add() after
> 'reserve', and pass 'meminfo' as an argument:

Here's a different approach which will work.  This pushes ARM further
towards using memblock for everything relating to memory init (although
we still have the old membank stuff around.)

The advantage with this is that memblock is now used as the basis for
determining where memory is, setting up the maps, freeing memory into
the pools, etc.

What this also means is that this code in the ->reserve callback:

	size = min(size, SZ_2M);
	base = memblock_alloc(size, min(align, SZ_2M));
	memblock_free(base, size);
	memblock_remove(base, size);

will result in [base+size] being removed from the available memory,
using highmem if available, if not from lowmem and removing it from
the lowmem memory map - which is exactly the behaviour we want.

 arch/arm/mm/init.c |  160 +++++++++++++++++++++++++++++++++++-----------------
 arch/arm/mm/mmu.c  |   43 ++++++++------
 mm/memblock.c      |    4 +
 3 files changed, 138 insertions(+), 69 deletions(-)

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 36c4553..66db6b2 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -121,34 +121,32 @@ void show_mem(void)
 	printk("%d pages swap cached\n", cached);
 }
 
-static void __init find_limits(struct meminfo *mi,
-	unsigned long *min, unsigned long *max_low, unsigned long *max_high)
+static void __init find_limits(unsigned long *min, unsigned long *max_low,
+	unsigned long *max_high)
 {
+	struct meminfo *mi = &meminfo;
 	int i;
 
-	*min = -1UL;
-	*max_low = *max_high = 0;
+	*min = memblock_start_pfn(&memblock.memory, 0);
+	*max_high = PFN_DOWN(memblock_end_of_DRAM());
 
+	/* Use the old method to find the top of lowmem */
+	*max_low = 0;
 	for_each_bank (i, mi) {
 		struct membank *bank = &mi->bank[i];
-		unsigned long start, end;
-
-		start = bank_pfn_start(bank);
-		end = bank_pfn_end(bank);
+		unsigned long end;
 
-		if (*min > start)
-			*min = start;
-		if (*max_high < end)
-			*max_high = end;
 		if (bank->highmem)
 			continue;
+
+		end = bank_pfn_end(bank);
 		if (*max_low < end)
 			*max_low = end;
 	}
 }
 
-static void __init arm_bootmem_init(struct meminfo *mi,
-	unsigned long start_pfn, unsigned long end_pfn)
+static void __init arm_bootmem_init(unsigned long start_pfn,
+	unsigned long end_pfn)
 {
 	unsigned int boot_pages;
 	phys_addr_t bitmap;
@@ -171,27 +169,35 @@ static void __init arm_bootmem_init(struct meminfo *mi,
 	pgdat = NODE_DATA(0);
 	init_bootmem_node(pgdat, __phys_to_pfn(bitmap), start_pfn, end_pfn);
 
-	for_each_bank(i, mi) {
-		struct membank *bank = &mi->bank[i];
-		if (!bank->highmem)
-			free_bootmem(bank_phys_start(bank), bank_phys_size(bank));
+	/* Free the lowmem regions from memblock into bootmem. */
+	for (i = 0; i < memblock.memory.cnt; i++) {
+		unsigned long start = memblock_start_pfn(&memblock.memory, i);
+		unsigned long end = memblock_end_pfn(&memblock.memory, i);
+
+		if (end >= end_pfn)
+			end = end_pfn;
+		if (start >= end)
+			break;
+
+		free_bootmem(__pfn_to_phys(start), (end - start) << PAGE_SHIFT);
 	}
 
-	/*
-	 * Reserve the memblock reserved regions in bootmem.
-	 */
+	/* Reserve the lowmem memblock reserved regions in bootmem. */
 	for (i = 0; i < memblock.reserved.cnt; i++) {
-		phys_addr_t start = memblock_start_pfn(&memblock.reserved, i);
-		if (start >= start_pfn &&
-		    memblock_end_pfn(&memblock.reserved, i) <= end_pfn)
-			reserve_bootmem_node(pgdat, __pfn_to_phys(start),
-				memblock_size_bytes(&memblock.reserved, i),
-				BOOTMEM_DEFAULT);
+		unsigned long start = memblock_start_pfn(&memblock.reserved, i);
+		unsigned long size = memblock_size_bytes(&memblock.reserved, i);
+
+		if (start >= end_pfn)
+			break;
+		if (start + PFN_UP(size) > end_pfn)
+			size = (end_pfn - start) << PAGE_SHIFT;
+
+		reserve_bootmem(__pfn_to_phys(start), size, BOOTMEM_DEFAULT);
 	}
 }
 
-static void __init arm_bootmem_free(struct meminfo *mi, unsigned long min,
-	unsigned long max_low, unsigned long max_high)
+static void __init arm_bootmem_free(unsigned long min, unsigned long max_low,
+	unsigned long max_high)
 {
 	unsigned long zone_size[MAX_NR_ZONES], zhole_size[MAX_NR_ZONES];
 	int i;
@@ -216,13 +222,23 @@ static void __init arm_bootmem_free(struct meminfo *mi, unsigned long min,
 	 *  holes = node_size - sum(bank_sizes)
 	 */
 	memcpy(zhole_size, zone_size, sizeof(zhole_size));
-	for_each_bank(i, mi) {
-		int idx = 0;
+	for (i = 0; i < memblock.memory.cnt; i++) {
+		unsigned long start = memblock_start_pfn(&memblock.memory, i);
+		unsigned long end = memblock_end_pfn(&memblock.memory, i);
+
+		if (start < max_low) {
+			unsigned long low_end = min(end, max_low);
+
+			zhole_size[0] -= low_end - start;
+		}
+
 #ifdef CONFIG_HIGHMEM
-		if (mi->bank[i].highmem)
-			idx = ZONE_HIGHMEM;
+		if (end > max_low) {
+			unsigned long high_start = max(start, max_low);
+
+			zhole_size[ZONE_HIGHMEM] -= end - high_start;
+		}
 #endif
-		zhole_size[idx] -= bank_pfn_size(&mi->bank[i]);
 	}
 
 	/*
@@ -303,14 +319,13 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc)
 
 void __init bootmem_init(void)
 {
-	struct meminfo *mi = &meminfo;
 	unsigned long min, max_low, max_high;
 
 	max_low = max_high = 0;
 
-	find_limits(mi, &min, &max_low, &max_high);
+	find_limits(&min, &max_low, &max_high);
 
-	arm_bootmem_init(mi, min, max_low);
+	arm_bootmem_init(min, max_low);
 
 	/*
 	 * Sparsemem tries to allocate bootmem in memory_present(),
@@ -328,7 +343,7 @@ void __init bootmem_init(void)
 	 * the sparse mem_map arrays initialized by sparse_init()
 	 * for memmap_init_zone(), otherwise all PFNs are invalid.
 	 */
-	arm_bootmem_free(mi, min, max_low, max_high);
+	arm_bootmem_free(min, max_low, max_high);
 
 	high_memory = __va((max_low << PAGE_SHIFT) - 1) + 1;
 
@@ -422,6 +437,57 @@ static void __init free_unused_memmap(struct meminfo *mi)
 	}
 }
 
+static void __init free_highpages(void)
+{
+#ifdef CONFIG_HIGHMEM
+	unsigned long max_low = max_low_pfn + PHYS_PFN_OFFSET;
+	int i, j;
+
+	/* set highmem page free */
+	for (i = j = 0; i < memblock.memory.cnt; i++) {
+		unsigned long start = memblock_start_pfn(&memblock.memory, i);
+		unsigned long end = memblock_end_pfn(&memblock.memory, i);
+
+		/* Ignore complete lowmem entries */
+		if (end <= max_low)
+			continue;
+
+		/* Truncate partial highmem entries */
+		if (start < max_low)
+			start = max_low;
+
+		/* Find and exclude any reserved regions */
+		for (; j < memblock.reserved.cnt; j++) {
+			unsigned long res_start;
+			unsigned long res_end;
+
+			res_start = memblock_start_pfn(&memblock.reserved, j);
+			res_end = res_start + PFN_UP(memblock_size_bytes(&memblock.reserved, j));
+
+			if (res_end < start)
+				continue;
+			if (res_start < start)
+				res_start = start;
+			if (res_start > end)
+				res_start = end;
+			if (res_end > end)
+				res_end = end;
+			if (res_start != start)
+				totalhigh_pages += free_area(start, res_start,
+							     NULL);
+			start = res_end;
+			if (start == end)
+				break;
+		}
+
+		/* And now free anything which remains */
+		if (start < end)
+			totalhigh_pages += free_area(start, end, NULL);
+	}
+	totalram_pages += totalhigh_pages;
+#endif
+}
+
 /*
  * mem_init() marks the free areas in the mem_map and tells us how much
  * memory is free.  This is done after various parts of the system have
@@ -450,16 +516,7 @@ void __init mem_init(void)
 				    __phys_to_pfn(__pa(swapper_pg_dir)), NULL);
 #endif
 
-#ifdef CONFIG_HIGHMEM
-	/* set highmem page free */
-	for_each_bank (i, &meminfo) {
-		unsigned long start = bank_pfn_start(&meminfo.bank[i]);
-		unsigned long end = bank_pfn_end(&meminfo.bank[i]);
-		if (start >= max_low_pfn + PHYS_PFN_OFFSET)
-			totalhigh_pages += free_area(start, end, NULL);
-	}
-	totalram_pages += totalhigh_pages;
-#endif
+	free_highpages();
 
 	reserved_pages = free_pages = 0;
 
@@ -489,9 +546,10 @@ void __init mem_init(void)
 	 */
 	printk(KERN_INFO "Memory:");
 	num_physpages = 0;
-	for (i = 0; i < meminfo.nr_banks; i++) {
-		num_physpages += bank_pfn_size(&meminfo.bank[i]);
-		printk(" %ldMB", bank_phys_size(&meminfo.bank[i]) >> 20);
+	for (i = 0; i < memblock.memory.cnt; i++) {
+		unsigned long pages = memblock_size_pages(&memblock.memory, i);
+		num_physpages += pages;
+		printk(" %luMB", pages >> (20 - PAGE_SHIFT));
 	}
 	printk(" = %luMB total\n", num_physpages >> (20 - PAGE_SHIFT));
 
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index e233581..6176e11 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -852,6 +852,7 @@ static void __init sanity_check_meminfo(void)
 static inline void prepare_page_table(void)
 {
 	unsigned long addr;
+	phys_addr_t end;
 
 	/*
 	 * Clear out all the mappings below the kernel image.
@@ -867,10 +868,18 @@ static inline void prepare_page_table(void)
 		pmd_clear(pmd_off_k(addr));
 
 	/*
+	 * Find the end of the first block of lowmem.  This is complicated
+	 * when we use memblock.
+	 */
+	end = memblock.memory.region[0].base + memblock.memory.region[0].size;
+	if (end >= lowmem_end_addr)
+		end = lowmem_end_addr;
+
+	/*
 	 * Clear out all the kernel space mappings, except for the first
 	 * memory bank, up to the end of the vmalloc region.
 	 */
-	for (addr = __phys_to_virt(bank_phys_end(&meminfo.bank[0]));
+	for (addr = __phys_to_virt(end);
 	     addr < VMALLOC_END; addr += PGDIR_SIZE)
 		pmd_clear(pmd_off_k(addr));
 }
@@ -987,29 +996,27 @@ static void __init kmap_init(void)
 #endif
 }
 
-static inline void map_memory_bank(struct membank *bank)
-{
-	struct map_desc map;
-
-	map.pfn = bank_pfn_start(bank);
-	map.virtual = __phys_to_virt(bank_phys_start(bank));
-	map.length = bank_phys_size(bank);
-	map.type = MT_MEMORY;
-
-	create_mapping(&map);
-}
-
 static void __init map_lowmem(void)
 {
-	struct meminfo *mi = &meminfo;
 	int i;
 
 	/* Map all the lowmem memory banks. */
-	for (i = 0; i < mi->nr_banks; i++) {
-		struct membank *bank = &mi->bank[i];
+	for (i = 0; i < memblock.memory.cnt; i++) {
+		phys_addr_t start = memblock.memory.region[i].base;
+		phys_addr_t end = start + memblock.memory.region[i].size;
+		struct map_desc map;
+
+		if (end >= lowmem_end_addr)
+			end = lowmem_end_addr;
+		if (start >= end)
+			break;
+
+		map.pfn = __phys_to_pfn(start);
+		map.virtual = __phys_to_virt(start);
+		map.length = end - start;
+		map.type = MT_MEMORY;
 
-		if (!bank->highmem)
-			map_memory_bank(bank);
+		create_mapping(&map);
 	}
 }
 
diff --git a/mm/memblock.c b/mm/memblock.c
index 43840b3..aa69191 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -196,6 +196,10 @@ static long memblock_add_region(struct memblock_region *rgn, u64 base, u64 size)
 long memblock_add(u64 base, u64 size)
 {
 	struct memblock_region *_rgn = &memblock.memory;
+	u64 end = base + size;
+
+	base = PAGE_ALIGN(base);
+	size = (end & PAGE_MASK) - base;
 
 	/* On pSeries LPAR systems, the first MEMBLOCK is our RMO region. */
 	if (base == 0)

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

[Nicolas Pitre]

On Mon, 11 Oct 2010, Catalin Marinas wrote:

> It may be just theoretical but architecture licensees implement the
> hardware according to the ARM ARM. If it says "unpredictable", they
> don't need to care much about this scenario as it is not allowed in
> software and the hardware can be further optimised (well, I think
> "unpredictable" doesn't allow hardware deadlocking or security
> implications, so the memory corruption cannot be that random, possibly
> just restricted to that memory range). It is probably ok on current
> hardware but I cannot guarantee, you would have to check on a
> case-by-case basis (CPU implementation).

I've seen real designs where "security implications" was interpreted 
rather liberally by the hardware people, and not exactly in the same way 
as software people do.


Nicolas

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

[Woodruff, Richard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1764 bytes --]

> From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm-kernel-
> bounces@lists.infradead.org] On Behalf Of Felipe Contreras
> Sent: Saturday, October 09, 2010 5:01 AM

> That's not true, drivers on ARMv6 and above do work. I still wonder
> how exactly to trigger the issue, and how often does this happen,
> because I've never seen it.

On a Cortex-A8 if you enable auxcr.asa speculative accesses will be allowed to happen more broadly (cross outside of L2).  There are few failures which have been seen in testing which go away with its disable. Most Linux kernels have this feature off.  They still prefetch but with reduced scope.

On Cortex-A9 to get performance there are prefetches at both A9 level and PL310 (L2 controller) level. Enabling them for some things gives a very good boost. It will increase your chances of hitting issues as it activates D-Side prefetch along with increasing the amount I side prefetch.  An A9 doesn't have auxcr to limit its prefetch to L2 like A8.  If there is a MMU table with a valid TLB its free game to speculate against and bring data in (for non device memory maps).

I'm told A15 gets much more aggressive here. It has dedicated hardware resources (fill buffers ++) for speculation and not just competing against the current instruction stream for these resources.

Having incoherent views and/or copies of data is not what you want. SMP should amplify the badness by doubling speculation sources and adding extra l1 caches. Killing the alias from ioremap is a good thing and something people should want for production.

Regards,
Richard W.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Felipe Contreras]

On Wed, Oct 13, 2010 at 7:17 PM, Woodruff, Richard <r-woodruff2@ti.com> wrote:
>> From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm-kernel-
>> bounces@lists.infradead.org] On Behalf Of Felipe Contreras
>> Sent: Saturday, October 09, 2010 5:01 AM
>
>> That's not true, drivers on ARMv6 and above do work. I still wonder
>> how exactly to trigger the issue, and how often does this happen,
>> because I've never seen it.
>
> On a Cortex-A8 if you enable auxcr.asa speculative accesses will be allowed to happen more broadly (cross outside of L2).  There are few failures which have been seen in testing which go away with its disable. Most Linux kernels have this feature off.  They still prefetch but with reduced scope.

I see, do you have some pointers as how to enable this to try it out?

> On Cortex-A9 to get performance there are prefetches at both A9 level and PL310 (L2 controller) level. Enabling them for some things gives a very good boost. It will increase your chances of hitting issues as it activates D-Side prefetch along with increasing the amount I side prefetch.  An A9 doesn't have auxcr to limit its prefetch to L2 like A8.  If there is a MMU table with a valid TLB its free game to speculate against and bring data in (for non device memory maps).
>
> I'm told A15 gets much more aggressive here. It has dedicated hardware resources (fill buffers ++) for speculation and not just competing against the current instruction stream for these resources.
>
> Having incoherent views and/or copies of data is not what you want. SMP should amplify the badness by doubling speculation sources and adding extra l1 caches. Killing the alias from ioremap is a good thing and something people should want for production.

Thanks for clarifying the situation.

However, nobody is proposing to not kill the ioremap() double alias,
only to make it a warning for now. One argument against this is that
would cause corruption on other parts of the system which doesn't seem
to be the case.

I take it you don't have a problem with making this a warning on .36,
and disable completely on .37 (or later). Right?

The real issue is the drivers relying on this behavior, which need to
be fixed regardless of this decision to warn, or disable.

-- 
Felipe Contreras

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

[Felipe Contreras]

On Mon, Oct 11, 2010 at 6:25 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Oct 10, 2010 at 04:52:36AM +0300, Felipe Contreras wrote:
>> At the time 'fixup' is called, 'meminfo' is empty; the tags haven't
>> been parsed. So my solution is to move the memblock_add() after
>> 'reserve', and pass 'meminfo' as an argument:
>
> Here's a different approach which will work.  This pushes ARM further
> towards using memblock for everything relating to memory init (although
> we still have the old membank stuff around.)
>
> The advantage with this is that memblock is now used as the basis for
> determining where memory is, setting up the maps, freeing memory into
> the pools, etc.
>
> What this also means is that this code in the ->reserve callback:
>
>        size = min(size, SZ_2M);
>        base = memblock_alloc(size, min(align, SZ_2M));
>        memblock_free(base, size);
>        memblock_remove(base, size);

Why align to SZ_2M both the start and end?

> will result in [base+size] being removed from the available memory,
> using highmem if available, if not from lowmem and removing it from
> the lowmem memory map - which is exactly the behaviour we want.

Makes sense to me.

>  arch/arm/mm/init.c |  160 +++++++++++++++++++++++++++++++++++-----------------
>  arch/arm/mm/mmu.c  |   43 ++++++++------
>  mm/memblock.c      |    4 +
>  3 files changed, 138 insertions(+), 69 deletions(-)

This works fine:
Tested-by: Felipe Contreras <felipe.contreras@gmail.com>

-- 
Felipe Contreras

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

[Felipe Contreras]

On Mon, Oct 11, 2010 at 3:30 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Mon, 2010-10-11 at 13:03 +0100, Felipe Contreras wrote:
>> On Mon, Oct 11, 2010 at 2:23 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Mon, 2010-10-11 at 11:39 +0100, Felipe Contreras wrote:
>> >> I will try that, but from what I can see this might still break some
>> >> drivers, right?
>> >
>> > Probably, it depends on how drivers use the memory returned by ioremap
>> > and the assumptions they make about the memory type (strict ordering,
>> > write buffers, speculation). I think such memory should be used via the
>> > I/O accessors and not directly, so you get the benefit of explicit
>> > memory barriers. If not, you have to cope with the loss of attributes
>> > because of the WC mapping.
>> >
>> > But as it has been said (and it's my opinion as well), the drivers are
>> > broken and are misusing the ioremap API.
>>
>> Indeed, that is understood by everyone.
>>
>> But what I think should happen on .36 is that the warning is on, and
>> the behavior doesn't change compared to .35. Your patch is changing
>> the behavior as you say in hacky way, which might have unintended
>> consequences in the affected drivers, I don't think that's ideal,
>> although it's better than breaking them completely.
>
> Well, even if you allow Device memory via ioremap in Linux and the
> hardware behaviour is not "unpredictable", it is not guaranteed to act
> as a Device memory. You get speculative accesses via the Normal alias
> and you may also get dirty cache lines evicted at random from the Normal
> mapping (unless you invalidated the cache before). The dirty eviction is
> valid even for pre-ARMv6 processors.

I'm not familiar with device memory, but anyway, IIUC you are saying
the normal mapping (original) might not work as expected... That's ok,
because the only alias that uses after the ioremap() is the second
one.

>> >> Anyway, I'm reading the TRM and I can't find any mention of this.
>> >> Catalin, can you point out where is this mentioned, and also, can you
>> >> confirm if this would affect only the memory that has the double
>> >> mapping, or it can corrupt other memory as well?
>> >
>> > That's in the ARM ARM (Architecture Reference Manual), not a TRM.
>> >
>> > You never know what "unpredictable" means and this is specific to the
>> > processor implementation, not the ARM ARM. You should not mix different
>> > memory types for the same page.
>>
>> Certainly, but saying corruption _will_ happen elsewhere is not
>> correct, there is no empirical evidence for that, although
>> theoretically it might happen.
>
> It may be just theoretical but architecture licensees implement the
> hardware according to the ARM ARM. If it says "unpredictable", they
> don't need to care much about this scenario as it is not allowed in
> software and the hardware can be further optimised (well, I think
> "unpredictable" doesn't allow hardware deadlocking or security
> implications, so the memory corruption cannot be that random, possibly
> just restricted to that memory range). It is probably ok on current
> hardware but I cannot guarantee, you would have to check on a
> case-by-case basis (CPU implementation).

Exactly, so it's not the end of the world if this is still allowed one
more release.

>> > Back in 2006 there was a discussion for a set of palliative measures in
>> > hardware (initially until the OSes are fixed) and these allow the same
>> > memory type but with different cacheability attributes (e.g. Normal
>> > Non-cacheable vs Normal Cacheable) given that care is taken in software
>> > wrt stale cache lines and speculation (but at least you don't get random
>> > corruption somewhere else). They would need to have the same
>> > shareability attributes though.
>> >
>> > With the hack above, MT_DEVICE_WC is actually Normal Non-cacheable
>> > memory. It needs cache flushing but it's not worse than the original
>> > ioremap allowing this. The driver may not work as expected though.
>>
>> I see, but are you saying that if the shareability is not the same,
>> then corruption elsewhere might actually happen? Or it would happen
>> most likely on the affected region? If it's the former, then I agree
>> that your patch should be used instead.
>
> Pretty much same answer as above. But why do you need different
> shareability attributes?

Well, I'm not familiar with the shareability attributes, but from what
I could grep from the code, the tidspbridge driver is doing
readl/writel on this memory, so I think device memory is supposed to
be used for that. Granted, this is probably a very bad usage from the
driver, but this was allowed on 2.6.35, so I think the same should
happen on 2.6.36, but with a warning.

However, as I said, I'm concerned about other drivers that have not
yet been detected that would be broken by 309caa9 once .36 is
released, I think your patch should be enough to detect them and at
the same time do something relatively sane. If they get broken by the
shareability change, they probably were in too bad shape anyway.

So, +1 to your patch.

FWIW, your patch seems to work with tidspbridge

Cheers.

-- 
Felipe Contreras

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

[Woodruff, Richard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3492 bytes --]


> From: Felipe Contreras [mailto:felipe.contreras@gmail.com]
> Sent: Thursday, October 14, 2010 8:48 AM

> > On a Cortex-A8 if you enable auxcr.asa speculative accesses will be allowed
> to happen more broadly (cross outside of L2).  There are few failures which
> have been seen in testing which go away with its disable. Most Linux kernels
> have this feature off.  They still prefetch but with reduced scope.
>
> I see, do you have some pointers as how to enable this to try it out?

It is bit4 of auxcr register for cortexa8. It is in the same register which enables L2 cache. The L2 bit is banked so can be accessed with normal MCR/MRC.  The other bits in this register for OMAP3 require a trustzone management smc/smi call to access. Setting the bit in the boot loader may be good enough. Code over time accessed the register in different spots so depending on your build you might be tricked. After OFF mode for MPU the register does get restored if value was saved correctly.

On OMAP the management API to call this register may vary across EMU/HS implementations. TI provided a reference for people but that doesn't mean it was followed 100%. GP devices have the same API for access.

Something like this modified u-boot extract might work for a GP device. You need to see what your device does. A N900 likely sets parameters up differently.

        /* Save r0, r12 and restore them after usage */
        __asm__ __volatile__("mov %0, r12":"=r" (j));
        __asm__ __volatile__("mov %0, r0":"=r" (i));

        /* GP Device ROM code API usage here */
        /* r12 = AUXCR Write function and r0 value */
        __asm__ __volatile__("mov r12, #0x3");
        __asm__ __volatile__("mrc p15, 0, r0, c1, c0, 1");
        __asm__ __volatile__("orr r0, r0, #0x10");  /* or in ASA bit */
        /* SMI instruction to call ROM Code API */
        __asm__ __volatile__(".word 0xE1600070");
        __asm__ __volatile__("mov r0, %0":"=r" (i));

> However, nobody is proposing to not kill the ioremap() double alias,
> only to make it a warning for now. One argument against this is that
> would cause corruption on other parts of the system which doesn't seem
> to be the case.

Maybe in your usage it has not been an issue. I have some visibility across diverse 'communities' and their issues are not always overlapping. For A8 we did have to shut down ASA for one group, other groups never reported any issue.

The probability of the problem impacting goes up with each higher performing v7.  Someone probably could construct a failure sequence even on A8. However, it may not exist in typical code today.  With A9 and A15 coming on this kind of thing needs fixing to head off pain.

> I take it you don't have a problem with making this a warning on .36,
> and disable completely on .37 (or later). Right?

Staging it seems reasonable.  It was kind of staged but ignored.

FWIW Russell seems to doing the right thing for the v7 code base against the big picture.

> The real issue is the drivers relying on this behavior, which need to
> be fixed regardless of this decision to warn, or disable.

Yes, a few drivers need updating. If you dump a few active systems mmu tables you find a few places which need changes for attribute alias. Ioremap was one generating a lot of these and was a good fix.

Regards,
Richard W.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Catalin Marinas]

On Thu, 2010-10-14 at 16:02 +0100, Felipe Contreras wrote:
> On Mon, Oct 11, 2010 at 3:30 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > Well, even if you allow Device memory via ioremap in Linux and the
> > hardware behaviour is not "unpredictable", it is not guaranteed to act
> > as a Device memory. You get speculative accesses via the Normal alias
> > and you may also get dirty cache lines evicted at random from the Normal
> > mapping (unless you invalidated the cache before). The dirty eviction is
> > valid even for pre-ARMv6 processors.
> 
> I'm not familiar with device memory, but anyway, IIUC you are saying
> the normal mapping (original) might not work as expected... That's ok,
> because the only alias that uses after the ioremap() is the second
> one.

No, I say that the Device memory that you are using may not work as
expected (i.e. Device memory). The other Normal mapping may be fine but
you don't care about it anyway.

> > It may be just theoretical but architecture licensees implement the
> > hardware according to the ARM ARM. If it says "unpredictable", they
> > don't need to care much about this scenario as it is not allowed in
> > software and the hardware can be further optimised (well, I think
> > "unpredictable" doesn't allow hardware deadlocking or security
> > implications, so the memory corruption cannot be that random, possibly
> > just restricted to that memory range). It is probably ok on current
> > hardware but I cannot guarantee, you would have to check on a
> > case-by-case basis (CPU implementation).
> 
> Exactly, so it's not the end of the world if this is still allowed one
> more release.

Would there be any incentive for driver maintainers to fix the drivers
after one more release? AFAICT, this is a violation of the ioremap API
already.

But I don't have a strong opinion, only that if we allow ioremap for one
more cycle it would at least be better to use the ioremap_wc() hack I
posted to avoid memory type aliasing (cacheability attributes aliasing
would still be present but that's not that bad).

-- 
Catalin

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

[Felipe Contreras]

On Thu, Oct 14, 2010 at 8:18 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Thu, 2010-10-14 at 16:02 +0100, Felipe Contreras wrote:
>> On Mon, Oct 11, 2010 at 3:30 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > Well, even if you allow Device memory via ioremap in Linux and the
>> > hardware behaviour is not "unpredictable", it is not guaranteed to act
>> > as a Device memory. You get speculative accesses via the Normal alias
>> > and you may also get dirty cache lines evicted at random from the Normal
>> > mapping (unless you invalidated the cache before). The dirty eviction is
>> > valid even for pre-ARMv6 processors.
>>
>> I'm not familiar with device memory, but anyway, IIUC you are saying
>> the normal mapping (original) might not work as expected... That's ok,
>> because the only alias that uses after the ioremap() is the second
>> one.
>
> No, I say that the Device memory that you are using may not work as
> expected (i.e. Device memory). The other Normal mapping may be fine but
> you don't care about it anyway.

Yes, the device memory mapping might not work as expected, it might
work as normal memory, by _may_ you are also implying it might work as
device memory on some cases. A driver might need only that, and your
change would make it _never_ work as device memory. That might or
might not be desirable, but it's a _change_, which was my point.

>> > It may be just theoretical but architecture licensees implement the
>> > hardware according to the ARM ARM. If it says "unpredictable", they
>> > don't need to care much about this scenario as it is not allowed in
>> > software and the hardware can be further optimised (well, I think
>> > "unpredictable" doesn't allow hardware deadlocking or security
>> > implications, so the memory corruption cannot be that random, possibly
>> > just restricted to that memory range). It is probably ok on current
>> > hardware but I cannot guarantee, you would have to check on a
>> > case-by-case basis (CPU implementation).
>>
>> Exactly, so it's not the end of the world if this is still allowed one
>> more release.
>
> Would there be any incentive for driver maintainers to fix the drivers
> after one more release? AFAICT, this is a violation of the ioremap API
> already.

I explained this already:

I think everyone agrees that the amount of people that try final
releases is way bigger than the ones that try release candidates, and
that's the intention. And driver maintainers (if they have actually
seen the warning) are not the only ones that can fix issues. So the
more exposure the warning has, the more chances of some contributor
fixing it.

I am not a driver maintainer, yet, for some strange reason I decided
to try 2.6.36-rc6 where I saw the warning for the first time, and I
felt compelled to fix it, and I did, thanks to Russell, but it's to
late for .36 to get this driver fixed. Who says there aren't other
people like that out there? Well, you would never know if you don't
put the warning there in the first place.

-- 
Felipe Contreras

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

[Benjamin Herrenschmidt]

On Fri, 2010-10-08 at 18:53 +0100, Russell King - ARM Linux wrote:
> As already discussed, it's nigh on impossible to unmap the existing
> direct mapped region (read the previous discussions about why this is)
> - which is precisely why there is no direct alternative solution.
> 
> The only possible solution is to exclude some memory at boot time from
> the system direct map so that it never appears in the direct map, and
> use ioremap on _that_.  Another possible alternative is to use
> highmem,
> obtain highmem pages (making sure that it doesn't fall back to lowmem)
> and remap them using interfaces such as vmap.
> 
> So there are solutions to the problem, but it seems that _no one_ is
> willing to discuss it other than "we want our old way back".
> 
> If you want the old way back, apply pressure to silicon vendors and
> ARM Ltd to change the architecture to lift this restriction - which
> will probably mean doing away with aggressive speculative prefetching
> so that it's possible to predict what will be in the cache at any
> point in time. 

Note that we have the exact same problem on powerpc. The only sane
solution is that SoCs designed around such cores or versions of the
architecture should be fully DMA coherent to avoid the need for funky
mapping attributes. Anything else is garbage HW, but sadly, it looks
like way too many idiots still find jobs as HW designers.

Cheers,
Ben.

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

[Benjamin Herrenschmidt]

On Fri, 2010-10-08 at 17:00 -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.
> 
> Any pointers to patches where people have fixed up the drivers?

Greg, this is true for normal API changes. In this case, this is silent
data corruption, I think Russell is absolutely right. That exact same
issues have been giving me nightmares on powerpc, mostly due to
(fortunately extremely rare) broken chipsets doing non coherent DMA on
processors that do not allow you architecturally to double map memory
with different attributes.

Cheers,
Ben.

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

[Felipe Contreras]

2010/10/16 Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> On Fri, 2010-10-08 at 17:00 -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.
>>
>> Any pointers to patches where people have fixed up the drivers?
>
> Greg, this is true for normal API changes. In this case, this is silent
> data corruption, I think Russell is absolutely right. That exact same
> issues have been giving me nightmares on powerpc, mostly due to
> (fortunately extremely rare) broken chipsets doing non coherent DMA on
> processors that do not allow you architecturally to double map memory
> with different attributes.

Yes, but the kernel could have a mechanism, like the constant memory
allocator so that people can configure a certain amount of RAM, like
CONFIG_CMA_RESERVE_SIZE, and then drivers could request blocks from
there which are not mapped as kernel memory. The constant memory
allocator could map them at the same time they are requested by
drivers with different attributes.

-- 
Felipe Contreras

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

[Woodruff, Richard]

> From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm-
> kernel-bounces@lists.infradead.org] On Behalf Of Benjamin
> Herrenschmidt

> > If you want the old way back, apply pressure to silicon vendors and
> > ARM Ltd to change the architecture to lift this restriction - which
> > will probably mean doing away with aggressive speculative
> prefetching
> > so that it's possible to predict what will be in the cache at any
> > point in time.
>
> Note that we have the exact same problem on powerpc. The only sane
> solution is that SoCs designed around such cores or versions of the
> architecture should be fully DMA coherent to avoid the need for funky
> mapping attributes. Anything else is garbage HW, but sadly, it looks
> like way too many idiots still find jobs as HW designers.

ARM has always been a rawer (and cheaper) environment than many other CPU architectures. Until maybe this year coherency was never even an option provided in the hardware. The vast majority of the systems currently in mass production in 2010 and before don't have coherency.  The software has to handle all the details.

Starting today and onwards coherency options will start to be seen in high end. It seems likely some of the old external IP won't be compliant and likely have bugs undercutting it.

People from high ends like x86 and PPC have different expectations and problems. Your chip set nightmares are our standard in some areas. In my narrow experience PPC pioneered many techniques 15 years back which are now just coming into ARM. They are coming in at a much reduced power and cost footprint.

Regards,
Richard W.

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

[Benjamin Herrenschmidt]

On Sun, 2010-10-17 at 08:05 -0500, Woodruff, Richard wrote:
> ARM has always been a rawer (and cheaper) environment than many other
> CPU architectures. Until maybe this year coherency was never even an
> option provided in the hardware. The vast majority of the systems
> currently in mass production in 2010 and before don't have coherency.
> The software has to handle all the details.
> 
> Starting today and onwards coherency options will start to be seen in
> high end. It seems likely some of the old external IP won't be
> compliant and likely have bugs undercutting it.
> 
> People from high ends like x86 and PPC have different expectations and
> problems. Your chip set nightmares are our standard in some areas. In
> my narrow experience PPC pioneered many techniques 15 years back which
> are now just coming into ARM. They are coming in at a much reduced
> power and cost footprint. 

This is all quite possible the the fact remains that at the end of the
day, if you're going to have a speculative memory model with prefetch
and your architecture (rightfully so) disallows aliasing of mappings
with different cache attributes, then you are screwed the minute your
devices do non-coherent DMA :-)

As Russell says, the only proper way to do that would be to set aside
memory at boot. That or provide a way to take chunks out of the linear
mapping, which may or may not be possible, I don't know how ARM does it
well enough. On some 32-bit powerpc's unfortunately, that would meant
taking whole 256M chunks out due to the way we implement it.

The main problem is usually related to the linear mapping using some
form of huge pages, I suppose this is the same for you here.

Now of course, you always have the un-satisfactory options of disabling
speculation/prefetch and/or disabling the use of such huge pages in the
linear mapping, which makes unmapping chunks of it a lot easier.

Cheers,
Ben.

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

[Colin Cross]

On Mon, Oct 11, 2010 at 8:25 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, Oct 10, 2010 at 04:52:36AM +0300, Felipe Contreras wrote:
> Here's a different approach which will work.  This pushes ARM further
> towards using memblock for everything relating to memory init (although
> we still have the old membank stuff around.)
>
> The advantage with this is that memblock is now used as the basis for
> determining where memory is, setting up the maps, freeing memory into
> the pools, etc.
>
> What this also means is that this code in the ->reserve callback:
>
>        size = min(size, SZ_2M);
>        base = memblock_alloc(size, min(align, SZ_2M));
>        memblock_free(base, size);
>        memblock_remove(base, size);
>
> will result in [base+size] being removed from the available memory,
> using highmem if available, if not from lowmem and removing it from
> the lowmem memory map - which is exactly the behaviour we want.
>
>  arch/arm/mm/init.c |  160 +++++++++++++++++++++++++++++++++++-----------------
>  arch/arm/mm/mmu.c  |   43 ++++++++------
>  mm/memblock.c      |    4 +
>  3 files changed, 138 insertions(+), 69 deletions(-)

If memblock_remove is used on the end of memory with this patch,
mem_init accesses off the end of the array of page structures because
of the discrepancy between memblock.memory and membank on the number
of the last pfn.   memblock.memory is used to determine the memory
zones in arm_bootmem_free, which eventually  is used to create the
array of page structures, but mem_init iterates over membank and calls
pfn_to_page on pfns up to bank_pfn_end.

Converting show_mem and mem_init to use memblock.memory fixes it:

diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 52192f4..6c2b8a0 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -81,18 +81,16 @@ void show_mem(void)
 {
        int free = 0, total = 0, reserved = 0;
        int shared = 0, cached = 0, slab = 0, i;
-       struct meminfo * mi = &meminfo;

        printk("Mem-info:\n");
        show_free_areas();

-       for_each_bank (i, mi) {
-               struct membank *bank = &mi->bank[i];
+       for (i = 0; i < memblock.memory.cnt; i++) {
                unsigned int pfn1, pfn2;
                struct page *page, *end;

-               pfn1 = bank_pfn_start(bank);
-               pfn2 = bank_pfn_end(bank);
+               pfn1 = memblock_start_pfn(&memblock.memory, i);
+               pfn2 = memblock_end_pfn(&memblock.memory, i);

                page = pfn_to_page(pfn1);
                end  = pfn_to_page(pfn2 - 1) + 1;
@@ -520,13 +518,12 @@ void __init mem_init(void)

        reserved_pages = free_pages = 0;

-       for_each_bank(i, &meminfo) {
-               struct membank *bank = &meminfo.bank[i];
+       for (i = 0; i < memblock.memory.cnt; i++) {
                unsigned int pfn1, pfn2;
                struct page *page, *end;

-               pfn1 = bank_pfn_start(bank);
-               pfn2 = bank_pfn_end(bank);
+               pfn1 = memblock_start_pfn(&memblock.memory, i);
+               pfn2 = memblock_end_pfn(&memblock.memory, i);

                page = pfn_to_page(pfn1);
                end  = pfn_to_page(pfn2 - 1) + 1;

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

[Russell King - ARM Linux]

On Tue, Oct 19, 2010 at 01:13:36AM -0700, Colin Cross wrote:
> If memblock_remove is used on the end of memory with this patch,
> mem_init accesses off the end of the array of page structures because
> of the discrepancy between memblock.memory and membank on the number
> of the last pfn.   memblock.memory is used to determine the memory
> zones in arm_bootmem_free, which eventually  is used to create the
> array of page structures, but mem_init iterates over membank and calls
> pfn_to_page on pfns up to bank_pfn_end.
> 
> Converting show_mem and mem_init to use memblock.memory fixes it:

I intentionally did not do this because it won't work.  membank
information is purposely not coalesced together when you have full
sparsemem regions - which may result in

	pfn_to_page(pfn) != pfn_to_page(pfn + 1) - 1

However, memblock information is coalesced, and so will cross these
boundaries.  This means using memblock instead of membank will make
things go pop.

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

[Russell King - ARM Linux]

On Tue, Oct 19, 2010 at 01:13:36AM -0700, Colin Cross wrote:
> On Mon, Oct 11, 2010 at 8:25 AM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, Oct 10, 2010 at 04:52:36AM +0300, Felipe Contreras wrote:
> > Here's a different approach which will work.  This pushes ARM further
> > towards using memblock for everything relating to memory init (although
> > we still have the old membank stuff around.)
> >
> > The advantage with this is that memblock is now used as the basis for
> > determining where memory is, setting up the maps, freeing memory into
> > the pools, etc.
> >
> > What this also means is that this code in the ->reserve callback:
> >
> >        size = min(size, SZ_2M);
> >        base = memblock_alloc(size, min(align, SZ_2M));
> >        memblock_free(base, size);
> >        memblock_remove(base, size);
> >
> > will result in [base+size] being removed from the available memory,
> > using highmem if available, if not from lowmem and removing it from
> > the lowmem memory map - which is exactly the behaviour we want.
> >
> >  arch/arm/mm/init.c |  160 +++++++++++++++++++++++++++++++++++-----------------
> >  arch/arm/mm/mmu.c  |   43 ++++++++------
> >  mm/memblock.c      |    4 +
> >  3 files changed, 138 insertions(+), 69 deletions(-)
> 
> If memblock_remove is used on the end of memory with this patch,
> mem_init accesses off the end of the array of page structures because
> of the discrepancy between memblock.memory and membank on the number
> of the last pfn.   memblock.memory is used to determine the memory
> zones in arm_bootmem_free, which eventually  is used to create the
> array of page structures, but mem_init iterates over membank and calls
> pfn_to_page on pfns up to bank_pfn_end.

BTW, reverting the change to find_limits() should resolve this problem.
Please confirm.

Re: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Uwe Kleine-König]

Hello,

[extended the audience to lkml and a few more]

On Tue, Nov 23, 2010 at 10:12:11AM +0000, Russell King - ARM Linux wrote:
> On Tue, Nov 23, 2010 at 10:43:02AM +0100, Uwe Kleine-König wrote:
> > There is no need to memzero the buffer because dma_alloc_coherent zeros
> > the memory for us.
> > 
> > This fixes:
> > 
> > 	BUG: Your driver calls ioremap() on system memory.  This leads
> > 	<4>to architecturally unpredictable behaviour on ARMv6+, and ioremap()
> > 	<4>will fail in the next kernel release.  Please fix your driver.
> > 
> > Tested-by: Michael Grzeschik <mgr@pengutronix.de>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> [...]
> > I assume sending a patch to remove the <4> is just a waste of time,
> > isn't it?
(this message is generated by:

	printk(KERN_WARNING "BUG: Your driver calls ioremap() on system memory.  This leads\n"
	       KERN_WARNING "to architecturally unpredictable behaviour on ARMv6+, and ioremap()\n"
	       KERN_WARNING "will fail in the next kernel release.  Please fix your driver.\n");

in arch/arm/mm/ioremap.c.)

> Hmm, someone changed the behaviour of printk - it used to require the
> tag after each newline.  It seems that it only requires it as the first
> few characters now, which is a pain in the backside if you split
> printk's.
> 
> IOW:
> 
> 	printk(KERN_ERR "foo bar baz ");
> 	printk("buz\n" KERN_WARNING "fiz\n");
> 
> used to result in "foo bar baz buz" being printed at error level, and
> "fiz" at warning level.  Today, you'll get "foo bar baz buz" at error
> level, and "<4>fiz" at the default level.
> 
> Note that it's not as simple as deleting the KERN_WARNING, because:
> 	printk(KERN_ERR "foo bar baz ");
> 	printk("buz\nfiz\n");
> 
> will result in the same as above except for the missing <4> - which means
> "fiz" still ends up at the wrong severity level.
> 
> Confusingly:
> 
> 	printk(KERN_ERR "foo bar baz");
> 	printk(KERN_WARNING "buz\nfiz\n");
> 
> will do as per the original, but is silly.  Wonder how many other places
> are broken by this change.
I wonder if this was intended.  It was introduced in v2.6.31-rc1~324
(printk: clean up handling of log-levels and newlines) by Linus.

Thinking about it I consider the current behaviour more sane, because
I cannot imagine a valid usecase to change the loglevel in a single
printk and having to repeat the same loglevel marker for each newline
isn't very effective, too.  This for example fixes the usage of
pr_warning et al for multiline strings.

Is it worth to add some logic to vprint that handles repeating (or even
changing) markers after a newline?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Uwe Kleine-König]

On Tue, Nov 23, 2010 at 11:39:40AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> [extended the audience to lkml and a few more]
> 
> On Tue, Nov 23, 2010 at 10:12:11AM +0000, Russell King - ARM Linux wrote:
> > On Tue, Nov 23, 2010 at 10:43:02AM +0100, Uwe Kleine-König wrote:
> > > There is no need to memzero the buffer because dma_alloc_coherent zeros
> > > the memory for us.
> > > 
> > > This fixes:
> > > 
> > > 	BUG: Your driver calls ioremap() on system memory.  This leads
> > > 	<4>to architecturally unpredictable behaviour on ARMv6+, and ioremap()
> > > 	<4>will fail in the next kernel release.  Please fix your driver.
> > > 
> > > Tested-by: Michael Grzeschik <mgr@pengutronix.de>
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > [...]
> > > I assume sending a patch to remove the <4> is just a waste of time,
> > > isn't it?
> (this message is generated by:
> 
> 	printk(KERN_WARNING "BUG: Your driver calls ioremap() on system memory.  This leads\n"
> 	       KERN_WARNING "to architecturally unpredictable behaviour on ARMv6+, and ioremap()\n"
> 	       KERN_WARNING "will fail in the next kernel release.  Please fix your driver.\n");
> 
> in arch/arm/mm/ioremap.c.)
> 
> > Hmm, someone changed the behaviour of printk - it used to require the
> > tag after each newline.  It seems that it only requires it as the first
> > few characters now, which is a pain in the backside if you split
> > printk's.
> > 
> > IOW:
> > 
> > 	printk(KERN_ERR "foo bar baz ");
> > 	printk("buz\n" KERN_WARNING "fiz\n");
> > 
> > used to result in "foo bar baz buz" being printed at error level, and
> > "fiz" at warning level.  Today, you'll get "foo bar baz buz" at error
> > level, and "<4>fiz" at the default level.
BTW, I just noticed that Linus wrote:

	Additionally, if no newline existed, one is added (unless the
	log-level is the explicit KERN_CONT marker, to explicitly show
	that it's a continuation of a previous line).

This seems to be unimplemented, otherwise the output of

	printk(KERN_ERR "foo bar baz ");
	printk("buz\n" KERN_WARNING "fiz\n");

should be

	"foo bar baz \n" at error level
	"buz\n<4>fiz\n" at default level

useless_mail_counter++;

Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Linus Torvalds]

10/11/23 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>
> BTW, I just noticed that Linus wrote:
>
>        Additionally, if no newline existed, one is added (unless the
>        log-level is the explicit KERN_CONT marker, to explicitly show
>        that it's a continuation of a previous line).
>
> This seems to be unimplemented, otherwise the output of
>
>        printk(KERN_ERR "foo bar baz ");
>        printk("buz\n" KERN_WARNING "fiz\n");
>
> should be
>
>        "foo bar baz \n" at error level
>        "buz\n<4>fiz\n" at default level

No. The KERN_WARNING in the middle of a string is always totally
bogus. There is no "should be". It's just wrong.

The "\n" is added automatically iff there is a log-level marker at the
beginning of the string (with LOG_CONT being the exception). So

   printk("foo bar baz ");
   printk(KERN_WARNING "fiz\n");

should output two lines ("foo bar baz" with the default loglevel, and
"fiz" with KERN_WARNING). Even though there is no explicit "\n" there
for the first one.

But KERN_XYZ anywhere but in the beginning of the string do not
matter. Adding newlines changes none of that. It doesn't make the
marker beginning of the string, it just makes it beginning of the
line.

   Linus

Re: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Russell King - ARM Linux]

On Wed, Nov 24, 2010 at 07:16:06AM +0900, Linus Torvalds wrote:
> No. The KERN_WARNING in the middle of a string is always totally
> bogus. There is no "should be". It's just wrong.
> 
> The "\n" is added automatically iff there is a log-level marker at the
> beginning of the string (with LOG_CONT being the exception). So
> 
>    printk("foo bar baz ");
>    printk(KERN_WARNING "fiz\n");
> 
> should output two lines ("foo bar baz" with the default loglevel, and
> "fiz" with KERN_WARNING). Even though there is no explicit "\n" there
> for the first one.
> 
> But KERN_XYZ anywhere but in the beginning of the string do not
> matter. Adding newlines changes none of that. It doesn't make the
> marker beginning of the string, it just makes it beginning of the
> line.

Oh dear.

Note that KERN_foo in the middle of strings, even after a newline are
preserved in the output.  So:

                printk(KERN_WARNING "BUG: Your driver calls ioremap() on system memory.  This leads\n"
                       KERN_WARNING "to architecturally unpredictable behaviour on ARMv6+, and ioremap()\n"
                       KERN_WARNING "will fail in the next kernel release.  Please fix your driver.\n");

results in <4>'s appearing on the console.  I've always written code
over the last 15 years assuming that after any newline in printk output,
the log level gets reset and so needs a new log level specifier.

Sounds like this is something which needs auditing as a result of your
change, and sounds like its something that kernelnewbies people could
do.  My own greps haven't revealed any cases though.

Re: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Joe Perches]

On Tue, 23 Nov 2010, Russell King - ARM Linux wrote:

> On Wed, Nov 24, 2010 at 07:16:06AM +0900, Linus Torvalds wrote:
> > No. The KERN_WARNING in the middle of a string is always totally
> > bogus. There is no "should be". It's just wrong.
> Oh dear.
> Sounds like this is something which needs auditing as a result of your
> change, and sounds like its something that kernelnewbies people could
> do.  My own greps haven't revealed any cases though.

They used to.  I tried to fix all of the ones I could find
about a year ago.

commit ad361c9884e809340f6daca80d56a9e9c871690a
Author: Joe Perches <joe@perches.com>
Date:   Mon Jul 6 13:05:40 2009 -0700

    Remove multiple KERN_ prefixes from printk formats

    Commit 5fd29d6ccbc98884569d6f3105aeca70858b3e0f ("printk: clean up
    handling of log-levels and newlines") changed printk semantics.  printk
    lines with multiple KERN_<level> prefixes are no longer emitted as
    before the patch.

    <level> is now included in the output on each additional use.

    Remove all uses of multiple KERN_<level>s in formats.

    Signed-off-by: Joe Perches <joe@perches.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

[PATCH] md: Fix single printks with multiple KERN_<level>s

[Joe Perches]

Noticed-by: Russell King <linux@arm.linux.org.uk>
Signed-off-by: Joe Perches <joe@perches.com>
---
> Note that KERN_foo in the middle of strings, even after a newline are
> preserved in the output.  So:
> 
>                 printk(KERN_WARNING "BUG: Your driver calls ioremap() on system memory.  This leads\n"
>                        KERN_WARNING "to architecturally unpredictable behaviour on ARMv6+, and ioremap()\n"
>                        KERN_WARNING "will fail in the next kernel release.  Please fix your driver.\n");
> 
> results in <4>'s appearing on the console.  I've always written code
> over the last 15 years assuming that after any newline in printk output,
> the log level gets reset and so needs a new log level specifier.
> 
> Sounds like this is something which needs auditing as a result of your
> change, and sounds like its something that kernelnewbies people could
> do.  My own greps haven't revealed any cases though.

 drivers/md/raid1.c  |    5 +++--
 drivers/md/raid10.c |    5 +++--
 drivers/md/raid5.c  |    1 -
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 45f8324..e05381b 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1027,8 +1027,9 @@ static void error(mddev_t *mddev, mdk_rdev_t *rdev)
 	} else
 		set_bit(Faulty, &rdev->flags);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
-	printk(KERN_ALERT "md/raid1:%s: Disk failure on %s, disabling device.\n"
-	       KERN_ALERT "md/raid1:%s: Operation continuing on %d devices.\n",
+	printk(KERN_ALERT
+	       "md/raid1:%s: Disk failure on %s, disabling device.\n"
+	       "md/raid1:%s: Operation continuing on %d devices.\n",
 	       mdname(mddev), bdevname(rdev->bdev, b),
 	       mdname(mddev), conf->raid_disks - mddev->degraded);
 }
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index c67aa54..686543d 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1051,8 +1051,9 @@ static void error(mddev_t *mddev, mdk_rdev_t *rdev)
 	}
 	set_bit(Faulty, &rdev->flags);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
-	printk(KERN_ALERT "md/raid10:%s: Disk failure on %s, disabling device.\n"
-	       KERN_ALERT "md/raid10:%s: Operation continuing on %d devices.\n",
+	printk(KERN_ALERT
+	       "md/raid10:%s: Disk failure on %s, disabling device.\n"
+	       "md/raid10:%s: Operation continuing on %d devices.\n",
 	       mdname(mddev), bdevname(rdev->bdev, b),
 	       mdname(mddev), conf->raid_disks - mddev->degraded);
 }
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index dc574f3..316fbe7 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -1721,7 +1721,6 @@ static void error(mddev_t *mddev, mdk_rdev_t *rdev)
 		set_bit(Faulty, &rdev->flags);
 		printk(KERN_ALERT
 		       "md/raid:%s: Disk failure on %s, disabling device.\n"
-		       KERN_ALERT
 		       "md/raid:%s: Operation continuing on %d devices.\n",
 		       mdname(mddev),
 		       bdevname(rdev->bdev, b),

Re: [PATCH] md: Fix single printks with multiple KERN_<level>s

[Neil Brown]

On Tue, 23 Nov 2010 16:57:40 -0800
Joe Perches <joe@perches.com> wrote:

> Noticed-by: Russell King <linux@arm.linux.org.uk>
> Signed-off-by: Joe Perches <joe@perches.com>

Applied by: NeilBrown

Thanks a lot!

NeilBrown


> ---
> > Note that KERN_foo in the middle of strings, even after a newline are
> > preserved in the output.  So:
> > 
> >                 printk(KERN_WARNING "BUG: Your driver calls ioremap() on system memory.  This leads\n"
> >                        KERN_WARNING "to architecturally unpredictable behaviour on ARMv6+, and ioremap()\n"
> >                        KERN_WARNING "will fail in the next kernel release.  Please fix your driver.\n");
> > 
> > results in <4>'s appearing on the console.  I've always written code
> > over the last 15 years assuming that after any newline in printk output,
> > the log level gets reset and so needs a new log level specifier.
> > 
> > Sounds like this is something which needs auditing as a result of your
> > change, and sounds like its something that kernelnewbies people could
> > do.  My own greps haven't revealed any cases though.
> 
>  drivers/md/raid1.c  |    5 +++--
>  drivers/md/raid10.c |    5 +++--
>  drivers/md/raid5.c  |    1 -
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 45f8324..e05381b 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1027,8 +1027,9 @@ static void error(mddev_t *mddev, mdk_rdev_t *rdev)
>  	} else
>  		set_bit(Faulty, &rdev->flags);
>  	set_bit(MD_CHANGE_DEVS, &mddev->flags);
> -	printk(KERN_ALERT "md/raid1:%s: Disk failure on %s, disabling device.\n"
> -	       KERN_ALERT "md/raid1:%s: Operation continuing on %d devices.\n",
> +	printk(KERN_ALERT
> +	       "md/raid1:%s: Disk failure on %s, disabling device.\n"
> +	       "md/raid1:%s: Operation continuing on %d devices.\n",
>  	       mdname(mddev), bdevname(rdev->bdev, b),
>  	       mdname(mddev), conf->raid_disks - mddev->degraded);
>  }
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index c67aa54..686543d 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1051,8 +1051,9 @@ static void error(mddev_t *mddev, mdk_rdev_t *rdev)
>  	}
>  	set_bit(Faulty, &rdev->flags);
>  	set_bit(MD_CHANGE_DEVS, &mddev->flags);
> -	printk(KERN_ALERT "md/raid10:%s: Disk failure on %s, disabling device.\n"
> -	       KERN_ALERT "md/raid10:%s: Operation continuing on %d devices.\n",
> +	printk(KERN_ALERT
> +	       "md/raid10:%s: Disk failure on %s, disabling device.\n"
> +	       "md/raid10:%s: Operation continuing on %d devices.\n",
>  	       mdname(mddev), bdevname(rdev->bdev, b),
>  	       mdname(mddev), conf->raid_disks - mddev->degraded);
>  }
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index dc574f3..316fbe7 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -1721,7 +1721,6 @@ static void error(mddev_t *mddev, mdk_rdev_t *rdev)
>  		set_bit(Faulty, &rdev->flags);
>  		printk(KERN_ALERT
>  		       "md/raid:%s: Disk failure on %s, disabling device.\n"
> -		       KERN_ALERT
>  		       "md/raid:%s: Operation continuing on %d devices.\n",
>  		       mdname(mddev),
>  		       bdevname(rdev->bdev, b),
> 
> 
> --
> 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: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Uwe Kleine-König]

Hello Linus,

On Wed, Nov 24, 2010 at 07:16:06AM +0900, Linus Torvalds wrote:
> 10/11/23 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> >
> > BTW, I just noticed that Linus wrote:
> >
> >        Additionally, if no newline existed, one is added (unless the
> >        log-level is the explicit KERN_CONT marker, to explicitly show
> >        that it's a continuation of a previous line).
> >
> > This seems to be unimplemented, otherwise the output of
> >
> >        printk(KERN_ERR "foo bar baz ");
> >        printk("buz\n" KERN_WARNING "fiz\n");
> >
> > should be
> >
> >        "foo bar baz \n" at error level
> >        "buz\n<4>fiz\n" at default level
> 
> No. The KERN_WARNING in the middle of a string is always totally
> bogus. There is no "should be". It's just wrong.
> 
> The "\n" is added automatically iff there is a log-level marker at the
> beginning of the string (with LOG_CONT being the exception).
So

	printk("anything that doesn't look like a loglevel marker"); 

always behaves like

	printk(KERN_CONT "anything that doesn't look like a loglevel marker");

so unless someone wants to print a literal kernel marker we can just do

-#define        KERN_CONT       "<c>"
+#define        KERN_CONT       ""

without any harm.

I wonder why you implemented "iff there is a log-level marker at the
beginning ot the string (with KERN_CONT being the exception)." and not
"unless there is a KERN_CONT marker".

>                                                              So
> 
>    printk("foo bar baz ");
>    printk(KERN_WARNING "fiz\n");
> 
> should output two lines ("foo bar baz" with the default loglevel, and
> "fiz" with KERN_WARNING). Even though there is no explicit "\n" there
> for the first one.
> 
> But KERN_XYZ anywhere but in the beginning of the string do not
> matter. Adding newlines changes none of that. It doesn't make the
> marker beginning of the string, it just makes it beginning of the
> line.
I see one reason to interpret markers after a newline.  In case
recursion_bug is true, printk_buf is initialized with recursion_bug_msg
and my message gets appended.  So currently the marker I pass with my
message is ignored.
Maybe wanting to fix that is just my addiction to overengineering :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 0/6] add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Hello,

> I wonder why you implemented "iff there is a log-level marker at the
> beginning ot the string (with KERN_CONT being the exception)." and not
> "unless there is a KERN_CONT marker".
While playing around with the latter variant some outputs broke (as
expected).

I don't know if we ever switch to do that, still make the continuation
lines explicit by adding the KERN_CONT marker.  These appeared when
booting am ARM/imx machine, there are probably more ...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 1/6] ARM: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 arch/arm/mm/fault-armv.c |    4 ++--
 arch/arm/mm/init.c       |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/fault-armv.c b/arch/arm/mm/fault-armv.c
index 83e59f8..c8fb352 100644
--- a/arch/arm/mm/fault-armv.c
+++ b/arch/arm/mm/fault-armv.c
@@ -258,9 +258,9 @@ void __init check_writebuffer_bugs(void)
 	}
 
 	if (v) {
-		printk("failed, %s\n", reason);
+		printk(KERN_CONT "failed, %s\n", reason);
 		shared_pte_mask = L_PTE_MT_UNCACHED;
 	} else {
-		printk("ok\n");
+		printk(KERN_CONT "ok\n");
 	}
 }
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 5164069..947663a 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -549,9 +549,9 @@ void __init mem_init(void)
 		unsigned long pages = memblock_region_memory_end_pfn(reg) -
 			memblock_region_memory_base_pfn(reg);
 		num_physpages += pages;
-		printk(" %ldMB", pages >> (20 - PAGE_SHIFT));
+		printk(KERN_CONT " %ldMB", pages >> (20 - PAGE_SHIFT));
 	}
-	printk(" = %luMB total\n", num_physpages >> (20 - PAGE_SHIFT));
+	printk(KERN_CONT " = %luMB total\n", num_physpages >> (20 - PAGE_SHIFT));
 
 	printk(KERN_NOTICE "Memory: %luk/%luk available, %luk reserved, %luK highmem\n",
 		nr_free_pages() << (PAGE_SHIFT-10),
-- 
1.7.2.3

[PATCH 2/6] block: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 block/genhd.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 5fa2b44..9437048 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -665,19 +665,19 @@ void __init printk_all_partitions(void)
 			if (part->info)
 				part_unpack_uuid(part->info->uuid, uuid);
 
-			printk("%s%s %10llu %s %s", is_part0 ? "" : "  ",
+			printk(KERN_CONT "%s%s %10llu %s %s", is_part0 ? "" : "  ",
 			       bdevt_str(part_devt(part), devt_buf),
 			       (unsigned long long)part->nr_sects >> 1,
 			       disk_name(disk, part->partno, name_buf), uuid);
 			if (is_part0) {
 				if (disk->driverfs_dev != NULL &&
 				    disk->driverfs_dev->driver != NULL)
-					printk(" driver: %s\n",
+					printk(KERN_CONT " driver: %s\n",
 					      disk->driverfs_dev->driver->name);
 				else
-					printk(" (driver?)\n");
+					printk(KERN_CONT " (driver?)\n");
 			} else
-				printk("\n");
+				printk(KERN_CONT "\n");
 		}
 		disk_part_iter_exit(&piter);
 	}
-- 
1.7.2.3

[PATCH 3/6] net: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Cc: netdev@vger.kernel.org
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/net/phy/phy.c |    4 ++--
 net/ipv4/ipconfig.c   |   32 ++++++++++++++++----------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7670aac..a8445c7 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -47,11 +47,11 @@ void phy_print_status(struct phy_device *phydev)
 	pr_info("PHY: %s - Link is %s", dev_name(&phydev->dev),
 			phydev->link ? "Up" : "Down");
 	if (phydev->link)
-		printk(" - %d/%s", phydev->speed,
+		printk(KERN_CONT " - %d/%s", phydev->speed,
 				DUPLEX_FULL == phydev->duplex ?
 				"Full" : "Half");
 
-	printk("\n");
+	printk(KERN_CONT "\n");
 }
 EXPORT_SYMBOL(phy_print_status);
 
diff --git a/net/ipv4/ipconfig.c b/net/ipv4/ipconfig.c
index 3a6e1ec..2b09775 100644
--- a/net/ipv4/ipconfig.c
+++ b/net/ipv4/ipconfig.c
@@ -1191,13 +1191,13 @@ static int __init ic_dynamic(void)
 		    (ic_proto_enabled & IC_USE_DHCP) &&
 		    ic_dhcp_msgtype != DHCPACK) {
 			ic_got_reply = 0;
-			printk(",");
+			printk(KERN_CONT ",");
 			continue;
 		}
 #endif /* IPCONFIG_DHCP */
 
 		if (ic_got_reply) {
-			printk(" OK\n");
+			printk(KERN_CONT " OK\n");
 			break;
 		}
 
@@ -1205,7 +1205,7 @@ static int __init ic_dynamic(void)
 			continue;
 
 		if (! --retries) {
-			printk(" timed out!\n");
+			printk(KERN_CONT " timed out!\n");
 			break;
 		}
 
@@ -1215,7 +1215,7 @@ static int __init ic_dynamic(void)
 		if (timeout > CONF_TIMEOUT_MAX)
 			timeout = CONF_TIMEOUT_MAX;
 
-		printk(".");
+		printk(KERN_CONT ".");
 	}
 
 #ifdef IPCONFIG_BOOTP
@@ -1236,7 +1236,7 @@ static int __init ic_dynamic(void)
 		((ic_got_reply & IC_RARP) ? "RARP"
 		 : (ic_proto_enabled & IC_USE_DHCP) ? "DHCP" : "BOOTP"),
 		&ic_servaddr);
-	printk("my address is %pI4\n", &ic_myaddr);
+	printk(KERN_CONT "my address is %pI4\n", &ic_myaddr);
 
 	return 0;
 }
@@ -1468,19 +1468,19 @@ static int __init ip_auto_config(void)
 	/*
 	 * Clue in the operator.
 	 */
-	printk("IP-Config: Complete:");
-	printk("\n     device=%s", ic_dev->name);
-	printk(", addr=%pI4", &ic_myaddr);
-	printk(", mask=%pI4", &ic_netmask);
-	printk(", gw=%pI4", &ic_gateway);
-	printk(",\n     host=%s, domain=%s, nis-domain=%s",
+	printk("IP-Config: Complete:\n");
+	printk("     device=%s", ic_dev->name);
+	printk(KERN_CONT ", addr=%pI4", &ic_myaddr);
+	printk(KERN_CONT ", mask=%pI4", &ic_netmask);
+	printk(KERN_CONT ", gw=%pI4", &ic_gateway);
+	printk(KERN_CONT ",\n     host=%s, domain=%s, nis-domain=%s",
 	       utsname()->nodename, ic_domain, utsname()->domainname);
-	printk(",\n     bootserver=%pI4", &ic_servaddr);
-	printk(", rootserver=%pI4", &root_server_addr);
-	printk(", rootpath=%s", root_server_path);
+	printk(KERN_CONT ",\n     bootserver=%pI4", &ic_servaddr);
+	printk(KERN_CONT ", rootserver=%pI4", &root_server_addr);
+	printk(KERN_CONT ", rootpath=%s", root_server_path);
 	if (ic_dev_mtu)
-		printk(", mtu=%d", ic_dev_mtu);
-	printk("\n");
+		printk(KERN_CONT ", mtu=%d", ic_dev_mtu);
+	printk(KERN_CONT "\n");
 #endif /* !SILENT */
 
 	return 0;
-- 
1.7.2.3

[PATCH 4/6] init: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 init/do_mounts.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/init/do_mounts.c b/init/do_mounts.c
index 830aaec..5d2afd4 100644
--- a/init/do_mounts.c
+++ b/init/do_mounts.c
@@ -348,8 +348,8 @@ retry:
 	printk_all_partitions();
 	printk("No filesystem could mount root, tried: ");
 	for (p = fs_names; *p; p += strlen(p)+1)
-		printk(" %s", p);
-	printk("\n");
+		printk(KERN_CONT " %s", p);
+	printk(KERN_CONT "\n");
 #ifdef CONFIG_BLOCK
 	__bdevname(ROOT_DEV, b);
 #endif
-- 
1.7.2.3

[PATCH 5/6] mm: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Cc: linux-mm@kvack.org
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 mm/percpu.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index efe8168..3356646 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1117,20 +1117,20 @@ static void pcpu_dump_alloc_info(const char *lvl,
 		for (alloc_end += gi->nr_units / upa;
 		     alloc < alloc_end; alloc++) {
 			if (!(alloc % apl)) {
-				printk("\n");
-				printk("%spcpu-alloc: ", lvl);
+				printk(KERN_CONT "\n");
+				printk("%spcpu-alloc:", lvl);
 			}
-			printk("[%0*d] ", group_width, group);
+			printk(KERN_CONT " [%0*d]", group_width, group);
 
 			for (unit_end += upa; unit < unit_end; unit++)
 				if (gi->cpu_map[unit] != NR_CPUS)
-					printk("%0*d ", cpu_width,
+					printk(KERN_CONT " %0*d", cpu_width,
 					       gi->cpu_map[unit]);
 				else
-					printk("%s ", empty_str);
+					printk(KERN_CONT " %s", empty_str);
 		}
 	}
-	printk("\n");
+	printk(KERN_CONT "\n");
 }
 
 /**
-- 
1.7.2.3

[PATCH 6/6] tty/vt: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/tty/vt/vt.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index a8ec48e..e4b05ad 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2930,11 +2930,11 @@ static int __init con_init(void)
 	gotoxy(vc, vc->vc_x, vc->vc_y);
 	csi_J(vc, 0);
 	update_screen(vc);
-	printk("Console: %s %s %dx%d",
+	printk(KERN_DEFAULT "Console: %s %s %dx%d",
 		vc->vc_can_do_color ? "colour" : "mono",
 		display_desc, vc->vc_cols, vc->vc_rows);
 	printable = 1;
-	printk("\n");
+	printk(KERN_CONT "\n");
 
 	release_console_sem();
 
@@ -3084,11 +3084,11 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
 
 	printk("Console: switching ");
 	if (!deflt)
-		printk("consoles %d-%d ", first+1, last+1);
+		printk(KERN_CONT "consoles %d-%d ", first+1, last+1);
 	if (j >= 0) {
 		struct vc_data *vc = vc_cons[j].d;
 
-		printk("to %s %s %dx%d\n",
+		printk(KERN_CONT "to %s %s %dx%d\n",
 		       vc->vc_can_do_color ? "colour" : "mono",
 		       desc, vc->vc_cols, vc->vc_rows);
 
@@ -3097,7 +3097,7 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
 			update_screen(vc);
 		}
 	} else
-		printk("to %s\n", desc);
+		printk(KERN_CONT "to %s\n", desc);
 
 	retval = 0;
 err:
-- 
1.7.2.3

Re: About multi-line printk and the need (not) to repeat loglevel markers [Was: Re: [PATCH] ARM: mx3/pcm037: properly allocate memory for mx3-camera]

[Michał Mirosław]

2010/11/24 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello Linus,
>
> On Wed, Nov 24, 2010 at 07:16:06AM +0900, Linus Torvalds wrote:
>> 10/11/23 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
>> >
>> > BTW, I just noticed that Linus wrote:
>> >
>> >        Additionally, if no newline existed, one is added (unless the
>> >        log-level is the explicit KERN_CONT marker, to explicitly show
>> >        that it's a continuation of a previous line).
>> >
>> > This seems to be unimplemented, otherwise the output of
>> >
>> >        printk(KERN_ERR "foo bar baz ");
>> >        printk("buz\n" KERN_WARNING "fiz\n");
>> >
>> > should be
>> >
>> >        "foo bar baz \n" at error level
>> >        "buz\n<4>fiz\n" at default level
>>
>> No. The KERN_WARNING in the middle of a string is always totally
>> bogus. There is no "should be". It's just wrong.
>>
>> The "\n" is added automatically iff there is a log-level marker at the
>> beginning of the string (with LOG_CONT being the exception).
> So
>
>        printk("anything that doesn't look like a loglevel marker");
>
> always behaves like
>
>        printk(KERN_CONT "anything that doesn't look like a loglevel marker");
>
> so unless someone wants to print a literal kernel marker we can just do
>
> -#define        KERN_CONT       "<c>"
> +#define        KERN_CONT       ""
>
> without any harm.
>
> I wonder why you implemented "iff there is a log-level marker at the
> beginning ot the string (with KERN_CONT being the exception)." and not
> "unless there is a KERN_CONT marker".
>
>>                                                              So
>>
>>    printk("foo bar baz ");
>>    printk(KERN_WARNING "fiz\n");
>>
>> should output two lines ("foo bar baz" with the default loglevel, and
>> "fiz" with KERN_WARNING). Even though there is no explicit "\n" there
>> for the first one.
>>
>> But KERN_XYZ anywhere but in the beginning of the string do not
>> matter. Adding newlines changes none of that. It doesn't make the
>> marker beginning of the string, it just makes it beginning of the
>> line.
> I see one reason to interpret markers after a newline.  In case
> recursion_bug is true, printk_buf is initialized with recursion_bug_msg
> and my message gets appended.  So currently the marker I pass with my
> message is ignored.
> Maybe wanting to fix that is just my addiction to overengineering :-)

Hello, Kernel Hackers!

I think that KERN_CONT and any other form of continuation printks() is
just source of trouble.

This is usually used in code like this:

int init_dev(...)
{
    ...
    printk("Initializing dev %s ... ", dev->name);
    /* do initialize, sleeping/scheduling is allowed */
    printk("%s.\n", ok ? "done" : "error");
    ...
}

When parallel initialization is done you might expect output like this:

Initializing dev A ...
Initializing dev B ... error.
done.

And now guess which one failed.

Second case against printk() continuations is netconsole. Every
printk() output is sent as separate packet with no ordering or
delivery guarantees, and usually logged as separate lines by remote
logging daemon. From the example code above, you might get this in
remote log:

Initializing dev A ...
done.
Initializing dev B ...
error.

When in reality A failed and B initialized properly. (Yes, I know this
is highly unlikely, but still - possible.)

Best Regards,
Michał Mirosław

Re: [PATCH 3/6] net: add some KERN_CONT markers to continuation lines

[David Miller]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Date: Wed, 24 Nov 2010 09:57:47 +0100

> Cc: netdev@vger.kernel.org
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied.

Re: [PATCH 6/6] tty/vt: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Hello Greg,

should this patch go via your tree?

Thanks
Uwe

On Wed, Nov 24, 2010 at 09:57:50AM +0100, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/vt/vt.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> index a8ec48e..e4b05ad 100644
> --- a/drivers/tty/vt/vt.c
> +++ b/drivers/tty/vt/vt.c
> @@ -2930,11 +2930,11 @@ static int __init con_init(void)
>  	gotoxy(vc, vc->vc_x, vc->vc_y);
>  	csi_J(vc, 0);
>  	update_screen(vc);
> -	printk("Console: %s %s %dx%d",
> +	printk(KERN_DEFAULT "Console: %s %s %dx%d",
>  		vc->vc_can_do_color ? "colour" : "mono",
>  		display_desc, vc->vc_cols, vc->vc_rows);
>  	printable = 1;
> -	printk("\n");
> +	printk(KERN_CONT "\n");
>  
>  	release_console_sem();
>  
> @@ -3084,11 +3084,11 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
>  
>  	printk("Console: switching ");
>  	if (!deflt)
> -		printk("consoles %d-%d ", first+1, last+1);
> +		printk(KERN_CONT "consoles %d-%d ", first+1, last+1);
>  	if (j >= 0) {
>  		struct vc_data *vc = vc_cons[j].d;
>  
> -		printk("to %s %s %dx%d\n",
> +		printk(KERN_CONT "to %s %s %dx%d\n",
>  		       vc->vc_can_do_color ? "colour" : "mono",
>  		       desc, vc->vc_cols, vc->vc_rows);
>  
> @@ -3097,7 +3097,7 @@ static int bind_con_driver(const struct consw *csw, int first, int last,
>  			update_screen(vc);
>  		}
>  	} else
> -		printk("to %s\n", desc);
> +		printk(KERN_CONT "to %s\n", desc);
>  
>  	retval = 0;
>  err:
> -- 
> 1.7.2.3
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 5/6] mm: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Hello,


On Wed, Nov 24, 2010 at 09:57:49AM +0100, Uwe Kleine-König wrote:
> Cc: linux-mm@kvack.org
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  mm/percpu.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index efe8168..3356646 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -1117,20 +1117,20 @@ static void pcpu_dump_alloc_info(const char *lvl,
>  		for (alloc_end += gi->nr_units / upa;
>  		     alloc < alloc_end; alloc++) {
>  			if (!(alloc % apl)) {
> -				printk("\n");
> -				printk("%spcpu-alloc: ", lvl);
> +				printk(KERN_CONT "\n");
> +				printk("%spcpu-alloc:", lvl);
>  			}
> -			printk("[%0*d] ", group_width, group);
> +			printk(KERN_CONT " [%0*d]", group_width, group);
>  
>  			for (unit_end += upa; unit < unit_end; unit++)
>  				if (gi->cpu_map[unit] != NR_CPUS)
> -					printk("%0*d ", cpu_width,
> +					printk(KERN_CONT " %0*d", cpu_width,
>  					       gi->cpu_map[unit]);
>  				else
> -					printk("%s ", empty_str);
> +					printk(KERN_CONT " %s", empty_str);
>  		}
>  	}
> -	printk("\n");
> +	printk(KERN_CONT "\n");
>  }
>  
>  /**
ping

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 6/6] tty/vt: add some KERN_CONT markers to continuation lines

[Greg KH]

On Mon, Feb 28, 2011 at 04:16:41PM +0100, Uwe Kleine-König wrote:
> Hello Greg,
> 
> should this patch go via your tree?

Yes, care to resend it in a format I can apply it in?

thanks,

greg k-h

Re: [PATCH 6/6] tty/vt: add some KERN_CONT markers to continuation lines

[Uwe Kleine-König]

Hello Greg,

On Mon, Feb 28, 2011 at 07:39:46AM -0800, Greg KH wrote:
> On Mon, Feb 28, 2011 at 04:16:41PM +0100, Uwe Kleine-König wrote:
> > Hello Greg,
> > 
> > should this patch go via your tree?
> 
> Yes, care to resend it in a format I can apply it in?
I bounced you the original mail assuming this is enough.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 6/6] tty/vt: add some KERN_CONT markers to continuation lines

[Greg KH]

On Mon, Feb 28, 2011 at 04:50:08PM +0100, Uwe Kleine-König wrote:
> Hello Greg,
> 
> On Mon, Feb 28, 2011 at 07:39:46AM -0800, Greg KH wrote:
> > On Mon, Feb 28, 2011 at 04:16:41PM +0100, Uwe Kleine-König wrote:
> > > Hello Greg,
> > > 
> > > should this patch go via your tree?
> > 
> > Yes, care to resend it in a format I can apply it in?
> I bounced you the original mail assuming this is enough.

Yup, I'll queue it up in a few days, thanks.

greg k-h

Re: [PATCH 6/6] tty/vt: add some KERN_CONT markers to continuation lines

[Greg KH]

On Wed, Nov 24, 2010 at 09:57:50AM +0100, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/tty/vt/vt.c |   10 +++++-----

This doesn't apply on the linux-next tree, care to refresh it and
resend so I can apply it?

thanks,

greg k-h

Re: [PATCH 5/6] mm: add some KERN_CONT markers to continuation lines

[Linus Torvalds]

2011/2/28 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> Hello,
>
>
> On Wed, Nov 24, 2010 at 09:57:49AM +0100, Uwe Kleine-König wrote:
>> -                             printk("\n");
>> -                             printk("%spcpu-alloc: ", lvl);
>> +                             printk(KERN_CONT "\n");
>> +                             printk("%spcpu-alloc:", lvl);

So I hate this kind of "mindless search-and-replace" patch.

The whole point is that with the modern printk semantics, the above
kind of crazy cdoe shouldn't be needed. You should be able to just
write

     printk("%spcpu-alloc:", lvl);

without that "\n" at all, because printk() will insert the \n if
necessary. So the concept of

    printk(KERN_CONT "\n")

is just crazy: you're saying "I want to continue the line, in order to
print a newline". Whaa?

>> -                     printk("[%0*d] ", group_width, group);
>> +                     printk(KERN_CONT " [%0*d]", group_width, group);
>> -                                     printk("%0*d ", cpu_width,
>> +                                     printk(KERN_CONT " %0*d", cpu_width,
>> -                                     printk("%s ", empty_str);
>> +                                     printk(KERN_CONT " %s", empty_str);

These look ok, but:

>> -     printk("\n");
>> +     printk(KERN_CONT "\n");

Same deal. Why do KERN_CONT + "\n"?

Yes, yes, it does have semantic meaning ("do newline _now_"), and can
matter if you are going to use KERN_CONT exclusively around it. But it
still smells like just being silly to me. The point of the printk
changes was to make things simpler. I really would suggest just
removing those KERN_CONT "\n" lines. Doesn't it end up looking fine
that way too?

                     Linus

Re: [PATCH 5/6] mm: add some KERN_CONT markers to continuation lines

[Joe Perches]

On Tue, 2011-03-01 at 13:46 -0800, Linus Torvalds wrote:
> the concept of
>     printk(KERN_CONT "\n")
> is just crazy: you're saying "I want to continue the line, in order to
> print a newline". Whaa?

It's a trivially useful "end of collected printk" mark,
which was made a bit superfluous by the code that added
any necessary newline before every KERN_<level>.

There are a thousand or so of them today.

$ grep -rP --include=*.[ch] "\b(printk\s*\(\s*KERN_CONT|pr_cont\s*\(|printk\s*\()\s*\"\\\n\"" * | wc -l
1061

That code made all message terminating newlines a bit
obsolete.  I won't be submitting any patches to remove
those EOM newlines any time soon.

I hope no one does that.

It would be actually useful to have some form like:

	cookie = collected_printk_start()
loop:
		collected_printk(cookie, ...) (...)
	collected_printk_end(cookie)

so that interleaved messages from multiple
concurrent streams could be sensibly collected
either post processed or buffered.