Re: [PATCH v7] media: i2c: Add Omnivision OV02C10 sensor driver

[Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>]

Hi

On Thu, Jan 16, 2025 at 04:22:07PM -0700, Heimir Thor Sverrisson wrote:
> +static int ov02c10_set_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct ov02c10 *ov02c10 = to_ov02c10(sd);
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	int ret = 0;
> +
> +	if (ov02c10->streaming == enable)
> +		return 0;
> +
> +	mutex_lock(&ov02c10->mutex);
> +	if (enable) {
> +		ret = pm_runtime_get_sync(&client->dev);
> +		if (ret < 0) {
> +			pm_runtime_put_noidle(&client->dev);
> +			mutex_unlock(&ov02c10->mutex);
> +			return ret;
> +		}
> +
> +		ret = ov02c10_start_streaming(ov02c10);
> +		if (ret) {
> +			enable = 0;
> +			ov02c10_stop_streaming(ov02c10);
> +			pm_runtime_put(&client->dev);
> +		}
> +	} else {
> +		ov02c10_stop_streaming(ov02c10);
> +		pm_runtime_put(&client->dev);
> +	}
> +
> +	ov02c10->streaming = enable;
> +	mutex_unlock(&ov02c10->mutex);
<snip>
> +static int __maybe_unused ov02c10_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct ov02c10 *ov02c10 = to_ov02c10(sd);
> +	int ret = 0;
> +
> +	mutex_lock(&ov02c10->mutex);
> +	if (!ov02c10->streaming)
> +		goto exit;

I think pm_runtime_get_sync() can call ov02c10_resume() internally so we
can deadlock on mutex_lock(&ov02c10->mutex). We should avoid calling 
pm_runtime_get_sync() with the mutex taken.

It's not practical issue as runtime pm seems to be configured
incorrectly and in any know setup is not needed. So I'm fine 
to leave it as is and eventually work on that in incremental patches
after driver merge.

Regards
Stanislaw