Patchwork [1/2] PCI: Device driver function reset notification

login
register
mail settings
Submitter Keith Busch
Date April 8, 2014, 11:42 p.m.
Message ID <1397000541-1085-1-git-send-email-keith.busch@intel.com>
Download mbox | patch
Permalink /patch/337748/
State Changes Requested
Headers show

Comments

Keith Busch - April 8, 2014, 11:42 p.m.
A user can issue a pci function level reset to a device using sysfs entry
/sys/bus/pci/devices/.../reset. A kernel driver handling the pci device
might like to know this reset is about to occur and when the reset attempt
completes. This is so the driver has a chance to take appropriate device
specific actions; for example, it may need to quiesce before the reset,
and reinitialize the device after.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/pci/pci.c   |   13 +++++++++++++
 include/linux/pci.h |    3 +++
 2 files changed, 16 insertions(+)
Bjorn Helgaas - April 30, 2014, 10:46 p.m.
On Tue, Apr 08, 2014 at 05:42:20PM -0600, Keith Busch wrote:
> A user can issue a pci function level reset to a device using sysfs entry
> /sys/bus/pci/devices/.../reset. A kernel driver handling the pci device
> might like to know this reset is about to occur and when the reset attempt
> completes. This is so the driver has a chance to take appropriate device
> specific actions; for example, it may need to quiesce before the reset,
> and reinitialize the device after.
> 
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/pci/pci.c   |   13 +++++++++++++
>  include/linux/pci.h |    3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index fdbc294..cb24bbe 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3320,6 +3320,15 @@ static int pci_dev_reset(struct pci_dev *dev, int probe)
>  
>  	return rc;
>  }
> +
> +static void pci_reset_notify(struct pci_dev *dev, bool prepare)
> +{
> +	const struct pci_error_handlers *err_handler =
> +			dev->driver ? dev->driver->err_handler : NULL;
> +	if (err_handler && err_handler->reset_notify)
> +		err_handler->reset_notify(dev, prepare);
> +}
> +
>  /**
>   * __pci_reset_function - reset a PCI device function
>   * @dev: PCI device to reset
> @@ -3408,11 +3417,13 @@ int pci_reset_function(struct pci_dev *dev)
>  	if (rc)
>  		return rc;
>  
> +	pci_reset_notify(dev, true);
>  	pci_dev_save_and_disable(dev);
>  
>  	rc = pci_dev_reset(dev, 0);
>  
>  	pci_dev_restore(dev);
> +	pci_reset_notify(dev, false);
>  
>  	return rc;
>  }
> @@ -3432,6 +3443,7 @@ int pci_try_reset_function(struct pci_dev *dev)
>  	if (rc)
>  		return rc;
>  
> +	pci_reset_notify(dev, true);
>  	pci_dev_save_and_disable(dev);
>  
>  	if (pci_dev_trylock(dev)) {
> @@ -3441,6 +3453,7 @@ int pci_try_reset_function(struct pci_dev *dev)
>  		rc = -EAGAIN;
>  
>  	pci_dev_restore(dev);
> +	pci_reset_notify(dev, false);

You put the notify in these functions:

  pci_reset_function()
  pci_try_reset_function()

but what about these:

  pci_reset_slot()
  pci_try_reset_slot()
  pci_reset_bus()
  pci_try_reset_bus()

It seems like this ought to work the same way over all kinds of reset.

>  	return rc;
>  }
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33aa2ca..d82dd3f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -603,6 +603,9 @@ struct pci_error_handlers {
>  	/* PCI slot has been reset */
>  	pci_ers_result_t (*slot_reset)(struct pci_dev *dev);
>  
> +	/* PCI function reset prepare or completed */
> +	void (*reset_notify)(struct pci_dev *dev, bool prepare);
> +
>  	/* Device driver may resume normal operations */
>  	void (*resume)(struct pci_dev *dev);
>  };
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Keith Busch - May 1, 2014, 7:57 p.m.
On Wed, 30 Apr 2014, Bjorn Helgaas wrote:
> On Tue, Apr 08, 2014 at 05:42:20PM -0600, Keith Busch wrote:
> You put the notify in these functions:
>
>  pci_reset_function()
>  pci_try_reset_function()
>
> but what about these:
>
>  pci_reset_slot()
>  pci_try_reset_slot()
>  pci_reset_bus()
>  pci_try_reset_bus()

