| Message ID | 20230720133147.1294337-11-sbinding@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix support for System Suspend for CS35L41 HDA | expand |
On Thu, 20 Jul 2023 15:31:46 +0200, Stefan Binding wrote: > > To ensure consistency between the HDA core and the CS35L41 HDA > driver, add a device_link between them. This ensures that the > HDA core will suspend first, and resume second, meaning the > amp driver will not miss any events from the playback hook from > the HDA core during system suspend and resume. > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > --- > sound/pci/hda/cs35l41_hda.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c > index 70aa819cfbd64..175378cdf9dfa 100644 > --- a/sound/pci/hda/cs35l41_hda.c > +++ b/sound/pci/hda/cs35l41_hda.c > @@ -1063,6 +1063,7 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas > { > struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > struct hda_component *comps = master_data; > + unsigned int sleep_flags; > int ret = 0; > > if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS) > @@ -1102,6 +1103,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas > > mutex_unlock(&cs35l41->fw_mutex); > > + sleep_flags = lock_system_sleep(); > + if (!device_link_add(&comps->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) > + dev_warn(dev, "Unable to create device link\n"); > + unlock_system_sleep(sleep_flags); Is lock_system_sleep() mandatory for device_link_add()? The function takes its rw lock for the race, I suppose. thanks, Takashi
On 20/07/2023 15:21, Takashi Iwai wrote: > On Thu, 20 Jul 2023 15:31:46 +0200, > Stefan Binding wrote: >> To ensure consistency between the HDA core and the CS35L41 HDA >> driver, add a device_link between them. This ensures that the >> HDA core will suspend first, and resume second, meaning the >> amp driver will not miss any events from the playback hook from >> the HDA core during system suspend and resume. >> >> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> >> --- >> sound/pci/hda/cs35l41_hda.c | 13 ++++++++++++- >> 1 file changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c >> index 70aa819cfbd64..175378cdf9dfa 100644 >> --- a/sound/pci/hda/cs35l41_hda.c >> +++ b/sound/pci/hda/cs35l41_hda.c >> @@ -1063,6 +1063,7 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas >> { >> struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); >> struct hda_component *comps = master_data; >> + unsigned int sleep_flags; >> int ret = 0; >> >> if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS) >> @@ -1102,6 +1103,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas >> >> mutex_unlock(&cs35l41->fw_mutex); >> >> + sleep_flags = lock_system_sleep(); >> + if (!device_link_add(&comps->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) >> + dev_warn(dev, "Unable to create device link\n"); >> + unlock_system_sleep(sleep_flags); > Is lock_system_sleep() mandatory for device_link_add()? The function > takes its rw lock for the race, I suppose. I believe this is mandatory, to ensure we don't try to add the device link during a suspend/resume transition. Its probably unlikely that that would occur during the component bind, but just in case. Thanks, Stefan > > > thanks, > > Takashi
On Fri, 21 Jul 2023 16:55:43 +0200, Stefan Binding wrote: > > > On 20/07/2023 15:21, Takashi Iwai wrote: > > On Thu, 20 Jul 2023 15:31:46 +0200, > > Stefan Binding wrote: > >> To ensure consistency between the HDA core and the CS35L41 HDA > >> driver, add a device_link between them. This ensures that the > >> HDA core will suspend first, and resume second, meaning the > >> amp driver will not miss any events from the playback hook from > >> the HDA core during system suspend and resume. > >> > >> Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > >> --- > >> sound/pci/hda/cs35l41_hda.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c > >> index 70aa819cfbd64..175378cdf9dfa 100644 > >> --- a/sound/pci/hda/cs35l41_hda.c > >> +++ b/sound/pci/hda/cs35l41_hda.c > >> @@ -1063,6 +1063,7 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas > >> { > >> struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); > >> struct hda_component *comps = master_data; > >> + unsigned int sleep_flags; > >> int ret = 0; > >> if (!comps || cs35l41->index < 0 || cs35l41->index >= > >> HDA_MAX_COMPONENTS) > >> @@ -1102,6 +1103,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas > >> mutex_unlock(&cs35l41->fw_mutex); > >> + sleep_flags = lock_system_sleep(); > >> + if (!device_link_add(&comps->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) > >> + dev_warn(dev, "Unable to create device link\n"); > >> + unlock_system_sleep(sleep_flags); > > Is lock_system_sleep() mandatory for device_link_add()? The function > > takes its rw lock for the race, I suppose. > > I believe this is mandatory, to ensure we don't try to add the device > link during a suspend/resume transition. > Its probably unlikely that that would occur during the component bind, > but just in case. OK, it seems needed in this case. I found the description in device_link.rst, too. thanks, Takashi
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 70aa819cfbd64..175378cdf9dfa 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -1063,6 +1063,7 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); struct hda_component *comps = master_data; + unsigned int sleep_flags; int ret = 0; if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS) @@ -1102,6 +1103,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas mutex_unlock(&cs35l41->fw_mutex); + sleep_flags = lock_system_sleep(); + if (!device_link_add(&comps->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) + dev_warn(dev, "Unable to create device link\n"); + unlock_system_sleep(sleep_flags); + pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); @@ -1112,9 +1118,14 @@ static void cs35l41_hda_unbind(struct device *dev, struct device *master, void * { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); struct hda_component *comps = master_data; + unsigned int sleep_flags; - if (comps[cs35l41->index].dev == dev) + if (comps[cs35l41->index].dev == dev) { memset(&comps[cs35l41->index], 0, sizeof(*comps)); + sleep_flags = lock_system_sleep(); + device_link_remove(&comps->codec->core.dev, cs35l41->dev); + unlock_system_sleep(sleep_flags); + } } static const struct component_ops cs35l41_hda_comp_ops = {
To ensure consistency between the HDA core and the CS35L41 HDA driver, add a device_link between them. This ensures that the HDA core will suspend first, and resume second, meaning the amp driver will not miss any events from the playback hook from the HDA core during system suspend and resume. Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> --- sound/pci/hda/cs35l41_hda.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)