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

From: Bjorn Helgaas <bhelgaas@google.com>
To: Jiang Liu <jiang.liu@huawei.com>
Cc: Jiang Liu <liuj97@gmail.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: Wed, 11 Jul 2012 11:52:05 -0600	[thread overview]
Message-ID: <CAErSpo7ZtrYtz_8iB8=8nbQBtntPtyi8BLg6K+D7mQjnfj-KCg@mail.gmail.com> (raw)
In-Reply-To: <4FFD1FE7.6010504@huawei.com>

On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu <jiang.liu@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