[PATCH 1/8] ASoC: amd: acp: add a null check for chip_pdev structure
When acp platform device creation is skipped, chip->chip_pdev value will remain NULL. Add NULL check for chip->chip_pdev structure in snd_acp_resume() function to avoid null pointer dereference.
Fixes: 088a40980efb ("ASoC: amd: acp: add pm ops support for acp pci driver") Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-pci.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/sound/soc/amd/acp/acp-pci.c b/sound/soc/amd/acp/acp-pci.c index ad320b29e87d..aa3e72d13451 100644 --- a/sound/soc/amd/acp/acp-pci.c +++ b/sound/soc/amd/acp/acp-pci.c @@ -199,10 +199,12 @@ static int __maybe_unused snd_acp_resume(struct device *dev) ret = acp_init(chip); if (ret) dev_err(dev, "ACP init failed\n"); - child = chip->chip_pdev->dev; - adata = dev_get_drvdata(&child); - if (adata) - acp_enable_interrupts(adata); + if (chip->chip_pdev) { + child = chip->chip_pdev->dev; + adata = dev_get_drvdata(&child); + if (adata) + acp_enable_interrupts(adata); + } return ret; }
ACP supports different pin configurations for I2S IO. Checking ACP pin configuration value against specific value breaks the functionality for other I2S pin configurations. This check is no longer required in i2s dai driver probe call as i2s configuration check will be verified during acp platform device creation sequence. Remove i2s_mode check in acp_i2s_probe() function.
Fixes: b24484c18b10 ("ASoC: amd: acp: ACP code generic to support newer platforms") Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-i2s.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/sound/soc/amd/acp/acp-i2s.c b/sound/soc/amd/acp/acp-i2s.c index 60cbc881be6e..ef12f97ddc69 100644 --- a/sound/soc/amd/acp/acp-i2s.c +++ b/sound/soc/amd/acp/acp-i2s.c @@ -588,20 +588,12 @@ static int acp_i2s_probe(struct snd_soc_dai *dai) { struct device *dev = dai->component->dev; struct acp_dev_data *adata = dev_get_drvdata(dev); - struct acp_resource *rsrc = adata->rsrc; - unsigned int val;
if (!adata->acp_base) { dev_err(dev, "I2S base is NULL\n"); return -EINVAL; }
- val = readl(adata->acp_base + rsrc->i2s_pin_cfg_offset); - if (val != rsrc->i2s_mode) { - dev_err(dev, "I2S Mode not supported val %x\n", val); - return -EINVAL; - } - return 0; }
chip->flag variable assignment will be skipped when acp platform device creation is skipped. In this case chip>flag value will not be set. chip->flag variable should be assigned along with other structure variables for 'chip' structure. Move chip->flag variable assignment prior to acp platform device creation.
Fixes: 3a94c8ad0aae ("ASoC: amd: acp: add code for scanning acp pdm controller") Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/amd/acp/acp-pci.c b/sound/soc/amd/acp/acp-pci.c index aa3e72d13451..777b5a78d8a9 100644 --- a/sound/soc/amd/acp/acp-pci.c +++ b/sound/soc/amd/acp/acp-pci.c @@ -100,6 +100,7 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id ret = -EINVAL; goto release_regions; } + chip->flag = flag; dmic_dev = platform_device_register_data(dev, "dmic-codec", PLATFORM_DEVID_NONE, NULL, 0); if (IS_ERR(dmic_dev)) { dev_err(dev, "failed to create DMIC device\n"); @@ -139,7 +140,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id } }
- chip->flag = flag; memset(&pdevinfo, 0, sizeof(pdevinfo));
pdevinfo.name = chip->name;
In acp_i2s_probe(), acp_base null check is verified. As already acp_base null check will be verified in acp platform driver probe sequence, additional NULL check in acp_i2s_probe() is not needed. Remove acp_i2s_probe() function.
Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-i2s.c | 14 -------------- 1 file changed, 14 deletions(-)
diff --git a/sound/soc/amd/acp/acp-i2s.c b/sound/soc/amd/acp/acp-i2s.c index ef12f97ddc69..7da414bc3b96 100644 --- a/sound/soc/amd/acp/acp-i2s.c +++ b/sound/soc/amd/acp/acp-i2s.c @@ -584,21 +584,7 @@ static int acp_i2s_startup(struct snd_pcm_substream *substream, struct snd_soc_d return 0; }
-static int acp_i2s_probe(struct snd_soc_dai *dai) -{ - struct device *dev = dai->component->dev; - struct acp_dev_data *adata = dev_get_drvdata(dev); - - if (!adata->acp_base) { - dev_err(dev, "I2S base is NULL\n"); - return -EINVAL; - } - - return 0; -} - const struct snd_soc_dai_ops asoc_acp_cpu_dai_ops = { - .probe = acp_i2s_probe, .startup = acp_i2s_startup, .hw_params = acp_i2s_hwparams, .prepare = acp_i2s_prepare,
Remove unused variables i2s_pin_cfg_offset and i2s_mode from acp_resource structure entries.
Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-rembrandt.c | 2 -- sound/soc/amd/acp/acp-renoir.c | 2 -- sound/soc/amd/acp/acp63.c | 2 -- sound/soc/amd/acp/acp70.c | 2 -- sound/soc/amd/acp/amd.h | 2 -- 5 files changed, 10 deletions(-)
diff --git a/sound/soc/amd/acp/acp-rembrandt.c b/sound/soc/amd/acp/acp-rembrandt.c index 158f819f8da4..953a793de9a3 100644 --- a/sound/soc/amd/acp/acp-rembrandt.c +++ b/sound/soc/amd/acp/acp-rembrandt.c @@ -39,8 +39,6 @@ static struct acp_resource rsrc = { .irqp_used = 1, .soc_mclk = true, .irq_reg_offset = 0x1a00, - .i2s_pin_cfg_offset = 0x1440, - .i2s_mode = 0x0a, .scratch_reg_offset = 0x12800, .sram_pte_offset = 0x03802800, }; diff --git a/sound/soc/amd/acp/acp-renoir.c b/sound/soc/amd/acp/acp-renoir.c index b0e181c9a733..db835ed7c208 100644 --- a/sound/soc/amd/acp/acp-renoir.c +++ b/sound/soc/amd/acp/acp-renoir.c @@ -32,8 +32,6 @@ static struct acp_resource rsrc = { .no_of_ctrls = 1, .irqp_used = 0, .irq_reg_offset = 0x1800, - .i2s_pin_cfg_offset = 0x1400, - .i2s_mode = 0x04, .scratch_reg_offset = 0x12800, .sram_pte_offset = 0x02052800, }; diff --git a/sound/soc/amd/acp/acp63.c b/sound/soc/amd/acp/acp63.c index 4d342441a650..f223311b6740 100644 --- a/sound/soc/amd/acp/acp63.c +++ b/sound/soc/amd/acp/acp63.c @@ -55,8 +55,6 @@ static struct acp_resource rsrc = { .irqp_used = 1, .soc_mclk = true, .irq_reg_offset = 0x1a00, - .i2s_pin_cfg_offset = 0x1440, - .i2s_mode = 0x0a, .scratch_reg_offset = 0x12800, .sram_pte_offset = 0x03802800, }; diff --git a/sound/soc/amd/acp/acp70.c b/sound/soc/amd/acp/acp70.c index 0d7cdd4017e5..a2cbdcca4313 100644 --- a/sound/soc/amd/acp/acp70.c +++ b/sound/soc/amd/acp/acp70.c @@ -31,8 +31,6 @@ static struct acp_resource rsrc = { .irqp_used = 1, .soc_mclk = true, .irq_reg_offset = 0x1a00, - .i2s_pin_cfg_offset = 0x1440, - .i2s_mode = 0x0a, .scratch_reg_offset = 0x12800, .sram_pte_offset = 0x03802800, }; diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h index d75b4eb34de8..cbf9ca26f3ee 100644 --- a/sound/soc/amd/acp/amd.h +++ b/sound/soc/amd/acp/amd.h @@ -162,8 +162,6 @@ struct acp_resource { int irqp_used; bool soc_mclk; u32 irq_reg_offset; - u32 i2s_pin_cfg_offset; - int i2s_mode; u64 scratch_reg_offset; u64 sram_pte_offset; };
ACP provides different IO configurations(ACP PDM, I2S and SoundWire). I2S mclk should be programmed only when I2S configuration is selected and I2S controller is programmed as clock master. Modify the conditional check for programming i2s mclk.
Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-rembrandt.c | 5 +++-- sound/soc/amd/acp/acp63.c | 5 +++-- sound/soc/amd/acp/amd.h | 1 + 3 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/sound/soc/amd/acp/acp-rembrandt.c b/sound/soc/amd/acp/acp-rembrandt.c index 953a793de9a3..e19981c7d65a 100644 --- a/sound/soc/amd/acp/acp-rembrandt.c +++ b/sound/soc/amd/acp/acp-rembrandt.c @@ -229,12 +229,13 @@ static int rembrandt_audio_probe(struct platform_device *pdev) adata->rsrc = &rsrc; adata->platform = REMBRANDT; adata->flag = chip->flag; + adata->is_i2s_config = chip->is_i2s_config; adata->machines = snd_soc_acpi_amd_rmb_acp_machines; acp_machine_select(adata);
dev_set_drvdata(dev, adata);
- if (chip->flag != FLAG_AMD_LEGACY_ONLY_DMIC) { + if (chip->is_i2s_config && rsrc.soc_mclk) { ret = acp6x_master_clock_generate(dev); if (ret) return ret; @@ -267,7 +268,7 @@ static int __maybe_unused rmb_pcm_resume(struct device *dev) snd_pcm_uframes_t buf_in_frames; u64 buf_size;
- if (adata->flag != FLAG_AMD_LEGACY_ONLY_DMIC) + if (adata->is_i2s_config && adata->rsrc->soc_mclk) acp6x_master_clock_generate(dev);
spin_lock(&adata->acp_lock); diff --git a/sound/soc/amd/acp/acp63.c b/sound/soc/amd/acp/acp63.c index f223311b6740..f340920b3289 100644 --- a/sound/soc/amd/acp/acp63.c +++ b/sound/soc/amd/acp/acp63.c @@ -239,11 +239,12 @@ static int acp63_audio_probe(struct platform_device *pdev) adata->rsrc = &rsrc; adata->platform = ACP63; adata->flag = chip->flag; + adata->is_i2s_config = chip->is_i2s_config; adata->machines = snd_soc_acpi_amd_acp63_acp_machines; acp_machine_select(adata); dev_set_drvdata(dev, adata);
- if (chip->flag != FLAG_AMD_LEGACY_ONLY_DMIC) { + if (chip->is_i2s_config && rsrc.soc_mclk) { ret = acp63_i2s_master_clock_generate(adata); if (ret) return ret; @@ -276,7 +277,7 @@ static int __maybe_unused acp63_pcm_resume(struct device *dev) snd_pcm_uframes_t buf_in_frames; u64 buf_size;
- if (adata->flag != FLAG_AMD_LEGACY_ONLY_DMIC) + if (adata->is_i2s_config && adata->rsrc->soc_mclk) acp63_i2s_master_clock_generate(adata);
spin_lock(&adata->acp_lock); diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h index cbf9ca26f3ee..87a4813783f9 100644 --- a/sound/soc/amd/acp/amd.h +++ b/sound/soc/amd/acp/amd.h @@ -173,6 +173,7 @@ struct acp_dev_data { unsigned int i2s_irq;
bool tdm_mode; + bool is_i2s_config; /* SOC specific dais */ struct snd_soc_dai_driver *dai_driver; int num_dai;
I2S clock generation registers should be programmed before starting the I2S dma when I2S controller is programmed as clock master. Move i2s clock generation register programming sequence prior to i2s dma start.
Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-i2s.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/amd/acp/acp-i2s.c b/sound/soc/amd/acp/acp-i2s.c index 7da414bc3b96..88985e9d318b 100644 --- a/sound/soc/amd/acp/acp-i2s.c +++ b/sound/soc/amd/acp/acp-i2s.c @@ -369,12 +369,12 @@ static int acp_i2s_trigger(struct snd_pcm_substream *substream, int cmd, struct } writel(period_bytes, adata->acp_base + water_val); writel(buf_size, adata->acp_base + buf_reg); + if (rsrc->soc_mclk) + acp_set_i2s_clk(adata, dai->driver->id); val = readl(adata->acp_base + reg_val); val = val | BIT(0); writel(val, adata->acp_base + reg_val); writel(1, adata->acp_base + ier_val); - if (rsrc->soc_mclk) - acp_set_i2s_clk(adata, dai->driver->id); return 0; case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND:
ACP common dma driver has a buffer size and period size restriction which should be 64 byte aligned. Add pcm constraints for the same.
Signed-off-by: Vijendar Mukunda Vijendar.Mukunda@amd.com --- sound/soc/amd/acp/acp-platform.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/sound/soc/amd/acp/acp-platform.c b/sound/soc/amd/acp/acp-platform.c index aaac8aa744cb..4f409cd09c11 100644 --- a/sound/soc/amd/acp/acp-platform.c +++ b/sound/soc/amd/acp/acp-platform.c @@ -197,6 +197,20 @@ static int acp_dma_open(struct snd_soc_component *component, struct snd_pcm_subs else runtime->hw = acp_pcm_hardware_capture;
+ ret = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, DMA_SIZE); + if (ret) { + dev_err(component->dev, "set hw constraint HW_PARAM_PERIOD_BYTES failed\n"); + kfree(stream); + return ret; + } + + ret = snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, DMA_SIZE); + if (ret) { + dev_err(component->dev, "set hw constraint HW_PARAM_BUFFER_BYTES failed\n"); + kfree(stream); + return ret; + } + ret = snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); if (ret < 0) { dev_err(component->dev, "set integer constraint failed\n");
On Mon, 17 Jun 2024 12:58:34 +0530, Vijendar Mukunda wrote:
When acp platform device creation is skipped, chip->chip_pdev value will remain NULL. Add NULL check for chip->chip_pdev structure in snd_acp_resume() function to avoid null pointer dereference.
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/8] ASoC: amd: acp: add a null check for chip_pdev structure commit: 98d919dfee1cc402ca29d45da642852d7c9a2301 [2/8] ASoC: amd: acp: remove i2s configuration check in acp_i2s_probe() commit: 70fa3900c3ed92158628710e81d274e5cb52f92b [3/8] ASoC: amd: acp: move chip->flag variable assignment commit: 379bcd2c9197bf2c429434e8a01cea0ee1852316
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
On Mon, 17 Jun 2024 12:58:34 +0530, Vijendar Mukunda wrote:
When acp platform device creation is skipped, chip->chip_pdev value will remain NULL. Add NULL check for chip->chip_pdev structure in snd_acp_resume() function to avoid null pointer dereference.
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[4/8] ASoC: amd: acp: remove acp_i2s_probe function commit: 75a08ec8c3a6aef914235c409a99046a3d29d1d4 [5/8] ASoC: amd: acp: remove unused variables from acp_resource structure commit: 50f1670145392398f4f2ffe9abe4599c86d11ec2 [6/8] ASoC: amd: acp: modify conditional check for programming i2s mclk commit: 5b162f60e7e051624e187e45b0fdc481c3573f17 [7/8] ASoC: amd: acp: move i2s clock generation sequence commit: d85695b01cbb2455a2f70528bb9e53f2463a39cf [8/8] ASoC: amd: acp: add pcm constraints for buffer size and period size commit: 8978e1f7bc26655e0373c4a6b31e17fcdd497329
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Mark Brown -
Vijendar Mukunda