| Message ID | 1508794608-15310-1-git-send-email-okaya@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[+cc Alex] On Mon, Oct 23, 2017 at 05:36:48PM -0400, Sinan Kaya wrote: > The return codes from various reset types are not consistent. The code is > assuming that all reset types will return -ENOTTY when things go wrong. > Instead of relying on negative error status, let's bail out if the > operation is successful instead. I like this (no surprise since I suggested something similar at http://lkml.kernel.org/r/20171011210057.GU25517@bhelgaas-glaptop.roam.corp.google.com), but I'd like Alex's opinion before merging it. Previously, we only tried the next reset method if one method failed with -ENOTTY. With this patch, we'll try the next reset method if one method fails for any reason, not just -ENOTTY. > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > --- > drivers/pci/pci.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 6078dfc..a753e07 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4200,20 +4200,20 @@ int __pci_reset_function_locked(struct pci_dev *dev) > might_sleep(); > > rc = pci_dev_specific_reset(dev, 0); > - if (rc != -ENOTTY) > + if (!rc) > return rc; > if (pcie_has_flr(dev)) { > pcie_flr(dev); > return 0; > } > rc = pci_af_flr(dev, 0); > - if (rc != -ENOTTY) > + if (!rc) > return rc; > rc = pci_pm_reset(dev, 0); > - if (rc != -ENOTTY) > + if (!rc) > return rc; > rc = pci_dev_reset_slot_function(dev, 0); > - if (rc != -ENOTTY) > + if (!rc) > return rc; > return pci_parent_bus_reset(dev, 0); > } > -- > 1.9.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, 25 Oct 2017 08:45:11 -0500 Bjorn Helgaas <helgaas@kernel.org> wrote: > [+cc Alex] > > On Mon, Oct 23, 2017 at 05:36:48PM -0400, Sinan Kaya wrote: > > The return codes from various reset types are not consistent. The code is > > assuming that all reset types will return -ENOTTY when things go wrong. > > Instead of relying on negative error status, let's bail out if the > > operation is successful instead. > > I like this (no surprise since I suggested something similar at > http://lkml.kernel.org/r/20171011210057.GU25517@bhelgaas-glaptop.roam.corp.google.com), > but I'd like Alex's opinion before merging it. > > Previously, we only tried the next reset method if one method failed > with -ENOTTY. With this patch, we'll try the next reset method if one > method fails for any reason, not just -ENOTTY. Hmm, I thought the return codes were pretty consistent. -ENOTTY means that the reset callback doesn't handle the device, move on. Many ioctls use the same return code to indicate an unknown ioctl. This allows us to differentiate success vs error vs unhandled. In the code below we lose the ability to, for instance, have a device specific reset that returns -EINVAL to prevent the PCI core for triggering further reset mechanisms which might be broken on the device. So, I don't see that this patch specifically fixes anything, but it does remove what seems like useful functionality... I'd veto it. Thanks, Alex > > Signed-off-by: Sinan Kaya <okaya@codeaurora.org> > > --- > > drivers/pci/pci.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > > index 6078dfc..a753e07 100644 > > --- a/drivers/pci/pci.c > > +++ b/drivers/pci/pci.c > > @@ -4200,20 +4200,20 @@ int __pci_reset_function_locked(struct pci_dev *dev) > > might_sleep(); > > > > rc = pci_dev_specific_reset(dev, 0); > > - if (rc != -ENOTTY) > > + if (!rc) > > return rc; > > if (pcie_has_flr(dev)) { > > pcie_flr(dev); > > return 0; > > } > > rc = pci_af_flr(dev, 0); > > - if (rc != -ENOTTY) > > + if (!rc) > > return rc; > > rc = pci_pm_reset(dev, 0); > > - if (rc != -ENOTTY) > > + if (!rc) > > return rc; > > rc = pci_dev_reset_slot_function(dev, 0); > > - if (rc != -ENOTTY) > > + if (!rc) > > return rc; > > return pci_parent_bus_reset(dev, 0); > > } > > -- > > 1.9.1 > > > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 10/25/2017 5:28 PM, Alex Williamson wrote: >> Previously, we only tried the next reset method if one method failed >> with -ENOTTY. With this patch, we'll try the next reset method if one >> method fails for any reason, not just -ENOTTY. > Hmm, I thought the return codes were pretty consistent. -ENOTTY means > that the reset callback doesn't handle the device, move on. Many > ioctls use the same return code to indicate an unknown ioctl. This > allows us to differentiate success vs error vs unhandled. In the code > below we lose the ability to, for instance, have a device specific > reset that returns -EINVAL to prevent the PCI core for triggering > further reset mechanisms which might be broken on the device. So, I > don't see that this patch specifically fixes anything, but it does > remove what seems like useful functionality... I'd veto it. Thanks, > OK, It was not obvious how the EINVAL and ENOTTY used by code inspection. Thank your very much for the clarification. I'm dropping the patch unless Bjorn has another idea. > Alex >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 6078dfc..a753e07 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4200,20 +4200,20 @@ int __pci_reset_function_locked(struct pci_dev *dev) might_sleep(); rc = pci_dev_specific_reset(dev, 0); - if (rc != -ENOTTY) + if (!rc) return rc; if (pcie_has_flr(dev)) { pcie_flr(dev); return 0; } rc = pci_af_flr(dev, 0); - if (rc != -ENOTTY) + if (!rc) return rc; rc = pci_pm_reset(dev, 0); - if (rc != -ENOTTY) + if (!rc) return rc; rc = pci_dev_reset_slot_function(dev, 0); - if (rc != -ENOTTY) + if (!rc) return rc; return pci_parent_bus_reset(dev, 0); }
The return codes from various reset types are not consistent. The code is assuming that all reset types will return -ENOTTY when things go wrong. Instead of relying on negative error status, let's bail out if the operation is successful instead. Signed-off-by: Sinan Kaya <okaya@codeaurora.org> --- drivers/pci/pci.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)