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.
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]
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
