linux-kernel - Re: [PATCH v4 2/2] media: i2c: Add support for the OV5648 image sensor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 9 Dec 2020 15:05:28 +0100
From:   Paul Kocialkowski <paul.kocialkowski@...tlin.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Maxime Ripard <mripard@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v4 2/2] media: i2c: Add support for the OV5648 image
 sensor

Hi Sakari,

On Wed 09 Dec 20, 01:34, Sakari Ailus wrote:
> Hi Paul,
> 
> On Sat, Nov 28, 2020 at 03:33:50PM +0100, Paul Kocialkowski wrote:
> ...
> > +	if (ret)
> > +		goto error_ctrls;
> > +
> > +	/* V4L2 subdev register */
> > +
> > +	ret = v4l2_async_register_subdev_sensor_common(subdev);
> 
> The driver's device node may be already available to the user here...
> 
> > +	if (ret)
> > +		goto error_ctrls;
> > +
> > +	/* Runtime PM */
> > +
> > +	pm_runtime_enable(sensor->dev);
> > +	pm_runtime_set_suspended(sensor->dev);
> 
> but runtime PM is enabled here.
> 
> This needs to be done in a different order. Otherwise chances are that the
> device node is accessed before the device is powered on.

Oh that makes sense yes. Good catch :)

> > +
> > +	return 0;
> > +
> > +error_ctrls:
> > +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> > +
> > +error_mutex:
> > +	mutex_destroy(&sensor->mutex);
> > +
> > +error_entity:
> > +	media_entity_cleanup(&sensor->subdev.entity);
> > +
> > +error_endpoint:
> > +	v4l2_fwnode_endpoint_free(&sensor->endpoint);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ov5648_remove(struct i2c_client *client)
> > +{
> > +	struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > +	struct ov5648_sensor *sensor = ov5648_subdev_sensor(subdev);
> > +
> > +	clk_rate_exclusive_put(sensor->xvclk);
> > +
> > +	v4l2_async_unregister_subdev(subdev);
> > +	mutex_destroy(&sensor->mutex);
> > +	media_entity_cleanup(&subdev->entity);
> > +	v4l2_device_unregister_subdev(subdev);
> > +	pm_runtime_disable(sensor->dev);
> > +
> > +	ov5648_sensor_power(sensor, false);
> 
> This needs to go, too, as there's no corresponding operation that powered
> on the device.
> 
> Also don't forget to release the control handler.
> 
> I believe these apply to both of the two drivers.

Fair enough and I think runtime PM will have called that already anyways.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)