* [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 related [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).