[PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[Bolarinwa Olayemi Saheed]

pcie_capability_read*() could return 0, -EINVAL, or any of the
PCIBIOS_* error codes (which are positive).
This is behaviour is now changed to return only PCIBIOS_* error
codes on error.
This is consistent with pci_read_config_*(). Callers can now have
a consitent way for checking which error has occured.

An audit of the callers of this function was made and no case was found
where there is need for a change within the caller function or their 
dependencies down the heirarchy.
Out of all caller functions discovered only 8 functions either persist the
return value of pcie_capability_read*() or directly pass on the return 
value.

1.) "./drivers/infiniband/hw/hfi1/pcie.c" :
=> pcie_speeds() line-306

	if (ret) {
		dd_dev_err(dd, "Unable to read from PCI config\n");
		return ret;
	}

remarks: The variable "ret" is the captured return value.
         This function passes on the return value. The return value was
	 store only by hfi1_init_dd() line-15076 in 
         ./drivers/infiniband/hw/hfi1/chip.c and it behave the same on all
	 errors. So this patch will not require a change in this function.

=> update_lbus_info() line-276

	if (ret) {
		dd_dev_err(dd, "Unable to read from PCI config\n");
		return;
	}

remarks: see below
=> save_pci_variables() line-415, 420, 425

	if (ret)
		goto error;

remarks: see below

=> tune_pcie_caps() line-471

	if ((!ret) && !(ectl & PCI_EXP_DEVCTL_EXT_TAG)) {
		dd_dev_info(dd, "Enabling PCIe extended tags\n");
		ectl |= PCI_EXP_DEVCTL_EXT_TAG;
		ret = pcie_capability_write_word(dd->pcidev,
						 PCI_EXP_DEVCTL, ectl);
		if (ret)
		     dd_dev_info(dd, "Unable to write to PCI config\n");
	}

remarks: see below

=> do_pcie_gen3_transition() line-1247, 1274

	if (ret) {
		dd_dev_err(dd, "Unable to read from PCI config\n");
		return_error = 1;
		goto done;
	}

remarks: The variable "ret" is the captured return value.
         These functions' behaviour is the same on all errors, so they are
	 not be affected by this patch.

2.) "./drivers/net/wireless/realtek/rtw88/pci.c":
=> rtw_pci_link_cfg*() line-1221

	if (ret) {
		rtw_err(rtwdev, "failed to read PCI cap, ret=%d\n", ret);
		return;
	}

remark: The variable "ret" is the captured return value.
        This function returns on all errors, so it will not be affected
	by this patch.

3.) "./drivers/pci/hotplug/pciehp_hpc.c":
=> pciehp_check_link_active() line-219

	if (ret == PCIBIOS_DEVICE_NOT_FOUND || lnk_status == (u16)~0)
		return -ENODEV;

remark: see below

=>pciehp_card_present() line-404

	{Same code as above}

remark: The variable "ret" is the captured return value.
        This 2 functions will not be affected by this patch since they are
	only testing for *DEVICE_NOT_FOUND error.

4.) "./drivers/pci/pcie/bw_notification.c":
=>pcie_link_bandwidth_notification_supported() line-26

	return (ret == PCIBIOS_SUCCESSFUL) 
			&& (lnk_cap & PCI_EXP_LNKCAP_LBNC);

remark: see below

=>pcie_bw_notification_irq() line-56

	if (ret != PCIBIOS_SUCCESSFUL || !events)
		return IRQ_NONE;

remark: The variable "ret" is the captured return value.
        In these 2 functions returning PCIBIOS_BAD_REGISTER_NUMBER instead
	of -EINVAL as done in this patch will not affect the behaviour.

5.) "./drivers/pci/probe.c":
=> pci_configure_extended_tags() line-1951

	if (ret)
		return 0;

remark: The variable "ret" is the captured return value.
        This function will not be affected by this patch since it retuns 0
	on ALL error codes.

6.) "./drivers/pci/pci-sysfs.c":
=> current_link_speed_show() line-180

	if (err)
		return -EINVAL;

remark: see below

=>current_link_width_show() line-215

        {same code as in the above function}

remark: The variable "err" is the captured return value.
        This 2 functions will not be affected directly by this patch since
	it retuns -EINVAL on ALL error codes. However, depending on the 
	intent, after this patch, this may now be to too generic. This is 
	because it will then be possible to use the returned PCIBIOS_* 
	error code to identify the error.

