Skip to content

Conversation

@kv2019i
Copy link
Contributor

@kv2019i kv2019i commented Nov 21, 2025

Add user-space support to the dai.h interface. No functional impact to builds when CONFIG_USERSPACE is not set.

@kv2019i
Copy link
Contributor Author

kv2019i commented Nov 21, 2025

Draft for now, thesofproject/sof#10373 shall go in first. I'll rebase on top of that before converting to ready for review.

* merge with Adrian's PR to add bespoke_cfg size */
K_OOPS(K_SYSCALL_MEMORY_READ(bespoke_cfg, CONFIG_MMU_PAGE_SIZE));

/* TODO: shall a kernel copy made of cfg */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, not sure what this comment means?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh So basicly, can we trust to pass an object that sits in user-space accessible memory to a kernel function. In Linux this is a no-no as another user-space thread could be active (on another CPU) and modify the data while kernel is accessing it (on another).

On a resource constrained device, this is a bit harder pill to swallow as some of these objects are rather big. But I'm thinking it we want to merge this in Zephyr, we need to do a copy of all input data to kernel, validate and run the function. In SOF side (the application using Zephyr), we could optimize this as we know there will not be other user-space threads (enforced by system design).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i ah, so the typo is "a kernel copy be made" - that's what I couldn't guess, sorry :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to deeply copy this (nothing referring to user mode accessible memory), then validate it. This will be rather troublesome with a void * at an API level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack @teburd , will at deep copies to next version. @abonislawski is added a size field in a separate PR, so that allows to handle the bespoke data objects in a sane fashion. I'll be adding a new variant of get_properties() that can be used from user-space.

@softwarecki
Copy link
Contributor

Can we wrap these changes into a more detailed config than CONFIG_USERSPACE? I'm not entirely sure what the idea behind these changes is, but I suspect it's about running the entire pipeline in userspace. In that case, I suggest something like CONFIG_USERSPACE_PIPELINE.

@kv2019i
Copy link
Contributor Author

kv2019i commented Nov 25, 2025

@softwarecki

Can we wrap these changes into a more detailed config than CONFIG_USERSPACE? I'm not entirely sure what the idea behind these changes is, but I suspect it's about running the entire pipeline in userspace. In that case, I suggest something like CONFIG_USERSPACE_PIPELINE.

Good question. This is a generic capability in Zephyr, so it's not really tied to any particular use (and not related to SOF). Currently user-space is a boolean build option in Zephyr, if you enable user-space, all interfaces that have a syscall interface (semaphores, cache.h, mutexes, now dai.h) , are exposed. I'd prefer to use some standard Zephyr option if user wants to build Zephyr with a subset of all possible syscalls-enables interfaces (e.g. have semaphore syscall impelemntation built in, but noth dma.h or dai.h). Let me study this a bit (and please chime in if anyone knows how to do this the Zephyr way).

Functionally this is not a problem. Even if you enable Zephyr CONFIG_USERSPACE, but you call dai.h from kernel, nothing changes. And user threads cannot access dai.h unless permission is explicitly given, so just having dai.h syscalls built-in does not mean user threads can use this functionality. But this does add code to the binary, so a valid question for sure. Same problem should apply for other interfaces as well. Let's see if there's a solution already.

Copy link
Contributor Author

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks all to reviewers so far!

* merge with Adrian's PR to add bespoke_cfg size */
K_OOPS(K_SYSCALL_MEMORY_READ(bespoke_cfg, CONFIG_MMU_PAGE_SIZE));

/* TODO: shall a kernel copy made of cfg */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack @teburd , will at deep copies to next version. @abonislawski is added a size field in a separate PR, so that allows to handle the bespoke data objects in a sane fashion. I'll be adding a new variant of get_properties() that can be used from user-space.

@kv2019i kv2019i force-pushed the 202511-userspace-dai branch from c16628a to 716366c Compare December 2, 2025 16:56
@kv2019i
Copy link
Contributor Author

kv2019i commented Dec 2, 2025

V2 pushed:

  • rebased on top of interface change from @abonislawski for set_config... now can do a more safe version of config_set
  • added copy of input blobs to kernel memory before validation/use as per feedback from @teburd
  • added new logic to leave the DAI user-space syscall handlers from build with a new Kconfig based on feedback from @softwarecki
  • marking as ready for review now

@kv2019i kv2019i marked this pull request as ready for review December 2, 2025 16:59
@kv2019i kv2019i requested review from dbaluta and dcpleung December 2, 2025 16:59
Add user-space support to the dai.h interface. No functional
impact to builds when CONFIG_USERSPACE is not set.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Add a variant of get_properties() method that writes the properties
to a caller provided pointer. Unlike the old variant, this copy
variant can be exported to user-space in a safe way.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
The DAI interface is not used from user-space in all configurations
where Zephyr user-space is enabled, so it is beneficial to have
a build option to contorl whether the DAI syscalls are included or
not.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Add support for new get_properties_copy() method. This allows to
use ssp driver from user-space threads.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i kv2019i force-pushed the 202511-userspace-dai branch from 716366c to 3c5607b Compare December 2, 2025 17:52
@kv2019i
Copy link
Contributor Author

kv2019i commented Dec 2, 2025

V3:

  • fix compliance check issues in V2

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 2, 2025

@kv2019i kv2019i added the platform: Intel ADSP Intel Audio platforms label Dec 3, 2025
@kv2019i kv2019i requested a review from nashif December 4, 2025 10:46
@kv2019i
Copy link
Contributor Author

kv2019i commented Dec 4, 2025

@lyakh @softwarecki @lgirdwood @nashif can you take a look?

Copy link
Contributor

