handle_vdev_rsc not checking remoteproc_allocate_id

[tammyleino]

Is the purpose of remoteproc_allocate_id in handle_vdev_rsc to ensure that the notify_id is unique? If so, it does not seem that the code is handling errors properly.

notifyid = remoteproc_allocate_id(rproc, notifyid, notifyid + 1U);
if (notifyid != RSC_NOTIFY_ID_ANY)
    vdev_rsc->notifyid = notifyid;

I would expect that the handle_vdev_rsc returns an error if the given ID is already in use.

[tammyleino]

I don't see where rproc->bitmap gets used anywhere else in the code. Are handle_vdev_rsc and the bitmap field of the remoteproc struct necessary?

[arnopo]

remoteproc_allocate_id

Right,
remoteproc_allocate_id always return an ID ( except if all the ID are reserved) as metal_bitmap_next_clear_bit seems return the end value by default. in such case the instruction metal_bitmap_next_clear_bit(bitmap, notifyid, notifyid + 1U) returns notifyid +1U if notifyid is reserved. But notifyid + 1U could also be reserved
Error cases seem not well implemented here

[arnopo]

I don't see where rproc->bitmap gets used anywhere else in the code. Are handle_vdev_rsc and the bitmap field of the remoteproc struct necessary?

• handle_vdev_rsc is called when parsing the resource table if a RSC_VDEV resource if found
• The bitmap is used to ensure that each resource has is own notifyID

[tammyleino]

Is it valid to use RSC_NOTIFY_ID_ANY as the notify ID in the resource table?

[arnopo]

Yes that's means that the device lets the host to define them. And pre-defined ID is not supported on Linux Side:
https://elixir.bootlin.com/linux/v5.18-rc1/source/drivers/remoteproc/remoteproc_core.c#L366

[arnopo]

Sorry @tammyleino
I saw your comment 3 times and tried to delete 2 instances, but deleting one instance deleted all 3...

Here is your comment with my answer

I have one more comment regarding using handle_vdev_rsc() with RSC_NOTIFY_ID_ANY. RSC_NOTIFY_ID_ANY is 32-bits and fw_rsc_vdev.notifyid is 32-bits, but handle_vdev_rsc assigns it to an unsigned int, which would be 64-bits on a 64-bit platform, so using notifyid + 1 at the call to remoteproc_allocate_id will not overflow the value to zero, which looks to be expected in remoteproc_allocate_id(). Furthermore, we then try to assign a 64-bit integer to a 32-bit integer on return from remoteproc_allocate_id(). It looks like we should change all notifyid variables to be uint32_t to match the resource table.

Right, That would be much better!

[tammyleino]

Thank you @arnopo - I was having an internet hiccup and it posted thrice. I deleted two of them - you were probably deleting at the same time as me.

[tammyleino]

@arnopo In the case that the notifyid is duplicated or we are somehow out of new IDs to assign to the device, should the resource table parsing fail or continue to the next resource? ie; should handle_vdev_rsc() return -RPROC_ERR_RSC_TAB_NS or another error that would cause parsing to stop and return failure?

[edmooring]

@tammyleino @arnopo,
I would favor handle_vdev_rsc() failing causing the parsing to fail. Duplicate IDs or resource exhaustion during initialization would seem to leave the system in an untrustworthy state.

[arnopo]

Agree with @edmooring, I guess that such cases should only occurs during a development phase.

• notifyid is duplicated probably means that it is a bug,
• out of new IDs should mean that you have reached the limit of the system.

[tammyleino]

I opened two separate pull requests for the items discussed in this issue.

[tammyleino]

#404