Re: [PATCH 1/2] mailbox: zynqmp-ipi: Fix CPU hotplug state cleanup

[Prasanna Kumar T S M]

Hi Radhey Shyam,

Thanks for reviewing this.

On 04-11-2026 11:23, Pandey, Radhey Shyam wrote:
> On 11/3/2026 11:50 AM, Prasanna Kumar T S M wrote:
> > On driver removal, the CPUHP_AP_ONLINE_DYN CPU hotplug state is removed
> > instead of the its actual state. This can leave the mailbox driver
> > callbacks dangling. Use the actual CPU hotplug state during driver
> > removal.
> >
> > Hotplug registration failures are also ignored, allowing probe to
> > succeed without the callbacks needed to enable interrupts on CPUs
> > brought online later.
> >
> > Fixes: 6ffb1635341be ("mailbox: zynqmp: handle SGI for shared IPI")
>
> Nit - only first 12 characters of the SHA-1 ID are sufficient.

Sure, I will update.

> Also describe the failure signature and how the error is reproduced.
>
>
> > Signed-off-by: Prasanna Kumar T S M <ptsm@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/mailbox/zynqmp-ipi-mailbox.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mailbox/zynqmp-ipi-mailbox.c b/drivers/mailbox/ 
> > zynqmp-ipi-mailbox.c
> > index 42d2583e44ca..b0643ee75db0 100644
> > --- a/drivers/mailbox/zynqmp-ipi-mailbox.c
> > +++ b/drivers/mailbox/zynqmp-ipi-mailbox.c
> > @@ -136,6 +136,7 @@ struct zynqmp_ipi_pdata {
> >       unsigned int method;
> >       u32 local_id;
> >       int virq_sgi;
> > +    enum cpuhp_state hp_state;
>
> Warning: drivers/mailbox/zynqmp-ipi-mailbox.c:141 struct member 
> 'hp_state' not described in 'zynqmp_ipi_pdata'

I will update this in v2.

> >       int num_mboxes;
> >       struct zynqmp_ipi_mbox ipi_mboxes[] __counted_by(num_mboxes);
> >   };
> > @@ -863,15 +864,22 @@ static int xlnx_mbox_init_sgi(struct 
> > platform_device *pdev,
> >       irq_set_status_flags(pdata->virq_sgi, IRQ_PER_CPU);
> >       /* Setup function for the CPU hot-plug cases */
> > -    cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mailbox/sgi:starting",
> > -              xlnx_mbox_cpuhp_start, xlnx_mbox_cpuhp_down);
> > +    pdata->hp_state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> > +              "mailbox/sgi:starting", xlnx_mbox_cpuhp_start,
>
> what is :starting here ?

I used the existing string.

> > +              xlnx_mbox_cpuhp_down);
> > +    if (pdata->hp_state < 0) {
> > +        irq_clear_status_flags(pdata->virq_sgi, IRQ_PER_CPU);
> > +        free_percpu_irq(pdata->virq_sgi, &per_cpu_pdata);
> > +        irq_dispose_mapping(pdata->virq_sgi);
> > +        return pdata->hp_state;
>
>   1. The new error path double-frees and then hits BUG().
> Please reconfirm.
>
> zynqmp_ipi_probe() sets pdata->irq_type before calling 
> xlnx_mbox_init_sgi(), and its only error label runs
>    zynqmp_ipi_free_mboxes() → xlnx_mbox_cleanup_sgi() whenever irq_type 
> == IPI_IRQ_TYPE_SGI. So the new unwind runs, then the caller's unwind 
> repeats
>    irq_clear_status_flags(), free_percpu_irq() and 
> irq_dispose_mapping(). Before any of that, cleanup calls 
> cpuhp_remove_state(pdata->hp_state) with the negative errno still in the 
> field, and _cpuhp_remove_state_cpuslocked() opens with 
> BUG_ON(cpuhp_cb_check(state)), which rejects anything <= CPUHP_OFFLINE. 
> A failed hotplug registration now panics instead of failing probe.

I will fix this in v2.

> > +    }
> >       return ret;
> >   }
> >   static void xlnx_mbox_cleanup_sgi(struct zynqmp_ipi_pdata *pdata)
> >   {
> > -    cpuhp_remove_state(CPUHP_AP_ONLINE_DYN);
> > +    cpuhp_remove_state(pdata->hp_state);
> >       on_each_cpu(xlnx_disable_percpu_irq, NULL, 1);

Thanks,
Prasanna Kumar