lkml.org 
[lkml]   [2012]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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 <liuj97@gmail.com> wrote:
    > On 07/17/2012 01:29 AM, Bjorn Helgaas wrote:
    >> On Sun, Jul 15, 2012 at 10:47 AM, Jiang Liu <liuj97@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


    \
     
     \ /
      Last update: 2012-07-17 02:41    [W:2.450 / U:0.068 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site