Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities to hide PCIe spec differences

[Bjorn Helgaas <bhelgaas@google.com>]

On Wed, Jul 11, 2012 at 8:56 PM, Jiang Liu <jiang.liu@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.
>