Received-SPF: pass (google.com: domain of linux-kernel+bounces-154088-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <958c27b1-26d7-4927-976b-4502f33f31f7@gmail.com>
Date: Mon, 22 Apr 2024 15:26:16 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] irqchip/irq-brcmstb-l2: Avoid saving mask on shutdown
To: Thomas Gleixner <tglx@linutronix.de>,
 Florian Fainelli <florian.fainelli@broadcom.com>,
 linux-kernel@vger.kernel.org
Cc: opendmb@gmail.com, Tim Ross <tim.ross@broadcom.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@broadcom.com>,
 "open list:BROADCOM BMIPS MIPS ARCHITECTURE" <linux-mips@vger.kernel.org>,
 "moderated list:BROADCOM BCM7XXX ARM ARCHITECTURE"
 <linux-arm-kernel@lists.infradead.org>
References: <20240416194343.469318-1-florian.fainelli@broadcom.com>
 <87le55ulw5.ffs@tglx>
Content-Language: en-US
From: Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <87le55ulw5.ffs@tglx>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 4/22/24 14:29, Thomas Gleixner wrote:
> On Tue, Apr 16 2024 at 12:43, Florian Fainelli wrote:
>> The interrupt controller shutdown path does not need to save the mask of
>> enabled interrupts because the next state the system is going to be in
>> is akin to a cold boot, or a kexec'd kernel.
> 
> Sure, but
> 
>> Reported-by: Tim Ross <tim.ross@broadcom.com>
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   drivers/irqchip/irq-brcmstb-l2.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-brcmstb-l2.c b/drivers/irqchip/irq-brcmstb-l2.c
>> index 2b0b3175cea0..c988886917f7 100644
>> --- a/drivers/irqchip/irq-brcmstb-l2.c
>> +++ b/drivers/irqchip/irq-brcmstb-l2.c
>> @@ -118,7 +118,7 @@ static void brcmstb_l2_intc_irq_handle(struct irq_desc *desc)
>>   	chained_irq_exit(chip, desc);
>>   }
>>   
>> -static void brcmstb_l2_intc_suspend(struct irq_data *d)
>> +static void __brcmstb_l2_intc_suspend(struct irq_data *d, bool save)
>>   {
>>   	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>>   	struct irq_chip_type *ct = irq_data_get_chip_type(d);
>> @@ -127,7 +127,8 @@ static void brcmstb_l2_intc_suspend(struct irq_data *d)
>>   
>>   	irq_gc_lock_irqsave(gc, flags);
>>   	/* Save the current mask */
>> -	b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
>> +	if (save)
>> +		b->saved_mask = irq_reg_readl(gc, ct->regs.mask);
> 
> what's the conditional actually buying you except more complex code?

Not much this is an optimization that is simple to carry out. There can 
be dozens of such L2 interrupt controllers in a given system and the 
MMIO accesses start adding up eventually.
-- 
Florian