[net-next,1/4] ixgbe: sparc: rename the ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER

Message ID 1491031554-19516-2-git-send-email-dingtianhong@huawei.com
State New
Headers show
Series
  • ixgbe: enable Relaxed Order for ARM64
Related show

Commit Message

Ding Tianhong April 1, 2017, 7:25 a.m.
Till now only the Intel ixgbe could support enable
Relaxed ordering in the drivers for special architecture,
but the ARCH_WANT_RELAX_ORDER is looks like a general name
for all arch, so rename to a specific name for intel
card looks more appropriate.

Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

---
 arch/Kconfig                                    | 2 +-
 arch/sparc/Kconfig                              | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

-- 
1.9.0

Comments

David Miller April 1, 2017, 6:26 p.m. | #1
From: Ding Tianhong <dingtianhong@huawei.com>

Date: Sat, 1 Apr 2017 15:25:51 +0800

> Till now only the Intel ixgbe could support enable

> Relaxed ordering in the drivers for special architecture,

> but the ARCH_WANT_RELAX_ORDER is looks like a general name

> for all arch, so rename to a specific name for intel

> card looks more appropriate.

> 

> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>


This is not a driver specific facility.

Any driver can test this symbol and act accordingly.

Just because IXGBE is the first and only user, doesn't mean
the facility is driver specific.

Thank you.
Ding Tianhong April 2, 2017, 6:49 a.m. | #2
On 2017/4/2 2:26, David Miller wrote:
> From: Ding Tianhong <dingtianhong@huawei.com>

> Date: Sat, 1 Apr 2017 15:25:51 +0800

> 

>> Till now only the Intel ixgbe could support enable

>> Relaxed ordering in the drivers for special architecture,

>> but the ARCH_WANT_RELAX_ORDER is looks like a general name

>> for all arch, so rename to a specific name for intel

>> card looks more appropriate.

>>

>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> 

> This is not a driver specific facility.

> 

> Any driver can test this symbol and act accordingly.

> 

> Just because IXGBE is the first and only user, doesn't mean

> the facility is driver specific.

> 


