Linux-Serial Archive on lore.kernel.org
 help / color / Atom feed
From: "Maciej W. Rozycki" <macro@wdc.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>,
	"Maciej W. Rozycki" <macro@linux-mips.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Richard Henderson <rth@twiddle.net>,
	Matt Turner <mattst88@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	alpha <linux-alpha@vger.kernel.org>,
	linux-serial@vger.kernel.org, linux-rtc@vger.kernel.org
Subject: Re: [PATCH v5] alpha: fix memory barriers so that they conform to the specification
Date: Sun, 24 May 2020 15:54:26 +0100 (BST)
Message-ID: <alpine.LFD.2.21.2005241500230.21168@redsun52.ssa.fujisawa.hgst.com> (raw)
In-Reply-To: <alpine.LRH.2.02.2005231134590.10727@file01.intranet.prod.int.rdu2.redhat.com>

Hi Mikulas,

> This patch makes barriers confiorm to the specification.
> 
> 1. We add mb() before readX_relaxed and writeX_relaxed -
>    memory-barriers.txt claims that these functions must be ordered w.r.t.
>    each other. Alpha doesn't order them, so we need an explicit barrier.
> 2. We add mb() before reads from the I/O space - so that if there's a
>    write followed by a read, there should be a barrier between them.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Fixes: cd0e00c10672 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering")
> Fixes: 92d7223a7423 ("alpha: io: reorder barriers to guarantee writeX() and iowriteX() ordering #2")
> Cc: stable@vger.kernel.org      # v4.17+
> Acked-by: Ivan Kokshaysky <ink@jurassic.park.msu.ru>

 Thank you for your effort to address this regression.  I have looked 
through your code and the context it is to be applied to.  Overall it 
looks good to me, however I still have one concern as detailed below 
(please accept my apologies if you find it tedious to address all the 
points raised in the course of this review).

