linux-kernel - Re: [PATCH v3 2/4] PCI/portdrv: Add necessary wait for disabling hotplug events

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: <Z70zyhZe6OrxNNT3@U-2FWC9VHC-2323.local>
Date: Tue, 25 Feb 2025 11:06:50 +0800
From: Feng Tang <feng.tang@...ux.alibaba.com>
To: Lukas Wunner <lukas@...ner.de>, rafael@...nel.org
Cc: Bjorn Helgaas <bhelgaas@...gle.com>,
	Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@...ux.intel.com>,
	Liguang Zhang <zhangliguang@...ux.alibaba.com>,
	Guanghui Feng <guanghuifeng@...ux.alibaba.com>, rafael@...nel.org,
	Markus Elfring <Markus.Elfring@....de>, lkp@...el.com,
	Jonathan Cameron <Jonathan.Cameron@...wei.com>,
	ilpo.jarvinen@...ux.intel.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/4] PCI/portdrv: Add necessary wait for disabling
 hotplug events

Hi Lukas,

On Mon, Feb 24, 2025 at 07:12:11PM +0100, Lukas Wunner wrote:
> On Mon, Feb 24, 2025 at 11:44:58AM +0800, Feng Tang wrote:
> > @@ -255,8 +271,7 @@ static int get_port_device_capability(struct pci_dev *dev)
> >  		 * Disable hot-plug interrupts in case they have been enabled
> >  		 * by the BIOS and the hot-plug service driver is not loaded.
> >  		 */
> > -		pcie_capability_clear_word(dev, PCI_EXP_SLTCTL,
> > -			  PCI_EXP_SLTCTL_CCIE | PCI_EXP_SLTCTL_HPIE);
> > +		pcie_disable_hp_interrupts_early(dev);
> >  	}
> 
> Moving the Slot Control code from pciehp to portdrv (as is done in
> patch 1 of this series) is hackish.  It should be avoided if at all
> possible.

I tried to remove the code duplication of 2 waiting function, according
to Bjorn's comment in https://lore.kernel.org/lkml/20250218223354.GA196886@bhelgaas/.
Maybe I didn't git it right. Any suggestion?

> 
> As I've already said before...
> 
> https://lore.kernel.org/all/Z6HYuBDP6uvE1Sf4@wunner.de/
> 
> ...it seems clearing those interrupts is only necessary
> in the CONFIG_HOTPLUG_PCI_PCIE=n case.  And in that case,
> there is no second Slot Control write, so there is no delay
> to be observed.
> 
> Hence the proper solution is to make the clearing of the
> interrupts conditional on: !IS_ENABLED(CONFIG_HOTPLUG_PCI_PCIE)
> 
> You keep sending new versions of these patches that do not
> incorporate the feedback I provided in the above-linked e-mail.
> 
> Please re-read that e-mail and verify if the solution that
> I proposed solves the problem.  That solution does not
> require moving the Slot Control code from pciehp to portdrv.

There might be some misunderstaning here :), I responded in
https://lore.kernel.org/lkml/Z6LRAozZm1UfgjqT@U-2FWC9VHC-2323.local/
that your suggestion could solve our issue.

And the reason I didn't take it is I was afraid that it might hurt
the problem what commit 2bd50dd800b5 ("PCI: PCIe: Disable PCIe port
services during port initialization") tried to solve.

As you mentioned, the comment for 2bd50dd800b5 is "a bit confusing",
and I tried to guess in my previous reply: 
"
I'm not sure what problem this piece of code try to avoid, maybe
something simliar to the irq storm isseu as mentioned in the 2/2 patch?
The code comments could be about the small time window between this
point and the loading of pciehp driver, which will request_irq and
enable hotplug interrupt again.
"

The code comment from 2bd50dd800b5 is:

	/*
	 * Disable hot-plug interrupts in case they have been
	 * enabled by the BIOS and the hot-plug service driver
	 * is not loaded.
	 */

The "is not loaded" has 2 possible meanings:
1. the pciehp driver is not loaded yet at this point inside
   get_port_device_capability(), and will be loaded later
2. the pciehp will never be loaded, i.e. CONFIG_HOTPLUG_PCI_PCIE=n 

If it's case 2, your suggestion can solve it nicely, but for case 1,
we may have to keep the interrupt disabling.

Thanks,
Feng