linux-kernel - RE: [RFT PATCH v2 11/12] drm/radeon: Call drm_helper_force_disable

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Sep 2023 15:49:01 +0000
From:   "Deucher, Alexander" <Alexander.Deucher@....com>
To:     Douglas Anderson <dianders@...omium.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        Maxime Ripard <mripard@...nel.org>
CC:     "Pan, Xinhui" <Xinhui.Pan@....com>,
        "airlied@...il.com" <airlied@...il.com>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [RFT PATCH v2 11/12] drm/radeon: Call
 drm_helper_force_disable_all() at shutdown/remove time

[Public]

> -----Original Message-----
> From: Douglas Anderson <dianders@...omium.org>
> Sent: Thursday, September 21, 2023 3:27 PM
> To: dri-devel@...ts.freedesktop.org; Maxime Ripard <mripard@...nel.org>
> Cc: Douglas Anderson <dianders@...omium.org>; Pan, Xinhui
> <Xinhui.Pan@....com>; airlied@...il.com; Deucher, Alexander
> <Alexander.Deucher@....com>; amd-gfx@...ts.freedesktop.org; Koenig,
> Christian <Christian.Koenig@....com>; daniel@...ll.ch; linux-
> kernel@...r.kernel.org
> Subject: [RFT PATCH v2 11/12] drm/radeon: Call
> drm_helper_force_disable_all() at shutdown/remove time
>
> Based on grepping through the source code, this driver appears to be missing
> a call to drm_atomic_helper_shutdown(), or in this case the non-atomic
> equivalent drm_helper_force_disable_all(), at system shutdown time and at
> driver remove time. This is important because
> drm_helper_force_disable_all() will cause panels to get disabled cleanly which
> may be important for their power sequencing. Future changes will remove any
> custom powering off in individual panel drivers so the DRM drivers need to
> start getting this right.
>
> The fact that we should call drm_atomic_helper_shutdown(), or in this case
> the non-atomic equivalent drm_helper_force_disable_all(), in the case of OS
> shutdown/restart comes straight out of the kernel doc "driver instance
> overview" in drm_drv.c.
>
> NOTE: in order to get things inserted in the right place, I had to replace the
> old/deprecated drm_put_dev() function with the equivalent new calls.
>
> Suggested-by: Maxime Ripard <mripard@...nel.org>
> Reviewed-by: Maxime Ripard <mripard@...nel.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> I honestly have no idea if I got this patch right. The shutdown() function
> already had some special case logic for PPC, Loongson, and VMs and I don't
> 100% for sure know how this interacts with those. Everything here is just
> compile tested.

I think the reason for most of this funniness is to reduce shutdown time.  Lots of users complain if driver takes a while to shutdown and there is a point to be made that if the system is going into power down, there is not much reason to spend a lot of time messing with the hardware.

Alex

>
> (no changes since v1)
>
>  drivers/gpu/drm/radeon/radeon_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
> b/drivers/gpu/drm/radeon/radeon_drv.c
> index 39cdede460b5..67995ea24852 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -38,6 +38,7 @@
>  #include <linux/pci.h>
>
>  #include <drm/drm_aperture.h>
> +#include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> @@ -357,7 +358,9 @@ radeon_pci_remove(struct pci_dev *pdev)  {
>       struct drm_device *dev = pci_get_drvdata(pdev);
>
> -     drm_put_dev(dev);
> +     drm_dev_unregister(dev);
> +     drm_helper_force_disable_all(dev);
> +     drm_dev_put(dev);
>  }
>
>  static void
> @@ -368,6 +371,8 @@ radeon_pci_shutdown(struct pci_dev *pdev)
>        */
>       if (radeon_device_is_virtual())
>               radeon_pci_remove(pdev);
> +     else
> +             drm_helper_force_disable_all(pci_get_drvdata(pdev));
>
>  #if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
>       /*
> --
> 2.42.0.515.g380fc7ccd1-goog