On Wed, 29 Jul 2020 18:06:25 +0200,
Takashi Iwai wrote:
>
> On Wed, 29 Jul 2020 17:03:22 +0200,
> Ranjani Sridharan wrote:
> >
> > On Wed, 2020-07-29 at 09:48 +0200, Takashi Iwai wrote:
> > > On Wed, 29 Jul 2020 01:10:11 +0200,
> > > Ranjani Sridharan wrote:
> > > > When the ASoC card registration fails and the codec component
> > > > driver
> > > > never probes, the codec device is not initialized and therefore
> > > > memory for codec->wcaps is not allocated. This results in a NULL
> > > > pointer
> > > > dereference when the codec driver suspend callback is invoked
> > > > during
> > > > system suspend. Fix this by returning without performing any
> > > > actions
> > > > during codec suspend/resume if the card was not registered
> > > > successfully.
> > > >
> > > > Reviewed-by: Pierre-Louis Bossart <
> > > > pierre-louis.bossart@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx
> > > > >
> > >
> > > The code changes look OK to apply, but I still wonder how the runtime
> > > PM gets invoked even if the device is not instantiated properly?
> > Hi Takashi,
> >
> > Its not runtime PM suspend but rather the system PM suspend callback
> > that is invoked when the system is suspended that ends up callling the
> > the runtime PM callback. So, the sequence is:
> > hda_codec_pm_suspend()
> > -> pm_runtime_force_suspend()
> > -> hda_codec_runtime_suspend()
>
> OK, but the problem is still same. The basic problem is that the
> hda_codec_driver_probe() is called for the hda_codec object that
> hasn't been initialized and bypasses to ext_ops.hdev_attach.
>
> So, we can factor out the fundamental part of
> snd_hda_codec_device_new() that is irrelevant with the card object and
> call it in hdac_hda_dev_probe().
I meant something like below (totally untested)
Takashi
---
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index f4cc364d837f..1f01e4d6b923 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -303,10 +303,11 @@ struct hda_codec {
/*
* constructors
*/
-int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
- unsigned int codec_addr, struct hda_codec **codecp);
-int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
- unsigned int codec_addr, struct hda_codec *codec);
+int snd_hda_codec_new(struct hda_bus *bus, unsigned int codec_addr,
+ struct hda_codec **codecp);
+int snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
+ struct hda_codec *codec);
+int snd_hda_codec_assign_card(struct hda_codec *codec);
int snd_hda_codec_configure(struct hda_codec *codec);
int snd_hda_codec_update_widgets(struct hda_codec *codec);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index 40f3c175954d..3079d32ba64d 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -867,15 +867,13 @@ static void snd_hda_codec_dev_release(struct device *dev)
#define DEV_NAME_LEN 31
-static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
- unsigned int codec_addr, struct hda_codec **codecp)
+static int hda_codec_new(struct hda_bus *bus, unsigned int card_number,
+ unsigned int codec_addr, struct hda_codec **codecp)
{
char name[DEV_NAME_LEN];
struct hda_codec *codec;
int err;
- dev_dbg(card->dev, "%s: entry\n", __func__);
-
if (snd_BUG_ON(!bus))
return -EINVAL;
if (snd_BUG_ON(codec_addr > HDA_MAX_CODEC_ADDRESS))
@@ -885,7 +883,7 @@ static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
if (!codec)
return -ENOMEM;
- sprintf(name, "hdaudioC%dD%d", card->number, codec_addr);
+ sprintf(name, "hdaudioC%dD%d", card_number, codec_addr);
err = snd_hdac_device_init(&codec->core, &bus->core, name, codec_addr);
if (err < 0) {
kfree(codec);
@@ -901,37 +899,41 @@ static int snd_hda_codec_device_init(struct hda_bus *bus, struct snd_card *card,
/**
* snd_hda_codec_new - create a HDA codec
* @bus: the bus to assign
- * @card: card for this codec
* @codec_addr: the codec address
* @codecp: the pointer to store the generated codec
*
* Returns 0 if successful, or a negative error code.
*/
-int snd_hda_codec_new(struct hda_bus *bus, struct snd_card *card,
- unsigned int codec_addr, struct hda_codec **codecp)
+int snd_hda_codec_new(struct hda_bus *bus, unsigned int codec_addr,
+ struct hda_codec **codecp)
{
int ret;
- ret = snd_hda_codec_device_init(bus, card, codec_addr, codecp);
+ ret = hda_codec_new(bus, bus->card->number, codec_addr, codecp);
if (ret < 0)
return ret;
- return snd_hda_codec_device_new(bus, card, codec_addr, *codecp);
+ ret = snd_hda_codec_device_init(bus, codec_addr, *codecp);
+ if (ret < 0)
+ goto error;
+
+ ret = snd_hda_codec_assign_card(*codecp);
+ if (ret < 0)
+ goto error;
+
+ return 0;
+
+ error:
+ put_device(hda_codec_dev(*codecp));
+ return ret;
}
EXPORT_SYMBOL_GPL(snd_hda_codec_new);
-int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
- unsigned int codec_addr, struct hda_codec *codec)
+int snd_hda_codec_device_init(struct hda_bus *bus, unsigned int codec_addr,
+ struct hda_codec *codec)
{
- char component[31];
hda_nid_t fg;
int err;
- static const struct snd_device_ops dev_ops = {
- .dev_register = snd_hda_codec_dev_register,
- .dev_free = snd_hda_codec_dev_free,
- };
-
- dev_dbg(card->dev, "%s: entry\n", __func__);
if (snd_BUG_ON(!bus))
return -EINVAL;
@@ -942,7 +944,6 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
codec->core.exec_verb = codec_exec_verb;
codec->bus = bus;
- codec->card = card;
codec->addr = codec_addr;
mutex_init(&codec->spdif_mutex);
mutex_init(&codec->control_mutex);
@@ -965,47 +966,50 @@ int snd_hda_codec_device_new(struct hda_bus *bus, struct snd_card *card,
codec->power_jiffies = jiffies;
#endif
- snd_hda_sysfs_init(codec);
-
if (codec->bus->modelname) {
codec->modelname = kstrdup(codec->bus->modelname, GFP_KERNEL);
- if (!codec->modelname) {
- err = -ENOMEM;
- goto error;
- }
+ if (!codec->modelname)
+ return -ENOMEM;
}
fg = codec->core.afg ? codec->core.afg : codec->core.mfg;
err = read_widget_caps(codec, fg);
if (err < 0)
- goto error;
+ return err;
err = read_pin_defaults(codec);
if (err < 0)
- goto error;
+ return err;
/* power-up all before initialization */
hda_set_power_state(codec, AC_PWRST_D0);
codec->core.dev.power.power_state = PMSG_ON;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(snd_hda_codec_device_init);
+
+int snd_hda_codec_assign_card(struct hda_codec *codec)
+{
+ static const struct snd_device_ops dev_ops = {
+ .dev_register = snd_hda_codec_dev_register,
+ .dev_free = snd_hda_codec_dev_free,
+ };
+ char component[31];
+
+ codec->card = codec->bus->card;
snd_hda_codec_proc_new(codec);
+ snd_hda_sysfs_init(codec);
+
snd_hda_create_hwdep(codec);
sprintf(component, "HDA:%08x,%08x,%08x", codec->core.vendor_id,
codec->core.subsystem_id, codec->core.revision_id);
- snd_component_add(card, component);
-
- err = snd_device_new(card, SNDRV_DEV_CODEC, codec, &dev_ops);
- if (err < 0)
- goto error;
-
- return 0;
+ snd_component_add(codec->card, component);
- error:
- put_device(hda_codec_dev(codec));
- return err;
+ return snd_device_new(codec->card, SNDRV_DEV_CODEC, codec, &dev_ops);
}
-EXPORT_SYMBOL_GPL(snd_hda_codec_device_new);
+EXPORT_SYMBOL_GPL(snd_hda_codec_assign_card);
/**
* snd_hda_codec_update_widgets - Refresh widget caps and pin defaults
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 80016b7b6849..e68ca57be30e 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1213,6 +1213,7 @@ EXPORT_SYMBOL_GPL(azx_bus_init);
int azx_probe_codecs(struct azx *chip, unsigned int max_slots)
{
struct hdac_bus *bus = azx_bus(chip);
+ struct hda_codec *codec;
int c, codecs, err;
codecs = 0;
@@ -1245,10 +1246,10 @@ int azx_probe_codecs(struct azx *chip, unsigned int max_slots)
/* Then create codec instances */
for (c = 0; c < max_slots; c++) {
if ((bus->codec_mask & (1 << c)) & chip->codec_probe_mask) {
- struct hda_codec *codec;
- err = snd_hda_codec_new(&chip->bus, chip->card, c, &codec);
+ err = snd_hda_codec_new(&chip->bus, c, &codec);
if (err < 0)
continue;
+
codec->jackpoll_interval = chip->jackpoll_interval;
codec->beep_mode = chip->beep_mode;
codecs++;
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c
index 473efe9ef998..298d9c46d85d 100644
--- a/sound/soc/codecs/hdac_hda.c
+++ b/sound/soc/codecs/hdac_hda.c
@@ -417,18 +417,12 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
snd_hdac_display_power(hdev->bus,
HDA_CODEC_IDX_CONTROLLER, true);
- ret = snd_hda_codec_device_new(hcodec->bus, component->card->snd_card,
- hdev->addr, hcodec);
+ hcodec->bus->card = dapm->card->snd_card;
+ ret = snd_hda_codec_assign_card(hcodec);
if (ret < 0) {
dev_err(&hdev->dev, "failed to create hda codec %d\n", ret);
goto error_no_pm;
}
- /*
- * Overwrite type to HDA_DEV_ASOC since it is a ASoC driver
- * hda_codec.c will check this flag to determine if unregister
- * device is needed.
- */
- hdev->type = HDA_DEV_ASOC;
/*
* snd_hda_codec_device_new decrements the usage count so call get pm
@@ -436,8 +430,6 @@ static int hdac_hda_codec_probe(struct snd_soc_component *component)
*/
pm_runtime_get_noresume(&hdev->dev);
- hcodec->bus->card = dapm->card->snd_card;
-
ret = snd_hda_codec_set_name(hcodec, hcodec->preset->name);
if (ret < 0) {
dev_err(&hdev->dev, "name failed %s\n", hcodec->preset->name);
@@ -574,6 +566,7 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
{
struct hdac_ext_link *hlink;
struct hdac_hda_priv *hda_pvt;
+ struct hda_codec *hcodec;
int ret;
/* hold the ref while we probe */
@@ -588,6 +581,18 @@ static int hdac_hda_dev_probe(struct hdac_device *hdev)
if (!hda_pvt)
return -ENOMEM;
+ ret = snd_hda_codec_device_init(hcodec->bus, hdev->addr,
+ &hda_pvt->codec);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * Overwrite type to HDA_DEV_ASOC since it is a ASoC driver
+ * hda_codec.c will check this flag to determine if unregister
+ * device is needed.
+ */
+ hdev->type = HDA_DEV_ASOC;
+
/* ASoC specific initialization */
ret = devm_snd_soc_register_component(&hdev->dev,
&hdac_hda_codec, hdac_hda_dais,
