Re: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 10/3/19 2:01 PM, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2019 at 01:37:26PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> On Thu, Oct 03, 2019 at 02:04:13PM -0500, Bjorn Helgaas wrote:
>>> On Thu, Oct 03, 2019 at 10:20:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
>>>> Hi Bjorn,
>>>>
>>>> Thanks for looking into this patch set.
>>>>
>>>> On 9/5/19 12:18 PM, Bjorn Helgaas wrote:
>>>>> On Wed, Aug 28, 2019 at 03:14:01PM -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.
>>>>>>
>>>>>> Fixes: e5567f5f6762 ("PCI/ATS: Add pci_prg_resp_pasid_required()
>>>>>> interface.")
>>>>> [Don't split tags, including "Fixes:" across lines]
>>>>>
>>>>> This definitely doesn't fix e5567f5f6762.  That commit added
>>>>> pci_prg_resp_pasid_required(), but with no dependency on
>>>>> CONFIG_PCI_PRI or CONFIG_PCI_PASID.
>>>>>
>>>>> This patch is only required when a subsequent patch is applied.  It
>>>>> should be squashed into the commit that requires it so it's obvious
>>>>> why it's needed.
>>>>>
>>>>> I've been poking at this series, and I'll post a v8 soon with this and
>>>>> other fixes.
>>>> In your v8 submission you did not merge this patch. You did not use
>>>> pri_cap or pasid_cap cached values. Instead you have re-read the
>>>> value from register. Is this intentional?
>>>>
>>>> Since this function will be called for every VF device we might loose some
>>>> performance benefit.
>>> This particular patch doesn't do any caching.  IIRC it fiddles with
>>> ifdefs to solve a problem that would be introduced by a future patch.
>>> I don't remember the exact details, but I think the series I merged
>>> doesn't have that problem.  If it does, let me know the details and we
>>> can fix it.
>> This patch by itself does not do any caching. But your caching patch
>> missed modifying this function to use cached values. Please check the
>> current implementation of this function. It still reads
>> PCI_EXT_CAP_ID_PRI register instead of using cached value. Please let
>> me know your comments.
>>
>> int pci_prg_resp_pasid_required(struct pci_dev *pdev)
>> {
>>      u16 status;
>>      int pri;
>>
>>      if (pdev->is_virtfn)
>>          pdev = pci_physfn(pdev);
>>
>>      pri = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI);
>>      if (!pri)
>>          return 0;
>>
>>      pci_read_config_word(pdev, pri + PCI_PRI_STATUS, &status);
>>
>>      if (status & PCI_PRI_STATUS_PASID)
>>          return 1;
>>
>>      return 0;
>> }
>> EXPORT_SYMBOL_GPL(pci_prg_resp_pasid_required);
>>
>> If caching is applied to this function then we need this #ifdef
>> dependency correction patch.
> IIRC this #ifdef patch wasn't connected to the actual *need* for the
> #ifdef, so it was very difficult to review.  I thought this function
> would be infrequently used and it wasn't worth trying to sort out the
> #ifdef muddle to do the caching.  But it does seem sort of pointless
> to chase the capability list again here, so maybe it *is* worth
> optimizing.
>
> The PRG Response PASID Required bit is read-only, so I wonder if it
> would be simpler if we just read PCI_PRI_STATUS once and save the bit
> in the struct pci_dev?  We could do that in pci_enable_pri(), or if we
> might need the value before that's called, we could add a
> pci_pri_init() and do it there.

Yes, caching PASID Required bit in pci_pri_init() function would provide 
performance
benefits. But another thing to consider is, since this bit is same for 
both PF/VF, is it worth to
add this bit it to struct pci_dev?or struct pci_sriov is the more 
appropriate place?

>
>>> I did include the caching patches for both PRI and PASID capabilities,
>>> but they're only performance optimizations so I moved them to the end
>>> so the functional fixes would be smaller and earlier in the series.
>>>
>>>>>> 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
>>>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux kernel developer

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer