[PATCH net-next 0/3] pds_core: AER handling

[PATCH net-next 0/3] pds_core: AER handling

[Shannon Nelson]

Add simple handlers for the PCI AER callbacks, and improve
the reset handling.

Shannon Nelson (3):
  pds_core: add simple AER handler
  pds_core: delete VF dev on reset
  pds_core: use pci_reset_function for health reset

 drivers/net/ethernet/amd/pds_core/auxbus.c | 18 ++++++++-
 drivers/net/ethernet/amd/pds_core/core.c   |  3 +-
 drivers/net/ethernet/amd/pds_core/core.h   |  3 --
 drivers/net/ethernet/amd/pds_core/main.c   | 47 ++++++++++++++++++++--
 4 files changed, 62 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH net-next 1/3] pds_core: add simple AER handler

[Shannon Nelson]

Set up the pci_error_handlers error_detected and resume to be
useful in handling AER events.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/main.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index 0050c5894563..e10451a5a89b 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -45,6 +45,7 @@ static void pdsc_unmap_bars(struct pdsc *pdsc)
 	for (i = 0; i < PDS_CORE_BARS_MAX; i++) {
 		if (bars[i].vaddr)
 			pci_iounmap(pdsc->pdev, bars[i].vaddr);
+		bars[i].vaddr = NULL;
 	}
 }
 
@@ -512,10 +513,33 @@ void pdsc_reset_done(struct pci_dev *pdev)
 	pdsc_restart_health_thread(pdsc);
 }
 
