[linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head:   4b358aabb93a2c654cd1dcab1a25a589f6e2b153
commit: 46abe13b5e3db187e52cd0de06c07bbce010726c [5053/7963] firmware: arm_scmi: Add virtio transport
config: ia64-randconfig-r032-20210815 (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=46abe13b5e3db187e52cd0de06c07bbce010726c
        git remote add linux-next https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
        git fetch --no-tags linux-next master
        git checkout 46abe13b5e3db187e52cd0de06c07bbce010726c
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_chan_available':
>> virtio.c:(.text+0x4b2): undefined reference to `virtio_check_driver_offered_feature'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_probe':
   virtio.c:(.text+0x622): undefined reference to `virtio_check_driver_offered_feature'
>> ia64-linux-ld: virtio.c:(.text+0x792): undefined reference to `virtqueue_get_vring_size'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_feed_vq_rx.isra.0':
>> virtio.c:(.text+0xa12): undefined reference to `virtqueue_add_inbuf'
>> ia64-linux-ld: virtio.c:(.text+0xb22): undefined reference to `virtqueue_kick'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_send_message':
>> virtio.c:(.text+0x1132): undefined reference to `virtqueue_add_sgs'
   ia64-linux-ld: virtio.c:(.text+0x1252): undefined reference to `virtqueue_kick'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_complete_cb':
>> virtio.c:(.text+0x1bc2): undefined reference to `virtqueue_disable_cb'
>> ia64-linux-ld: virtio.c:(.text+0x1bd2): undefined reference to `virtqueue_get_buf'
>> ia64-linux-ld: virtio.c:(.text+0x1e92): undefined reference to `virtqueue_enable_cb'
   ia64-linux-ld: virtio.c:(.text+0x2182): undefined reference to `virtqueue_enable_cb'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_scmi_init':
>> virtio.c:(.init.text+0x22): undefined reference to `register_virtio_driver'
   `virtio_scmi_exit' referenced in section `.rodata' of drivers/firmware/arm_scmi/virtio.o: defined in discarded section `.exit.text' of drivers/firmware/arm_scmi/virtio.o

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 40338 bytes --]

[linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head:   4b358aabb93a2c654cd1dcab1a25a589f6e2b153
commit: 46abe13b5e3db187e52cd0de06c07bbce010726c [5053/7963] firmware: arm_scmi: Add virtio transport
config: ia64-randconfig-r032-20210815 (attached as .config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=46abe13b5e3db187e52cd0de06c07bbce010726c
        git remote add linux-next https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
        git fetch --no-tags linux-next master
        git checkout 46abe13b5e3db187e52cd0de06c07bbce010726c
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_chan_available':
>> virtio.c:(.text+0x4b2): undefined reference to `virtio_check_driver_offered_feature'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_probe':
   virtio.c:(.text+0x622): undefined reference to `virtio_check_driver_offered_feature'
>> ia64-linux-ld: virtio.c:(.text+0x792): undefined reference to `virtqueue_get_vring_size'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_feed_vq_rx.isra.0':
>> virtio.c:(.text+0xa12): undefined reference to `virtqueue_add_inbuf'
>> ia64-linux-ld: virtio.c:(.text+0xb22): undefined reference to `virtqueue_kick'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_send_message':
>> virtio.c:(.text+0x1132): undefined reference to `virtqueue_add_sgs'
   ia64-linux-ld: virtio.c:(.text+0x1252): undefined reference to `virtqueue_kick'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_complete_cb':
>> virtio.c:(.text+0x1bc2): undefined reference to `virtqueue_disable_cb'
>> ia64-linux-ld: virtio.c:(.text+0x1bd2): undefined reference to `virtqueue_get_buf'
>> ia64-linux-ld: virtio.c:(.text+0x1e92): undefined reference to `virtqueue_enable_cb'
   ia64-linux-ld: virtio.c:(.text+0x2182): undefined reference to `virtqueue_enable_cb'
   ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_scmi_init':
>> virtio.c:(.init.text+0x22): undefined reference to `register_virtio_driver'
   `virtio_scmi_exit' referenced in section `.rodata' of drivers/firmware/arm_scmi/virtio.o: defined in discarded section `.exit.text' of drivers/firmware/arm_scmi/virtio.o

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 40338 bytes --]

Re: [linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[Cristian Marussi]

On Sun, Aug 15, 2021 at 12:54:38PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head:   4b358aabb93a2c654cd1dcab1a25a589f6e2b153
> commit: 46abe13b5e3db187e52cd0de06c07bbce010726c [5053/7963] firmware: arm_scmi: Add virtio transport
> config: ia64-randconfig-r032-20210815 (attached as .config)
> compiler: ia64-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=46abe13b5e3db187e52cd0de06c07bbce010726c
>         git remote add linux-next https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>         git fetch --no-tags linux-next master
>         git checkout 46abe13b5e3db187e52cd0de06c07bbce010726c
>         # save the attached .config to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 

Hi,

>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_chan_available':
> >> virtio.c:(.text+0x4b2): undefined reference to `virtio_check_driver_offered_feature'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_probe':
>    virtio.c:(.text+0x622): undefined reference to `virtio_check_driver_offered_feature'
> >> ia64-linux-ld: virtio.c:(.text+0x792): undefined reference to `virtqueue_get_vring_size'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_feed_vq_rx.isra.0':
> >> virtio.c:(.text+0xa12): undefined reference to `virtqueue_add_inbuf'
> >> ia64-linux-ld: virtio.c:(.text+0xb22): undefined reference to `virtqueue_kick'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_send_message':
> >> virtio.c:(.text+0x1132): undefined reference to `virtqueue_add_sgs'
>    ia64-linux-ld: virtio.c:(.text+0x1252): undefined reference to `virtqueue_kick'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_complete_cb':
> >> virtio.c:(.text+0x1bc2): undefined reference to `virtqueue_disable_cb'
> >> ia64-linux-ld: virtio.c:(.text+0x1bd2): undefined reference to `virtqueue_get_buf'
> >> ia64-linux-ld: virtio.c:(.text+0x1e92): undefined reference to `virtqueue_enable_cb'
>    ia64-linux-ld: virtio.c:(.text+0x2182): undefined reference to `virtqueue_enable_cb'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_scmi_init':
> >> virtio.c:(.init.text+0x22): undefined reference to `register_virtio_driver'
>    `virtio_scmi_exit' referenced in section `.rodata' of drivers/firmware/arm_scmi/virtio.o: defined in discarded section `.exit.text' of drivers/firmware/arm_scmi/virtio.o

This is due to the attached random config which sports:

ARM_SCMI_PROTOCOL=y
ARM_SCMI_TRANSPORT_VIRTIO=y

while

VIRTIO=m.

It is really due to how Kconfig has been restructured previously by me
in the scmi-virtio series in:

e8419c24bace firmware: arm_scmi: Make SCMI transports configurable

Since that commit, ARM_SCMI_TRANSPORT_* carry needed per-transport
dependencies (like MAILBOX/VIRTIO), but ARM_SCMI_TRANSPORT_* are "bool" not
"tristate", since they are meant to indicate if you want or not some
specific transports supports to be built into the SCMI stack identified by
ARM_SCMI_PROTOCOL: this latter, a tristate, is the only recognized entity
at built time which includes (as a builtin or as a scmi-module.ko) all
configured transports

This intermediate ARM_SCMI_TRANSPORT_* bool cause the dependency on =m to
be lost in ARM_SCMI_PROTOCOL (so you can have the SCMI stack with virtio
transport as builtin (ARM_SCMI_PROTOCOL=y) while core VIRTIO=m...and
everything breaks)

I'm testing the snippet below, which seems to solve and support all the
cases.

A similar one would be needed for ARM_SCMI_TRANSPORT_MAILBOX to be sure.

ARM_SCMI_TRANSPORT_SMC instead does not need, and should not use, this kind
of fix being based on HAVE_ARM_SMCCC_DISCOVERY.

Not sure if this is the proper way, but it's the only way I've found to
make the above configs possible (or impossible) without drowning in
circular dependencies.

If nobody shout about this I'll post 2 patches along this lines to fix
scmi virtio/mbox transport kconfig dependencies.

Thanks,
Cristian

-----8<

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index 7f4d2435503b..daa349615e91 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -4,6 +4,7 @@ menu "ARM System Control and Management Interface Protocol"
 config ARM_SCMI_PROTOCOL
        tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
        depends on ARM || ARM64 || COMPILE_TEST
+       select VIRTIO if ARM_SCMI_TRANSPORT_VIRTIO
        help
          ARM System Control and Management Interface (SCMI) protocol is a
          set of operating system-independent software interfaces that are
@@ -68,7 +69,6 @@ config ARM_SCMI_TRANSPORT_SMC
 
 config ARM_SCMI_TRANSPORT_VIRTIO
        bool "SCMI transport based on VirtIO"
-       depends on VIRTIO
        select ARM_SCMI_HAVE_TRANSPORT
        select ARM_SCMI_HAVE_MSG
        help

Re: [linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[Cristian Marussi]

[-- Attachment #1: Type: text/plain, Size: 5624 bytes --]

On Sun, Aug 15, 2021 at 12:54:38PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head:   4b358aabb93a2c654cd1dcab1a25a589f6e2b153
> commit: 46abe13b5e3db187e52cd0de06c07bbce010726c [5053/7963] firmware: arm_scmi: Add virtio transport
> config: ia64-randconfig-r032-20210815 (attached as .config)
> compiler: ia64-linux-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=46abe13b5e3db187e52cd0de06c07bbce010726c
>         git remote add linux-next https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>         git fetch --no-tags linux-next master
>         git checkout 46abe13b5e3db187e52cd0de06c07bbce010726c
>         # save the attached .config to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 

Hi,

>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_chan_available':
> >> virtio.c:(.text+0x4b2): undefined reference to `virtio_check_driver_offered_feature'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_probe':
>    virtio.c:(.text+0x622): undefined reference to `virtio_check_driver_offered_feature'
> >> ia64-linux-ld: virtio.c:(.text+0x792): undefined reference to `virtqueue_get_vring_size'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_feed_vq_rx.isra.0':
> >> virtio.c:(.text+0xa12): undefined reference to `virtqueue_add_inbuf'
> >> ia64-linux-ld: virtio.c:(.text+0xb22): undefined reference to `virtqueue_kick'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_send_message':
> >> virtio.c:(.text+0x1132): undefined reference to `virtqueue_add_sgs'
>    ia64-linux-ld: virtio.c:(.text+0x1252): undefined reference to `virtqueue_kick'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `scmi_vio_complete_cb':
> >> virtio.c:(.text+0x1bc2): undefined reference to `virtqueue_disable_cb'
> >> ia64-linux-ld: virtio.c:(.text+0x1bd2): undefined reference to `virtqueue_get_buf'
> >> ia64-linux-ld: virtio.c:(.text+0x1e92): undefined reference to `virtqueue_enable_cb'
>    ia64-linux-ld: virtio.c:(.text+0x2182): undefined reference to `virtqueue_enable_cb'
>    ia64-linux-ld: drivers/firmware/arm_scmi/virtio.o: in function `virtio_scmi_init':
> >> virtio.c:(.init.text+0x22): undefined reference to `register_virtio_driver'
>    `virtio_scmi_exit' referenced in section `.rodata' of drivers/firmware/arm_scmi/virtio.o: defined in discarded section `.exit.text' of drivers/firmware/arm_scmi/virtio.o

This is due to the attached random config which sports:

ARM_SCMI_PROTOCOL=y
ARM_SCMI_TRANSPORT_VIRTIO=y

while

VIRTIO=m.

It is really due to how Kconfig has been restructured previously by me
in the scmi-virtio series in:

e8419c24bace firmware: arm_scmi: Make SCMI transports configurable

Since that commit, ARM_SCMI_TRANSPORT_* carry needed per-transport
dependencies (like MAILBOX/VIRTIO), but ARM_SCMI_TRANSPORT_* are "bool" not
"tristate", since they are meant to indicate if you want or not some
specific transports supports to be built into the SCMI stack identified by
ARM_SCMI_PROTOCOL: this latter, a tristate, is the only recognized entity
at built time which includes (as a builtin or as a scmi-module.ko) all
configured transports

This intermediate ARM_SCMI_TRANSPORT_* bool cause the dependency on =m to
be lost in ARM_SCMI_PROTOCOL (so you can have the SCMI stack with virtio
transport as builtin (ARM_SCMI_PROTOCOL=y) while core VIRTIO=m...and
everything breaks)

I'm testing the snippet below, which seems to solve and support all the
cases.

A similar one would be needed for ARM_SCMI_TRANSPORT_MAILBOX to be sure.

ARM_SCMI_TRANSPORT_SMC instead does not need, and should not use, this kind
of fix being based on HAVE_ARM_SMCCC_DISCOVERY.

Not sure if this is the proper way, but it's the only way I've found to
make the above configs possible (or impossible) without drowning in
circular dependencies.

If nobody shout about this I'll post 2 patches along this lines to fix
scmi virtio/mbox transport kconfig dependencies.

Thanks,
Cristian

-----8<

diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
index 7f4d2435503b..daa349615e91 100644
--- a/drivers/firmware/arm_scmi/Kconfig
+++ b/drivers/firmware/arm_scmi/Kconfig
@@ -4,6 +4,7 @@ menu "ARM System Control and Management Interface Protocol"
 config ARM_SCMI_PROTOCOL
        tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
        depends on ARM || ARM64 || COMPILE_TEST
+       select VIRTIO if ARM_SCMI_TRANSPORT_VIRTIO
        help
          ARM System Control and Management Interface (SCMI) protocol is a
          set of operating system-independent software interfaces that are
@@ -68,7 +69,6 @@ config ARM_SCMI_TRANSPORT_SMC
 
 config ARM_SCMI_TRANSPORT_VIRTIO
        bool "SCMI transport based on VirtIO"
-       depends on VIRTIO
        select ARM_SCMI_HAVE_TRANSPORT
        select ARM_SCMI_HAVE_MSG
        help

Re: [linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[Arnd Bergmann]

On Mon, Aug 16, 2021 at 12:22 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
> On Sun, Aug 15, 2021 at 12:54:38PM +0800, kernel test robot wrote:
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index 7f4d2435503b..daa349615e91 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -4,6 +4,7 @@ menu "ARM System Control and Management Interface Protocol"
>  config ARM_SCMI_PROTOCOL
>         tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
>         depends on ARM || ARM64 || COMPILE_TEST
> +       select VIRTIO if ARM_SCMI_TRANSPORT_VIRTIO
>         help
>           ARM System Control and Management Interface (SCMI) protocol is a
>           set of operating system-independent software interfaces that are
> @@ -68,7 +69,6 @@ config ARM_SCMI_TRANSPORT_SMC
>
>  config ARM_SCMI_TRANSPORT_VIRTIO
>         bool "SCMI transport based on VirtIO"
> -       depends on VIRTIO
>         select ARM_SCMI_HAVE_TRANSPORT
>         select ARM_SCMI_HAVE_MSG
>         help

Generally speaking it's bad to have both 'select' and 'depends on' for
drivers using
the same subsystem. This can lead to both circular dependencies, and to general
confusion when users are surprised by this.

My recommendation is to stick with 'depends on' in general, as that is usually
what you want. In this case, there are two things you could do that work better
to solve this problem:

a) add 'depends on VIRTIO || !VIRTIO' under ARM_SCMI_PROTOCOL, to prevent
    it from being built-in when virtio is a loadable module

b) change the dependency in ARM_SCMI_TRANSPORT_VIRTIO to list the
    correct thing, as in 'depends on VIRTIO=y || VIRTIO=ARM_SCMI_PROTOCOL'.

I don't have a strong preference between those two, as neither of them is
perfect: either you and up not seeing the virtio transport option, or you force
ARM_SCMI_PROTOCOL to be non-built-in when virtio is a module even
when the virtio transport is disabled.

        Arnd

Re: [linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[Arnd Bergmann]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

On Mon, Aug 16, 2021 at 12:22 PM Cristian Marussi
<cristian.marussi@arm.com> wrote:
> On Sun, Aug 15, 2021 at 12:54:38PM +0800, kernel test robot wrote:
>
> diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> index 7f4d2435503b..daa349615e91 100644
> --- a/drivers/firmware/arm_scmi/Kconfig
> +++ b/drivers/firmware/arm_scmi/Kconfig
> @@ -4,6 +4,7 @@ menu "ARM System Control and Management Interface Protocol"
>  config ARM_SCMI_PROTOCOL
>         tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
>         depends on ARM || ARM64 || COMPILE_TEST
> +       select VIRTIO if ARM_SCMI_TRANSPORT_VIRTIO
>         help
>           ARM System Control and Management Interface (SCMI) protocol is a
>           set of operating system-independent software interfaces that are
> @@ -68,7 +69,6 @@ config ARM_SCMI_TRANSPORT_SMC
>
>  config ARM_SCMI_TRANSPORT_VIRTIO
>         bool "SCMI transport based on VirtIO"
> -       depends on VIRTIO
>         select ARM_SCMI_HAVE_TRANSPORT
>         select ARM_SCMI_HAVE_MSG
>         help

Generally speaking it's bad to have both 'select' and 'depends on' for
drivers using
the same subsystem. This can lead to both circular dependencies, and to general
confusion when users are surprised by this.

My recommendation is to stick with 'depends on' in general, as that is usually
what you want. In this case, there are two things you could do that work better
to solve this problem:

a) add 'depends on VIRTIO || !VIRTIO' under ARM_SCMI_PROTOCOL, to prevent
    it from being built-in when virtio is a loadable module

b) change the dependency in ARM_SCMI_TRANSPORT_VIRTIO to list the
    correct thing, as in 'depends on VIRTIO=y || VIRTIO=ARM_SCMI_PROTOCOL'.

I don't have a strong preference between those two, as neither of them is
perfect: either you and up not seeing the virtio transport option, or you force
ARM_SCMI_PROTOCOL to be non-built-in when virtio is a module even
when the virtio transport is disabled.

        Arnd

Re: [linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[Cristian Marussi]

On Mon, Aug 16, 2021 at 02:11:19PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 16, 2021 at 12:22 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> > On Sun, Aug 15, 2021 at 12:54:38PM +0800, kernel test robot wrote:
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index 7f4d2435503b..daa349615e91 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -4,6 +4,7 @@ menu "ARM System Control and Management Interface Protocol"
> >  config ARM_SCMI_PROTOCOL
> >         tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
> >         depends on ARM || ARM64 || COMPILE_TEST
> > +       select VIRTIO if ARM_SCMI_TRANSPORT_VIRTIO
> >         help
> >           ARM System Control and Management Interface (SCMI) protocol is a
> >           set of operating system-independent software interfaces that are
> > @@ -68,7 +69,6 @@ config ARM_SCMI_TRANSPORT_SMC
> >
> >  config ARM_SCMI_TRANSPORT_VIRTIO
> >         bool "SCMI transport based on VirtIO"
> > -       depends on VIRTIO
> >         select ARM_SCMI_HAVE_TRANSPORT
> >         select ARM_SCMI_HAVE_MSG
> >         help
> 

Hi Arnd,

> Generally speaking it's bad to have both 'select' and 'depends on' for
> drivers using
> the same subsystem. This can lead to both circular dependencies, and to general
> confusion when users are surprised by this.
> 

Thanks a lot for the feedback/explanation.

> My recommendation is to stick with 'depends on' in general, as that is usually
> what you want. In this case, there are two things you could do that work better
> to solve this problem:
> 
> a) add 'depends on VIRTIO || !VIRTIO' under ARM_SCMI_PROTOCOL, to prevent
>     it from being built-in when virtio is a loadable module
> 
> b) change the dependency in ARM_SCMI_TRANSPORT_VIRTIO to list the
>     correct thing, as in 'depends on VIRTIO=y || VIRTIO=ARM_SCMI_PROTOCOL'.
> 
> I don't have a strong preference between those two, as neither of them is
> perfect: either you and up not seeing the virtio transport option, or you force
> ARM_SCMI_PROTOCOL to be non-built-in when virtio is a module even
> when the virtio transport is disabled.

I'll go for b) and post a fix.

Thanks,
Cristian

Re: [linux-next:master 5053/7963] virtio.c:undefined reference to `virtio_check_driver_offered_feature'

[Cristian Marussi]

[-- Attachment #1: Type: text/plain, Size: 2332 bytes --]

On Mon, Aug 16, 2021 at 02:11:19PM +0200, Arnd Bergmann wrote:
> On Mon, Aug 16, 2021 at 12:22 PM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> > On Sun, Aug 15, 2021 at 12:54:38PM +0800, kernel test robot wrote:
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index 7f4d2435503b..daa349615e91 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -4,6 +4,7 @@ menu "ARM System Control and Management Interface Protocol"
> >  config ARM_SCMI_PROTOCOL
> >         tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
> >         depends on ARM || ARM64 || COMPILE_TEST
> > +       select VIRTIO if ARM_SCMI_TRANSPORT_VIRTIO
> >         help
> >           ARM System Control and Management Interface (SCMI) protocol is a
> >           set of operating system-independent software interfaces that are
> > @@ -68,7 +69,6 @@ config ARM_SCMI_TRANSPORT_SMC
> >
> >  config ARM_SCMI_TRANSPORT_VIRTIO
> >         bool "SCMI transport based on VirtIO"
> > -       depends on VIRTIO
> >         select ARM_SCMI_HAVE_TRANSPORT
> >         select ARM_SCMI_HAVE_MSG
> >         help
> 

Hi Arnd,

> Generally speaking it's bad to have both 'select' and 'depends on' for
> drivers using
> the same subsystem. This can lead to both circular dependencies, and to general
> confusion when users are surprised by this.
> 

Thanks a lot for the feedback/explanation.

> My recommendation is to stick with 'depends on' in general, as that is usually
> what you want. In this case, there are two things you could do that work better
> to solve this problem:
> 
> a) add 'depends on VIRTIO || !VIRTIO' under ARM_SCMI_PROTOCOL, to prevent
>     it from being built-in when virtio is a loadable module
> 
> b) change the dependency in ARM_SCMI_TRANSPORT_VIRTIO to list the
>     correct thing, as in 'depends on VIRTIO=y || VIRTIO=ARM_SCMI_PROTOCOL'.
> 
> I don't have a strong preference between those two, as neither of them is
> perfect: either you and up not seeing the virtio transport option, or you force
> ARM_SCMI_PROTOCOL to be non-built-in when virtio is a module even
> when the virtio transport is disabled.

I'll go for b) and post a fix.

Thanks,
Cristian