Re: [PATCH v3 1/3] media: i2c: cvs: Add driver of Intel Computer Vision Sensing Controller(CVS)

["Vadillo, Miguel" <miguel.vadillo@intel.com>]

Hi Mehdi,

Thanks for reviewing.

On 5/22/26 7:41 AM, Mehdi Djait wrote:
> Hi Miguel,
> 
> Thank you for the patch!
> 
> A couple of comments below.
> 
> On Thu, May 21, 2026 at 03:23:57PM -0700, Miguel Vadillo wrote:
> 
> [..]
> 
>> +
>> +/* I2C transport helpers */
>> +
>> +/**
>> + * cvs_read_i2c - Issue a read-type command and fetch device response
>> + * @ctx: CVS device context
>> + * @cmd_id: Command identifier (big endian)
>> + * @resp: Destination buffer for response payload
>> + * @size: Size of payload to read into @resp (without prefix)
>> + *
>> + * Sends @cmd_id and reads back the response in a single I2C transaction.
>> + * When the device prepends a 4-byte protocol prefix, the combined
>> + * prefix+payload is read into a temporary buffer and only the payload is
>> + * copied to @resp, avoiding any dependency on the layout of the caller's
>> + * buffer.
>> + *
>> + * Return: 0 on success or negative errno.
>> + */
>> +static int cvs_read_i2c(struct icvs *ctx, __be16 cmd_id, void *resp,
>> +			size_t size)
>> +{
>> +	size_t prefix_size = ctx->prefix ? sizeof(u32) : 0;
>> +	size_t read_size = size + prefix_size;
>> +	struct i2c_client *i2c = ctx->i2c_client;
>> +	u8 *buf;
> 
> use this instead:
> u8 *buf __free(kfree) = NULL;
> 
> and then remove the two calls to kfree() below.

This makes sense, ack.
> 
>> +	int cnt;
>> +
>> +	if (!resp || !size)
>> +		return -EINVAL;
>> +
>> +	cnt = i2c_master_send(i2c, (const char *)&cmd_id, sizeof(cmd_id));
>> +	if (cnt != sizeof(cmd_id))
>> +		return cnt < 0 ? cnt : -EIO;
>> +
>> +	buf = kmalloc(read_size, GFP_KERNEL);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	cnt = i2c_master_recv(i2c, buf, read_size);
>> +	if (cnt != read_size) {
>> +		dev_dbg(cvs_dev(ctx), "recv cmd 0x%04x short read (%d/%zu)\n",
>> +			be16_to_cpu(cmd_id), cnt, read_size);
>> +		kfree(buf);
>> +		return cnt < 0 ? cnt : -EIO;
>> +	}
>> +
>> +	memcpy(resp, buf + prefix_size, size);
>> +	kfree(buf);
>> +
>> +	return 0;
>> +}
> 
> [..]
> 
>> +/**
>> + * cvs_send - Common command submission path
>> + * @ctx: CVS device context
>> + * @cmd: Command buffer (icvs_cmd) with cmd_id and param populated
>> + * @len: Buffer length
>> + *
>> + * Dispatches a set of supported commands:
>> + * - ICVS_SET_DEV_HOST_ID,
>> + * - ICVS_HOST_SENSOR_OWNER,
>> + * - ICVS_HOST_SET_MIPI_CONFIG
>> + * - ICVS_FW_LOADER_*
>> + *
>> + * For I2C based commands it sets big-endian cmd ids, writes to the device
>> + * and waits (via delayed work) for completion or timeout.
>> + * GPIO based ownership toggles are handled locally.
>> + *
>> + * Caller must hold ctx->lock when invoking this function and check for i2c
>> + * bus availability.
>> + *
>> + * Return: 0 on success, negative errno, -EINVAL for unsupported command
>> + * or status from device in ctx->wq_resp.
>> + */
>> +int cvs_send(struct icvs *ctx, struct icvs_cmd *cmd, size_t len)
>> +{
>> +	int ret, status = 0;
>> +
>> +	dev_dbg(cvs_dev(ctx), "send cmd = 0x%04x", be16_to_cpu(cmd->cmd_id));
> 
>  From where I see this function being called, it needs to hold the
> ctx->lock. It is also mentioned in the documentation of function
> 
> Let's add a lockdep_assert_held() to make sure of it at the beginning of
> cvs_send()

Agree and ack'ed

> 
>> +
>> +	reinit_completion(&ctx->cmd_completion);
>> +
>> +	switch (be16_to_cpu(cmd->cmd_id)) {
>> +	case ICVS_SET_DEV_HOST_ID:
>> +		cmd->cmd_id = cpu_to_be16(ICVS_SET_DEV_HOST_ID);
>> +		ret = cvs_write_i2c(ctx, cmd, len);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
>> +					    CMD_TIMEOUT);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		status = ctx->wq_resp.resp.state &
>> +			  ICVS_DEV_STATE_ERROR ? -EINVAL : 0;
>> +		break;
>> +	case ICVS_HOST_SENSOR_OWNER:
>> +		gpiod_set_value_cansleep(ctx->req, cmd->param.param);
>> +		fsleep(FW_READY_DELAY_MS * USEC_PER_MSEC);
>> +		ret = gpiod_get_value_cansleep(ctx->resp);
>> +		status = cmd->param.param == ret ? 0 : -EINVAL;
>> +		ret = 0; /* success */
>> +		break;
>> +	case ICVS_HOST_SET_MIPI_CONFIG:
>> +		cmd->cmd_id = cpu_to_be16(ICVS_HOST_SET_MIPI_CONFIG);
>> +		ret = cvs_config_mipi(ctx, cmd, len);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
>> +					    CMD_TIMEOUT);
>> +		status = (ctx->wq_resp.resp.state &
>> +			  ICVS_DEV_STATE_ERROR) ? -EINVAL : 0;
>> +		break;
>> +	case ICVS_FW_LOADER_START:
>> +		cmd->cmd_id = cpu_to_be16(ICVS_FW_LOADER_START);
>> +		ret = cvs_write_i2c(ctx, cmd, len);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		ret = cvs_wait_wake_or_sleep(ctx, CMD_TIMEOUT,
>> +					     FW_READY_DELAY_MS);
>> +		if (ret)
>> +			break;
>> +
>> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
>> +					    CMD_TIMEOUT);
>> +		status = (ctx->wq_resp.resp.state &
>> +			  ICVS_DEV_STATE_DOWNLOAD) ? 0 : -EINVAL;
>> +		break;
>> +	case ICVS_FW_LOADER_DATA:
>> +		/* Quirk for older protocols */
>> +		if (ctx->caps.protocol_version_major >= 2 &&
>> +		    ctx->caps.protocol_version_minor >= 2) {
>> +			cmd->cmd_id = cpu_to_be16(ICVS_FW_LOADER_DATA);
>> +			ret = cvs_write_i2c(ctx, cmd, len);
>> +		} else {
>> +			ret = cvs_write_i2c(ctx, &cmd->param,
>> +					    len - sizeof(cmd->cmd_id));
>> +		}
>> +
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = cvs_wait_wake_or_sleep(ctx, FW_READY_DELAY_MS,
>> +					     FW_READY_DELAY_MS);
>> +		if (ret)
>> +			break;
>> +
>> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
>> +					    CMD_TIMEOUT);
>> +		status = ctx->wq_resp.resp.state &
>> +			  ICVS_DEV_STATE_ERROR ? -EINVAL : 0;
>> +		break;
>> +	case ICVS_FW_LOADER_END:
>> +		cmd->cmd_id = cpu_to_be16(ICVS_FW_LOADER_END);
>> +		ret = cvs_write_i2c(ctx, cmd, len);
>> +		if (ret < 0)
>> +			break;
>> +
>> +		ret = cvs_wait_wake_or_sleep(ctx, CMD_TIMEOUT,
>> +					     FW_READY_DELAY_MS);
>> +		if (ret)
>> +			break;
>> +
>> +		ret = cvs_schedule_and_wait(ctx, FW_READY_DELAY_MS,
>> +					    CMD_TIMEOUT);
>> +		status = !(ctx->wq_resp.resp.state &
>> +			   ICVS_DEV_STATE_DOWNLOAD) ? 0 : -EINVAL;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return ctx->wq_resp.status = status;
>> +}
> 
> [..]
> 
>> + * cvs_core_remove - Shared remove logic
>> + * @dev: Device
>> + */
>> +static void cvs_core_remove(struct device *dev)
>> +{
>> +	struct icvs *ctx = dev_get_drvdata(dev);
>> +
>> +	cvs_csi_remove(ctx);
> 
> Shouldn't we cancel ctx work first ? And then do the CSI2 v4l2 cleanup ?

Good catch I think this was leaving a window for race condition, ack'ed

--
regards,
Miguel

> 
>> +	cancel_delayed_work_sync(&ctx->work);
>> +
>> +	if (ctx->ipu_link)
>> +		device_link_del(ctx->ipu_link);
>> +
>> +	pm_runtime_put_noidle(dev);
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_set_suspended(dev);
>> +
>> +	cvs_reset(ctx);
>> +}
>> +
>> +/**
>> + * cvs_remove - I2C driver remove
>> + * @client: I2C client
>> + */
>> +static void cvs_remove(struct i2c_client *client)
>> +{
>> +	cvs_core_remove(&client->dev);
> 
> [..]
> 
> --
> Kind Regards
> Mehdi Djait