7.) "./drivers/dma/ioat/init.c":
=>ioat3_dma_probe() line-1193

	if (err)
		return err;

remark: The variable "ret" is the captured return value.
        This function passes on the return value. Only ioat_pci_probe()
	line-1392 in the same file persists the return value and it's
	behaviour is the same on all errors. So this patch will not change
	the behaviour of these functions.

8.) "./drivers/pci/access.c":
=>pcie_capability_clear_and_set_word() line-493

	if (!ret) {
		val &= ~clear;
		val |= set;
		ret = pcie_capability_write_word(dev, pos, val);
	}

	return ret;

=>pcie_capability_clear_and_set_dword() line-508

    {same as above function}

remark: The variable "ret" is the captured return value.
     This 2 functions will not be affected directly. But after this patch
     they will now be returning PCIBIOS_BAD_REGISTER_NUMBER instead of 
     -EINVAL. No case was found where the return value of any of both 
     functions were persisted. But their return values are passed on
     directly by:
	- pcie_capability_set_{word|dword}() and 
	  pcie_capability_clear_{word|dword}() in ./include/linux/pci.h
          lines(1100-1136): these pass on the recieved return values 
	  directly to:
          - pcie_capability_clear_dword() is not referenced anywhere
          - pcie_capability_set_dword() is referenced but it's return 
	    values are not cached
          - pcie_capability_{set, clear}_word() : return value passed on 
	    by pci_enable_pcie_error_reporting() in drivers/pci/pcie/aer.c
	    lines-(350,362) these are used by other drivers to log errors,
            in all examined cases all errors are treated the same.
	  - pcie_capability_clear_word() : return value passed on by 
	    pci_disable_pcie_error_reporting() in /drivers/pci/pcie/aer.c 
	    lines-362 which treats all errors are treated the same.

	- pcie_set_readrq() line-5662 and pcie_set_mps() line-5703 in 
	  ./drivers/pci/pci.c : both functions pass on the return value of
	  pcie_capability_clear_and_set_word() but also return -EINVAL in
	  cases of some other errors. Currently these errors will not be 
	  differentiated, this patch will help differentiate errors in 
	  this kind of situation. The function will not be affected but
	  rather it will be enhanced in correctness.

        - pqi_set_pcie_completion_timeout() line-7423 in 
	  ./drivers/scsi/smartpqi/smartpqi_init.c : This function will not
          be affected. Although, it passes on the return value, all error 
          values are handled the same way by the only reference found at 
          line-7473 in the same file.

          
 
Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
---
Changed in version 4:
 - make patch independent of earlier versions
 - add commit log
 - add justificaation and report on audit of affected functions

NOTE:
 Please let me know if I have missed some possible callers
 I am not sure if pcie_capability_write*() needs this kind of fix

 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 79c4a2ef269a..451f2b8b2b3c 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 -EINVAL;
+		return 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 -EINVAL;
+		return 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

