On Tue, Oct 12, 2021 at 11:21 AM Naveen Naidu <naveennaidu479@xxxxxxxxx> wrote: > > On 11/10, 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@xxxxxxxxx> > > > --- > > > 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. > > > > Apologies, if this is a lame question. Why is the macro > SET_PCI_ERROR_RESPONSE not a kernel style. I ask this so that I do not > end up making the same mistake again. Generally, we don't do macros if a static inline function will work because you get more type checking with a function. There's exceptions like struct initializers which need to work in declarations. Second, I think the above obfuscates the code. I know exactly what the original line is doing to val. With SET_PCI_ERROR_RESPONSE(), I have to go look and it hasn't saved us any LOC to make the caller more readable. The downside of the original way, is I don't know why we set val to ~0, but just a define would tell me that. > Bjorn, did initally make a patch with only replacing '~0' but then > Andrew suggested in the patch [1] that we should use the macro. > > [1]: > https://lore.kernel.org/linux-pci/20190823104415.GC14582@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ > [Adding Andrew in the CC for this] He's no longer at Arm nor active upstream. > Apologies, I should have added this link in the cover letter but I > completely forgot about it. > > That's why I decided to go with the macro. If that is not the right > approach please let me know and I can fix it up. > > > > 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. I don't either. My guess is either just too many places to fix or somehow it trickles up to userspace (but probably not since the error codes aren't in a uapi header). > > 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. I'm expecting Bjorn to chime in. Rob