Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Message-ID: <4d6760c3-6931-4ab3-bda7-408801ad77e1@collabora.com>
Date:   Sun, 10 Dec 2023 10:56:51 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 09/11] ASoC: SOF: amd: Compute file paths on firmware load
Content-Language: en-US
To:     Venkata Prasad Potturu <venkataprasad.potturu@amd.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Peter Ujfalusi <peter.ujfalusi@linux.intel.com>,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
        Daniel Baluta <daniel.baluta@nxp.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Alper Nebi Yasak <alpernebiyasak@gmail.com>,
        Syed Saba Kareem <Syed.SabaKareem@amd.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        Marian Postevca <posteuca@mutex.one>,
        Vijendar Mukunda <Vijendar.Mukunda@amd.com>,
        V sujith kumar Reddy <Vsujithkumar.Reddy@amd.com>,
        Mastan Katragadda <Mastan.Katragadda@amd.com>,
        Ajit Kumar Pandey <AjitKumar.Pandey@amd.com>
Cc:     linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org,
        sound-open-firmware@alsa-project.org, kernel@collabora.com
References: <20231209205351.880797-1-cristian.ciocaltea@collabora.com>
 <20231209205351.880797-10-cristian.ciocaltea@collabora.com>
 <6e25398f-9a3d-4cad-a66a-ebe43a723843@amd.com>
From:   Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
In-Reply-To: <6e25398f-9a3d-4cad-a66a-ebe43a723843@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@collabora.com>
>> ---
>>   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;