---------- Forwarded message --------- From: Saheed Bolarinwa <refactormyself@xxxxxxxxx> Date: Thu, Mar 19, 2020 at 4:33 PM Subject: Re: Linux Kernel Mentorship Summer 2020 To: Bjorn Helgaas <helgaas@xxxxxxxxxx> Cc: Bjorn Helgaas <bjorn@xxxxxxxxxxx>, Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>, <linux-pci@xxxxxxxxxxxxxxx> Hello, Thank you for the detailed explanation. So please just to clarify, I think it will be better to ONLY set *val = 0 when returning -EINVAL In this case, we can just return pci_read_config_word(), then there is nothing to reset. Then the remaining tasks will now be to deal with the references to pcie_capability_read_word(). - Saheed On Wed, Mar 18, 2020 at 4:04 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > [+cc linux-pci because there are lots of interesting wrinkles in this > issue -- as background, my idea for this project was to make > pcie_capability_read_word() return errors more like > pci_read_config_word() does. When a PCI error occurs, > pci_read_config_word() returns ~0 data, but > pcie_capability_read_word() return 0 data.] > > On Mon, Mar 16, 2020 at 02:03:57PM +0100, Saheed Bolarinwa wrote: > > I have checked the function the pcie_capability_read_word() that > > sets *val = 0 when pci_read_config_word() fails. > > > > I am still trying to get familiar to the code, I just wondering why > > the result of pci_read_config_word() is not being returned directly > > since *val is passed into it. > > pci_read_config_word() needs to return two things: > > 1) A success/failure indication. This is either 0 or an error like > PCIBIOS_DEVICE_NOT_FOUND, PCIBIOS_BAD_REGISTER_NUMBER, etc. This is > the function return value. > > 2) A 16-bit value read from PCI config space. This is what goes in > *val. > > pcie_capability_read_word() is similar. The idea of this project is > to change part 2, the value in *val. > > The reason is that sometimes the config read fails on PCI. This can > happen because the device has been physically removed, it's been > electrically isolated, or some other PCI error. When a config read > fails, the PCI host bridge generally returns ~0 data to satisfy the > CPU load instruction. > > The PCI core, i.e., pci_read_config_word() can't tell whether ~0 means > (a) the config read was successful and the register on the PCI card > happened to contain ~0, or (b) the config read failed because of a PCI > error. > > Only the driver knows whether ~0 is a valid value for a config space > register, so the driver has to be involved in looking for this error. > > My idea for this project is to make this checking more uniform between > pci_read_config_word() and pcie_capability_read_word(). > > For example, pci_read_config_word() sets *val = ~0 when it returns > PCIBIOS_DEVICE_NOT_FOUND. > > On the other hand, pcie_capability_read_word() sets *val = 0 when it > returns -EINVAL, PCIBIOS_DEVICE_NOT_FOUND, etc. > > > This is what is done inside pci_read_config_word() when > > pci_bus_read_config_word() is called. I tried to find the definition > > of pci_bus_read_config_word() but I couldn't find it. I am not > > sure I need it, though. > > pci_bus_read_config_word() and similar functions are defined by the > PCI_OP_READ() macro in drivers/pci/access.c. It's sort of annoying > because grep/cscope/etc don't find those functions very well. But > you're right; they aren't really relevant to this project. > > > I am also confused with the last statement of the Project > > description on the Project List > > <https://wiki.linuxfoundation.org/lkmp/lkmp_project_list> page. It > > says, "*This will require changes to some callers of > > pci_read_config_word() that assume the read returns 0 on error.*" I > > think the callers pcie_capability_read_word() are supposedly being > > referred to here. > > Yes, that's a typo. The idea is to change pcie_capability_read_*(), > so we'd probably need to change some callers to match.
