[PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[YueHaibing]

when do randconfig like this:
CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
CONFIG_SND_SOC_SOF_IMX8=y
CONFIG_SND_SOC_SOF_OF=y
CONFIG_IMX_DSP=m
CONFIG_IMX_SCU=y

there is a link error:

sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'

Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this

Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
 sound/soc/sof/imx/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
index bae4f7b..81274906 100644
--- a/sound/soc/sof/imx/Kconfig
+++ b/sound/soc/sof/imx/Kconfig
@@ -14,7 +14,7 @@ if SND_SOC_SOF_IMX_TOPLEVEL
 config SND_SOC_SOF_IMX8_SUPPORT
 	bool "SOF support for i.MX8"
 	depends on IMX_SCU
-	depends on IMX_DSP
+	select IMX_DSP
 	help
 	  This adds support for Sound Open Firmware for NXP i.MX8 platforms
 	  Say Y if you have such a device.
-- 
2.7.4



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Pierre-Louis Bossart]

On 2/10/20 12:15 AM, YueHaibing wrote:
> when do randconfig like this:
> CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
> CONFIG_SND_SOC_SOF_IMX8=y
> CONFIG_SND_SOC_SOF_OF=y
> CONFIG_IMX_DSP=m
> CONFIG_IMX_SCU=y
> 
> there is a link error:
> 
> sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
> imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'
> 
> Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this
> 
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>

Thanks for the report.

Would you mind sharing the .config and instructions to reproduce this 
case? we have an unrelated issue with allyesconfig that was reviewed here:

https://github.com/thesofproject/linux/pull/1778

and I'd probably a good thing to fix everything in one shot.

Thanks!

> ---
>   sound/soc/sof/imx/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
> index bae4f7b..81274906 100644
> --- a/sound/soc/sof/imx/Kconfig
> +++ b/sound/soc/sof/imx/Kconfig
> @@ -14,7 +14,7 @@ if SND_SOC_SOF_IMX_TOPLEVEL
>   config SND_SOC_SOF_IMX8_SUPPORT
>   	bool "SOF support for i.MX8"
>   	depends on IMX_SCU
> -	depends on IMX_DSP
> +	select IMX_DSP
>   	help
>   	  This adds support for Sound Open Firmware for NXP i.MX8 platforms
>   	  Say Y if you have such a device.
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Daniel Baluta]

On Tue, Feb 11, 2020 at 3:59 AM Yuehaibing <yuehaibing@huawei.com> wrote:
>
> On 2020/2/11 5:00, Pierre-Louis Bossart wrote:
> >
> >
> > On 2/10/20 12:15 AM, YueHaibing wrote:
> >> when do randconfig like this:
> >> CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
> >> CONFIG_SND_SOC_SOF_IMX8=y
> >> CONFIG_SND_SOC_SOF_OF=y
> >> CONFIG_IMX_DSP=m
> >> CONFIG_IMX_SCU=y
> >>
> >> there is a link error:
> >>
> >> sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
> >> imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'
> >>
> >> Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this
> >>
> >> Reported-by: Hulk Robot <hulkci@huawei.com>
> >> Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> >
> > Thanks for the report.
> >
> > Would you mind sharing the .config and instructions to reproduce this case? we have an unrelated issue with allyesconfig that was reviewed here:
> >
> > https://github.com/thesofproject/linux/pull/1778
> >
> > and I'd probably a good thing to fix everything in one shot.
>
> config is attached, which is on x86_64

Thanks, I think this is legit. It was introduced with:

commit f52cdcce9197fef9d4a68792dd3b840ad2b77117
Author: Daniel Baluta <daniel.baluta@nxp.com>
Date:   Sat Jan 4 15:39:53 2020 +0000

    firmware: imx: Allow IMX DSP to be selected as module

    IMX DSP is only needed by SOF or any other module that
    wants to communicate with the DSP. When SOF is build
    as a module IMX DSP is forced to be built inside the
    kernel image. This is not optimal, so allow IMX DSP
    to be built as a module.

    Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
    Signed-off-by: Shawn Guo <shawnguo@kernel.org>

So, I think we should change the Fixes tag. Are there
any clear rules on when to use select vs depends?

On my side, I know what both are doing but it is not clear
when to use them.

