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
