| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <97f1abc9-7e1c-4d13-a46f-451612f94a4d@ti.com>
Date: Tue, 4 Nov 2025 09:58:45 +0530
From: Beleswar Prasad Padhi <b-padhi@...com>
To: Andrew Davis <afd@...com>, <jassisinghbrar@...il.com>,
<linux-kernel@...r.kernel.org>, <hiago.franco@...adex.com>,
<francesco.dolcini@...adex.com>
CC: <hnagalla@...com>, <u-kumar1@...com>, <nm@...com>, <vigneshr@...com>
Subject: Re: [PATCH v2] mailbox: omap-mailbox: Check for pending msgs only
when mbox is exclusive
On 04/11/25 03:40, Andrew Davis wrote:
> On 11/3/25 2:11 PM, Beleswar Padhi wrote:
>> On TI K3 devices, the mailbox resides in the Always-On power domain
>> (LPSC_main_alwayson) and is shared among multiple processors. The
>> mailbox is not solely exclusive to Linux.
>>
>> Currently, the suspend path checks all FIFO queues for pending messages
>> and blocks suspend if any are present. This behavior is unnecessary for
>> K3 devices, since some of the FIFOs are used for RTOS<->RTOS
>> communication and are independent of Linux.
>>
>> For FIFOs used in Linux<->RTOS communication, any pending message would
>> trigger an interrupt, which naturally prevents suspend from completing.
>> Hence, there is no need for the mailbox driver to explicitly check for
>> pending messages on K3 platforms.
>>
>> Introduce a device match flag to indicate whether the mailbox instance
>> is exclusive to Linux, and skip the pending message check for
>> non-exclusive instances (such as in K3).
>>
>> Fixes: a49f991e740f ("arm64: dts: ti: k3-am62-verdin: Add missing cfg for TI IPC Firmware")
>> Closes: https://lore.kernel.org/all/sid7gtg5vay5qgicsl6smnzwg5mnneoa35cempt5ddwjvedaio@hzsgcx6oo74l/
>> Signed-off-by: Beleswar Padhi <b-padhi@...com>
>> ---
>> Cc: Francesco Dolcini <francesco.dolcini@...adex.com>
>> Cc: Hiago De Franco <hiago.franco@...adex.com>
>> Please help in testing the patch on Toradex platforms.
>>
>> Testing Done:
>> 1. Tested Boot across all TI K3 EVM/SK boards.
>> 2. Tested IPC on all TI K3 J7* EVM/SK boards (& AM62x SK).
>> 3. Tested mbox driver probe & device boot on AM57x-evm (OMAP5 based).
>> 4. Tested that the patch generates no new warnings/errors.
>>
>> Changes since v1:
>> 1. Use device_get_match_data() in probe and store result for re-use.
>>
>> Link to v1:
>> https://lore.kernel.org/all/20251103075920.2611642-1-b-padhi@ti.com/
>>
>> Changes since RFC:
>> 1. Skip checking pending messages instead of flushing
>> them explicitly for K3 devices.
>>
>> Link to RFC Version:
>> https://lore.kernel.org/all/20251022102015.1345696-1-b-padhi@ti.com/
>>
>> drivers/mailbox/omap-mailbox.c | 35 +++++++++++++++++++---------------
>> 1 file changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
>> index 680243751d62..17fe6545875d 100644
>> --- a/drivers/mailbox/omap-mailbox.c
>> +++ b/drivers/mailbox/omap-mailbox.c
>> @@ -68,6 +68,7 @@ struct omap_mbox_fifo {
>> struct omap_mbox_match_data {
>> u32 intr_type;
>> + bool is_exclusive;
>> };
>> struct omap_mbox_device {
>> @@ -78,6 +79,7 @@ struct omap_mbox_device {
>> u32 num_users;
>> u32 num_fifos;
>> u32 intr_type;
>> + const struct omap_mbox_match_data *mbox_data;
>
> Not a fan of this, you could have stored just the relevant
> flag from the match data here in the instance data, not a
> pointer to the whole const struct.
Umm, will this scale up? What if tomorrow another field
gets added to the match data struct, why should we
manually have to maintain those fields in this struct
instead of holding a pointer?
> The issue being now you
> have the same info stored in two places, `intr_type` in the
> match data above, and intr_type on the line above that.
>
> Looking more into it, the use of `intr_type` doesn't need to be
> stored in each mbox's instance data either, but that is an existing
> issue.
Yup, now that we have the pointer to the match_data,
we can remove the `intr_type`. A cleanup for another
day though.
Thanks,
Beleswar
> For now I don't want to hold up an otherwise good bugfix
> over that, but we will want to come back here and clean some of
> this up next cycle, for now,
>
> Reviewed-by: Andrew Davis <afd@...com>
>
>> };
>> struct omap_mbox {
>> @@ -341,11 +343,13 @@ static int omap_mbox_suspend(struct device *dev)
>> if (pm_runtime_status_suspended(dev))
>> return 0;
>> - for (fifo = 0; fifo < mdev->num_fifos; fifo++) {
>> - if (mbox_read_reg(mdev, MAILBOX_MSGSTATUS(fifo))) {
>> - dev_err(mdev->dev, "fifo %d has unexpected unread messages\n",
>> - fifo);
>> - return -EBUSY;
>> + if (mdev->mbox_data->is_exclusive) {
>> + for (fifo = 0; fifo < mdev->num_fifos; fifo++) {
>> + if (mbox_read_reg(mdev, MAILBOX_MSGSTATUS(fifo))) {
>> + dev_err(mdev->dev, "fifo %d has unexpected unread messages\n",
>> + fifo);
>> + return -EBUSY;
>> + }
>> }
>> }
>> @@ -378,8 +382,9 @@ static const struct dev_pm_ops omap_mbox_pm_ops = {
>> SET_SYSTEM_SLEEP_PM_OPS(omap_mbox_suspend, omap_mbox_resume)
>> };
>> -static const struct omap_mbox_match_data omap2_data = { MBOX_INTR_CFG_TYPE1 };
>> -static const struct omap_mbox_match_data omap4_data = { MBOX_INTR_CFG_TYPE2 };
>> +static const struct omap_mbox_match_data omap2_data = { MBOX_INTR_CFG_TYPE1, true };
>> +static const struct omap_mbox_match_data omap4_data = { MBOX_INTR_CFG_TYPE2, true };
>> +static const struct omap_mbox_match_data am654_data = { MBOX_INTR_CFG_TYPE2, false };
>> static const struct of_device_id omap_mailbox_of_match[] = {
>> {
>> @@ -396,11 +401,11 @@ static const struct of_device_id omap_mailbox_of_match[] = {
>> },
>> {
>> .compatible = "ti,am654-mailbox",
>> - .data = &omap4_data,
>> + .data = &am654_data,
>> },
>> {
>> .compatible = "ti,am64-mailbox",
>> - .data = &omap4_data,
>> + .data = &am654_data,
>> },
>> {
>> /* end */
>> @@ -449,7 +454,6 @@ static int omap_mbox_probe(struct platform_device *pdev)
>> struct omap_mbox_fifo *fifo;
>> struct device_node *node = pdev->dev.of_node;
>> struct device_node *child;
>> - const struct omap_mbox_match_data *match_data;
>> struct mbox_controller *controller;
>> u32 intr_type, info_count;
>> u32 num_users, num_fifos;
>> @@ -462,11 +466,6 @@ static int omap_mbox_probe(struct platform_device *pdev)
>> return -ENODEV;
>> }
>> - match_data = of_device_get_match_data(&pdev->dev);
>> - if (!match_data)
>> - return -ENODEV;
>> - intr_type = match_data->intr_type;
>> -
>> if (of_property_read_u32(node, "ti,mbox-num-users", &num_users))
>> return -ENODEV;
>> @@ -483,6 +482,12 @@ static int omap_mbox_probe(struct platform_device *pdev)
>> if (!mdev)
>> return -ENOMEM;
>> + mdev->mbox_data = device_get_match_data(&pdev->dev);
>> + if (!mdev->mbox_data)
>> + return -ENODEV;
>> +
>> + intr_type = mdev->mbox_data->intr_type;
>> +
>> mdev->mbox_base = devm_platform_ioremap_resource(pdev, 0);
>> if (IS_ERR(mdev->mbox_base))
>> return PTR_ERR(mdev->mbox_base);
>
Powered by blists - more mailing lists