Re: [PATCH v2 1/2] mailbox: cix: validate fast channel index before request_irq()

On Wed, Aug 12, 2026 at 03:36:22PM +0530, [email protected] wrote:
> [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> EXTERNAL EMAIL
> 
> From: Surendra Singh Chouhan <[email protected]>
> 
> cix_mbox_startup() checked fast channel index constraints (index < 0 ||
> index > CIX_MBOX_FAST_IDX) inside the channel switch block after
> calling request_irq(). If validation failed, it triggered a free_irq()
> cleanup path.
> 
> Validating channel parameters prior to request_irq() avoids unnecessary
> IRQ registration and teardown churn.
> 
> Signed-off-by: Surendra Singh Chouhan <[email protected]>
> ---
>  drivers/mailbox/cix-mailbox.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mailbox/cix-mailbox.c b/drivers/mailbox/cix-mailbox.c
> index 43c76cdab24a..615218c69eeb 100644
> --- a/drivers/mailbox/cix-mailbox.c
> +++ b/drivers/mailbox/cix-mailbox.c
> @@ -403,6 +403,13 @@ static int cix_mbox_startup(struct mbox_chan *chan)
>         int index = cp->index, ret;
>         u32 val;
> 
> +       if (cp->type == CIX_MBOX_TYPE_FAST && priv->dir == CIX_MBOX_RX) {
> +               if (index < 0 || index > CIX_MBOX_FAST_IDX) {
> +                       dev_err(priv->dev, "Invalid index %d\n", index);
> +                       return -EINVAL;
> +               }
> +       }
> +
>         ret = request_irq(priv->irq, cix_mbox_isr, IRQF_NO_SUSPEND,
>                           dev_name(priv->dev), chan);
>         if (ret) {
> @@ -448,11 +455,6 @@ static int cix_mbox_startup(struct mbox_chan *chan)
>         case CIX_MBOX_TYPE_FAST:
>                 /* Only RX channel has intterupt */
>                 if (priv->dir == CIX_MBOX_RX) {
> -                       if (index < 0 || index > CIX_MBOX_FAST_IDX) {
> -                               dev_err(priv->dev, "Invalid index %d\n", index);
> -                               ret = -EINVAL;
> -                               goto failed;
> -                       }
>                         /* enable fast channel interrupt */
>                         val = cix_mbox_read(priv, CIX_INT_ENABLE_SIDE_B);
>                         val |= CIX_FAST_CH_INT(index);
> @@ -461,14 +463,10 @@ static int cix_mbox_startup(struct mbox_chan *chan)
>                 break;
>         default:
>                 dev_err(priv->dev, "Invalid channel type: %d\n", cp->type);
> -               ret = -EINVAL;
> -               goto failed;
> +               free_irq(priv->irq, chan);
> +               return -EINVAL;
>         }
>         return 0;
> -
> -failed:
> -       free_irq(priv->irq, chan);
> -       return ret;
>  }
The premise of the commit message doesn't hold, so I don't think this
patch should be applied as-is.

The check being moved,index < 0 || index > CIX_MBOX_FAST_IDX, is
unreachable for a CIX_MBOX_TYPE_FAST channel: cp->index and cp->type
are only ever assigned in cix_mbox_init(), where cp->index = i (so >= 0)
and CIX_MBOX_TYPE_FAST is only set when cp->index <= CIX_MBOX_FAST_IDX.
There is no DT or probe path that overrides either field. So for any
FAST channel the condition is always false, and the "unnecessary IRQ
registration and teardown churn" the message describes cannot actually
occur at runtime.

Given that, the patch is reshuffling dead code: it duplicates the
(type == FAST && dir == RX) condition outside the switch, adding a
second spot that has to stay in sync with the FAST case, in exchange
for optimizing a path that never executes. That's net negative as-is.

Best regards,
Guomin.Chen
>
>  static void cix_mbox_shutdown(struct mbox_chan *chan)
> --
> 2.55.0
>