Re: [PATCH] pinctrl: qcom: Replace open coded eoi call with irq_chip_eoi_parent()

[Maulik Shah (mkshah)]

On 5/19/2026 9:03 AM, Bjorn Andersson wrote:
> On Mon, May 18, 2026 at 10:20:55PM -0500, Bjorn Andersson wrote:
>> On Thu, May 14, 2026 at 02:08:25PM +0530, Maulik Shah wrote:
>>> Replace open coded eoi call to parent irqchip with irq_chip_eoi_parent().
>>>
>>> No functional impact.
>>>
>>
>> Reviewed-by: Bjorn Andersson <andersson@xxxxxxxxxx>
>>
> 
> On second though, I'm not sure I want to r-b this patch.
> 
> The commit message explains the action of the patch, not the reasons for
> the patch. From the description I inferred that irq_chip_eoi_parent()
> does implement what is open coded here, and a quick glance confirms
> that.
> 
> I'm guessing that irq_chip_eoi_parent() didn't exist when
> msm_gpio_irq_eoi() was written? Or was it not used for some reason?

irq_chip_eoi_parent() did exist before msm_gpio_irq_eoi() and msmgpio irqchip was using same
with a condition that pctrl->irq_chip.irq_eoi was only initialized to point irq_chip_eoi_parent()
for the cases where the gpio had a parent pdc/mpm irq number.

Later via [1] (as part to make irqchip immutable) pctrl->irq_chip.irq_eoi callback was updated to
use for each gpio interrupt and hence requiring the check like below inside eoi call to invoke only
if parent data (PDC/MPM) is valid.

   d = d->parent_data;
   if (d)
      d->chip->irq_eoi(d);

[1] https://lore.kernel.org/all/20220419141846.598305-8-maz@xxxxxxxxxx/ 

> 
>> Regards,
>> Bjorn
>>
>>> Signed-off-by: Maulik Shah <maulik.shah@xxxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/pinctrl/qcom/pinctrl-msm.c | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> index 45b3a2763eb8..6771f5eb29e4 100644
>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
>>> @@ -1012,10 +1012,8 @@ static void msm_gpio_irq_ack(struct irq_data *d)
>>>  
>>>  static void msm_gpio_irq_eoi(struct irq_data *d)
>>>  {
>>> -	d = d->parent_data;
>>> -
>>> -	if (d)
>>> -		d->chip->irq_eoi(d);
>>> +	if (d->parent_data)
> 
> "I know that irq_chip_eoi_parent() will peak into d->parent_data, so
> let's peek into the object first to avoid it dereferencing a NULL
> pointer".
> 
> I see one other caller to irq_chip_eoi_parent() doing this, most
> everyone else just register irq_chip_eoi_parent directly in the ops
> struct.
> 
> Are we doing it right?

Yes, because some gpios may not be wake up capable (they don't have their PDC / MPM interrupt)
and for such cases irq_domain_trim_hierarchy() makes irqd->parent_data = NULL, so this has to be
checked using if (d->parent_data) before invoking irq_chip_eoi_parent(), since internally
irq_chip_eoi_parent() de-references without a valid pointer check.

one can argue that the valid pointer check should be taken care inside irq_chip_eoi_parent()
(and all other irq_chip_*_parent() APIs) but i guess not all irqchip have cases where
irq_domain_trim_hierarchy() may be invoked and it would be overhead for them.

Thanks,
Maulik

> 
> Regards,
> Bjorn
> 
>>> +		irq_chip_eoi_parent(d);
>>>  }
>>>  
>>>  static bool msm_gpio_needs_dual_edge_parent_workaround(struct irq_data *d,
>>>
>>> ---
>>> base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83
>>> change-id: 20260514-pinctrl_msm_irq_eoi-ab736e16d411
>>>
>>> Best regards,
>>> --  
>>> Maulik Shah <maulik.shah@xxxxxxxxxxxxxxxx>
>>>
>>