[PATCH 00/10] PCI: Use FIELD_GET() and FIELD_PREP()

[PATCH 00/10] PCI: Use FIELD_GET() and FIELD_PREP()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() and FIELD_PREP() to avoid the need for shifting.

These apply on top of the PCI patches from Ilpo's series:
  https://lore.kernel.org/r/20230919125648.1920-1-ilpo.jarvinen@linux.intel.com

Bjorn Helgaas (10):
  PCI: Use FIELD_GET()
  PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk
  PCI/ASPM: Use FIELD_GET()
  PCI/ATS: Show PASID Capability register width in bitmasks
  PCI/ATS: Use FIELD_GET()
  PCI/DPC: Use FIELD_GET()
  PCI/PME: Use FIELD_GET()
  PCI/PTM: Use FIELD_GET()
  PCI/VC: Use FIELD_GET()
  PCI/portdrv: Use FIELD_GET()

 drivers/pci/ats.c             |  7 ++---
 drivers/pci/pci.c             | 53 +++++++++++++++++------------------
 drivers/pci/pcie/aspm.c       | 31 +++++++++++---------
 drivers/pci/pcie/dpc.c        |  9 +++---
 drivers/pci/pcie/pme.c        |  4 ++-
 drivers/pci/pcie/portdrv.c    |  7 +++--
 drivers/pci/pcie/ptm.c        |  5 ++--
 drivers/pci/probe.c           |  8 +++---
 drivers/pci/quirks.c          |  2 +-
 drivers/pci/vc.c              |  9 +++---
 include/uapi/linux/pci_regs.h | 15 ++++++----
 11 files changed, 81 insertions(+), 69 deletions(-)

-- 
2.34.1

[PATCH 01/10] PCI: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c   | 45 ++++++++++++++++++++++-----------------------
 drivers/pci/probe.c |  8 ++++----
 2 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a8adc34dc86f..848c9ee65d7f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1776,8 +1776,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
 		return;
 
 	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
-	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
-		    PCI_REBAR_CTRL_NBAR_SHIFT;
+	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
 
 	for (i = 0; i < nbars; i++, pos += 8) {
 		struct resource *res;
@@ -1788,7 +1787,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
 		res = pdev->resource + bar_idx;
 		size = pci_rebar_bytes_to_size(resource_size(res));
 		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
-		ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
+		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
 		pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
 	}
 }
@@ -3229,7 +3228,7 @@ void pci_pm_init(struct pci_dev *dev)
 			 (pmc & PCI_PM_CAP_PME_D2) ? " D2" : "",
 			 (pmc & PCI_PM_CAP_PME_D3hot) ? " D3hot" : "",
 			 (pmc & PCI_PM_CAP_PME_D3cold) ? " D3cold" : "");
-		dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
+		dev->pme_support = FIELD_GET(PCI_PM_CAP_PME_MASK, pmc);
 		dev->pme_poll = true;
 		/*
 		 * Make device's PM flags reflect the wake-up capability, but
@@ -3300,20 +3299,20 @@ static int pci_ea_read(struct pci_dev *dev, int offset)
 	ent_offset += 4;
 
 	/* Entry size field indicates DWORDs after 1st */
-	ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
+	ent_size = (FIELD_GET(PCI_EA_ES, dw0) + 1) << 2;
 
 	if (!(dw0 & PCI_EA_ENABLE)) /* Entry not enabled */
 		goto out;
 
-	bei = (dw0 & PCI_EA_BEI) >> 4;
-	prop = (dw0 & PCI_EA_PP) >> 8;
+	bei = FIELD_GET(PCI_EA_BEI, dw0);
+	prop = FIELD_GET(PCI_EA_PP, dw0);
 
 	/*
 	 * If the Property is in the reserved range, try the Secondary
 	 * Property instead.
 	 */
 	if (prop > PCI_EA_P_BRIDGE_IO && prop < PCI_EA_P_MEM_RESERVED)
-		prop = (dw0 & PCI_EA_SP) >> 16;
+		prop = FIELD_GET(PCI_EA_SP, dw0);
 	if (prop > PCI_EA_P_BRIDGE_IO)
 		goto out;
 
@@ -3720,14 +3719,13 @@ static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
 		return -ENOTSUPP;
 
 	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
-	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
-		    PCI_REBAR_CTRL_NBAR_SHIFT;
+	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
 
 	for (i = 0; i < nbars; i++, pos += 8) {
 		int bar_idx;
 
 		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
-		bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
+		bar_idx = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, ctrl);
 		if (bar_idx == bar)
 			return pos;
 	}
@@ -3782,7 +3780,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int bar)
 		return pos;
 
 	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
-	return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
+	return FIELD_GET(PCI_REBAR_CTRL_BAR_SIZE, ctrl);
 }
 
 /**
@@ -3805,7 +3803,7 @@ int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size)
 
 	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
 	ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
-	ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
+	ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
 	pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
 	return 0;
 }
@@ -6043,7 +6041,7 @@ int pcix_get_max_mmrbc(struct pci_dev *dev)
 	if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
 		return -EINVAL;
 
-	return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
+	return 512 << FIELD_GET(PCI_X_STATUS_MAX_READ, stat);
 }
 EXPORT_SYMBOL(pcix_get_max_mmrbc);
 
@@ -6066,7 +6064,7 @@ int pcix_get_mmrbc(struct pci_dev *dev)
 	if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
 		return -EINVAL;
 
-	return 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
+	return 512 << FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
 }
 EXPORT_SYMBOL(pcix_get_mmrbc);
 
@@ -6097,19 +6095,19 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
 	if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
 		return -EINVAL;
 
-	if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
+	if (v > FIELD_GET(PCI_X_STATUS_MAX_READ, stat))
 		return -E2BIG;
 
 	if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
 		return -EINVAL;
 
-	o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
+	o = FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
 	if (o != v) {
 		if (v > o && (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
 			return -EIO;
 
 		cmd &= ~PCI_X_CMD_MAX_READ;
-		cmd |= v << 2;
+		cmd |= FIELD_PREP(PCI_X_CMD_MAX_READ, v);
 		if (pci_write_config_word(dev, cap + PCI_X_CMD, cmd))
 			return -EIO;
 	}
@@ -6129,7 +6127,7 @@ int pcie_get_readrq(struct pci_dev *dev)
 
 	pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
 
-	return 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
+	return 128 << FIELD_GET(PCI_EXP_DEVCTL_READRQ, ctl);
 }
 EXPORT_SYMBOL(pcie_get_readrq);
 
@@ -6162,7 +6160,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
 			rq = mps;
 	}
 
-	v = (ffs(rq) - 8) << 12;
+	v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
 
 	if (bridge->no_inc_mrrs) {
 		int max_mrrs = pcie_get_readrq(dev);
@@ -6192,7 +6190,7 @@ int pcie_get_mps(struct pci_dev *dev)
 
 	pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
 
-	return 128 << ((ctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5);
+	return 128 << FIELD_GET(PCI_EXP_DEVCTL_PAYLOAD, ctl);
 }
 EXPORT_SYMBOL(pcie_get_mps);
 
@@ -6215,7 +6213,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
 	v = ffs(mps) - 8;
 	if (v > dev->pcie_mpss)
 		return -EINVAL;
-	v <<= 5;
+	v = FIELD_PREP(PCI_EXP_DEVCTL_PAYLOAD, v);
 
 	ret = pcie_capability_clear_and_set_word(dev, PCI_EXP_DEVCTL,
 						  PCI_EXP_DEVCTL_PAYLOAD, v);
@@ -6257,7 +6255,8 @@ u32 pcie_bandwidth_available(struct pci_dev *dev, struct pci_dev **limiting_dev,
 	while (dev) {
 		pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &lnksta);
 
-		next_speed = pcie_link_speed[lnksta & PCI_EXP_LNKSTA_CLS];
+		next_speed = pcie_link_speed[FIELD_GET(PCI_EXP_LNKSTA_CLS,
+						       lnksta)];
 		next_width = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
 
 		next_bw = next_width * PCIE_SPEED2MBS_ENC(next_speed);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 795534589b98..2036c3a120ee 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -807,8 +807,8 @@ static void pci_set_bus_speed(struct pci_bus *bus)
 		}
 
 		bus->max_bus_speed = max;
-		bus->cur_bus_speed = pcix_bus_speed[
-			(status & PCI_X_SSTATUS_FREQ) >> 6];
+		bus->cur_bus_speed =
+			pcix_bus_speed[FIELD_GET(PCI_X_SSTATUS_FREQ, status)];
 
 		return;
 	}
@@ -1217,8 +1217,8 @@ static bool pci_ea_fixed_busnrs(struct pci_dev *dev, u8 *sec, u8 *sub)
 
 	offset = ea + PCI_EA_FIRST_ENT;
 	pci_read_config_dword(dev, offset, &dw);
-	ea_sec =  dw & PCI_EA_SEC_BUS_MASK;
-	ea_sub = (dw & PCI_EA_SUB_BUS_MASK) >> PCI_EA_SUB_BUS_SHIFT;
+	ea_sec = FIELD_GET(PCI_EA_SEC_BUS_MASK, dw);
+	ea_sub = FIELD_GET(PCI_EA_SUB_BUS_MASK, dw);
 	if (ea_sec  == 0 || ea_sub < ea_sec)
 		return false;
 
-- 
2.34.1

[PATCH 02/10] PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Separate because this isn't as trivial as the other FIELD_GET() changes.

See 907830b0fc9e ("PCI: Add a REBAR size quirk for Sapphire RX 5600 XT
Pulse")

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Nirmoy Das <nirmoy.das@amd.com>
---
 drivers/pci/pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 848c9ee65d7f..5dc6e7cdfb71 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3751,14 +3751,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
 		return 0;
 
 	pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
-	cap &= PCI_REBAR_CAP_SIZES;
+	cap = FIELD_GET(PCI_REBAR_CAP_SIZES, cap);
 
 	/* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
 	if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
-	    bar == 0 && cap == 0x7000)
-		cap = 0x3f000;
+	    bar == 0 && cap == 0x700)
+		return 0x3f00;
 
-	return cap >> 4;
+	return cap;
 }
 EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
 
-- 
2.34.1

[PATCH 03/10] PCI/ASPM: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Add #defines for T_POWER_ON in the L1 PM Substates Capability and use
FIELD_PREP() and FIELD_GET() when possible.  These remove the need for
explicit shifts.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c       | 31 ++++++++++++++++++-------------
 include/uapi/linux/pci_regs.h |  2 ++
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 1bf630059264..06f175d8dee5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -7,6 +7,7 @@
  * Copyright (C) Shaohua Li (shaohua.li@intel.com)
  */
 
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/math.h>
 #include <linux/module.h>
@@ -267,7 +268,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
 /* Convert L0s latency encoding to ns */
 static u32 calc_l0s_latency(u32 lnkcap)
 {
-	u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L0SEL) >> 12;
+	u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L0SEL, lnkcap);
 
 	if (encoding == 0x7)
 		return (5 * 1000);	/* > 4us */
@@ -285,7 +286,7 @@ static u32 calc_l0s_acceptable(u32 encoding)
 /* Convert L1 latency encoding to ns */
 static u32 calc_l1_latency(u32 lnkcap)
 {
-	u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L1EL) >> 15;
+	u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L1EL, lnkcap);
 
 	if (encoding == 0x7)
 		return (65 * 1000);	/* > 64us */
@@ -371,11 +372,11 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 	link = endpoint->bus->self->link_state;
 
 	/* Calculate endpoint L0s acceptable latency */
-	encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L0S) >> 6;
+	encoding = FIELD_GET(PCI_EXP_DEVCAP_L0S, endpoint->devcap);
 	acceptable_l0s = calc_l0s_acceptable(encoding);
 
 	/* Calculate endpoint L1 acceptable latency */
-	encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L1) >> 9;
+	encoding = FIELD_GET(PCI_EXP_DEVCAP_L1, endpoint->devcap);
 	acceptable_l1 = calc_l1_acceptable(encoding);
 
 	while (link) {
@@ -446,22 +447,24 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	u32 pl1_2_enables, cl1_2_enables;
 
 	/* Choose the greater of the two Port Common_Mode_Restore_Times */
-	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
-	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
+	val1 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, parent_l1ss_cap);
+	val2 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, child_l1ss_cap);
 	t_common_mode = max(val1, val2);
 
 	/* Choose the greater of the two Port T_POWER_ON times */
-	val1   = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
-	scale1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
-	val2   = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
-	scale2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
+	val1   = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, parent_l1ss_cap);
+	scale1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, parent_l1ss_cap);
+	val2   = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, child_l1ss_cap);
+	scale2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, child_l1ss_cap);
 
 	if (calc_l12_pwron(parent, scale1, val1) >
 	    calc_l12_pwron(child, scale2, val2)) {
-		ctl2 |= scale1 | (val1 << 3);
+		ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale1) |
+			FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val1);
 		t_power_on = calc_l12_pwron(parent, scale1, val1);
 	} else {
-		ctl2 |= scale2 | (val2 << 3);
+		ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale2) |
+			FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val2);
 		t_power_on = calc_l12_pwron(child, scale2, val2);
 	}
 
@@ -477,7 +480,9 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
 	 */
 	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
 	encode_l12_threshold(l1_2_threshold, &scale, &value);
-	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
+	ctl1 |= FIELD_PREP(PCI_L1SS_CTL1_CM_RESTORE_TIME, t_common_mode) |
+		FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_VALUE, value) |
+		FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_SCALE, scale);
 
 	/* Some broken devices only support dword access to L1 SS */
 	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e5f558d96493..34bf037993f3 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1088,6 +1088,8 @@
 #define  PCI_L1SS_CTL1_LTR_L12_TH_VALUE	0x03ff0000  /* LTR_L1.2_THRESHOLD_Value */
 #define  PCI_L1SS_CTL1_LTR_L12_TH_SCALE	0xe0000000  /* LTR_L1.2_THRESHOLD_Scale */
 #define PCI_L1SS_CTL2		0x0c	/* Control 2 Register */
+#define  PCI_L1SS_CTL2_T_PWR_ON_SCALE   0x00000003  /* T_POWER_ON Scale */
+#define  PCI_L1SS_CTL2_T_PWR_ON_VALUE   0x000000f8  /* T_POWER_ON Value */
 
 /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
 #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
-- 
2.34.1

[PATCH 04/10] PCI/ATS: Show PASID Capability register width in bitmasks

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The PASID Capability and Control registers are both 16 bits wide.  Use
16-bit wide constants in field names to match the register width.  No
functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/uapi/linux/pci_regs.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 34bf037993f3..6af1f8d53e97 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -930,12 +930,12 @@
 
 /* Process Address Space ID */
 #define PCI_PASID_CAP		0x04    /* PASID feature register */
-#define  PCI_PASID_CAP_EXEC	0x02	/* Exec permissions Supported */
-#define  PCI_PASID_CAP_PRIV	0x04	/* Privilege Mode Supported */
+#define  PCI_PASID_CAP_EXEC	0x0002	/* Exec permissions Supported */
+#define  PCI_PASID_CAP_PRIV	0x0004	/* Privilege Mode Supported */
 #define PCI_PASID_CTRL		0x06    /* PASID control register */
-#define  PCI_PASID_CTRL_ENABLE	0x01	/* Enable bit */
-#define  PCI_PASID_CTRL_EXEC	0x02	/* Exec permissions Enable */
-#define  PCI_PASID_CTRL_PRIV	0x04	/* Privilege Mode Enable */
+#define  PCI_PASID_CTRL_ENABLE	0x0001	/* Enable bit */
+#define  PCI_PASID_CTRL_EXEC	0x0002	/* Exec permissions Enable */
+#define  PCI_PASID_CTRL_PRIV	0x0004	/* Privilege Mode Enable */
 #define PCI_EXT_CAP_PASID_SIZEOF	8
 
 /* Single Root I/O Virtualization */
-- 
2.34.1

[PATCH 05/10] PCI/ATS: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/ats.c             | 7 ++-----
 include/uapi/linux/pci_regs.h | 1 +
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
index f9cc2e10b676..c570892b2090 100644
--- a/drivers/pci/ats.c
+++ b/drivers/pci/ats.c
@@ -9,6 +9,7 @@
  * Copyright (C) 2011 Advanced Micro Devices,
  */
 
+#include <linux/bitfield.h>
 #include <linux/export.h>
 #include <linux/pci-ats.h>
 #include <linux/pci.h>
@@ -480,8 +481,6 @@ int pci_pasid_features(struct pci_dev *pdev)
 }
 EXPORT_SYMBOL_GPL(pci_pasid_features);
 
-#define PASID_NUMBER_SHIFT	8
-#define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
 /**
  * pci_max_pasids - Get maximum number of PASIDs supported by device
  * @pdev: PCI device structure
@@ -503,9 +502,7 @@ int pci_max_pasids(struct pci_dev *pdev)
 
 	pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
 
-	supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
-
-	return (1 << supported);
+	return (1 << FIELD_GET(PCI_PASID_CAP_WIDTH, supported));
 }
 EXPORT_SYMBOL_GPL(pci_max_pasids);
 #endif /* CONFIG_PCI_PASID */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 6af1f8d53e97..833e5fb40ea5 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -932,6 +932,7 @@
 #define PCI_PASID_CAP		0x04    /* PASID feature register */
 #define  PCI_PASID_CAP_EXEC	0x0002	/* Exec permissions Supported */
 #define  PCI_PASID_CAP_PRIV	0x0004	/* Privilege Mode Supported */
+#define  PCI_PASID_CAP_WIDTH	0x1f00
 #define PCI_PASID_CTRL		0x06    /* PASID control register */
 #define  PCI_PASID_CTRL_ENABLE	0x0001	/* Enable bit */
 #define  PCI_PASID_CTRL_EXEC	0x0002	/* Exec permissions Enable */
-- 
2.34.1

[PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/dpc.c        | 9 +++++----
 drivers/pci/quirks.c          | 2 +-
 include/uapi/linux/pci_regs.h | 1 +
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 3ceed8e3de41..6e551f34ec63 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -8,6 +8,7 @@
 
 #define dev_fmt(fmt) "DPC: " fmt
 
+#include <linux/bitfield.h>
 #include <linux/aer.h>
 #include <linux/delay.h>
 #include <linux/interrupt.h>
@@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
 
 	/* Get First Error Pointer */
 	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
-	first_error = (dpc_status & 0x1f00) >> 8;
+	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
 
 	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
 		if ((status & ~mask) & (1 << i))
@@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
 	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
 		 status, source);
 
