Re: [PATCH RFC] PCI: Set PCIBIOS_* error values to generic error number

Re: [PATCH RFC] PCI: Set PCIBIOS_* error values to generic error number

[Saheed Bolarinwa]

Hello Bjorn,

On 5/5/20 11:04 PM, Bjorn Helgaas wrote:
> On Tue, May 05, 2020 at 07:15:13PM +0200, refactormyself@gmail.com wrote:
>> From: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
>>
>> PCIe capability accessors return 0 on success, otherwise it could return
>> either a negative or positive error value. Positive error values are from
>> PCIBIOS_* error values. This behaviour contradicts that of PCI config.
>> accessors which only returns PCIBIOS_* error values.
>>
>> There is no caller of these accessors that directly utilize the returned
>> positive error values. They either use pcibios_err_to_errno() to convert
>> it to a generic error value or simply pass it on or in some cases discard
>> any positive error value.
> We *do* still need to check all the callers to see that they do the
> right thing.  The intent of a patch like this one is that it causes no
> functional change -- everything should work exactly the same before
> and after the patch.
>
> For example, the case we talked about earlier in this chain:
>
>    local_pci_probe
>      pci_drv->probe(..)
>        init_one                        # hfi1_pci_driver.probe method
>          hfi1_init_dd
>            pcie_speeds
>              pcie_capability_read_dword
>
> Before this patch, pcie_capability_read_dword() could return:
>
>    - 0 (success)
>    - a negative value (e.g., -EINVAL)
>    - a positive value (e.g., PCIBIOS_BAD_REGISTER_NUMBER (0x87))
>
> The positive return value would cause local_pci_probe() to warn that
> "Driver probe function unexpectedly returned %d".
>
> After this patch, pcie_capability_read_dword() will never return a
> positive value because PCIBIOS_BAD_REGISTER_NUMBER is now -EFAULT, and
> local_pci_probe() will no longer warn.
>
> In this case, that's a bug fix, but we don't want bugs to be silently,
> magically fixed by patches that don't seem to be connected to the
> problem.
>
> So we need to account for that somehow.  We could:
>
>    - Add a preparatory patch that calls pcibios_err_to_errno() from
>      pcie_speeds() as I mentioned before.  Of course, we plan to remove
>      that soon anyway.
>
>    - Maybe call pcibios_err_to_errno() inside
>      pcie_capability_read_dword()?
>
> We really need to identify all places that have this problem so we can
> see which way makes more sense.
>
> Can you start posting these to linux-pci so they're visible on the
> mailing list?  Sometimes other folks have great ideas, like Yicong's.
>
I will start working on it.

Thank you.

Saheed