diff mbox series

[v5,1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

Message ID 0d7e0e0d079c438897f4da8cdca4b55994b1233b.1564702313.git.sathyanarayanan.kuppuswamy@linux.intel.com
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Fix PF/VF dependency issue | expand

Commit Message

Kuppuswamy Sathyanarayanan Aug. 2, 2019, 12:05 a.m. UTC 
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.

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(-)

Comments

Bjorn Helgaas Aug. 12, 2019, 8:04 p.m. UTC | #1 
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?

> 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
>
Kuppuswamy Sathyanarayanan Aug. 12, 2019, 8:20 p.m. UTC | #2 
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.
>
>> 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
>>
Bjorn Helgaas Aug. 13, 2019, 3:51 a.m. UTC | #3 
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
>
Kuppuswamy Sathyanarayanan Aug. 16, 2019, 6:06 p.m. UTC | #4 
On Mon, Aug 12, 2019 at 10:51:48PM -0500, Bjorn Helgaas wrote:
> 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.
Its not only pri_lock. This function also uses "pri_cap" which is also
only defined for CONFIG_PCI_PRI. "pri_cap" is added by next patch in the
series which adds caching support for PRI capability check. So this
patch is still required even if we remove use of pri_lock in this
function.
> 
> > > > 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
> >
diff mbox series

Patch

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*/