Re: [PATCH V5 1/3] ARM64 LPC: Indirect ISA port IO introduced

[Will Deacon]

On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> On 08/11/2016 16:12, Will Deacon wrote:
> >On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> >>+static inline void arm64_set_extops(struct extio_ops *ops)
> >>+{
> >>+	if (ops)
> >>+		WRITE_ONCE(arm64_extio_ops, ops);
> >
> >Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
> >side. Also, what if multiple drivers want to set different ops for distinct
> >address ranges?
> 
> I think that the idea here is that we only have possibly one master in the
> system which offers indirectIO backend, so another one could not possibly
> re-set this value.

Why is that assumption valid, and why does WRITE_ONCE help there? It's not
ONCE as in WARN_ONCE, more ONCE as in exactly-once-per-invocation.

> >>diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> >>new file mode 100644
> >>index 0000000..647b3fa
> >>--- /dev/null
> >>+++ b/arch/arm64/kernel/extio.c
> >>@@ -0,0 +1,27 @@
> >>+/*
> >>+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> >>+ * Author: Zhichang Yuan <yuanzhichang@xxxxxxxxxxxxx>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License version 2 as
> >>+ * published by the Free Software Foundation.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License for more details.
> >>+ *
> >>+ * You should have received a copy of the GNU General Public License
> >>+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> >>+ */
> >>+
> >>+#include <linux/io.h>
> >>+
> >>+struct extio_ops *arm64_extio_ops;
> >>+
> >>+
> >>+BUILD_EXTIO(b, u8)
> >>+
> >>+BUILD_EXTIO(w, u16)
> >>+
> >>+BUILD_EXTIO(l, u32)
> >
> >Is there no way to make this slightly more generic, so that it can be
> >re-used elsewhere? For example, if struct extio_ops was common, then
> >you could have the singleton (which maybe should be an interval tree?),
> >type definition, setter function and the BUILD_EXTIO invocations
> >somewhere generic, rather than squirelled away in the arch backend.
> >
> The concern would be that some architecture which uses generic higher-level
> ISA accessor ops, but have IO space, could be affected.

You're already adding a Kconfig symbol for this stuff, so you can keep
that if you don't want it on other architectures. I'm just arguing that
plumbing drivers directly into arch code via arm64_set_extops is not
something I'm particularly fond of, especially when it looks like it
could be avoided with a small amount of effort.

Will