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@xxxxxxxxx>
Suggested-by: Bjorn Helgaas <bjorn@xxxxxxxxxxx>
---
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);
.
