Re: Linux Kernel Mentorship Summer 2020

Re: Linux Kernel Mentorship Summer 2020

[Bjorn Helgaas]

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

Fwd: Linux Kernel Mentorship Summer 2020

[Saheed Bolarinwa]

---------- Forwarded message ---------
From: Saheed Bolarinwa <refactormyself@gmail.com>
Date: Thu, Mar 19, 2020 at 4:33 PM
Subject: Re: Linux Kernel Mentorship Summer 2020
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bjorn Helgaas <bjorn@helgaas.com>, Shuah Khan
<skhan@linuxfoundation.org>, <linux-pci@vger.kernel.org>


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

Re: Linux Kernel Mentorship Summer 2020

[Bjorn Helgaas]

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