-	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
-	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
+	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
+	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
 	pci_warn(pdev, "%s detected\n",
 		 (reason == 0) ? "unmasked uncorrectable error" :
 		 (reason == 1) ? "ERR_NONFATAL" :
@@ -338,7 +339,7 @@ void pci_dpc_init(struct pci_dev *pdev)
 	/* Quirks may set dpc_rp_log_size if device or firmware is buggy */
 	if (!pdev->dpc_rp_log_size) {
 		pdev->dpc_rp_log_size =
-			(cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
+				FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
 		if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
 			pci_err(pdev, "RP PIO log size %u is invalid\n",
 				pdev->dpc_rp_log_size);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index eeec1d6f9023..a9fdc2e3f110 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
 	if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
 		return;
 
-	if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
+	if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
 		pci_info(dev, "Overriding RP PIO Log Size to 4\n");
 		dev->dpc_rp_log_size = 4;
 	}
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 833e5fb40ea5..e97a06b50f95 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1046,6 +1046,7 @@
 #define  PCI_EXP_DPC_STATUS_INTERRUPT	    0x0008 /* Interrupt Status */
 #define  PCI_EXP_DPC_RP_BUSY		    0x0010 /* Root Port Busy */
 #define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
+#define  PCI_EXP_DPC_STATUS_FIRST_ERR	    0x1f00 /* RP PIO First Error Ptr */
 
 #define PCI_EXP_DPC_SOURCE_ID		 0x0A	/* DPC Source Identifier */
 
-- 
2.34.1

[PATCH 07/10] PCI/PME: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/pme.c        | 4 +++-
 include/uapi/linux/pci_regs.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
index ef8ce436ead9..a2daebd9806c 100644
--- a/drivers/pci/pcie/pme.c
+++ b/drivers/pci/pcie/pme.c
@@ -9,6 +9,7 @@
 
 #define dev_fmt(fmt) "PME: " fmt
 
+#include <linux/bitfield.h>
 #include <linux/pci.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
@@ -235,7 +236,8 @@ static void pcie_pme_work_fn(struct work_struct *work)
 			pcie_clear_root_pme_status(port);
 
 			spin_unlock_irq(&data->lock);
-			pcie_pme_handle_request(port, rtsta & 0xffff);
+			pcie_pme_handle_request(port,
+				    FIELD_GET(PCI_EXP_RTSTA_PME_RQ_ID, rtsta));
 			spin_lock_irq(&data->lock);
 
 			continue;
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e97a06b50f95..9fb8a69241f4 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -637,6 +637,7 @@
 #define PCI_EXP_RTCAP		0x1e	/* Root Capabilities */
 #define  PCI_EXP_RTCAP_CRSVIS	0x0001	/* CRS Software Visibility capability */
 #define PCI_EXP_RTSTA		0x20	/* Root Status */
+#define  PCI_EXP_RTSTA_PME_RQ_ID 0x0000ffff /* PME Requester ID */
 #define  PCI_EXP_RTSTA_PME	0x00010000 /* PME status */
 #define  PCI_EXP_RTSTA_PENDING	0x00020000 /* PME pending */
 /*
-- 
2.34.1

[PATCH 08/10] PCI/PTM: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/ptm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
index b4e5f553467c..7cfb6c0d5dcb 100644
--- a/drivers/pci/pcie/ptm.c
+++ b/drivers/pci/pcie/ptm.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2016, Intel Corporation.
  */
 
+#include <linux/bitfield.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/pci.h>
@@ -53,7 +54,7 @@ void pci_ptm_init(struct pci_dev *dev)
 	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u32));
 
 	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
-	dev->ptm_granularity = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
+	dev->ptm_granularity = FIELD_GET(PCI_PTM_GRANULARITY_MASK, cap);
 
 	/*
 	 * Per the spec recommendation (PCIe r6.0, sec 7.9.15.3), select the
@@ -146,7 +147,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
 
 	ctrl |= PCI_PTM_CTRL_ENABLE;
 	ctrl &= ~PCI_PTM_GRANULARITY_MASK;
-	ctrl |= dev->ptm_granularity << 8;
+	ctrl |= FIELD_PREP(PCI_PTM_GRANULARITY_MASK, dev->ptm_granularity);
 	if (dev->ptm_root)
 		ctrl |= PCI_PTM_CTRL_ROOT;
 
-- 
2.34.1

[PATCH 09/10] PCI/VC: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/vc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
index 5fc59ac31145..a4ff7f5f66dd 100644
--- a/drivers/pci/vc.c
+++ b/drivers/pci/vc.c
@@ -6,6 +6,7 @@
  *     Author: Alex Williamson <alex.williamson@redhat.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -201,9 +202,9 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
 	/* Extended VC Count (not counting VC0) */
 	evcc = cap1 & PCI_VC_CAP1_EVCC;
 	/* Low Priority Extended VC Count (not counting VC0) */
-	lpevcc = (cap1 & PCI_VC_CAP1_LPEVCC) >> 4;
+	lpevcc = FIELD_GET(PCI_VC_CAP1_LPEVCC, cap1);
 	/* Port Arbitration Table Entry Size (bits) */
-	parb_size = 1 << ((cap1 & PCI_VC_CAP1_ARB_SIZE) >> 10);
+	parb_size = 1 << FIELD_GET(PCI_VC_CAP1_ARB_SIZE, cap1);
 
 	/*
 	 * Port VC Control Register contains VC Arbitration Select, which
@@ -231,7 +232,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
 		int vcarb_offset;
 
 		pci_read_config_dword(dev, pos + PCI_VC_PORT_CAP2, &cap2);
-		vcarb_offset = ((cap2 & PCI_VC_CAP2_ARB_OFF) >> 24) * 16;
+		vcarb_offset = FIELD_GET(PCI_VC_CAP2_ARB_OFF, cap2) * 16;
 
 		if (vcarb_offset) {
 			int size, vcarb_phases = 0;
@@ -277,7 +278,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
 
 		pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
 				      (i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
-		parb_offset = ((cap & PCI_VC_RES_CAP_ARB_OFF) >> 24) * 16;
+		parb_offset = FIELD_GET(PCI_VC_RES_CAP_ARB_OFF, cap) * 16;
 		if (parb_offset) {
 			int size, parb_phases = 0;
 
-- 
2.34.1

[PATCH 10/10] PCI/portdrv: Use FIELD_GET()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Use FIELD_GET() to remove dependences on the field position, i.e., the
shift value.  No functional change intended.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/portdrv.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 46fad0d813b2..14a4b89a3b83 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -6,6 +6,7 @@
  * Copyright (C) Tom Long Nguyen (tom.l.nguyen@intel.com)
  */
 
+#include <linux/bitfield.h>
 #include <linux/dmi.h>
 #include <linux/init.h>
 #include <linux/module.h>
@@ -69,7 +70,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
 	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP |
 		    PCIE_PORT_SERVICE_BWNOTIF)) {
 		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
-		*pme = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
+		*pme = FIELD_GET(PCI_EXP_FLAGS_IRQ, reg16);
 		nvec = *pme + 1;
 	}
 
@@ -81,7 +82,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
 		if (pos) {
 			pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS,
 					      &reg32);
-			*aer = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
+			*aer = FIELD_GET(PCI_ERR_ROOT_AER_IRQ, reg32);
 			nvec = max(nvec, *aer + 1);
 		}
 	}
@@ -92,7 +93,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
 		if (pos) {
 			pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP,
 					     &reg16);
-			*dpc = reg16 & PCI_EXP_DPC_IRQ;
+			*dpc = FIELD_GET(PCI_EXP_DPC_IRQ, reg16);
 			nvec = max(nvec, *dpc + 1);
 		}
 	}
-- 
2.34.1

Re: [PATCH 03/10] PCI/ASPM: Use FIELD_GET()

[Bjorn Helgaas]

On Tue, Oct 10, 2023 at 03:44:29PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Add #defines for T_POWER_ON in the L1 PM Substates Capability and use
> FIELD_PREP() and FIELD_GET() when possible.  These remove the need for
> explicit shifts.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

I see this is identical to your patch at
https://lore.kernel.org/r/20230915155752.84640-3-ilpo.jarvinen@linux.intel.com,
Ilpo, so I'll drop this one.

> ---
>  drivers/pci/pcie/aspm.c       | 31 ++++++++++++++++++-------------
>  include/uapi/linux/pci_regs.h |  2 ++
>  2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 1bf630059264..06f175d8dee5 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -7,6 +7,7 @@
>   * Copyright (C) Shaohua Li (shaohua.li@intel.com)
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/kernel.h>
>  #include <linux/math.h>
>  #include <linux/module.h>
> @@ -267,7 +268,7 @@ static void pcie_aspm_configure_common_clock(struct pcie_link_state *link)
>  /* Convert L0s latency encoding to ns */
>  static u32 calc_l0s_latency(u32 lnkcap)
>  {
> -	u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L0SEL) >> 12;
> +	u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L0SEL, lnkcap);
>  
>  	if (encoding == 0x7)
>  		return (5 * 1000);	/* > 4us */
> @@ -285,7 +286,7 @@ static u32 calc_l0s_acceptable(u32 encoding)
>  /* Convert L1 latency encoding to ns */
>  static u32 calc_l1_latency(u32 lnkcap)
>  {
> -	u32 encoding = (lnkcap & PCI_EXP_LNKCAP_L1EL) >> 15;
> +	u32 encoding = FIELD_GET(PCI_EXP_LNKCAP_L1EL, lnkcap);
>  
>  	if (encoding == 0x7)
>  		return (65 * 1000);	/* > 64us */
> @@ -371,11 +372,11 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>  	link = endpoint->bus->self->link_state;
>  
>  	/* Calculate endpoint L0s acceptable latency */
> -	encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L0S) >> 6;
> +	encoding = FIELD_GET(PCI_EXP_DEVCAP_L0S, endpoint->devcap);
>  	acceptable_l0s = calc_l0s_acceptable(encoding);
>  
>  	/* Calculate endpoint L1 acceptable latency */
> -	encoding = (endpoint->devcap & PCI_EXP_DEVCAP_L1) >> 9;
> +	encoding = FIELD_GET(PCI_EXP_DEVCAP_L1, endpoint->devcap);
>  	acceptable_l1 = calc_l1_acceptable(encoding);
>  
>  	while (link) {
> @@ -446,22 +447,24 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
>  	u32 pl1_2_enables, cl1_2_enables;
>  
>  	/* Choose the greater of the two Port Common_Mode_Restore_Times */
> -	val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> -	val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8;
> +	val1 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, parent_l1ss_cap);
> +	val2 = FIELD_GET(PCI_L1SS_CAP_CM_RESTORE_TIME, child_l1ss_cap);
>  	t_common_mode = max(val1, val2);
>  
>  	/* Choose the greater of the two Port T_POWER_ON times */
> -	val1   = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> -	scale1 = (parent_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> -	val2   = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_VALUE) >> 19;
> -	scale2 = (child_l1ss_cap & PCI_L1SS_CAP_P_PWR_ON_SCALE) >> 16;
> +	val1   = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, parent_l1ss_cap);
> +	scale1 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, parent_l1ss_cap);
> +	val2   = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_VALUE, child_l1ss_cap);
> +	scale2 = FIELD_GET(PCI_L1SS_CAP_P_PWR_ON_SCALE, child_l1ss_cap);
>  
>  	if (calc_l12_pwron(parent, scale1, val1) >
>  	    calc_l12_pwron(child, scale2, val2)) {
> -		ctl2 |= scale1 | (val1 << 3);
> +		ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale1) |
> +			FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val1);
>  		t_power_on = calc_l12_pwron(parent, scale1, val1);
>  	} else {
> -		ctl2 |= scale2 | (val2 << 3);
> +		ctl2 |= FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_SCALE, scale2) |
> +			FIELD_PREP(PCI_L1SS_CTL2_T_PWR_ON_VALUE, val2);
>  		t_power_on = calc_l12_pwron(child, scale2, val2);
>  	}
>  
> @@ -477,7 +480,9 @@ static void aspm_calc_l12_info(struct pcie_link_state *link,
>  	 */
>  	l1_2_threshold = 2 + 4 + t_common_mode + t_power_on;
>  	encode_l12_threshold(l1_2_threshold, &scale, &value);
> -	ctl1 |= t_common_mode << 8 | scale << 29 | value << 16;
> +	ctl1 |= FIELD_PREP(PCI_L1SS_CTL1_CM_RESTORE_TIME, t_common_mode) |
> +		FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_VALUE, value) |
> +		FIELD_PREP(PCI_L1SS_CTL1_LTR_L12_TH_SCALE, scale);
>  
>  	/* Some broken devices only support dword access to L1 SS */
>  	pci_read_config_dword(parent, parent->l1ss + PCI_L1SS_CTL1, &pctl1);
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e5f558d96493..34bf037993f3 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1088,6 +1088,8 @@
>  #define  PCI_L1SS_CTL1_LTR_L12_TH_VALUE	0x03ff0000  /* LTR_L1.2_THRESHOLD_Value */
>  #define  PCI_L1SS_CTL1_LTR_L12_TH_SCALE	0xe0000000  /* LTR_L1.2_THRESHOLD_Scale */
>  #define PCI_L1SS_CTL2		0x0c	/* Control 2 Register */
> +#define  PCI_L1SS_CTL2_T_PWR_ON_SCALE   0x00000003  /* T_POWER_ON Scale */
> +#define  PCI_L1SS_CTL2_T_PWR_ON_VALUE   0x000000f8  /* T_POWER_ON Value */
>  
>  /* Designated Vendor-Specific (DVSEC, PCI_EXT_CAP_ID_DVSEC) */
>  #define PCI_DVSEC_HEADER1		0x4 /* Designated Vendor-Specific Header1 */
> -- 
> 2.34.1
> 

