linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] PCI: Update BAR #/window messages
@ 2021-11-06 11:26 Puranjay Mohan
  2021-11-06 11:26 ` [PATCH 1/2] PCI: Update BAR # and window messages Puranjay Mohan
  2021-11-06 11:26 ` [PATCH 2/2] PCI: Use resource names in PCI log messages Puranjay Mohan
  0 siblings, 2 replies; 6+ messages in thread
From: Puranjay Mohan @ 2021-11-06 11:26 UTC (permalink / raw)
  To: bhelgaas, linux-pci; +Cc: Puranjay Mohan

The log messages in the PCI subsystem print the register offsets of the
BARs that can't be easily understood. At some places it prints the BAR
numbers. These should all be more meaningful, e.g., "BAR 0", "I/O window"
,"Prefetchable MMIO window", etc.

The first patch in this series adds a helper function that returns the
name of the PCI resource when called with index of the resource.
The second patch uses this function and updates the messages in one of
the places.

Many more messages have to be updated to make this a uniform process, I
will be sending subsequent patches that do the same.

Puranjay Mohan (2):
  PCI: Update BAR # and window messages
  PCI: Use resource names in PCI log messages

 drivers/pci/pci.c   | 47 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h   |  2 ++
 drivers/pci/probe.c | 20 ++++++++++---------
 3 files changed, 60 insertions(+), 9 deletions(-)

-- 
2.30.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] PCI: Update BAR # and window messages
  2021-11-06 11:26 [PATCH 0/2] PCI: Update BAR #/window messages Puranjay Mohan
@ 2021-11-06 11:26 ` Puranjay Mohan
  2021-11-06 11:58   ` Lukas Wunner
  2021-11-06 11:26 ` [PATCH 2/2] PCI: Use resource names in PCI log messages Puranjay Mohan
  1 sibling, 1 reply; 6+ messages in thread
From: Puranjay Mohan @ 2021-11-06 11:26 UTC (permalink / raw)
  To: bhelgaas, linux-pci; +Cc: Puranjay Mohan

The PCI logs messages print the register offsets at some places and
BAR numbers at other places. There is no uniformity in this logging
mechanism. It would be better to print names than register offsets.

Add a helper function that aids in printing more meaningful information
about the BAR numbers like "VF BAR", "ROM", "Bridge Window", etc.
This function can be called while printing PCI log messages.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 drivers/pci/pci.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h |  2 ++
 2 files changed, 49 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ce2ab62b64cf..1c2dfb2b9754 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -800,6 +800,53 @@ struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
 }
 EXPORT_SYMBOL(pci_find_resource);
 
+/**
+ * pci_resource_name - Return the name of the PCI resource.
+ * @dev: PCI device to query
+ * @i: index of the resource
+ *
+ * Returns the standard PCI resource(BAR) names according to their index.
+ */
+const char *pci_resource_name(struct pci_dev *dev, int i)
+{
+	if (dev->hdr_type == PCI_HEADER_TYPE_NORMAL) {
+		switch (i) {
+		case 0: return "BAR 0";
+		case 1: return "BAR 1";
+		case 2: return "BAR 2";
+		case 3: return "BAR 3";
+		case 4: return "BAR 4";
+		case 5: return "BAR 5";
+		case PCI_ROM_RESOURCE: return "ROM";
+		#ifdef CONFIG_PCI_IOV
+		case PCI_IOV_RESOURCES + 0: return "VF BAR 0";
+		case PCI_IOV_RESOURCES + 1: return "VF BAR 1";
+		case PCI_IOV_RESOURCES + 2: return "VF BAR 2";
+		case PCI_IOV_RESOURCES + 3: return "VF BAR 3";
+		case PCI_IOV_RESOURCES + 4: return "VF BAR 4";
+		case PCI_IOV_RESOURCES + 5: return "VF BAR 5";
+		#endif
+		}
+	} else if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
+		switch (i) {
+		case 0: return "BAR 0";
+		case 1: return "BAR 1";
+		case PCI_BRIDGE_IO_WINDOW: return "bridge I/O window";
+		case PCI_BRIDGE_MEM_WINDOW: return "bridge mem window";
+		case PCI_BRIDGE_PREF_MEM_WINDOW: return "bridge mem pref window";
+		}
+	} else if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
+		switch (i) {
+		case 0: return "BAR 0";
+		case PCI_CB_BRIDGE_IO_0_WINDOW: return "CardBus bridge I/O 0 window";
+		case PCI_CB_BRIDGE_IO_1_WINDOW: return "CardBus bridge I/O 1 window";
+		case PCI_CB_BRIDGE_MEM_0_WINDOW: return "CardBus bridge mem 0 window";
+		case PCI_CB_BRIDGE_MEM_1_WINDOW: return "CardBus bridge mem 1 window";
+		}
+	}
+	return "unknown";
+}
+
 /**
  * pci_wait_for_pending - wait for @mask bit(s) to clear in status word @pos
  * @dev: the PCI device to operate on
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 1cce56c2aea0..ee0738c7731a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -281,6 +281,8 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
 				struct list_head *fail_head);
 bool pci_bus_clip_resource(struct pci_dev *dev, int idx);
 
+const char *pci_resource_name(struct pci_dev *dev, int i);
+
 void pci_reassigndev_resource_alignment(struct pci_dev *dev);
 void pci_disable_bridge_window(struct pci_dev *dev);
 struct pci_bus *pci_bus_get(struct pci_bus *bus);
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] PCI: Use resource names in PCI log messages
  2021-11-06 11:26 [PATCH 0/2] PCI: Update BAR #/window messages Puranjay Mohan
  2021-11-06 11:26 ` [PATCH 1/2] PCI: Update BAR # and window messages Puranjay Mohan
