* [PATCH 0/2 v5] powerpc/kvm: support to handle sw breakpoint
From: Madhavan Srinivasan @ 2014-09-07 16:31 UTC (permalink / raw)
To: agraf, benh, paulus, mpe; +Cc: Madhavan Srinivasan, linuxppc-dev, kvm-ppc, kvm
This patchset adds ppc64 server side support for software breakpoint
and extends the use of illegal instruction as software
breakpoint across ppc platform.
Patch 1, adds kernel side support for software breakpoint.
Design is that, by using an illegal instruction, we trap to
hypervisor via Emulation Assistance interrupt, where we check
for the illegal instruction and accordingly we return to Host
or Guest. Patch also adds support for software breakpoint
in PR KVM.
Patch 2,extends the use of illegal instruction as software
breakpoint instruction across the ppc platform. Patch extends
booke program interrupt code to support software breakpoint.
Patch 2 is only compile tested. Will really help if
someone can try it out and let me know comments.
Changes v4->v5:
Made changes to code comments and commit messages
Added debugging active checks for illegal instr comparison
Added debug instruction check in emulate code
Extended SW breakpoint to booke
Changes v3->v4:
Made changes to code comments and removed #define of zero opcode
Added a new function to handle the debug instruction emulation in book3s_hv
Rebased the code to latest upstream source.
Changes v2->v3:
Changed the debug instructions. Using the all zero opcode in the instruction word
as illegal instruction as mentioned in Power ISA instead of ABS
Removed reg updated in emulation assist and added a call to
kvmppc_emulate_instruction for reg update.
Changes v1->v2:
Moved the debug instruction #def to kvm_book3s.h. This way PR_KVM can also share it.
Added code to use KVM get one reg infrastructure to get debug opcode.
Updated emulate.c to include emulation of debug instruction incase of PR_KVM.
Made changes to commit message.
Madhavan Srinivasan (2):
powerpc/kvm: support to handle sw breakpoint
powerpc/kvm: common sw breakpoint instr across ppc
arch/powerpc/include/asm/kvm_ppc.h | 6 ++++++
arch/powerpc/kvm/book3s.c | 3 ++-
arch/powerpc/kvm/book3s_hv.c | 41 ++++++++++++++++++++++++++++++++++----
arch/powerpc/kvm/book3s_pr.c | 3 +++
arch/powerpc/kvm/booke.c | 18 +++++++++++++++--
arch/powerpc/kvm/emulate.c | 18 +++++++++++++++++
6 files changed, 82 insertions(+), 7 deletions(-)
--
1.7.11.4
^ permalink raw reply
* [PATCH 1/2 v5] powerpc/kvm: support to handle sw breakpoint
From: Madhavan Srinivasan @ 2014-09-07 16:31 UTC (permalink / raw)
To: agraf, benh, paulus, mpe; +Cc: Madhavan Srinivasan, linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1410107494-25556-1-git-send-email-maddy@linux.vnet.ibm.com>
This patch adds kernel side support for software breakpoint.
Design is that, by using an illegal instruction, we trap to hypervisor
via Emulation Assistance interrupt, where we check for the illegal instruction
and accordingly we return to Host or Guest. Patch also adds support for
software breakpoint in PR KVM.
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/kvm_ppc.h | 6 ++++++
arch/powerpc/kvm/book3s.c | 3 ++-
arch/powerpc/kvm/book3s_hv.c | 41 ++++++++++++++++++++++++++++++++++----
arch/powerpc/kvm/book3s_pr.c | 3 +++
arch/powerpc/kvm/emulate.c | 18 +++++++++++++++++
5 files changed, 66 insertions(+), 5 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index fb86a22..dd83c9a 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -38,6 +38,12 @@
#include <asm/paca.h>
#endif
+/*
+ * KVMPPC_INST_SW_BREAKPOINT is debug Instruction
+ * for supporting software breakpoint.
+ */
+#define KVMPPC_INST_SW_BREAKPOINT 0x00dddd00
+
enum emulation_result {
EMULATE_DONE, /* no further processing */
EMULATE_DO_MMIO, /* kvm_run filled with MMIO request */
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index dd03f6b..00e9c9f 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -778,7 +778,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
struct kvm_guest_debug *dbg)
{
- return -EINVAL;
+ vcpu->guest_debug = dbg->control;
+ return 0;
}
void kvmppc_decrementer_func(unsigned long data)
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 27cced9..3a2414c 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -725,6 +725,30 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
return kvmppc_hcall_impl_hv_realmode(cmd);
}
+static int kvmppc_emulate_debug_inst(struct kvm_run *run,
+ struct kvm_vcpu *vcpu)
+{
+ u32 last_inst;
+
+ if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
+ EMULATE_DONE) {
+ /*
+ * Fetch failed, so return to guest and
+ * try executing it again.
+ */
+ return RESUME_GUEST;
+ } else {
+ if (last_inst == KVMPPC_INST_SW_BREAKPOINT) {
+ run->exit_reason = KVM_EXIT_DEBUG;
+ run->debug.arch.address = kvmppc_get_pc(vcpu);
+ return RESUME_HOST;
+ } else {
+ kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+ return RESUME_GUEST;
+ }
+ }
+}
+
static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
struct task_struct *tsk)
{
@@ -807,12 +831,18 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
break;
/*
* This occurs if the guest executes an illegal instruction.
- * We just generate a program interrupt to the guest, since
- * we don't emulate any guest instructions at this stage.
+ * If the guest debug is disabled, generate a program interrupt
+ * to the guest. If guest debug is enabled, we need to check
+ * whether the instruction is a software breakpoint instruction.
+ * Accordingly return to Guest or Host.
*/
case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
- kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
- r = RESUME_GUEST;
+ if(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
+ r = kvmppc_emulate_debug_inst(run, vcpu);
+ } else {
+ kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+ r = RESUME_GUEST;
+ }
break;
/*
* This occurs if the guest (kernel or userspace), does something that
@@ -922,6 +952,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
long int i;
switch (id) {
+ case KVM_REG_PPC_DEBUG_INST:
+ *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
+ break;
case KVM_REG_PPC_HIOR:
*val = get_reg_val(id, 0);
break;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index faffb27..6d73708 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -1319,6 +1319,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
int r = 0;
switch (id) {
+ case KVM_REG_PPC_DEBUG_INST:
+ *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
+ break;
case KVM_REG_PPC_HIOR:
*val = get_reg_val(id, to_book3s(vcpu)->hior);
break;
diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index e96b50d..30f326b 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -274,6 +274,24 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
}
break;
+ case 0:
+ /*
+ * Instruction with primary opcode 0. Based on PowerISA
+ * these are illegal instructions.
+ */
+ if(inst == KVMPPC_INST_SW_BREAKPOINT) {
+ run->exit_reason = KVM_EXIT_DEBUG;
+ run->debug.arch.address = kvmppc_get_pc(vcpu);
+ emulated = EMULATE_EXIT_USER;
+ advance = 0;
+ printk(KERN_INFO "pr_emulate: debug instr \n");
+ } else {
+ kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+ emulated = EMULATE_DONE;
+ advance = 0;
+ }
+ break;
+
default:
emulated = EMULATE_FAIL;
}
--
1.7.11.4
^ permalink raw reply related
* [PATCH 2/2 v5] powerpc/kvm: common sw breakpoint instr across ppc
From: Madhavan Srinivasan @ 2014-09-07 16:31 UTC (permalink / raw)
To: agraf, benh, paulus, mpe; +Cc: Madhavan Srinivasan, linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1410107494-25556-1-git-send-email-maddy@linux.vnet.ibm.com>
This patch extends the use of illegal instruction as software
breakpoint instruction across the ppc platform. Patch extends
booke program interrupt code to support software breakpoint.
Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
---
Patch is only compile tested. Will really help if
someone can try it out and let me know comments.
arch/powerpc/kvm/booke.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index b4c89fa..1b84853 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -870,6 +870,10 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
case BOOKE_INTERRUPT_HV_PRIV:
emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
break;
+ case BOOKE_INTERRUPT_PROGRAM:
+ /*SW breakpoints arrive as illegal instructions on HV */
+ emulated = kvmppc_get_last_inst(vcpu, false, &last_inst);
+ break;
default:
break;
}
@@ -947,7 +951,17 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
break;
case BOOKE_INTERRUPT_PROGRAM:
- if (vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) {
+ if ((vcpu->arch.shared->msr & (MSR_PR | MSR_GS)) &&
+ (last_inst == KVMPPC_INST_SW_BREAKPOINT)) {
+ /*
+ * We are here because of an SW breakpoint instr,
+ * so lets return to host to handle.
+ */
+ r = kvmppc_handle_debug(run, vcpu);
+ run->exit_reason = KVM_EXIT_DEBUG;
+ kvmppc_account_exit(vcpu, DEBUG_EXITS);
+ break;
+ } else {
/*
* Program traps generated by user-level software must
* be handled by the guest kernel.
@@ -1505,7 +1519,7 @@ int kvm_vcpu_ioctl_get_one_reg(struct kvm_vcpu *vcpu, struct kvm_one_reg *reg)
val = get_reg_val(reg->id, vcpu->arch.tsr);
break;
case KVM_REG_PPC_DEBUG_INST:
- val = get_reg_val(reg->id, KVMPPC_INST_EHPRIV_DEBUG);
+ val = get_reg_val(reg->id, KVMPPC_INST_SW_BREAKPOINT);
break;
case KVM_REG_PPC_VRSAVE:
val = get_reg_val(reg->id, vcpu->arch.vrsave);
--
1.7.11.4
^ permalink raw reply related
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-07 19:04 UTC (permalink / raw)
To: paulmck
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Tony Luck, Paul Mackerras, H. Peter Anvin, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <20140907162146.GK5001@linux.vnet.ibm.com>
On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
> On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
> > On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
> > > On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
> > > > Hi James,
> > > >
> > > > On 09/04/2014 10:11 PM, James Bottomley wrote:
> > > > > On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
> > > > >> +And there are anti-guarantees:
> > > > >> +
> > > > >> + (*) These guarantees do not apply to bitfields, because compilers often
> > > > >> + generate code to modify these using non-atomic read-modify-write
> > > > >> + sequences. Do not attempt to use bitfields to synchronize parallel
> > > > >> + algorithms.
> > > > >> +
> > > > >> + (*) Even in cases where bitfields are protected by locks, all fields
> > > > >> + in a given bitfield must be protected by one lock. If two fields
> > > > >> + in a given bitfield are protected by different locks, the compiler's
> > > > >> + non-atomic read-modify-write sequences can cause an update to one
> > > > >> + field to corrupt the value of an adjacent field.
> > > > >> +
> > > > >> + (*) These guarantees apply only to properly aligned and sized scalar
> > > > >> + variables. "Properly sized" currently means "int" and "long",
> > > > >> + because some CPU families do not support loads and stores of
> > > > >> + other sizes. ("Some CPU families" is currently believed to
> > > > >> + be only Alpha 21064. If this is actually the case, a different
> > > > >> + non-guarantee is likely to be formulated.)
> > > > >
> > > > > This is a bit unclear. Presumably you're talking about definiteness of
> > > > > the outcome (as in what's seen after multiple stores to the same
> > > > > variable).
> > > >
> > > > No, the last conditions refers to adjacent byte stores from different
> > > > cpu contexts (either interrupt or SMP).
> > > >
> > > > > The guarantees are only for natural width on Parisc as well,
> > > > > so you would get a mess if you did byte stores to adjacent memory
> > > > > locations.
> > > >
> > > > For a simple test like:
> > > >
> > > > struct x {
> > > > long a;
> > > > char b;
> > > > char c;
> > > > char d;
> > > > char e;
> > > > };
> > > >
> > > > void store_bc(struct x *p) {
> > > > p->b = 1;
> > > > p->c = 2;
> > > > }
> > > >
> > > > on parisc, gcc generates separate byte stores
> > > >
> > > > void store_bc(struct x *p) {
> > > > 0: 34 1c 00 02 ldi 1,ret0
> > > > 4: 0f 5c 12 08 stb ret0,4(r26)
> > > > 8: 34 1c 00 04 ldi 2,ret0
> > > > c: e8 40 c0 00 bv r0(rp)
> > > > 10: 0f 5c 12 0a stb ret0,5(r26)
> > > >
> > > > which appears to confirm that on parisc adjacent byte data
> > > > is safe from corruption by concurrent cpu updates; that is,
> > > >
> > > > CPU 0 | CPU 1
> > > > |
> > > > p->b = 1 | p->c = 2
> > > > |
> > > >
> > > > will result in p->b == 1 && p->c == 2 (assume both values
> > > > were 0 before the call to store_bc()).
> > >
> > > What Peter said. I would ask for suggestions for better wording, but
> > > I would much rather be able to say that single-byte reads and writes
> > > are atomic and that aligned-short reads and writes are also atomic.
> > >
> > > Thus far, it looks like we lose only very old Alpha systems, so unless
> > > I hear otherwise, I update my patch to outlaw these very old systems.
> >
> > This isn't universally true according to the architecture manual. The
> > PARISC CPU can make byte to long word stores atomic against the memory
> > bus but not against the I/O bus for instance. Atomicity is a property
> > of the underlying substrate, not of the CPU. Implying that atomicity is
> > a CPU property is incorrect.
>
> OK, fair point.
>
> But are there in-use-for-Linux PARISC memory fabrics (for normal memory,
> not I/O) that do not support single-byte and double-byte stores?
For aligned access, I believe that's always the case for the memory bus
(on both 32 and 64 bit systems). However, it only applies to machine
instruction loads and stores of the same width.. If you mix the widths
on the loads and stores, all bets are off. That means you have to
beware of the gcc penchant for coalescing loads and stores: if it sees
two adjacent byte stores it can coalesce them into a short store
instead ... that screws up the atomicity guarantees.
James
^ permalink raw reply
* Re: [PATCH 0/2] PCI/MSI: Remove arch_msi_check_device()
From: Alexander Gordeev @ 2014-09-07 19:07 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-pci@vger.kernel.org, linuxppc-dev,
linux-kernel@vger.kernel.org
In-Reply-To: <CAErSpo4XN01hgTUpcca=i-=LFV_bM-Cok8614LJzrOfVbx8MAA@mail.gmail.com>
On Fri, Sep 05, 2014 at 03:27:49PM -0600, Bjorn Helgaas wrote:
> > I applied these (with Michael's ack on the first, and v2 of the second) to
> > pci/msi for v3.18, thanks!
Hi Bjorn,
I resent a series with updates that fix kbuild robot errors.
Hopefully, the rebase for pci/msi would not cause trouble for anyone.
> Oh, I forgot -- if you'd rather take the first one through the PPC
> tree, you can do that and I can merge the second one later. Let me
> know if you want to do that.
Nah, your treee is just fine.
Thanks!
> Bjorn
--
Regards,
Alexander Gordeev
agordeev@redhat.com
^ permalink raw reply
* Re: bit fields && data tearing
From: Peter Hurley @ 2014-09-07 20:41 UTC (permalink / raw)
To: James Bottomley, paulmck
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Tony Luck,
Paul Mackerras, H. Peter Anvin, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <1410116687.2027.19.camel@jarvis.lan>
On 09/07/2014 03:04 PM, James Bottomley wrote:
> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
>> On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
>>> On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
>>>> On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
>>>>> Hi James,
>>>>>
>>>>> On 09/04/2014 10:11 PM, James Bottomley wrote:
>>>>>> On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
>>>>>>> +And there are anti-guarantees:
>>>>>>> +
>>>>>>> + (*) These guarantees do not apply to bitfields, because compilers often
>>>>>>> + generate code to modify these using non-atomic read-modify-write
>>>>>>> + sequences. Do not attempt to use bitfields to synchronize parallel
>>>>>>> + algorithms.
>>>>>>> +
>>>>>>> + (*) Even in cases where bitfields are protected by locks, all fields
>>>>>>> + in a given bitfield must be protected by one lock. If two fields
>>>>>>> + in a given bitfield are protected by different locks, the compiler's
>>>>>>> + non-atomic read-modify-write sequences can cause an update to one
>>>>>>> + field to corrupt the value of an adjacent field.
>>>>>>> +
>>>>>>> + (*) These guarantees apply only to properly aligned and sized scalar
>>>>>>> + variables. "Properly sized" currently means "int" and "long",
>>>>>>> + because some CPU families do not support loads and stores of
>>>>>>> + other sizes. ("Some CPU families" is currently believed to
>>>>>>> + be only Alpha 21064. If this is actually the case, a different
>>>>>>> + non-guarantee is likely to be formulated.)
>>>>>>
>>>>>> This is a bit unclear. Presumably you're talking about definiteness of
>>>>>> the outcome (as in what's seen after multiple stores to the same
>>>>>> variable).
>>>>>
>>>>> No, the last conditions refers to adjacent byte stores from different
>>>>> cpu contexts (either interrupt or SMP).
>>>>>
>>>>>> The guarantees are only for natural width on Parisc as well,
>>>>>> so you would get a mess if you did byte stores to adjacent memory
>>>>>> locations.
>>>>>
>>>>> For a simple test like:
>>>>>
>>>>> struct x {
>>>>> long a;
>>>>> char b;
>>>>> char c;
>>>>> char d;
>>>>> char e;
>>>>> };
>>>>>
>>>>> void store_bc(struct x *p) {
>>>>> p->b = 1;
>>>>> p->c = 2;
>>>>> }
>>>>>
>>>>> on parisc, gcc generates separate byte stores
>>>>>
>>>>> void store_bc(struct x *p) {
>>>>> 0: 34 1c 00 02 ldi 1,ret0
>>>>> 4: 0f 5c 12 08 stb ret0,4(r26)
>>>>> 8: 34 1c 00 04 ldi 2,ret0
>>>>> c: e8 40 c0 00 bv r0(rp)
>>>>> 10: 0f 5c 12 0a stb ret0,5(r26)
>>>>>
>>>>> which appears to confirm that on parisc adjacent byte data
>>>>> is safe from corruption by concurrent cpu updates; that is,
>>>>>
>>>>> CPU 0 | CPU 1
>>>>> |
>>>>> p->b = 1 | p->c = 2
>>>>> |
>>>>>
>>>>> will result in p->b == 1 && p->c == 2 (assume both values
>>>>> were 0 before the call to store_bc()).
>>>>
>>>> What Peter said. I would ask for suggestions for better wording, but
>>>> I would much rather be able to say that single-byte reads and writes
>>>> are atomic and that aligned-short reads and writes are also atomic.
>>>>
>>>> Thus far, it looks like we lose only very old Alpha systems, so unless
>>>> I hear otherwise, I update my patch to outlaw these very old systems.
>>>
>>> This isn't universally true according to the architecture manual. The
>>> PARISC CPU can make byte to long word stores atomic against the memory
>>> bus but not against the I/O bus for instance. Atomicity is a property
>>> of the underlying substrate, not of the CPU. Implying that atomicity is
>>> a CPU property is incorrect.
To go back to this briefly, while it's true that atomicity is not
a CPU property, atomicity is not possible if the CPU is not cooperating.
>> OK, fair point.
>>
>> But are there in-use-for-Linux PARISC memory fabrics (for normal memory,
>> not I/O) that do not support single-byte and double-byte stores?
>
> For aligned access, I believe that's always the case for the memory bus
> (on both 32 and 64 bit systems). However, it only applies to machine
> instruction loads and stores of the same width.. If you mix the widths
> on the loads and stores, all bets are off. That means you have to
> beware of the gcc penchant for coalescing loads and stores: if it sees
> two adjacent byte stores it can coalesce them into a short store
> instead ... that screws up the atomicity guarantees.
Two things: I think that gcc has given up on combining adjacent writes,
perhaps because unaligned writes on some arches are prohibitive, so
whatever minor optimization was believed to be gained was quickly lost,
multi-fold. (Although this might not be the reason since one would
expect that gcc would know the alignment of the promoted store).
But additionally, even if gcc combines adjacent writes _that are part
of the program flow_ then I believe the situation is no worse than
would otherwise exist.
For instance, given the following:
struct x {
spinlock_t lock;
long a;
byte b;
byte c;
};
void locked_store_b(struct x *p)
{
spin_lock(&p->lock);
p->b = 1;
spin_unlock(&p->lock);
p->c = 2;
}
Granted, the author probably expects ordered writes of
STORE B
STORE C
but that's not guaranteed because there is no memory barrier
ordering B before C.
I see no problem with gcc write-combining in the absence of
memory barriers (as long as alignment is still respected,
ie., the size-promoted write is still naturally aligned). The
combined write is still atomic.
Regards,
Peter Hurley
^ permalink raw reply
* Re: bit fields && data tearing
From: Paul E. McKenney @ 2014-09-07 23:00 UTC (permalink / raw)
To: James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Tony Luck, Paul Mackerras, H. Peter Anvin, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <1410116687.2027.19.camel@jarvis.lan>
On Sun, Sep 07, 2014 at 12:04:47PM -0700, James Bottomley wrote:
> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
> > On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
> > > On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
> > > > On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
> > > > > Hi James,
> > > > >
> > > > > On 09/04/2014 10:11 PM, James Bottomley wrote:
> > > > > > On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
> > > > > >> +And there are anti-guarantees:
> > > > > >> +
> > > > > >> + (*) These guarantees do not apply to bitfields, because compilers often
> > > > > >> + generate code to modify these using non-atomic read-modify-write
> > > > > >> + sequences. Do not attempt to use bitfields to synchronize parallel
> > > > > >> + algorithms.
> > > > > >> +
> > > > > >> + (*) Even in cases where bitfields are protected by locks, all fields
> > > > > >> + in a given bitfield must be protected by one lock. If two fields
> > > > > >> + in a given bitfield are protected by different locks, the compiler's
> > > > > >> + non-atomic read-modify-write sequences can cause an update to one
> > > > > >> + field to corrupt the value of an adjacent field.
> > > > > >> +
> > > > > >> + (*) These guarantees apply only to properly aligned and sized scalar
> > > > > >> + variables. "Properly sized" currently means "int" and "long",
> > > > > >> + because some CPU families do not support loads and stores of
> > > > > >> + other sizes. ("Some CPU families" is currently believed to
> > > > > >> + be only Alpha 21064. If this is actually the case, a different
> > > > > >> + non-guarantee is likely to be formulated.)
> > > > > >
> > > > > > This is a bit unclear. Presumably you're talking about definiteness of
> > > > > > the outcome (as in what's seen after multiple stores to the same
> > > > > > variable).
> > > > >
> > > > > No, the last conditions refers to adjacent byte stores from different
> > > > > cpu contexts (either interrupt or SMP).
> > > > >
> > > > > > The guarantees are only for natural width on Parisc as well,
> > > > > > so you would get a mess if you did byte stores to adjacent memory
> > > > > > locations.
> > > > >
> > > > > For a simple test like:
> > > > >
> > > > > struct x {
> > > > > long a;
> > > > > char b;
> > > > > char c;
> > > > > char d;
> > > > > char e;
> > > > > };
> > > > >
> > > > > void store_bc(struct x *p) {
> > > > > p->b = 1;
> > > > > p->c = 2;
> > > > > }
> > > > >
> > > > > on parisc, gcc generates separate byte stores
> > > > >
> > > > > void store_bc(struct x *p) {
> > > > > 0: 34 1c 00 02 ldi 1,ret0
> > > > > 4: 0f 5c 12 08 stb ret0,4(r26)
> > > > > 8: 34 1c 00 04 ldi 2,ret0
> > > > > c: e8 40 c0 00 bv r0(rp)
> > > > > 10: 0f 5c 12 0a stb ret0,5(r26)
> > > > >
> > > > > which appears to confirm that on parisc adjacent byte data
> > > > > is safe from corruption by concurrent cpu updates; that is,
> > > > >
> > > > > CPU 0 | CPU 1
> > > > > |
> > > > > p->b = 1 | p->c = 2
> > > > > |
> > > > >
> > > > > will result in p->b == 1 && p->c == 2 (assume both values
> > > > > were 0 before the call to store_bc()).
> > > >
> > > > What Peter said. I would ask for suggestions for better wording, but
> > > > I would much rather be able to say that single-byte reads and writes
> > > > are atomic and that aligned-short reads and writes are also atomic.
> > > >
> > > > Thus far, it looks like we lose only very old Alpha systems, so unless
> > > > I hear otherwise, I update my patch to outlaw these very old systems.
> > >
> > > This isn't universally true according to the architecture manual. The
> > > PARISC CPU can make byte to long word stores atomic against the memory
> > > bus but not against the I/O bus for instance. Atomicity is a property
> > > of the underlying substrate, not of the CPU. Implying that atomicity is
> > > a CPU property is incorrect.
> >
> > OK, fair point.
> >
> > But are there in-use-for-Linux PARISC memory fabrics (for normal memory,
> > not I/O) that do not support single-byte and double-byte stores?
>
> For aligned access, I believe that's always the case for the memory bus
> (on both 32 and 64 bit systems). However, it only applies to machine
> instruction loads and stores of the same width.. If you mix the widths
> on the loads and stores, all bets are off. That means you have to
> beware of the gcc penchant for coalescing loads and stores: if it sees
> two adjacent byte stores it can coalesce them into a short store
> instead ... that screws up the atomicity guarantees.
OK, that means that to make PARISC work reliably, we need to use
ACCESS_ONCE() for loads and stores that could have racing accesses.
If I understand correctly, this will -not- be needed for code guarded
by locks, even with Peter's examples.
So if we have something like this:
struct foo {
char a;
char b;
};
struct foo *fp;
then this code would be bad:
fp->a = 1;
fp->b = 2;
The reason is (as you say) that GCC would be happy to store 0x0102
(or vice versa, depending on endianness) to the pair. We instead
need:
ACCESS_ONCE(fp->a) = 1;
ACCESS_ONCE(fp->b) = 2;
However, if the code is protected by locks, no problem:
struct foo {
spinlock_t lock_a;
spinlock_t lock_b;
char a;
char b;
};
Then it is OK to do the following:
spin_lock(fp->lock_a);
fp->a = 1;
spin_unlock(fp->lock_a);
spin_lock(fp->lock_b);
fp->b = 1;
spin_unlock(fp->lock_b);
Or even this, assuming ->lock_a precedes ->lock_b in the locking hierarchy:
spin_lock(fp->lock_a);
spin_lock(fp->lock_b);
fp->a = 1;
fp->b = 1;
spin_unlock(fp->lock_a);
spin_unlock(fp->lock_b);
Here gcc might merge the assignments to fp->a and fp->b, but that is OK
because both locks are held, presumably preventing other assignments or
references to fp->a and fp->b.
On the other hand, if either fp->a or fp->b are referenced outside of their
respective locks, even once, then this last code fragment would still need
ACCESS_ONCE() as follows:
spin_lock(fp->lock_a);
spin_lock(fp->lock_b);
ACCESS_ONCE(fp->a) = 1;
ACCESS_ONCE(fp->b) = 1;
spin_unlock(fp->lock_a);
spin_unlock(fp->lock_b);
Does that cover it? If so, I will update memory-barriers.txt accordingly.
Thanx, Paul
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-07 23:17 UTC (permalink / raw)
To: paulmck, James Bottomley
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <20140907230019.GO5001@linux.vnet.ibm.com>
I'm confused why storing 0x0102 would be a problem. I think gcc does that even on other cpus.
More atomicity can't hurt, can it?
On September 7, 2014 4:00:19 PM PDT, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>On Sun, Sep 07, 2014 at 12:04:47PM -0700, James Bottomley wrote:
>> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
>> > On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
>> > > On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
>> > > > On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
>> > > > > Hi James,
>> > > > >
>> > > > > On 09/04/2014 10:11 PM, James Bottomley wrote:
>> > > > > > On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
>> > > > > >> +And there are anti-guarantees:
>> > > > > >> +
>> > > > > >> + (*) These guarantees do not apply to bitfields, because
>compilers often
>> > > > > >> + generate code to modify these using non-atomic
>read-modify-write
>> > > > > >> + sequences. Do not attempt to use bitfields to
>synchronize parallel
>> > > > > >> + algorithms.
>> > > > > >> +
>> > > > > >> + (*) Even in cases where bitfields are protected by
>locks, all fields
>> > > > > >> + in a given bitfield must be protected by one lock.
>If two fields
>> > > > > >> + in a given bitfield are protected by different
>locks, the compiler's
>> > > > > >> + non-atomic read-modify-write sequences can cause an
>update to one
>> > > > > >> + field to corrupt the value of an adjacent field.
>> > > > > >> +
>> > > > > >> + (*) These guarantees apply only to properly aligned and
>sized scalar
>> > > > > >> + variables. "Properly sized" currently means "int"
>and "long",
>> > > > > >> + because some CPU families do not support loads and
>stores of
>> > > > > >> + other sizes. ("Some CPU families" is currently
>believed to
>> > > > > >> + be only Alpha 21064. If this is actually the case,
>a different
>> > > > > >> + non-guarantee is likely to be formulated.)
>> > > > > >
>> > > > > > This is a bit unclear. Presumably you're talking about
>definiteness of
>> > > > > > the outcome (as in what's seen after multiple stores to the
>same
>> > > > > > variable).
>> > > > >
>> > > > > No, the last conditions refers to adjacent byte stores from
>different
>> > > > > cpu contexts (either interrupt or SMP).
>> > > > >
>> > > > > > The guarantees are only for natural width on Parisc as
>well,
>> > > > > > so you would get a mess if you did byte stores to adjacent
>memory
>> > > > > > locations.
>> > > > >
>> > > > > For a simple test like:
>> > > > >
>> > > > > struct x {
>> > > > > long a;
>> > > > > char b;
>> > > > > char c;
>> > > > > char d;
>> > > > > char e;
>> > > > > };
>> > > > >
>> > > > > void store_bc(struct x *p) {
>> > > > > p->b = 1;
>> > > > > p->c = 2;
>> > > > > }
>> > > > >
>> > > > > on parisc, gcc generates separate byte stores
>> > > > >
>> > > > > void store_bc(struct x *p) {
>> > > > > 0: 34 1c 00 02 ldi 1,ret0
>> > > > > 4: 0f 5c 12 08 stb ret0,4(r26)
>> > > > > 8: 34 1c 00 04 ldi 2,ret0
>> > > > > c: e8 40 c0 00 bv r0(rp)
>> > > > > 10: 0f 5c 12 0a stb ret0,5(r26)
>> > > > >
>> > > > > which appears to confirm that on parisc adjacent byte data
>> > > > > is safe from corruption by concurrent cpu updates; that is,
>> > > > >
>> > > > > CPU 0 | CPU 1
>> > > > > |
>> > > > > p->b = 1 | p->c = 2
>> > > > > |
>> > > > >
>> > > > > will result in p->b == 1 && p->c == 2 (assume both values
>> > > > > were 0 before the call to store_bc()).
>> > > >
>> > > > What Peter said. I would ask for suggestions for better
>wording, but
>> > > > I would much rather be able to say that single-byte reads and
>writes
>> > > > are atomic and that aligned-short reads and writes are also
>atomic.
>> > > >
>> > > > Thus far, it looks like we lose only very old Alpha systems, so
>unless
>> > > > I hear otherwise, I update my patch to outlaw these very old
>systems.
>> > >
>> > > This isn't universally true according to the architecture manual.
> The
>> > > PARISC CPU can make byte to long word stores atomic against the
>memory
>> > > bus but not against the I/O bus for instance. Atomicity is a
>property
>> > > of the underlying substrate, not of the CPU. Implying that
>atomicity is
>> > > a CPU property is incorrect.
>> >
>> > OK, fair point.
>> >
>> > But are there in-use-for-Linux PARISC memory fabrics (for normal
>memory,
>> > not I/O) that do not support single-byte and double-byte stores?
>>
>> For aligned access, I believe that's always the case for the memory
>bus
>> (on both 32 and 64 bit systems). However, it only applies to machine
>> instruction loads and stores of the same width.. If you mix the
>widths
>> on the loads and stores, all bets are off. That means you have to
>> beware of the gcc penchant for coalescing loads and stores: if it
>sees
>> two adjacent byte stores it can coalesce them into a short store
>> instead ... that screws up the atomicity guarantees.
>
>OK, that means that to make PARISC work reliably, we need to use
>ACCESS_ONCE() for loads and stores that could have racing accesses.
>If I understand correctly, this will -not- be needed for code guarded
>by locks, even with Peter's examples.
>
>So if we have something like this:
>
> struct foo {
> char a;
> char b;
> };
> struct foo *fp;
>
>then this code would be bad:
>
> fp->a = 1;
> fp->b = 2;
>
>The reason is (as you say) that GCC would be happy to store 0x0102
>(or vice versa, depending on endianness) to the pair. We instead
>need:
>
> ACCESS_ONCE(fp->a) = 1;
> ACCESS_ONCE(fp->b) = 2;
>
>However, if the code is protected by locks, no problem:
>
> struct foo {
> spinlock_t lock_a;
> spinlock_t lock_b;
> char a;
> char b;
> };
>
>Then it is OK to do the following:
>
> spin_lock(fp->lock_a);
> fp->a = 1;
> spin_unlock(fp->lock_a);
> spin_lock(fp->lock_b);
> fp->b = 1;
> spin_unlock(fp->lock_b);
>
>Or even this, assuming ->lock_a precedes ->lock_b in the locking
>hierarchy:
>
> spin_lock(fp->lock_a);
> spin_lock(fp->lock_b);
> fp->a = 1;
> fp->b = 1;
> spin_unlock(fp->lock_a);
> spin_unlock(fp->lock_b);
>
>Here gcc might merge the assignments to fp->a and fp->b, but that is OK
>because both locks are held, presumably preventing other assignments or
>references to fp->a and fp->b.
>
>On the other hand, if either fp->a or fp->b are referenced outside of
>their
>respective locks, even once, then this last code fragment would still
>need
>ACCESS_ONCE() as follows:
>
> spin_lock(fp->lock_a);
> spin_lock(fp->lock_b);
> ACCESS_ONCE(fp->a) = 1;
> ACCESS_ONCE(fp->b) = 1;
> spin_unlock(fp->lock_a);
> spin_unlock(fp->lock_b);
>
>Does that cover it? If so, I will update memory-barriers.txt
>accordingly.
>
> Thanx, Paul
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
^ permalink raw reply
* Re: bit fields && data tearing
From: Paul E. McKenney @ 2014-09-07 23:36 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
James Bottomley, Paul Mackerras, linux-arch, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <6092b453-e0c9-4f6d-922b-48bce988f774@email.android.com>
On Sun, Sep 07, 2014 at 04:17:30PM -0700, H. Peter Anvin wrote:
> I'm confused why storing 0x0102 would be a problem. I think gcc does that even on other cpus.
>
> More atomicity can't hurt, can it?
I must defer to James for any additional details on why PARISC systems
don't provide atomicity for partially overlapping stores. ;-)
Thanx, Paul
> On September 7, 2014 4:00:19 PM PDT, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >On Sun, Sep 07, 2014 at 12:04:47PM -0700, James Bottomley wrote:
> >> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
> >> > On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
> >> > > On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
> >> > > > On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
> >> > > > > Hi James,
> >> > > > >
> >> > > > > On 09/04/2014 10:11 PM, James Bottomley wrote:
> >> > > > > > On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
> >> > > > > >> +And there are anti-guarantees:
> >> > > > > >> +
> >> > > > > >> + (*) These guarantees do not apply to bitfields, because
> >compilers often
> >> > > > > >> + generate code to modify these using non-atomic
> >read-modify-write
> >> > > > > >> + sequences. Do not attempt to use bitfields to
> >synchronize parallel
> >> > > > > >> + algorithms.
> >> > > > > >> +
> >> > > > > >> + (*) Even in cases where bitfields are protected by
> >locks, all fields
> >> > > > > >> + in a given bitfield must be protected by one lock.
> >If two fields
> >> > > > > >> + in a given bitfield are protected by different
> >locks, the compiler's
> >> > > > > >> + non-atomic read-modify-write sequences can cause an
> >update to one
> >> > > > > >> + field to corrupt the value of an adjacent field.
> >> > > > > >> +
> >> > > > > >> + (*) These guarantees apply only to properly aligned and
> >sized scalar
> >> > > > > >> + variables. "Properly sized" currently means "int"
> >and "long",
> >> > > > > >> + because some CPU families do not support loads and
> >stores of
> >> > > > > >> + other sizes. ("Some CPU families" is currently
> >believed to
> >> > > > > >> + be only Alpha 21064. If this is actually the case,
> >a different
> >> > > > > >> + non-guarantee is likely to be formulated.)
> >> > > > > >
> >> > > > > > This is a bit unclear. Presumably you're talking about
> >definiteness of
> >> > > > > > the outcome (as in what's seen after multiple stores to the
> >same
> >> > > > > > variable).
> >> > > > >
> >> > > > > No, the last conditions refers to adjacent byte stores from
> >different
> >> > > > > cpu contexts (either interrupt or SMP).
> >> > > > >
> >> > > > > > The guarantees are only for natural width on Parisc as
> >well,
> >> > > > > > so you would get a mess if you did byte stores to adjacent
> >memory
> >> > > > > > locations.
> >> > > > >
> >> > > > > For a simple test like:
> >> > > > >
> >> > > > > struct x {
> >> > > > > long a;
> >> > > > > char b;
> >> > > > > char c;
> >> > > > > char d;
> >> > > > > char e;
> >> > > > > };
> >> > > > >
> >> > > > > void store_bc(struct x *p) {
> >> > > > > p->b = 1;
> >> > > > > p->c = 2;
> >> > > > > }
> >> > > > >
> >> > > > > on parisc, gcc generates separate byte stores
> >> > > > >
> >> > > > > void store_bc(struct x *p) {
> >> > > > > 0: 34 1c 00 02 ldi 1,ret0
> >> > > > > 4: 0f 5c 12 08 stb ret0,4(r26)
> >> > > > > 8: 34 1c 00 04 ldi 2,ret0
> >> > > > > c: e8 40 c0 00 bv r0(rp)
> >> > > > > 10: 0f 5c 12 0a stb ret0,5(r26)
> >> > > > >
> >> > > > > which appears to confirm that on parisc adjacent byte data
> >> > > > > is safe from corruption by concurrent cpu updates; that is,
> >> > > > >
> >> > > > > CPU 0 | CPU 1
> >> > > > > |
> >> > > > > p->b = 1 | p->c = 2
> >> > > > > |
> >> > > > >
> >> > > > > will result in p->b == 1 && p->c == 2 (assume both values
> >> > > > > were 0 before the call to store_bc()).
> >> > > >
> >> > > > What Peter said. I would ask for suggestions for better
> >wording, but
> >> > > > I would much rather be able to say that single-byte reads and
> >writes
> >> > > > are atomic and that aligned-short reads and writes are also
> >atomic.
> >> > > >
> >> > > > Thus far, it looks like we lose only very old Alpha systems, so
> >unless
> >> > > > I hear otherwise, I update my patch to outlaw these very old
> >systems.
> >> > >
> >> > > This isn't universally true according to the architecture manual.
> > The
> >> > > PARISC CPU can make byte to long word stores atomic against the
> >memory
> >> > > bus but not against the I/O bus for instance. Atomicity is a
> >property
> >> > > of the underlying substrate, not of the CPU. Implying that
> >atomicity is
> >> > > a CPU property is incorrect.
> >> >
> >> > OK, fair point.
> >> >
> >> > But are there in-use-for-Linux PARISC memory fabrics (for normal
> >memory,
> >> > not I/O) that do not support single-byte and double-byte stores?
> >>
> >> For aligned access, I believe that's always the case for the memory
> >bus
> >> (on both 32 and 64 bit systems). However, it only applies to machine
> >> instruction loads and stores of the same width.. If you mix the
> >widths
> >> on the loads and stores, all bets are off. That means you have to
> >> beware of the gcc penchant for coalescing loads and stores: if it
> >sees
> >> two adjacent byte stores it can coalesce them into a short store
> >> instead ... that screws up the atomicity guarantees.
> >
> >OK, that means that to make PARISC work reliably, we need to use
> >ACCESS_ONCE() for loads and stores that could have racing accesses.
> >If I understand correctly, this will -not- be needed for code guarded
> >by locks, even with Peter's examples.
> >
> >So if we have something like this:
> >
> > struct foo {
> > char a;
> > char b;
> > };
> > struct foo *fp;
> >
> >then this code would be bad:
> >
> > fp->a = 1;
> > fp->b = 2;
> >
> >The reason is (as you say) that GCC would be happy to store 0x0102
> >(or vice versa, depending on endianness) to the pair. We instead
> >need:
> >
> > ACCESS_ONCE(fp->a) = 1;
> > ACCESS_ONCE(fp->b) = 2;
> >
> >However, if the code is protected by locks, no problem:
> >
> > struct foo {
> > spinlock_t lock_a;
> > spinlock_t lock_b;
> > char a;
> > char b;
> > };
> >
> >Then it is OK to do the following:
> >
> > spin_lock(fp->lock_a);
> > fp->a = 1;
> > spin_unlock(fp->lock_a);
> > spin_lock(fp->lock_b);
> > fp->b = 1;
> > spin_unlock(fp->lock_b);
> >
> >Or even this, assuming ->lock_a precedes ->lock_b in the locking
> >hierarchy:
> >
> > spin_lock(fp->lock_a);
> > spin_lock(fp->lock_b);
> > fp->a = 1;
> > fp->b = 1;
> > spin_unlock(fp->lock_a);
> > spin_unlock(fp->lock_b);
> >
> >Here gcc might merge the assignments to fp->a and fp->b, but that is OK
> >because both locks are held, presumably preventing other assignments or
> >references to fp->a and fp->b.
> >
> >On the other hand, if either fp->a or fp->b are referenced outside of
> >their
> >respective locks, even once, then this last code fragment would still
> >need
> >ACCESS_ONCE() as follows:
> >
> > spin_lock(fp->lock_a);
> > spin_lock(fp->lock_b);
> > ACCESS_ONCE(fp->a) = 1;
> > ACCESS_ONCE(fp->b) = 1;
> > spin_unlock(fp->lock_a);
> > spin_unlock(fp->lock_b);
> >
> >Does that cover it? If so, I will update memory-barriers.txt
> >accordingly.
> >
> > Thanx, Paul
>
> --
> Sent from my mobile phone. Please pardon brevity and lack of formatting.
>
^ permalink raw reply
* Re: bit fields && data tearing
From: H. Peter Anvin @ 2014-09-07 23:39 UTC (permalink / raw)
To: paulmck
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
James Bottomley, Paul Mackerras, linux-arch, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <20140907233655.GR5001@linux.vnet.ibm.com>
How many PARISC systems do we have that actually do real work on Linux?
On September 7, 2014 4:36:55 PM PDT, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>On Sun, Sep 07, 2014 at 04:17:30PM -0700, H. Peter Anvin wrote:
>> I'm confused why storing 0x0102 would be a problem. I think gcc does
>that even on other cpus.
>>
>> More atomicity can't hurt, can it?
>
>I must defer to James for any additional details on why PARISC systems
>don't provide atomicity for partially overlapping stores. ;-)
>
> Thanx, Paul
>
>> On September 7, 2014 4:00:19 PM PDT, "Paul E. McKenney"
><paulmck@linux.vnet.ibm.com> wrote:
>> >On Sun, Sep 07, 2014 at 12:04:47PM -0700, James Bottomley wrote:
>> >> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
>> >> > On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
>> >> > > On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
>> >> > > > On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley
>wrote:
>> >> > > > > Hi James,
>> >> > > > >
>> >> > > > > On 09/04/2014 10:11 PM, James Bottomley wrote:
>> >> > > > > > On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney
>wrote:
>> >> > > > > >> +And there are anti-guarantees:
>> >> > > > > >> +
>> >> > > > > >> + (*) These guarantees do not apply to bitfields,
>because
>> >compilers often
>> >> > > > > >> + generate code to modify these using non-atomic
>> >read-modify-write
>> >> > > > > >> + sequences. Do not attempt to use bitfields to
>> >synchronize parallel
>> >> > > > > >> + algorithms.
>> >> > > > > >> +
>> >> > > > > >> + (*) Even in cases where bitfields are protected by
>> >locks, all fields
>> >> > > > > >> + in a given bitfield must be protected by one
>lock.
>> >If two fields
>> >> > > > > >> + in a given bitfield are protected by different
>> >locks, the compiler's
>> >> > > > > >> + non-atomic read-modify-write sequences can cause
>an
>> >update to one
>> >> > > > > >> + field to corrupt the value of an adjacent field.
>> >> > > > > >> +
>> >> > > > > >> + (*) These guarantees apply only to properly aligned
>and
>> >sized scalar
>> >> > > > > >> + variables. "Properly sized" currently means
>"int"
>> >and "long",
>> >> > > > > >> + because some CPU families do not support loads
>and
>> >stores of
>> >> > > > > >> + other sizes. ("Some CPU families" is currently
>> >believed to
>> >> > > > > >> + be only Alpha 21064. If this is actually the
>case,
>> >a different
>> >> > > > > >> + non-guarantee is likely to be formulated.)
>> >> > > > > >
>> >> > > > > > This is a bit unclear. Presumably you're talking about
>> >definiteness of
>> >> > > > > > the outcome (as in what's seen after multiple stores to
>the
>> >same
>> >> > > > > > variable).
>> >> > > > >
>> >> > > > > No, the last conditions refers to adjacent byte stores
>from
>> >different
>> >> > > > > cpu contexts (either interrupt or SMP).
>> >> > > > >
>> >> > > > > > The guarantees are only for natural width on Parisc as
>> >well,
>> >> > > > > > so you would get a mess if you did byte stores to
>adjacent
>> >memory
>> >> > > > > > locations.
>> >> > > > >
>> >> > > > > For a simple test like:
>> >> > > > >
>> >> > > > > struct x {
>> >> > > > > long a;
>> >> > > > > char b;
>> >> > > > > char c;
>> >> > > > > char d;
>> >> > > > > char e;
>> >> > > > > };
>> >> > > > >
>> >> > > > > void store_bc(struct x *p) {
>> >> > > > > p->b = 1;
>> >> > > > > p->c = 2;
>> >> > > > > }
>> >> > > > >
>> >> > > > > on parisc, gcc generates separate byte stores
>> >> > > > >
>> >> > > > > void store_bc(struct x *p) {
>> >> > > > > 0: 34 1c 00 02 ldi 1,ret0
>> >> > > > > 4: 0f 5c 12 08 stb ret0,4(r26)
>> >> > > > > 8: 34 1c 00 04 ldi 2,ret0
>> >> > > > > c: e8 40 c0 00 bv r0(rp)
>> >> > > > > 10: 0f 5c 12 0a stb ret0,5(r26)
>> >> > > > >
>> >> > > > > which appears to confirm that on parisc adjacent byte data
>> >> > > > > is safe from corruption by concurrent cpu updates; that
>is,
>> >> > > > >
>> >> > > > > CPU 0 | CPU 1
>> >> > > > > |
>> >> > > > > p->b = 1 | p->c = 2
>> >> > > > > |
>> >> > > > >
>> >> > > > > will result in p->b == 1 && p->c == 2 (assume both values
>> >> > > > > were 0 before the call to store_bc()).
>> >> > > >
>> >> > > > What Peter said. I would ask for suggestions for better
>> >wording, but
>> >> > > > I would much rather be able to say that single-byte reads
>and
>> >writes
>> >> > > > are atomic and that aligned-short reads and writes are also
>> >atomic.
>> >> > > >
>> >> > > > Thus far, it looks like we lose only very old Alpha systems,
>so
>> >unless
>> >> > > > I hear otherwise, I update my patch to outlaw these very old
>> >systems.
>> >> > >
>> >> > > This isn't universally true according to the architecture
>manual.
>> > The
>> >> > > PARISC CPU can make byte to long word stores atomic against
>the
>> >memory
>> >> > > bus but not against the I/O bus for instance. Atomicity is a
>> >property
>> >> > > of the underlying substrate, not of the CPU. Implying that
>> >atomicity is
>> >> > > a CPU property is incorrect.
>> >> >
>> >> > OK, fair point.
>> >> >
>> >> > But are there in-use-for-Linux PARISC memory fabrics (for normal
>> >memory,
>> >> > not I/O) that do not support single-byte and double-byte stores?
>> >>
>> >> For aligned access, I believe that's always the case for the
>memory
>> >bus
>> >> (on both 32 and 64 bit systems). However, it only applies to
>machine
>> >> instruction loads and stores of the same width.. If you mix the
>> >widths
>> >> on the loads and stores, all bets are off. That means you have to
>> >> beware of the gcc penchant for coalescing loads and stores: if it
>> >sees
>> >> two adjacent byte stores it can coalesce them into a short store
>> >> instead ... that screws up the atomicity guarantees.
>> >
>> >OK, that means that to make PARISC work reliably, we need to use
>> >ACCESS_ONCE() for loads and stores that could have racing accesses.
>> >If I understand correctly, this will -not- be needed for code
>guarded
>> >by locks, even with Peter's examples.
>> >
>> >So if we have something like this:
>> >
>> > struct foo {
>> > char a;
>> > char b;
>> > };
>> > struct foo *fp;
>> >
>> >then this code would be bad:
>> >
>> > fp->a = 1;
>> > fp->b = 2;
>> >
>> >The reason is (as you say) that GCC would be happy to store 0x0102
>> >(or vice versa, depending on endianness) to the pair. We instead
>> >need:
>> >
>> > ACCESS_ONCE(fp->a) = 1;
>> > ACCESS_ONCE(fp->b) = 2;
>> >
>> >However, if the code is protected by locks, no problem:
>> >
>> > struct foo {
>> > spinlock_t lock_a;
>> > spinlock_t lock_b;
>> > char a;
>> > char b;
>> > };
>> >
>> >Then it is OK to do the following:
>> >
>> > spin_lock(fp->lock_a);
>> > fp->a = 1;
>> > spin_unlock(fp->lock_a);
>> > spin_lock(fp->lock_b);
>> > fp->b = 1;
>> > spin_unlock(fp->lock_b);
>> >
>> >Or even this, assuming ->lock_a precedes ->lock_b in the locking
>> >hierarchy:
>> >
>> > spin_lock(fp->lock_a);
>> > spin_lock(fp->lock_b);
>> > fp->a = 1;
>> > fp->b = 1;
>> > spin_unlock(fp->lock_a);
>> > spin_unlock(fp->lock_b);
>> >
>> >Here gcc might merge the assignments to fp->a and fp->b, but that is
>OK
>> >because both locks are held, presumably preventing other assignments
>or
>> >references to fp->a and fp->b.
>> >
>> >On the other hand, if either fp->a or fp->b are referenced outside
>of
>> >their
>> >respective locks, even once, then this last code fragment would
>still
>> >need
>> >ACCESS_ONCE() as follows:
>> >
>> > spin_lock(fp->lock_a);
>> > spin_lock(fp->lock_b);
>> > ACCESS_ONCE(fp->a) = 1;
>> > ACCESS_ONCE(fp->b) = 1;
>> > spin_unlock(fp->lock_a);
>> > spin_unlock(fp->lock_b);
>> >
>> >Does that cover it? If so, I will update memory-barriers.txt
>> >accordingly.
>> >
>> > Thanx, Paul
>>
>> --
>> Sent from my mobile phone. Please pardon brevity and lack of
>formatting.
>>
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
^ permalink raw reply
* [PATCH v2 3/3] PCI/MSI: Remove arch_msi_check_device()
From: Alexander Gordeev @ 2014-09-07 18:57 UTC (permalink / raw)
To: linux-kernel; +Cc: Alexander Gordeev, linuxppc-dev, linux-pci
In-Reply-To: <cover.1410078503.git.agordeev@redhat.com>
There are no archs that override arch_msi_check_device()
hook. Remove it as it is completely redundant.
If an arch would need to check MSI/MSI-X possibility for a
device it should make it within arch_setup_msi_irqs() hook.
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
drivers/pci/msi.c | 49 +++++++++++++------------------------------------
include/linux/msi.h | 3 ---
2 files changed, 13 insertions(+), 39 deletions(-)
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 5a40516..6c2cc41 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -56,16 +56,6 @@ void __weak arch_teardown_msi_irq(unsigned int irq)
chip->teardown_irq(chip, irq);
}
-int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
-{
- struct msi_chip *chip = dev->bus->msi;
-
- if (!chip || !chip->check_device)
- return 0;
-
- return chip->check_device(chip, dev, nvec, type);
-}
-
int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
{
struct msi_desc *entry;
@@ -806,22 +796,23 @@ out_free:
}
/**
- * pci_msi_check_device - check whether MSI may be enabled on a device
+ * pci_msi_supported - check whether MSI may be enabled on a device
* @dev: pointer to the pci_dev data structure of MSI device function
* @nvec: how many MSIs have been requested ?
- * @type: are we checking for MSI or MSI-X ?
*
* Look at global flags, the device itself, and its parent buses
* to determine if MSI/-X are supported for the device. If MSI/-X is
* supported return 0, else return an error code.
**/
-static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
+static int pci_msi_supported(struct pci_dev *dev, int nvec)
{
struct pci_bus *bus;
- int ret;
/* MSI must be globally enabled and supported by the device */
- if (!pci_msi_enable || !dev || dev->no_msi)
+ if (!pci_msi_enable)
+ return -EINVAL;
+
+ if (!dev || dev->no_msi || dev->current_state != PCI_D0)
return -EINVAL;
/*
@@ -843,10 +834,6 @@ static int pci_msi_check_device(struct pci_dev *dev, int nvec, int type)
if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
return -EINVAL;
- ret = arch_msi_check_device(dev, nvec, type);
- if (ret)
- return ret;
-
return 0;
}
@@ -949,13 +936,13 @@ int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries, int nvec)
int status, nr_entries;
int i, j;
- if (!entries || !dev->msix_cap || dev->current_state != PCI_D0)
- return -EINVAL;
-
- status = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSIX);
+ status = pci_msi_supported(dev, nvec);
if (status)
return status;
+ if (!entries)
+ return -EINVAL;
+
nr_entries = pci_msix_vec_count(dev);
if (nr_entries < 0)
return nr_entries;
@@ -1062,8 +1049,9 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
int nvec;
int rc;
- if (dev->current_state != PCI_D0)
- return -EINVAL;
+ rc = pci_msi_supported(dev, minvec);
+ if (rc)
+ return rc;
WARN_ON(!!dev->msi_enabled);
@@ -1086,17 +1074,6 @@ int pci_enable_msi_range(struct pci_dev *dev, int minvec, int maxvec)
nvec = maxvec;
do {
- rc = pci_msi_check_device(dev, nvec, PCI_CAP_ID_MSI);
- if (rc < 0) {
- return rc;
- } else if (rc > 0) {
- if (rc < minvec)
- return -ENOSPC;
- nvec = rc;
- }
- } while (rc);
-
- do {
rc = msi_capability_init(dev, nvec);
if (rc < 0) {
return rc;
diff --git a/include/linux/msi.h b/include/linux/msi.h
index 8103f32..dbf7cc9 100644
--- a/include/linux/msi.h
+++ b/include/linux/msi.h
@@ -60,7 +60,6 @@ int arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc);
void arch_teardown_msi_irq(unsigned int irq);
int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
void arch_teardown_msi_irqs(struct pci_dev *dev);
-int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
void arch_restore_msi_irqs(struct pci_dev *dev);
void default_teardown_msi_irqs(struct pci_dev *dev);
@@ -77,8 +76,6 @@ struct msi_chip {
int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
struct msi_desc *desc);
void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
- int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
- int nvec, int type);
};
#endif /* LINUX_MSI_H */
--
1.9.3
^ permalink raw reply related
* [PATCH v2 1/3] PCI/MSI/PPC: Remove arch_msi_check_device()
From: Alexander Gordeev @ 2014-09-07 18:57 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-pci, Alexander Gordeev, linuxppc-dev
In-Reply-To: <cover.1410078503.git.agordeev@redhat.com>
Moving MSI checks from arch_msi_check_device() function to
arch_setup_msi_irqs() function makes code more compact and
allows removing unnecessary hook arch_msi_check_device()
from generic MSI code.
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Alexander Gordeev <agordeev@redhat.com>
---
arch/powerpc/include/asm/machdep.h | 2 --
arch/powerpc/kernel/msi.c | 12 +---------
arch/powerpc/platforms/cell/axon_msi.c | 9 --------
arch/powerpc/platforms/powernv/pci.c | 19 ++++-----------
arch/powerpc/platforms/pseries/msi.c | 42 +++++++++++++---------------------
arch/powerpc/sysdev/fsl_msi.c | 12 +++-------
arch/powerpc/sysdev/mpic_pasemi_msi.c | 11 ++-------
arch/powerpc/sysdev/mpic_u3msi.c | 28 +++++++++--------------
arch/powerpc/sysdev/ppc4xx_hsta_msi.c | 18 +++++----------
arch/powerpc/sysdev/ppc4xx_msi.c | 19 +++++----------
10 files changed, 50 insertions(+), 122 deletions(-)
diff --git a/arch/powerpc/include/asm/machdep.h b/arch/powerpc/include/asm/machdep.h
index b125cea..3af7216 100644
--- a/arch/powerpc/include/asm/machdep.h
+++ b/arch/powerpc/include/asm/machdep.h
@@ -136,8 +136,6 @@ struct machdep_calls {
int (*pci_setup_phb)(struct pci_controller *host);
#ifdef CONFIG_PCI_MSI
- int (*msi_check_device)(struct pci_dev* dev,
- int nvec, int type);
int (*setup_msi_irqs)(struct pci_dev *dev,
int nvec, int type);
void (*teardown_msi_irqs)(struct pci_dev *dev);
diff --git a/arch/powerpc/kernel/msi.c b/arch/powerpc/kernel/msi.c
index 8bbc12d..71bd161 100644
--- a/arch/powerpc/kernel/msi.c
+++ b/arch/powerpc/kernel/msi.c
@@ -13,7 +13,7 @@
#include <asm/machdep.h>
-int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
+int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
{
if (!ppc_md.setup_msi_irqs || !ppc_md.teardown_msi_irqs) {
pr_debug("msi: Platform doesn't provide MSI callbacks.\n");
@@ -24,16 +24,6 @@ int arch_msi_check_device(struct pci_dev* dev, int nvec, int type)
if (type == PCI_CAP_ID_MSI && nvec > 1)
return 1;
- if (ppc_md.msi_check_device) {
- pr_debug("msi: Using platform check routine.\n");
- return ppc_md.msi_check_device(dev, nvec, type);
- }
-
- return 0;
-}
-
-int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
-{
return ppc_md.setup_msi_irqs(dev, nvec, type);
}
diff --git a/arch/powerpc/platforms/cell/axon_msi.c b/arch/powerpc/platforms/cell/axon_msi.c
index 85825b5..862b327 100644
--- a/arch/powerpc/platforms/cell/axon_msi.c
+++ b/arch/powerpc/platforms/cell/axon_msi.c
@@ -199,14 +199,6 @@ out_error:
return msic;
}
-static int axon_msi_check_device(struct pci_dev *dev, int nvec, int type)
-{
- if (!find_msi_translator(dev))
- return -ENODEV;
-
- return 0;
-}
-
static int setup_msi_msg_address(struct pci_dev *dev, struct msi_msg *msg)
{
struct device_node *dn;
@@ -416,7 +408,6 @@ static int axon_msi_probe(struct platform_device *device)
ppc_md.setup_msi_irqs = axon_msi_setup_msi_irqs;
ppc_md.teardown_msi_irqs = axon_msi_teardown_msi_irqs;
- ppc_md.msi_check_device = axon_msi_check_device;
axon_msi_debug_setup(dn, msic);
diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
index b854b57..b45c492 100644
--- a/arch/powerpc/platforms/powernv/pci.c
+++ b/arch/powerpc/platforms/powernv/pci.c
@@ -46,29 +46,21 @@
//#define cfg_dbg(fmt...) printk(fmt)
#ifdef CONFIG_PCI_MSI
-static int pnv_msi_check_device(struct pci_dev* pdev, int nvec, int type)
-{
- struct pci_controller *hose = pci_bus_to_host(pdev->bus);
- struct pnv_phb *phb = hose->private_data;
- struct pci_dn *pdn = pci_get_pdn(pdev);
-
- if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
- return -ENODEV;
-
- return (phb && phb->msi_bmp.bitmap) ? 0 : -ENODEV;
-}
-
static int pnv_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
{
struct pci_controller *hose = pci_bus_to_host(pdev->bus);
struct pnv_phb *phb = hose->private_data;
+ struct pci_dn *pdn = pci_get_pdn(pdev);
struct msi_desc *entry;
struct msi_msg msg;
int hwirq;
unsigned int virq;
int rc;
- if (WARN_ON(!phb))
+ if (WARN_ON(!phb) || !phb->msi_bmp.bitmap)
+ return -ENODEV;
+
+ if (pdn && pdn->force_32bit_msi && !phb->msi32_support)
return -ENODEV;
list_for_each_entry(entry, &pdev->msi_list, list) {
@@ -860,7 +852,6 @@ void __init pnv_pci_init(void)
/* Configure MSIs */
#ifdef CONFIG_PCI_MSI
- ppc_md.msi_check_device = pnv_msi_check_device;
ppc_md.setup_msi_irqs = pnv_setup_msi_irqs;
ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
#endif
diff --git a/arch/powerpc/platforms/pseries/msi.c b/arch/powerpc/platforms/pseries/msi.c
index 18ff462..e68c196 100644
--- a/arch/powerpc/platforms/pseries/msi.c
+++ b/arch/powerpc/platforms/pseries/msi.c
@@ -336,26 +336,6 @@ out:
return request;
}
-static int rtas_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
- int quota, rc;
-
- if (type == PCI_CAP_ID_MSIX)
- rc = check_req_msix(pdev, nvec);
- else
- rc = check_req_msi(pdev, nvec);
-
- if (rc)
- return rc;
-
- quota = msi_quota_for_device(pdev, nvec);
-
- if (quota && quota < nvec)
- return quota;
-
- return 0;
-}
-
static int check_msix_entries(struct pci_dev *pdev)
{
struct msi_desc *entry;
@@ -397,15 +377,24 @@ static void rtas_hack_32bit_msi_gen2(struct pci_dev *pdev)
static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
{
struct pci_dn *pdn;
- int hwirq, virq, i, rc;
+ int hwirq, virq, i, quota, rc;
struct msi_desc *entry;
struct msi_msg msg;
int nvec = nvec_in;
int use_32bit_msi_hack = 0;
- pdn = pci_get_pdn(pdev);
- if (!pdn)
- return -ENODEV;
+ if (type == PCI_CAP_ID_MSIX)
+ rc = check_req_msix(pdev, nvec);
+ else
+ rc = check_req_msi(pdev, nvec);
+
+ if (rc)
+ return rc;
+
+ quota = msi_quota_for_device(pdev, nvec);
+
+ if (quota && quota < nvec)
+ return quota;
if (type == PCI_CAP_ID_MSIX && check_msix_entries(pdev))
return -EINVAL;
@@ -416,12 +405,14 @@ static int rtas_setup_msi_irqs(struct pci_dev *pdev, int nvec_in, int type)
*/
if (type == PCI_CAP_ID_MSIX) {
int m = roundup_pow_of_two(nvec);
- int quota = msi_quota_for_device(pdev, m);
+ quota = msi_quota_for_device(pdev, m);
if (quota >= m)
nvec = m;
}
+ pdn = pci_get_pdn(pdev);
+
/*
* Try the new more explicit firmware interface, if that fails fall
* back to the old interface. The old interface is known to never
@@ -526,7 +517,6 @@ static int rtas_msi_init(void)
WARN_ON(ppc_md.setup_msi_irqs);
ppc_md.setup_msi_irqs = rtas_setup_msi_irqs;
ppc_md.teardown_msi_irqs = rtas_teardown_msi_irqs;
- ppc_md.msi_check_device = rtas_msi_check_device;
WARN_ON(ppc_md.pci_irq_fixup);
ppc_md.pci_irq_fixup = rtas_msi_pci_irq_fixup;
diff --git a/arch/powerpc/sysdev/fsl_msi.c b/arch/powerpc/sysdev/fsl_msi.c
index 77efbae..b32e79d 100644
--- a/arch/powerpc/sysdev/fsl_msi.c
+++ b/arch/powerpc/sysdev/fsl_msi.c
@@ -109,14 +109,6 @@ static int fsl_msi_init_allocator(struct fsl_msi *msi_data)
return 0;
}
-static int fsl_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
- if (type == PCI_CAP_ID_MSIX)
- pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
-
- return 0;
-}
-
static void fsl_teardown_msi_irqs(struct pci_dev *pdev)
{
struct msi_desc *entry;
@@ -173,6 +165,9 @@ static int fsl_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
struct msi_msg msg;
struct fsl_msi *msi_data;
+ if (type == PCI_CAP_ID_MSIX)
+ pr_debug("fslmsi: MSI-X untested, trying anyway.\n");
+
/*
* If the PCI node has an fsl,msi property, then we need to use it
* to find the specific MSI.
@@ -527,7 +522,6 @@ static int fsl_of_msi_probe(struct platform_device *dev)
if (!ppc_md.setup_msi_irqs) {
ppc_md.setup_msi_irqs = fsl_setup_msi_irqs;
ppc_md.teardown_msi_irqs = fsl_teardown_msi_irqs;
- ppc_md.msi_check_device = fsl_msi_check_device;
} else if (ppc_md.setup_msi_irqs != fsl_setup_msi_irqs) {
dev_err(&dev->dev, "Different MSI driver already installed!\n");
err = -ENODEV;
diff --git a/arch/powerpc/sysdev/mpic_pasemi_msi.c b/arch/powerpc/sysdev/mpic_pasemi_msi.c
index 38e6238..15dccd3 100644
--- a/arch/powerpc/sysdev/mpic_pasemi_msi.c
+++ b/arch/powerpc/sysdev/mpic_pasemi_msi.c
@@ -63,14 +63,6 @@ static struct irq_chip mpic_pasemi_msi_chip = {
.name = "PASEMI-MSI",
};
-static int pasemi_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
- if (type == PCI_CAP_ID_MSIX)
- pr_debug("pasemi_msi: MSI-X untested, trying anyway\n");
-
- return 0;
-}
-
static void pasemi_msi_teardown_msi_irqs(struct pci_dev *pdev)
{
struct msi_desc *entry;
@@ -97,6 +89,8 @@ static int pasemi_msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
struct msi_msg msg;
int hwirq;
+ if (type == PCI_CAP_ID_MSIX)
+ pr_debug("pasemi_msi: MSI-X untested, trying anyway\n");
pr_debug("pasemi_msi_setup_msi_irqs, pdev %p nvec %d type %d\n",
pdev, nvec, type);
@@ -169,7 +163,6 @@ int mpic_pasemi_msi_init(struct mpic *mpic)
WARN_ON(ppc_md.setup_msi_irqs);
ppc_md.setup_msi_irqs = pasemi_msi_setup_msi_irqs;
ppc_md.teardown_msi_irqs = pasemi_msi_teardown_msi_irqs;
- ppc_md.msi_check_device = pasemi_msi_check_device;
return 0;
}
diff --git a/arch/powerpc/sysdev/mpic_u3msi.c b/arch/powerpc/sysdev/mpic_u3msi.c
index 9a7aa0e..623d7fb 100644
--- a/arch/powerpc/sysdev/mpic_u3msi.c
+++ b/arch/powerpc/sysdev/mpic_u3msi.c
@@ -105,22 +105,6 @@ static u64 find_u4_magic_addr(struct pci_dev *pdev, unsigned int hwirq)
return 0;
}
-static int u3msi_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
- if (type == PCI_CAP_ID_MSIX)
- pr_debug("u3msi: MSI-X untested, trying anyway.\n");
-
- /* If we can't find a magic address then MSI ain't gonna work */
- if (find_ht_magic_addr(pdev, 0) == 0 &&
- find_u4_magic_addr(pdev, 0) == 0) {
- pr_debug("u3msi: no magic address found for %s\n",
- pci_name(pdev));
- return -ENXIO;
- }
-
- return 0;
-}
-
static void u3msi_teardown_msi_irqs(struct pci_dev *pdev)
{
struct msi_desc *entry;
@@ -146,6 +130,17 @@ static int u3msi_setup_msi_irqs(struct pci_dev *pdev, int nvec, int type)
u64 addr;
int hwirq;
+ if (type == PCI_CAP_ID_MSIX)
+ pr_debug("u3msi: MSI-X untested, trying anyway.\n");
+
+ /* If we can't find a magic address then MSI ain't gonna work */
+ if (find_ht_magic_addr(pdev, 0) == 0 &&
+ find_u4_magic_addr(pdev, 0) == 0) {
+ pr_debug("u3msi: no magic address found for %s\n",
+ pci_name(pdev));
+ return -ENXIO;
+ }
+
list_for_each_entry(entry, &pdev->msi_list, list) {
hwirq = msi_bitmap_alloc_hwirqs(&msi_mpic->msi_bitmap, 1);
if (hwirq < 0) {
@@ -202,7 +197,6 @@ int mpic_u3msi_init(struct mpic *mpic)
WARN_ON(ppc_md.setup_msi_irqs);
ppc_md.setup_msi_irqs = u3msi_setup_msi_irqs;
ppc_md.teardown_msi_irqs = u3msi_teardown_msi_irqs;
- ppc_md.msi_check_device = u3msi_msi_check_device;
return 0;
}
diff --git a/arch/powerpc/sysdev/ppc4xx_hsta_msi.c b/arch/powerpc/sysdev/ppc4xx_hsta_msi.c
index 11c8884..a6a4dbd 100644
--- a/arch/powerpc/sysdev/ppc4xx_hsta_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_hsta_msi.c
@@ -44,6 +44,12 @@ static int hsta_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
int irq, hwirq;
u64 addr;
+ /* We don't support MSI-X */
+ if (type == PCI_CAP_ID_MSIX) {
+ pr_debug("%s: MSI-X not supported.\n", __func__);
+ return -EINVAL;
+ }
+
list_for_each_entry(entry, &dev->msi_list, list) {
irq = msi_bitmap_alloc_hwirqs(&ppc4xx_hsta_msi.bmp, 1);
if (irq < 0) {
@@ -117,17 +123,6 @@ static void hsta_teardown_msi_irqs(struct pci_dev *dev)
}
}
-static int hsta_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
- /* We don't support MSI-X */
- if (type == PCI_CAP_ID_MSIX) {
- pr_debug("%s: MSI-X not supported.\n", __func__);
- return -EINVAL;
- }
-
- return 0;
-}
-
static int hsta_msi_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -178,7 +173,6 @@ static int hsta_msi_probe(struct platform_device *pdev)
ppc_md.setup_msi_irqs = hsta_setup_msi_irqs;
ppc_md.teardown_msi_irqs = hsta_teardown_msi_irqs;
- ppc_md.msi_check_device = hsta_msi_check_device;
return 0;
out2:
diff --git a/arch/powerpc/sysdev/ppc4xx_msi.c b/arch/powerpc/sysdev/ppc4xx_msi.c
index 43948da..22b5200 100644
--- a/arch/powerpc/sysdev/ppc4xx_msi.c
+++ b/arch/powerpc/sysdev/ppc4xx_msi.c
@@ -85,8 +85,12 @@ static int ppc4xx_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
struct msi_desc *entry;
struct ppc4xx_msi *msi_data = &ppc4xx_msi;
- msi_data->msi_virqs = kmalloc((msi_irqs) * sizeof(int),
- GFP_KERNEL);
+ dev_dbg(&dev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
+ __func__, nvec, type);
+ if (type == PCI_CAP_ID_MSIX)
+ pr_debug("ppc4xx msi: MSI-X untested, trying anyway.\n");
+
+ msi_data->msi_virqs = kmalloc((msi_irqs) * sizeof(int), GFP_KERNEL);
if (!msi_data->msi_virqs)
return -ENOMEM;
@@ -134,16 +138,6 @@ void ppc4xx_teardown_msi_irqs(struct pci_dev *dev)
}
}
-static int ppc4xx_msi_check_device(struct pci_dev *pdev, int nvec, int type)
-{
- dev_dbg(&pdev->dev, "PCIE-MSI:%s called. vec %x type %d\n",
- __func__, nvec, type);
- if (type == PCI_CAP_ID_MSIX)
- pr_debug("ppc4xx msi: MSI-X untested, trying anyway.\n");
-
- return 0;
-}
-
static int ppc4xx_setup_pcieh_hw(struct platform_device *dev,
struct resource res, struct ppc4xx_msi *msi)
{
@@ -259,7 +253,6 @@ static int ppc4xx_msi_probe(struct platform_device *dev)
ppc_md.setup_msi_irqs = ppc4xx_setup_msi_irqs;
ppc_md.teardown_msi_irqs = ppc4xx_teardown_msi_irqs;
- ppc_md.msi_check_device = ppc4xx_msi_check_device;
return err;
error_out:
--
1.9.3
^ permalink raw reply related
* [PATCH v2 0/3] PCI/MSI: Remove arch_msi_check_device()
From: Alexander Gordeev @ 2014-09-07 18:57 UTC (permalink / raw)
To: linux-kernel
Cc: Jason Cooper, Alexander Gordeev, linux-pci, Thomas Gleixner,
linuxppc-dev
Hello,
This is a cleanup effort to get rid of arch_msi_check_device() function.
I am sending v2 series, since kbuild for v1 reports compile errors on
ppc4xx and Armada 370. Still, I have not checked the fixes on these
platforms.
Changes since v1:
- patch 1: 'pdev' undeclared compile error fixed on ppc4xx. I updated
changelog and removed ACK from this patch in case there are
any objections;
- patch 2: 'struct msi_chip' has no 'check_device' error fixed on
Armada 370 - armada_370_xp_check_msi_device() hook removed;
- patch 3: msi_check_device() renamed to pci_msi_supported(). This is
the very same patch I sent earlier;
Thanks!
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Alexander Gordeev (3):
patch 1 - PCI/MSI/PPC: Remove arch_msi_check_device()
patch 2 - PCI/MSI/Armada-370-xp: Remove arch_msi_check_device()
patch 3 - PCI/MSI: Remove arch_msi_check_device()
arch/powerpc/include/asm/machdep.h | 2 --
arch/powerpc/kernel/msi.c | 12 +--------
arch/powerpc/platforms/cell/axon_msi.c | 9 -------
arch/powerpc/platforms/powernv/pci.c | 19 ++++---------
arch/powerpc/platforms/pseries/msi.c | 42 +++++++++++------------------
arch/powerpc/sysdev/fsl_msi.c | 12 +++------
arch/powerpc/sysdev/mpic_pasemi_msi.c | 11 ++------
arch/powerpc/sysdev/mpic_u3msi.c | 28 ++++++++-----------
arch/powerpc/sysdev/ppc4xx_hsta_msi.c | 18 +++++--------
arch/powerpc/sysdev/ppc4xx_msi.c | 19 +++++--------
drivers/irqchip/irq-armada-370-xp.c | 14 +++-------
drivers/pci/msi.c | 49 +++++++++-------------------------
include/linux/msi.h | 3 ---
13 files changed, 67 insertions(+), 171 deletions(-)
--
1.9.3
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-08 5:50 UTC (permalink / raw)
To: Peter Hurley
Cc: Jakub Jelinek, One Thousand Gnomes, linux-arch, linux-ia64,
Mikael Pettersson, Oleg Nesterov, linux-kernel, Tony Luck,
Paul Mackerras, H. Peter Anvin, paulmck, linuxppc-dev,
Miroslav Franc, Richard Henderson
In-Reply-To: <540CC305.8010407@hurleysoftware.com>
On Sun, 2014-09-07 at 16:41 -0400, Peter Hurley wrote:
> On 09/07/2014 03:04 PM, James Bottomley wrote:
> > On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
> >> On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
> >>> On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
> >>>> On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley wrote:
> >>>>> Hi James,
> >>>>>
> >>>>> On 09/04/2014 10:11 PM, James Bottomley wrote:
> >>>>>> On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney wrote:
> >>>>>>> +And there are anti-guarantees:
> >>>>>>> +
> >>>>>>> + (*) These guarantees do not apply to bitfields, because compilers often
> >>>>>>> + generate code to modify these using non-atomic read-modify-write
> >>>>>>> + sequences. Do not attempt to use bitfields to synchronize parallel
> >>>>>>> + algorithms.
> >>>>>>> +
> >>>>>>> + (*) Even in cases where bitfields are protected by locks, all fields
> >>>>>>> + in a given bitfield must be protected by one lock. If two fields
> >>>>>>> + in a given bitfield are protected by different locks, the compiler's
> >>>>>>> + non-atomic read-modify-write sequences can cause an update to one
> >>>>>>> + field to corrupt the value of an adjacent field.
> >>>>>>> +
> >>>>>>> + (*) These guarantees apply only to properly aligned and sized scalar
> >>>>>>> + variables. "Properly sized" currently means "int" and "long",
> >>>>>>> + because some CPU families do not support loads and stores of
> >>>>>>> + other sizes. ("Some CPU families" is currently believed to
> >>>>>>> + be only Alpha 21064. If this is actually the case, a different
> >>>>>>> + non-guarantee is likely to be formulated.)
> >>>>>>
> >>>>>> This is a bit unclear. Presumably you're talking about definiteness of
> >>>>>> the outcome (as in what's seen after multiple stores to the same
> >>>>>> variable).
> >>>>>
> >>>>> No, the last conditions refers to adjacent byte stores from different
> >>>>> cpu contexts (either interrupt or SMP).
> >>>>>
> >>>>>> The guarantees are only for natural width on Parisc as well,
> >>>>>> so you would get a mess if you did byte stores to adjacent memory
> >>>>>> locations.
> >>>>>
> >>>>> For a simple test like:
> >>>>>
> >>>>> struct x {
> >>>>> long a;
> >>>>> char b;
> >>>>> char c;
> >>>>> char d;
> >>>>> char e;
> >>>>> };
> >>>>>
> >>>>> void store_bc(struct x *p) {
> >>>>> p->b = 1;
> >>>>> p->c = 2;
> >>>>> }
> >>>>>
> >>>>> on parisc, gcc generates separate byte stores
> >>>>>
> >>>>> void store_bc(struct x *p) {
> >>>>> 0: 34 1c 00 02 ldi 1,ret0
> >>>>> 4: 0f 5c 12 08 stb ret0,4(r26)
> >>>>> 8: 34 1c 00 04 ldi 2,ret0
> >>>>> c: e8 40 c0 00 bv r0(rp)
> >>>>> 10: 0f 5c 12 0a stb ret0,5(r26)
> >>>>>
> >>>>> which appears to confirm that on parisc adjacent byte data
> >>>>> is safe from corruption by concurrent cpu updates; that is,
> >>>>>
> >>>>> CPU 0 | CPU 1
> >>>>> |
> >>>>> p->b = 1 | p->c = 2
> >>>>> |
> >>>>>
> >>>>> will result in p->b == 1 && p->c == 2 (assume both values
> >>>>> were 0 before the call to store_bc()).
> >>>>
> >>>> What Peter said. I would ask for suggestions for better wording, but
> >>>> I would much rather be able to say that single-byte reads and writes
> >>>> are atomic and that aligned-short reads and writes are also atomic.
> >>>>
> >>>> Thus far, it looks like we lose only very old Alpha systems, so unless
> >>>> I hear otherwise, I update my patch to outlaw these very old systems.
> >>>
> >>> This isn't universally true according to the architecture manual. The
> >>> PARISC CPU can make byte to long word stores atomic against the memory
> >>> bus but not against the I/O bus for instance. Atomicity is a property
> >>> of the underlying substrate, not of the CPU. Implying that atomicity is
> >>> a CPU property is incorrect.
>
> To go back to this briefly, while it's true that atomicity is not
> a CPU property, atomicity is not possible if the CPU is not cooperating.
You mean if it doesn't have the bus logic to emit? Like ia32 mostly
can't do 64 bit writes ... sure.
> >> OK, fair point.
> >>
> >> But are there in-use-for-Linux PARISC memory fabrics (for normal memory,
> >> not I/O) that do not support single-byte and double-byte stores?
> >
> > For aligned access, I believe that's always the case for the memory bus
> > (on both 32 and 64 bit systems). However, it only applies to machine
> > instruction loads and stores of the same width.. If you mix the widths
> > on the loads and stores, all bets are off. That means you have to
> > beware of the gcc penchant for coalescing loads and stores: if it sees
> > two adjacent byte stores it can coalesce them into a short store
> > instead ... that screws up the atomicity guarantees.
>
> Two things: I think that gcc has given up on combining adjacent writes,
> perhaps because unaligned writes on some arches are prohibitive, so
> whatever minor optimization was believed to be gained was quickly lost,
> multi-fold. (Although this might not be the reason since one would
> expect that gcc would know the alignment of the promoted store).
Um, gcc assumes architecturally correct alignment; that's why it pads
structures. Only when accessing packed structures will it use the
lowest unit load/store.
if you have a struct { char a, char b }; and load first a then b with a
constant gcc will obligingly optimise to a short store.
> But additionally, even if gcc combines adjacent writes _that are part
> of the program flow_ then I believe the situation is no worse than
> would otherwise exist.
>
> For instance, given the following:
>
> struct x {
> spinlock_t lock;
> long a;
> byte b;
> byte c;
> };
>
> void locked_store_b(struct x *p)
> {
> spin_lock(&p->lock);
> p->b = 1;
> spin_unlock(&p->lock);
> p->c = 2;
> }
>
> Granted, the author probably expects ordered writes of
> STORE B
> STORE C
> but that's not guaranteed because there is no memory barrier
> ordering B before C.
Yes, there is: loads and stores may not migrate into or out of critical
sections.
> I see no problem with gcc write-combining in the absence of
> memory barriers (as long as alignment is still respected,
> ie., the size-promoted write is still naturally aligned). The
> combined write is still atomic.
Actual alignment is pretty irrelevant. That's why all architectures
which require alignment also have to implement misaligned traps ... this
is a fundamental requirement of the networking code, for instance.
The main problem is gcc thinking there's a misalignment (or a packed
structure). That causes splitting of loads and stores and that destroys
atomicity.
James
^ permalink raw reply
* Re: bit fields && data tearing
From: James Bottomley @ 2014-09-08 5:56 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Jakub Jelinek, One Thousand Gnomes, Tony Luck, linux-ia64,
Peter Hurley, Mikael Pettersson, Oleg Nesterov, linux-kernel,
Paul Mackerras, linux-arch, paulmck, linuxppc-dev, Miroslav Franc,
Richard Henderson
In-Reply-To: <154b540a-df47-4f3e-bdda-ab5d2e72723a@email.android.com>
On Sun, 2014-09-07 at 16:39 -0700, H. Peter Anvin wrote:
> How many PARISC systems do we have that actually do real work on Linux?
I'd be very surprised if this problem didn't exist on all alignment
requiring architectures, like PPC and Sparc as well. I know it would be
very convenient if all the world were an x86 ... but it would also be
very boring as well.
The rules for coping with it are well known and a relaxation of what we
currently do in the kernel, so I don't see what the actual problem is.
In the context of this thread, PA can't do atomic bit sets (no atomic
RMW except the ldcw operation) it can do atomic writes to fundamental
sizes (byte, short, int, long) provided gcc emits the correct primitive
(there are lots of gotchas in this, but that's not an architectural
problem). These atomicity guarantees depend on the underlying storage
and are respected for main memory but not for any other type of bus.
James
> On September 7, 2014 4:36:55 PM PDT, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> >On Sun, Sep 07, 2014 at 04:17:30PM -0700, H. Peter Anvin wrote:
> >> I'm confused why storing 0x0102 would be a problem. I think gcc does
> >that even on other cpus.
> >>
> >> More atomicity can't hurt, can it?
> >
> >I must defer to James for any additional details on why PARISC systems
> >don't provide atomicity for partially overlapping stores. ;-)
> >
> > Thanx, Paul
> >
> >> On September 7, 2014 4:00:19 PM PDT, "Paul E. McKenney"
> ><paulmck@linux.vnet.ibm.com> wrote:
> >> >On Sun, Sep 07, 2014 at 12:04:47PM -0700, James Bottomley wrote:
> >> >> On Sun, 2014-09-07 at 09:21 -0700, Paul E. McKenney wrote:
> >> >> > On Sat, Sep 06, 2014 at 10:07:22PM -0700, James Bottomley wrote:
> >> >> > > On Thu, 2014-09-04 at 21:06 -0700, Paul E. McKenney wrote:
> >> >> > > > On Thu, Sep 04, 2014 at 10:47:24PM -0400, Peter Hurley
> >wrote:
> >> >> > > > > Hi James,
> >> >> > > > >
> >> >> > > > > On 09/04/2014 10:11 PM, James Bottomley wrote:
> >> >> > > > > > On Thu, 2014-09-04 at 17:17 -0700, Paul E. McKenney
> >wrote:
> >> >> > > > > >> +And there are anti-guarantees:
> >> >> > > > > >> +
> >> >> > > > > >> + (*) These guarantees do not apply to bitfields,
> >because
> >> >compilers often
> >> >> > > > > >> + generate code to modify these using non-atomic
> >> >read-modify-write
> >> >> > > > > >> + sequences. Do not attempt to use bitfields to
> >> >synchronize parallel
> >> >> > > > > >> + algorithms.
> >> >> > > > > >> +
> >> >> > > > > >> + (*) Even in cases where bitfields are protected by
> >> >locks, all fields
> >> >> > > > > >> + in a given bitfield must be protected by one
> >lock.
> >> >If two fields
> >> >> > > > > >> + in a given bitfield are protected by different
> >> >locks, the compiler's
> >> >> > > > > >> + non-atomic read-modify-write sequences can cause
> >an
> >> >update to one
> >> >> > > > > >> + field to corrupt the value of an adjacent field.
> >> >> > > > > >> +
> >> >> > > > > >> + (*) These guarantees apply only to properly aligned
> >and
> >> >sized scalar
> >> >> > > > > >> + variables. "Properly sized" currently means
> >"int"
> >> >and "long",
> >> >> > > > > >> + because some CPU families do not support loads
> >and
> >> >stores of
> >> >> > > > > >> + other sizes. ("Some CPU families" is currently
> >> >believed to
> >> >> > > > > >> + be only Alpha 21064. If this is actually the
> >case,
> >> >a different
> >> >> > > > > >> + non-guarantee is likely to be formulated.)
> >> >> > > > > >
> >> >> > > > > > This is a bit unclear. Presumably you're talking about
> >> >definiteness of
> >> >> > > > > > the outcome (as in what's seen after multiple stores to
> >the
> >> >same
> >> >> > > > > > variable).
> >> >> > > > >
> >> >> > > > > No, the last conditions refers to adjacent byte stores
> >from
> >> >different
> >> >> > > > > cpu contexts (either interrupt or SMP).
> >> >> > > > >
> >> >> > > > > > The guarantees are only for natural width on Parisc as
> >> >well,
> >> >> > > > > > so you would get a mess if you did byte stores to
> >adjacent
> >> >memory
> >> >> > > > > > locations.
> >> >> > > > >
> >> >> > > > > For a simple test like:
> >> >> > > > >
> >> >> > > > > struct x {
> >> >> > > > > long a;
> >> >> > > > > char b;
> >> >> > > > > char c;
> >> >> > > > > char d;
> >> >> > > > > char e;
> >> >> > > > > };
> >> >> > > > >
> >> >> > > > > void store_bc(struct x *p) {
> >> >> > > > > p->b = 1;
> >> >> > > > > p->c = 2;
> >> >> > > > > }
> >> >> > > > >
> >> >> > > > > on parisc, gcc generates separate byte stores
> >> >> > > > >
> >> >> > > > > void store_bc(struct x *p) {
> >> >> > > > > 0: 34 1c 00 02 ldi 1,ret0
> >> >> > > > > 4: 0f 5c 12 08 stb ret0,4(r26)
> >> >> > > > > 8: 34 1c 00 04 ldi 2,ret0
> >> >> > > > > c: e8 40 c0 00 bv r0(rp)
> >> >> > > > > 10: 0f 5c 12 0a stb ret0,5(r26)
> >> >> > > > >
> >> >> > > > > which appears to confirm that on parisc adjacent byte data
> >> >> > > > > is safe from corruption by concurrent cpu updates; that
> >is,
> >> >> > > > >
> >> >> > > > > CPU 0 | CPU 1
> >> >> > > > > |
> >> >> > > > > p->b = 1 | p->c = 2
> >> >> > > > > |
> >> >> > > > >
> >> >> > > > > will result in p->b == 1 && p->c == 2 (assume both values
> >> >> > > > > were 0 before the call to store_bc()).
> >> >> > > >
> >> >> > > > What Peter said. I would ask for suggestions for better
> >> >wording, but
> >> >> > > > I would much rather be able to say that single-byte reads
> >and
> >> >writes
> >> >> > > > are atomic and that aligned-short reads and writes are also
> >> >atomic.
> >> >> > > >
> >> >> > > > Thus far, it looks like we lose only very old Alpha systems,
> >so
> >> >unless
> >> >> > > > I hear otherwise, I update my patch to outlaw these very old
> >> >systems.
> >> >> > >
> >> >> > > This isn't universally true according to the architecture
> >manual.
> >> > The
> >> >> > > PARISC CPU can make byte to long word stores atomic against
> >the
> >> >memory
> >> >> > > bus but not against the I/O bus for instance. Atomicity is a
> >> >property
> >> >> > > of the underlying substrate, not of the CPU. Implying that
> >> >atomicity is
> >> >> > > a CPU property is incorrect.
> >> >> >
> >> >> > OK, fair point.
> >> >> >
> >> >> > But are there in-use-for-Linux PARISC memory fabrics (for normal
> >> >memory,
> >> >> > not I/O) that do not support single-byte and double-byte stores?
> >> >>
> >> >> For aligned access, I believe that's always the case for the
> >memory
> >> >bus
> >> >> (on both 32 and 64 bit systems). However, it only applies to
> >machine
> >> >> instruction loads and stores of the same width.. If you mix the
> >> >widths
> >> >> on the loads and stores, all bets are off. That means you have to
> >> >> beware of the gcc penchant for coalescing loads and stores: if it
> >> >sees
> >> >> two adjacent byte stores it can coalesce them into a short store
> >> >> instead ... that screws up the atomicity guarantees.
> >> >
> >> >OK, that means that to make PARISC work reliably, we need to use
> >> >ACCESS_ONCE() for loads and stores that could have racing accesses.
> >> >If I understand correctly, this will -not- be needed for code
> >guarded
> >> >by locks, even with Peter's examples.
> >> >
> >> >So if we have something like this:
> >> >
> >> > struct foo {
> >> > char a;
> >> > char b;
> >> > };
> >> > struct foo *fp;
> >> >
> >> >then this code would be bad:
> >> >
> >> > fp->a = 1;
> >> > fp->b = 2;
> >> >
> >> >The reason is (as you say) that GCC would be happy to store 0x0102
> >> >(or vice versa, depending on endianness) to the pair. We instead
> >> >need:
> >> >
> >> > ACCESS_ONCE(fp->a) = 1;
> >> > ACCESS_ONCE(fp->b) = 2;
> >> >
> >> >However, if the code is protected by locks, no problem:
> >> >
> >> > struct foo {
> >> > spinlock_t lock_a;
> >> > spinlock_t lock_b;
> >> > char a;
> >> > char b;
> >> > };
> >> >
> >> >Then it is OK to do the following:
> >> >
> >> > spin_lock(fp->lock_a);
> >> > fp->a = 1;
> >> > spin_unlock(fp->lock_a);
> >> > spin_lock(fp->lock_b);
> >> > fp->b = 1;
> >> > spin_unlock(fp->lock_b);
> >> >
> >> >Or even this, assuming ->lock_a precedes ->lock_b in the locking
> >> >hierarchy:
> >> >
> >> > spin_lock(fp->lock_a);
> >> > spin_lock(fp->lock_b);
> >> > fp->a = 1;
> >> > fp->b = 1;
> >> > spin_unlock(fp->lock_a);
> >> > spin_unlock(fp->lock_b);
> >> >
> >> >Here gcc might merge the assignments to fp->a and fp->b, but that is
> >OK
> >> >because both locks are held, presumably preventing other assignments
> >or
> >> >references to fp->a and fp->b.
> >> >
> >> >On the other hand, if either fp->a or fp->b are referenced outside
> >of
> >> >their
> >> >respective locks, even once, then this last code fragment would
> >still
> >> >need
> >> >ACCESS_ONCE() as follows:
> >> >
> >> > spin_lock(fp->lock_a);
> >> > spin_lock(fp->lock_b);
> >> > ACCESS_ONCE(fp->a) = 1;
> >> > ACCESS_ONCE(fp->b) = 1;
> >> > spin_unlock(fp->lock_a);
> >> > spin_unlock(fp->lock_b);
> >> >
> >> >Does that cover it? If so, I will update memory-barriers.txt
> >> >accordingly.
> >> >
> >> > Thanx, Paul
> >>
> >> --
> >> Sent from my mobile phone. Please pardon brevity and lack of
> >formatting.
> >>
>
^ permalink raw reply
* Re: [PATCH v9 2/6] clk: Move all drivers to use internal API
From: Mike Turquette @ 2014-09-08 6:13 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Ulf Hansson, Prashant Gaikwad,
Stephen Warren <swarren@wwwdotorg.org>, Thierry Reding <thierry.reding@gmail.com>, tomasz.figa@gmail.com, Peter De Schrijver <pdeschrijver@nvidia.com>, rabin@rab.in, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Jason Cooper <jason@lakedaemon.net>, Andrew Lunn <andrew@lunn.ch>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Russell King <linux@arm.linux.org.uk>, Shawn Guo <shawn.guo@freescale.com>, Sascha Hauer <kernel@pengutronix.de>, David Brown <davidb@codeaurora.org>, Daniel Walker <dwalker@fifo99.com>, Bryan Huntsman <bryanh@codeaurora.org>, Paul Walmsley <paul@pwsan.com>, Tony Lindgren <tony@atomide.com>, " Benoît Cousson <bcousson@baylibre.com>, Kevin Hilman,
alsa-devel, Jaroslav Kysela, Paul Mackerras, Sylwester Nawrocki,
linux-arch, Boris Brezillon, Kukjin Kim, " Emilio López,
patches, Michal Simek, Takashi Iwai, Santosh Shilimkar,
Anatolij Gustschin, Dinh Nguyen, linux-media, Arnd Bergmann,
linux-arm-msm, spear-devel, Mark Brown, linux-rpi-kernel,
Ben Dooks, linux-tegra, linux-omap, linux-samsung-soc, Barry Song,
Tomeu Vizoso, Liam Girdwood, Kyungmin Park, Viresh Kumar,
Maxime Ripard, linuxppc-dev, Mauro Carvalho Chehab
In-Reply-To: <1409758317-20564-1-git-send-email-tomeu.vizoso@collabora.com>
Quoting Tomeu Vizoso (2014-09-03 08:31:57)
> In preparation to change the public API to return a per-user clk structur=
e,
> remove any usage of this public API from the clock implementations.
> =
> The reason for having this in a separate commit from the one that introdu=
ces
> the implementation of the new functions is to separate the changes genera=
ted
> with Coccinelle from the rest, and keep the patches' size reasonable.
> =
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Tested-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Hi Tomeu,
Looks like the Coccinelle script had a false-positive.
asm-generic/clkdev.h was converted from clk->clk_core and this blowed up
clock drivers for architectures that don't provide an asm-specific
clkdev.h implementation. This fixes x86's LPSS and a Microblaze driver.
I've rolled the following fix into your 2/9 patch. No action is
necessary.
Regards,
Mike
diff --git a/include/asm-generic/clkdev.h b/include/asm-generic/clkdev.h
index 4320225..90a32a6 100644
--- a/include/asm-generic/clkdev.h
+++ b/include/asm-generic/clkdev.h
@@ -15,10 +15,10 @@
#include <linux/slab.h>
-struct clk_core;
+struct clk;
-static inline int __clk_get(struct clk_core *clk) { return 1; }
-static inline void __clk_put(struct clk_core *clk) { }
+static inline int __clk_get(struct clk *clk) { return 1; }
+static inline void __clk_put(struct clk *clk) { }
static inline struct clk_lookup_alloc *__clkdev_alloc(size_t size)
{
^ permalink raw reply related
* Re: [PATCH] drivers: char: hw_random: printk replacement
From: Sudip Mukherjee @ 2014-09-08 8:43 UTC (permalink / raw)
To: Matt Mackall, Herbert Xu, Olof Johansson
Cc: linuxppc-dev, linux-kernel, linux-geode
In-Reply-To: <1409238178-16496-1-git-send-email-sudipm.mukherjee@gmail.com>
On Thu, Aug 28, 2014 at 08:32:58PM +0530, Sudip Mukherjee wrote:
> as pr_* macros are more preffered over printk, so printk replaced with corresponding pr_* macros
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>
> The replacement was done by a bash script to avoid copy paste error. The script is as follows :
>
> OLD="printk(KERN_ERR \?"
> OLD1="printk(KERN_NOTICE \?"
> OLD2="printk(KERN_WARNING \?"
> OLD3="printk(KERN_INFO \?"
> NEW="pr_err("
> NEW1="pr_notice("
> NEW2="pr_warn("
> NEW3="pr_info("
> TFILE="/tmp/out.tmp.$$"
> for f in *.c
> do
> sed -e "s/$OLD/$NEW/g" -e "s/$OLD1/$NEW1/g" -e "s/$OLD2/$NEW2/g" -e "s/$OLD3/$NEW3/g" "$f" > $TFILE && mv $TFILE "$f"
> done
>
> Frankly speaking this script has missed one instance of printk(warning) in intel-rng.c file , and this one was edited manually.
>
> drivers/char/hw_random/amd-rng.c | 4 ++--
> drivers/char/hw_random/geode-rng.c | 4 ++--
> drivers/char/hw_random/intel-rng.c | 12 ++++++------
> drivers/char/hw_random/pasemi-rng.c | 2 +-
> drivers/char/hw_random/pseries-rng.c | 2 +-
> drivers/char/hw_random/via-rng.c | 8 ++++----
> 6 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/char/hw_random/amd-rng.c b/drivers/char/hw_random/amd-rng.c
> index c6af038..48f6a83 100644
> --- a/drivers/char/hw_random/amd-rng.c
> +++ b/drivers/char/hw_random/amd-rng.c
> @@ -142,10 +142,10 @@ found:
> amd_rng.priv = (unsigned long)pmbase;
> amd_pdev = pdev;
>
> - printk(KERN_INFO "AMD768 RNG detected\n");
> + pr_info("AMD768 RNG detected\n");
> err = hwrng_register(&amd_rng);
> if (err) {
> - printk(KERN_ERR PFX "RNG registering failed (%d)\n",
> + pr_err(PFX "RNG registering failed (%d)\n",
> err);
> release_region(pmbase + 0xF0, 8);
> goto out;
> diff --git a/drivers/char/hw_random/geode-rng.c b/drivers/char/hw_random/geode-rng.c
> index 4c4d4e1..0d0579f 100644
> --- a/drivers/char/hw_random/geode-rng.c
> +++ b/drivers/char/hw_random/geode-rng.c
> @@ -109,10 +109,10 @@ found:
> goto out;
> geode_rng.priv = (unsigned long)mem;
>
> - printk(KERN_INFO "AMD Geode RNG detected\n");
> + pr_info("AMD Geode RNG detected\n");
> err = hwrng_register(&geode_rng);
> if (err) {
> - printk(KERN_ERR PFX "RNG registering failed (%d)\n",
> + pr_err(PFX "RNG registering failed (%d)\n",
> err);
> goto err_unmap;
> }
> diff --git a/drivers/char/hw_random/intel-rng.c b/drivers/char/hw_random/intel-rng.c
> index 86fe45c..3122376 100644
> --- a/drivers/char/hw_random/intel-rng.c
> +++ b/drivers/char/hw_random/intel-rng.c
> @@ -199,7 +199,7 @@ static int intel_rng_init(struct hwrng *rng)
> if ((hw_status & INTEL_RNG_ENABLED) == 0)
> hw_status = hwstatus_set(mem, hw_status | INTEL_RNG_ENABLED);
> if ((hw_status & INTEL_RNG_ENABLED) == 0) {
> - printk(KERN_ERR PFX "cannot enable RNG, aborting\n");
> + pr_err(PFX "cannot enable RNG, aborting\n");
> goto out;
> }
> err = 0;
> @@ -216,7 +216,7 @@ static void intel_rng_cleanup(struct hwrng *rng)
> if (hw_status & INTEL_RNG_ENABLED)
> hwstatus_set(mem, hw_status & ~INTEL_RNG_ENABLED);
> else
> - printk(KERN_WARNING PFX "unusual: RNG already disabled\n");
> + pr_warn(PFX "unusual: RNG already disabled\n");
> }
>
>
> @@ -274,7 +274,7 @@ static int __init intel_rng_hw_init(void *_intel_rng_hw)
> if (mfc != INTEL_FWH_MANUFACTURER_CODE ||
> (dvc != INTEL_FWH_DEVICE_CODE_8M &&
> dvc != INTEL_FWH_DEVICE_CODE_4M)) {
> - printk(KERN_NOTICE PFX "FWH not detected\n");
> + pr_notice(PFX "FWH not detected\n");
> return -ENODEV;
> }
>
> @@ -314,7 +314,7 @@ PFX "RNG, try using the 'no_fwh_detect' option.\n";
>
> if (no_fwh_detect)
> return -ENODEV;
> - printk(warning);
> + pr_warn(warning);
> return -EBUSY;
> }
>
> @@ -392,10 +392,10 @@ fwh_done:
> goto out;
> }
>
> - printk(KERN_INFO "Intel 82802 RNG detected\n");
> + pr_info("Intel 82802 RNG detected\n");
> err = hwrng_register(&intel_rng);
> if (err) {
> - printk(KERN_ERR PFX "RNG registering failed (%d)\n",
> + pr_err(PFX "RNG registering failed (%d)\n",
> err);
> iounmap(mem);
> }
> diff --git a/drivers/char/hw_random/pasemi-rng.c b/drivers/char/hw_random/pasemi-rng.c
> index c66279b..c0347d1 100644
> --- a/drivers/char/hw_random/pasemi-rng.c
> +++ b/drivers/char/hw_random/pasemi-rng.c
> @@ -113,7 +113,7 @@ static int rng_probe(struct platform_device *ofdev)
>
> pasemi_rng.priv = (unsigned long)rng_regs;
>
> - printk(KERN_INFO "Registering PA Semi RNG\n");
> + pr_info("Registering PA Semi RNG\n");
>
> err = hwrng_register(&pasemi_rng);
>
> diff --git a/drivers/char/hw_random/pseries-rng.c b/drivers/char/hw_random/pseries-rng.c
> index ab7ffde..6226aa0 100644
> --- a/drivers/char/hw_random/pseries-rng.c
> +++ b/drivers/char/hw_random/pseries-rng.c
> @@ -86,7 +86,7 @@ static struct vio_driver pseries_rng_driver = {
>
> static int __init rng_init(void)
> {
> - printk(KERN_INFO "Registering IBM pSeries RNG driver\n");
> + pr_info("Registering IBM pSeries RNG driver\n");
> return vio_register_driver(&pseries_rng_driver);
> }
>
> diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
> index de5a6dc..a3bebef 100644
> --- a/drivers/char/hw_random/via-rng.c
> +++ b/drivers/char/hw_random/via-rng.c
> @@ -141,7 +141,7 @@ static int via_rng_init(struct hwrng *rng)
> * register */
> if ((c->x86 == 6) && (c->x86_model >= 0x0f)) {
> if (!cpu_has_xstore_enabled) {
> - printk(KERN_ERR PFX "can't enable hardware RNG "
> + pr_err(PFX "can't enable hardware RNG "
> "if XSTORE is not enabled\n");
> return -ENODEV;
> }
> @@ -180,7 +180,7 @@ static int via_rng_init(struct hwrng *rng)
> unneeded */
> rdmsr(MSR_VIA_RNG, lo, hi);
> if ((lo & VIA_RNG_ENABLE) == 0) {
> - printk(KERN_ERR PFX "cannot enable VIA C3 RNG, aborting\n");
> + pr_err(PFX "cannot enable VIA C3 RNG, aborting\n");
> return -ENODEV;
> }
>
> @@ -202,10 +202,10 @@ static int __init mod_init(void)
>
> if (!cpu_has_xstore)
> return -ENODEV;
> - printk(KERN_INFO "VIA RNG detected\n");
> + pr_info("VIA RNG detected\n");
> err = hwrng_register(&via_rng);
> if (err) {
> - printk(KERN_ERR PFX "RNG registering failed (%d)\n",
> + pr_err(PFX "RNG registering failed (%d)\n",
> err);
> goto out;
> }
> --
> 1.8.1.2
>
gentle ping
thanks
sudip
^ permalink raw reply
* [PATCH v2 0/5] Change vendor prefix for Intersil Corporation
From: Philipp Zabel @ 2014-09-08 9:19 UTC (permalink / raw)
To: devicetree
Cc: Mark Rutland, Alessandro Zummo, Russell King, Pawel Moll,
Ian Campbell, Mark Brown, Wolfram Sang, Rob Herring,
Paul Mackerras, Philipp Zabel, Kumar Gala, linuxppc-dev,
linux-arm-kernel
Hi,
currently there is a wild mixture of isl, isil, and intersil
compatibles in the kernel. I figure at this point it is still
possible to change this to use "isil" everywhere without too
much pain, but it might be preferred to keep the already
documented "isl" prefix, even though it doesn't follow the
convention to use the NASDAQ symbol where available.
This is an updated version of the RFC I've sent previously:
https://www.mail-archive.com/devicetree@vger.kernel.org/msg38505.html
Changes since RFC:
- Drop powerpc/85xx patch, that device tree set in stone.
- Added commit summaries where referencing commit ids
- Added deprecated names to the documentation
regards
Philipp
Philipp Zabel (5):
of: Change vendor prefix for Intersil Corporation to isil
Documentation: Add isl1208 and isl12022 to trivial-devices list
ARM: mvebu: Change vendor prefix for Intersil Corporation to isil
rtc: rtc-isl12022: Change vendor prefix for Intersil Corporation to
isil
rtc: rtc-isl12057: Change vendor prefix for Intersil Corporation to
isil
Documentation/devicetree/bindings/i2c/trivial-devices.txt | 6 +++++-
Documentation/devicetree/bindings/vendor-prefixes.txt | 4 +++-
arch/arm/boot/dts/armada-370-netgear-rn102.dts | 2 +-
arch/arm/boot/dts/armada-370-netgear-rn104.dts | 2 +-
arch/arm/boot/dts/armada-xp-netgear-rn2120.dts | 2 +-
drivers/rtc/rtc-isl12022.c | 3 ++-
drivers/rtc/rtc-isl12057.c | 3 ++-
7 files changed, 15 insertions(+), 7 deletions(-)
--
2.1.0.rc1
^ permalink raw reply
* [PATCH v2 5/5] rtc: rtc-isl12057: Change vendor prefix for Intersil Corporation to isil
From: Philipp Zabel @ 2014-09-08 9:19 UTC (permalink / raw)
To: devicetree
Cc: Mark Rutland, Alessandro Zummo, Russell King, Pawel Moll,
Ian Campbell, Mark Brown, Wolfram Sang, Rob Herring,
Paul Mackerras, Philipp Zabel, Kumar Gala, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1410167960-554-1-git-send-email-p.zabel@pengutronix.de>
Currently there is a wild mixture of isl, isil, and intersil compatibles
in the kernel. At this point, changing the vendor symbol to the most often
used variant, which is equal to the NASDAQ symbol, isil, should not hurt.
Patch 70e123373c05 (rtc: Add support for Intersil ISL12057 I2C RTC chip)
added this driver with device tree support using the then documented isl
vendor prefix, so we keep that around for backwards compatibility.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/rtc/rtc-isl12057.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-isl12057.c b/drivers/rtc/rtc-isl12057.c
index 455b601..8276bd6 100644
--- a/drivers/rtc/rtc-isl12057.c
+++ b/drivers/rtc/rtc-isl12057.c
@@ -279,7 +279,8 @@ static int isl12057_probe(struct i2c_client *client,
#ifdef CONFIG_OF
static const struct of_device_id isl12057_dt_match[] = {
- { .compatible = "isl,isl12057" },
+ { .compatible = "isil,isl12057" },
+ { .compatible = "isl,isl12057" }, /* for backwards compatibility */
{ },
};
#endif
--
2.1.0.rc1
^ permalink raw reply related
* [PATCH v2 2/5] Documentation: Add isl1208 and isl12022 to trivial-devices list
From: Philipp Zabel @ 2014-09-08 9:19 UTC (permalink / raw)
To: devicetree
Cc: Mark Rutland, Alessandro Zummo, Russell King, Pawel Moll,
Ian Campbell, Mark Brown, Wolfram Sang, Rob Herring,
Paul Mackerras, Philipp Zabel, Kumar Gala, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1410167960-554-1-git-send-email-p.zabel@pengutronix.de>
This patch adds the Intersil ISL1208 and ISL12022 I2C RTCs to the
trivial-devices list. For ISL1208 a deprecated intersil,isl1208
entry is added since that is used in ppa8548.dts.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
- Added deprecated entries that are still in use
---
Documentation/devicetree/bindings/i2c/trivial-devices.txt | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 72e399c..60ecea5 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -56,6 +56,9 @@ fsl,sgtl5000 SGTL5000: Ultra Low-Power Audio Codec
gmt,g751 G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire Interface
infineon,slb9635tt Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
infineon,slb9645tt Infineon SLB9645 I2C TPM (new protocol, max 400khz)
+intersil,isl1208 Intersil ISL1208 I2C RTC Chip (deprecated, use isil,isl1208)
+isil,isl1208 Intersil ISL1208 I2C RTC Chip
+isil,isl12022 Intersil ISL12022 I2C RTC Chip
isil,isl12057 Intersil ISL12057 I2C RTC Chip
isl,isl12057 Intersil ISL12057 I2C RTC Chip (deprecated, use isil,isl12057)
maxim,ds1050 5 Bit Programmable, Pulse-Width Modulator
--
2.1.0.rc1
^ permalink raw reply related
* [PATCH v2 4/5] rtc: rtc-isl12022: Change vendor prefix for Intersil Corporation to isil
From: Philipp Zabel @ 2014-09-08 9:19 UTC (permalink / raw)
To: devicetree
Cc: Mark Rutland, Alessandro Zummo, Russell King, Pawel Moll,
Ian Campbell, Mark Brown, Wolfram Sang, Rob Herring,
Paul Mackerras, Philipp Zabel, Kumar Gala, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1410167960-554-1-git-send-email-p.zabel@pengutronix.de>
Currently there is a wild mixture of isl, isil, and intersil compatibles
in the kernel. At this point, changing the vendor symbol to the most often
used variant, which is equal to the NASDAQ symbol, isil, should not hurt.
Patch db04d6284e2a (drivers/rtc/rtc-isl12022.c: device tree support)
added device tree support using the then documented isl vendor prefix,
so we keep that around for backwards compatibility.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/rtc/rtc-isl12022.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-isl12022.c b/drivers/rtc/rtc-isl12022.c
index aa55f08..df20f18 100644
--- a/drivers/rtc/rtc-isl12022.c
+++ b/drivers/rtc/rtc-isl12022.c
@@ -275,7 +275,8 @@ static int isl12022_probe(struct i2c_client *client,
#ifdef CONFIG_OF
static struct of_device_id isl12022_dt_match[] = {
- { .compatible = "isl,isl12022" },
+ { .compatible = "isil,isl12022" },
+ { .compatible = "isl,isl12022" }, /* for backwards compatibility */
{ },
};
#endif
--
2.1.0.rc1
^ permalink raw reply related
* [PATCH v2 3/5] ARM: mvebu: Change vendor prefix for Intersil Corporation to isil
From: Philipp Zabel @ 2014-09-08 9:19 UTC (permalink / raw)
To: devicetree
Cc: Mark Rutland, Alessandro Zummo, Russell King, Pawel Moll,
Ian Campbell, Mark Brown, Wolfram Sang, Rob Herring,
Paul Mackerras, Philipp Zabel, Kumar Gala, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1410167960-554-1-git-send-email-p.zabel@pengutronix.de>
Currently there is a wild mixture of isl, isil, and intersil
compatibles in the kernel. At this point, changing the vendor
symbol to the most often used variant, which is equal to the
NASDAQ symbol, isil, should not hurt.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
arch/arm/boot/dts/armada-370-netgear-rn102.dts | 2 +-
arch/arm/boot/dts/armada-370-netgear-rn104.dts | 2 +-
arch/arm/boot/dts/armada-xp-netgear-rn2120.dts | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/armada-370-netgear-rn102.dts b/arch/arm/boot/dts/armada-370-netgear-rn102.dts
index d6d572e..edc381c 100644
--- a/arch/arm/boot/dts/armada-370-netgear-rn102.dts
+++ b/arch/arm/boot/dts/armada-370-netgear-rn102.dts
@@ -122,7 +122,7 @@
status = "okay";
isl12057: isl12057@68 {
- compatible = "isl,isl12057";
+ compatible = "isil,isl12057";
reg = <0x68>;
};
diff --git a/arch/arm/boot/dts/armada-370-netgear-rn104.dts b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
index c5fe8b5..7367b4c 100644
--- a/arch/arm/boot/dts/armada-370-netgear-rn104.dts
+++ b/arch/arm/boot/dts/armada-370-netgear-rn104.dts
@@ -117,7 +117,7 @@
status = "okay";
isl12057: isl12057@68 {
- compatible = "isl,isl12057";
+ compatible = "isil,isl12057";
reg = <0x68>;
};
diff --git a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
index 0cf999a..252def8 100644
--- a/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
+++ b/arch/arm/boot/dts/armada-xp-netgear-rn2120.dts
@@ -174,7 +174,7 @@
status = "okay";
isl12057: isl12057@68 {
- compatible = "isl,isl12057";
+ compatible = "isil,isl12057";
reg = <0x68>;
};
--
2.1.0.rc1
^ permalink raw reply related
* [PATCH v2 1/5] of: Change vendor prefix for Intersil Corporation to isil
From: Philipp Zabel @ 2014-09-08 9:19 UTC (permalink / raw)
To: devicetree
Cc: Mark Rutland, Alessandro Zummo, Russell King, Pawel Moll,
Ian Campbell, Mark Brown, Wolfram Sang, Rob Herring,
Paul Mackerras, Philipp Zabel, Kumar Gala, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <1410167960-554-1-git-send-email-p.zabel@pengutronix.de>
Currently there is a wild mixture of isl, isil, and intersil
compatibles in the kernel. At this point, changing the vendor
symbol to the most often used variant, which is equal to the
NASDAQ symbol, isil, should not hurt.
This patch marks both intersil and isl prefix as deprecated
and documents the isil prefix in the vendor prefix list.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
- Added deprecated entries that are still in use
---
Documentation/devicetree/bindings/i2c/trivial-devices.txt | 3 ++-
Documentation/devicetree/bindings/vendor-prefixes.txt | 4 +++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 6af570e..72e399c 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -56,7 +56,8 @@ fsl,sgtl5000 SGTL5000: Ultra Low-Power Audio Codec
gmt,g751 G751: Digital Temperature Sensor and Thermal Watchdog with Two-Wire Interface
infineon,slb9635tt Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
infineon,slb9645tt Infineon SLB9645 I2C TPM (new protocol, max 400khz)
-isl,isl12057 Intersil ISL12057 I2C RTC Chip
+isil,isl12057 Intersil ISL12057 I2C RTC Chip
+isl,isl12057 Intersil ISL12057 I2C RTC Chip (deprecated, use isil,isl12057)
maxim,ds1050 5 Bit Programmable, Pulse-Width Modulator
maxim,max1237 Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs
maxim,max6625 9-Bit/12-Bit Temperature Sensors with I²C-Compatible Serial Interface
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index ac7269f..4941ac2 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -68,7 +68,9 @@ img Imagination Technologies Ltd.
intel Intel Corporation
intercontrol Inter Control Group
isee ISEE 2007 S.L.
-isl Intersil
+intersil Intersil Corporation (deprecated, use isil)
+isil Intersil Corporation
+isl Intersil Corporation (deprecated, use isil)
karo Ka-Ro electronics GmbH
keymile Keymile GmbH
lacie LaCie
--
2.1.0.rc1
^ permalink raw reply related
* Re: [RFC PATCH 0/6] Change vendor prefix for Intersil Corporation
From: Philipp Zabel @ 2014-09-08 9:20 UTC (permalink / raw)
To: Jason Cooper
Cc: Mark Rutland, devicetree, Alessandro Zummo, Russell King,
Pawel Moll, Ian Campbell, Rob Herring, Paul Mackerras, Kumar Gala,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20140907133218.GA30828@titan.lakedaemon.net>
Hi Jason,
Am Sonntag, den 07.09.2014, 09:32 -0400 schrieb Jason Cooper:
> Just to dot our i's and cross our t's, would you mind sending out a
> non-rfc version of this series?
Thank you for the reminder, I have sent an updated version.
regards
Philipp
^ permalink raw reply
* Re: [PATCH 1/2 v5] powerpc/kvm: support to handle sw breakpoint
From: Alexander Graf @ 2014-09-08 13:05 UTC (permalink / raw)
To: Madhavan Srinivasan, benh, paulus, mpe; +Cc: linuxppc-dev, kvm-ppc, kvm
In-Reply-To: <1410107494-25556-2-git-send-email-maddy@linux.vnet.ibm.com>
On 07.09.14 18:31, Madhavan Srinivasan wrote:
> This patch adds kernel side support for software breakpoint.
> Design is that, by using an illegal instruction, we trap to hypervisor
> via Emulation Assistance interrupt, where we check for the illegal instruction
> and accordingly we return to Host or Guest. Patch also adds support for
> software breakpoint in PR KVM.
>
> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/kvm_ppc.h | 6 ++++++
> arch/powerpc/kvm/book3s.c | 3 ++-
> arch/powerpc/kvm/book3s_hv.c | 41 ++++++++++++++++++++++++++++++++++----
> arch/powerpc/kvm/book3s_pr.c | 3 +++
> arch/powerpc/kvm/emulate.c | 18 +++++++++++++++++
> 5 files changed, 66 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
> index fb86a22..dd83c9a 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -38,6 +38,12 @@
> #include <asm/paca.h>
> #endif
>
> +/*
> + * KVMPPC_INST_SW_BREAKPOINT is debug Instruction
> + * for supporting software breakpoint.
> + */
> +#define KVMPPC_INST_SW_BREAKPOINT 0x00dddd00
> +
> enum emulation_result {
> EMULATE_DONE, /* no further processing */
> EMULATE_DO_MMIO, /* kvm_run filled with MMIO request */
> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
> index dd03f6b..00e9c9f 100644
> --- a/arch/powerpc/kvm/book3s.c
> +++ b/arch/powerpc/kvm/book3s.c
> @@ -778,7 +778,8 @@ int kvm_arch_vcpu_ioctl_translate(struct kvm_vcpu *vcpu,
> int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> struct kvm_guest_debug *dbg)
> {
> - return -EINVAL;
> + vcpu->guest_debug = dbg->control;
> + return 0;
> }
>
> void kvmppc_decrementer_func(unsigned long data)
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 27cced9..3a2414c 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -725,6 +725,30 @@ static int kvmppc_hcall_impl_hv(unsigned long cmd)
> return kvmppc_hcall_impl_hv_realmode(cmd);
> }
>
> +static int kvmppc_emulate_debug_inst(struct kvm_run *run,
> + struct kvm_vcpu *vcpu)
> +{
> + u32 last_inst;
> +
> + if(kvmppc_get_last_inst(vcpu, INST_GENERIC, &last_inst) !=
> + EMULATE_DONE) {
> + /*
> + * Fetch failed, so return to guest and
> + * try executing it again.
> + */
> + return RESUME_GUEST;
Please end the if() here. In the kernel we usually treat abort
situations as separate.
> + } else {
> + if (last_inst == KVMPPC_INST_SW_BREAKPOINT) {
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + return RESUME_HOST;
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + return RESUME_GUEST;
> + }
> + }
> +}
> +
> static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> struct task_struct *tsk)
> {
> @@ -807,12 +831,18 @@ static int kvmppc_handle_exit_hv(struct kvm_run *run, struct kvm_vcpu *vcpu,
> break;
> /*
> * This occurs if the guest executes an illegal instruction.
> - * We just generate a program interrupt to the guest, since
> - * we don't emulate any guest instructions at this stage.
> + * If the guest debug is disabled, generate a program interrupt
> + * to the guest. If guest debug is enabled, we need to check
> + * whether the instruction is a software breakpoint instruction.
> + * Accordingly return to Guest or Host.
> */
> case BOOK3S_INTERRUPT_H_EMUL_ASSIST:
> - kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> - r = RESUME_GUEST;
> + if(vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP) {
> + r = kvmppc_emulate_debug_inst(run, vcpu);
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + r = RESUME_GUEST;
> + }
> break;
> /*
> * This occurs if the guest (kernel or userspace), does something that
> @@ -922,6 +952,9 @@ static int kvmppc_get_one_reg_hv(struct kvm_vcpu *vcpu, u64 id,
> long int i;
>
> switch (id) {
> + case KVM_REG_PPC_DEBUG_INST:
> + *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
> + break;
> case KVM_REG_PPC_HIOR:
> *val = get_reg_val(id, 0);
> break;
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index faffb27..6d73708 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -1319,6 +1319,9 @@ static int kvmppc_get_one_reg_pr(struct kvm_vcpu *vcpu, u64 id,
> int r = 0;
>
> switch (id) {
> + case KVM_REG_PPC_DEBUG_INST:
> + *val = get_reg_val(id, KVMPPC_INST_SW_BREAKPOINT);
> + break;
> case KVM_REG_PPC_HIOR:
> *val = get_reg_val(id, to_book3s(vcpu)->hior);
> break;
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> index e96b50d..30f326b 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -274,6 +274,24 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
> }
> break;
>
> + case 0:
> + /*
> + * Instruction with primary opcode 0. Based on PowerISA
> + * these are illegal instructions.
> + */
> + if(inst == KVMPPC_INST_SW_BREAKPOINT) {
Here you're missing a space between if and (. Are you sure that you ran
checkpatch.pl?
> + run->exit_reason = KVM_EXIT_DEBUG;
> + run->debug.arch.address = kvmppc_get_pc(vcpu);
> + emulated = EMULATE_EXIT_USER;
> + advance = 0;
> + printk(KERN_INFO "pr_emulate: debug instr \n");
> + } else {
> + kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
> + emulated = EMULATE_DONE;
> + advance = 0;
Just set emulated = EMULATE_FAIL here, the same as default:.
Alex
> + }
> + break;
> +
> default:
> emulated = EMULATE_FAIL;
> }
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox