[PATCH] PCI: Return error if cannot probe VF

[PATCH] PCI: Return error if cannot probe VF

[Alex Williamson]

Commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
VF driver binding") allows the user to specify that drivers for VFs of
a PF should not be probed, but it actually causes pci_device_probe() to
return success back to the driver core in this case.  Therefore by all
sysfs appearances the device is bound to a driver, the driver link from
the device exists as does the device link back from the driver, yet the
driver's probe function is never called on the device.  We also fail to
do any sort of cleanup when we're prohibited from probing the device,
the irq setup remains in place and we even hold a device reference.

Instead, abort with errno before any setup or references are taken when
pci_device_can_probe() prevents us from trying to probe the device.

Fixes: 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

This issue is easily tested by disabling sriov_drivers_autoprobe and
creating VFs:

# echo 0 > sriov_drivers_autoprobe
# echo 3 > sriov_numvfs
# readlink -f virtfn*/driver
/sys/bus/pci/drivers/iavf
/sys/bus/pci/drivers/iavf
/sys/bus/pci/drivers/iavf
(yet no netdevs exist for these VFs)

The semantics of this autoprobe disabling are a bit strange for the
user as well, I suppose it works if we force a bind through a driver's
bind attribute, but tools like libvirt and driverctl expect to bind
devices by setting the driver_override and then pushing the device
through driver_probe on the bus.  Is the intention of disabling
"autoprobe" that a driver_override should still work?  Otherwise the
user needs to set the driver_override for each VF, re-enable
sriov_drivers_autoprobe on the PF, and then probe the VFs.  Thus maybe
pci_device_can_probe() should allow probes of the driver_override
driver?  Thanks,

Alex

 drivers/pci/pci-driver.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 71853befd435..da7b82e56c83 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -414,6 +414,9 @@ static int pci_device_probe(struct device *dev)
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	struct pci_driver *drv = to_pci_driver(dev->driver);
 
+	if (!pci_device_can_probe(pci_dev))
+		return -ENODEV;
+
 	pci_assign_irq(pci_dev);
 
 	error = pcibios_alloc_irq(pci_dev);
@@ -421,12 +424,10 @@ static int pci_device_probe(struct device *dev)
 		return error;
 
 	pci_dev_get(pci_dev);
