boards: drop board_init() from board.h

[benpicco]

Contribution description

This removes the per-board declaration of board_init() and places it in kernel_init.h next to kernel_init().

Testing procedure

Issues/PRs references

split off #17008
similar to #16013

[chrysn]

Nice how the declaration in kernel_init.h appears to resolve all the trouble I've had and never resolved in #16013.

Two extern int _...; are removed in the native version, but I checked that they are not used anywhere else, so they're good to go. (Could be committed separately, but then again it's an obvious cleanup. They were probably missed when the extern introduced in 864267f and its use discontinued in 0e4386a).

I reviewed this in code changes alone and did not test it -- but I don't see any realistic and not-already-highly-nondeterministic possibility that something here results in a runtime change but not a build failure.

[fjmolinas]

@benpicco I don't like how this PR was merged, in #17008 I expressed that I was unsure that kernel_init.h was the right place for this function declaration #17008 (comment), in the matrix I also brought this up, so just merging this as soon as the first ACK was in seems does not seem like a good way to proceed. I'm not, in any case, asking for a revert or anything like that, I just want to state that I don't like how this was handled.

[chrysn]

I was unaware of that comment and didn't go through all details of 17008 in my review. Sorry for the oversight. A comment on this thread might have helped me find the concern, but so would have a more thorough read of the related issues.

I don't see this big an importance in where it is placed: Sure it makes it visible to users of kernel_init.h, but then again, nobody outside the single location in startup would ever call board_init anyway. (Unlike the various other init functions that may need to be called when some auto-init stuff is disabled). That is not to play down the concern, but to remove urgency: It's not like any new caller of board_init will pop up for the duration this sits in kernel_init.h that will later be missing the declaration.

Quite some value of the declaration is documentation (and now that there's a central location to document, I'd like to extend it or at least reference to the existing docs). What does it do outside of docs? It ensures that no extern void board_init(void); are needed at the users' site (there are already a few), and makes warnings pop up when an explicit implementer messes up the signature. The user case suggests to put it into a dedicated header (say, board_init.h), but only weakly (as all users of one also use the other). The latter case of the implementers indicates that it may be good to place it where it is included commonly (because implementing the function w/o a preceding forward declaration is not something the compiler would complain about). Inconveniently, there doesn't appear to be a common include from board.h files (cpu.h is included often, but that sounds weird).

So my suggestion would be a new board_init.h -- but it's all weak cases. I wouldn't hope for strong consensus on where to place it (I expect many "don't care"s), but at least a strong opinion would be good before seettling on a location (and mine is weak).

[benpicco]

@fjmolinas sorry I didn't understand your comment as a general objection to place it in kernel_init.h but rather a concern that @chrysn opted for board_init.h and you wanted his option on this too - so when he ACKed this PR I assumed all is fine.

Anyway I don't think the location matters much - what do you propose?

[kaspar030]

what do you propose?

Enable the branch protection that forces all conversations of a PR to be "resolved". Just so we don't accidentally forget concerns.

From my side, I think the location really doesn't matter that much, and this is a single line void foo(void) define that really doesn't hurt. I think a dedicated header would hurt more.

[benpicco]

Enable the branch protection that forces all conversations of a PR to be "resolved". Just so we don't accidentally forget concerns.

I have a feeling we are more likely to accidentally forget PRs than to forget concerns…

[chrysn]

Enable the branch protection that forces all conversations of a PR to be "resolved". Just so we don't accidentally forget concerns.

Wouldn't have helped here because the comment was in another issue.

I have no strong preference whether or not open comments should stop merging, because objections can be encoded either in threads-left-unresolved or in reviews-requesting-changes.

[fjmolinas]

I'm fine with the changes in this PR, my comment was solely to point out that I thought concerns I raised in the previous PR that was doing the same were ignored, and since the PR was opened on Friday, it didn't really give me time to comment again on it.

But as @benpicco explained this was just a misunderstanding, no hard feelings here, just thought the comment needed to be made. I think the lesson is that when a PR is opened spawning from another person that opens the PR as well the reviewer could try to resume the status of the previous PR in the PR description (best effort, of course).

Branch protection on all conversations resolved would not have helped here, and I don't think it really needed, I think as a community we usually never disregard comments made in a PR.

[kaspar030]

ok, I see the extra branch protections feel like more complexity where there should be less.