Skip to content
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.

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

drivers: dai: intel: dmic: don't use assert for error handling #53394

Merged

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Dec 30, 2022

The dai.h interface does not prohibit calling dai_config_get() with different direction values. The dmic driver should handle invalid direction value explicitly and not rely on an assert.

Link: thesofproject/sof#6896
Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com

The dai.h interface does not prohibit calling dai_config_get()
with different direction values. The dmic driver should handle
invalid direction value explicitly and not rely on an assert.

Link: thesofproject/sof#6896
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@@ -743,7 +743,9 @@ static const struct dai_config *dai_dmic_get_config(const struct device *dev, en
{
struct dai_intel_dmic *dmic = (struct dai_intel_dmic *)dev->data;

__ASSERT_NO_MSG(dir == DAI_DIR_RX);
if (dir != DAI_DIR_RX)
Copy link
Collaborator

@marcinszkudlinski marcinszkudlinski Jan 2, 2023

Choose a reason for hiding this comment

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

Are you sure the exit value is checked everywhere?
i.e.

dai-zephyr.c, line 159:

image

dereferenced without checking for null

same in dai.c line 157:
image

Please add checking for null at every call to the function

EDIT: the references in question are in SOF code, not in zephyr, anyway NULL checking should be introduced prior to removing assert

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcinszkudlinski Ack, let me check. I was only fixing cases where the assert is already hit now (meaning you cannot actually enable asserts in builds to catch other errors). But true, need to check all cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@marcinszkudlinski both cases now handled thesofproject/sof#6897 and thesofproject/sof#6899

Given a) there are no Zephyr in-tree inproper usage and b) asserts are not enable by default (yet) in SOF builds, I'd propose we proceed and merge this fix.

@carlescufi carlescufi merged commit 8374325 into zephyrproject-rtos:main Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants