diff mbox

ASoC: dwc: disallow building designware_pcm as a module

Message ID 20170418105954.23368-1-lkundrak@v3.sk (mailing list archive)
State New, archived
Headers show

Commit Message

Lubomir Rintel April 18, 2017, 10:59 a.m. UTC
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%)

Comments

Mark Brown April 18, 2017, 3:18 p.m. UTC | #1
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.
Lubomir Rintel April 18, 2017, 4:13 p.m. UTC | #2
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
Mark Brown April 18, 2017, 5:15 p.m. UTC | #3
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.
Jose Abreu April 19, 2017, 4:12 p.m. UTC | #4
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
Lubomir Rintel April 19, 2017, 4:14 p.m. UTC | #5
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
Jose Abreu April 19, 2017, 4:48 p.m. UTC | #6
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
Mark Brown April 20, 2017, 7:46 p.m. UTC | #7
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.
Takashi Iwai April 20, 2017, 8:24 p.m. UTC | #8
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
Mark Brown April 20, 2017, 8:25 p.m. UTC | #9
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.
Jose Abreu April 21, 2017, 10:34 a.m. UTC | #10
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
Takashi Iwai April 21, 2017, 10:39 a.m. UTC | #11
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
Mark Brown April 21, 2017, 10:49 a.m. UTC | #12
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.
Jose Abreu April 27, 2017, 6:49 p.m. UTC | #13
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
diff mbox

Patch

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>