Re: [PATCH] alpha: add udelay to io port paths
On Fri, 5 Apr 2019, Maciej W. Rozycki wrote:
> On Fri, 5 Apr 2019, Mikulas Patocka wrote:
>
> > > > I did some more testing and it turns out that mb() is not entirely
> > > > sufficient to prevent the boot hang. mb()'s latecy varies, sometimes it
> > > > is
> > > > sufficient, sometimes not.
> > > >
> > > > So, I submit this patch that adds 1us delay between any I/O accesses
> > > > directed at the ISA bus. This patch makes my machine boot. 1us seems to
> > > > be
> > > > minimal acceptable value, with 800ns I still get hangs.
> > >
> > > Why wasn't the delay needed then before commit cd0e00c10672 ("alpha: io:
> > > reorder barriers to guarantee writeX() and iowriteX() ordering"), which
> > > only moved `mb' around?
> > >
> > > Maciej
> >
> > Suppose that someone does
> > outl(123, INDEX); x = inl(DATA);
> >
> > Before the patch cd0e00c10672, the kernel would do
> > __iowrite(123, INDEX);
> > mb();
> > x = __ioread(DATA);
> > mb();
> >
> > After the patch cd0e00c10672, the kernel would do
> > mb();
> > __iowrite(123, INDEX);
> > x = __ioread(DATA);
> > mb();
> >
> > The patch changes the timing between the write and the read and the
> > hardware doesn't like it.
>
> 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'.
So, take my patch that adds the "mb()" there:
https://www.spinics.net/lists/linux-alpha/msg05007.html
This bug can be fixed by adding "mb()", by adding udelay() or by adding a
dummy read of port 0x80.
So, choose one of these ways to fix it and commit it to the kernel.
Mikulas
[PATCH] alpha: fix rtc port ranges
Alpha incorrectly reports "0070-0080 : rtc" in /proc/ioports. Fix this, so that it is "0070-007f". Signed-off-by: Mikulas Patocka --- arch/alpha/kernel/setup.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Index: linux-stable/arch/alpha/kernel/setup.c === --- linux-stable.orig/arch/alpha/kernel/setup.c 2019-04-05 12:42:34.0 +0200 +++ linux-stable/arch/alpha/kernel/setup.c 2019-04-05 12:42:34.0 +0200 @@ -254,7 +254,7 @@ reserve_std_resources(void) /* Fix up for the Jensen's queer RTC placement. */ standard_io_resources[0].start = RTC_PORT(0); - standard_io_resources[0].end = RTC_PORT(0) + 0x10; + standard_io_resources[0].end = RTC_PORT(0) + 0x0f; for (i = 0; i < ARRAY_SIZE(standard_io_resources); ++i) request_resource(io, standard_io_resources+i);
Re: [PATCH] alpha: add udelay to io port paths
On Thu, 4 Apr 2019, Linus Torvalds wrote:
> On Wed, Apr 3, 2019 at 9:28 AM Mikulas Patocka wrote:
> >
> > So, I submit this patch that adds 1us delay between any I/O accesses
> > directed at the ISA bus. This patch makes my machine boot. 1us seems to be
> > minimal acceptable value, with 800ns I still get hangs.
>
> I don't think this is right.
>
> The *read* in 'in[bwl]' likely should not need an extra delay. An ISA
> port access will simply take that long on proper ISA hardware.
>
> The problem is almost c ertainly on the write side. An 'out[bwl]' on
> x86 will actualluy wait for the write to complete, while I suspect the
> problem on alpha is that writes are posted, so they happen
> "immediately" as far as the CPU is concerned. The 1us delay will then
> effectively wait for the write to complete on the device.
>
> I wonder if there is some way on alpha to wait for ISA writes to
> complete (perhaps doing a dummy read from the IO complex)?
>
> Linus
You're right that the delay is only needed for writes, there's no need for
it when reading.
Here I'm resending the patch, so that it only delays after writes and it
uses dummy reads of port 0x80 instead of udelay.
Mikulas
From: Mikulas Patocka
Subject: [PATCH] alpha: add delay to io port paths
The patches cd0e00c106722eca40b38ebf11cf134c01901086 and
92d7223a74235054f2aa7227d207d9c57f84dca0 fix a theoretical issue where the
code didn't follow the specification. Unfortunatelly, they also reduce
timing between IO port write and IO port read.
This causes problem on Alpha Avanti. Old chipsets based in the ISA bus
have problems with back-to-back I/O accesses and require delays. The
"mb()" call used to serve as a delay, hiding the bug.
However, using mb() for delay is unreliable, so we'd better add
explicit delays.
This patch adds the required delays after writes to the ISA bus, using
dummy read of the port 0x80.
Signed-off-by: Mikulas Patocka
Fixes: cd0e00c10672 ("alpha: io: reorder barriers to guarantee writeX() and
iowriteX() ordering")
Cc: sta...@vger.kernel.org # v4.17+
---
arch/alpha/kernel/io.c | 12
1 file changed, 12 insertions(+)
Index: linux-stable/arch/alpha/kernel/io.c
===
--- linux-stable.orig/arch/alpha/kernel/io.c2019-04-05 12:47:35.0
+0200
+++ linux-stable/arch/alpha/kernel/io.c 2019-04-05 12:47:43.0 +0200
@@ -60,6 +60,12 @@ EXPORT_SYMBOL(iowrite8);
EXPORT_SYMBOL(iowrite16);
EXPORT_SYMBOL(iowrite32);
+static void delay_if_isa(unsigned long port)
+{
+ if (port < 0x400)
+ inb(0x80);
+}
+
u8 inb(unsigned long port)
{
return ioread8(ioport_map(port, 1));
@@ -78,16 +84,19 @@ u32 inl(unsigned long port)
void outb(u8 b, unsigned long port)
{
iowrite8(b, ioport_map(port, 1));
+ delay_if_isa(port);
}
void outw(u16 b, unsigned long port)
{
iowrite16(b, ioport_map(port, 2));
+ delay_if_isa(port);
}
void outl(u32 b, unsigned long port)
{
iowrite32(b, ioport_map(port, 4));
+ delay_if_isa(port);
}
EXPORT_SYMBOL(inb);
@@ -336,6 +345,7 @@ void iowrite8_rep(void __iomem *port, co
void outsb(unsigned long port, const void *src, unsigned long count)
{
iowrite8_rep(ioport_map(port, 1), src, count);
+ delay_if_isa(port);
}
EXPORT_SYMBOL(iowrite8_rep);
@@ -376,6 +386,7 @@ void iowrite16_rep(void __iomem *port, c
void outsw(unsigned long port, const void *src, unsigned long count)
{
iowrite16_rep(ioport_map(port, 2), src, count);
+ delay_if_isa(port);
}
EXPORT_SYMBOL(iowrite16_rep);
@@ -408,6 +419,7 @@ void iowrite32_rep(void __iomem *port, c
void outsl(unsigned long port, const void *src, unsigned long count)
{
iowrite32_rep(ioport_map(port, 4), src, count);
+ delay_if_isa(port);
}
EXPORT_SYMBOL(iowrite32_rep);
Re: [PATCH] alpha: add udelay to io port paths
On Thu, 4 Apr 2019, Maciej W. Rozycki wrote:
> On Wed, 3 Apr 2019, Mikulas Patocka wrote:
>
> > I did some more testing and it turns out that mb() is not entirely
> > sufficient to prevent the boot hang. mb()'s latecy varies, sometimes it is
> > sufficient, sometimes not.
> >
> > So, I submit this patch that adds 1us delay between any I/O accesses
> > directed at the ISA bus. This patch makes my machine boot. 1us seems to be
> > minimal acceptable value, with 800ns I still get hangs.
>
> Why wasn't the delay needed then before commit cd0e00c10672 ("alpha: io:
> reorder barriers to guarantee writeX() and iowriteX() ordering"), which
> only moved `mb' around?
>
> Maciej
Suppose that someone does
outl(123, INDEX); x = inl(DATA);
Before the patch cd0e00c10672, the kernel would do
__iowrite(123, INDEX);
mb();
x = __ioread(DATA);
mb();
After the patch cd0e00c10672, the kernel would do
mb();
__iowrite(123, INDEX);
x = __ioread(DATA);
mb();
The patch changes the timing between the write and the read and the
hardware doesn't like it.
Mikulas
[PATCH] alpha: add udelay to io port paths
On Wed, 20 Feb 2019, Maciej W. Rozycki wrote:
> On Wed, 20 Feb 2019, Mikulas Patocka wrote:
>
> > > Well, actually `iowriteX' is generic and for MMIO you have `writeX'.
> > > See lib/iomap.c, the comment at the top and e.g. `iowrite8' there for an
> > > actual proof. Alpha may have an oddball implementation, but there you
> > > go.
> > > Drivers will assume they can do `iowriteX' to any kind of I/O resource,
> > > and ordering must be respected as per Documentation/memory-barriers.txt.
> >
> > So, do you think that the barrier whould be added to iowriteX and slow
> > down every MMIO access?
>
> We need it either for `outX' and `iowriteX' calls operating on port I/O
> resources, or for neither of them, both at a time, to ensure the required
> consistency between the two interfaces. If that badly affects MMIO (and
> is not required there; please remind me what the exact justification to
> use `mb' here is, as it's not entirely clear to me from the commit
> message; `mb' is a barrier and not means for a delay), then we need to
> find a away for `iowriteX' to tell port I/O and MMIO accesses apart and
> only apply the barrier for the former kind.
>
> Maciej
I did some more testing and it turns out that mb() is not entirely
sufficient to prevent the boot hang. mb()'s latecy varies, sometimes it is
sufficient, sometimes not.
So, I submit this patch that adds 1us delay between any I/O accesses
directed at the ISA bus. This patch makes my machine boot. 1us seems to be
minimal acceptable value, with 800ns I still get hangs.
Mikulas
From: Mikulas Patocka
Subject: [PATCH] alpha: add udelay to io port paths
The patches cd0e00c106722eca40b38ebf11cf134c01901086 and
92d7223a74235054f2aa7227d207d9c57f84dca0 fix a theoretical issue where the
code didn't follow the specification. Unfortunatelly, they also reduce
timing between IO port write and IO port read.
This causes problem on Alpha Avanti. Old chipsets based in the ISA bus
have problems with back-to-back I/O accesses and require delays. The
"mb()" call used to serve as a delay, hiding the bug.
However, using mb() for delay is unreliable, so we'd better add
explicit delays.
This patch adds the required delays between accesses to the ISA bus.
Signed-off-by: Mikulas Patocka
Fixes: cd0e00c10672 ("alpha: io: reorder barriers to guarantee writeX() and
iowriteX() ordering")
Cc: sta...@vger.kernel.org # v4.17+
---
arch/alpha/kernel/io.c | 28 +---
1 file changed, 25 insertions(+), 3 deletions(-)
Index: linux-stable/arch/alpha/kernel/io.c
===
--- linux-stable.orig/arch/alpha/kernel/io.c2019-04-03 15:26:53.0
+0200
+++ linux-stable/arch/alpha/kernel/io.c 2019-04-03 15:28:57.0 +0200
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
/* Out-of-line versions of the i/o routines that redirect into the
platform-specific version. Note that "platform-specific" may mean
@@ -60,34 +61,49 @@ EXPORT_SYMBOL(iowrite8);
EXPORT_SYMBOL(iowrite16);
EXPORT_SYMBOL(iowrite32);
+static void udelay_if_isa(unsigned long port)
+{
+ if (port < 0x400)
+ udelay(1);
+}
+
u8 inb(unsigned long port)
{
- return ioread8(ioport_map(port, 1));
+ u8 r = ioread8(ioport_map(port, 1));
+ udelay_if_isa(port);
+ return r;
}
u16 inw(unsigned long port)
{
- return ioread16(ioport_map(port, 2));
+ u16 r = ioread16(ioport_map(port, 2));
+ udelay_if_isa(port);
+ return r;
}
u32 inl(unsigned long port)
{
- return ioread32(ioport_map(port, 4));
+ u32 r = ioread32(ioport_map(port, 4));
+ udelay_if_isa(port);
+ return r;
}
void outb(u8 b, unsigned long port)
{
iowrite8(b, ioport_map(port, 1));
+ udelay_if_isa(port);
}
void outw(u16 b, unsigned long port)
{
iowrite16(b, ioport_map(port, 2));
+ udelay_if_isa(port);
}
void outl(u32 b, unsigned long port)
{
iowrite32(b, ioport_map(port, 4));
+ udelay_if_isa(port);
}
EXPORT_SYMBOL(inb);
@@ -242,6 +258,7 @@ void ioread8_rep(void __iomem *port, voi
void insb(unsigned long port, void *dst, unsigned long count)
{
ioread8_rep(ioport_map(port, 1), dst, count);
+ udelay_if_isa(port);
}
EXPORT_SYMBOL(ioread8_rep);
@@ -282,6 +299,7 @@ void ioread16_rep(void __iomem *port, vo
void insw(unsigned long port, void *dst, unsigned long count)
{
ioread16_rep(ioport_map(port, 2), dst, count);
+ udelay_if_isa(port);
}
EXPORT_SYMBOL(ioread16_rep);
@@ -314,6 +332,7 @@ void ioread32_rep(void __iomem *port, vo
void insl(unsigned long port, void *dst, unsigned long count)
{
ioread32_rep(ioport_map(port, 4), dst, count);
+ udelay_if_isa(port);
}
EXPORT_SYMBOL(
Re: [PATCH] add delay between port write and port read
On Tue, 26 Feb 2019, Linus Torvalds wrote: > Does anybody see any worries with the "just move the mb() earlier in > the ioread functions" model? > > Linus 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. Mikulas
Re: [PATCH] add delay between port write and port read
On Tue, 26 Feb 2019, Linus Torvalds wrote: > On Tue, Feb 26, 2019 at 10:38 AM Will Deacon wrote: > > > > That makes sense to me for this Alpha-specific case, but in general I > > don't think we require that I/O accesses to different endpoints are > > ordered with respect to each other, which was implied by one of Maciej's > > examples. e.g. > > > > writeb(0x42, _mmio_reg); > > readb(_mmio_reg); > > If they are the same device (just different data ports), I'd > *definitely* expect them to be ordered. > > We have tons of code that depends on that. Almost every driver out > there, in fact. > > So we need the mb() on alpha to guarantee the access ordering on the > CPU side, and then PCI itself ends up guaranteeing that accesses to > the same device will remain ordered outside the CPU. > > Agreed? > > Linus Should "writeb_relaxed" on Alpha also use the barrier? The understanding is that readX_relaxed and writeX_relaxed are ordered with other accesses to the same PCI device. If Alpha doesn't gaurantee that, they should use barriers too. Mikulas
Re: [PATCH] add delay between port write and port read
Do you think that we should fix this by identifying hardware that needs the delays and adding the delays there? In my opinion, adding mb() to the port accessing functions is safer - it is 6 line patch. Reading all the hardware manuals is time consuming and hardly anyone will do it for 25 years old hardware. Mikulas On Wed, 20 Feb 2019, Maciej W. Rozycki wrote: > On Wed, 20 Feb 2019, Mikulas Patocka wrote: > > > > Will Deacon is in the process of sanitizing our documentation for this, > > > and this part is still under discussion. > > > > > > I personally don't see a problem with having different barrier > > > semantics between outb() and iowrite8(), the reasoning being that > > > any caller of iowriteX() would already have to be careful about > > > posted writes when iowriteX is backed by ioremapped memory > > > space rather than port I/O. > > > > > > I think the more important question we have to figure out here > > > however is why exactly the barrier is needed for alpha, as I still > > > don't fully understand the issue: > > > > Port delays were common on MS-DOS with the ISA bus. On the ISA bus, the > > device cannot reject a transaction, so the programmer has to wait until > > the device is ready before talking to it. > > > > My hypothesis is that the engineers who built this Alpha Avanti machine > > simply bought some crappy serial line and real-time clock logic from some > > ISA bus vendor. And so it requires delays. The PCI stuff on the same > > machine doesn't require any extra delays. > > What you call crap was in the day, i.e. back in 1994, a state-of-the art > ISA super I/O chip (the NS87332) and a standard RTC chip (the BQ3287). > There was no LPC in existence yet when the Avanti line was built, the > south bridge used was the i82378 SIO, additionally wiring two actual ISA > slots. > > Datasheets fo all these pieces should be available; I'm fairly sure I > have at least some of them myself. Also full engineering documentation is > available for the Avanti. So it can all be verified quite easily. > > If any of these devices need actual ISA access delays, then `outX_p' and > `inX_p' accessors should be used by the respective drivers, and we need to > implement these accordingly; that would be no different to a standard x86 > PC with say an ISA serial port (I still have such hardware BTW). > > BTW, I have recently got an Avanti system myself, an AS/250 I believe, > although for an unknown reason placed in an AS/300 enclosure. The main > board of the two systems is identical and the only difference is the > maximum amount of RAM supported anyway. I have't wired it properly yet > though, so I can't do any verification ATM, but I will try sometime later. > > > > a) maybe the barrier here is only needed to provide the > > >non-posted PCI write behavior required by outb(), in that > > >case I would suggest adding the barriers to outX() but > > >perhaps not iowriteX() > > > > Except for serial line and real-time-clock, the machine works OK. So > > there's no need to slow down regural PCI accesses that use iowriteX. > > If actual delays are required rather than strong ordering only, then the > respective drivers need to be fixed, however platform code still has to > enforce the ordering semantics we put into the respective internal > interfaces. > > > > b) or the barriers are in fact needed for /any/ I/O operation > > >on alpha, to ensure that a store is ordered with regard to > > >a following load. If this is the case, we need the barriers > > >in all three families: outX(), writeX() and iowriteX(). > > > > My understanding is that multiple accesses to the same PCI space are > > ordered. > > I believe they are not; there's not such thing as "a PCI space" from the > CPU's point of view, its just another MMIO location, and it's the CPU that > is weakly ordered, not the chipset. > > I'll come back with the relevant citations from the architecture manual, > ISTR I quoted them before already. > > Maciej >
Re: [PATCH] add delay between port write and port read
On Wed, 20 Feb 2019, Arnd Bergmann wrote: > On Wed, Feb 20, 2019 at 4:38 PM Maciej W. Rozycki > wrote: > > On Wed, 20 Feb 2019, Mikulas Patocka wrote: > > > > Well, actually `iowriteX' is generic and for MMIO you have `writeX'. > > > > See lib/iomap.c, the comment at the top and e.g. `iowrite8' there for an > > > > actual proof. Alpha may have an oddball implementation, but there you > > > > go. > > > > Drivers will assume they can do `iowriteX' to any kind of I/O resource, > > > > and ordering must be respected as per Documentation/memory-barriers.txt. > > > > > > So, do you think that the barrier whould be added to iowriteX and slow > > > down every MMIO access? > > > > We need it either for `outX' and `iowriteX' calls operating on port I/O > > resources, or for neither of them, both at a time, to ensure the required > > consistency between the two interfaces. If that badly affects MMIO (and > > is not required there; please remind me what the exact justification to > > use `mb' here is, as it's not entirely clear to me from the commit > > message; `mb' is a barrier and not means for a delay), then we need to > > find a away for `iowriteX' to tell port I/O and MMIO accesses apart and > > only apply the barrier for the former kind. > > Will Deacon is in the process of sanitizing our documentation for this, > and this part is still under discussion. > > I personally don't see a problem with having different barrier > semantics between outb() and iowrite8(), the reasoning being that > any caller of iowriteX() would already have to be careful about > posted writes when iowriteX is backed by ioremapped memory > space rather than port I/O. > > I think the more important question we have to figure out here > however is why exactly the barrier is needed for alpha, as I still > don't fully understand the issue: Port delays were common on MS-DOS with the ISA bus. On the ISA bus, the device cannot reject a transaction, so the programmer has to wait until the device is ready before talking to it. My hypothesis is that the engineers who built this Alpha Avanti machine simply bought some crappy serial line and real-time clock logic from some ISA bus vendor. And so it requires delays. The PCI stuff on the same machine doesn't require any extra delays. > a) maybe the barrier here is only needed to provide the >non-posted PCI write behavior required by outb(), in that >case I would suggest adding the barriers to outX() but >perhaps not iowriteX() Except for serial line and real-time-clock, the machine works OK. So there's no need to slow down regural PCI accesses that use iowriteX. > b) or the barriers are in fact needed for /any/ I/O operation >on alpha, to ensure that a store is ordered with regard to >a following load. If this is the case, we need the barriers >in all three families: outX(), writeX() and iowriteX(). > > Arnd My understanding is that multiple accesses to the same PCI space are ordered. Mikulas
Re: [PATCH] add delay between port write and port read
On Tue, 19 Feb 2019, Maciej W. Rozycki wrote: > On Tue, 19 Feb 2019, Mikulas Patocka wrote: > > > > As I say, shouldn't the barrier be in `iowriteX' instead? I don't > > > suppose these are allowed to be weakly ordered. > > > > > > Maciej > > > > iowriteX is for memory-mapped I/O, out[bwl] is for I/O ports. > > Well, actually `iowriteX' is generic and for MMIO you have `writeX'. > See lib/iomap.c, the comment at the top and e.g. `iowrite8' there for an > actual proof. Alpha may have an oddball implementation, but there you go. > Drivers will assume they can do `iowriteX' to any kind of I/O resource, > and ordering must be respected as per Documentation/memory-barriers.txt. > > Maciej So, do you think that the barrier whould be added to iowriteX and slow down every MMIO access? Mikulas
Re: [PATCH] add delay between port write and port read
On Tue, 19 Feb 2019, Arnd Bergmann wrote:
> On Tue, Feb 19, 2019 at 2:44 PM Mikulas Patocka wrote:
> > On Tue, 19 Feb 2019, Mikulas Patocka wrote:
> >
> > > The patches cd0e00c106722eca40b38ebf11cf134c01901086 and
> > > 92d7223a74235054f2aa7227d207d9c57f84dca0 fix a theoretical issue where the
> > > code didn't follow the specification. Unfortunatelly, they also reduce
> > > timing when port write is followed by a port read.
> > >
> > > These reduced timing cause hang on boot on the Avanti platform when
> > > probing serial ports. This patch adds memory barrier after the outb, outw,
> > > outl functions, so that there is delay between port write and subsequent
> > > port read - just like before.
> > >
> > > Fixes: cd0e00c10672 ("alpha: io: reorder barriers to guarantee writeX()
> > > and iowriteX() ordering")
> > > Cc: sta...@vger.kernel.org# v4.17+
> >
> > you can also add:
> >
> > Tested-by: Mikulas Patocka
>
> Acked-by: Arnd Bergmann
>
> but I notice you are missing Signed-off-by.
I forgot about it. You made that patch, so there should be your signed-off
too.
Signed-off-by: Mikulas Patocka
> We clearly need this patch, but I assumed the alpha maintainers would pick
> it up, not me. I merged the original changes since they were
> cross-architecture,
> but I don't normally take patches for a particular architecture through the
> asm-generic tree (or the soc tree for that matter).
>
> Arnd
>
Re: [PATCH] add delay between port write and port read
On Tue, 19 Feb 2019, Maciej W. Rozycki wrote: > On Tue, 19 Feb 2019, Arnd Bergmann wrote: > > > We clearly need this patch, but I assumed the alpha maintainers would pick > > it up, not me. I merged the original changes since they were > > cross-architecture, > > but I don't normally take patches for a particular architecture through the > > asm-generic tree (or the soc tree for that matter). > > As I say, shouldn't the barrier be in `iowriteX' instead? I don't > suppose these are allowed to be weakly ordered. > > Maciej iowriteX is for memory-mapped I/O, out[bwl] is for I/O ports. If someone finds a problem with memory-mapped device, he can add the barier there. Mikulas
[PATCH] add delay between port write and port read
The patches cd0e00c106722eca40b38ebf11cf134c01901086 and
92d7223a74235054f2aa7227d207d9c57f84dca0 fix a theoretical issue where the
code didn't follow the specification. Unfortunatelly, they also reduce
timing when port write is followed by a port read.
These reduced timing cause hang on boot on the Avanti platform when
probing serial ports. This patch adds memory barrier after the outb, outw,
outl functions, so that there is delay between port write and subsequent
port read - just like before.
Fixes: cd0e00c10672 ("alpha: io: reorder barriers to guarantee writeX() and
iowriteX() ordering")
Cc: sta...@vger.kernel.org # v4.17+
---
arch/alpha/kernel/io.c |6 ++
1 file changed, 6 insertions(+)
Index: linux-stable/arch/alpha/kernel/io.c
===
--- linux-stable.orig/arch/alpha/kernel/io.c2019-02-19 14:06:03.0
+0100
+++ linux-stable/arch/alpha/kernel/io.c 2019-02-19 14:12:29.0 +0100
@@ -78,16 +78,19 @@ u32 inl(unsigned long port)
void outb(u8 b, unsigned long port)
{
iowrite8(b, ioport_map(port, 1));
+ mb();
}
void outw(u16 b, unsigned long port)
{
iowrite16(b, ioport_map(port, 2));
+ mb();
}
void outl(u32 b, unsigned long port)
{
iowrite32(b, ioport_map(port, 4));
+ mb();
}
EXPORT_SYMBOL(inb);
@@ -336,6 +339,7 @@ void iowrite8_rep(void __iomem *port, co
void outsb(unsigned long port, const void *src, unsigned long count)
{
iowrite8_rep(ioport_map(port, 1), src, count);
+ mb();
}
EXPORT_SYMBOL(iowrite8_rep);
@@ -376,6 +380,7 @@ void iowrite16_rep(void __iomem *port, c
void outsw(unsigned long port, const void *src, unsigned long count)
{
iowrite16_rep(ioport_map(port, 2), src, count);
+ mb();
}
EXPORT_SYMBOL(iowrite16_rep);
@@ -408,6 +413,7 @@ void iowrite32_rep(void __iomem *port, c
void outsl(unsigned long port, const void *src, unsigned long count)
{
iowrite32_rep(ioport_map(port, 4), src, count);
+ mb();
}
EXPORT_SYMBOL(iowrite32_rep);
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Tue, 21 Aug 2018, Arnd Bergmann wrote:
> On Mon, Aug 20, 2018 at 11:42 PM Mikulas Patocka wrote:
> > On Mon, 20 Aug 2018, Arnd Bergmann wrote:
> >
> > > On Mon, Aug 20, 2018 at 4:17 PM Mikulas Patocka
> > > wrote:
> > > > On Sun, 19 Aug 2018, ok...@codeaurora.org wrote:
> > > >
> > > > > +my new email
> > > > >
> > > > > On 2018-08-18 19:03, Arnd Bergmann wrote:
> > > > > > On Sat, Aug 18, 2018 at 12:05 AM Maciej W. Rozycki
> > > > > >
> > >
> > > > > I think we need to identify the driver that is failing.
> > > >
> > > > It also may be some timing issue.
> > > >
> > > > I observed that not every kernel with the patch
> > > > 92d7223a74235054f2aa7227d207d9c57f84dca0 fails, some of them get stuck
> > > > only at boot, some get stuck only at shutdown, some not at all. Although
> > > > all the kernels with this patch reverted work.
> > > >
> > > > So the patch may have uncovered some timing problem somewhere.
> > > >
> > > > x86 has the function io_delay that injects delays between I/O accesses
> > > > for
> > > > hardware that needs it - does alpha have something like this?
> > >
> > > The I/O delay would be very low on my list of possible root causes
> > > for this, hardly any hardware at all relies on it, and all uses I see
> > > are related to outb(), which you've already shown not to be the problem
> > > with my test patch.
> > >
> > The lockup happens somewhere in the function autoconfig in
> > drivers/tty/serial/8250/8250_port.c, but I don't know where exactly
> > because serial console doesn't work while the port is being probed.
> >
> > When I use console on a graphics card, the lockup doesn't happen.
>
> Ok, this does strongly suggest that it is the outb() operation that I
> suspected after all, I just sent you a wrong patch to test, failing
> to realize that alpha has two implementations of outb, and that the
> extern one is the one that gets used in a defconfig build.
>
> Could you try again with this patch added in? (Sorry for the whitespace
> damage, you'll have to apply it by hand). Presumably a wmb()
> is sufficient here, but I'm trying to play safe here by restoring the
> barrier that was part of outb() before it broke.
>
>Arnd
Hi
Will you commit this patch?
The avanti platform is still broken in the kernel 5.0 and I tested that
this patch fixes it.
Mikulas
> diff --git a/arch/alpha/kernel/io.c b/arch/alpha/kernel/io.c
> index c025a3e5e357..604237fa821f 100644
> --- a/arch/alpha/kernel/io.c
> +++ b/arch/alpha/kernel/io.c
> @@ -78,16 +78,19 @@ u32 inl(unsigned long port)
> void outb(u8 b, unsigned long port)
> {
> iowrite8(b, ioport_map(port, 1));
> + mb();
> }
>
> void outw(u16 b, unsigned long port)
> {
> iowrite16(b, ioport_map(port, 2));
> + mb();
> }
>
> void outl(u32 b, unsigned long port)
> {
> iowrite32(b, ioport_map(port, 4));
> + mb();
> }
>
> EXPORT_SYMBOL(inb);
> @@ -336,6 +339,7 @@ void iowrite8_rep(void __iomem *port, const void
> *xsrc, unsigned long count)
> void outsb(unsigned long port, const void *src, unsigned long count)
> {
> iowrite8_rep(ioport_map(port, 1), src, count);
> + mb();
> }
>
> EXPORT_SYMBOL(iowrite8_rep);
> @@ -376,6 +380,7 @@ void iowrite16_rep(void __iomem *port, const void
> *src, unsigned long count)
> void outsw(unsigned long port, const void *src, unsigned long count)
> {
> iowrite16_rep(ioport_map(port, 2), src, count);
> + mb();
> }
>
> EXPORT_SYMBOL(iowrite16_rep);
> @@ -408,6 +413,7 @@ void iowrite32_rep(void __iomem *port, const void
> *src, unsigned long count)
> void outsl(unsigned long port, const void *src, unsigned long count)
> {
> iowrite32_rep(ioport_map(port, 4), src, count);
> + mb();
> }
>
> EXPORT_SYMBOL(iowrite32_rep);
>
Re: [PATCH] alpha: add barrier before writing to the hae register
On Wed, 22 Aug 2018, Richard Henderson wrote: > On 08/22/2018 01:42 PM, Mikulas Patocka wrote: > > unsigned long flags = swpipl(IPL_MAX); > > > > - barrier(); > > + mb(); > > Maybe, but I doubt it makes a difference in practice. > > The swpipl is a transition through PALcode, and the > instruction queue will have been flushed into and out > of PALmode. Architecturally, PAL calls aren't memory barriers. It's hard to tell whether they are in practice. The possible problem here is reordering inside the chipset, not inside the CPU (ev45 is in-order). And the chipset doesn't care whether the CPU is executing inside PAL or not. > But of course the barrier shouldn't be required either. > > > r~ Mikulas
[PATCH] alpha: add barrier before writing to the hae register
BTW - this doesn't fix the hang, but the code seems to be wrong here. The
write to the hae register is not ordered with previous writel,
writel_relaxed or readl_relaxed to device memory that is mapped by hae.
Suppose that writel is followed by readl and readl changes the hae
register. The write to the hae register is not ordered with the previous
writel. This patch adds a barrier.
Signed-off-by: Mikulas Patocka
---
arch/alpha/include/asm/io.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux-2.6/arch/alpha/include/asm/io.h
===
--- linux-2.6.orig/arch/alpha/include/asm/io.h 2018-05-31 18:04:35.89600
+0200
+++ linux-2.6/arch/alpha/include/asm/io.h 2018-08-22 22:33:59.13000
+0200
@@ -39,7 +39,7 @@ extern inline void __set_hae(unsigned lo
{
unsigned long flags = swpipl(IPL_MAX);
- barrier();
+ mb();
alpha_mv.hae_cache = new_hae;
*alpha_mv.hae_register = new_hae;
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
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. Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Wed, 22 Aug 2018, Arnd Bergmann wrote: > On Wed, Aug 22, 2018 at 5:50 PM Mikulas Patocka wrote: > > On Wed, 22 Aug 2018, Maciej W. Rozycki wrote: > > > On Wed, 22 Aug 2018, Sinan Kaya wrote: > > > > According to the Alpha handbook, non-overlapping accesses may be > > reordered. > > > > So if someone does > > writel(REG1); > > readl(REG2); > > > > readl may (according to the spec) reach the device before writel. Although > > actual experiments suggests that the read flushes the queued writes. > > > > I would be quite interested why did Linux developers decide that readl > > should be implemented as "read+barrier" and writel should be implemented > > as "barrier+write". Why is there this assymetry in the barriers? > > I can explain this part: those two barriers are used specifically do order > an MMIO access against a DMA access: a writel() may be used to start > a DMA operation copying data from RAM to the device, so we must > have a barrier between the store to that data and the store to the register > to ensure the data is visible to the device. > Similarly, a readl() may check the status of a register that tells us when > a DMA from device to RAM has completed. We must have a read > barrier between that mmio load and the load from RAM to prevent > the data to be prefetched while the MMIO is still in progress. Then - the question is - why not just use barriers before and after accesses to DMA'd memory? For DMA into non-coheren memory, the barrier could be injected into dma_map_* and dma_unmap_* functions (with no change in drivers) - and for DMA into coherent memory you could have something like dma_coherent_barrier(). Why does Linux add the barriers between every read and write to memory mapped registers? > > Does ARM have some hardware magic that prevents reordering the write and > > the read in this case? > > Most architecture have this AFAICT, ARM and x86 definitely do, and > PCI requires this to be true on the bus: > > All MMIO accesses from a given CPU to a given device (according > to an architecture-specific definition of "device") are ordered with respect > to one another. If ARM guarantees that the accesses to a given device are not reordered - then the barriers in readl and writel are superfluous. > If the hardware does not guarantee that, for simple load/store operations > on uncached device memory, then we need a full barrier after each store > in addition to the write barrier needed for the DMA synchronization. > > Arnd Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Wed, 22 Aug 2018, Maciej W. Rozycki wrote: > On Wed, 22 Aug 2018, Sinan Kaya wrote: > > > > It's hard to tell. The Alpha manual says that only overlapping accesses > > > are ordered. > > > > > > 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. > > > > Do you know if the framebuffer BAR you are using is non-prefetchable? (you > > can find out from lspci) > > > > Ordering rule only applies to non-prefetchable BARs only. Architectures are > > allowed to do whatever they want for for prefetchable BARs. > > Well, data accesses have to reach the relevant PCI host bridge first > (i.e. leave the CPU and pass through any intermediate bus bridges between > the CPU and the PCI bus tree accessed) for any PCI data ordering rules to > apply. Depending on the system architecture this may or may not require > OS software intervention. NB this is a general observation, not specific > to Alpha. > > "Alpha Architecture Handbook" has an extensive discussion on data > ordering, concerning both memory and MMIO (termed "memory-like region" and > "non-memory-like region" respectively in the said document), and I'll try > to get through all of it in the coming days to see if I can get to a > conclusion which will let us avoid excessive synchronisation. > > Meanwhile I'll be happy of course to accept any input backed with > suitable references. > > Maciej According to the Alpha handbook, non-overlapping accesses may be reordered. So if someone does writel(REG1); readl(REG2); readl may (according to the spec) reach the device before writel. Although actual experiments suggests that the read flushes the queued writes. I would be quite interested why did Linux developers decide that readl should be implemented as "read+barrier" and writel should be implemented as "barrier+write". Why is there this assymetry in the barriers? Does ARM have some hardware magic that prevents reordering the write and the read in this case? Will Deacon made the change to "memory-barriers.txt" to specify this requirement - could you please describe why did you specify it this way? Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Wed, 22 Aug 2018, Sinan Kaya wrote: > On 8/22/2018 7:59 AM, Mikulas Patocka wrote: > > > > > > On Tue, 21 Aug 2018, Arnd Bergmann wrote: > > > > > On Tue, Aug 21, 2018 at 3:40 PM Mikulas Patocka > > > wrote: > > > > On Tue, 21 Aug 2018, Arnd Bergmann wrote: > > > > > On Mon, Aug 20, 2018 at 11:42 PM Mikulas Patocka > > > > > wrote: > > > > > > On Mon, 20 Aug 2018, Arnd Bergmann wrote: > > > > > > > On Mon, Aug 20, 2018 at 4:17 PM Mikulas Patocka > > > > > > > wrote: > > > > > > > > On Sun, 19 Aug 2018, ok...@codeaurora.org wrote: > > > > > > > > > > Ok, this does strongly suggest that it is the outb() operation that I > > > > > suspected after all, I just sent you a wrong patch to test, failing > > > > > to realize that alpha has two implementations of outb, and that the > > > > > extern one is the one that gets used in a defconfig build. > > > > > > > > > > Could you try again with this patch added in? (Sorry for the > > > > > whitespace > > > > > damage, you'll have to apply it by hand). Presumably a wmb() > > > > > is sufficient here, but I'm trying to play safe here by restoring the > > > > > barrier that was part of outb() before it broke. > > > > > > > > > > Arnd > > > > > > > > This patch fixes both hangs. > > > > > > Ok, thanks for confirming. Now the question is whether this is only needed > > > for I/O space writes to ensure that an outb() etc completes before we > > > start > > > the next instruction as in my first theory above, or if the readl() > > > getting moved > > > ahead of a prior writel() as Maciej suggested could also happen for > > > memory space. > > > > > > My guess would be that even on something as weakly ordered as Alpha, > > > the PCI semantics are kept that a load from a non-prefetchable PCI MMIO > > > space can not get moved ahead of a preceding store to the same PCI > > > device from the same CPU, but I don't really know enough about alpha. > > > > > >Arnd > > > > It's hard to tell. The Alpha manual says that only overlapping accesses > > are ordered. > > > > 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. > > Do you know if the framebuffer BAR you are using is non-prefetchable? (you > can find out from lspci) > > Ordering rule only applies to non-prefetchable BARs only. Architectures are > allowed to do whatever they want for for prefetchable BARs. Alpha doesn't differentiate between prefetchable and non-prefetchable memory. ioremap_wc is the same as ioremap_uc on alpha. Alpha seems to do write queuing on the framebuffer but there's no read queuing. Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Tue, 21 Aug 2018, Arnd Bergmann wrote: > On Tue, Aug 21, 2018 at 3:40 PM Mikulas Patocka wrote: > > On Tue, 21 Aug 2018, Arnd Bergmann wrote: > > > On Mon, Aug 20, 2018 at 11:42 PM Mikulas Patocka > > > wrote: > > > > On Mon, 20 Aug 2018, Arnd Bergmann wrote: > > > > > On Mon, Aug 20, 2018 at 4:17 PM Mikulas Patocka > > > > > wrote: > > > > > > On Sun, 19 Aug 2018, ok...@codeaurora.org wrote: > > > > > > Ok, this does strongly suggest that it is the outb() operation that I > > > suspected after all, I just sent you a wrong patch to test, failing > > > to realize that alpha has two implementations of outb, and that the > > > extern one is the one that gets used in a defconfig build. > > > > > > Could you try again with this patch added in? (Sorry for the whitespace > > > damage, you'll have to apply it by hand). Presumably a wmb() > > > is sufficient here, but I'm trying to play safe here by restoring the > > > barrier that was part of outb() before it broke. > > > > > >Arnd > > > > This patch fixes both hangs. > > Ok, thanks for confirming. Now the question is whether this is only needed > for I/O space writes to ensure that an outb() etc completes before we start > the next instruction as in my first theory above, or if the readl() > getting moved > ahead of a prior writel() as Maciej suggested could also happen for > memory space. > > My guess would be that even on something as weakly ordered as Alpha, > the PCI semantics are kept that a load from a non-prefetchable PCI MMIO > space can not get moved ahead of a preceding store to the same PCI > device from the same CPU, but I don't really know enough about alpha. > > Arnd It's hard to tell. The Alpha manual says that only overlapping accesses are ordered. 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. arm and arm64 is also using barrier after read and before write. Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Tue, 21 Aug 2018, Arnd Bergmann wrote:
> On Mon, Aug 20, 2018 at 11:42 PM Mikulas Patocka wrote:
> > On Mon, 20 Aug 2018, Arnd Bergmann wrote:
> >
> > > On Mon, Aug 20, 2018 at 4:17 PM Mikulas Patocka
> > > wrote:
> > > > On Sun, 19 Aug 2018, ok...@codeaurora.org wrote:
> > > >
> > > > > +my new email
> > > > >
> > > > > On 2018-08-18 19:03, Arnd Bergmann wrote:
> > > > > > On Sat, Aug 18, 2018 at 12:05 AM Maciej W. Rozycki
> > > > > >
> > >
> > > > > I think we need to identify the driver that is failing.
> > > >
> > > > It also may be some timing issue.
> > > >
> > > > I observed that not every kernel with the patch
> > > > 92d7223a74235054f2aa7227d207d9c57f84dca0 fails, some of them get stuck
> > > > only at boot, some get stuck only at shutdown, some not at all. Although
> > > > all the kernels with this patch reverted work.
> > > >
> > > > So the patch may have uncovered some timing problem somewhere.
> > > >
> > > > x86 has the function io_delay that injects delays between I/O accesses
> > > > for
> > > > hardware that needs it - does alpha have something like this?
> > >
> > > The I/O delay would be very low on my list of possible root causes
> > > for this, hardly any hardware at all relies on it, and all uses I see
> > > are related to outb(), which you've already shown not to be the problem
> > > with my test patch.
> > >
> > The lockup happens somewhere in the function autoconfig in
> > drivers/tty/serial/8250/8250_port.c, but I don't know where exactly
> > because serial console doesn't work while the port is being probed.
> >
> > When I use console on a graphics card, the lockup doesn't happen.
>
> Ok, this does strongly suggest that it is the outb() operation that I
> suspected after all, I just sent you a wrong patch to test, failing
> to realize that alpha has two implementations of outb, and that the
> extern one is the one that gets used in a defconfig build.
>
> Could you try again with this patch added in? (Sorry for the whitespace
> damage, you'll have to apply it by hand). Presumably a wmb()
> is sufficient here, but I'm trying to play safe here by restoring the
> barrier that was part of outb() before it broke.
>
>Arnd
This patch fixes both hangs.
Mikulas
> diff --git a/arch/alpha/kernel/io.c b/arch/alpha/kernel/io.c
> index c025a3e5e357..604237fa821f 100644
> --- a/arch/alpha/kernel/io.c
> +++ b/arch/alpha/kernel/io.c
> @@ -78,16 +78,19 @@ u32 inl(unsigned long port)
> void outb(u8 b, unsigned long port)
> {
> iowrite8(b, ioport_map(port, 1));
> + mb();
> }
>
> void outw(u16 b, unsigned long port)
> {
> iowrite16(b, ioport_map(port, 2));
> + mb();
> }
>
> void outl(u32 b, unsigned long port)
> {
> iowrite32(b, ioport_map(port, 4));
> + mb();
> }
>
> EXPORT_SYMBOL(inb);
> @@ -336,6 +339,7 @@ void iowrite8_rep(void __iomem *port, const void
> *xsrc, unsigned long count)
> void outsb(unsigned long port, const void *src, unsigned long count)
> {
> iowrite8_rep(ioport_map(port, 1), src, count);
> + mb();
> }
>
> EXPORT_SYMBOL(iowrite8_rep);
> @@ -376,6 +380,7 @@ void iowrite16_rep(void __iomem *port, const void
> *src, unsigned long count)
> void outsw(unsigned long port, const void *src, unsigned long count)
> {
> iowrite16_rep(ioport_map(port, 2), src, count);
> + mb();
> }
>
> EXPORT_SYMBOL(iowrite16_rep);
> @@ -408,6 +413,7 @@ void iowrite32_rep(void __iomem *port, const void
> *src, unsigned long count)
> void outsl(unsigned long port, const void *src, unsigned long count)
> {
> iowrite32_rep(ioport_map(port, 4), src, count);
> + mb();
> }
>
> EXPORT_SYMBOL(iowrite32_rep);
>
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Sun, 19 Aug 2018, ok...@codeaurora.org wrote:
> +my new email
>
> On 2018-08-18 19:03, Arnd Bergmann wrote:
> > On Sat, Aug 18, 2018 at 12:05 AM Maciej W. Rozycki
> > wrote:
> > >
> > > On Fri, 17 Aug 2018, Arnd Bergmann wrote:
> > >
> > > > - For outb()/outw()/outl(), we ought to provide stronger barriers than
> > > for
> > > > writeb()/writew()/writel(), as PCI drivers should expect the store to
> > > have
> > > > arrived at the device by the time the function returns.
> > > >
> > > > Could you try the patch below that would address the second issue but
> > > not
> > > > the first?
> > > >
> > > > Arnd
> > > >
> > > > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> > > > index 4c533fc94d62..0a479ac27cab 100644
> > > > --- a/arch/alpha/include/asm/io.h
> > > > +++ b/arch/alpha/include/asm/io.h
> > > > @@ -364,11 +364,13 @@ extern inline u16 inw(unsigned long port)
> > > > extern inline void outb(u8 b, unsigned long port)
> > > > {
> > > > iowrite8(b, ioport_map(port, 1));
> > > > + mb();
> > >
> > > But is `mb' the right kind of high-level barrier for MMIO rather than
> > > regular memory access?
> > >
> > > I believe it is not and so I was told in the past when an ordering
> > > barrier was required for MMIO in a network driver for a weakly ordered
> > > system. I think different kind of barrier is required here, such as
> > > `mmiowb', if appropriate. Of course at the low level it may indeed expand
> > > to an MB hardware instruction, but that's another matter, we need to
> > > abstract things properly.
> >
> > What's important here is that for this experiment, adding mb() puts
> > the barrier back that used to be there before the regression in commit
> > 92d7223a74235. Once we know whether that helps, we can debate
> > what the problem is and how it should be addressed correctly.
>
> Strange that adding barrier after write fixes the issue.
>
> There must be something specific to the code following write on this
> architecture .
>
> I think we need to identify the driver that is failing.
It also may be some timing issue.
I observed that not every kernel with the patch
92d7223a74235054f2aa7227d207d9c57f84dca0 fails, some of them get stuck
only at boot, some get stuck only at shutdown, some not at all. Although
all the kernels with this patch reverted work.
So the patch may have uncovered some timing problem somewhere.
x86 has the function io_delay that injects delays between I/O accesses for
hardware that needs it - does alpha have something like this?
Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Fri, 17 Aug 2018, Maciej W. Rozycki wrote:
> On Fri, 17 Aug 2018, Arnd Bergmann wrote:
>
> > - For outb()/outw()/outl(), we ought to provide stronger barriers than for
> > writeb()/writew()/writel(), as PCI drivers should expect the store to have
> > arrived at the device by the time the function returns.
> >
> > Could you try the patch below that would address the second issue but not
> > the first?
> >
> > Arnd
> >
> > diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> > index 4c533fc94d62..0a479ac27cab 100644
> > --- a/arch/alpha/include/asm/io.h
> > +++ b/arch/alpha/include/asm/io.h
> > @@ -364,11 +364,13 @@ extern inline u16 inw(unsigned long port)
> > extern inline void outb(u8 b, unsigned long port)
> > {
> > iowrite8(b, ioport_map(port, 1));
> > + mb();
>
> But is `mb' the right kind of high-level barrier for MMIO rather than
> regular memory access?
>
> I believe it is not and so I was told in the past when an ordering
> barrier was required for MMIO in a network driver for a weakly ordered
> system. I think different kind of barrier is required here, such as
> `mmiowb', if appropriate. Of course at the low level it may indeed expand
> to an MB hardware instruction, but that's another matter, we need to
> abstract things properly.
>
> A while ago I proposed a set of different MMIO barriers, that some
> systems may require, corresponding to the respective regular memory
> barriers, but in the I/O context. I never got to implementing that
> proposal, but I still think it's the right thing to do and will see if I
> can find some time to try doing that. Right now we have quite a mess with
> that. :(
>
> Maciej
The alpha architecture has only two barrier instructions - "mb" that
orders both reads and writes and "wmb" that orders only writes.
Mikulas
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Fri, 17 Aug 2018, Arnd Bergmann wrote:
> On Fri, Aug 17, 2018 at 9:10 PM Mikulas Patocka wrote:
> >
> > Hi
> >
> > The commit 9ce8654323d69273b4977f76f11c9e2d345ab130 breaks the Alpha
> > Avanti platform. There is temporary 40-second hang during boot when
> > detecting serial ports (although the hang eventually resolves and the
> > machine boots) and there's total hang during shutdown when saving the
> > hardware clock.
> >
> > The bug can be fixed with the patch below - apparently, there's something
> > in the Alpha Avanti platform code that expects that the barrier should be
> > after the I/O write instructions.
>
> I have two theories here:
>
> - there might be a driver oddity (can't really call it bug) that
> relies on writel()
> etc to imply a barrier afterwards when that guarantee isn't there elsewhere
>
> - For outb()/outw()/outl(), we ought to provide stronger barriers than for
> writeb()/writew()/writel(), as PCI drivers should expect the store to have
> arrived at the device by the time the function returns.
>
> Could you try the patch below that would address the second issue but not
> the first?
>
> Arnd
I've tried this patch and it doesn't help.
Mikulas
> diff --git a/arch/alpha/include/asm/io.h b/arch/alpha/include/asm/io.h
> index 4c533fc94d62..0a479ac27cab 100644
> --- a/arch/alpha/include/asm/io.h
> +++ b/arch/alpha/include/asm/io.h
> @@ -364,11 +364,13 @@ extern inline u16 inw(unsigned long port)
> extern inline void outb(u8 b, unsigned long port)
> {
> iowrite8(b, ioport_map(port, 1));
> + mb();
> }
>
> extern inline void outw(u16 b, unsigned long port)
> {
> iowrite16(b, ioport_map(port, 2));
> + mb();
> }
> #endif
>
> @@ -394,6 +396,7 @@ extern inline u32 inl(unsigned long port)
> extern inline void outl(u32 b, unsigned long port)
> {
> iowrite32(b, ioport_map(port, 4));
> + mb();
> }
> #endif
>
Re: Alpha Avanti broken by 9ce8654323d69273b4977f76f11c9e2d345ab130
On Sun, 19 Aug 2018, Arnd Bergmann wrote:
> On Sun, Aug 19, 2018 at 5:12 PM wrote:
> > On 2018-08-18 19:03, Arnd Bergmann wrote:
> > > On Sat, Aug 18, 2018 at 12:05 AM Maciej W. Rozycki
> > >
> > >> A while ago I proposed a set of different MMIO barriers, that some
> > >> systems may require, corresponding to the respective regular memory
> > >> barriers, but in the I/O context. I never got to implementing that
> > >> proposal, but I still think it's the right thing to do and will see if
> > >> I
> > >> can find some time to try doing that. Right now we have quite a mess
> > >> with
> > >> that. :(
> > >
> > > My understanding for mmiowb() is that it should not be implied by
> > > writel() itself but that it would be added between a writel() and a
> > > spin_unlock(). Putting it into writel() itself would make it completely
> > > pointless as an abstraction.
> >
> > 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.
>
> Arnd
Alpha already has a memory barrier inside arch_spin_unlock.
BTW. I think that arch_spinlock_t (and atomic_t) on alpha should be
8-byte. See this case - writing the variable "x" performs
read-modify-write cycle on the spinlock, corrupting it.
struct s {
spinlock_t lock;
unsigned char x;
};
Mikulas
Re: [PATCH] block: use 32-bit blk_status_t on Alpha
On Wed, 21 Mar 2018, Jens Axboe wrote: > On 3/21/18 10:42 AM, Mikulas Patocka wrote: > > Early alpha processors cannot write a single byte or word; they read 8 > > bytes, modify the value in registers and write back 8 bytes. > > > > The type blk_status_t is defined as one byte, it is often written > > asynchronously by I/O completion routines, this asynchronous modification > > can corrupt content of nearby bytes if these nearby bytes can be written > > simultaneously by another CPU. > > > > - one example of such corruption is the structure dm_io where > > "blk_status_t status" is written by an asynchronous completion routine > > and "atomic_t io_count" is modified synchronously > > - another example is the structure dm_buffer where "unsigned hold_count" > > is modified synchronously from process context and "blk_status_t > > write_error" is modified asynchronously from bio completion routine > > > > This patch fixes the bug by changing the type blk_status_t to 32 bits if > > we are on Alpha and if we are compiling for a processor that doesn't have > > the byte-word-extension. > > That's nasty. Is alpha the only problematic arch here? Yes. All the other architectures supported by Linux have byte writes. > As to the patch in question, normally I'd just say we should make it > unconditionally u32. But we pack so nicely in the bio, and I don't think > the bio itself has this issue as the rest of the members that share this > word are all set before the bio is submitted. But callers embedding > the status var in other structures don't necessarily have that > guarantee, as your dm examples show. > > -- > Jens Axboe Keeping blk_status_t 8-bit for most architectures will save a few bytes in some of device mapper structures. Mikulas -- 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] block: use 32-bit blk_status_t on Alpha
Early alpha processors cannot write a single byte or word; they read 8 bytes, modify the value in registers and write back 8 bytes. The type blk_status_t is defined as one byte, it is often written asynchronously by I/O completion routines, this asynchronous modification can corrupt content of nearby bytes if these nearby bytes can be written simultaneously by another CPU. - one example of such corruption is the structure dm_io where "blk_status_t status" is written by an asynchronous completion routine and "atomic_t io_count" is modified synchronously - another example is the structure dm_buffer where "unsigned hold_count" is modified synchronously from process context and "blk_status_t write_error" is modified asynchronously from bio completion routine This patch fixes the bug by changing the type blk_status_t to 32 bits if we are on Alpha and if we are compiling for a processor that doesn't have the byte-word-extension. Signed-off-by: Mikulas Patocka <mpato...@redhat.com> Cc: sta...@vger.kernel.org # 4.13+ --- include/linux/blk_types.h |5 + 1 file changed, 5 insertions(+) Index: linux-2.6/include/linux/blk_types.h === --- linux-2.6.orig/include/linux/blk_types.h2018-02-14 20:24:42.038255000 +0100 +++ linux-2.6/include/linux/blk_types.h 2018-03-21 15:04:54.96000 +0100 @@ -20,8 +20,13 @@ typedef void (bio_end_io_t) (struct bio /* * Block error status values. See block/blk-core:blk_errors for the details. + * Alpha cannot write a byte atomically, so we need to use 32-bit value. */ +#if defined(CONFIG_ALPHA) && !defined(__alpha_bwx__) +typedef u32 __bitwise blk_status_t; +#else typedef u8 __bitwise blk_status_t; +#endif #defineBLK_STS_OK 0 #define BLK_STS_NOTSUPP((__force blk_status_t)1) #define BLK_STS_TIMEOUT((__force blk_status_t)2) -- 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: [PULL] alpha.git
On Tue, 23 Jan 2018, Matt Turner wrote: > On Tue, Jan 23, 2018 at 2:23 AM, Mikulas Patocka <mpato...@redhat.com> wrote: > > > > > > On Sat, 20 Jan 2018, Matt Turner wrote: > > > >> Hi Linus, > >> > >> Please pull my alpha git tree. It contains a build fix and a regression > >> fix. > >> > >> Hopefully still in time for 4.15 :) > >> > >> Thanks, > >> Matt > > > > Hi > > > > Will you also submit these patches? The first one fixes a crash when > > pthread_create races with signal delivery, it could cause random crashing > > in applications. > > > > https://marc.info/?l=linux-alpha=151491969711913=2 > > https://marc.info/?l=linux-alpha=151491960011839=2 > > https://marc.info/?l=linux-alpha=151491963911901=2 > > Yes, I've already queued them up for the next merge window. I wasn't > sure if they were appropriate for 4.15 so late in the cycle. If you > think they are, I can send another pull request for 4.15. > > https://git.kernel.org/pub/scm/linux/kernel/git/mattst88/alpha.git/log/?h=alpha-next > > Thanks, > Matt It's OK to send these patches in the 4.16 merge window. They will be backported to the stable kernels anyway. Mikulas -- 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: the patch "alpha/PCI: Replace pci_fixup_irqs()" breaks networking
On Tue, 9 Jan 2018, Lorenzo Pieralisi wrote:
> On Mon, Jan 08, 2018 at 01:35:27PM -0500, Mikulas Patocka wrote:
> >
> >
> > On Mon, 8 Jan 2018, Lorenzo Pieralisi wrote:
> >
> > > On Fri, Jan 05, 2018 at 01:49:58PM -0500, Mikulas Patocka wrote:
> > > >
> > > >
> > > > On Fri, 5 Jan 2018, Lorenzo Pieralisi wrote:
> > > >
> > > > > On Tue, Jan 02, 2018 at 04:32:45PM -0500, Mikulas Patocka wrote:
> > > > > > Hi
> > > > > >
> > > > > > The patch 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b ("alpha/PCI:
> > > > > > Replace
> > > > > > pci_fixup_irqs() call with host bridge IRQ mapping hooks") breaks
> > > > > > networking on Alpha for me. I have an Alpha Avanti server with
> > > > > > tulip
> > > > > > network card.
> > > > > >
> > > > > > The patch 0e4c2eeb breaks it so that I get MCE when the network
> > > > > > card
> > > > > > driver is loaded. The patch 814eae59 fixes the MCE, the system boot
> > > > > > completes, but the network card doesn't receive any interrupts (and
> > > > > > soon
> > > > > > it reports warning about timeout on tx queue). All kernels in the
> > > > > > 4.14
> > > > > > branch have this bug.
> > > > >
> > > > > I reworked the patch, please let me know if it does fix the issue.
> > > > >
> > > > > Thanks,
> > > > > Lorenzo
> > > >
> > > > Networking doesn't work, other interrupts work.
> > > >
> > > > With this patch, /proc/interrupts shows one interrupt for the network
> > > > card
> > > > (it used to be zero without this patch).
> > >
> > > It is getting harder to understand what's going on, here's an
> > > incremental patch with some logging, it would be ideal to add
> > > a similar logging with a working kernel so that we can actually
> > > compare the IRQ level registers programming, please test it
> > > when you have time.
> >
> > This patch works. The patch contains some small changes in the function
> > noname_map_irq compared to your previous patch and these changes make it
> > work. If I revert these changes, networking doesn't work.
>
> Yes - I am slowly understanding how this Alpha platform deals with IRQs,
> thank you for your help and apologies for the drawn-out fix, I just
> can't test on this platform.
>
> Patch below (minus pr_info that I will remove) should be the final one,
> please test it and provide me with logs so that I can check.
>
> Thank you very much.
This patch works - dmesg output is attached.
Mikulas
> -- >8 --
> diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c
> index 37bd6d9b8eb9..754a890c8403 100644
> --- a/arch/alpha/kernel/sys_sio.c
> +++ b/arch/alpha/kernel/sys_sio.c
> @@ -102,6 +102,15 @@ sio_pci_route(void)
> alpha_mv.sys.sio.route_tab);
> }
>
> +static bool sio_pci_dev_irq_needs_level(const struct pci_dev *dev)
> +{
> + if ((dev->class >> 16 == PCI_BASE_CLASS_BRIDGE) &&
> + (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> + return false;
> +
> + return true;
> +}
> +
> static unsigned int __init
> sio_collect_irq_levels(void)
> {
> @@ -110,8 +119,7 @@ sio_collect_irq_levels(void)
>
> /* Iterate through the devices, collecting IRQ levels. */
> for_each_pci_dev(dev) {
> - if ((dev->class >> 16 == PCI_BASE_CLASS_BRIDGE) &&
> - (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> + if (!sio_pci_dev_irq_needs_level(dev))
> continue;
>
> if (dev->irq)
> @@ -120,8 +128,7 @@ sio_collect_irq_levels(void)
> return level_bits;
> }
>
> -static void __init
> -sio_fixup_irq_levels(unsigned int level_bits)
> +static void __sio_fixup_irq_levels(unsigned int level_bits, bool reset)
> {
> unsigned int old_level_bits;
>
> @@ -139,10 +146,21 @@ sio_fixup_irq_levels(unsigned int level_bits)
>*/
> old_level_bits = inb(0x4d0) | (inb(0x4d1) << 8);
>
> - level_bits |= (old_level_bits & 0x71ff);
> + if (reset)
> + old_level_bits &= 0x71ff;
> +
> + level_bits |= old_level_bits;
>
> outb((level_bits >> 0) &
Re: the patch "alpha/PCI: Replace pci_fixup_irqs()" breaks networking
On Mon, 8 Jan 2018, Lorenzo Pieralisi wrote:
> On Fri, Jan 05, 2018 at 01:49:58PM -0500, Mikulas Patocka wrote:
> >
> >
> > On Fri, 5 Jan 2018, Lorenzo Pieralisi wrote:
> >
> > > On Tue, Jan 02, 2018 at 04:32:45PM -0500, Mikulas Patocka wrote:
> > > > Hi
> > > >
> > > > The patch 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b ("alpha/PCI: Replace
> > > > pci_fixup_irqs() call with host bridge IRQ mapping hooks") breaks
> > > > networking on Alpha for me. I have an Alpha Avanti server with tulip
> > > > network card.
> > > >
> > > > The patch 0e4c2eeb breaks it so that I get MCE when the network card
> > > > driver is loaded. The patch 814eae59 fixes the MCE, the system boot
> > > > completes, but the network card doesn't receive any interrupts (and
> > > > soon
> > > > it reports warning about timeout on tx queue). All kernels in the 4.14
> > > > branch have this bug.
> > >
> > > I reworked the patch, please let me know if it does fix the issue.
> > >
> > > Thanks,
> > > Lorenzo
> >
> > Networking doesn't work, other interrupts work.
> >
> > With this patch, /proc/interrupts shows one interrupt for the network card
> > (it used to be zero without this patch).
>
> It is getting harder to understand what's going on, here's an
> incremental patch with some logging, it would be ideal to add
> a similar logging with a working kernel so that we can actually
> compare the IRQ level registers programming, please test it
> when you have time.
This patch works. The patch contains some small changes in the function
noname_map_irq compared to your previous patch and these changes make it
work. If I revert these changes, networking doesn't work.
I'm attaching dmesg of working kernel (4.14.12 with this patch) and
non-working kernel (4.14.12 with your previous patch with 'pr_info("%s:
level bits' added).
Mikulas
> Patch here:
>
> -- >8 --
> diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c
> index 37bd6d9b8eb9..086f53a89dfc 100644
> --- a/arch/alpha/kernel/sys_sio.c
> +++ b/arch/alpha/kernel/sys_sio.c
> @@ -102,6 +102,15 @@ sio_pci_route(void)
> alpha_mv.sys.sio.route_tab);
> }
>
> +static bool sio_pci_dev_irq_needs_level(const struct pci_dev *dev)
> +{
> + if ((dev->class >> 16 == PCI_BASE_CLASS_BRIDGE) &&
> + (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> + return false;
> +
> + return true;
> +}
> +
> static unsigned int __init
> sio_collect_irq_levels(void)
> {
> @@ -110,8 +119,7 @@ sio_collect_irq_levels(void)
>
> /* Iterate through the devices, collecting IRQ levels. */
> for_each_pci_dev(dev) {
> - if ((dev->class >> 16 == PCI_BASE_CLASS_BRIDGE) &&
> - (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> + if (!sio_pci_dev_irq_needs_level(dev))
> continue;
>
> if (dev->irq)
> @@ -120,8 +128,7 @@ sio_collect_irq_levels(void)
> return level_bits;
> }
>
> -static void __init
> -sio_fixup_irq_levels(unsigned int level_bits)
> +static void __sio_fixup_irq_levels(unsigned int level_bits, bool reset)
> {
> unsigned int old_level_bits;
>
> @@ -139,10 +146,21 @@ sio_fixup_irq_levels(unsigned int level_bits)
>*/
> old_level_bits = inb(0x4d0) | (inb(0x4d1) << 8);
>
> - level_bits |= (old_level_bits & 0x71ff);
> + if (reset)
> + old_level_bits &= 0x71ff;
> +
> + level_bits |= old_level_bits;
>
> outb((level_bits >> 0) & 0xff, 0x4d0);
> outb((level_bits >> 8) & 0xff, 0x4d1);
> +
> + pr_info("%s: level bits %ux\n", __func__, level_bits);
> +}
> +
> +static inline void
> +sio_fixup_irq_levels(unsigned int level_bits)
> +{
> + __sio_fixup_irq_levels(level_bits, true);
> }
>
> static inline int
> @@ -181,6 +199,11 @@ noname_map_irq(const struct pci_dev *dev, u8 slot, u8
> pin)
> const long min_idsel = 6, max_idsel = 14, irqs_per_slot = 5;
> int irq = COMMON_TABLE_LOOKUP, tmp;
> tmp = __kernel_extbl(alpha_mv.sys.sio.route_tab, irq);
> +
> + /* Fixup IRQ level if an actual IRQ mapping is detected */
> + if (sio_pci_dev_irq_needs_level(dev) && (irq >= 0 && tmp))
> + __sio_fixup_irq_levels(1 << tmp, false);
> +
> return irq >= 0 ? tmp : -1;
> }
>
>
Re: the patch "alpha/PCI: Replace pci_fixup_irqs()" breaks networking
On Thu, 4 Jan 2018, Lorenzo Pieralisi wrote:
> On Tue, Jan 02, 2018 at 04:32:45PM -0500, Mikulas Patocka wrote:
> > Hi
> >
> > The patch 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b ("alpha/PCI: Replace
> > pci_fixup_irqs() call with host bridge IRQ mapping hooks") breaks
> > networking on Alpha for me. I have an Alpha Avanti server with tulip
> > network card.
> >
> > The patch 0e4c2eeb breaks it so that I get MCE when the network card
> > driver is loaded. The patch 814eae59 fixes the MCE, the system boot
> > completes, but the network card doesn't receive any interrupts (and soon
> > it reports warning about timeout on tx queue). All kernels in the 4.14
> > branch have this bug.
>
> I have reworked the sio code so that now the IRQ level is set upon IRQ
> mapping; patch below - it needs thorough testing since I can't test on
> alpha and I do not have in-depth knowledge of alpha platform code.
>
> Please let me know if it fixes the issues.
>
> Thanks !
The IDE controller doesn't receive any interrupts with this patch.
Mikulas
> -- >8 --
> diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c
> index 37bd6d9b8eb9..e91518b6010d 100644
> --- a/arch/alpha/kernel/sys_sio.c
> +++ b/arch/alpha/kernel/sys_sio.c
> @@ -102,44 +102,48 @@ sio_pci_route(void)
> alpha_mv.sys.sio.route_tab);
> }
>
> -static unsigned int __init
> -sio_collect_irq_levels(void)
> +static bool sio_pci_dev_irq_needs_level(const struct pci_dev *dev)
> {
> - unsigned int level_bits = 0;
> - struct pci_dev *dev = NULL;
> + if ((dev->class >> 16 == PCI_BASE_CLASS_BRIDGE) &&
> + (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> + return false;
>
> - /* Iterate through the devices, collecting IRQ levels. */
> - for_each_pci_dev(dev) {
> - if ((dev->class >> 16 == PCI_BASE_CLASS_BRIDGE) &&
> - (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> - continue;
> + return true;
> +}
>
> - if (dev->irq)
> - level_bits |= (1 << dev->irq);
> - }
> - return level_bits;
> +static void sio_fixup_irq_level(unsigned int irq)
> +{
> + unsigned int level_bits;
> +
> + /*
> + * Now, make PCI interrupt level sensitive.
> + * Notice: these registers must be accessed byte-wise. inw()/outw()
> + * don't work.
> + */
> + level_bits = inb(0x4d0) | (inb(0x4d1) << 8);
> +
> + level_bits |= (1 << irq);
> +
> + outb((level_bits >> 0) & 0xff, 0x4d0);
> + outb((level_bits >> 8) & 0xff, 0x4d1);
> }
>
> -static void __init
> -sio_fixup_irq_levels(unsigned int level_bits)
> +static void __init sio_reset_irq_levels(void)
> {
> - unsigned int old_level_bits;
> + unsigned int level_bits;
>
> /*
> - * Now, make all PCI interrupts level sensitive. Notice:
> - * these registers must be accessed byte-wise. inw()/outw()
> + * Notice: these registers must be accessed byte-wise. inw()/outw()
>* don't work.
>*
> - * Make sure to turn off any level bits set for IRQs 9,10,11,15,
> - * so that the only bits getting set are for devices actually found.
> + * Make sure to turn off any level bits set for IRQs 9,10,11,15.
>* Note that we do preserve the remainder of the bits, which we hope
> - * will be set correctly by ARC/SRM.
> + * will be set correctly by ARC/SRM.
>*
> - * Note: we at least preserve any level-set bits on AlphaBook1
>*/
> - old_level_bits = inb(0x4d0) | (inb(0x4d1) << 8);
> + level_bits = inb(0x4d0) | (inb(0x4d1) << 8);
>
> - level_bits |= (old_level_bits & 0x71ff);
> + level_bits &= 0x71ff;
>
> outb((level_bits >> 0) & 0xff, 0x4d0);
> outb((level_bits >> 8) & 0xff, 0x4d1);
> @@ -181,6 +185,11 @@ noname_map_irq(const struct pci_dev *dev, u8 slot, u8
> pin)
> const long min_idsel = 6, max_idsel = 14, irqs_per_slot = 5;
> int irq = COMMON_TABLE_LOOKUP, tmp;
> tmp = __kernel_extbl(alpha_mv.sys.sio.route_tab, irq);
> +
> + /* Fixup IRQ level if an actual IRQ mapping is detected */
> + if (sio_pci_dev_irq_needs_level(dev) && irq > 0)
> + sio_fixup_irq_level(irq);
> +
> return irq >= 0 ? tmp : -1;
> }
>
> @@ -208,7 +217,7 @@ noname_init_pci(void)
> {
> common_init_pci();
> sio_pci_route();
> - sio_fix
Re: the patch "alpha/PCI: Replace pci_fixup_irqs()" breaks networking
On Tue, 2 Jan 2018, Meelis Roos wrote:
> > The patch 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b ("alpha/PCI: Replace
> > pci_fixup_irqs() call with host bridge IRQ mapping hooks") breaks
> > networking on Alpha for me. I have an Alpha Avanti server with tulip
> > network card.
>
> Matbe the map is wrong for Avanti? Is there some way to check that?
> >
> > The patch 0e4c2eeb breaks it so that I get MCE when the network card
> > driver is loaded. The patch 814eae59 fixes the MCE, the system boot
> > completes, but the network card doesn't receive any interrupts (and soon
> > it reports warning about timeout on tx queue). All kernels in the 4.14
> > branch have this bug.
>
> Does some other IRQ icrease instead?
I doesn't seem to increase any other counter when I send a network packet.
Mikulas
> > Mikulas
> >
> >
> > # cat /proc/interrupts
> >CPU0
> > 1: 3XT-PIC i8042
> > 2: 0XT-PIC cascade
> > 4:752XT-PIC ttyS0
> > 8: 58118 dummy-RTC timer
> > 10: 1613XT-PIC ide0, ide1
> > 11:739XT-PIC sym53c8xx
> > 12: 5XT-PIC i8042
> > 15: 0XT-PIC eth0 <--- note that the counter is zero
> > PMI: 0 Performance Monitoring
> > ERR: 0
> >
> > # lspci -vv
> > 00:06.0 SCSI storage controller: LSI Logic / Symbios Logic 53c810 (rev 01)
> > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+
> > Stepping- SERR- FastB2B- DisINTx-
> > Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
> > SERR- > Latency: 255
> > Interrupt: pin A routed to IRQ 11
> > Region 0: I/O ports at 8000 [size=256]
> > Region 1: Memory at 0130 (32-bit, non-prefetchable) [size=256]
> > Kernel driver in use: sym53c8xx
> > Kernel modules: sym53c8xx
> >
> > 00:07.0 ISA bridge: Intel Corporation 82378ZB/IB, 82379AB (SIO, SIO.A) PCI
> > to ISA Bridge (rev 43)
> > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> > Stepping- SERR- FastB2B- DisINTx-
> > Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
> > SERR- > Latency: 0
> >
> > 00:0b.0 Ethernet controller: Digital Equipment Corporation DECchip 21140
> > [FasterNet] (rev 22)
> > Subsystem: Digital Equipment Corporation DECchip 21140 [FasterNet]
> > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop- ParErr+
> > Stepping- SERR- FastB2B- DisINTx-
> > Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
> > SERR- > Latency: 255 (5000ns min, 1ns max), Cache Line Size: 64 bytes
> > Interrupt: pin A routed to IRQ 15
> > Region 0: I/O ports at 8400 [size=128]
> > Region 1: Memory at 01302000 (32-bit, non-prefetchable) [size=128]
> > Expansion ROM at 0128 [disabled] [size=256K]
> > Kernel driver in use: tulip
> > Kernel modules: tulip
> >
> > 00:0c.0 Display controller: Digital Equipment Corporation DECchip 21030
> > [TGA] (rev 02)
> > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
> > Stepping+ SERR- FastB2B- DisINTx-
> > Status: Cap- 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
> > SERR- > Latency: 255
> > Interrupt: pin A routed to IRQ 5
> > Region 0: Memory at 0200 (32-bit, prefetchable) [size=32M]
> > Expansion ROM at 012c [disabled] [size=256K]
> > Kernel driver in use: tgafb
> >
> > 00:0d.0 RAID bus controller: Silicon Image, Inc. PCI0680 Ultra ATA-133 Host
> > Controller (rev 02)
> > Subsystem: Silicon Image, Inc. Winic W-680 (Silicon Image 680 based)
> > Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr+
> > Stepping- SERR- FastB2B- DisINTx-
> > Status: Cap+ 66MHz- UDF- FastB2B+ ParErr- DEVSEL=medium >TAbort-
> > SERR- > Latency: 240, Cache Line Size: 4 bytes
> > Interrupt: pin A routed to IRQ 10
> > Region 0: I/O ports at 8490 [size=8]
> > Region 1: I/O ports at 84a0 [size=4]
> > Region 2: I/O ports at 8498 [size=8]
> > Region 3: I/O ports at 84a4 [size=4]
> > Region 4: I/O ports at 8480 [size=16]
> > Region 5: Memory at 01301000 (32-bit, non-prefetchable) [size=256]
> > Expansion ROM at 0120 [disabled] [size=512K]
> > Capabilities: [60] Power Management version 2
> > Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA
> > PME(D0-,D1-,D2-,D3hot-,D3cold-)
> > Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=2 PME-
> > Kernel driver in use: SiI_IDE
> >
>
> --
> Meelis Roos (mr...@ut.ee) http://www.cs.ut.ee/~mroos/
>
--
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
Re: the patch "alpha/PCI: Replace pci_fixup_irqs()" breaks networking
On Wed, 3 Jan 2018, Lorenzo Pieralisi wrote:
> On Tue, Jan 02, 2018 at 11:54:47PM +0200, Meelis Roos wrote:
> > > The patch 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b ("alpha/PCI: Replace
> > > pci_fixup_irqs() call with host bridge IRQ mapping hooks") breaks
> > > networking on Alpha for me. I have an Alpha Avanti server with tulip
> > > network card.
> >
> > Matbe the map is wrong for Avanti? Is there some way to check that?
>
> I think the problem is in noname_init_pci(), that, AFAICS, in order to set-up
> the IRQ level, it requires IRQ mapping to be carried out. Joy.
>
> See:
>
> sio_fixup_irq_levels()
>
> and in particular
>
> sio_collect_irq_levels()
>
> Does this horrid hack help (to understand where the problem is - how to
> fix it that's another story) ? Compile tested only.
>
> -- >8 --
> diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c
> index 37bd6d9b8eb9..ca090307ff54 100644
> --- a/arch/alpha/kernel/sys_sio.c
> +++ b/arch/alpha/kernel/sys_sio.c
> @@ -120,7 +120,7 @@ sio_collect_irq_levels(void)
> return level_bits;
> }
>
> -static void __init
> +static void
> sio_fixup_irq_levels(unsigned int level_bits)
> {
> unsigned int old_level_bits;
> @@ -181,6 +181,14 @@ noname_map_irq(const struct pci_dev *dev, u8 slot, u8
> pin)
> const long min_idsel = 6, max_idsel = 14, irqs_per_slot = 5;
> int irq = COMMON_TABLE_LOOKUP, tmp;
> tmp = __kernel_extbl(alpha_mv.sys.sio.route_tab, irq);
> +
> + if ((dev->class >> 16 != PCI_BASE_CLASS_BRIDGE) ||
> + (dev->class >> 8 == PCI_CLASS_BRIDGE_PCMCIA)) {
> +
> + if (irq >= 0)
> + sio_fixup_irq_levels(1 << tmp);
> + }
> +
> return irq >= 0 ? tmp : -1;
> }
>
This patch breaks it even more - so that the IDE controller doesn't
receive any interrupts. I can't tell if the netowrk card receives
interrupts or not, because it doesn't mount the root filesystem.
Mikulas
--
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: the patch "alpha/PCI: Replace pci_fixup_irqs()" breaks networking
On Wed, 3 Jan 2018, Lorenzo Pieralisi wrote:
> On Tue, Jan 02, 2018 at 04:32:45PM -0500, Mikulas Patocka wrote:
> > Hi
> >
> > The patch 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b ("alpha/PCI: Replace
> > pci_fixup_irqs() call with host bridge IRQ mapping hooks") breaks
> > networking on Alpha for me. I have an Alpha Avanti server with tulip
> > network card.
> >
> > The patch 0e4c2eeb breaks it so that I get MCE when the network card
> > driver is loaded. The patch 814eae59 fixes the MCE, the system boot
> > completes, but the network card doesn't receive any interrupts (and soon
> > it reports warning about timeout on tx queue). All kernels in the 4.14
> > branch have this bug.
>
> Can you check if this patch fixes the issue please ?
>
> I suspect we ought to map IRQs early in this platform so that we are
> able to detect how platform code should set them up.
>
> Thanks,
> Lorenzo
>
> -- >8 --
> diff --git a/arch/alpha/kernel/sys_sio.c b/arch/alpha/kernel/sys_sio.c
> index 37bd6d9b8eb9..407ab603e9b1 100644
> --- a/arch/alpha/kernel/sys_sio.c
> +++ b/arch/alpha/kernel/sys_sio.c
> @@ -114,6 +114,8 @@ sio_collect_irq_levels(void)
> (dev->class >> 8 != PCI_CLASS_BRIDGE_PCMCIA))
> continue;
>
> + pci_assign_irq(dev);
> +
> if (dev->irq)
> level_bits |= (1 << dev->irq);
> }
This patch fixes the bug.
Mikulas
--
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: fix crash if pthread_create races with signal delivery
On Wed, 3 Jan 2018, Michael Cree wrote: > On Tue, Jan 02, 2018 at 02:01:34PM -0500, Mikulas Patocka wrote: > > On alpha, a process will crash if it attempts to start a thread and a > > signal is delivered at the same time. The crash can be reproduced with > > this program: https://cygwin.com/ml/cygwin/2014-11/msg00473.html > > > > The reason for the crash is this: > > * we call the clone syscall > > * we go to the function copy_process > > * copy process calls copy_thread_tls, it is a wrapper around copy_thread > > * copy_thread sets the tls pointer: childti->pcb.unique = regs->r20 > > * copy_thread sets regs->r20 to zero > > * we go back to copy_process > > * copy process checks "if (signal_pending(current))" and returns > > -ERESTARTNOINTR > > * the clone syscall is restarted, but this time, regs->r20 is zero, so > > the new thread is created with zero tls pointer > > * the new thread crashes in start_thread when attempting to access tls > > > > The comment in the code says that setting the register r20 is some > > compatibility with OSF/1. But OSF/1 doesn't use the CLONE_SETTLS flag, so > > we don't have to zero r20 if CLONE_SETTLS is set. This patch fixes the bug > > by zeroing regs->r20 only if CLONE_SETTLS is not set. > > This bug was identified some three years ago; it triggers a failure > in the glibc nptl/tst-eintr3 test. See: > > https://marc.info/?l=linux-alpha=140610647213217=2 > > and a fix was proposed by RTH, namely: > > https://marc.info/?l=linux-alpha=140675667715872=2 > > but was never included in the kernel because someone objected to > breaking the ability to run OSF/1 executables. That patch also > deleted the line to set childregs->r20 to 1 which I mark below. > > > > > Signed-off-by: Mikulas Patocka <mpato...@redhat.com> > > Cc: sta...@vger.kernel.org > > > > --- > > arch/alpha/kernel/process.c |3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Index: linux-stable/arch/alpha/kernel/process.c > > === > > --- linux-stable.orig/arch/alpha/kernel/process.c 2017-12-31 > > 17:42:12.0 +0100 > > +++ linux-stable/arch/alpha/kernel/process.c2018-01-02 > > 18:06:24.0 +0100 > > @@ -265,12 +265,13 @@ copy_thread(unsigned long clone_flags, u > >application calling fork. */ > > if (clone_flags & CLONE_SETTLS) > > childti->pcb.unique = regs->r20; > > + else > > + regs->r20 = 0; /* OSF/1 has some strange fork() semantics. */ > > childti->pcb.usp = usp ?: rdusp(); > > *childregs = *regs; > > childregs->r0 = 0; > > childregs->r19 = 0; > > childregs->r20 = 1; /* OSF/1 has some strange fork() semantics. */ > > This line. Is it not also problematic? If a signal is delivered to the parent process, the incomplete child process is deleted and it is recreated when the syscall is restarted. So, setting "childregs->r20 = 1" shouldn't cause any problems. Mikulas > Cheers > Michael. > > > - regs->r20 = 0; > > stack = ((struct switch_stack *) regs) - 1; > > *childstack = *stack; > > childstack->r26 = (unsigned long) ret_from_fork; > > -- > > 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 > -- 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: fix crash if pthread_create races with signal delivery
On alpha, a process will crash if it attempts to start a thread and a signal is delivered at the same time. The crash can be reproduced with this program: https://cygwin.com/ml/cygwin/2014-11/msg00473.html The reason for the crash is this: * we call the clone syscall * we go to the function copy_process * copy process calls copy_thread_tls, it is a wrapper around copy_thread * copy_thread sets the tls pointer: childti->pcb.unique = regs->r20 * copy_thread sets regs->r20 to zero * we go back to copy_process * copy process checks "if (signal_pending(current))" and returns -ERESTARTNOINTR * the clone syscall is restarted, but this time, regs->r20 is zero, so the new thread is created with zero tls pointer * the new thread crashes in start_thread when attempting to access tls The comment in the code says that setting the register r20 is some compatibility with OSF/1. But OSF/1 doesn't use the CLONE_SETTLS flag, so we don't have to zero r20 if CLONE_SETTLS is set. This patch fixes the bug by zeroing regs->r20 only if CLONE_SETTLS is not set. Signed-off-by: Mikulas Patocka <mpato...@redhat.com> Cc: sta...@vger.kernel.org --- arch/alpha/kernel/process.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-stable/arch/alpha/kernel/process.c === --- linux-stable.orig/arch/alpha/kernel/process.c 2017-12-31 17:42:12.0 +0100 +++ linux-stable/arch/alpha/kernel/process.c2018-01-02 18:06:24.0 +0100 @@ -265,12 +265,13 @@ copy_thread(unsigned long clone_flags, u application calling fork. */ if (clone_flags & CLONE_SETTLS) childti->pcb.unique = regs->r20; + else + regs->r20 = 0; /* OSF/1 has some strange fork() semantics. */ childti->pcb.usp = usp ?: rdusp(); *childregs = *regs; childregs->r0 = 0; childregs->r19 = 0; childregs->r20 = 1; /* OSF/1 has some strange fork() semantics. */ - regs->r20 = 0; stack = ((struct switch_stack *) regs) - 1; *childstack = *stack; childstack->r26 = (unsigned long) ret_from_fork; -- 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: fix formating of stack content
Since version 4.9, the kernel automatically breaks printk calls into
multiple newlines unless pr_cont is used. Fix the alpha stacktrace code,
so that it prints stack trace in four columns, as it was initially
intended.
Signed-off-by: Mikulas Patocka <mpato...@redhat.com>
Cc: sta...@vger.kernel.org # v4.9+
---
arch/alpha/kernel/traps.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
Index: linux-stable/arch/alpha/kernel/traps.c
===
--- linux-stable.orig/arch/alpha/kernel/traps.c 2017-12-30 11:37:13.0
+0100
+++ linux-stable/arch/alpha/kernel/traps.c 2017-12-30 11:47:29.0
+0100
@@ -158,11 +158,16 @@ void show_stack(struct task_struct *task
for(i=0; i < kstack_depth_to_print; i++) {
if (((long) stack & (THREAD_SIZE-1)) == 0)
break;
- if (i && ((i % 4) == 0))
- printk("\n ");
- printk("%016lx ", *stack++);
+ if ((i % 4) == 0) {
+ if (i)
+ pr_cont("\n");
+ printk(" ");
+ } else {
+ pr_cont(" ");
+ }
+ pr_cont("%016lx", *stack++);
}
- printk("\n");
+ pr_cont("\n");
dik_show_trace(sp);
}
--
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: fix reboot on Avanti platform
We need to define NEED_SRM_SAVE_RESTORE on the Avanti, otherwise we get machine check exception when attempting to reboot the machine. Signed-off-by: Mikulas Patocka <mpato...@redhat.com> Cc: sta...@vger.kernel.org --- arch/alpha/kernel/pci_impl.h |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-stable/arch/alpha/kernel/pci_impl.h === --- linux-stable.orig/arch/alpha/kernel/pci_impl.h 2017-12-30 11:21:22.0 +0100 +++ linux-stable/arch/alpha/kernel/pci_impl.h 2017-12-30 11:22:10.0 +0100 @@ -143,7 +143,8 @@ struct pci_iommu_arena }; #if defined(CONFIG_ALPHA_SRM) && \ -(defined(CONFIG_ALPHA_CIA) || defined(CONFIG_ALPHA_LCA)) +(defined(CONFIG_ALPHA_CIA) || defined(CONFIG_ALPHA_LCA) || \ + defined(CONFIG_ALPHA_AVANTI)) # define NEED_SRM_SAVE_RESTORE #else # undef NEED_SRM_SAVE_RESTORE -- 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