@ 2021-11-06 11:26 ` Puranjay Mohan
  1 sibling, 0 replies; 6+ messages in thread
From: Puranjay Mohan @ 2021-11-06 11:26 UTC (permalink / raw)
  To: bhelgaas, linux-pci; +Cc: Puranjay Mohan

Use the pci_resource_name() to get the name of the resource and use it
while printing log messages.

Signed-off-by: Puranjay Mohan <puranjay12@gmail.com>
---
 drivers/pci/probe.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index d9fc02a71baa..407854d17efa 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -181,6 +181,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	u64 l64, sz64, mask64;
 	u16 orig_cmd;
 	struct pci_bus_region region, inverted_region;
+	int idx = res - dev->resource;
+	const char *resource_name = pci_resource_name(dev, idx);
 
 	mask = type ? PCI_ROM_ADDRESS_MASK : ~0;
 
@@ -255,8 +257,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 
 	sz64 = pci_size(l64, sz64, mask64);
 	if (!sz64) {
-		pci_info(dev, FW_BUG "reg 0x%x: invalid BAR (can't size)\n",
-			 pos);
+		pci_info(dev, FW_BUG "%s: invalid BAR (can't size)\n",
+			 resource_name);
 		goto fail;
 	}
 
@@ -266,8 +268,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			res->flags |= IORESOURCE_UNSET | IORESOURCE_DISABLED;
 			res->start = 0;
 			res->end = 0;
-			pci_err(dev, "reg 0x%x: can't handle BAR larger than 4GB (size %#010llx)\n",
-				pos, (unsigned long long)sz64);
+			pci_err(dev, "%s: can't handle BAR larger than 4GB (size %#010llx)\n",
+				resource_name, (unsigned long long)sz64);
 			goto out;
 		}
 
@@ -276,8 +278,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 			res->flags |= IORESOURCE_UNSET;
 			res->start = 0;
 			res->end = sz64 - 1;
-			pci_info(dev, "reg 0x%x: can't handle BAR above 4GB (bus address %#010llx)\n",
-				 pos, (unsigned long long)l64);
+			pci_info(dev, "%s: can't handle BAR above 4GB (bus address %#010llx)\n",
+				 resource_name, (unsigned long long)l64);
 			goto out;
 		}
 	}
@@ -303,8 +305,8 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		res->flags |= IORESOURCE_UNSET;
 		res->start = 0;
 		res->end = region.end - region.start;
-		pci_info(dev, "reg 0x%x: initial BAR value %#010llx invalid\n",
-			 pos, (unsigned long long)region.start);
+		pci_info(dev, "%s: initial BAR value %#010llx invalid\n",
+			 resource_name, (unsigned long long)region.start);
 	}
 
 	goto out;
@@ -314,7 +316,7 @@ int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	res->flags = 0;
 out:
 	if (res->flags)
-		pci_info(dev, "reg 0x%x: %pR\n", pos, res);
+		pci_info(dev, "%s: %pR\n", resource_name, res);
 
 	return (res->flags & IORESOURCE_MEM_64) ? 1 : 0;
 }
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] PCI: Update BAR # and window messages
  2021-11-06 11:26 ` [PATCH 1/2] PCI: Update BAR # and window messages Puranjay Mohan
@ 2021-11-06 11:58   ` Lukas Wunner
  2021-11-19 21:43     ` Bjorn Helgaas
  0 siblings, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2021-11-06 11:58 UTC (permalink / raw)
  To: Puranjay Mohan; +Cc: bhelgaas, linux-pci

On Sat, Nov 06, 2021 at 04:56:05PM +0530, Puranjay Mohan wrote:
> +		switch (i) {
> +		case 0: return "BAR 0";
> +		case 1: return "BAR 1";
> +		case 2: return "BAR 2";
> +		case 3: return "BAR 3";
> +		case 4: return "BAR 4";
> +		case 5: return "BAR 5";
> +		case PCI_ROM_RESOURCE: return "ROM";
> +		#ifdef CONFIG_PCI_IOV
> +		case PCI_IOV_RESOURCES + 0: return "VF BAR 0";
> +		case PCI_IOV_RESOURCES + 1: return "VF BAR 1";
> +		case PCI_IOV_RESOURCES + 2: return "VF BAR 2";
> +		case PCI_IOV_RESOURCES + 3: return "VF BAR 3";
> +		case PCI_IOV_RESOURCES + 4: return "VF BAR 4";
> +		case PCI_IOV_RESOURCES + 5: return "VF BAR 5";
> +		#endif
> +		}

Use a static const array of char * instead of a switch/case ladder
to reduce LoC count and improve performance.

See pcie_to_hpx3_type[] or state_conv[] in drivers/pci/pci-acpi.c
for an example.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] PCI: Update BAR # and window messages
  2021-11-06 11:58   ` Lukas Wunner
@ 2021-11-19 21:43     ` Bjorn Helgaas
  2021-11-24  4:28       ` Lukas Wunner
  0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2021-11-19 21:43 UTC (permalink / raw)
  To: Lukas Wunner; +Cc: Puranjay Mohan, bhelgaas, linux-pci

On Sat, Nov 06, 2021 at 12:58:31PM +0100, Lukas Wunner wrote:
> On Sat, Nov 06, 2021 at 04:56:05PM +0530, Puranjay Mohan wrote:
> > +		switch (i) {
> > +		case 0: return "BAR 0";
> > +		case 1: return "BAR 1";
> > +		case 2: return "BAR 2";
> > +		case 3: return "BAR 3";
> > +		case 4: return "BAR 4";
> > +		case 5: return "BAR 5";
> > +		case PCI_ROM_RESOURCE: return "ROM";
> > +		#ifdef CONFIG_PCI_IOV
> > +		case PCI_IOV_RESOURCES + 0: return "VF BAR 0";
> > +		case PCI_IOV_RESOURCES + 1: return "VF BAR 1";
> > +		case PCI_IOV_RESOURCES + 2: return "VF BAR 2";
> > +		case PCI_IOV_RESOURCES + 3: return "VF BAR 3";
> > +		case PCI_IOV_RESOURCES + 4: return "VF BAR 4";
> > +		case PCI_IOV_RESOURCES + 5: return "VF BAR 5";
> > +		#endif
> > +		}
> 
> Use a static const array of char * instead of a switch/case ladder
> to reduce LoC count and improve performance.
> 
> See pcie_to_hpx3_type[] or state_conv[] in drivers/pci/pci-acpi.c
> for an example.

I tried converting this and came up with the below.  Is that the sort
of thing you're thinking?  Gcc *does* generate slightly smaller code
for it, but Puranjay's original source code is smaller and IMO a
little easier to read.

And I just noticed that Puranjay's code nicely returns "unknown" for
BAR 2-5 of bridges, while my code below does not.  Could be fixed, of
course, but would take a little more code.

  const char *pci_resource_name(struct pci_dev *dev, int i)
  {
	  static const char *bar_name[] = {
		  "BAR 0",
		  "BAR 1",
		  "BAR 2",
		  "BAR 3",
		  "BAR 4",
		  "BAR 5",
		  "ROM",
  #ifdef CONFIG_PCI_IOV
		  "VF BAR 0",
		  "VF BAR 1",
		  "VF BAR 2",
		  "VF BAR 3",
		  "VF BAR 4",
		  "VF BAR 5",
  #endif
		  "bridge I/O window",
		  "bridge mem window",
		  "bridge mem pref window",
	  };
	  static const char *cardbus_name[] = {
		  "BAR 0",
		  "unknown",
		  "unknown",
		  "unknown",
		  "unknown",
		  "unknown",
		  "unknown",
  #ifdef CONFIG_PCI_IOV
		  "unknown",
		  "unknown",
		  "unknown",
		  "unknown",
		  "unknown",
		  "unknown",
  #endif
		  "CardBus bridge I/O 0 window",
		  "CardBus bridge I/O 1 window",
		  "CardBus bridge mem 0 window",
		  "CardBus bridge mem 1 window",
	  };

	  if (dev->hdr_type == PCI_HEADER_TYPE_CARDBUS &&
	      i < ARRAY_SIZE(cardbus_name))
		  return cardbus_name[i];
	  else if (i < ARRAY_SIZE(bar_name))
		  return bar_name[i];

	  return "unknown";
  }

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] PCI: Update BAR # and window messages
  2021-11-19 21:43     ` Bjorn Helgaas
@ 2021-11-24  4:28       ` Lukas Wunner
  0 siblings, 0 replies; 6+ messages in thread
From: Lukas Wunner @ 2021-11-24  4:28 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Puranjay Mohan, linux-pci

On Fri, Nov 19, 2021 at 03:43:04PM -0600, Bjorn Helgaas wrote:
> On Sat, Nov 06, 2021 at 12:58:31PM +0100, Lukas Wunner wrote:
> > Use a static const array of char * instead of a switch/case ladder
> > to reduce LoC count and improve performance.
> 
> I tried converting this and came up with the below.  Is that the sort
> of thing you're thinking?  Gcc *does* generate slightly smaller code
> for it, but Puranjay's original source code is smaller and IMO a
> little easier to read.

Yes, that's what I had in mind.  Aside from binary or source code size,
retrieving the name from an array is just a quick direct access, whereas
a switch/case ladder becomes a lot of conditional branches in binary code,
which is slower.

Another option is to use case ranges.  See max3191x_set_config() in
drivers/gpio/gpio-max3191x.c for an example.  gcc is smart enough to
generate an optimized set of conditional greater-than / less-than branches.
That could be used to shorten your cardbus_name[] array.

Thanks,

Lukas

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-11-24  4:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-06 11:26 [PATCH 0/2] PCI: Update BAR #/window messages Puranjay Mohan
2021-11-06 11:26 ` [PATCH 1/2] PCI: Update BAR # and window messages Puranjay Mohan
2021-11-06 11:58   ` Lukas Wunner
2021-11-19 21:43     ` Bjorn Helgaas
2021-11-24  4:28       ` Lukas Wunner
2021-11-06 11:26 ` [PATCH 2/2] PCI: Use resource names in PCI log messages Puranjay Mohan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).