Skip to content

Minor fixes #4860

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

Closed
keyonjie wants to merge 3 commits into from
Closed

Minor fixes #4860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a9a4a1
ipc-common: add checks of buffer allocation return
keyonjie Oct 12, 2021
42ab3bd
platforms: return failure in case ipc_init() fails
keyonjie Oct 12, 2021
bd4b2da
dma-trace: avoid multiple allocations to fix fuzzy IPC
keyonjie Oct 10, 2021
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
5 changes: 5 additions & 0 deletions src/ipc/ipc-common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,13 @@ int ipc_init(struct sof *sof)

/* init ipc data */
sof->ipc = rzalloc(SOF_MEM_ZONE_SYS_SHARED, 0, SOF_MEM_CAPS_RAM, sizeof(*sof->ipc));
if (!sof->ipc)
return -ENOMEM;

sof->ipc->comp_data = rzalloc(SOF_MEM_ZONE_SYS_SHARED, 0,
SOF_MEM_CAPS_RAM, SOF_IPC_MSG_MAX_SIZE);
if (!sof->ipc->comp_data)
return -ENOMEM;
Copy link
Collaborator

@lyakh lyakh Oct 12, 2021

Choose a reason for hiding this comment

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

I think SOF_MEM_ZONE_SYS_SHARED works the same way as SOF_MEM_ZONE_SYS in that it panics if allocation fails. No need to check return value, this is never done on purpose.

Copy link
Contributor Author

@keyonjie keyonjie Oct 12, 2021

Choose a reason for hiding this comment

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

I think SOF_MEM_ZONE_SYS_SHARED works the same way as SOF_MEM_ZONE_SYS in that it panics if allocation fails. No need to check return value, this is never done on purpose.

We should not depend on the panic inside the allocator, I will merge the _SYS_SHARED zone to BUFFER zone in PR #4747.

Copy link
Collaborator

@lyakh lyakh Oct 13, 2021

Choose a reason for hiding this comment

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

we rely on them in many locations and I have been reminded about that at least twice and since then I myself repeated that request to others. I don't find this particularly pretty, but I also see the logic behind that design. However, if you move those allocations to a non-system zone, we might have more work to do to clean those up. I currently count more than 15 allocations from SOF_MEM_ZONE_SYS_SHARED in my local tree. If you really change them all, I think a separate patch would be good for that, addressing them all.
Also, those allocations are of type "allocate once at init and never free" so, maybe we can switch most or all of them to use .data or .bss?

Copy link
Collaborator

@kv2019i kv2019i Oct 13, 2021

Choose a reason for hiding this comment

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

@lyakh who corrected? this really seems to be a policy call. if SYS_SHARED has guaranteed allocations don't have to be checked (like glib in Linux, you panic if you fail to allocate), this should be followed universally. or if not, it should be the same all over the place.

This is basicly a fundamental design question for any reusable C code. Either you check allocs or you do not. Both are deployed successfully, but you have to stick to the plan. If you start mixing both approaches, then it gets really complicated.

Personally, I don't know the history, but I'm a bit skeptical of the usefulness of SYS/SYS_SHARED. Some code/checks is saved, but it seems non-trivial to enforce the policy when SYS/SYS_SHARED should be used. E..g IPC is really application code and one can argue whether it should use the SYS heap at all, or should it be limited to RTOS usage. OTOH, in most products using SOF, IPC is mandatory part of the system, so use of SYS makes sense, but this is now a product call. And so forth and forth, seems complex when scaled up to more and more products.

Copy link
Contributor Author

@keyonjie keyonjie Oct 13, 2021

Choose a reason for hiding this comment

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

Thanks for filing #4865 to track the TODOs, @lyakh .

@kv2019i agree that most of the sys/sys_shared usage today are actually runtime usage for us, imagine that those secondary cores related should be freed at powering down a specific core, DMA trace and be disabled. On the other hand, we reserve relative big zones for sys/sys_runtime for APL today, that's one of the reason that we are hitting OOM issues there.

Copy link
Member

@lgirdwood lgirdwood Oct 13, 2021

Choose a reason for hiding this comment

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

We do need to check now as Zephyr mode requires a check.

Copy link
Collaborator

@lyakh lyakh Oct 14, 2021

Choose a reason for hiding this comment

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

@kv2019i I'm sure this is the current official design: SYS/SYS_SHARED failures cause a panic, they must not be checked. And I haven't seen any violations of this in the code, if I had, I'd probably complain.

I'm not against changing that, but then we have to declare that change properly, not as a minor fix, and change it globally.

As I commented elsewhere in this PR - we need a formal API, once we have one, we can follow it.

Copy link
Member

@lgirdwood lgirdwood Nov 18, 2021

Choose a reason for hiding this comment

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

This is fine, we need to assume we are using Zephyr allocator and this could fail. No harm in xtos mode as we transition.


spinlock_init(&sof->ipc->lock);
list_init(&sof->ipc->msg_list);
Expand Down
5 changes: 4 additions & 1 deletion src/platform/amd/renoir/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,10 @@ int platform_init(struct sof *sof)
sizeof(sof->dma_info->dma_array), PLATFORM_DEFAULT_CLOCK, true);
scheduler_init_ll(sof->platform_dma_domain);
/* initialize the host IPC mechanisms */
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

