1. Patchew
  2. linux
  3. View series

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

Wolfram Sang posted 1 patch 1 month, 1 week ago 
drivers/mailbox/mailbox.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
[PATCH v3] mailbox: don't free the channel if the startup callback failed
Posted by Wolfram Sang 1 month, 1 week ago 
If the optional startup() callbacks fails, we need to clear some states.
Currently, this is done by freeing the channel. This does, however, more
than needed which creates problems. Namely, it is calling the shutdown()
callback. This is totally not intuitive. No user expects that shutdown()
is called when startup() fails, similar to remove() not being called
when probe() fails. Currently, 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. Other subtle issues could arise from
this unexpected behaviour.

To solve this problem, introduce a helper which does the minimal cleanup
and use it in both, in free_channel() and after startup() failed.

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>
---

Changes since v2:
* moved helper function up and made it static (buildbot)
* rebased to v7.1-rc2

 drivers/mailbox/mailbox.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
index bbc9fd75a95f..006ea5a5c320 100644
--- a/drivers/mailbox/mailbox.c
+++ b/drivers/mailbox/mailbox.c
@@ -327,6 +327,19 @@ int mbox_flush(struct mbox_chan *chan, unsigned long timeout)
 }
 EXPORT_SYMBOL_GPL(mbox_flush);
 
+static void mbox_clean_and_put_channel(struct mbox_chan *chan)
+{
+	/* The queued TX requests are simply aborted, no callbacks are made */
+	scoped_guard(spinlock_irqsave, &chan->lock) {
+		chan->cl = NULL;
+		chan->active_req = MBOX_NO_MSG;
+		if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
+			chan->txdone_method = MBOX_TXDONE_BY_POLL;
+	}
+
+	module_put(chan->mbox->dev->driver->owner);
+}
+
 static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
 {
 	struct device *dev = cl->dev;
@@ -350,10 +363,9 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
 
 	if (chan->mbox->ops->startup) {
 		ret = chan->mbox->ops->startup(chan);
-
 		if (ret) {
 			dev_err(dev, "Unable to startup the chan (%d)\n", ret);
-			mbox_free_channel(chan);
+			mbox_clean_and_put_channel(chan);
 			return ret;
 		}
 	}
@@ -495,15 +507,7 @@ void mbox_free_channel(struct mbox_chan *chan)
 	if (chan->mbox->ops->shutdown)
 		chan->mbox->ops->shutdown(chan);
 
-	/* The queued TX requests are simply aborted, no callbacks are made */
-	scoped_guard(spinlock_irqsave, &chan->lock) {
-		chan->cl = NULL;
-		chan->active_req = MBOX_NO_MSG;
-		if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
-			chan->txdone_method = MBOX_TXDONE_BY_POLL;
-	}
-
-	module_put(chan->mbox->dev->driver->owner);
+	mbox_clean_and_put_channel(chan);
 }
 EXPORT_SYMBOL_GPL(mbox_free_channel);
 
-- 
2.51.0
Re: [PATCH v3] mailbox: don't free the channel if the startup callback failed
Posted by Jassi Brar 3 weeks, 4 days ago 
On Wed, May 6, 2026 at 2:11 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> If the optional startup() callbacks fails, we need to clear some states.
> Currently, this is done by freeing the channel. This does, however, more
> than needed which creates problems. Namely, it is calling the shutdown()
> callback. This is totally not intuitive. No user expects that shutdown()
> is called when startup() fails, similar to remove() not being called
> when probe() fails. Currently, 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. Other subtle issues could arise from
> this unexpected behaviour.
>
> To solve this problem, introduce a helper which does the minimal cleanup
> and use it in both, in free_channel() and after startup() failed.
>
> 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>
> ---
>
> Changes since v2:
> * moved helper function up and made it static (buildbot)
> * rebased to v7.1-rc2
>
>  drivers/mailbox/mailbox.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mailbox/mailbox.c b/drivers/mailbox/mailbox.c
> index bbc9fd75a95f..006ea5a5c320 100644
> --- a/drivers/mailbox/mailbox.c
> +++ b/drivers/mailbox/mailbox.c
> @@ -327,6 +327,19 @@ int mbox_flush(struct mbox_chan *chan, unsigned long timeout)
>  }
>  EXPORT_SYMBOL_GPL(mbox_flush);
>
> +static void mbox_clean_and_put_channel(struct mbox_chan *chan)
> +{
> +       /* The queued TX requests are simply aborted, no callbacks are made */
> +       scoped_guard(spinlock_irqsave, &chan->lock) {
> +               chan->cl = NULL;
> +               chan->active_req = MBOX_NO_MSG;
> +               if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
> +                       chan->txdone_method = MBOX_TXDONE_BY_POLL;
> +       }
> +
> +       module_put(chan->mbox->dev->driver->owner);
> +}
> +
>  static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
>  {
>         struct device *dev = cl->dev;
> @@ -350,10 +363,9 @@ static int __mbox_bind_client(struct mbox_chan *chan, struct mbox_client *cl)
>
>         if (chan->mbox->ops->startup) {
>                 ret = chan->mbox->ops->startup(chan);
> -
>                 if (ret) {
>                         dev_err(dev, "Unable to startup the chan (%d)\n", ret);
> -                       mbox_free_channel(chan);
> +                       mbox_clean_and_put_channel(chan);
>                         return ret;
>                 }
>         }
> @@ -495,15 +507,7 @@ void mbox_free_channel(struct mbox_chan *chan)
>         if (chan->mbox->ops->shutdown)
>                 chan->mbox->ops->shutdown(chan);
>
> -       /* The queued TX requests are simply aborted, no callbacks are made */
> -       scoped_guard(spinlock_irqsave, &chan->lock) {
> -               chan->cl = NULL;
> -               chan->active_req = MBOX_NO_MSG;
> -               if (chan->txdone_method == MBOX_TXDONE_BY_ACK)
> -                       chan->txdone_method = MBOX_TXDONE_BY_POLL;
> -       }
> -
> -       module_put(chan->mbox->dev->driver->owner);
> +       mbox_clean_and_put_channel(chan);
>  }
>  EXPORT_SYMBOL_GPL(mbox_free_channel);
>
> --
> 2.51.0
>
Applied to mailbox/for-next
Thanks
Jassi

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.