All of lore.kernel.org
 help / color / mirror / Atom feed
From: robherring2@gmail.com (Rob Herring)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC PATCH 1/3] pstore-ram: use write-combine mappings
Date: Mon, 15 Apr 2013 18:59:16 -0500	[thread overview]
Message-ID: <516C9454.4060009@gmail.com> (raw)
In-Reply-To: <CAMbhsRS1hnBRoomoiNijrXZB-938d2RSs1cu9EdSfmXD3xpSQg@mail.gmail.com>

On 04/15/2013 05:21 PM, Colin Cross wrote:
> On Wed, Apr 10, 2013 at 6:30 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On 04/09/2013 10:53 PM, Colin Cross wrote:
>>> On Tue, Apr 9, 2013 at 8:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> Atomic operations are undefined behavior on ARM for device or strongly
>>>> ordered memory types. So use write-combine variants for mappings. This
>>>> corresponds to normal, non-cacheable memory on ARM. For many other
>>>> architectures, this change should not change the mapping type.
>>>
>>> This is going to make ramconsole less reliable.  A debugging printk
>>> followed by a __raw_writel that causes an immediate hard crash is
>>> likely to lose the last updates, including the most useful message, in
>>> the write buffers.
>>
>> It would have to be a write that hangs the bus. In my experience with
>> AXI, the bus doesn't actually hang until you hit max outstanding
>> transactions.
> 
> I've seen many cases where a single write to device memory in an
> unclocked slave will completely and instantly hang all cpus, and the
> next write will never happen.
> 
>> I think exclusive stores will limit the buffering, but that is probably
>> not architecturally guaranteed.
>>
>> I could put a wb() in at the end of persistent_ram_write.
>>
>>> Also, isn't this patch unnecessary after patch 3 in this set?
>>
>> It is still needed in the main memory case to be architecturally correct
>> to avoid multiple mappings of different memory types and exclusive
>> accesses to device memory. At least on an A9, it doesn't really seem to
>> matter. I could remove this for the ioremap case.
> 
> According to my reading of the latest ARM ARM (Issue C, section
> A3.5.7), and Catalin's excellent explanation
> (http://lists.linaro.org/pipermail/linaro-dev/2012-February/010239.html),
> it is no longer considered unpredictable to have both cached and
> non-cached mappings to the same memory, as long as you use proper
> cache maintenance between accessing the two mappings.
> 
> In pstore_ram the cached mapping will never be accessed (and we don't
> care about speculative accesses), so no cache maintenance is
> necessary.  I don't see any need for this patch, and I see plenty of
> possible problems.

Exclusive accesses still have further restrictions. From section 3.4.5:

? It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be
performed to a memory region
   with the Device or Strongly-ordered memory attribute. Unless the
implementation documentation explicitly
  states that LDREX and STREX operations to a memory region with the
Device or Strongly-ordered attribute are
 permitted, the effect of such operations is UNPREDICTABLE.


Given that it is implementation defined, I don't see how Linux can rely
on that behavior.

Rob

WARNING: multiple messages have this Message-ID (diff)
From: Rob Herring <robherring2@gmail.com>
To: Colin Cross <ccross@android.com>
Cc: lkml <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Rob Herring <rob.herring@calxeda.com>,
	Anton Vorontsov <cbouatmailru@gmail.com>,
	Kees Cook <keescook@chromium.org>,
	Tony Luck <tony.luck@intel.com>,
	Will Deacon <will.deacon@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>
Subject: Re: [RFC PATCH 1/3] pstore-ram: use write-combine mappings
Date: Mon, 15 Apr 2013 18:59:16 -0500	[thread overview]
Message-ID: <516C9454.4060009@gmail.com> (raw)
In-Reply-To: <CAMbhsRS1hnBRoomoiNijrXZB-938d2RSs1cu9EdSfmXD3xpSQg@mail.gmail.com>

On 04/15/2013 05:21 PM, Colin Cross wrote:
> On Wed, Apr 10, 2013 at 6:30 AM, Rob Herring <robherring2@gmail.com> wrote:
>> On 04/09/2013 10:53 PM, Colin Cross wrote:
>>> On Tue, Apr 9, 2013 at 8:08 PM, Rob Herring <robherring2@gmail.com> wrote:
>>>> From: Rob Herring <rob.herring@calxeda.com>
>>>>
>>>> Atomic operations are undefined behavior on ARM for device or strongly
>>>> ordered memory types. So use write-combine variants for mappings. This
>>>> corresponds to normal, non-cacheable memory on ARM. For many other
>>>> architectures, this change should not change the mapping type.
>>>
>>> This is going to make ramconsole less reliable.  A debugging printk
>>> followed by a __raw_writel that causes an immediate hard crash is
>>> likely to lose the last updates, including the most useful message, in
>>> the write buffers.
>>
>> It would have to be a write that hangs the bus. In my experience with
>> AXI, the bus doesn't actually hang until you hit max outstanding
>> transactions.
> 
> I've seen many cases where a single write to device memory in an
> unclocked slave will completely and instantly hang all cpus, and the
> next write will never happen.
> 
>> I think exclusive stores will limit the buffering, but that is probably
>> not architecturally guaranteed.
>>
>> I could put a wb() in at the end of persistent_ram_write.
>>
>>> Also, isn't this patch unnecessary after patch 3 in this set?
>>
>> It is still needed in the main memory case to be architecturally correct
>> to avoid multiple mappings of different memory types and exclusive
>> accesses to device memory. At least on an A9, it doesn't really seem to
>> matter. I could remove this for the ioremap case.
> 
> According to my reading of the latest ARM ARM (Issue C, section
> A3.5.7), and Catalin's excellent explanation
> (http://lists.linaro.org/pipermail/linaro-dev/2012-February/010239.html),
> it is no longer considered unpredictable to have both cached and
> non-cached mappings to the same memory, as long as you use proper
> cache maintenance between accessing the two mappings.
> 
> In pstore_ram the cached mapping will never be accessed (and we don't
> care about speculative accesses), so no cache maintenance is
> necessary.  I don't see any need for this patch, and I see plenty of
> possible problems.

Exclusive accesses still have further restrictions. From section 3.4.5:

• It is IMPLEMENTATION DEFINED whether LDREX and STREX operations can be
performed to a memory region
   with the Device or Strongly-ordered memory attribute. Unless the
implementation documentation explicitly
  states that LDREX and STREX operations to a memory region with the
Device or Strongly-ordered attribute are
 permitted, the effect of such operations is UNPREDICTABLE.


Given that it is implementation defined, I don't see how Linux can rely
on that behavior.

Rob

  reply	other threads:[~2013-04-15 23:59 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-10  3:08 [RFC PATCH 1/3] pstore-ram: use write-combine mappings Rob Herring
2013-04-10  3:08 ` Rob Herring
2013-04-10  3:08 ` [RFC PATCH 2/3] pstore ram: remove the power of buffer size limitation Rob Herring
2013-04-10  3:08   ` Rob Herring
2013-04-10  3:08 ` [RFC PATCH 3/3] pstore/ram: avoid atomic accesses for ioremapped regions Rob Herring
2013-04-10  3:08   ` Rob Herring
2013-04-10  4:10   ` Colin Cross
2013-04-10  4:10     ` Colin Cross
2013-04-10 15:55     ` Rob Herring
2013-04-10 15:55       ` Rob Herring
2013-04-10  3:53 ` [RFC PATCH 1/3] pstore-ram: use write-combine mappings Colin Cross
2013-04-10  3:53   ` Colin Cross
2013-04-10 13:30   ` Rob Herring
2013-04-10 13:30     ` Rob Herring
2013-04-15 22:21     ` Colin Cross
2013-04-15 22:21       ` Colin Cross
2013-04-15 23:59       ` Rob Herring [this message]
2013-04-15 23:59         ` Rob Herring
2013-04-16  0:43         ` Colin Cross
2013-04-16  0:43           ` Colin Cross
2013-04-16  8:44           ` Will Deacon
2013-04-16  8:44             ` Will Deacon
2013-04-16 12:58             ` Rob Herring
2013-04-16 12:58               ` Rob Herring
2013-04-16 13:48               ` Catalin Marinas
2013-04-16 13:48                 ` Catalin Marinas
2013-04-19  9:54   ` Russell King - ARM Linux
2013-04-19  9:54     ` Russell King - ARM Linux
  -- strict thread matches above, loose matches on Subject: below --
2013-06-19 11:22 Wei Ni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=516C9454.4060009@gmail.com \
    --to=robherring2@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.