Re: [PATCH v8 7/7] drm/tyr: add Microcontroller Unit (MCU) booting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Firmware sections lacking page alignment cause GPUVM mapping failures.
- [High] The MCU is not halted before its firmware memory is unmapped and freed, causing hardware faults.
- [Medium] Firmware sections with CacheMode::CachedCoherent are incorrectly mapped as Uncached.
- [Medium] The MCU boot completion interrupt is not cleared before starting the MCU, risking premature boot detection.
--

--- Patch [7]: [PATCH v8 7/7] drm/tyr: add Microcontroller Unit (MCU) booting ---
commit fab6374bf2ef501e789803698dbff3b27a24a705
Author: Deborah Brouwer <deborah.brouwer@collabora.com>

drm/tyr: add Microcontroller Unit (MCU) booting

This patch adds a firmware module to load, parse, and map the MCU firmware
sections into shared GEM memory. It creates a firmware instance during probe 
to boot the MCU, and updates Kconfig dependencies accordingly.

> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index b6528d8cd3cee..ebfb74e89c4c8 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
[ ... ]
> @@ -144,10 +148,21 @@ fn probe<'bound>(
>  
>          let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, Ok(()))?;
>  
> -        let _mmu = Mmu::new(pdev.as_ref(), iomem.as_arc_borrow(), &gpu_info)?;
> +        let mmu = Mmu::new(pdev.as_ref(), iomem.as_arc_borrow(), &gpu_info)?;
> +
> +        let firmware = Firmware::new(
> +            pdev.as_ref(),
> +            iomem.clone(),
> +            &unreg_dev,
> +            mmu.as_arc_borrow(),
> +            &gpu_info,
> +        )?;
> +
> +        firmware.boot()?;
>  
>          let reg_data = try_pin_init!(TyrDrmRegistrationData {
>                  pdev,
> +                fw: firmware,

[Severity: High]
If try_pin_init!() fails, will the MCU crash because the firmware is dropped?

When try_pin_init!() fails, the firmware instance will be dropped, calling
Firmware::drop() which destroys the VM (self.vm.kill()). Because the MCU was
already started in firmware.boot(), it will continue running and try to fetch 
instructions from memory that has just been unmapped and freed, potentially 
triggering IOMMU faults or hardware hangs.

>                  clks <- new_mutex!(Clocks {
>                      core: core_clk,
>                      stacks: stacks_clk,
> diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/fw.rs
[ ... ]
> +    fn load(
> +        dev: &Device,
> +        ddev: &TyrDrmDevice,
> +        gpu_info: &GpuInfo,
> +    ) -> Result<(kernel::firmware::Firmware, KVec<ParsedSection>)> {
> +        let fw = Self::request(ddev, gpu_info)?;
> +        let mut parser = FwParser::new(dev, fw.data());
> +
> +        let parsed_sections = parser.parse()?;

[Severity: Medium]
Does the firmware parser map coherent memory as uncached?

While parsing the firmware sections, the code checks the cache mode:

drivers/gpu/drm/tyr/fw/parser.rs:parse_section_entry() {
    ...
    if section_hdr.section_flags.cache_mode() != CacheMode::Cached {
        vm_map_flags |= VmFlag::Uncached;
    }
    ...
}

Because CacheMode::CachedCoherent has a value of 3, the check for inequality
with CacheMode::Cached evaluates to true. This erroneously assigns the
Uncached flag to CachedCoherent sections, which can break hardware
coherency expectations and degrade performance.

> +
> +        Ok((fw, parsed_sections))
> +    }
> +
> +    /// Load firmware and map sections into MCU VM.
> +    pub(crate) fn new(
> +        dev: &'bound Device<Bound>,
> +        iomem: Arc<IoMem<'bound>>,
> +        ddev: &TyrDrmDevice,
> +        mmu: ArcBorrow<'_, Mmu<'bound>>,
> +        gpu_info: &GpuInfo,
> +    ) -> Result<Firmware<'bound>> {
> +        let vm = Vm::new(dev, ddev, mmu, gpu_info)?;
> +        vm.activate()?;
> +
> +        let result = (|| {
> +            let (fw, parsed_sections) = Self::load(dev, ddev, gpu_info)?;
> +            let mut sections = KVec::new();
> +            for parsed in parsed_sections {
> +                let size = u64::from(parsed.va.end.checked_sub(parsed.va.start).ok_or(EINVAL)?);

[Severity: High]
Can this unaligned size cause GPUVM mapping failures later?

The size is computed directly from the firmware without aligning it to page 
boundaries. When the driver attempts to map this unaligned size into the VM,
the underlying page table mapping code will fail:

drivers/gpu/drm/tyr/vm.rs:pt_map() {
    ...
    let (pgsize, pgcount) = get_pgsize(curr_iova | curr_paddr, remaining);
    ...
        if mapped == 0 {
            dev_err!(dev, "Failed to map any pages at iova {:#x}\n", curr_iova);
            if segment_mapped > 0 {
                let _ = pt_unmap(dev, pt, iova..(iova + segment_mapped));
            }
            return Err(ENOMEM);
        }
    ...
}

For unaligned sizes, the final chunk produces a pgcount of 0 from
get_pgsize(), causing mapped to be 0 and triggering an unconditional
ENOMEM error, which causes the firmware load to fail.

> +
> +                let va = u64::from(parsed.va.start);
> +
> +                let mut mem = KernelBo::new(
> +                    ddev,
> +                    vm.clone(),
> +                    size,
> +                    KernelBoVaAlloc::Explicit(va),
> +                    parsed.vm_map_flags,
> +                )?;
[ ... ]
> +    pub(crate) fn boot(&self) -> Result {
> +        let io = &self.iomem;
> +        io.write_reg(MCU_CONTROL::zeroed().with_req(McuControlMode::Auto));
> +
> +        if let Err(e) = poll::read_poll_timeout(
> +            || Ok((io.read(MCU_STATUS), io.read(JOB_IRQ_RAWSTAT))),
> +            |(mcu_status, irq_rawstat)| {
> +                mcu_status.value() == McuStatus::Enabled && irq_rawstat.glb()
> +            },
> +            time::Delta::from_millis(1),
> +            time::Delta::from_millis(100),
> +        ) {
> +            let status = io.read(MCU_STATUS);
> +            dev_err!(
> +                self.vm.dev(),
> +                "MCU failed to boot, status: {:?}\n",
> +                status.value()
> +            );
> +            return Err(e);
> +        }
> +
> +        io.write_reg(JOB_IRQ_CLEAR::zeroed().with_glb(true));

[Severity: Medium]
Should the interrupt be cleared before polling for MCU boot completion?

Because JOB_IRQ_CLEAR is written only after the polling loop, a stale 
interrupt bit (e.g. from the bootloader or a prior crashed state) could be 
present when polling starts. This would cause the polling loop to succeed 
instantly before the newly started MCU actually completes its boot.

> +
> +        Ok(())
> +    }
> +}

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