Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/imagination: inverted PVR_FEATURE_VALUE() check in layout_is_mars() - [High] drm/imagination: wrong default wait_fw_idle in must_wait_fw_idle_before_stop() --
commit 00700f5214fca29e9b17f38e573b7f84347d02b8 Author: Alessio Belle <[email protected]> drm/imagination: Limit access to the META firmware interface This commit restricts direct access to the META firmware interface by routing all META-specific checks through struct pvr_fw_defs callbacks and properties, removing pvr_fw_meta.h, and guarding pvr_rogue_meta_internal.h. > diff --git a/drivers/gpu/drm/imagination/pvr_fw_startstop.c > b/drivers/gpu/drm/imagination/pvr_fw_startstop.c > index 98ca2692239d0..9a617f78256ed 100644 > --- a/drivers/gpu/drm/imagination/pvr_fw_startstop.c > +++ b/drivers/gpu/drm/imagination/pvr_fw_startstop.c [ ... ] > @@ -199,6 +197,30 @@ pvr_fw_start(struct pvr_device *pvr_dev) > return err; > } > > +static inline bool layout_is_mars(struct pvr_device *pvr_dev) > +{ > + u64 layout_mars_value = 0; > + > + if (PVR_FEATURE_VALUE(pvr_dev, layout_mars, &layout_mars_value)) > + return layout_mars_value > 0; > + > + return false; > +} [Severity: High] Is the PVR_FEATURE_VALUE() check inverted in layout_is_mars()? In drivers/gpu/drm/imagination/pvr_device.h, PVR_FEATURE_VALUE() returns 0 on success and -EINVAL when the feature is absent: #define PVR_FEATURE_VALUE(pvr_dev, feature, value_out) \ ({ \ struct pvr_device *_pvr_dev = pvr_dev; \ int _ret = -EINVAL; \ if (_pvr_dev->features.has_##feature) { \ *(value_out) = _pvr_dev->features.feature; \ _ret = 0; \ } \ _ret; \ }) Previously, pvr_fw_stop() checked: if (PVR_FEATURE_VALUE(pvr_dev, layout_mars, &layout_mars_value) == 0) layout_mars = layout_mars_value > 0; Without the == 0 check in layout_is_mars(), when features.has_layout_mars is true, PVR_FEATURE_VALUE() returns 0 and the function falls through to return false. When features.has_layout_mars is false, PVR_FEATURE_VALUE() returns -EINVAL without modifying layout_mars_value (which remains 0), so layout_mars_value > 0 also evaluates to false. Doesn't this cause layout_is_mars() to always return false, making pvr_fw_stop() poll the powered-down ROGUE_CR_SIDEKICK_IDLE and ROGUE_CR_SLC_IDLE registers instead of ROGUE_CR_MARS_IDLE on GPUs with the layout_mars feature? > + > +static inline int must_wait_fw_idle_before_stop(struct pvr_device *pvr_dev) > +{ > + bool wait_fw_idle = false; > + int err; > + > + if (pvr_dev->fw_dev.defs->must_wait_fw_idle) { > + err = pvr_dev->fw_dev.defs->must_wait_fw_idle(pvr_dev, > &wait_fw_idle); > + if (err) > + return err; > + } > + > + return wait_fw_idle; > +} [Severity: High] Should wait_fw_idle be initialized to true instead of false in must_wait_fw_idle_before_stop()? The kernel-doc for struct pvr_fw_defs in drivers/gpu/drm/imagination/pvr_fw.h states: * This function is optional. The assumption is that we must always wait * for the firmware to reach idle; implement this function and return * %false to alter this behavior. Neither pvr_fw_defs_mips nor pvr_fw_defs_riscv implements must_wait_fw_idle. With wait_fw_idle initialized to false, calling pvr_fw_stop() -> must_wait_fw_idle_before_stop() on MIPS or non-MARS RISC-V firmware processors returns 0, which skips polling ROGUE_CR_SIDEKICK_IDLE for ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN before asserting ROGUE_CR_SOFT_RESET. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261028-prep-meta-fw-arch-independent-v1-0-73e116f02...@imgtec.com?part=4
