Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Mar 2021 12:58:29 -0700
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Pali =?UTF-8?B?Um9ow6Fy?= <pali@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        Amey Narkhede <ameynarkhede02@gmail.com>
Subject: Re: RFC: sysfs node for Secondary PCI bus reset (PCIe Hot Reset)
Message-ID: <20210302125829.216784cd@omen.home.shazbot.org>
In-Reply-To: <20210301202817.GA201451@bjorn-Precision-5520>
References: <20210301171221.3d42a55i7h5ubqsb@pali>
        <20210301202817.GA201451@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, 1 Mar 2021 14:28:17 -0600
Bjorn Helgaas <helgaas@kernel.org> wrote:

> [+cc Alex, reset expert]
>=20
> On Mon, Mar 01, 2021 at 06:12:21PM +0100, Pali Roh=C3=A1r wrote:
> > Hello!
> >=20
> > PCIe card can be reset via in-band Hot Reset signal which can be
> > triggered by PCIe bridge via Secondary Bus Reset bit in PCI config
> > space.
> >=20
> > Kernel already exports sysfs node "reset" for triggering Functional
> > Reset of particular function of PCI device. But in some cases Functional
> > Reset is not enough and Hot Reset is required.
> >=20
> > Following RFC patch exports sysfs node "reset_bus" for PCI bridges which
> > triggers Secondary Bus Reset and therefore for PCIe bridges it resets
> > connected PCIe card.
> >=20
> > What do you think about it?
> >=20
> > Currently there is userspace script which can trigger PCIe Hot Reset by
> > modifying PCI config space from userspace:
> >=20
> > https://alexforencich.com/wiki/en/pcie/hot-reset-linux
> >=20
> > But because kernel already provides way how to trigger Functional Reset
> > it could provide also way how to trigger PCIe Hot Reset.

What that script does and what this does, or what the existing reset
attribute does, are very different.  The script finds the upstream
bridge for a given device, removes the device (ignoring that more than
one device might be affected by the bus reset), uses setpci to trigger
a secondary bus reset, then rescans devices.  The below only triggers
the secondary bus reset, neither saving and restoring affected device
state like the existing function level reset attribute, nor removing
and rescanning as the script does.  It simply leaves an entire
hierarchy of PCI devices entirely un-programmed yet still has struct
pci_devs attached to them for untold future misery.

In fact, for the case of a single device affected by the bus reset, as
intended by the script, the existing reset attribute will already do
that if the device supports no other reset mechanism.  There's actually
a running LFX mentorship project that aims to allow the user to control
the type of reset performed by the existing reset attribute such that a
user could force the bus reset behavior over other reset methods.

There might be some justification for an attribute that actually
implements the referenced script correctly, perhaps in kernel we could
avoid races with bus rescans, but simply triggering an SBR to quietly
de-program all downstream devices with no state restore or device
rescan is not it.  Any affected device would be unusable.  Was this
tested?  Thanks,

Alex

> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 50fcb62d59b5..f5e11c589498 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -1321,6 +1321,30 @@ static ssize_t reset_store(struct device *dev, s=
truct device_attribute *attr,
> > =20
> >  static DEVICE_ATTR(reset, 0200, NULL, reset_store);
> > =20
> > +static ssize_t reset_bus_store(struct device *dev, struct device_attri=
bute *attr,
> > +			       const char *buf, size_t count)
> > +{
> > +	struct pci_dev *pdev =3D to_pci_dev(dev);
> > +	unsigned long val;
> > +	ssize_t result =3D kstrtoul(buf, 0, &val);
> > +
> > +	if (result < 0)
> > +		return result;
> > +
> > +	if (val !=3D 1)
> > +		return -EINVAL;
> > +
> > +	pm_runtime_get_sync(dev);
> > +	result =3D pci_bridge_secondary_bus_reset(pdev);
> > +	pm_runtime_put(dev);
> > +	if (result < 0)
> > +		return result;
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR(reset_bus, 0200, NULL, reset_bus_store);
> > +
> >  static int pci_create_capabilities_sysfs(struct pci_dev *dev)
> >  {
> >  	int retval;
> > @@ -1332,8 +1356,15 @@ static int pci_create_capabilities_sysfs(struct =
pci_dev *dev)
> >  		if (retval)
> >  			goto error;
> >  	}
> > +	if (dev->hdr_type =3D=3D PCI_HEADER_TYPE_BRIDGE) {
> > +		retval =3D device_create_file(&dev->dev, &dev_attr_reset_bus);
> > +		if (retval)
> > +			goto error_reset_bus;
> > +	}
> >  	return 0;
> > =20
> > +error_reset_bus:
> > +	device_remove_file(&dev->dev, &dev_attr_reset);
> >  error:
> >  	pcie_vpd_remove_sysfs_dev_files(dev);
> >  	return retval;
> > @@ -1414,6 +1445,8 @@ static void pci_remove_capabilities_sysfs(struct =
pci_dev *dev)
> >  		device_remove_file(&dev->dev, &dev_attr_reset);
> >  		dev->reset_fn =3D 0;
> >  	}
> > +	if (dev->hdr_type =3D=3D PCI_HEADER_TYPE_BRIDGE)
> > +		device_remove_file(&dev->dev, &dev_attr_reset_bus);
> >  }
> > =20
> >  /** =20
>=20