[RFC PATCH] mailbox: don't free the channel if the startup callback failed

[RFC PATCH] mailbox: don't free the channel if the startup callback failed

[Wolfram Sang]

If the optional startup callbacks fails, we need to clear some states.
Currently, this is done by freeing the channel. This does, however, much
more than needed which creates problems.

One thing is calling the shutdown callback. This is totally not
intuitive. No user expects that shutdown() is called when startup()
fails. E.g. quite some mailbox users register the IRQ in startup() and
free them in shutdown(). These drivers will get a WARN about freeing an
already free IRQ.

A second thing is that module_put is called. This should also not be
done when startup() fails. It breaks the expected symmetry that
request_channel() gets the module and only free_channel() puts it again.

To solve these problems, do only the required cleanups manually when
startup() fails and do not use free_channel() as a helper.

Link: https://sashiko.dev/#/patchset/20260402112709.13002-1-wsa%2Brenesas%40sang-engineering.com # second issue
Fixes: 2b6d83e2b8b7 ("mailbox: Introduce framework for mailbox")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

This patch is RFC because I am still somewhat new to the mailbox
subsystem and might miss something. However, I think the Sashiko comment
in the above Link-tag is correct. I could reproduce the WARN with the
Renesas MFIS driver and injecting an error when obtaining the irq.
Looking at other drivers, I see this problem as well.

 drivers/mailbox/mailbox.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index a7a690acd90d..dcde4efa5be8 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -353,7 +353,11 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
 
 		if (ret) {
 			dev_err(dev, "Unable to startup the chan (%d)\n", ret);
-			mbox_free_channel(chan);
+			scoped_guard(spinlock_irqsave, &chan->lock) {
+				chan->cl = NULL;
+				if (chan->txdone_method == TXDONE_BY_ACK)
+					chan->txdone_method = TXDONE_BY_POLL;
+			}
 			return ret;
 		}
 	}
-- 
2.47.3

Re: [RFC PATCH] mailbox: don't free the channel if the startup callback failed

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]


> A second thing is that module_put is called. This should also not be
> done when startup() fails. It breaks the expected symmetry that
> request_channel() gets the module and only free_channel() puts it again.

This part is bogus...

> This patch is RFC because I am still somewhat new to the mailbox
> subsystem and might miss something.

... which proves this point ;) ...


>  			dev_err(dev, "Unable to startup the chan (%d)\n", ret);
> -			mbox_free_channel(chan);
> +			scoped_guard(spinlock_irqsave, &chan->lock) {
> +				chan->cl = NULL;
> +				if (chan->txdone_method == TXDONE_BY_ACK)
> +					chan->txdone_method = TXDONE_BY_POLL;
> +			}

... module_put needs to be here.

Still, not calling shutdown() still seems essential to me, so I'd
appreciate comments about the patch still.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] mailbox: don't free the channel if the startup callback failed

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]


> Still, not calling shutdown() still seems essential to me, so I'd
> appreciate comments about the patch still.

Another option which I discarded but maybe favored is to introduce

__mbox_free_channel(struct mbox_chan *chan, bool do_shutdown)

When startup() fails, it is called with 'false'. mbox_free_channel wraps
it with 'true'.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]