Re: [alsa-devel] [BUGFIX PATCH] ASoC: fsl: fix miscompilation of snd-soc-imx-pcm
On Thu, Nov 22, 2012 at 01:31:06PM +0100, Lothar Waßmann wrote:
obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o snd-soc-imx-pcm-y := imx-pcm.o -snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_FIQ) += imx-pcm-fiq.o -snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_DMA) += imx-pcm-dma.o +ifneq ($(CONFIG_SND_SOC_IMX_PCM_FIQ),)
- snd-soc-imx-pcm-objs += imx-pcm-fiq.o
+endif +ifneq ($(CONFIG_SND_SOC_IMX_PCM_DMA),)
- snd-soc-imx-pcm-objs += imx-pcm-dma.o
+endif
What is the actual bug here? This fix doesn't look obviously right, and if we do need to move to -objs for some reason then the block ought to be in the -objs section of the Makefile.
Hi,
Mark Brown writes:
On Thu, Nov 22, 2012 at 01:31:06PM +0100, Lothar Waßmann wrote:
obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o snd-soc-imx-pcm-y := imx-pcm.o -snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_FIQ) += imx-pcm-fiq.o -snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_DMA) += imx-pcm-dma.o +ifneq ($(CONFIG_SND_SOC_IMX_PCM_FIQ),)
- snd-soc-imx-pcm-objs += imx-pcm-fiq.o
+endif +ifneq ($(CONFIG_SND_SOC_IMX_PCM_DMA),)
- snd-soc-imx-pcm-objs += imx-pcm-dma.o
+endif
What is the actual bug here? This fix doesn't look obviously right, and if we do need to move to -objs for some reason then the block ought to be in the -objs section of the Makefile.
As I mentioned in the part of my email that you didn't quote, if the sound driver is being compiled as module, the files imx-pcm-fiq.c or imx-pcm-dma.c which should be compiled as part of imx-pcm.o will not be touched by the compiler: CONFIG_SND_SOC_IMX_PCM=m CONFIG_SND_SOC_IMX_PCM_DMA=m CONFIG_SND_SOC_IMX_AUDMUX=m CONFIG_SND_SOC_IMX_SGTL5000=m GEN /usr/local/src/arm/projects/imx53/Release/rel-next/.build-snd-bugfix/Makefile HOSTCC scripts/kconfig/conf.o HOSTCC scripts/kconfig/zconf.tab.o HOSTLD scripts/kconfig/conf scripts/kconfig/conf --silentoldconfig Kconfig # # configuration written to .config # GEN /projects/imx53/Release/rel-next/.build-snd-bugfix/Makefile CHK include/generated/uapi/linux/version.h Using /usr/local/src/arm/projects/imx53/Release/rel-next/linux-next as source for kernel CHK include/generated/utsrelease.h make[2]: `include/generated/mach-types.h' is up to date. CALL /usr/local/src/arm/projects/imx53/Release/rel-next/linux-next/scripts/checksyscalls.sh LD sound/core/built-in.o CC [M] sound/core/compress_offload.o LD sound/soc/built-in.o CC [M] sound/soc/soc-core.o CC [M] sound/soc/soc-dapm.o CC [M] sound/soc/soc-jack.o CC [M] sound/core/memalloc.o CC [M] sound/core/pcm.o CC [M] sound/soc/soc-cache.o CC [M] sound/core/pcm_native.o CC [M] sound/core/pcm_lib.o CC [M] sound/soc/soc-utils.o CC [M] sound/core/pcm_timer.o CC [M] sound/soc/soc-pcm.o CC [M] sound/soc/soc-compress.o CC [M] sound/soc/soc-io.o CC [M] sound/soc/soc-dmaengine-pcm.o CC [M] sound/core/pcm_misc.o CC [M] sound/core/pcm_memory.o CC [M] sound/core/timer.o LD sound/soc/codecs/built-in.o CC [M] sound/soc/codecs/sgtl5000.o LD sound/soc/fsl/built-in.o CC [M] sound/soc/fsl/fsl_ssi.o CC [M] sound/soc/fsl/fsl_utils.o CC [M] sound/soc/fsl/imx-audmux.o LD [M] sound/soc/codecs/snd-soc-sgtl5000.o CC [M] sound/soc/fsl/imx-pcm.o CC [M] sound/soc/fsl/imx-sgtl5000.o LD [M] sound/soc/snd-soc-core.o LD [M] sound/core/snd-timer.o LD [M] sound/core/snd-hrtimer.o LD [M] sound/core/snd-pcm.o LD [M] sound/core/snd-page-alloc.o LD [M] sound/core/snd-compress.o LD [M] sound/soc/fsl/snd-soc-fsl-ssi.o LD [M] sound/soc/fsl/snd-soc-fsl-utils.o LD [M] sound/soc/fsl/snd-soc-imx-audmux.o LD [M] sound/soc/fsl/snd-soc-imx-pcm.o LD [M] sound/soc/fsl/snd-soc-imx-sgtl5000.o LD sound/built-in.o
With the following config: CONFIG_SND_SOC_IMX_PCM=y CONFIG_SND_SOC_IMX_PCM_DMA=y CONFIG_SND_SOC_IMX_AUDMUX=y CONFIG_SND_SOC_IMX_SGTL5000=y GEN /usr/local/src/arm/projects/imx53/Release/rel-next/.build-snd-bugfix/Makefile scripts/kconfig/conf --silentoldconfig Kconfig GEN /usr/local/src/arm/projects/imx53/Release/rel-next/.build-snd-bugfix/Makefile CHK include/generated/uapi/linux/version.h Using /usr/local/src/arm/projects/imx53/Release/rel-next/linux-next as source for kernel CHK include/generated/utsrelease.h make[2]: `include/generated/mach-types.h' is up to date. CALL /usr/local/src/arm/projects/imx53/Release/rel-next/linux-next/scripts/checksyscalls.sh CC sound/soc/soc-core.o CC sound/soc/soc-dapm.o CC sound/soc/soc-jack.o CC sound/soc/soc-cache.o CC sound/soc/soc-utils.o CC sound/soc/soc-pcm.o CC sound/soc/soc-compress.o CC sound/soc/soc-io.o CC sound/soc/soc-dmaengine-pcm.o LD sound/soc/atmel/built-in.o LD sound/soc/au1x/built-in.o LD sound/soc/blackfin/built-in.o LD sound/soc/cirrus/built-in.o CC sound/soc/codecs/sgtl5000.o LD sound/soc/davinci/built-in.o LD sound/soc/dwc/built-in.o CC sound/soc/fsl/fsl_ssi.o LD sound/soc/generic/built-in.o LD sound/soc/jz4740/built-in.o LD sound/soc/kirkwood/built-in.o LD sound/soc/mid-x86/built-in.o LD sound/soc/mxs/built-in.o LD sound/soc/nuc900/built-in.o LD sound/soc/omap/built-in.o CC sound/soc/fsl/fsl_utils.o LD sound/soc/pxa/built-in.o LD sound/soc/s6000/built-in.o LD sound/soc/samsung/built-in.o LD sound/soc/sh/built-in.o LD sound/soc/tegra/built-in.o CC sound/soc/fsl/imx-audmux.o LD sound/soc/txx9/built-in.o CC sound/soc/fsl/imx-pcm.o LD sound/soc/ux500/built-in.o CC sound/soc/fsl/imx-pcm-dma.o ^^^^^^^^^^^^^^^^^^^^^^^^^^^ LD sound/soc/codecs/snd-soc-sgtl5000.o LD sound/soc/codecs/built-in.o CC sound/soc/fsl/imx-sgtl5000.o LD sound/soc/snd-soc-core.o LD sound/soc/fsl/snd-soc-fsl-ssi.o LD sound/soc/fsl/snd-soc-fsl-utils.o LD sound/soc/fsl/snd-soc-imx-audmux.o LD sound/soc/fsl/snd-soc-imx-pcm.o LD sound/soc/fsl/snd-soc-imx-sgtl5000.o LD sound/soc/fsl/built-in.o LD sound/soc/built-in.o
Lothar Waßmann
On Fri, Nov 23, 2012 at 07:59:56AM +0100, Lothar Waßmann wrote:
Mark Brown writes:
What is the actual bug here? This fix doesn't look obviously right, and if we do need to move to -objs for some reason then the block ought to be in the -objs section of the Makefile.
As I mentioned in the part of my email that you didn't quote, if the sound driver is being compiled as module, the files imx-pcm-fiq.c or imx-pcm-dma.c which should be compiled as part of imx-pcm.o will not be touched by the compiler:
That's more detailed than your previous mail but you're still only describing the symptom, not the what the actual error is or how it's fixed...
Due to a broken make rule, sound/soc/fsl/imx-pcm-dma.c or sound/soc/fsl/imx-pcm-fiq.c (whatever is selected via Kconfig) will not be compiled into imx-pcm.o when building as module, i.e.: CONFIG_SND_SOC_IMX_PCM=m CONFIG_SND_SOC_IMX_PCM_DMA=m resulting in a non-functional sound driver.
This gives the error messages: | imx-sgtl5000 sound.1: platform imx-pcm-audio not registered | imx-sgtl5000 sound.1: snd_soc_register_card failed (-517) | platform sound.1: Driver imx-sgtl5000 requests probe deferral when loading the driver instead of what's to be expected: | imx-sgtl5000 sound.1: sgtl5000 <-> 63fcc000.ssi mapping ok
Signed-off-by: Lothar Waßmann LW@KARO-electronics.de --- sound/soc/fsl/Makefile | 10 +++++++--- 1 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile index 515ba66..afd3479 100644 --- a/sound/soc/fsl/Makefile +++ b/sound/soc/fsl/Makefile @@ -30,14 +30,18 @@ obj-$(CONFIG_SND_MPC52xx_SOC_EFIKA) += efika-audio-fabric.o # i.MX Platform Support snd-soc-imx-ssi-objs := imx-ssi.o snd-soc-imx-audmux-objs := imx-audmux.o +snd-soc-imx-pcm-objs := imx-pcm.o +ifneq ($(CONFIG_SND_SOC_IMX_PCM_FIQ),) + snd-soc-imx-pcm-objs += imx-pcm-fiq.o +endif +ifneq ($(CONFIG_SND_SOC_IMX_PCM_DMA),) + snd-soc-imx-pcm-objs += imx-pcm-dma.o +endif
obj-$(CONFIG_SND_SOC_IMX_SSI) += snd-soc-imx-ssi.o obj-$(CONFIG_SND_SOC_IMX_AUDMUX) += snd-soc-imx-audmux.o
obj-$(CONFIG_SND_SOC_IMX_PCM) += snd-soc-imx-pcm.o -snd-soc-imx-pcm-y := imx-pcm.o -snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_FIQ) += imx-pcm-fiq.o -snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_DMA) += imx-pcm-dma.o
# i.MX Machine Support snd-soc-eukrea-tlv320-objs := eukrea-tlv320.o
On Fri, Nov 23, 2012 at 10:12:13AM +0100, Lothar Waßmann wrote:
Due to a broken make rule, sound/soc/fsl/imx-pcm-dma.c or sound/soc/fsl/imx-pcm-fiq.c (whatever is selected via Kconfig) will not be compiled into imx-pcm.o when building as module, i.e.:
What I've been trying to do is get you to tell me what the problem is with the Makefile rule, it's really not clear to me what the issue is or why your change is better. I can see the practical results but nothing about how they came to be which makes it very hard to tell if this is a good fix.
This is a common issue with stuff you send, there's lots of analysis missing so it's hard to understand the thought process behind the change.
Hi,
Mark Brown writes:
On Fri, Nov 23, 2012 at 10:12:13AM +0100, Lothar Waßmann wrote:
Due to a broken make rule, sound/soc/fsl/imx-pcm-dma.c or sound/soc/fsl/imx-pcm-fiq.c (whatever is selected via Kconfig) will not be compiled into imx-pcm.o when building as module, i.e.:
What I've been trying to do is get you to tell me what the problem is with the Makefile rule, it's really not clear to me what the issue is or why your change is better. I can see the practical results but nothing about how they came to be which makes it very hard to tell if this is a good fix.
I already included the actual error messages I got when loading the sound module. I also mentioned, that in case of module build some source files aren't being compiled with those rules. What else do you expect?
Lothar Waßmann
On Fri, Nov 23, 2012 at 11:21:42AM +0100, Lothar Waßmann wrote:
I already included the actual error messages I got when loading the sound module. I also mentioned, that in case of module build some source files aren't being compiled with those rules. What else do you expect?
The why part of things - what is the reason there was a problem and how does your change address it? What is it that you believe your patch does to address the problem?
Hi Mark,
thanks your insisting on a better description for the patch, I found a more appropriate solution for the problem:
Compiling the SoC Audio driver for Freescale i.MX as a module (CONFIG_SND_SOC_IMX_PCM=m) results in a non-functional sound driver indicated by the error message: | imx-sgtl5000 sound.1: platform imx-pcm-audio not registered | imx-sgtl5000 sound.1: snd_soc_register_card failed (-517) | platform sound.1: Driver imx-sgtl5000 requests probe deferral instead of the message: | imx-sgtl5000 sound.1: sgtl5000 <-> 63fcc000.ssi mapping ok that is to be expected upon loading the snd-soc-imx-pcm.ko module.
The build log reveals, that the file imx-pcm-dma.o (or imx-pcm-fiq.o depending on the kernel configuration), which should be linked together with imx-pcm.o into snd-imx-pcm.ko, is not being compiled in this case.
The make rules for these files shows that the target object imx-pcm.o is assigned to the variable snd-soc-imx-pcm-y while imx-pcm-{dma,fiq}.o are added to to snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_DMA) and snd-soc-imx-pcm-$(CONFIG_SND_SOC_IMX_PCM_FIQ) which resolve to snd-soc-imx-pcm-m in this case.
According to Documentation/kbuild/modules.txt: |When the module is built from multiple sources, an additional line is |needed listing the files: | | <module_name>-y := <src1>.o <src2>.o ... Thus the type of the config variables CONFIG_SND_SOC_IMX_PCM_DMA and CONFIG_SND_SOC_IMX_PCM_FIQ should be 'bool' instead of 'tristate' to resolve to 'y' when selected.
Signed-off-by: Lothar Waßmann LW@KARO-electronics.de --- sound/soc/fsl/Kconfig | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig index 10fd172..3b98159 100644 --- a/sound/soc/fsl/Kconfig +++ b/sound/soc/fsl/Kconfig @@ -112,12 +112,12 @@ config SND_SOC_IMX_PCM tristate
config SND_SOC_IMX_PCM_FIQ - tristate + bool select FIQ select SND_SOC_IMX_PCM
config SND_SOC_IMX_PCM_DMA - tristate + bool select SND_SOC_DMAENGINE_PCM select SND_SOC_IMX_PCM
On Fri, Nov 23, 2012 at 10:12:13AM +0100, Lothar Waßmann wrote:
Due to a broken make rule, sound/soc/fsl/imx-pcm-dma.c or sound/soc/fsl/imx-pcm-fiq.c (whatever is selected via Kconfig) will not be compiled into imx-pcm.o when building as module, i.e.: CONFIG_SND_SOC_IMX_PCM=m CONFIG_SND_SOC_IMX_PCM_DMA=m resulting in a non-functional sound driver.
Applied, thanks.
On Fri, Nov 23, 2012 at 10:12:13AM +0100, Lothar Waßmann wrote:
Due to a broken make rule, sound/soc/fsl/imx-pcm-dma.c or sound/soc/fsl/imx-pcm-fiq.c (whatever is selected via Kconfig) will not be compiled into imx-pcm.o when building as module, i.e.: CONFIG_SND_SOC_IMX_PCM=m CONFIG_SND_SOC_IMX_PCM_DMA=m resulting in a non-functional sound driver.
Sorry, replied to the wrong version here - I did apply v3.
participants (2)
-
Lothar Waßmann -
Mark Brown