Re: [PATCH v2 4/5] drm/imagination: Limit access to the META firmware interface

[sashiko-bot@kernel.org]

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 in must_wait_fw_idle_before_stop()
--

commit 37bd66d7d521adbf18d35a370cc34ba7ca02c2dd
Author: Alessio Belle <alessio.belle@imgtec.com>

drm/imagination: Limit access to the META firmware interface

This commit abstracts META firmware-specific operations behind struct
pvr_fw_defs callbacks and properties so that pvr_fw_startstop.c no longer
accesses the META firmware interface directly. It also renames
pvr_rogue_meta.h to pvr_rogue_meta_internal.h and removes pvr_fw_meta.h.

> diff --git a/drivers/gpu/drm/imagination/pvr_fw.h b/drivers/gpu/drm/imagination/pvr_fw.h
> index 4b25291135b69..610406668bbb2 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw.h
> +++ b/drivers/gpu/drm/imagination/pvr_fw.h
> @@ -184,6 +184,23 @@ struct pvr_fw_defs {
>  	 */
>  	void (*irq_clear)(struct pvr_device *pvr_dev);
>  
> +	/**
> +	 * @must_wait_fw_idle: Check if the FW stop sequence should wait for hardware
> +	 *                     to report FW idle before proceeding.
> +	 *
> +	 * @pvr_dev: Target PowerVR device.
> +	 * @wait[out]: Whether the FW stop sequence should wait for FW idle.
> +	 *
> +	 * 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.
> +	 *
> +	 * Returns:
> +	 *  * 0 on success.
> +	 *  * Any appropriate error on failure.
> +	 */
> +	int (*must_wait_fw_idle)(struct pvr_device *pvr_dev, bool *wait);

[ ... ]

> 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
when the feature is present and -EINVAL when it 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;                                              \
	})

Prior to this change, pvr_fw_stop() checked PVR_FEATURE_VALUE(...) == 0.
Without the == 0 comparison, layout_is_mars() returns false when
has_layout_mars is true (since PVR_FEATURE_VALUE() returns 0), and also
returns false when has_layout_mars is false (since layout_mars_value remains
0).

As a result, when pvr_fw_stop() calls layout_is_mars(), layout_mars is
always false, causing pvr_fw_stop() on cores with the layout_mars feature to
poll powered-down ROGUE_CR_SIDEKICK_IDLE and ROGUE_CR_SLC_IDLE registers and
skip polling ROGUE_CR_MARS_IDLE before asserting ROGUE_CR_SOFT_RESET.

> +
> +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 must_wait_fw_idle in struct pvr_fw_defs states that the
callback is optional and that waiting for the firmware to reach idle is the
default behavior when it is not implemented. Neither pvr_fw_defs_mips in
drivers/gpu/drm/imagination/pvr_fw_mips.c nor pvr_fw_defs_riscv in
drivers/gpu/drm/imagination/pvr_fw_riscv.c implements must_wait_fw_idle.

Because wait_fw_idle defaults to false here, calling pvr_fw_stop() ->
must_wait_fw_idle_before_stop() on MIPS or RISC-V devices returns 0, causing
pvr_fw_stop() to skip polling ROGUE_CR_SIDEKICK_IDLE for
ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN before asserting ROGUE_CR_SOFT_RESET,
whereas previously skip_garten_idle defaulted to false.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-prep-meta-fw-arch-independent-v2-0-b9f5d69b7a52@imgtec.com?part=4