linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] um: add dummy ioremap and iounmap functions
@ 2017-05-25 15:42 Logan Gunthorpe
  2017-05-25 15:48 ` Richard Weinberger
  0 siblings, 1 reply; 11+ messages in thread
From: Logan Gunthorpe @ 2017-05-25 15:42 UTC (permalink / raw)
  To: linux-kernel, user-mode-linux-devel
  Cc: Logan Gunthorpe, Stephen Bates, Jeff Dike, Richard Weinberger, Al Viro

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

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-05-25 15:42 [PATCH v2] um: add dummy ioremap and iounmap functions Logan Gunthorpe
@ 2017-05-25 15:48 ` Richard Weinberger
  2017-05-25 15:53   ` Logan Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2017-05-25 15:48 UTC (permalink / raw)
  To: Logan Gunthorpe, linux-kernel, user-mode-linux-devel
  Cc: Stephen Bates, Jeff Dike, Al Viro, Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-05-25 15:48 ` Richard Weinberger
@ 2017-05-25 15:53   ` Logan Gunthorpe
  2017-05-27 18:08     ` Geert Uytterhoeven
  0 siblings, 1 reply; 11+ messages in thread
From: Logan Gunthorpe @ 2017-05-25 15:53 UTC (permalink / raw)
  To: Richard Weinberger, linux-kernel, user-mode-linux-devel
  Cc: Stephen Bates, Jeff Dike, Al Viro, Arnd Bergmann



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-05-25 15:53   ` Logan Gunthorpe
@ 2017-05-27 18:08     ` Geert Uytterhoeven
  2017-05-27 18:15       ` Logan Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2017-05-27 18:08 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: Richard Weinberger, linux-kernel, uml-devel, Stephen Bates,
	Jeff Dike, Al Viro, Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-05-27 18:08     ` Geert Uytterhoeven
@ 2017-05-27 18:15       ` Logan Gunthorpe
  2017-06-08 18:53         ` Logan Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Logan Gunthorpe @ 2017-05-27 18:15 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Richard Weinberger, linux-kernel, uml-devel, Stephen Bates,
	Jeff Dike, Al Viro, Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-05-27 18:15       ` Logan Gunthorpe
@ 2017-06-08 18:53         ` Logan Gunthorpe
  2017-06-08 19:17           ` Richard Weinberger
  0 siblings, 1 reply; 11+ messages in thread
From: Logan Gunthorpe @ 2017-06-08 18:53 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Richard Weinberger, linux-kernel, uml-devel, Stephen Bates,
	Jeff Dike, Al Viro, Arnd Bergmann

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-06-08 18:53         ` Logan Gunthorpe
@ 2017-06-08 19:17           ` Richard Weinberger
  2017-06-21 21:25             ` Logan Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2017-06-08 19:17 UTC (permalink / raw)
  To: Logan Gunthorpe, Geert Uytterhoeven
  Cc: linux-kernel, uml-devel, Stephen Bates, Jeff Dike, Al Viro,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-06-08 19:17           ` Richard Weinberger
@ 2017-06-21 21:25             ` Logan Gunthorpe
  2017-06-21 21:30               ` Richard Weinberger
  0 siblings, 1 reply; 11+ messages in thread
From: Logan Gunthorpe @ 2017-06-21 21:25 UTC (permalink / raw)
  To: Richard Weinberger, Geert Uytterhoeven
  Cc: linux-kernel, uml-devel, Stephen Bates, Jeff Dike, Al Viro,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-06-21 21:25             ` Logan Gunthorpe
@ 2017-06-21 21:30               ` Richard Weinberger
  2017-06-21 21:31                 ` Logan Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Richard Weinberger @ 2017-06-21 21:30 UTC (permalink / raw)
  To: Logan Gunthorpe, Geert Uytterhoeven
  Cc: linux-kernel, uml-devel, Stephen Bates, Jeff Dike, Al Viro,
	Arnd Bergmann

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-06-21 21:30               ` Richard Weinberger
@ 2017-06-21 21:31                 ` Logan Gunthorpe
  2017-06-21 21:36                   ` Arnd Bergmann
  0 siblings, 1 reply; 11+ messages in thread
From: Logan Gunthorpe @ 2017-06-21 21:31 UTC (permalink / raw)
  To: Richard Weinberger, Geert Uytterhoeven
  Cc: linux-kernel, uml-devel, Stephen Bates, Jeff Dike, Al Viro,
	Arnd Bergmann



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] um: add dummy ioremap and iounmap functions
  2017-06-21 21:31                 ` Logan Gunthorpe
@ 2017-06-21 21:36                   ` Arnd Bergmann
  0 siblings, 0 replies; 11+ messages in thread
From: Arnd Bergmann @ 2017-06-21 21:36 UTC (permalink / raw)
  To: Logan Gunthorpe
  Cc: Richard Weinberger, Geert Uytterhoeven, linux-kernel, uml-devel,
	Stephen Bates, Jeff Dike, Al Viro

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-06-21 21:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-25 15:42 [PATCH v2] um: add dummy ioremap and iounmap functions Logan Gunthorpe
2017-05-25 15:48 ` Richard Weinberger
2017-05-25 15:53   ` Logan Gunthorpe
2017-05-27 18:08     ` Geert Uytterhoeven
2017-05-27 18:15       ` Logan Gunthorpe
2017-06-08 18:53         ` Logan Gunthorpe
2017-06-08 19:17           ` Richard Weinberger
2017-06-21 21:25             ` Logan Gunthorpe
2017-06-21 21:30               ` Richard Weinberger
2017-06-21 21:31                 ` Logan Gunthorpe
2017-06-21 21:36                   ` Arnd Bergmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).