| Message ID | 20231209205351.880797-10-cristian.ciocaltea@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve SOF support for Steam Deck OLED | expand |
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. > + 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;
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;
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); + 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;
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(-)