libmetal: lib: linux: improve Linux UIO-backed device open and test coverage#365
libmetal: lib: linux: improve Linux UIO-backed device open and test coverage#365bentheredonethat wants to merge 3 commits into
Conversation
The public device-open API already takes bus and device names, but the current interface does not make that contract explicit. Add metal_device_open_from_bus() as a named helper for bus/device based open and keep metal_device_open() as a compatibility wrapper. This makes the Linux bus-based device flow easier to reference from downstream code without changing existing callers. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
Refactor the Linux UIO device path so the backend can reuse the same population flow after resolving a device from either a platform bus name or a UIO class name. Track the resolved Linux identities separately, validate UIO map offsets, retain the raw mmap pointers for correct unmap, and clear Linux IRQ registration state when a device is closed. Also make metal_sys_init() tolerate Linux bus probe failure so Linux initialization can still complete even when bus-based open is unavailable. This keeps the existing Linux UIO path working while making it easier for downstream host applications to use UIO-exposed devices. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
Add a standalone Linux system test for the device-open and Linux helper paths added by the UIO-backed device open work. The new test registers a temporary in-process bus and device to verify metal_device_open_from_bus(), the existing metal_device_open() compatibility wrapper, and expected error handling for invalid arguments, missing buses, missing devices, and buses without dev_open callbacks. It also covers the Linux-specific helper behavior without requiring real UIO hardware by validating UIO map offset checks and exercising IRQ device bookkeeping with an eventfd-backed IRQ. The IRQ coverage verifies that a registered device is visible, enable state is tracked, and unregistering the device clears both the device pointer and enabled state. Only the Linux system test collection is updated so the coverage remains isolated from the existing cross-platform tests. Signed-off-by: Ben Levinsky <ben.levinsky@amd.com>
6dbafca to
758f084
Compare
| #include <metal/utilities.h> | ||
| #include <metal/irq.h> | ||
|
|
||
| #include <stdbool.h> |
There was a problem hiding this comment.
This commit is quite large and difficult to understand if you are not familiar with the AMD implementation based on UIO, which is my case.
- Could you split it into smaller, more readable commits?
- Does it make sense to keep UIO support in the Linux subsystem, or is it specific to AMD?
| #include "irq.h" | ||
|
|
||
| #define MAX_DRIVERS 64 | ||
| #define METAL_UIO_CLASS_PATH "/sys/class/uio" |
There was a problem hiding this comment.
Does UIO support is required on the target in order to use OpenAMP in Linux user space?
| void metal_linux_irq_register_dev(struct metal_device *dev, int irq); | ||
| void metal_linux_irq_unregister_dev(int irq); | ||
| struct metal_device *metal_linux_irq_get_dev(int irq); | ||
| int metal_linux_irq_is_enabled(int irq); |
There was a problem hiding this comment.
APIs should have a doxygen documentation header
| void *output, int len); | ||
| int metal_linux_uio_validate_offset(const char *dev_name, | ||
| unsigned int index, | ||
| unsigned long offset); |
There was a problem hiding this comment.
doxygen documentation header
|
|
||
| int metal_device_open(const char *bus_name, const char *dev_name, | ||
| struct metal_device **device) | ||
| int metal_device_open_from_bus(const char *bus_name, const char *dev_name, |
There was a problem hiding this comment.
I do not understand the need to create a new API. It looks to me like this introduces confusion by duplicating APIs instead of making things more obvious.
Having a single system-agnostic API makes more sense to me.
|
Hi @arnopo Thanks for the review. I agree the Linux change is too large as posted, and I can split it into smaller commits before respinning. The main problem I am trying to solve is narrower than the current series makes it look. Linux userspace libmetal already opens platform/PCI devices through the existing Linux UIO-backed path. For demos and portable userspace applications, we would like to use a logical name such as "demo-ipi". On systems using UIO, that logical name can be exposed by the kernel as the UIO class name, The Linux libmetal backend can then resolve that UIO class name back to the real bus device: and continue through the existing Linux bus open/bind/map/IRQ/close flow. So the intended direction is not to make UIO mandatory for OpenAMP generally, and not to introduce a new Linux device model. It is only to let the existing Linux UIO backend accept the UIO class I also agree with your comment about the new public API. The UIO-name use case does not require adding The Linux backend would interpret the device argument as either:
Internally I plan to keep both identities clear, e.g. requested name / resolved bus device name / UIO name. For minimum behavior change, the returned For the respin, does the following direction sound acceptable?
If this direction makes sense, I will rework the series around that. |
What about using symbolic link for that, as proposed by @tnmysh in OpenAMP/openamp-system-reference#101.
Sound good. Thanks
|
|
Thanks @arnopo, this is a good point. Using a udev-created symlink such as:
For upstream libmetal, I would prefer to treat this as optional platform integration rather than a hard dependency, because not all deployments guarantee custom udev rules. So my plan is:
That keeps behavior portable out of the box while allowing integrators to use the symlink approach for faster/cleaner lookup. If you agree, I will document this in the commit message as “optional udev optimization, generic fallback preserved”. |
I would prefer that we handle this in the same way we manage /dev/rpmsgX or /sys/class/remoteproc/remoteprocX devices, rather than adding it to libmetal. I propose adding this PR to the agenda for the next OpenAMP meeting so we can discuss it further. |
This series improves the Linux UIO-backed device-open flow in libmetal and
adds test coverage for the new API and Linux-specific helper paths.
The immediate motivation is to support Linux userspace applications that open
UIO-exposed devices through libmetal while keeping the existing bus/device
contract intact. In the current Linux implementation, the basic UIO path is
already present, but the backend is tightly coupled to bus probing, does not
cleanly separate resolved Linux device identities, does not unregister Linux
IRQ device state on close, and does not correctly retain the raw mapping
needed when a UIO map uses a non-zero offset.
This series addresses those gaps in three steps:
With these changes, downstream Linux host applications can continue to use
libmetal's device-open and IRQ registration model while relying on the
improved Linux UIO device handling in the library.