diff mbox

[PATCHv6,3/5] pci: No config access for disconnected devices

Message ID 1486495957-26177-4-git-send-email-keith.busch@intel.com
State Accepted
Headers show

Commit Message

Keith Busch Feb. 7, 2017, 7:32 p.m. UTC 
If we've  detected the PCI device is disconnected, there is no need to
attempt to access its config space since we know the operation will
fail. This patch has all the config reads and writes return -ENODEV
error immediately when in such a state.

If a config read is sent to a disconnected device, the value will be
set to all 1's. This is the same as what hardware is expected to return
when accessing a removed device, but software can do this faster without
relying on hardware.

Signed-off-by: Keith Busch <keith.busch@intel.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/access.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Lukas Wunner Feb. 8, 2017, 6:04 a.m. UTC | #1 
On Tue, Feb 07, 2017 at 02:32:35PM -0500, Keith Busch wrote:
> If we've  detected the PCI device is disconnected, there is no need to
> attempt to access its config space since we know the operation will
> fail. This patch has all the config reads and writes return -ENODEV
> error immediately when in such a state.
> 
> If a config read is sent to a disconnected device, the value will be
> set to all 1's. This is the same as what hardware is expected to return
> when accessing a removed device, but software can do this faster without
> relying on hardware.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/pci/access.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index d37b2ed..d63e9dd 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -892,12 +892,20 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
>  
>  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
>  {
> +	if (pci_dev_is_disconnected(dev)) {

You used to have unlikely() here up until v4 but dropped it in v5.
Why?  Seemed sensible to me.

Thanks,

Lukas

> +		*val = ~0;
> +		return -ENODEV;
> +	}
>  	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_read_config_byte);
>  
>  int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
>  {
> +	if (pci_dev_is_disconnected(dev)) {
> +		*val = ~0;
> +		return -ENODEV;
> +	}
>  	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_read_config_word);
> @@ -905,18 +913,26 @@ EXPORT_SYMBOL(pci_read_config_word);
>  int pci_read_config_dword(const struct pci_dev *dev, int where,
>  					u32 *val)
>  {
> +	if (pci_dev_is_disconnected(dev)) {
> +		*val = ~0;
> +		return -ENODEV;
> +	}
>  	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_read_config_dword);
>  
>  int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
>  {
> +	if (pci_dev_is_disconnected(dev))
> +		return -ENODEV;
>  	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_write_config_byte);
>  
>  int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
>  {
> +	if (pci_dev_is_disconnected(dev))
> +		return -ENODEV;
>  	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_write_config_word);
> @@ -924,6 +940,8 @@ EXPORT_SYMBOL(pci_write_config_word);
>  int pci_write_config_dword(const struct pci_dev *dev, int where,
>  					 u32 val)
>  {
> +	if (pci_dev_is_disconnected(dev))
> +		return -ENODEV;
>  	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
>  }
>  EXPORT_SYMBOL(pci_write_config_dword);
> -- 
> 2.7.2
>
Keith Busch Feb. 10, 2017, 6:38 p.m. UTC | #2 
On Wed, Feb 08, 2017 at 07:04:10AM +0100, Lukas Wunner wrote:
> On Tue, Feb 07, 2017 at 02:32:35PM -0500, Keith Busch wrote:
> > If we've  detected the PCI device is disconnected, there is no need to
> > attempt to access its config space since we know the operation will
> > fail. This patch has all the config reads and writes return -ENODEV
> > error immediately when in such a state.
> > 
> > If a config read is sent to a disconnected device, the value will be
> > set to all 1's. This is the same as what hardware is expected to return
> > when accessing a removed device, but software can do this faster without
> > relying on hardware.
> > 
> > Signed-off-by: Keith Busch <keith.busch@intel.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  drivers/pci/access.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index d37b2ed..d63e9dd 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -892,12 +892,20 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> >  
> >  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
> >  {
> > +	if (pci_dev_is_disconnected(dev)) {
> 
> You used to have unlikely() here up until v4 but dropped it in v5.
> Why?  Seemed sensible to me.

Didn't mean to remove the unlikely. The micro-optimization isn't in a
performance path, but I'll build it back in.
Greg Kroah-Hartman Feb. 10, 2017, 7:54 p.m. UTC | #3 
On Fri, Feb 10, 2017 at 01:38:52PM -0500, Keith Busch wrote:
> On Wed, Feb 08, 2017 at 07:04:10AM +0100, Lukas Wunner wrote:
> > On Tue, Feb 07, 2017 at 02:32:35PM -0500, Keith Busch wrote:
> > > If we've  detected the PCI device is disconnected, there is no need to
> > > attempt to access its config space since we know the operation will
> > > fail. This patch has all the config reads and writes return -ENODEV
> > > error immediately when in such a state.
> > > 
> > > If a config read is sent to a disconnected device, the value will be
> > > set to all 1's. This is the same as what hardware is expected to return
> > > when accessing a removed device, but software can do this faster without
> > > relying on hardware.
> > > 
> > > Signed-off-by: Keith Busch <keith.busch@intel.com>
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > ---
> > >  drivers/pci/access.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index d37b2ed..d63e9dd 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -892,12 +892,20 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
> > >  
> > >  int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
> > >  {
> > > +	if (pci_dev_is_disconnected(dev)) {
> > 
> > You used to have unlikely() here up until v4 but dropped it in v5.
> > Why?  Seemed sensible to me.
> 
> Didn't mean to remove the unlikely. The micro-optimization isn't in a
> performance path, but I'll build it back in.

Only do so if you can measure the difference, otherwise it is useless,
and can actually hurt.

thanks,

greg k-h
Keith Busch Feb. 10, 2017, 8:54 p.m. UTC | #4 
On Fri, Feb 10, 2017 at 08:54:13PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Feb 10, 2017 at 01:38:52PM -0500, Keith Busch wrote:
> > Didn't mean to remove the unlikely. The micro-optimization isn't in a
> > performance path, but I'll build it back in.
> 
> Only do so if you can measure the difference, otherwise it is useless,
> and can actually hurt.

Okay, that sounds good to me then, and I can't measure a difference here
in either case.

Bjorn,
What do you think? Is this series okay or any other conerncs I may
help address?

Thanks,
Keith
diff mbox

Patch

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index d37b2ed..d63e9dd 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -892,12 +892,20 @@  EXPORT_SYMBOL(pcie_capability_clear_and_set_dword);
 
 int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val)
 {
+	if (pci_dev_is_disconnected(dev)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_read_config_byte);
 
 int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val)
 {
+	if (pci_dev_is_disconnected(dev)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_read_config_word);
@@ -905,18 +913,26 @@  EXPORT_SYMBOL(pci_read_config_word);
 int pci_read_config_dword(const struct pci_dev *dev, int where,
 					u32 *val)
 {
+	if (pci_dev_is_disconnected(dev)) {
+		*val = ~0;
+		return -ENODEV;
+	}
 	return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_read_config_dword);
 
 int pci_write_config_byte(const struct pci_dev *dev, int where, u8 val)
 {
+	if (pci_dev_is_disconnected(dev))
+		return -ENODEV;
 	return pci_bus_write_config_byte(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_byte);
 
 int pci_write_config_word(const struct pci_dev *dev, int where, u16 val)
 {
+	if (pci_dev_is_disconnected(dev))
+		return -ENODEV;
 	return pci_bus_write_config_word(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_word);
@@ -924,6 +940,8 @@  EXPORT_SYMBOL(pci_write_config_word);
 int pci_write_config_dword(const struct pci_dev *dev, int where,
 					 u32 val)
 {
+	if (pci_dev_is_disconnected(dev))
+		return -ENODEV;
 	return pci_bus_write_config_dword(dev->bus, dev->devfn, where, val);
 }
 EXPORT_SYMBOL(pci_write_config_dword);