Re: [PATCH 01/10] PCI: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:27 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

It's a little unfortunate that some of the masks are called *_MASK and
others are not (e.g. PCI_RBAR_CTRL_BAR_SIZE which is a mask).
but given they are in include/uapi not sure we can tidy that up unless
we add more defines that stick to consistent naming...

Otherwise very nice.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 02/10] PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:28 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Separate because this isn't as trivial as the other FIELD_GET() changes.
> 
> See 907830b0fc9e ("PCI: Add a REBAR size quirk for Sapphire RX 5600 XT
> Pulse")
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Nirmoy Das <nirmoy.das@amd.com>

Change would be a tiny bit more obvious without the early return, but I can see
why you think that is an improvement over relying on the return just below.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 848c9ee65d7f..5dc6e7cdfb71 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3751,14 +3751,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
>  		return 0;
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> -	cap &= PCI_REBAR_CAP_SIZES;
> +	cap = FIELD_GET(PCI_REBAR_CAP_SIZES, cap);
>  
>  	/* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
>  	if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
> -	    bar == 0 && cap == 0x7000)
> -		cap = 0x3f000;
> +	    bar == 0 && cap == 0x700)
> +		return 0x3f00;
>  
> -	return cap >> 4;
> +	return cap;
>  }
>  EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
>  

Re: [PATCH 04/10] PCI/ATS: Show PASID Capability register width in bitmasks

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:30 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The PASID Capability and Control registers are both 16 bits wide.  Use
> 16-bit wide constants in field names to match the register width.  No
> functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/uapi/linux/pci_regs.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 34bf037993f3..6af1f8d53e97 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -930,12 +930,12 @@
>  
>  /* Process Address Space ID */
>  #define PCI_PASID_CAP		0x04    /* PASID feature register */
> -#define  PCI_PASID_CAP_EXEC	0x02	/* Exec permissions Supported */
> -#define  PCI_PASID_CAP_PRIV	0x04	/* Privilege Mode Supported */
> +#define  PCI_PASID_CAP_EXEC	0x0002	/* Exec permissions Supported */
> +#define  PCI_PASID_CAP_PRIV	0x0004	/* Privilege Mode Supported */
>  #define PCI_PASID_CTRL		0x06    /* PASID control register */
> -#define  PCI_PASID_CTRL_ENABLE	0x01	/* Enable bit */
> -#define  PCI_PASID_CTRL_EXEC	0x02	/* Exec permissions Enable */
> -#define  PCI_PASID_CTRL_PRIV	0x04	/* Privilege Mode Enable */
> +#define  PCI_PASID_CTRL_ENABLE	0x0001	/* Enable bit */
> +#define  PCI_PASID_CTRL_EXEC	0x0002	/* Exec permissions Enable */
> +#define  PCI_PASID_CTRL_PRIV	0x0004	/* Privilege Mode Enable */
>  #define PCI_EXT_CAP_PASID_SIZEOF	8
>  
>  /* Single Root I/O Virtualization */

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/dpc.c        | 9 +++++----
>  drivers/pci/quirks.c          | 2 +-
>  include/uapi/linux/pci_regs.h | 1 +
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 3ceed8e3de41..6e551f34ec63 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -8,6 +8,7 @@
>  
>  #define dev_fmt(fmt) "DPC: " fmt
>  
> +#include <linux/bitfield.h>
>  #include <linux/aer.h>
>  #include <linux/delay.h>
>  #include <linux/interrupt.h>
> @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
>  
>  	/* Get First Error Pointer */
>  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> -	first_error = (dpc_status & 0x1f00) >> 8;
> +	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
>  
>  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
>  		if ((status & ~mask) & (1 << i))
> @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
>  	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
>  		 status, source);
>  
> -	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> -	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> +	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> +	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
>  	pci_warn(pdev, "%s detected\n",
>  		 (reason == 0) ? "unmasked uncorrectable error" :
>  		 (reason == 1) ? "ERR_NONFATAL" :

BTW, it seems we're doing overlapping work here with many of these 
patches. It takes some time to think these through one by one, I don't 
just autorun through them with coccinelle so I've not posted my changes
yet.

I went to a different direction here and named all the reasons too with 
defines and used & to get the reason in order to be able to compare with 
the named reasons.

You also missed convering one 0xfff4 to use define (although I suspect it 
never was your goal to go beyond FIELD_GET() here).
 
> @@ -338,7 +339,7 @@ void pci_dpc_init(struct pci_dev *pdev)
>  	/* Quirks may set dpc_rp_log_size if device or firmware is buggy */
>  	if (!pdev->dpc_rp_log_size) {
>  		pdev->dpc_rp_log_size =
> -			(cap & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8;
> +				FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
>  		if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
>  			pci_err(pdev, "RP PIO log size %u is invalid\n",
>  				pdev->dpc_rp_log_size);
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..a9fdc2e3f110 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
>  	if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
>  		return;
>  
> -	if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> +	if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
>  		pci_info(dev, "Overriding RP PIO Log Size to 4\n");
>  		dev->dpc_rp_log_size = 4;
>  	}
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 833e5fb40ea5..e97a06b50f95 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1046,6 +1046,7 @@
>  #define  PCI_EXP_DPC_STATUS_INTERRUPT	    0x0008 /* Interrupt Status */
>  #define  PCI_EXP_DPC_RP_BUSY		    0x0010 /* Root Port Busy */
>  #define  PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT 0x0060 /* Trig Reason Extension */
> +#define  PCI_EXP_DPC_STATUS_FIRST_ERR	    0x1f00 /* RP PIO First Error Ptr */

If you prefer consistency, there already FEP used for "First 
Error Pointer" used in another define.

>  #define PCI_EXP_DPC_SOURCE_ID		 0x0A	/* DPC Source Identifier */



-- 
 i.

Re: [PATCH 05/10] PCI/ATS: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:31 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
One trivial comment inline. Either way.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/ats.c             | 7 ++-----
>  include/uapi/linux/pci_regs.h | 1 +
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..c570892b2090 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -9,6 +9,7 @@
>   * Copyright (C) 2011 Advanced Micro Devices,
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/export.h>
>  #include <linux/pci-ats.h>
>  #include <linux/pci.h>
> @@ -480,8 +481,6 @@ int pci_pasid_features(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL_GPL(pci_pasid_features);
>  
> -#define PASID_NUMBER_SHIFT	8
> -#define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
>  /**
>   * pci_max_pasids - Get maximum number of PASIDs supported by device
>   * @pdev: PCI device structure
> @@ -503,9 +502,7 @@ int pci_max_pasids(struct pci_dev *pdev)
>  
>  	pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
>  
> -	supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
> -
> -	return (1 << supported);
> +	return (1 << FIELD_GET(PCI_PASID_CAP_WIDTH, supported));

Could drop the bonus set of brackets..

>  }
>  EXPORT_SYMBOL_GPL(pci_max_pasids);
>  #endif /* CONFIG_PCI_PASID */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 6af1f8d53e97..833e5fb40ea5 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -932,6 +932,7 @@
>  #define PCI_PASID_CAP		0x04    /* PASID feature register */
>  #define  PCI_PASID_CAP_EXEC	0x0002	/* Exec permissions Supported */
>  #define  PCI_PASID_CAP_PRIV	0x0004	/* Privilege Mode Supported */
> +#define  PCI_PASID_CAP_WIDTH	0x1f00
>  #define PCI_PASID_CTRL		0x06    /* PASID control register */
>  #define  PCI_PASID_CTRL_ENABLE	0x0001	/* Enable bit */
>  #define  PCI_PASID_CTRL_EXEC	0x0002	/* Exec permissions Enable */

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:32 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
A question about what 'rules' you are applying for using these macros
vs choosing not not do so. Personally I prefer using them even for
flag fields mostly because it makes the code more consistent and
the compiler should remove any unnecessary shifts that result.

> ---

> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index eeec1d6f9023..a9fdc2e3f110 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
>  	if (!(val & PCI_EXP_DPC_CAP_RP_EXT))

This is what I'm commenting on below.

>  		return;
>  
> -	if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> +	if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {

Why do this one and not the one just above?
In both cases extracting a field then comparing it to 0, I'm not sure
it makes sense to care if that field is 1 bit or multiple bit.

>  		pci_info(dev, "Overriding RP PIO Log Size to 4\n");
>  		dev->dpc_rp_log_size = 4;
>  	}

Re: [PATCH 07/10] PCI/PME: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:33 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/pci/pcie/pme.c        | 4 +++-
>  include/uapi/linux/pci_regs.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index ef8ce436ead9..a2daebd9806c 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -9,6 +9,7 @@
>  
>  #define dev_fmt(fmt) "PME: " fmt
>  
> +#include <linux/bitfield.h>
>  #include <linux/pci.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> @@ -235,7 +236,8 @@ static void pcie_pme_work_fn(struct work_struct *work)
>  			pcie_clear_root_pme_status(port);
>  
>  			spin_unlock_irq(&data->lock);
> -			pcie_pme_handle_request(port, rtsta & 0xffff);
> +			pcie_pme_handle_request(port,
> +				    FIELD_GET(PCI_EXP_RTSTA_PME_RQ_ID, rtsta));
>  			spin_lock_irq(&data->lock);
>  
>  			continue;
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e97a06b50f95..9fb8a69241f4 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -637,6 +637,7 @@
>  #define PCI_EXP_RTCAP		0x1e	/* Root Capabilities */
>  #define  PCI_EXP_RTCAP_CRSVIS	0x0001	/* CRS Software Visibility capability */
>  #define PCI_EXP_RTSTA		0x20	/* Root Status */
> +#define  PCI_EXP_RTSTA_PME_RQ_ID 0x0000ffff /* PME Requester ID */
>  #define  PCI_EXP_RTSTA_PME	0x00010000 /* PME status */
>  #define  PCI_EXP_RTSTA_PENDING	0x00020000 /* PME pending */
>  /*

Re: [PATCH 08/10] PCI/PTM: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:34 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

>  

Re: [PATCH 09/10] PCI/VC: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:35 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

On Wed, 11 Oct 2023, Jonathan Cameron wrote:

> On Tue, 10 Oct 2023 15:44:32 -0500
> Bjorn Helgaas <helgaas@kernel.org> wrote:
> 
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value.  No functional change intended.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> A question about what 'rules' you are applying for using these macros
> vs choosing not not do so. Personally I prefer using them even for
> flag fields mostly because it makes the code more consistent and
> the compiler should remove any unnecessary shifts that result.
> 
> > ---
> 
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index eeec1d6f9023..a9fdc2e3f110 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -6154,7 +6154,7 @@ static void dpc_log_size(struct pci_dev *dev)
> >  	if (!(val & PCI_EXP_DPC_CAP_RP_EXT))
> 
> This is what I'm commenting on below.
> 
> >  		return;
> >  
> > -	if (!((val & PCI_EXP_DPC_RP_PIO_LOG_SIZE) >> 8)) {
> > +	if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
> 
> Why do this one and not the one just above?
> In both cases extracting a field then comparing it to 0, I'm not sure
> it makes sense to care if that field is 1 bit or multiple bit.

I cannot speak for Bjorn but at least I've left flag checks untouched
(but when pulling the flag into a variable, I've made it with FIELD_GET()).

In anycase, that seems minor issue though compared with defined values of 
the field being incompatible with the FIELD_GET()ed value (when the shift 
is non-zero). I wish there would be good solution to that but so far I've 
not come up anything that would be short and simple enough.

-- 
 i.

Re: [PATCH 10/10] PCI/portdrv: Use FIELD_GET()

[Jonathan Cameron]

On Tue, 10 Oct 2023 15:44:36 -0500
Bjorn Helgaas <helgaas@kernel.org> wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Re: [PATCH 08/10] PCI/PTM: Use FIELD_GET()

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

You seem to be using FIELD_PREP() too below.


> ---
>  drivers/pci/pcie/ptm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/ptm.c b/drivers/pci/pcie/ptm.c
> index b4e5f553467c..7cfb6c0d5dcb 100644
> --- a/drivers/pci/pcie/ptm.c
> +++ b/drivers/pci/pcie/ptm.c
> @@ -4,6 +4,7 @@
>   * Copyright (c) 2016, Intel Corporation.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/module.h>
>  #include <linux/init.h>
>  #include <linux/pci.h>
> @@ -53,7 +54,7 @@ void pci_ptm_init(struct pci_dev *dev)
>  	pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_PTM, sizeof(u32));
>  
>  	pci_read_config_dword(dev, ptm + PCI_PTM_CAP, &cap);
> -	dev->ptm_granularity = (cap & PCI_PTM_GRANULARITY_MASK) >> 8;
> +	dev->ptm_granularity = FIELD_GET(PCI_PTM_GRANULARITY_MASK, cap);
>  
>  	/*
>  	 * Per the spec recommendation (PCIe r6.0, sec 7.9.15.3), select the
> @@ -146,7 +147,7 @@ static int __pci_enable_ptm(struct pci_dev *dev)
>  
>  	ctrl |= PCI_PTM_CTRL_ENABLE;
>  	ctrl &= ~PCI_PTM_GRANULARITY_MASK;
> -	ctrl |= dev->ptm_granularity << 8;
> +	ctrl |= FIELD_PREP(PCI_PTM_GRANULARITY_MASK, dev->ptm_granularity);
>  	if (dev->ptm_root)
>  		ctrl |= PCI_PTM_CTRL_ROOT;
>  
> 


-- 
 i.

Re: [PATCH 05/10] PCI/ATS: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/ats.c             | 7 ++-----
>  include/uapi/linux/pci_regs.h | 1 +
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index f9cc2e10b676..c570892b2090 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -9,6 +9,7 @@
>   * Copyright (C) 2011 Advanced Micro Devices,
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/export.h>
>  #include <linux/pci-ats.h>
>  #include <linux/pci.h>
> @@ -480,8 +481,6 @@ int pci_pasid_features(struct pci_dev *pdev)
>  }
>  EXPORT_SYMBOL_GPL(pci_pasid_features);
>  
> -#define PASID_NUMBER_SHIFT	8
> -#define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
>  /**
>   * pci_max_pasids - Get maximum number of PASIDs supported by device
>   * @pdev: PCI device structure
> @@ -503,9 +502,7 @@ int pci_max_pasids(struct pci_dev *pdev)
>  
>  	pci_read_config_word(pdev, pasid + PCI_PASID_CAP, &supported);
>  
> -	supported = (supported & PASID_NUMBER_MASK) >> PASID_NUMBER_SHIFT;
> -
> -	return (1 << supported);
> +	return (1 << FIELD_GET(PCI_PASID_CAP_WIDTH, supported));
>  }
>  EXPORT_SYMBOL_GPL(pci_max_pasids);
>  #endif /* CONFIG_PCI_PASID */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 6af1f8d53e97..833e5fb40ea5 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -932,6 +932,7 @@
>  #define PCI_PASID_CAP		0x04    /* PASID feature register */
>  #define  PCI_PASID_CAP_EXEC	0x0002	/* Exec permissions Supported */
>  #define  PCI_PASID_CAP_PRIV	0x0004	/* Privilege Mode Supported */
> +#define  PCI_PASID_CAP_WIDTH	0x1f00
>  #define PCI_PASID_CTRL		0x06    /* PASID control register */
>  #define  PCI_PASID_CTRL_ENABLE	0x0001	/* Enable bit */
>  #define  PCI_PASID_CTRL_EXEC	0x0002	/* Exec permissions Enable */

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 01/10] PCI: Use FIELD_GET()

[Ilpo Järvinen]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.

FIELD_PREP() is also used below.

> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pci.c   | 45 ++++++++++++++++++++++-----------------------
>  drivers/pci/probe.c |  8 ++++----
>  2 files changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a8adc34dc86f..848c9ee65d7f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1776,8 +1776,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
>  		return;
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> -	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
> -		    PCI_REBAR_CTRL_NBAR_SHIFT;
> +	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
>  
>  	for (i = 0; i < nbars; i++, pos += 8) {
>  		struct resource *res;
> @@ -1788,7 +1787,7 @@ static void pci_restore_rebar_state(struct pci_dev *pdev)
>  		res = pdev->resource + bar_idx;
>  		size = pci_rebar_bytes_to_size(resource_size(res));
>  		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> -		ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> +		ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
>  		pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
>  	}
>  }
> @@ -3229,7 +3228,7 @@ void pci_pm_init(struct pci_dev *dev)
>  			 (pmc & PCI_PM_CAP_PME_D2) ? " D2" : "",
>  			 (pmc & PCI_PM_CAP_PME_D3hot) ? " D3hot" : "",
>  			 (pmc & PCI_PM_CAP_PME_D3cold) ? " D3cold" : "");
> -		dev->pme_support = pmc >> PCI_PM_CAP_PME_SHIFT;
> +		dev->pme_support = FIELD_GET(PCI_PM_CAP_PME_MASK, pmc);
>  		dev->pme_poll = true;
>  		/*
>  		 * Make device's PM flags reflect the wake-up capability, but
> @@ -3300,20 +3299,20 @@ static int pci_ea_read(struct pci_dev *dev, int offset)
>  	ent_offset += 4;
>  
>  	/* Entry size field indicates DWORDs after 1st */
> -	ent_size = ((dw0 & PCI_EA_ES) + 1) << 2;
> +	ent_size = (FIELD_GET(PCI_EA_ES, dw0) + 1) << 2;
>  
>  	if (!(dw0 & PCI_EA_ENABLE)) /* Entry not enabled */
>  		goto out;
>  
> -	bei = (dw0 & PCI_EA_BEI) >> 4;
> -	prop = (dw0 & PCI_EA_PP) >> 8;
> +	bei = FIELD_GET(PCI_EA_BEI, dw0);
> +	prop = FIELD_GET(PCI_EA_PP, dw0);
>  
>  	/*
>  	 * If the Property is in the reserved range, try the Secondary
>  	 * Property instead.
>  	 */
>  	if (prop > PCI_EA_P_BRIDGE_IO && prop < PCI_EA_P_MEM_RESERVED)
> -		prop = (dw0 & PCI_EA_SP) >> 16;
> +		prop = FIELD_GET(PCI_EA_SP, dw0);
>  	if (prop > PCI_EA_P_BRIDGE_IO)
>  		goto out;
>  
> @@ -3720,14 +3719,13 @@ static int pci_rebar_find_pos(struct pci_dev *pdev, int bar)
>  		return -ENOTSUPP;
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> -	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >>
> -		    PCI_REBAR_CTRL_NBAR_SHIFT;
> +	nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl);
>  
>  	for (i = 0; i < nbars; i++, pos += 8) {
>  		int bar_idx;
>  
>  		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> -		bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> +		bar_idx = FIELD_GET(PCI_REBAR_CTRL_BAR_IDX, ctrl);
>  		if (bar_idx == bar)
>  			return pos;
>  	}
> @@ -3782,7 +3780,7 @@ int pci_rebar_get_current_size(struct pci_dev *pdev, int bar)
>  		return pos;
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> -	return (ctrl & PCI_REBAR_CTRL_BAR_SIZE) >> PCI_REBAR_CTRL_BAR_SHIFT;
> +	return FIELD_GET(PCI_REBAR_CTRL_BAR_SIZE, ctrl);
>  }
>  
>  /**
> @@ -3805,7 +3803,7 @@ int pci_rebar_set_size(struct pci_dev *pdev, int bar, int size)
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
>  	ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE;
> -	ctrl |= size << PCI_REBAR_CTRL_BAR_SHIFT;
> +	ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size);
>  	pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
>  	return 0;
>  }
> @@ -6043,7 +6041,7 @@ int pcix_get_max_mmrbc(struct pci_dev *dev)
>  	if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
>  		return -EINVAL;
>  
> -	return 512 << ((stat & PCI_X_STATUS_MAX_READ) >> 21);
> +	return 512 << FIELD_GET(PCI_X_STATUS_MAX_READ, stat);
>  }
>  EXPORT_SYMBOL(pcix_get_max_mmrbc);
>  
> @@ -6066,7 +6064,7 @@ int pcix_get_mmrbc(struct pci_dev *dev)
>  	if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
>  		return -EINVAL;
>  
> -	return 512 << ((cmd & PCI_X_CMD_MAX_READ) >> 2);
> +	return 512 << FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
>  }
>  EXPORT_SYMBOL(pcix_get_mmrbc);
>  
> @@ -6097,19 +6095,19 @@ int pcix_set_mmrbc(struct pci_dev *dev, int mmrbc)
>  	if (pci_read_config_dword(dev, cap + PCI_X_STATUS, &stat))
>  		return -EINVAL;
>  
> -	if (v > (stat & PCI_X_STATUS_MAX_READ) >> 21)
> +	if (v > FIELD_GET(PCI_X_STATUS_MAX_READ, stat))
>  		return -E2BIG;
>  
>  	if (pci_read_config_word(dev, cap + PCI_X_CMD, &cmd))
>  		return -EINVAL;
>  
> -	o = (cmd & PCI_X_CMD_MAX_READ) >> 2;
> +	o = FIELD_GET(PCI_X_CMD_MAX_READ, cmd);
>  	if (o != v) {
>  		if (v > o && (dev->bus->bus_flags & PCI_BUS_FLAGS_NO_MMRBC))
>  			return -EIO;
>  
>  		cmd &= ~PCI_X_CMD_MAX_READ;
> -		cmd |= v << 2;
> +		cmd |= FIELD_PREP(PCI_X_CMD_MAX_READ, v);
>  		if (pci_write_config_word(dev, cap + PCI_X_CMD, cmd))
>  			return -EIO;
>  	}
> @@ -6129,7 +6127,7 @@ int pcie_get_readrq(struct pci_dev *dev)
>  
>  	pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>  
> -	return 128 << ((ctl & PCI_EXP_DEVCTL_READRQ) >> 12);
> +	return 128 << FIELD_GET(PCI_EXP_DEVCTL_READRQ, ctl);
>  }
>  EXPORT_SYMBOL(pcie_get_readrq);
>  
> @@ -6162,7 +6160,7 @@ int pcie_set_readrq(struct pci_dev *dev, int rq)
>  			rq = mps;
>  	}
>  
> -	v = (ffs(rq) - 8) << 12;
> +	v = FIELD_PREP(PCI_EXP_DEVCTL_READRQ, ffs(rq) - 8);
>  
>  	if (bridge->no_inc_mrrs) {
>  		int max_mrrs = pcie_get_readrq(dev);
> @@ -6192,7 +6190,7 @@ int pcie_get_mps(struct pci_dev *dev)
>  
>  	pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &ctl);
>  
> -	return 128 << ((ctl & PCI_EXP_DEVCTL_PAYLOAD) >> 5);
> +	return 128 << FIELD_GET(PCI_EXP_DEVCTL_PAYLOAD, ctl);
>  }
>  EXPORT_SYMBOL(pcie_get_mps);
>  
> @@ -6215,7 +6213,7 @@ int pcie_set_mps(struct pci_dev *dev, int mps)
>  	v = ffs(mps) - 8;
>  	if (v > dev->pcie_mpss)
>  		return -EINVAL;
> -	v <<= 5;
> +	v = FIELD_PREP(PCI_EXP_DEVCTL_PAYLOAD, v);


-- 
 i.

Re: [PATCH 02/10] PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Separate because this isn't as trivial as the other FIELD_GET() changes.
> 
> See 907830b0fc9e ("PCI: Add a REBAR size quirk for Sapphire RX 5600 XT
> Pulse")
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Nirmoy Das <nirmoy.das@amd.com>
> ---
>  drivers/pci/pci.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 848c9ee65d7f..5dc6e7cdfb71 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3751,14 +3751,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
>  		return 0;
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> -	cap &= PCI_REBAR_CAP_SIZES;
> +	cap = FIELD_GET(PCI_REBAR_CAP_SIZES, cap);
>  
>  	/* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
>  	if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
> -	    bar == 0 && cap == 0x7000)
> -		cap = 0x3f000;
> +	    bar == 0 && cap == 0x700)
> +		return 0x3f00;
>  
> -	return cap >> 4;
> +	return cap;
>  }
>  EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
>  
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 04/10] PCI/ATS: Show PASID Capability register width in bitmasks

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> The PASID Capability and Control registers are both 16 bits wide.  Use
> 16-bit wide constants in field names to match the register width.  No
> functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  include/uapi/linux/pci_regs.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 34bf037993f3..6af1f8d53e97 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -930,12 +930,12 @@
>  
>  /* Process Address Space ID */
>  #define PCI_PASID_CAP		0x04    /* PASID feature register */
> -#define  PCI_PASID_CAP_EXEC	0x02	/* Exec permissions Supported */
> -#define  PCI_PASID_CAP_PRIV	0x04	/* Privilege Mode Supported */
> +#define  PCI_PASID_CAP_EXEC	0x0002	/* Exec permissions Supported */
> +#define  PCI_PASID_CAP_PRIV	0x0004	/* Privilege Mode Supported */
>  #define PCI_PASID_CTRL		0x06    /* PASID control register */
> -#define  PCI_PASID_CTRL_ENABLE	0x01	/* Enable bit */
> -#define  PCI_PASID_CTRL_EXEC	0x02	/* Exec permissions Enable */
> -#define  PCI_PASID_CTRL_PRIV	0x04	/* Privilege Mode Enable */
> +#define  PCI_PASID_CTRL_ENABLE	0x0001	/* Enable bit */
> +#define  PCI_PASID_CTRL_EXEC	0x0002	/* Exec permissions Enable */
> +#define  PCI_PASID_CTRL_PRIV	0x0004	/* Privilege Mode Enable */
>  #define PCI_EXT_CAP_PASID_SIZEOF	8

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 07/10] PCI/PME: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/pme.c        | 4 +++-
>  include/uapi/linux/pci_regs.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/pme.c b/drivers/pci/pcie/pme.c
> index ef8ce436ead9..a2daebd9806c 100644
> --- a/drivers/pci/pcie/pme.c
> +++ b/drivers/pci/pcie/pme.c
> @@ -9,6 +9,7 @@
>  
>  #define dev_fmt(fmt) "PME: " fmt
>  
> +#include <linux/bitfield.h>
>  #include <linux/pci.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> @@ -235,7 +236,8 @@ static void pcie_pme_work_fn(struct work_struct *work)
>  			pcie_clear_root_pme_status(port);
>  
>  			spin_unlock_irq(&data->lock);
> -			pcie_pme_handle_request(port, rtsta & 0xffff);
> +			pcie_pme_handle_request(port,
> +				    FIELD_GET(PCI_EXP_RTSTA_PME_RQ_ID, rtsta));
>  			spin_lock_irq(&data->lock);
>  
>  			continue;
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e97a06b50f95..9fb8a69241f4 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -637,6 +637,7 @@
>  #define PCI_EXP_RTCAP		0x1e	/* Root Capabilities */
>  #define  PCI_EXP_RTCAP_CRSVIS	0x0001	/* CRS Software Visibility capability */
>  #define PCI_EXP_RTSTA		0x20	/* Root Status */
> +#define  PCI_EXP_RTSTA_PME_RQ_ID 0x0000ffff /* PME Requester ID */
>  #define  PCI_EXP_RTSTA_PME	0x00010000 /* PME status */
>  #define  PCI_EXP_RTSTA_PENDING	0x00020000 /* PME pending */

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

