Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <YWS1QtNJh7vPCftH@robh.at.kernel.org> <20211013024355.GA1865721@bhelgaas>
In-Reply-To: <20211013024355.GA1865721@bhelgaas>
From:   Rob Herring <robh@kernel.org>
Date:   Wed, 13 Oct 2021 08:06:42 -0500
Message-ID: <CAL_JsqLobP9MM0EFnof_nDOBrox=gKH3xe3EQbqPceq8pRRgyA@mail.gmail.com>
Subject: Re: [PATCH 02/22] PCI: Unify PCI error response checking
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Naveen Naidu <naveennaidu479@gmail.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        PCI <linux-pci@vger.kernel.org>,
        linux-kernel-mentees@lists.linuxfoundation.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        =?UTF-8?Q?Pali_Roh=C3=A1r?= <pali@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Oct 12, 2021 at 9:43 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> [+cc Pali]
>
> On Mon, Oct 11, 2021 at 05:05:54PM -0500, Rob Herring wrote:
> > On Mon, Oct 11, 2021 at 11:08:32PM +0530, Naveen Naidu wrote:
> > > An MMIO read from a PCI device that doesn't exist or doesn't respond
> > > causes a PCI error.  There's no real data to return to satisfy the
> > > CPU read, so most hardware fabricates ~0 data.
> > >
> > > Use SET_PCI_ERROR_RESPONSE() to set the error response and
> > > RESPONSE_IS_PCI_ERROR() to check the error response during hardware
> > > read.
> > >
> > > These definitions make error checks consistent and easier to find.
> > >
> > > Signed-off-by: Naveen Naidu <naveennaidu479@gmail.com>
> > > ---
> > >  drivers/pci/access.c | 22 +++++++++++-----------
> > >  1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > > index 46935695cfb9..e1954bbbd137 100644
> > > --- a/drivers/pci/access.c
> > > +++ b/drivers/pci/access.c
> > > @@ -81,7 +81,7 @@ int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> > >
> > >     addr = bus->ops->map_bus(bus, devfn, where);
> > >     if (!addr) {
> > > -           *val = ~0;
> > > +           SET_PCI_ERROR_RESPONSE(val);
> >
> > This to me doesn't look like kernel style. I'd rather see a define
> > replace just '~0', but I defer to Bjorn.
> >
> > >             return PCIBIOS_DEVICE_NOT_FOUND;
> >
> > Neither does this using custom error codes rather than standard Linux
> > errno. I point this out as I that's were I'd start with the config
> > accessors. Though there are lots of occurrences so we'd need a way to do
> > this in manageable steps.
>
> I would love to see PCIBIOS_* confined to arch/x86 and everywhere else
> using standard Linux error codes.

Based on Pali's and your replies, I take it that these values
originate in x86 firmware, so the x86 code needs to convert to Linux
error codes and everywhere else can use Linux error codes everywhere.

> That's probably a lot of work, but
> Naveen has a lot of energy :)

There's 210 in drivers/pci/, 62 in the rest of drivers/ and 437 in
arch/. 332 are PCIBIOS_SUCCESSFUL which won't change values. Most of
drivers/pci/ and arch/ returning the value while the rest of drivers/
is comparing the returned value (mostly to PCIBIOS_SUCCESSFUL). There
could be checks such as 'if (ret > 0)' which are harder to find. A
coccinelle patch might be helpful here.

I think we want to do things in the following order:
- Rework any callers expecting a positive return value
- Make the config accessor defines convert positive error codes to
Linux error codes
- Convert pci_ops implementations to Linux error codes one by one.

I also considered we could make the accessors convert negative error
codes back to positive PCIBIOS_ values, then no callers have to be
checked/fixed first.

> > Can't we make PCI_OP_READ and PCI_USER_READ_CONFIG set the data value
> > and delete the drivers all doing this? Then we have 2 copies (in source)
> > rather than the many this series modifies. Though I'm not sure if there
> > are other cases of calling pci_bus.ops.read() which expect to get ~0.
>
> That does seem like a really good idea.

I don't it matters what order we do these, so this can happen first.

Rob