remoteproc_virtio: guard null notify and check return status - #705
Conversation
| { | ||
| int ret; | ||
|
|
||
| if (!rpvdev->notify) |
There was a problem hiding this comment.
I'd prefer to put this check way early where rpvdev is created. If the notify() callback is not provided, then probably fail there. This way, we force platforms to provide atleast stub function when notify() callback is not needed. This way, we don't have to make this check everytime.
As far as the error check goes, we should leave it to the platforms if they want to print error message or not on the failure. Here it's just printing a warning message and not taking any other action.
There was a problem hiding this comment.
agreed, the platform's own notify() implementation is better positioned to decide whether/how to report a failure than a generic warning at this layer. I'll drop the metal_log call.
On moving the check to vdev-creation time, want to flag that this reverses something explicit in the original discussion, #343. arnopo's comment states rpvdev->notify == 0 is valid when a platform doesn't use a mailbox to notify the remote processor. If we move the check to rproc_virtio_create_vdev() and fail there, any platform currently passing NULL for that reason would need to start supplying an explicit stub instead. That's a real behavior change for existing callers, not just a relocation of the same check.
I'm fine making that change if it's the direction you'd rather go, happy to enforce a non-NULL notify at creation time (in rproc_virtio_create_vdev()) and drop the per-call guard entirely. Just want to confirm that's the intended tradeoff before I do it.
There was a problem hiding this comment.
Since we cannot guarantee that polling mode is not used, we have to keep it. Otherwise, we should apply the deprecation process.
| { | ||
| int ret; | ||
|
|
||
| if (!rpvdev->notify) |
There was a problem hiding this comment.
Since we cannot guarantee that polling mode is not used, we have to keep it. Otherwise, we should apply the deprecation process.
cea87a4 to
60c17a0
Compare
|
Pushed an update: kept the NULL guard for polling mode, switched to METAL_LOG_ERROR, and dropped the rpvdev prefix from the message. Also rebased onto current main. PS: would the maintainers be open to an OpenAMP Discord server for broader, less formal conversations? Review threads are great for code, but design questions like the polling-mode/deprecation discussion here might fit a lower-friction channel. |
The channel already exists https://discord.com/channels/881957442341208095/1030188457739427911 |
d5e36cc to
0a76ec7
Compare
rpvdev->notify is called from four places in this file: virtqueue notify, set_status, set_features, and write_config. None guarded against notify being NULL, valid when no mailbox is configured, and none checked the returned status. Add a shared rpvdev_notify() helper used at all four sites: skip when notify is NULL, log a warning via metal_log on nonzero return. Exported notify signature is unchanged, per discussion in issue OpenAMP#343. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: Atul Akella <atul.akella@gmail.com>
0a76ec7 to
f82660f
Compare
3 participants
rpvdev->notifyis called from four places inremoteproc_virtio.c(rproc_virtio_virtqueue_notify,rproc_virtio_set_status,rproc_virtio_set_features,rproc_virtio_write_config). None guarded againstnotifybeing NULL, valid when no mailbox/transport is configured, and none checked the returned status, so a failed notify was silently dropped.Refs #343
Added a shared
rpvdev_notify()helper used at all four call sites: skip whennotifyis NULL, log a warning viametal_logon nonzero return.The original issue referenced a single call site, but after checking the current codebase I found the same gap in four places, so the fix is consolidated into one helper rather than four inline patches.