diff mbox series

[net-next,v2] igbvf: add PCI reset handler functions

Message ID 20230217085749.348624-1-kamil.maziarz@intel.com
State Changes Requested
Headers show
Series [net-next,v2] igbvf: add PCI reset handler functions | expand

Commit Message

Kamil Maziarz Feb. 17, 2023, 8:57 a.m. UTC 
From: Dawid Wesierski <dawidx.wesierski@intel.com>

There was a problem with resuming ping after conducting a PCI reset.
This commit adds two functions, igbvf_io_prepare and igbvf_io_done,
which, after being added to the pci_error_handlers struct,
will prepare the drivers for a PCI reset and then bring the interface up
and reset it after the reset. This will prevent the driver from
ending up in an incorrect state.

Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
---
v2: added newlines
---
 drivers/net/ethernet/intel/igbvf/netdev.c | 29 +++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Paul Menzel Feb. 17, 2023, 9:56 a.m. UTC | #1 
Dear Kamil, dear Dawid,


Thank you for the patch.

Am 17.02.23 um 09:57 schrieb Kamil Maziarz:
> From: Dawid Wesierski <dawidx.wesierski@intel.com>
> 
> There was a problem with resuming ping after conducting a PCI reset.
> This commit adds two functions, igbvf_io_prepare and igbvf_io_done,
> which, after being added to the pci_error_handlers struct,
> will prepare the drivers for a PCI reset and then bring the interface up
> and reset it after the reset. This will prevent the driver from
> ending up in an incorrect state.

“and reset it after the reset” sounds confusing. Can you clear that up 
please?

Additionally, where is that requirement specified? Please document the 
datasheet name, version and section.

> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> ---
> v2: added newlines
> ---
>   drivers/net/ethernet/intel/igbvf/netdev.c | 29 +++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
> index 72cb1b56e9f2..7ff2752dd763 100644
> --- a/drivers/net/ethernet/intel/igbvf/netdev.c
> +++ b/drivers/net/ethernet/intel/igbvf/netdev.c
> @@ -2593,6 +2593,33 @@ static void igbvf_io_resume(struct pci_dev *pdev)
>   	netif_device_attach(netdev);
>   }
>   
> +/**
> + * igbvf_io_prepare - prepare device driver for PCI reset
> + * @pdev: PCI device information struct
> + */
> +static void igbvf_io_prepare(struct pci_dev *pdev)
> +{
> +	struct net_device *netdev = pci_get_drvdata(pdev);
> +	struct igbvf_adapter *adapter = netdev_priv(netdev);
> +
> +	while (test_and_set_bit(__IGBVF_RESETTING, &adapter->state))
> +		usleep_range(1000, 2000);

This looks like a potential endless loop? Should a timeout be added, and 
an error message, in case the timeout is hit?

As you introduce delays here, please document in the commit message, how 
much delay you experienced on your test systems.

> +	igbvf_down(adapter);
> +}
> +
> +/**
> + * igbvf_io_reset_done - PCI reset done, device driver reset can begin
> + * @pdev: PCI device information struct
> + */
> +static void igbvf_io_reset_done(struct pci_dev *pdev)
> +{
> +	struct net_device *netdev = pci_get_drvdata(pdev);
> +	struct igbvf_adapter *adapter = netdev_priv(netdev);
> +
> +	igbvf_up(adapter);
> +	clear_bit(__IGBVF_RESETTING, &adapter->state);
> +}
> +
>   static void igbvf_print_device_info(struct igbvf_adapter *adapter)
>   {
>   	struct e1000_hw *hw = &adapter->hw;
> @@ -2920,6 +2947,8 @@ static const struct pci_error_handlers igbvf_err_handler = {
>   	.error_detected = igbvf_io_error_detected,
>   	.slot_reset = igbvf_io_slot_reset,
>   	.resume = igbvf_io_resume,
> +	.reset_prepare = igbvf_io_prepare,
> +	.reset_done = igbvf_io_reset_done,
>   };
>   
>   static const struct pci_device_id igbvf_pci_tbl[] = {


Kind regards,

Paul
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igbvf/netdev.c b/drivers/net/ethernet/intel/igbvf/netdev.c
index 72cb1b56e9f2..7ff2752dd763 100644
--- a/drivers/net/ethernet/intel/igbvf/netdev.c
+++ b/drivers/net/ethernet/intel/igbvf/netdev.c
@@ -2593,6 +2593,33 @@  static void igbvf_io_resume(struct pci_dev *pdev)
 	netif_device_attach(netdev);
 }
 
+/**
+ * igbvf_io_prepare - prepare device driver for PCI reset
+ * @pdev: PCI device information struct
+ */
+static void igbvf_io_prepare(struct pci_dev *pdev)
+{
+	struct net_device *netdev = pci_get_drvdata(pdev);
+	struct igbvf_adapter *adapter = netdev_priv(netdev);
+
+	while (test_and_set_bit(__IGBVF_RESETTING, &adapter->state))
+		usleep_range(1000, 2000);
+	igbvf_down(adapter);
+}
+
+/**
+ * igbvf_io_reset_done - PCI reset done, device driver reset can begin
+ * @pdev: PCI device information struct
+ */
+static void igbvf_io_reset_done(struct pci_dev *pdev)
+{
+	struct net_device *netdev = pci_get_drvdata(pdev);
+	struct igbvf_adapter *adapter = netdev_priv(netdev);
+
+	igbvf_up(adapter);
+	clear_bit(__IGBVF_RESETTING, &adapter->state);
+}
+
 static void igbvf_print_device_info(struct igbvf_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
@@ -2920,6 +2947,8 @@  static const struct pci_error_handlers igbvf_err_handler = {
 	.error_detected = igbvf_io_error_detected,
 	.slot_reset = igbvf_io_slot_reset,
 	.resume = igbvf_io_resume,
+	.reset_prepare = igbvf_io_prepare,
+	.reset_done = igbvf_io_reset_done,
 };
 
 static const struct pci_device_id igbvf_pci_tbl[] = {