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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <743fadf9-34d9-ca25-bdd6-f69bd922c3ec@linux.intel.com>
Date:   Tue, 29 Aug 2023 19:14:36 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Rob Herring <robh@...nel.org>
cc:     linux-pci@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>,
        Andi Shyti <andi.shyti@...nel.org>,
        Philippe Mathieu-Daudé <philmd@...aro.org>,
        Toan Le <toan@...amperecomputing.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        linux-arm-kernel@...ts.infradead.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 7/8] PCI: xgene: Do PCI error check on own line

On Tue, 29 Aug 2023, Rob Herring wrote:

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


-- 
 i.