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

[Prasanna Kumar T S M <ptsm@linux.microsoft.com>]

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@linux.microsoft.com>
>> ---
>>   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