[PATCH 0/3] PCI: Resource helper improvements

[PATCH 0/3] PCI: Resource helper improvements

[Ilpo Järvinen]

This series introduces resource_set_{range,size}() helpers to replace
often repeated code fragments to set resource start and end addresses.
The last patch cleans up nearby code to use pre-existing helpers &
defines.

For now, this series only converts PCI subsystem. There are plenty of
resource start/end setting code elsewhere in the kernel but those can
be converted once the helpers reach Linus' tree.

Ilpo Järvinen (3):
  resource: Add resource set range and size helpers
  PCI: Use resource_set_{range,size}() helpers
  PCI: Use align and resource helpers, and SZ_* in quirk_s3_64M()

 drivers/pci/controller/pci-tegra.c       |  2 +-
 drivers/pci/controller/pci-thunder-pem.c |  4 +--
 drivers/pci/ecam.c                       |  2 +-
 drivers/pci/iov.c                        |  6 ++--
 drivers/pci/pci.c                        |  3 +-
 drivers/pci/quirks.c                     | 23 +++++++---------
 drivers/pci/setup-bus.c                  | 35 ++++++++++--------------
 drivers/pci/setup-res.c                  |  7 ++---
 include/linux/ioport.h                   | 32 ++++++++++++++++++++++
 9 files changed, 68 insertions(+), 46 deletions(-)

-- 
2.39.2

[PATCH 1/3] resource: Add resource set range and size helpers

[Ilpo Järvinen]

Setting the end address for a resource with a given size lacks a helper
and is therefore open coded unlike the getter side which has a helper
for resource size calculation. Also, almost all callsites that
calculate end address for a resource also set the start address right
before it like this:

	res->start = start_addr;
	res->end = res->start + size - 1;

Thus, add resource_set_range(res, start_addr, size) that sets the start
address and calculates the end address to simplify this often repeated
fragment. In addition, introduce resource_set_size() for the cases
where setting the start address of the resource is not necessary but
note resource_set_range() is preferred.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index db7fe25f3370..2a1d33ad151c 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start);
 int adjust_resource(struct resource *res, resource_size_t start,
 		    resource_size_t size);
 resource_size_t resource_alignment(struct resource *res);
+
+/**
+ * resource_set_size - Calculates resource end address from size and start address
+ * @res: The resource descriptor
+ * @size: The size of the resource
+ *
+ * Calculates the end address for @res based on @size.
+ *
+ * Note: The start address of @res must be set when calling this function.
+ * Use resource_set_range() if setting both the start address and @size.
+ */
+static inline void resource_set_size(struct resource *res, resource_size_t size)
+{
+	res->end = res->start + size - 1;
+}
+
+/**
+ * resource_set_range - Sets resource start and end addresses
+ * @res: The resource descriptor
+ * @start: The start address for the resource
+ * @size: The size of the resource
+ *
+ * Sets @res start address and calculates the end address based on @size.
+ */
+static inline void resource_set_range(struct resource *res,
+				      resource_size_t start,
+				      resource_size_t size)
+{
+	res->start = start;
+	resource_set_size(res, size);
+}
+
 static inline resource_size_t resource_size(const struct resource *res)
 {
 	return res->end - res->start + 1;
-- 
2.39.2

[PATCH 2/3] PCI: Use resource_set_{range,size}() helpers

[Ilpo Järvinen]

Convert open-coded resource size calculations to use
resource_set_{range,size}() helpers.

While at it, use SZ_* for size parameter where appropriate which makes
the intent of code more obvious.

Also, cast sizes to resource_size_t, not u64.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-tegra.c       |  2 +-
 drivers/pci/controller/pci-thunder-pem.c |  4 +--
 drivers/pci/ecam.c                       |  2 +-
 drivers/pci/iov.c                        |  6 ++--
 drivers/pci/pci.c                        |  3 +-
 drivers/pci/quirks.c                     | 20 ++++++--------
 drivers/pci/setup-bus.c                  | 35 ++++++++++--------------
 drivers/pci/setup-res.c                  |  7 ++---
 8 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c
index 038d974a318e..2ce55ed1cd8b 100644
--- a/drivers/pci/controller/pci-tegra.c
+++ b/drivers/pci/controller/pci-tegra.c
@@ -1460,7 +1460,7 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie)
 	pcie->cs = *res;
 
 	/* constrain configuration space to 4 KiB */
-	pcie->cs.end = pcie->cs.start + SZ_4K - 1;
+	resource_set_size(&pcie->cs, SZ_4K);
 
 	pcie->cfg = devm_ioremap_resource(dev, &pcie->cs);
 	if (IS_ERR(pcie->cfg)) {
diff --git a/drivers/pci/controller/pci-thunder-pem.c b/drivers/pci/controller/pci-thunder-pem.c
index 06a9855cb431..f1bd5de67997 100644
--- a/drivers/pci/controller/pci-thunder-pem.c
+++ b/drivers/pci/controller/pci-thunder-pem.c
@@ -400,9 +400,9 @@ static int thunder_pem_acpi_init(struct pci_config_window *cfg)
 		 * Reserve 64K size PEM specific resources. The full 16M range
 		 * size is required for thunder_pem_init() call.
 		 */
-		res_pem->end = res_pem->start + SZ_64K - 1;
+		resource_set_size(res_pem, SZ_64K);
 		thunder_pem_reserve_range(dev, root->segment, res_pem);
-		res_pem->end = res_pem->start + SZ_16M - 1;
+		resource_set_size(res_pem, SZ_16M);
 
 		/* Reserve PCI configuration space as well. */
 		thunder_pem_reserve_range(dev, root->segment, &cfg->res);
diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 1c40d2506aef..260b7de2dbd5 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -55,7 +55,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
 	bus_range_max = resource_size(cfgres) >> bus_shift;
 	if (bus_range > bus_range_max) {
 		bus_range = bus_range_max;
-		cfg->busr.end = busr->start + bus_range - 1;
+		resource_set_size(&cfg->busr, bus_range);
 		dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
 			 cfgres, &cfg->busr, busr);
 	}
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
index aaa33e8dc4c9..4be402fe9ab9 100644
--- a/drivers/pci/iov.c
+++ b/drivers/pci/iov.c
@@ -327,8 +327,8 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
 		virtfn->resource[i].name = pci_name(virtfn);
 		virtfn->resource[i].flags = res->flags;
 		size = pci_iov_resource_size(dev, i + PCI_IOV_RESOURCES);
-		virtfn->resource[i].start = res->start + size * id;
-		virtfn->resource[i].end = virtfn->resource[i].start + size - 1;
+		resource_set_range(&virtfn->resource[i],
+				   res->start + size * id, size);
 		rc = request_resource(res, &virtfn->resource[i]);
 		BUG_ON(rc);
 	}
@@ -804,7 +804,7 @@ static int sriov_init(struct pci_dev *dev, int pos)
 			goto failed;
 		}
 		iov->barsz[i] = resource_size(res);
-		res->end = res->start + resource_size(res) * total - 1;
+		resource_set_size(res, resource_size(res) * total);
 		pci_info(dev, "%s %pR: contains BAR %d for %d VFs\n",
 			 res_name, res, i, total);
 		i += bar64;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 59e0949fb079..afcd8e49e82c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6559,8 +6559,7 @@ static void pci_request_resource_alignment(struct pci_dev *dev, int bar,
 	} else {
 		r->flags &= ~IORESOURCE_SIZEALIGN;
 		r->flags |= IORESOURCE_STARTALIGN;
-		r->start = align;
-		r->end = r->start + size - 1;
+		resource_set_range(r, align, size);
 	}
 	r->flags |= IORESOURCE_UNSET;
 }
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 568410e64ce6..bde0f5388d06 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -29,6 +29,7 @@
 #include <linux/nvme.h>
 #include <linux/platform_data/x86/apple.h>
 #include <linux/pm_runtime.h>
+#include <linux/sizes.h>
 #include <linux/suspend.h>
 #include <linux/switchtec.h>
 #include "pci.h"
@@ -573,8 +574,7 @@ static void quirk_extend_bar_to_page(struct pci_dev *dev)
 		const char *r_name = pci_resource_name(dev, i);
 
 		if (r->flags & IORESOURCE_MEM && resource_size(r) < PAGE_SIZE) {
-			r->end = PAGE_SIZE - 1;
-			r->start = 0;
+			resource_set_range(r, 0, PAGE_SIZE);
 			r->flags |= IORESOURCE_UNSET;
 			pci_info(dev, "%s %pR: expanded to page size\n",
 				 r_name, r);
@@ -593,8 +593,7 @@ static void quirk_s3_64M(struct pci_dev *dev)
 
 	if ((r->start & 0x3ffffff) || r->end != r->start + 0x3ffffff) {
 		r->flags |= IORESOURCE_UNSET;
-		r->start = 0;
-		r->end = 0x3ffffff;
+		resource_set_range(r, 0, SZ_64M);
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_S3,	PCI_DEVICE_ID_S3_868,		quirk_s3_64M);
@@ -1329,8 +1328,7 @@ static void quirk_dunord(struct pci_dev *dev)
 	struct resource *r = &dev->resource[1];
 
 	r->flags |= IORESOURCE_UNSET;
-	r->start = 0;
-	r->end = 0xffffff;
+	resource_set_range(r, 0, SZ_16M);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DUNORD,	PCI_DEVICE_ID_DUNORD_I3000,	quirk_dunord);
 
@@ -2327,8 +2325,7 @@ static void quirk_tc86c001_ide(struct pci_dev *dev)
 
 	if (r->start & 0x8) {
 		r->flags |= IORESOURCE_UNSET;
-		r->start = 0;
-		r->end = 0xf;
+		resource_set_range(r, 0, SZ_16);
 	}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TOSHIBA_2,
@@ -2356,8 +2353,7 @@ static void quirk_plx_pci9050(struct pci_dev *dev)
 			pci_info(dev, "Re-allocating PLX PCI 9050 BAR %u to length 256 to avoid bit 7 bug\n",
 				 bar);
 			r->flags |= IORESOURCE_UNSET;
-			r->start = 0;
-			r->end = 0xff;
+			resource_set_range(r, 0, SZ_256);
 		}
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050,
@@ -3509,13 +3505,13 @@ static void quirk_intel_ntb(struct pci_dev *dev)
 	if (rc)
 		return;
 
-	dev->resource[2].end = dev->resource[2].start + ((u64) 1 << val) - 1;
+	resource_set_size(&dev->resource[2], (resource_size_t)1 << val);
 
 	rc = pci_read_config_byte(dev, 0x00D1, &val);
 	if (rc)
 		return;
 
-	dev->resource[4].end = dev->resource[4].start + ((u64) 1 << val) - 1;
+	resource_set_size(&dev->resource[4], (resource_size_t)1 << val);
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0e08, quirk_intel_ntb);
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x0e0d, quirk_intel_ntb);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 909e6a7c3cc3..004405edf290 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -243,8 +243,7 @@ static void reassign_resources_sorted(struct list_head *realloc_head,
 		add_size = add_res->add_size;
 		align = add_res->min_align;
 		if (!resource_size(res)) {
-			res->start = align;
-			res->end = res->start + add_size - 1;
+			resource_set_range(res, align, add_size);
 			if (pci_assign_resource(add_res->dev, idx))
 				reset_resource(res);
 		} else {
@@ -937,8 +936,7 @@ static void pbus_size_io(struct pci_bus *bus, resource_size_t min_size,
 		return;
 	}
 
-	b_res->start = min_align;
-	b_res->end = b_res->start + size0 - 1;
+	resource_set_range(b_res, min_align, size0);
 	b_res->flags |= IORESOURCE_STARTALIGN;
 	if (bus->self && size1 > size0 && realloc_head) {
 		add_to_list(realloc_head, bus->self, b_res, size1-size0,
@@ -1127,8 +1125,7 @@ static void pci_bus_size_cardbus(struct pci_bus *bus,
 	 * Reserve some resources for CardBus.  We reserve a fixed amount
 	 * of bus space for CardBus bridges.
 	 */
-	b_res->start = pci_cardbus_io_size;
-	b_res->end = b_res->start + pci_cardbus_io_size - 1;
+	resource_set_range(b_res, pci_cardbus_io_size, pci_cardbus_io_size);
 	b_res->flags |= IORESOURCE_IO | IORESOURCE_STARTALIGN;
 	if (realloc_head) {
 		b_res->end -= pci_cardbus_io_size;
@@ -1140,8 +1137,7 @@ static void pci_bus_size_cardbus(struct pci_bus *bus,
 	b_res = &bridge->resource[PCI_CB_BRIDGE_IO_1_WINDOW];
 	if (b_res->parent)
 		goto handle_b_res_2;
-	b_res->start = pci_cardbus_io_size;
-	b_res->end = b_res->start + pci_cardbus_io_size - 1;
+	resource_set_range(b_res, pci_cardbus_io_size, pci_cardbus_io_size);
 	b_res->flags |= IORESOURCE_IO | IORESOURCE_STARTALIGN;
 	if (realloc_head) {
 		b_res->end -= pci_cardbus_io_size;
@@ -1174,8 +1170,8 @@ static void pci_bus_size_cardbus(struct pci_bus *bus,
 	 * Otherwise, allocate one region of twice the size.
 	 */
 	if (ctrl & PCI_CB_BRIDGE_CTL_PREFETCH_MEM0) {
-		b_res->start = pci_cardbus_mem_size;
-		b_res->end = b_res->start + pci_cardbus_mem_size - 1;
+		resource_set_range(b_res, pci_cardbus_mem_size,
+				   pci_cardbus_mem_size);
 		b_res->flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH |
 				    IORESOURCE_STARTALIGN;
 		if (realloc_head) {
@@ -1192,8 +1188,7 @@ static void pci_bus_size_cardbus(struct pci_bus *bus,
 	b_res = &bridge->resource[PCI_CB_BRIDGE_MEM_1_WINDOW];
 	if (b_res->parent)
 		goto handle_done;
-	b_res->start = pci_cardbus_mem_size;
-	b_res->end = b_res->start + b_res_3_size - 1;
+	resource_set_range(b_res, pci_cardbus_mem_size, b_res_3_size);
 	b_res->flags |= IORESOURCE_MEM | IORESOURCE_STARTALIGN;
 	if (realloc_head) {
 		b_res->end -= b_res_3_size;
@@ -1772,7 +1767,7 @@ static void adjust_bridge_window(struct pci_dev *bridge, struct resource *res,
 		return;
 	}
 
-	res->end = res->start + new_size - 1;
+	resource_set_size(res, new_size);
 
 	/* If the resource is part of the add_list, remove it now */
 	if (add_list)
@@ -1935,8 +1930,8 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 		 * what is available).
 		 */
 		align = pci_resource_alignment(dev, res);
-		io.end = align ? io.start + ALIGN_DOWN(io_per_b, align) - 1
-			       : io.start + io_per_b - 1;
+		resource_set_size(&io, align ? ALIGN_DOWN(io_per_b, align)
+					     : io_per_b);
 
 		/*
 		 * The x_per_b holds the extra resource space that can be
@@ -1948,15 +1943,15 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
 
 		res = &dev->resource[PCI_BRIDGE_MEM_WINDOW];
 		align = pci_resource_alignment(dev, res);
-		mmio.end = align ? mmio.start + ALIGN_DOWN(mmio_per_b, align) - 1
-				 : mmio.start + mmio_per_b - 1;
+		resource_set_size(&mmio, align ? ALIGN_DOWN(mmio_per_b, align)
+					       : mmio_per_b);
 		mmio.start -= resource_size(res);
 
 		res = &dev->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
 		align = pci_resource_alignment(dev, res);
-		mmio_pref.end = align ? mmio_pref.start +
-					ALIGN_DOWN(mmio_pref_per_b, align) - 1
-				      : mmio_pref.start + mmio_pref_per_b - 1;
+		resource_set_size(&mmio_pref,
+				  align ? ALIGN_DOWN(mmio_pref_per_b, align)
+					: mmio_pref_per_b);
 		mmio_pref.start -= resource_size(res);
 
 		pci_bus_distribute_available_resources(b, add_list, io, mmio,
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index c6d933ddfd46..ca14576bf2bf 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -211,8 +211,7 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev,
 
 	start = res->start;
 	end = res->end;
-	res->start = fw_addr;
-	res->end = res->start + size - 1;
+	resource_set_range(res, fw_addr, size);
 	res->flags &= ~IORESOURCE_UNSET;
 
 	root = pci_find_parent_resource(dev, res);
@@ -463,7 +462,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 	if (ret)
 		return ret;
 
-	res->end = res->start + pci_rebar_size_to_bytes(size) - 1;
+	resource_set_size(res, pci_rebar_size_to_bytes(size));
 
 	/* Check if the new config works by trying to assign everything. */
 	if (dev->bus->self) {
@@ -475,7 +474,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
 
 error_resize:
 	pci_rebar_set_size(dev, resno, old);
-	res->end = res->start + pci_rebar_size_to_bytes(old) - 1;
+	resource_set_size(res, pci_rebar_size_to_bytes(old));
 	return ret;
 }
 EXPORT_SYMBOL(pci_resize_resource);
-- 
2.39.2

[PATCH 3/3] PCI: Use align and resource helpers, and SZ_* in quirk_s3_64M()

[Ilpo Järvinen]

Use IS_ALIGNED(), resource_size(), and SZ_* defines in quirk_s3_64M().

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/quirks.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index bde0f5388d06..125fc1cbad95 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -12,6 +12,7 @@
  * file, where their drivers can use them.
  */
 
+#include <linux/align.h>
 #include <linux/bitfield.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -591,7 +592,7 @@ static void quirk_s3_64M(struct pci_dev *dev)
 {
 	struct resource *r = &dev->resource[0];
 
-	if ((r->start & 0x3ffffff) || r->end != r->start + 0x3ffffff) {
+	if (!IS_ALIGNED(r->start, SZ_64M) || resource_size(r) != SZ_64M) {
 		r->flags |= IORESOURCE_UNSET;
 		resource_set_range(r, 0, SZ_64M);
 	}
-- 
2.39.2

Re: [PATCH 1/3] resource: Add resource set range and size helpers

[Philipp Stanner]

On Wed, 2024-06-12 at 11:56 +0300, Ilpo Järvinen wrote:
> Setting the end address for a resource with a given size lacks a
> helper
> and is therefore open coded unlike the getter side which has a helper
> for resource size calculation. 

"open coded"? 

How about "coded manually unlike [...]"

> Also, almost all callsites that
> calculate end address for a resource also set the start address right

"an end address" or "the end address"?

> before it like this:
> 
>         res->start = start_addr;
>         res->end = res->start + size - 1;
> 
> Thus, add resource_set_range(res, start_addr, size) that sets the
> start
> address and calculates the end address to simplify this often
> repeated
> fragment. In addition, introduce resource_set_size() for the cases
> where setting the start address of the resource is not necessary but
> note resource_set_range() is preferred.

"note"? I don't fully get that sentence.


Looks like a cool little improvement otherwise :)


P.

> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index db7fe25f3370..2a1d33ad151c 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource
> *root, resource_size_t start);
>  int adjust_resource(struct resource *res, resource_size_t start,
>                     resource_size_t size);
>  resource_size_t resource_alignment(struct resource *res);
> +
> +/**
> + * resource_set_size - Calculates resource end address from size and
> start address
> + * @res: The resource descriptor
> + * @size: The size of the resource
> + *
> + * Calculates the end address for @res based on @size.
> + *
> + * Note: The start address of @res must be set when calling this
> function.
> + * Use resource_set_range() if setting both the start address and
> @size.
> + */
> +static inline void resource_set_size(struct resource *res,
> resource_size_t size)
> +{
> +       res->end = res->start + size - 1;
> +}
> +
> +/**
> + * resource_set_range - Sets resource start and end addresses
> + * @res: The resource descriptor
> + * @start: The start address for the resource
> + * @size: The size of the resource
> + *
> + * Sets @res start address and calculates the end address based on
> @size.
> + */
> +static inline void resource_set_range(struct resource *res,
> +                                     resource_size_t start,
> +                                     resource_size_t size)
> +{
> +       res->start = start;
> +       resource_set_size(res, size);
> +}
> +
>  static inline resource_size_t resource_size(const struct resource
> *res)
>  {
>         return res->end - res->start + 1;

Re: [PATCH 1/3] resource: Add resource set range and size helpers

[Jonathan Cameron]

On Wed, 12 Jun 2024 11:56:27 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Setting the end address for a resource with a given size lacks a helper
> and is therefore open coded unlike the getter side which has a helper
> for resource size calculation. Also, almost all callsites that
> calculate end address for a resource also set the start address right
> before it like this:
> 
> 	res->start = start_addr;
> 	res->end = res->start + size - 1;
> 
> Thus, add resource_set_range(res, start_addr, size) that sets the start
> address and calculates the end address to simplify this often repeated
> fragment. In addition, introduce resource_set_size() for the cases
> where setting the start address of the resource is not necessary but
> note resource_set_range() is preferred.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

We have a bunch of cases of this in CXL. Adding this helper seems like
a good idea to me.

I'm not sure the odd semantics of resource_set_size() are a good idea.
Maybe it could by naming hint that it's relying internally on
size already being set.

resource_update_size() for instance might make people think or perhaps
that's just more obscure.  Meh, I've argued myself around to there
not being a better name.

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


> ---
>  include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index db7fe25f3370..2a1d33ad151c 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start);
>  int adjust_resource(struct resource *res, resource_size_t start,
>  		    resource_size_t size);
>  resource_size_t resource_alignment(struct resource *res);
> +
> +/**
> + * resource_set_size - Calculates resource end address from size and start address
> + * @res: The resource descriptor
> + * @size: The size of the resource
> + *
> + * Calculates the end address for @res based on @size.
> + *
> + * Note: The start address of @res must be set when calling this function.
> + * Use resource_set_range() if setting both the start address and @size.
> + */
> +static inline void resource_set_size(struct resource *res, resource_size_t size)
> +{
> +	res->end = res->start + size - 1;
> +}
> +
> +/**
> + * resource_set_range - Sets resource start and end addresses
> + * @res: The resource descriptor
> + * @start: The start address for the resource
> + * @size: The size of the resource
> + *
> + * Sets @res start address and calculates the end address based on @size.
> + */
> +static inline void resource_set_range(struct resource *res,
> +				      resource_size_t start,
> +				      resource_size_t size)
> +{
> +	res->start = start;
> +	resource_set_size(res, size);
> +}
> +
>  static inline resource_size_t resource_size(const struct resource *res)
>  {
>  	return res->end - res->start + 1;

Re: [PATCH 2/3] PCI: Use resource_set_{range,size}() helpers

[Jonathan Cameron]

On Wed, 12 Jun 2024 11:56:28 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Convert open-coded resource size calculations to use
> resource_set_{range,size}() helpers.
> 
> While at it, use SZ_* for size parameter where appropriate which makes
> the intent of code more obvious.
> 
> Also, cast sizes to resource_size_t, not u64.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

LGTM - one comment inline.

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



> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 909e6a7c3cc3..004405edf290 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c

> @@ -1948,15 +1943,15 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,

>  
>  		res = &dev->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
>  		align = pci_resource_alignment(dev, res);
> -		mmio_pref.end = align ? mmio_pref.start +
> -					ALIGN_DOWN(mmio_pref_per_b, align) - 1
> -				      : mmio_pref.start + mmio_pref_per_b - 1;
> +		resource_set_size(&mmio_pref,
> +				  align ? ALIGN_DOWN(mmio_pref_per_b, align)
> +					: mmio_pref_per_b);

I wonder.  Maybe it's worth defining an ALIGN_DOWN_IF_NON_ZERO() as locally
at least this pattern is annoying common and we can't just change
pci_resource_alignment() to return 1 in those cases because we want a
clean way to check it's not set in a lot of places.

Bikeshedding that name might take longer than it's worth though.




>  		mmio_pref.start -= resource_size(res);
>  
>  		pci_bus_distribute_available_resources(b, add_list, io, mmio,

Re: [PATCH 3/3] PCI: Use align and resource helpers, and SZ_* in quirk_s3_64M()

[Jonathan Cameron]

On Wed, 12 Jun 2024 11:56:29 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Use IS_ALIGNED(), resource_size(), and SZ_* defines in quirk_s3_64M().
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

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

Re: [PATCH 1/3] resource: Add resource set range and size helpers

[Ilpo Järvinen]

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

On Thu, 13 Jun 2024, Jonathan Cameron wrote:

> On Wed, 12 Jun 2024 11:56:27 +0300
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > Setting the end address for a resource with a given size lacks a helper
> > and is therefore open coded unlike the getter side which has a helper
> > for resource size calculation. Also, almost all callsites that
> > calculate end address for a resource also set the start address right
> > before it like this:
> > 
> > 	res->start = start_addr;
> > 	res->end = res->start + size - 1;
> > 
> > Thus, add resource_set_range(res, start_addr, size) that sets the start
> > address and calculates the end address to simplify this often repeated
> > fragment. In addition, introduce resource_set_size() for the cases
> > where setting the start address of the resource is not necessary but
> > note resource_set_range() is preferred.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> We have a bunch of cases of this in CXL. Adding this helper seems like
> a good idea to me.

Sadly this won't help struct range cases which feature the same math.

> I'm not sure the odd semantics of resource_set_size() are a good idea.
> Maybe it could by naming hint that it's relying internally on
> size already being set.
> 
> resource_update_size() for instance might make people think or perhaps
> that's just more obscure.  Meh, I've argued myself around to there
> not being a better name.

Yeah, I tried to figure out solution to this very challenge too, but alas,
couldn't really find any good solution to it.

__ prefix would have kind of conveyed the meaning that you better know 
what you're doing but as some people oppose __ too, I didn't want to stir 
that pot. So it is what it is.

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

FYI, I dropped the extra ` from that (no need to reply because of it).

Thanks for the reviews.

-- 
 i.


> > ---
> >  include/linux/ioport.h | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> > index db7fe25f3370..2a1d33ad151c 100644
> > --- a/include/linux/ioport.h
> > +++ b/include/linux/ioport.h
> > @@ -216,6 +216,38 @@ struct resource *lookup_resource(struct resource *root, resource_size_t start);
> >  int adjust_resource(struct resource *res, resource_size_t start,
> >  		    resource_size_t size);
> >  resource_size_t resource_alignment(struct resource *res);
> > +
> > +/**
> > + * resource_set_size - Calculates resource end address from size and start address
> > + * @res: The resource descriptor
> > + * @size: The size of the resource
> > + *
> > + * Calculates the end address for @res based on @size.
> > + *
> > + * Note: The start address of @res must be set when calling this function.
> > + * Use resource_set_range() if setting both the start address and @size.
> > + */
> > +static inline void resource_set_size(struct resource *res, resource_size_t size)
> > +{
> > +	res->end = res->start + size - 1;
> > +}
> > +
> > +/**
> > + * resource_set_range - Sets resource start and end addresses
> > + * @res: The resource descriptor
> > + * @start: The start address for the resource
> > + * @size: The size of the resource
> > + *
> > + * Sets @res start address and calculates the end address based on @size.
> > + */
> > +static inline void resource_set_range(struct resource *res,
> > +				      resource_size_t start,
> > +				      resource_size_t size)
> > +{
> > +	res->start = start;
> > +	resource_set_size(res, size);
> > +}
> > +
> >  static inline resource_size_t resource_size(const struct resource *res)
> >  {
> >  	return res->end - res->start + 1;
>