(I'd have used REQ myself but it seems both forms are commonly used by 
the existing defines.)

-- 
 i.

Re: [PATCH 09/10] PCI/VC: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/vc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/vc.c b/drivers/pci/vc.c
> index 5fc59ac31145..a4ff7f5f66dd 100644
> --- a/drivers/pci/vc.c
> +++ b/drivers/pci/vc.c
> @@ -6,6 +6,7 @@
>   *     Author: Alex Williamson <alex.williamson@redhat.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -201,9 +202,9 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
>  	/* Extended VC Count (not counting VC0) */
>  	evcc = cap1 & PCI_VC_CAP1_EVCC;
>  	/* Low Priority Extended VC Count (not counting VC0) */
> -	lpevcc = (cap1 & PCI_VC_CAP1_LPEVCC) >> 4;
> +	lpevcc = FIELD_GET(PCI_VC_CAP1_LPEVCC, cap1);
>  	/* Port Arbitration Table Entry Size (bits) */
> -	parb_size = 1 << ((cap1 & PCI_VC_CAP1_ARB_SIZE) >> 10);
> +	parb_size = 1 << FIELD_GET(PCI_VC_CAP1_ARB_SIZE, cap1);
>  
>  	/*
>  	 * Port VC Control Register contains VC Arbitration Select, which
> @@ -231,7 +232,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
>  		int vcarb_offset;
>  
>  		pci_read_config_dword(dev, pos + PCI_VC_PORT_CAP2, &cap2);
> -		vcarb_offset = ((cap2 & PCI_VC_CAP2_ARB_OFF) >> 24) * 16;
> +		vcarb_offset = FIELD_GET(PCI_VC_CAP2_ARB_OFF, cap2) * 16;
>  
>  		if (vcarb_offset) {
>  			int size, vcarb_phases = 0;
> @@ -277,7 +278,7 @@ static int pci_vc_do_save_buffer(struct pci_dev *dev, int pos,
>  
>  		pci_read_config_dword(dev, pos + PCI_VC_RES_CAP +
>  				      (i * PCI_CAP_VC_PER_VC_SIZEOF), &cap);
> -		parb_offset = ((cap & PCI_VC_RES_CAP_ARB_OFF) >> 24) * 16;
> +		parb_offset = FIELD_GET(PCI_VC_RES_CAP_ARB_OFF, cap) * 16;
>  		if (parb_offset) {
>  			int size, parb_phases = 0;

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 10/10] PCI/portdrv: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1824 bytes --]

On Tue, 10 Oct 2023, Bjorn Helgaas wrote:

> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() to remove dependences on the field position, i.e., the
> shift value.  No functional change intended.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/pci/pcie/portdrv.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 46fad0d813b2..14a4b89a3b83 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -6,6 +6,7 @@
>   * Copyright (C) Tom Long Nguyen (tom.l.nguyen@intel.com)
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/dmi.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
> @@ -69,7 +70,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
>  	if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP |
>  		    PCIE_PORT_SERVICE_BWNOTIF)) {
>  		pcie_capability_read_word(dev, PCI_EXP_FLAGS, &reg16);
> -		*pme = (reg16 & PCI_EXP_FLAGS_IRQ) >> 9;
> +		*pme = FIELD_GET(PCI_EXP_FLAGS_IRQ, reg16);
>  		nvec = *pme + 1;
>  	}
>  
> @@ -81,7 +82,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
>  		if (pos) {
>  			pci_read_config_dword(dev, pos + PCI_ERR_ROOT_STATUS,
>  					      &reg32);
> -			*aer = (reg32 & PCI_ERR_ROOT_AER_IRQ) >> 27;
> +			*aer = FIELD_GET(PCI_ERR_ROOT_AER_IRQ, reg32);
>  			nvec = max(nvec, *aer + 1);
>  		}
>  	}
> @@ -92,7 +93,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask,
>  		if (pos) {
>  			pci_read_config_word(dev, pos + PCI_EXP_DPC_CAP,
>  					     &reg16);
> -			*dpc = reg16 & PCI_EXP_DPC_IRQ;
> +			*dpc = FIELD_GET(PCI_EXP_DPC_IRQ, reg16);
>  			nvec = max(nvec, *dpc + 1);

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH 00/10] PCI: Use FIELD_GET() and FIELD_PREP()

[Kuppuswamy Sathyanarayanan]

On 10/10/2023 1:44 PM, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() and FIELD_PREP() to avoid the need for shifting.
> 
> These apply on top of the PCI patches from Ilpo's series:
>   https://lore.kernel.org/r/20230919125648.1920-1-ilpo.jarvinen@linux.intel.com
> 

Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>


> Bjorn Helgaas (10):
>   PCI: Use FIELD_GET()
>   PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk
>   PCI/ASPM: Use FIELD_GET()
>   PCI/ATS: Show PASID Capability register width in bitmasks
>   PCI/ATS: Use FIELD_GET()
>   PCI/DPC: Use FIELD_GET()
>   PCI/PME: Use FIELD_GET()
>   PCI/PTM: Use FIELD_GET()
>   PCI/VC: Use FIELD_GET()
>   PCI/portdrv: Use FIELD_GET()
> 
>  drivers/pci/ats.c             |  7 ++---
>  drivers/pci/pci.c             | 53 +++++++++++++++++------------------
>  drivers/pci/pcie/aspm.c       | 31 +++++++++++---------
>  drivers/pci/pcie/dpc.c        |  9 +++---
>  drivers/pci/pcie/pme.c        |  4 ++-
>  drivers/pci/pcie/portdrv.c    |  7 +++--
>  drivers/pci/pcie/ptm.c        |  5 ++--
>  drivers/pci/probe.c           |  8 +++---
>  drivers/pci/quirks.c          |  2 +-
>  drivers/pci/vc.c              |  9 +++---
>  include/uapi/linux/pci_regs.h | 15 ++++++----
>  11 files changed, 81 insertions(+), 69 deletions(-)
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2692 bytes --]

On Wed, 11 Oct 2023, Ilpo Järvinen wrote:

> On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> 
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value.  No functional change intended.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pcie/dpc.c        | 9 +++++----
> >  drivers/pci/quirks.c          | 2 +-
> >  include/uapi/linux/pci_regs.h | 1 +
> >  3 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3ceed8e3de41..6e551f34ec63 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -8,6 +8,7 @@
> >  
> >  #define dev_fmt(fmt) "DPC: " fmt
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/aer.h>
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> >  
> >  	/* Get First Error Pointer */
> >  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > -	first_error = (dpc_status & 0x1f00) >> 8;
> > +	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> >  		if ((status & ~mask) & (1 << i))
> > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> >  	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> >  		 status, source);
> >  
> > -	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > -	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > +	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > +	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> >  	pci_warn(pdev, "%s detected\n",
> >  		 (reason == 0) ? "unmasked uncorrectable error" :
> >  		 (reason == 1) ? "ERR_NONFATAL" :
> 
> BTW, it seems we're doing overlapping work here with many of these 
> patches. It takes some time to think these through one by one, I don't 
> just autorun through them with coccinelle so I've not posted my changes
> yet.
> 
> I went to a different direction here and named all the reasons too with 
> defines and used & to get the reason in order to be able to compare with 
> the named reasons.
> 
> You also missed convering one 0xfff4 to use define (although I suspect it 
> never was your goal to go beyond FIELD_GET() here).

I posted my approach onto the list so you can see what it looks like:
  https://lore.kernel.org/linux-pci/20231013112004.4239-1-ilpo.jarvinen@linux.intel.com/T/#t

(It will obviously conflict with this patch so both cannot be applied as 
is).

-- 
 i.

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Bjorn Helgaas]

