From: Bjorn Helgaas <bhelgaas@google.com>
To: Jiang Liu <liuj97@gmail.com>
Cc: Jiang Liu <jiang.liu@huawei.com>, Don Dutile <ddutile@redhat.com>,
Yinghai Lu <yinghai@kernel.org>,
Taku Izumi <izumi.taku@jp.fujitsu.com>,
"Rafael J . Wysocki" <rjw@sisk.pl>,
Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>,
Yijing Wang <wangyijing@huawei.com>,
Keping Chen <chenkeping@huawei.com>,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org
Subject: Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences
Date: Mon, 16 Jul 2012 18:14:25 -0600 [thread overview]
Message-ID: <CAErSpo6NVx0Vp9U5mY0cMPhrQFDVEVPZKnoMOjN=5hRz2DJ9hQ@mail.gmail.com> (raw)
In-Reply-To: <5004AD3A.7060701@gmail.com>
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
next prev parent reply other threads:[~2012-07-17 0:14 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-04 7:44 [Resend with Ack][PATCH v1] PCI: allow acpiphp to handle PCIe ports without native PCIe hotplug capability Jiang Liu
2012-06-04 8:23 ` Kenji Kaneshige
2012-07-03 4:16 ` Bjorn Helgaas
2012-07-03 15:59 ` Bjorn Helgaas
2012-07-03 19:50 ` Don Dutile
2012-07-04 18:07 ` Bjorn Helgaas
2012-07-09 10:05 ` Jiang Liu
2012-07-09 17:05 ` Bjorn Helgaas
2012-07-04 2:52 ` Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 00/14] improve PCIe capabilities registers handling Jiang Liu
2012-07-10 18:44 ` Bjorn Helgaas
2012-07-10 15:54 ` [RFC PATCH 01/14] PCI: add pcie_flags into struct pci_dev to cache PCIe capabilities register Jiang Liu
2012-07-11 9:01 ` Taku Izumi
2012-07-11 14:27 ` Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 02/14] PCI: introduce pci_pcie_type(dev) to replace pci_dev->pcie_type Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 03/14] PCI: remove unused field pcie_type from struct pci_dev Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 04/14] PCI: refine and move pcie_cap_has_*() macros to include/linux/pci.h Jiang Liu
2012-07-10 18:49 ` Bjorn Helgaas
2012-07-10 15:54 ` [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences Jiang Liu
2012-07-10 18:35 ` Bjorn Helgaas
2012-07-11 3:07 ` Jiang Liu
2012-07-11 3:40 ` Bjorn Helgaas
2012-07-11 6:40 ` Jiang Liu
2012-07-11 17:52 ` Bjorn Helgaas
2012-07-12 2:56 ` Jiang Liu
2012-07-12 20:49 ` Bjorn Helgaas
2012-07-15 16:47 ` Jiang Liu
2012-07-16 17:29 ` Bjorn Helgaas
2012-07-16 18:57 ` Don Dutile
2012-07-17 0:09 ` Jiang Liu
2012-07-17 0:14 ` Bjorn Helgaas [this message]
2012-07-10 15:54 ` [RFC PATCH 06/14] PCI: use PCIe cap access functions to simplify PCI core implementation Jiang Liu
2012-07-10 18:35 ` Bjorn Helgaas
2012-07-11 2:49 ` Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 07/14] hotplug/PCI: use PCIe cap access functions to simplify implementation Jiang Liu
2012-07-10 18:35 ` Bjorn Helgaas
2012-07-10 15:54 ` [RFC PATCH 08/14] portdrv/PCI: " Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 09/14] pciehp/PCI: " Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 10/14] PME/PCI: " Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 11/14] AER/PCI: " Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 12/14] ASPM/PCI: " Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 13/14] r8169/PCI: " Jiang Liu
2012-07-10 15:54 ` [RFC PATCH 14/14] qib/PCI: " Jiang Liu
2012-08-15 19:12 ` [Resend with Ack][PATCH v1] PCI: allow acpiphp to handle PCIe ports without native PCIe hotplug capability Bjorn Helgaas
2012-08-16 15:15 ` Jiang Liu
2012-08-22 15:16 ` [PATCH v2] PCI: allow acpiphp to handle PCIe ports w/o " Jiang Liu
2012-09-24 22:10 ` Bjorn Helgaas
2012-09-25 15:16 ` Jiang Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAErSpo6NVx0Vp9U5mY0cMPhrQFDVEVPZKnoMOjN=5hRz2DJ9hQ@mail.gmail.com' \
--to=bhelgaas@google.com \
--cc=chenkeping@huawei.com \
--cc=ddutile@redhat.com \
--cc=izumi.taku@jp.fujitsu.com \
--cc=jiang.liu@huawei.com \
--cc=kaneshige.kenji@jp.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=liuj97@gmail.com \
--cc=rjw@sisk.pl \
--cc=wangyijing@huawei.com \
--cc=yinghai@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).