Understand clearly,but the ARCH_WANT_RELAX_ORDER is really too generic and simple,
cause much misleading to indicate that it looks like the hack code for some architecture.
what do you think of the ETHERNET_ALLOW_RELAXED_ORDER in the drivers/net/ethernet/*,
it will only affect ethernet and not only for Ixgbe.

Thanks
Ding


> Thank you.

> 

> .

>
John Garry April 5, 2017, 1:05 p.m. | #3
On 02/04/2017 07:49, Ding Tianhong wrote:
>

>

> On 2017/4/2 2:26, David Miller wrote:

>> From: Ding Tianhong <dingtianhong@huawei.com>

>> Date: Sat, 1 Apr 2017 15:25:51 +0800

>>

>>> Till now only the Intel ixgbe could support enable

>>> Relaxed ordering in the drivers for special architecture,

>>> but the ARCH_WANT_RELAX_ORDER is looks like a general name

>>> for all arch, so rename to a specific name for intel

>>> card looks more appropriate.

>>>

>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

>>

>> This is not a driver specific facility.

>>

>> Any driver can test this symbol and act accordingly.

>>

>> Just because IXGBE is the first and only user, doesn't mean

>> the facility is driver specific.

>>

>

> Understand clearly,but the ARCH_WANT_RELAX_ORDER is really too generic and simple,

> cause much misleading to indicate that it looks like the hack code for some architecture.

> what do you think of the ETHERNET_ALLOW_RELAXED_ORDER in the drivers/net/ethernet/*,

> it will only affect ethernet and not only for Ixgbe.

>


Hi Ding,

I think the actual original config ARCH_WANT_RELAX_ORDER is quite 
dubious, and it does not really tell us which feature(s) of the 
architecture supports this (if indeed it is arch specific).

According to the original commit, 1a8b6d76dc5b net:add one common config 
ARCH_WANT_RELAX_ORDER to support relax ordering, this is specific to 
SPARC only:
"Currently it only supports one special cpu architecture(SPARC) in 82599 
driver to enable RO feature, this is not very common for other cpu 
architecture which really needs RO feature".

This sounds wooly.

So I think that we need to know which specific architecture, memory 
model, or PCI host/port/EP features, or combination of them, allows this 
so called relaxed ordering.

And a config option is probably not even the proper check.

John

> Thanks

> Ding

>

>

>> Thank you.

>>

>> .

>>

>

>

> .

>
Ding Tianhong April 6, 2017, 11:28 a.m. | #4
On 2017/4/5 21:05, John Garry wrote:
> On 02/04/2017 07:49, Ding Tianhong wrote:

>>

>>

>> On 2017/4/2 2:26, David Miller wrote:

>>> From: Ding Tianhong <dingtianhong@huawei.com>

>>> Date: Sat, 1 Apr 2017 15:25:51 +0800

>>>

>>>> Till now only the Intel ixgbe could support enable

>>>> Relaxed ordering in the drivers for special architecture,

>>>> but the ARCH_WANT_RELAX_ORDER is looks like a general name

>>>> for all arch, so rename to a specific name for intel

>>>> card looks more appropriate.

>>>>

>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

>>>

>>> This is not a driver specific facility.

>>>

>>> Any driver can test this symbol and act accordingly.

>>>

>>> Just because IXGBE is the first and only user, doesn't mean

>>> the facility is driver specific.

>>>

>>

>> Understand clearly,but the ARCH_WANT_RELAX_ORDER is really too generic and simple,

>> cause much misleading to indicate that it looks like the hack code for some architecture.

>> what do you think of the ETHERNET_ALLOW_RELAXED_ORDER in the drivers/net/ethernet/*,

>> it will only affect ethernet and not only for Ixgbe.

>>

> 


Hi John:

> Hi Ding,

> 

> I think the actual original config ARCH_WANT_RELAX_ORDER is quite dubious, and it does not really tell us which feature(s) of the architecture supports this (if indeed it is arch specific).

> 


Agree.

> According to the original commit, 1a8b6d76dc5b net:add one common config ARCH_WANT_RELAX_ORDER to support relax ordering, this is specific to SPARC only:

> "Currently it only supports one special cpu architecture(SPARC) in 82599 driver to enable RO feature, this is not very common for other cpu architecture which really needs RO feature".

> 


Relaxed Ordering is a general setting compare to the SO for PCIE controller, if the drivers support it, the architecture could choose to enable it, of course the feature is not support for
every arch.

> This sounds wooly.

> 

> So I think that we need to know which specific architecture, memory model, or PCI host/port/EP features, or combination of them, allows this so called relaxed ordering.

> 


This depends on the PCIE design in the chip, I couldn't know whether other arch has some issues when enable RO,if the chip totally support PCIE3.0 standard and has no defect,should both support RO and SO.

Thanks
Ding
> And a config option is probably not even the proper check.

> 

> John






> 

>> Thanks

>> Ding

>>

>>

>>> Thank you.

>>>

>>> .

>>>

>>

>>

>> .

>>

> 

> 

> 

> .

>
Gabriele Paoloni April 13, 2017, 9:10 a.m. | #5
Hi David

> -----Original Message-----

> Subject: Re: [PATCH net-next 1/4] ixgbe: sparc: rename the

> ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER

> Date: Sat, 1 Apr 2017 11:26:03 -0700

> From: David Miller <davem@davemloft.net>

> To: dingtianhong@huawei.com

> CC: catalin.marinas@arm.com, will.deacon@arm.com, mark.rutland@arm.com,

> robin.murphy@arm.com, jeffrey.t.kirsher@intel.com,

> alexander.duyck@gmail.com, linux-arm-kernel@lists.infradead.org,

> netdev@vger.kernel.org

> 

> From: Ding Tianhong <dingtianhong@huawei.com>

> Date: Sat, 1 Apr 2017 15:25:51 +0800

> 

> > Till now only the Intel ixgbe could support enable

> > Relaxed ordering in the drivers for special architecture,

> > but the ARCH_WANT_RELAX_ORDER is looks like a general name

> > for all arch, so rename to a specific name for intel

> > card looks more appropriate.

> >

> > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> 

> This is not a driver specific facility.

> 

> Any driver can test this symbol and act accordingly.

> 

> Just because IXGBE is the first and only user, doesn't mean

> the facility is driver specific.



Please correct me if I am wrong but my understanding is that the standard
way to enable/disable relaxed ordering is to set/clear bit 4 of the Device
Control Register (PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed
ordering */).
Now I have looked up for all drivers either enabling or disabling relaxed
ordering and none of them seems to need a symbol to decide whether to
enable it or not.
Also it seems to me enabling/disabling relaxed ordering is never bound to the
host architecture.

So why this should be (or it is expected to be) a generic symbol?
Wouldn't it be more correct to have this as a driver specific symbol now and
move it to a generic one later once we have other drivers requiring it?
  
Many thanks
Gab

> 

> Thank you.

> 

> .

>
David Miller April 13, 2017, 2:53 p.m. | #6
From: Gabriele Paoloni <gabriele.paoloni@huawei.com>

Date: Thu, 13 Apr 2017 09:10:32 +0000

> Wouldn't it be more correct to have this as a driver specific symbol

> now and move it to a generic one later once we have other drivers

> requiring it?


No, it would not.
David Laight April 18, 2017, 1:25 p.m. | #7
From: Gabriele Paoloni

> Sent: 13 April 2017 10:11

> > > Till now only the Intel ixgbe could support enable

> > > Relaxed ordering in the drivers for special architecture,

> > > but the ARCH_WANT_RELAX_ORDER is looks like a general name

> > > for all arch, so rename to a specific name for intel

> > > card looks more appropriate.

> > >

> > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> >

> > This is not a driver specific facility.

> >

> > Any driver can test this symbol and act accordingly.

> >

> > Just because IXGBE is the first and only user, doesn't mean

> > the facility is driver specific.

> 

> 

> Please correct me if I am wrong but my understanding is that the standard

> way to enable/disable relaxed ordering is to set/clear bit 4 of the Device

> Control Register (PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed

> ordering */).

> Now I have looked up for all drivers either enabling or disabling relaxed

> ordering and none of them seems to need a symbol to decide whether to

> enable it or not.

> Also it seems to me enabling/disabling relaxed ordering is never bound to the

> host architecture.

> 

> So why this should be (or it is expected to be) a generic symbol?

> Wouldn't it be more correct to have this as a driver specific symbol now and

> move it to a generic one later once we have other drivers requiring it?


'Relaxed ordering' is a bit in the TLP header.
A device (like the ixgbe hardware) can set it for some transactions and
still have the transactions 'ordered enough' for the driver to work.
(If all transactions have 'relaxed ordering' set then I suspect it is
almost impossible to write a working driver.)
The host side could (probably does) have a bit to enable 'relaxed ordering',
it could also enforce stronger ordering than required by the PCIe spec
(eg keeping reads in order).

Clearly, on some sparc64 systems, ixgbe needs to use 'relaxed ordering'.
To me this requires two separate bits be enabled:
1) to the ixgbe driver to generate the 'relaxed' TLP.
2) to the host to actually act on them.
If the ixgbe driver works when both are enabled why have options to
disable either (except for bug-finding)?

	David
Gabriele Paoloni April 19, 2017, 2:28 p.m. | #8
Hi David

Many thanks for your reply

> -----Original Message-----

> From: David Laight [mailto:David.Laight@ACULAB.COM]

> Sent: 18 April 2017 14:26

> To: Gabriele Paoloni; davem@davemloft.net

> Cc: Catalin Marinas; Will Deacon; Mark Rutland; Robin Murphy;

> jeffrey.t.kirsher@intel.com; alexander.duyck@gmail.com; linux-arm-

> kernel@lists.infradead.org; netdev@vger.kernel.org; Dingtianhong;

> Linuxarm

> Subject: RE: Re: [PATCH net-next 1/4] ixgbe: sparc: rename the

> ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER

> 

> From: Gabriele Paoloni

> > Sent: 13 April 2017 10:11

> > > > Till now only the Intel ixgbe could support enable

> > > > Relaxed ordering in the drivers for special architecture,

> > > > but the ARCH_WANT_RELAX_ORDER is looks like a general name

> > > > for all arch, so rename to a specific name for intel

> > > > card looks more appropriate.

> > > >

> > > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> > >

> > > This is not a driver specific facility.

> > >

> > > Any driver can test this symbol and act accordingly.

> > >

> > > Just because IXGBE is the first and only user, doesn't mean

> > > the facility is driver specific.

> >

> >

> > Please correct me if I am wrong but my understanding is that the

> standard

> > way to enable/disable relaxed ordering is to set/clear bit 4 of the

> Device

> > Control Register (PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed

> > ordering */).

> > Now I have looked up for all drivers either enabling or disabling

> relaxed

> > ordering and none of them seems to need a symbol to decide whether to

> > enable it or not.

> > Also it seems to me enabling/disabling relaxed ordering is never

> bound to the

> > host architecture.

> >

> > So why this should be (or it is expected to be) a generic symbol?

> > Wouldn't it be more correct to have this as a driver specific symbol

> now and

> > move it to a generic one later once we have other drivers requiring

> it?

> 

> 'Relaxed ordering' is a bit in the TLP header.

> A device (like the ixgbe hardware) can set it for some transactions and

