diff mbox series

[1/5] PCI: let 'pcibios_root_bridge_prepare()' access to 'bridge->windows'

Message ID 20211115070809.15529-2-sergio.paracuellos@gmail.com (mailing list archive)
State Not Applicable
Headers show
Series PCI: mt7621: remove specific MIPS code from driver | expand

Commit Message

Sergio Paracuellos Nov. 15, 2021, 7:08 a.m. UTC
When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
already available. However this windows are being moved temporarily from
there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
move this windows movement after call this function. This is interesting for
MIPS ralink mt7621 platform to be able to properly set I/O coherence units
with this information and avoid custom MIPs code in generic PCIe controller
drivers.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/pci/probe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Nov. 19, 2021, 11:20 p.m. UTC | #1
[+cc Thierry]

In subject,

  PCI: Let pcibios_root_bridge_prepare() access bridge->windows

On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> already available. However this windows are being moved temporarily from
> there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> move this windows movement after call this function. This is interesting for
> MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> with this information and avoid custom MIPs code in generic PCIe controller
> drivers.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/pci/probe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 087d3658f75c..372a70efccc6 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  
>  	bridge->bus = bus;
>  
> -	/* Temporarily move resources off the list */
> -	list_splice_init(&bridge->windows, &resources);

Arnd added this with 37d6a0a6f470 ("PCI: Add
pci_register_host_bridge() interface") [1].

I can't remember why this was done, but we did go to some trouble to
move things around, so there must have been a good reason.

Arnd or Thierry, do you remember?

>  	bus->sysdata = bridge->sysdata;
>  	bus->ops = bridge->ops;
>  	bus->number = bus->busn_res.start = bridge->busnr;
> @@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
>  	if (err)
>  		goto free;
>  
> +	/* Temporarily move resources off the list */
> +	list_splice_init(&bridge->windows, &resources);
>  	err = device_add(&bridge->dev);
>  	if (err) {
>  		put_device(&bridge->dev);
> -- 
> 2.33.0
> 

[1] https://git.kernel.org/linus/37d6a0a6f470
Bjorn Helgaas Dec. 1, 2021, 8:24 p.m. UTC | #2
On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> [+cc Thierry]
> 
> In subject,
> 
>   PCI: Let pcibios_root_bridge_prepare() access bridge->windows
> 
> On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> > already available. However this windows are being moved temporarily from
> > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> > move this windows movement after call this function. This is interesting for
> > MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> > with this information and avoid custom MIPs code in generic PCIe controller
> > drivers.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > ---
> >  drivers/pci/probe.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 087d3658f75c..372a70efccc6 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> >  
> >  	bridge->bus = bus;
> >  
> > -	/* Temporarily move resources off the list */
> > -	list_splice_init(&bridge->windows, &resources);
> 
> Arnd added this with 37d6a0a6f470 ("PCI: Add
> pci_register_host_bridge() interface") [1].
> 
> I can't remember why this was done, but we did go to some trouble to
> move things around, so there must have been a good reason.
> 
> Arnd or Thierry, do you remember?

Nobody seems to remember, so I think we should go ahead and make this
change after the usual due diligence (audit the code between the old
site and the new site to look for any uses of bridge->windows).

I think this would be material for v5.17.

> >  	bus->sysdata = bridge->sysdata;
> >  	bus->ops = bridge->ops;
> >  	bus->number = bus->busn_res.start = bridge->busnr;
> > @@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> >  	if (err)
> >  		goto free;
> >  
> > +	/* Temporarily move resources off the list */
> > +	list_splice_init(&bridge->windows, &resources);
> >  	err = device_add(&bridge->dev);
> >  	if (err) {
> >  		put_device(&bridge->dev);
> > -- 
> > 2.33.0
> > 
> 
> [1] https://git.kernel.org/linus/37d6a0a6f470
Bjorn Helgaas Dec. 1, 2021, 8:27 p.m. UTC | #3
On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> already available. However this windows are being moved temporarily from
> there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> move this windows movement after call this function. This is interesting for
> MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> with this information and avoid custom MIPs code in generic PCIe controller
> drivers.

Oops, forgot to mention:

s/this windows/these windows/
s/MIPs/MIPS/

You can drop the single quote around function names, too; the "()" is
enough of a hint.

And s/PCI: let/PCI: Let/ in the subject.
Arnd Bergmann Dec. 1, 2021, 8:50 p.m. UTC | #4
On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> >
> > Arnd added this with 37d6a0a6f470 ("PCI: Add
> > pci_register_host_bridge() interface") [1].
> >
> > I can't remember why this was done, but we did go to some trouble to
> > move things around, so there must have been a good reason.
> >
> > Arnd or Thierry, do you remember?
>
> Nobody seems to remember, so I think we should go ahead and make this
> change after the usual due diligence (audit the code between the old
> site and the new site to look for any uses of bridge->windows).
>
> I think this would be material for v5.17.

Sorry I forgot to reply to your earlier mail. I think this is fine, as far as I
remember, the only reason the bridge windows are moved from the list
and added back one at a time was to preserve the exact orderorks.

We could probably even skip that step entirely and iterate throughing
that was there originally, to keep the behavior after a series of reworks.

We could probably even skip that step entirely and iterate through
bridge->windows instead of the local list to simplify this.

For Sergio's patch:

Acked-by: Arnd Bergmann <arnd@arndb.de>
Sergio Paracuellos Dec. 1, 2021, 8:56 p.m. UTC | #5
On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> > [+cc Thierry]
> >
> > In subject,
> >
> >   PCI: Let pcibios_root_bridge_prepare() access bridge->windows
> >
> > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> > > already available. However this windows are being moved temporarily from
> > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> > > move this windows movement after call this function. This is interesting for
> > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> > > with this information and avoid custom MIPs code in generic PCIe controller
> > > drivers.
> > >
> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > ---
> > >  drivers/pci/probe.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 087d3658f75c..372a70efccc6 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > >
> > >     bridge->bus = bus;
> > >
> > > -   /* Temporarily move resources off the list */
> > > -   list_splice_init(&bridge->windows, &resources);
> >
> > Arnd added this with 37d6a0a6f470 ("PCI: Add
> > pci_register_host_bridge() interface") [1].
> >
> > I can't remember why this was done, but we did go to some trouble to
> > move things around, so there must have been a good reason.
> >
> > Arnd or Thierry, do you remember?
>
> Nobody seems to remember, so I think we should go ahead and make this
> change after the usual due diligence (audit the code between the old
> site and the new site to look for any uses of bridge->windows).

It seems any user of the pcibios_root_bridge_prepare() does nothing
with 'bridge->windows'. But I don't get the point of passing around a
complete bridge pointer if windows are temporarily removed from there.
That is an incomplete bridge and after parsing  windows from dts are
supposed to be there... What do you mean with 'audit the code between
the old and new site'?

>
> I think this would be material for v5.17.

Do you prefer me to parse dts again inside
pcibios_root_bridge_prepare() for ralink mt7621?. Not real sense since
'windows' should be already there, but it would be a way to get this
patchset added for v5.16. Something like (not tested yet but it should
work):

int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
 {
         resource_size_t mask;
         struct device_node *np;
         struct of_range range;
         struct of_range_parser parser;
         struct resource res;

        np = of_find_compatible_node(NULL, NULL, "mediatek,mt7621-pci");
        if (!np) {
                 pr_err("Cannot find pci node\n");
                 return -ENODEV;
       }

      if (of_range_parser_init(&parser, np)) {
          pr_err("Error parsing resources\n");
          of_node_put(np);
          return -EINVAL;
      }

      for_each_of_range(&parser, &range) {
             switch (range.flags & IORESOURCE_TYPE_BITS) {
             case IORESOURCE_MEM:
                 res.start = range.cpu_addr;
                 res.end = range.cpu_addr + range.size - 1;
                 break;
             }
     }

      if (mips_cps_numiocu(0)) {
            mask = ~(res.end - res.start);
            write_gcr_reg1_base(res.start);
            write_gcr_reg1_mask(mask | CM_GCR_REGn_MASK_CMTGT_IOCU0);
            pr_info("PCI coherence region base: 0x%08llx,
mask/settings: 0x%08llx\n",
                         (unsigned long long)read_gcr_reg1_base(),
                         (unsigned long long)read_gcr_reg1_mask());
     }

     return 0;
}

We can change this for v5.17 with the change in PCI core.

I have just seen Arnd's Acked-by for PATCH 1 while I was writing this,
so I am not sure if we can consider now this patchset as it is with
proposed changes for v5.16.

Thanks in advance for clarification.

Best regards,
    Sergio Paracuellos



>
> > >     bus->sysdata = bridge->sysdata;
> > >     bus->ops = bridge->ops;
> > >     bus->number = bus->busn_res.start = bridge->busnr;
> > > @@ -925,6 +923,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > >     if (err)
> > >             goto free;
> > >
> > > +   /* Temporarily move resources off the list */
> > > +   list_splice_init(&bridge->windows, &resources);
> > >     err = device_add(&bridge->dev);
> > >     if (err) {
> > >             put_device(&bridge->dev);
> > > --
> > > 2.33.0
> > >
> >
> > [1] https://git.kernel.org/linus/37d6a0a6f470
Bjorn Helgaas Dec. 1, 2021, 9:12 p.m. UTC | #6
[+cc Guenter from other thread:
https://lore.kernel.org/r/20211129015909.GA921717@roeck-us.net]

On Wed, Dec 01, 2021 at 09:56:22PM +0100, Sergio Paracuellos wrote:
> On Wed, Dec 1, 2021 at 9:24 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Nov 19, 2021 at 05:20:17PM -0600, Bjorn Helgaas wrote:
> > > [+cc Thierry]
> > >
> > > In subject,
> > >
> > >   PCI: Let pcibios_root_bridge_prepare() access bridge->windows
> > >
> > > On Mon, Nov 15, 2021 at 08:08:05AM +0100, Sergio Paracuellos wrote:
> > > > When function 'pci_register_host_bridge()' is called, 'bridge->windows' are
> > > > already available. However this windows are being moved temporarily from
> > > > there. To let 'pcibios_root_bridge_prepare()' to have access to this windows
> > > > move this windows movement after call this function. This is interesting for
> > > > MIPS ralink mt7621 platform to be able to properly set I/O coherence units
> > > > with this information and avoid custom MIPs code in generic PCIe controller
> > > > drivers.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> > > > ---
> > > >  drivers/pci/probe.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 087d3658f75c..372a70efccc6 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -898,8 +898,6 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
> > > >
> > > >     bridge->bus = bus;
> > > >
> > > > -   /* Temporarily move resources off the list */
> > > > -   list_splice_init(&bridge->windows, &resources);
> > >
> > > Arnd added this with 37d6a0a6f470 ("PCI: Add
> > > pci_register_host_bridge() interface") [1].
> > >
> > > I can't remember why this was done, but we did go to some trouble to
> > > move things around, so there must have been a good reason.
> > >
> > > Arnd or Thierry, do you remember?
> >
> > Nobody seems to remember, so I think we should go ahead and make this
> > change after the usual due diligence (audit the code between the old
> > site and the new site to look for any uses of bridge->windows).
> 
> It seems any user of the pcibios_root_bridge_prepare() does nothing
> with 'bridge->windows'. But I don't get the point of passing around a
> complete bridge pointer if windows are temporarily removed from there.
> That is an incomplete bridge and after parsing  windows from dts are
> supposed to be there... What do you mean with 'audit the code between
> the old and new site'?

I mean "look at all the code that is run between the old site and the
new site to make sure that none of that code depends on
bridge->windows being temporarily emptied."

> > I think this would be material for v5.17.
> 
> Do you prefer me to parse dts again inside
> pcibios_root_bridge_prepare() for ralink mt7621?. Not real sense since
> 'windows' should be already there, but it would be a way to get this
> patchset added for v5.16. Something like (not tested yet but it should
> work):

This is definitely too big for v5.16, regardless of which way you go.
For v5.16, the only thing that's practical is to avoid building as a
module.  It'd be *nice* if it could be built as a module, but it is
not a requirement.

Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 087d3658f75c..372a70efccc6 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -898,8 +898,6 @@  static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 
 	bridge->bus = bus;
 
-	/* Temporarily move resources off the list */
-	list_splice_init(&bridge->windows, &resources);
 	bus->sysdata = bridge->sysdata;
 	bus->ops = bridge->ops;
 	bus->number = bus->busn_res.start = bridge->busnr;
@@ -925,6 +923,8 @@  static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 	if (err)
 		goto free;
 
+	/* Temporarily move resources off the list */
+	list_splice_init(&bridge->windows, &resources);
 	err = device_add(&bridge->dev);
 	if (err) {
 		put_device(&bridge->dev);