Re: [PATCH v6 1/5] hw/isa/Kconfig: Fix missing dependency ISA_SUPERIO -> FDC

[Thomas Huth]

On 19/05/2021 18.34, Philippe Mathieu-Daudé wrote:
> isa_superio_realize() calls isa_fdc_init_drives(), which is defined
> in hw/block/fdc.c, so ISA_SUPERIO needs to select the FDC symbol.
>
> Add a isa_fdc_init_drives() stub for when FDC is not selected.
>
> Reported-by: John Snow <jsnow@redhat.com>
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Fixes: c0ff3795143 ("Introduce a CONFIG_ISA_SUPERIO switch for isa-superio.c")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/fdc-isa-stubs.c | 22 ++++++++++++++++++++++
>   MAINTAINERS              |  1 +
>   hw/block/meson.build     |  5 ++++-
>   hw/isa/Kconfig           |  1 +
>   4 files changed, 28 insertions(+), 1 deletion(-)
>   create mode 100644 hw/block/fdc-isa-stubs.c
>
> diff --git a/hw/block/fdc-isa-stubs.c b/hw/block/fdc-isa-stubs.c
> new file mode 100644
> index 00000000000..60180531e11
> --- /dev/null
> +++ b/hw/block/fdc-isa-stubs.c
> @@ -0,0 +1,22 @@
> +/*
> + * QEMU Floppy disk emulator (Intel 82078) stubs
> + *
> + * Copyright (c) 2021 Red Hat, Inc.
> + *
> + * Author:
> + *   Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/block/fdc.h"
> +#include "hw/isa/isa.h"
> +
> +void isa_fdc_init_drives(ISADevice *fdc, DriveInfo **fds)
> +{
> +    g_assert_not_reached();
> +}
> diff --git a/MAINTAINERS b/MAINTAINERS
> index eab178aeee5..8fa85e40a52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1659,6 +1659,7 @@ M: John Snow <jsnow@redhat.com>
>   L: qemu-block@nongnu.org
>   S: Supported
>   F: hw/block/fdc.c
> +F: hw/block/fdc-isa-stubs.c
>   F: include/hw/block/fdc.h
>   F: tests/qtest/fdc-test.c
>   T: git https://gitlab.com/jsnow/qemu.git ide
> diff --git a/hw/block/meson.build b/hw/block/meson.build
> index 8b0de54db1f..bb5b331d86a 100644
> --- a/hw/block/meson.build
> +++ b/hw/block/meson.build
> @@ -4,7 +4,8 @@
>     'hd-geometry.c'
>   ))
>   softmmu_ss.add(when: 'CONFIG_ECC', if_true: files('ecc.c'))
> -softmmu_ss.add(when: 'CONFIG_FDC', if_true: files('fdc.c'))
> +softmmu_ss.add(when: 'CONFIG_FDC', if_true: files('fdc.c'),
> +                                       if_false: files('fdc-isa-stubs.c'))
>   softmmu_ss.add(when: 'CONFIG_NAND', if_true: files('nand.c'))
>   softmmu_ss.add(when: 'CONFIG_ONENAND', if_true: files('onenand.c'))
>   softmmu_ss.add(when: 'CONFIG_PFLASH_CFI01', if_true: files('pflash_cfi01.c'))
> @@ -18,3 +19,5 @@
>   specific_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: 
> files('vhost-user-blk.c'))
>   
>   subdir('dataplane')
> +
> +softmmu_ss.add(when: 'CONFIG_ALL', if_true: files('fdc-isa-stubs.c'))
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index 55e0003ce40..7216f66a54a 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -17,6 +17,7 @@ config ISA_SUPERIO
>       bool
>       select ISA_BUS
>       select PCKBD
> +    select FDC

Adding both, the stub and the select FDC here, does not make much sense ... 
I thought that there would be some superio chips where the FDC is always 
disabled, so I expected the "select FDC" to show up at the individual 
superio implementations instead.

However, looking more closely at the code, it seems like there is always the 
possibility to attach a FDC to all of them. So seems like I gave you a bad 
advice, sorry - the first version of your patch (without the stub) makes 
more sense, I think.

 Thomas