Re: [PATCH] alpha: add udelay to io port paths
On 4/5/2019 2:28 PM, Maciej W. Rozycki wrote: You basically can't remove the mb() after __ioread() and before __iowrite() unless your architecture guarantees IO vs. memory ordering. I never suggested that; please check my example too. Sounds good. Sorry for the noise. I just wanted to make sure you are not removing mb() after read(). I'm catching up here.
Re: [PATCH] alpha: add udelay to io port paths
On 4/5/2019 1:29 PM, Maciej W. Rozycki wrote: Obviously you do need that `mb' before `__ioread' in the second case, just like in the first one, because otherwise the read bus access issued by `__ioread' can be reordered ahead of the write bus access issued by the preceding `__iowrite'. Please also not that you also need a mb() after read to prevent stale from being read from memory. write_to_memory_for_dma() mb(); < DMA ordering requirement.Prefer wmb() if possible. __iowrite(123, INDEX); mb(); < alpha arch requirement due to instruction reordering. x = __ioread(DATA); mb(); < DMA ordering requirement. Prefer rmb() if possible. read_from_dma_memory() You basically can't remove the mb() after __ioread() and before __iowrite() unless your architecture guarantees IO vs. memory ordering.
Re: [PATCH] add delay between port write and port read
On 2/27/2019 12:12 PM, Mikulas Patocka wrote: It used to be like that and it worked. Then, commits cd0e00c106722eca40b38ebf11cf134c01901086 and 92d7223a74235054f2aa7227d207d9c57f84dca0 came. These commits claim that they changed the code to be consistent with the specification (now we have barrier after ioread and before iowrite). But they broke serial port and RTC on my Alpha machine. A barrier before write is needed to ensure that memory operations done are visible to the hardware before you send a write command. Barrier after read is needed to ensure that if you read a memory location after register read, memory contents are coherent. What we missed is the fact that alpha reorders accesses across two register accesses. This is guaranteed in other architectures. To satisfy this, you need something like barrier register write barrier register read barrier
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/22/2018 3:56 PM, Mikulas Patocka wrote: On Wed, 22 Aug 2018, Sinan Kaya wrote: On 8/22/2018 1:47 PM, Mikulas Patocka wrote: If ARM guarantees that the accesses to a given device are not reordered - then the barriers in readl and writel are superfluous. It is not. ARM only guarantees ordering of read/write transactions targeting a device not memory. example: write memory raw write to device or raw read from device read memory these can bypass each other on ARM unless a barrier is placed in the right place either via readl()/writel() or explicitly. Yes - but - why does Linux insert the barriers into readl() and writel() instead of inserting them between accesses to registers and memory? A lot of drivers have long sequences of accesses to memory-mapped registers with no interleaving accesses to coherent memory and these implicit barriers slow them down with no gain at all. It is an abstraction issue. Majority of drivers are developed against x86 and the developers have no idea about the weakly ordered architecture implications. Now, Will Deacon added new primitives to address your concern. There are new APIs as readl_relaxed() and writel_relaxed() as opposed to readl() and writel(). Relaxed version still guarantee of register accesses with respect to each other but no guaranteed with respect to memory. Relaxed versions could be used in performance critical path. Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/22/2018 1:47 PM, Mikulas Patocka wrote: If ARM guarantees that the accesses to a given device are not reordered - then the barriers in readl and writel are superfluous. It is not. ARM only guarantees ordering of read/write transactions targeting a device not memory. example: write memory raw write to device or raw read from device read memory these can bypass each other on ARM unless a barrier is placed in the right place either via readl()/writel() or explicitly. raw write to device raw write to device or raw write to device raw read from device or raw read from device raw read from device are guaranteed to be ordered on ARM without needing any explicit barrier.
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/22/2018 7:59 AM, Mikulas Patocka wrote: I did some tests on framebuffer and found out that "read+read+write+write" is faster than "read+write+read+write" - that may suggest that the reads flush the write queue. I think we are worried about correctness at this moment like a raw_read following raw_write is getting ahead of raw_write and reading some stale data. I think a test scenario for this can be constructed as follows: 1. raw_write magic 2. raw_read 3. fail if value is not magic 4. barrier to go to sane state 5. raw_write magic2 6. barrier to ensure magic2 is observed 7. goto 1 If raw_read is getting ahead of raw_write, it should observe magic2 at step #3.
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/20/2018 6:39 PM, Maciej W. Rozycki wrote:
On Mon, 20 Aug 2018, Sinan Kaya wrote:
That is that the caller must not
assume that writes issued by `writeX' calls will be observed in order on
the external bus or specifically by the device addressed.
Where do you see it?
Right in the first paragraph of io_ordering.txt, and then further
demonstrated by the examples provided ("In the case above, the device may
receive newval2 before it receives newval, which could cause problems.").
I interpret that two writeX() need to be observed in order with respect
to each other without requiring an explicit barrier. Same goes for reads.
Nope, only if `readX' is in between.
Likewise see memory-barriers.txt throughout concerning `mmiowb' (which is
an obviously lighter weight barrier compared to `readX').
Here is a better reference from memory-barriers.txt
(*) readX(), writeX():
Whether these are guaranteed to be fully ordered and uncombined with
respect to each other on the issuing CPU depends on the
characteristics
defined for the memory window through which they're accessing. On
later
i386 architecture machines, for example, this is controlled by way
of the
MTRR registers.
Ordinarily, these will be guaranteed to be fully ordered and
uncombined,
provided they're not accessing a prefetchable device.
Maciej
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/20/2018 5:50 PM, Maciej W. Rozycki wrote: That is that the caller must not assume that writes issued by `writeX' calls will be observed in order on the external bus or specifically by the device addressed. Where do you see it? I interpret that two writeX() need to be observed in order with respect to each other without requiring an explicit barrier. Same goes for reads. Hardware can reshuffle things on flight for performance but observability is the critical piece here for correctness.
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/19/2018 11:41 AM, Arnd Bergmann wrote: Sounds like we need a similar solution for all architectures that require mmiowb(). That depends on how expensive the barrier is on a given architecture. It's possible that doing adding the barrier every time is actually cheaper than keeping track of whether it's needed. I agree I have no idea about these mentioned architectures and their barrier types. If adding mmiowb() is cheaper that could be the way to go as well. If the barrier is heavy, we definitely want to execute as many register writes as possible before issuing the barrier.
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On 8/19/2018 11:21 AM, Arnd Bergmann wrote: This matches my understanding of mmiowb. In fact, mmiowb is a powerpc thing. All other architectures stub out. There are a few architectures that define mmiowb to something other than a nop: some ia64, some mips and sh in particular, plus also the new riscv. It does make some sense that alpha might need a barrier between an mmio store and a spin_unlock, but it was decided earlier to make that barrier implied by the writel() function rather than require an explicit mmiowb after it. I see. When I talked to PowerPC guys a couple of months ago, they told me that they keep a dirty flag to keep track of mmiowb() required condition on each writel() execution. If the dirty flag is set; they issue the mmiowb() inside the spin_unlock() function. Sounds like we need a similar solution for all architectures that require mmiowb().
Re: [PATCH] alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2
On 4/20/2018 12:20 PM, Sinan Kaya wrote: > Hi Matt, > > On 4/17/2018 2:43 PM, Sinan Kaya wrote: >> On 4/16/2018 6:16 PM, Sinan Kaya wrote: >>> memory-barriers.txt has been updated with the following requirement. >>> >>> "When using writel(), a prior wmb() is not needed to guarantee that the >>> cache coherent memory writes have completed before writing to the MMIO >>> region." >>> >>> Current writeX() and iowriteX() implementations on alpha are not >>> satisfying this requirement as the barrier is after the register write. >>> >>> Move mb() in writeX() and iowriteX() functions to guarantee that HW >>> observes memory changes before performing register operations. >>> >>> Signed-off-by: Sinan Kaya <ok...@codeaurora.org> >>> Reported-by: Arnd Bergmann <a...@arndb.de> >>> --- >>> arch/alpha/kernel/io.c | 14 +++--- >>> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> Sorry for catching this late but this also needs to go to 4.17 after >> review. >> >> I missed the writel() implementation on arch/alpha/kernel/io.c file >> on my first patch. >> > > Can you also queue this for 4.17? > > There are already drivers checked into 4.17 that dropped the unnecessary > barriers. > > I really hate to see Alpha broken because of this. ping. > > Sinan > -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-alpha" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2
Hi Matt, On 4/17/2018 2:43 PM, Sinan Kaya wrote: > On 4/16/2018 6:16 PM, Sinan Kaya wrote: >> memory-barriers.txt has been updated with the following requirement. >> >> "When using writel(), a prior wmb() is not needed to guarantee that the >> cache coherent memory writes have completed before writing to the MMIO >> region." >> >> Current writeX() and iowriteX() implementations on alpha are not >> satisfying this requirement as the barrier is after the register write. >> >> Move mb() in writeX() and iowriteX() functions to guarantee that HW >> observes memory changes before performing register operations. >> >> Signed-off-by: Sinan Kaya <ok...@codeaurora.org> >> Reported-by: Arnd Bergmann <a...@arndb.de> >> --- >> arch/alpha/kernel/io.c | 14 +++--- >> 1 file changed, 7 insertions(+), 7 deletions(-) > > Sorry for catching this late but this also needs to go to 4.17 after > review. > > I missed the writel() implementation on arch/alpha/kernel/io.c file > on my first patch. > Can you also queue this for 4.17? There are already drivers checked into 4.17 that dropped the unnecessary barriers. I really hate to see Alpha broken because of this. Sinan -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. -- To unsubscribe from this list: send the line "unsubscribe linux-alpha" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering
On 4/2/2018 1:48 PM, Sinan Kaya wrote:
> memory-barriers.txt has been updated with the following requirement.
>
> "When using writel(), a prior wmb() is not needed to guarantee that the
> cache coherent memory writes have completed before writing to the MMIO
> region."
>
> Current writeX() and iowriteX() implementations on alpha are not
> satisfying this requirement as the barrier is after the register write.
>
> Move mb() in writeX() and iowriteX() functions to guarantee that HW
> observes memory changes before performing register operations.
>
> Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> Reported-by: Arnd Bergmann <a...@arndb.de>
> ---
> arch/alpha/include/asm/io.h | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> index d123ff9..4c533fc 100644
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -341,14 +341,14 @@ extern inline unsigned int ioread16(void __iomem *addr)
>
> extern inline void iowrite8(u8 b, void __iomem *addr)
> {
> - IO_CONCAT(__IO_PREFIX,iowrite8)(b, addr);
> mb();
> + IO_CONCAT(__IO_PREFIX, iowrite8)(b, addr);
> }
>
> extern inline void iowrite16(u16 b, void __iomem *addr)
> {
> - IO_CONCAT(__IO_PREFIX,iowrite16)(b, addr);
> mb();
> + IO_CONCAT(__IO_PREFIX, iowrite16)(b, addr);
> }
>
> extern inline u8 inb(unsigned long port)
> @@ -382,8 +382,8 @@ extern inline unsigned int ioread32(void __iomem *addr)
>
> extern inline void iowrite32(u32 b, void __iomem *addr)
> {
> - IO_CONCAT(__IO_PREFIX,iowrite32)(b, addr);
> mb();
> + IO_CONCAT(__IO_PREFIX, iowrite32)(b, addr);
> }
>
> extern inline u32 inl(unsigned long port)
> @@ -434,14 +434,14 @@ extern inline u16 readw(const volatile void __iomem
> *addr)
>
> extern inline void writeb(u8 b, volatile void __iomem *addr)
> {
> - __raw_writeb(b, addr);
> mb();
> + __raw_writeb(b, addr);
> }
>
> extern inline void writew(u16 b, volatile void __iomem *addr)
> {
> - __raw_writew(b, addr);
> mb();
> + __raw_writew(b, addr);
> }
> #endif
>
> @@ -482,14 +482,14 @@ extern inline u64 readq(const volatile void __iomem
> *addr)
>
> extern inline void writel(u32 b, volatile void __iomem *addr)
> {
> - __raw_writel(b, addr);
> mb();
> + __raw_writel(b, addr);
> }
>
> extern inline void writeq(u64 b, volatile void __iomem *addr)
> {
> - __raw_writeq(b, addr);
> mb();
> + __raw_writeq(b, addr);
> }
> #endif
>
>
Can we get these merged to 4.17?
There was a consensus to fix the architectures having API violation issues.
https://www.mail-archive.com/netdev@vger.kernel.org/msg225971.html
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering
memory-barriers.txt has been updated with the following requirement.
"When using writel(), a prior wmb() is not needed to guarantee that the
cache coherent memory writes have completed before writing to the MMIO
region."
Current writeX() and iowriteX() implementations on alpha are not
satisfying this requirement as the barrier is after the register write.
Move mb() in writeX() and iowriteX() functions to guarantee that HW
observes memory changes before performing register operations.
Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
Reported-by: Arnd Bergmann <a...@arndb.de>
---
arch/alpha/include/asm/io.h | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
index d123ff9..4c533fc 100644
--- a/arch/alpha/include/asm/io.h
+++ b/arch/alpha/include/asm/io.h
@@ -341,14 +341,14 @@ extern inline unsigned int ioread16(void __iomem *addr)
extern inline void iowrite8(u8 b, void __iomem *addr)
{
- IO_CONCAT(__IO_PREFIX,iowrite8)(b, addr);
mb();
+ IO_CONCAT(__IO_PREFIX, iowrite8)(b, addr);
}
extern inline void iowrite16(u16 b, void __iomem *addr)
{
- IO_CONCAT(__IO_PREFIX,iowrite16)(b, addr);
mb();
+ IO_CONCAT(__IO_PREFIX, iowrite16)(b, addr);
}
extern inline u8 inb(unsigned long port)
@@ -382,8 +382,8 @@ extern inline unsigned int ioread32(void __iomem *addr)
extern inline void iowrite32(u32 b, void __iomem *addr)
{
- IO_CONCAT(__IO_PREFIX,iowrite32)(b, addr);
mb();
+ IO_CONCAT(__IO_PREFIX, iowrite32)(b, addr);
}
extern inline u32 inl(unsigned long port)
@@ -434,14 +434,14 @@ extern inline u16 readw(const volatile void __iomem *addr)
extern inline void writeb(u8 b, volatile void __iomem *addr)
{
- __raw_writeb(b, addr);
mb();
+ __raw_writeb(b, addr);
}
extern inline void writew(u16 b, volatile void __iomem *addr)
{
- __raw_writew(b, addr);
mb();
+ __raw_writew(b, addr);
}
#endif
@@ -482,14 +482,14 @@ extern inline u64 readq(const volatile void __iomem *addr)
extern inline void writel(u32 b, volatile void __iomem *addr)
{
- __raw_writel(b, addr);
mb();
+ __raw_writel(b, addr);
}
extern inline void writeq(u64 b, volatile void __iomem *addr)
{
- __raw_writeq(b, addr);
mb();
+ __raw_writeq(b, addr);
}
#endif
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Broken DMA vs MMIO ordering
Hi Alpha Maintainers,
memory-barriers.txt has been updated not to require a wmb() before writel()
since Linus asked all infrastructures to follow Intel paradigm where writes
are ordered and they do not require a barrier between a memory update and
HW observation.
https://www.mail-archive.com/netdev@vger.kernel.org/msg225806.html
https://lkml.org/lkml/2018/3/27/431
We have been auditing all architectures to see if they follow this requirement
or not.
Arnd raised the following concern
"
extern inline u32 readl(const volatile void __iomem *addr)
{
u32 ret = __raw_readl(addr);
mb();
return ret;
}
extern inline void writel(u32 b, volatile void __iomem *addr)
{
__raw_writel(b, addr);
mb();
}
There is a barrier in writel /after/ the access but not before."
Can somebody familiar with alpha evaluate this?
Sinan
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 01/26] alpha/PCI: deprecate pci_get_bus_and_slot()
On 12/19/2017 12:37 AM, Sinan Kaya wrote:
> pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
> where a PCI device is present. This restricts the device drivers to be
> reused for other domain numbers.
>
> Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't
> extract the domain number. Other places, use the actual domain number from
> the device.
>
> Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> ---
> arch/alpha/kernel/pci.c | 2 +-
> arch/alpha/kernel/sys_nautilus.c | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
> index 87da005..2e86ebb 100644
> --- a/arch/alpha/kernel/pci.c
> +++ b/arch/alpha/kernel/pci.c
> @@ -425,7 +425,7 @@ struct resource * __init
> if (bus == 0 && dfn == 0) {
> hose = pci_isa_hose;
> } else {
> - dev = pci_get_bus_and_slot(bus, dfn);
> + dev = pci_get_domain_bus_and_slot(0, bus, dfn);
> if (!dev)
> return -ENODEV;
> hose = dev->sysdata;
> diff --git a/arch/alpha/kernel/sys_nautilus.c
> b/arch/alpha/kernel/sys_nautilus.c
> index 239dc0e..ff4f54b 100644
> --- a/arch/alpha/kernel/sys_nautilus.c
> +++ b/arch/alpha/kernel/sys_nautilus.c
> @@ -237,7 +237,7 @@
> bus = hose->bus = bridge->bus;
> pcibios_claim_one_bus(bus);
>
> - irongate = pci_get_bus_and_slot(0, 0);
> + irongate = pci_get_domain_bus_and_slot(pci_domain_nr(bus), 0, 0);
> bus->self = irongate;
> bus->resource[0] = _io;
> bus->resource[1] = _mem;
>
Any feedback here? most of the remaining patches have the ACK except these.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux
Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/30] alpha/PCI: deprecate pci_get_bus_and_slot()
pci_get_bus_and_slot() is restrictive such that it assumes domain=0 as
where a PCI device is present. This restricts the device drivers to be
reused for other domain numbers.
Use pci_get_domain_bus_and_slot() with a domain number of 0 where we can't
extract the domain number. Other places, use the actual domain number from
the device.
Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
---
arch/alpha/kernel/pci.c | 2 +-
arch/alpha/kernel/sys_nautilus.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 08235bb..e866daf 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -416,7 +416,7 @@ struct resource * __init
if (bus == 0 && dfn == 0) {
hose = pci_isa_hose;
} else {
- dev = pci_get_bus_and_slot(bus, dfn);
+ dev = pci_get_domain_bus_and_slot(0, bus, dfn);
if (!dev)
return -ENODEV;
hose = dev->sysdata;
diff --git a/arch/alpha/kernel/sys_nautilus.c b/arch/alpha/kernel/sys_nautilus.c
index 239dc0e..5eacaf9 100644
--- a/arch/alpha/kernel/sys_nautilus.c
+++ b/arch/alpha/kernel/sys_nautilus.c
@@ -237,7 +237,7 @@
bus = hose->bus = bridge->bus;
pcibios_claim_one_bus(bus);
- irongate = pci_get_bus_and_slot(0, 0);
+ irongate = pci_get_domain_bus_and_slot(0, 0, 0);
bus->self = irongate;
bus->resource[0] = _io;
bus->resource[1] = _mem;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-alpha" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
