Re: [alsa-devel] Build regressions/improvements in v4.20-rc1 (sound/pci/hda/patch_ca0132.c)

[Takashi Iwai <tiwai@suse.de>]

On Wed, 07 Nov 2018 09:44:25 +0100,
Geert Uytterhoeven wrote:
> 
> Hi Iwai-san,
> 
> On Tue, Nov 6, 2018 at 5:18 PM Takashi Iwai <tiwai@suse.de> wrote:
> > On Tue, 06 Nov 2018 02:04:47 +0100,
> > Randy Dunlap wrote:
> > >
> > > On 11/5/18 2:12 PM, Geert Uytterhoeven wrote:
> > > > On Mon, Nov 5, 2018 at 11:07 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > >> Below is the list of build error/warning regressions/improvements in
> > > >> v4.20-rc1[1] compared to v4.19[2].
> > > >>
> > > >> Summarized:
> > > >>   - build errors: +3/-0
> > > >>   - build warnings: +449/-2712
> > > >>
> > > >> Happy fixing! ;-)
> > > >>
> > > >> Thanks to the linux-next team for providing the build service.
> > > >>
> > > >> [1] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/651022382c7f8da46cb4872a545ee1da6d097d2a/ (all 240 configs)
> > > >> [2] http://kisskb.ellerman.id.au/kisskb/branch/linus/head/84df9525b0c27f3ebc2ebb1864fa62a97fdedb7d/ (all 240 configs)
> > > >>
> > > >>
> > > >> *** ERRORS ***
> > > >>
> > > >>   + /kisskb/src/sound/pci/hda/patch_ca0132.c: error: implicit declaration of function 'pci_iomap' [-Werror=implicit-function-declaration]:  => 8799:3
> > > >
> > > > sh4-all{mod,yes}config
> > > >
> > > > Looks like d9b84a15892c0233 ("ALSA: hda: Fix implicit definition of
> > > > pci_iomap() on SH")
> > > > is not sufficient?
> > >
> > > Different problem.  This is about "select":
> > >
> > > config SND_SOC_ALL_CODECS
> > >       tristate "Build all ASoC CODEC drivers"
> > >
> > > That enables (sets):
> > >       select SND_SOC_HDAC_HDA
> > > which selects SND_HDA even though CONFIG_PCI is not enabled.
> >
> > Actually it is OK to enable CONFIG_SND_HDA_CODEC_CA0132 without
> > CONFIG_PCI.  IIRC, there was a system like that, too.
> > The commit above should have covered the build failure on SH, but
> > apparently isn't enough for some arch setups, as it seems.
> >
> > The cause is clear now: pci_iomap() is defined in
> > asm-generic/pci_iomap.h only when CONFIG_GENERIC_PCI_IOMAP is
> > defined.  Including asm/io.h doesn't help unless CONFIG_PCI is set.
> >
> > Below is a quick fix for this.
> >
> >
> > thanks,
> >
> > Takashi
> >
> > -- 8< --
> >
> > From: Takashi Iwai <tiwai@suse.de>
> > Subject: [PATCH] ALSA: hda/ca0132 - Yet more fix on build breakage without PCI
> >  support
> >
> > The recent change in CA0132 codec driver for supporting more
> > Creative boards includes the pci_iomap() call to get the extra
> > register accesses.  This is supposed to work on all archs and setups,
> > by the implicit assumption that every arch would provide a dummy
> > function returning NULL when no PCI is available.  But the reality
> > bites, of course; as Geert's regular build test shows, some configs
> > (at least SH4 without CONFIG_PCI) leads to a build error due to the
> > implicit function declaration.
> >
> > So this is another attempt to fix the issue: now we add an ifdef
> > CONFIG_PCI line, so that pci_iomap() won't be called unless PCI is
> > really usable.  This should fall back to the standard quirk again with
> > a warning.
> >
> > Fixes: d9b84a15892c0233 ("ALSA: hda: Fix implicit definition of pci_iomap() on SH")
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> Thanks for your patch!
> 
> > --- a/sound/pci/hda/patch_ca0132.c
> > +++ b/sound/pci/hda/patch_ca0132.c
> > @@ -8796,7 +8796,13 @@ static int patch_ca0132(struct hda_codec *codec)
> >         }
> >
> >         if (spec->use_pci_mmio) {
> > +               /*
> > +                * ifdef below needed due to lack of pci_iomap() decleration
> > +                * for some archs when no PCI is defined
> > +                */
> > +#ifdef CONFIG_PCI
> >                 spec->mem_base = pci_iomap(codec->bus->pci, 2, 0xC20);
> > +#endif
> 
> I'm sorry, but that is not a proper fix.
> This should be fixed in the SH-specific code, to behave like other
> architectures.

OK, this is the thing I didn't want to go deeply, as the problem is
messy...

The top of iceberg is that asm/pci_iomap.h defines the dummy function
of pci_iomap() only when CONFIG_PCI=n && CONFIG_GENERIC_PCI_IOMAP=y.
This has been so for long time since the commit 97a29d59fc22
    [PARISC] fix compile break caused by iomap: make IOPORT/PCI
    mapping function s conditional

Before that point, the header defined pci_iomap() and pci_ioumap()
whenever CONFIG_PCi=n.

And, looking at other arch codes, most of them define
CONFIG_GENERIC_PCI_IOMAP no matter whether CONFIG_PCI is set or not.
Actually this is supposedly OK, as the generic code (lib/pci_iomap.c)
contains a large ifdef CONFIG_PCI in it, so essentially it's empty
when CONFIG_PCI=n.

So, one possible fix would be to convert all fallout archs (alpha,
powerpc and sh) to define always CONFIG_GENERIC_PCI_IOMAP.  I'm not
100% sure whether this is safe, but we may try.

Another fix would be to revert the commit 97a29d59fc22, and let
asm/pci_iomap.h providing the dummy functions whenever CONFIG_PCi=n.
Along with it, we need to fix the arch side, the duplicated (and
useless) definitions in arch/parisc/lib/iomap.c.  Through a quick
look, arch/sparc also has the pci_iounmap() definition without
CONFIG_PCI check, so this needs to be fixed as well.

And, what makes the situation messy is that the handling of
pci_iounmap() is totally different from pci_iomap().  pci_iomap() is
covered by CONFIG_GENERIC_PCI_IOMAP while pci_iounmap() is by
CONFIG_GENERIC_IOMAP.  And, yet there are multiple dummy function
definitions of pci_iounmap().  Once in include/asm-generic/iomap.h,
and once in include/asm-generic/io.h.

Overall, there are a few rooms of cleanups / fixes, yeah.


thanks,

Takashi