Re: [PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[Yicong Yang]

Hi Bolarinwa,

Please +cc me in the following patches,  and find some minor comments below.

On 2020/4/19 14:51, Bolarinwa Olayemi Saheed wrote:
> pcie_capability_read*() could return 0, -EINVAL, or any of the
> PCIBIOS_* error codes (which are positive).
> This is behaviour is now changed to return only PCIBIOS_* error
> codes on error.
> This is consistent with pci_read_config_*(). Callers can now have
> a consitent way for checking which error has occured.
>
> An audit of the callers of this function was made and no case was found
> where there is need for a change within the caller function or their 
> dependencies down the heirarchy.
> Out of all caller functions discovered only 8 functions either persist the
> return value of pcie_capability_read*() or directly pass on the return 
> value.
>
> 1.) "./drivers/infiniband/hw/hfi1/pcie.c" :
> => pcie_speeds() line-306
>
> 	if (ret) {
> 		dd_dev_err(dd, "Unable to read from PCI config\n");
> 		return ret;
> 	}
>
> remarks: The variable "ret" is the captured return value.
>          This function passes on the return value. The return value was
> 	 store only by hfi1_init_dd() line-15076 in 
>          ./drivers/infiniband/hw/hfi1/chip.c and it behave the same on all
> 	 errors. So this patch will not require a change in this function.
>
> => update_lbus_info() line-276
>
> 	if (ret) {
> 		dd_dev_err(dd, "Unable to read from PCI config\n");
> 		return;
> 	}
>
> remarks: see below
> => save_pci_variables() line-415, 420, 425
>
> 	if (ret)
> 		goto error;
>
> remarks: see below
>
> => tune_pcie_caps() line-471
>
> 	if ((!ret) && !(ectl & PCI_EXP_DEVCTL_EXT_TAG)) {
> 		dd_dev_info(dd, "Enabling PCIe extended tags\n");
> 		ectl |= PCI_EXP_DEVCTL_EXT_TAG;
> 		ret = pcie_capability_write_word(dd->pcidev,
> 						 PCI_EXP_DEVCTL, ectl);
> 		if (ret)
> 		     dd_dev_info(dd, "Unable to write to PCI config\n");
> 	}
>
> remarks: see below
>
> => do_pcie_gen3_transition() line-1247, 1274
>
> 	if (ret) {
> 		dd_dev_err(dd, "Unable to read from PCI config\n");
> 		return_error = 1;
> 		goto done;
> 	}
>
> remarks: The variable "ret" is the captured return value.
>          These functions' behaviour is the same on all errors, so they are
> 	 not be affected by this patch.
>
> 2.) "./drivers/net/wireless/realtek/rtw88/pci.c":
> => rtw_pci_link_cfg*() line-1221
>
> 	if (ret) {
> 		rtw_err(rtwdev, "failed to read PCI cap, ret=%d\n", ret);
> 		return;
> 	}
>
> remark: The variable "ret" is the captured return value.
>         This function returns on all errors, so it will not be affected
> 	by this patch.
>
> 3.) "./drivers/pci/hotplug/pciehp_hpc.c":
> => pciehp_check_link_active() line-219
>
> 	if (ret == PCIBIOS_DEVICE_NOT_FOUND || lnk_status == (u16)~0)
> 		return -ENODEV;
>
> remark: see below
>
> =>pciehp_card_present() line-404
>
> 	{Same code as above}
>
> remark: The variable "ret" is the captured return value.
>         This 2 functions will not be affected by this patch since they are
> 	only testing for *DEVICE_NOT_FOUND error.
>
> 4.) "./drivers/pci/pcie/bw_notification.c":
> =>pcie_link_bandwidth_notification_supported() line-26
>
> 	return (ret == PCIBIOS_SUCCESSFUL) 
> 			&& (lnk_cap & PCI_EXP_LNKCAP_LBNC);
>
> remark: see below
>
> =>pcie_bw_notification_irq() line-56
>
> 	if (ret != PCIBIOS_SUCCESSFUL || !events)
> 		return IRQ_NONE;
>
> remark: The variable "ret" is the captured return value.
>         In these 2 functions returning PCIBIOS_BAD_REGISTER_NUMBER instead
> 	of -EINVAL as done in this patch will not affect the behaviour.
>
> 5.) "./drivers/pci/probe.c":
> => pci_configure_extended_tags() line-1951
>
> 	if (ret)
> 		return 0;
>
> remark: The variable "ret" is the captured return value.
>         This function will not be affected by this patch since it retuns 0
> 	on ALL error codes.
>
> 6.) "./drivers/pci/pci-sysfs.c":
> => current_link_speed_show() line-180
>
> 	if (err)
> 		return -EINVAL;
>
> remark: see below
>
> =>current_link_width_show() line-215
>
>         {same code as in the above function}
>
> remark: The variable "err" is the captured return value.
>         This 2 functions will not be affected directly by this patch since
> 	it retuns -EINVAL on ALL error codes. However, depending on the 
> 	intent, after this patch, this may now be to too generic. This is 
> 	because it will then be possible to use the returned PCIBIOS_* 
> 	error code to identify the error.
>
> 7.) "./drivers/dma/ioat/init.c":
> =>ioat3_dma_probe() line-1193
>
> 	if (err)
> 		return err;
>
> remark: The variable "ret" is the captured return value.
>         This function passes on the return value. Only ioat_pci_probe()
> 	line-1392 in the same file persists the return value and it's
> 	behaviour is the same on all errors. So this patch will not change
> 	the behaviour of these functions.
>
> 8.) "./drivers/pci/access.c":
> =>pcie_capability_clear_and_set_word() line-493
>
> 	if (!ret) {
> 		val &= ~clear;
> 		val |= set;
> 		ret = pcie_capability_write_word(dev, pos, val);
> 	}
>
> 	return ret;
>
> =>pcie_capability_clear_and_set_dword() line-508
>
>     {same as above function}
>
> remark: The variable "ret" is the captured return value.
>      This 2 functions will not be affected directly. But after this patch
>      they will now be returning PCIBIOS_BAD_REGISTER_NUMBER instead of 
>      -EINVAL. No case was found where the return value of any of both 
>      functions were persisted. But their return values are passed on
>      directly by:
> 	- pcie_capability_set_{word|dword}() and 
> 	  pcie_capability_clear_{word|dword}() in ./include/linux/pci.h
>           lines(1100-1136): these pass on the recieved return values 
> 	  directly to:
>           - pcie_capability_clear_dword() is not referenced anywhere
>           - pcie_capability_set_dword() is referenced but it's return 
> 	    values are not cached
>           - pcie_capability_{set, clear}_word() : return value passed on 
> 	    by pci_enable_pcie_error_reporting() in drivers/pci/pcie/aer.c
> 	    lines-(350,362) these are used by other drivers to log errors,
>             in all examined cases all errors are treated the same.
> 	  - pcie_capability_clear_word() : return value passed on by 
> 	    pci_disable_pcie_error_reporting() in /drivers/pci/pcie/aer.c 
> 	    lines-362 which treats all errors are treated the same.
>
> 	- pcie_set_readrq() line-5662 and pcie_set_mps() line-5703 in 
> 	  ./drivers/pci/pci.c : both functions pass on the return value of
> 	  pcie_capability_clear_and_set_word() but also return -EINVAL in
> 	  cases of some other errors. Currently these errors will not be 
> 	  differentiated, this patch will help differentiate errors in 
> 	  this kind of situation. The function will not be affected but
> 	  rather it will be enhanced in correctness.
>
>         - pqi_set_pcie_completion_timeout() line-7423 in 
> 	  ./drivers/scsi/smartpqi/smartpqi_init.c : This function will not
>           be affected. Although, it passes on the return value, all error 
>           values are handled the same way by the only reference found at 
>           line-7473 in the same file.
>
>           
>  
> Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
> Suggested-by: Bjorn Helgaas <bjorn@helgaas.com>
> ---
> Changed in version 4:
>  - make patch independent of earlier versions
>  - add commit log
>  - add justificaation and report on audit of affected functions
>
> NOTE:
>  Please let me know if I have missed some possible callers
>  I am not sure if pcie_capability_write*() needs this kind of fix
>
>  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 79c4a2ef269a..451f2b8b2b3c 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)