> >
> > Thanks!
> >
> >> ---
> >>   sound/soc/sof/imx/Kconfig | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
> >> index bae4f7b..81274906 100644
> >> --- a/sound/soc/sof/imx/Kconfig
> >> +++ b/sound/soc/sof/imx/Kconfig
> >> @@ -14,7 +14,7 @@ if SND_SOC_SOF_IMX_TOPLEVEL
> >>   config SND_SOC_SOF_IMX8_SUPPORT
> >>       bool "SOF support for i.MX8"
> >>       depends on IMX_SCU
> >> -    depends on IMX_DSP
> >> +    select IMX_DSP
> >>       help
> >>         This adds support for Sound Open Firmware for NXP i.MX8 platforms
> >>         Say Y if you have such a device.
> >>
> >
> > .
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Krzysztof Kozlowski]

On Tue, 11 Feb 2020 at 10:46, Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> On Tue, Feb 11, 2020 at 3:59 AM Yuehaibing <yuehaibing@huawei.com> wrote:
> >
> > On 2020/2/11 5:00, Pierre-Louis Bossart wrote:
> > >
> > >
> > > On 2/10/20 12:15 AM, YueHaibing wrote:
> > >> when do randconfig like this:
> > >> CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
> > >> CONFIG_SND_SOC_SOF_IMX8=y
> > >> CONFIG_SND_SOC_SOF_OF=y
> > >> CONFIG_IMX_DSP=m
> > >> CONFIG_IMX_SCU=y
> > >>
> > >> there is a link error:
> > >>
> > >> sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
> > >> imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'
> > >>
> > >> Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this
> > >>
> > >> Reported-by: Hulk Robot <hulkci@huawei.com>
> > >> Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
> > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > >
> > > Thanks for the report.
> > >
> > > Would you mind sharing the .config and instructions to reproduce this case? we have an unrelated issue with allyesconfig that was reviewed here:
> > >
> > > https://github.com/thesofproject/linux/pull/1778
> > >
> > > and I'd probably a good thing to fix everything in one shot.
> >
> > config is attached, which is on x86_64
>
> Thanks, I think this is legit. It was introduced with:
>
> commit f52cdcce9197fef9d4a68792dd3b840ad2b77117
> Author: Daniel Baluta <daniel.baluta@nxp.com>
> Date:   Sat Jan 4 15:39:53 2020 +0000
>
>     firmware: imx: Allow IMX DSP to be selected as module
>
>     IMX DSP is only needed by SOF or any other module that
>     wants to communicate with the DSP. When SOF is build
>     as a module IMX DSP is forced to be built inside the
>     kernel image. This is not optimal, so allow IMX DSP
>     to be built as a module.
>
>     Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>     Signed-off-by: Shawn Guo <shawnguo@kernel.org>

Hi,

Since it's a module, don't you just miss EXPORT_SYMBOL there?

> So, I think we should change the Fixes tag. Are there
> any clear rules on when to use select vs depends?
>
> On my side, I know what both are doing but it is not clear
> when to use them.

Visible symbols usually should not be selected. The same with symbols
with dependencies. The docs have this rule mentioned.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Daniel Baluta]

On Tue, Feb 11, 2020 at 11:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, 11 Feb 2020 at 10:46, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> >
> > On Tue, Feb 11, 2020 at 3:59 AM Yuehaibing <yuehaibing@huawei.com> wrote:
> > >
> > > On 2020/2/11 5:00, Pierre-Louis Bossart wrote:
> > > >
> > > >
> > > > On 2/10/20 12:15 AM, YueHaibing wrote:
> > > >> when do randconfig like this:
> > > >> CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
> > > >> CONFIG_SND_SOC_SOF_IMX8=y
> > > >> CONFIG_SND_SOC_SOF_OF=y
> > > >> CONFIG_IMX_DSP=m
> > > >> CONFIG_IMX_SCU=y
> > > >>
> > > >> there is a link error:
> > > >>
> > > >> sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
> > > >> imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'
> > > >>
> > > >> Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this
> > > >>
> > > >> Reported-by: Hulk Robot <hulkci@huawei.com>
> > > >> Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
> > > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > > >
> > > > Thanks for the report.
> > > >
> > > > Would you mind sharing the .config and instructions to reproduce this case? we have an unrelated issue with allyesconfig that was reviewed here:
> > > >
> > > > https://github.com/thesofproject/linux/pull/1778
> > > >
> > > > and I'd probably a good thing to fix everything in one shot.
> > >
> > > config is attached, which is on x86_64
> >
> > Thanks, I think this is legit. It was introduced with:
> >
> > commit f52cdcce9197fef9d4a68792dd3b840ad2b77117
> > Author: Daniel Baluta <daniel.baluta@nxp.com>
> > Date:   Sat Jan 4 15:39:53 2020 +0000
> >
> >     firmware: imx: Allow IMX DSP to be selected as module
> >
> >     IMX DSP is only needed by SOF or any other module that
> >     wants to communicate with the DSP. When SOF is build
> >     as a module IMX DSP is forced to be built inside the
> >     kernel image. This is not optimal, so allow IMX DSP
> >     to be built as a module.
> >
> >     Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> >     Signed-off-by: Shawn Guo <shawnguo@kernel.org>
>
> Hi,
>
> Since it's a module, don't you just miss EXPORT_SYMBOL there?

