Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Per-CPU MSR read-modify-write and paired updates are performed without disabling preemption, risking cross-CPU corruption on migration. - [High] Infinite loop during TLB flush caused by a missing timeout break statement. -- commit 7751b392f6b4d557ed9824450e0e222fa5475b9b Author: Juergen Gross <[email protected]> agp/nvidia: Stop using 32-bit MSR interfaces The 32-bit MSR interfaces rdmsr() and wrmsr() are planned to be removed. This commit replaces them with the related 64-bit variants. > diff --git a/drivers/char/agp/nvidia-agp.c b/drivers/char/agp/nvidia-agp.c > index 4787391bb6b4f..3e760bc00afaa 100644 > --- a/drivers/char/agp/nvidia-agp.c > +++ b/drivers/char/agp/nvidia-agp.c [ ... ] > @@ -89,16 +87,16 @@ static int nvidia_init_iorr(u32 base, u32 size) > if (iorr_addr >= AMD_K7_NUM_IORR) > return -EINVAL; > } > - base_hi = 0x0; > - base_lo = (base & ~0xfff) | 0x18; > - mask_hi = 0xf; > - mask_lo = ((~(size - 1)) & 0xfffff000) | 0x800; > - wrmsr(IORR_BASE0 + 2 * iorr_addr, base_lo, base_hi); > - wrmsr(IORR_MASK0 + 2 * iorr_addr, mask_lo, mask_hi); > - > - rdmsr(SYSCFG, sys_lo, sys_hi); > - sys_lo |= 0x00100000; > - wrmsr(SYSCFG, sys_lo, sys_hi); > + base_msr.h = 0x0; > + base_msr.l = (base & ~0xfff) | 0x18; > + mask_msr.h = 0xf; > + mask_msr.l = ((~(size - 1)) & 0xfffff000) | 0x800; > + wrmsrq(IORR_BASE0 + 2 * iorr_addr, base_msr.q); > + wrmsrq(IORR_MASK0 + 2 * iorr_addr, mask_msr.q); > + > + rdmsrq(SYSCFG, sys_msr.q); > + sys_msr.l |= 0x00100000; > + wrmsrq(SYSCFG, sys_msr.q); [Severity: High] This isn't a bug introduced by this patch, but does this per-CPU MSR read-modify-write sequence need to run with preemption disabled? Because nvidia_init_iorr() is called during device probe and resume in standard process context, could the thread be preempted and migrated to another CPU between reading the MSR and writing it? If so, the target CPU's MSR could be overwritten with the source CPU's state. Similarly, could paired MSR updates (BASE and MASK) be split across two CPUs if a migration occurs here, leaving both in a corrupted state? > > return 0; > } [Severity: High] This isn't a bug introduced by this patch, but while reviewing this file I noticed a potential issue in nvidia_tlbflush(): drivers/char/agp/nvidia-agp.c:nvidia_tlbflush() { ... end = jiffies + 3*HZ; do { pci_read_config_dword(nvidia_private.dev_1, NVIDIA_1_WBC, &wbc_reg); if (time_before_eq(end, jiffies)) { printk(KERN_ERR PFX "TLB flush took more than 3 seconds.\n"); } } while (wbc_reg & nvidia_private.wbc_mask); ... } Should this loop break when the 3-second timeout is reached? If the hardware fails to clear the wbc_mask bit, there is no sleep or break mechanism, which seems like it could lead to an infinite loop and spam the syslog indefinitely. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
