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

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

Hi
 
On Wed, Jan 22, 2025 at 10:36:27AM +0100, Stanislaw Gruszka wrote:
> 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.

Actually I was wrong, code is fine. ov02c10_resume() is used
by system resume, for runtime pm resume ov02c10_power_on() is used
so no deadlock issue at all.

Regards
Stanislaw