Maybe provide some comments for the function, to notify the outside users to do the error code
conversion.

BTW, pci_{read, write}_config_*() may also have the issues that export the private err code
outside. You may want to solve these in a series along with this patch.

Regards,
Yicong


>  
>  	*val = 0;
>  	if (pos & 1)
> -		return -EINVAL;
> +		return 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 -EINVAL;
> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>  
>  	if (pcie_capability_reg_implemented(dev, pos)) {
>  		ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);

Re: [PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[Bjorn Helgaas]

On Thu, Apr 23, 2020 at 07:55:17PM +0800, Yicong Yang wrote:
> On 2020/4/19 14:51, Bolarinwa Olayemi Saheed wrote:

> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 79c4a2ef269a..451f2b8b2b3c 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)
> 
> Maybe provide some comments for the function, to notify the outside
> users to do the error code conversion.

A short function comment about the set of possible return values
wouldn't hurt.  We don't have those for pci_read_config_word() and
friends, and there are several pcie_capability_*() functions.  I don't
think it's worth repeating the comment for every function, so maybe we
could just extend the existing comment at pcie_capability_read_word().

> BTW, pci_{read, write}_config_*() may also have the issues that
> export the private err code outside. You may want to solve these in
> a series along with this patch.

If you see a specific issue, please point it out.

I looked at pci_read_config_word(), and it can return
PCIBIOS_DEVICE_NOT_FOUND, PCIBIOS_BAD_REGISTER_NUMBER, or the return
value from bus->ops->read().

