Re: [PATCH v2 2/2] staging: i3c: add Realtek RTS490x I3C HUB driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] OF node references are not properly managed during DT iteration, causing memory leaks and dangling pointers.
- [High] Probe error paths fail to unregister successfully added I2C adapters, leaving dangling pointers to freed memory.
- [High] Missing `reinit_completion()` before starting SMBus transactions causes premature completion following a timeout.
- [High] The driver temporarily overwrites the hub device's `of_node` during logical bus registration without restoring it.
- [High] Incorrect teardown order in `i3c_hub_remove` allows delayed works to execute after resources are destroyed.
- [High] Concurrent transfers corrupt the shared device descriptor's master pointer and cause bus faults.
- [High] Dynamic Address Assignment (DAA) fails to open target ports, preventing devices from receiving addresses.
- [High] GPIO IRQ callbacks use sleeping regmap operations while the IRQ core holds raw spinlocks.
- [High] Dropping a mutex before calling an I2C slave event callback allows concurrent unregistration and freeing of the target driver.
- [High] Parsing the I3C port ID using `%i` allows negative indices, bypassing bounds checks.
- [Medium] SMBus target logic incorrectly drops 0-byte writes (e.g., SMBus Quick Commands) instead of notifying the I2C slave backend.
- [Medium] Return value of `regmap_write` is ignored before reading buffer length.
- [Medium] Early returns in `i3c_hub_delayed_work` error paths leak dynamically registered logical buses and abort partial initialization.
--

commit 9f58d1e76a5b04c7b2e855b10a1eea1283c0e9ee
Author: Yin Zhou <zain_zhou@realsil.com.cn>

staging: i3c: add Realtek RTS490x I3C HUB driver

Add driver for Realtek RTS490x series I3C HUB devices. It supports DT-based
configuration, LDO and pull-up management, logical I3C bus registration,
SMBus agent functionality, and GPIO expansion.

> diff --git a/drivers/staging/rts490x/rts490xa-i3c-hub.c b/drivers/staging/rts490x/rts490xa-i3c-hub.c
> new file mode 100644
> index 0000000000000..fdfff5c6dff5d
> --- /dev/null
> +++ b/drivers/staging/rts490x/rts490xa-i3c-hub.c
[ ... ]
> @@ -354,12 +354,6 @@ static void i3c_hub_tp_of_get_setting(struct device *dev,
>  		if (id >= priv->dev_info->n_ports) {
>  			dev_warn(dev,
>  				 "Invalid target port index found in DT: %i\n",
>  				 id);
>  			continue;
>  		}
>  
>  		priv->smbus_port_adapter[id].of_node = tp_node;

[Severity: High]
Could this lead to dangling pointers? The iterator loop drops the reference
to the node when it advances, leaving priv->smbus_port_adapter[id].of_node
pointing to freed memory since of_node_get() isn't called.

[ ... ]
> @@ -818,12 +818,7 @@ static void i3c_hub_of_get_conf_runtime(struct device *dev,
>  	struct device_node *i3c_node;
>  	int i3c_id;
>  	u8 tp_mask;
>  
>  	for_each_available_child_of_node(node, i3c_node) {
>  		if (!i3c_node->full_name ||
>  		    (sscanf(i3c_node->full_name, "i3c%i@%hhx", &i3c_id,
>  			    &tp_mask) != 2))
>  			continue;
>  
>  		if (i3c_id < priv->dev_info->n_ports) {

[Severity: High]
Could the use of %i allow negative values like "i3c-1@0" to be parsed?

If i3c_id is negative, it gets promoted to an unsigned value in the bounds
check (i3c_id < priv->dev_info->n_ports), allowing it to bypass the check
and write out of bounds.

>  			priv->logical_bus[i3c_id].available = true;
>  			priv->logical_bus[i3c_id].of_node = i3c_node;

[Severity: High]
Is an of_node_get() missing here as well before saving i3c_node?

>  			priv->logical_bus[i3c_id].tp_map = tp_mask;
>  			priv->logical_bus[i3c_id].priv = priv;
>  			priv->logical_bus[i3c_id].tp_id = i3c_id;
>  		}
>  	}
>  }
[ ... ]
> @@ -861,15 +861,15 @@ static struct device_node *i3c_hub_get_dt_hub_node(struct device_node *node,
>  
>  			if (!found_id) {
>  				/*
>  				 * Just keep reference to first HUB node with no ID in case no ID
>  				 * matching
>  				 */
>  				if (!hub_node_no_id &&
>  				    priv->hub_dt_sel_id == -1 &&
>  				    priv->hub_dt_cp1_id == -1)
>  					hub_node_no_id = hub_node;
>  			} else {
>  				return hub_node;
>  			}
>  		}
>  	}
>  
>  	return hub_node_no_id;
>  }

