Re: [PATCH v2 2/2] pinctrl: meson: restore non-sleeping GPIO access

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

Pre-existing issues:
- [High] Setting `can_sleep = false` without configuring regmap to use raw spinlocks causes a 'sleeping while atomic' BUG on PREEMPT_RT.
- [Medium] Data race on global `meson_regmap_config` during concurrent pinctrl probing causes unpredictable device initialization.
- [Medium] Unchecked `regmap_read()` in `meson_gpio_get()` can expose uninitialized stack memory.
- [Medium] Hardware glitch due to configuring GPIO direction to output before setting the drive value.
--

commit 8315fdbb201c9d14689366b3fb143e70fef66b77
Author: Viacheslav Bocharov <[email protected]>

    pinctrl: meson: restore non-sleeping GPIO access

This commit restores the non-sleeping behavior of the meson GPIO controller by
setting the can_sleep flag to false. The workaround for the sleeping lock in
the shared-proxy has been fixed upstream, meaning the controller-wide
workaround is no longer necessary.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 4507dc8b5563c..18295b15ecd9d 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.c
> +++ b/drivers/pinctrl/meson/pinctrl-meson.c
> @@ -619,7 +619,7 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
>  	pc->chip.set = meson_gpio_set;
>  	pc->chip.base = -1;
>  	pc->chip.ngpio = pc->data->num_pins;
> -	pc->chip.can_sleep = true;
> +	pc->chip.can_sleep = false;

[Severity: High]
This isn't a bug introduced by this patch, but will setting the can_sleep flag
to false trigger a 'sleeping while atomic' lockdep splat on PREEMPT_RT kernels?

The regmap configuration for this device lacks .use_raw_spinlock = true:

drivers/pinctrl/meson/pinctrl-meson.c:
static struct regmap_config meson_regmap_config = {
	.reg_bits = 32,
	.val_bits = 32,
	.reg_stride = 4,
};

Without the raw spinlock flag, regmap defaults to standard spinlocks which
become sleepable rt_mutexes on PREEMPT_RT. Callers like w1-gpio invoke these
callbacks under local_irq_save(), so trying to acquire the sleepable lock
with IRQs disabled is invalid. Could the regmap config be updated to ensure
the hardware access is truly non-sleeping?

[Severity: Medium]
This is a pre-existing issue, but is there a data race when configuring the
regmap in meson_map_resource()?

drivers/pinctrl/meson/pinctrl-meson.c:meson_map_resource() {
    ...
	meson_regmap_config.max_register = resource_size(&res) - 4;
	meson_regmap_config.name = devm_kasprintf(pc->dev, GFP_KERNEL,
						  "%pOFn-%s", node,
						  name);
    ...
}

If multiple Amlogic pinctrl instances (e.g. standard and AO) probe
concurrently or via deferred probing, they could overwrite the static
meson_regmap_config before devm_regmap_init_mmio() is called. Might this
result in one controller receiving another's configuration, or cause
out-of-bounds MMIO accesses?

[Severity: Medium]
This isn't a bug introduced by this patch, but is uninitialized stack memory
leaked if the MMIO read fails?

drivers/pinctrl/meson/pinctrl-meson.c:meson_gpio_get() {
    ...
	regmap_read(pc->reg_gpio, reg, &val);

	return !!(val & BIT(bit));
}

Since the return value of regmap_read() is ignored, a failed read (e.g. due
to an out-of-bounds register from the max_register data race) leaves val
uninitialized. Evaluating and returning val unconditionally could leak a bit
of kernel stack memory.

[Severity: Medium]
This is a pre-existing issue, but could meson_pinconf_set_output_drive()
cause an observable hardware glitch?

drivers/pinctrl/meson/pinctrl-meson.c:meson_pinconf_set_output_drive() {
    ...
	ret = meson_pinconf_set_output(pc, pin, true);
	if (ret)
		return ret;

	return meson_pinconf_set_drive(pc, pin, high);
}

By configuring the pin direction to output before setting the correct drive
value, the line is driven using whatever stale bit is present in the
MESON_REG_OUT register. Can this drive an incorrect level onto the line for a
brief interval, potentially false-triggering edge-sensitive external
peripherals?

>  
>  	ret = gpiochip_add_data(&pc->chip, pc);
>  	if (ret) {

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

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic