Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751742AbdFIJOu (ORCPT ); Fri, 9 Jun 2017 05:14:50 -0400 Received: from szxga02-in.huawei.com ([45.249.212.188]:7349 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751666AbdFIJOp (ORCPT ); Fri, 9 Jun 2017 05:14:45 -0400 Subject: Re: [PATCH v3 2/3] PCI: Enable PCIe Relaxed Ordering if supported To: John Garry , , , , , , , , , , , , , , , , , , , , , , , , References: <1496826968-10152-1-git-send-email-dingtianhong@huawei.com> <1496826968-10152-3-git-send-email-dingtianhong@huawei.com> CC: Linuxarm From: Ding Tianhong Message-ID: Date: Fri, 9 Jun 2017 17:13:37 +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="windows-1252" 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.0A090206.593A66D9.0051,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: 5aebf9bd2b4edfdddfd48c1ca06cedad Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5914 Lines: 185 Hi John: Thanks for the reviewing, I will fix it in next version. Ding On 2017/6/8 1:55, John Garry wrote: > On 07/06/2017 10:16, Ding Tianhong wrote: > > Hi Ding, > > A few general style comments: > >> The PCIe Device Control Register use the bit 4 to indicate that >> whether the device is permitted to enable relaxed ordering or not. >> But relaxed ordering is not safe for some platform which could only >> use strong write ordering, so devices are allowed (but not required) >> to enable relaxed ordering bit by default. >> >> If a PCIe device didn't enable the relaxed ordering attribute default, >> we should not do anything in the PCIe configuration, otherwise we >> should check if any of the devices above us do not support relaxed >> ordering by the PCI_DEV_FLAGS_NO_RELAXED_ORDERING flag, then base on >> the result if we get a return that indicate that the relaxed ordering >> is not supported we should update our device to disable relaxed ordering >> in configuration space. If the device above us doesn't exist or isn't >> the PCIe device, we shouldn't do anything and skip updating relaxed ordering >> because we are probably running in a guest. > > A guest machine/environment > >> >> Signed-off-by: Ding Tianhong >> --- >> drivers/pci/pci.c | 29 +++++++++++++++++++++++++++++ >> drivers/pci/probe.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> include/linux/pci.h | 2 ++ >> 3 files changed, 74 insertions(+) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index b01bd5b..3d42b38 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -4878,6 +4878,35 @@ int pcie_set_mps(struct pci_dev *dev, int mps) >> EXPORT_SYMBOL(pcie_set_mps); >> >> /** >> + * pcie_clear_relaxed_ordering - clear PCI Express relexed 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_get_relaxed_ordering - check PCI Express relexed ordering bit > > s/relexed/relaxed/ > > Check what on relaxed ordering bit? > > And the function name is inconsistent with this discription. > >> + * @dev: PCI device to query >> + * >> + * Returns true if relaxed ordering is been set > > If you want to return true/false, then use !!, below in the function > >> + */ >> +int pcie_get_relaxed_ordering(struct pci_dev *dev) >> +{ >> + u16 v; >> + >> + pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &v); >> + >> + return (v & PCI_EXP_DEVCTL_RELAX_EN) >> 4; >> +} >> +EXPORT_SYMBOL(pcie_get_relaxed_ordering); >> + >> +/** >> * 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..0c94c80 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -1701,6 +1701,48 @@ static void pci_configure_extended_tags(struct pci_dev *dev) >> PCI_EXP_DEVCTL_EXT_TAG); >> } >> >> +/** >> + * pci_dev_disable_relaxed_ordering - check if the PCI device >> + * should disable the relaxed ordering attribute. > > I think that we need a more accurate description. I know some people think a function which just "checks" is vague. > >> + * @dev: PCI device >> + * >> + * Return true if any of the PCI devices above us do not support >> + * relaxed ordering. >> + */ >> +static int pci_dev_disable_relaxed_ordering(struct pci_dev *dev) > > The function name implies an action - disabling - but this function does nothing except return a value > >> +{ >> + int ro_disabled = 0; >> + >> + while(dev) { > > Did you run checkpatch? > >> + if (dev->dev_flags & PCI_DEV_FLAGS_NO_RELAXED_ORDERING) { >> + ro_disabled = 1; > > just return true, and return false at the bottom, so you can do away with ro_disabled (which is not a bool) > >> + break; >> + } >> + dev = dev->bus->self; >> + } >> + >> + return ro_disabled; >> +} >> + >> +static void pci_configure_relaxed_ordering(struct pci_dev *dev) >> +{ >> + struct pci_dev *bridge = pci_upstream_bridge(dev); >> + int origin_ero; > > You don't need this variable > >> + >> + if (!pci_is_pcie(dev) || !bridge || !pci_is_pcie(bridge)) >> + return; >> + >> + origin_ero = pcie_get_relaxed_ordering(dev); >> + /* If the releaxed ordering enable bit is not set, do nothing. */ >> + if (!origin_ero) >> + return; >> + >> + if (pci_dev_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 +1750,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..299d2f3 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); >> +int pcie_get_relaxed_ordering(struct pci_dev *dev); >> >> static inline int pci_enable_wake(struct pci_dev *dev, pci_power_t state, >> bool enable) >> > > > > . >