| Message ID | 252491a9c3fb015383ac757220c5df43d168fe4e.1585544197.git.sbobroff@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/eeh: Release EEH device state synchronously | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (c6624071c338732402e8c726df6a4074473eaa0e) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (7074695ac6fb965d478f373b95bc5c636e9f21b0) |
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch linus/master (7111951b8d4973bda27ff663f2cf18b663d15b48) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 29 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote: > When EEH device state was released asynchronously by the device > release handler, it was possible for an outstanding reference to > prevent it's release and it was necessary to work around that if a > device was re-discovered at the same PCI location. I think this is a bit misleading. The main situation where you'll hit this hack is when recovering a device with a driver that doesn't implement the error handling callbacks. In that case the device is removed, reset, then re-probed by the PCI core, but we assume it's the same physical device so the eeh_device state remains active. If you actually changed the underlying device I suspect something bad would happen. > Now that the state is released synchronously that is no longer > possible and the workaround is no longer necessary. You could probably fold this into the previous patch, but eh. You could probably fold this into the previous patch, but eh. > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > --- > arch/powerpc/kernel/eeh.c | 23 +---------------------- > 1 file changed, 1 insertion(+), 22 deletions(-) > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > index c36c5a7db5ca..12c248a16527 100644 > --- a/arch/powerpc/kernel/eeh.c > +++ b/arch/powerpc/kernel/eeh.c > @@ -1206,28 +1206,7 @@ void eeh_add_device_late(struct pci_dev *dev) > eeh_edev_dbg(edev, "Device already referenced!\n"); > return; > } > - > - /* > - * The EEH cache might not be removed correctly because of > - * unbalanced kref to the device during unplug time, which > - * relies on pcibios_release_device(). So we have to remove > - * that here explicitly. > - */ > - if (edev->pdev) { > - eeh_rmv_from_parent_pe(edev); > - eeh_addr_cache_rmv_dev(edev->pdev); > - eeh_sysfs_remove_device(edev->pdev); > - > - /* > - * We definitely should have the PCI device removed > - * though it wasn't correctly. So we needn't call > - * into error handler afterwards. > - */ > - edev->mode |= EEH_DEV_NO_HANDLER; > - > - edev->pdev = NULL; > - dev->dev.archdata.edev = NULL; > - } > + WARN_ON_ONCE(edev->pdev); > > if (eeh_has_flag(EEH_PROBE_MODE_DEV)) > eeh_ops->probe(pdn, NULL);
On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote: > On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote: > > When EEH device state was released asynchronously by the device > > release handler, it was possible for an outstanding reference to > > prevent it's release and it was necessary to work around that if a > > device was re-discovered at the same PCI location. > > I think this is a bit misleading. The main situation where you'll hit > this hack is when recovering a device with a driver that doesn't > implement the error handling callbacks. In that case the device is > removed, reset, then re-probed by the PCI core, but we assume it's the > same physical device so the eeh_device state remains active. > > If you actually changed the underlying device I suspect something bad > would happen. I'm not sure I understand. Isn't the case you're talking about caught by the earlier check (just above the patch)? if (edev->pdev == dev) { eeh_edev_dbg(edev, "Device already referenced!\n"); return; } > > > Now that the state is released synchronously that is no longer > > possible and the workaround is no longer necessary. > > You could probably fold this into the previous patch, but eh. You could > probably fold this into the previous patch, but eh. True. > > Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> > > --- > > arch/powerpc/kernel/eeh.c | 23 +---------------------- > > 1 file changed, 1 insertion(+), 22 deletions(-) > > > > diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c > > index c36c5a7db5ca..12c248a16527 100644 > > --- a/arch/powerpc/kernel/eeh.c > > +++ b/arch/powerpc/kernel/eeh.c > > @@ -1206,28 +1206,7 @@ void eeh_add_device_late(struct pci_dev *dev) > > eeh_edev_dbg(edev, "Device already referenced!\n"); > > return; > > } > > - > > - /* > > - * The EEH cache might not be removed correctly because of > > - * unbalanced kref to the device during unplug time, which > > - * relies on pcibios_release_device(). So we have to remove > > - * that here explicitly. > > - */ > > - if (edev->pdev) { > > - eeh_rmv_from_parent_pe(edev); > > - eeh_addr_cache_rmv_dev(edev->pdev); > > - eeh_sysfs_remove_device(edev->pdev); > > - > > - /* > > - * We definitely should have the PCI device removed > > - * though it wasn't correctly. So we needn't call > > - * into error handler afterwards. > > - */ > > - edev->mode |= EEH_DEV_NO_HANDLER; > > - > > - edev->pdev = NULL; > > - dev->dev.archdata.edev = NULL; > > - } > > + WARN_ON_ONCE(edev->pdev); > > > > if (eeh_has_flag(EEH_PROBE_MODE_DEV)) > > eeh_ops->probe(pdn, NULL); >
On Wed, Apr 8, 2020 at 4:22 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote: > > On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote: > > > When EEH device state was released asynchronously by the device > > > release handler, it was possible for an outstanding reference to > > > prevent it's release and it was necessary to work around that if a > > > device was re-discovered at the same PCI location. > > > > I think this is a bit misleading. The main situation where you'll hit > > this hack is when recovering a device with a driver that doesn't > > implement the error handling callbacks. In that case the device is > > removed, reset, then re-probed by the PCI core, but we assume it's the > > same physical device so the eeh_device state remains active. > > > > If you actually changed the underlying device I suspect something bad > > would happen. > > I'm not sure I understand. Isn't the case you're talking about caught by > the earlier check (just above the patch)? > > if (edev->pdev == dev) { > eeh_edev_dbg(edev, "Device already referenced!\n"); > return; > } No, in the case I'm talking about the pci_dev is torn down and freed(). After the PE is reset we re-probe the device and create a new pci_dev. If the release of the old pci_dev is delayed we need the hack this patch is removing. The check above should probably be a WARN_ON() since we should never be re-running the EEH probe on the same device. I think there is a case where that can happen, but I don't remember the details. Oliver
On Wed, Apr 08, 2020 at 04:53:36PM +1000, Oliver O'Halloran wrote: > On Wed, Apr 8, 2020 at 4:22 PM Sam Bobroff <sbobroff@linux.ibm.com> wrote: > > > > On Fri, Apr 03, 2020 at 05:08:32PM +1100, Oliver O'Halloran wrote: > > > On Mon, 2020-03-30 at 15:56 +1100, Sam Bobroff wrote: > > > > When EEH device state was released asynchronously by the device > > > > release handler, it was possible for an outstanding reference to > > > > prevent it's release and it was necessary to work around that if a > > > > device was re-discovered at the same PCI location. > > > > > > I think this is a bit misleading. The main situation where you'll hit > > > this hack is when recovering a device with a driver that doesn't > > > implement the error handling callbacks. In that case the device is > > > removed, reset, then re-probed by the PCI core, but we assume it's the > > > same physical device so the eeh_device state remains active. > > > > > > If you actually changed the underlying device I suspect something bad > > > would happen. > > > > I'm not sure I understand. Isn't the case you're talking about caught by > > the earlier check (just above the patch)? > > > > if (edev->pdev == dev) { > > eeh_edev_dbg(edev, "Device already referenced!\n"); > > return; > > } > > No, in the case I'm talking about the pci_dev is torn down and > freed(). After the PE is reset we re-probe the device and create a new > pci_dev. If the release of the old pci_dev is delayed we need the > hack this patch is removing. Oh, yes, that is the case I was intending to change here. But I must be missing something, isn't it also the case that's changed by patch 2/4? What I intended was, after patch 2, eeh_remove_device() is called from the bus notifier so it happens imediately when recovery calls pci_stop_and_remove_bus_device(). Once it returns, edev->pdev has already been set to NULL by eeh_remove_device() so this case can't be hit anymore, and we should clean it up (this patch). (There is a slight difference in the way EEH_PE_KEEP is handled between the code removed here and the body of eeh_remove_device(), but checking and explaining that is already on my list for v2.) (I did test recovery on an unaware device and didn't hit the WARN_ON_ONCE().) > The check above should probably be a WARN_ON() since we should never > be re-running the EEH probe on the same device. I think there is a > case where that can happen, but I don't remember the details. Yeah, I also certainly see the "Device already referenced!" message while debugging, and it would be good to track down. > Oliver
diff --git a/arch/powerpc/kernel/eeh.c b/arch/powerpc/kernel/eeh.c index c36c5a7db5ca..12c248a16527 100644 --- a/arch/powerpc/kernel/eeh.c +++ b/arch/powerpc/kernel/eeh.c @@ -1206,28 +1206,7 @@ void eeh_add_device_late(struct pci_dev *dev) eeh_edev_dbg(edev, "Device already referenced!\n"); return; } - - /* - * The EEH cache might not be removed correctly because of - * unbalanced kref to the device during unplug time, which - * relies on pcibios_release_device(). So we have to remove - * that here explicitly. - */ - if (edev->pdev) { - eeh_rmv_from_parent_pe(edev); - eeh_addr_cache_rmv_dev(edev->pdev); - eeh_sysfs_remove_device(edev->pdev); - - /* - * We definitely should have the PCI device removed - * though it wasn't correctly. So we needn't call - * into error handler afterwards. - */ - edev->mode |= EEH_DEV_NO_HANDLER; - - edev->pdev = NULL; - dev->dev.archdata.edev = NULL; - } + WARN_ON_ONCE(edev->pdev); if (eeh_has_flag(EEH_PROBE_MODE_DEV)) eeh_ops->probe(pdn, NULL);
When EEH device state was released asynchronously by the device release handler, it was possible for an outstanding reference to prevent it's release and it was necessary to work around that if a device was re-discovered at the same PCI location. Now that the state is released synchronously that is no longer possible and the workaround is no longer necessary. Signed-off-by: Sam Bobroff <sbobroff@linux.ibm.com> --- arch/powerpc/kernel/eeh.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)