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

Intrng: adjusting pic_init_secondary for further use cases #1280

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ehem
Copy link
Contributor

@ehem ehem commented Jun 7, 2024

Issue is pic_init_secondary() was implemented for root PICs-only. The author's theory appears to have been the root calls the function on child PICs. At least one secondary PIC needs the function called, but the device-tree doesn't have it as a child of the root PIC. As such instead simply calling the function on all registered PICs appears to make more sense.

@ehem
Copy link
Contributor Author

ehem commented Jun 7, 2024

The conversion from SLIST to STAILQ is since RISC-V is expected to need the functionality and have it called on the first registered PIC before secondary ones. The driver I have on-hand doesn't need this, but since it was brought up with D40474 I have no concern with this (which my case didn't need this, I foresaw others needing it).

There was an objection to the initial implementation, but no updates have been seen in Phabricator, so I'm dumping this on GitHub which seems better at getting these pushed through.

@ehem
Copy link
Contributor Author

ehem commented Jun 11, 2024

This was simply rebased to aid keeping track of local branches, everything is identical to the original. Notably the CI pass is still valid.

@ehem
Copy link
Contributor Author

ehem commented Jun 12, 2024

Note to @jrtc27, you had pointed to an issue and objected in Phabricator. This has a simple solution to the issue, but you never updated in Phabricator. I'm unsure how GitHub handles mentions, but your review would be appropriate.

@ehem ehem force-pushed the intrng branch 3 times, most recently from 3256e6c to 2cdd3aa Compare June 22, 2024 04:23
@ehem
Copy link
Contributor Author

ehem commented Jul 23, 2024

I'm planning to update to FreeBSD's HEAD soon. There will be a rebase in a few hours.

@ehem
Copy link
Contributor Author

ehem commented Sep 12, 2024

The issue in the final commit is still there. The present mechanism is intr_pic_init_secondary() calls PIC_INIT_SECONDARY() then those calls need to propagate the call. Unfortunately only the GICv3 driver attempts to do this and the default implementation is a NOP. Additionally this requires all PICs to be presented as children of the root PIC and trees for some devices don't match this assumption.

As the comment in intr_pic_init_secondary() says,

QQQ: Only root PIC is aware of other CPUs ???
Alas with theoretical objections to the proposal and no suggestions for alternatives, despite known real world cases, I'm rather stuck.

Good news is #1363 can be leveraged as an alternative for the case I'm looking at, but I imagine someone else is going to end up stuck here in the future. Since I've got a workaround, I guess I have to leave this as a draft.

Switch to INTR_ROOT_COUNT as this better matches the purpose of the
value.

Remove the default from the core.  Better to require the architectures
to declare the type since they will routinely deviate and a default
chosen now will likely be suboptimal.
@ehem ehem marked this pull request as ready for review November 5, 2024 01:16
There is potential for non-root PICs to need per-processor
initialization.  Few root PICs try to propogate the call.  As such add
a pass of calling pic_init_secondary() on all children with a non-root
value.

Differential Revision: https://reviews.freebsd.org/D40474
@ehem
Copy link
Contributor Author

ehem commented Nov 5, 2024

Given how things are shaping up, this might be superior for my purpose. Alas as the comments mention, what about setup for non-PIC devices with PPI interrupts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant