coccinelle: final pass standardizing integer timeout parameters #19650

pabigot · 2019-10-07T12:57:22Z

(At least I think this is final, but I've thought that before.)

This PR like the others is best reviewed at the commit level, as there are a lot of rote changes. Only the changes to the .cocci script are by hand.

Three features in this:

Fixed a few errors in the existing regular expression intended to detect API calls that passed a timeout in the last parameter. No errors resulted in incorrect conversion in previous PRs, and only one missed function resulted in a new conversion.
Convert k_sleep to take timeouts. There's been some discussion of whether this should remain milliseconds, but the consensus was that being able to use explicit duration macros like K_MSEC and K_SECONDS was highly desired. The scale tipped to "timeout" for me when I discovered that k_sleep() specifically relies on the thread timeout infrastructure (it's a no-op if threads are disabled). If the consensus switches again, those commits can be dropped.

I have a proposal in Generalizing duration representation in clock-agnostic way #19656 that would preserve use those API macros for time-defined timeouts even when New timeout API #17155 revisions are done so unifying it now seems to make sense. If that proposal doesn't fly we'd still want to replace these.
Convert static and runtime thread start delays to timeouts. Most static definitions did this already, but it took me a while to figure out how to make Coccinelle convert the missing ones.

zephyrbot · 2019-10-07T13:27:15Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:3065: WARNING:LONG_LINE_COMMENT: line over 80 characters
#3065: FILE: tests/kernel/mutex/sys_mutex/src/main.c:348:
+	k_sleep(K_MSEC(1));     /* Give thread_12 a chance to block on the mutex */

- total: 0 errors, 1 warnings, 3079 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

nvlsianpu

looks good in my areas

nashif · 2019-10-08T21:18:05Z

.shippable.yml

@@ -6,13 +6,28 @@ env:
    global:


please remove this change, we will need to override CI in this case..

nashif · 2019-10-08T21:20:37Z

tests/kernel/mbox/mbox_api/src/test_mbox_api.c

@@ -419,7 +419,8 @@ static void tmbox_get(struct k_mbox *pmbox)
 			     NULL);


Please put this with other code change and separate it from the coccinelle script change.

Sort the functions within the regular expression so they can be checked more easily. Remove k_thread_deadline_set as it takes an argument in cycles. (The one in-tree call to this function was not affected by this error.) Add missed k_mbox_data_block_get. Fix an overly ambitious multi-match disjunct that covered some non-existent functions. Signed-off-by: Peter Bigot <[email protected]>

Re-run with updated script to convert integer literal delay arguments to k_mbox_data_block_get to use the standard timeout macros. Signed-off-by: Peter Bigot <[email protected]>

k_sleep uses the same underlying thread infrastructure as the other functions that take timeouts, so the delay should be specified as a timeout rather than milliseconds. Signed-off-by: Peter Bigot <[email protected]>

Re-run with updated script to convert integer literal delay arguments to k_sleep to use the standard timeout macros. Signed-off-by: Peter Bigot <[email protected]>

…tion k_thread_create and K_THREAD_DEFINE both take a delay as the final parameter. Most uses of K_THREAD_DEFINE pass either `K_NO_WAIT` or `K_FOREVER`. Ensure that all uses of K_THREAD_DEFINE follow that practice, and that the runtime k_thread_create calls do so as well. Signed-off-by: Peter Bigot <[email protected]>

…eouts Re-run with updated script to convert integer literal delay arguments to k_thread_create and K_THREAD_DEFINE to use the standard timeout macros. Signed-off-by: Peter Bigot <[email protected]>

pabigot · 2019-10-08T21:35:47Z

@nashif shippable patch removed, API change separated, rebased on current master (no new changes were required).

vanwinkeljan

display related changes LGTM

aescolar

No complaints for the few files I own.

pabigot mentioned this pull request Oct 7, 2019

Milestones toward generalized representation of timeouts #19655

Closed

5 tasks

pabigot force-pushed the cocci/20191006 branch from e888dbf to bffd4fc Compare October 7, 2019 23:05

nvlsianpu approved these changes Oct 8, 2019

View reviewed changes

pabigot mentioned this pull request Oct 8, 2019

nrf: clock reimplementation breaks test #19683

Closed

joerchan approved these changes Oct 8, 2019

View reviewed changes

pabigot force-pushed the cocci/20191006 branch from bffd4fc to 4bbcdd0 Compare October 8, 2019 14:31

nashif requested changes Oct 8, 2019

View reviewed changes

.shippable.yml Outdated

@@ -6,13 +6,28 @@ env:

global:

Copy link

Member

nashif Oct 8, 2019

Choose a reason for hiding this comment

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

please remove this change, we will need to override CI in this case..

nashif requested changes Oct 8, 2019

View reviewed changes

pabigot force-pushed the cocci/20191006 branch from 4bbcdd0 to 02c2427 Compare October 8, 2019 21:33

pabigot added 6 commits October 8, 2019 16:35

coccinelle: standardize k_mbox_data_block_get call with timeout

aa19c99

Re-run with updated script to convert integer literal delay arguments to k_mbox_data_block_get to use the standard timeout macros. Signed-off-by: Peter Bigot <[email protected]>

coccinelle: standardize k_sleep calls with integer timeouts

5d359e9

Re-run with updated script to convert integer literal delay arguments to k_sleep to use the standard timeout macros. Signed-off-by: Peter Bigot <[email protected]>

coccinelle: standardize k_thread create/define calls with integer tim…

ca34569

…eouts Re-run with updated script to convert integer literal delay arguments to k_thread_create and K_THREAD_DEFINE to use the standard timeout macros. Signed-off-by: Peter Bigot <[email protected]>

pabigot force-pushed the cocci/20191006 branch from 02c2427 to ca34569 Compare October 8, 2019 21:35

vanwinkeljan approved these changes Oct 9, 2019

View reviewed changes

jfischer-no approved these changes Oct 9, 2019

View reviewed changes

aescolar approved these changes Oct 9, 2019

View reviewed changes

nashif approved these changes Oct 9, 2019

View reviewed changes

nashif merged commit e28f330 into zephyrproject-rtos:master Oct 9, 2019

pabigot deleted the cocci/20191006 branch October 15, 2019 10:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

coccinelle: final pass standardizing integer timeout parameters #19650

coccinelle: final pass standardizing integer timeout parameters #19650

pabigot commented Oct 7, 2019 •

edited

Loading

zephyrbot commented Oct 7, 2019 •

edited

Loading

nvlsianpu left a comment

nashif Oct 8, 2019

nashif Oct 8, 2019

pabigot commented Oct 8, 2019

vanwinkeljan left a comment

aescolar left a comment

		@@ -419,7 +419,8 @@ static void tmbox_get(struct k_mbox *pmbox)
		NULL);

coccinelle: final pass standardizing integer timeout parameters #19650

coccinelle: final pass standardizing integer timeout parameters #19650

Conversation

pabigot commented Oct 7, 2019 • edited Loading

zephyrbot commented Oct 7, 2019 • edited Loading

checkpatch (informational only, not a failure)

nvlsianpu left a comment

Choose a reason for hiding this comment

nashif Oct 8, 2019

Choose a reason for hiding this comment

nashif Oct 8, 2019

Choose a reason for hiding this comment

pabigot commented Oct 8, 2019

vanwinkeljan left a comment

Choose a reason for hiding this comment

aescolar left a comment

Choose a reason for hiding this comment

pabigot commented Oct 7, 2019 •

edited

Loading

zephyrbot commented Oct 7, 2019 •

edited

Loading