Received-SPF: pass (google.com: domain of linux-kernel+bounces-65613-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Jim Harris <jim.harris@samsung.com>
To: Leon Romanovsky <leonro@nvidia.com>
CC: Bjorn Helgaas <helgaas@kernel.org>, Kuppuswamy Sathyanarayanan
	<sathyanarayanan.kuppuswamy@linux.intel.com>, Bjorn Helgaas
	<bhelgaas@google.com>, "linux-pci@vger.kernel.org"
	<linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>, "Alex
 Williamson" <alex.williamson@redhat.com>, =?iso-8859-1?Q?Pierre_Cr=E9gut?=
	<pierre.cregut@orange.com>
Subject: Re: [PATCH v2 1/2] PCI/IOV: Revert
 "PCI/IOV: Serialize sysfs sriov_numvfs reads vs writes"
Thread-Topic: [PATCH v2 1/2] PCI/IOV: Revert "PCI/IOV: Serialize sysfs
 sriov_numvfs reads vs writes"
Thread-Index: AQHaW7MAtwi2xwjfZkaUZDAOVEJQA7EDbuYAgAHuDQCAAlV9AIAAuocAgACNHQCAAB2nAIAAIYAAgADA4wCAAKQ8gA==
Date: Wed, 14 Feb 2024 17:04:08 +0000
Message-ID: <Zczyhya/+454IaQM@ubuntu>
In-Reply-To: <20240214071618.GE52640@unreal>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <29CAD9892A45E6448C9198E23903385E@ssi.samsung.com>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
CMS-TYPE: 301P
References: <20240213174602.GD52640@unreal>
	<20240213194556.GA1219770@bhelgaas> <20240214071618.GE52640@unreal>
	<CGME20240214170409uscas1p1e04d5916cb91670ea7f6413f0347d9ca@uscas1p1.samsung.com>

On Wed, Feb 14, 2024 at 09:16:18AM +0200, Leon Romanovsky wrote:
> On Tue, Feb 13, 2024 at 01:45:56PM -0600, Bjorn Helgaas wrote:
> > On Tue, Feb 13, 2024 at 07:46:02PM +0200, Leon Romanovsky wrote:
> > > On Tue, Feb 13, 2024 at 09:59:54AM -0600, Bjorn Helgaas wrote:
> > > ...
> >=20
> > > > I guess that means that if we apply this revert, the problem Pierre
> > > > reported will return.  Obviously the deadlock is more important tha=
n
> > > > the inconsistency Pierre observed, but from the user's point of vie=
w
> > > > this will look like a regression.
> > > >=20
> > > > Maybe listening to netlink and then looking at sysfs isn't the
> > > > "correct" way to do this, but I don't want to just casually break
> > > > existing user code.  If we do contemplate doing the revert, at the
> > > > very least we should include specific details about what the user c=
ode
> > > > *should* do instead, at the level of the actual commands to use
> > > > instead of "ip monitor dev; cat ${path}/device/sriov_numvfs".
> > >=20
> > > udevadm monitor will do the trick.
> > >=20
> > > Another possible solution is to refactor the code to make sure that
> > > .probe on VFs happens only after sriov_numvfs is updated.
> >=20
> > I like the idea of refactoring it so as to preserve the existing
> > ordering while also fixing the deadlock.
>=20
> I think something like this will be enough (not tested). It will et the n=
umber of VFs
> before we make VFs visible to probe:

I'll push a v3, replacing the second patch with this one instead. Although
based on this discussion it seems we're moving towards squashing the revert
with Leon's suggested patch. Bjorn, I'll assume you're still OK with just
squashing these on your end.

I would like some input on how to actually test this though. Presumably we =
see
some event on device PF and we want to make sure if we read PF/device/sriov=
_numvfs
that we see the updated value. But the only type of event I think we can
expect is the PF's sriov_numvfs CHANGE event.

Is there any way for VFs to be created outside of writing to the
sriov_numvfs sysfs file? My understanding is some older devices/drivers wil=
l
auto-create VFs when the PF is initialized, but it wasn't clear from the bu=
g
report whether that was part of the configuration here. Pierre, do you have
any recollection on this?

Or maybe testing for this case just means compile and verify with udevadm
monitor that we see the CHANGE event before any of the VFs are actually
created...

>=20
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index aaa33e8dc4c9..0cdfaae80594 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -679,12 +679,14 @@ static int sriov_enable(struct pci_dev *dev, int nr=
_virtfn)
>  	msleep(100);
>  	pci_cfg_access_unlock(dev);
> =20
> +	iov->num_VFs =3D nr_virtfn;
>  	rc =3D sriov_add_vfs(dev, initial);
> -	if (rc)
> +	if (rc) {
> +		iov->num_VFs =3D 0;
>  		goto err_pcibios;
> +	}
> =20
>  	kobject_uevent(&dev->dev.kobj, KOBJ_CHANGE);
> -	iov->num_VFs =3D nr_virtfn;
> =20
>  	return 0;
>  =