Re: [PATCH 1/2] ASoC: amd: acp: fix for acp platform device creation failure

From: Mukunda,Vijendar
Date: Fri May 03 2024 - 07:03:50 EST

Next message: syzbot: "Re: [syzbot] Re: [syzbot] [f2fs?] KASAN: slab-use-after-free Read in sanity_check_extent_cache"
Previous message: Sebastian Ott: "Re: [PATCH v2 5/6] KVM: arm64: show writable masks for feature registers"
In reply to: Vijendar Mukunda: "[PATCH 2/2] ASoC: amd: acp: move chip->flag variable assignment"
Next in thread: Mark Brown: "Re: [PATCH 1/2] ASoC: amd: acp: fix for acp platform device creation failure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 02/05/24 19:33, Vijendar Mukunda wrote:
> ACP pin configuration varies based on acp version.
> ACP PCI driver should read the ACP PIN config value and based on config
> value, it has to create a platform device in below two conditions.
> 1) If ACP PDM configuration is selected from BIOS and ACP PDM controller
> exists.
> 2) If ACP I2S configuration is selected from BIOS.
>
> Other than above scenarios, ACP PCI driver should skip the platform
> device creation logic, i.e. ACP PCI driver probe sequence should never
> fail if other acp pin configuration is selected. It should skip platform
> device creation logic.
>
> check_acp_pdm() function was implemented for ACP6.x platforms to check
> ACP PDM configuration. Previously, this code was safe guarded by
> FLAG_AMD_LEGACY_ONLY_DMIC flag check.
>
> This implementation breaks audio use cases for Huawei Matebooks which are
> based on ACP3.x varaint uses I2S configuration.
> In current scenario, check_acp_pdm() function returns -ENODEV value
> which results in ACP PCI driver probe failure without creating a platform
> device even in case of valid ACP pin configuration.
>
> Implement check_acp_config() as a common function which invokes platform
> specific acp pin configuration check functions for ACP3.x, ACP6.0 & ACP6.3
> & ACP7.0 variants and checks for ACP PDM controller.
>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218780
> Fixes: 4af565de9f8c ("ASoC: amd: acp: fix for acp pdm configuration check")
> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@xxxxxxx>
> ---
> sound/soc/amd/acp/acp-legacy-common.c | 96 ++++++++++++++++++++++-----
> sound/soc/amd/acp/acp-pci.c | 9 ++-
> sound/soc/amd/acp/amd.h | 10 ++-
> sound/soc/amd/acp/chip_offset_byte.h | 1 +
> 4 files changed, 95 insertions(+), 21 deletions(-)
>
> diff --git a/sound/soc/amd/acp/acp-legacy-common.c b/sound/soc/amd/acp/acp-legacy-common.c
> index b5aff3f230be..3be7c6d55a6f 100644
> --- a/sound/soc/amd/acp/acp-legacy-common.c
> +++ b/sound/soc/amd/acp/acp-legacy-common.c
> @@ -358,11 +358,25 @@ int smn_read(struct pci_dev *dev, u32 smn_addr)
> }
> EXPORT_SYMBOL_NS_GPL(smn_read, SND_SOC_ACP_COMMON);
>
> -int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip)
> +static void check_acp3x_config(struct acp_chip_info *chip)
> {
> - struct acpi_device *pdm_dev;
> - const union acpi_object *obj;
> - u32 pdm_addr, val;
> + u32 val;
> +
> + val = readl(chip->base + ACP3X_PIN_CONFIG);
> + switch (val) {
> + case ACP_CONFIG_4:
> + chip->is_i2s_config = true;
> + chip->is_pdm_config = true;
> + break;
> + default:
> + chip->is_pdm_config = true;
> + break;
> + }
> +}
> +
> +static void check_acp6x_config(struct acp_chip_info *chip)
> +{
> + u32 val;
>
> val = readl(chip->base + ACP_PIN_CONFIG);
> switch (val) {
> @@ -371,42 +385,94 @@ int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip)
> case ACP_CONFIG_6:
> case ACP_CONFIG_7:
> case ACP_CONFIG_8:
> - case ACP_CONFIG_10:
> case ACP_CONFIG_11:
> + case ACP_CONFIG_14:
> + chip->is_pdm_config = true;
> + break;
> + case ACP_CONFIG_9:
> + chip->is_i2s_config = true;
> + break;
> + case ACP_CONFIG_10:
> case ACP_CONFIG_12:
> case ACP_CONFIG_13:
> + chip->is_i2s_config = true;
> + chip->is_pdm_config = true;
> + break;
> + default:
> + break;
> + }
> +}
> +
> +static void check_acp70_config(struct acp_chip_info *chip)
> +{
> + u32 val;
> +
> + val = readl(chip->base + ACP_PIN_CONFIG);
> + switch (val) {
> + case ACP_CONFIG_4:
> + case ACP_CONFIG_5:
> + case ACP_CONFIG_6:
> + case ACP_CONFIG_7:
> + case ACP_CONFIG_8:
> + case ACP_CONFIG_11:
> case ACP_CONFIG_14:
> + case ACP_CONFIG_17:
> + case ACP_CONFIG_18:
> + chip->is_pdm_config = true;
> + break;
> + case ACP_CONFIG_9:
> + chip->is_i2s_config = true;
> + break;
> + case ACP_CONFIG_10:
> + case ACP_CONFIG_12:
> + case ACP_CONFIG_13:
> + case ACP_CONFIG_19:
> + case ACP_CONFIG_20:
> + chip->is_i2s_config = true;
> + chip->is_pdm_config = true;
> break;
> default:
> - return -EINVAL;
> + break;
> }
> +}
> +
> +void check_acp_config(struct pci_dev *pci, struct acp_chip_info *chip)
> +{
> + struct acpi_device *pdm_dev;
> + const union acpi_object *obj;
> + u32 pdm_addr;
>
> switch (chip->acp_rev) {
> case ACP3X_DEV:
> pdm_addr = ACP_RENOIR_PDM_ADDR;
> + check_acp3x_config(chip);
> break;
> case ACP6X_DEV:
> pdm_addr = ACP_REMBRANDT_PDM_ADDR;
> + check_acp6x_config(chip);
> break;
> case ACP63_DEV:
> pdm_addr = ACP63_PDM_ADDR;
> + check_acp6x_config(chip);
> break;
> case ACP70_DEV:
> pdm_addr = ACP70_PDM_ADDR;
> + check_acp70_config(chip);
> break;
> default:
> - return -EINVAL;
> + break;
> }
>
> - pdm_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), pdm_addr, 0);
> - if (pdm_dev) {
> - if (!acpi_dev_get_property(pdm_dev, "acp-audio-device-type",
> - ACPI_TYPE_INTEGER, &obj) &&
> - obj->integer.value == pdm_addr)
> - return 0;
> + if (chip->is_pdm_config) {
> + pdm_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), pdm_addr, 0);
> + if (pdm_dev) {
> + if (!acpi_dev_get_property(pdm_dev, "acp-audio-device-type",
> + ACPI_TYPE_INTEGER, &obj) &&
> + obj->integer.value == pdm_addr)
> + chip->is_pdm_dev = true;
> + }
> }
> - return -ENODEV;
> }
> -EXPORT_SYMBOL_NS_GPL(check_acp_pdm, SND_SOC_ACP_COMMON);
> +EXPORT_SYMBOL_NS_GPL(check_acp_config, SND_SOC_ACP_COMMON);
>
> MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/sound/soc/amd/acp/acp-pci.c b/sound/soc/amd/acp/acp-pci.c
> index 5f35b90eab8d..ad320b29e87d 100644
> --- a/sound/soc/amd/acp/acp-pci.c
> +++ b/sound/soc/amd/acp/acp-pci.c
> @@ -100,7 +100,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
> ret = -EINVAL;
> goto release_regions;
> }
> -
Empty line drop is not relevant to this patch.
Will fix and re-spin the patch series.
> 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");
> @@ -119,6 +118,10 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
> if (ret)
> goto unregister_dmic_dev;
>
> + check_acp_config(pci, chip);
> + if (!chip->is_pdm_dev && !chip->is_i2s_config)
> + goto skip_pdev_creation;
> +
> res = devm_kcalloc(&pci->dev, num_res, sizeof(struct resource), GFP_KERNEL);
> if (!res) {
> ret = -ENOMEM;
> @@ -136,10 +139,6 @@ static int acp_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id
> }
> }
>
> - ret = check_acp_pdm(pci, chip);
> - if (ret < 0)
> - goto skip_pdev_creation;
> -
> chip->flag = flag;
> memset(&pdevinfo, 0, sizeof(pdevinfo));
>
> diff --git a/sound/soc/amd/acp/amd.h b/sound/soc/amd/acp/amd.h
> index 5017e868f39b..d75b4eb34de8 100644
> --- a/sound/soc/amd/acp/amd.h
> +++ b/sound/soc/amd/acp/amd.h
> @@ -138,6 +138,9 @@ struct acp_chip_info {
> void __iomem *base; /* ACP memory PCI base */
> struct platform_device *chip_pdev;
> unsigned int flag; /* Distinguish b/w Legacy or Only PDM */
> + bool is_pdm_dev; /* flag set to true when ACP PDM controller exists */
> + bool is_pdm_config; /* flag set to true when PDM configuration is selected from BIOS */
> + bool is_i2s_config; /* flag set to true when I2S configuration is selected from BIOS */
> };
>
> struct acp_stream {
> @@ -212,6 +215,11 @@ enum acp_config {
> ACP_CONFIG_13,
> ACP_CONFIG_14,
> ACP_CONFIG_15,
> + ACP_CONFIG_16,
> + ACP_CONFIG_17,
> + ACP_CONFIG_18,
> + ACP_CONFIG_19,
> + ACP_CONFIG_20,
> };
>
> extern const struct snd_soc_dai_ops asoc_acp_cpu_dai_ops;
> @@ -240,7 +248,7 @@ void restore_acp_pdm_params(struct snd_pcm_substream *substream,
> int restore_acp_i2s_params(struct snd_pcm_substream *substream,
> struct acp_dev_data *adata, struct acp_stream *stream);
>
> -int check_acp_pdm(struct pci_dev *pci, struct acp_chip_info *chip);
> +void check_acp_config(struct pci_dev *pci, struct acp_chip_info *chip);
>
> static inline u64 acp_get_byte_count(struct acp_dev_data *adata, int dai_id, int direction)
> {
> diff --git a/sound/soc/amd/acp/chip_offset_byte.h b/sound/soc/amd/acp/chip_offset_byte.h
> index cfd6c4d07594..18da734c0e9e 100644
> --- a/sound/soc/amd/acp/chip_offset_byte.h
> +++ b/sound/soc/amd/acp/chip_offset_byte.h
> @@ -20,6 +20,7 @@
> #define ACP_SOFT_RESET 0x1000
> #define ACP_CONTROL 0x1004
> #define ACP_PIN_CONFIG 0x1440
> +#define ACP3X_PIN_CONFIG 0x1400
>
> #define ACP_EXTERNAL_INTR_REG_ADDR(adata, offset, ctrl) \
> (adata->acp_base + adata->rsrc->irq_reg_offset + offset + (ctrl * 0x04))

Next message: syzbot: "Re: [syzbot] Re: [syzbot] [f2fs?] KASAN: slab-use-after-free Read in sanity_check_extent_cache"
Previous message: Sebastian Ott: "Re: [PATCH v2 5/6] KVM: arm64: show writable masks for feature registers"
In reply to: Vijendar Mukunda: "[PATCH 2/2] ASoC: amd: acp: move chip->flag variable assignment"
Next in thread: Mark Brown: "Re: [PATCH 1/2] ASoC: amd: acp: fix for acp platform device creation failure"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]