Re: Linux Kernel Mentorship Summer 2020

[Bjorn Helgaas <helgaas@kernel.org>]

[+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.