Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660052 > unrolled thread

Re: [PATCH v3 2/3] PCI: Enable PCIe Relaxed Ordering if supported

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] PCI: Enable PCIe Relaxed Ordering if supported John Garry <john.garry@huawei.com> - 2017-06-07 20:00 +0200
    Re: [PATCH v3 2/3] PCI: Enable PCIe Relaxed Ordering if supported Ding Tianhong <dingtianhong@huawei.com> - 2017-06-09 11:20 +0200

#1660052 — Re: [PATCH v3 2/3] PCI: Enable PCIe Relaxed Ordering if supported

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 <dingtianhong@huawei.com>
> ---
>  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)
>

[toc] | [next] | [standalone]

#1662162

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 <dingtianhong@huawei.com>
>> ---
>>  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)
>>
> 
> 
> 
> .
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web