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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardware address space slots and `Vm` memory leak on `Firmware::new` error paths.
- [High] Integer underflow when parsing firmware entry sizes causes a kernel panic.
- [Medium] The firmware boot sequence polls for the global interrupt to signal completion but fails to clear it.
--

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

drm/tyr: add Microcontroller Unit (MCU) booting

Add a firmware module to load, parse, and map the MCU firmware sections
into shared GEM memory at the required virtual addresses accessible by the
GPU.

> diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
> index 0000000000000..554808a792aa5
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/fw.rs

[ ... ]

> +    fn load(
> +        ddev: &TyrDrmDevice,
> +        gpu_info: &GpuInfo,
> +    ) -> Result<(kernel::firmware::Firmware, KVec<ParsedSection>)> {
> +        let fw = Self::request(ddev, gpu_info)?;
> +        let mut parser = FwParser::new(fw.data());
> +
> +        let parsed_sections = parser.parse()?;

[Severity: High]
When parsing the firmware entry sizes, is there a risk of an integer underflow
if the entry size is 0?

Looking at FwParser::parse_entry() in parser.rs, the alignment check doesn't
seem to reject a size of 0:

    if self.cursor.pos() % size_of::<u32>() != 0
        || entry_section.entry_hdr.size() as usize % size_of::<u32>() != 0
    {
        ...
        return Err(EINVAL);
    }

Since 0 % 4 == 0, the check passes. Then it calculates section_hdr_size:

    let section_hdr_size = entry_section.entry_hdr.size() as usize - size_of::<EntryHeader>();

If the size is 0, subtracting size_of::<EntryHeader>() (which is 4) will
underflow, causing a kernel panic. Should there be a check to ensure the
size is at least size_of::<EntryHeader>()?

> +
> +        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 (fw, parsed_sections) = Self::load(ddev, gpu_info)?;
> +        let mut sections = KVec::new();

[Severity: High]
If an error occurs after vm.activate(), such as in Self::load() or
KernelBo::new(), does this leak the hardware address space slot and the
Vm allocation?

vm.activate() creates a circular reference by storing an Arc<VmAsData> in the
hardware slot manager. This cycle is normally broken by self.vm.kill() in
Firmware::drop():

impl<'bound> Drop for Firmware<'bound> {
    fn drop(&mut self) {
        // AS slots retain a VM ref, we need to kill the circular ref manually.
        self.vm.kill();
    }
}

If Firmware::new() returns early with an error, the Firmware object is never
constructed, so its Drop implementation won't run. Should vm.kill() be
called on these error paths before returning?

[ ... ]

> +    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);
> +            pr_err!("MCU failed to boot, status: {:?}", status.value());
> +            return Err(e);
> +        }
> +        Ok(())
> +    }
> +}

[Severity: Medium]
Does this leave the global interrupt asserted in the hardware's RAWSTAT
register?

The polling loop checks for the interrupt:

    |(mcu_status, irq_rawstat)| {
        mcu_status.value() == McuStatus::Enabled && irq_rawstat.glb()
    }

But once the loop succeeds, it returns without writing to JOB_IRQ_CLEAR to
acknowledge and clear the interrupt. Could this cause an immediate spurious
interrupt to fire once the DRM driver registers its interrupt handler and
unmasks interrupts at the GIC?

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