lkml.org 
[lkml]   [2017]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4] PCI: Workaround wrong flags completions for IDT switch
On 07/03/2017 09:56 PM, Ethan Zhao wrote:
> James,
>
> On Tue, Jul 4, 2017 at 2:17 AM, james puthukattukaran
> <james.puthukattukaran@oracle.com> wrote:
>>
>> Ethan -
>>
>>
>> On 7/2/2017 9:55 PM, Ethan Zhao wrote:
>>>
>>> James,
>>>
>>> On Wed, Jun 28, 2017 at 5:42 AM, James Puthukattukaran
>>> <james.puthukattukaran@oracle.com> wrote:
>>>>
>>>> From: James Puthukattukaran <james.puthukattukaran@oracle.com>
>>>>
>>>> The IDT switch incorrectly flags an ACS source violation on a read config
>>>> request to an end point device on the completion (IDT 89H32H8G3-YC,
>>>> errata #36) even though the PCI Express spec states that completions are
>>>> never affected by ACS source violation (PCI Spec 3.1, Section 6.12.1.1).
>>>>
>>>> The suggested workaround by IDT is to issue a configuration write to the
>>>> downstream device before issuing the first config read. This allows the
>>>> downstream device to capture its bus number, thus avoiding the ACS
>>>> violation on the completion.
>>>>
>>>> The patch does the following -
>>>>
>>>> 1. Disable ACS source violation if enabled
>>>> 2. Wait for config space access to become available by reading vendor id
>>>> 3. Do a config write to the end point (errata workaround)
>>>> 4. Enable ACS source validation (if it was enabled to begin with)
>>>>
>>>> -v2: move workaround to pci_bus_read_dev_vendor_id() from
>>>> pci_bus_check_dev()
>>>> and move enable_acs_sv to drivers/pci/pci.c -- by Yinghai
>>>> -v3: add bus->self check for root bus and virtual bus for sriov vfs.
>>>> -v4: only do workaround for IDT switches
>>>>
>>>> Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com>
>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
>>>>
>>>> --
>>>>
>>>> drivers/pci/pci.c | 33 +++++++++++++++++++++++++++++++++
>>>> drivers/pci/pci.h | 1 +
>>>> drivers/pci/probe.c | 38 ++++++++++++++++++++++++++++++++++++--
>>>> 3 files changed, 70 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index 563901c..a7a2e2b 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -2835,6 +2835,39 @@ static bool pci_acs_flags_enabled(struct pci_dev
>>>> *pdev, u16 acs_flags)
>>>> }
>>>>
>>>> /**
>>>> + * pci_std_enable_acs_sv - enable/disable ACS source validation if
>>>> supported by the switch
>>>> + * @dev - pcie switch/RP
>>>> + * @enable - enable (1) or disable (0) source validation
>>>> + *
>>>> + * Returns : < 0 on failure
>>>
>>> You didn't define the meaning of 0 and >0, but you check it later against
>>>> 0,
>>> Then what does it mean 0 and >0 ?
>>
>> see below..
>>>
>>>
>>>> + * previous acs_sv state
>>
>>
>> It returns the previous acs_sv state (0 or 1).
>
> You didn't clarify the meaning of previous acs_sv state, or possible value,
> you check it later with >0 also confused the possibility.
>
>

>>>>
>>>> + */
>>>> +int pci_std_enable_acs_sv(struct pci_dev *dev, bool enable)
>>>> +{
>>>> + int pos;
>>>> + u16 cap;
>>>> + u16 ctrl;
>>>> + int retval;
>>>> +
>>>> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_ACS);
>>>> + if (!pos)
>>>> + return -ENODEV;
>>>> +
>>>> + pci_read_config_word(dev, pos + PCI_ACS_CAP, &cap);
>>>> + pci_read_config_word(dev, pos + PCI_ACS_CTRL, &ctrl);
>>>> +
>>>> + retval = !!(ctrl & cap & PCI_ACS_SV);
>>>
>>> If the device's ACS SV( ACS Source Validation) capability wasn't
>>> implemented, the return value of this function will still tell us the
>>> operation of enabling is successful ? though it might be rare case.
>>
>> If the ACS capability is implemented, then all bits are expected to have
>> meaning and are valid. If SV is not implemented by the switch, the control
>> bit for it should return zero (no source validation done). This is the PCI
>> specification. The onus is on the switch designer to keep it so.
>
> PCI spec doesn't say SV must be implemented in every device even it
> has ACS Cap, see also:
>
> "6.12.1.2. ACS Functions in Multi-Function Devices This section
> applies to multi-Function device ACS Functions, with the exception of
> Downstream Port Functions, which are covered in the preceding section.
>  ACS Source Validation: must not be implemented. 20  ACS Translation
> Blocking: must not be implemented.  ACS P2P Request Redirect: must be
> implemented by Functions that support peer-to-peer traffic with other
> Functions.
> "
> Here pci_std_enable_acs_sv() is common function, once implemented,
> possible be used by other code to enable acs beside this workaround.
>
> Then how about it is called with a MF device ?

I will add code to check if SV is implemented and return failure (< 0)
in case it is not.
thanks
James

>
> Thanks,
> Ethan
>
>>
>> thanks,
>> James
>>
>>
>>>> + if (enable)
>>>> + ctrl |= (cap & PCI_ACS_SV);
>>>> + else
>>>> + ctrl &= ~(cap & PCI_ACS_SV);
>>>> +
>>>> + pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
>>>> +
>>>> + return retval;
>>>> +}
>>>> +
>>>> +/**
>>>> * pci_acs_enabled - test ACS against required flags for a given device
>>>> * @pdev: device to test
>>>> * @acs_flags: required PCI ACS flags
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index f8113e5..3960c2a 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -343,6 +343,7 @@ static inline resource_size_t
>>>> pci_resource_alignment(struct pci_dev *dev,
>>>> }
>>>>
>>>> void pci_enable_acs(struct pci_dev *dev);
>>>> +int pci_std_enable_acs_sv(struct pci_dev *dev, bool enable);
>>>>
>>>> #ifdef CONFIG_PCIE_PTM
>>>> void pci_ptm_init(struct pci_dev *dev);
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 19c8950..c154a90 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1763,8 +1763,8 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
>>>> }
>>>> EXPORT_SYMBOL(pci_alloc_dev);
>>>>
>>>> -bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>>>> - int crs_timeout)
>>>> +static bool __pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn,
>>>> + u32 *l, int crs_timeout)
>>>> {
>>>> int delay = 1;
>>>>
>>>> @@ -1801,6 +1801,40 @@ bool pci_bus_read_dev_vendor_id(struct pci_bus
>>>> *bus,
>>>> int devfn, u32 *l,
>>>>
>>>> return true;
>>>> }
>>>> +
>>>> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l,
>>>> + int crs_timeout)
>>>> +{
>>>> + int found;
>>>> + int enable = -1;
>>>> + int idt_workaround = (bus->self && (bus->self->vendor ==
>>>> PCI_VENDOR_ID_IDT));
>>>> + /*
>>>> + * Some IDT switches flag an ACS violation for config reads
>>>> + * even though the PCI spec allows for it (PCIe 3.1, 6.1.12.1)
>>>> + * It flags it because the bus number is not properly set in the
>>>> + * completion. The workaround is to do a dummy write to properly
>>>> + * latch number once the device is ready for config operations
>>>> + */
>>>> +
>>>> + if (idt_workaround)
>>>> + enable = pci_std_enable_acs_sv(bus->self, false);
>>>> +
>>>> + found = __pci_bus_read_dev_vendor_id(bus, devfn, l, crs_timeout);
>>>> +
>>>> + /*
>>>> + * The fact that we can read the vendor id indicates that the
>>>> device
>>>> + * is ready for config operations. Do the write as part of the
>>>> errata
>>>> + * workaround.
>>>> + */
>>>> + if (idt_workaround) {
>>>> + if (found)
>>>> + pci_bus_write_config_word(bus, devfn,
>>>> PCI_VENDOR_ID,
>>>> 0);
>>>> + if (enable > 0)
>>>> + pci_std_enable_acs_sv(bus->self, enable);
>>>> + }
>>>> +
>>>> + return found;
>>>> +}
>>>> EXPORT_SYMBOL(pci_bus_read_dev_vendor_id);
>>>>
>>

\
 
 \ /
  Last update: 2017-07-06 18:01    [W:0.049 / U:1.844 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site