Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: "Haeuptle, Michael" <michael.haeuptle@hpe.com>
To: Christoph Hellwig <hch@lst.de>, Lukas Wunner <lukas@wunner.de>
Cc: "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"michaelhaeuptle@gmail.com" <michaelhaeuptle@gmail.com>
Subject: RE: Deadlock during PCIe hot remove
Date: Thu, 26 Mar 2020 19:30:12 +0000
Message-ID: <CS1PR8401MB0728B33075A430750519DE7C95CF0@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM> (raw)
In-Reply-To: <20200325104018.GA30853@lst.de>

Folks, it seems that I can avoid the deadlock by replacing the down_write in pciehp_reset_slot with a down_write_trylock.

What would be the down side of doing that? As a reminder, vfio ultimately calls this function when it disables a vfio device.

int pciehp_reset_slot(struct hotplug_slot *hotplug_slot, int probe)
{
	struct controller *ctrl = to_ctrl(hotplug_slot);
	struct pci_dev *pdev = ctrl_dev(ctrl);
	u16 stat_mask = 0, ctrl_mask = 0;
	int rc;

	if (probe)
		return 0;

	// original:
	// down_write(&ctrl->reset_lock);
	// change:
	if (!down_write_trylock(&ctrl->reset_lock)) {
		return -EAGAIN; // ????? or just 0?
	}
    
	if (!ATTN_BUTTN(ctrl)) {
		ctrl_mask |= PCI_EXP_SLTCTL_PDCE;
		stat_mask |= PCI_EXP_SLTSTA_PDC;
	}
	ctrl_mask |= PCI_EXP_SLTCTL_DLLSCE;
	stat_mask |= PCI_EXP_SLTSTA_DLLSC;

	pcie_write_cmd(ctrl, 0, ctrl_mask);
	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, 0);

	rc = pci_bridge_secondary_bus_reset(ctrl->pcie->port);

	pcie_capability_write_word(pdev, PCI_EXP_SLTSTA, stat_mask);
	pcie_write_cmd_nowait(ctrl, ctrl_mask, ctrl_mask);
	ctrl_dbg(ctrl, "%s: SLOTCTRL %x write cmd %x\n", __func__,
		 pci_pcie_cap(ctrl->pcie->port) + PCI_EXP_SLTCTL, ctrl_mask);

	up_write(&ctrl->reset_lock);

	return rc;
}

-----Original Message-----
From: Christoph Hellwig [mailto:hch@lst.de] 
Sent: Wednesday, March 25, 2020 4:40 AM
To: Lukas Wunner <lukas@wunner.de>
Cc: Haeuptle, Michael <michael.haeuptle@hpe.com>; Christoph Hellwig <hch@lst.de>; linux-pci@vger.kernel.org; michaelhaeuptle@gmail.com
Subject: Re: Deadlock during PCIe hot remove

On Tue, Mar 24, 2020 at 05:15:34PM +0100, Lukas Wunner wrote:
> The pci_dev_trylock() in pci_try_reset_function() looks questionable 
> to me.  It was added by commit b014e96d1abb ("PCI: Protect
> pci_error_handlers->reset_notify() usage with device_lock()") with the 
> following rationale:
> 
>     Every method in struct device_driver or structures derived from it like
>     struct pci_driver MUST provide exclusion vs the driver's ->remove()
>     method, usually by using device_lock().
>     [...]
>     Without this, ->reset_notify() may race with ->remove() calls, which
>     can be easily triggered in NVMe.
> 
> The intersection of drivers defining a ->reset_notify() hook and files 
> invoking pci_try_reset_function() appears to be empty.  So I don't 
> quite understand the problem the commit sought to address.  What am I missing?

No driver defines ->reset_notify as that has been split into
->reset_prepare and ->reset_done a while ago, and plenty of drivers
define those.  And we can't call into drivers unless we know the driver actually still is bound to the device, which is why we need the locking.


  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-24 15:21 Haeuptle, Michael
2020-03-24 15:35 ` Hoyer, David
2020-03-24 15:37   ` Haeuptle, Michael
2020-03-24 15:39     ` Hoyer, David
2020-03-24 16:26       ` Haeuptle, Michael
2020-03-24 16:32         ` Hoyer, David
2020-03-24 16:15 ` Lukas Wunner
2020-03-25 10:40   ` Christoph Hellwig
2020-03-26 19:30     ` Haeuptle, Michael [this message]
2020-03-29 13:04     ` Lukas Wunner
2020-03-31  8:14       ` Christoph Hellwig
2020-03-29 15:43 ` Lukas Wunner
2020-03-30 16:15   ` Haeuptle, Michael
2020-03-31 13:01     ` Lukas Wunner
2020-03-31 15:02       ` Haeuptle, Michael
2020-04-02 19:24         ` Haeuptle, Michael

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CS1PR8401MB0728B33075A430750519DE7C95CF0@CS1PR8401MB0728.NAMPRD84.PROD.OUTLOOK.COM \
    --to=michael.haeuptle@hpe.com \
    --cc=hch@lst.de \
    --cc=linux-pci@vger.kernel.org \
    --cc=lukas@wunner.de \
    --cc=michaelhaeuptle@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git