[alsa-devel] [PATCH] ASoC: dwc: disallow building designware_pcm as a module
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
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.
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
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.
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
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
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
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.
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
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.
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
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
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.
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
participants (4)
-
Jose Abreu -
Lubomir Rintel -
Mark Brown -
Takashi Iwai