> Index: linux-stable/arch/alpha/include/asm/io.h
> ===================================================================
> --- linux-stable.orig/arch/alpha/include/asm/io.h	2020-05-23 10:01:22.000000000 +0200
> +++ linux-stable/arch/alpha/include/asm/io.h	2020-05-23 17:29:22.000000000 +0200
[...]
> @@ -487,16 +501,59 @@ extern inline void writeq(u64 b, volatil
>  #define outb_p		outb
>  #define outw_p		outw
>  #define outl_p		outl
> -#define readb_relaxed(addr)	__raw_readb(addr)
> -#define readw_relaxed(addr)	__raw_readw(addr)
> -#define readl_relaxed(addr)	__raw_readl(addr)
> -#define readq_relaxed(addr)	__raw_readq(addr)
> -#define writeb_relaxed(b, addr)	__raw_writeb(b, addr)
> -#define writew_relaxed(b, addr)	__raw_writew(b, addr)
> -#define writel_relaxed(b, addr)	__raw_writel(b, addr)
> -#define writeq_relaxed(b, addr)	__raw_writeq(b, addr)
>  
>  /*
> + * The _relaxed functions must be ordered w.r.t. each other, but they don't
> + * have to be ordered w.r.t. other memory accesses.
> + */
> +static inline u8 readb_relaxed(const volatile void __iomem *addr)
> +{
> +	mb();
> +	return __raw_readb(addr);
> +}
[etc.]

 Please observe that changing the `*_relaxed' entry points from merely 
aliasing the corresponding `__raw_*' handlers to more elaborate code 
sequences (though indeed slightly only, but still) makes the situation 
analogous to one we have with the ordinary MMIO accessor entry points.  
Those regular entry points have been made `extern inline' and wrapped 
into:

#if IO_CONCAT(__IO_PREFIX,trivial_rw_bw) == 1

and:

#if IO_CONCAT(__IO_PREFIX,trivial_rw_lq) == 1

respectively, with corresponding out-of-line entry points available, so 
that there is no extra inline code produced where the call to the relevant 
MMIO accessor is going to end up with an actual function call, as this 
would not help performance in any way and would expand code unnecessarily 
at all call sites.

 Therefore I suggest that your new `static inline' functions follow the 
pattern, perhaps by grouping them with the corresponding ordinary accessor 
functions in arch/alpha/include/asm/io.h within the relevant existing 
#ifdef, and then by making them `extern inline' and providing out-of-line 
implementations in arch/alpha/kernel/io.c, with the individual symbols 
exported.  Within arch/alpha/kernel/io.c the compiler will still inline 
code as it sees fit as it already does, e.g. `__raw_readq' might get 
inlined in `readq' if it turns out cheaper than arranging for an actual 
call, including all the stack frame preparation for `ra' preservation; 
it's less likely with say `writeq' which probably always ends with a tail 
call to `__raw_writeq' as no stack frame is required in that case.

 That for the read accessors.

> +static inline void writeb_relaxed(u8 b, volatile void __iomem *addr)
> +{
> +	mb();
> +	__raw_writeb(b, addr);
> +}
[etc.]

 Conversely for the write accessors, keeping in mind what I have noted 
above, I suggest that you just redirect the existing aliases to the 
ordinary accessors, as there will be no difference anymore between the 
respective ordinary and relaxed accessors.  That is:

#define writeb_relaxed(b, addr)	writeb(b, addr)

etc.

 Let me know if you have any further questions or comments.

  Maciej

  reply index

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-06 11:21 [PATCH 1/2] alpha: add a delay between RTC port write and read Mikulas Patocka
2020-05-06 14:20 ` Arnd Bergmann
2020-05-06 17:12   ` [PATCH 1/2 v2] alpha: add a delay to inb_p, inb_w and inb_l Mikulas Patocka
2020-05-07  8:06     ` [PATCH 1/2 v3] " Mikulas Patocka
2020-05-07  8:20       ` Greg Kroah-Hartman
2020-05-07 10:53         ` Mikulas Patocka
2020-05-07 13:30       ` Arnd Bergmann
2020-05-07 14:09         ` Mikulas Patocka
2020-05-07 15:08           ` Arnd Bergmann
2020-05-07 15:45             ` Mikulas Patocka
2020-05-07 15:46             ` [PATCH v4] alpha: add a barrier after outb, outw and outl Mikulas Patocka
2020-05-07 19:12               ` Arnd Bergmann
2020-05-10  1:27                 ` Maciej W. Rozycki
2020-05-10  1:25             ` [PATCH 1/2 v3] alpha: add a delay to inb_p, inb_w and inb_l Maciej W. Rozycki
2020-05-10 18:50               ` Mikulas Patocka
2020-05-11 14:58                 ` Maciej W. Rozycki
2020-05-12 19:35                   ` Mikulas Patocka
2020-05-13 14:41                   ` Ivan Kokshaysky
2020-05-13 16:13                     ` Greg Kroah-Hartman
2020-05-13 17:17                     ` Maciej W. Rozycki
2020-05-22 13:03                       ` Mikulas Patocka
2020-05-22 13:37                         ` Maciej W. Rozycki
2020-05-22 13:26                     ` Mikulas Patocka
2020-05-22 20:00                       ` Mikulas Patocka
2020-05-23 10:26                         ` [PATCH v4] alpha: fix memory barriers so that they conform to the specification Mikulas Patocka
2020-05-23 15:10                           ` Ivan Kokshaysky
2020-05-23 15:34                             ` Mikulas Patocka
2020-05-23 15:37                               ` [PATCH v5] " Mikulas Patocka
2020-05-24 14:54                                 ` Maciej W. Rozycki [this message]
2020-05-25 13:56                                   ` Mikulas Patocka
2020-05-25 14:07                                     ` Arnd Bergmann
2020-05-25 14:45                                     ` Maciej W. Rozycki
2020-05-25 15:53                                       ` [PATCH v6] " Mikulas Patocka
2020-05-26 14:47                                         ` [PATCH v7] " Mikulas Patocka
2020-05-27  0:18                                           ` Maciej W. Rozycki
2020-06-08  6:58                                             ` Mikulas Patocka
2020-06-08 23:49                                               ` Matt Turner
2020-05-25 15:54                                       ` [PATCH v5] " Mikulas Patocka
2020-05-25 16:39                                         ` Maciej W. Rozycki
2020-05-26 14:48                                           ` Mikulas Patocka
2020-05-27  0:23                                             ` Maciej W. Rozycki
2020-05-23 16:44                               ` [PATCH v4] " Maciej W. Rozycki
2020-05-23 17:09                                 ` Mikulas Patocka
2020-05-23 19:27                                   ` Maciej W. Rozycki
2020-05-23 20:11                                     ` Mikulas Patocka

Reply instructions:

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

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

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

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

  git send-email \
    --in-reply-to=alpine.LFD.2.21.2005241500230.21168@redsun52.ssa.fujisawa.hgst.com \
    --to=macro@wdc.com \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=ink@jurassic.park.msu.ru \
    --cc=linux-alpha@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=macro@linux-mips.org \
    --cc=mattst88@gmail.com \
    --cc=mpatocka@redhat.com \
    --cc=rth@twiddle.net \
    /path/to/YOUR_REPLY

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

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Serial Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-serial/0 linux-serial/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-serial linux-serial/ https://lore.kernel.org/linux-serial \
		linux-serial@vger.kernel.org
	public-inbox-index linux-serial

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-serial


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git