[Severity: High]
Can returning the node directly from the iterator cause a memory leak?

Returning hub_node without releasing the reference held by the loop iterator
appears to leak it. Conversely, if it returns hub_node_no_id, isn't that
pointer already freed since the loop has advanced or completed?

[ ... ]
> @@ -1429,20 +1429,19 @@ static int i3c_hub_read_transaction_status(struct i3c_hub *priv, u8 target_port,
>  					   u8 target_port_status, u32 data_len)
>  {
>  	unsigned int status_read;
>  	int ret;
>  	struct i2c_adapter_group *smbus =
>  		&priv->smbus_port_adapter[target_port];
>  	u32 smbus_clk = priv->settings.tp[target_port].clock_frequency / 1000;
>  	u8 status;
>  	u8 ret_code;
>  
>  	if (!priv->settings.tp[target_port].poll_interval_ms) {
>  		ret = wait_for_completion_timeout(&smbus->completion,
>  						  smbus->i2c.timeout);

[Severity: High]
Should there be a call to reinit_completion() before starting the SMBus
transaction?

If a timeout occurs, a delayed hardware interrupt might call complete()
later. The next transaction would then return immediately from
wait_for_completion_timeout() because the leftover completion count wasn't
consumed.

[ ... ]
> @@ -1664,15 +1664,13 @@ static void i3c_hub_bus_cleanup(struct i3c_master_controller *controller)
>  {
>  	controller->this = NULL;
>  }
>  
>  static int i3c_hub_attach_i3c_dev(struct i3c_dev_desc *dev)
[ ... ]
> @@ -1714,14 +1714,10 @@ static void i3c_hub_detach_i3c_dev(struct i3c_dev_desc *dev)
>  static int i3c_hub_do_daa(struct i3c_master_controller *controller)
>  {
>  	struct i3c_master_controller *parent =
>  		parent_from_controller(controller);
>  	int ret;
>  
>  	down_write(&parent->bus.lock);
>  	ret = parent->ops->do_daa(parent);
>  	up_write(&parent->bus.lock);
>  	return ret;
>  }

[Severity: High]
Is this missing a call to i3c_hub_trans_pre_cb(bus) before doing DAA?

Without opening the logical bus's target port beforehand, devices on that
port wouldn't receive ENTDAA broadcasts or dynamic addresses.

[ ... ]
> @@ -1742,16 +1742,14 @@ static int i3c_hub_send_ccc_cmd(struct i3c_master_controller *controller,
>  static int i3c_hub_priv_xfers(struct i3c_dev_desc *dev,
>  			      struct i3c_xfer *xfers, int nxfers,
>  			      enum i3c_xfer_mode mode)
>  {
>  	struct i3c_master_controller *parent =
>  		parent_controller_from_i3c_desc(dev);
>  	struct i3c_master_controller *orig_parent;
>  	struct logical_bus *bus = bus_from_i3c_desc(dev);
>  	int res;
>  
>  	i3c_hub_trans_pre_cb(bus);
>  	orig_parent = update_i3c_i2c_desc_parent(&dev->common, parent);
>  	down_read(&parent->bus.lock);
>  	res = parent->ops->i3c_xfers(dev, xfers, nxfers, mode);
>  	up_read(&parent->bus.lock);
>  	restore_i3c_i2c_desc_parent(&dev->common, orig_parent);
>  	i3c_hub_trans_post_cb(bus);
>  
>  	return res;
>  }

[Severity: High]
Could concurrent transfers to the same device cause data corruption and bus
faults?

Since update_i3c_i2c_desc_parent() modifies the shared desc->master without
synchronization, and the I3C core holds the bus lock only as a reader,
concurrent accesses might restore incorrect pointers.

Additionally, if two threads communicate on the same port concurrently, won't
the first thread to finish close the target port in i3c_hub_trans_post_cb
while the second is still communicating?

[ ... ]
> @@ -2037,21 +2037,18 @@ static void i3c_hub_delayed_work(struct work_struct *work)
>  	for (i = 0; i < priv->dev_info->n_ports; ++i) {
>  		bus = &priv->logical_bus[i];
>  		if (bus->available) {
>  			ret = regmap_update_bits(priv->regmap,
>  						 I3C_HUB_TP_NET_CON_CONF,
>  						 GENMASK(bus->tp_id, bus->tp_id),
>  						 bus->tp_map);
>  			if (ret) {
>  				dev_warn(dev,
>  					 "Failed to open Target Port(s)\n");
>  				return;
>  			}
>  
>  			dev->of_node = bus->of_node;

[Severity: High]
Does this permanently overwrite the physical hub device's dev->of_node
pointer?

By not restoring it after registration, it gets left pointing to its last
processed child port, which might break subsequent DT lookups.

>  			ret = i3c_hub_logic_register(&bus->controller, dev);
>  			if (ret) {
>  				dev_warn(dev,
>  					 "Failed to register i3c controller - bus id:%i\n",
>  					 i);
>  				return;
>  			}

[Severity: Medium]
Will returning early here leak any logical buses that were successfully
registered in previous loop iterations? The early return also aborts the DAA
sequence and scheduling of the polling works, leaving the hub in a partially
initialized state.

[ ... ]
> @@ -2091,18 +2091,18 @@ static int send_smbus_target_data_to_backend(struct i2c_adapter_group *smbus,
>  					     u8 address, u8 *local_buffer,
>  					     u8 len)
>  {
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	struct smbus_backend *backend;
>  	struct i2c_client *client;
>  	int i, ret;
>  	u8 tmp;
>  
>  	mutex_lock(&smbus->mutex);
>  
>  	list_for_each_entry(backend, &smbus->backend_entry, list) {
>  		client = backend->client;
>  		if (client->addr == address >> 1) {
>  			mutex_unlock(&smbus->mutex);
>  			ret = i2c_slave_event(client, I2C_SLAVE_WRITE_REQUESTED,
>  					      &address);

[Severity: High]
Can dropping the mutex here lead to a use-after-free?

This breaks synchronization with unreg_i2c_target(), allowing a target
driver to concurrently unregister and free the backend structure while
i2c_slave_event() is invoking the callback using the pointer derived from
the freed backend.

[ ... ]
> @@ -2126,20 +2126,17 @@ static int read_smbus_target_buffer_page(struct i2c_adapter_group *smbus,
>  					 u8 target_buffer_page, u8 *address,
>  					 u8 *local_buffer, u8 *len)
>  {
>  	struct i3c_hub *hub = smbus->priv;
>  	struct device *dev = i3cdev_to_dev(hub->i3cdev);
>  	u32 status;
>  	int ret;
>  
>  	mutex_lock(&hub->page_mutex);
>  	regmap_write(hub->regmap, I3C_HUB_PAGE_PTR, target_buffer_page);

[Severity: Medium]
Should the return value of regmap_write() be checked here? If this write
fails, the subsequent reads will silently operate on whatever page was
previously selected.

>  
>  	ret = regmap_read(hub->regmap, I3C_HUB_TARGET_BUFF_LENGTH, &status);
>  	if (ret)
>  		goto error;
>  
>  	*len = status - 1;
>  	if (!*len)
>  		goto error;

[Severity: Medium]
Will this incorrectly drop 0-byte writes, such as SMBus Quick Commands?

For a quick command containing only the address byte, status is 1, making
*len evaluate to 0. Jumping to error skips sending the event to the I2C
slave backend.

[ ... ]
> @@ -2598,16 +2598,12 @@ static void i3c_hub_gpio_irq_mask(struct irq_data *d)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct i3c_hub *hub = gpiochip_get_data(gc);
>  	struct hub_gpio *gpio = &hub->gpio;
>  	u8 reg, hwirq = 0, mask = 0;
>  
>  	hwirq = irqd_to_hwirq(d);
>  
>  	reg = hwirq % GPIO_BANK_SZ ? I3C_HUB_TP_SDA_IN_DETECT_IBI_EN :
>  				     I3C_HUB_TP_SCL_IN_DETECT_IBI_EN;
>  	mask = BIT(gpio->tp[hwirq / GPIO_BANK_SZ]);
>  
>  	regmap_update_bits(hub->regmap, reg, mask, 0);
>  }

[Severity: High]
Does calling regmap_update_bits() here cause a "scheduling while atomic" BUG?

The generic IRQ subsystem holds the IRQ descriptor's raw spinlock while
invoking the irq_mask/irq_unmask/irq_set_type callbacks, but regmap over I3C
acquires a mutex and can sleep.

[ ... ]
> @@ -2880,14 +2880,14 @@ static int i3c_hub_probe(struct i3c_device *i3cdev)
>  
>  	/* Register logic for native smbus ports */
>  	for (i = 0; i < priv->dev_info->n_ports; i++) {
>  		priv->smbus_port_adapter[i].used = 0;
>  		if (priv->settings.tp[i].mode == I3C_HUB_DT_TP_MODE_SMBUS) {
>  			ret = i3c_hub_smbus_tp_algo(priv, i);
>  			if (ret)
>  				dev_warn(dev,
>  					 "Failed to setup SMBus adapter, port: %d\n",
>  					 i);
>  		}
>  	}
[ ... ]
>  	/* IBI */
>  	ibireq.handler = i3c_hub_ibi_handler;
>  	ibireq.max_payload_len = IBI_MAX_PAYLOAD_LEN;
>  	ibireq.num_slots = IBI_SLOT_NUMS;
>  
>  	ret = i3c_device_request_ibi(i3cdev, &ibireq);
>  	if (ret) {
>  		dev_err(dev, "Failed to requeset ibi!\n");
>  		goto error;
>  	}

[Severity: High]
If an error occurs after registering the SMBus adapters, does jumping to the
error labels bypass unregistering the I2C adapters?

Since priv is managed by devm_kzalloc and gets freed when probe fails,
wouldn't the I2C subsystem retain references to freed memory?

[ ... ]
> @@ -2965,11 +2965,11 @@ static void i3c_hub_remove(struct i3c_device *i3cdev)
>  {
>  	struct i3c_hub *priv = i3cdev_get_drvdata(i3cdev);
>  	int i;
>  
>  	i3c_device_disable_ibi(i3cdev);
>  	i3c_device_free_ibi(i3cdev);
>  
>  	for (i = 0; i < priv->dev_info->n_ports; i++) {
>  		if (priv->smbus_port_adapter[i].used) {
>  			cancel_delayed_work_sync(&priv->smbus_port_adapter[i].delayed_work_polling);
>  			i2c_del_adapter(&priv->smbus_port_adapter[i].i2c);
>  		}
>  
>  		if (priv->logical_bus[i].registered)
>  			i3c_master_unregister(&priv->logical_bus[i].controller);
>  	}
>  
>  	cancel_delayed_work_sync(&priv->delayed_work);

[Severity: High]
Is this teardown order safe?

By canceling priv->delayed_work after tearing down the polling works and
logical buses, if priv->delayed_work runs concurrently, it could schedule
new polling works or register logical buses right before the device is
removed, leading to resource leaks or use-after-free conditions.

>  	debugfs_remove_recursive(priv->debug_dir);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525125128.297-1-zain_zhou@realsil.com.cn?part=2