Re: [PATCH 02/22] PCI: Unify PCI error response checking

From: "Pali Rohár" <pali@kernel.org>
To: Naveen Naidu <naveennaidu479@gmail.com>
Cc: Rob Herring <robh@kernel.org>,
	bhelgaas@google.com,
	linux-kernel-mentees@lists.linuxfoundation.org,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	andrew.murray@arm.com
Subject: Re: [PATCH 02/22] PCI: Unify PCI error response checking
Date: Wed, 13 Oct 2021 00:52:44 +0200	[thread overview]
Message-ID: <20211012225244.y75fsd7777ubvljl@pali> (raw)
In-Reply-To: <20211012162054.rxx7aubwdvhl2eqj@theprophet>

On Tuesday 12 October 2021 21:51:08 Naveen Naidu wrote:
> On 11/10, Rob Herring wrote:
> > On Mon, Oct 11, 2021 at 11:08:32PM +0530, Naveen Naidu wrote:
> > >  		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 am sorry, but I do not have any answer for this. I really do not know
> why we return custom error codes instead of standard Linux errno. Maybe
> someone else can pitch in on this.

More people are asking same question and Bjorn recently wrote answer:
https://lore.kernel.org/linux-pci/20211011205851.GA1690395@bhelgaas/

It looks like some relict from past when PCI was implemented only for x86.

Anyway, I was looking at PCI firmware spec (where are these PCBIOS_*
codes defined) and config read function can return only
PCIBIOS_SUCCESSFUL value.

So if kernel PCI API is following this PCBIOS API it means that
controller drivers are implementing it improperly if they returns also
non-success value for read method in some cases. And for me it looks
like very "stupid" API for read as it has basically same meaning as
function with void return value.

> > 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.
> > 
> 
> This seems like a really good idea :) But again, I am not entirely sure
> if doing so would give us any unexpected behaviour. I'll wait for some
> one to reply to this and if people agree to it, I would be glad to make
> the changes to PCI_OP_READ and PCI_USER_READ_CONFIG and send a new
> patch.

If you are going to discuss and change API of config read / write
functions, please CC me.

For example pci-aardvark controller reports not only "unknown error"
happened (via 0xffffffff) but can report exact PCIe error (CRS, UR, CA)
which can be mapped to some linux errnos. I can imagine that it can be
useful for some callers.