Re: [PATCH v9 14/22] drm/rockchip: dw_dp: Implement out-of-band HPD handling

Thank you for your contribution! Sashiko AI review found 3 potential issue(=
s) to consider:
- [High] Unpowered Register Access (SError) in OOB HPD callbacks.
- [Medium] Unreachable fallback logic in `dw_dp_rockchip_get_vo_grf`.
- [Medium] Physical IO address truncation due to `u32` typing in `ctrl_ids`.
--

--- Patch [14]: [PATCH v9 14/22] drm/rockchip: dw_dp: Implement out-of-band=
 HPD handling ---
commit 48b6797379434de3ea2e6ba62d355e782a42ada0
Author: Sebastian Reichel <[email protected]>

drm/rockchip: dw_dp: Implement out-of-band HPD handling

This patch implements out-of-band hotplug handling to receive external
hotplug information from the USB-C state machine via the vo_grf regmap.
It aims to avoid SError problems when runtime PM is enabled.

> diff --git a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c b/drivers/gpu/drm/=
rockchip/dw_dp-rockchip.c
> index 005938dc66c9f..d516b3910b516 100644
> --- a/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_dp-rockchip.c
[ ... ]
> @@ -23,12 +26,48 @@
> =20
>  #include "rockchip_drm_drv.h"
> =20
> +#define ROCKCHIP_MAX_CTRLS 2
> +
> +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL BIT(10)
> +#define ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG BIT(11)
> +
> +struct rockchip_dw_dp_plat_data {
> +	u8 num_ctrls;
> +	u32 ctrl_ids[ROCKCHIP_MAX_CTRLS];

[Severity: Medium]
Will this truncate physical IO addresses on 64-bit architectures?=20

The ctrl_ids array stores base IO addresses using u32, but in=20
dw_dp_rockchip_probe() this is compared against the 64-bit=20
resource_size_t res->start:

	if (res->start =3D=3D plat_data_const->ctrl_ids[id]) {

If a future variant maps the DP controller above the 4GB boundary,=20
the upper 32 bits of the address will be lost, causing the hardware=20
controller ID lookup to fail.

> +	u32 max_link_rate;
> +	u8 pixel_mode;
> +	u32 hpd_reg[ROCKCHIP_MAX_CTRLS];
> +};
> +
>  struct rockchip_dw_dp {
>  	struct dw_dp *base;
>  	struct device *dev;
> +	const struct rockchip_dw_dp_plat_data *pdata;
> +	struct regmap *vo_grf;
>  	struct rockchip_encoder *encoder;
> +	int id;
>  };
> =20
> +static void dw_dp_rockchip_hpd_sw_sel(void *data, bool force_hpd_from_sw)
> +{
> +	struct rockchip_dw_dp *dp =3D data;
> +	u32 hpd_reg =3D dp->pdata->hpd_reg[dp->id];
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_SEL, force_hpd_from_s=
w));

[Severity: High]
Does this code risk triggering an SError by accessing unpowered registers?=
=20

The commit message states that accessing the vo_grf register requires the D=
P=20
controller power domain to be active. Since this function is called during=
=20
initialization and the driver enables runtime PM with autosuspend, the writ=
e=20
might execute against an unpowered register domain. Should this acquire a P=
M=20
runtime reference via pm_runtime_get_sync(dp->dev) first?

> +}
> +
> +static void dw_dp_rockchip_hpd_sw_cfg(void *data, bool hpd)
> +{
> +	struct rockchip_dw_dp *dp =3D data;
> +	u32 hpd_reg =3D dp->pdata->hpd_reg[dp->id];
> +
> +	dev_dbg(dp->dev, "Force HPD connected=3D%s\n", str_yes_no(hpd));
> +
> +	regmap_write(dp->vo_grf, hpd_reg,
> +		     FIELD_PREP_WM16(ROCKCHIP_VO_GRF_DP_SINK_HPD_CFG, hpd));

[Severity: High]
Similarly, because this out-of-band hotplug notification is triggered=20
asynchronously by the Type-C controller, could the DP controller be suspend=
ed=20
when this executes? This seems to risk triggering the exact SError the patc=
h=20
intended to fix if a wake up isn't performed before the regmap write.

> +}
> +
[ ... ]
> @@ -71,6 +110,35 @@ static const struct drm_encoder_helper_funcs dw_dp_en=
coder_helper_funcs =3D {
>  	.atomic_check		=3D dw_dp_encoder_atomic_check,
>  };
> =20
> +static struct regmap *dw_dp_rockchip_get_vo_grf(struct rockchip_dw_dp *d=
p)
> +{
> +	struct device_node *np =3D dev_of_node(dp->dev);
> +	struct of_phandle_args args;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	ret =3D of_parse_phandle_with_args(np, "phys", "#phy-cells", 0, &args);
> +	if (ret)
> +		return ERR_PTR(-ENODEV);
> +
> +	/*
> +	 * Limit this workaround to RK3576 and RK3588, potential future platfor=
ms
> +	 * reusing the driver should just add a VO GRF phandle in the DisplayPo=
rt
> +	 * controller DT node.
> +	 */
> +	if (!of_device_is_compatible(args.np, "rockchip,rk3576-usbdp-phy") &&
> +	    !of_device_is_compatible(args.np, "rockchip,rk3588-usbdp-phy")) {
> +		regmap =3D ERR_PTR(-ENODEV);
> +		goto out_put_node;
> +	}

[Severity: Medium]
Will this unconditionally fail for future platforms instead of attempting=20
the fallback mechanism?=20

The comment above states that future platforms should add a VO GRF phandle=
=20
in the DP controller DT node. However, this logic explicitly returns -ENODE=
V=20
if the phy isn't one of the hardcoded compatible strings, without ever pars=
ing=20
the rockchip,vo-grf phandle from the DisplayPort node.=20

> +
> +	regmap =3D syscon_regmap_lookup_by_phandle(args.np, "rockchip,vo-grf");
> +
> +out_put_node:
> +	of_node_put(args.np);
> +	return regmap;
> +}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-synopsys-d=
[email protected]?part=3D14