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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAErSpo7ZtrYtz_8iB8=8nbQBtntPtyi8BLg6K+D7mQjnfj-KCg@mail.gmail.com>
Date:	Wed, 11 Jul 2012 11:52:05 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Jiang Liu <jiang.liu@...wei.com>
Cc:	Jiang Liu <liuj97@...il.com>, Don Dutile <ddutile@...hat.com>,
	Yinghai Lu <yinghai@...nel.org>,
	Taku Izumi <izumi.taku@...fujitsu.com>,
	"Rafael J . Wysocki" <rjw@...k.pl>,
	Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>,
	Yijing Wang <wangyijing@...wei.com>,
	Keping Chen <chenkeping@...wei.com>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [RFC PATCH 05/14] PCI: add access functions for PCIe capabilities
 to hide PCIe spec differences

On Wed, Jul 11, 2012 at 12:40 AM, Jiang Liu <jiang.liu@...wei.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/