This is a good point. Will have a deeper look at this asap and sent a
fix .

>
> > So, I think we should change the Fixes tag. Are there
> > any clear rules on when to use select vs depends?
> >
> > On my side, I know what both are doing but it is not clear
> > when to use them.
>
> Visible symbols usually should not be selected. The same with symbols
> with dependencies. The docs have this rule mentioned.

I see. Thanks!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Daniel Baluta]

On Tue, Feb 11, 2020 at 11:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, 11 Feb 2020 at 10:46, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> >
> > On Tue, Feb 11, 2020 at 3:59 AM Yuehaibing <yuehaibing@huawei.com> wrote:
> > >
> > > On 2020/2/11 5:00, Pierre-Louis Bossart wrote:
> > > >
> > > >
> > > > On 2/10/20 12:15 AM, YueHaibing wrote:
> > > >> when do randconfig like this:
> > > >> CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
> > > >> CONFIG_SND_SOC_SOF_IMX8=y
> > > >> CONFIG_SND_SOC_SOF_OF=y
> > > >> CONFIG_IMX_DSP=m
> > > >> CONFIG_IMX_SCU=y
> > > >>
> > > >> there is a link error:
> > > >>
> > > >> sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
> > > >> imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'
> > > >>
> > > >> Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this
> > > >>
> > > >> Reported-by: Hulk Robot <hulkci@huawei.com>
> > > >> Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
> > > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > > >
> > > > Thanks for the report.
> > > >
> > > > Would you mind sharing the .config and instructions to reproduce this case? we have an unrelated issue with allyesconfig that was reviewed here:
> > > >
> > > > https://github.com/thesofproject/linux/pull/1778
> > > >
> > > > and I'd probably a good thing to fix everything in one shot.
> > >
> > > config is attached, which is on x86_64
> >
> > Thanks, I think this is legit. It was introduced with:
> >
> > commit f52cdcce9197fef9d4a68792dd3b840ad2b77117
> > Author: Daniel Baluta <daniel.baluta@nxp.com>
> > Date:   Sat Jan 4 15:39:53 2020 +0000
> >
> >     firmware: imx: Allow IMX DSP to be selected as module
> >
> >     IMX DSP is only needed by SOF or any other module that
> >     wants to communicate with the DSP. When SOF is build
> >     as a module IMX DSP is forced to be built inside the
> >     kernel image. This is not optimal, so allow IMX DSP
> >     to be built as a module.
> >
> >     Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> >     Signed-off-by: Shawn Guo <shawnguo@kernel.org>
>
> Hi,
>
> Since it's a module, don't you just miss EXPORT_SYMBOL there?

Hi Krzysztof,

Which symbol misses an EXPORT_SYMBOL?
We already have EXPORT_SYMBOL(imx_dsp_ring_doorbell);

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Daniel Baluta]

