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 <[email protected]> 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/[email protected]?part=7
