Re: Linux Kernel Mentorship Summer 2020

[Bjorn Helgaas <helgaas@kernel.org>]

1) Your first couple emails were multi-part MIME messages, which do
not make it to the mailing lists reliably [1].  It looks like maybe
they were sent via Gmail, which I'm sorry to say is not very convenient
in this respect.  There should be a "plain text" option in the three
dots menu next to the trash can in the draft window, which helps a
little bit.

2) Please use "interleaved" style [2] when responding, so the
conversation is easier to follow.  Gmail again makes this more of a
hassle than it should be.

3) The "**val = 0*" below is confusing; I think the surrounding
asterisks were unhelpfully added by Gmail to indicate italicization.
If you just use plain text that should help.

On Thu, Mar 19, 2020 at 04:33:34PM +0100, Saheed Bolarinwa wrote:
> 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.

4) I'm not sure we should be returning -EINVAL from
pcie_capability_read_*().  Those functions can also return errors from
pci_read_config_*(), which are generally PCIBIOS_SUCCESSFUL (0) or a
PCIBIOS_* error (a positive integer).  It seems sort of weird to
return either a negative errno or a positive PCIBIOS* number on error.

Other similar cases, e.g., bonito64_pcibios_read(), return
PCIBIOS_BAD_REGISTER_NUMBER if the address isn't aligned correctly.

I suggest you do some research and see whether anything would break if
we made pcie_capability_read_*() return PCIBIOS_BAD_REGISTER_NUMBER in
that case.  If not, that could be its own preliminary patch.

5) Then there are two other questions.  The first is whether we should
set *val = 0 before returning -EINVAL/PCIBIOS_BAD_REGISTER_NUMBER.
This situation is a programming error (the caller should know to use
the correct alignment).  I looked at other config accessors that
return PCIBIOS_BAD_REGISTER_NUMBER; some of them set *val = ~0
(ixp4xx_pci_read_config(), msp_pcibios_read_config_word()), but most
do not (bonito64_pcibios_read(), loongson_pcibios_read(),
msc_pcibios_read(), nile4_pcibios_read(), etc).

Again, this will require some research to make sure we don't break
anything, but I suspect we may be able to leave *val untouched when
returning error due to unaligned address.  This should be a separate
patch.

6) The second question is whether pcie_capability_read_*() should set
*val = 0 before passing on an error from pci_read_config_word().  This
is the case I originally envisioned for this project, and my thought
was that it should not set *val = 0, and it should just leave *val as
whatever pci_read_config_word() set it to.

My reasoning is that callers of pci_read_config_word() really need to
check for "val == ~0" if they care about detecting errors, and it
probably should be the same for pcie_capability_read_*().

This also requires research to see how it would affect callers.  Many
callers check for success and never look at *val if
pcie_capability_read_*() fails; those are easy.  Some callers consume
the data (*val) without checking for success -- those are the
important ones to look at.

I think the first step here is to *identify* those callers and analyze
whether they need to check for failure and how they should do it.

[1] http://vger.kernel.org/majordomo-info.html
[2] https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

> On Wed, Mar 18, 2020 at 4:04 PM Bjorn Helgaas <helgaas@kernel.org> 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.
> >