linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
@ 2020-04-09 16:16 Bolarinwa Olayemi Saheed
  2020-04-10  1:28 ` Yicong Yang
  0 siblings, 1 reply; 7+ messages in thread
From: Bolarinwa Olayemi Saheed @ 2020-04-09 16:16 UTC (permalink / raw)
  To: helgaas; +Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
  2020-04-09 16:16 [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER Bolarinwa Olayemi Saheed
@ 2020-04-10  1:28 ` Yicong Yang
  2020-04-10 16:13   ` Saheed Bolarinwa
  2020-04-10 20:22   ` Bjorn Helgaas
  0 siblings, 2 replies; 7+ messages in thread
From: Yicong Yang @ 2020-04-10  1:28 UTC (permalink / raw)
  To: Bolarinwa Olayemi Saheed, helgaas; +Cc: bjorn, skhan, linux-pci

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
  2020-04-10  1:28 ` Yicong Yang
@ 2020-04-10 16:13   ` Saheed Bolarinwa
  2020-04-10 20:22   ` Bjorn Helgaas
  1 sibling, 0 replies; 7+ messages in thread
From: Saheed Bolarinwa @ 2020-04-10 16:13 UTC (permalink / raw)
  To: Yicong Yang, helgaas; +Cc: bjorn, skhan, linux-pci


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
  2020-04-10  1:28 ` Yicong Yang
  2020-04-10 16:13   ` Saheed Bolarinwa
@ 2020-04-10 20:22   ` Bjorn Helgaas
  2020-04-11  2:10     ` Yicong Yang
  1 sibling, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2020-04-10 20:22 UTC (permalink / raw)
  To: Yicong Yang; +Cc: Bolarinwa Olayemi Saheed, bjorn, skhan, linux-pci

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
  2020-04-10 20:22   ` Bjorn Helgaas
@ 2020-04-11  2:10     ` Yicong Yang
  2020-04-19  5:56       ` Saheed Bolarinwa
  0 siblings, 1 reply; 7+ messages in thread
From: Yicong Yang @ 2020-04-11  2:10 UTC (permalink / raw)
  To: Bjorn Helgaas, Bolarinwa Olayemi Saheed, bjorn; +Cc: skhan, linux-pci

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
  2020-04-11  2:10     ` Yicong Yang
@ 2020-04-19  5:56       ` Saheed Bolarinwa
  2020-04-24 15:17         ` Bjorn Helgaas
  0 siblings, 1 reply; 7+ messages in thread
From: Saheed Bolarinwa @ 2020-04-19  5:56 UTC (permalink / raw)
  To: Yicong Yang, Bjorn Helgaas, bjorn; +Cc: skhan, linux-pci

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER
  2020-04-19  5:56       ` Saheed Bolarinwa
@ 2020-04-24 15:17         ` Bjorn Helgaas
  0 siblings, 0 replies; 7+ messages in thread
From: Bjorn Helgaas @ 2020-04-24 15:17 UTC (permalink / raw)
  To: Saheed Bolarinwa; +Cc: Yicong Yang, bjorn, skhan, linux-pci

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-04-24 15:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-09 16:16 [PATCH] Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER Bolarinwa Olayemi Saheed
2020-04-10  1:28 ` Yicong Yang
2020-04-10 16:13   ` Saheed Bolarinwa
2020-04-10 20:22   ` Bjorn Helgaas
2020-04-11  2:10     ` Yicong Yang
2020-04-19  5:56       ` Saheed Bolarinwa
2020-04-24 15:17         ` Bjorn Helgaas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).