linux-kernel - Re: [PATCH 1/2] drm/tidss: Clear the interrupt status for interrupts being disabled

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: <3d85ac05-150b-4917-a374-5974d376e416@ideasonboard.com>
Date: Mon, 21 Oct 2024 14:15:01 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Devarsh Thakkar <devarsht@...com>
Cc: jyri.sarha@....fi, airlied@...il.com, maarten.lankhorst@...ux.intel.com,
 mripard@...nel.org, tzimmermann@...e.de, dri-devel@...ts.freedesktop.org,
 simona@...ll.ch, linux-kernel@...r.kernel.org, praneeth@...com,
 vigneshr@...com, aradhya.bhatia@...ux.dev, s-jain1@...com,
 r-donadkar@...com, sam@...nborg.org, bparrot@...com,
 jcormier@...ticallink.com
Subject: Re: [PATCH 1/2] drm/tidss: Clear the interrupt status for interrupts
 being disabled

Hi,

On 12/10/2024 18:07, Devarsh Thakkar wrote:
> It is possible that dispc_{k2g/k3}_set_irqenable can be called for
> disabling some interrupt events which were previously enabled. However
> instead of clearing any pending events for the interrupt events that are
> required to be disabled, it was instead clearing the new interrupt events
> which were not even enabled.

That's on purpose. When we enable a new interrupt, we want to first 
clear the irqstatus for that interrupt to make sure there's no old 
status left lying around. If I'm not mistaken, enabling an interrupt 
with an irqstatus bit set will immediately trigger the interrupt.

> For e.g. While disabling the vsync events, dispc_k3_set_irqenable tries to
> clear DSS_IRQ_DEVICE_OCP_ERR which was not enabled per the old_mask at all
> as shown below :
> 
> "dispc_k3_set_irqenable : irqenabled - mask = 91, old = f0, clr = 1" where
> clr = (mask ^ old_mask) & old_mask

That's a bit odd... Why was the DSS_IRQ_DEVICE_OCP_ERR not already 
enabled? It is enabled in the tidss_irq_install().

Or maybe it had been enabled by the driver, but as the HW doesn't 
support that bit, it reads always as 0. I have an unsent patch to drop 
DSS_IRQ_DEVICE_OCP_ERR.

> This corrects the bit mask to make sure that it always clears any pending
> interrupt events that are requested to be disabled before disabling them
> actually.

I think the point here makes sense: if we disable interrupts with 
dispc_set_irqenable(), we don't want to see interrupt handling for the 
disabled interrupts after the call.

However, if you clear the irqstatus for an interrupt that will be 
disabled, but clear it _before_ disabling the interrupt, the interrupt 
might trigger right after clearing the irqstatus but before disabling it.

  Tomi

> Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> Reported-by: Jonathan Cormier <jcormier@...ticallink.com>
> Signed-off-by: Devarsh Thakkar <devarsht@...com>
> ---
>   drivers/gpu/drm/tidss/tidss_dispc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
> index 1ad711f8d2a8..b04419b24863 100644
> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
> @@ -700,8 +700,8 @@ void dispc_k2g_set_irqenable(struct dispc_device *dispc, dispc_irq_t mask)
>   {
>   	dispc_irq_t old_mask = dispc_k2g_read_irqenable(dispc);
>   
> -	/* clear the irqstatus for newly enabled irqs */
> -	dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & mask);
> +	/* clear the irqstatus for irqs that are being disabled now */
> +	dispc_k2g_clear_irqstatus(dispc, (mask ^ old_mask) & old_mask);
>   
>   	dispc_k2g_vp_set_irqenable(dispc, 0, mask);
>   	dispc_k2g_vid_set_irqenable(dispc, 0, mask);
> @@ -843,8 +843,8 @@ static void dispc_k3_set_irqenable(struct dispc_device *dispc,
>   
>   	old_mask = dispc_k3_read_irqenable(dispc);
>   
> -	/* clear the irqstatus for newly enabled irqs */
> -	dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & mask);
> +	/* clear the irqstatus for irqs that are being disabled now */
> +	dispc_k3_clear_irqstatus(dispc, (old_mask ^ mask) & old_mask);
>   
>   	for (i = 0; i < dispc->feat->num_vps; ++i) {
>   		dispc_k3_vp_set_irqenable(dispc, i, mask);