The current code to unregister our ALSA device needs to be undone manually
when we remove the HDMI driver.
Since ALSA doesn't seem to support any mechanism to defer freeing something
until the last user of the ALSA device is gone, we can either use a
device-managed or a DRM-managed action.
The consistent way would be to use a DRM-managed one, just like pretty much
any framework-facing structure should be doing. However, ALSA does a lot of
allocation and registration using device-managed calls. Thus, if we're
going that way, by the time the DRM-managed action would run all of those
allocation would have been freed and we would end up with a use-after-free.
Thus, let's do a device-managed action. It's been tested with KASAN enabled
and doesn't seem to trigger any issue, so it's as good as anything.
Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 43 ++++++++++++++++++++++++++++------
1 file changed, 36 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 283fea78f3cf..bf7d56bbcba0 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -2056,6 +2056,14 @@ static struct hdmi_codec_pdata vc4_hdmi_codec_pdata = {
.i2s = 1,
};
+static void vc4_hdmi_audio_codec_release(void *ptr)
+{
+ struct vc4_hdmi *vc4_hdmi = ptr;
+
+ platform_device_unregister(vc4_hdmi->audio.codec_pdev);
+ vc4_hdmi->audio.codec_pdev = NULL;
+}
+
static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
{
const struct vc4_hdmi_register *mai_data =
@@ -2097,6 +2105,30 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
vc4_hdmi->audio.dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
vc4_hdmi->audio.dma_data.maxburst = 2;
+ /*
+ * NOTE: Strictly speaking, we should probably use a DRM-managed
+ * registration there to avoid removing all the audio components
+ * by the time the driver doesn't have any user anymore.
+ *
+ * However, the ASoC core uses a number of devm_kzalloc calls
+ * when registering, even when using non-device-managed
+ * functions (such as in snd_soc_register_component()).
+ *
+ * If we call snd_soc_unregister_component() in a DRM-managed
+ * action, the device-managed actions have already been executed
+ * and thus we would access memory that has been freed.
+ *
+ * Using device-managed hooks here probably leaves us open to a
+ * bunch of issues if userspace still has a handle on the ALSA
+ * device when the device is removed. However, this is mitigated
+ * by the use of drm_dev_enter()/drm_dev_exit() in the audio
+ * path to prevent the access to the device resources if it
+ * isn't there anymore.
+ *
+ * Then, the vc4_hdmi structure is DRM-managed and thus only
+ * freed whenever the last user has closed the DRM device file.
+ * It should thus outlive ALSA in most situations.
+ */
ret = devm_snd_dmaengine_pcm_register(dev, &pcm_conf, 0);
if (ret) {
dev_err(dev, "Could not register PCM component: %d\n", ret);
@@ -2120,6 +2152,10 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
}
vc4_hdmi->audio.codec_pdev = codec_pdev;
+ ret = devm_add_action_or_reset(dev, vc4_hdmi_audio_codec_release, vc4_hdmi);
+ if (ret)
+ return ret;
+
dai_link->cpus = &vc4_hdmi->audio.cpu;
dai_link->codecs = &vc4_hdmi->audio.codec;
dai_link->platforms = &vc4_hdmi->audio.platform;
@@ -2158,12 +2194,6 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi *vc4_hdmi)
}
-static void vc4_hdmi_audio_exit(struct vc4_hdmi *vc4_hdmi)
-{
- platform_device_unregister(vc4_hdmi->audio.codec_pdev);
- vc4_hdmi->audio.codec_pdev = NULL;
-}
-
static irqreturn_t vc4_hdmi_hpd_irq_thread(int irq, void *priv)
{
struct vc4_hdmi *vc4_hdmi = priv;
@@ -3062,7 +3092,6 @@ static void vc4_hdmi_unbind(struct device *dev, struct device *master,
kfree(vc4_hdmi->hdmi_regset.regs);
kfree(vc4_hdmi->hd_regset.regs);
- vc4_hdmi_audio_exit(vc4_hdmi);
vc4_hdmi_cec_exit(vc4_hdmi);
vc4_hdmi_hotplug_exit(vc4_hdmi);
--
2.36.1