+static pci_ers_result_t pdsc_pci_error_detected(struct pci_dev *pdev,
+						pci_channel_state_t error)
+{
+	if (error == pci_channel_io_frozen) {
+		pdsc_reset_prepare(pdev);
+		return PCI_ERS_RESULT_NEED_RESET;
+	}
+
+	return PCI_ERS_RESULT_NONE;
+}
+
+static void pdsc_pci_error_resume(struct pci_dev *pdev)
+{
+	struct pdsc *pdsc = pci_get_drvdata(pdev);
+
+	if (test_bit(PDSC_S_FW_DEAD, &pdsc->state))
+		pci_reset_function_locked(pdev);
+}
+
 static const struct pci_error_handlers pdsc_err_handler = {
 	/* FLR handling */
 	.reset_prepare      = pdsc_reset_prepare,
 	.reset_done         = pdsc_reset_done,
+
+	/* AER handling */
+	.error_detected     = pdsc_pci_error_detected,
+	.resume             = pdsc_pci_error_resume,
 };
 
 static struct pci_driver pdsc_driver = {
-- 
2.17.1

Re: [PATCH net-next 1/3] pds_core: add simple AER handler

[Lukas Wunner]

On Fri, Feb 16, 2024 at 02:29:50PM -0800, Shannon Nelson wrote:
> Set up the pci_error_handlers error_detected and resume to be
> useful in handling AER events.

The above was committed as d740f4be7cf0 ("pds_core: add simple
AER handler").

Just noticed the following while inspecting the pci_error_handlers
of this driver:

> +static pci_ers_result_t pdsc_pci_error_detected(struct pci_dev *pdev,
> +						pci_channel_state_t error)
> +{
> +	if (error == pci_channel_io_frozen) {
> +		pdsc_reset_prepare(pdev);
> +		return PCI_ERS_RESULT_NEED_RESET;
> +	}
> +
> +	return PCI_ERS_RESULT_NONE;
> +}

The ->error_detected() callback of this driver invokes
pdsc_reset_prepare(), which unmaps BARs and calls pci_disable_device(),
but there is no corresponding ->slot_reset() callback which would invoke
pdsc_reset_done() to re-enable the device after reset recovery.

I don't have this hardware available for testing, hence do not feel
comfortable submitting a fix.  But this definitely looks broken.

Thanks,

Lukas

Re: [PATCH net-next 1/3] pds_core: add simple AER handler

[Brett Creeley]

On 8/5/2025 8:01 AM, Lukas Wunner wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, Feb 16, 2024 at 02:29:50PM -0800, Shannon Nelson wrote:
>> Set up the pci_error_handlers error_detected and resume to be
>> useful in handling AER events.
> 
> The above was committed as d740f4be7cf0 ("pds_core: add simple
> AER handler").
> 
> Just noticed the following while inspecting the pci_error_handlers
> of this driver:
> 
>> +static pci_ers_result_t pdsc_pci_error_detected(struct pci_dev *pdev,
>> +                                             pci_channel_state_t error)
>> +{
>> +     if (error == pci_channel_io_frozen) {
>> +             pdsc_reset_prepare(pdev);
>> +             return PCI_ERS_RESULT_NEED_RESET;
>> +     }
>> +
>> +     return PCI_ERS_RESULT_NONE;
>> +}
> 
> The ->error_detected() callback of this driver invokes
> pdsc_reset_prepare(), which unmaps BARs and calls pci_disable_device(),
> but there is no corresponding ->slot_reset() callback which would invoke
> pdsc_reset_done() to re-enable the device after reset recovery.
> 
> I don't have this hardware available for testing, hence do not feel
> comfortable submitting a fix.  But this definitely looks broken.

Hi Lukas,

Thanks for the note. It's been a bit since I have looked at this, but I 
believe that it's working in the following way:

1. pds_core's pci_error_handlers.error_detected callback returns 
PCI_ERS_RESULT_NEED_RESET
2. status is initialized to PCI_ERS_RESULT_RECOVERED in the pci core and 
since pds_core doesn't have a slot_reset callback then status remains 
PCI_ERS_RESULT_RECOVERED
3. pds_core's pci_error_handlers.resume callback is called, which will 
attempt reset/recover the device to a functional state

I also know that, at the very least, Shannon tested this when adding it 
to the driver.

Let me know if you still think otherwise.

Thanks again for the feedback,

Brett

Re: [PATCH net-next 1/3] pds_core: add simple AER handler

[Shannon Nelson]

On 8/5/25 3:10 PM, Brett Creeley wrote:
> On 8/5/2025 8:01 AM, Lukas Wunner wrote:
>> Caution: This message originated from an External Source. Use proper 
>> caution when opening attachments, clicking links, or responding.
>>
>>
>> On Fri, Feb 16, 2024 at 02:29:50PM -0800, Shannon Nelson wrote:
>>> Set up the pci_error_handlers error_detected and resume to be
>>> useful in handling AER events.
>>
>> The above was committed as d740f4be7cf0 ("pds_core: add simple
>> AER handler").
>>
>> Just noticed the following while inspecting the pci_error_handlers
>> of this driver:
>>
>>> +static pci_ers_result_t pdsc_pci_error_detected(struct pci_dev *pdev,
>>> + pci_channel_state_t error)
>>> +{
>>> +     if (error == pci_channel_io_frozen) {
>>> +             pdsc_reset_prepare(pdev);
>>> +             return PCI_ERS_RESULT_NEED_RESET;
>>> +     }
>>> +
>>> +     return PCI_ERS_RESULT_NONE;
>>> +}
>>
>> The ->error_detected() callback of this driver invokes
>> pdsc_reset_prepare(), which unmaps BARs and calls pci_disable_device(),
>> but there is no corresponding ->slot_reset() callback which would invoke
>> pdsc_reset_done() to re-enable the device after reset recovery.
>>
>> I don't have this hardware available for testing, hence do not feel
>> comfortable submitting a fix.  But this definitely looks broken.
>
> Hi Lukas,
>
> Thanks for the note. It's been a bit since I have looked at this, but 
> I believe that it's working in the following way:
>
> 1. pds_core's pci_error_handlers.error_detected callback returns 
> PCI_ERS_RESULT_NEED_RESET
> 2. status is initialized to PCI_ERS_RESULT_RECOVERED in the pci core 
> and since pds_core doesn't have a slot_reset callback then status 
> remains PCI_ERS_RESULT_RECOVERED
> 3. pds_core's pci_error_handlers.resume callback is called, which will 
> attempt reset/recover the device to a functional state

Thanks, Brett - yes this sounds about right by my memory.  The resume 
callback takes care of getting the device reset and re -enabled.

>
> I also know that, at the very least, Shannon tested this when adding 
> it to the driver.

Yep, several cycles, several scenarios.

Cheers,
sln

>
> Let me know if you still think otherwise.
>
> Thanks again for the feedback,
>
> Brett
>
>

Re: [PATCH net-next 1/3] pds_core: add simple AER handler

[Lukas Wunner]

On Tue, Aug 05, 2025 at 03:10:19PM -0700, Brett Creeley wrote:
> On 8/5/2025 8:01 AM, Lukas Wunner wrote:
> > On Fri, Feb 16, 2024 at 02:29:50PM -0800, Shannon Nelson wrote:
> > > Set up the pci_error_handlers error_detected and resume to be
> > > useful in handling AER events.
> > 
> > The above was committed as d740f4be7cf0 ("pds_core: add simple
> > AER handler").
> > 
> > Just noticed the following while inspecting the pci_error_handlers
> > of this driver:
> > 
> > > +static pci_ers_result_t pdsc_pci_error_detected(struct pci_dev *pdev,
> > > +                                             pci_channel_state_t error)
> > > +{
> > > +     if (error == pci_channel_io_frozen) {
> > > +             pdsc_reset_prepare(pdev);
> > > +             return PCI_ERS_RESULT_NEED_RESET;
> > > +     }
> > > +
> > > +     return PCI_ERS_RESULT_NONE;
> > > +}
> > 
> > The ->error_detected() callback of this driver invokes
> > pdsc_reset_prepare(), which unmaps BARs and calls pci_disable_device(),
> > but there is no corresponding ->slot_reset() callback which would invoke
> > pdsc_reset_done() to re-enable the device after reset recovery.
> 
> Thanks for the note. It's been a bit since I have looked at this, but I
> believe that it's working in the following way:
> 
> 1. pds_core's pci_error_handlers.error_detected callback returns
> PCI_ERS_RESULT_NEED_RESET
> 2. status is initialized to PCI_ERS_RESULT_RECOVERED in the pci core and
> since pds_core doesn't have a slot_reset callback then status remains
> PCI_ERS_RESULT_RECOVERED
> 3. pds_core's pci_error_handlers.resume callback is called, which will
> attempt reset/recover the device to a functional state

My point is, you're calling pdsc_reset_prepare() but you're never calling
pdsc_reset_done().  The former performs various teardown steps and calls
pci_disable_device(), which disables MMIO access to the device.  Since
you're never calling pdsc_reset_done(), you're not re-enabling MMIO
access to the device and re-initializing the device.  So I'd expect
any subsequent device access to fail.

Normally you'd have a ->slot_reset() callback which would call
pdsc_reset_done().  Then the code would look sane.

Moreover, the AER driver in the PCI core performs an unconditional 
Secondary Bus Reset on Fatal Errors (channel state pci_channel_io_frozen).
You're performing an additional reset of the PCI Function in
pdsc_pci_error_resume().  At least for Fatal Errors, this seems
superfluous.

You're only resetting the PCI Function if the PDSC_S_FW_DEAD bit is set,
irrespective whether you're dealing with a Fatal or Non-Fatal Error.

Normally I'd expect that you need to perform some re-initialization
after resetting the PCI Function, so this also looks weird.

Thanks,

Lukas

Re: [PATCH net-next 1/3] pds_core: add simple AER handler

[Lukas Wunner]

On Wed, Aug 06, 2025 at 08:58:40AM +0200, Lukas Wunner wrote:
> My point is, you're calling pdsc_reset_prepare() but you're never calling
> pdsc_reset_done().  The former performs various teardown steps and calls
> pci_disable_device(), which disables MMIO access to the device.  Since
> you're never calling pdsc_reset_done(), you're not re-enabling MMIO
> access to the device and re-initializing the device.  So I'd expect
> any subsequent device access to fail.
> 
> Normally you'd have a ->slot_reset() callback which would call
> pdsc_reset_done().  Then the code would look sane.
> 
> Moreover, the AER driver in the PCI core performs an unconditional 
> Secondary Bus Reset on Fatal Errors (channel state pci_channel_io_frozen).
> You're performing an additional reset of the PCI Function in
> pdsc_pci_error_resume().  At least for Fatal Errors, this seems
> superfluous.

I note that the pensando ionic driver uses the same weird pattern,
see commit c3a910e1c47a ("ionic: fill out pci error handlers").

So I suppose it got copy-pasted from there to the pds_core driver.

Thanks,

Lukas

[PATCH net-next 2/3] pds_core: delete VF dev on reset

[Shannon Nelson]

When the VF is hit with a reset, remove the aux device in
the prepare for reset and try to restore it after the reset.
The userland mechanics will need to recover and rebuild whatever
uses the device afterwards.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/auxbus.c | 18 +++++++++++++++++-
 drivers/net/ethernet/amd/pds_core/main.c   | 16 ++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/pds_core/auxbus.c b/drivers/net/ethernet/amd/pds_core/auxbus.c
index 11c23a7f3172..a3c79848a69a 100644
--- a/drivers/net/ethernet/amd/pds_core/auxbus.c
+++ b/drivers/net/ethernet/amd/pds_core/auxbus.c
@@ -184,6 +184,9 @@ int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
 	struct pds_auxiliary_dev *padev;
 	int err = 0;
 
+	if (!cf)
+		return -ENODEV;
+
 	mutex_lock(&pf->config_lock);
 
 	padev = pf->vfs[cf->vf_id].padev;
@@ -202,14 +205,27 @@ int pdsc_auxbus_dev_del(struct pdsc *cf, struct pdsc *pf)
 int pdsc_auxbus_dev_add(struct pdsc *cf, struct pdsc *pf)
 {
 	struct pds_auxiliary_dev *padev;
-	enum pds_core_vif_types vt;
 	char devname[PDS_DEVNAME_LEN];
+	enum pds_core_vif_types vt;
+	unsigned long mask;
 	u16 vt_support;
 	int client_id;
 	int err = 0;
 
+	if (!cf)
+		return -ENODEV;
+
 	mutex_lock(&pf->config_lock);
 
+	mask = BIT_ULL(PDSC_S_FW_DEAD) |
+	       BIT_ULL(PDSC_S_STOPPING_DRIVER);
+	if (cf->state & mask) {
+		dev_err(pf->dev, "%s: can't add dev, VF client in bad state %#lx\n",
+			__func__, cf->state);
+		err = -ENXIO;
+		goto out_unlock;
+	}
+
 	/* We only support vDPA so far, so it is the only one to
 	 * be verified that it is available in the Core device and
 	 * enabled in the devlink param.  In the future this might
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index e10451a5a89b..82901a847306 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -476,6 +476,14 @@ void pdsc_reset_prepare(struct pci_dev *pdev)
 	pdsc_stop_health_thread(pdsc);
 	pdsc_fw_down(pdsc);
 
+	if (pdev->is_virtfn) {
+		struct pdsc *pf;
+
+		pf = pdsc_get_pf_struct(pdsc->pdev);
+		if (!IS_ERR(pf))
+			pdsc_auxbus_dev_del(pdsc, pf);
+	}
+
 	pdsc_unmap_bars(pdsc);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
@@ -511,6 +519,14 @@ void pdsc_reset_done(struct pci_dev *pdev)
 
 	pdsc_fw_up(pdsc);
 	pdsc_restart_health_thread(pdsc);
+
+	if (pdev->is_virtfn) {
+		struct pdsc *pf;
+
+		pf = pdsc_get_pf_struct(pdsc->pdev);
+		if (!IS_ERR(pf))
+			pdsc_auxbus_dev_add(pdsc, pf);
+	}
 }
 
 static pci_ers_result_t pdsc_pci_error_detected(struct pci_dev *pdev,
-- 
2.17.1

[PATCH net-next 3/3] pds_core: use pci_reset_function for health reset

[Shannon Nelson]

We get the benefit of all the PCI reset locking and recovery if
we use the existing pci_reset_function() that will call our
local reset handlers.

Reviewed-by: Brett Creeley <brett.creeley@amd.com>
Signed-off-by: Shannon Nelson <shannon.nelson@amd.com>
---
 drivers/net/ethernet/amd/pds_core/core.c | 3 +--
 drivers/net/ethernet/amd/pds_core/core.h | 3 ---
 drivers/net/ethernet/amd/pds_core/main.c | 7 ++++---
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/amd/pds_core/core.c b/drivers/net/ethernet/amd/pds_core/core.c
index 1234a4a8a4ae..9662ee72814c 100644
--- a/drivers/net/ethernet/amd/pds_core/core.c
+++ b/drivers/net/ethernet/amd/pds_core/core.c
@@ -607,8 +607,7 @@ static void pdsc_check_pci_health(struct pdsc *pdsc)
 	if (fw_status != PDS_RC_BAD_PCI)
 		return;
 
-	pdsc_reset_prepare(pdsc->pdev);
-	pdsc_reset_done(pdsc->pdev);
+	pci_reset_function(pdsc->pdev);
 }
 
 void pdsc_health_thread(struct work_struct *work)
diff --git a/drivers/net/ethernet/amd/pds_core/core.h b/drivers/net/ethernet/amd/pds_core/core.h
index 3468a63f5ae9..92d7657dd614 100644
--- a/drivers/net/ethernet/amd/pds_core/core.h
+++ b/drivers/net/ethernet/amd/pds_core/core.h
@@ -284,9 +284,6 @@ int pdsc_devcmd_reset(struct pdsc *pdsc);
 int pdsc_dev_init(struct pdsc *pdsc);
 void pdsc_dev_uninit(struct pdsc *pdsc);
 
-void pdsc_reset_prepare(struct pci_dev *pdev);
-void pdsc_reset_done(struct pci_dev *pdev);
-
 int pdsc_intr_alloc(struct pdsc *pdsc, char *name,
 		    irq_handler_t handler, void *data);
 void pdsc_intr_free(struct pdsc *pdsc, int index);
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index 82901a847306..ab6133e7db42 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -469,7 +469,7 @@ static void pdsc_restart_health_thread(struct pdsc *pdsc)
 	mod_timer(&pdsc->wdtimer, jiffies + 1);
 }
 
-void pdsc_reset_prepare(struct pci_dev *pdev)
+static void pdsc_reset_prepare(struct pci_dev *pdev)
 {
 	struct pdsc *pdsc = pci_get_drvdata(pdev);
 
@@ -486,10 +486,11 @@ void pdsc_reset_prepare(struct pci_dev *pdev)
 
 	pdsc_unmap_bars(pdsc);
 	pci_release_regions(pdev);
-	pci_disable_device(pdev);
+	if (pci_is_enabled(pdev))
+		pci_disable_device(pdev);
 }
 
-void pdsc_reset_done(struct pci_dev *pdev)
+static void pdsc_reset_done(struct pci_dev *pdev)
 {
 	struct pdsc *pdsc = pci_get_drvdata(pdev);
 	struct device *dev = pdsc->dev;
-- 
2.17.1

Re: [PATCH net-next 0/3] pds_core: AER handling

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Fri, 16 Feb 2024 14:29:49 -0800 you wrote:
> Add simple handlers for the PCI AER callbacks, and improve
> the reset handling.
> 
> Shannon Nelson (3):
>   pds_core: add simple AER handler
>   pds_core: delete VF dev on reset
>   pds_core: use pci_reset_function for health reset
> 
> [...]

Here is the summary with links:
  - [net-next,1/3] pds_core: add simple AER handler
    https://git.kernel.org/netdev/net-next/c/d740f4be7cf0
  - [net-next,2/3] pds_core: delete VF dev on reset
    https://git.kernel.org/netdev/net-next/c/2dac60e06234
  - [net-next,3/3] pds_core: use pci_reset_function for health reset
    https://git.kernel.org/netdev/net-next/c/2cbab3c296f1

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html