linux-kernel - Re: [PATCH 09/10] drm/tidss: IRQ code cleanup

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: <20231101145243.GY12764@pendragon.ideasonboard.com>
Date:   Wed, 1 Nov 2023 16:52:43 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Cc:     Aradhya Bhatia <a-bhatia1@...com>,
        Devarsh Thakkar <devarsht@...com>,
        Jyri Sarha <jyri.sarha@....fi>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/10] drm/tidss: IRQ code cleanup

Hi Tomi,

Thank you for the patch.

On Wed, Nov 01, 2023 at 11:17:46AM +0200, Tomi Valkeinen wrote:
> The IRQ setup code is overly complex. All we really need to do is
> initialize the related fields in struct tidss_device, and request the
> IRQ.
> 
> We can drop all the HW accesses, as they are pointless: the driver will
> set the IRQs correctly when it needs any of the IRQs, and at probe time
> we have done a reset, so we know that all the IRQs are masked by default
> in the hardware.

Even for K2G ?

> Thus we can combine the tidss_irq_preinstall() and
> tidss_irq_postinstall() into the tidss_irq_install() function, drop the
> HW accesses, and drop the use of spinlock, as this is done at init time
> and there can be no races.
> 
> We can also drop the HW access from the tidss_irq_uninstall(), as the
> driver will anyway disable and suspend the hardware at remove time.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> ---
>  drivers/gpu/drm/tidss/tidss_drv.c |  2 ++
>  drivers/gpu/drm/tidss/tidss_irq.c | 54 ++++++---------------------------------
>  2 files changed, 10 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c
> index 64914331715a..37693f30d98b 100644
> --- a/drivers/gpu/drm/tidss/tidss_drv.c
> +++ b/drivers/gpu/drm/tidss/tidss_drv.c
> @@ -138,6 +138,8 @@ static int tidss_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, tidss);
>  
> +	spin_lock_init(&tidss->wait_lock);
> +
>  	ret = dispc_init(tidss);
>  	if (ret) {
>  		dev_err(dev, "failed to initialize dispc: %d\n", ret);
> diff --git a/drivers/gpu/drm/tidss/tidss_irq.c b/drivers/gpu/drm/tidss/tidss_irq.c
> index 0c681c7600bc..604334ef526a 100644
> --- a/drivers/gpu/drm/tidss/tidss_irq.c
> +++ b/drivers/gpu/drm/tidss/tidss_irq.c
> @@ -93,33 +93,21 @@ void tidss_irq_resume(struct tidss_device *tidss)
>  	spin_unlock_irqrestore(&tidss->wait_lock, flags);
>  }
>  
> -static void tidss_irq_preinstall(struct drm_device *ddev)
> -{
> -	struct tidss_device *tidss = to_tidss(ddev);
> -
> -	spin_lock_init(&tidss->wait_lock);
> -
> -	tidss_runtime_get(tidss);
> -
> -	dispc_set_irqenable(tidss->dispc, 0);
> -	dispc_read_and_clear_irqstatus(tidss->dispc);
> -
> -	tidss_runtime_put(tidss);
> -}
> -
> -static void tidss_irq_postinstall(struct drm_device *ddev)
> +int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
>  {
>  	struct tidss_device *tidss = to_tidss(ddev);
> -	unsigned long flags;
> -	unsigned int i;
> +	int ret;
>  
> -	tidss_runtime_get(tidss);
> +	if (irq == IRQ_NOTCONNECTED)
> +		return -ENOTCONN;
>  
> -	spin_lock_irqsave(&tidss->wait_lock, flags);
> +	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> +	if (ret)
> +		return ret;
>  
>  	tidss->irq_mask = DSS_IRQ_DEVICE_OCP_ERR;
>  
> -	for (i = 0; i < tidss->num_crtcs; ++i) {
> +	for (unsigned int i = 0; i < tidss->num_crtcs; ++i) {
>  		struct tidss_crtc *tcrtc = to_tidss_crtc(tidss->crtcs[i]);
>  
>  		tidss->irq_mask |= DSS_IRQ_VP_SYNC_LOST(tcrtc->hw_videoport);
> @@ -127,28 +115,6 @@ static void tidss_irq_postinstall(struct drm_device *ddev)
>  		tidss->irq_mask |= DSS_IRQ_VP_FRAME_DONE(tcrtc->hw_videoport);
>  	}
>  
> -	tidss_irq_update(tidss);
> -
> -	spin_unlock_irqrestore(&tidss->wait_lock, flags);
> -
> -	tidss_runtime_put(tidss);
> -}
> -
> -int tidss_irq_install(struct drm_device *ddev, unsigned int irq)
> -{
> -	int ret;
> -
> -	if (irq == IRQ_NOTCONNECTED)
> -		return -ENOTCONN;
> -
> -	tidss_irq_preinstall(ddev);
> -
> -	ret = request_irq(irq, tidss_irq_handler, 0, ddev->driver->name, ddev);
> -	if (ret)
> -		return ret;
> -
> -	tidss_irq_postinstall(ddev);
> -
>  	return 0;
>  }
>  
> @@ -156,9 +122,5 @@ void tidss_irq_uninstall(struct drm_device *ddev)
>  {
>  	struct tidss_device *tidss = to_tidss(ddev);
>  
> -	tidss_runtime_get(tidss);
> -	dispc_set_irqenable(tidss->dispc, 0);
> -	tidss_runtime_put(tidss);
> -
>  	free_irq(tidss->irq, ddev);
>  }
> 

-- 
Regards,

Laurent Pinchart