| Message ID | 20230827133705.12991-8-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote: > > Instead of a if condition with a line split, use the usual error > handling pattern with a separate variable to improve readability. > > No functional changes intended. > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > drivers/pci/controller/pci-xgene.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c > index 887b4941ff32..b7f338de160b 100644 > --- a/drivers/pci/controller/pci-xgene.c > +++ b/drivers/pci/controller/pci-xgene.c > @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn, > int where, int size, u32 *val) > { > struct xgene_pcie *port = pcie_bus_to_port(bus); > + int ret; > > - if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) != > - PCIBIOS_SUCCESSFUL) > + ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val); > + if (ret != PCIBIOS_SUCCESSFUL) Long term I think we want to replace these error codes with standard linux ones. As PCIBIOS_SUCCESSFUL is 0, I would change this to just: if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val)) return PCIBIOS_DEVICE_NOT_FOUND; > return PCIBIOS_DEVICE_NOT_FOUND; > > /* > -- > 2.30.2 >
On Tue, 29 Aug 2023, Rob Herring wrote: > On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen > <ilpo.jarvinen@linux.intel.com> wrote: > > > > Instead of a if condition with a line split, use the usual error > > handling pattern with a separate variable to improve readability. > > > > No functional changes intended. > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > drivers/pci/controller/pci-xgene.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c > > index 887b4941ff32..b7f338de160b 100644 > > --- a/drivers/pci/controller/pci-xgene.c > > +++ b/drivers/pci/controller/pci-xgene.c > > @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn, > > int where, int size, u32 *val) > > { > > struct xgene_pcie *port = pcie_bus_to_port(bus); > > + int ret; > > > > - if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) != > > - PCIBIOS_SUCCESSFUL) > > + ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val); > > + if (ret != PCIBIOS_SUCCESSFUL) > > Long term I think we want to replace these error codes with standard > linux ones. This series is preparatory work for this very goal you stated! > As PCIBIOS_SUCCESSFUL is 0, I would change this to just: > > if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val)) > return PCIBIOS_DEVICE_NOT_FOUND; I'm not so sure about this suggestion as it will overwrite the original error code (like the current approach unfortunately also does). To me it would seem more appropriate is to return the original error code instead. But more discussion is needed before making such changes to values these functions return. (And there are plenty of similar examples besides this one.)
On Tue, 29 Aug 2023, Ilpo Järvinen wrote: > On Tue, 29 Aug 2023, Rob Herring wrote: > > > On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen > > <ilpo.jarvinen@linux.intel.com> wrote: > > > > > > Instead of a if condition with a line split, use the usual error > > > handling pattern with a separate variable to improve readability. > > > > > > No functional changes intended. > > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > --- > > > drivers/pci/controller/pci-xgene.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c > > > index 887b4941ff32..b7f338de160b 100644 > > > --- a/drivers/pci/controller/pci-xgene.c > > > +++ b/drivers/pci/controller/pci-xgene.c > > > @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn, > > > int where, int size, u32 *val) > > > { > > > struct xgene_pcie *port = pcie_bus_to_port(bus); > > > + int ret; > > > > > > - if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) != > > > - PCIBIOS_SUCCESSFUL) > > > + ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val); > > > + if (ret != PCIBIOS_SUCCESSFUL) > > > > Long term I think we want to replace these error codes with standard > > linux ones. > > This series is preparatory work for this very goal you stated! > > > As PCIBIOS_SUCCESSFUL is 0, I would change this to just: > > > > if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val)) > > return PCIBIOS_DEVICE_NOT_FOUND; > > I'm not so sure about this suggestion as it will overwrite the original > error code (like the current approach unfortunately also does). To me it > would seem more appropriate is to return the original error code instead. > But more discussion is needed before making such changes to values these > functions return. (And there are plenty of similar examples besides this > one.) Actually, it looks in this case I could do that transformation safely now since pci_generic_config_read32() can only return either PCIBIOS_DEVICE_NOT_FOUND or PCIBIOS_SUCCESSFUL so it won't alter anything.
diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c index 887b4941ff32..b7f338de160b 100644 --- a/drivers/pci/controller/pci-xgene.c +++ b/drivers/pci/controller/pci-xgene.c @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val) { struct xgene_pcie *port = pcie_bus_to_port(bus); + int ret; - if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) != - PCIBIOS_SUCCESSFUL) + ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val); + if (ret != PCIBIOS_SUCCESSFUL) return PCIBIOS_DEVICE_NOT_FOUND; /*
Instead of a if condition with a line split, use the usual error handling pattern with a separate variable to improve readability. No functional changes intended. Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- drivers/pci/controller/pci-xgene.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)