On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > shift value.  No functional change intended.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > ---
> >  drivers/pci/pcie/dpc.c        | 9 +++++----
> >  drivers/pci/quirks.c          | 2 +-
> >  include/uapi/linux/pci_regs.h | 1 +
> >  3 files changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 3ceed8e3de41..6e551f34ec63 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -8,6 +8,7 @@
> >  
> >  #define dev_fmt(fmt) "DPC: " fmt
> >  
> > +#include <linux/bitfield.h>
> >  #include <linux/aer.h>
> >  #include <linux/delay.h>
> >  #include <linux/interrupt.h>
> > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> >  
> >  	/* Get First Error Pointer */
> >  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > -	first_error = (dpc_status & 0x1f00) >> 8;
> > +	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> >  
> >  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> >  		if ((status & ~mask) & (1 << i))
> > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> >  	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> >  		 status, source);
> >  
> > -	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > -	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > +	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > +	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> >  	pci_warn(pdev, "%s detected\n",
> >  		 (reason == 0) ? "unmasked uncorrectable error" :
> >  		 (reason == 1) ? "ERR_NONFATAL" :
> 
> BTW, it seems we're doing overlapping work here with many of these 
> patches. It takes some time to think these through one by one, I don't 
> just autorun through them with coccinelle so I've not posted my changes
> yet.
>
> I went to a different direction here and named all the reasons too with 
> defines and used & to get the reason in order to be able to compare with 
> the named reasons.
> 
> You also missed convering one 0xfff4 to use define (although I suspect it 
> never was your goal to go beyond FIELD_GET() here).

Pure FIELD_GET() and FIELD_PREP() was my goal.

If you have patches you prefer, I'll drop mine.  I did these about a
year ago and it seemed like the time to do something with them since
you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
work.  Since we've started, I'd like to get as much of it done this
cycle as possible.

Bjorn

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3183 bytes --]

