OpenAMP / open-amp

The main OpenAMP library implementing RPMSG, Virtio, and Remoteproc for RTOS etc
https://www.openampproject.org/
Other
721 stars 291 forks source link

rpmsg_virtio: rpmsg_deinit_vdev: Add check for empty endpoint list #605

Closed bentheredonethat closed 2 months ago

bentheredonethat commented 3 months ago

There can exist scenario where the endpoint list in a rpmsg vdev can be empty of endpoint-created nodes but the original remains, check if that is the case to ensure the while loop can properly terminate.

bentheredonethat commented 3 months ago

CC @tnmysh

wmamills commented 3 months ago

@bentheredonethat can you describe the scenario that where this arises?

bentheredonethat commented 3 months ago

I see this as bug in rpmsg_deinit_vdev because the initial list head is created before rpsmg-create endpoint and this adds check for that initial list head node.

bentheredonethat commented 3 months ago

I took a further look. Because there can exist independtly the RPMsg vdev's ns_ept I believe that the alternative solution of moving the RPMsg vdev's list of endpoints to create endpoint is not valid.

Given that, I think keeping the logic as is (the existing patch) is valid.

CC @wmamills

arnopo commented 2 months ago

The point is not clear to me I cannot figure out how we can have "node->next == NULL" regarding metal_list_XXX macro after metal_list_init(&rdev->endpoints)

    rdev->endpoints.prev = rdev->endpoints;
    rdev->endpoints.next = rdev->endpoints;

After rpmsg_register_endpoint(rdev, &rdev->ns_ept,, ...)

    rdev->endpoints.prev =  rdev->ns_ept->node;
    rdev->endpoints.next =  rdev->ns_ept->node;

    rdev->ns_ept->node.prev =  rdev->endpoints;
    rdev->ns_ept->node.next =  rdev->endpoints;

After metal_list_del(&ept->node)

    rdev->endpoints.prev =   rdev->endpoints;
    rdev->endpoints.next =   rdev->endpoints;

   rdev->ns_ept->node =  rdev->ns_ept->node;
   rdev->ns_ept->node =  rdev->ns_ept->node;

Please could you detail the sequence?

bentheredonethat commented 2 months ago

we can skip this after further inspection. will close