Skip to content

Fix the openamp_rsc_table sample by specifying the IPM capability for maximum data transfer size #89016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

arnopo
Copy link
Collaborator

@arnopo arnopo commented Apr 24, 2025

The samples/subsys/ipc/openamp_rsc_table sample requests to transfer data for mailboxes that support data transfer.
Some existing drivers return an error if size is non-null or print a warning message on each transfer.
Update these drivers to be compatible with the subsys/ipc/openamp_rsc_table sample, using LOG_WRN_ONCE to indicate only the first time that the driver does not support data transfers.

This fixes issue introduced in e50a2ba

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

ipm_send api specificies:

 * @retval -EMSGSIZE If the supplied data size is unsupported by the driver.

Based on this, is that ok not to return an error in the driver?

Alternatively, could the sample first check driver support (be resilient to a first error EMSGSIZE received) before actually trying to send data ?

Comment on lines 87 to 88
if (size)
LOG_WRN_ONCE("HSEM driver does not support data transfer over IPM");
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised CI didn't complain but please add {}

Comment on lines 162 to 163
if (size)
LOG_WRN_ONCE("IPCC driver does not support data transfer over IPM");
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@arnopo
Copy link
Collaborator Author

arnopo commented Apr 24, 2025

ipm_send api specificies:

 * @retval -EMSGSIZE If the supplied data size is unsupported by the driver.

Based on this, is that ok not to return an error in the driver?

For me yes. This is the way of working in Linux kernel.

Alternatively, could the sample first check driver support (be resilient to a first error EMSGSIZE received) before actually trying to send data?

That would mean that the sample takes into account drivers specificity. I would prefer to keep the sample simple.

That said if your concern is about HSEM driver, I can remove HSEM update. the driver is not used by the subsys/ipc/openamp_rsc_table sample.

@erwango
Copy link
Member

erwango commented Apr 24, 2025

For me yes. This is the way of working in Linux kernel.

Ok.

That would mean that the sample takes into account drivers specificity. I would prefer to keep the sample simple.

My issue is that this change will prevent application to detect driver specificity. So yes user will be informed (if subsystem logs are enabled), but application won't be able to detect issue (underlying hw is not able to transmit data as expected) and report an error to higher levels.
If the issue is that ywe need to keep the sample simple run w/o errors, then it would be better to find ways (sample.yaml filters for instance) to not run it on HW that don't support the feature.

That said if your concern is about HSEM driver, I can remove HSEM update.

No need, I prefer we find a consistent behavior across drivers.

@arnopo
Copy link
Collaborator Author

arnopo commented Apr 24, 2025

For me yes. This is the way of working in Linux kernel.

Ok.

That would mean that the sample takes into account drivers specificity. I would prefer to keep the sample simple.

My issue is that this change will prevent application to detect driver specificity. So yes user will be informed (if subsystem logs are enabled), but application won't be able to detect issue (underlying hw is not able to transmit data as expected) and report an error to higher levels. If the issue is that ywe need to keep the sample simple run w/o errors, then it would be better to find ways (sample.yaml filters for instance) to not run it on HW that don't support the feature.

There are (at least) two types of mailbox:

  • mailbox that works as a dorbell ( non data transmitted only an inter processor notification)
  • mailbox that allow to transmit and receive data with a notification

We should support both.

for that an alternative could be to add an IPM config to indicate the type of IPM used.

  • in drivers/ipm/Kconfig
config IPM_DATA_TRANSFER
	bool "IPM data transfer support"
	default y
	help
	  Enable this if the IPM device support data transmission and reception.

In subsys/ipc/openamp_rsc_table sample

 int mailbox_notify(void *priv, uint32_t id)
{
	ARG_UNUSED(priv);

	LOG_DBG("%s: msg received", __func__);
-	ipm_send(ipm_handle, 0, id, &id, 4);
+	if (!IS_ENABLED(CONFIG_IPM_DATA_TRANSFER)) {
+		ipm_send(ipm_handle, 0, id, NULL, 0);
+	} else {
+		ipm_send(ipm_handle, 0, id, &id, 4);

	return 0;
}

Does this implementation align with your expectations?

@erwango
Copy link
Member

erwango commented Apr 24, 2025

Does this implementation align with your expectations?

Yes, fine for me. Thanks!

@github-actions github-actions bot added area: IPC Inter-Process Communication area: Open AMP area: Samples Samples labels Apr 25, 2025
@github-actions github-actions bot requested a review from doki-nordic April 25, 2025 13:30
@arnopo
Copy link
Collaborator Author

arnopo commented Apr 25, 2025

Finally, I replaced the IPM_DATA_TRANSFER configuration with IPM_MAX_DATA_SIZE, which can be reused later by other IPM drivers to specify their capabilities in terms of data transfer.

@arnopo arnopo requested a review from erwango April 25, 2025 13:32
@arnopo arnopo changed the title drivers: ipm: allow non-null size on ipm send Fix the openamp_rsc_table sample by specifying the IPM capability for maximum data transfer size Apr 25, 2025
arnopo added 2 commits April 25, 2025 15:42
The ipm_send() allows transferring data through the IPM device.
However, depending on the platform, the mailbox peripheral may either
transmit a limited amount of data or not transfer data at all.
Introducing this configuration allows exposing the IPM capability to the
application.
This commit defines a default value of 1024 bytes to avoid impacting
existing IPM drivers and sets the value to 0 for the STM32 IPCC and
STM32 HSEM devices.

This allows, in a next step, other devices to use this configuration to
expose their capabilities instead of using proprietary configuration or
a static definition.

Signed-off-by: Arnaud Pouliquen <[email protected]>
…vice

Some IPM devices return an error if we request to transfer data.
Use IPM_MAX_DATA_SIZE to determine if the virtio ID can be transferred
through the IPM device.
Send data to IPM only if CONFIG_IPM_MAX_DATA_SIZE is not zero.

Signed-off-by: Arnaud Pouliquen <[email protected]>
@arnopo
Copy link
Collaborator Author

arnopo commented May 7, 2025

@anangl , @masz-nordic: Could you review please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: IPC Inter-Process Communication area: IPM Inter-Processor Mailbox area: Open AMP area: Samples Samples platform: nRF Nordic nRFx platform: STM32 ST Micro STM32
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants