risc-v/mpfs: ihc: cleanup DEBUGASSERTs and reboot #183
Conversation
|
I would convert the PANIC() into simply return; where possible. If there is a clear error and the code should not continue, then PANIC() is the correct operation, but when an incoming parameter is invalid, it is better to just not do the operation. I can figure out two reasons for doing the return instead of panic:
|
eenurkka
force-pushed
the
rpmsg-cleanup
branch
from
502bf10 to
ec30a81
Compare
|
Not very familiar with the driver but LGTM now ! |
eenurkka
force-pushed
the
rpmsg-cleanup
branch
2 times, most recently
from
b1267e9 to
586132b
Compare
Replace DEBUGASSERTs with sanity checks. DEBUGASSERT()s are not necessarily enabled at all, thus risking the functionality especially in that case. Remove PANICs as well. Don't enable the ihc irq too early. If enabled, and the master is already up, the irq is being issued so that the system gets stuck or is severely slowed down. Master may be already if this NuttX hart is rebooted, for example. Signed-off-by: Eero Nurkkala <[email protected]>
eenurkka
force-pushed
the
rpmsg-cleanup
branch
from
586132b to
bdb1359
Compare
| @@ -1256,8 +1284,12 @@ static void mpfs_rptun_worker(void *arg) | |||
| return; | |||
| } | |||
|
|
|||
| DEBUGASSERT((g_vq_idx - VRING0_NOTIFYID) < VRINGS); | |||
| info = &g_mpfs_virtqueue_table[g_vq_idx - VRING0_NOTIFYID]; | |||
| if (g_vq_idx >= VRINGS) | |||
There was a problem hiding this comment.
This is safe as it is. I'm just wondering, does it no longer support 2 channels / CONFIG_MPFS_IHC_RPMSG_CH2 (VRINGS = 2, VRING0_NOTIFYID = 2 or VRING1_NOTIFYID = 3) ? Not even sure we're ever going to need it, just out of curiosity.
There was a problem hiding this comment.
I was thinking of that also, but as 2x channels are currently not needed / hard to test, it's there waiting to be fixed if it will ever be needed. Maybe a comment should be written there.
Replace DEBUGASSERTs with sanity checks. DEBUGASSERT()s are not necessarily enabled at all, thus risking the functionality especially in that case.
Don't enable the ihc irq too early. If enabled, and the master is already up, the irq is being issued so that the system gets stuck or is severely slowed down. Master may be already if this NuttX hart is rebooted, for example.
Summary
DP-7330 (partial fix)
DP-6809 (complete fix)
Impact
Testing
SalukiV2 / AMP mode with kernel / ping -A and rebooting NuttX