-	if (pci_device_can_probe(pci_dev)) {
-		error = __pci_device_probe(drv, pci_dev);
-		if (error) {
-			pcibios_free_irq(pci_dev);
-			pci_dev_put(pci_dev);
-		}
+	error = __pci_device_probe(drv, pci_dev);
+	if (error) {
+		pcibios_free_irq(pci_dev);
+		pci_dev_put(pci_dev);
 	}
 
 	return error;

Re: [PATCH] PCI: Return error if cannot probe VF

[Laine Stump]

On 5/1/19 1:00 PM, Alex Williamson wrote:
> Commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
> VF driver binding") allows the user to specify that drivers for VFs of
> a PF should not be probed, but it actually causes pci_device_probe() to
> return success back to the driver core in this case.  Therefore by all
> sysfs appearances the device is bound to a driver, the driver link from
> the device exists as does the device link back from the driver, yet the
> driver's probe function is never called on the device.  We also fail to
> do any sort of cleanup when we're prohibited from probing the device,
> the irq setup remains in place and we even hold a device reference.
> 
> Instead, abort with errno before any setup or references are taken when
> pci_device_can_probe() prevents us from trying to probe the device.
> 
> Fixes: 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> This issue is easily tested by disabling sriov_drivers_autoprobe and
> creating VFs:
> 
> # echo 0 > sriov_drivers_autoprobe
> # echo 3 > sriov_numvfs
> # readlink -f virtfn*/driver
> /sys/bus/pci/drivers/iavf
> /sys/bus/pci/drivers/iavf
> /sys/bus/pci/drivers/iavf
> (yet no netdevs exist for these VFs)
> 
> The semantics of this autoprobe disabling are a bit strange for the
> user as well, I suppose it works if we force a bind through a driver's
> bind attribute, but tools like libvirt and driverctl expect to bind
> devices by setting the driver_override and then pushing the device
> through driver_probe on the bus.  Is the intention of disabling
> "autoprobe" that a driver_override should still work?  Otherwise the
> user needs to set the driver_override for each VF, re-enable
> sriov_drivers_autoprobe on the PF, and then probe the VFs.  Thus maybe
> pci_device_can_probe() should allow probes of the driver_override
> driver?  


IMO, while the autoprobe feature is well intentioned, the current 
semantics are cumbersome at best. Not only do consumers need to set 
sriov_drivers_autoprobe=1 before setting driver_override=vfio-pci and 
reprobing the device in order to make it assignable to a vm with vfio, 
but prior to doing any of that they will also need to retrieve the 
original setting of autoprobe, save it off somewhere, and then after the 
vm is finished with the device, they will need to restore the saved 
value before removing driver_override and reprobing the device. It makes 
much more sense to me that the autoprobe setting be ignored if 
driver_override is set (if someone has gone to the trouble of setting 
driver_override, then it's pretty obvious that they really do want the 
device bound to that driver).



>Thanks,
> 
> Alex
> 
>   drivers/pci/pci-driver.c |   13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 71853befd435..da7b82e56c83 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -414,6 +414,9 @@ static int pci_device_probe(struct device *dev)
>   	struct pci_dev *pci_dev = to_pci_dev(dev);
>   	struct pci_driver *drv = to_pci_driver(dev->driver);
>   
> +	if (!pci_device_can_probe(pci_dev))
> +		return -ENODEV;
> +
>   	pci_assign_irq(pci_dev);
>   
>   	error = pcibios_alloc_irq(pci_dev);
> @@ -421,12 +424,10 @@ static int pci_device_probe(struct device *dev)
>   		return error;
>   
>   	pci_dev_get(pci_dev);
> -	if (pci_device_can_probe(pci_dev)) {
> -		error = __pci_device_probe(drv, pci_dev);
> -		if (error) {
> -			pcibios_free_irq(pci_dev);
> -			pci_dev_put(pci_dev);
> -		}
> +	error = __pci_device_probe(drv, pci_dev);
> +	if (error) {
> +		pcibios_free_irq(pci_dev);
> +		pci_dev_put(pci_dev);
>   	}
>   
>   	return error;
> 

Re: [PATCH] PCI: Return error if cannot probe VF

[Bjorn Helgaas]

On Wed, May 01, 2019 at 11:00:16AM -0600, Alex Williamson wrote:
> Commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
> VF driver binding") allows the user to specify that drivers for VFs of
> a PF should not be probed, but it actually causes pci_device_probe() to
> return success back to the driver core in this case.  Therefore by all
> sysfs appearances the device is bound to a driver, the driver link from
> the device exists as does the device link back from the driver, yet the
> driver's probe function is never called on the device.  We also fail to
> do any sort of cleanup when we're prohibited from probing the device,
> the irq setup remains in place and we even hold a device reference.
> 
> Instead, abort with errno before any setup or references are taken when
> pci_device_can_probe() prevents us from trying to probe the device.
> 
> Fixes: 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Applied to pci/enumeration for v5.3, thanks!

The scenario you describe, Laine, indeed sounds cumbersome.  If you
want to propose an alternate or additional patch to address that, or
if you think Alex's patch will make it harder to clean up that
scenario, I'm all ears.  But it seems like Alex's patch is an
improvement even if it leaves some problems unsolved.

> ---
> 
> This issue is easily tested by disabling sriov_drivers_autoprobe and
> creating VFs:
> 
> # echo 0 > sriov_drivers_autoprobe
> # echo 3 > sriov_numvfs
> # readlink -f virtfn*/driver
> /sys/bus/pci/drivers/iavf
> /sys/bus/pci/drivers/iavf
> /sys/bus/pci/drivers/iavf
> (yet no netdevs exist for these VFs)
> 
> The semantics of this autoprobe disabling are a bit strange for the
> user as well, I suppose it works if we force a bind through a driver's
> bind attribute, but tools like libvirt and driverctl expect to bind
> devices by setting the driver_override and then pushing the device
> through driver_probe on the bus.  Is the intention of disabling
> "autoprobe" that a driver_override should still work?  Otherwise the
> user needs to set the driver_override for each VF, re-enable
> sriov_drivers_autoprobe on the PF, and then probe the VFs.  Thus maybe
> pci_device_can_probe() should allow probes of the driver_override
> driver?  Thanks,
> 
> Alex
> 
>  drivers/pci/pci-driver.c |   13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 71853befd435..da7b82e56c83 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -414,6 +414,9 @@ static int pci_device_probe(struct device *dev)
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	struct pci_driver *drv = to_pci_driver(dev->driver);
>  
> +	if (!pci_device_can_probe(pci_dev))
> +		return -ENODEV;
> +
>  	pci_assign_irq(pci_dev);
>  
>  	error = pcibios_alloc_irq(pci_dev);
> @@ -421,12 +424,10 @@ static int pci_device_probe(struct device *dev)
>  		return error;
>  
>  	pci_dev_get(pci_dev);
> -	if (pci_device_can_probe(pci_dev)) {
> -		error = __pci_device_probe(drv, pci_dev);
> -		if (error) {
> -			pcibios_free_irq(pci_dev);
> -			pci_dev_put(pci_dev);
> -		}
> +	error = __pci_device_probe(drv, pci_dev);
> +	if (error) {
> +		pcibios_free_irq(pci_dev);
> +		pci_dev_put(pci_dev);
>  	}
>  
>  	return error;
> 

Re: [PATCH] PCI: Return error if cannot probe VF

[Alex Williamson]

On Thu, 30 May 2019 08:47:27 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> On Wed, May 01, 2019 at 11:00:16AM -0600, Alex Williamson wrote:
> > Commit 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control
> > VF driver binding") allows the user to specify that drivers for VFs of
> > a PF should not be probed, but it actually causes pci_device_probe() to
> > return success back to the driver core in this case.  Therefore by all
> > sysfs appearances the device is bound to a driver, the driver link from
> > the device exists as does the device link back from the driver, yet the
> > driver's probe function is never called on the device.  We also fail to
> > do any sort of cleanup when we're prohibited from probing the device,
> > the irq setup remains in place and we even hold a device reference.
> > 
> > Instead, abort with errno before any setup or references are taken when
> > pci_device_can_probe() prevents us from trying to probe the device.
> > 
> > Fixes: 0e7df22401a3 ("PCI: Add sysfs sriov_drivers_autoprobe to control VF driver binding")
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>  
> 
> Applied to pci/enumeration for v5.3, thanks!
> 
> The scenario you describe, Laine, indeed sounds cumbersome.  If you
> want to propose an alternate or additional patch to address that, or
> if you think Alex's patch will make it harder to clean up that
> scenario, I'm all ears.  But it seems like Alex's patch is an
> improvement even if it leaves some problems unsolved.

Hi Bjorn,

It's probably deeper in your queue, but I've posted:

https://patchwork.kernel.org/patch/10937577/

which allows devices with a driver_override to always probe.  I think
it gives us the more desirable usage model.  Thanks,

Alex


> > ---
> > 
> > This issue is easily tested by disabling sriov_drivers_autoprobe and
> > creating VFs:
> > 
> > # echo 0 > sriov_drivers_autoprobe
> > # echo 3 > sriov_numvfs
> > # readlink -f virtfn*/driver
> > /sys/bus/pci/drivers/iavf
> > /sys/bus/pci/drivers/iavf
> > /sys/bus/pci/drivers/iavf
> > (yet no netdevs exist for these VFs)
> > 
> > The semantics of this autoprobe disabling are a bit strange for the
> > user as well, I suppose it works if we force a bind through a driver's
> > bind attribute, but tools like libvirt and driverctl expect to bind
> > devices by setting the driver_override and then pushing the device
> > through driver_probe on the bus.  Is the intention of disabling
> > "autoprobe" that a driver_override should still work?  Otherwise the
> > user needs to set the driver_override for each VF, re-enable
> > sriov_drivers_autoprobe on the PF, and then probe the VFs.  Thus maybe
> > pci_device_can_probe() should allow probes of the driver_override
> > driver?  Thanks,
> > 
> > Alex
> > 
> >  drivers/pci/pci-driver.c |   13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 71853befd435..da7b82e56c83 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -414,6 +414,9 @@ static int pci_device_probe(struct device *dev)
> >  	struct pci_dev *pci_dev = to_pci_dev(dev);
> >  	struct pci_driver *drv = to_pci_driver(dev->driver);
> >  
> > +	if (!pci_device_can_probe(pci_dev))
> > +		return -ENODEV;
> > +
> >  	pci_assign_irq(pci_dev);
> >  
> >  	error = pcibios_alloc_irq(pci_dev);
> > @@ -421,12 +424,10 @@ static int pci_device_probe(struct device *dev)
> >  		return error;
> >  
> >  	pci_dev_get(pci_dev);
> > -	if (pci_device_can_probe(pci_dev)) {
> > -		error = __pci_device_probe(drv, pci_dev);
> > -		if (error) {
> > -			pcibios_free_irq(pci_dev);
> > -			pci_dev_put(pci_dev);
> > -		}
> > +	error = __pci_device_probe(drv, pci_dev);
> > +	if (error) {
> > +		pcibios_free_irq(pci_dev);
> > +		pci_dev_put(pci_dev);
> >  	}
> >  
> >  	return error;
> >