Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751438AbdFSGOD (ORCPT ); Mon, 19 Jun 2017 02:14:03 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:8326 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750848AbdFSGOB (ORCPT ); Mon, 19 Jun 2017 02:14:01 -0400 Subject: Re: [PATCH v4 2/3] PCI: Enable PCIe Relaxed Ordering if supported To: Alexander Duyck References: <1497265525-4752-1-git-send-email-dingtianhong@huawei.com> <1497265525-4752-3-git-send-email-dingtianhong@huawei.com> <5854930e-af6a-4274-2822-cf38e5967da3@huawei.com> CC: Casey Leedom , Ashok Raj , "Bjorn Helgaas" , Michael Werner , "Ganesh Goudar" , Asit K Mallick , Patrick J Cramer , Suravee Suthikulpanit , Bob Shaw , h , Amir Ancel , Gabriele Paoloni , David Laight , "Jeff Kirsher" , Catalin Marinas , Will Deacon , Mark Rutland , Robin Murphy , David Miller , , Netdev , "linux-pci@vger.kernel.org" , "linux-kernel@vger.kernel.org" From: Ding Tianhong Message-ID: <627799c4-7086-b2bb-eda9-1ea14a1a4bc1@huawei.com> Date: Mon, 19 Jun 2017 14:12:22 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.177.23.32] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020203.59476B59.005E,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 887e834276d5e754bc8cf63cae6d94a1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6548 Lines: 184 On 2017/6/16 22:39, Alexander Duyck wrote: > On Thu, Jun 15, 2017 at 6:10 PM, Ding Tianhong wrote: >> >> >> On 2017/6/13 5:28, Alexander Duyck wrote: >>> On Mon, Jun 12, 2017 at 4:05 AM, Ding Tianhong 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 >>>> >>>> >>> >>> . >>> >> > > . >