Skip to content

Fix HDaudio probe/remove #274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 10 commits into from
Nov 17, 2018
Merged

Fix HDaudio probe/remove #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8c4b558
ASoC: SOF: Intel: hda: fix error handling in probe
plbossart Nov 12, 2018
36bc509
ASoC: SOF: Intel: hda: fix sof_hda_bus_init
plbossart Nov 12, 2018
06f2402
ASoC: SOF: Intel: hda: unmap on hda_init error
plbossart Nov 12, 2018
90277c3
ASoC: SOF: Intel: hda: handle error on DSP BAR remap
plbossart Nov 12, 2018
304a296
ASoC: SOF: Intel: hda: free streams on error
plbossart Nov 12, 2018
52e0435
ASoC: SOF: hda: Intel: free dsp bar mapping
plbossart Nov 12, 2018
bdfd569
ASoC: SOF: hda: be consistent and free hda->irq instead of pci->irq
plbossart Nov 12, 2018
aeb1db3
ASoC: SOF: Intel: hda: unmap on remove
plbossart Nov 12, 2018
dc91aea
ASoC: SOF: Intel: clean-up i915 support
plbossart Nov 12, 2018
d8f615a
ASoC: SOF: Intel: hda: add i915_exit
plbossart Nov 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions sound/soc/sof/intel/hda-bus.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -75,10 +75,9 @@ static const struct hdac_io_ops io_ops = {

/*
* This can be used for both with/without hda link support.
* Returns 0 if successful, or a negative error code.
*/
int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_ext_bus_ops *ext_ops)
void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_ext_bus_ops *ext_ops)
{
static int idx;

Expand All@@ -103,6 +102,4 @@ int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,

mutex_init(&bus->lock);
bus->cmd_dma_state = true;

return 0;
}
49 changes: 46 additions & 3 deletions sound/soc/sof/intel/hda-codec.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -112,6 +112,34 @@ int hda_codec_probe_bus(struct snd_sof_dev *sdev)
}
EXPORT_SYMBOL(hda_codec_probe_bus);

int hda_codec_i915_get(struct snd_sof_dev *sdev)
{
struct hdac_bus *bus = sof_to_bus(sdev);
int ret;

dev_dbg(bus->dev, "Turning i915 HDAC power on\n");
ret = snd_hdac_display_power(bus, true);
if (ret < 0)
dev_err(bus->dev, "i915 HDAC power on failed %d\n", ret);

return ret;
}
EXPORT_SYMBOL(hda_codec_i915_get);

int hda_codec_i915_put(struct snd_sof_dev *sdev)
{
struct hdac_bus *bus = sof_to_bus(sdev);
int ret;

dev_dbg(bus->dev, "Turning i915 HDAC power off\n");
ret = snd_hdac_display_power(bus, false);
if (ret < 0)
dev_err(bus->dev, "i915 HDAC power off failed %d\n", ret);

return ret;
}
EXPORT_SYMBOL(hda_codec_i915_put);

int hda_codec_i915_init(struct snd_sof_dev *sdev)
{
struct hdac_bus *bus = sof_to_bus(sdev);
Expand All@@ -122,12 +150,27 @@ int hda_codec_i915_init(struct snd_sof_dev *sdev)
if (ret < 0)
return ret;

ret = snd_hdac_display_power(bus, true);
if (ret < 0)
dev_err(bus->dev, "i915 HDAC power on failed %d\n", ret);
ret = hda_codec_i915_get(sdev);

return ret;
}
EXPORT_SYMBOL(hda_codec_i915_init);

int hda_codec_i915_exit(struct snd_sof_dev *sdev)
{
struct hdac_bus *bus = sof_to_bus(sdev);
int ret;

/*
* we don't need to decrease the refcount with
* hda_codec_i915_put() on exit since the device cannot be
* active
*/

ret = snd_hdac_i915_exit(bus);

return ret;
}
EXPORT_SYMBOL(hda_codec_i915_exit);

MODULE_LICENSE("Dual BSD/GPL");
4 changes: 2 additions & 2 deletions sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -380,7 +380,7 @@ int hda_dsp_resume(struct snd_sof_dev *sdev)

/* turn display power on */
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
ret = snd_hdac_display_power(bus, true);
ret = hda_codec_i915_get(sdev);
if (ret < 0) {
dev_err(bus->dev, "Cannot turn on display power on i915 after resume\n");
return ret;
Expand DownExpand Up@@ -417,7 +417,7 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev, int state)

/* turn display power off */
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
ret = snd_hdac_display_power(bus, false);
ret = hda_codec_i915_put(sdev);
if (ret < 0) {
dev_err(bus->dev, "Cannot turn OFF display power on i915 during suspend\n");
return ret;
Expand Down
38 changes: 24 additions & 14 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -459,8 +459,11 @@ static int hda_init_caps(struct snd_sof_dev *sdev)
ret = hda_dsp_ctrl_init_chip(sdev, true);
if (ret < 0) {
dev_err(bus->dev, "Init chip failed with ret: %d\n", ret);
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
snd_hdac_display_power(bus, false);
if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
ret = hda_codec_i915_put(sdev);
if (ret < 0)
return ret;
}
return ret;
}

Expand All@@ -477,11 +480,9 @@ static int hda_init_caps(struct snd_sof_dev *sdev)
hda_codec_probe_bus(sdev);

if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
ret = snd_hdac_display_power(bus, false);
if (ret < 0) {
dev_err(bus->dev, "Cannot turn off display power on i915\n");
ret = hda_codec_i915_put(sdev);
if (ret < 0)
return ret;
}
}

/*
Expand DownExpand Up@@ -563,15 +564,17 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
#endif

/* set up HDA base */
bus = sof_to_bus(sdev);
ret = hda_init(sdev);
if (ret < 0)
return ret;
goto hdac_bus_unmap;

/* DSP base */
sdev->bar[HDA_DSP_BAR] = pci_ioremap_bar(pci, HDA_DSP_BAR);
if (!sdev->bar[HDA_DSP_BAR]) {
dev_err(&pci->dev, "error: ioremap error\n");
return -ENXIO;
ret = -ENXIO;
goto hdac_bus_unmap;
}

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This 'goto hdac_bus_unmap' is fine, but we may need add some error point e.g. 'dsp_bar_unmap' where we will do iounmap for sdev->bar[HDA_DSP_BAR].


sdev->mmio_bar = HDA_DSP_BAR;
Expand All@@ -595,7 +598,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
* not all errors are due to memory issues, but trying
* to free everything does not harm
*/
goto err;
goto free_streams;
}

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial streams fails, no streams to be freed? if streams are partially initialized success, better to do free to those initialized inside stream_init(), at e.g. it's exiting of function.
should be "goto dsp_bar_unmap;"


/*
Expand All@@ -619,7 +622,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
sdev->ipc_irq = sdev->hda->irq;
}

bus = sof_to_bus(sdev);
dev_dbg(sdev->dev, "using HDA IRQ %d\n", sdev->hda->irq);
ret = request_threaded_irq(sdev->hda->irq, hda_dsp_stream_interrupt,
hda_dsp_stream_threaded_handler,
Expand DownExpand Up@@ -720,10 +722,11 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
pci_free_irq_vectors(pci);
free_streams:
hda_dsp_stream_free(sdev);
/* dsp_unmap: not currently used */
iounmap(sdev->bar[HDA_DSP_BAR]);

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is what I mentioned above.

hdac_bus_unmap:
iounmap(bus->remap_addr);
err:

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (bus->remap_addr)
iounmap(bus->remap_addr);

And when looking into hda_init() more, only pci_ioremap_bar() may get error and return the error(hda_dsp_ctrl_get_caps() returns 0 always, maybe we should change its return type to void also), so we may don't need this iounmap() here, and do iounmap() in hda_dsp_remove().

/* disable DSP */
snd_sof_dsp_update_bits(sdev, HDA_DSP_PP_BAR, SOF_HDA_REG_PP_PPCTL,
SOF_HDA_PPCTL_GPROCEN, 0);
return ret;

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this removing is good.
We may need add hda_exit() to this err handling where we can do e.g. hda bus de-init.

}

Expand DownExpand Up@@ -756,10 +759,17 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
SOF_HDA_PPCTL_GPROCEN, 0);

free_irq(sdev->ipc_irq, sdev);
free_irq(sdev->pci->irq, bus);
free_irq(sdev->hda->irq, bus);
pci_free_irq_vectors(pci);

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

@lgirdwoodlgirdwoodNov 14, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, IRQ can be the same physical ID or source for free_irq() as long as the private data pointer are different. Iirc, one of the APL platforms shared the same IRQ ID for both IPC and PCI HDA IRQs.

@plbossartplbossartNov 14, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have no idea what this all means. we use both sdev->pci->irq or sdev->hda->irq and it'd be useful to align on a single way of releasing stuff.

@keyonjiekeyonjieNov 15, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, Pierre, agree on that, I suggest we change this here to sdev->hda->irq.


hda_dsp_stream_free(sdev);

iounmap(sdev->bar[HDA_DSP_BAR]);
iounmap(bus->remap_addr);

if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI))
hda_codec_i915_exit(sdev);

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this LGTM.

return 0;

@keyonjiekeyonjieNov 14, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are good, but I still like adding conditions such as 'if (bus->remap_addr)'.

@plbossartplbossartNov 14, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not necessary, you will not have a case where this is NULL since .remove is not called when .probe fails?

}

Expand Down
12 changes: 9 additions & 3 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -504,17 +504,23 @@ int hda_dsp_ctrl_init_chip(struct snd_sof_dev *sdev, bool full_reset);
/*
* HDA bus operations.
*/
int sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_ext_bus_ops *ext_ops);
void sof_hda_bus_init(struct hdac_bus *bus, struct device *dev,
const struct hdac_ext_bus_ops *ext_ops);

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
/*
* HDA Codec operations.
*/
int hda_codec_probe_bus(struct snd_sof_dev *sdev);

#if IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)
int hda_codec_i915_get(struct snd_sof_dev *sdev);
int hda_codec_i915_put(struct snd_sof_dev *sdev);
int hda_codec_i915_init(struct snd_sof_dev *sdev);
#endif
int hda_codec_i915_exit(struct snd_sof_dev *sdev);
#endif /* CONFIG_SND_SOC_HDAC_HDMI */

#endif /* CONFIG_SND_SOC_SOF_HDA */
/*
* Trace Control.
*/
Expand Down