Skip to content

ASoC: SOF: loader: release_firmware() on failures to avoid batching #2954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 1 commit into from
Jul 7, 2021
Merged

ASoC: SOF: loader: release_firmware() on failures to avoid batching #2954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions sound/soc/sof/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,6 @@ int snd_sof_device_remove(struct device *dev)
dev_warn(dev, "error: %d failed to prepare DSP for device removal",
ret);

snd_sof_fw_unload(sdev);
snd_sof_ipc_free(sdev);
snd_sof_free_debug(sdev);
snd_sof_free_trace(sdev);
Expand All @@ -394,8 +393,7 @@ int snd_sof_device_remove(struct device *dev)
snd_sof_remove(sdev);

/* release firmware */
release_firmware(pdata->fw);
pdata->fw = NULL;
snd_sof_fw_unload(sdev);

return 0;
}
Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/loader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -855,5 +855,7 @@ EXPORT_SYMBOL(snd_sof_run_firmware);
void snd_sof_fw_unload(struct snd_sof_dev *sdev)
{
/* TODO: support module unloading at runtime */
release_firmware(sdev->pdata->fw);
sdev->pdata->fw = NULL;

@ranj063 ranj063 Jul 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in terms of what it achieves today , this is fine. But when and if we do start supporting module unload at runtime, won't this need to be moved before snd_sof_remove()?

@marc-hb marc-hb Jul 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 I don't understand your comment sorry. Do you mean release_firmware() should be invoked before snd_sof_remove() in the future? I don't see why. As @ujfalusi wrote, the only reason we need the struct firmware again is to resume after suspend (especially on platforms without IMR resume), even for that it shouldn't be needed eventually. So releasing struct firmware before or after some releasing other things does not matter, does it? Of course the sooner the better but I don't see any required order or dependency.

I asked the compiler to list all places where we use struct firmware, here is the complete list as of commit 132b2ee

- loader.c
snd_sof_load_firmware_raw()
snd_sof_load_firmware_memcpy()
snd_sof_device_remove() or snd_sof_fw_unload() # this PR
- hda-loader.c
hda_dsp_cl_boot_firmware()
hda_dsp_cl_boot_firmware_iccmax()

It's not used anywhere else with my current .config.

@ujfalusi ujfalusi Jul 5, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the order of release_firmware() would matter, in snd_sof_device_remove() the firmware is not needed, we could have drop it first thing or last thing. It does not really matter.

@ranj063, what does "module unload at runtime" means? Do you want to be able to remove the modules while audio is playing or something else?
Are we preparing for hot-plugable DSPs?

@ranj063 ranj063 Jul 7, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from what I understood, I think runtime module unload means you load/unload FW modules based on topology. But I could be wrong. And in any cae, we're a bit far from supporting this at the moment. So this PR is good to go for now.

}
EXPORT_SYMBOL(snd_sof_fw_unload);