lkml.org 
[lkml]   [2017]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 2/3] PCI: Enable PCIe Relaxed Ordering if supported
From
Date


On 2017/6/16 22:39, Alexander Duyck wrote:
> On Thu, Jun 15, 2017 at 6:10 PM, Ding Tianhong <dingtianhong@huawei.com> wrote:
>>
>>
>> On 2017/6/13 5:28, Alexander Duyck wrote:
>>> On Mon, Jun 12, 2017 at 4:05 AM, Ding Tianhong <dingtianhong@huawei.com> wrote:
>> ...
>>>> /**
>>>> + * pcie_clear_relaxed_ordering - clear PCI Express relaxed ordering bit
>>>> + * @dev: PCI device to query
>>>> + *
>>>> + * If possible clear relaxed ordering
>>>> + */
>>>> +int pcie_clear_relaxed_ordering(struct pci_dev *dev)
>>>> +{
>>>> + return pcie_capability_clear_word(dev, PCI_EXP_DEVCTL,
>>>> + PCI_EXP_DEVCTL_RELAX_EN);
>>>> +}
>>>> +EXPORT_SYMBOL(pcie_clear_relaxed_ordering);
>>>> +
>>>> +/**
>>>> + * pcie_relaxed_ordering_supported - Probe for PCIe relexed ordering support
>>>> + * @dev: PCI device to query
>>>> + *
>>>> + * Returns true if the device support relaxed ordering attribute.
>>>> + */
>>>> +bool pcie_relaxed_ordering_supported(struct pci_dev *dev)
>>>> +{
>>>> + bool ro_supported = false;
>>>> + u16 v;
>>>> +
>>>> + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v);
>>>> + if ((v & PCI_EXP_DEVCTL_RELAX_EN) >> 4)
>>>> + ro_supported = true;
>>>
>>> Instead of "return ro_supported" why not just "return !!(v &
>>> PCIE_EXP_DEVCTL_RELAX_EN)"? You can cut out the extra steps and save
>>> yourself some extra steps this way since the shift by 4 shouldn't even
>>> really be needed since you are just testing for a bit anyway.
>>>
>>
>> OK.
>>
>>>> +
>>>> + return ro_supported;
>>>> +}
>>>> +EXPORT_SYMBOL(pcie_relaxed_ordering_supported);
>>>> +
>>>> +/**
>>>> * pcie_get_minimum_link - determine minimum link settings of a PCI device
>>>> * @dev: PCI device to query
>>>> * @speed: storage for minimum speed
>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>> index 19c8950..ed1f717 100644
>>>> --- a/drivers/pci/probe.c
>>>> +++ b/drivers/pci/probe.c
>>>> @@ -1701,6 +1701,46 @@ static void pci_configure_extended_tags(struct pci_dev *dev)
>>>> PCI_EXP_DEVCTL_EXT_TAG);
>>>> }
>>>>
>>>> +/**
>>>> + * pci_dev_should_disable_relaxed_ordering - check if the PCI device
>>>> + * should disable the relaxed ordering attribute.
>>>> + * @dev: PCI device
>>>> + *
>>>> + * Return true if any of the PCI devices above us do not support
>>>> + * relaxed ordering.
>>>> + */
>>>> +static bool pci_dev_should_disable_relaxed_ordering(struct pci_dev *dev)
>>>> +{
>>>> + bool ro_disabled = false;
>>>> +
>>>> + while (dev) {
>>>> + if (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING) {
>>>> + ro_disabled = true;
>>>> + break;
>>>> + }
>>>> + dev = dev->bus->self;
>>>> + }
>>>> +
>>>> + return ro_disabled;
>>>
>>> Same thing here. I would suggest just returning either true or false,
>>> and drop the ro_disabled value. It will return the lines of code and
>>> make things a bit bit more direct.
>>>
>>
>> OK.
>>
>>>> +}
>>>> +
>>>> +static void pci_configure_relaxed_ordering(struct pci_dev *dev)
>>>> +{
>>>> + struct pci_dev *bridge = pci_upstream_bridge(dev);
>>>> +
>>>> + if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge))
>>>> + return;
>>>
>>> The pci_is_pcie check is actually redundant based on the
>>> pcie_relaxed_ordering_supported check using pcie_capability_read_word.
>>>
>>
>> Yes, pcie_capability_read_word already check it, thanks.
>>
>>
>>> Also I am not sure what the point is of the pci_upstream_bridge()
>>> check is, it seems like you should be able to catch all the same stuff
>>> in your pci_dev_should_disable_relaxed_ordering() call. Though it did
>>> give me a thought. I don't think we can alter this for a VF, so you
>>> might want to add a check for dev->is_virtfn to the list of checks and
>>> if it is a virtual function just return since I don't think there are
>>> any VFs that would let you alter this bit anyway.
>>>
>> If the upstream device is null, does it mean that it is in a guest OS device? maybe I miss something.
>> also I will check the dev->is_virtfn to avoid trying to change the configuration space for VF.
>
> Yes, usually the upstream device is NULL in guest setups where all the
> devices are hung off of a single PCI bus.
>

OK, no need for the pci_upstream_bridge, the pci_dev_should_disable_relaxed_ordering() will check and return result
as we need.

>> Another question: Because it looks like that maybe the Casey is too busy these days, should we
>> delay the modification of the cxgb4 and instead to update the ixgbe? what do you think about it. :)
>
> I would still submit the cxgb4 changes with the one change we have
> made. It should work as is. We can just leave any follow-up work to
> Casey in terms of enabling the peer-to-peer mode if the bits related
> to relaxed ordering are cleared.
>

OK, thanks.

>> Thanks.
>> Ding
>>
>>>> + /* If the releaxed ordering enable bit is not set, do nothing. */
>>>> + if (!pcie_relaxed_ordering_supported(dev))
>>>> + return;
>>>> +
>>>> + if (pci_dev_should_disable_relaxed_ordering(dev)) {
>>>> + pcie_clear_relaxed_ordering(dev);
>>>> + dev_info(&dev->dev, "Disable Relaxed Ordering\n");
>>>> + }
>>>> +}
>>>> +
>>>> static void pci_configure_device(struct pci_dev *dev)
>>>> {
>>>> struct hotplug_params hpp;
>>>> @@ -1708,6 +1748,7 @@ static void pci_configure_device(struct pci_dev *dev)
>>>>
>>>> pci_configure_mps(dev);
>>>> pci_configure_extended_tags(dev);
>>>> + pci_configure_relaxed_ordering(dev);
>>>>
>>>> memset(&hpp, 0, sizeof(hpp));
>>>> ret = pci_get_hp_params(dev, &hpp);
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index e1e8428..9870781 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -1105,6 +1105,8 @@ int __pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>>>> void pci_pme_wakeup_bus(struct pci_bus *bus);
>>>> void pci_d3cold_enable(struct pci_dev *dev);
>>>> void pci_d3cold_disable(struct pci_dev *dev);
>>>> +int pcie_clear_relaxed_ordering(struct pci_dev *dev);
>>>> +bool pcie_relaxed_ordering_supported(struct pci_dev *dev);
>>>>
>>>> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state,
>>>> bool enable)
>>>> --
>>>> 1.9.0
>>>>
>>>>
>>>
>>> .
>>>
>>
>
> .
>

\
 
 \ /
  Last update: 2017-06-19 08:15    [W:0.064 / U:1.424 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site