On Fri, 13 Oct 2023, Bjorn Helgaas wrote:

> On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > 
> > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > shift value.  No functional change intended.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > ---
> > >  drivers/pci/pcie/dpc.c        | 9 +++++----
> > >  drivers/pci/quirks.c          | 2 +-
> > >  include/uapi/linux/pci_regs.h | 1 +
> > >  3 files changed, 7 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > index 3ceed8e3de41..6e551f34ec63 100644
> > > --- a/drivers/pci/pcie/dpc.c
> > > +++ b/drivers/pci/pcie/dpc.c
> > > @@ -8,6 +8,7 @@
> > >  
> > >  #define dev_fmt(fmt) "DPC: " fmt
> > >  
> > > +#include <linux/bitfield.h>
> > >  #include <linux/aer.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/interrupt.h>
> > > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > >  
> > >  	/* Get First Error Pointer */
> > >  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > > -	first_error = (dpc_status & 0x1f00) >> 8;
> > > +	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> > >  
> > >  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > >  		if ((status & ~mask) & (1 << i))
> > > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > >  	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > >  		 status, source);
> > >  
> > > -	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > > -	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > > +	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > +	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > >  	pci_warn(pdev, "%s detected\n",
> > >  		 (reason == 0) ? "unmasked uncorrectable error" :
> > >  		 (reason == 1) ? "ERR_NONFATAL" :
> > 
> > BTW, it seems we're doing overlapping work here with many of these 
> > patches. It takes some time to think these through one by one, I don't 
> > just autorun through them with coccinelle so I've not posted my changes
> > yet.
> >
> > I went to a different direction here and named all the reasons too with 
> > defines and used & to get the reason in order to be able to compare with 
> > the named reasons.
> > 
> > You also missed convering one 0xfff4 to use define (although I suspect it 
> > never was your goal to go beyond FIELD_GET() here).
> 
> Pure FIELD_GET() and FIELD_PREP() was my goal.
> 
> If you have patches you prefer, I'll drop mine.  I did these about a
> year ago and it seemed like the time to do something with them since
> you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> work.  Since we've started, I'd like to get as much of it done this
> cycle as possible.

Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting 
more and more complicated. I can build a nice set of small changes about 
what remains to do in DPC on top of your patch.

-- 
 i.

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3593 bytes --]

