Re: [PATCH v2 7/8] PCI: xgene: Do PCI error check on own line

[Ilpo Järvinen]

On Tue, 29 Aug 2023, Rob Herring wrote:

> On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen
> <ilpo.jarvinen@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> >  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.)


-- 
 i.