Skip to content

Conversation

@plbossart
Copy link
Member

This is an untested WIP patchset to gather feedback on the HDaudio error flow and remove capabilities. The next step will be to make sure all the streams and links are stopped correctly on remove (currently kernel oops 100% of the time...)

Copy link

@keyonjie keyonjie left a comment

Choose a reason for hiding this comment

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

Thanks for re-visit this part, some comments inline.

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.

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().

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].

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;"

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.

Choose a reason for hiding this comment

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

should be sdev->hda->irq, though they are actually same values here.
Considering when we are calling pci_irq_vector() with nr!=0, there might be offset there.

Choose a reason for hiding this comment

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

ditto.

Copy link
Member

@lgirdwood lgirdwood Nov 14, 2018

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.

Copy link
Member Author

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.

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.

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)'.

Copy link
Member Author

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?

Choose a reason for hiding this comment

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

this LGTM.

The GPROCEN bit is only set when there is no error path, so don't
clear it on all errors.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
This function returns zero always, change return type to void

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add new goto to handle possible errors and unmap bar

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Jump to correct goto instead of just returning -ENXIO

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
The comments make no sense, the streams are not freed on error. Fix.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add label (not currently used) which is always reached with free_streams

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow Keyon'd recommendation

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Follow same pattern as for error handling on probe

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Add missing exit and get/put helpers.

It's assumed that the device will not be active on exit - so we don't
need to decrease the refcount.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Start dealing with resource free to enable module load/unload

FIXME: is this necessary or is this handled by the _suspend cases?

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
@plbossart plbossart changed the title [RFC] Fix HDaudio probe/remove Fix HDaudio probe/remove Nov 17, 2018
@plbossart plbossart merged commit e5d6e44 into thesofproject:topic/sof-dev Nov 17, 2018
@plbossart plbossart deleted the fix/hda_init_free branch November 17, 2018 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants