linux-kernel - Re: [PATCH 3/4] PCI/hotplug: reset_lock is not required synchronizing with irq thread

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: <c4023681-3899-ad3b-82d8-53cc42312970@linux.intel.com>
Date: Fri, 14 Mar 2025 13:18:07 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Lukas Wunner <lukas@...ner.de>
cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, 
    Guenter Roeck <groeck@...iper.net>, 
    Mika Westerberg <mika.westerberg@...ux.intel.com>, 
    "Rafael J. Wysocki" <rafael.j.wysocki@...el.com>, 
    Rajat Jain <rajatxjain@...il.com>, 
    Joel Mathew Thomas <proxy0@...amail.com>, 
    LKML <linux-kernel@...r.kernel.org>, 
    Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH 3/4] PCI/hotplug: reset_lock is not required synchronizing
 with irq thread

On Fri, 14 Mar 2025, Lukas Wunner wrote:

> On Thu, Mar 13, 2025 at 04:23:32PM +0200, Ilpo Järvinen wrote:
> > Disabling HPIE (Hot-Plug Interrupt Enable) and synchronizing with irq
> > handling in pciehp_reset_slot() is enough to ensure no pending events
> > are processed during the slot reset. Thus, there is no need to take
> > reset_lock in the IRQ thread.
> [...]
> > --- a/drivers/pci/hotplug/pciehp_hpc.c
> > +++ b/drivers/pci/hotplug/pciehp_hpc.c
> > @@ -748,12 +748,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
> >  	 * Disable requests have higher priority than Presence Detect Changed
> >  	 * or Data Link Layer State Changed events.
> >  	 */
> > -	down_read_nested(&ctrl->reset_lock, ctrl->depth);
> >  	if (events & DISABLE_SLOT)
> >  		pciehp_handle_disable_request(ctrl);
> >  	else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC))
> >  		pciehp_handle_presence_or_link_change(ctrl, events);
> > -	up_read(&ctrl->reset_lock);
> >  
> >  	ret = IRQ_HANDLED;
> >  out:
> 
> The release and re-acquisition of reset_lock in
> pciehp_configure_device() and pciehp_unconfigure_device()
> needs to be removed as well if the above hunk is applied.

Ah, right. I also now removed reset_lock from 
pciehp_ignore_dpc_link_change() that is directly called from pciehp_ist().

This leaves reset_lock only into pciehp_free_irq(), pciehp_reset_slot(), 
and pciehp_check_presence(). It seems to me pciehp_check_presence() 
requires keeping reset_lock as is. The former two could have synchronized 
with a mutex and shorter critical sections in pciehp_reset_slot() but it 
doesn't look worth the effort (IMO) to divide reset_lock into two to 
realize that.

> But please wait a little while before respinning so that I can
> think through the whole series.

Sure, take your time. :-)

-- 
 i.