Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i8771445c:Fastmail
Date:   Fri, 19 Aug 2022 09:29:30 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Danilo Krummrich <dakr@redhat.com>
Cc:     daniel@ffwll.ch, airlied@linux.ie, tzimmermann@suse.de,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH drm-misc-next 3/3] drm/vc4: crtc: protect device
 resources after removal
Message-ID: <20220819072930.fg56dkzbdu6f7s25@houat>
References: <20220819002905.82095-1-dakr@redhat.com>
 <20220819002905.82095-4-dakr@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="v6qavohxtl7q5awx"
Content-Disposition: inline
In-Reply-To: <20220819002905.82095-4-dakr@redhat.com>
Precedence: bulk


--v6qavohxtl7q5awx
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hi,

On Fri, Aug 19, 2022 at 02:29:05AM +0200, Danilo Krummrich wrote:
> (Hardware) resources which are bound to the driver and device lifecycle
> must not be accessed after the device and driver are unbound.
>=20
> However, the DRM device isn't freed as long as the last user closed it,
> hence userspace can still call into the driver.
>=20
> Therefore protect the critical sections which are accessing those
> resources with drm_dev_enter() and drm_dev_exit().
>=20
> Fixes: 7cc4214c27cf ("drm/vc4: crtc: Switch to drmm_kzalloc")
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 41 +++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crt=
c.c
> index 2def6e2ad6f0..51daf190196e 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -39,6 +39,7 @@
>  #include <drm/drm_atomic_uapi.h>
>  #include <drm/drm_fb_dma_helper.h>
>  #include <drm/drm_framebuffer.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
> @@ -295,10 +296,17 @@ struct drm_encoder *vc4_get_crtc_encoder(struct drm=
_crtc *crtc,
>  static void vc4_crtc_pixelvalve_reset(struct drm_crtc *crtc)
>  {
>  	struct vc4_crtc *vc4_crtc =3D to_vc4_crtc(crtc);
> +	struct drm_device *dev =3D crtc->dev;
> +	int idx;
> +
> +	if (!drm_dev_enter(dev, &idx))
> +		return;
> =20
>  	/* The PV needs to be disabled before it can be flushed */
>  	CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) & ~PV_CONTROL_EN);
>  	CRTC_WRITE(PV_CONTROL, CRTC_READ(PV_CONTROL) | PV_CONTROL_FIFO_CLR);
> +
> +	drm_dev_exit(idx);
>  }
> =20
>  static void vc4_crtc_config_pv(struct drm_crtc *crtc, struct drm_encoder=
 *encoder,
> @@ -321,6 +329,10 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc=
, struct drm_encoder *encode
>  	u32 format =3D is_dsi1 ? PV_CONTROL_FORMAT_DSIV_24 : PV_CONTROL_FORMAT_=
24;
>  	u8 ppc =3D pv_data->pixels_per_clock;
>  	bool debug_dump_regs =3D false;
> +	int idx;
> +
> +	if (!drm_dev_enter(dev, &idx))
> +		return;
> =20
>  	if (debug_dump_regs) {
>  		struct drm_printer p =3D drm_info_printer(&vc4_crtc->pdev->dev);
> @@ -410,6 +422,8 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc,=
 struct drm_encoder *encode
>  			 drm_crtc_index(crtc));
>  		drm_print_regset32(&p, &vc4_crtc->regset);
>  	}
> +
> +	drm_dev_exit(idx);
>  }
> =20
>  static void require_hvs_enabled(struct drm_device *dev)
> @@ -430,13 +444,18 @@ static int vc4_crtc_disable(struct drm_crtc *crtc,
>  	struct vc4_crtc *vc4_crtc =3D to_vc4_crtc(crtc);
>  	struct drm_device *dev =3D crtc->dev;
>  	struct vc4_dev *vc4 =3D to_vc4_dev(dev);
> -	int ret;
> +	int idx, ret;
> +
> +	if (!drm_dev_enter(dev, &idx))
> +		return -ENODEV;
> =20
>  	CRTC_WRITE(PV_V_CONTROL,
>  		   CRTC_READ(PV_V_CONTROL) & ~PV_VCONTROL_VIDEN);
>  	ret =3D wait_for(!(CRTC_READ(PV_V_CONTROL) & PV_VCONTROL_VIDEN), 1);
>  	WARN_ONCE(ret, "Timeout waiting for !PV_VCONTROL_VIDEN\n");
> =20
> +	drm_dev_exit(idx);
> +

I think this would be easier to follow if we were protecting the entire
function with our lock.

Having locks taken in the middle of the function is harder to identify
whether or not one particular function is safe or not.

The same applies to the plane patch

Maxime

--v6qavohxtl7q5awx
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCYv872gAKCRDj7w1vZxhR
xS9MAQCa/qo5n5/nskuFs/e0cpI3NV0AjoLytdAPrVVvecr6PwEAuCU6l3kgsan/
a/tIIqMlOAakafUEU6K3H8wEyR1/yAU=
=/eO7
-----END PGP SIGNATURE-----

--v6qavohxtl7q5awx--