> still have the transactions 'ordered enough' for the driver to work.

> (If all transactions have 'relaxed ordering' set then I suspect it is

> almost impossible to write a working driver.)

> The host side could (probably does) have a bit to enable 'relaxed

> ordering',

> it could also enforce stronger ordering than required by the PCIe spec

> (eg keeping reads in order).


My understanding is that from the host side the host is always allowed
(as long as it complies with the rules specified in sec.2.4.1 of the PCIe
Specs) to set the RO attribute in the TLP and the target function should
be abel to cope with it.

On the device side the device is allowed to set the RO attribute in the
TLP only if bit4 of the "Device Control Register" is set.

> 

> Clearly, on some sparc64 systems, ixgbe needs to use 'relaxed

> ordering'.

> To me this requires two separate bits be enabled:

> 1) to the ixgbe driver to generate the 'relaxed' TLP.

> 2) to the host to actually act on them.


My understanding is that for performance reasons when possible we
should enable relaxed ordering and I think this is up to the host
(i.e. the host somehow should know when he is capable of handling
RO TLPs and therefore it will try to enable it on the driver)

> If the ixgbe driver works when both are enabled why have options to

> disable either (except for bug-finding)?


I think that by default the ixgbe driver disable RO since there are
issues with "some chipsets" according to commit 3d5c520727ce "ixgbe:
move disabling of relaxed ordering in start_hw()".
What this means is a bit obscure to me and seems to be not related to
the host architecture

Also looking at where and why the other drivers set/clear the "Enable
Relaxed Ordering" bit it seems that currently this is not tied to the
host architecture nor to any global symbol; instead it seems purely
dependent on the PCIe device chipset itself.

> 

> 	David
Gabriele Paoloni April 19, 2017, 2:46 p.m. | #9
Hi Amir
 
> From: Amir Ancel [mailto:amira@mellanox.com]

> Sent: 18 April 2017 21:18

> To: David Laight; Gabriele Paoloni; davem@davemloft.net

> Cc: Catalin Marinas; Will Deacon; Mark Rutland; Robin Murphy;

> jeffrey.t.kirsher@intel.com; alexander.duyck@gmail.com; linux-arm-

> kernel@lists.infradead.org; netdev@vger.kernel.org; Dingtianhong;

> Linuxarm

> Subject: Re: Re: [PATCH net-next 1/4] ixgbe: sparc: rename the

> ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER

> 

> Hi,

> mlx5 driver is planned to have RO support this year.

> I believe drivers should be able to query whether the arch support it


I guess that here when you say query you mean having a config symbol
that is set accordingly to the host architecture, right?

As already said I have looked around a bit and other drivers do not seem
to enable/disable RO for their EP on the basis of the host architecture.
So why should mlx5 do it according to the host?

Also my understating is that some architectures (like ARM64 for example)
can have different PCI host controller implementations depending on the
vendor...therefore maybe it is not appropriate there to have a Kconfig
symbol selected by the architecture...  

Thanks
Gab

> or not and enable it in the network adapter accordingly.

> 

> -Amir

> ________________________________________

> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> on

> behalf of David Laight <David.Laight@ACULAB.COM>

> Sent: Tuesday, April 18, 2017 4:25:44 PM

> To: 'Gabriele Paoloni'; davem@davemloft.net

> Cc: Catalin Marinas; Will Deacon; Mark Rutland; Robin Murphy;

> jeffrey.t.kirsher@intel.com; alexander.duyck@gmail.com; linux-arm-

> kernel@lists.infradead.org; netdev@vger.kernel.org; Dingtianhong;

> Linuxarm

> Subject: RE: Re: [PATCH net-next 1/4] ixgbe: sparc: rename the

> ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER

> 

> From: Gabriele Paoloni

> > Sent: 13 April 2017 10:11

> > > > Till now only the Intel ixgbe could support enable

> > > > Relaxed ordering in the drivers for special architecture,

> > > > but the ARCH_WANT_RELAX_ORDER is looks like a general name

> > > > for all arch, so rename to a specific name for intel

> > > > card looks more appropriate.

> > > >

> > > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

> > >

> > > This is not a driver specific facility.

> > >

> > > Any driver can test this symbol and act accordingly.

> > >

> > > Just because IXGBE is the first and only user, doesn't mean

> > > the facility is driver specific.

> >

> >

> > Please correct me if I am wrong but my understanding is that the

> standard

> > way to enable/disable relaxed ordering is to set/clear bit 4 of the

> Device

