Re: [PATCH v5 2/7] PCI/ATS: Initialize PRI in pci_ats_init()

[Bjorn Helgaas]

On Thu, Aug 15, 2019 at 10:30:03AM -0700, Kuppuswamy Sathyanarayanan wrote:
> On Wed, Aug 14, 2019 at 11:46:57PM -0500, Bjorn Helgaas wrote:
> > On Thu, Aug 01, 2019 at 05:05:59PM -0700, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > 
> > > Currently, PRI Capability checks are repeated across all PRI API's.
> > > Instead, cache the capability check result in pci_pri_init() and use it
> > > in other PRI API's. Also, since PRI is a shared resource between PF/VF,
> > > initialize default values for common PRI features in pci_pri_init().
> > > 
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/pci/ats.c       | 80 ++++++++++++++++++++++++++++-------------
> > >  include/linux/pci-ats.h |  5 +++
> > >  include/linux/pci.h     |  1 +
> > >  3 files changed, 61 insertions(+), 25 deletions(-)
> > > 
> > 
> > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > index cdd936d10f68..280be911f190 100644
> > > --- a/drivers/pci/ats.c
> > > +++ b/drivers/pci/ats.c
> > 
> > > @@ -28,6 +28,8 @@ void pci_ats_init(struct pci_dev *dev)
> > >  		return;
> > >  
> > >  	dev->ats_cap = pos;
> > > +
> > > +	pci_pri_init(dev);
> > >  }
> > >  
> > >  /**
> > > @@ -170,36 +172,72 @@ int pci_ats_page_aligned(struct pci_dev *pdev)
> > >  EXPORT_SYMBOL_GPL(pci_ats_page_aligned);
> > >  
> > >  #ifdef CONFIG_PCI_PRI
> > > +
> > > +void pci_pri_init(struct pci_dev *pdev)
> > > +{
> > > ...
> > > +}
> > 
> > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> > > index 1a0bdaee2f32..33653d4ca94f 100644
> > > --- a/include/linux/pci-ats.h
> > > +++ b/include/linux/pci-ats.h
> > > @@ -6,6 +6,7 @@
> > >  
> > >  #ifdef CONFIG_PCI_PRI
> > >  
> > > +void pci_pri_init(struct pci_dev *pdev);
> > 
> > pci_pri_init() is implemented and called in drivers/pci/ats.c.  Unless
> > there's a need to call this from outside ats.c, it should be static
> > and should not be declared here.
> > 
> > If you can make it static, please also reorder the code so you don't
> > need a forward declaration in ats.c.

> Initially I did implement it as static function in drivers/pci/ats.c
> and protected the calling of pci_pri_init() with #ifdef CONFIG_PCI_PRI.
> But Keith did not like the implementation using #ifdefs and asked me to
> define empty functions. That's the reason for moving it to header file.

Defining empty functions doesn't mean it has to be in a header file.
It's only needed inside ats.c, so the whole thing should be static
there.  You can easily #ifdef the implementation, e.g., do the
following in ats.c:

  static void pci_pri_init(struct pci_dev *pdev)
  {
  #ifdef CONFIG_PCI_PRI
    ...
  #endif
  }