Re: [PATCH v13 18/22] media: i2c: maxim-serdes: add MAX9296A driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional hardware link reset during `select_links` breaks concurrent streaming.

Pre-existing issues:
- [High] Race condition and potential use-after-free in I2C bus notifier unregistration.
- [High] Missing restoration of I2C `select_links` mask permanently isolates other active links.
--

commit bf07e545a3c441399f342386db38d37ea5e3b979
Author: Cosmin Tanislav <demonsingur@gmail.com>

    media: i2c: maxim-serdes: add MAX9296A driver

This commit introduces the MAX9296A driver for Quad GMSL2 Deserializers.
It adds support for multiple Maxim deserializer chips and integrates them
with the common Deserializer framework.

> diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/i2c/maxim-serdes/max9296a.c
> new file mode 100644
> index 0000000000000..3dcdb63dbb9ee
> --- /dev/null
> +++ b/drivers/media/i2c/maxim-serdes/max9296a.c

[ ... ]

> +static int max9296a_select_links(struct max_des *des, unsigned int mask)
> +{
> +	struct max9296a_priv *priv = des_to_priv(des);
> +	int ret;
> +
> +	if (des->info->num_links == 1)
> +		return 0;
> +
> +	if (!mask) {
> +		dev_err(priv->dev, "Disable all links unsupported\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(priv->regmap, MAX9296A_GMSL1_EN,
> +				 MAX9296A_GMSL1_EN_LINK_EN,
> +				 FIELD_PREP(MAX9296A_GMSL1_EN_LINK_EN, mask));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->regmap, MAX9296A_CTRL0,
> +				 MAX9296A_CTRL0_AUTO_LINK |
> +				 MAX9296A_CTRL0_LINK_CFG |
> +				 MAX9296A_CTRL0_RESET_ONESHOT,
> +				 FIELD_PREP(MAX9296A_CTRL0_LINK_CFG, mask) |
> +				 FIELD_PREP(MAX9296A_CTRL0_RESET_ONESHOT, 1));
> +	if (ret)
> +		return ret;
> +
> +	if (priv->info->has_per_link_reset) {
> +		ret = max9296a_reset_link(priv, 1);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does this regression unconditionally reset the hardware links regardless of the
mask?

Link 0 is reset via MAX9296A_CTRL0_RESET_ONESHOT, and Link 1 is reset via
max9296a_reset_link(priv, 1).

Since select_links() can be called dynamically to isolate I2C communications
(such as when a new camera is probed via the ATR), does this reset break
concurrent multi-camera operation by dropping other active streams?

[ ... ]


While reviewing how this driver interacts with the common framework, I noticed
a couple of pre-existing issues in drivers/media/i2c/maxim-serdes/max_des.c
that might need your attention.

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition and
use-after-free when unregistering the I2C bus notifier.

drivers/media/i2c/maxim-serdes/max_des.c:max_des_i2c_mux_deinit() {
    ...
	i2c_mux_del_adapters(priv->mux);
	bus_unregister_notifier(&i2c_bus_type, &priv->i2c_nb);
}

The child I2C adapters are deleted and their memory freed before the I2C bus
notifier is unregistered. Could an unrelated I2C device binding during this
window trigger max_des_i2c_mux_bus_notifier_call()?

Since the notifier compares the newly bound client->adapter against the freed
des->links[i].adapter pointers, if memory is reallocated and addresses match,
could the driver perform I2C operations on an unrelated bus, potentially
corrupting hardware state or crashing?


[Severity: High]
This is another pre-existing issue, but could the lack of state restoration
here permanently isolate other active links?

drivers/media/i2c/maxim-serdes/max_des.c:max_des_init_link_ser_xlate() {
    ...
	ret = des->ops->select_links(des, BIT(link->index));
	if (ret)
		return ret;

	ret = max_ser_wait_for_multiple(adapter, addrs, ARRAY_SIZE(addrs),
					&current_addr);
    ...
}

During I2C ATR attachment, this restricts I2C communication to a specific link
to modify serializer addresses, but the function returns without restoring the
select_links mask to include all enabled links.

Because hardware address translation (ATR) relies on the deserializer
forwarding commands to all active links concurrently, does this leave the
other links permanently disconnected from I2C control and break access to
previously probed cameras?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-gmsl2-3_serdes-v13-0-9d8a4919983b@analog.com?part=18