Re: Adding movable PCIe BARs support in snd_hda_intel

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

On Tue, 23 Mar 2021 19:25:53 +0100,
Andrey Grodzovsky wrote:
> 
> >> This will cover IOCTLs and any
> >> mmapped accesses i guess. Interrupts we discussed above. What above any
> >> possible background kernel work going on in dedicated threads or work
> >> items ? Any pointers there what should be blocked and waited for ?
> >
> > An alternative idea would be the analogy of the system suspend /
> > resume.  That is, we forcibly suspend the devices at first somehow,
> > and also restricts the further accesses by some way.  Then do remap,
> 
> But that the point I guess, how you block further accesses without those
> big locks, during S3 i believe user mode gets suspended before the
> driver and so you don't need to worry about concurrent IOCTLs when going
> through suspend sequence

ALSA core still has some legacy card-level power management code,
which was introduced many years ago at the time we still managed the
power state via an extra ioctl (hence working individually from the
base PM code), and a few pieces are still effective for this kind of
purposes.  Through a quick glance, a couple of places need band-aids,
but the rest should work.

A bit more difficult problem is the floating control API calls.  The
get/put calls might be still in flight when we perform the PCI
rescan.  This has to be filtered out additionally.

Below are a patch series I cooked quickly.  Totally untested, just
checked the compilation.  The first patch is a fix I'll merge in
anyway, while the rest are RFC.


thanks,

Takashi

[-- Attachment #2: 0001-ALSA-hda-Re-add-dropped-snd_poewr_change_state-calls.patch --]
[-- Type: application/octet-stream, Size: 1410 bytes --]

From 52e2fad720c04345499e165be92063897cf64914 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 24 Mar 2021 08:53:56 +0100
Subject: [PATCH 1/7] ALSA: hda: Re-add dropped snd_poewr_change_state() calls

The card power state change via snd_power_change_state() at the system
suspend/resume seems dropped mistakenly during the PM code rewrite.
The card power state doesn't play much role nowadays but it's still
referred in a few places such as the HDMI codec driver.

This patch restores them, but in a more appropriate place now in the
prepare and complete callbacks.

Fixes: f5dac54d9d93 ("ALSA: hda: Separate runtime and system suspend")
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_intel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 5eea130dcf0a..c4146e8617de 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1025,6 +1025,7 @@ static int azx_prepare(struct device *dev)
 
 	chip = card->private_data;
 	chip->pm_prepared = 1;
+	snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
 
 	flush_work(&azx_bus(chip)->unsol_work);
 
@@ -1040,6 +1041,7 @@ static void azx_complete(struct device *dev)
 	struct azx *chip;
 
 	chip = card->private_data;
+	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
 	chip->pm_prepared = 0;
 }
 
-- 
2.26.2


