| Message ID | c632b07eb1b08cc7d4346455a55641436a379abd.1633972263.git.naveennaidu479@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Unify PCI error response checking | expand |
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. 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. Rob
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
On Tue, Oct 12, 2021 at 11:21 AM Naveen Naidu <naveennaidu479@gmail.com> 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@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. 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@e119886-lin.cambridge.arm.com/ > [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
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.
[+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. That's probably a lot of work, but Naveen has a lot of energy :) > 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. Bjorn
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
On 13/10, Rob Herring wrote: > 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. > Digging through the mailing list, I see that something similar was attempted here https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214437.html which did not move forward because there were a lot of moving parts (I guess). But reading through the thread did give me an overview of what we might wanna do. The thread does bring up a good point, about not returning any error values in pci_read_config_*() and converting the function definition to something like void pci_read_config_word(struct pci_dev *dev, int where, u16 *val) The reason stated in the thread was that, the error values returned from these functions are either ignored or are not used properly. And whenever an error occurs, the error value ~0 is anyway stored in val, we could use that to test errors. Ref: https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.html I bring this point because Pali mentioned that config read function can return only PCIBIOS_SUCCESSFUL value. Maybe instead of us trying to change pci_read_config_word, we might wanna start small with changing PCI_OP_READ and PCI_USER_READ_CONFIG such that they would only ever return PCI_SUCCESFUL and if any these config accessor defines detect any error they can fabricate the value ~0 for "val" argument. And at the caller site, instead of checking the return value of PCI_OP_READ to detect errors, we could check the "val" for ~0 value. But I am unable to gauge, if we should take this task before we begin the project of removing PCIBIOS_* OR if this should be done after we compelete with PCIBIOS_* work. I guess the better question would be, if making PCI_OP_READ return only PCI_SUCCESSFULL or converting it to a void, help the PCIBIOS_* work easier? > 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. > Thank you very much for this list, this really helps me. I have been starting at the screen since morning to come up with something like this. IIUC, you mean: 1. When you mean "PCIBIOS_SUCCESSFUL which won't change values", did you mean to say, that we would keep "PCIBIOS_SUCCESSFUL" define as it is and not bother replacing it with "0"? (Atleast for the first version of patch, and can be done in a later series) 2. "Rework any callers expecting a positive return value" This means, find out the places where we have something like err = pci_read_config_dword(); if (err > 0) Then change it to: err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc); if (err != PCIBIOS_SUCCESSFUL) Is there any easy way to search for these patterns, or should I look for each instance of pci_read_config_* and other such variants and see if such an case exists? 3. "Make the config accessor defines convert positive error codes to Linux error codes" Do you mean something like: #define PCI_OP_READ(size, type, len) \ int noinline pci_bus_read_config_##size \ (struct pci_bus *bus, unsigned int devfn, int pos, type *value) \ { if (PCI_##size##_BAD) return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER); ... ... return pcibios_err_to_errno(res); 4. "Convert pci_ops implementations to Linux error codes one by one" Finally, remove all the PCIBIOS_* references from the pci_ops implementation of various drivers. > 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 Rob, When you say this do you mean - we have something like: #define PCI_OPS_READ() res = bus->ops->read(); if (res != PCIBIOS_SUCCESSFUL) SET_PCI_ERROR_RESPONSE(val); And the pci_ops implementation would look like: pci_generic_config_read() { addr = bus->ops->map_bus(); if (!addr) return PCIBIOS_DEVICE_NOT_FOUND; } This way the controller/drivers does not have to bother fabricating the ~0 value, all they have to do when they detect any error is return the error. And the PCI_OP_READ and PCI_USER_READ_CONFIG will set the ~0 value for "val". Pali, would you have concerns with the above design? > > > 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. > Yes, this makes sense. I can send a patch for this first and then start working on the PCIBIOS_* project. If anybody has any objection please do let me know. Thanks for the comment, it cleared up a lot of my doubts ^^ Thanks, Naveen
Hello! On Wednesday 13 October 2021 22:46:53 Naveen Naidu wrote: > On 13/10, Rob Herring wrote: > > 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. > > > > Digging through the mailing list, I see that something similar was > attempted here > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214437.html > which did not move forward because there were a lot of moving parts (I > guess). But reading through the thread did give me an overview of what > we might wanna do. > > The thread does bring up a good point, about not returning any error > values in pci_read_config_*() and converting the function definition to > something like > > void pci_read_config_word(struct pci_dev *dev, int where, u16 *val) > > The reason stated in the thread was that, the error values returned from > these functions are either ignored or are not used properly. And > whenever an error occurs, the error value ~0 is anyway stored in val, we > could use that to test errors. > > Ref: > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.html > > I bring this point because Pali mentioned that config read function can > return only PCIBIOS_SUCCESSFUL value. Just correction. It is PCBIOS API (for x86). Read Configuration Byte has defined only one return code: SUCCESSFUL. So if our kernel PCI API follows this PCBIOS API then it can have only SUCCESSFUL return value. Read Configuration Word and Read Configuration Dword have defined also one additional return value: BAD_REGISTER_NUMBER. This return value is retuned when you try to read 16 or 32-bit value from register which is not aligned to 16 or 32 bits. Obviously this should not happen as kernel code should call read function with correct argument. But bugs in kernel code are possible... Maybe todays compilers could allows us to define compile time checks for PCI_OP_READ with len = 2 and 4 that register is valid number? I'm not sure. Anyway, runtime check for register alignment is already done in PCI_OP_READ and PCI_USER_READ_CONFIG and it already returns PCIBIOS_BAD_REGISTER_NUMBER. So ->read() callback itself should not be called with incorrect argument (unless there is invisible bug). > Maybe instead of us trying to change pci_read_config_word, we might > wanna start small with changing PCI_OP_READ and PCI_USER_READ_CONFIG > such that they would only ever return PCI_SUCCESFUL and if any these > config accessor defines detect any error they can fabricate the value ~0 > for "val" argument. > > And at the caller site, instead of checking the return value of > PCI_OP_READ to detect errors, we could check the "val" for ~0 value. > > But I am unable to gauge, if we should take this task before we begin > the project of removing PCIBIOS_* OR if this should be done after we > compelete with PCIBIOS_* work. > > I guess the better question would be, if making PCI_OP_READ return only > PCI_SUCCESSFULL or converting it to a void, help the PCIBIOS_* work > easier? I would like to still see ability to indicate linux errno from read and write config functions. Just as additional information why operation failed. Some hardware supports it. For example it could be used for better workarounding / fixing doing retry on CRS response when HW does not support it (even it is mandatory per PCIe spec). For example read / write config functions could return -EAGAIN and PCI core would know about it... See thread and patch: https://lore.kernel.org/linux-pci/20211007192553.GA1259781@bhelgaas/ > > 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. > > > > Thank you very much for this list, this really helps me. I have been > starting at the screen since morning to come up with something like > this. IIUC, you mean: > > 1. When you mean "PCIBIOS_SUCCESSFUL which won't change values", did you > mean to say, that we would keep "PCIBIOS_SUCCESSFUL" define as it is > and not bother replacing it with "0"? (Atleast for the first version > of patch, and can be done in a later series) > > 2. "Rework any callers expecting a positive return value" > > This means, find out the places where we have something like > > err = pci_read_config_dword(); > if (err > 0) > > Then change it to: > > err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc); > if (err != PCIBIOS_SUCCESSFUL) > > Is there any easy way to search for these patterns, or should I look > for each instance of pci_read_config_* and other such variants and > see if such an case exists? > > 3. "Make the config accessor defines convert positive error codes to Linux error codes" > > Do you mean something like: > > #define PCI_OP_READ(size, type, len) \ > int noinline pci_bus_read_config_##size \ > (struct pci_bus *bus, unsigned int devfn, int pos, type *value) > \ > { > if (PCI_##size##_BAD) return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER); > ... > ... > return pcibios_err_to_errno(res); > > > 4. "Convert pci_ops implementations to Linux error codes one by one" > > Finally, remove all the PCIBIOS_* references from the pci_ops > implementation of various drivers. > > > 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 > > Rob, When you say this do you mean - we have something like: > > #define PCI_OPS_READ() > res = bus->ops->read(); > if (res != PCIBIOS_SUCCESSFUL) > SET_PCI_ERROR_RESPONSE(val); > > And the pci_ops implementation would look like: > > pci_generic_config_read() > { > addr = bus->ops->map_bus(); > if (!addr) > return PCIBIOS_DEVICE_NOT_FOUND; > } > > This way the controller/drivers does not have to bother fabricating the > ~0 value, all they have to do when they detect any error is return the > error. And the PCI_OP_READ and PCI_USER_READ_CONFIG will set the ~0 > value for "val". > > Pali, would you have concerns with the above design? Beware that not all controllers are using map_bus callback. E.g. pci-aardvark does not use map_bus and neither pci_generic_config_read(), instead it implements own read callback. But idea to fabricate 0xffffffff in PCI core code based on return value and let controller drivers to just return -errno (without need to fabricate *value in ptr) is a nice cleanup. It removes lot of repeated code. And also some bugs :-) as I see that in some cases fabricated 0xffffffff is not set on error. I like it. > > > > 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. > > > > Yes, this makes sense. I can send a patch for this first and then start > working on the PCIBIOS_* project. If anybody has any objection please do > let me know. > > Thanks for the comment, it cleared up a lot of my doubts ^^ > > Thanks, > Naveen
On Wed, Oct 13, 2021 at 10:46:53PM +0530, Naveen Naidu wrote: > 2. "Rework any callers expecting a positive return value" > > This means, find out the places where we have something like > > err = pci_read_config_dword(); > if (err > 0) > > Then change it to: > > err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc); > if (err != PCIBIOS_SUCCESSFUL) I'm sure this is obvious, but I would try hard not to add any new uses of PCIBIOS_SUCCESSFUL. > Is there any easy way to search for these patterns, or should I look > for each instance of pci_read_config_* and other such variants and > see if such an case exists? coccigrep might be able to find things like this, but I'ver never really become friends with it.
On Wed, Oct 13, 2021 at 12:17 PM Naveen Naidu <naveennaidu479@gmail.com> wrote: > > On 13/10, Rob Herring wrote: > > 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. > > > > Digging through the mailing list, I see that something similar was > attempted here > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214437.html > which did not move forward because there were a lot of moving parts (I > guess). But reading through the thread did give me an overview of what > we might wanna do. Skimming it, looks like good advice from Arnd on what to do or not do. > The thread does bring up a good point, about not returning any error > values in pci_read_config_*() and converting the function definition to > something like > > void pci_read_config_word(struct pci_dev *dev, int where, u16 *val) > > The reason stated in the thread was that, the error values returned from > these functions are either ignored or are not used properly. And > whenever an error occurs, the error value ~0 is anyway stored in val, we > could use that to test errors. Presumably, there could be some register somewhere where all 1s is valid? So I think we need the error values. Also, I seem to recall only the vendor/device IDs are defined to be all 1s for non-existent devices. Other errors are undefined? > Ref: > https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-July/214562.html > > I bring this point because Pali mentioned that config read function can > return only PCIBIOS_SUCCESSFUL value. > > Maybe instead of us trying to change pci_read_config_word, we might > wanna start small with changing PCI_OP_READ and PCI_USER_READ_CONFIG > such that they would only ever return PCI_SUCCESFUL and if any these > config accessor defines detect any error they can fabricate the value ~0 > for "val" argument. > > And at the caller site, instead of checking the return value of > PCI_OP_READ to detect errors, we could check the "val" for ~0 value. > > But I am unable to gauge, if we should take this task before we begin > the project of removing PCIBIOS_* OR if this should be done after we > compelete with PCIBIOS_* work. > > I guess the better question would be, if making PCI_OP_READ return only > PCI_SUCCESSFULL or converting it to a void, help the PCIBIOS_* work > easier? > > > 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. > > > > Thank you very much for this list, this really helps me. I have been > starting at the screen since morning to come up with something like > this. IIUC, you mean: > > 1. When you mean "PCIBIOS_SUCCESSFUL which won't change values", did you > mean to say, that we would keep "PCIBIOS_SUCCESSFUL" define as it is > and not bother replacing it with "0"? (Atleast for the first version > of patch, and can be done in a later series) Yes, removal of PCIBIOS_SUCCESSFUL can be done after/separately. That greatly reduces the number of callers to touch. > 2. "Rework any callers expecting a positive return value" > > This means, find out the places where we have something like > > err = pci_read_config_dword(); > if (err > 0) > > Then change it to: > > err = pci_read_config_dword(pdev, PCI_REG_NPKDSC, &npkdsc); > if (err != PCIBIOS_SUCCESSFUL) As Bjorn said, don't add more! Just: if (err) Because that works whether we change the error codes or not. > Is there any easy way to search for these patterns, or should I look > for each instance of pci_read_config_* and other such variants and > see if such an case exists? Besides grep and/or coccigrep, change the function definitions to return void (or a ptr) and do allyesconfig builds (with 'make -k') . Using coccinelle directly would make the changes for you. It's fairly hard to understand and use in my limited experience. Also keep in mind the error could get passed out of a function and then checked elsewhere. That you can't really automate checking. Searching, that seems to be fairly common, but I would guess most cases are just comparing to 0 if they check. This is what I used: git grep -W '=\spci_read_config_' You could then grep/sed the result of this to get the functions, and then grep using those functions to check the callers. I also see several cases checking for < 0 already, so we'd actually be fixing those. :) > > 3. "Make the config accessor defines convert positive error codes to Linux error codes" > > Do you mean something like: > > #define PCI_OP_READ(size, type, len) \ > int noinline pci_bus_read_config_##size \ > (struct pci_bus *bus, unsigned int devfn, int pos, type *value) > \ > { > if (PCI_##size##_BAD) return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER); > ... > ... > return pcibios_err_to_errno(res); Right. > > 4. "Convert pci_ops implementations to Linux error codes one by one" > > Finally, remove all the PCIBIOS_* references from the pci_ops > implementation of various drivers. Right. > > > 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 > > Rob, When you say this do you mean - we have something like: > > #define PCI_OPS_READ() > res = bus->ops->read(); > if (res != PCIBIOS_SUCCESSFUL) if (res) > SET_PCI_ERROR_RESPONSE(val); I still don't like that style, but when there's only 2 occurrences, I don't really care. > And the pci_ops implementation would look like: > > pci_generic_config_read() > { > addr = bus->ops->map_bus(); > if (!addr) > return PCIBIOS_DEVICE_NOT_FOUND; > } > > This way the controller/drivers does not have to bother fabricating the > ~0 value, all they have to do when they detect any error is return the > error. And the PCI_OP_READ and PCI_USER_READ_CONFIG will set the ~0 > value for "val". Right. > > Pali, would you have concerns with the above design? > > > > > 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. > > > > Yes, this makes sense. I can send a patch for this first and then start > working on the PCIBIOS_* project. If anybody has any objection please do > let me know. > > Thanks for the comment, it cleared up a lot of my doubts ^^ Sure, thanks for working on this. Rob
On Wednesday 13 October 2021 16:47:43 Rob Herring wrote: > On Wed, Oct 13, 2021 at 12:17 PM Naveen Naidu <naveennaidu479@gmail.com> wrote: > > The thread does bring up a good point, about not returning any error > > values in pci_read_config_*() and converting the function definition to > > something like > > > > void pci_read_config_word(struct pci_dev *dev, int where, u16 *val) > > > > The reason stated in the thread was that, the error values returned from > > these functions are either ignored or are not used properly. And > > whenever an error occurs, the error value ~0 is anyway stored in val, we > > could use that to test errors. > > Presumably, there could be some register somewhere where all 1s is > valid? So I think we need the error values. I guess that "Prefetchable Base/Limit Upper 32 Bits" PCI registers can contains all-ones value and it is valid value in these registers. And also PCIe regs like "Slot Capabilities Register" can also have all bits set. So 0xffffffff does not mean that error happened. It is needed some application logic which can decide based on other things (like register number, device state, etc...) if 0xffffffff indicates error or not. Therefore return errno values can help, but only for controllers which provide this additional errno information. > Also, I seem to recall only the vendor/device IDs are defined to be > all 1s for non-existent devices. Other errors are undefined? In PCIe spec for vendor id register is mentioned that 0xffff indicates no Function is present.
On Wed, Oct 13, 2021 at 04:47:43PM -0500, Rob Herring wrote: > Presumably, there could be some register somewhere where all 1s is > valid? So I think we need the error values. We have to assume ~0 is a valid value for any config registers except the few defined by the spec that have bits required to be 0. There can be all kinds of vendor-defined stuff in config space that can be anything. > Also, I seem to recall only the vendor/device IDs are defined to be > all 1s for non-existent devices. Other errors are undefined? I think this case is actually an instance of the PCI controller fabricating ~0 because a PCI/PCIe error occurred (I think on PCI it's a Master Abort when nothing responds; on PCIe the read terminates as an Unsupported Request (PCIe r5.0, sec 2.3.2)).
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); return PCIBIOS_DEVICE_NOT_FOUND; } @@ -123,7 +123,7 @@ int pci_generic_config_read32(struct pci_bus *bus, unsigned int devfn, addr = bus->ops->map_bus(bus, devfn, where & ~0x3); if (!addr) { - *val = ~0; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_DEVICE_NOT_FOUND; } @@ -411,10 +411,10 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val) ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val); /* * Reset *val to 0 if pci_read_config_word() fails, it may - * have been written as 0xFFFF if hardware error happens - * during pci_read_config_word(). + * have been written as 0xFFFF (PCI_ERROR_RESPONSE) if hardware error + * happens during pci_read_config_word(). */ - if (ret) + if (RESPONSE_IS_PCI_ERROR(val)) *val = 0; return ret; } @@ -446,10 +446,10 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val) ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val); /* * Reset *val to 0 if pci_read_config_dword() fails, it may - * have been written as 0xFFFFFFFF if hardware error happens - * during pci_read_config_dword(). + * have been written as 0xFFFFFFFF (PCI_ERROR_RESPONSE) if hardware + * error happens during pci_read_config_dword(). */ - if (ret) + if (RESPONSE_IS_PCI_ERROR(val)) *val = 0; return ret; } @@ -523,7 +523,7 @@ EXPORT_SYMBOL(pcie_capability_clear_and_set_dword); int pci_read_config_byte(const struct pci_dev *dev, int where, u8 *val) { if (pci_dev_is_disconnected(dev)) { - *val = ~0; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_DEVICE_NOT_FOUND; } return pci_bus_read_config_byte(dev->bus, dev->devfn, where, val); @@ -533,7 +533,7 @@ EXPORT_SYMBOL(pci_read_config_byte); int pci_read_config_word(const struct pci_dev *dev, int where, u16 *val) { if (pci_dev_is_disconnected(dev)) { - *val = ~0; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_DEVICE_NOT_FOUND; } return pci_bus_read_config_word(dev->bus, dev->devfn, where, val); @@ -544,7 +544,7 @@ int pci_read_config_dword(const struct pci_dev *dev, int where, u32 *val) { if (pci_dev_is_disconnected(dev)) { - *val = ~0; + SET_PCI_ERROR_RESPONSE(val); return PCIBIOS_DEVICE_NOT_FOUND; } return pci_bus_read_config_dword(dev->bus, dev->devfn, where, val);
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(-)