[PATCH v2 0/7] ALSA: hda: Improvements to hda_component
This series of patches makes sure that the existing consumers of the infrastructure unbind their interface before they begin destroying the driver, moves duplicated members from the instanced component structures into a new parent structure and introduces locking so that consumers of the interface do not use stale data.
v2: - Correct application of sizeof to pointer flagged by kernel test robot
Simon Trimmer (7): ALSA: hda: cs35l56: Component should be unbound before deconstruction ALSA: hda: cs35l41: Component should be unbound before deconstruction ALSA: hda/tas2781: Component should be unbound before deconstruction ALSA: hda: hda_component: Introduce component parent structure ALSA: hda: hda_component: Change codecs to use component parent structure ALSA: hda: hda_component: Move codec field into the parent ALSA: hda: hda_component: Protect shared data with a mutex
sound/pci/hda/cs35l41_hda.c | 47 ++++++++++++--------- sound/pci/hda/cs35l56_hda.c | 29 +++++++------ sound/pci/hda/hda_component.c | 75 ++++++++++++++++++++------------- sound/pci/hda/hda_component.h | 48 ++++++++++++++------- sound/pci/hda/patch_realtek.c | 17 ++++---- sound/pci/hda/tas2781_hda_i2c.c | 37 ++++++++-------- 6 files changed, 147 insertions(+), 106 deletions(-)
The interface associated with the hda_component should be deactivated before the driver is deconstructed during removal.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/cs35l56_hda.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index 0923e2589f5f..e134ede6c5aa 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -1077,12 +1077,12 @@ void cs35l56_hda_remove(struct device *dev) { struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev);
+ component_del(cs35l56->base.dev, &cs35l56_hda_comp_ops); + pm_runtime_dont_use_autosuspend(cs35l56->base.dev); pm_runtime_get_sync(cs35l56->base.dev); pm_runtime_disable(cs35l56->base.dev);
- component_del(cs35l56->base.dev, &cs35l56_hda_comp_ops); - cs_dsp_remove(&cs35l56->cs_dsp);
kfree(cs35l56->system_name);
The interface associated with the hda_component should be deactivated before the driver is deconstructed during removal.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/cs35l41_hda.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index d54d4d60b03e..031703f010be 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -2019,6 +2019,8 @@ void cs35l41_hda_remove(struct device *dev) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev);
+ component_del(cs35l41->dev, &cs35l41_hda_comp_ops); + pm_runtime_get_sync(cs35l41->dev); pm_runtime_dont_use_autosuspend(cs35l41->dev); pm_runtime_disable(cs35l41->dev); @@ -2026,8 +2028,6 @@ void cs35l41_hda_remove(struct device *dev) if (cs35l41->halo_initialized) cs35l41_remove_dsp(cs35l41);
- component_del(cs35l41->dev, &cs35l41_hda_comp_ops); - acpi_dev_put(cs35l41->dacpi);
pm_runtime_put_noidle(cs35l41->dev);
The interface associated with the hda_component should be deactivated before the driver is deconstructed during removal.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/tas2781_hda_i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 75f7674c66ee..fdee6592c502 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -777,11 +777,11 @@ static void tas2781_hda_remove(struct device *dev) { struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
+ component_del(tas_hda->dev, &tas2781_hda_comp_ops); + pm_runtime_get_sync(tas_hda->dev); pm_runtime_disable(tas_hda->dev);
- component_del(tas_hda->dev, &tas2781_hda_comp_ops); - pm_runtime_put_noidle(tas_hda->dev);
tasdevice_remove(tas_hda->priv);
In preparation for moving duplicated members from the hda_component structure introduce a parent structure that wraps the array of components. This also allows us to confine the knowledge of the maximum number of entries to the hda_component files and eliminate passing that redundant information around and making direct accesses to the array.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/hda_component.c | 65 +++++++++++++++++++---------------- sound/pci/hda/hda_component.h | 42 ++++++++++++++-------- sound/pci/hda/patch_realtek.c | 17 +++++---- 3 files changed, 71 insertions(+), 53 deletions(-)
diff --git a/sound/pci/hda/hda_component.c b/sound/pci/hda/hda_component.c index d02589014a3f..b05a0b87d32a 100644 --- a/sound/pci/hda/hda_component.c +++ b/sound/pci/hda/hda_component.c @@ -15,35 +15,39 @@ #include "hda_local.h"
#ifdef CONFIG_ACPI -void hda_component_acpi_device_notify(struct hda_component *comps, int num_comps, +void hda_component_acpi_device_notify(struct hda_component_parent *parent, acpi_handle handle, u32 event, void *data) { + struct hda_component *comp; int i;
- for (i = 0; i < num_comps; i++) { - if (comps[i].dev && comps[i].acpi_notify) - comps[i].acpi_notify(acpi_device_handle(comps[i].adev), event, - comps[i].dev); + for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { + comp = hda_component_from_index(parent, i); + if (comp->dev && comp->acpi_notify) + comp->acpi_notify(acpi_device_handle(comp->adev), event, comp->dev); } } EXPORT_SYMBOL_NS_GPL(hda_component_acpi_device_notify, SND_HDA_SCODEC_COMPONENT);
int hda_component_manager_bind_acpi_notifications(struct hda_codec *cdc, - struct hda_component *comps, int num_comps, + struct hda_component_parent *parent, acpi_notify_handler handler, void *data) { bool support_notifications = false; struct acpi_device *adev; + struct hda_component *comp; int ret; int i;
- adev = comps[0].adev; + adev = parent->comps[0].adev; if (!acpi_device_handle(adev)) return 0;
- for (i = 0; i < num_comps; i++) + for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { + comp = hda_component_from_index(parent, i); support_notifications = support_notifications || - comps[i].acpi_notifications_supported; + comp->acpi_notifications_supported; + }
if (support_notifications) { ret = acpi_install_notify_handler(adev->handle, ACPI_DEVICE_NOTIFY, @@ -61,13 +65,13 @@ int hda_component_manager_bind_acpi_notifications(struct hda_codec *cdc, EXPORT_SYMBOL_NS_GPL(hda_component_manager_bind_acpi_notifications, SND_HDA_SCODEC_COMPONENT);
void hda_component_manager_unbind_acpi_notifications(struct hda_codec *cdc, - struct hda_component *comps, + struct hda_component_parent *parent, acpi_notify_handler handler) { struct acpi_device *adev; int ret;
- adev = comps[0].adev; + adev = parent->comps[0].adev; if (!acpi_device_handle(adev)) return;
@@ -78,21 +82,25 @@ void hda_component_manager_unbind_acpi_notifications(struct hda_codec *cdc, EXPORT_SYMBOL_NS_GPL(hda_component_manager_unbind_acpi_notifications, SND_HDA_SCODEC_COMPONENT); #endif /* ifdef CONFIG_ACPI */
-void hda_component_manager_playback_hook(struct hda_component *comps, int num_comps, int action) +void hda_component_manager_playback_hook(struct hda_component_parent *parent, int action) { + struct hda_component *comp; int i;
- for (i = 0; i < num_comps; i++) { - if (comps[i].dev && comps[i].pre_playback_hook) - comps[i].pre_playback_hook(comps[i].dev, action); + for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { + comp = hda_component_from_index(parent, i); + if (comp->dev && comp->pre_playback_hook) + comp->pre_playback_hook(comp->dev, action); } - for (i = 0; i < num_comps; i++) { - if (comps[i].dev && comps[i].playback_hook) - comps[i].playback_hook(comps[i].dev, action); + for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { + comp = hda_component_from_index(parent, i); + if (comp->dev && comp->playback_hook) + comp->playback_hook(comp->dev, action); } - for (i = 0; i < num_comps; i++) { - if (comps[i].dev && comps[i].post_playback_hook) - comps[i].post_playback_hook(comps[i].dev, action); + for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { + comp = hda_component_from_index(parent, i); + if (comp->dev && comp->post_playback_hook) + comp->post_playback_hook(comp->dev, action); } } EXPORT_SYMBOL_NS_GPL(hda_component_manager_playback_hook, SND_HDA_SCODEC_COMPONENT); @@ -124,22 +132,21 @@ static int hda_comp_match_dev_name(struct device *dev, void *data) }
int hda_component_manager_bind(struct hda_codec *cdc, - struct hda_component *comps, int count) + struct hda_component_parent *parent) { int i;
- /* Init shared data */ - for (i = 0; i < count; ++i) { - memset(&comps[i], 0, sizeof(comps[i])); - comps[i].codec = cdc; - } + /* Init shared and component specific data */ + memset(parent, 0, sizeof(*parent)); + for (i = 0; i < ARRAY_SIZE(parent->comps); i++) + parent->comps[i].codec = cdc;
- return component_bind_all(hda_codec_dev(cdc), comps); + return component_bind_all(hda_codec_dev(cdc), &parent->comps); } EXPORT_SYMBOL_NS_GPL(hda_component_manager_bind, SND_HDA_SCODEC_COMPONENT);
int hda_component_manager_init(struct hda_codec *cdc, - struct hda_component *comps, int count, + struct hda_component_parent *parent, int count, const char *bus, const char *hid, const char *match_str, const struct component_master_ops *ops) diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h index c70b3de68ab2..a016f1b942a2 100644 --- a/sound/pci/hda/hda_component.h +++ b/sound/pci/hda/hda_component.h @@ -28,18 +28,21 @@ struct hda_component { void (*post_playback_hook)(struct device *dev, int action); };
+struct hda_component_parent { + struct hda_component comps[HDA_MAX_COMPONENTS]; +}; + #ifdef CONFIG_ACPI -void hda_component_acpi_device_notify(struct hda_component *comps, int num_comps, +void hda_component_acpi_device_notify(struct hda_component_parent *parent, acpi_handle handle, u32 event, void *data); int hda_component_manager_bind_acpi_notifications(struct hda_codec *cdc, - struct hda_component *comps, int num_comps, + struct hda_component_parent *parent, acpi_notify_handler handler, void *data); void hda_component_manager_unbind_acpi_notifications(struct hda_codec *cdc, - struct hda_component *comps, + struct hda_component_parent *parent, acpi_notify_handler handler); #else -static inline void hda_component_acpi_device_notify(struct hda_component *comps, - int num_comps, +static inline void hda_component_acpi_device_notify(struct hda_component_parent *parent, acpi_handle handle, u32 event, void *data) @@ -47,8 +50,7 @@ static inline void hda_component_acpi_device_notify(struct hda_component *comps, }
static inline int hda_component_manager_bind_acpi_notifications(struct hda_codec *cdc, - struct hda_component *comps, - int num_comps, + struct hda_component_parent *parent, acpi_notify_handler handler, void *data)
@@ -57,17 +59,16 @@ static inline int hda_component_manager_bind_acpi_notifications(struct hda_codec }
static inline void hda_component_manager_unbind_acpi_notifications(struct hda_codec *cdc, - struct hda_component *comps, + struct hda_component_parent *parent, acpi_notify_handler handler) { } #endif /* ifdef CONFIG_ACPI */
-void hda_component_manager_playback_hook(struct hda_component *comps, int num_comps, - int action); +void hda_component_manager_playback_hook(struct hda_component_parent *parent, int action);
int hda_component_manager_init(struct hda_codec *cdc, - struct hda_component *comps, int count, + struct hda_component_parent *parent, int count, const char *bus, const char *hid, const char *match_str, const struct component_master_ops *ops); @@ -75,13 +76,24 @@ int hda_component_manager_init(struct hda_codec *cdc, void hda_component_manager_free(struct hda_codec *cdc, const struct component_master_ops *ops);
-int hda_component_manager_bind(struct hda_codec *cdc, - struct hda_component *comps, int count); +int hda_component_manager_bind(struct hda_codec *cdc, struct hda_component_parent *parent); + +static inline struct hda_component *hda_component_from_index(struct hda_component_parent *parent, + int index) +{ + if (!parent) + return NULL; + + if (index < 0 || index >= ARRAY_SIZE(parent->comps)) + return NULL; + + return &parent->comps[index]; +}
static inline void hda_component_manager_unbind(struct hda_codec *cdc, - struct hda_component *comps) + struct hda_component_parent *parent) { - component_unbind_all(hda_codec_dev(cdc), comps); + component_unbind_all(hda_codec_dev(cdc), &parent->comps); }
#endif /* ifndef __HDA_COMPONENT_H__ */ diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index aa76d1c88589..ab9d13e7dcaa 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -131,7 +131,7 @@ struct alc_spec { u8 alc_mute_keycode_map[1];
/* component binding */ - struct hda_component comps[HDA_MAX_COMPONENTS]; + struct hda_component_parent comps; };
/* @@ -6789,8 +6789,7 @@ static void comp_acpi_device_notify(acpi_handle handle, u32 event, void *data)
codec_info(cdc, "ACPI Notification %d\n", event);
- hda_component_acpi_device_notify(spec->comps, ARRAY_SIZE(spec->comps), - handle, event, data); + hda_component_acpi_device_notify(&spec->comps, handle, event, data); }
static int comp_bind(struct device *dev) @@ -6799,12 +6798,12 @@ static int comp_bind(struct device *dev) struct alc_spec *spec = cdc->spec; int ret;
- ret = hda_component_manager_bind(cdc, spec->comps, ARRAY_SIZE(spec->comps)); + ret = hda_component_manager_bind(cdc, &spec->comps); if (ret) return ret;
return hda_component_manager_bind_acpi_notifications(cdc, - spec->comps, ARRAY_SIZE(spec->comps), + &spec->comps, comp_acpi_device_notify, cdc); }
@@ -6813,8 +6812,8 @@ static void comp_unbind(struct device *dev) struct hda_codec *cdc = dev_to_hda_codec(dev); struct alc_spec *spec = cdc->spec;
- hda_component_manager_unbind_acpi_notifications(cdc, spec->comps, comp_acpi_device_notify); - hda_component_manager_unbind(cdc, spec->comps); + hda_component_manager_unbind_acpi_notifications(cdc, &spec->comps, comp_acpi_device_notify); + hda_component_manager_unbind(cdc, &spec->comps); }
static const struct component_master_ops comp_master_ops = { @@ -6827,7 +6826,7 @@ static void comp_generic_playback_hook(struct hda_pcm_stream *hinfo, struct hda_ { struct alc_spec *spec = cdc->spec;
- hda_component_manager_playback_hook(spec->comps, ARRAY_SIZE(spec->comps), action); + hda_component_manager_playback_hook(&spec->comps, action); }
static void comp_generic_fixup(struct hda_codec *cdc, int action, const char *bus, @@ -6838,7 +6837,7 @@ static void comp_generic_fixup(struct hda_codec *cdc, int action, const char *bu
switch (action) { case HDA_FIXUP_ACT_PRE_PROBE: - ret = hda_component_manager_init(cdc, spec->comps, count, bus, hid, + ret = hda_component_manager_init(cdc, &spec->comps, count, bus, hid, match_str, &comp_master_ops); if (ret) return;
Change the hda_component binding APIs to pass the hds_component_parent as the parameter so the array of components can be abstracted.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/cs35l41_hda.c | 42 +++++++++++++++++++-------------- sound/pci/hda/cs35l56_hda.c | 25 +++++++++++--------- sound/pci/hda/hda_component.c | 2 +- sound/pci/hda/hda_component.h | 2 +- sound/pci/hda/tas2781_hda_i2c.c | 33 +++++++++++++------------- 5 files changed, 57 insertions(+), 47 deletions(-)
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index 031703f010be..ceba4f2c85f1 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -1419,27 +1419,28 @@ static void cs35l41_acpi_device_notify(acpi_handle handle, u32 event, struct dev static int cs35l41_hda_bind(struct device *dev, struct device *master, void *master_data) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); - struct hda_component *comps = master_data; + struct hda_component_parent *parent = master_data; + struct hda_component *comp; unsigned int sleep_flags; int ret = 0;
- if (!comps || cs35l41->index < 0 || cs35l41->index >= HDA_MAX_COMPONENTS) + comp = hda_component_from_index(parent, cs35l41->index); + if (!comp) return -EINVAL;
- comps = &comps[cs35l41->index]; - if (comps->dev) + if (comp->dev) return -EBUSY;
pm_runtime_get_sync(dev);
mutex_lock(&cs35l41->fw_mutex);
- comps->dev = dev; + comp->dev = dev; if (!cs35l41->acpi_subsystem_id) cs35l41->acpi_subsystem_id = kasprintf(GFP_KERNEL, "%.8x", - comps->codec->core.subsystem_id); - cs35l41->codec = comps->codec; - strscpy(comps->name, dev_name(dev), sizeof(comps->name)); + comp->codec->core.subsystem_id); + cs35l41->codec = comp->codec; + strscpy(comp->name, dev_name(dev), sizeof(comp->name));
cs35l41->firmware_type = HDA_CS_DSP_FW_SPK_PROT;
@@ -1454,13 +1455,13 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas
ret = cs35l41_create_controls(cs35l41);
- comps->playback_hook = cs35l41_hda_playback_hook; - comps->pre_playback_hook = cs35l41_hda_pre_playback_hook; - comps->post_playback_hook = cs35l41_hda_post_playback_hook; - comps->acpi_notify = cs35l41_acpi_device_notify; - comps->adev = cs35l41->dacpi; + comp->playback_hook = cs35l41_hda_playback_hook; + comp->pre_playback_hook = cs35l41_hda_pre_playback_hook; + comp->post_playback_hook = cs35l41_hda_post_playback_hook; + comp->acpi_notify = cs35l41_acpi_device_notify; + comp->adev = cs35l41->dacpi;
- comps->acpi_notifications_supported = cs35l41_dsm_supported(acpi_device_handle(comps->adev), + comp->acpi_notifications_supported = cs35l41_dsm_supported(acpi_device_handle(comp->adev), CS35L41_DSM_GET_MUTE);
cs35l41->mute_override = cs35l41_get_acpi_mute_state(cs35l41, @@ -1469,7 +1470,7 @@ 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)) + if (!device_link_add(&comp->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) dev_warn(dev, "Unable to create device link\n"); unlock_system_sleep(sleep_flags);
@@ -1489,14 +1490,19 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas static void cs35l41_hda_unbind(struct device *dev, struct device *master, void *master_data) { struct cs35l41_hda *cs35l41 = dev_get_drvdata(dev); - struct hda_component *comps = master_data; + struct hda_component_parent *parent = master_data; + struct hda_component *comp; unsigned int sleep_flags;
- if (comps[cs35l41->index].dev == dev) { - memset(&comps[cs35l41->index], 0, sizeof(*comps)); + comp = hda_component_from_index(parent, cs35l41->index); + if (!comp) + return; + + if (comp->dev == dev) { sleep_flags = lock_system_sleep(); device_link_remove(&cs35l41->codec->core.dev, cs35l41->dev); unlock_system_sleep(sleep_flags); + memset(comp, 0, sizeof(*comp)); } }
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index e134ede6c5aa..df4498c77171 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -685,20 +685,21 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56) static int cs35l56_hda_bind(struct device *dev, struct device *master, void *master_data) { struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev); - struct hda_component *comps = master_data; + struct hda_component_parent *parent = master_data; + struct hda_component *comp; int ret;
- if (!comps || cs35l56->index < 0 || cs35l56->index >= HDA_MAX_COMPONENTS) + comp = hda_component_from_index(parent, cs35l56->index); + if (!comp) return -EINVAL;
- comps = &comps[cs35l56->index]; - if (comps->dev) + if (comp->dev) return -EBUSY;
- comps->dev = dev; - cs35l56->codec = comps->codec; - strscpy(comps->name, dev_name(dev), sizeof(comps->name)); - comps->playback_hook = cs35l56_hda_playback_hook; + comp->dev = dev; + cs35l56->codec = comp->codec; + strscpy(comp->name, dev_name(dev), sizeof(comp->name)); + comp->playback_hook = cs35l56_hda_playback_hook;
ret = cs35l56_hda_fw_load(cs35l56); if (ret) @@ -720,7 +721,8 @@ static int cs35l56_hda_bind(struct device *dev, struct device *master, void *mas static void cs35l56_hda_unbind(struct device *dev, struct device *master, void *master_data) { struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev); - struct hda_component *comps = master_data; + struct hda_component_parent *parent = master_data; + struct hda_component *comp;
cs35l56_hda_remove_controls(cs35l56);
@@ -732,8 +734,9 @@ static void cs35l56_hda_unbind(struct device *dev, struct device *master, void * if (cs35l56->base.fw_patched) cs_dsp_power_down(&cs35l56->cs_dsp);
- if (comps[cs35l56->index].dev == dev) - memset(&comps[cs35l56->index], 0, sizeof(*comps)); + comp = hda_component_from_index(parent, cs35l56->index); + if (comp && (comp->dev == dev)) + memset(comp, 0, sizeof(*comp));
cs35l56->codec = NULL;
diff --git a/sound/pci/hda/hda_component.c b/sound/pci/hda/hda_component.c index b05a0b87d32a..8c11c8b37799 100644 --- a/sound/pci/hda/hda_component.c +++ b/sound/pci/hda/hda_component.c @@ -141,7 +141,7 @@ int hda_component_manager_bind(struct hda_codec *cdc, for (i = 0; i < ARRAY_SIZE(parent->comps); i++) parent->comps[i].codec = cdc;
- return component_bind_all(hda_codec_dev(cdc), &parent->comps); + return component_bind_all(hda_codec_dev(cdc), parent); } EXPORT_SYMBOL_NS_GPL(hda_component_manager_bind, SND_HDA_SCODEC_COMPONENT);
diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h index a016f1b942a2..e547e1f1e674 100644 --- a/sound/pci/hda/hda_component.h +++ b/sound/pci/hda/hda_component.h @@ -93,7 +93,7 @@ static inline struct hda_component *hda_component_from_index(struct hda_componen static inline void hda_component_manager_unbind(struct hda_codec *cdc, struct hda_component_parent *parent) { - component_unbind_all(hda_codec_dev(cdc), &parent->comps); + component_unbind_all(hda_codec_dev(cdc), parent); }
#endif /* ifndef __HDA_COMPONENT_H__ */ diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index fdee6592c502..c6c1e8e81972 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -706,20 +706,20 @@ static int tas2781_hda_bind(struct device *dev, struct device *master, void *master_data) { struct tas2781_hda *tas_hda = dev_get_drvdata(dev); - struct hda_component *comps = master_data; + struct hda_component_parent *parent = master_data; + struct hda_component *comp; struct hda_codec *codec; unsigned int subid; int ret;
- if (!comps || tas_hda->priv->index < 0 || - tas_hda->priv->index >= HDA_MAX_COMPONENTS) + comp = hda_component_from_index(parent, tas_hda->priv->index); + if (!comp) return -EINVAL;
- comps = &comps[tas_hda->priv->index]; - if (comps->dev) + if (comp->dev) return -EBUSY;
- codec = comps->codec; + codec = comp->codec; subid = codec->core.subsystem_id >> 16;
switch (subid) { @@ -733,13 +733,13 @@ static int tas2781_hda_bind(struct device *dev, struct device *master,
pm_runtime_get_sync(dev);
- comps->dev = dev; + comp->dev = dev;
- strscpy(comps->name, dev_name(dev), sizeof(comps->name)); + strscpy(comp->name, dev_name(dev), sizeof(comp->name));
ret = tascodec_init(tas_hda->priv, codec, THIS_MODULE, tasdev_fw_ready); if (!ret) - comps->playback_hook = tas2781_hda_playback_hook; + comp->playback_hook = tas2781_hda_playback_hook;
pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); @@ -751,13 +751,14 @@ static void tas2781_hda_unbind(struct device *dev, struct device *master, void *master_data) { struct tas2781_hda *tas_hda = dev_get_drvdata(dev); - struct hda_component *comps = master_data; - comps = &comps[tas_hda->priv->index]; - - if (comps->dev == dev) { - comps->dev = NULL; - memset(comps->name, 0, sizeof(comps->name)); - comps->playback_hook = NULL; + struct hda_component_parent *parent = master_data; + struct hda_component *comp; + + comp = hda_component_from_index(parent, tas_hda->priv->index); + if (comp && (comp->dev == dev)) { + comp->dev = NULL; + memset(comp->name, 0, sizeof(comp->name)); + comp->playback_hook = NULL; }
tas2781_hda_remove_controls(tas_hda);
There is one codec shared across all of the bound HDA components and a copy is usually stashed in the amp driver so it doesn't need to be in every hda_component.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/cs35l41_hda.c | 7 ++++--- sound/pci/hda/cs35l56_hda.c | 2 +- sound/pci/hda/hda_component.c | 5 +---- sound/pci/hda/hda_component.h | 2 +- sound/pci/hda/tas2781_hda_i2c.c | 2 +- 5 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/sound/pci/hda/cs35l41_hda.c b/sound/pci/hda/cs35l41_hda.c index ceba4f2c85f1..ee9f83b737de 100644 --- a/sound/pci/hda/cs35l41_hda.c +++ b/sound/pci/hda/cs35l41_hda.c @@ -1436,10 +1436,11 @@ static int cs35l41_hda_bind(struct device *dev, struct device *master, void *mas mutex_lock(&cs35l41->fw_mutex);
comp->dev = dev; + cs35l41->codec = parent->codec; if (!cs35l41->acpi_subsystem_id) cs35l41->acpi_subsystem_id = kasprintf(GFP_KERNEL, "%.8x", - comp->codec->core.subsystem_id); - cs35l41->codec = comp->codec; + cs35l41->codec->core.subsystem_id); + strscpy(comp->name, dev_name(dev), sizeof(comp->name));
cs35l41->firmware_type = HDA_CS_DSP_FW_SPK_PROT; @@ -1470,7 +1471,7 @@ 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(&comp->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) + if (!device_link_add(&cs35l41->codec->core.dev, cs35l41->dev, DL_FLAG_STATELESS)) dev_warn(dev, "Unable to create device link\n"); unlock_system_sleep(sleep_flags);
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c index df4498c77171..cc4aa90db1d1 100644 --- a/sound/pci/hda/cs35l56_hda.c +++ b/sound/pci/hda/cs35l56_hda.c @@ -697,7 +697,7 @@ static int cs35l56_hda_bind(struct device *dev, struct device *master, void *mas return -EBUSY;
comp->dev = dev; - cs35l56->codec = comp->codec; + cs35l56->codec = parent->codec; strscpy(comp->name, dev_name(dev), sizeof(comp->name)); comp->playback_hook = cs35l56_hda_playback_hook;
diff --git a/sound/pci/hda/hda_component.c b/sound/pci/hda/hda_component.c index 8c11c8b37799..1a9950b76866 100644 --- a/sound/pci/hda/hda_component.c +++ b/sound/pci/hda/hda_component.c @@ -134,12 +134,9 @@ static int hda_comp_match_dev_name(struct device *dev, void *data) int hda_component_manager_bind(struct hda_codec *cdc, struct hda_component_parent *parent) { - int i; - /* Init shared and component specific data */ memset(parent, 0, sizeof(*parent)); - for (i = 0; i < ARRAY_SIZE(parent->comps); i++) - parent->comps[i].codec = cdc; + parent->codec = cdc;
return component_bind_all(hda_codec_dev(cdc), parent); } diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h index e547e1f1e674..dd4dabeae9ee 100644 --- a/sound/pci/hda/hda_component.h +++ b/sound/pci/hda/hda_component.h @@ -19,7 +19,6 @@ struct hda_component { struct device *dev; char name[HDA_MAX_NAME_SIZE]; - struct hda_codec *codec; struct acpi_device *adev; bool acpi_notifications_supported; void (*acpi_notify)(acpi_handle handle, u32 event, struct device *dev); @@ -29,6 +28,7 @@ struct hda_component { };
struct hda_component_parent { + struct hda_codec *codec; struct hda_component comps[HDA_MAX_COMPONENTS]; };
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index c6c1e8e81972..d7af4fd10714 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -719,7 +719,7 @@ static int tas2781_hda_bind(struct device *dev, struct device *master, if (comp->dev) return -EBUSY;
- codec = comp->codec; + codec = parent->codec; subid = codec->core.subsystem_id >> 16;
switch (subid) {
The hda_component contains information shared from the amp drivers to the codec that can be altered (for example as the driver unloads). Guard the update and use of these to prevent use of stale data.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com --- sound/pci/hda/hda_component.c | 13 ++++++++++++- sound/pci/hda/hda_component.h | 4 ++++ 2 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_component.c b/sound/pci/hda/hda_component.c index 1a9950b76866..7b19cb38b4e0 100644 --- a/sound/pci/hda/hda_component.c +++ b/sound/pci/hda/hda_component.c @@ -21,11 +21,13 @@ void hda_component_acpi_device_notify(struct hda_component_parent *parent, struct hda_component *comp; int i;
+ mutex_lock(&parent->mutex); for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { comp = hda_component_from_index(parent, i); if (comp->dev && comp->acpi_notify) comp->acpi_notify(acpi_device_handle(comp->adev), event, comp->dev); } + mutex_unlock(&parent->mutex); } EXPORT_SYMBOL_NS_GPL(hda_component_acpi_device_notify, SND_HDA_SCODEC_COMPONENT);
@@ -87,6 +89,7 @@ void hda_component_manager_playback_hook(struct hda_component_parent *parent, in struct hda_component *comp; int i;
+ mutex_lock(&parent->mutex); for (i = 0; i < ARRAY_SIZE(parent->comps); i++) { comp = hda_component_from_index(parent, i); if (comp->dev && comp->pre_playback_hook) @@ -102,6 +105,7 @@ void hda_component_manager_playback_hook(struct hda_component_parent *parent, in if (comp->dev && comp->post_playback_hook) comp->post_playback_hook(comp->dev, action); } + mutex_unlock(&parent->mutex); } EXPORT_SYMBOL_NS_GPL(hda_component_manager_playback_hook, SND_HDA_SCODEC_COMPONENT);
@@ -134,11 +138,18 @@ static int hda_comp_match_dev_name(struct device *dev, void *data) int hda_component_manager_bind(struct hda_codec *cdc, struct hda_component_parent *parent) { + int ret; + /* Init shared and component specific data */ memset(parent, 0, sizeof(*parent)); + mutex_init(&parent->mutex); parent->codec = cdc;
- return component_bind_all(hda_codec_dev(cdc), parent); + mutex_lock(&parent->mutex); + ret = component_bind_all(hda_codec_dev(cdc), parent); + mutex_unlock(&parent->mutex); + + return ret; } EXPORT_SYMBOL_NS_GPL(hda_component_manager_bind, SND_HDA_SCODEC_COMPONENT);
diff --git a/sound/pci/hda/hda_component.h b/sound/pci/hda/hda_component.h index dd4dabeae9ee..9f786608144c 100644 --- a/sound/pci/hda/hda_component.h +++ b/sound/pci/hda/hda_component.h @@ -11,6 +11,7 @@
#include <linux/acpi.h> #include <linux/component.h> +#include <linux/mutex.h> #include <sound/hda_codec.h>
#define HDA_MAX_COMPONENTS 4 @@ -28,6 +29,7 @@ struct hda_component { };
struct hda_component_parent { + struct mutex mutex; struct hda_codec *codec; struct hda_component comps[HDA_MAX_COMPONENTS]; }; @@ -93,7 +95,9 @@ static inline struct hda_component *hda_component_from_index(struct hda_componen static inline void hda_component_manager_unbind(struct hda_codec *cdc, struct hda_component_parent *parent) { + mutex_lock(&parent->mutex); component_unbind_all(hda_codec_dev(cdc), parent); + mutex_unlock(&parent->mutex); }
#endif /* ifndef __HDA_COMPONENT_H__ */
On Mon, 03 Jun 2024 12:35:17 +0200, Simon Trimmer wrote:
This series of patches makes sure that the existing consumers of the infrastructure unbind their interface before they begin destroying the driver, moves duplicated members from the instanced component structures into a new parent structure and introduces locking so that consumers of the interface do not use stale data.
v2:
- Correct application of sizeof to pointer flagged by kernel test robot
Simon Trimmer (7): ALSA: hda: cs35l56: Component should be unbound before deconstruction ALSA: hda: cs35l41: Component should be unbound before deconstruction ALSA: hda/tas2781: Component should be unbound before deconstruction ALSA: hda: hda_component: Introduce component parent structure ALSA: hda: hda_component: Change codecs to use component parent structure ALSA: hda: hda_component: Move codec field into the parent ALSA: hda: hda_component: Protect shared data with a mutex
The first three patches look rather like independent fixes. Could you split those out and add proper Fixes tags, so that stable trees can pick up?
The rest are a code refactoring and additional protection of the mutex.
thanks,
Takashi
sound/pci/hda/cs35l41_hda.c | 47 ++++++++++++--------- sound/pci/hda/cs35l56_hda.c | 29 +++++++------ sound/pci/hda/hda_component.c | 75 ++++++++++++++++++++------------- sound/pci/hda/hda_component.h | 48 ++++++++++++++------- sound/pci/hda/patch_realtek.c | 17 ++++---- sound/pci/hda/tas2781_hda_i2c.c | 37 ++++++++-------- 6 files changed, 147 insertions(+), 106 deletions(-)
-- 2.34.1
participants (2)
-
Simon Trimmer -
Takashi Iwai