Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
Hi Andy,
On Thu, Mar 23, 2023 at 4:15 PM Andy Shevchenko
wrote:
> On Thu, Mar 23, 2023 at 10:02:38AM -0500, Bjorn Helgaas wrote:
> > I poked around looking for similar patterns elsewhere with:
> > git grep "#define.*for_each_.*_p("
> > git grep "#define.*for_each_.*_idx("
> >
> > I didn't find any other "_p" iterators and just a few "_idx" ones, so
> > my hope is to follow what little precedent there is, as well as
> > converge on the basic "*_for_each_resource()" iterators and remove the
> > "_idx()" versions over time by doing things like the
> > pci_claim_resource() change.
>
> The p is heavily used in the byte order conversion helpers.
I can't seem to find them. Example?
Or do you mean cpu_to_be32p()? There "p" means pointer,
which is something completely different.
> > What do you think? If it seems like excessive churn, we can do it
> > as-is and still try to reduce the use of the index variable over time.
>
> I think _p has a precedent as well. But I can think about it a bit, maybe
> we can come up with something smarter.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
On Thu, Mar 23, 2023 at 10:02:38AM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 23, 2023 at 04:30:01PM +0200, Andy Shevchenko wrote:
...
> I poked around looking for similar patterns elsewhere with:
>
> git grep "#define.*for_each_.*_p("
> git grep "#define.*for_each_.*_idx("
>
> I didn't find any other "_p" iterators and just a few "_idx" ones, so
> my hope is to follow what little precedent there is, as well as
> converge on the basic "*_for_each_resource()" iterators and remove the
> "_idx()" versions over time by doing things like the
> pci_claim_resource() change.
The p is heavily used in the byte order conversion helpers.
> What do you think? If it seems like excessive churn, we can do it
> as-is and still try to reduce the use of the index variable over time.
I think _p has a precedent as well. But I can think about it a bit, maybe
we can come up with something smarter.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
On Thu, Mar 23, 2023 at 04:30:01PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 22, 2023 at 02:28:04PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 20, 2023 at 03:16:30PM +0200, Andy Shevchenko wrote:
> ...
>
> > > + pci_dev_for_each_resource_p(dev, r) {
> > > /* zap the 2nd function of the winbond chip */
> > > - if (dev->resource[i].flags & IORESOURCE_IO
> > > - && dev->bus->number == 0 && dev->devfn == 0x81)
> > > - dev->resource[i].flags &= ~IORESOURCE_IO;
> > > - if (dev->resource[i].start == 0 && dev->resource[i].end) {
> > > - dev->resource[i].flags = 0;
> > > - dev->resource[i].end = 0;
> > > + if (dev->bus->number == 0 && dev->devfn == 0x81 &&
> > > + r->flags & IORESOURCE_IO)
> >
> > This is a nice literal conversion, but it's kind of lame to test
> > bus->number and devfn *inside* the loop here, since they can't change
> > inside the loop.
>
> Hmm... why are you asking me, even if I may agree on that? It's
> in the original code and out of scope of this series.
Yeah, I don't think it would be *unreasonable* to clean this up at the
same time so the maintainers can look at both at the same time (this
is arch/powerpc/platforms/pseries/pci.c, so Michael, et al), but no
need for you to do anything, certainly. I can post a follow-up patch.
> > but
> > since we're converging on the "(dev, res)" style, I think we should
> > reverse the names so we have something like:
> >
> > pci_dev_for_each_resource(dev, res)
> > pci_dev_for_each_resource_idx(dev, res, i)
>
> Wouldn't it be more churn, including pci_bus_for_each_resource() correction?
Yes, it definitely is a little more churn because we already have
pci_bus_for_each_resource() that would have to be changed.
I poked around looking for similar patterns elsewhere with:
git grep "#define.*for_each_.*_p("
git grep "#define.*for_each_.*_idx("
I didn't find any other "_p" iterators and just a few "_idx" ones, so
my hope is to follow what little precedent there is, as well as
converge on the basic "*_for_each_resource()" iterators and remove the
"_idx()" versions over time by doing things like the
pci_claim_resource() change.
What do you think? If it seems like excessive churn, we can do it
as-is and still try to reduce the use of the index variable over time.
Bjorn
Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
On Wed, Mar 22, 2023 at 02:28:04PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 20, 2023 at 03:16:30PM +0200, Andy Shevchenko wrote:
...
> > + pci_dev_for_each_resource_p(dev, r) {
> > /* zap the 2nd function of the winbond chip */
> > - if (dev->resource[i].flags & IORESOURCE_IO
> > - && dev->bus->number == 0 && dev->devfn == 0x81)
> > - dev->resource[i].flags &= ~IORESOURCE_IO;
> > - if (dev->resource[i].start == 0 && dev->resource[i].end) {
> > - dev->resource[i].flags = 0;
> > - dev->resource[i].end = 0;
> > + if (dev->bus->number == 0 && dev->devfn == 0x81 &&
> > + r->flags & IORESOURCE_IO)
>
> This is a nice literal conversion, but it's kind of lame to test
> bus->number and devfn *inside* the loop here, since they can't change
> inside the loop.
Hmm... why are you asking me, even if I may agree on that? It's
in the original code and out of scope of this series.
> > + r->flags &= ~IORESOURCE_IO;
> > + if (r->start == 0 && r->end) {
> > + r->flags = 0;
> > + r->end = 0;
> > }
> > }
...
> > #define pci_resource_len(dev,bar) \
> > ((pci_resource_end((dev), (bar)) == 0) ? 0 :\
> > \
> > -(pci_resource_end((dev), (bar)) - \
> > - pci_resource_start((dev), (bar)) + 1))
> > +resource_size(pci_resource_n((dev), (bar
>
> I like this change, but it's unrelated to pci_dev_for_each_resource()
> and unmentioned in the commit log.
And as you rightfully noticed this either. I can split it to a separate one.
...
> > +#define __pci_dev_for_each_resource(dev, res, __i, vartype)
> > \
> > + for (vartype __i = 0; \
> > +res = pci_resource_n(dev, __i), __i < PCI_NUM_RESOURCES; \
> > +__i++)
> > +
> > +#define pci_dev_for_each_resource(dev, res, i)
> > \
> > + __pci_dev_for_each_resource(dev, res, i, )
> > +
> > +#define pci_dev_for_each_resource_p(dev, res)
> > \
> > + __pci_dev_for_each_resource(dev, res, __i, unsigned int)
>
> This series converts many cases to drop the iterator variable ("i"),
> which is fantastic.
>
> Several of the remaining places need the iterator variable only to
> call pci_claim_resource(), which could be converted to take a "struct
> resource *" directly without much trouble.
>
> We don't have to do that pci_claim_resource() conversion now,
Exactly, it's definitely should be separate change.
> but
> since we're converging on the "(dev, res)" style, I think we should
> reverse the names so we have something like:
>
> pci_dev_for_each_resource(dev, res)
> pci_dev_for_each_resource_idx(dev, res, i)
Wouldn't it be more churn, including pci_bus_for_each_resource() correction?
...
> Not sure __pci_dev_for_each_resource() is worthwhile since it only
> avoids repeating that single "for" statement, and passing in "vartype"
> (sometimes empty to implicitly avoid the declaration) is a little
> complicated to read. I think it'd be easier to read like this:
No objections here.
> #define pci_dev_for_each_resource(dev, res) \
> for (unsigned int __i = 0; \
> res = pci_resource_n(dev, __i), __i < PCI_NUM_RESOURCES; \
> __i++)
>
> #define pci_dev_for_each_resource_idx(dev, res, idx) \
> for (idx = 0; \
> res = pci_resource_n(dev, idx), idx < PCI_NUM_RESOURCES; \
> idx++)
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
Hi Andy and Mika,
I really like the improvements here. They make the code read much
better.
On Mon, Mar 20, 2023 at 03:16:30PM +0200, Andy Shevchenko wrote:
> From: Mika Westerberg
> ...
> static void fixup_winbond_82c105(struct pci_dev* dev)
> {
> - int i;
> + struct resource *r;
> unsigned int reg;
>
> if (!machine_is(pseries))
> @@ -251,14 +251,14 @@ static void fixup_winbond_82c105(struct pci_dev* dev)
> /* Enable LEGIRQ to use INTC instead of ISA interrupts */
> pci_write_config_dword(dev, 0x40, reg | (1<<11));
>
> - for (i = 0; i < DEVICE_COUNT_RESOURCE; ++i) {
> + pci_dev_for_each_resource_p(dev, r) {
> /* zap the 2nd function of the winbond chip */
> - if (dev->resource[i].flags & IORESOURCE_IO
> - && dev->bus->number == 0 && dev->devfn == 0x81)
> - dev->resource[i].flags &= ~IORESOURCE_IO;
> - if (dev->resource[i].start == 0 && dev->resource[i].end) {
> - dev->resource[i].flags = 0;
> - dev->resource[i].end = 0;
> + if (dev->bus->number == 0 && dev->devfn == 0x81 &&
> + r->flags & IORESOURCE_IO)
This is a nice literal conversion, but it's kind of lame to test
bus->number and devfn *inside* the loop here, since they can't change
inside the loop.
> + r->flags &= ~IORESOURCE_IO;
> + if (r->start == 0 && r->end) {
> + r->flags = 0;
> + r->end = 0;
> }
> }
> #define pci_resource_len(dev,bar) \
> ((pci_resource_end((dev), (bar)) == 0) ? 0 :\
> \
> - (pci_resource_end((dev), (bar)) - \
> - pci_resource_start((dev), (bar)) + 1))
> + resource_size(pci_resource_n((dev), (bar
I like this change, but it's unrelated to pci_dev_for_each_resource()
and unmentioned in the commit log.
> +#define __pci_dev_for_each_resource(dev, res, __i, vartype) \
> + for (vartype __i = 0; \
> + res = pci_resource_n(dev, __i), __i < PCI_NUM_RESOURCES; \
> + __i++)
> +
> +#define pci_dev_for_each_resource(dev, res, i)
> \
> + __pci_dev_for_each_resource(dev, res, i, )
> +
> +#define pci_dev_for_each_resource_p(dev, res)
> \
> + __pci_dev_for_each_resource(dev, res, __i, unsigned int)
This series converts many cases to drop the iterator variable ("i"),
which is fantastic.
Several of the remaining places need the iterator variable only to
call pci_claim_resource(), which could be converted to take a "struct
resource *" directly without much trouble.
We don't have to do that pci_claim_resource() conversion now, but
since we're converging on the "(dev, res)" style, I think we should
reverse the names so we have something like:
pci_dev_for_each_resource(dev, res)
pci_dev_for_each_resource_idx(dev, res, i)
Not sure __pci_dev_for_each_resource() is worthwhile since it only
avoids repeating that single "for" statement, and passing in "vartype"
(sometimes empty to implicitly avoid the declaration) is a little
complicated to read. I think it'd be easier to read like this:
#define pci_dev_for_each_resource(dev, res) \
for (unsigned int __i = 0; \
res = pci_resource_n(dev, __i), __i < PCI_NUM_RESOURCES; \
__i++)
#define pci_dev_for_each_resource_idx(dev, res, idx) \
for (idx = 0; \
res = pci_resource_n(dev, idx), idx < PCI_NUM_RESOURCES; \
idx++)
Bjorn
[PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()
From: Mika Westerberg
Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.
While at it drop doubled empty line before pdev_sort_resources().
No functional changes intended.
Suggested-by: Andy Shevchenko
Signed-off-by: Mika Westerberg
Signed-off-by: Andy Shevchenko
Reviewed-by: Krzysztof Wilczyński
---
.clang-format | 2 ++
arch/alpha/kernel/pci.c | 5 ++--
arch/arm/kernel/bios32.c | 16 ++---
arch/arm/mach-dove/pcie.c | 10
arch/arm/mach-mv78xx0/pcie.c | 10
arch/arm/mach-orion5x/pci.c | 10
arch/mips/pci/ops-bcm63xx.c | 8 +++
arch/mips/pci/pci-legacy.c| 3 +--
arch/powerpc/kernel/pci-common.c | 21
arch/powerpc/platforms/4xx/pci.c | 8 +++
arch/powerpc/platforms/52xx/mpc52xx_pci.c | 5 ++--
arch/powerpc/platforms/pseries/pci.c | 16 ++---
arch/sh/drivers/pci/pcie-sh7786.c | 10
arch/sparc/kernel/leon_pci.c | 5 ++--
arch/sparc/kernel/pci.c | 10
arch/sparc/kernel/pcic.c | 5 ++--
drivers/pci/remove.c | 5 ++--
drivers/pci/setup-bus.c | 27 -
drivers/pci/setup-res.c | 4 +---
drivers/pci/vgaarb.c | 17 -
drivers/pci/xen-pcifront.c| 4 +---
drivers/pnp/quirks.c | 29 ---
include/linux/pci.h | 15 ++--
23 files changed, 111 insertions(+), 134 deletions(-)
diff --git a/.clang-format b/.clang-format
index d988e9fa9b26..266abb843654 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,8 @@ ForEachMacros:
- 'of_property_for_each_string'
- 'of_property_for_each_u32'
- 'pci_bus_for_each_resource'
+ - 'pci_dev_for_each_resource'
+ - 'pci_dev_for_each_resource_p'
- 'pci_doe_for_each_off'
- 'pcl_for_each_chunk'
- 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
list_for_each_entry(dev, >devices, bus_list) {
+ struct resource *r;
int i;
- for (i = 0; i < PCI_NUM_RESOURCES; i++) {
- struct resource *r = >resource[i];
-
+ pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..5254734b23e6 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2,
PCI_DEVICE_ID_WINBOND2_89C940F,
*/
static void pci_fixup_dec21285(struct pci_dev *dev)
{
- int i;
-
if (dev->devfn == 0) {
+ struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
- for (i = 0; i < PCI_NUM_RESOURCES; i++) {
- dev->resource[i].start = 0;
- dev->resource[i].end = 0;
- dev->resource[i].flags = 0;
+ pci_dev_for_each_resource_p(dev, r) {
+ r->start = 0;
+ r->end = 0;
+ r->flags = 0;
}
}
}
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC,
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
static void pci_fixup_ide_bases(struct pci_dev *dev)
{
struct resource *r;
- int i;
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
- for (i = 0; i < PCI_NUM_RESOURCES; i++) {
- r = dev->resource + i;
+ pci_dev_for_each_resource_p(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 754ca381f600..58cecd79a204 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -142,14 +142,14 @@ static struct pci_ops pcie_ops = {
static void rc_pci_fixup(struct pci_dev *dev)
{
if (dev->bus->parent == NULL && dev->devfn == 0) {
- int i;
+ struct resource *r;
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
- for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
-
