| 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
| ||
|
Message-ID: <64a0049e-00dc-8b10-6c9a-0b270c0a181d@nvidia.com>
Date: Wed, 28 Jul 2021 16:58:14 -0500
From: Shanker R Donthineni <sdonthineni@...dia.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Amey Narkhede <ameynarkhede03@...il.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
<alex.williamson@...hat.com>,
Raphael Norwitz <raphael.norwitz@...anix.com>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kw@...ux.com>, Sinan Kaya <okaya@...nel.org>,
Len Brown <lenb@...nel.org>,
"Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v10 1/8] PCI: Add pcie_reset_flr to follow calling
convention of other reset methods
Hi Bjorn,
On 7/28/21 3:23 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Jul 28, 2021 at 01:54:16PM -0500, Shanker R Donthineni wrote:
>> On 7/27/21 5:12 PM, Bjorn Helgaas wrote:
>>> On Fri, Jul 09, 2021 at 06:08:06PM +0530, Amey Narkhede wrote:
>>>> Add has_pcie_flr bitfield in struct pci_dev to indicate support for PCIe
>>>> FLR to avoid reading PCI_EXP_DEVCAP multiple times.
>>>>
>>>> Currently there is separate function pcie_has_flr() to probe if PCIe FLR
>>>> is supported by the device which does not match the calling convention
>>>> followed by reset methods which use second function argument to decide
>>>> whether to probe or not. Add new function pcie_reset_flr() that follows
>>>> the calling convention of reset methods.
>>>>
>>>> Signed-off-by: Amey Narkhede <ameynarkhede03@...il.com>
>>>> ---
>>>> drivers/crypto/cavium/nitrox/nitrox_main.c | 4 +-
>>>> drivers/pci/pci.c | 59 +++++++++++-----------
>>>> drivers/pci/pcie/aer.c | 12 ++---
>>>> drivers/pci/probe.c | 6 ++-
>>>> drivers/pci/quirks.c | 9 ++--
>>>> include/linux/pci.h | 3 +-
>>>> 6 files changed, 45 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/drivers/crypto/cavium/nitrox/nitrox_main.c b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>> index facc8e6bc..15d6c8452 100644
>>>> --- a/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>> +++ b/drivers/crypto/cavium/nitrox/nitrox_main.c
>>>> @@ -306,9 +306,7 @@ static int nitrox_device_flr(struct pci_dev *pdev)
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> - /* check flr support */
>>>> - if (pcie_has_flr(pdev))
>>>> - pcie_flr(pdev);
>>>> + pcie_reset_flr(pdev, 0);
>>> I'm not really a fan of exposing the "probe" argument outside
>>> drivers/pci/. I think this would be the only occurrence. Is there a
>>> way to avoid that?
>>>
>>> Can we just make pcie_flr() do the equivalent of pcie_has_flr()
>>> internally?
>>>
>> I like your suggestion don't change the existing definition of
>> pcie_has_flr()/pcie_flr() and define a new function pcie_reset_flr()
>> to fit into the reset framework. This way no need to modify
>> logic/drivers outside of driver/pci/xxx.
>>
>> int pcie_reset_flr(struct pci_dev *dev, int probe)
>> {
>> if (!pcie_has_flr(dev))
>> return -ENOTTY;
>>
>> if (probe)
>> return 0;
>>
>> return pcie_flr(dev);
>> }
> Can't remember the whole context of this in the series, but I assume
> this would be static?
It should be static since it's referenced in driver/pci/qrirk.c and aer.c.
>> And add a new patch to begging of the series for caching 'devcap' in
>> pci_dev structure.
>>
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -333,6 +333,7 @@ struct pci_dev {
>> struct rcec_ea *rcec_ea; /* RCEC cached endpoint association */
>> struct pci_dev *rcec; /* Associated RCEC device */
>> #endif
>> + u32 devcap; /* Cached PCIe device capabilities */
>> u8 pcie_cap; /* PCIe capability offset */
>> u8 msi_cap; /* MSI capability offset */
>> u8 msix_cap; /* MSI-X capability offset */
>>
>>
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -31,6 +31,7 @@
>> #include <linux/vmalloc.h>
>> #include <asm/dma.h>
>> #include <linux/aer.h>
>> +#include <linux/bitfield.h>
>> #include "pci.h"
>>
>> DEFINE_MUTEX(pci_slot_mutex);
>> @@ -4630,13 +4631,10 @@ EXPORT_SYMBOL(pci_wait_for_pending_transaction);
>> */
>> bool pcie_has_flr(struct pci_dev *dev)
>> {
>> - u32 cap;
>> -
>> if (dev->dev_flags & PCI_DEV_FLAGS_NO_FLR_RESET)
>> return false;
>>
>> - pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap);
>> - return cap & PCI_EXP_DEVCAP_FLR;
>> + return !!FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap);
> Nice, thanks for reminding me of FIELD_GET(). I like how that works
> without having to #define *_SHIFT values. I personally don't care for
> "!!" and would probably write something like:
>
> return FIELD_GET(PCI_EXP_DEVCAP_FLR, dev->devcap) == 1;
Both are same since FLR is a single bit value.
>> }
>>
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -19,6 +19,7 @@
>> #include <linux/hypervisor.h>
>> #include <linux/irqdomain.h>
>> #include <linux/pm_runtime.h>
>> +#include <linux/bitfield.h>
>> #include "pci.h"
>>
>> #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
>> @@ -1498,8 +1499,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
>> pdev->pcie_cap = pos;
>> pci_read_config_word(pdev, pos + PCI_EXP_FLAGS, ®16);
>> pdev->pcie_flags_reg = reg16;
>> - pci_read_config_word(pdev, pos + PCI_EXP_DEVCAP, ®16);
>> - pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
>> + pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
>> + pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
>>
Powered by blists - more mailing lists