Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: sathyanarayanan kuppuswamy  <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	ashok.raj@intel.com, keith.busch@intel.com
Subject: Re: [PATCH v5 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues
Date: Mon, 12 Aug 2019 22:51:48 -0500
Message-ID: <20190813035148.GI7302@google.com> (raw)
In-Reply-To: <09a2faf0-a26f-6374-130a-3b33b1b712d5@linux.intel.com>

On Mon, Aug 12, 2019 at 01:20:55PM -0700, sathyanarayanan kuppuswamy wrote:
> On 8/12/19 1:04 PM, Bjorn Helgaas wrote:
> > On Thu, Aug 01, 2019 at 05:05:58PM -0700, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > Since pci_prg_resp_pasid_required() function has dependency on both
> > > PASID and PRI, define it only if both CONFIG_PCI_PRI and
> > > CONFIG_PCI_PASID config options are enabled.

> > I don't really like this.  It makes the #ifdefs more complicated and I
> > don't think it really buys us anything.  Will anything break if we
> > just drop this patch?

> Yes, this function uses "pri_lock" mutex which is only defined if
> CONFIG_PCI_PRI is enabled. So not protecting this function within
> CONFIG_PCI_PRI will lead to compilation issues.

Ah, OK.  That helps a lot.  "pri_lock" doesn't exist at this point in
the series, so the patch makes no sense without knowing that.

I'm still not convinced this is the right thing because I'm not sure
the lock is necessary.  I'll respond to the patch that adds the lock.

> > > Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
> > > interface.")
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > ---
> > >   drivers/pci/ats.c       | 10 ++++++----
> > >   include/linux/pci-ats.h | 12 +++++++++---
> > >   2 files changed, 15 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> > > index e18499243f84..cdd936d10f68 100644
> > > --- a/drivers/pci/ats.c
> > > +++ b/drivers/pci/ats.c
> > > @@ -395,6 +395,8 @@ int pci_pasid_features(struct pci_dev *pdev)
> > >   }
> > >   EXPORT_SYMBOL_GPL(pci_pasid_features);
> > > +#ifdef CONFIG_PCI_PRI
> > > +
> > >   /**
> > >    * pci_prg_resp_pasid_required - Return PRG Response PASID Required bit
> > >    *				 status.
> > > @@ -402,10 +404,8 @@ EXPORT_SYMBOL_GPL(pci_pasid_features);
> > >    *
> > >    * Returns 1 if PASID is required in PRG Response Message, 0 otherwise.
> > >    *
> > > - * Even though the PRG response PASID status is read from PRI Status
> > > - * Register, since this API will mainly be used by PASID users, this
> > > - * function is defined within #ifdef CONFIG_PCI_PASID instead of
> > > - * CONFIG_PCI_PRI.
> > > + * Since this API has dependency on both PRI and PASID, protect it
> > > + * with both CONFIG_PCI_PRI and CONFIG_PCI_PASID.
> > >    */
> > >   int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > >   {
> > > @@ -425,6 +425,8 @@ int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > >   }
> > >   EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
> > > +#endif
> > > +
> > >   #define PASID_NUMBER_SHIFT	8
> > >   #define PASID_NUMBER_MASK	(0x1f << PASID_NUMBER_SHIFT)
> > >   /**
> > > diff --git a/include/linux/pci-ats.h b/include/linux/pci-ats.h
> > > index 1ebb88e7c184..1a0bdaee2f32 100644
> > > --- a/include/linux/pci-ats.h
> > > +++ b/include/linux/pci-ats.h
> > > @@ -40,7 +40,6 @@ void pci_disable_pasid(struct pci_dev *pdev);
> > >   void pci_restore_pasid_state(struct pci_dev *pdev);
> > >   int pci_pasid_features(struct pci_dev *pdev);
> > >   int pci_max_pasids(struct pci_dev *pdev);
> > > -int pci_prg_resp_pasid_required(struct pci_dev *pdev);
> > >   #else  /* CONFIG_PCI_PASID */
> > > @@ -67,11 +66,18 @@ static inline int pci_max_pasids(struct pci_dev *pdev)
> > >   	return -EINVAL;
> > >   }
> > > +#endif /* CONFIG_PCI_PASID */
> > > +
> > > +#if defined(CONFIG_PCI_PRI) && defined(CONFIG_PCI_PASID)
> > > +
> > > +int pci_prg_resp_pasid_required(struct pci_dev *pdev);
> > > +
> > > +#else /* CONFIG_PCI_PASID && CONFIG_PCI_PRI */
> > > +
> > >   static inline int pci_prg_resp_pasid_required(struct pci_dev *pdev)
> > >   {
> > >   	return 0;
> > >   }
> > > -#endif /* CONFIG_PCI_PASID */
> > > -
> > > +#endif
> > >   #endif /* LINUX_PCI_ATS_H*/
> > > -- 
> > > 2.21.0
> > > 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux kernel developer
> 

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-02  0:05 [PATCH v5 0/7] Fix PF/VF dependency issue sathyanarayanan.kuppuswamy
2019-08-02  0:05 ` [PATCH v5 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues sathyanarayanan.kuppuswamy
2019-08-12 20:04   ` Bjorn Helgaas
2019-08-12 20:20     ` sathyanarayanan kuppuswamy
2019-08-13  3:51       ` Bjorn Helgaas [this message]
2019-08-16 18:06         ` Kuppuswamy Sathyanarayanan
2019-08-02  0:05 ` [PATCH v5 2/7] PCI/ATS: Initialize PRI in pci_ats_init() sathyanarayanan.kuppuswamy
2019-08-12 20:04   ` Bjorn Helgaas
2019-08-12 21:35     ` sathyanarayanan kuppuswamy
2019-08-13  4:10       ` Bjorn Helgaas
2019-08-15  4:46   ` Bjorn Helgaas
2019-08-15 17:30     ` Kuppuswamy Sathyanarayanan
2019-08-16 17:31       ` Bjorn Helgaas
2019-08-02  0:06 ` [PATCH v5 3/7] PCI/ATS: Initialize PASID " sathyanarayanan.kuppuswamy
2019-08-12 20:04   ` Bjorn Helgaas
2019-08-15  4:48   ` Bjorn Helgaas
2019-08-15  4:56   ` Bjorn Helgaas
2019-08-15 17:31     ` Kuppuswamy Sathyanarayanan
2019-08-02  0:06 ` [PATCH v5 4/7] PCI/ATS: Add PRI support for PCIe VF devices sathyanarayanan.kuppuswamy
2019-08-12 20:04   ` Bjorn Helgaas
2019-08-12 21:40     ` sathyanarayanan kuppuswamy
2019-08-13  4:16   ` Bjorn Helgaas
2019-08-15 22:20   ` Bjorn Helgaas
2019-08-15 22:39     ` Kuppuswamy Sathyanarayanan
2019-08-19 14:15       ` Bjorn Helgaas
2019-08-19 22:53         ` Kuppuswamy Sathyanarayanan
2019-08-19 23:19           ` Bjorn Helgaas
2019-08-28 18:21             ` Kuppuswamy Sathyanarayanan
2019-08-28 18:57               ` Bjorn Helgaas
2019-08-02  0:06 ` [PATCH v5 5/7] PCI/ATS: Add PASID " sathyanarayanan.kuppuswamy
2019-08-12 20:05   ` Bjorn Helgaas
2019-08-13 22:19     ` Kuppuswamy Sathyanarayanan
2019-08-15  5:04       ` Bjorn Helgaas
2019-08-16  1:21         ` Kuppuswamy Sathyanarayanan
2019-08-02  0:06 ` [PATCH v5 6/7] PCI/ATS: Disable PF/VF ATS service independently sathyanarayanan.kuppuswamy
2019-08-02  0:06 ` [PATCH v5 7/7] PCI: Skip Enhanced Allocation (EA) initialization for VF device sathyanarayanan.kuppuswamy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190813035148.GI7302@google.com \
    --to=helgaas@kernel.org \
    --cc=ashok.raj@intel.com \
    --cc=keith.busch@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox