Re: [PATCH v2 1/7] pinctrl: renesas: rzg2l: Generalize the power source code

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Acquiring an IRQ-safe lock without disabling interrupts causes lockdep inconsistency and potential deadlocks.
--

commit 22349bc4fa49f3c73863f4e8930404081ba4603f
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

pinctrl: renesas: rzg2l: Generalize the power source code

This commit generalizes the get/set pin power source functions to allow
extension for other functionalities like I3C on RZ/G3S. It introduces a
register masks data structure and updates the register modification in
rzg2l_set_power_source() to use a generic read-modify-write approach.

> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index ac42093fc579c..a648d75a2bd23 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c

[ ... ]

> @@ -1077,76 +1144,51 @@ static int rzg2l_get_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps
>  static int rzg2l_set_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps, u32 ps)
>  {

[ ... ]

> -	if (pwr_reg == OTHER_POC) {
> -		scoped_guard(raw_spinlock, &pctrl->lock) {
> -			val = readb(pctrl->base + pwr_reg);
> -			if (poc_val)
> -				val |= mask;
> -			else
> -				val &= ~mask;
> -			writeb(val, pctrl->base + pwr_reg);
> -		}
> -	} else {
> -		writeb(poc_val, pctrl->base + pwr_reg);
> +	scoped_guard(raw_spinlock, &pctrl->lock) {
> +		u8 tmp = readb(pctrl->base + offset);

[Severity: High]
This isn't a bug introduced by this patch, but since this change expands the
lock usage to all power source updates, could this result in a deadlock?

The lock pctrl->lock is an IRQ-safe raw spinlock, acquired with interrupts
disabled in functions like rzg2l_gpio_irq_disable().

Using scoped_guard(raw_spinlock, &pctrl->lock) disables preemption but
leaves local hardware interrupts enabled. If a GPIO interrupt fires on the
same CPU while this lock is held, and the client interrupt handler calls
disable_irq_nosync(), the system would attempt to acquire the already-held
lock.

Would it be safer to use scoped_guard(raw_spinlock_irqsave, &pctrl->lock)
instead?

>  
> +		tmp &= ~mask;
> +		tmp |= field_prep(mask, val);
> +		writeb(tmp, pctrl->base + offset);
>  	}
>  
>  	pctrl->settings[pin].power_source = ps;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528080439.615958-1-claudiu.beznea@kernel.org?part=1