Rename IS_ALIGN() to SOF_IS_ALIGN() #8755

marc-hb · 2024-01-17T23:32:57Z

This is the simplest, cheapest and safest to avoid the collision with
the incoming Zephyr macro IS_ALIGN() submitted in
zephyrproject-rtos/zephyr#67243

As SOF depends on Zephyr, most SOF symbols should be prefixed SOF_ or
something specific.

This is compliant with:
https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-3-macro-name-collisions

Signed-off-by: Marc Herbert [email protected]

This reverts commit 0a3b816. Opportunistic, `#ifndef` definitions make macro troubleshooting even more of a nightmare than it already is. https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-3-macro-name-collisions forbids #ifndef in Zephyr. Signed-off-by: Marc Herbert <[email protected]>

This is the simplest, cheapest and safest to avoid the collision with the incoming Zephyr macro IS_ALIGN() submitted in zephyrproject-rtos/zephyr#67243 As SOF depends on Zephyr, most SOF symbols should be prefixed SOF_ or something specific. This is compliant with: https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-3-macro-name-collisions Signed-off-by: Marc Herbert <[email protected]>

marc-hb · 2024-01-17T23:45:11Z

Failures to compile with the Zephyr main branch are known and unrelated:
https://github.com/thesofproject/sof/actions/runs/7562861040/job/20594168710?pr=8754

implicit declaration of function 'z_smp_thread_init';

They're caused by backwards-incompatible change:

kernel: introduce k_smp_cpu_start() and k_smp_cpu_resume() zephyrproject-rtos/zephyr#64755

marc-hb · 2024-01-18T02:03:14Z

Unrelated pause-resume failure in ACE https://sof-ci.01.org/sofpr/PR8755/build1982/devicetest/index.html . pause-resume is not reliable right now. Maybe #8651?

Unrelated FW reported error: 104 - Other failure of module instance initialization request in https://sof-ci.01.org/sofpr/PR8755/build1983/devicetest/index.html, thesofproject/linux#4832

checkpatch https://github.com/thesofproject/sof/actions/runs/7563103579/job/20594859309?pr=8755 complains about the URL being too long.

Everything else green.

lyakh

We want to use native Zephyr APIs and this seems to be taking us away from it

lyakh · 2024-01-18T09:52:57Z

src/include/sof/common.h

 #define IS_ALIGNED(size, alignment) ((size) % (alignment) == 0)
-#endif


if zephyrproject-rtos/zephyr#67243 happens to be merged before this (yes, I know that it's currently blocked) and we move to a Zephyr version after it, this would break compilation. Why not just merge these two commits?

we probably should hold of until we can get the Zephyr interface merged in. We have the SMP interface in flight (plus the regression), so the air is getting too thick with incompatible combinations.

lyakh · 2024-01-18T09:54:49Z

src/audio/dai-zephyr.c

