Re: [PATCH v2 02/10] mfd: rz-mtu3: use device-managed reset deassert

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

New issues:
- [Low] The commit message description is not capitalized.
- [Low] The newly renamed function `rz_mtu3_mfd_remove` includes the string 'mfd'.

Pre-existing issues:
- [High] Child drivers silently overwrite shared parent state (`ch->dev`), leading to incorrect PM runtime behavior and potential crashes if one sibling driver is unbound.
--

--- Patch [2]: [PATCH v2 02/10] mfd: rz-mtu3: use device-managed reset deassert ---
commit 24acc3a25ff8c1834d9bfb7c89b33b5e39d1f472
Author: Cosmin Tanislav <[email protected]>

mfd: rz-mtu3: use device-managed reset deassert

Replace devm_reset_control_get_exclusive() and the manual
reset_control_deassert()/reset_control_assert() with handling by
devm_reset_control_get_exclusive_deasserted().

> mfd: rz-mtu3: use device-managed reset deassert

[Severity: Low]
This isn't a bug, but should the description after the subsystem prefix be
capitalized? MFD guidelines typically prefer "mfd: <Driver>: <Capitalized
description>".

> diff --git a/drivers/mfd/rz-mtu3.c b/drivers/mfd/rz-mtu3.c
> index 9cdfef610398f..6b9c6831dffa9 100644
> --- a/drivers/mfd/rz-mtu3.c
> +++ b/drivers/mfd/rz-mtu3.c
[ ... ]
> -static void rz_mtu3_reset_assert(void *data)
> +static void rz_mtu3_mfd_remove(void *data)
>  {
>  	mfd_remove_devices(data);
>  }

[Severity: Low]
This isn't a bug, but does this function name violate the MFD naming
conventions? The guidelines suggest avoiding the string "mfd" in driver,
struct, or device names to prevent hard-coding implementation details.

[Severity: High]
This is a pre-existing issue, but does this design allow child drivers to
silently overwrite shared parent state?

The parent MFD driver allocates a shared array of channels. Both pwm-rz-mtu3
and rz-mtu3-cnt drivers share access to channels 1, 2, and 3.

In drivers/pwm/pwm-rz-mtu3.c:rz_mtu3_pwm_probe(), the PWM child driver
unconditionally sets its own device pointer on the shared channel:

    rz_mtu3_pwm->channel_data[j].mtu->dev = dev;

In drivers/counter/rz-mtu3-cnt.c:rz_mtu3_cnt_probe(), the Counter child
driver also unconditionally sets its own device pointer on the same shared
channel:

    ch->dev = dev;

Whichever driver probes last will overwrite the pointer. If the surviving
driver calls pm_runtime_get_sync(ch->dev), it will inadvertently invoke PM
runtime on the other driver's device structure.

If that other driver is unbound via sysfs, its PM runtime is disabled,
causing pm_runtime_get_sync() to fail with -EACCES. The counter driver might
ignore this return value and proceed to access hardware registers without
being properly clocked or powered on.

Could this lead to a synchronous external abort or silent data corruption?

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