Messages in this thread | | | Date | Tue, 12 Oct 2021 21:51:08 +0530 | From | Naveen Naidu <> | Subject | Re: [PATCH 02/22] PCI: Unify PCI error response checking |
| |
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@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. >
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.
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@e119886-lin.cambridge.arm.com/ [Adding Andrew in the CC for this]
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.
> 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.
Thank you very much for the review :-)
> Rob
| |