On 07/16/2012 01:29 PM, Bjorn Helgaas wrote:
On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu<liuj97@xxxxxxxxx> 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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
