Skip to content

[TEST] ASoC: soc-component: make sure device is resumed before probe/remove#3664

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
plbossart wants to merge 1 commit into from
Closed

[TEST] ASoC: soc-component: make sure device is resumed before probe/remove #3664

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
19 changes: 18 additions & 1 deletion sound/soc/soc-component.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -339,18 +339,35 @@ int snd_soc_component_is_suspended(struct snd_soc_component *component)

int snd_soc_component_probe(struct snd_soc_component *component)
{
int ret = 0;
int ret;

ret = pm_runtime_resume_and_get(component->dev);
if (ret < 0 && ret != -EACCES)
return soc_component_ret(component, ret);

ret = 0;
if (component->driver->probe)
ret = component->driver->probe(component);

pm_runtime_mark_last_busy(component->dev);
pm_runtime_put_autosuspend(component->dev);

@ranj063ranj063May 20, 2022

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.

@plbossart what happens if you use pm_runtime_put_noidle() instead of pm_runtime_put_autosuspend() here?

@plbossartplbossartMay 20, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@ranj063 that would drop the refcount but not undo the resume. put_noidle is usually called in error scenarios, this wouldn't be a symmetrical call.

This doesn't change anything in my local tests, I think it's the resume part that's problematic.

That's my life right there, a seemingly logical and innocuous change that breaks the whole stack. Gah.

@ranj063ranj063May 20, 2022

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.

@plbossart I tested pm_runtime_get_sync() instead of pm_runtime_resume_and_get() and I no longer have the xrun:

diff --git a/sound/soc/soc-component.c b/sound/soc/soc-component.c
index e12f8244242b..fd50c2f5a0c3 100644
--- a/sound/soc/soc-component.c
+++ b/sound/soc/soc-component.c
@@ -339,18 +339,35 @@ int snd_soc_component_is_suspended(struct snd_soc_component *component)
int snd_soc_component_probe(struct snd_soc_component *component)
{
- int ret = 0;
+ int ret;
+ ret = pm_runtime_get_sync(component->dev);
+ if (ret < 0 && ret != -EACCES)
+ return soc_component_ret(component, ret);
+
+ ret = 0;
if (component->driver->probe)
ret = component->driver->probe(component);
+ pm_runtime_mark_last_busy(component->dev);
+ pm_runtime_put_autosuspend(component->dev);
+
return soc_component_ret(component, ret);
}
void snd_soc_component_remove(struct snd_soc_component *component)
{
+ int ret;
+
+ ret = pm_runtime_get_sync(component->dev);
+ if (ret < 0 && ret != -EACCES)
+ soc_component_ret(component, ret);
+
if (component->driver->remove)
component->driver->remove(component);
+
+ pm_runtime_mark_last_busy(component->dev);
+ pm_runtime_put_autosuspend(component->dev);
}
int snd_soc_component_of_xlate_dai_id(struct snd_soc_component *component,

@ranj063ranj063May 20, 2022

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 don't know how to explain this though

@plbossartplbossartMay 20, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

the sequence pm_runtime_get_noresume/put_noidle works. So logically it's the resume part that's problematic.

It's possible though that the card creation happens BEFORE the pci device is full probed and initialized. There's a race condition of some sort here.

@ranj063ranj063May 20, 2022•
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.

But how is that possible though? The machine driver is registered towards the end of the pci device probe isnt it? The difference between get_sync() and resume_and_get() is quite a mystery. In theory they do the same thing

@ranj063ranj063May 20, 2022•
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.

ahh I think I know the problem @plbossart. There's a small but important difference between runtime_get_sync() and runtime_resume_and_get(). If runtime_resume_and_get() returns a -EACCES, we do a runtime_put_noidle() before returning 0 whereas with runtime_get_sync(), we will not do the put in case of -EACCES

@ranj063ranj063May 20, 2022

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.

this now makes me question if the switch to use of runtime_resume_and_get() in the SOF driver was wrong.

@plbossartplbossartMay 20, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

if I resume the SOF component only, things work fine. It's the codec driver component that's problematic, which means we have to resume during component probe only if needed, on a component by component basis. This cannot be an ASoC-level change, too invasive and maybe not efficient to resume if not needed.

@plbossartplbossartMay 20, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

  •  ret = pm_runtime_get_sync(component->dev);

  •  if (ret < 0 && ret != -EACCES)

  •  return soc_component_ret(component, ret);

That code is wrong @ranj063, if there is an error different from -EACCES the reference count is not decreased.


return soc_component_ret(component, ret);
}

void snd_soc_component_remove(struct snd_soc_component *component)
{
int ret;

ret = pm_runtime_resume_and_get(component->dev);
if (ret < 0 && ret != -EACCES)
soc_component_ret(component, ret);

if (component->driver->remove)
component->driver->remove(component);

pm_runtime_mark_last_busy(component->dev);
pm_runtime_put_autosuspend(component->dev);
}

int snd_soc_component_of_xlate_dai_id(struct snd_soc_component *component,
Expand Down