Good point. These all call pci_dev_save_and_disable at the start and
pci_dev_restore after, so I think just adding the reset prepare/complete
to those functions should cover all scenarios. Does this sound ok?
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas - May 1, 2014, 8:10 p.m.
On Thu, May 1, 2014 at 1:57 PM, Keith Busch <keith.busch@intel.com> wrote:
> On Wed, 30 Apr 2014, Bjorn Helgaas wrote:
>>
>> On Tue, Apr 08, 2014 at 05:42:20PM -0600, Keith Busch wrote:
>> You put the notify in these functions:
>>
>>  pci_reset_function()
>>  pci_try_reset_function()
>>
>> but what about these:
>>
>>  pci_reset_slot()
>>  pci_try_reset_slot()
>>  pci_reset_bus()
>>  pci_try_reset_bus()
>
>
> Good point. These all call pci_dev_save_and_disable at the start and
> pci_dev_restore after, so I think just adding the reset prepare/complete
> to those functions should cover all scenarios. Does this sound ok?

Sound OK to me, as long as the save/restore functions aren't used elsewhere.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Keith Busch - May 1, 2014, 8:20 p.m.
On Thu, 1 May 2014, Bjorn Helgaas wrote:
> On Thu, May 1, 2014 at 1:57 PM, Keith Busch <keith.busch@intel.com> wrote:
>> On Wed, 30 Apr 2014, Bjorn Helgaas wrote:
>>>
>>> On Tue, Apr 08, 2014 at 05:42:20PM -0600, Keith Busch wrote:
>>> You put the notify in these functions:
>>>
>>>  pci_reset_function()
>>>  pci_try_reset_function()
>>>
>>> but what about these:
>>>
>>>  pci_reset_slot()
>>>  pci_try_reset_slot()
>>>  pci_reset_bus()
>>>  pci_try_reset_bus()
>>
>>
>> Good point. These all call pci_dev_save_and_disable at the start and
>> pci_dev_restore after, so I think just adding the reset prepare/complete
>> to those functions should cover all scenarios. Does this sound ok?
>
> Sound OK to me, as long as the save/restore functions aren't used elsewhere.

Yep, there is currently 1:1 symmetry for these in all cases. I'll make
a note for any future use that the calling disabling/restore should
maintain this property.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index fdbc294..cb24bbe 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3320,6 +3320,15 @@  static int pci_dev_reset(struct pci_dev *dev, int probe)
 
 	return rc;
 }
+
+static void pci_reset_notify(struct pci_dev *dev, bool prepare)
+{
+	const struct pci_error_handlers *err_handler =
+			dev->driver ? dev->driver->err_handler : NULL;
+	if (err_handler && err_handler->reset_notify)
+		err_handler->reset_notify(dev, prepare);
+}
+
 /**
  * __pci_reset_function - reset a PCI device function
  * @dev: PCI device to reset
@@ -3408,11 +3417,13 @@  int pci_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
+	pci_reset_notify(dev, true);
 	pci_dev_save_and_disable(dev);
 
 	rc = pci_dev_reset(dev, 0);
 
 	pci_dev_restore(dev);
+	pci_reset_notify(dev, false);
 
 	return rc;
 }
@@ -3432,6 +3443,7 @@  int pci_try_reset_function(struct pci_dev *dev)
 	if (rc)
 		return rc;
 
+	pci_reset_notify(dev, true);
 	pci_dev_save_and_disable(dev);
 
 	if (pci_dev_trylock(dev)) {
@@ -3441,6 +3453,7 @@  int pci_try_reset_function(struct pci_dev *dev)
 		rc = -EAGAIN;
 
 	pci_dev_restore(dev);
+	pci_reset_notify(dev, false);
 
 	return rc;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33aa2ca..d82dd3f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -603,6 +603,9 @@  struct pci_error_handlers {
 	/* PCI slot has been reset */
 	pci_ers_result_t (*slot_reset)(struct pci_dev *dev);
 
+	/* PCI function reset prepare or completed */
+	void (*reset_notify)(struct pci_dev *dev, bool prepare);
+
 	/* Device driver may resume normal operations */
 	void (*resume)(struct pci_dev *dev);
 };