@softwarecki softwarecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job wrapping zephyr_syscall_header in if(CONFIG_DAI_USERSPACE).
This was missing in thesofproject/sof#10341, so it's great to see it addressed here.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds user-space support to the DAI (Digital Audio Interface) driver interface by converting inline functions to syscalls and adding appropriate validation handlers.

Key changes:

  • Converted 10 DAI API functions to syscalls with __syscall declarations and z_impl_* implementations
  • Added new dai_get_properties_copy API function for user-space safe property retrieval
  • Implemented syscall validation handlers in dai_handlers.c with memory access checks and parameter validation

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
include/zephyr/drivers/dai.h Converts inline functions to syscalls, adds get_properties_copy API and callback, includes syscall header
drivers/dai/dai_handlers.c New file implementing syscall verification handlers with memory validation and bespoke config size limits
drivers/dai/intel/ssp/ssp.c Implements dai_ssp_get_properties_copy callback for SSP driver
drivers/dai/Kconfig Adds DAI_USERSPACE configuration option dependent on USERSPACE
drivers/dai/CMakeLists.txt Conditionally includes syscall generation and handlers compilation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (!prop) {
return -EINVAL;
}

Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing NULL check for kernel_prop before dereferencing it in memcpy. While dai_ssp_get_properties currently never returns NULL, defensive programming requires checking the return value. If it were NULL, line 2560 would cause a crash. Add a check: if (!kernel_prop) { return -ENODATA; } after line 2554.

Suggested change
if (!kernel_prop) {
return -ENODATA;
}

Copilot uses AI. Check for mistakes.
Comment on lines +464 to +471
* @brief Fetch properties of a DAI driver
*
* @param dev Pointer to the device structure for the driver instance
* @param dir Stream direction: RX or TX as defined by DAI_DIR_*
* @param stream_id Stream id: some drivers may have stream specific
* properties, this id specifies the stream.
* @param dst address where to write properties to
* @retval Zero on success
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for dai_get_properties_copy is incomplete. It should specify the expected error return codes (e.g., -EINVAL for invalid parameters, -ENODATA if properties are unavailable). Additionally, it should clarify the relationship between this function and dai_get_properties, explaining that this is the user-space-safe variant.

Suggested change
* @brief Fetch properties of a DAI driver
*
* @param dev Pointer to the device structure for the driver instance
* @param dir Stream direction: RX or TX as defined by DAI_DIR_*
* @param stream_id Stream id: some drivers may have stream specific
* properties, this id specifies the stream.
* @param dst address where to write properties to
* @retval Zero on success
* @brief Fetch properties of a DAI driver (user-space-safe variant)
*
* This function is the user-space-safe variant of @ref dai_get_properties.
* It copies the properties into the provided destination buffer, allowing
* safe access from user space.
*
* @param dev Pointer to the device structure for the driver instance
* @param dir Stream direction: RX or TX as defined by DAI_DIR_*
* @param stream_id Stream id: some drivers may have stream specific
* properties, this id specifies the stream.
* @param dst Address where to write properties to
*
* @retval 0 On success.
* @retval -EINVAL Invalid parameters (e.g., null pointer, invalid stream id).
* @retval -ENODATA Properties are unavailable for the specified stream.

Copilot uses AI. Check for mistakes.
struct dai_properties *dst)
{
const struct dai_driver_api *api = (const struct dai_driver_api *)dev->api;

Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing NULL check for the get_properties_copy API callback before calling it. If a driver doesn't implement this callback (like alh, dmic, hda, esai, micfil, and sai drivers), this will cause a NULL pointer dereference. Add a check: if (!api->get_properties_copy) { return -ENOSYS; } before the function call.

Suggested change
if (!api->get_properties_copy) {
return -ENOSYS;
}

Copilot uses AI. Check for mistakes.
return -EINVAL;
}

K_OOPS(K_SYSCALL_DRIVER_DAI(dev, config_set));
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation for the cfg parameter. The syscall handler should validate that userspace has read access to the dai_config structure before passing it to the kernel implementation. Add K_OOPS(K_SYSCALL_MEMORY_READ(cfg, sizeof(*cfg))); after the driver validation check.

Suggested change
K_OOPS(K_SYSCALL_DRIVER_DAI(dev, config_set));
K_OOPS(K_SYSCALL_DRIVER_DAI(dev, config_set));
K_OOPS(K_SYSCALL_MEMORY_READ(cfg, sizeof(*cfg)));

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +48
K_OOPS(k_usermode_from_copy(bespoke_cfg_kernel, bespoke_cfg, size));

return z_impl_dai_config_set(dev, cfg, bespoke_cfg_kernel, size);
}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing validation for NULL or zero-size bespoke_cfg. If size is 0 and bespoke_cfg is NULL, the k_usermode_from_copy will fail. Consider checking if size is 0 and handling it appropriately, potentially skipping the copy when size == 0. This pattern should also be applied to z_vrfy_dai_config_update.

Suggested change
K_OOPS(k_usermode_from_copy(bespoke_cfg_kernel, bespoke_cfg, size));
return z_impl_dai_config_set(dev, cfg, bespoke_cfg_kernel, size);
}
if (size > 0) {
K_OOPS(k_usermode_from_copy(bespoke_cfg_kernel, bespoke_cfg, size));
return z_impl_dai_config_set(dev, cfg, bespoke_cfg_kernel, size);
} else {
/* No bespoke config to copy, pass NULL */
return z_impl_dai_config_set(dev, cfg, NULL, 0);
}

Copilot uses AI. Check for mistakes.
@fabiobaltieri fabiobaltieri merged commit 3b7b2ae into zephyrproject-rtos:main Dec 10, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants