That would work, but I don't completely like it.
(a) I want s390x to select NEED_PROC_VMCORE_DEVICE_RAM instead. Staring at a
bunch of similar cases (git grep "config NEED" | grep Kconfig, git grep
"config ARCH_WANTS" | grep Kconfig), "select" is the common way to do it.
So unless there is a pretty good reason, I'll keep
NEED_PROC_VMCORE_DEVICE_RAM as is.
That's easy to satify, see below:
Yes, this is mostly what I have right now, except
============simple version=====
fs/proc/Kconfig:
config NEED_PROC_VMCORE_DEVICE_RAM
def n
using "bool" here like other code. (I assume you meant "def_bool n",
"bool" seems to achieve the same thing)
config PROC_VMCORE_DEVICE_RAM
def_bool y
depends on PROC_VMCORE && VIRTIO_MEM
depends on NEED_PROC_VMCORE_DEVICE_RAM
arch/s390/Kconfig:
config S390
select NEED_PROC_VMCORE_DEVICE_RAM
==============================
(b) In the context of this patch, "depends on VIRTIO_MEM" does not make
sense. We could have an intermediate:
config PROC_VMCORE_DEVICE_RAM
def_bool n
depends on PROC_VMCORE
depends on NEED_PROC_VMCORE_DEVICE_RAM
And change that with VIRTIO_MEM support in the relevant patch.
Oh, it's not comment for this patch, I made the simple version based on
the whole patchset. When I had a glance at this patch, I also took
several iterations to get it after I applied the whole patchset and
tried to understand the whole code.
Makes sense, I'm figuring out how I can split that up.
If we can avoid the PROVIDE_* thing for now, great. Not a big fan of
that myself.
I faintly remember that we try avoiding such dependencies and prefer
selecting Kconfigs instead. Just look at the SPLIT_PTE_PTLOCKS mess we still
have to clean up. But as we don't expect that many providers for now, I
don't care.
With the simple version, Kconfig learner as me can easily understand what
they are doing. If it took you a couple of iterations to make them as
you had mentioned earlier, and it took me several iterations to
understand them, I believe there must be room to improve the presented
ones in this patchset. These are only my humble opinion, and I am not
aware of virtio-mem at all, I'll leave this to you and other virtio-mem
dev to decide what should be taken. Thanks for your patience and
provided information, I learned a lot from this discussion.
I hope I didn't express myself poorly: thanks a lot for the review and
the discussion! It helped to make the Kconfig stuff better. I'll get rid
of the PROVIDE_* thing for now and just depend on virtio-mem.
===================
fs/proc/Kconfig:
config PROVIDE_PROC_VMCORE_DEVICE_RAM
def_bool n
config NEED_PROC_VMCORE_DEVICE_RAM
def_bool n
config PROC_VMCORE_DEVICE_RAM
def_bool y
depends on PROC_VMCORE
depends on NEED_PROC_VMCORE_DEVICE_RAM
depends on PROVIDE_PROC_VMCORE_DEVICE_RAM
drivers/virtio/Kconfig:
config VIRTIO_MEM
select PROVIDE_PROC_VMCORE_DEVICE_RAM if PROC_VMCORE
~~~~~~~~~~~~~~
arch/s390/Kconfig:
config S390
select NEED_PROC_VMCORE_DEVICE_RAM if PROC_VMCORE
~~~~~~~~~~~~~~
========================
One last thing I haven't got well, If PROC_VMCORE_DEVICE_RAM has had
dependency on PROC_VMCORE, can we take off the ' if PROC_VMCORE' when
select PROVIDE_PROC_VMCORE_DEVICE_RAM and NEED_PROC_VMCORE_DEVICE_RAM?
We could; it would mean that in a .config file you would end up with
"NEED_PROC_VMCORE_DEVICE_RAM=y" with "#PROC_VMCORE" and no notion of
"PROC_VMCORE_DEVICE_RAM".
I don't particularly like that -- needing something that apparently does
not exist. Not sure if there is a best practice here, staring at some
examples I don't seem to find a consistent rule. I can just drop it, not
the end of the world.
Did you get to look at the other code changes in this patch set? Your
feedback would be highly appreciated!
--
Cheers,
David / dhildenb
