netdev - Re: [PATCH net-next 12/13] ice: implement low latency PHY timer updates

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: <5ee4a134-009b-4ba0-a25c-f5f8a955da22@intel.com>
Date: Mon, 13 Jan 2025 13:17:46 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Michal Schmidt <mschmidt@...hat.com>
CC: Jakub Kicinski <kuba@...nel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, <davem@...emloft.net>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <andrew+netdev@...n.ch>, <netdev@...r.kernel.org>,
	<anton.nadezhdin@...el.com>, <przemyslaw.kitszel@...el.com>,
	<milena.olech@...el.com>, <arkadiusz.kubalewski@...el.com>,
	<richardcochran@...il.com>, Karol Kolacinski <karol.kolacinski@...el.com>,
	Rinitha S <sx.rinitha@...el.com>
Subject: Re: [PATCH net-next 12/13] ice: implement low latency PHY timer
 updates



On 1/13/2025 12:46 PM, Michal Schmidt wrote:
> On Mon, Jan 13, 2025 at 7:51 PM Jacob Keller <jacob.e.keller@...el.com> wrote:
>> On 1/9/2025 6:18 PM, Jakub Kicinski wrote:
>>> On Wed,  8 Jan 2025 14:17:49 -0800 Tony Nguyen wrote:
>>>> +    spin_lock_irqsave(&params->atqbal_wq.lock, flags);
>>>> +
>>>> +    /* Wait for any pending in-progress low latency interrupt */
>>>> +    err = wait_event_interruptible_locked_irq(params->atqbal_wq,
>>>
>>> Don't you need an irqsave() flavor of
>>> wait_event_interruptible_locked_irq() for this to work correctly? 🤔️
>>
>> My understanding was that spin_lock_irqsave saves the IRQ state, where
>> as spin_lock_irq doesn't save the state and assumes the interrupts
>> should always be enabled.
>>
>> In this case, we lock with irqsave, keeping track of the interrupt state
>> before, then wait_event_interruptible_locked_irq would enable interrupts
>> when it unlocks to sleep.. Hm
> 
> Do you even need spin_lock_irqsave() here? It seems to me that all the
> functions where you're adding the
> wait_event_interruptible_locked_irq() calls are always entered with
> interrupts enabled, so it should be safe to just use spin_lock_irq().
> 
> Michal
> 

Thats a good point actually, and would be much simpler than adding the
irqsave variation wait_event.

>> So this code will correctly restore the interrupt state at the end after
>> we call spin_unlock_irqrestore, but there is a window within the
>> wait_event_interruptible_locked_irq where interrupts will be enabled
>> when they potentially shouldn't be..
>