| 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: <293b90ca-dab1-eef3-e718-c93295442d9@linux.intel.com>
Date: Wed, 18 Oct 2023 17:16:36 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Serge Semin <fancer.lancer@...il.com>
cc: Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Rob Herring <robh@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Jingoo Han <jingoohan1@...il.com>,
Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
Manivannan Sadhasivam <mani@...nel.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
LKML <linux-kernel@...r.kernel.org>, linux-tegra@...r.kernel.org
Subject: Re: [PATCH 2/7] PCI: dwc: Use FIELD_GET/PREP()
On Wed, 18 Oct 2023, Serge Semin wrote:
> On Wed, Oct 18, 2023 at 02:32:49PM +0300, Ilpo Järvinen wrote:
> > Convert open-coded variants of PCI field access into FIELD_GET/PREP()
> > to make the code easier to understand.
> >
> > Add two missing defines into pci_regs.h. Logically, the Max No-Snoop
> > Latency Register is a separate word sized register in the PCIe spec,
> > but the pre-existing LTR defines in pci_regs.h with dword long values
> > seem to consider the registers together (the same goes for the only
> > user). Thus, follow the custom and make the new values also take both
> > word long LTR registers as a joint dword register.
>
> Nice work. Thanks! Could you also have a look at
> drivers/pci/controller/dwc/pcie-designware.c
> ?
> It contains two open-coded patterns:
> (bar << 8) - FIELD_PREP()
> next_cap_ptr = (reg & 0xff00) >> 8; - FIELD_GET().
> next_cap_ptr = (reg & 0x00ff); - FIELD_GET().
> At least the later two statements concern the generic PCIe capability CSR.
The problem with cap id / next cap is that there are currently no defines
for them AFAICT, at least not in pci_regs.h. And pci_regs.h defines those
as different registers so if I'm to add defines from them, I don't know
which size would be the most appropriate since that 0xff00 goes across
that register boundary. I've not had time to study the related core code
yet but I intend to take a look at it to see what's the best course of
action forward.
--
i.
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > ---
> > drivers/pci/controller/dwc/pcie-designware-ep.c | 7 ++++---
> > drivers/pci/controller/dwc/pcie-tegra194.c | 5 ++---
> > include/uapi/linux/pci_regs.h | 2 ++
> > 3 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index f9182f8d552f..20bef1436bfb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -6,6 +6,7 @@
> > * Author: Kishon Vijay Abraham I <kishon@...com>
> > */
> >
> > +#include <linux/bitfield.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> >
> > @@ -334,7 +335,7 @@ static int dw_pcie_ep_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
> > if (!(val & PCI_MSI_FLAGS_ENABLE))
> > return -EINVAL;
> >
> > - val = (val & PCI_MSI_FLAGS_QSIZE) >> 4;
> > + val = FIELD_GET(PCI_MSI_FLAGS_QSIZE, val);
> >
> > return val;
> > }
> > @@ -357,7 +358,7 @@ static int dw_pcie_ep_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
> > reg = ep_func->msi_cap + func_offset + PCI_MSI_FLAGS;
> > val = dw_pcie_readw_dbi(pci, reg);
> > val &= ~PCI_MSI_FLAGS_QMASK;
> > - val |= (interrupts << 1) & PCI_MSI_FLAGS_QMASK;
> > + val |= FIELD_PREP(PCI_MSI_FLAGS_QMASK, interrupts);
> > dw_pcie_dbi_ro_wr_en(pci);
> > dw_pcie_writew_dbi(pci, reg, val);
> > dw_pcie_dbi_ro_wr_dis(pci);
> > @@ -584,7 +585,7 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
> >
> > reg = ep_func->msix_cap + func_offset + PCI_MSIX_TABLE;
> > tbl_offset = dw_pcie_readl_dbi(pci, reg);
> > - bir = (tbl_offset & PCI_MSIX_TABLE_BIR);
> > + bir = FIELD_GET(PCI_MSIX_TABLE_BIR, tbl_offset);
> > tbl_offset &= PCI_MSIX_TABLE_OFFSET;
> >
> > msix_tbl = ep->epf_bar[bir]->addr + tbl_offset;
> > diff --git a/drivers/pci/controller/dwc/pcie-tegra194.c b/drivers/pci/controller/dwc/pcie-tegra194.c
> > index 248cd9347e8f..12d5ab2f5219 100644
> > --- a/drivers/pci/controller/dwc/pcie-tegra194.c
> > +++ b/drivers/pci/controller/dwc/pcie-tegra194.c
> > @@ -126,7 +126,6 @@
> >
> > #define APPL_LTR_MSG_1 0xC4
> > #define LTR_MSG_REQ BIT(15)
> > -#define LTR_MST_NO_SNOOP_SHIFT 16
> >
> > #define APPL_LTR_MSG_2 0xC8
> > #define APPL_LTR_MSG_2_LTR_MSG_REQ_STATE BIT(3)
> > @@ -496,8 +495,8 @@ static irqreturn_t tegra_pcie_ep_irq_thread(int irq, void *arg)
> > ktime_t timeout;
> >
> > /* 110us for both snoop and no-snoop */
> > - val = 110 | (2 << PCI_LTR_SCALE_SHIFT) | LTR_MSG_REQ;
> > - val |= (val << LTR_MST_NO_SNOOP_SHIFT);
> > + val = 110 | FIELD_PREP(PCI_LTR_SCALE_SHIFT, 2) | LTR_MSG_REQ;
> > + val |= FIELD_PREP(PCI_LTR_NOSNOOP_VALUE, val);
> > appl_writel(pcie, val, APPL_LTR_MSG_1);
> >
> > /* Send LTR upstream */
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index e5f558d96493..495f0ae4ecd5 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -975,6 +975,8 @@
> > #define PCI_LTR_VALUE_MASK 0x000003ff
> > #define PCI_LTR_SCALE_MASK 0x00001c00
> > #define PCI_LTR_SCALE_SHIFT 10
> > +#define PCI_LTR_NOSNOOP_VALUE 0x03ff0000 /* Max No-Snoop Latency Value */
> > +#define PCI_LTR_NOSNOOP_SCALE 0x1c000000 /* Scale for Max Value */
> > #define PCI_EXT_CAP_LTR_SIZEOF 8
> >
> > /* Access Control Service */
> > --
> > 2.30.2
> >
>
Powered by blists - more mailing lists