Re: [PATCH v8 1/7] drm/tyr: add resources to RegistrationData

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Clocks are enabled before power domains (regulators) are powered on.
- [Medium] Clock enable state is leaked on early probe failure paths.
--

--- Patch [1]: [PATCH v8 1/7] drm/tyr: add resources to RegistrationData ---
commit ebb25c59afc71f766bee84e4619205c5e8db839b
Author: Deborah Brouwer <deborah.brouwer@collabora.com>

drm/tyr: add resources to RegistrationData

This commit moves Tyr's device-private resources and GPU information
from drm::Driver::Data to drm::Driver::RegistrationData. This ensures
safe access to the data during the platform device binding lifetime and
while registered with userspace.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index 8348c6cd39295..46ce5c41e310c 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs

[ ... ]

> @@ -134,10 +137,10 @@ fn probe<'bound>(
>          // other threads of execution.
>          unsafe { pdev.dma_set_mask_and_coherent(DmaMask::try_new(pa_bits)?)? };
>  
> -        let platform: ARef<platform::Device> = pdev.into();
> +        let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, Ok(()))?;
>  
> -        let data = try_pin_init!(TyrDrmDeviceData {
> -                pdev: platform.clone(),
> +        let reg_data = try_pin_init!(TyrDrmRegistrationData {
> +                pdev,
>                  clks <- new_mutex!(Clocks {

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential resource leak in 
probe(). 

Earlier in this function, core_clk, stacks_clk, and coregroup_clk are 
enabled:

drivers/gpu/drm/tyr/driver.rs:probe() {
    ...
    core_clk.prepare_enable()?;
    stacks_clk.prepare_enable()?;
    coregroup_clk.prepare_enable()?;

    let mali_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"mali")?;
    ...
}

If getting a regulator or another step fails and returns early via ?, does 
the clock enable state leak? Because the Clocks struct isn't created until 
this point in the diff, it seems the enable count might remain incremented 
on those early error paths.

[Severity: High]
This is also a pre-existing issue, but looking at that same snippet in 
probe(), the hardware clocks are enabled before the mali and sram regulators 
are fetched and enabled. 

Could this sequencing cause problems? Toggling clocks for unpowered IP 
domains can sometimes lead to bus lockups or undefined hardware behavior 
during initialization.

>                      core: core_clk,
>                      stacks: stacks_clk,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-fw-boot-b4-v8-0-b28f744581c9@collabora.com?part=1