[-- Attachment #3: 0002-ALSA-control-Add-power-state-check-commonly-for-ioct.patch --]
[-- Type: application/octet-stream, Size: 1139 bytes --]

From 57afd6d49b994e10e24f421abe517bde590ee120 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 24 Mar 2021 09:02:33 +0100
Subject: [PATCH 2/7] ALSA: control: Add power state check commonly for ioctl
 handlers

The card power state check can be better put in the common ioctl
handler, as basically we want to prevent ioctls during the power off
state.  Although this situation won't happen normally, it'll be
helpful for catching in the future faked suspend that is needed for
PCI rescan.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/core/control.c b/sound/core/control.c
index 5165741a8400..70b60dbc6b93 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1709,6 +1709,9 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	card = ctl->card;
 	if (snd_BUG_ON(!card))
 		return -ENXIO;
+	err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
+	if (err < 0)
+		return err;
 	switch (cmd) {
 	case SNDRV_CTL_IOCTL_PVERSION:
 		return put_user(SNDRV_CTL_VERSION, ip) ? -EFAULT : 0;
-- 
2.26.2


[-- Attachment #4: 0003-ALSA-control-Drop-superfluous-snd_power_wait-calls.patch --]
[-- Type: application/octet-stream, Size: 1626 bytes --]

From 5e3fa9ca4e28acd21e492a36127b99f99a9cbb5d Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 24 Mar 2021 09:03:39 +0100
Subject: [PATCH 3/7] ALSA: control: Drop superfluous snd_power_wait() calls

Now the card power state is checked in the common ioctl handler, drop
the same redundant checks in each ioctl.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 70b60dbc6b93..bfe9aceaaf76 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1015,9 +1015,6 @@ static int snd_ctl_elem_info_user(struct snd_ctl_file *ctl,
 
 	if (copy_from_user(&info, _info, sizeof(info)))
 		return -EFAULT;
-	result = snd_power_wait(ctl->card, SNDRV_CTL_POWER_D0);
-	if (result < 0)
-		return result;
 	result = snd_ctl_elem_info(ctl, &info);
 	if (result < 0)
 		return result;
@@ -1085,10 +1082,6 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
 	if (IS_ERR(control))
 		return PTR_ERR(control);
 
-	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
-	if (result < 0)
-		goto error;
-
 	down_read(&card->controls_rwsem);
 	result = snd_ctl_elem_read(card, control);
 	up_read(&card->controls_rwsem);
@@ -1146,10 +1139,6 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
 		return PTR_ERR(control);
 
 	card = file->card;
-	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
-	if (result < 0)
-		goto error;
-
 	down_write(&card->controls_rwsem);
 	result = snd_ctl_elem_write(card, file, control);
 	up_write(&card->controls_rwsem);
-- 
2.26.2


[-- Attachment #5: 0004-ALSA-control-Minor-optimization-for-SNDRV_CTL_IOCTL_.patch --]
[-- Type: application/octet-stream, Size: 1065 bytes --]

From ec3c3ca67004483a7dbc02dc1807aae9a179306f Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 24 Mar 2021 09:04:05 +0100
Subject: [PATCH 4/7] ALSA: control: Minor optimization for
 SNDRV_CTL_IOCTL_POWER_STATE

The SNDRV_CTL_IOCTL_POWER_STATE is an obsoleted ioctl and now
practically it returns always D0.  Do some minor optimization.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index bfe9aceaaf76..b9701b31de0e 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1744,11 +1744,7 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
-#ifdef CONFIG_PM
-		return put_user(card->power_state, ip) ? -EFAULT : 0;
-#else
 		return put_user(SNDRV_CTL_POWER_D0, ip) ? -EFAULT : 0;
-#endif
 	}
 	down_read(&snd_ioctl_rwsem);
 	list_for_each_entry(p, &snd_control_ioctls, list) {
-- 
2.26.2


[-- Attachment #6: 0005-ALSA-pcm-Block-the-release-until-the-system-resume-f.patch --]
[-- Type: application/octet-stream, Size: 1155 bytes --]

From 9502e76fcd9bf4ea3307811d40f5ac2be8725044 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 24 Mar 2021 09:05:25 +0100
Subject: [PATCH 5/7] ALSA: pcm: Block the release until the system resume
 finishes

The normal PCM operations are already blocked during the card power
off state in the PCM common ioctl handler, but the release isn't
covered.  As the PCM stream release may also access the hardware,
let's block the release until the card power turns on.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 17a85f4815d5..664d2975def4 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2799,6 +2799,10 @@ static int snd_pcm_release(struct inode *inode, struct file *file)
 	if (snd_BUG_ON(!substream))
 		return -ENXIO;
 	pcm = substream->pcm;
+
+	/* block until the device gets woken up as it may touch the hardware */
+	snd_power_wait(pcm->card, SNDRV_CTL_POWER_D0);
+
 	mutex_lock(&pcm->open_mutex);
 	snd_pcm_release_substream(substream);
 	kfree(pcm_file);
-- 
2.26.2


[-- Attachment #7: 0006-ALSA-hda-Add-frozen-flag-for-prohibiting-hardware-ac.patch --]
[-- Type: application/octet-stream, Size: 2349 bytes --]

From f49966c9be8d1f7d58b14b2394264b412127e31e Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Wed, 24 Mar 2021 09:13:29 +0100
Subject: [PATCH 6/7] ALSA: hda: Add frozen flag for prohibiting hardware
 access

The PCI rescan feature we're going to implement requires the prohibit
of the hardware access during rescan and remap.  This patch introduce
a flag, frozen, to hdac_bus for preventing the CORB/RIRB accesses, as
well as the helper function to flip it up/down.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hdaudio.h |  2 ++
 sound/hda/hdac_bus.c    | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index 22af68b01426..b270dd1f63ac 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -333,6 +333,7 @@ struct hdac_bus {
 
 	/* operation state */
 	bool chip_init:1;		/* h/w initialized */
+	bool frozen:1;			/* h/w access prohibited */
 
 	/* behavior flags */
 	bool aligned_mmio:1;		/* aligned MMIO access */
@@ -377,6 +378,7 @@ struct hdac_bus {
 int snd_hdac_bus_init(struct hdac_bus *bus, struct device *dev,
 		      const struct hdac_bus_ops *ops);
 void snd_hdac_bus_exit(struct hdac_bus *bus);
+void snd_hdac_bus_freeze(struct hdac_bus *bus, bool freeze);
 int snd_hdac_bus_exec_verb_unlocked(struct hdac_bus *bus, unsigned int addr,
 				    unsigned int cmd, unsigned int *res);
 
diff --git a/sound/hda/hdac_bus.c b/sound/hda/hdac_bus.c
index 71db8592b33d..91e8f3c25a08 100644
--- a/sound/hda/hdac_bus.c
+++ b/sound/hda/hdac_bus.c
@@ -75,6 +75,14 @@ void snd_hdac_bus_exit(struct hdac_bus *bus)
 }
 EXPORT_SYMBOL_GPL(snd_hdac_bus_exit);
 
+void snd_hdac_bus_freeze(struct hdac_bus *bus, bool frozen)
+{
+	mutex_lock(&bus->cmd_mutex);
+	bus->frozen = frozen;
+	mutex_unlock(&bus->cmd_mutex);
+}
+EXPORT_SYMBOL_GPL(snd_hdac_bus_freeze);
+
 /**
  * snd_hdac_bus_exec_verb - execute a HD-audio verb on the given bus
  * @bus: bus object
@@ -90,7 +98,10 @@ int snd_hdac_bus_exec_verb(struct hdac_bus *bus, unsigned int addr,
 	int err;
 
 	mutex_lock(&bus->cmd_mutex);
-	err = snd_hdac_bus_exec_verb_unlocked(bus, addr, cmd, res);
+	if (bus->frozen)
+		err = -EBADFD;
+	else
+		err = snd_hdac_bus_exec_verb_unlocked(bus, addr, cmd, res);
 	mutex_unlock(&bus->cmd_mutex);
 	return err;
 }
-- 
2.26.2


[-- Attachment #8: 0007-ALSA-hda-Add-support-for-BARs-move-on-PCI-rescan.patch --]
[-- Type: application/octet-stream, Size: 5350 bytes --]

From 6f35db2b5744de28dbf9b75eb2e62cc9015b17fd Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Tue, 16 Mar 2021 23:01:02 -0400
Subject: [PATCH 7/7] ALSA: hda: Add support for BARs move on PCI rescan

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pci/hda/hda_controller.c |   9 +++
 sound/pci/hda/hda_controller.h |   1 +
 sound/pci/hda/hda_intel.c      | 111 ++++++++++++++++++++++++++++++---
 3 files changed, 112 insertions(+), 9 deletions(-)

diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index ca2f2ecd1488..5e32aa78aa32 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1057,6 +1057,15 @@ void azx_stop_chip(struct azx *chip)
 }
 EXPORT_SYMBOL_GPL(azx_stop_chip);
 
+void azx_suspend_streams(struct azx *chip)
+{
+	struct azx_pcm *apcm;
+
+	list_for_each_entry(apcm, &chip->pcm_list, list)
+		snd_pcm_suspend_all(apcm->pcm);
+}
+EXPORT_SYMBOL_GPL(azx_suspend_streams);
+
 /*
  * interrupt handler
  */
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index 68f9668788ea..d40a5d87d34e 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -212,5 +212,6 @@ int azx_probe_codecs(struct azx *chip, unsigned int max_slots);
 int azx_codec_configure(struct azx *chip);
 int azx_init_streams(struct azx *chip);
 void azx_free_streams(struct azx *chip);
+void azx_suspend_streams(struct azx *chip);
 
 #endif /* __SOUND_HDA_CONTROLLER_H */
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c4146e8617de..a710a4108dd8 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1868,6 +1868,26 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	return 0;
 }
 
+static int azx_request_pci_regions(struct azx *chip)
+{
+	struct hdac_bus *bus = azx_bus(chip);
+	struct pci_dev *pci = chip->pci;
+	int err;
+
+	err = pci_request_regions(pci, "ICH HD audio");
+	if (err < 0)
+		return err;
+	chip->region_requested = 1;
+
+	bus->addr = pci_resource_start(pci, 0);
+	bus->remap_addr = pci_ioremap_bar(pci, 0);
+	if (bus->remap_addr == NULL) {
+		dev_err(&pci->dev, "ioremap error\n");
+		return -ENXIO;
+	}
+	return 0;
+}
+
 static int azx_first_init(struct azx *chip)
 {
 	int dev = chip->dev_index;
@@ -1888,17 +1908,9 @@ static int azx_first_init(struct azx *chip)
 	}
 #endif
 
-	err = pci_request_regions(pci, "ICH HD audio");
+	err = azx_request_pci_regions(chip);
 	if (err < 0)
 		return err;
-	chip->region_requested = 1;
-
-	bus->addr = pci_resource_start(pci, 0);
-	bus->remap_addr = pci_ioremap_bar(pci, 0);
-	if (bus->remap_addr == NULL) {
-		dev_err(card->dev, "ioremap error\n");
-		return -ENXIO;
-	}
 
 	if (chip->driver_type == AZX_DRIVER_SKL)
 		snd_hdac_bus_parse_capabilities(bus);
@@ -2416,6 +2428,84 @@ static void azx_shutdown(struct pci_dev *pci)
 		azx_stop_chip(chip);
 }
 
+static bool azx_bar_fixed(struct pci_dev *pdev, int resno)
+{
+	return false;
+}
+
+static void azx_rescan_prepare(struct pci_dev *pdev)
+{
+	struct snd_card *card = pci_get_drvdata(pdev);
+	struct azx *chip = card->private_data;
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+	struct hdac_bus *bus = azx_bus(chip);
+	struct hda_codec *codec;
+
+	// FIXME: need unlock/lock dance as in azx_remove()?
+	flush_work_sync(&hda->probe_work);
+
+	if (hda->freed || hda->init_failed)
+		return;
+
+	if (chip->running) {
+		pm_runtime_get_sync(&pdev->dev);
+		azx_prepare(&pdev->dev);
+		azx_suspend_streams(chip);
+		list_for_each_codec(codec, &chip->bus) {
+			pm_runtime_suspend(hda_codec_dev(codec));
+			pm_runtime_disable(hda_codec_dev(codec));
+		}
+		snd_hdac_bus_freeze(bus, true);
+		azx_suspend(&pdev->dev);
+	}
+
+	/* Unmap MMIO and release BAR resource */
+	iounmap(bus->remap_addr);
+	if (chip->region_requested) {
+		pci_release_regions(chip->pci);
+		chip->region_requested = 0;
+	}
+}
+
+static void azx_rescan_done(struct pci_dev *pdev)
+{
+	struct snd_card *card = pci_get_drvdata(pdev);
+	struct azx *chip = card->private_data;
+	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+	struct hdac_bus *bus = azx_bus(chip);
+	struct hdac_stream *azx_dev;
+	struct hda_codec *codec;
+	int err;
+
+	if (hda->freed || hda->init_failed)
+		return;
+
+	/* Reassign BAR and remap */
+	err = azx_request_pci_regions(chip);
+	if (err < 0) {
+		dev_err(card->dev, "Rescan failed: disabling the device\n");
+		card->shutdown = 1;
+		chip->init_failed = true;
+		// FIXME: any better handling?
+		return;
+	}
+
+	// FIXME: should be in hdac_stream.c
+	list_for_each_entry(azx_dev, &bus->stream_list, list)
+		azx_dev->sd_addr = bus->remap_addr + (0x20 * azx_dev->index + 0x80);
+
+	if (chip->running) {
+		azx_resume(&pdev->dev);
+		snd_hdac_bus_freeze(bus, false);
+		list_for_each_codec(codec, &chip->bus) {
+			pm_runtime_enable(hda_codec_dev(codec));
+			pm_runtime_resume(hda_codec_dev(codec));
+		}
+		azx_complete(&pdev->dev);
+		pm_runtime_put_sync(&pdev->dev);
+	}
+}
+
 /* PCI IDs */
 static const struct pci_device_id azx_ids[] = {
 	/* CPT */
@@ -2783,6 +2873,9 @@ static struct pci_driver azx_driver = {
 	.driver = {
 		.pm = AZX_PM_OPS,
 	},
+	.rescan_prepare	= azx_rescan_prepare,
+	.rescan_done	= azx_rescan_done,
+	.bar_fixed	= azx_bar_fixed,
 };
 
 module_pci_driver(azx_driver);
-- 
2.26.2