Re: [PATCH v2 2/4] drm/fl2000: Add ITE IT66121 HDMI transmitter support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] drm/fl2000: duplicate IT66121 driver and unbound devres in
  it66121_create()
- [Medium] drm/fl2000: commit message claims EDID caching not in
  it66121_bridge_edid_read()
- [Medium] drm/fl2000: use-after-free of priv->client in
  it66121_bridge_detect()
--

commit a3c9c50f6dcb635240f532485f69c80008aed756
Author: Angel Gabriel Crispin Valdivia <angelcrispinvaldivia@gmail.com>

drm/fl2000: Add ITE IT66121 HDMI transmitter support

This commit adds a DRM bridge driver for the ITE IT66121 HDMI transmitter
on the FL2000 I2C bus and wires its creation and destruction into the USB
driver probe and disconnect paths.

> diff --git a/drivers/gpu/drm/fl2000/fl2000_it66121.c b/drivers/gpu/drm/fl2000/fl2000_it66121.c
> new file mode 100644
> index 0000000000000..7235317b0ceb7
> --- /dev/null
> +++ b/drivers/gpu/drm/fl2000/fl2000_it66121.c
[ ... ]
> +int it66121_hpd_connected(struct drm_bridge *bridge)
> +{
> +	struct it66121_priv *priv = container_of(bridge, struct it66121_priv, bridge);
> +	unsigned int val;
> +	int idx, ret;
> +
> +	if (!drm_bridge_enter(bridge, &idx))
> +		return -ENODEV;
> +
> +	ret = regmap_field_read(priv->hpd, &val);
> +	drm_bridge_exit(idx);
> +	if (ret)
> +		return ret;
> +
> +	return !!val;
> +}
[ ... ]
> +static enum drm_connector_status it66121_bridge_detect(struct drm_bridge *bridge,
> +						       struct drm_connector *connector)
> +{
> +	struct it66121_priv *priv = container_of(bridge, struct it66121_priv, bridge);
> +	int ret = it66121_hpd_connected(bridge);
> +
> +	if (ret > 0)
> +		return connector_status_connected;
> +
> +	/* Gone with the USB device */
> +	if (ret == -ENODEV)
> +		return connector_status_disconnected;
> +
> +	if (ret < 0) {
> +		/* Not bridge->dev: the chip, not the DRM device, failed to answer */
> +		dev_err_ratelimited(&priv->client->dev, "Cannot get monitor status (%d)\n", ret);
> +		return connector_status_unknown;
> +	}
> +
> +	return connector_status_disconnected;
> +}

[Severity: Medium]
Can it66121_bridge_detect() cause a use-after-free on priv->client if the
USB device is unplugged while regmap_field_read() is in flight?

When a USB control transfer fails during disconnect, fl2000_i2c_dword()
maps the error to -EIO:

drivers/gpu/drm/fl2000/fl2000_registers.c:fl2000_i2c_dword() {
    ...
	ret = regmap_write_bits(regmap, FL2000_VGA_I2C_SC_REG, mask, val);
	if (ret)
		return -EIO;
    ...
}

Because it66121_hpd_connected() calls drm_bridge_exit() before returning
that -EIO to it66121_bridge_detect(), a concurrent USB disconnect can
finish drm_bridge_unplug() and free priv->client before
dev_err_ratelimited() runs:

CPU0                                  CPU1
it66121_bridge_detect()
  it66121_hpd_connected()
    drm_bridge_enter()
    regmap_field_read() -> -EIO
    drm_bridge_exit()
                                      fl2000_disconnect()
                                        it66121_destroy()
                                          drm_bridge_unplug()
                                          i2c_unregister_device(priv->client)
  dev_err_ratelimited(&priv->client->dev, ...)
    <-- use-after-free

