linux-kernel - Re: [PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier

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]

Message-ID: <a9e436f8-34ec-123a-9c69-efabb4576bea@samsung.com>
Date:   Mon, 16 Apr 2018 11:57:15 +0200
From:   Andrzej Hajda <a.hajda@...sung.com>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        architt@...eaurora.org, inki.dae@...sung.com,
        thierry.reding@...il.com, hjc@...k-chips.com,
        seanpaul@...omium.org, airlied@...ux.ie, tfiga@...omium.org,
        heiko@...ech.de
Cc:     dri-devel@...ts.freedesktop.org, dianders@...omium.org,
        ykk@...k-chips.com, kernel@...labora.com, m.szyprowski@...sung.com,
        linux-samsung-soc@...r.kernel.org, jy0922.shim@...sung.com,
        rydberg@...math.org, krzk@...nel.org,
        linux-rockchip@...ts.infradead.org, kgene@...nel.org,
        linux-input@...r.kernel.org, orjan.eide@....com,
        wxt@...k-chips.com, jeffy.chen@...k-chips.com,
        linux-arm-kernel@...ts.infradead.org, mark.yao@...k-chips.com,
        wzz@...k-chips.com, hl@...k-chips.com, jingoohan1@...il.com,
        sw0312.kim@...sung.com, linux-kernel@...r.kernel.org,
        kyungmin.park@...sung.com, Laurent.pinchart@...asonboard.com,
        kuankuan.y@...il.com, hshi@...omium.org
Subject: Re: [PATCH v6 28/30] drm/rockchip: Disable PSR from reboot notifier

On 05.04.2018 11:49, Enric Balletbo i Serra wrote:
> From: Tomasz Figa <tfiga@...omium.org>
>
> It looks like the driver subsystem detaches devices from power domains
> at shutdown without consent of the drivers.

It looks bit strange. Could you elaborate more on it. Could you show the
code performing the detach?


Regards
Andrzej


>  This means that we might have
> our power domain turned off behind our back and the only way to avoid
> problems is to stop doing any hardware programming at some point before
> the power is cut. A reboot notifier, despite being a misnomer and
> handling shutdowns as well, is a good place to do it.
> Signed-off-by: Tomasz Figa <tfiga@...omium.org>
> Signed-off-by: Thierry Escande <thierry.escande@...labora.com>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> Tested-by: Marek Szyprowski <m.szyprowski@...sung.com>
> ---
>
>  drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> index e7e16d92d5a1..1bf5cba9a64d 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/input.h>
> +#include <linux/reboot.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -33,6 +34,7 @@ struct psr_drv {
>  	struct delayed_work	flush_work;
>  	struct work_struct	disable_work;
>  
> +	struct notifier_block	reboot_nb;
>  	struct input_handler    input_handler;
>  
>  	int (*set)(struct drm_encoder *encoder, bool enable);
> @@ -309,6 +311,24 @@ static const struct input_device_id psr_ids[] = {
>  	{ },
>  };
>  
> +static int rockchip_drm_psr_reboot_notifier(struct notifier_block *nb,
> +					    unsigned long action, void *data)
> +{
> +	struct psr_drv *psr = container_of(nb, struct psr_drv, reboot_nb);
> +
> +	/*
> +	 * It looks like the driver subsystem detaches devices from power
> +	 * domains at shutdown without consent of the drivers. This means
> +	 * that we might have our power domain turned off behind our back
> +	 * and the only way to avoid problems is to stop doing any hardware
> +	 * programming after this point, which is achieved by the unbalanced
> +	 * call below.
> +	 */
> +	rockchip_drm_psr_inhibit_get(psr->encoder);
> +
> +	return 0;
> +}
> +
>  /**
>   * rockchip_drm_psr_register - register encoder to psr driver
>   * @encoder: encoder that obtain the PSR function
> @@ -361,6 +381,9 @@ int rockchip_drm_psr_register(struct drm_encoder *encoder,
>  	if (error)
>  		goto err1;
>  
> +	psr->reboot_nb.notifier_call = rockchip_drm_psr_reboot_notifier;
> +	register_reboot_notifier(&psr->reboot_nb);
> +
>  	mutex_lock(&drm_drv->psr_list_lock);
>  	list_add_tail(&psr->list, &drm_drv->psr_list);
>  	mutex_unlock(&drm_drv->psr_list_lock);
> @@ -403,6 +426,7 @@ void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
>  			WARN_ON(psr->inhibit_count != 1);
>  
>  			list_del(&psr->list);
> +			unregister_reboot_notifier(&psr->reboot_nb);
>  			input_unregister_handler(&psr->input_handler);
>  			kfree(psr->input_handler.name);
>  			kfree(psr);