I looked at all the users of PCIBIOS_*.  There's really no interesting
use of any of them except by pcibios_err_to_errno() and
xen_pcibios_err_to_errno(), so I'm not sure it's even worth keeping
them.

But I think it's probably more work to excise all of them than it is
to simply make pci_read_config_word() and pcie_capability_read_word()
return the same set of error values.  So I think we should do this
first.

> >  	*val = 0;
> >  	if (pos & 1)
> > -		return -EINVAL;
> > +		return 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 -EINVAL;
> > +		return PCIBIOS_BAD_REGISTER_NUMBER;
> >  
> >  	if (pcie_capability_reg_implemented(dev, pos)) {
> >  		ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
> 

Re: [PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[Saheed Bolarinwa]

On 4/24/20 12:38 AM, Bjorn Helgaas wrote:
> On Thu, Apr 23, 2020 at 07:55:17PM +0800, Yicong Yang wrote:
>> On 2020/4/19 14:51, Bolarinwa Olayemi Saheed wrote:
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index 79c4a2ef269a..451f2b8b2b3c 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)
>> Maybe provide some comments for the function, to notify the outside
>> users to do the error code conversion.
> A short function comment about the set of possible return values
> wouldn't hurt.  We don't have those for pci_read_config_word() and
> friends, and there are several pcie_capability_*() functions.  I don't
> think it's worth repeating the comment for every function, so maybe we
> could just extend the existing comment at pcie_capability_read_word().

Is enough to adjust the comment on pcie_capability_read_word() to this:

    /*
      * Note that these accessor functions are only for the "PCI Express
      * Capability" (see PCIe spec r3.0, sec 7.8).  They do not apply to the
      * other "PCI Express Extended Capabilities" (AER, VC, ACS, MFVC, etc.)
      *
      * On error, this function returns a PCIBIOS_* error code,
      * you may want to use pcibios_err_to_errno()(include/linux/pci.h)
      * to convert to a non-PCI code
      */

>> BTW, pci_{read, write}_config_*() may also have the issues that
>> export the private err code outside. You may want to solve these in
>> a series along with this patch.
> If you see a specific issue, please point it out.
>
> I looked at pci_read_config_word(), and it can return
> PCIBIOS_DEVICE_NOT_FOUND, PCIBIOS_BAD_REGISTER_NUMBER, or the return
> value from bus->ops->read().
>
> I looked at all the users of PCIBIOS_*.  There's really no interesting
> use of any of them except by pcibios_err_to_errno() and
> xen_pcibios_err_to_errno(), so I'm not sure it's even worth keeping
> them.
>
> But I think it's probably more work to excise all of them than it is
> to simply make pci_read_config_word() and pcie_capability_read_word()
> return the same set of error values.  So I think we should do this
> first.
>
>>>   	*val = 0;
>>>   	if (pos & 1)
>>> -		return -EINVAL;
>>> +		return 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 -EINVAL;
>>> +		return PCIBIOS_BAD_REGISTER_NUMBER;
>>>   
>>>   	if (pcie_capability_reg_implemented(dev, pos)) {
>>>   		ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);

Also, can I just send this as a single patch while we conclude on 
pcie_capability_write*() ?

Thank you.

- Saheed

Re: [PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[Yicong Yang]

On 2020/4/24 14:02, Saheed Bolarinwa wrote:
>
> On 4/24/20 12:38 AM, Bjorn Helgaas wrote:
>> On Thu, Apr 23, 2020 at 07:55:17PM +0800, Yicong Yang wrote:
>>> On 2020/4/19 14:51, Bolarinwa Olayemi Saheed wrote:
>>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>>> index 79c4a2ef269a..451f2b8b2b3c 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)
>>> Maybe provide some comments for the function, to notify the outside
>>> users to do the error code conversion.
>> A short function comment about the set of possible return values
>> wouldn't hurt.  We don't have those for pci_read_config_word() and
>> friends, and there are several pcie_capability_*() functions.  I don't
>> think it's worth repeating the comment for every function, so maybe we
>> could just extend the existing comment at pcie_capability_read_word().
>
> Is enough to adjust the comment on pcie_capability_read_word() to this:
>
>    /*
>      * Note that these accessor functions are only for the "PCI Express
>      * Capability" (see PCIe spec r3.0, sec 7.8).  They do not apply to the
>      * other "PCI Express Extended Capabilities" (AER, VC, ACS, MFVC, etc.)
>      *
>      * On error, this function returns a PCIBIOS_* error code,
>      * you may want to use pcibios_err_to_errno()(include/linux/pci.h)
>      * to convert to a non-PCI code
>      */

okay. Or we can provide kernel-doc as you wish.

>
>>> BTW, pci_{read, write}_config_*() may also have the issues that
>>> export the private err code outside. You may want to solve these in
>>> a series along with this patch.
>> If you see a specific issue, please point it out.

arch/x86/platform/intel/iosf_mbi.c, iosf_mbi_pci_read_mdr():
        result = pci_read_config_dword(mbi_pdev, MBI_MDR_OFFSET, mdr);
        if (result < 0)
            goto fail_read;
        return 0;
    fail_read:
        dev_err(&mbi_pdev->dev, "PCI config access failed with %d\n", result);
        return result;

Not sure if there is much in the kernel.

>>
>> I looked at pci_read_config_word(), and it can return
>> PCIBIOS_DEVICE_NOT_FOUND, PCIBIOS_BAD_REGISTER_NUMBER, or the return
>> value from bus->ops->read().
>>
>> I looked at all the users of PCIBIOS_*.  There's really no interesting
>> use of any of them except by pcibios_err_to_errno() and
>> xen_pcibios_err_to_errno(), so I'm not sure it's even worth keeping
>> them.

maybe we can mark them as deprecated. I can send a RFC one to do so.

>>
>> But I think it's probably more work to excise all of them than it is
>> to simply make pci_read_config_word() and pcie_capability_read_word()
>> return the same set of error values.  So I think we should do this
>> first.
>>

okay.

>>>>       *val = 0;
>>>>       if (pos & 1)
>>>> -        return -EINVAL;
>>>> +        return 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 -EINVAL;
>>>> +        return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>         if (pcie_capability_reg_implemented(dev, pos)) {
>>>>           ret = pci_read_config_dword(dev, pci_pcie_cap(dev) + pos, val);
>
> Also, can I just send this as a single patch while we conclude on pcie_capability_write*() ?

Sure. As Bjorn suggested.

Regards.
Yicong

>
> Thank you.
>
> - Saheed
>
> .
>

Re: [PATCH RFC] pci: Make return value of pcie_capability_read*() consistent

[Bjorn Helgaas]

On Fri, Apr 24, 2020 at 05:11:44PM +0800, Yicong Yang wrote:
> On 2020/4/24 14:02, Saheed Bolarinwa wrote:
> > On 4/24/20 12:38 AM, Bjorn Helgaas wrote:
> >> On Thu, Apr 23, 2020 at 07:55:17PM +0800, Yicong Yang wrote:

> >>> BTW, pci_{read, write}_config_*() may also have the issues that
> >>> export the private err code outside. You may want to solve these in
> >>> a series along with this patch.
> >>
> >> If you see a specific issue, please point it out.
> 
> arch/x86/platform/intel/iosf_mbi.c, iosf_mbi_pci_read_mdr():
>         result = pci_read_config_dword(mbi_pdev, MBI_MDR_OFFSET, mdr);
>         if (result < 0)
>             goto fail_read;
>         return 0;
>     fail_read:
>         dev_err(&mbi_pdev->dev, "PCI config access failed with %d\n", result);
>         return result;

This is a problem in the caller, not in pci_read_config*().  This
caller is definitely broken, but fixing it is material for other
patches, not the current effort to align pcie_capability_read*() and
pci_read_config*().

> >> I looked at pci_read_config_word(), and it can return
> >> PCIBIOS_DEVICE_NOT_FOUND, PCIBIOS_BAD_REGISTER_NUMBER, or the return
> >> value from bus->ops->read().
> >>
> >> I looked at all the users of PCIBIOS_*.  There's really no interesting
> >> use of any of them except by pcibios_err_to_errno() and
> >> xen_pcibios_err_to_errno(), so I'm not sure it's even worth keeping
> >> them.
> 
> maybe we can mark them as deprecated. I can send a RFC one to do so.

Let's put this on a list for later.  I want to make sure this first
effort is successful before throwing more stuff into the mix.

Bjorn