Re: [PATCH v4 11/13] gpio: eic-sprd: use devm_atomic_notifier_chain_register()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbinding one EIC sub-module will break interrupts for all other active sub-modules sharing the same parent IRQ.
--

--- Patch [11]: [PATCH v4 11/13] gpio: eic-sprd: use devm_atomic_notifier_chain_register() ---
commit 55c8bafe72677fc98fb136d26d8a1306172a3b1f
Author: Eliav Farber <[email protected]>

gpio: eic-sprd: use devm_atomic_notifier_chain_register()

This commit replaces the atomic_notifier_chain_register() and
devm_add_action_or_reset() pattern with a single call to
devm_atomic_notifier_chain_register(). This removes the need for the custom
sprd_eic_unregister_notifier() callback.

> diff --git a/drivers/gpio/gpio-eic-sprd.c b/drivers/gpio/gpio-eic-sprd.c
> index 3b7ebcf12fe7f..86eb9624d38ac 100644
> --- a/drivers/gpio/gpio-eic-sprd.c
> +++ b/drivers/gpio/gpio-eic-sprd.c

[ ... ]

> @@ -690,14 +683,8 @@ static int sprd_eic_probe(struct platform_device *pdev)

[Severity: High]
This is a pre-existing issue, but I noticed something about how the parent
interrupt is configured earlier in sprd_eic_probe().

The driver sets up the shared parent interrupt line as a non-shared chained
handler before registering the gpiochip:

    irq->parent_handler = sprd_eic_irq_handler;

Since the SPRD EIC hardware exposes up to 4 sub-modules that share the same
parent interrupt line, does unbinding one of these sub-modules break
interrupts for all other active sub-modules?

When a sub-module is unbound via sysfs or fails a late probe step, its
devres cleanup invokes gpiochip_remove(). This unconditionally calls
irq_set_chained_handler_and_data(parent_irq, NULL, NULL) which clears the
shared chained handler completely.

Will this result in a silent loss of GPIO interrupt functionality for the
remaining active EIC sub-modules sharing that parent IRQ?

>  	}
>  
>  	sprd_eic->irq_nb.notifier_call = sprd_eic_irq_notify;
> -	ret = atomic_notifier_chain_register(&sprd_eic_irq_notifier,
> -					     &sprd_eic->irq_nb);
> -	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "Failed to register with the interrupt notifier");
> -
> -	return devm_add_action_or_reset(dev, sprd_eic_unregister_notifier,
> -					&sprd_eic->irq_nb);
> +	return devm_atomic_notifier_chain_register(dev, &sprd_eic_irq_notifier,
> +						   &sprd_eic->irq_nb);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11