[PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Bolarinwa Olayemi Saheed]

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);
-- 
2.17.1

Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Yicong Yang]

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?

Regards,
Yicong


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);

Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Saheed Bolarinwa]

On 4/10/20 3:28 AM, 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?

Hello Yicong,

Thank you for pointing out that function to me. I have resent the patch.

Saheed


> Regards,
> Yicong
>
> 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);

Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Bjorn Helgaas]

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);
> 

Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Yicong Yang]

Hi Bjorn and Saheed,

Callers use return value(most callers even don't check) of
pcie_capability_{read,write}_*() I found lists below. some
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

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

Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Saheed Bolarinwa]

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

Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER

[Bjorn Helgaas]

On Sun, Apr 19, 2020 at 07:56:22AM +0200, Saheed Bolarinwa wrote:
> 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_*()?

Yes.  pcie_capability_write_*() can return either -EINVAL or
PCIBIOS_*, so it has the same consistency problem.  I'd like to fix
all of pcie_capability_read_*() and pcie_capability_write_*() in a
single patch because it's logically the same change and they can all
be reviewed together.

Thanks for the audit of the callers.  I'd include a brief summary in
the commit log, but not the entire thing because it's a little too
detailed for the log.  But it could be useful in a cover letter (which
will be acccessible from the commit via the "Link:
https://lore.kernel.org/r/..." URL).

Bjorn