| Message ID | 20200409161609.2034-1-refactormyself@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Replace -EINVAL with PCIBIOS_BAD_REGISTER_NUMBER | expand |
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);
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);
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); >
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);
> .
>
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); >> . >>
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
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);
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(-)