rpmsg: fix stale src address causing rebind to fail depending on order#3363
Conversation
|
This is a continuation of #3257, which was merged into adsp-6.12.0-y, but it should first go mainline since it touches purely upstream code |
| rpdrv->remove(rpdev); | ||
| destroy_ept: | ||
| if (ept) | ||
| rpmsg_destroy_ept(ept); |
There was a problem hiding this comment.
Does rpdev->ept also need a = NULL here as in remove() below? And perhaps also rpdev->src?
| if (rpdev->ept) | ||
| if (rpdev->ept) { | ||
| rpmsg_destroy_ept(rpdev->ept); | ||
| rpdev->ept = NULL; |
There was a problem hiding this comment.
Looks like this change isn't documented in the commit message
When a channel gets unbound, rpmsg_dev_remove() destroys the endpoint and frees the IDR slot, but forgets to reset rpdev->src back to RPMSG_ADDR_ANY. So the next time the channel gets bound, rpmsg_dev_probe() passes the old stale address as a specific IDR request. If another channel snached that address in the meantime, idr_alloc() fails with -ENOSPC (-28) and probe blows up with: rpmsg_chrdev: idr_alloc failed: -28 rpmsg_chrdev: failed to create endpoint rpmsg_chrdev: probe with driver rpmsg_chrdev failed with error -12 This makes the sucess of bind/unbind sequences order-dependent when it really shouldn't be. Fix: reset rpdev->src to RPMSG_ADDR_ANY after tearing down the endpoint, but only for channels with dynamically-assigned addresses (!rpdev->announce). Channels with a predefined local address need to keep their src so they re-announce the same address on rebind. Since the rpmsg_device now outlives the unbind and is re-probed on the next bind, also clear rpdev->ept after destroying the endpoint so the stale pointer isn't left dangling for the next probe to trip over. Fixes: bbd1880 ("rpmsg: Support drivers without primary endpoint") Signed-off-by: Vasileios Bimpikas <Vasileios.Bimpikas@analog.com>
| * claimed by another endpoint in the meantime. | ||
| */ | ||
| if (!rpdev->announce) | ||
| rpdev->src = RPMSG_ADDR_ANY; |
There was a problem hiding this comment.
I also wonder if this belongs to the core? Doing a fast grep I see the only (upstream) user of ->announce is:
https://elixir.bootlin.com/linux/v7.1-rc7/source/drivers/rpmsg/virtio_rpmsg_bus.c#L422
I assume we have our own rpmsg driver. Can we fix it in there?
Anyways, if this is a valid core fix I would suggest to send the patch upstream and get feedback there. Then we won't be increasing our of tree burden :)
3 participants
Opening PR to track upstream contribution - to be merged when upstreamed
When a channel gets unbound, rpmsg_dev_remove() destroys the endpoint and frees the IDR slot, but forgets to reset rpdev->src back to RPMSG_ADDR_ANY. So the next time the channel gets bound, rpmsg_dev_probe() passes the old stale address as a specific IDR request. If another channel snached that address in the meantime, idr_alloc() fails with -ENOSPC (-28) and probe blows up with:
rpmsg_chrdev: idr_alloc failed: -28
rpmsg_chrdev: failed to create endpoint
rpmsg_chrdev: probe with driver rpmsg_chrdev failed with error -12
This makes the sucess of bind/unbind sequences order-dependent when it really shouldn't be.
Fix: reset rpdev->src to RPMSG_ADDR_ANY after tearing down the endpoint, but only for channels with dynamically-assigned addresses (!rpdev->announce). Channels with a predefined local address need to keep their src so they re-announce the same address on rebind.
Since the rpmsg_device now outlives the unbind and is re-probed on the next bind, also clear rpdev->ept after destroying the endpoint so the stale pointer isn't left dangling for the next probe to trip over.
Fixes: bbd1880 ("rpmsg: Support drivers without primary endpoint")
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist