[PATCH 1/2] igb: Add macro for size of RETA indirection table

[PATCH 1/2] igb: Add macro for size of RETA indirection table

[Laura Mihaela Vasilescu]

Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
---
 drivers/net/ethernet/intel/igb/igb.h      |    2 ++
 drivers/net/ethernet/intel/igb/igb_main.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 15ea8dc..5a2659b 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -343,6 +343,8 @@ struct hwmon_buff {
 	};
 #endif
 
+#define IGB_RETA_SIZE	128
+
 /* board specific private data structure */
 struct igb_adapter {
 	unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 6a0c1b6..00f5d96 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3127,7 +3127,7 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
 	 * we are generating the results for n and n+2 and then interleaving
 	 * those with the results with n+1 and n+3.
 	 */
-	for (j = 0; j < 32; j++) {
+	for (j = 0; j < IGB_RETA_SIZE / 4; j++) {
 		/* first pass generates n and n+2 */
 		u32 base = ((j * 0x00040004) + 0x00020000) * num_rx_queues;
 		u32 reta = (base & 0x07800780) >> (7 - shift);
-- 
1.7.10.4

[PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Laura Mihaela Vasilescu]

Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
---
 drivers/net/ethernet/intel/igb/igb.h         |    3 +
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   87 ++++++++++++++++++++++++++
 drivers/net/ethernet/intel/igb/igb_main.c    |   28 +++------
 3 files changed, 97 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 5a2659b..c1fae7a 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -446,6 +446,8 @@ struct igb_adapter {
 	struct i2c_algo_bit_data i2c_algo;
 	struct i2c_adapter i2c_adap;
 	struct i2c_client *i2c_client;
+	u32 rss_indir_tbl_init;
+	u8 rss_indir_tbl[IGB_RETA_SIZE];
 };
 
 #define IGB_FLAG_HAS_MSI		(1 << 0)
@@ -482,6 +484,7 @@ extern int igb_up(struct igb_adapter *);
 extern void igb_down(struct igb_adapter *);
 extern void igb_reinit_locked(struct igb_adapter *);
 extern void igb_reset(struct igb_adapter *);
+extern void igb_write_rss_indir_tbl(struct igb_adapter *);
 extern int igb_set_spd_dplx(struct igb_adapter *, u32, u8);
 extern int igb_setup_tx_resources(struct igb_ring *);
 extern int igb_setup_rx_resources(struct igb_ring *);
diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 85fe7b5..3e1e51e 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2771,6 +2771,90 @@ static void igb_ethtool_complete(struct net_device *netdev)
 	pm_runtime_put(&adapter->pdev->dev);
 }
 
+static u32 igb_get_rxfh_indir_size(struct net_device *netdev)
+{
+	return IGB_RETA_SIZE;
+}
+
+static int igb_get_rxfh_indir(struct net_device *netdev, u32 *indir)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	int i;
+
+	for (i = 0; i < IGB_RETA_SIZE; i++)
+		indir[i] = adapter->rss_indir_tbl[i];
+
+	return 0;
+}
+
+void igb_write_rss_indir_tbl(struct igb_adapter *adapter)
+{
+	struct e1000_hw *hw = &adapter->hw;
+	u32 reg = E1000_RETA(0);
+	u32 shift = 0;
+	int i = 0;
+
+	switch (hw->mac.type) {
+	case e1000_82575:
+		shift = 6;
+		break;
+	case e1000_82576:
+		/* 82576 supports 2 RSS queues for SR-IOV */
+		if (adapter->vfs_allocated_count)
+			shift = 3;
+		break;
+	default:
+		break;
+	}
+
+	while (i < IGB_RETA_SIZE) {
+		u32 val = 0;
+		int j;
+
+		for (j = 3; j >= 0; j--) {
+			val <<= 8;
+			val |= adapter->rss_indir_tbl[i + j];
+		}
+
+		wr32(reg, val << shift);
+		reg += 4;
+		i += 4;
+	}
+}
+
+static int igb_set_rxfh_indir(struct net_device *netdev, const u32 *indir)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	int i;
+	u32 num_queues;
+
+	num_queues = adapter->rss_queues;
+
+	switch (hw->mac.type) {
+	case e1000_82576:
+		/* 82576 supports 2 RSS queues for SR-IOV */
+		if (adapter->vfs_allocated_count)
+			num_queues = 2;
+		break;
+	default:
+		break;
+	}
+
+	/* Verify user input. */
+	for (i = 0; i < IGB_RETA_SIZE; i++)
+		if (indir[i] > num_queues)
+			return -EINVAL;
+
+
+	for (i = 0; i < IGB_RETA_SIZE; i++)
+		adapter->rss_indir_tbl[i] = indir[i];
+
+	igb_write_rss_indir_tbl(adapter);
+
+	return 0;
+}
+
 static const struct ethtool_ops igb_ethtool_ops = {
 	.get_settings		= igb_get_settings,
 	.set_settings		= igb_set_settings,
@@ -2804,6 +2888,9 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.set_eee		= igb_set_eee,
 	.get_module_info	= igb_get_module_info,
 	.get_module_eeprom	= igb_get_module_eeprom,
+	.get_rxfh_indir_size	= igb_get_rxfh_indir_size,
+	.get_rxfh_indir		= igb_get_rxfh_indir,
+	.set_rxfh_indir		= igb_set_rxfh_indir,
 	.begin			= igb_ethtool_begin,
 	.complete		= igb_ethtool_complete,
 };
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 00f5d96..600b80d 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3096,7 +3096,7 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
 	u32 mrqc, rxcsum;
-	u32 j, num_rx_queues, shift = 0;
+	u32 j, num_rx_queues;
 	static const u32 rsskey[10] = { 0xDA565A6D, 0xC20E5B25, 0x3D256741,
 					0xB08FA343, 0xCB2BCAD0, 0xB4307BAE,
 					0xA32DCB77, 0x0CF23080, 0x3BB7426A,
@@ -3109,35 +3109,21 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
 	num_rx_queues = adapter->rss_queues;
 
 	switch (hw->mac.type) {
-	case e1000_82575:
-		shift = 6;
-		break;
 	case e1000_82576:
 		/* 82576 supports 2 RSS queues for SR-IOV */
-		if (adapter->vfs_allocated_count) {
-			shift = 3;
+		if (adapter->vfs_allocated_count)
 			num_rx_queues = 2;
-		}
 		break;
 	default:
 		break;
 	}
 
-	/* Populate the indirection table 4 entries at a time.  To do this
-	 * we are generating the results for n and n+2 and then interleaving
-	 * those with the results with n+1 and n+3.
-	 */
-	for (j = 0; j < IGB_RETA_SIZE / 4; j++) {
-		/* first pass generates n and n+2 */
-		u32 base = ((j * 0x00040004) + 0x00020000) * num_rx_queues;
-		u32 reta = (base & 0x07800780) >> (7 - shift);
-
-		/* second pass generates n+1 and n+3 */
-		base += 0x00010001 * num_rx_queues;
-		reta |= (base & 0x07800780) << (1 + shift);
-
-		wr32(E1000_RETA(j), reta);
+	if (adapter->rss_indir_tbl_init != num_rx_queues) {
+		for (j = 0; j < IGB_RETA_SIZE; j++)
+			adapter->rss_indir_tbl[j] = (j * num_rx_queues) / IGB_RETA_SIZE;
+		adapter->rss_indir_tbl_init = num_rx_queues;
 	}
+	igb_write_rss_indir_tbl(adapter);
 
 	/* Disable raw packet checksumming so that RSS hash is placed in
 	 * descriptor on writeback.  No need to enable TCP/UDP/IP checksum
-- 
1.7.10.4

Re: [PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 461 bytes --]

On Wed, 2013-07-31 at 11:43 +0300, Laura Mihaela Vasilescu wrote:
> Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
> ---
>  drivers/net/ethernet/intel/igb/igb.h         |    3 +
>  drivers/net/ethernet/intel/igb/igb_ethtool.c |   87
> ++++++++++++++++++++++++++
>  drivers/net/ethernet/intel/igb/igb_main.c    |   28 +++------
>  3 files changed, 97 insertions(+), 21 deletions(-) 

Not applying, see response to the first patch.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Ben Hutchings]

On Wed, 2013-07-31 at 11:43 +0300, Laura Mihaela Vasilescu wrote:
[...]
> +static int igb_set_rxfh_indir(struct net_device *netdev, const u32 *indir)
> +{
> +	struct igb_adapter *adapter = netdev_priv(netdev);
> +	struct e1000_hw *hw = &adapter->hw;
> +	int i;
> +	u32 num_queues;
> +
> +	num_queues = adapter->rss_queues;
> +
> +	switch (hw->mac.type) {
> +	case e1000_82576:
> +		/* 82576 supports 2 RSS queues for SR-IOV */
> +		if (adapter->vfs_allocated_count)
> +			num_queues = 2;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* Verify user input. */
> +	for (i = 0; i < IGB_RETA_SIZE; i++)
> +		if (indir[i] > num_queues)
> +			return -EINVAL;
[...]

Surely the comparison should be >= ?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/2] igb: Add macro for size of RETA indirection table

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On Wed, 2013-07-31 at 11:43 +0300, Laura Mihaela Vasilescu wrote:
> Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
> ---
>  drivers/net/ethernet/intel/igb/igb.h      |    2 ++
>  drivers/net/ethernet/intel/igb/igb_main.c |    2 +-
>  2 files changed, 3 insertions(+), 1 deletion(-) 

Not sure if I mentioned it in your first submission, if I did not then
my bad. The title of your patch is descriptive of the changes, but it is
always nice to have a detailed patch description.  I noticed your second
patch is similar to the first, in that it does not have a detailed patch
description.

Information that would be good to include in the patch description is:
- why the change or reason for the change
- advantages/disadvantages to the change

Laura please re-submit your two patches with a detailed/meaningful patch
description to at least the second patch (but both would be great).

Think of it this way, if someone were to look at your changes 10 years
from now and did not understand why the change, your patch description
would explain all.

Cheers,
Jeff


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] igb: Add macro for size of RETA indirection table

[Laura Mihaela Vasilescu]

On Wed, Jul 31, 2013 at 12:14 PM, Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
> Laura please re-submit your two patches with a detailed/meaningful patch
> description to at least the second patch (but both would be great).
>
> Think of it this way, if someone were to look at your changes 10 years
> from now and did not understand why the change, your patch description
> would explain all.

I re-submit the patches. I hope there are OK now.

Thanks for the feedback! :)
Laura

Re: [PATCH 1/2] igb: Add macro for size of RETA indirection table

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Wed, 2013-07-31 at 14:50 +0300, Laura Mihaela Vasilescu wrote:
> On Wed, Jul 31, 2013 at 12:14 PM, Jeff Kirsher
> <jeffrey.t.kirsher@intel.com> wrote:
> > Laura please re-submit your two patches with a detailed/meaningful patch
> > description to at least the second patch (but both would be great).
> >
> > Think of it this way, if someone were to look at your changes 10 years
> > from now and did not understand why the change, your patch description
> > would explain all.
> 
> I re-submit the patches. I hope there are OK now.
> 
> Thanks for the feedback! :)
> Laura

Much better, thank you Laura.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/2] igb: Add macro for RETA indirection table

[Laura Mihaela Vasilescu]

Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
---
 drivers/net/ethernet/intel/igb/igb.h      |    2 ++
 drivers/net/ethernet/intel/igb/igb_main.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 15ea8dc..96fdfe2 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -456,6 +456,8 @@ struct igb_adapter {
 #define IGB_FLAG_RSS_FIELD_IPV6_UDP	(1 << 7)
 #define IGB_FLAG_WOL_SUPPORTED		(1 << 8)
 
+#define IGB_RETA_SIZE		32
+
 /* DMA Coalescing defines */
 #define IGB_MIN_TXPBSIZE	20408
 #define IGB_TX_BUF_4096		4096
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 6a0c1b6..09ea855a 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3127,7 +3127,7 @@ static void igb_setup_mrqc(struct igb_adapter *adapter)
 	 * we are generating the results for n and n+2 and then interleaving
 	 * those with the results with n+1 and n+3.
 	 */
-	for (j = 0; j < 32; j++) {
+	for (j = 0; j < IGB_RETA_SIZE; j++) {
 		/* first pass generates n and n+2 */
 		u32 base = ((j * 0x00040004) + 0x00020000) * num_rx_queues;
 		u32 reta = (base & 0x07800780) >> (7 - shift);
-- 
1.7.10.4

[PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Laura Mihaela Vasilescu]

Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   32 ++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index 85fe7b5..7e18dfa 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -2771,6 +2771,35 @@ static void igb_ethtool_complete(struct net_device *netdev)
 	pm_runtime_put(&adapter->pdev->dev);
 }
 
+static u32 igb_get_rss_table_size(struct net_device *netdev)
+{
+	return IGB_RETA_SIZE;
+}
+
+static int igb_get_rss_table(struct net_device *netdev, u32 *rxfh_indir_tbl)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	int i;
+
+	for (i = 0; i < IGB_RETA_SIZE; i++)
+		rxfh_indir_tbl[i] = rd32(E1000_RETA(i));
+
+	return 0;
+}
+
+static int igb_set_rss_table(struct net_device *netdev, const u32 *rxfh_indir_tbl)
+{
+	struct igb_adapter *adapter = netdev_priv(netdev);
+	struct e1000_hw *hw = &adapter->hw;
+	int i;
+
+	for (i = 0; i < IGB_RETA_SIZE; i++)
+		wr32(E1000_RETA(i), rxfh_indir_tbl[i]);
+
+	return 0;
+}
+
 static const struct ethtool_ops igb_ethtool_ops = {
 	.get_settings		= igb_get_settings,
 	.set_settings		= igb_set_settings,
@@ -2804,6 +2833,9 @@ static const struct ethtool_ops igb_ethtool_ops = {
 	.set_eee		= igb_set_eee,
 	.get_module_info	= igb_get_module_info,
 	.get_module_eeprom	= igb_get_module_eeprom,
+	.get_rxfh_indir_size	= igb_get_rss_table_size,
+	.get_rxfh_indir		= igb_get_rss_table,
+	.set_rxfh_indir		= igb_set_rss_table,
 	.begin			= igb_ethtool_begin,
 	.complete		= igb_ethtool_complete,
 };
-- 
1.7.10.4

Re: [PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 316 bytes --]

On Wed, 2013-07-17 at 09:50 +0300, Laura Mihaela Vasilescu wrote:
> Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
> ---
>  drivers/net/ethernet/intel/igb/igb_ethtool.c |   32
> ++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+) 

Added this one as well to my queue, thanks!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Ben Hutchings]

This is wrong, as there are 4 entries packed into each register.

On Wed, 2013-07-17 at 09:50 +0300, Laura Mihaela Vasilescu wrote:
> Signed-off-by: Laura Mihaela Vasilescu <laura.vasilescu@rosedu.org>
> ---
>  drivers/net/ethernet/intel/igb/igb_ethtool.c |   32 ++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 85fe7b5..7e18dfa 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2771,6 +2771,35 @@ static void igb_ethtool_complete(struct net_device *netdev)
>  	pm_runtime_put(&adapter->pdev->dev);
>  }
>  
> +static u32 igb_get_rss_table_size(struct net_device *netdev)
> +{
> +	return IGB_RETA_SIZE;

* 4

> +}
> +
> +static int igb_get_rss_table(struct net_device *netdev, u32 *rxfh_indir_tbl)
> +{
> +	struct igb_adapter *adapter = netdev_priv(netdev);
> +	struct e1000_hw *hw = &adapter->hw;
> +	int i;
> +
> +	for (i = 0; i < IGB_RETA_SIZE; i++)
> +		rxfh_indir_tbl[i] = rd32(E1000_RETA(i));

Unpack each register into entries i*4, i*4+1, i*4+2, i*4+3.

> +
> +	return 0;
> +}
> +
> +static int igb_set_rss_table(struct net_device *netdev, const u32 *rxfh_indir_tbl)
> +{
> +	struct igb_adapter *adapter = netdev_priv(netdev);
> +	struct e1000_hw *hw = &adapter->hw;
> +	int i;
> +
> +	for (i = 0; i < IGB_RETA_SIZE; i++)
> +		wr32(E1000_RETA(i), rxfh_indir_tbl[i]);

Pack entries i*4, i*4+1, i*4+2, i*4+3 into each register (as in
igb_setup_mrqc()).

> +
> +	return 0;
> +}

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 2/2] igb: Expose RSS indirection table for ethtool

[Ben Hutchings]

On Wed, 2013-07-17 at 17:52 +0100, Ben Hutchings wrote:
[...]
> > +static int igb_get_rss_table(struct net_device *netdev, u32 *rxfh_indir_tbl)
> > +{
> > +	struct igb_adapter *adapter = netdev_priv(netdev);
> > +	struct e1000_hw *hw = &adapter->hw;
> > +	int i;
> > +
> > +	for (i = 0; i < IGB_RETA_SIZE; i++)
> > +		rxfh_indir_tbl[i] = rd32(E1000_RETA(i));
> 
> Unpack each register into entries i*4, i*4+1, i*4+2, i*4+3.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int igb_set_rss_table(struct net_device *netdev, const u32 *rxfh_indir_tbl)
> > +{
> > +	struct igb_adapter *adapter = netdev_priv(netdev);
> > +	struct e1000_hw *hw = &adapter->hw;
> > +	int i;
> > +
> > +	for (i = 0; i < IGB_RETA_SIZE; i++)
> > +		wr32(E1000_RETA(i), rxfh_indir_tbl[i]);
> 
> Pack entries i*4, i*4+1, i*4+2, i*4+3 into each register (as in
> igb_setup_mrqc()).
[...]

Also, the indirection table should be preserved in the driver across
hardware resets, although it may need to be reset if the number of RX
queues changes.

Commit 90415477bf13 'tg3: Make the RSS indir tbl admin configurable' is
a good example of how to do this properly.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.