Skip to content

ASoC/soundwire: Intel: avoid transfer timed out in module unload #4833

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

Closed
bardliao wants to merge 2 commits into from
Closed

ASoC/soundwire: Intel: avoid transfer timed out in module unload #4833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
339e840
ASoC: Intel: disable hda sdw int after sdw_intel_exit
bardliao Feb 22, 2024
9fc1dfe
soundwire: intel_init: Disable pm runtime before removing link device
bardliao Feb 22, 2024
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
8 changes: 8 additions & 0 deletions drivers/soundwire/intel_init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ static void intel_link_dev_unregister(struct sdw_intel_link_dev *ldev)
static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
{
struct sdw_intel_link_dev *ldev;
struct sdw_slave *slave;
u32 link_mask;
int i;

Expand All @@ -132,6 +133,13 @@ static int sdw_intel_cleanup(struct sdw_intel_ctx *ctx)
if (!ldev->link_res.clock_stop_quirks)
pm_runtime_put_noidle(ldev->link_res.dev);

/*
* Disable runtime PM to prevent peripherals from being resume in the
* link device unregistering process.
*/
list_for_each_entry(slave, &ldev->link_res.cdns->bus.slaves, node)
pm_runtime_disable(&slave->dev);

@plbossart plbossart Feb 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this really needed? We already disable pm_runtime in codec drivers, e.g.

linux/sound/soc/codecs/rt711-sdca-sdw.c

Line 360 in 59c5a8d

static int rt711_sdca_sdw_remove(struct sdw_slave *slave)

@bardliao bardliao Feb 23, 2024

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.

is this really needed? We already disable pm_runtime in codec drivers, e.g.

linux/sound/soc/codecs/rt711-sdca-sdw.c

Line 360 in 59c5a8d

static int rt711_sdca_sdw_remove(struct sdw_slave *slave)

The resume runtime is called before codec drivers being removed. So, disabling it in the .remove function is too late.

@plbossart plbossart Feb 24, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bardliao alternatively you could do something like we do in the .prepare step:

linux/drivers/soundwire/intel_auxdevice.c

Line 517 in e915e8b

ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);

But we would need a pm_runtime_resume() which would wait for the devices to be fully resumed (I don't recall why we didn't do this for .prepare).

@bardliao bardliao Feb 26, 2024

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.

@bardliao alternatively you could do something like we do in the .prepare step:

linux/drivers/soundwire/intel_auxdevice.c

Line 517 in e915e8b

ret = device_for_each_child(bus->dev, NULL, intel_resume_child_device);

But we would need a pm_runtime_resume() which would wait for the devices to be fully resumed (I don't recall why we didn't do this for .prepare).

Let me study this.

intel_link_dev_unregister(ldev);
}

Expand Down
3 changes: 1 addition & 2 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,12 +220,11 @@ static int hda_sdw_exit(struct snd_sof_dev *sdev)

hdev = sdev->pdata->hw_pdata;

hda_sdw_int_enable(sdev, false);

if (hdev->sdw)
sdw_intel_exit(hdev->sdw);
hdev->sdw = NULL;

hda_sdw_int_enable(sdev, false);

@plbossart plbossart Feb 22, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can we give an example of when "Codecs may need to access sdw registers via sdw bus in the exit process".

I don't fully buy the argument. IIRC the initial code was intentional so that we wouldn't have ANY interrupts while removing the devices.

@bardliao bardliao Feb 23, 2024

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.

can we give an example of when "Codecs may need to access sdw registers via sdw bus in the exit process".

When we run sudo modprobe -r snd_sof_pci_intel_lnl, snd_sof_runtime_resume will be trigger first. And hda_sdw_process_wakeen will be called when dsp boot. Due to intel_shim_check_wake return 1 unconditionally, soundwire_intel.links will be powered up. And the codecs will be attached.
SCP Msg trf timed out will happen if hda_sdw_exit is called before codec is initialized, and SDW interrupt is disabled.

Please see the log in detail.
lnl_remove.txt

I don't fully buy the argument. IIRC the initial code was intentional so that we wouldn't have ANY interrupts while removing the devices.

I agree that we don't want any interrupts while removing the devices. However, the interrupts are also used in sdw commend transfer. Can we cancel the sdw commend transfer before disabling SDW interrupt?

@bardliao bardliao Feb 23, 2024

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.

@plbossart Alternative solution #4838. Instead of canceling the sdw commend, #4838 wait for them be finished.

return 0;
}

Expand Down