[PATCH] ASoC: dwc: disallow building designware_pcm as a module

[PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Lubomir Rintel]

It makes not sense: the whether the PIO PCM extension is used is
hardcoded to the designware_i2s driver and designware_pcm doesn't
have any module metadata, causing a kernel taint:

  [   44.287000] designware_pcm: module license 'unspecified' taints kernel.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 sound/soc/dwc/Kconfig                                     | 4 ++--
 sound/soc/dwc/Makefile                                    | 6 +++++-
 sound/soc/dwc/{designware_i2s.c => designware_i2s_main.c} | 2 +-
 sound/soc/dwc/{designware_pcm.c => designware_i2s_pcm.c}  | 2 +-
 4 files changed, 9 insertions(+), 5 deletions(-)
 rename sound/soc/dwc/{designware_i2s.c => designware_i2s_main.c} (99%)
 rename sound/soc/dwc/{designware_pcm.c => designware_i2s_pcm.c} (99%)

diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
index c297efe..704b7b2 100644
--- a/sound/soc/dwc/Kconfig
+++ b/sound/soc/dwc/Kconfig
@@ -8,10 +8,10 @@ config SND_DESIGNWARE_I2S
 	 maximum of 8 channels each for play and record.
 
 config SND_DESIGNWARE_PCM
-	tristate "PCM PIO extension for I2S driver"
+	bool "PCM PIO extension for I2S driver"
 	depends on SND_DESIGNWARE_I2S
 	help
-	 Say Y, M or N if you want to add a custom ALSA extension that registers
+	 Say Y if you want to add a custom ALSA extension that registers
 	 a PCM and uses PIO to transfer data.
 
 	 This functionality is specially suited for I2S devices that don't have
diff --git a/sound/soc/dwc/Makefile b/sound/soc/dwc/Makefile
index 38f1ca3..05c594f 100644
--- a/sound/soc/dwc/Makefile
+++ b/sound/soc/dwc/Makefile
@@ -1,5 +1,9 @@
 # SYNOPSYS Platform Support
+
 obj-$(CONFIG_SND_DESIGNWARE_I2S) += designware_i2s.o
+
+designware_i2s-objs := designware_i2s_main.o
+
 ifdef CONFIG_SND_DESIGNWARE_PCM
-obj-$(CONFIG_SND_DESIGNWARE_I2S) += designware_pcm.o
+designware_i2s-objs += designware_i2s_pcm.o
 endif
diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s_main.c
similarity index 99%
rename from sound/soc/dwc/designware_i2s.c
rename to sound/soc/dwc/designware_i2s_main.c
index bdf8398..23c2212 100644
--- a/sound/soc/dwc/designware_i2s.c
+++ b/sound/soc/dwc/designware_i2s_main.c
@@ -1,7 +1,7 @@
 /*
  * ALSA SoC Synopsys I2S Audio Layer
  *
- * sound/soc/dwc/designware_i2s.c
+ * sound/soc/dwc/designware_i2s_main.c
  *
  * Copyright (C) 2010 ST Microelectronics
  * Rajeev Kumar <rajeevkumar.linux@gmail.com>
diff --git a/sound/soc/dwc/designware_pcm.c b/sound/soc/dwc/designware_i2s_pcm.c
similarity index 99%
rename from sound/soc/dwc/designware_pcm.c
rename to sound/soc/dwc/designware_i2s_pcm.c
index 4a83a22..27664e2 100644
--- a/sound/soc/dwc/designware_pcm.c
+++ b/sound/soc/dwc/designware_i2s_pcm.c
@@ -1,7 +1,7 @@
 /*
  * ALSA SoC Synopsys PIO PCM for I2S driver
  *
- * sound/soc/dwc/designware_pcm.c
+ * sound/soc/dwc/designware_i2s_pcm.c
  *
  * Copyright (C) 2016 Synopsys
  * Jose Abreu <joabreu@synopsys.com>
-- 
2.9.3

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Mark Brown]

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

On Tue, Apr 18, 2017 at 12:59:54PM +0200, Lubomir Rintel wrote:
> It makes not sense: the whether the PIO PCM extension is used is
> hardcoded to the designware_i2s driver and designware_pcm doesn't
> have any module metadata, causing a kernel taint:
> 
>   [   44.287000] designware_pcm: module license 'unspecified' taints kernel.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

This is not a a good approach, there is no technical reason to force the
driver to be built in.  If you need a license tag in the module then add
that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Lubomir Rintel]

On Tue, 2017-04-18 at 16:18 +0100, Mark Brown wrote:
> On Tue, Apr 18, 2017 at 12:59:54PM +0200, Lubomir Rintel wrote:
> > It makes not sense: the whether the PIO PCM extension is used is
> > hardcoded to the designware_i2s driver and designware_pcm doesn't
> > have any module metadata, causing a kernel taint:
> > 
> >   [   44.287000] designware_pcm: module license 'unspecified' taints kernel.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> This is not a a good approach, there is no technical reason to force the
> driver to be built in.  If you need a license tag in the module then add
> that.

I don't think designware_pcm is a separate driver. It looks tightly
coupled with designware_i2s: you can either disable designware_pcm
altogether at build time or always load it together with
designware_i2s.

See sound/soc/dwc/local.h:

  #if IS_ENABLED(CONFIG_SND_DESIGNWARE_PCM)
  void dw_pcm_push_tx(struct dw_i2s_dev *dev);
  int dw_pcm_register(struct platform_device *pdev);
  #else
  void dw_pcm_push_tx(struct dw_i2s_dev *dev) { }
  int dw_pcm_register(struct platform_device *pdev)
  {
          return -EINVAL;
  }
  #endif

Lubo
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 358 bytes --]

On Tue, Apr 18, 2017 at 06:13:30PM +0200, Lubomir Rintel wrote:

> I don't think designware_pcm is a separate driver. It looks tightly
> coupled with designware_i2s: you can either disable designware_pcm
> altogether at build time or always load it together with
> designware_i2s.

Yes, they're closely coupled but we might still want them both as a
module.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Jose Abreu]

Hi Lubomir,


On 18-04-2017 18:15, Mark Brown wrote:
> On Tue, Apr 18, 2017 at 06:13:30PM +0200, Lubomir Rintel wrote:
>
>> I don't think designware_pcm is a separate driver. It looks tightly
>> coupled with designware_i2s: you can either disable designware_pcm
>> altogether at build time or always load it together with
>> designware_i2s.
> Yes, they're closely coupled but we might still want them both as a
> module.

Thanks for the patch but I agree with Mark.

For the record you can add "MODULE_LICENSE(“Dual MIT/GPL”)".

Best regards,
Jose Miguel Abreu

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Lubomir Rintel]

On Wed, 2017-04-19 at 17:12 +0100, Jose Abreu wrote:
> Hi Lubomir,
> 
> 
> On 18-04-2017 18:15, Mark Brown wrote:
> > On Tue, Apr 18, 2017 at 06:13:30PM +0200, Lubomir Rintel wrote:
> > 
> > > I don't think designware_pcm is a separate driver. It looks
> > > tightly
> > > coupled with designware_i2s: you can either disable
> > > designware_pcm
> > > altogether at build time or always load it together with
> > > designware_i2s.
> > 
> > Yes, they're closely coupled but we might still want them both as a
> > module.
> 
> Thanks for the patch but I agree with Mark.
> 
> For the record you can add "MODULE_LICENSE(“Dual MIT/GPL”)".

Sorry if I'm missing something; but I still fail to understand why is
that a good idea.

What is the point of having a pair of modules that depend on each other
and can only be loaded together?

> Best regards,
> Jose Miguel Abreu

Lubo
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Jose Abreu]

On 19-04-2017 17:14, Lubomir Rintel wrote:
> On Wed, 2017-04-19 at 17:12 +0100, Jose Abreu wrote:
>> Hi Lubomir,
>>
>>
>> On 18-04-2017 18:15, Mark Brown wrote:
>>> On Tue, Apr 18, 2017 at 06:13:30PM +0200, Lubomir Rintel wrote:
>>>
>>>> I don't think designware_pcm is a separate driver. It looks
>>>> tightly
>>>> coupled with designware_i2s: you can either disable
>>>> designware_pcm
>>>> altogether at build time or always load it together with
>>>> designware_i2s.
>>> Yes, they're closely coupled but we might still want them both as a
>>> module.
>> Thanks for the patch but I agree with Mark.
>>
>> For the record you can add "MODULE_LICENSE(“Dual MIT/GPL”)".
> Sorry if I'm missing something; but I still fail to understand why is
> that a good idea.
>
> What is the point of having a pair of modules that depend on each other
> and can only be loaded together?

Sorry, I don't know why but I was thinking that PCM module will
not load when we have a DMA platform but indeed it will load the
module because of the dependencies, even though the register
function will never be called. Without this I don't really have
anything against the patch.

What do you think Mark? If you want to keep the PCM as a module
then we will need to abstract this more, by reducing the
dependencies.

Best regards,
Jose Miguel Abreu

>
>> Best regards,
>> Jose Miguel Abreu
> Lubo

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 550 bytes --]

On Wed, Apr 19, 2017 at 05:48:15PM +0100, Jose Abreu wrote:

> What do you think Mark? If you want to keep the PCM as a module
> then we will need to abstract this more, by reducing the
> dependencies.

I think forcing this to be built in to the kernel (which is what the
commit message says the change is going to do) is an obviously bad
idea.  Anything we add to the base kernel image needs to have a good
reason to be there and it is hard to think what that reason might be for
any audio driver, we need to be able to put this code into a module.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Takashi Iwai]

On Thu, 20 Apr 2017 21:46:46 +0200,
Mark Brown wrote:
> 
> On Wed, Apr 19, 2017 at 05:48:15PM +0100, Jose Abreu wrote:
> 
> > What do you think Mark? If you want to keep the PCM as a module
> > then we will need to abstract this more, by reducing the
> > dependencies.
> 
> I think forcing this to be built in to the kernel (which is what the
> commit message says the change is going to do) is an obviously bad
> idea.  Anything we add to the base kernel image needs to have a good
> reason to be there and it is hard to think what that reason might be for
> any audio driver, we need to be able to put this code into a module.

Well, I guess the original patch description caused a big confusion.
As far as I see, the intention of the patch is not about the module or
built-in kernel.  Instead it's rather to fold designware_pcm stuff
into the single designware_i2s driver.

The former is merely an extension of the latter driver, and the latter
invokes the former directly.  Thus there is little merit to keep them
separate.  I think the current code is even buggy, which allows to
leave CONFIG_SND_DESIGNWARE_I2S=y and CONFIG_SND_DESIGNWARE_PCM=m.

So, I think Lubomir's change is right.  But the patch subject and
description should be rephrased.

One thing I don't like is the rename of the file.  But in this
particular case, it's unavoidable unless we rename the module name.


BTW, we should drop the superfluous EXPORT_SYMBOL*(), too.


thanks,

Takashi

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]

On Thu, Apr 20, 2017 at 10:24:14PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > I think forcing this to be built in to the kernel (which is what the
> > commit message says the change is going to do) is an obviously bad
> > idea.  Anything we add to the base kernel image needs to have a good
> > reason to be there and it is hard to think what that reason might be for
> > any audio driver, we need to be able to put this code into a module.

> Well, I guess the original patch description caused a big confusion.
> As far as I see, the intention of the patch is not about the module or
> built-in kernel.  Instead it's rather to fold designware_pcm stuff
> into the single designware_i2s driver.

Ah, right.  That'd be fine.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Jose Abreu]

Hi,


On 20-04-2017 21:24, Takashi Iwai wrote:
> So, I think Lubomir's change is right.  But the patch subject and
> description should be rephrased.
>
> One thing I don't like is the rename of the file.  But in this
> particular case, it's unavoidable unless we rename the module name.
>

Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is
called "dwc") and let the module still be called "designware-i2s"?

> BTW, we should drop the superfluous EXPORT_SYMBOL*(), too.
>
>
>

Lubomir, could you please remove the EXPORT_SYMBOL, change the
commit message and resend?

Best regards,
Jose Miguel Abreu

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Takashi Iwai]

On Fri, 21 Apr 2017 12:34:00 +0200,
Jose Abreu wrote:
> 
> Hi,
> 
> 
> On 20-04-2017 21:24, Takashi Iwai wrote:
> > So, I think Lubomir's change is right.  But the patch subject and
> > description should be rephrased.
> >
> > One thing I don't like is the rename of the file.  But in this
> > particular case, it's unavoidable unless we rename the module name.
> >
> 
> Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is
> called "dwc") and let the module still be called "designware-i2s"?

Lubomir's patch keeps the module name intact.  My point is that rename
of a file isn't nice to look at the git commit history, so it's better
to be avoided as much as possible.  But in this case, it looks
unavoidable.


Takashi

Re: [alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Mark Brown]

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

On Fri, Apr 21, 2017 at 12:39:30PM +0200, Takashi Iwai wrote:
> Jose Abreu wrote:

> > Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is
> > called "dwc") and let the module still be called "designware-i2s"?

> Lubomir's patch keeps the module name intact.  My point is that rename
> of a file isn't nice to look at the git commit history, so it's better
> to be avoided as much as possible.  But in this case, it looks
> unavoidable.

Right.  Renaming the source file is the lesser evil, it's possible
people might have the module name in their configuration for their
systems somewhere.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: dwc: disallow building designware_pcm as a module

[Jose Abreu]

Hi,


On 21-04-2017 11:49, Mark Brown wrote:
> On Fri, Apr 21, 2017 at 12:39:30PM +0200, Takashi Iwai wrote:
>> Jose Abreu wrote:
>>> Maybe rename to "dwc-i2s.c" and "dwc-pcm.c" (as the folder is
>>> called "dwc") and let the module still be called "designware-i2s"?
>> Lubomir's patch keeps the module name intact.  My point is that rename
>> of a file isn't nice to look at the git commit history, so it's better
>> to be avoided as much as possible.  But in this case, it looks
>> unavoidable.
> Right.  Renaming the source file is the lesser evil, it's possible
> people might have the module name in their configuration for their
> systems somewhere.

I agree. I just noticed today that without a valid license tag we
get unresolved symbols when inserting pcm module so this patch is
really needed as a fix.

Lubomir, could you address the review comments and resend? If you
are not available let me know and I will fix and resend the patch
with your sign-off.

Best regards,
Jose Miguel Abreu