Re: [PATCH v7 09/26] PCI: hotplug: Calculate immovable parts of bridge windows

[Sergei Miroshnichenko <s.miroshnichenko@yadro.com>]

On Thu, 2020-01-30 at 15:26 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 29, 2020 at 06:29:20PM +0300, Sergei Miroshnichenko
> wrote:
> > When movable BARs are enabled, and if a bridge contains a device
> > with fixed
> > (IORESOURCE_PCI_FIXED) or immovable BARs, the corresponding windows
> > can't
> 
> What is the difference between fixed (IORESOURCE_PCI_FIXED) and
> immovable BARs?  I'm hesitant to add a new concept ("immovable") with
> a different name but very similar meaning.
> 
> I understand that in the case of bridge windows, you may need to
> track
> only part of the window, as opposed to a BAR where the entire BAR has
> to be either fixed or movable, but I think adding a new term will be
> confusing.
> 

I thought the term "fixed BAR" has some legacy scent, and those marked
with flag IORESOURCE_PCI_FIXED are fixed forever. But a BAR may become
movable after rmmod-ing its driver that didn't support movable BARs.

Still, the IORESOURCE_PCI_FIXED is used just a few times in the kernel,
so the "fixed BAR" term is probably not so well-established, so I don't
mind referring all non-movables as "fixed": both marked with the flag
and not. Will change all of them in v8.

> > be moved too far away from their original positions - they must
> > still
> > contain all the fixed/immovable BARs, like that:
> > 
> > 1) Window position before a bus rescan:
> > 
> >    | <--                    root bridge
> > window                        --> |
> >    |                                                               
> >        |
> >    | | <--     bridge window    -->
> > |                                     |
> >    | | movable BARs | **fixed BAR**
> > |                                     |
> >        ^^^^^^^^^^^^
> > 
> > 2) Possible valid outcome after rescan and move:
> > 
> >    | <--                    root bridge
> > window                        --> |
> >    |                                                               
> >        |
> >    |                | <--     bridge window    -->
> > |                      |
> >    |                | **fixed BAR** | Movable BARs
> > |                      |
> >                                       ^^^^^^^^^^^^
> > 
> > An immovable area of a bridge window is a range that covers all the
> > fixed
> > and immovable BARs of direct children, and all the fixed area of
> > children
> > bridges:
> > 
> >    | <--                    root bridge
> > window                        --> |
> >    |                                                               
> >        |
> >    |  | <--                  bridge window level 1                -
> > -> |   |
> >    |  | ******************** immovable area
> > *******************       |   |
> >    |  |                                                            
> >    |   |
> >    |  | **fixed BAR** | <--      bridge window level 2     --> |
> > BARs |   |
> >    |  |               | *********** immovable area ***********
> > |      |   |
> >    |  |               |                                        |   
> >    |   |
> >    |  |               | **fixed BAR** |  BARs  | **fixed BAR**
> > |      |   |
> >                                          ^^^^
> > 
> > To store these areas, the .immovable_range field has been added to
> > struct
> > pci_bus for every bridge window type: IO, MEM and PREFETCH. It is
> > filled
> > recursively from leaves to the root before a rescan.
> > 
> > Signed-off-by: Sergei Miroshnichenko <s.miroshnichenko@yadro.com>
> > ---
> >  drivers/pci/pci.h   | 14 ++++++++
> >  drivers/pci/probe.c | 88
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h |  6 ++++
> >  3 files changed, 108 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 3b4c982772d3..5f2051c8531c 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -404,6 +404,20 @@ static inline bool
> > pci_dev_is_disconnected(const struct pci_dev *dev)
> >  	return dev->error_state == pci_channel_io_perm_failure;
> >  }
> >  
> > +static inline int pci_get_bridge_resource_idx(struct resource *r)
> > +{
> > +	int idx = 1;
> > +
> > +	if (r->flags & IORESOURCE_IO)
> > +		idx = 0;
> > +	else if (!(r->flags & IORESOURCE_PREFETCH))
> > +		idx = 1;
> > +	else if (r->flags & IORESOURCE_MEM_64)
> > +		idx = 2;
> 
> Random nit: No variables or elses required:
> 
>   if (r->flags & IORESOURCE_IO)
>     return 0;
>   if (!(r->flags & IORESOURCE_PREFETCH))
>     return 1;
>   ...
> 

Thank you!

Best regards,
Serge

> > +	return idx;
> > +}