| Message ID | 20240123090016.281252-1-stanislaw.gruszka@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [RFC] PCI/AER: Block runtime suspend when handling errors | expand |
On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote: > PM runtime can be done simultaneously with AER error handling. > Avoid that by using pm_runtime_get_sync() just after pci_dev_get() > and pm_runtime_put() just before pci_dev_put() in AER recovery > procedures. I guess there must be a general rule here, like "PCI core must use pm_runtime_get_sync() whenever touching the device asynchronously, i.e., when it's doing something unrelated to a call from the driver"? Probably would apply to all subsystem cores, not just PCI. > I'm not sure about DPC case since I do not see get/put there. It > just call pci_do_recovery() from threaded irq dcd_handler(). > I think pm_runtime* should be added to this handler as well. s/dcd_handler/dpc_handler/ I'm guessing the "threaded" part really doesn't matter; just the fact that this is in response to an interrupt, not something directly called by a driver? > pm_runtime_get_sync() will increase dev->power.usage_count counter to > prevent any rpm actives. When there is suspending pending, it will wait > for it and do the rpm resume. Not sure if that problem, on my testing > I did not encounter issues with that. Sorry, I didn't catch your meaning here. IIUC, you can reproduce the problem with the simultaneous aer_inject and rpm suspend/resume, and this patch fixes it. But there's some other scenario where you *don't* see the problem? > I tested with igc device by doing simultaneous aer_inject and > rpm suspend/resume via /sys/bus/pci/devices/PCI_ID/power/control > and can reproduce: > > [ 853.253938] igc 0000:02:00.0: not ready 65535ms after bus reset; giving up > [ 853.253973] pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) > [ 853.253996] pcieport 0000:00:1c.2: AER: subordinate device reset failed > [ 853.254099] pcieport 0000:00:1c.2: AER: device recovery failed > [ 853.254178] igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible Drop the timestamps; they don't add to understanding the problem. > The problem disappears when applied this patch. > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/pci/pcie/aer.c | 8 ++++++++ > drivers/pci/pcie/edr.c | 3 +++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index 42a3bd35a3e1..9b56460edc76 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -23,6 +23,7 @@ > #include <linux/kernel.h> > #include <linux/errno.h> > #include <linux/pm.h> > +#include <linux/pm_runtime.h> > #include <linux/init.h> > #include <linux/interrupt.h> > #include <linux/delay.h> > @@ -813,6 +814,7 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) > { > if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) { > e_info->dev[e_info->error_dev_num] = pci_dev_get(dev); > + pm_runtime_get_sync(&dev->dev); > e_info->error_dev_num++; > return 0; > } > @@ -1111,6 +1113,8 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) > { > cxl_rch_handle_error(dev, info); > pci_aer_handle_error(dev, info); > + > + pm_runtime_put(&dev->dev); > pci_dev_put(dev); > } > > @@ -1143,6 +1147,8 @@ static void aer_recover_work_func(struct work_struct *work) > PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn)); > continue; > } > + pm_runtime_get_sync(&pdev->dev); > + > pci_print_aer(pdev, entry.severity, entry.regs); > /* > * Memory for aer_capability_regs(entry.regs) is being allocated from the > @@ -1159,6 +1165,8 @@ static void aer_recover_work_func(struct work_struct *work) > else if (entry.severity == AER_FATAL) > pcie_do_recovery(pdev, pci_channel_io_frozen, > aer_root_reset); > + > + pm_runtime_put(&pdev->dev); > pci_dev_put(pdev); > } > } > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > index 5f4914d313a1..bd96babd7249 100644 > --- a/drivers/pci/pcie/edr.c > +++ b/drivers/pci/pcie/edr.c > @@ -10,6 +10,7 @@ > > #include <linux/pci.h> > #include <linux/pci-acpi.h> > +#include <linux/pm_runtime.h> > > #include "portdrv.h" > #include "../pci.h" > @@ -169,6 +170,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > return; > } > > + pm_runtime_get_sync(&edev->dev); > pci_dbg(pdev, "Reported EDR dev: %s\n", pci_name(edev)); > > /* If port does not support DPC, just send the OST */ > @@ -209,6 +211,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > acpi_send_edr_status(pdev, edev, EDR_OST_FAILED); > } > > + pm_runtime_put(&edev->dev); > pci_dev_put(edev); > } > > -- > 2.34.1 >
On Mon, Jan 29, 2024 at 06:14:36PM -0600, Bjorn Helgaas wrote: > On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote: > > PM runtime can be done simultaneously with AER error handling. > > Avoid that by using pm_runtime_get_sync() just after pci_dev_get() > > and pm_runtime_put() just before pci_dev_put() in AER recovery > > procedures. > > I guess there must be a general rule here, like "PCI core must use > pm_runtime_get_sync() whenever touching the device asynchronously, > i.e., when it's doing something unrelated to a call from the driver"? Clear rules would be nice, that's for sure. > Probably would apply to all subsystem cores, not just PCI. If they have something similar like AER. > > I'm not sure about DPC case since I do not see get/put there. It > > just call pci_do_recovery() from threaded irq dcd_handler(). > > I think pm_runtime* should be added to this handler as well. > > s/dcd_handler/dpc_handler/ > > I'm guessing the "threaded" part really doesn't matter; just the fact > that this is in response to an interrupt, not something directly > called by a driver? Yes. I added "threaded" just to emphasis that it's safe to add sleeping functions there. In context of possible solution, not related to the problem itself. > > pm_runtime_get_sync() will increase dev->power.usage_count counter to > > prevent any rpm actives. When there is suspending pending, it will wait > > for it and do the rpm resume. Not sure if that problem, on my testing > > I did not encounter issues with that. > > Sorry, I didn't catch your meaning here. I tired to write two things: First, pm_runtime_get_sync() after exit prevents possibility of runtime suspend, so we are sure device will not be powered off Second, if during pm_runtime_get_sync(), there is pending attempt to suspend device, it will be synchronized and device will be resumed. This can be problematic as device is in error state. But at least from software perspective we should end in device being in active state and then we can perform reset of it. [ Third scenario, i.e device is suspended, and device has to be resumed by pm_runtime_get_sync() can not really happen, because we can not get error from disabled device. ] ... after writing this I realized that is not true, as we can do recovery for several devices on subordinate bus, some of them can be rpm suspended, I presume. Need to think more about this case. > IIUC, you can reproduce the > problem with the simultaneous aer_inject and rpm suspend/resume, and > this patch fixes it. Yes. > But there's some other scenario where you *don't* see the problem? For example there are no runtime pm related race conditions with .probe, .remove , (system) .suspend, .resume. All of those have proper synchronization with runtime pm in the device core. > > I tested with igc device by doing simultaneous aer_inject and > > rpm suspend/resume via /sys/bus/pci/devices/PCI_ID/power/control > > and can reproduce: > > > > [ 853.253938] igc 0000:02:00.0: not ready 65535ms after bus reset; giving up > > [ 853.253973] pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) > > [ 853.253996] pcieport 0000:00:1c.2: AER: subordinate device reset failed > > [ 853.254099] pcieport 0000:00:1c.2: AER: device recovery failed > > [ 853.254178] igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible > > Drop the timestamps; they don't add to understanding the problem. Ok. Regards Stanislaw > > > The problem disappears when applied this patch. > > > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > > --- > > drivers/pci/pcie/aer.c | 8 ++++++++ > > drivers/pci/pcie/edr.c | 3 +++ > > 2 files changed, 11 insertions(+) > > > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index 42a3bd35a3e1..9b56460edc76 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -23,6 +23,7 @@ > > #include <linux/kernel.h> > > #include <linux/errno.h> > > #include <linux/pm.h> > > +#include <linux/pm_runtime.h> > > #include <linux/init.h> > > #include <linux/interrupt.h> > > #include <linux/delay.h> > > @@ -813,6 +814,7 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) > > { > > if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) { > > e_info->dev[e_info->error_dev_num] = pci_dev_get(dev); > > + pm_runtime_get_sync(&dev->dev); > > e_info->error_dev_num++; > > return 0; > > } > > @@ -1111,6 +1113,8 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) > > { > > cxl_rch_handle_error(dev, info); > > pci_aer_handle_error(dev, info); > > + > > + pm_runtime_put(&dev->dev); > > pci_dev_put(dev); > > } > > > > @@ -1143,6 +1147,8 @@ static void aer_recover_work_func(struct work_struct *work) > > PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn)); > > continue; > > } > > + pm_runtime_get_sync(&pdev->dev); > > + > > pci_print_aer(pdev, entry.severity, entry.regs); > > /* > > * Memory for aer_capability_regs(entry.regs) is being allocated from the > > @@ -1159,6 +1165,8 @@ static void aer_recover_work_func(struct work_struct *work) > > else if (entry.severity == AER_FATAL) > > pcie_do_recovery(pdev, pci_channel_io_frozen, > > aer_root_reset); > > + > > + pm_runtime_put(&pdev->dev); > > pci_dev_put(pdev); > > } > > } > > diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c > > index 5f4914d313a1..bd96babd7249 100644 > > --- a/drivers/pci/pcie/edr.c > > +++ b/drivers/pci/pcie/edr.c > > @@ -10,6 +10,7 @@ > > > > #include <linux/pci.h> > > #include <linux/pci-acpi.h> > > +#include <linux/pm_runtime.h> > > > > #include "portdrv.h" > > #include "../pci.h" > > @@ -169,6 +170,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > > return; > > } > > > > + pm_runtime_get_sync(&edev->dev); > > pci_dbg(pdev, "Reported EDR dev: %s\n", pci_name(edev)); > > > > /* If port does not support DPC, just send the OST */ > > @@ -209,6 +211,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) > > acpi_send_edr_status(pdev, edev, EDR_OST_FAILED); > > } > > > > + pm_runtime_put(&edev->dev); > > pci_dev_put(edev); > > } > > > > -- > > 2.34.1 > > >
On Thu, Feb 1, 2024 at 10:36 AM Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> wrote: > > On Mon, Jan 29, 2024 at 06:14:36PM -0600, Bjorn Helgaas wrote: > > On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote: > > > PM runtime can be done simultaneously with AER error handling. > > > Avoid that by using pm_runtime_get_sync() just after pci_dev_get() > > > and pm_runtime_put() just before pci_dev_put() in AER recovery > > > procedures. > > > > I guess there must be a general rule here, like "PCI core must use > > pm_runtime_get_sync() whenever touching the device asynchronously, > > i.e., when it's doing something unrelated to a call from the driver"? > > Clear rules would be nice, that's for sure. > > > Probably would apply to all subsystem cores, not just PCI. > > If they have something similar like AER. > > > > I'm not sure about DPC case since I do not see get/put there. It > > > just call pci_do_recovery() from threaded irq dcd_handler(). > > > I think pm_runtime* should be added to this handler as well. > > > > s/dcd_handler/dpc_handler/ > > > > I'm guessing the "threaded" part really doesn't matter; just the fact > > that this is in response to an interrupt, not something directly > > called by a driver? > > Yes. I added "threaded" just to emphasis that it's safe to add sleeping > functions there. In context of possible solution, not related to > the problem itself. > > > > pm_runtime_get_sync() will increase dev->power.usage_count counter to > > > prevent any rpm actives. When there is suspending pending, it will wait > > > for it and do the rpm resume. Not sure if that problem, on my testing > > > I did not encounter issues with that. > > > > Sorry, I didn't catch your meaning here. > I tired to write two things: > > First, pm_runtime_get_sync() after exit prevents possibility of > runtime suspend, so we are sure device will not be powered off > > Second, if during pm_runtime_get_sync(), there is pending attempt > to suspend device, it will be synchronized and device will > be resumed. Not exactly. pm_runtime_get_sync() will resume the device synchronously no matter what. > This can be problematic as device is in error state. If this is a real possibility (I mean, device in a low-power state and in an error state at the same time), it would be better to call __pm_runtime_disable(dev, false) instead of pm_runtime_get_sync(), as that would prevent runtime PM from changing the device state. > But at least from software perspective we should end in device > being in active state and then we can perform reset of it. I'm not sure about this. It may be better to power-cycle the device in D3hot instead of attempting to put it into D0 beforehand.
On Thu, Feb 01, 2024 at 03:10:35PM +0100, Rafael J. Wysocki wrote: > On Thu, Feb 1, 2024 at 10:36 AM Stanislaw Gruszka > <stanislaw.gruszka@linux.intel.com> wrote: > > > > On Mon, Jan 29, 2024 at 06:14:36PM -0600, Bjorn Helgaas wrote: > > > On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote: > > > > PM runtime can be done simultaneously with AER error handling. > > > > Avoid that by using pm_runtime_get_sync() just after pci_dev_get() > > > > and pm_runtime_put() just before pci_dev_put() in AER recovery > > > > procedures. > > > > > > I guess there must be a general rule here, like "PCI core must use > > > pm_runtime_get_sync() whenever touching the device asynchronously, > > > i.e., when it's doing something unrelated to a call from the driver"? > > > > Clear rules would be nice, that's for sure. > > > > > Probably would apply to all subsystem cores, not just PCI. > > > > If they have something similar like AER. > > > > > > I'm not sure about DPC case since I do not see get/put there. It > > > > just call pci_do_recovery() from threaded irq dcd_handler(). > > > > I think pm_runtime* should be added to this handler as well. > > > > > > s/dcd_handler/dpc_handler/ > > > > > > I'm guessing the "threaded" part really doesn't matter; just the fact > > > that this is in response to an interrupt, not something directly > > > called by a driver? > > > > Yes. I added "threaded" just to emphasis that it's safe to add sleeping > > functions there. In context of possible solution, not related to > > the problem itself. > > > > > > pm_runtime_get_sync() will increase dev->power.usage_count counter to > > > > prevent any rpm actives. When there is suspending pending, it will wait > > > > for it and do the rpm resume. Not sure if that problem, on my testing > > > > I did not encounter issues with that. > > > > > > Sorry, I didn't catch your meaning here. > > I tired to write two things: > > > > First, pm_runtime_get_sync() after exit prevents possibility of > > runtime suspend, so we are sure device will not be powered off > > > > Second, if during pm_runtime_get_sync(), there is pending attempt > > to suspend device, it will be synchronized and device will > > be resumed. > > Not exactly. pm_runtime_get_sync() will resume the device > synchronously no matter what. > > > This can be problematic as device is in error state. > > If this is a real possibility (I mean, device in a low-power state and > in an error state at the same time), it would be better to call > __pm_runtime_disable(dev, false) instead of pm_runtime_get_sync(), as > that would prevent runtime PM from changing the device state. __pm_runtime_disable(dev, false) does not work as reliable in my test as pm_runtime_get_sync(), the igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible message disappears, but sill have: igc 0000:02:00.0: not ready 65535ms after bus reset; giving up pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) pcieport 0000:00:1c.2: AER: subordinate device reset failed pcieport 0000:00:1c.2: AER: device recovery fail > > But at least from software perspective we should end in device > > being in active state and then we can perform reset of it. > > I'm not sure about this. It may be better to power-cycle the device > in D3hot instead of attempting to put it into D0 beforehand. Me nigher, but in pci_reset_function() and similar resetting procedures we always call pci_dev_save_and_disable() before actual reset and it set device in D0, so I think it's ok to do so. Regards Stanislaw
On Mon, Feb 5, 2024 at 9:11 PM Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> wrote: > > On Thu, Feb 01, 2024 at 03:10:35PM +0100, Rafael J. Wysocki wrote: > > On Thu, Feb 1, 2024 at 10:36 AM Stanislaw Gruszka > > <stanislaw.gruszka@linux.intel.com> wrote: > > > > > > On Mon, Jan 29, 2024 at 06:14:36PM -0600, Bjorn Helgaas wrote: > > > > On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote: > > > > > PM runtime can be done simultaneously with AER error handling. > > > > > Avoid that by using pm_runtime_get_sync() just after pci_dev_get() > > > > > and pm_runtime_put() just before pci_dev_put() in AER recovery > > > > > procedures. > > > > > > > > I guess there must be a general rule here, like "PCI core must use > > > > pm_runtime_get_sync() whenever touching the device asynchronously, > > > > i.e., when it's doing something unrelated to a call from the driver"? > > > > > > Clear rules would be nice, that's for sure. > > > > > > > Probably would apply to all subsystem cores, not just PCI. > > > > > > If they have something similar like AER. > > > > > > > > I'm not sure about DPC case since I do not see get/put there. It > > > > > just call pci_do_recovery() from threaded irq dcd_handler(). > > > > > I think pm_runtime* should be added to this handler as well. > > > > > > > > s/dcd_handler/dpc_handler/ > > > > > > > > I'm guessing the "threaded" part really doesn't matter; just the fact > > > > that this is in response to an interrupt, not something directly > > > > called by a driver? > > > > > > Yes. I added "threaded" just to emphasis that it's safe to add sleeping > > > functions there. In context of possible solution, not related to > > > the problem itself. > > > > > > > > pm_runtime_get_sync() will increase dev->power.usage_count counter to > > > > > prevent any rpm actives. When there is suspending pending, it will wait > > > > > for it and do the rpm resume. Not sure if that problem, on my testing > > > > > I did not encounter issues with that. > > > > > > > > Sorry, I didn't catch your meaning here. > > > I tired to write two things: > > > > > > First, pm_runtime_get_sync() after exit prevents possibility of > > > runtime suspend, so we are sure device will not be powered off > > > > > > Second, if during pm_runtime_get_sync(), there is pending attempt > > > to suspend device, it will be synchronized and device will > > > be resumed. > > > > Not exactly. pm_runtime_get_sync() will resume the device > > synchronously no matter what. > > > > > This can be problematic as device is in error state. > > > > If this is a real possibility (I mean, device in a low-power state and > > in an error state at the same time), it would be better to call > > __pm_runtime_disable(dev, false) instead of pm_runtime_get_sync(), as > > that would prevent runtime PM from changing the device state. > > __pm_runtime_disable(dev, false) does not work as reliable in my > test as pm_runtime_get_sync(), the > > igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible > > message disappears, but sill have: > > igc 0000:02:00.0: not ready 65535ms after bus reset; giving up > pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) > pcieport 0000:00:1c.2: AER: subordinate device reset failed > pcieport 0000:00:1c.2: AER: device recovery fail But what exactly do you do? (1) __pm_runtime_disable(dev, false) (2) Check power state (a) If D0 (and device runtime-active), proceed (b) If > D0, remove power (if possible) and put into D0 or something else? > > > But at least from software perspective we should end in device > > > being in active state and then we can perform reset of it. > > > > I'm not sure about this. It may be better to power-cycle the device > > in D3hot instead of attempting to put it into D0 beforehand. > > Me nigher, but in pci_reset_function() and similar resetting > procedures we always call pci_dev_save_and_disable() before > actual reset and it set device in D0, so I think it's ok to do so. OK
On Mon, Feb 05, 2024 at 10:00:45PM +0100, Rafael J. Wysocki wrote: > On Mon, Feb 5, 2024 at 9:11 PM Stanislaw Gruszka > <stanislaw.gruszka@linux.intel.com> wrote: > > > > On Thu, Feb 01, 2024 at 03:10:35PM +0100, Rafael J. Wysocki wrote: > > > On Thu, Feb 1, 2024 at 10:36 AM Stanislaw Gruszka > > > <stanislaw.gruszka@linux.intel.com> wrote: > > > > > > > > On Mon, Jan 29, 2024 at 06:14:36PM -0600, Bjorn Helgaas wrote: > > > > > On Tue, Jan 23, 2024 at 10:00:16AM +0100, Stanislaw Gruszka wrote: > > > > > > PM runtime can be done simultaneously with AER error handling. > > > > > > Avoid that by using pm_runtime_get_sync() just after pci_dev_get() > > > > > > and pm_runtime_put() just before pci_dev_put() in AER recovery > > > > > > procedures. > > > > > > > > > > I guess there must be a general rule here, like "PCI core must use > > > > > pm_runtime_get_sync() whenever touching the device asynchronously, > > > > > i.e., when it's doing something unrelated to a call from the driver"? > > > > > > > > Clear rules would be nice, that's for sure. > > > > > > > > > Probably would apply to all subsystem cores, not just PCI. > > > > > > > > If they have something similar like AER. > > > > > > > > > > I'm not sure about DPC case since I do not see get/put there. It > > > > > > just call pci_do_recovery() from threaded irq dcd_handler(). > > > > > > I think pm_runtime* should be added to this handler as well. > > > > > > > > > > s/dcd_handler/dpc_handler/ > > > > > > > > > > I'm guessing the "threaded" part really doesn't matter; just the fact > > > > > that this is in response to an interrupt, not something directly > > > > > called by a driver? > > > > > > > > Yes. I added "threaded" just to emphasis that it's safe to add sleeping > > > > functions there. In context of possible solution, not related to > > > > the problem itself. > > > > > > > > > > pm_runtime_get_sync() will increase dev->power.usage_count counter to > > > > > > prevent any rpm actives. When there is suspending pending, it will wait > > > > > > for it and do the rpm resume. Not sure if that problem, on my testing > > > > > > I did not encounter issues with that. > > > > > > > > > > Sorry, I didn't catch your meaning here. > > > > I tired to write two things: > > > > > > > > First, pm_runtime_get_sync() after exit prevents possibility of > > > > runtime suspend, so we are sure device will not be powered off > > > > > > > > Second, if during pm_runtime_get_sync(), there is pending attempt > > > > to suspend device, it will be synchronized and device will > > > > be resumed. > > > > > > Not exactly. pm_runtime_get_sync() will resume the device > > > synchronously no matter what. > > > > > > > This can be problematic as device is in error state. > > > > > > If this is a real possibility (I mean, device in a low-power state and > > > in an error state at the same time), it would be better to call > > > __pm_runtime_disable(dev, false) instead of pm_runtime_get_sync(), as > > > that would prevent runtime PM from changing the device state. > > > > __pm_runtime_disable(dev, false) does not work as reliable in my > > test as pm_runtime_get_sync(), the > > > > igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible > > > > message disappears, but sill have: > > > > igc 0000:02:00.0: not ready 65535ms after bus reset; giving up > > pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) > > pcieport 0000:00:1c.2: AER: subordinate device reset failed > > pcieport 0000:00:1c.2: AER: device recovery fail > > But what exactly do you do? > > (1) __pm_runtime_disable(dev, false) > (2) Check power state > (a) If D0 (and device runtime-active), proceed > (b) If > D0, remove power (if possible) and put into D0 > > or something else? I just did point (1), did not check power state (2). I tested below patch with replaced: pm_runtime_get_sync -> __pm_runtime_disable(false) pm_runtime_put -> pm_runtime_enable() I could try to test with (1)+(2), but now sure how do do step (2b), by: pci_set_power_state(D3cold) pci_set_power_state(D0) ? diff --git a/drivers/pci/pcie/err.c b/drivers/pci/pcie/err.c index 59c90d04a609..705893b5f7b0 100644 --- a/drivers/pci/pcie/err.c +++ b/drivers/pci/pcie/err.c @@ -13,6 +13,7 @@ #define dev_fmt(fmt) "AER: " fmt #include <linux/pci.h> +#include <linux/pm_runtime.h> #include <linux/module.h> #include <linux/kernel.h> #include <linux/errno.h> @@ -85,6 +86,18 @@ static int report_error_detected(struct pci_dev *dev, return 0; } +static int pci_pm_runtime_get_sync(struct pci_dev *pdev, void *data) +{ + pm_runtime_get_sync(&pdev->dev); + return 0; +} + +static int pci_pm_runtime_put(struct pci_dev *pdev, void *data) +{ + pm_runtime_put(&pdev->dev); + return 0; +} + static int report_frozen_detected(struct pci_dev *dev, void *data) { return report_error_detected(dev, pci_channel_io_frozen, data); @@ -207,6 +220,8 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, else bridge = pci_upstream_bridge(dev); + pci_walk_bridge(bridge, pci_pm_runtime_get_sync, NULL); + pci_dbg(bridge, "broadcast error_detected message\n"); if (state == pci_channel_io_frozen) { pci_walk_bridge(bridge, report_frozen_detected, &status); @@ -251,10 +266,15 @@ pci_ers_result_t pcie_do_recovery(struct pci_dev *dev, pcie_clear_device_status(dev); pci_aer_clear_nonfatal_status(dev); } + + pci_walk_bridge(bridge, pci_pm_runtime_put, NULL); + pci_info(bridge, "device recovery successful\n"); return status; failed: + pci_walk_bridge(bridge, pci_pm_runtime_put, NULL); + pci_uevent_ers(bridge, PCI_ERS_RESULT_DISCONNECT); /* TODO: Should kernel panic here? */
On Tue, Feb 06, 2024 at 04:37:36PM +0100, Stanislaw Gruszka wrote: > > > > If this is a real possibility (I mean, device in a low-power state and > > > > in an error state at the same time), it would be better to call > > > > __pm_runtime_disable(dev, false) instead of pm_runtime_get_sync(), as > > > > that would prevent runtime PM from changing the device state. > > > > > > __pm_runtime_disable(dev, false) does not work as reliable in my > > > test as pm_runtime_get_sync(), the > > > > > > igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible > > > > > > message disappears, but sill have: > > > > > > igc 0000:02:00.0: not ready 65535ms after bus reset; giving up > > > pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) > > > pcieport 0000:00:1c.2: AER: subordinate device reset failed > > > pcieport 0000:00:1c.2: AER: device recovery fail > > > > But what exactly do you do? > > > > (1) __pm_runtime_disable(dev, false) > > (2) Check power state > > (a) If D0 (and device runtime-active), proceed > > (b) If > D0, remove power (if possible) and put into D0 > > > > or something else? > > I just did point (1), did not check power state (2). > I tested below patch with replaced: > > pm_runtime_get_sync -> __pm_runtime_disable(false) > pm_runtime_put -> pm_runtime_enable() > > I could try to test with (1)+(2), but now sure how do do step (2b), > by: > > pci_set_power_state(D3cold) > pci_set_power_state(D0) The problematic case is indeed when after __pm_runtime_disable(), device is in D3hot state. In such case we need to do the same operations as pci_pm_runtime_resume() does, otherwise AER code is not able to work. I think, just doing pm_runtime_get_sync() is better. While I'm able to reproduce D3hot & error state using aer_inject on the same smae device, more practical case is recovery running on all devices connected to a port (else case in pcie_do_recovery type == PCIE_EXP_TYPE* check). On such case some devices can be suspend, and from AER code comments I conclude they have to be reset. > +static int pci_pm_runtime_put(struct pci_dev *pdev, void *data) > +{ > + pm_runtime_put(&pdev->dev); > + return 0; > +} > + <snip> > + > + pci_walk_bridge(bridge, pci_pm_runtime_put, NULL); This can happen after driver is unbind from device. I had concern about that, but after drivers/base/ code examination, seems to be fine do do pm_runtime_put() after unbind. Regards Stanislaw
diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index 42a3bd35a3e1..9b56460edc76 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -23,6 +23,7 @@ #include <linux/kernel.h> #include <linux/errno.h> #include <linux/pm.h> +#include <linux/pm_runtime.h> #include <linux/init.h> #include <linux/interrupt.h> #include <linux/delay.h> @@ -813,6 +814,7 @@ static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev) { if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) { e_info->dev[e_info->error_dev_num] = pci_dev_get(dev); + pm_runtime_get_sync(&dev->dev); e_info->error_dev_num++; return 0; } @@ -1111,6 +1113,8 @@ static void handle_error_source(struct pci_dev *dev, struct aer_err_info *info) { cxl_rch_handle_error(dev, info); pci_aer_handle_error(dev, info); + + pm_runtime_put(&dev->dev); pci_dev_put(dev); } @@ -1143,6 +1147,8 @@ static void aer_recover_work_func(struct work_struct *work) PCI_SLOT(entry.devfn), PCI_FUNC(entry.devfn)); continue; } + pm_runtime_get_sync(&pdev->dev); + pci_print_aer(pdev, entry.severity, entry.regs); /* * Memory for aer_capability_regs(entry.regs) is being allocated from the @@ -1159,6 +1165,8 @@ static void aer_recover_work_func(struct work_struct *work) else if (entry.severity == AER_FATAL) pcie_do_recovery(pdev, pci_channel_io_frozen, aer_root_reset); + + pm_runtime_put(&pdev->dev); pci_dev_put(pdev); } } diff --git a/drivers/pci/pcie/edr.c b/drivers/pci/pcie/edr.c index 5f4914d313a1..bd96babd7249 100644 --- a/drivers/pci/pcie/edr.c +++ b/drivers/pci/pcie/edr.c @@ -10,6 +10,7 @@ #include <linux/pci.h> #include <linux/pci-acpi.h> +#include <linux/pm_runtime.h> #include "portdrv.h" #include "../pci.h" @@ -169,6 +170,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) return; } + pm_runtime_get_sync(&edev->dev); pci_dbg(pdev, "Reported EDR dev: %s\n", pci_name(edev)); /* If port does not support DPC, just send the OST */ @@ -209,6 +211,7 @@ static void edr_handle_event(acpi_handle handle, u32 event, void *data) acpi_send_edr_status(pdev, edev, EDR_OST_FAILED); } + pm_runtime_put(&edev->dev); pci_dev_put(edev); }
PM runtime can be done simultaneously with AER error handling. Avoid that by using pm_runtime_get_sync() just after pci_dev_get() and pm_runtime_put() just before pci_dev_put() in AER recovery procedures. I'm not sure about DPC case since I do not see get/put there. It just call pci_do_recovery() from threaded irq dcd_handler(). I think pm_runtime* should be added to this handler as well. pm_runtime_get_sync() will increase dev->power.usage_count counter to prevent any rpm actives. When there is suspending pending, it will wait for it and do the rpm resume. Not sure if that problem, on my testing I did not encounter issues with that. I tested with igc device by doing simultaneous aer_inject and rpm suspend/resume via /sys/bus/pci/devices/PCI_ID/power/control and can reproduce: [ 853.253938] igc 0000:02:00.0: not ready 65535ms after bus reset; giving up [ 853.253973] pcieport 0000:00:1c.2: AER: Root Port link has been reset (-25) [ 853.253996] pcieport 0000:00:1c.2: AER: subordinate device reset failed [ 853.254099] pcieport 0000:00:1c.2: AER: device recovery failed [ 853.254178] igc 0000:02:00.0: Unable to change power state from D3hot to D0, device inaccessible The problem disappears when applied this patch. Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> --- drivers/pci/pcie/aer.c | 8 ++++++++ drivers/pci/pcie/edr.c | 3 +++ 2 files changed, 11 insertions(+)