On Mon, 16 Oct 2023, Ilpo Järvinen wrote:

> On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
> 
> > On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > 
> > > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > > shift value.  No functional change intended.
> > > > 
> > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > ---
> > > >  drivers/pci/pcie/dpc.c        | 9 +++++----
> > > >  drivers/pci/quirks.c          | 2 +-
> > > >  include/uapi/linux/pci_regs.h | 1 +
> > > >  3 files changed, 7 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > index 3ceed8e3de41..6e551f34ec63 100644
> > > > --- a/drivers/pci/pcie/dpc.c
> > > > +++ b/drivers/pci/pcie/dpc.c
> > > > @@ -8,6 +8,7 @@
> > > >  
> > > >  #define dev_fmt(fmt) "DPC: " fmt
> > > >  
> > > > +#include <linux/bitfield.h>
> > > >  #include <linux/aer.h>
> > > >  #include <linux/delay.h>
> > > >  #include <linux/interrupt.h>
> > > > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > > >  
> > > >  	/* Get First Error Pointer */
> > > >  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > > > -	first_error = (dpc_status & 0x1f00) >> 8;
> > > > +	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> > > >  
> > > >  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > >  		if ((status & ~mask) & (1 << i))
> > > > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > > >  	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > > >  		 status, source);
> > > >  
> > > > -	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > > > -	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > > > +	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > > +	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > >  	pci_warn(pdev, "%s detected\n",
> > > >  		 (reason == 0) ? "unmasked uncorrectable error" :
> > > >  		 (reason == 1) ? "ERR_NONFATAL" :
> > > 
> > > BTW, it seems we're doing overlapping work here with many of these 
> > > patches. It takes some time to think these through one by one, I don't 
> > > just autorun through them with coccinelle so I've not posted my changes
> > > yet.
> > >
> > > I went to a different direction here and named all the reasons too with 
> > > defines and used & to get the reason in order to be able to compare with 
> > > the named reasons.
> > > 
> > > You also missed convering one 0xfff4 to use define (although I suspect it 
> > > never was your goal to go beyond FIELD_GET() here).
> > 
> > Pure FIELD_GET() and FIELD_PREP() was my goal.
> > 
> > If you have patches you prefer, I'll drop mine.  I did these about a
> > year ago and it seemed like the time to do something with them since
> > you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> > work.  Since we've started, I'd like to get as much of it done this
> > cycle as possible.
> 
> Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting 
> more and more complicated. I can build a nice set of small changes about 
> what remains to do in DPC on top of your patch.

Err, actually, there's still the naming of the define, should _FEP be used 
for First Error Pointer for consistency? You should make that small change 
into your patch if you think _FEP is better because of consistency.


-- 
 i.

Re: [PATCH 06/10] PCI/DPC: Use FIELD_GET()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3901 bytes --]

On Mon, 16 Oct 2023, Ilpo Järvinen wrote:

> On Mon, 16 Oct 2023, Ilpo Järvinen wrote:
> 
> > On Fri, 13 Oct 2023, Bjorn Helgaas wrote:
> > 
> > > On Wed, Oct 11, 2023 at 02:01:13PM +0300, Ilpo Järvinen wrote:
> > > > On Tue, 10 Oct 2023, Bjorn Helgaas wrote:
> > > > > From: Bjorn Helgaas <bhelgaas@google.com>
> > > > > 
> > > > > Use FIELD_GET() to remove dependences on the field position, i.e., the
> > > > > shift value.  No functional change intended.
> > > > > 
> > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> > > > > ---
> > > > >  drivers/pci/pcie/dpc.c        | 9 +++++----
> > > > >  drivers/pci/quirks.c          | 2 +-
> > > > >  include/uapi/linux/pci_regs.h | 1 +
> > > > >  3 files changed, 7 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > > > > index 3ceed8e3de41..6e551f34ec63 100644
> > > > > --- a/drivers/pci/pcie/dpc.c
> > > > > +++ b/drivers/pci/pcie/dpc.c
> > > > > @@ -8,6 +8,7 @@
> > > > >  
> > > > >  #define dev_fmt(fmt) "DPC: " fmt
> > > > >  
> > > > > +#include <linux/bitfield.h>
> > > > >  #include <linux/aer.h>
> > > > >  #include <linux/delay.h>
> > > > >  #include <linux/interrupt.h>
> > > > > @@ -202,7 +203,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > > > >  
> > > > >  	/* Get First Error Pointer */
> > > > >  	pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, &dpc_status);
> > > > > -	first_error = (dpc_status & 0x1f00) >> 8;
> > > > > +	first_error = FIELD_GET(PCI_EXP_DPC_STATUS_FIRST_ERR, dpc_status);
> > > > >  
> > > > >  	for (i = 0; i < ARRAY_SIZE(rp_pio_error_string); i++) {
> > > > >  		if ((status & ~mask) & (1 << i))
> > > > > @@ -270,8 +271,8 @@ void dpc_process_error(struct pci_dev *pdev)
> > > > >  	pci_info(pdev, "containment event, status:%#06x source:%#06x\n",
> > > > >  		 status, source);
> > > > >  
> > > > > -	reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN) >> 1;
> > > > > -	ext_reason = (status & PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT) >> 5;
> > > > > +	reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN, status);
> > > > > +	ext_reason = FIELD_GET(PCI_EXP_DPC_STATUS_TRIGGER_RSN_EXT, status);
> > > > >  	pci_warn(pdev, "%s detected\n",
> > > > >  		 (reason == 0) ? "unmasked uncorrectable error" :
> > > > >  		 (reason == 1) ? "ERR_NONFATAL" :
> > > > 
> > > > BTW, it seems we're doing overlapping work here with many of these 
> > > > patches. It takes some time to think these through one by one, I don't 
> > > > just autorun through them with coccinelle so I've not posted my changes
> > > > yet.
> > > >
> > > > I went to a different direction here and named all the reasons too with 
> > > > defines and used & to get the reason in order to be able to compare with 
> > > > the named reasons.
> > > > 
> > > > You also missed convering one 0xfff4 to use define (although I suspect it 
> > > > never was your goal to go beyond FIELD_GET() here).
> > > 
> > > Pure FIELD_GET() and FIELD_PREP() was my goal.
> > > 
> > > If you have patches you prefer, I'll drop mine.  I did these about a
> > > year ago and it seemed like the time to do something with them since
> > > you did the PCI_EXP_LNKSTA_NLW ones and to try to prevent overlapping
> > > work.  Since we've started, I'd like to get as much of it done this
> > > cycle as possible.
> > 
> > Okay, I suggest you keep your FIELD_GET/PREP() patch since mine is getting 
> > more and more complicated. I can build a nice set of small changes about 
> > what remains to do in DPC on top of your patch.
> 
> Err, actually, there's still the naming of the define, should _FEP be used 
> for First Error Pointer for consistency? You should make that small change 
> into your patch if you think _FEP is better because of consistency.

There's also #include order so it seems you should just drop the patch, I 
can handle this along my series.

-- 
 i.

Re: [PATCH 00/10] PCI: Use FIELD_GET() and FIELD_PREP()

[Bjorn Helgaas]

On Tue, Oct 10, 2023 at 03:44:26PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Use FIELD_GET() and FIELD_PREP() to avoid the need for shifting.
> 
> These apply on top of the PCI patches from Ilpo's series:
>   https://lore.kernel.org/r/20230919125648.1920-1-ilpo.jarvinen@linux.intel.com
> 
> Bjorn Helgaas (10):
>   PCI: Use FIELD_GET()
>   PCI: Use FIELD_GET() in Sapphire RX 5600 XT Pulse quirk
>   PCI/ASPM: Use FIELD_GET()
>   PCI/ATS: Show PASID Capability register width in bitmasks
>   PCI/ATS: Use FIELD_GET()
>   PCI/DPC: Use FIELD_GET()
>   PCI/PME: Use FIELD_GET()
>   PCI/PTM: Use FIELD_GET()
>   PCI/VC: Use FIELD_GET()
>   PCI/portdrv: Use FIELD_GET()
> 
>  drivers/pci/ats.c             |  7 ++---
>  drivers/pci/pci.c             | 53 +++++++++++++++++------------------
>  drivers/pci/pcie/aspm.c       | 31 +++++++++++---------
>  drivers/pci/pcie/dpc.c        |  9 +++---
>  drivers/pci/pcie/pme.c        |  4 ++-
>  drivers/pci/pcie/portdrv.c    |  7 +++--
>  drivers/pci/pcie/ptm.c        |  5 ++--
>  drivers/pci/probe.c           |  8 +++---
>  drivers/pci/quirks.c          |  2 +-
>  drivers/pci/vc.c              |  9 +++---
>  include/uapi/linux/pci_regs.h | 15 ++++++----
>  11 files changed, 81 insertions(+), 69 deletions(-)

I applied these to pci/field-get for v6.7, thanks for all the reviews!