> > Control Register (PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed

> > ordering */).

> > Now I have looked up for all drivers either enabling or disabling

> relaxed

> > ordering and none of them seems to need a symbol to decide whether to

> > enable it or not.

> > Also it seems to me enabling/disabling relaxed ordering is never

> bound to the

> > host architecture.

> >

> > So why this should be (or it is expected to be) a generic symbol?

> > Wouldn't it be more correct to have this as a driver specific symbol

> now and

> > move it to a generic one later once we have other drivers requiring

> it?

> 

> 'Relaxed ordering' is a bit in the TLP header.

> A device (like the ixgbe hardware) can set it for some transactions and

> still have the transactions 'ordered enough' for the driver to work.

> (If all transactions have 'relaxed ordering' set then I suspect it is

> almost impossible to write a working driver.)

> The host side could (probably does) have a bit to enable 'relaxed

> ordering',

> it could also enforce stronger ordering than required by the PCIe spec

> (eg keeping reads in order).

> 

> Clearly, on some sparc64 systems, ixgbe needs to use 'relaxed

> ordering'.

> To me this requires two separate bits be enabled:

> 1) to the ixgbe driver to generate the 'relaxed' TLP.

> 2) to the host to actually act on them.

> If the ixgbe driver works when both are enabled why have options to

> disable either (except for bug-finding)?

> 

>         David
Will Deacon April 24, 2017, 2:53 p.m. | #10
On Wed, Apr 19, 2017 at 02:46:19PM +0000, Gabriele Paoloni wrote:
> > From: Amir Ancel [mailto:amira@mellanox.com]

> > Sent: 18 April 2017 21:18

> > To: David Laight; Gabriele Paoloni; davem@davemloft.net

> > Cc: Catalin Marinas; Will Deacon; Mark Rutland; Robin Murphy;

> > jeffrey.t.kirsher@intel.com; alexander.duyck@gmail.com; linux-arm-

> > kernel@lists.infradead.org; netdev@vger.kernel.org; Dingtianhong;

> > Linuxarm

> > Subject: Re: Re: [PATCH net-next 1/4] ixgbe: sparc: rename the

> > ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER

> > 

> > Hi,

> > mlx5 driver is planned to have RO support this year.

> > I believe drivers should be able to query whether the arch support it

> 

> I guess that here when you say query you mean having a config symbol

> that is set accordingly to the host architecture, right?

> 

> As already said I have looked around a bit and other drivers do not seem

> to enable/disable RO for their EP on the basis of the host architecture.

> So why should mlx5 do it according to the host?

> 

> Also my understating is that some architectures (like ARM64 for example)

> can have different PCI host controller implementations depending on the

> vendor...therefore maybe it is not appropriate there to have a Kconfig

> symbol selected by the architecture...  


Indeed. We're not able to determine whether or not RO is supported at
compile time, so we'd have to detect this dynamically if we want to support
it for arm64 with a single kernel Image. That means either passing something
through firmware, having the PCI host controller opt-in or something coarse
like a command-line option.

Will

Patch hide | download patch | download mbox

diff --git a/arch/Kconfig b/arch/Kconfig
index cd211a1..bc0ab44 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -844,7 +844,7 @@  config STRICT_MODULE_RWX
 	  and non-text memory will be made non-executable. This provides
 	  protection against certain security exploits (e.g. writing to text)
 
-config ARCH_WANT_RELAX_ORDER
+config IXGBE_ALLOW_RELAXED_ORDER
 	bool
 
 source "kernel/gcov/Kconfig"
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 68ac5c7..f56bcf4 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -44,7 +44,7 @@  config SPARC
 	select CPU_NO_EFFICIENT_FFS
 	select HAVE_ARCH_HARDENED_USERCOPY
 	select PROVE_LOCKING_SMALL if PROVE_LOCKING
-	select ARCH_WANT_RELAX_ORDER
+	select IXGBE_ALLOW_RELAXED_ORDER
 
 config SPARC32
 	def_bool !64BIT
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index c38d50c..563ea15 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
@@ -350,7 +350,7 @@  s32 ixgbe_start_hw_gen2(struct ixgbe_hw *hw)
 	}
 	IXGBE_WRITE_FLUSH(hw);
 
-#ifndef CONFIG_ARCH_WANT_RELAX_ORDER
+#ifndef CONFIG_IXGBE_ALLOW_RELAX_ORDER
 	/* Disable relaxed ordering */
 	for (i = 0; i < hw->mac.max_tx_queues; i++) {
 		u32 regval;