On Tue, Feb 11, 2020 at 11:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, 11 Feb 2020 at 10:46, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> >
> > On Tue, Feb 11, 2020 at 3:59 AM Yuehaibing <yuehaibing@huawei.com> wrote:
> > >
> > > On 2020/2/11 5:00, Pierre-Louis Bossart wrote:
> > > >
> > > >
> > > > On 2/10/20 12:15 AM, YueHaibing wrote:
> > > >> when do randconfig like this:
> > > >> CONFIG_SND_SOC_SOF_IMX8_SUPPORT=y
> > > >> CONFIG_SND_SOC_SOF_IMX8=y
> > > >> CONFIG_SND_SOC_SOF_OF=y
> > > >> CONFIG_IMX_DSP=m
> > > >> CONFIG_IMX_SCU=y
> > > >>
> > > >> there is a link error:
> > > >>
> > > >> sound/soc/sof/imx/imx8.o: In function 'imx8_send_msg':
> > > >> imx8.c:(.text+0x380): undefined reference to 'imx_dsp_ring_doorbell'
> > > >>
> > > >> Select IMX_DSP in SND_SOC_SOF_IMX8_SUPPORT to fix this
> > > >>
> > > >> Reported-by: Hulk Robot <hulkci@huawei.com>
> > > >> Fixes: f9ad75468453 ("ASoC: SOF: imx: fix reverse CONFIG_SND_SOC_SOF_OF dependency")
> > > >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > > >
> > > > Thanks for the report.
> > > >
> > > > Would you mind sharing the .config and instructions to reproduce this case? we have an unrelated issue with allyesconfig that was reviewed here:
> > > >
> > > > https://github.com/thesofproject/linux/pull/1778
> > > >
> > > > and I'd probably a good thing to fix everything in one shot.
> > >
> > > config is attached, which is on x86_64
> >
> > Thanks, I think this is legit. It was introduced with:
> >
> > commit f52cdcce9197fef9d4a68792dd3b840ad2b77117
> > Author: Daniel Baluta <daniel.baluta@nxp.com>
> > Date:   Sat Jan 4 15:39:53 2020 +0000
> >
> >     firmware: imx: Allow IMX DSP to be selected as module
> >
> >     IMX DSP is only needed by SOF or any other module that
> >     wants to communicate with the DSP. When SOF is build
> >     as a module IMX DSP is forced to be built inside the
> >     kernel image. This is not optimal, so allow IMX DSP
> >     to be built as a module.
> >
> >     Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
> >     Signed-off-by: Shawn Guo <shawnguo@kernel.org>
>
> Hi,
>
> Since it's a module, don't you just miss EXPORT_SYMBOL there?
>
> > So, I think we should change the Fixes tag. Are there
> > any clear rules on when to use select vs depends?
> >
> > On my side, I know what both are doing but it is not clear
> > when to use them.
>
> Visible symbols usually should not be selected. The same with symbols
> with dependencies. The docs have this rule mentioned.

You mean if module X depends on module Y, we shouldn't use select?
But this exactly what this patch does :).

The problem here is that when X depends on Y, and X=y and Y=m
when we try to compile X if get an error because we cannot find a symbol from Y.

I think if X depends on Y, and X is forced to "y" then also Y should
be forced on "y".

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Krzysztof Kozlowski]

On Thu, 13 Feb 2020 at 22:09, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> Hi Krzysztof,
>
> Which symbol misses an EXPORT_SYMBOL?
> We already have EXPORT_SYMBOL(imx_dsp_ring_doorbell);

Hi,

Yes, exactly this one. That was just my guess.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix randbuild error

[Krzysztof Kozlowski]

On Thu, 13 Feb 2020 at 22:12, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> >
> > Visible symbols usually should not be selected. The same with symbols
> > with dependencies. The docs have this rule mentioned.
>
> You mean if module X depends on module Y, we shouldn't use select?
> But this exactly what this patch does :).

There are two different cases (hints against using select):
1. select A, if A is a user-visible (possible to select) option,
2. select only A if A depends on B (and B is kind of independent).

These cases are discouraged.

> The problem here is that when X depends on Y, and X=y and Y=m
> when we try to compile X if get an error because we cannot find a symbol from Y.
>
> I think if X depends on Y, and X is forced to "y" then also Y should
> be forced on "y".

If X is bool, then depends should be =y.
If X is tristate, then probably you need something like:
    depends (Y!=m || m)

There is also solution like:
config CEPH_FSCACHE
    depends on CEPH_FS=m && FSCACHE || CEPH_FS=y && FSCACHE=y
but it works if the upper-level option (CEPH_FS) is a tristate.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel