| Message ID | 20221103164644.70554-5-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: Add pci_dev_for_each_resource() helper and refactor bus one | expand |
Am Thu, Nov 03, 2022 at 06:46:44PM +0200 schrieb Andy Shevchenko: > The pci_bus_for_each_resource_p() hides the iterator loop since > it may be not used otherwise. With this, we may drop that iterator > variable definition. Thanks for your patch! > diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c > index ad1141fddb4c..9d92d4bb6239 100644 > --- a/drivers/pcmcia/rsrc_nonstatic.c > +++ b/drivers/pcmcia/rsrc_nonstatic.c > @@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int action, unsigned long > static int nonstatic_autoadd_resources(struct pcmcia_socket *s) > { > struct resource *res; > - int i, done = 0; > + int done = 0; > > if (!s->cb_dev || !s->cb_dev->bus) > return -ENODEV; > @@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct pcmcia_socket *s) > */ > if (s->cb_dev->bus->number == 0) > return -EINVAL; > - > - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { > - res = s->cb_dev->bus->resource[i]; > -#else > - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { > #endif > + > + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { > if (!res) > continue; Doesn't this remove the proper iterator for X86? Even if that is the right thing to do, it needs an explict explanation. > > diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c > index 3966a6ceb1ac..b200f2b99a7a 100644 > --- a/drivers/pcmcia/yenta_socket.c > +++ b/drivers/pcmcia/yenta_socket.c > @@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, struct resource *res, > u32 min) > { > struct resource *root; > - int i; > > - pci_bus_for_each_resource(socket->dev->bus, root, i) { > + pci_bus_for_each_resource_p(socket->dev->bus, root) { > if (!root) > continue; > That looks fine! Thanks, Dominik
On Thu, Nov 03, 2022 at 06:03:24PM +0100, Dominik Brodowski wrote: > Am Thu, Nov 03, 2022 at 06:46:44PM +0200 schrieb Andy Shevchenko: ... > > - > > - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { > > - res = s->cb_dev->bus->resource[i]; > > -#else > > - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { > > #endif > > + > > + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { > > if (!res) > > continue; > > Doesn't this remove the proper iterator for X86? Even if that is the right > thing to do, it needs an explict explanation. I dunno what was in 2010, but reading code now I have found no differences in the logic on how resources are being iterated in these two pieces of code. But fine, I will add a line to a commit message about this change. Considering this is done, can you issue your conditional tag so I will incorporate it in v3?
Am Thu, Nov 03, 2022 at 07:12:45PM +0200 schrieb Andy Shevchenko: > On Thu, Nov 03, 2022 at 06:03:24PM +0100, Dominik Brodowski wrote: > > Am Thu, Nov 03, 2022 at 06:46:44PM +0200 schrieb Andy Shevchenko: > > ... > > > > - > > > - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { > > > - res = s->cb_dev->bus->resource[i]; > > > -#else > > > - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { > > > #endif > > > + > > > + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { > > > if (!res) > > > continue; > > > > Doesn't this remove the proper iterator for X86? Even if that is the right > > thing to do, it needs an explict explanation. > > I dunno what was in 2010, but reading code now I have found no differences in > the logic on how resources are being iterated in these two pieces of code. > > But fine, I will add a line to a commit message about this change. > > Considering this is done, can you issue your conditional tag so I will > incorporate it in v3? Certainly, feel free to add Acked-by: Dominik Brodowski <linux@dominikbrodowski.net> Thanks, Dominik
On Thu, Nov 03, 2022 at 06:25:45PM +0100, Dominik Brodowski wrote: > Am Thu, Nov 03, 2022 at 07:12:45PM +0200 schrieb Andy Shevchenko: > > On Thu, Nov 03, 2022 at 06:03:24PM +0100, Dominik Brodowski wrote: ... > > Considering this is done, can you issue your conditional tag so I will > > incorporate it in v3? > > Certainly, feel free to add > > Acked-by: Dominik Brodowski <linux@dominikbrodowski.net> Thank you for the review!
Hello, [...] > > > - > > > - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { > > > - res = s->cb_dev->bus->resource[i]; > > > -#else > > > - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { > > > #endif > > > + > > > + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { > > > if (!res) > > > continue; > > > > Doesn't this remove the proper iterator for X86? Even if that is the right > > thing to do, it needs an explict explanation. > > I dunno what was in 2010, but reading code now I have found no differences in > the logic on how resources are being iterated in these two pieces of code. This code is over a decade old (13 years old to be precise) and there was something odd between Bjorn's and Jesse's patches, as per: 89a74ecccd1f ("PCI: add pci_bus_for_each_resource(), remove direct bus->resource[] refs") cf26e8dc4194 ("pcmcia: do not autoadd root PCI bus resources") > But fine, I will add a line to a commit message about this change. I wouldn't, personally. The change you are proposing is self-explanatory and somewhat in-line with what is there already - unless I am also reading the current implementation wrong. That said, Dominik is the maintainer of PCMCIA driver, so his is the last word, so to speak. :) > Considering this is done, can you issue your conditional tag so I will > incorporate it in v3? No need, really. Again, unless Dominik thinks otherwise. Krzysztof
Am Fri, Nov 04, 2022 at 03:29:44AM +0900 schrieb Krzysztof Wilczyński: > Hello, > > [...] > > > > - > > > > - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { > > > > - res = s->cb_dev->bus->resource[i]; > > > > -#else > > > > - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { > > > > #endif > > > > + > > > > + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { > > > > if (!res) > > > > continue; > > > > > > Doesn't this remove the proper iterator for X86? Even if that is the right > > > thing to do, it needs an explict explanation. > > > > I dunno what was in 2010, but reading code now I have found no differences in > > the logic on how resources are being iterated in these two pieces of code. > > This code is over a decade old (13 years old to be precise) and there was > something odd between Bjorn's and Jesse's patches, as per: > > 89a74ecccd1f ("PCI: add pci_bus_for_each_resource(), remove direct bus->resource[] refs") > cf26e8dc4194 ("pcmcia: do not autoadd root PCI bus resources") > > > But fine, I will add a line to a commit message about this change. > > I wouldn't, personally. The change you are proposing is self-explanatory > and somewhat in-line with what is there already - unless I am also reading > the current implementation wrong. > > That said, Dominik is the maintainer of PCMCIA driver, so his is the last > word, so to speak. :) > > > Considering this is done, can you issue your conditional tag so I will > > incorporate it in v3? > > No need, really. Again, unless Dominik thinks otherwise. Ah, thanks for the correction. Then v2 is perfectly fine. Thanks, Dominik
On Fri, Nov 04, 2022 at 03:29:44AM +0900, Krzysztof Wilczyński wrote: > > > > - > > > > - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { > > > > - res = s->cb_dev->bus->resource[i]; > > > > -#else > > > > - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { > > > > #endif > > > > + > > > > + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { > > > > if (!res) > > > > continue; > > > > > > Doesn't this remove the proper iterator for X86? Even if that is the right > > > thing to do, it needs an explict explanation. > > > > I dunno what was in 2010, but reading code now I have found no differences in > > the logic on how resources are being iterated in these two pieces of code. > > This code is over a decade old (13 years old to be precise) and there was > something odd between Bjorn's and Jesse's patches, as per: > > 89a74ecccd1f ("PCI: add pci_bus_for_each_resource(), remove direct bus->resource[] refs") > cf26e8dc4194 ("pcmcia: do not autoadd root PCI bus resources") Yeah, thanks for pointing out to the other patch from the same 2010 year. It seems the code was completely identical that time, now it uses more sophisticated way of getting bus resources, but it's kept the same for the resources under PCI_BRIDGE_RESOURCE_NUM threshold. > > But fine, I will add a line to a commit message about this change. > > I wouldn't, personally. The change you are proposing is self-explanatory > and somewhat in-line with what is there already - unless I am also reading > the current implementation wrong. But it wouldn't be harmful either. > That said, Dominik is the maintainer of PCMCIA driver, so his is the last > word, so to speak. :) > > > Considering this is done, can you issue your conditional tag so I will > > incorporate it in v3? > > No need, really. Again, unless Dominik thinks otherwise. I think that what is wanted to have to get his tag. Thanks for review, both of you, guys!
On Thu, Nov 03, 2022 at 07:38:07PM +0100, Dominik Brodowski wrote: > Am Fri, Nov 04, 2022 at 03:29:44AM +0900 schrieb Krzysztof Wilczyński: ... > > That said, Dominik is the maintainer of PCMCIA driver, so his is the last > > word, so to speak. :) > > > > > Considering this is done, can you issue your conditional tag so I will > > > incorporate it in v3? > > > > No need, really. Again, unless Dominik thinks otherwise. > > Ah, thanks for the correction. Then v2 is perfectly fine. I'm fine with either, thanks!
diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c index ad1141fddb4c..9d92d4bb6239 100644 --- a/drivers/pcmcia/rsrc_nonstatic.c +++ b/drivers/pcmcia/rsrc_nonstatic.c @@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int action, unsigned long static int nonstatic_autoadd_resources(struct pcmcia_socket *s) { struct resource *res; - int i, done = 0; + int done = 0; if (!s->cb_dev || !s->cb_dev->bus) return -ENODEV; @@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct pcmcia_socket *s) */ if (s->cb_dev->bus->number == 0) return -EINVAL; - - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) { - res = s->cb_dev->bus->resource[i]; -#else - pci_bus_for_each_resource(s->cb_dev->bus, res, i) { #endif + + pci_bus_for_each_resource_p(s->cb_dev->bus, res) { if (!res) continue; diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c index 3966a6ceb1ac..b200f2b99a7a 100644 --- a/drivers/pcmcia/yenta_socket.c +++ b/drivers/pcmcia/yenta_socket.c @@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, struct resource *res, u32 min) { struct resource *root; - int i; - pci_bus_for_each_resource(socket->dev->bus, root, i) { + pci_bus_for_each_resource_p(socket->dev->bus, root) { if (!root) continue;
The pci_bus_for_each_resource_p() hides the iterator loop since it may be not used otherwise. With this, we may drop that iterator variable definition. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pcmcia/rsrc_nonstatic.c | 9 +++------ drivers/pcmcia/yenta_socket.c | 3 +-- 2 files changed, 4 insertions(+), 8 deletions(-)