linux-kernel - Re: [PATCH net 2/2] net: ipa: re-enable NAPI before enabling interrupt

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] [day] [month] [year] [list]

Message-ID: <49105454-8f5e-6fa6-bdef-fa68c4510d62@linaro.org>
Date:   Fri, 8 Jan 2021 14:16:18 -0600
From:   Alex Elder <elder@...aro.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, evgreen@...omium.org,
        bjorn.andersson@...aro.org, cpratapa@...eaurora.org,
        subashab@...eaurora.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 2/2] net: ipa: re-enable NAPI before enabling
 interrupt

On 1/7/21 8:38 PM, Jakub Kicinski wrote:
> On Thu,  7 Jan 2021 15:43:25 -0600 Alex Elder wrote:
>> @@ -743,21 +743,21 @@ static void gsi_channel_freeze(struct gsi_channel *channel)
>>   	set_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags);
>>   	smp_mb__after_atomic();	/* Ensure gsi_channel_poll() sees new value */
>>   
>> -	napi_disable(&channel->napi);
>> -
>>   	gsi_irq_ieob_disable(channel->gsi, channel->evt_ring_id);
>> +
>> +	napi_disable(&channel->napi);
>>   }
> 
> So patch 1 is entirely for the purpose of keeping the code symmetric
> here? I can't think of other reason why masking this IRQ couldn't be
> left after NAPI is disabled, and that should work as you expect.

No, that is not the purpose of the first patch.

But regardless, I'm really glad you pushed back on this
because it made me step back and re-evaluate in a different
way what was happening during suspend.  Your earlier response
(about what happens during napi_disable()) also helped me to
see there's probably something *else* wrong with how the
driver is stopping channels.

I was going to go into more detail here but for now
let me just rescind this series.  I will be reworking
the channel stop/suspend logic and will send that work
out when it's tested and ready.

Thanks.

					-Alex

>>   /* Allow transactions to be used on the channel again. */
>>   static void gsi_channel_thaw(struct gsi_channel *channel)
>>   {
>> -	gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id);
>> -
>>   	/* Allow the NAPI poll loop to re-enable interrupts again */
>>   	clear_bit(GSI_CHANNEL_FLAG_STOPPING, channel->flags);
>>   	smp_mb__after_atomic();	/* Ensure gsi_channel_poll() sees new value */
>>   
>>   	napi_enable(&channel->napi);
>> +
>> +	gsi_irq_ieob_enable(channel->gsi, channel->evt_ring_id);
>>   }