Re: [PATCH v2] Replace -EINVAL with a positive error number

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Apr 10, 2020 at 07:07:13PM +0200, Bolarinwa Olayemi Saheed wrote:

Hi Saheed,

Thanks for the patch!  This is a tricky area and we'll have to proceed
carefully.  A few procedural nits:

1) This is labeled v2, but it looks the same as the first posting.  If
it *is* the same, there's no need to repost it.  You can check
https://lore.kernel.org/linux-pci/ to see if it made it to the mailing
list.

If v2 is different from v1, please include a note about what changed
since v1.

2) This needs a commit log, even if it repeats the subject.  The
subject is the title, the log itself is the body, and we need both.

It should include the rationale for the change, e.g., in this case
we're trying to make the return values of the pcie_capability_*()
accessors consistent with the plain pci_*config*() accessors.

3) The subject doesn't make sense: -EINVAL doesn't appear in the patch
at all.  And pcibios_err_to_errno() always returns a *negative*
number, not a positive number.  But most importantly, it's a little
too low-level -- it needs to say something about the purpose of the
patch.

4) This patch appears to be made to apply on top of your original
patch [1], since it expects:

>  	if (pos & 1)
> -		return PCIBIOS_BAD_REGISTER_NUMBER;

But a revised patch (v2, v3, etc) doesn't get added on top of previous
versions; it completely *replaces* previous versions.  In general,
patches you post should apply cleanly on my "master" branch [2], which
generally is the -rc1 tag.

And finally, the most important and tricky part:

5) We need some indication that this change is safe for all callers,
so we need to audit them all and either include a note that no changes
are needed, or include the changes in this patch so everything that
worked before this patch also works after this patch.

We're not trying to change any behavior (unless we trip over a bug
when auditing the callers).

The current situation is that pcie_capability_*() accessors can return
0, -EINVAL, or PCIBIOS_FUNC_NOT_SUPPORTED, etc (PCIBIOS_* errors are
all positive).  That makes it harder than it should be for callers to
check for errors.

[1] https://lore.kernel.org/r/20200409161609.2034-1-refactormyself@gmail.com
[2] https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=master

> Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
> ---
>  drivers/pci/access.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 451f2b8b2b3c..d5460eb92c12 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -409,7 +409,7 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val)
>  
>  	*val = 0;
>  	if (pos & 1)
> -		return PCIBIOS_BAD_REGISTER_NUMBER;
> +		return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER);
>  
>  	if (pcie_capability_reg_implemented(dev, pos)) {
>  		ret = pci_read_config_word(dev, pci_pcie_cap(dev) + pos, val);
> @@ -444,7 +444,7 @@ int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val)
>  
>  	*val = 0;
>  	if (pos & 3)
> -		return PCIBIOS_BAD_REGISTER_NUMBER;
> +		return pcibios_err_to_errno(PCIBIOS_BAD_REGISTER_NUMBER);
>  
>  	if (pcie_capability_reg_implemented(dev, pos)) {
>  		ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
> -- 
> 2.17.1
>