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

Message-ID: <a8dcf9d5-14d6-4765-9fba-ae2ca0bc9dd3@intel.com>
Date: Mon, 13 Jan 2025 13:40:28 -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 1:17 PM, Jacob Keller wrote:
> 
> 
> 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.
> 

Looking through the code, the function is called ultimately from either
a thread function of a threaded IRQ, or from a workqueue thread. I think
those are both safe for spin_lock_irq. I don't believe this function can
ever be called from a path where interrupts were disabled. I don't think
this code can be called from a path which uses spin_lock_irqsave
anyways, since we need to sleep and its hard to imagine a flow where you
want to sleep but also have interrupts disabled...

It would also make sense why we don't see any issues with irqsave in our
tests: interrupts were always enabled so its effectively equivalent to
the non-save variant in this case.

I believe we can respin this to use spin_lock_irq.