Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Saheed Bolarinwa <refactormyself@gmail.com>]

Hello Bjorn and Yicong

On 4/11/20 4:10 AM, Yicong Yang wrote:
> Hi Bjorn and Saheed,
>
> Callers use return value(most callers even don't check) of
> pcie_capability_{read,write}_*() I found lists below. some

In this patch I have focused only on pcie_capability_read_{word & dword}()

Do you think the consistency issue is of concern also with 
pcie_capability_write_*()?

> may directly print them to dmesg, others return the error
> codes to its caller. I think we should do the conversion in
> both condition.
>
> - pcie_speeds() in drivers/infiniband/hw/hfi1/pcie.c, line 306
> - amd_ntb_get_link_status() in drivers/ntb/hw/amd/ntb_hw_amd.c, line 216, line 233

Thank you Yicong for helping me with this. I have also search for 
functions that

either persist the return value of pcie_capability_read_*() or return it 
directly.

I have sent a RFC Patch with a report of my audit of the functions I 
found. I will

appreciate your comment.

- Saheed


> the probably change may look like:
>
>      ret = pcie_capability_{read, write}_*();
>      if (ret)
>          return pcibios_err_to_errno(ret);
>
> However, pci_{read, write}_config*() also have such problem, as they are also
> used widely outside pci driver and these drivers don't do the conversion. for example
> in arch/x86/platform/intel/iosf_mbi.c, iosf_mbi_pci_read_mdr() at line 39:
>
>      result = pci_read_config_dword();
>      if (result < 0)
>          goto fail_read;
>
> Seems it'll nevet get a failure result. Perhaps another patch is needed to solve these issues.
>
> AS PCIBIOS_* error code canbe *equivalent* to generic error code, why can't we
> directly use the generic ones? Considering of compatibility, maybe possible
> change will be like:
>
>      - #define PCIBIOS_FUNC_NOT_SUPPORTED 0X81
>      + #define PCIBIOS_FUNC_NOT_SUPPORTED -ENOENT
>      ......
>
> and pcibios_err_to_errno() is not neccessary any more.
>
> I don't know why we didn't use generic error code and define positive private errors.
> Please tell me if there is any background.
>
> Regards,
> Yicong
>
>
> On 2020/4/11 4:22, Bjorn Helgaas wrote:
>> On Fri, Apr 10, 2020 at 09:28:07AM +0800, Yicong Yang wrote:
>>> Hi Bolarinwa,
>>>
>>> I notice some drivers use these functions and if there is an error,
>>> pass the error code directly to the userspace. As it's our private
>>> error code, is it appropriate to pass or should we call
>>> pcibios_err_to_errno()(include/linux/pci.h, line 672) to do the
>>> conversion?
>> The whole point of this is to make the return values of the
>> pcie_capability_{read,write,etc}*() functions work the same as
>> the pci_{read,write}_config*() functions.
>>
>> The latter return PCIBIOS_* error codes, so the former should as well.
>>
>> When we do this, we do need to audit every caller of the
>> pcie_capability_{read,write}*() functions to make sure we don't break
>> them.  If some callers pass the error code directly to userspace, they
>> may need some change.
>>
>> Yicong, can you point to the ones you noticed so Saheed can check them
>> out?
>>
>> Bjorn
>>
>>> On 2020/4/10 0:16, Bolarinwa Olayemi Saheed wrote:
>>>> Signed-off-by: Bolarinwa Olayemi Saheed <refactormyself@gmail.com>
>>>> Suggested-by: Bjorn Helgaas <bjorn@helgaas.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 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);
>> .
>>