linux-kernel - Re: [PATCH v3 3/3] drm/panfrost: Synchronize and disable interrupts before powering off

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: <5cad3d21-5485-49aa-ae97-589dcb831925@arm.com>
Date:   Fri, 1 Dec 2023 12:34:39 +0000
From:   Steven Price <steven.price@....com>
To:     Boris Brezillon <boris.brezillon@...labora.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     robh@...nel.org, maarten.lankhorst@...ux.intel.com,
        mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com,
        daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com,
        m.szyprowski@...sung.com, krzysztof.kozlowski@...aro.org
Subject: Re: [PATCH v3 3/3] drm/panfrost: Synchronize and disable interrupts
 before powering off

On 01/12/2023 11:14, Boris Brezillon wrote:
> On Fri,  1 Dec 2023 11:40:27 +0100
> AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
> wrote:
> 
>> To make sure that we don't unintentionally perform any unclocked and/or
>> unpowered R/W operation on GPU registers, before turning off clocks and
>> regulators we must make sure that no GPU, JOB or MMU ISR execution is
>> pending: doing that required to add a mechanism to synchronize the
> 
>                       ^ requires the addition of a mechanism...
> 
>> interrupts on suspend.
>>
>> Add functions panfrost_{gpu,job,mmu}_suspend_irq() which will perform
>> interrupts masking and ISR execution synchronization, and then call
>> those in the panfrost_device_runtime_suspend() handler in the exact
>> sequence of job (may require mmu!) -> mmu -> gpu.
>>
>> As a side note, JOB and MMU suspend_irq functions needed some special
>> treatment: as their interrupt handlers will unmask interrupts, it was
>> necessary to add a bitmap for `is_suspended` which is used to address
> 
>             to add an `is_suspended` bitmap which is used...
> 
>> the possible corner case of unintentional IRQ unmasking because of ISR
>> execution after a call to synchronize_irq().
> 
> Also fixes the case where the interrupt handler is called when the
> device is suspended because the IRQ line is shared with another device.
> No need to update the commit message for that though.
> 
>>
>> At resume, clear each is_suspended bit in the reset path of JOB/MMU
>> to allow unmasking the interrupts.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
>> ---

<snip>

>>  static void panfrost_job_handle_err(struct panfrost_device *pfdev,
>>  				    struct panfrost_job *job,
>>  				    unsigned int js)
>> @@ -792,9 +802,13 @@ static irqreturn_t panfrost_job_irq_handler_thread(int irq, void *data)
>>  	struct panfrost_device *pfdev = data;
>>  
>>  	panfrost_job_handle_irqs(pfdev);
>> -	job_write(pfdev, JOB_INT_MASK,
>> -		  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>> -		  GENMASK(NUM_JOB_SLOTS - 1, 0));
>> +
>> +	/* Enable interrupts only if we're not about to get suspended */
>> +	if (!test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended))
>> +		job_write(pfdev, JOB_INT_MASK,
>> +			  GENMASK(16 + NUM_JOB_SLOTS - 1, 16) |
>> +			  GENMASK(NUM_JOB_SLOTS - 1, 0));
>> +
> 
> Missing if (test_bit(PANFROST_COMP_BIT_JOB, pfdev->is_suspended)) in
> panfrost_job_irq_handler(), to make sure you don't access the registers
> if the GPU is suspended.

I think generally these IRQ handler functions should simply check the
is_suspended flag and early out if the flag is set. It's not the
re-enabling of the interrupts specifically that we want to gate - it's
any access to the hardware as in the shared-IRQ case the GPU might
already have been powered down/unclocked.

Steve