linux-kernel - Re: [PATCH v3 00/32] provide interfaces to access PCIe capabilities registers

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]

Date:	Mon, 20 Aug 2012 09:35:18 -0600
From:	Bjorn Helgaas <bhelgaas@...gle.com>
To:	Jiang Liu <liuj97@...il.com>
Cc:	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>,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 00/32] provide interfaces to access PCIe capabilities registers

On Mon, Aug 20, 2012 at 9:26 AM, Jiang Liu <liuj97@...il.com> wrote:
> On 08/14/2012 12:25 PM, Bjorn Helgaas wrote:
>> On Wed, Aug 1, 2012 at 8:54 AM, Jiang Liu <liuj97@...il.com> wrote:
>>> From: Jiang Liu <liuj97@...il.com>
>>>
>>> As suggested by Bjorn Helgaas and Don Dutile in threads
>>> http://www.spinics.net/lists/linux-pci/msg15663.html, we could improve access
>>> to PCIe capabilities register in to way:
>>> 1) cache content of PCIe Capabilities Register into struct pce_dev to avoid
>>>    repeatedly reading this register because it's read only.
>>> 2) provide access functions for PCIe Capabilities registers to hide differences
>>>    among PCIe base specifications, so the caller don't need to handle those
>>>    differences.
>>>
>>> This patch set applies to
>>> git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci-next
>>
>> Would you mind rebasing this to v3.6-rc1?  I think you posted this
>> when my branch was still 3.5-based, and there are some upstream
>> changes that cause minor conflicts here.
>>
>> You currently have:
>>
>>     int pci_pcie_capability_change_word(struct pci_dev *dev, int pos,
>> u16 set_bits, u16 clear_bits)
>>
>> I think this is a bit awkward because the function name doesn't
>> suggest *how* the word will be changed, and the clearing happens
>> before the setting (opposite the parameter order).  Something like:
>>
>>     int pci_pcie_capability_mask_and_set_word(..., u16 mask, u16 set) or
>>     int pci_pcie_capability_clear_and_set_word(..., u16 clear, u16 set)
>>
>> would be more obvious.  If you use "mask_and_set", I think the
>> function should do "(val & mask) | set" with the complement being at
>> the call site.  If you use "clear_and_set", I think it's OK to do
>> "(val & ~mask) | set" as in your current patch.
>>
>> I know I suggested the "pci_pcie_capability_*" names, but they're
>> getting a bit unwieldy, especially if we do "mask_and_set" or similar.
>>  There are already several "pcie_*" functions, so maybe we should
>> drop the leading "pci_" from these and just have:
>>
>>     pcie_capability_read_word
>>     pcie_capability_write_word
>>     pcie_capability_mask_and_set_word
>>
>> Bjorn
>>
> Hi Bjorn,
>         I have made following changes according to your suggestions,
>         1) get rid of the "pci_" prefix for access functions.
>         2) rename pci_pcie_capability_change_{word|dword}() to
>         pcie_capability_clear_and_set_{word|dword}.
>         3) add pcie_capability_{set|clear}_{word|dword}().

Are 2) and 3) really the same?  If they're really different, we'll end up with:

    pcie_capability_clear_and_set_word()
    pcie_capability_clear_and_set_dword()
    pcie_capability_set_word()
    pcie_capability_set_dword()
    pcie_capability_clear_word()
    pcie_capability_clear_dword()

It seems a little excessive to have all six interfaces, since the
first two are sufficient to provide all the functionality.

>         4) Add "Acked-by" and "Reviewed-by"
>         5) rebase to your latest pci-next tree
>
>         So could you please help to pull from "https://github.com/jiangliu/linux.git topic/pcie-cap"
> or should I send all the patches to mail list again?

Just let me know what you think about the above, and I'll try pulling
from your tree.  We're done to nits, and it hardly seems worthwhile to
flood LKML with 32 more almost-identical patches.

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/