/* initialize the DAI mechanisms */
ret = dai_init(sof);
if (ret < 0)
Expand Down
4 changes: 3 additions & 1 deletion src/platform/baytrail/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,9 @@ int platform_init(struct sof *sof)

/* initialise the host IPC mechanisms */
trace_point(TRACE_BOOT_PLATFORM_IPC);
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

trace_point(TRACE_BOOT_PLATFORM_DAI);
ret = dai_init(sof);
Expand Down
4 changes: 3 additions & 1 deletion src/platform/haswell/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,9 @@ int platform_init(struct sof *sof)

/* initialise the host IPC mechanisms */
trace_point(TRACE_BOOT_PLATFORM_IPC);
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

trace_point(TRACE_BOOT_PLATFORM_DAI);
ret = dai_init(sof);
Expand Down
4 changes: 3 additions & 1 deletion src/platform/imx8/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ int platform_init(struct sof *sof)
scheduler_init_ll(sof->platform_dma_domain);

/* initialize the host IPC mechanims */
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

ret = dai_init(sof);
if (ret < 0)
Expand Down
4 changes: 3 additions & 1 deletion src/platform/imx8m/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ int platform_init(struct sof *sof)
scheduler_init_ll(sof->platform_dma_domain);

/* initialize the host IPC mechanims */
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

ret = dai_init(sof);
if (ret < 0)
Expand Down
4 changes: 3 additions & 1 deletion src/platform/imx8ulp/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,9 @@ int platform_init(struct sof *sof)
scheduler_init_ll(sof->platform_dma_domain);

/* initialize the host IPC mechanims */
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

ret = dai_init(sof);
if (ret < 0)
Expand Down
4 changes: 3 additions & 1 deletion src/platform/intel/cavs/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,9 @@ int platform_init(struct sof *sof)

/* initialize the host IPC mechanisms */
trace_point(TRACE_BOOT_PLATFORM_IPC);
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

/* initialize IDC mechanism */
trace_point(TRACE_BOOT_PLATFORM_IDC);
Expand Down
4 changes: 3 additions & 1 deletion src/platform/mt8195/platform.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,9 @@ int platform_init(struct sof *sof)
scheduler_init_ll(sof->platform_dma_domain);

/* initialize the host IPC mechanims */
ipc_init(sof);
ret = ipc_init(sof);
if (ret < 0)
return ret;

ret = dai_init(sof);
if (ret < 0)
Expand Down
4 changes: 4 additions & 0 deletions src/trace/dma-trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,10 @@ int dma_trace_enable(struct dma_trace_data *d)
{
int err;

/* the existence of the trace buffer means the dma trace already initialized */
if (d->dmatb.addr)
Copy link
Collaborator

@marc-hb marc-hb Oct 14, 2021

Choose a reason for hiding this comment

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

There is a safer dma_trace_initialized()

Copy link
Collaborator

@marc-hb marc-hb Oct 15, 2021

Choose a reason for hiding this comment

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

@ujfalusi , @plbossart , others: there is no IPC to _disable() the trace, correct? I found some IPC only for trace_off() but none to unwind this.

Just making sure running dma_trace_enable() a second time is never a valid use case and that this fix is purely "defensive programming". If correct the comment should say that this is never expected.

Copy link
Contributor

@ujfalusi ujfalusi Oct 15, 2021

Choose a reason for hiding this comment

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

@ujfalusi , @plbossart , others: there is no IPC to _disable() the trace, correct?

It is under work by @ranj063.

I found some IPC only for trace_off() but none to unwind this.

Just making sure running dma_trace_enable() a second time is never a valid use case and that this fix is purely "defensive programming". If correct the comment should say that this is never expected.

It is going to be valid with the SOF client support, but if we have the #4849 then yes, this is purely safe coding against broken or old host

return 0;
Copy link
Contributor

@ujfalusi ujfalusi Oct 15, 2021

Choose a reason for hiding this comment

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

This is not correct, you must proceed to dma_trace_start() to get the new buffer parameters into use and reconfigure the channel.

if (!d->dmatb.addr) {
	err = dma_trace_buffer_init(d);

	if (err < 0) {
		mtrace_printf(LOG_LEVEL_ERROR, "dma_trace_enable: buffer_init failed");
		goto out;
	}
}

Copy link
Contributor

@ujfalusi ujfalusi Oct 15, 2021

Choose a reason for hiding this comment

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

@keyonjie, @marc-hb, see my PR to fix this memory leak and allow the dtrace re-configuration: #4879

Copy link
Contributor Author

@keyonjie keyonjie Oct 18, 2021

Choose a reason for hiding this comment

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

@keyonjie, @marc-hb, see my PR to fix this memory leak and allow the dtrace re-configuration: #4879

Thanks @ujfalusi. This was reported by CI fuzzy IPC test, is it OK to leave this to you together with your dtrace refinement? If so, let me remove it from here.


/* initialize dma trace buffer */
err = dma_trace_buffer_init(d);

Expand Down