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