> +static const struct drm_edid *it66121_bridge_edid_read(struct drm_bridge *bridge,
> +						       struct drm_connector *connector)
> +{
> +	struct it66121_priv *priv = container_of(bridge, struct it66121_priv, bridge);
> +	const struct drm_edid *drm_edid;
> +	int idx, ret;
> +
> +	if (!drm_bridge_enter(bridge, &idx))
> +		return NULL;
> +
> +	if (priv->ddc_held) {
> +		/* Not fatal: the read tells whether the bus is free */
> +		ret = it66121_release_ddc(priv);
> +		if (ret)
> +			dev_dbg(&priv->client->dev, "Cannot release the DDC bus (%d)\n", ret);
> +		priv->ddc_held = false;
> +	}
> +
> +	drm_edid = drm_edid_read_custom(connector, it66121_get_edid_block, priv);
> +	if (!drm_edid)
> +		dev_warn(&priv->client->dev, "EDID read failed, using fallback modes\n");
> +
> +	drm_bridge_exit(idx);
> +
> +	return drm_edid;
> +}

[Severity: Medium]
The commit message says "The EDID is kept while the monitor stays, since
reading it over USB takes about a second", but does
it66121_bridge_edid_read() actually cache the EDID?

struct it66121_priv has no field to store the EDID, and
drm_bridge_connector_get_modes_edid() frees the struct drm_edid returned by
drm_bridge_edid_read() on each call, so every get_modes() call reads the
EDID over USB again.

[ ... ]

> +static struct i2c_client *it66121_i2c_init(struct i2c_adapter *adapter)
> +{
> +	/*
> +	 * Not "it66121": that name would make the ite-it66121 driver, if built,
> +	 * try to bind this client and release our devres when its probe fails.
> +	 */
> +	struct i2c_board_info board_info = { I2C_BOARD_INFO("fl2000-it66121", 0) };
> +
> +	/* According to datasheet IT66121 addresses are 0x98 or 0x9A including cmd */
> +	const unsigned short it66121_addr[] = { (0x98 >> 1), (0x9A >> 1), I2C_CLIENT_END };
> +	struct i2c_client *client;
> +
> +	/* The chip may still be waking up when the USB device has just enumerated */
> +	for (int i = 0; i < IT66121_DETECT_TRIES; i++) {
> +		if (i)
> +			msleep(IT66121_DETECT_DELAY_MS);
> +
> +		client = i2c_new_scanned_device(adapter, &board_info, it66121_addr,
> +						it66121_i2c_probe);
> +		if (!IS_ERR(client))
> +			break;
> +	}
> +
> +	return client;
> +}
[ ... ]
> +struct drm_bridge *it66121_create(struct i2c_adapter *adapter)
> +{
> +	int ret;
> +	struct it66121_priv *priv;
> +	struct i2c_client *client;
> +
> +	client = it66121_i2c_init(adapter);
> +	if (IS_ERR(client)) {
> +		dev_err(&adapter->dev, "No IT66121 found (%pe): adapters without one are not supported\n",
> +			client);
> +		return ERR_CAST(client);
> +	}
> +
> +	/*
> +	 * DRM core takes references on the bridge, so it must be allocated with
> +	 * devm_drm_bridge_alloc(). Freeing happens automatically when the last
> +	 * reference is dropped after i2c_unregister_device().
> +	 */
> +	priv = devm_drm_bridge_alloc(&client->dev, struct it66121_priv, bridge,
> +				     &it66121_bridge_funcs);

[Severity: Medium]
Why add a second driver for the ITE IT66121 instead of extending
drivers/gpu/drm/bridge/ite-it66121.c, and is it valid to attach devm
resources to an unbound i2c_client?

To keep the existing ite-it66121 driver from binding the client,
it66121_i2c_init() registers a dummy "fl2000-it66121" device with no driver
and attaches devm_drm_bridge_alloc(), devm_regmap_init_i2c() and
devm_regmap_field_alloc() to &client->dev outside of any driver probe.
The driver core treats devres entries on an unbound device as a bug in
really_probe() ("Resources present before probing").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-fl2000-upstream-v2-0-201f87708cb0@gmail.com?part=2