@@ -633,7 +633,7 @@ static int dai_set_sg_config(struct dai_data *dd, struct comp_dev *dev, uint32_t
 period_count);
 buf_size = period_count * period_bytes;
 do {
- if (IS_ALIGNED(buf_size, max_block_count)) {
+ if (SOF_IS_ALIGNED(buf_size, max_block_count)) {


is it the plan then after zephyrproject-rtos/zephyr#67243 is merged to move to the "proper" Zephyr-provided IS_ALIGNED() and to rename back?

I think an alternative would be to take the Zephyr change with IS_ALIGNED in, then add IS_ALIGNED definition only for xtos builds in SOF, and call it a day.

The current ifdef can carry us until then.

Or we merge this, but I agree this is going against our long term plans. We want to use native Zephyr interfaces in generic SOF code whenever possible (e.g. #5794).

kv2019i

I think this we should consider also moving this to rtos abstraction and keep using native interfaces.

kv2019i · 2024-01-18T11:35:11Z

src/include/sof/common.h

 #define IS_ALIGNED(size, alignment) ((size) % (alignment) == 0)
-#endif


we probably should hold of until we can get the Zephyr interface merged in. We have the SMP interface in flight (plus the regression), so the air is getting too thick with incompatible combinations.

kv2019i · 2024-01-18T11:35:23Z

src/audio/dai-zephyr.c

@@ -633,7 +633,7 @@ static int dai_set_sg_config(struct dai_data *dd, struct comp_dev *dev, uint32_t
 period_count);
 buf_size = period_count * period_bytes;
 do {
- if (IS_ALIGNED(buf_size, max_block_count)) {
+ if (SOF_IS_ALIGNED(buf_size, max_block_count)) {


I think an alternative would be to take the Zephyr change with IS_ALIGNED in, then add IS_ALIGNED definition only for xtos builds in SOF, and call it a day.

The current ifdef can carry us until then.

Or we merge this, but I agree this is going against our long term plans. We want to use native Zephyr interfaces in generic SOF code whenever possible (e.g. #5794).

marc-hb

We want to use native Zephyr APIs and this seems to be taking us away from it

In theory yes. In practice these are 4 lines in 2 files.

Every time we want to use a "Zephyr API", we have to implement some indirection logic for XTOS that we (Intel) can't test any more.

Calling this one-liner macro a "Zephyr API" is blowing it out of proportion.

I'm aware the "air is getting thick" and this is precisely why this PR is the simplest, cheapest and safest solution in the short term. I understand and agree in general but this particular example is just 4 lines; let's not over-engineer it.

marc-hb · 2024-01-19T00:06:35Z

In theory yes. In practice these are 4 lines in 2 files.

4 lines including... 3 lines in unused and never compiled kpb.c. I had to change the defconfig to compile-test it.

I think we have much bigger problems to solve than making this one-liner macro used once "API policy compliant"

lyakh

Sorry, no, I don't like this. I really don't think we should add a SOF_* version for every Zephyr macro. We want to use them directly and for non-Zephyr builds define them in compatibility headers.

marc-hb · 2024-01-19T15:55:32Z

I really don't think we should add a SOF_* version for every Zephyr macro.

I forgot how many times I wrote "just this one, with have bigger problems"

We want to use them directly and for non-Zephyr builds define them in compatibility headers.

Agreed, please send a better (and tested) PR since you have the time.

marc-hb · 2024-01-19T16:19:19Z

After a chat with @lyakh I realized I forgot to explicit the main concern behind this PR: if the SOF definition is guarded with the (evil) #ifndef and the Zephyr definition is not (cause it's evil), then compilation success depends on which order the .h files are included. Anything that depends on inclusion order is of course super evil too.

I admit I didn't check whether inclusion order this an actual problem right now, it was much faster (and safer) to submit this PR. Actually testing this inclusion order problem could require testing multiple defconfig variations again.

Even if not today, the inclusion order could very easily become a real problem in any future refactoring. Hence this dead simple PR to move on with our lives.

lyakh · 2024-01-19T16:23:29Z

After a chat with @lyakh I realized I forgot to explicit the main concern behind this PR: if the SOF definition is guarded with the (evil) #ifndef and the Zephyr definition is not (cause it's evil), then compilation success depends on which order the .h files are included. Anything that depends on inclusion order is of course super evil too.

I admit I didn't check whether inclusion order this an actual problem right now, it was much faster (and safer) to submit this PR. Actually testing this inclusion order problem could require testing multiple defconfig variations again.

Even if not today, the inclusion order could very easily become a real problem in any future refactoring. Hence this dead simple PR to move on with our lives.

@marc-hb no, there's no order problem there. The header is already included before the #ifndef:

sof/src/include/sof/common.h

Lines 17 to 24 in 0a3b816

 #ifdef __ZEPHYR__ 

 #include <zephyr/sys/util.h> 

 #endif 

 /* Align the number to the nearest alignment value */ 

 #ifndef IS_ALIGNED 

 #define IS_ALIGNED(size, alignment) ((size) % (alignment) == 0) 

 #endif

marc-hb · 2024-01-19T17:37:19Z

OK, let's not worry then.

marc-hb added 2 commits January 17, 2024 23:31

marc-hb marked this pull request as ready for review January 17, 2024 23:45

marc-hb requested review from fkwasowi, iganakov, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners January 17, 2024 23:45

marc-hb mentioned this pull request Jan 17, 2024

common.h: Guard definition of IS_ALIGNED for Zephyr builds #8708

Merged

marc-hb requested a review from andyross January 17, 2024 23:49

lyakh reviewed Jan 18, 2024

View reviewed changes

kv2019i reviewed Jan 18, 2024

View reviewed changes

marc-hb commented Jan 18, 2024

View reviewed changes

lyakh requested changes Jan 19, 2024

View reviewed changes

marc-hb closed this Jan 19, 2024

marc-hb mentioned this pull request Jan 28, 2024

sys/util.h: Add IS_ALIGNED macro zephyrproject-rtos/zephyr#67243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename IS_ALIGN() to SOF_IS_ALIGN() #8755

Rename IS_ALIGN() to SOF_IS_ALIGN() #8755

marc-hb commented Jan 17, 2024

marc-hb commented Jan 17, 2024

marc-hb commented Jan 18, 2024 •

edited

Loading

lyakh left a comment

lyakh Jan 18, 2024

kv2019i Jan 18, 2024

lyakh Jan 18, 2024

kv2019i Jan 18, 2024

kv2019i left a comment

kv2019i Jan 18, 2024

kv2019i Jan 18, 2024

marc-hb left a comment •

edited

Loading

marc-hb commented Jan 19, 2024 •

edited

Loading

lyakh left a comment

marc-hb commented Jan 19, 2024

marc-hb commented Jan 19, 2024 •

edited

Loading

lyakh commented Jan 19, 2024

marc-hb commented Jan 19, 2024

		#define IS_ALIGNED(size, alignment) ((size) % (alignment) == 0)
		#endif

Rename IS_ALIGN() to SOF_IS_ALIGN() #8755

Rename IS_ALIGN() to SOF_IS_ALIGN() #8755

Conversation

marc-hb commented Jan 17, 2024

marc-hb commented Jan 17, 2024

marc-hb commented Jan 18, 2024 • edited Loading

lyakh left a comment

Choose a reason for hiding this comment

lyakh Jan 18, 2024

Choose a reason for hiding this comment

kv2019i Jan 18, 2024

Choose a reason for hiding this comment

lyakh Jan 18, 2024

Choose a reason for hiding this comment

kv2019i Jan 18, 2024

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i Jan 18, 2024

Choose a reason for hiding this comment

kv2019i Jan 18, 2024

Choose a reason for hiding this comment

marc-hb left a comment • edited Loading

Choose a reason for hiding this comment

marc-hb commented Jan 19, 2024 • edited Loading

lyakh left a comment

Choose a reason for hiding this comment

marc-hb commented Jan 19, 2024

marc-hb commented Jan 19, 2024 • edited Loading

lyakh commented Jan 19, 2024

marc-hb commented Jan 19, 2024

marc-hb commented Jan 18, 2024 •

edited

Loading

marc-hb left a comment •

edited

Loading

marc-hb commented Jan 19, 2024 •

edited

Loading

marc-hb commented Jan 19, 2024 •

edited

Loading