[PATCH v2] um: add dummy ioremap and iounmap functions

[PATCH v2] um: add dummy ioremap and iounmap functions

[Logan Gunthorpe]

The user mode architecture does not provide ioremap or iounmap, and
because of this, the arch won't build when the functions are used in some
core libraries.

I have designs to use these functions in scatterlist.c where they'd
almost certainly never be called on the um architecture but it does need
to compile. Thus, if the function is ever hit it returns NULL.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Stephen Bates <sbates@raithlin.com>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
---

Changes for v2:

* Per feedback from Al Viro, the ioremap function was changed to
  fail if it ever actually gets used.

 arch/um/include/asm/io.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 arch/um/include/asm/io.h

diff --git a/arch/um/include/asm/io.h b/arch/um/include/asm/io.h
new file mode 100644
index 0000000..f734673
--- /dev/null
+++ b/arch/um/include/asm/io.h
@@ -0,0 +1,17 @@
+#ifndef _ASM_UM_IO_H
+#define _ASM_UM_IO_H
+
+#define ioremap ioremap
+static inline void __iomem *ioremap(phys_addr_t offset, size_t size)
+{
+	return NULL;
+}
+
+#define iounmap iounmap
+static inline void iounmap(void __iomem *addr)
+{
+}
+
+#include <asm-generic/io.h>
+
+#endif
--
2.1.4

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Richard Weinberger]

Logan,

Am 25.05.2017 um 17:42 schrieb Logan Gunthorpe:
> The user mode architecture does not provide ioremap or iounmap, and
> because of this, the arch won't build when the functions are used in some
> core libraries.

Which ones are failing?
I thought we killed the problem by making CONFIG_COMPILE_TEST depend on !UML.

> I have designs to use these functions in scatterlist.c where they'd
> almost certainly never be called on the um architecture but it does need
> to compile. Thus, if the function is ever hit it returns NULL.

I was never a fan of that approach because in my opinion drivers should have
proper dependencies, including a dependency on HAS_IOMEM.

But I'll no longer block these attempts if we can get rid of tons of fallout
every kernel release.

Thanks,
//richard

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Logan Gunthorpe]

On 25/05/17 09:48 AM, Richard Weinberger wrote:
> Which ones are failing?
> I thought we killed the problem by making CONFIG_COMPILE_TEST depend on !UML.

None, at the moment. My work is trying to add iomem support to scatter
lists and thus I want to call ioremap in scatterlist.c. That's when
things fail to build. We could put an '#ifdef HAS_IOMEM' around only the
uses, but I thought this approach was cleaner. However, if people would
rather I do that, let me know and I will.

> I was never a fan of that approach because in my opinion drivers should have
> proper dependencies, including a dependency on HAS_IOMEM.

This doesn't really work when we try to use it in a core library which
can't depend on HAS_IOMEM.

Thanks,

Logan

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Geert Uytterhoeven]

Hi Logan,

On Thu, May 25, 2017 at 5:53 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
> On 25/05/17 09:48 AM, Richard Weinberger wrote:
>> Which ones are failing?
>> I thought we killed the problem by making CONFIG_COMPILE_TEST depend on !UML.
>
> None, at the moment. My work is trying to add iomem support to scatter
> lists and thus I want to call ioremap in scatterlist.c. That's when
> things fail to build. We could put an '#ifdef HAS_IOMEM' around only the
> uses, but I thought this approach was cleaner. However, if people would
> rather I do that, let me know and I will.
>
>> I was never a fan of that approach because in my opinion drivers should have
>> proper dependencies, including a dependency on HAS_IOMEM.
>
> This doesn't really work when we try to use it in a core library which
> can't depend on HAS_IOMEM.

Still, those code patch could be protected by #ifdef CONFIG_HAS_IOMEM,
or better, if (IS_ENABLED(CONFIG_HAS_IOMEM)).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Logan Gunthorpe]

Hi,

On 27/05/17 12:08 PM, Geert Uytterhoeven wrote:
> Still, those code patch could be protected by #ifdef CONFIG_HAS_IOMEM,
> or better, if (IS_ENABLED(CONFIG_HAS_IOMEM)).

Well I think it would have to be the former seeing the latter would
still end up trying to compile the missing function. But having ifdefs
inside code is not generally seen as good idea[1].

I'd really like to go forward with either this patch or something like
it. The other two arches that have this problem are fine with merging a
fix and adding ifdefs to work around a single arch doesn't feel right to me.

Thanks,

Logan

[1] http://yarchive.net/comp/linux/ifdefs.html

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Logan Gunthorpe]

Any thoughts on this? My patches for the other architectures are already
in linux-next. um is the only one that remains.

Thanks,

Logan

On 27/05/17 12:15 PM, Logan Gunthorpe wrote:
> Hi,
> 
> On 27/05/17 12:08 PM, Geert Uytterhoeven wrote:
>> Still, those code patch could be protected by #ifdef CONFIG_HAS_IOMEM,
>> or better, if (IS_ENABLED(CONFIG_HAS_IOMEM)).
> 
> Well I think it would have to be the former seeing the latter would
> still end up trying to compile the missing function. But having ifdefs
> inside code is not generally seen as good idea[1].
> 
> I'd really like to go forward with either this patch or something like
> it. The other two arches that have this problem are fine with merging a
> fix and adding ifdefs to work around a single arch doesn't feel right to me.
> 
> Thanks,
> 
> Logan
> 
> [1] http://yarchive.net/comp/linux/ifdefs.html
> 

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Richard Weinberger]

Am 08.06.2017 um 20:53 schrieb Logan Gunthorpe:
> Any thoughts on this? My patches for the other architectures are already
> in linux-next. um is the only one that remains.

IMHO an ifdef in scatterlist code does not hurt.
It is equally ugly than mocking ioremap for UML.

So, I'm puzzled.
Arnd, what do you think?
Shall !HAS_IOMEM archs just mock these functions?

Thanks,
//richard

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Logan Gunthorpe]

Poke.

On 08/06/17 01:17 PM, Richard Weinberger wrote:
> Am 08.06.2017 um 20:53 schrieb Logan Gunthorpe:
> IMHO an ifdef in scatterlist code does not hurt.
> It is equally ugly than mocking ioremap for UML.

I disagree. Having ifdefs scattered around all call sites of a function
is *much* worse than having an extra mock function tucked away in a
header file somewhere.

It's a very common pattern in the kernel for providers of functions that
depend on a configuration option to provide mock functions when the
configuration option is not selected. This prevents needing every caller
of said function to put #ifdefs around the call. For a few examples:

include/linux/blkdev.h:1952
include/linux/dax.h:24
include/linux/pci.h:1329

And, frankly, it's _exactly_ what Linus Torvalds himself was arguing
against in the link I sent up-thread [1].

> So, I'm puzzled.
> Arnd, what do you think?
> Shall !HAS_IOMEM archs just mock these functions?

So, once again, 'um' is now the only architecture that has this problem
since tile and s390 accepted my patches (and without a fuss too). So can
you please consider merging this patch or proposing something that will
also fix the problem?

Thanks,

Logan


[1] http://yarchive.net/comp/linux/ifdefs.html

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Richard Weinberger]

Logan,

Am 21.06.2017 um 23:25 schrieb Logan Gunthorpe:
> Poke.
> 
> On 08/06/17 01:17 PM, Richard Weinberger wrote:
>> Am 08.06.2017 um 20:53 schrieb Logan Gunthorpe:
>> IMHO an ifdef in scatterlist code does not hurt.
>> It is equally ugly than mocking ioremap for UML.
> 
> I disagree. Having ifdefs scattered around all call sites of a function
> is *much* worse than having an extra mock function tucked away in a
> header file somewhere.
> 
> It's a very common pattern in the kernel for providers of functions that
> depend on a configuration option to provide mock functions when the
> configuration option is not selected. This prevents needing every caller
> of said function to put #ifdefs around the call. For a few examples:
> 
> include/linux/blkdev.h:1952
> include/linux/dax.h:24
> include/linux/pci.h:1329
> 
> And, frankly, it's _exactly_ what Linus Torvalds himself was arguing
> against in the link I sent up-thread [1].
> 
>> So, I'm puzzled.
>> Arnd, what do you think?
>> Shall !HAS_IOMEM archs just mock these functions?
> 
> So, once again, 'um' is now the only architecture that has this problem
> since tile and s390 accepted my patches (and without a fuss too). So can
> you please consider merging this patch or proposing something that will
> also fix the problem?

Since Arnd stays silent and um is the only remaining odd-ball, let's merge this
in v4.13.

Thanks,
//richard

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Logan Gunthorpe]

On 21/06/17 03:30 PM, Richard Weinberger wrote:
> Since Arnd stays silent and um is the only remaining odd-ball, let's merge this
> in v4.13.

Great! Thanks!

Logan

Re: [PATCH v2] um: add dummy ioremap and iounmap functions

[Arnd Bergmann]

On Wed, Jun 21, 2017 at 11:31 PM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 21/06/17 03:30 PM, Richard Weinberger wrote:
>> Since Arnd stays silent and um is the only remaining odd-ball, let's merge this
>> in v4.13.
>
> Great! Thanks!

Sorry for missing most of the discussion earlier. The last proposed version
(returning NULL to always fail) seems fine to me, too.

       Arnd