Re: [PATCH 09/11] ASoC: SOF: amd: Compute file paths on firmware load
From: Cristian Ciocaltea
Date: Sun Dec 10 2023 - 04:06:37 EST
On 12/10/23 05:50, Venkata Prasad Potturu wrote:
>
> On 12/10/23 02:23, Cristian Ciocaltea wrote:
>> Commit 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback if
>> firmware files are missing") changed the order of some operations and
>> the firmware paths are not available anymore at snd_sof_probe() time.
>>
>> Precisely, fw_filename_prefix is set by sof_select_ipc_and_paths() via
>>
>> plat_data->fw_filename_prefix = out_profile.fw_path;
>>
>> but sof_init_environment() which calls this function was moved from
>> snd_sof_device_probe() to sof_probe_continue(). Moreover,
>> snd_sof_probe() was moved from sof_probe_continue() to
>> sof_init_environment(), but before the call to
>> sof_select_ipc_and_paths().
>>
>> The problem here is that amd_sof_acp_probe() uses fw_filename_prefix to
>> compute fw_code_bin and fw_data_bin paths, and because the field is not
>> yet initialized, the paths end up containing (null):
>>
>> snd_sof_amd_vangogh 0000:04:00.5: Direct firmware load for
>> (null)/sof-vangogh-code.bin failed with error -2
>> snd_sof_amd_vangogh 0000:04:00.5: sof signed firmware code bin is missing
>> snd_sof_amd_vangogh 0000:04:00.5: error: failed to load DSP firmware -2
>> snd_sof_amd_vangogh: probe of 0000:04:00.5 failed with error -2
>>
>> Move usage of fw_filename_prefix right before request_firmware() calls
>> in acp_sof_load_signed_firmware().
>>
>> Fixes: 6c393ebbd74a ("ASoC: SOF: core: Implement IPC version fallback
>> if firmware files are missing")
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>
>> ---
>> sound/soc/sof/amd/acp-loader.c | 32 ++++++++++++++++++++++++++------
>> sound/soc/sof/amd/acp.c | 7 ++-----
>> 2 files changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/soc/sof/amd/acp-loader.c
>> b/sound/soc/sof/amd/acp-loader.c
>> index e05eb7a86dd4..d2d21478399e 100644
>> --- a/sound/soc/sof/amd/acp-loader.c
>> +++ b/sound/soc/sof/amd/acp-loader.c
>> @@ -267,29 +267,49 @@ int acp_sof_load_signed_firmware(struct
>> snd_sof_dev *sdev)
>> {
>> struct snd_sof_pdata *plat_data = sdev->pdata;
>> struct acp_dev_data *adata = plat_data->hw_pdata;
>> + const char *fw_filename;
>> int ret;
>> - ret = request_firmware(&sdev->basefw.fw, adata->fw_code_bin,
>> sdev->dev);
>> + fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
>> + plat_data->fw_filename_prefix,
>> + adata->fw_code_bin);
> File path already saved in adata->fw_code_bin in amd_sof_acp_probe
> function.
> No need to get it again.
As already stated in the patch description, fw_filename_prefix is not
available anymore when amd_sof_acp_probe() gets invoked, and that is the
root cause of ending up with (null) in the computed file path.
Hence, the patch ensures amd_sof_acp_probe() computes the file name,
without prefix, while acp_sof_load_signed_firmware() adds the prefix.
>> + if (!fw_filename)
>> + return -ENOMEM;
>> +
>> + ret = request_firmware(&sdev->basefw.fw, fw_filename, sdev->dev);
>> if (ret < 0) {
>> + kfree(fw_filename);
>> dev_err(sdev->dev, "sof signed firmware code bin is
>> missing\n");
>> return ret;
>> } else {
>> - dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> adata->fw_code_bin);
>> + dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> fw_filename);
>> }
>> + kfree(fw_filename);
>> +
>> ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_IRAM, 0,
>> - (void *)sdev->basefw.fw->data,
>> sdev->basefw.fw->size);
>> + (void *)sdev->basefw.fw->data,
>> + sdev->basefw.fw->size);
>> +
>> + fw_filename = kasprintf(GFP_KERNEL, "%s/%s",
>> + plat_data->fw_filename_prefix,
>> + adata->fw_data_bin);
>> + if (!fw_filename)
>> + return -ENOMEM;
>> - ret = request_firmware(&adata->fw_dbin, adata->fw_data_bin,
>> sdev->dev);
>> + ret = request_firmware(&adata->fw_dbin, fw_filename, sdev->dev);
>> if (ret < 0) {
>> + kfree(fw_filename);
>> dev_err(sdev->dev, "sof signed firmware data bin is
>> missing\n");
>> return ret;
>> } else {
>> - dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> adata->fw_data_bin);
>> + dev_dbg(sdev->dev, "request_firmware %s successful\n",
>> fw_filename);
>> }
>> + kfree(fw_filename);
>> ret = snd_sof_dsp_block_write(sdev, SOF_FW_BLK_TYPE_DRAM, 0,
>> - (void *)adata->fw_dbin->data,
>> adata->fw_dbin->size);
>> + (void *)adata->fw_dbin->data,
>> + adata->fw_dbin->size);
>> return ret;
>> }
>> EXPORT_SYMBOL_NS(acp_sof_load_signed_firmware, SND_SOC_SOF_AMD_COMMON);
>> diff --git a/sound/soc/sof/amd/acp.c b/sound/soc/sof/amd/acp.c
>> index 1e9840ae8938..87c5c71eac68 100644
>> --- a/sound/soc/sof/amd/acp.c
>> +++ b/sound/soc/sof/amd/acp.c
>> @@ -479,7 +479,6 @@ EXPORT_SYMBOL_NS(amd_sof_acp_resume,
>> SND_SOC_SOF_AMD_COMMON);
>> int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>> {
>> struct pci_dev *pci = to_pci_dev(sdev->dev);
>> - struct snd_sof_pdata *plat_data = sdev->pdata;
>> struct acp_dev_data *adata;
>> const struct sof_amd_acp_desc *chip;
>> const struct dmi_system_id *dmi_id;
>> @@ -547,8 +546,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>> dmi_id = dmi_first_match(acp_sof_quirk_table);
>> if (dmi_id && dmi_id->driver_data) {
>> adata->fw_code_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
>> - "%s/sof-%s-code.bin",
>> - plat_data->fw_filename_prefix,
>> + "sof-%s-code.bin",
>> chip->name);
>> if (!adata->fw_code_bin) {
>> ret = -ENOMEM;
>> @@ -556,8 +554,7 @@ int amd_sof_acp_probe(struct snd_sof_dev *sdev)
>> }
>> adata->fw_data_bin = devm_kasprintf(sdev->dev, GFP_KERNEL,
>> - "%s/sof-%s-data.bin",
>> - plat_data->fw_filename_prefix,
>> + "sof-%s-data.bin",
>> chip->name);
>> if (!adata->fw_data_bin) {
>> ret = -ENOMEM;