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

[Jassi Brar]

On Wed, May 6, 2026 at 2:11 AM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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