Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Mon, Jul 16, 2012 at 6:09 PM, Jiang Liu wrote: > On 07/17/2012 01:29 AM, Bjorn Helgaas wrote: >> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu wrote: >>> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: > Hi Bjorn, > It's a little risk to change these PCIe capabilities access > functions as void. On some platform with hardware error > detecting/correcting > capabilities, such as EEH on Power, it would be better to return > error code if hardware error happens during accessing configuration > registers. > As I know, coming Intel Xeon processor may provide PCIe hardware > error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. >>> >>> Hi Bjorn, >>> Sorry for later reply, on travel these days. >>> Yeah, it's true that most driver doesn't check return values of >>> configuration >>> access functions, but there are still some drivers which do check return >>> value of >>> pci_read_config_xxx(). For example, pciehp driver checks return value of >>> CFG access >>> functions. >>> >>> It's not realistic to enhance all drivers, but we may focus on a >>> small set of >>> drivers for hardwares on specific high-end servers. For RAS features, we >>> can never provide >>> perfect solutions, so we prefer some improvements. After all a small >>> improvement is still >>> an improvement:) >>> >>> I'm only familiar with PCI on IA64 and x86. For PowerPC, I just >>> know that the OS >>> may query firmware whether there's some hardware faults if >>> pci_cfg_read_xxx() returns >>> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return >>> error code to >>> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report >>> hardware faults >>> like SAL on IA64. >>> >>> So how about keeping consistence with pci_cfg_read_xxx() and >>> pci_user_cfg_read_xxx()? >> >> My goal is "the caller should never have to know whether this is a v1 >> or v2 capability." Returning any error other than one passed along >> from pci_read/write_config_xxx() means we miss that goal. Perhaps the >> goal is unattainable, but I haven't been convinced yet. >> >> I think hardware error detection is irrelevant to this discussion. >> After reading Documentation/PCI/pci-error-recovery.txt, I'm even less >> convinced that checking return values from pci_read/write_config_xxx() >> or pci_pcie_capability_read/write_xxx() is a useful way to detect >> hardware errors. >> >> Having drivers detect hardware failures by checking for config access >> errors is neither necessary nor sufficient. It's not necessary >> because a platform can implement a config accessor that checks *every* >> access and reports failures to the driver via the pci_error_handler >> framework. It's not sufficient because config accesses are rare >> (usually only at init-time), and hardware failures may happen at >> arbitrary other times. >> >> In my opinion, the only relevant question is whether a caller of >> pci_pcie_capability_read/write_xxx() needs to know whether a register >> is implemented (i.e., we have a v2 capability) or not. For reads, I >> don't think there's a case where fabricating a value of zero when >> reading an unimplemented register is a problem. >> >> Writes are obviously more interesting, but I'm still not sure there's >> a case where silently dropping a write to an unimplemented register is >> a problem. The "capability" registers are read-only, so there's no >> problem if we drop writes to them. The "status" registers are >> generally RO or RW1C, where it's only meaningful to write a non-zero >> value if you're previously *read* a non-zero value. The "control" >> registers are often RW, of course, but generally it's only meaningful >> to write a non-zero value when a non-zero bit in the "capability" >> register has previously told you that something is supported. > Hi Bjorn, > I'm convinced by you that we shouldn't return error when accessing > an unimplemented PCIe capabilities register and just hide the differences > among V1/V2 specification. Then how about returning error from > "pci_read/write_config_xxx()" to callers of > pci_pcie_capabilitiy_read/write_xxx()? > I still prefer to return error code to keep consistence with other > configuration > space access interfaces:) I think it's
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 07/17/2012 01:29 AM, Bjorn Helgaas wrote: > On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu wrote: >> On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: Hi Bjorn, It's a little risk to change these PCIe capabilities access functions as void. On some platform with hardware error detecting/correcting capabilities, such as EEH on Power, it would be better to return error code if hardware error happens during accessing configuration registers. As I know, coming Intel Xeon processor may provide PCIe hardware error detecting capability similar to EEH on power. >>> >>> I guess I'm playing devil's advocate here. As a general rule, people >>> don't check the return value of pci_read_config_*() or >>> pci_write_config_*(). Unless you change them all, most callers of >>> pci_pcie_capability_read_*() and _write_*() won't check the returns >>> either. So I'm not sure return values are an effective way to detect >>> those hardware errors. >>> >>> How do these EEH errors get detected or reported today? Do the >>> drivers check every config access for success? Adding those checks >>> and figuring out how to handle errors at every possible point doesn't >>> seem like a recipe for success. >> >> Hi Bjorn, >> Sorry for later reply, on travel these days. >> Yeah, it's true that most driver doesn't check return values of >> configuration >> access functions, but there are still some drivers which do check return >> value of >> pci_read_config_xxx(). For example, pciehp driver checks return value of CFG >> access >> functions. >> >> It's not realistic to enhance all drivers, but we may focus on a >> small set of >> drivers for hardwares on specific high-end servers. For RAS features, we can >> never provide >> perfect solutions, so we prefer some improvements. After all a small >> improvement is still >> an improvement:) >> >> I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know >> that the OS >> may query firmware whether there's some hardware faults if >> pci_cfg_read_xxx() returns >> all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error >> code to >> pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report >> hardware faults >> like SAL on IA64. >> >> So how about keeping consistence with pci_cfg_read_xxx() and >> pci_user_cfg_read_xxx()? > > My goal is "the caller should never have to know whether this is a v1 > or v2 capability." Returning any error other than one passed along > from pci_read/write_config_xxx() means we miss that goal. Perhaps the > goal is unattainable, but I haven't been convinced yet. > > I think hardware error detection is irrelevant to this discussion. > After reading Documentation/PCI/pci-error-recovery.txt, I'm even less > convinced that checking return values from pci_read/write_config_xxx() > or pci_pcie_capability_read/write_xxx() is a useful way to detect > hardware errors. > > Having drivers detect hardware failures by checking for config access > errors is neither necessary nor sufficient. It's not necessary > because a platform can implement a config accessor that checks *every* > access and reports failures to the driver via the pci_error_handler > framework. It's not sufficient because config accesses are rare > (usually only at init-time), and hardware failures may happen at > arbitrary other times. > > In my opinion, the only relevant question is whether a caller of > pci_pcie_capability_read/write_xxx() needs to know whether a register > is implemented (i.e., we have a v2 capability) or not. For reads, I > don't think there's a case where fabricating a value of zero when > reading an unimplemented register is a problem. > > Writes are obviously more interesting, but I'm still not sure there's > a case where silently dropping a write to an unimplemented register is > a problem. The "capability" registers are read-only, so there's no > problem if we drop writes to them. The "status" registers are > generally RO or RW1C, where it's only meaningful to write a non-zero > value if you're previously *read* a non-zero value. The "control" > registers are often RW, of course, but generally it's only meaningful > to write a non-zero value when a non-zero bit in the "capability" > register has previously told you that something is supported. Hi Bjorn, I'm convinced by you that we shouldn't return error when accessing an unimplemented PCIe capabilities register and just hide the differences among V1/V2 specification. Then how about returning error from "pci_read/write_config_xxx()" to callers of pci_pcie_capabilitiy_read/write_xxx()? I still prefer to return error code to keep consistence with other configuration space access interfaces:) Thanks! Gerry > > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 07/16/2012 01:29 PM, Bjorn Helgaas wrote: On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu wrote: On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: Hi Bjorn, It's a little risk to change these PCIe capabilities access functions as void. On some platform with hardware error detecting/correcting capabilities, such as EEH on Power, it would be better to return error code if hardware error happens during accessing configuration registers. As I know, coming Intel Xeon processor may provide PCIe hardware error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. Hi Bjorn, Sorry for later reply, on travel these days. Yeah, it's true that most driver doesn't check return values of configuration access functions, but there are still some drivers which do check return value of pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access functions. It's not realistic to enhance all drivers, but we may focus on a small set of drivers for hardwares on specific high-end servers. For RAS features, we can never provide perfect solutions, so we prefer some improvements. After all a small improvement is still an improvement:) I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults like SAL on IA64. So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()? My goal is "the caller should never have to know whether this is a v1 or v2 capability." Returning any error other than one passed along from pci_read/write_config_xxx() means we miss that goal. Perhaps the goal is unattainable, but I haven't been convinced yet. I think hardware error detection is irrelevant to this discussion. After reading Documentation/PCI/pci-error-recovery.txt, I'm even less convinced that checking return values from pci_read/write_config_xxx() or pci_pcie_capability_read/write_xxx() is a useful way to detect hardware errors. Having drivers detect hardware failures by checking for config access errors is neither necessary nor sufficient. It's not necessary because a platform can implement a config accessor that checks *every* access and reports failures to the driver via the pci_error_handler framework. It's not sufficient because config accesses are rare (usually only at init-time), and hardware failures may happen at arbitrary other times. In my opinion, the only relevant question is whether a caller of pci_pcie_capability_read/write_xxx() needs to know whether a register is implemented (i.e., we have a v2 capability) or not. For reads, I don't think there's a case where fabricating a value of zero when reading an unimplemented register is a problem. Writes are obviously more interesting, but I'm still not sure there's a case where silently dropping a write to an unimplemented register is a problem. The "capability" registers are read-only, so there's no problem if we drop writes to them. The "status" registers are generally RO or RW1C, where it's only meaningful to write a non-zero value if you're previously *read* a non-zero value. The "control" registers are often RW, of course, but generally it's only meaningful to write a non-zero value when a non-zero bit in the "capability" register has previously told you that something is supported. Bjorn +1 Returning 0 on capability reads -- due to unimplemented features/register or due to failures, should translate into the (core) code doing no writes. Thus, the reason I suggested returning 0 on failure in original posting. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu wrote: > On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: >>> Hi Bjorn, >>> It's a little risk to change these PCIe capabilities access >>> functions as void. On some platform with hardware error detecting/correcting >>> capabilities, such as EEH on Power, it would be better to return >>> error code if hardware error happens during accessing configuration >>> registers. >>> As I know, coming Intel Xeon processor may provide PCIe hardware >>> error detecting capability similar to EEH on power. >> >> I guess I'm playing devil's advocate here. As a general rule, people >> don't check the return value of pci_read_config_*() or >> pci_write_config_*(). Unless you change them all, most callers of >> pci_pcie_capability_read_*() and _write_*() won't check the returns >> either. So I'm not sure return values are an effective way to detect >> those hardware errors. >> >> How do these EEH errors get detected or reported today? Do the >> drivers check every config access for success? Adding those checks >> and figuring out how to handle errors at every possible point doesn't >> seem like a recipe for success. > > Hi Bjorn, > Sorry for later reply, on travel these days. > Yeah, it's true that most driver doesn't check return values of > configuration > access functions, but there are still some drivers which do check return > value of > pci_read_config_xxx(). For example, pciehp driver checks return value of CFG > access > functions. > > It's not realistic to enhance all drivers, but we may focus on a > small set of > drivers for hardwares on specific high-end servers. For RAS features, we can > never provide > perfect solutions, so we prefer some improvements. After all a small > improvement is still > an improvement:) > > I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know > that the OS > may query firmware whether there's some hardware faults if pci_cfg_read_xxx() > returns > all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error > code to > pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report > hardware faults > like SAL on IA64. > > So how about keeping consistence with pci_cfg_read_xxx() and > pci_user_cfg_read_xxx()? My goal is "the caller should never have to know whether this is a v1 or v2 capability." Returning any error other than one passed along from pci_read/write_config_xxx() means we miss that goal. Perhaps the goal is unattainable, but I haven't been convinced yet. I think hardware error detection is irrelevant to this discussion. After reading Documentation/PCI/pci-error-recovery.txt, I'm even less convinced that checking return values from pci_read/write_config_xxx() or pci_pcie_capability_read/write_xxx() is a useful way to detect hardware errors. Having drivers detect hardware failures by checking for config access errors is neither necessary nor sufficient. It's not necessary because a platform can implement a config accessor that checks *every* access and reports failures to the driver via the pci_error_handler framework. It's not sufficient because config accesses are rare (usually only at init-time), and hardware failures may happen at arbitrary other times. In my opinion, the only relevant question is whether a caller of pci_pcie_capability_read/write_xxx() needs to know whether a register is implemented (i.e., we have a v2 capability) or not. For reads, I don't think there's a case where fabricating a value of zero when reading an unimplemented register is a problem. Writes are obviously more interesting, but I'm still not sure there's a case where silently dropping a write to an unimplemented register is a problem. The "capability" registers are read-only, so there's no problem if we drop writes to them. The "status" registers are generally RO or RW1C, where it's only meaningful to write a non-zero value if you're previously *read* a non-zero value. The "control" registers are often RW, of course, but generally it's only meaningful to write a non-zero value when a non-zero bit in the "capability" register has previously told you that something is supported. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu liu...@gmail.com wrote: On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: Hi Bjorn, It's a little risk to change these PCIe capabilities access functions as void. On some platform with hardware error detecting/correcting capabilities, such as EEH on Power, it would be better to return error code if hardware error happens during accessing configuration registers. As I know, coming Intel Xeon processor may provide PCIe hardware error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. Hi Bjorn, Sorry for later reply, on travel these days. Yeah, it's true that most driver doesn't check return values of configuration access functions, but there are still some drivers which do check return value of pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access functions. It's not realistic to enhance all drivers, but we may focus on a small set of drivers for hardwares on specific high-end servers. For RAS features, we can never provide perfect solutions, so we prefer some improvements. After all a small improvement is still an improvement:) I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults like SAL on IA64. So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()? My goal is the caller should never have to know whether this is a v1 or v2 capability. Returning any error other than one passed along from pci_read/write_config_xxx() means we miss that goal. Perhaps the goal is unattainable, but I haven't been convinced yet. I think hardware error detection is irrelevant to this discussion. After reading Documentation/PCI/pci-error-recovery.txt, I'm even less convinced that checking return values from pci_read/write_config_xxx() or pci_pcie_capability_read/write_xxx() is a useful way to detect hardware errors. Having drivers detect hardware failures by checking for config access errors is neither necessary nor sufficient. It's not necessary because a platform can implement a config accessor that checks *every* access and reports failures to the driver via the pci_error_handler framework. It's not sufficient because config accesses are rare (usually only at init-time), and hardware failures may happen at arbitrary other times. In my opinion, the only relevant question is whether a caller of pci_pcie_capability_read/write_xxx() needs to know whether a register is implemented (i.e., we have a v2 capability) or not. For reads, I don't think there's a case where fabricating a value of zero when reading an unimplemented register is a problem. Writes are obviously more interesting, but I'm still not sure there's a case where silently dropping a write to an unimplemented register is a problem. The capability registers are read-only, so there's no problem if we drop writes to them. The status registers are generally RO or RW1C, where it's only meaningful to write a non-zero value if you're previously *read* a non-zero value. The control registers are often RW, of course, but generally it's only meaningful to write a non-zero value when a non-zero bit in the capability register has previously told you that something is supported. Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 07/16/2012 01:29 PM, Bjorn Helgaas wrote: On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liuliu...@gmail.com wrote: On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: Hi Bjorn, It's a little risk to change these PCIe capabilities access functions as void. On some platform with hardware error detecting/correcting capabilities, such as EEH on Power, it would be better to return error code if hardware error happens during accessing configuration registers. As I know, coming Intel Xeon processor may provide PCIe hardware error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. Hi Bjorn, Sorry for later reply, on travel these days. Yeah, it's true that most driver doesn't check return values of configuration access functions, but there are still some drivers which do check return value of pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access functions. It's not realistic to enhance all drivers, but we may focus on a small set of drivers for hardwares on specific high-end servers. For RAS features, we can never provide perfect solutions, so we prefer some improvements. After all a small improvement is still an improvement:) I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults like SAL on IA64. So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()? My goal is the caller should never have to know whether this is a v1 or v2 capability. Returning any error other than one passed along from pci_read/write_config_xxx() means we miss that goal. Perhaps the goal is unattainable, but I haven't been convinced yet. I think hardware error detection is irrelevant to this discussion. After reading Documentation/PCI/pci-error-recovery.txt, I'm even less convinced that checking return values from pci_read/write_config_xxx() or pci_pcie_capability_read/write_xxx() is a useful way to detect hardware errors. Having drivers detect hardware failures by checking for config access errors is neither necessary nor sufficient. It's not necessary because a platform can implement a config accessor that checks *every* access and reports failures to the driver via the pci_error_handler framework. It's not sufficient because config accesses are rare (usually only at init-time), and hardware failures may happen at arbitrary other times. In my opinion, the only relevant question is whether a caller of pci_pcie_capability_read/write_xxx() needs to know whether a register is implemented (i.e., we have a v2 capability) or not. For reads, I don't think there's a case where fabricating a value of zero when reading an unimplemented register is a problem. Writes are obviously more interesting, but I'm still not sure there's a case where silently dropping a write to an unimplemented register is a problem. The capability registers are read-only, so there's no problem if we drop writes to them. The status registers are generally RO or RW1C, where it's only meaningful to write a non-zero value if you're previously *read* a non-zero value. The control registers are often RW, of course, but generally it's only meaningful to write a non-zero value when a non-zero bit in the capability register has previously told you that something is supported. Bjorn +1 Returning 0 on capability reads -- due to unimplemented features/register or due to failures, should translate into the (core) code doing no writes. Thus, the reason I suggested returning 0 on failure in original posting. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 07/17/2012 01:29 AM, Bjorn Helgaas wrote: On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu liu...@gmail.com wrote: On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: Hi Bjorn, It's a little risk to change these PCIe capabilities access functions as void. On some platform with hardware error detecting/correcting capabilities, such as EEH on Power, it would be better to return error code if hardware error happens during accessing configuration registers. As I know, coming Intel Xeon processor may provide PCIe hardware error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. Hi Bjorn, Sorry for later reply, on travel these days. Yeah, it's true that most driver doesn't check return values of configuration access functions, but there are still some drivers which do check return value of pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access functions. It's not realistic to enhance all drivers, but we may focus on a small set of drivers for hardwares on specific high-end servers. For RAS features, we can never provide perfect solutions, so we prefer some improvements. After all a small improvement is still an improvement:) I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults like SAL on IA64. So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()? My goal is the caller should never have to know whether this is a v1 or v2 capability. Returning any error other than one passed along from pci_read/write_config_xxx() means we miss that goal. Perhaps the goal is unattainable, but I haven't been convinced yet. I think hardware error detection is irrelevant to this discussion. After reading Documentation/PCI/pci-error-recovery.txt, I'm even less convinced that checking return values from pci_read/write_config_xxx() or pci_pcie_capability_read/write_xxx() is a useful way to detect hardware errors. Having drivers detect hardware failures by checking for config access errors is neither necessary nor sufficient. It's not necessary because a platform can implement a config accessor that checks *every* access and reports failures to the driver via the pci_error_handler framework. It's not sufficient because config accesses are rare (usually only at init-time), and hardware failures may happen at arbitrary other times. In my opinion, the only relevant question is whether a caller of pci_pcie_capability_read/write_xxx() needs to know whether a register is implemented (i.e., we have a v2 capability) or not. For reads, I don't think there's a case where fabricating a value of zero when reading an unimplemented register is a problem. Writes are obviously more interesting, but I'm still not sure there's a case where silently dropping a write to an unimplemented register is a problem. The capability registers are read-only, so there's no problem if we drop writes to them. The status registers are generally RO or RW1C, where it's only meaningful to write a non-zero value if you're previously *read* a non-zero value. The control registers are often RW, of course, but generally it's only meaningful to write a non-zero value when a non-zero bit in the capability register has previously told you that something is supported. Hi Bjorn, I'm convinced by you that we shouldn't return error when accessing an unimplemented PCIe capabilities register and just hide the differences among V1/V2 specification. Then how about returning error from pci_read/write_config_xxx() to callers of pci_pcie_capabilitiy_read/write_xxx()? I still prefer to return error code to keep consistence with other configuration space access interfaces:) Thanks! Gerry Bjorn -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Mon, Jul 16, 2012 at 6:09 PM, Jiang Liu liu...@gmail.com wrote:
On 07/17/2012 01:29 AM, Bjorn Helgaas wrote:
On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu liu...@gmail.com wrote:
On 07/13/2012 04:49 AM, Bjorn Helgaas wrote:
Hi Bjorn,
It's a little risk to change these PCIe capabilities access
functions as void. On some platform with hardware error
detecting/correcting
capabilities, such as EEH on Power, it would be better to return
error code if hardware error happens during accessing configuration
registers.
As I know, coming Intel Xeon processor may provide PCIe hardware
error detecting capability similar to EEH on power.
I guess I'm playing devil's advocate here. As a general rule, people
don't check the return value of pci_read_config_*() or
pci_write_config_*(). Unless you change them all, most callers of
pci_pcie_capability_read_*() and _write_*() won't check the returns
either. So I'm not sure return values are an effective way to detect
those hardware errors.
How do these EEH errors get detected or reported today? Do the
drivers check every config access for success? Adding those checks
and figuring out how to handle errors at every possible point doesn't
seem like a recipe for success.
Hi Bjorn,
Sorry for later reply, on travel these days.
Yeah, it's true that most driver doesn't check return values of
configuration
access functions, but there are still some drivers which do check return
value of
pci_read_config_xxx(). For example, pciehp driver checks return value of
CFG access
functions.
It's not realistic to enhance all drivers, but we may focus on a
small set of
drivers for hardwares on specific high-end servers. For RAS features, we
can never provide
perfect solutions, so we prefer some improvements. After all a small
improvement is still
an improvement:)
I'm only familiar with PCI on IA64 and x86. For PowerPC, I just
know that the OS
may query firmware whether there's some hardware faults if
pci_cfg_read_xxx() returns
all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return
error code to
pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report
hardware faults
like SAL on IA64.
So how about keeping consistence with pci_cfg_read_xxx() and
pci_user_cfg_read_xxx()?
My goal is the caller should never have to know whether this is a v1
or v2 capability. Returning any error other than one passed along
from pci_read/write_config_xxx() means we miss that goal. Perhaps the
goal is unattainable, but I haven't been convinced yet.
I think hardware error detection is irrelevant to this discussion.
After reading Documentation/PCI/pci-error-recovery.txt, I'm even less
convinced that checking return values from pci_read/write_config_xxx()
or pci_pcie_capability_read/write_xxx() is a useful way to detect
hardware errors.
Having drivers detect hardware failures by checking for config access
errors is neither necessary nor sufficient. It's not necessary
because a platform can implement a config accessor that checks *every*
access and reports failures to the driver via the pci_error_handler
framework. It's not sufficient because config accesses are rare
(usually only at init-time), and hardware failures may happen at
arbitrary other times.
In my opinion, the only relevant question is whether a caller of
pci_pcie_capability_read/write_xxx() needs to know whether a register
is implemented (i.e., we have a v2 capability) or not. For reads, I
don't think there's a case where fabricating a value of zero when
reading an unimplemented register is a problem.
Writes are obviously more interesting, but I'm still not sure there's
a case where silently dropping a write to an unimplemented register is
a problem. The capability registers are read-only, so there's no
problem if we drop writes to them. The status registers are
generally RO or RW1C, where it's only meaningful to write a non-zero
value if you're previously *read* a non-zero value. The control
registers are often RW, of course, but generally it's only meaningful
to write a non-zero value when a non-zero bit in the capability
register has previously told you that something is supported.
Hi Bjorn,
I'm convinced by you that we shouldn't return error when accessing
an unimplemented PCIe capabilities register and just hide the differences
among V1/V2 specification. Then how about returning error from
pci_read/write_config_xxx() to callers of
pci_pcie_capabilitiy_read/write_xxx()?
I still prefer to return error code to keep consistence with other
configuration
space access interfaces:)
I think it's fine to return the status of pci_read/write_config_xxx(), e.g.,
int pci_pcie_cap_read_word(...)
{
...
if (implemented)
return pci_read_config_word(...);
...
return 0;
}
Bjorn
--
To unsubscribe
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: >> Hi Bjorn, >> It's a little risk to change these PCIe capabilities access >> functions as void. On some platform with hardware error detecting/correcting >> capabilities, such as EEH on Power, it would be better to return >> error code if hardware error happens during accessing configuration >> registers. >> As I know, coming Intel Xeon processor may provide PCIe hardware >> error detecting capability similar to EEH on power. > > I guess I'm playing devil's advocate here. As a general rule, people > don't check the return value of pci_read_config_*() or > pci_write_config_*(). Unless you change them all, most callers of > pci_pcie_capability_read_*() and _write_*() won't check the returns > either. So I'm not sure return values are an effective way to detect > those hardware errors. > > How do these EEH errors get detected or reported today? Do the > drivers check every config access for success? Adding those checks > and figuring out how to handle errors at every possible point doesn't > seem like a recipe for success. Hi Bjorn, Sorry for later reply, on travel these days. Yeah, it's true that most driver doesn't check return values of configuration access functions, but there are still some drivers which do check return value of pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access functions. It's not realistic to enhance all drivers, but we may focus on a small set of drivers for hardwares on specific high-end servers. For RAS features, we can never provide perfect solutions, so we prefer some improvements. After all a small improvement is still an improvement:) I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults like SAL on IA64. So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()? Thanks! Gerry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 07/13/2012 04:49 AM, Bjorn Helgaas wrote: Hi Bjorn, It's a little risk to change these PCIe capabilities access functions as void. On some platform with hardware error detecting/correcting capabilities, such as EEH on Power, it would be better to return error code if hardware error happens during accessing configuration registers. As I know, coming Intel Xeon processor may provide PCIe hardware error detecting capability similar to EEH on power. I guess I'm playing devil's advocate here. As a general rule, people don't check the return value of pci_read_config_*() or pci_write_config_*(). Unless you change them all, most callers of pci_pcie_capability_read_*() and _write_*() won't check the returns either. So I'm not sure return values are an effective way to detect those hardware errors. How do these EEH errors get detected or reported today? Do the drivers check every config access for success? Adding those checks and figuring out how to handle errors at every possible point doesn't seem like a recipe for success. Hi Bjorn, Sorry for later reply, on travel these days. Yeah, it's true that most driver doesn't check return values of configuration access functions, but there are still some drivers which do check return value of pci_read_config_xxx(). For example, pciehp driver checks return value of CFG access functions. It's not realistic to enhance all drivers, but we may focus on a small set of drivers for hardwares on specific high-end servers. For RAS features, we can never provide perfect solutions, so we prefer some improvements. After all a small improvement is still an improvement:) I'm only familiar with PCI on IA64 and x86. For PowerPC, I just know that the OS may query firmware whether there's some hardware faults if pci_cfg_read_xxx() returns all 1s. For PCI on IA64, SAL may handle PCI hardware errors and return error code to pci_cfg_read_xxx(). For x86, I think it will have some mechanisms to report hardware faults like SAL on IA64. So how about keeping consistence with pci_cfg_read_xxx() and pci_user_cfg_read_xxx()? Thanks! Gerry -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Wed, Jul 11, 2012 at 8:56 PM, Jiang Liu wrote:
> On 2012-7-12 1:52, Bjorn Helgaas wrote:
>>> Hi Bjorn,
>>> Seems it would be better to return error code for unimplemented
>>> registers, otherwise following code will becomes more complex. A special
>>> error code for unimplemented registers, such as -EIO?
>>
>> I think you're asking about returning error for *reads* of
>> unimplemented registers? I guess I still think it's OK to completely
>> hide the v1 nastiness inside these accessors, and return success with
>> a zero value when reading. Having several different error returns
>> seems like overkill for this case. Nobody wants to distinguish
>> between different reasons for failure.
>>
>> I'm actually not sure that it's worth returning an error even when
>> *writing* an unimplemented register. What if we return success and
>> just drop the write?
>>
>> Maybe these should even be void functions. It feels like the only
>> real use of the return value is to detect programmer error, and I
>> don't think that's very effective. If we remove the return values,
>> people will have to focus on the *data*, which seems more important
>> anyway.
> Hi Bjorn,
> It's a little risk to change these PCIe capabilities access
> functions as void. On some platform with hardware error detecting/correcting
> capabilities, such as EEH on Power, it would be better to return
> error code if hardware error happens during accessing configuration registers.
> As I know, coming Intel Xeon processor may provide PCIe hardware
> error detecting capability similar to EEH on power.
I guess I'm playing devil's advocate here. As a general rule, people
don't check the return value of pci_read_config_*() or
pci_write_config_*(). Unless you change them all, most callers of
pci_pcie_capability_read_*() and _write_*() won't check the returns
either. So I'm not sure return values are an effective way to detect
those hardware errors.
How do these EEH errors get detected or reported today? Do the
drivers check every config access for success? Adding those checks
and figuring out how to handle errors at every possible point doesn't
seem like a recipe for success.
>>> static void rtl_disable_clock_request(struct pci_dev *pdev)
>>> {
>>> u16 ctl;
>>>
>>> if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, )) {
>>> ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>>> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>>> }
>>> }
>>
>> I would write that as:
>>
>> if (!pci_is_pcie(pdev)
>> return;
>>
>> pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, );
>> if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
>> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
>> ~PCI_EXP_LNKCTL_CLKREQ_EN);
>>
>> which does the right thing regardless of what we do for return values,
>> and saves a config write in the case where LNKCTL is implemented and
>> CLKREQ_EN is already cleared.
> When clearing a flag, we could do that. But if we are trying to set a
> flag, it would be better to make sure the target register does exist.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Wed, Jul 11, 2012 at 8:56 PM, Jiang Liu jiang@huawei.com wrote:
On 2012-7-12 1:52, Bjorn Helgaas wrote:
Hi Bjorn,
Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?
I think you're asking about returning error for *reads* of
unimplemented registers? I guess I still think it's OK to completely
hide the v1 nastiness inside these accessors, and return success with
a zero value when reading. Having several different error returns
seems like overkill for this case. Nobody wants to distinguish
between different reasons for failure.
I'm actually not sure that it's worth returning an error even when
*writing* an unimplemented register. What if we return success and
just drop the write?
Maybe these should even be void functions. It feels like the only
real use of the return value is to detect programmer error, and I
don't think that's very effective. If we remove the return values,
people will have to focus on the *data*, which seems more important
anyway.
Hi Bjorn,
It's a little risk to change these PCIe capabilities access
functions as void. On some platform with hardware error detecting/correcting
capabilities, such as EEH on Power, it would be better to return
error code if hardware error happens during accessing configuration registers.
As I know, coming Intel Xeon processor may provide PCIe hardware
error detecting capability similar to EEH on power.
I guess I'm playing devil's advocate here. As a general rule, people
don't check the return value of pci_read_config_*() or
pci_write_config_*(). Unless you change them all, most callers of
pci_pcie_capability_read_*() and _write_*() won't check the returns
either. So I'm not sure return values are an effective way to detect
those hardware errors.
How do these EEH errors get detected or reported today? Do the
drivers check every config access for success? Adding those checks
and figuring out how to handle errors at every possible point doesn't
seem like a recipe for success.
static void rtl_disable_clock_request(struct pci_dev *pdev)
{
u16 ctl;
if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl)) {
ctl = ~PCI_EXP_LNKCTL_CLKREQ_EN;
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
}
}
I would write that as:
if (!pci_is_pcie(pdev)
return;
pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl);
if (ctl PCI_EXP_LNKCTL_CLKREQ_EN)
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl
~PCI_EXP_LNKCTL_CLKREQ_EN);
which does the right thing regardless of what we do for return values,
and saves a config write in the case where LNKCTL is implemented and
CLKREQ_EN is already cleared.
When clearing a flag, we could do that. But if we are trying to set a
flag, it would be better to make sure the target register does exist.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 2012-7-12 1:52, Bjorn Helgaas wrote:
>> Hi Bjorn,
>> Seems it would be better to return error code for unimplemented
>> registers, otherwise following code will becomes more complex. A special
>> error code for unimplemented registers, such as -EIO?
>
> I think you're asking about returning error for *reads* of
> unimplemented registers? I guess I still think it's OK to completely
> hide the v1 nastiness inside these accessors, and return success with
> a zero value when reading. Having several different error returns
> seems like overkill for this case. Nobody wants to distinguish
> between different reasons for failure.
>
> I'm actually not sure that it's worth returning an error even when
> *writing* an unimplemented register. What if we return success and
> just drop the write?
>
> Maybe these should even be void functions. It feels like the only
> real use of the return value is to detect programmer error, and I
> don't think that's very effective. If we remove the return values,
> people will have to focus on the *data*, which seems more important
> anyway.
Hi Bjorn,
It's a little risk to change these PCIe capabilities access
functions as void. On some platform with hardware error detecting/correcting
capabilities, such as EEH on Power, it would be better to return
error code if hardware error happens during accessing configuration registers.
As I know, coming Intel Xeon processor may provide PCIe hardware
error detecting capability similar to EEH on power.
>> static void rtl_disable_clock_request(struct pci_dev *pdev)
>> {
>> u16 ctl;
>>
>> if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, )) {
>> ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
>> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
>> }
>> }
>
> I would write that as:
>
> if (!pci_is_pcie(pdev)
> return;
>
> pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, );
> if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
> ~PCI_EXP_LNKCTL_CLKREQ_EN);
>
> which does the right thing regardless of what we do for return values,
> and saves a config write in the case where LNKCTL is implemented and
> CLKREQ_EN is already cleared.
When clearing a flag, we could do that. But if we are trying to set a
flag, it would be better to make sure the target register does exist.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu wrote:
> On 2012-7-11 11:40, Bjorn Helgaas wrote:
>
>>> Good point. Return success when reading unimplemented registeres, that
>>> may simplify code. For we still should return -EINVAL when writing
>>> unimplemented registers, right?
>>
>> Yeah, I guess it's OK to return -EINVAL when *writing* to an
>> unimplemented register. Hopefully the caller is structured such that
>> we don't even try to write in that case. It'd be interesting to audit
>> the callers and explore that, but I haven't done that.
> Hi Bjorn,
> Seems it would be better to return error code for unimplemented
> registers, otherwise following code will becomes more complex. A special
> error code for unimplemented registers, such as -EIO?
I think you're asking about returning error for *reads* of
unimplemented registers? I guess I still think it's OK to completely
hide the v1 nastiness inside these accessors, and return success with
a zero value when reading. Having several different error returns
seems like overkill for this case. Nobody wants to distinguish
between different reasons for failure.
I'm actually not sure that it's worth returning an error even when
*writing* an unimplemented register. What if we return success and
just drop the write?
Maybe these should even be void functions. It feels like the only
real use of the return value is to detect programmer error, and I
don't think that's very effective. If we remove the return values,
people will have to focus on the *data*, which seems more important
anyway.
> static void rtl_disable_clock_request(struct pci_dev *pdev)
> {
> u16 ctl;
>
> if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, )) {
> ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
> pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
> }
> }
I would write that as:
if (!pci_is_pcie(pdev)
return;
pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, );
if (ctl & PCI_EXP_LNKCTL_CLKREQ_EN)
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl &
~PCI_EXP_LNKCTL_CLKREQ_EN);
which does the right thing regardless of what we do for return values,
and saves a config write in the case where LNKCTL is implemented and
CLKREQ_EN is already cleared.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 2012-7-11 11:40, Bjorn Helgaas wrote:
>> Good point. Return success when reading unimplemented registeres, that
>> may simplify code. For we still should return -EINVAL when writing
>> unimplemented registers, right?
>
> Yeah, I guess it's OK to return -EINVAL when *writing* to an
> unimplemented register. Hopefully the caller is structured such that
> we don't even try to write in that case. It'd be interesting to audit
> the callers and explore that, but I haven't done that.
Hi Bjorn,
Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?
static void rtl_disable_clock_request(struct pci_dev *pdev)
{
u16 ctl;
if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, )) {
ctl &= ~PCI_EXP_LNKCTL_CLKREQ_EN;
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 2012-7-11 11:40, Bjorn Helgaas wrote:
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?
Yeah, I guess it's OK to return -EINVAL when *writing* to an
unimplemented register. Hopefully the caller is structured such that
we don't even try to write in that case. It'd be interesting to audit
the callers and explore that, but I haven't done that.
Hi Bjorn,
Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?
static void rtl_disable_clock_request(struct pci_dev *pdev)
{
u16 ctl;
if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl)) {
ctl = ~PCI_EXP_LNKCTL_CLKREQ_EN;
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
}
}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu jiang@huawei.com wrote:
On 2012-7-11 11:40, Bjorn Helgaas wrote:
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?
Yeah, I guess it's OK to return -EINVAL when *writing* to an
unimplemented register. Hopefully the caller is structured such that
we don't even try to write in that case. It'd be interesting to audit
the callers and explore that, but I haven't done that.
Hi Bjorn,
Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?
I think you're asking about returning error for *reads* of
unimplemented registers? I guess I still think it's OK to completely
hide the v1 nastiness inside these accessors, and return success with
a zero value when reading. Having several different error returns
seems like overkill for this case. Nobody wants to distinguish
between different reasons for failure.
I'm actually not sure that it's worth returning an error even when
*writing* an unimplemented register. What if we return success and
just drop the write?
Maybe these should even be void functions. It feels like the only
real use of the return value is to detect programmer error, and I
don't think that's very effective. If we remove the return values,
people will have to focus on the *data*, which seems more important
anyway.
static void rtl_disable_clock_request(struct pci_dev *pdev)
{
u16 ctl;
if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl)) {
ctl = ~PCI_EXP_LNKCTL_CLKREQ_EN;
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
}
}
I would write that as:
if (!pci_is_pcie(pdev)
return;
pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl);
if (ctl PCI_EXP_LNKCTL_CLKREQ_EN)
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl
~PCI_EXP_LNKCTL_CLKREQ_EN);
which does the right thing regardless of what we do for return values,
and saves a config write in the case where LNKCTL is implemented and
CLKREQ_EN is already cleared.
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 2012-7-12 1:52, Bjorn Helgaas wrote:
Hi Bjorn,
Seems it would be better to return error code for unimplemented
registers, otherwise following code will becomes more complex. A special
error code for unimplemented registers, such as -EIO?
I think you're asking about returning error for *reads* of
unimplemented registers? I guess I still think it's OK to completely
hide the v1 nastiness inside these accessors, and return success with
a zero value when reading. Having several different error returns
seems like overkill for this case. Nobody wants to distinguish
between different reasons for failure.
I'm actually not sure that it's worth returning an error even when
*writing* an unimplemented register. What if we return success and
just drop the write?
Maybe these should even be void functions. It feels like the only
real use of the return value is to detect programmer error, and I
don't think that's very effective. If we remove the return values,
people will have to focus on the *data*, which seems more important
anyway.
Hi Bjorn,
It's a little risk to change these PCIe capabilities access
functions as void. On some platform with hardware error detecting/correcting
capabilities, such as EEH on Power, it would be better to return
error code if hardware error happens during accessing configuration registers.
As I know, coming Intel Xeon processor may provide PCIe hardware
error detecting capability similar to EEH on power.
static void rtl_disable_clock_request(struct pci_dev *pdev)
{
u16 ctl;
if (!pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl)) {
ctl = ~PCI_EXP_LNKCTL_CLKREQ_EN;
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl);
}
}
I would write that as:
if (!pci_is_pcie(pdev)
return;
pci_pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, ctl);
if (ctl PCI_EXP_LNKCTL_CLKREQ_EN)
pci_pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, ctl
~PCI_EXP_LNKCTL_CLKREQ_EN);
which does the right thing regardless of what we do for return values,
and saves a config write in the case where LNKCTL is implemented and
CLKREQ_EN is already cleared.
When clearing a flag, we could do that. But if we are trying to set a
flag, it would be better to make sure the target register does exist.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Tue, Jul 10, 2012 at 9:07 PM, Jiang Liu wrote:
> On 2012-7-11 2:35, Bjorn Helgaas wrote:
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index ba91a7e..80ae022 100644
>>> --- a/drivers/pci/access.c
>>> +++ b/drivers/pci/access.c
>>> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>>> raw_spin_unlock_irqrestore(_lock, flags);
>>> }
>>> EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
>>> +
>>> +static int
>>> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
>>> +{
>>> + bool valid;
>>> +
>>> + if (!pci_is_pcie(dev))
>>> + return -EINVAL;
>>> + if (where & (sz - 1))
>>> + return -EINVAL;
>>> +
>>> + if (where < 0)
>>> + valid = false;
>>> + else if (where < PCI_EXP_DEVCAP)
>>> + valid = true;
>>> + else if (where < PCI_EXP_LNKCAP)
>>> + valid = pci_pcie_cap_has_devctl(dev);
>>> + else if (where < PCI_EXP_SLTCAP)
>>> + valid = pci_pcie_cap_has_lnkctl(dev);
>>> + else if (where < PCI_EXP_RTCTL)
>>> + valid = pci_pcie_cap_has_sltctl(dev);
>>> + else if (where < PCI_EXP_DEVCAP2)
>>> + valid = pci_pcie_cap_has_rtctl(dev);
>>> + else if (where < PCI_EXP_CAP2_SIZE)
>>> + valid = pci_pcie_cap_has_cap2(dev);
>>> + else
>>> + valid = false;
>>> +
>>> + return valid ? where + pci_pcie_cap(dev) : -EINVAL;
>>> +}
>>> +
>>> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
>>> +{
>>> + *valp = 0;
>>> + where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
>>
>> This is a really slick factorization; I like it much better than my
>> proposal. I would like it even *better* if it read something like
>> this:
>>
>> bool implemented;
>>
>> *valp = 0;
>> if (!pci_is_pcie(dev) || where & 1)
>> return -EINVAL;
>>
>> implemented = pci_pcie_cap_implemented(dev, where);
>> if (implemented)
>> return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
>>
>> if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
>>
>> because I think it's useful to have the "pos + where" visual pattern
>> in the pci_read_config_word() arguments.
> Sure, for better readability.
>
>>
>>> + if (where >= 0)
>>> + return pci_read_config_word(dev, where, valp);
>>> +
>>> + if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
>>> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
>>> + *valp = PCI_EXP_SLTSTA_PDS;
>>
>> I think we should be returning success in this case (SLTSTA for
>> downstream port). In fact, I think we should return success even when
>> we're emulating the read of an unimplemented register from a v1
>> capability. The caller should not be aware at all that there is a
>> difference between v1 and v2 capabilities.
>>
>> I'd put the spec reference here rather than in read_dword(), since
>> SLTSTA is a u16 and this is the natural way to read it. Then maybe a
>> short comment in read_dword() below.
> Good point. Return success when reading unimplemented registeres, that
> may simplify code. For we still should return -EINVAL when writing
> unimplemented registers, right?
Yeah, I guess it's OK to return -EINVAL when *writing* to an
unimplemented register. Hopefully the caller is structured such that
we don't even try to write in that case. It'd be interesting to audit
the callers and explore that, but I haven't done that.
>>> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 346b2d9..78767b2 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const
>>> struct pci_dev *pdev)
>>>type == PCI_EXP_TYPE_RC_EC;
>>> }
>>>
>>> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16
>>> *valp);
>>> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32
>>> *valp);
>>> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16
>>> val);
>>> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32
>>> val);
>>
>> You don't need the "extern" here (and I think you'll probably remove
>> these altogether, see below).
>>
>>> +
>>> void pci_request_acs(void);
>>> bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>>> bool pci_acs_path_enabled(struct pci_dev *start,
>>> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev
>>> *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>>> */
>>> struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>>>
>>> +int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16
>>> *val);
>>> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32
>>> *val);
>>> +int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16
>>> val);
>>> +int
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 2012-7-11 2:35, Bjorn Helgaas wrote:
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index ba91a7e..80ae022 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
>> raw_spin_unlock_irqrestore(_lock, flags);
>> }
>> EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
>> +
>> +static int
>> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
>> +{
>> + bool valid;
>> +
>> + if (!pci_is_pcie(dev))
>> + return -EINVAL;
>> + if (where & (sz - 1))
>> + return -EINVAL;
>> +
>> + if (where < 0)
>> + valid = false;
>> + else if (where < PCI_EXP_DEVCAP)
>> + valid = true;
>> + else if (where < PCI_EXP_LNKCAP)
>> + valid = pci_pcie_cap_has_devctl(dev);
>> + else if (where < PCI_EXP_SLTCAP)
>> + valid = pci_pcie_cap_has_lnkctl(dev);
>> + else if (where < PCI_EXP_RTCTL)
>> + valid = pci_pcie_cap_has_sltctl(dev);
>> + else if (where < PCI_EXP_DEVCAP2)
>> + valid = pci_pcie_cap_has_rtctl(dev);
>> + else if (where < PCI_EXP_CAP2_SIZE)
>> + valid = pci_pcie_cap_has_cap2(dev);
>> + else
>> + valid = false;
>> +
>> + return valid ? where + pci_pcie_cap(dev) : -EINVAL;
>> +}
>> +
>> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
>> +{
>> + *valp = 0;
>> + where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
>
> This is a really slick factorization; I like it much better than my
> proposal. I would like it even *better* if it read something like
> this:
>
> bool implemented;
>
> *valp = 0;
> if (!pci_is_pcie(dev) || where & 1)
> return -EINVAL;
>
> implemented = pci_pcie_cap_implemented(dev, where);
> if (implemented)
> return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
>
> if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
>
> because I think it's useful to have the "pos + where" visual pattern
> in the pci_read_config_word() arguments.
Sure, for better readability.
>
>> + if (where >= 0)
>> + return pci_read_config_word(dev, where, valp);
>> +
>> + if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
>> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
>> + *valp = PCI_EXP_SLTSTA_PDS;
>
> I think we should be returning success in this case (SLTSTA for
> downstream port). In fact, I think we should return success even when
> we're emulating the read of an unimplemented register from a v1
> capability. The caller should not be aware at all that there is a
> difference between v1 and v2 capabilities.
>
> I'd put the spec reference here rather than in read_dword(), since
> SLTSTA is a u16 and this is the natural way to read it. Then maybe a
> short comment in read_dword() below.
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?
>> +EXPORT_SYMBOL(pci_pcie_cap_write_dword);
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 346b2d9..78767b2 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const
>> struct pci_dev *pdev)
>>type == PCI_EXP_TYPE_RC_EC;
>> }
>>
>> +extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16
>> *valp);
>> +extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32
>> *valp);
>> +extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
>> +extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32
>> val);
>
> You don't need the "extern" here (and I think you'll probably remove
> these altogether, see below).
>
>> +
>> void pci_request_acs(void);
>> bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
>> bool pci_acs_path_enabled(struct pci_dev *start,
>> @@ -1843,5 +1848,10 @@ static inline struct eeh_dev
>> *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>> */
>> struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
>>
>> +int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
>> +int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32
>> *val);
>> +int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
>> +int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32
>> val);
>
> There's some confusion here: pci_pcie_cap_* versus
> pci_pcie_capability_*. I think you only need one set, and I prefer
> pci_pcie_capability_* to follow the example of
> pci_bus_find_capability().
The above confusion was caused by a dirty merge.
>
>> +
>> #endif /* __KERNEL__ */
>> #endif /* LINUX_PCI_H */
>> diff --git a/include/linux/pci_regs.h
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu wrote:
> From: Jiang Liu
>
> Introduce four configuration access functions for PCIe capabilities to
> hide difference among PCIe Base Spec versions. With these functions,
> we can remove callers responsible for using pci_pcie_cap_has_*().
>
> pci_pcie_cap_read_word/dword() functions will store the pcie cap register
> value by passed parameter val,if related pcie cap register is not implemented
> on the pcie device, the passed parameter val will be set 0 and return -EINVAL.
>
> pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
> cap registers,if related pcie cap register is not implemented on the pcie
> device, it will return -EINVAL.
>
> Signed-off-by: Jiang Liu
> Signed-off-by: Yijing Wang
> ---
> drivers/pci/access.c | 88
> ++
> include/linux/pci.h | 10 ++
> include/linux/pci_regs.h | 19 --
> 3 files changed, 115 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba91a7e..80ae022 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
> raw_spin_unlock_irqrestore(_lock, flags);
> }
> EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
> +
> +static int
> +pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
> +{
> + bool valid;
> +
> + if (!pci_is_pcie(dev))
> + return -EINVAL;
> + if (where & (sz - 1))
> + return -EINVAL;
> +
> + if (where < 0)
> + valid = false;
> + else if (where < PCI_EXP_DEVCAP)
> + valid = true;
> + else if (where < PCI_EXP_LNKCAP)
> + valid = pci_pcie_cap_has_devctl(dev);
> + else if (where < PCI_EXP_SLTCAP)
> + valid = pci_pcie_cap_has_lnkctl(dev);
> + else if (where < PCI_EXP_RTCTL)
> + valid = pci_pcie_cap_has_sltctl(dev);
> + else if (where < PCI_EXP_DEVCAP2)
> + valid = pci_pcie_cap_has_rtctl(dev);
> + else if (where < PCI_EXP_CAP2_SIZE)
> + valid = pci_pcie_cap_has_cap2(dev);
> + else
> + valid = false;
> +
> + return valid ? where + pci_pcie_cap(dev) : -EINVAL;
> +}
> +
> +int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
> +{
> + *valp = 0;
> + where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
This is a really slick factorization; I like it much better than my
proposal. I would like it even *better* if it read something like
this:
bool implemented;
*valp = 0;
if (!pci_is_pcie(dev) || where & 1)
return -EINVAL;
implemented = pci_pcie_cap_implemented(dev, where);
if (implemented)
return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA ...
because I think it's useful to have the "pos + where" visual pattern
in the pci_read_config_word() arguments.
> + if (where >= 0)
> + return pci_read_config_word(dev, where, valp);
> +
> + if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> + *valp = PCI_EXP_SLTSTA_PDS;
I think we should be returning success in this case (SLTSTA for
downstream port). In fact, I think we should return success even when
we're emulating the read of an unimplemented register from a v1
capability. The caller should not be aware at all that there is a
difference between v1 and v2 capabilities.
I'd put the spec reference here rather than in read_dword(), since
SLTSTA is a u16 and this is the natural way to read it. Then maybe a
short comment in read_dword() below.
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_read_word);
> +
> +int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
> +{
> + *valp = 0;
> + where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
> + if (where >= 0)
> + return pci_read_config_dword(dev, where, valp);
> +
> + /*
> +* Quotation from PCIe Base Spec 3.0:
> +* For Functions that do not implement the Slot Capabilities,
> +* Slot Status, and Slot Control registers, these spaces must
> +* be hardwired to 0b, with the exception of the Presence Detect
> +* State bit in the Slot Status register of Downstream Ports,
> +* which must be hardwired to 1b.
> +*/
> + if (pci_is_pcie(dev) && where == PCI_EXP_SLTCTL &&
> + pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
> + *valp = PCI_EXP_SLTSTA_PDS << 16;
Return success here, too.
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(pci_pcie_cap_read_dword);
> +
> +int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
> +{
> + where = pci_pcie_cap_get_offset(dev, where,
[RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
From: Jiang Liu
Introduce four configuration access functions for PCIe capabilities to
hide difference among PCIe Base Spec versions. With these functions,
we can remove callers responsible for using pci_pcie_cap_has_*().
pci_pcie_cap_read_word/dword() functions will store the pcie cap register
value by passed parameter val,if related pcie cap register is not implemented
on the pcie device, the passed parameter val will be set 0 and return -EINVAL.
pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
cap registers,if related pcie cap register is not implemented on the pcie
device, it will return -EINVAL.
Signed-off-by: Jiang Liu
Signed-off-by: Yijing Wang
---
drivers/pci/access.c | 88 ++
include/linux/pci.h | 10 ++
include/linux/pci_regs.h | 19 --
3 files changed, 115 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
raw_spin_unlock_irqrestore(_lock, flags);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+ bool valid;
+
+ if (!pci_is_pcie(dev))
+ return -EINVAL;
+ if (where & (sz - 1))
+ return -EINVAL;
+
+ if (where < 0)
+ valid = false;
+ else if (where < PCI_EXP_DEVCAP)
+ valid = true;
+ else if (where < PCI_EXP_LNKCAP)
+ valid = pci_pcie_cap_has_devctl(dev);
+ else if (where < PCI_EXP_SLTCAP)
+ valid = pci_pcie_cap_has_lnkctl(dev);
+ else if (where < PCI_EXP_RTCTL)
+ valid = pci_pcie_cap_has_sltctl(dev);
+ else if (where < PCI_EXP_DEVCAP2)
+ valid = pci_pcie_cap_has_rtctl(dev);
+ else if (where < PCI_EXP_CAP2_SIZE)
+ valid = pci_pcie_cap_has_cap2(dev);
+ else
+ valid = false;
+
+ return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+ if (where >= 0)
+ return pci_read_config_word(dev, where, valp);
+
+ if (pci_is_pcie(dev) && where == PCI_EXP_SLTSTA &&
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_word);
+
+int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+ if (where >= 0)
+ return pci_read_config_dword(dev, where, valp);
+
+ /*
+* Quotation from PCIe Base Spec 3.0:
+* For Functions that do not implement the Slot Capabilities,
+* Slot Status, and Slot Control registers, these spaces must
+* be hardwired to 0b, with the exception of the Presence Detect
+* State bit in the Slot Status register of Downstream Ports,
+* which must be hardwired to 1b.
+*/
+ if (pci_is_pcie(dev) && where == PCI_EXP_SLTCTL &&
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS << 16;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_dword);
+
+int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
+{
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+ if (where >= 0)
+ return pci_write_config_word(dev, where, val);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_word);
+
+int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val)
+{
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+ if (where >= 0)
+ return pci_write_config_dword(dev, where, val);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 346b2d9..78767b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct
pci_dev *pdev)
type == PCI_EXP_TYPE_RC_EC;
}
+extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
+extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
+extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
+extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
+
void pci_request_acs(void);
bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
bool pci_acs_path_enabled(struct pci_dev *start,
@@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct
pci_dev *pdev)
*/
struct pci_dev
[RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
From: Jiang Liu jiang@huawei.com
Introduce four configuration access functions for PCIe capabilities to
hide difference among PCIe Base Spec versions. With these functions,
we can remove callers responsible for using pci_pcie_cap_has_*().
pci_pcie_cap_read_word/dword() functions will store the pcie cap register
value by passed parameter val,if related pcie cap register is not implemented
on the pcie device, the passed parameter val will be set 0 and return -EINVAL.
pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
cap registers,if related pcie cap register is not implemented on the pcie
device, it will return -EINVAL.
Signed-off-by: Jiang Liu liu...@gmail.com
Signed-off-by: Yijing Wang wangyij...@huawei.com
---
drivers/pci/access.c | 88 ++
include/linux/pci.h | 10 ++
include/linux/pci_regs.h | 19 --
3 files changed, 115 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
raw_spin_unlock_irqrestore(pci_lock, flags);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+ bool valid;
+
+ if (!pci_is_pcie(dev))
+ return -EINVAL;
+ if (where (sz - 1))
+ return -EINVAL;
+
+ if (where 0)
+ valid = false;
+ else if (where PCI_EXP_DEVCAP)
+ valid = true;
+ else if (where PCI_EXP_LNKCAP)
+ valid = pci_pcie_cap_has_devctl(dev);
+ else if (where PCI_EXP_SLTCAP)
+ valid = pci_pcie_cap_has_lnkctl(dev);
+ else if (where PCI_EXP_RTCTL)
+ valid = pci_pcie_cap_has_sltctl(dev);
+ else if (where PCI_EXP_DEVCAP2)
+ valid = pci_pcie_cap_has_rtctl(dev);
+ else if (where PCI_EXP_CAP2_SIZE)
+ valid = pci_pcie_cap_has_cap2(dev);
+ else
+ valid = false;
+
+ return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+ if (where = 0)
+ return pci_read_config_word(dev, where, valp);
+
+ if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_word);
+
+int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+ if (where = 0)
+ return pci_read_config_dword(dev, where, valp);
+
+ /*
+* Quotation from PCIe Base Spec 3.0:
+* For Functions that do not implement the Slot Capabilities,
+* Slot Status, and Slot Control registers, these spaces must
+* be hardwired to 0b, with the exception of the Presence Detect
+* State bit in the Slot Status register of Downstream Ports,
+* which must be hardwired to 1b.
+*/
+ if (pci_is_pcie(dev) where == PCI_EXP_SLTCTL
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS 16;
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_dword);
+
+int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
+{
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+ if (where = 0)
+ return pci_write_config_word(dev, where, val);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_word);
+
+int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val)
+{
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+ if (where = 0)
+ return pci_write_config_dword(dev, where, val);
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_write_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 346b2d9..78767b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const struct
pci_dev *pdev)
type == PCI_EXP_TYPE_RC_EC;
}
+extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp);
+extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp);
+extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
+extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32 val);
+
void pci_request_acs(void);
bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
bool pci_acs_path_enabled(struct pci_dev *start,
@@ -1843,5 +1848,10 @@ static inline struct eeh_dev *pci_dev_to_eeh_dev(struct
pci_dev *pdev)
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Tue, Jul 10, 2012 at 9:54 AM, Jiang Liu liu...@gmail.com wrote:
From: Jiang Liu jiang@huawei.com
Introduce four configuration access functions for PCIe capabilities to
hide difference among PCIe Base Spec versions. With these functions,
we can remove callers responsible for using pci_pcie_cap_has_*().
pci_pcie_cap_read_word/dword() functions will store the pcie cap register
value by passed parameter val,if related pcie cap register is not implemented
on the pcie device, the passed parameter val will be set 0 and return -EINVAL.
pci_pcie_capability_write_word/dowrd() functions will write the value to pcie
cap registers,if related pcie cap register is not implemented on the pcie
device, it will return -EINVAL.
Signed-off-by: Jiang Liu liu...@gmail.com
Signed-off-by: Yijing Wang wangyij...@huawei.com
---
drivers/pci/access.c | 88
++
include/linux/pci.h | 10 ++
include/linux/pci_regs.h | 19 --
3 files changed, 115 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
raw_spin_unlock_irqrestore(pci_lock, flags);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+ bool valid;
+
+ if (!pci_is_pcie(dev))
+ return -EINVAL;
+ if (where (sz - 1))
+ return -EINVAL;
+
+ if (where 0)
+ valid = false;
+ else if (where PCI_EXP_DEVCAP)
+ valid = true;
+ else if (where PCI_EXP_LNKCAP)
+ valid = pci_pcie_cap_has_devctl(dev);
+ else if (where PCI_EXP_SLTCAP)
+ valid = pci_pcie_cap_has_lnkctl(dev);
+ else if (where PCI_EXP_RTCTL)
+ valid = pci_pcie_cap_has_sltctl(dev);
+ else if (where PCI_EXP_DEVCAP2)
+ valid = pci_pcie_cap_has_rtctl(dev);
+ else if (where PCI_EXP_CAP2_SIZE)
+ valid = pci_pcie_cap_has_cap2(dev);
+ else
+ valid = false;
+
+ return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
This is a really slick factorization; I like it much better than my
proposal. I would like it even *better* if it read something like
this:
bool implemented;
*valp = 0;
if (!pci_is_pcie(dev) || where 1)
return -EINVAL;
implemented = pci_pcie_cap_implemented(dev, where);
if (implemented)
return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA ...
because I think it's useful to have the pos + where visual pattern
in the pci_read_config_word() arguments.
+ if (where = 0)
+ return pci_read_config_word(dev, where, valp);
+
+ if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS;
I think we should be returning success in this case (SLTSTA for
downstream port). In fact, I think we should return success even when
we're emulating the read of an unimplemented register from a v1
capability. The caller should not be aware at all that there is a
difference between v1 and v2 capabilities.
I'd put the spec reference here rather than in read_dword(), since
SLTSTA is a u16 and this is the natural way to read it. Then maybe a
short comment in read_dword() below.
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_word);
+
+int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u32));
+ if (where = 0)
+ return pci_read_config_dword(dev, where, valp);
+
+ /*
+* Quotation from PCIe Base Spec 3.0:
+* For Functions that do not implement the Slot Capabilities,
+* Slot Status, and Slot Control registers, these spaces must
+* be hardwired to 0b, with the exception of the Presence Detect
+* State bit in the Slot Status register of Downstream Ports,
+* which must be hardwired to 1b.
+*/
+ if (pci_is_pcie(dev) where == PCI_EXP_SLTCTL
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS 16;
Return success here, too.
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(pci_pcie_cap_read_dword);
+
+int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val)
+{
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
+ if (where = 0)
+ return
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On 2012-7-11 2:35, Bjorn Helgaas wrote:
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
raw_spin_unlock_irqrestore(pci_lock, flags);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+ bool valid;
+
+ if (!pci_is_pcie(dev))
+ return -EINVAL;
+ if (where (sz - 1))
+ return -EINVAL;
+
+ if (where 0)
+ valid = false;
+ else if (where PCI_EXP_DEVCAP)
+ valid = true;
+ else if (where PCI_EXP_LNKCAP)
+ valid = pci_pcie_cap_has_devctl(dev);
+ else if (where PCI_EXP_SLTCAP)
+ valid = pci_pcie_cap_has_lnkctl(dev);
+ else if (where PCI_EXP_RTCTL)
+ valid = pci_pcie_cap_has_sltctl(dev);
+ else if (where PCI_EXP_DEVCAP2)
+ valid = pci_pcie_cap_has_rtctl(dev);
+ else if (where PCI_EXP_CAP2_SIZE)
+ valid = pci_pcie_cap_has_cap2(dev);
+ else
+ valid = false;
+
+ return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
This is a really slick factorization; I like it much better than my
proposal. I would like it even *better* if it read something like
this:
bool implemented;
*valp = 0;
if (!pci_is_pcie(dev) || where 1)
return -EINVAL;
implemented = pci_pcie_cap_implemented(dev, where);
if (implemented)
return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA ...
because I think it's useful to have the pos + where visual pattern
in the pci_read_config_word() arguments.
Sure, for better readability.
+ if (where = 0)
+ return pci_read_config_word(dev, where, valp);
+
+ if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS;
I think we should be returning success in this case (SLTSTA for
downstream port). In fact, I think we should return success even when
we're emulating the read of an unimplemented register from a v1
capability. The caller should not be aware at all that there is a
difference between v1 and v2 capabilities.
I'd put the spec reference here rather than in read_dword(), since
SLTSTA is a u16 and this is the natural way to read it. Then maybe a
short comment in read_dword() below.
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?
+EXPORT_SYMBOL(pci_pcie_cap_write_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 346b2d9..78767b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const
struct pci_dev *pdev)
type == PCI_EXP_TYPE_RC_EC;
}
+extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16
*valp);
+extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32
*valp);
+extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16 val);
+extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32
val);
You don't need the extern here (and I think you'll probably remove
these altogether, see below).
+
void pci_request_acs(void);
bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
bool pci_acs_path_enabled(struct pci_dev *start,
@@ -1843,5 +1848,10 @@ static inline struct eeh_dev
*pci_dev_to_eeh_dev(struct pci_dev *pdev)
*/
struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16 *val);
+int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32
*val);
+int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16 val);
+int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32
val);
There's some confusion here: pci_pcie_cap_* versus
pci_pcie_capability_*. I think you only need one set, and I prefer
pci_pcie_capability_* to follow the example of
pci_bus_find_capability().
The above confusion was caused by a dirty merge.
+
#endif /* __KERNEL__ */
#endif /* LINUX_PCI_H */
diff --git a/include/linux/pci_regs.h b/include/linux/pci_regs.h
index 53274bf..ac60e22 100644
--- a/include/linux/pci_regs.h
+++ b/include/linux/pci_regs.h
@@ -542,9 +542,24 @@
#define PCI_EXP_OBFF_MSGA_EN 0x2000 /* OBFF enable with Message type A */
#define
Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
On Tue, Jul 10, 2012 at 9:07 PM, Jiang Liu jiang@huawei.com wrote:
On 2012-7-11 2:35, Bjorn Helgaas wrote:
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba91a7e..80ae022 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -469,3 +469,91 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
raw_spin_unlock_irqrestore(pci_lock, flags);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);
+
+static int
+pci_pcie_cap_get_offset(struct pci_dev *dev, int where, size_t sz)
+{
+ bool valid;
+
+ if (!pci_is_pcie(dev))
+ return -EINVAL;
+ if (where (sz - 1))
+ return -EINVAL;
+
+ if (where 0)
+ valid = false;
+ else if (where PCI_EXP_DEVCAP)
+ valid = true;
+ else if (where PCI_EXP_LNKCAP)
+ valid = pci_pcie_cap_has_devctl(dev);
+ else if (where PCI_EXP_SLTCAP)
+ valid = pci_pcie_cap_has_lnkctl(dev);
+ else if (where PCI_EXP_RTCTL)
+ valid = pci_pcie_cap_has_sltctl(dev);
+ else if (where PCI_EXP_DEVCAP2)
+ valid = pci_pcie_cap_has_rtctl(dev);
+ else if (where PCI_EXP_CAP2_SIZE)
+ valid = pci_pcie_cap_has_cap2(dev);
+ else
+ valid = false;
+
+ return valid ? where + pci_pcie_cap(dev) : -EINVAL;
+}
+
+int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16 *valp)
+{
+ *valp = 0;
+ where = pci_pcie_cap_get_offset(dev, where, sizeof(u16));
This is a really slick factorization; I like it much better than my
proposal. I would like it even *better* if it read something like
this:
bool implemented;
*valp = 0;
if (!pci_is_pcie(dev) || where 1)
return -EINVAL;
implemented = pci_pcie_cap_implemented(dev, where);
if (implemented)
return pci_read_config_word(dev, pci_pcie_cap(dev) + where, valp);
if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA ...
because I think it's useful to have the pos + where visual pattern
in the pci_read_config_word() arguments.
Sure, for better readability.
+ if (where = 0)
+ return pci_read_config_word(dev, where, valp);
+
+ if (pci_is_pcie(dev) where == PCI_EXP_SLTSTA
+ pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM)
+ *valp = PCI_EXP_SLTSTA_PDS;
I think we should be returning success in this case (SLTSTA for
downstream port). In fact, I think we should return success even when
we're emulating the read of an unimplemented register from a v1
capability. The caller should not be aware at all that there is a
difference between v1 and v2 capabilities.
I'd put the spec reference here rather than in read_dword(), since
SLTSTA is a u16 and this is the natural way to read it. Then maybe a
short comment in read_dword() below.
Good point. Return success when reading unimplemented registeres, that
may simplify code. For we still should return -EINVAL when writing
unimplemented registers, right?
Yeah, I guess it's OK to return -EINVAL when *writing* to an
unimplemented register. Hopefully the caller is structured such that
we don't even try to write in that case. It'd be interesting to audit
the callers and explore that, but I haven't done that.
+EXPORT_SYMBOL(pci_pcie_cap_write_dword);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 346b2d9..78767b2 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1703,6 +1703,11 @@ static inline bool pci_pcie_cap_has_rtctl(const
struct pci_dev *pdev)
type == PCI_EXP_TYPE_RC_EC;
}
+extern int pci_pcie_cap_read_word(struct pci_dev *dev, int where, u16
*valp);
+extern int pci_pcie_cap_read_dword(struct pci_dev *dev, int where, u32
*valp);
+extern int pci_pcie_cap_write_word(struct pci_dev *dev, int where, u16
val);
+extern int pci_pcie_cap_write_dword(struct pci_dev *dev, int where, u32
val);
You don't need the extern here (and I think you'll probably remove
these altogether, see below).
+
void pci_request_acs(void);
bool pci_acs_enabled(struct pci_dev *pdev, u16 acs_flags);
bool pci_acs_path_enabled(struct pci_dev *start,
@@ -1843,5 +1848,10 @@ static inline struct eeh_dev
*pci_dev_to_eeh_dev(struct pci_dev *pdev)
*/
struct pci_dev *pci_find_upstream_pcie_bridge(struct pci_dev *pdev);
+int pci_pcie_capability_read_word(struct pci_dev *dev, int where, u16
*val);
+int pci_pcie_capability_read_dword(struct pci_dev *dev, int where, u32
*val);
+int pci_pcie_capability_write_word(struct pci_dev *dev, int where, u16
val);
+int pci_pcie_capability_write_dword(struct pci_dev *dev, int where, u32
val);
There's some confusion here: pci_pcie_cap_* versus
pci_pcie_capability_*. I think you only need one set, and I prefer
pci_pcie_capability_* to follow the example of
pci_bus_find_capability().
The above confusion was caused by a dirty merge.
