lkml.org 
[lkml]   [2019]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v7 1/7] PCI/ATS: Fix pci_prg_resp_pasid_required() dependency issues
From
Date

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

\
 
 \ /
  Last update: 2019-10-03 23:14    [W:1.286 / U:0.220 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site