[6/7] ALSA: hda/tegra: fix kernel panic

Commit Message

Sameer Pujar Jan. 21, 2019, 5:41 p.m. UTC

Kernel panic is seen during device boot. It appears that before the
probe completes, runtime callbacks happen and the device setup is not
done yet. This could be initiated from framework through exposed
callbacks. This issue can be fixed by having a flag to indicate
completion of device probe. Hence 'probed' flag is introduced to notify
completion of probe and runtime callbacks can check this flag before
doing any device access.

Signed-off-by: Sameer Pujar <[e-mail]>
Reviewed-by: Ravindra Lokhande <[e-mail]>
Reviewed-by: Mohan Kumar D <[e-mail]>
---
 sound/pci/hda/hda_tegra.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Takashi Iwai Jan. 21, 2019, 9:28 p.m. UTC | #1

On Mon, 21 Jan 2019 18:41:36 +0100,
Sameer Pujar wrote:
> 
> Kernel panic is seen during device boot. It appears that before the
> probe completes, runtime callbacks happen and the device setup is not
> done yet. This could be initiated from framework through exposed
> callbacks. This issue can be fixed by having a flag to indicate
> completion of device probe. Hence 'probed' flag is introduced to notify
> completion of probe and runtime callbacks can check this flag before
> doing any device access.

Such a fix should be rather folded into the previous, especially if
you know it's already broken :)

And, IMO, it's better to put such a check into runtime_idle callback
instead of doing in each (runtime_)suspend/resume callback.


thanks,

Takashi

Sameer Pujar Jan. 22, 2019, 3:41 a.m. UTC | #2

On 1/22/2019 2:58 AM, Takashi Iwai wrote:
> On Mon, 21 Jan 2019 18:41:36 +0100,
> Sameer Pujar wrote:
>> Kernel panic is seen during device boot. It appears that before the
>> probe completes, runtime callbacks happen and the device setup is not
>> done yet. This could be initiated from framework through exposed
>> callbacks. This issue can be fixed by having a flag to indicate
>> completion of device probe. Hence 'probed' flag is introduced to notify
>> completion of probe and runtime callbacks can check this flag before
>> doing any device access.
> Such a fix should be rather folded into the previous, especially if
> you know it's already broken :)
Ok, I will meld this with previous commit.
>
> And, IMO, it's better to put such a check into runtime_idle callback
> instead of doing in each (runtime_)suspend/resume callback.
Check in runtime_idle would take care of suspend path. But how the check in
runtime_resume can be avoided? Kernel panic happened during 
runtime_resume().
>
>
> thanks,
>
> Takashi

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Takashi Iwai Jan. 22, 2019, 6:24 a.m. UTC | #3

On Tue, 22 Jan 2019 04:41:16 +0100,
Sameer Pujar wrote:
> 
> 
> On 1/22/2019 2:58 AM, Takashi Iwai wrote:
> > On Mon, 21 Jan 2019 18:41:36 +0100,
> > Sameer Pujar wrote:
> >> Kernel panic is seen during device boot. It appears that before the
> >> probe completes, runtime callbacks happen and the device setup is not
> >> done yet. This could be initiated from framework through exposed
> >> callbacks. This issue can be fixed by having a flag to indicate
> >> completion of device probe. Hence 'probed' flag is introduced to notify
> >> completion of probe and runtime callbacks can check this flag before
> >> doing any device access.
> > Such a fix should be rather folded into the previous, especially if
> > you know it's already broken :)
> Ok, I will meld this with previous commit.
> >
> > And, IMO, it's better to put such a check into runtime_idle callback
> > instead of doing in each (runtime_)suspend/resume callback.
> Check in runtime_idle would take care of suspend path. But how the check in
> runtime_resume can be avoided? Kernel panic happened during
> runtime_resume().

Hm, right, we need the check in both places, since runtime PM is
called from the system PM, too.

BTW, I guess we can check chip->running instead of adding a new flag.


thanks,

Takashi

Patch

diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c
index 5546e29..80c4042 100644
--- a/sound/pci/hda/hda_tegra.c
+++ b/sound/pci/hda/hda_tegra.c
@@ -76,6 +76,7 @@  struct hda_tegra {
 	struct clk *hda2hdmi_clk;
 	void __iomem *regs;
 	struct work_struct probe_work;
+	bool probed;
 };
 
 #ifdef CONFIG_PM
@@ -265,9 +266,11 @@  static int hda_tegra_runtime_suspend(struct device *dev)
 	struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip);
 	struct hdac_bus *bus = azx_bus(chip);
 
-	azx_stop_chip(chip);
-	synchronize_irq(bus->irq);
-	azx_enter_link_reset(chip);
+	if (hda->probed) {
+		azx_stop_chip(chip);
+		synchronize_irq(bus->irq);
+		azx_enter_link_reset(chip);
+	}
 	hda_tegra_disable_clocks(hda);
 
 	return 0;
@@ -283,8 +286,10 @@  static int hda_tegra_runtime_resume(struct device *dev)
 	rc = hda_tegra_enable_clocks(hda);
 	if (rc != 0)
 		return rc;
-	hda_tegra_init(hda);
-	azx_init_chip(chip, 1);
+	if (hda->probed) {
+		hda_tegra_init(hda);
+		azx_init_chip(chip, 1);
+	}
 
 	return 0;
 }
@@ -527,6 +532,7 @@  static int hda_tegra_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	hda->dev = &pdev->dev;
 	chip = &hda->chip;
+	hda->probed = false;
 
 	err = snd_card_new(&pdev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
 			   THIS_MODULE, 0, &card);
@@ -585,6 +591,7 @@  static void hda_tegra_probe_work(struct work_struct *work)
 		goto out_free;
 
 	chip->running = 1;
+	hda->probed = true;
 	snd_hda_set_power_save(&chip->bus, power_save * 1000);
 
  out_free: