* Re: [patch] isdn: return -EFAULT if copy_from_user() fails
From: Nicolas Kaiser @ 2010-12-11 0:41 UTC (permalink / raw)
To: Dan Carpenter; +Cc: David S. Miller, Karsten Keil, netdev, kernel-janitors
In-Reply-To: <20101210124009.GC10623@bicker>
Ahem, we're printing this return value:
* Dan Carpenter <error27@gmail.com>:
> - if ((ret = copy_from_user(&size, p, sizeof(int)))) {
^^^
> + if (copy_from_user(&size, p, sizeof(int))) {
> printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
^^^
> - return ret;
> + return -EFAULT;
Cheers,
Nicolas Kaiser
^ permalink raw reply
* Re: [patch] isdn: return -EFAULT if copy_from_user() fails
From: David Miller @ 2010-12-11 0:47 UTC (permalink / raw)
To: nikai; +Cc: error27, isdn, netdev, kernel-janitors
In-Reply-To: <20101211014154.3157588b@absol.kitzblitz>
From: Nicolas Kaiser <nikai@nikai.net>
Date: Sat, 11 Dec 2010 01:41:54 +0100
> Ahem, we're printing this return value:
>
> * Dan Carpenter <error27@gmail.com>:
>> - if ((ret = copy_from_user(&size, p, sizeof(int)))) {
> ^^^
>> + if (copy_from_user(&size, p, sizeof(int))) {
>> printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
> ^^^
>> - return ret;
>> + return -EFAULT;
I'll fix this, thanks.
^ permalink raw reply
* Re: [patch] isdn: return -EFAULT if copy_from_user() fails
From: David Miller @ 2010-12-11 0:50 UTC (permalink / raw)
To: nikai; +Cc: error27, isdn, netdev, kernel-janitors
In-Reply-To: <20101210.164721.85429801.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Fri, 10 Dec 2010 16:47:21 -0800 (PST)
> From: Nicolas Kaiser <nikai@nikai.net>
> Date: Sat, 11 Dec 2010 01:41:54 +0100
>
>> Ahem, we're printing this return value:
>>
>> * Dan Carpenter <error27@gmail.com>:
>>> - if ((ret = copy_from_user(&size, p, sizeof(int)))) {
>> ^^^
>>> + if (copy_from_user(&size, p, sizeof(int))) {
>>> printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
>> ^^^
>>> - return ret;
>>> + return -EFAULT;
>
> I'll fix this, thanks.
As follows:
>From cf108fdd482e80161128c2ed01e7f4fb5bc728b9 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@davemloft.net>
Date: Fri, 10 Dec 2010 16:49:24 -0800
Subject: [PATCH] isdn: Fix printed out copy_from_user() return value after previous change.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
drivers/isdn/hisax/isar.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/hisax/isar.c b/drivers/isdn/hisax/isar.c
index c9cd4d2..313ce2d 100644
--- a/drivers/isdn/hisax/isar.c
+++ b/drivers/isdn/hisax/isar.c
@@ -189,7 +189,7 @@ ISARVersion(struct IsdnCardState *cs, char *s)
static int
isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
{
- int ret, size, cnt, debug;
+ int cfu_ret, size, cnt, debug;
u_char len, nom, noc;
u_short sadr, left, *sp;
u_char __user *p = buf;
@@ -212,8 +212,9 @@ isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
cs->debug &= ~(L1_DEB_HSCX | L1_DEB_HSCX_FIFO);
#endif
- if (copy_from_user(&size, p, sizeof(int))) {
- printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
+ cfu_ret = copy_from_user(&size, p, sizeof(int));
+ if (cfu_ret) {
+ printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", cfu_ret);
return -EFAULT;
}
p += sizeof(int);
--
1.7.3.2
^ permalink raw reply related
* Re: [patch] isdn: return -EFAULT if copy_from_user() fails
From: David Miller @ 2010-12-11 0:52 UTC (permalink / raw)
To: nikai; +Cc: error27, isdn, netdev, kernel-janitors
In-Reply-To: <20101210.165003.267942493.davem@davemloft.net>
From: David Miller <davem@davemloft.net>
Date: Fri, 10 Dec 2010 16:50:03 -0800 (PST)
> From: David Miller <davem@davemloft.net>
> Date: Fri, 10 Dec 2010 16:47:21 -0800 (PST)
>
>> From: Nicolas Kaiser <nikai@nikai.net>
>> Date: Sat, 11 Dec 2010 01:41:54 +0100
>>
>>> Ahem, we're printing this return value:
>>>
>>> * Dan Carpenter <error27@gmail.com>:
>>>> - if ((ret = copy_from_user(&size, p, sizeof(int)))) {
>>> ^^^
>>>> + if (copy_from_user(&size, p, sizeof(int))) {
>>>> printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
>>> ^^^
>>>> - return ret;
>>>> + return -EFAULT;
>>
>> I'll fix this, thanks.
>
> As follows:
Ugh, that was buggy, let's try this :-)
--------------------
isdn: Fix printed out copy_from_user() return value after previous change.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
drivers/isdn/hisax/isar.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/isdn/hisax/isar.c b/drivers/isdn/hisax/isar.c
index c9cd4d2..d4cce33 100644
--- a/drivers/isdn/hisax/isar.c
+++ b/drivers/isdn/hisax/isar.c
@@ -189,7 +189,7 @@ ISARVersion(struct IsdnCardState *cs, char *s)
static int
isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
{
- int ret, size, cnt, debug;
+ int cfu_ret, ret, size, cnt, debug;
u_char len, nom, noc;
u_short sadr, left, *sp;
u_char __user *p = buf;
@@ -212,8 +212,9 @@ isar_load_firmware(struct IsdnCardState *cs, u_char __user *buf)
cs->debug &= ~(L1_DEB_HSCX | L1_DEB_HSCX_FIFO);
#endif
- if (copy_from_user(&size, p, sizeof(int))) {
- printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", ret);
+ cfu_ret = copy_from_user(&size, p, sizeof(int));
+ if (cfu_ret) {
+ printk(KERN_ERR"isar_load_firmware copy_from_user ret %d\n", cfu_ret);
return -EFAULT;
}
p += sizeof(int);
--
1.7.3.2
^ permalink raw reply related
* Re: [PATCH 2/7] vxge: fix crash of VF when unloading PF
From: Chris Wright @ 2010-12-11 1:04 UTC (permalink / raw)
To: Jon Mason
Cc: David S. Miller, netdev, Sivakumar Subramani, Sreenivasa Honnur,
Ram Vepa
In-Reply-To: <1292025782-16372-2-git-send-email-jon.mason@exar.com>
* Jon Mason (jon.mason@exar.com) wrote:
> +static int __devinit is_sriov_initialized(struct pci_dev *pdev)
> +{
> + int pos;
> + u16 ctrl;
> +
> + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_SRIOV);
> + if (pos) {
> + pci_read_config_word(pdev, pos + PCI_SRIOV_CTRL, &ctrl);
> + if (ctrl & PCI_SRIOV_CTRL_VFE)
> + return 1;
> + }
> + return 0;
> +}
This is a helper that should go in drivers/pci/iov.c (it's a pure
pci thing).
> +
> /**
> * vxge_probe
> * @pdev : structure containing the PCI related information of the device.
> @@ -4370,14 +4384,13 @@ vxge_probe(struct pci_dev *pdev, const struct pci_device_id *pre)
> num_vfs = vxge_get_num_vfs(function_mode) - 1;
>
> /* Enable SRIOV mode, if firmware has SRIOV support and if it is a PF */
> - if (is_sriov(function_mode) && (max_config_dev > 1) &&
> - (ll_config->intr_type != INTA) &&
> - (is_privileged == VXGE_HW_OK)) {
> - ret = pci_enable_sriov(pdev, ((max_config_dev - 1) < num_vfs)
> - ? (max_config_dev - 1) : num_vfs);
> + if (is_sriov(function_mode) && !is_sriov_initialized(pdev) &&
> + (ll_config->intr_type != INTA)) {
> + ret = pci_enable_sriov(pdev, num_vfs);
This fundamentally changes the way VF's are allocated. Now you cannot
specifiy the number of vfs to allocate with max_config_dev module
parameter.
> if (ret)
> vxge_debug_ll_config(VXGE_ERR,
> "Failed in enabling SRIOV mode: %d\n", ret);
> + /* No need to fail out, as an error here is non-fatal */
> }
>
> /*
> @@ -4673,8 +4686,6 @@ static void __devexit vxge_remove(struct pci_dev *pdev)
>
> iounmap(vdev->bar0);
>
> - pci_disable_sriov(pdev);
> -
And you can never disable sriov.
This doesn't look like the right behaviour.
thanks,
-chris
^ permalink raw reply
* RE: [PATCH 2/7] vxge: fix crash of VF when unloading PF
From: Ramkrishna Vepa @ 2010-12-11 1:35 UTC (permalink / raw)
To: Chris Wright, Jon Mason
Cc: David S. Miller, netdev@vger.kernel.org, Sivakumar Subramani,
Sreenivasa Honnur
In-Reply-To: <20101211010441.GI4040@sequoia.sous-sol.org>
> > +
> > /**
> > * vxge_probe
> > * @pdev : structure containing the PCI related information of the
> device.
> > @@ -4370,14 +4384,13 @@ vxge_probe(struct pci_dev *pdev, const struct
> pci_device_id *pre)
> > num_vfs = vxge_get_num_vfs(function_mode) - 1;
> >
> > /* Enable SRIOV mode, if firmware has SRIOV support and if it is a
> PF */
> > - if (is_sriov(function_mode) && (max_config_dev > 1) &&
> > - (ll_config->intr_type != INTA) &&
> > - (is_privileged == VXGE_HW_OK)) {
> > - ret = pci_enable_sriov(pdev, ((max_config_dev - 1) < num_vfs)
> > - ? (max_config_dev - 1) : num_vfs);
> > + if (is_sriov(function_mode) && !is_sriov_initialized(pdev) &&
> > + (ll_config->intr_type != INTA)) {
> > + ret = pci_enable_sriov(pdev, num_vfs);
>
> This fundamentally changes the way VF's are allocated. Now you cannot
> specifiy the number of vfs to allocate with max_config_dev module
> parameter.
The X3100 supports 11 different pci function modes where the user has the ability to choose the number of functions for each mode. This is more efficient usage of the hardware as the resources are carved out equally for the functions. By configuring the max_config_dev less than num_vfs, there's unnecessary wastage of resources.
>
> > if (ret)
> > vxge_debug_ll_config(VXGE_ERR,
> > "Failed in enabling SRIOV mode: %d\n", ret);
> > + /* No need to fail out, as an error here is non-fatal */
> > }
> >
> > /*
> > @@ -4673,8 +4686,6 @@ static void __devexit vxge_remove(struct pci_dev
> *pdev)
> >
> > iounmap(vdev->bar0);
> >
> > - pci_disable_sriov(pdev);
> > -
>
> And you can never disable sriov.
If the device's pci function mode is changed, a power cycle is required in which case the functions are re-enumerated.
>
> This doesn't look like the right behaviour.
When the driver is loaded for the X3100 in SRIOV mode, it will be working in that mode even after it is unloaded and reloaded. As mentioned earlier, a change in the function mode requires and power cycle of the system.
The SRIOV feature is shipping in many distros and we need this fix back ported to prevent a possible crash when the PF is unloaded while the VFs are running in the guest OS in pass through mode.
If you have a better or simpler solution, that may take longer to implement, I would suggest that this solution be accepted in the interim.
Thanks,
Ram
The information and any attached documents contained in this message
may be confidential and/or legally privileged. The message is
intended solely for the addressee(s). If you are not the intended
recipient, you are hereby notified that any use, dissemination, or
reproduction is strictly prohibited and may be unlawful. If you are
not the intended recipient, please contact the sender immediately by
return e-mail and destroy all copies of the original message.
^ permalink raw reply
* [PATCH 1/4] s2io: rx_ring_sz bounds checking
From: Jon Mason @ 2010-12-11 1:40 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Sivakumar Subramani, Sreenivasa Honnur, Ram Vepa
modparm rx_ring_sz can be set to be greater than the maximum allowable
number of blocks. This results in an array overrun when probing the
driver, and causes memory corruption.
Also, the MAX_RX_DESC_1 multiply the max number of rings by max number
of blocker per ring by 127, but the driver does the same calculation
with 127+1. This results in the possibility of the value being set
being larger than the maximum allowable value.
Finally, clean-up the s2io_ethtool_gringparam code to be more
intuitive.
Signed-off-by: Jon Mason <jon.mason@exar.com>
---
drivers/net/s2io.c | 40 ++++++++++++++++++++++++----------------
drivers/net/s2io.h | 5 ++---
2 files changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 0f4219c..a6d3eaf 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -5568,30 +5568,27 @@ static void s2io_ethtool_gringparam(struct net_device *dev,
struct s2io_nic *sp = netdev_priv(dev);
int i, tx_desc_count = 0, rx_desc_count = 0;
- if (sp->rxd_mode == RXD_MODE_1)
+ if (sp->rxd_mode == RXD_MODE_1) {
ering->rx_max_pending = MAX_RX_DESC_1;
- else if (sp->rxd_mode == RXD_MODE_3B)
+ ering->rx_jumbo_max_pending = MAX_RX_DESC_1;
+ } else {
ering->rx_max_pending = MAX_RX_DESC_2;
+ ering->rx_jumbo_max_pending = MAX_RX_DESC_2;
+ }
+ ering->rx_mini_max_pending = 0;
ering->tx_max_pending = MAX_TX_DESC;
- for (i = 0 ; i < sp->config.tx_fifo_num ; i++)
- tx_desc_count += sp->config.tx_cfg[i].fifo_len;
- DBG_PRINT(INFO_DBG, "max txds: %d\n", sp->config.max_txds);
- ering->tx_pending = tx_desc_count;
- rx_desc_count = 0;
- for (i = 0 ; i < sp->config.rx_ring_num ; i++)
+ for (i = 0; i < sp->config.rx_ring_num; i++)
rx_desc_count += sp->config.rx_cfg[i].num_rxd;
-
ering->rx_pending = rx_desc_count;
-
- ering->rx_mini_max_pending = 0;
- ering->rx_mini_pending = 0;
- if (sp->rxd_mode == RXD_MODE_1)
- ering->rx_jumbo_max_pending = MAX_RX_DESC_1;
- else if (sp->rxd_mode == RXD_MODE_3B)
- ering->rx_jumbo_max_pending = MAX_RX_DESC_2;
ering->rx_jumbo_pending = rx_desc_count;
+ ering->rx_mini_pending = 0;
+
+ for (i = 0; i < sp->config.tx_fifo_num; i++)
+ tx_desc_count += sp->config.tx_cfg[i].fifo_len;
+ ering->tx_pending = tx_desc_count;
+ DBG_PRINT(INFO_DBG, "max txds: %d\n", sp->config.max_txds);
}
/**
@@ -7692,6 +7689,8 @@ static void s2io_init_pci(struct s2io_nic *sp)
static int s2io_verify_parm(struct pci_dev *pdev, u8 *dev_intr_type,
u8 *dev_multiq)
{
+ int i;
+
if ((tx_fifo_num > MAX_TX_FIFOS) || (tx_fifo_num < 1)) {
DBG_PRINT(ERR_DBG, "Requested number of tx fifos "
"(%d) not supported\n", tx_fifo_num);
@@ -7750,6 +7749,15 @@ static int s2io_verify_parm(struct pci_dev *pdev, u8 *dev_intr_type,
DBG_PRINT(ERR_DBG, "Defaulting to 1-buffer mode\n");
rx_ring_mode = 1;
}
+
+ for (i = 0; i < MAX_RX_RINGS; i++)
+ if (rx_ring_sz[i] > MAX_RX_BLOCKS_PER_RING) {
+ DBG_PRINT(ERR_DBG, "Requested rx ring size not "
+ "supported\nDefaulting to %d\n",
+ MAX_RX_BLOCKS_PER_RING);
+ rx_ring_sz[i] = MAX_RX_BLOCKS_PER_RING;
+ }
+
return SUCCESS;
}
diff --git a/drivers/net/s2io.h b/drivers/net/s2io.h
index 00b8614..1671443 100644
--- a/drivers/net/s2io.h
+++ b/drivers/net/s2io.h
@@ -355,9 +355,8 @@ struct stat_block {
#define FIFO_OTHER_MAX_NUM 1
-#define MAX_RX_DESC_1 (MAX_RX_RINGS * MAX_RX_BLOCKS_PER_RING * 127 )
-#define MAX_RX_DESC_2 (MAX_RX_RINGS * MAX_RX_BLOCKS_PER_RING * 85 )
-#define MAX_RX_DESC_3 (MAX_RX_RINGS * MAX_RX_BLOCKS_PER_RING * 85 )
+#define MAX_RX_DESC_1 (MAX_RX_RINGS * MAX_RX_BLOCKS_PER_RING * 128)
+#define MAX_RX_DESC_2 (MAX_RX_RINGS * MAX_RX_BLOCKS_PER_RING * 86)
#define MAX_TX_DESC (MAX_AVAILABLE_TXDS)
/* FIFO mappings for all possible number of fifos configured */
--
1.7.0.4
^ permalink raw reply related
* [PATCH 2/4] s2io: make strings at tables const
From: Jon Mason @ 2010-12-11 1:40 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Sivakumar Subramani, Sreenivasa Honnur, Ram Vepa,
Stephen Hemminger
In-Reply-To: <1292031604-16628-1-git-send-email-jon.mason@exar.com>
Put immutable data in read/only section.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
Signed-off-by: Jon Mason <jon.mason@exar.com>
---
drivers/net/s2io.c | 8 ++++----
drivers/net/s2io.h | 4 ++--
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index a6d3eaf..6a87b8c 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -91,11 +91,11 @@
#define DRV_VERSION "2.0.26.27"
/* S2io Driver name & version. */
-static char s2io_driver_name[] = "Neterion";
-static char s2io_driver_version[] = DRV_VERSION;
+static const char s2io_driver_name[] = "Neterion";
+static const char s2io_driver_version[] = DRV_VERSION;
-static int rxd_size[2] = {32, 48};
-static int rxd_count[2] = {127, 85};
+static const int rxd_size[2] = {32, 48};
+static const int rxd_count[2] = {127, 85};
static inline int RXD_IS_UP2DT(struct RxD_t *rxdp)
{
diff --git a/drivers/net/s2io.h b/drivers/net/s2io.h
index 1671443..7d16030 100644
--- a/drivers/net/s2io.h
+++ b/drivers/net/s2io.h
@@ -360,7 +360,7 @@ struct stat_block {
#define MAX_TX_DESC (MAX_AVAILABLE_TXDS)
/* FIFO mappings for all possible number of fifos configured */
-static int fifo_map[][MAX_TX_FIFOS] = {
+static const int fifo_map[][MAX_TX_FIFOS] = {
{0, 0, 0, 0, 0, 0, 0, 0},
{0, 0, 0, 0, 1, 1, 1, 1},
{0, 0, 0, 1, 1, 1, 2, 2},
@@ -371,7 +371,7 @@ static int fifo_map[][MAX_TX_FIFOS] = {
{0, 1, 2, 3, 4, 5, 6, 7},
};
-static u16 fifo_selector[MAX_TX_FIFOS] = {0, 1, 3, 3, 7, 7, 7, 7};
+static const u16 fifo_selector[MAX_TX_FIFOS] = {0, 1, 3, 3, 7, 7, 7, 7};
/* Maintains Per FIFO related information. */
struct tx_fifo_config {
--
1.7.0.4
^ permalink raw reply related
* [PATCH 3/4] s2io: Update Driver Version
From: Jon Mason @ 2010-12-11 1:40 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Sivakumar Subramani, Sreenivasa Honnur, Ram Vepa
In-Reply-To: <1292031604-16628-1-git-send-email-jon.mason@exar.com>
Update Driver Version
Signed-off-by: Jon Mason <jon.mason@exar.com>
---
drivers/net/s2io.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 6a87b8c..80efc05 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -88,7 +88,7 @@
#include "s2io.h"
#include "s2io-regs.h"
-#define DRV_VERSION "2.0.26.27"
+#define DRV_VERSION "2.0.26.28"
/* S2io Driver name & version. */
static const char s2io_driver_name[] = "Neterion";
--
1.7.0.4
^ permalink raw reply related
* [PATCH 4/4] Using static const generally increases object text and decreases data size. It also generally decreases overall object size.
From: Jon Mason @ 2010-12-11 1:40 UTC (permalink / raw)
To: David S. Miller
Cc: netdev, Sivakumar Subramani, Sreenivasa Honnur, Ram Vepa,
Joe Perches
In-Reply-To: <1292031604-16628-1-git-send-email-jon.mason@exar.com>
text data bss dec hex filename
109387 389 24432 134208 20c40 drivers/net/s2io.o.old
109358 389 24432 134179 20c23 drivers/net/s2io.o.new
Signed-off-by: Joe Perches <joe@perches.com>
Acked-by: Jon Mason <jon.mason@exar.com>
---
drivers/net/s2io.c | 20 ++++++++++++--------
1 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/net/s2io.c b/drivers/net/s2io.c
index 80efc05..9a1e32f 100644
--- a/drivers/net/s2io.c
+++ b/drivers/net/s2io.c
@@ -3598,10 +3598,12 @@ static int s2io_set_swapper(struct s2io_nic *sp)
val64 = readq(&bar0->pif_rd_swapper_fb);
if (val64 != 0x0123456789ABCDEFULL) {
int i = 0;
- u64 value[] = { 0xC30000C3C30000C3ULL, /* FE=1, SE=1 */
- 0x8100008181000081ULL, /* FE=1, SE=0 */
- 0x4200004242000042ULL, /* FE=0, SE=1 */
- 0}; /* FE=0, SE=0 */
+ static const u64 value[] = {
+ 0xC30000C3C30000C3ULL, /* FE=1, SE=1 */
+ 0x8100008181000081ULL, /* FE=1, SE=0 */
+ 0x4200004242000042ULL, /* FE=0, SE=1 */
+ 0 /* FE=0, SE=0 */
+ };
while (i < 4) {
writeq(value[i], &bar0->swapper_ctrl);
@@ -3627,10 +3629,12 @@ static int s2io_set_swapper(struct s2io_nic *sp)
if (val64 != valt) {
int i = 0;
- u64 value[] = { 0x00C3C30000C3C300ULL, /* FE=1, SE=1 */
- 0x0081810000818100ULL, /* FE=1, SE=0 */
- 0x0042420000424200ULL, /* FE=0, SE=1 */
- 0}; /* FE=0, SE=0 */
+ static const u64 value[] = {
+ 0x00C3C30000C3C300ULL, /* FE=1, SE=1 */
+ 0x0081810000818100ULL, /* FE=1, SE=0 */
+ 0x0042420000424200ULL, /* FE=0, SE=1 */
+ 0 /* FE=0, SE=0 */
+ };
while (i < 4) {
writeq((value[i] | valr), &bar0->swapper_ctrl);
--
1.7.0.4
^ permalink raw reply related
* [PATCH] rfc: ethtool: early-orphan control
From: Simon Horman @ 2010-12-11 4:13 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, Ben Hutchings, Simon Horman
Early orphaning is an optimisation which avoids unnecessary cache misses by
orphaning an skb just before it is handed to a device for transmit thus
avoiding the case where the orphaning occurs on a different CPU.
In the case of bonded devices this has the unfortunate side-effect of
breaking down flow control allowing a socket to send UDP packets as fast as
the CPU will allow. This is particularly undesirable in virtualised
network environments.
This patch introduces ethtool control of early orphaning.
It remains on by default by it now may be disabled on a per-interface basis.
I have implemented this as a generic flag.
As it seems to be the first generic flag that requires
no driver awareness I also supplied a default flag handler.
I am unsure if any aspect of this approach is acceptable.
I believe Eric has it in mind that some of the calls
to skb_orphan() in drivers can be removed with the addition
of this feature. I need to discuss that with him further.
A patch for the ethtool user-space utility accompanies this patch.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
include/linux/ethtool.h | 1 +
include/linux/netdevice.h | 1 +
net/core/dev.c | 10 +++++++---
net/core/ethtool.c | 16 +++++++++++++---
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 1908929..e444d1e 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -314,6 +314,7 @@ enum ethtool_flags {
ETH_FLAG_LRO = (1 << 15), /* LRO is enabled */
ETH_FLAG_NTUPLE = (1 << 27), /* N-tuple filters enabled */
ETH_FLAG_RXHASH = (1 << 28),
+ ETH_FLAG_EARLY_ORPHAN = (1 << 29),
};
/* The following structures are for supporting RX network flow
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index d31bc3c..4aa85d6 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -905,6 +905,7 @@ struct net_device {
#define NETIF_F_FCOE_MTU (1 << 26) /* Supports max FCoE MTU, 2158 bytes*/
#define NETIF_F_NTUPLE (1 << 27) /* N-tuple filters supported */
#define NETIF_F_RXHASH (1 << 28) /* Receive hashing offload */
+#define NETIF_F_EARLY_ORPHAN (1 << 29) /* Early Orphaning of skbs */
/* Segmentation offload features */
#define NETIF_F_GSO_SHIFT 16
diff --git a/net/core/dev.c b/net/core/dev.c
index d28b3a0..39e8c38 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1958,11 +1958,12 @@ static int dev_gso_segment(struct sk_buff *skb)
* We cannot orphan skb if tx timestamp is requested or the sk-reference
* is needed on driver level for other reasons, e.g. see net/can/raw.c
*/
-static inline void skb_orphan_try(struct sk_buff *skb)
+static inline void skb_orphan_try(struct sk_buff *skb, struct net_device *dev)
{
struct sock *sk = skb->sk;
- if (sk && !skb_shinfo(skb)->tx_flags) {
+ if (dev->features & NETIF_F_EARLY_ORPHAN &&
+ sk && !skb_shinfo(skb)->tx_flags) {
/* skb_tx_hash() wont be able to get sk.
* We copy sk_hash into skb->rxhash
*/
@@ -2032,7 +2033,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
if (!list_empty(&ptype_all))
dev_queue_xmit_nit(skb, dev);
- skb_orphan_try(skb);
+ skb_orphan_try(skb, dev);
if (vlan_tx_tag_present(skb) &&
!(dev->features & NETIF_F_HW_VLAN_TX)) {
@@ -5216,6 +5217,9 @@ int register_netdevice(struct net_device *dev)
if (dev->features & NETIF_F_SG)
dev->features |= NETIF_F_GSO;
+ /* Enable early orphaning - everything supports it */
+ dev->features |= NETIF_F_EARLY_ORPHAN;
+
/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,
* vlan_dev_init() will do the dev->features check, so these features
* are enabled only if supported by underlying device.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 1774178..f63bdce 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -133,7 +133,7 @@ EXPORT_SYMBOL(ethtool_op_set_ufo);
*/
static const u32 flags_dup_features =
(ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | ETH_FLAG_NTUPLE |
- ETH_FLAG_RXHASH);
+ ETH_FLAG_RXHASH | ETH_FLAG_EARLY_ORPHAN);
u32 ethtool_op_get_flags(struct net_device *dev)
{
@@ -148,7 +148,8 @@ EXPORT_SYMBOL(ethtool_op_get_flags);
int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
{
- if (data & ~supported)
+ /* Everything supports early orphan */
+ if (data & ~(supported | NETIF_F_EARLY_ORPHAN))
return -EINVAL;
dev->features = ((dev->features & ~flags_dup_features) |
@@ -157,6 +158,13 @@ int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
}
EXPORT_SYMBOL(ethtool_op_set_flags);
+static int ethtool_op_set_flags_early_orphan(struct net_device *dev, u32 data)
+{
+ dev->features = ((dev->features & ~NETIF_F_EARLY_ORPHAN) |
+ (data & NETIF_F_EARLY_ORPHAN));
+ return 0;
+}
+
void ethtool_ntuple_flush(struct net_device *dev)
{
struct ethtool_rx_ntuple_flow_spec_container *fsc, *f;
@@ -1644,7 +1652,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
break;
case ETHTOOL_SFLAGS:
rc = ethtool_set_value(dev, useraddr,
- dev->ethtool_ops->set_flags);
+ dev->ethtool_ops->set_flags ?
+ dev->ethtool_ops->set_flags :
+ ethtool_op_set_flags_early_orphan);
break;
case ETHTOOL_GPFLAGS:
rc = ethtool_get_value(dev, useraddr, ethcmd,
--
1.7.2.3
^ permalink raw reply related
* [PATCH] rfc: ethtool: early-orphan control (user-space)
From: Simon Horman @ 2010-12-11 4:13 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, Ben Hutchings, Simon Horman
Early orphaning is an optimisation which avoids unnecessary cache misses by
orphaning an skb just before it is handed to a device for transmit thus
avoiding the case where the orphaning occurs on a different CPU.
In the case of bonded devices this has the unfortunate side-effect of
breaking down flow control allowing a socket to send UDP packets as fast as
the CPU will allow. This is particularly undesirable in virtualised
network environments.
This patch introduces ethtool control of early orphaning.
It remains on by default by it now may be disabled on a per-interface basis.
I have implemented this as a generic option using a generic flag.
I am unsure if any aspect of this approach is acceptable.
A patch for the kernel accompanies this patch.
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
ethtool-copy.h | 1 +
ethtool.8 | 1 +
ethtool.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/ethtool-copy.h b/ethtool-copy.h
index 75c3ae7..9ed758f 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -314,6 +314,7 @@ enum ethtool_flags {
ETH_FLAG_LRO = (1 << 15), /* LRO is enabled */
ETH_FLAG_NTUPLE = (1 << 27), /* N-tuple filters enabled */
ETH_FLAG_RXHASH = (1 << 28),
+ ETH_FLAG_EARLY_ORPHAN = (1 << 29),
};
/* The following structures are for supporting RX network flow
diff --git a/ethtool.8 b/ethtool.8
index 1760924..6384681 100644
--- a/ethtool.8
+++ b/ethtool.8
@@ -209,6 +209,7 @@ ethtool \- Display or change ethernet card settings
.BI msglvl \ type
.A1 on off
.RB ...]
+.B2 early-orphan on off
.B ethtool \-n
.I ethX
diff --git a/ethtool.c b/ethtool.c
index 239912b..579b4e4 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -163,7 +163,8 @@ static struct option {
" [ xcvr internal|external ]\n"
" [ wol p|u|m|b|a|g|s|d... ]\n"
" [ sopass %x:%x:%x:%x:%x:%x ]\n"
- " [ msglvl %d | msglvl type on|off ... ]\n" },
+ " [ msglvl %d | msglvl type on|off ... ]\n"
+ " [ early-orphan on|off ]\n" },
{ "-a", "--show-pause", MODE_GPAUSE, "Show pause options" },
{ "-A", "--pause", MODE_SPAUSE, "Set pause options",
" [ autoneg on|off ]\n"
@@ -411,6 +412,10 @@ static int msglvl_changed;
static u32 msglvl_wanted = 0;
static u32 msglvl_mask = 0;
+static u32 generic_flags_changed;
+static u32 generic_flags_wanted = 0;
+static u32 generic_flags_mask = 0;
+
static enum {
ONLINE=0,
OFFLINE,
@@ -608,6 +613,11 @@ static struct cmdline_info cmdline_msglvl[] = {
NETIF_MSG_WOL, &msglvl_mask },
};
+static struct cmdline_info cmdline_generic_flags[] = {
+ { "early-orphan", CMDL_FLAG, &generic_flags_wanted, NULL,
+ ETH_FLAG_EARLY_ORPHAN, &generic_flags_mask },
+};
+
static long long
get_int_range(char *str, int base, long long min, long long max)
{
@@ -1125,7 +1135,13 @@ static void parse_cmdline(int argc, char **argp)
}
break;
}
- show_usage(1);
+ parse_generic_cmdline(
+ argc, argp, i,
+ &generic_flags_changed,
+ cmdline_generic_flags,
+ ARRAY_SIZE(cmdline_generic_flags));
+ i = argc;
+ break;
}
}
@@ -2498,6 +2514,18 @@ static int do_gset(int fd, struct ifreq *ifr)
perror("Cannot get link status");
}
+ edata.cmd = ETHTOOL_GFLAGS;
+ ifr->ifr_data = (caddr_t)&edata;
+ err = ioctl(fd, SIOCETHTOOL, ifr);
+ if (err) {
+ perror("Cannot get device flags");
+ } else {
+ int eo = (edata.data & ETH_FLAG_EARLY_ORPHAN) != 0;
+ fprintf(stdout, " Early Orphan: %s\n",
+ eo ? "on" : "off");
+ allfail = 0;
+ }
+
if (allfail) {
fprintf(stdout, "No data available\n");
return 75;
@@ -2623,6 +2651,28 @@ static int do_sset(int fd, struct ifreq *ifr)
}
}
+ if (generic_flags_changed) {
+ printf("generic flags changed\n");
+ struct ethtool_value edata;
+
+ edata.cmd = ETHTOOL_GFLAGS;
+ edata.data = 0;
+ ifr->ifr_data = (caddr_t)&edata;
+ err = ioctl(fd, SIOCETHTOOL, ifr);
+ if (err) {
+ perror("Cannot get device flag settings");
+ return 91;
+ }
+
+ edata.cmd = ETHTOOL_SFLAGS;
+ edata.data = ((edata.data & ~generic_flags_mask) |
+ generic_flags_wanted);
+
+ err = ioctl(fd, SIOCETHTOOL, ifr);
+ if (err)
+ perror("Cannot set device flag settings");
+ }
+
return 0;
}
--
1.7.2.3
^ permalink raw reply related
* Re: [PATCH] rfc: ethtool: early-orphan control
From: Simon Horman @ 2010-12-11 4:24 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet, Ben Hutchings
In-Reply-To: <1292040815-10439-1-git-send-email-horms@verge.net.au>
On Sat, Dec 11, 2010 at 01:13:35PM +0900, Simon Horman wrote:
> Early orphaning is an optimisation which avoids unnecessary cache misses by
> orphaning an skb just before it is handed to a device for transmit thus
> avoiding the case where the orphaning occurs on a different CPU.
>
> In the case of bonded devices this has the unfortunate side-effect of
> breaking down flow control allowing a socket to send UDP packets as fast as
> the CPU will allow. This is particularly undesirable in virtualised
> network environments.
>
> This patch introduces ethtool control of early orphaning.
> It remains on by default by it now may be disabled on a per-interface basis.
>
> I have implemented this as a generic flag.
> As it seems to be the first generic flag that requires
> no driver awareness I also supplied a default flag handler.
> I am unsure if any aspect of this approach is acceptable.
>
> I believe Eric has it in mind that some of the calls
> to skb_orphan() in drivers can be removed with the addition
> of this feature. I need to discuss that with him further.
>
> A patch for the ethtool user-space utility accompanies this patch.
The following results were measured using kvm using virto without vhost net.
The virtio device is bridged to a bond device which has one gigabit slave.
bonding device with early-orphan on (default, current behaviour since 2.6.35)
# netperf -C -c -4 -t UDP_STREAM -H 172.17.60.216
UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216 (172.17.60.216) port 0 AF_INET
Socket Message Elapsed Messages CPU Service
Size Size Time Okay Errors Throughput Util Demand
bytes bytes secs # # 10^6bits/sec % SS us/KB
114688 65507 10.01 42908 0 2247.0 94.11 51.186
116736 10.01 2876 150.6 0.17 0.761
bonding device with early-orphan off (behaviour prior to 2.6.35)
# netperf -C -c -4 -t UDP_STREAM -H 172.17.60.216
UDP UNIDIRECTIONAL SEND TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 172.17.60.216 (172.17.60.216) port 0 AF_INET
Socket Message Elapsed Messages CPU Service
Size Size Time Okay Errors Throughput Util Demand
bytes bytes secs # # 10^6bits/sec % SS us/KB
114688 65507 10.02 18405 0 963.0 40.12 3.413
116736 10.02 18405 963.0 0.78 0.528
^ permalink raw reply
* Re: [PATCH] rfc: ethtool: early-orphan control
From: Ben Hutchings @ 2010-12-11 4:37 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, Eric Dumazet
In-Reply-To: <1292040815-10439-1-git-send-email-horms@verge.net.au>
On Sat, 2010-12-11 at 13:13 +0900, Simon Horman wrote:
> Early orphaning is an optimisation which avoids unnecessary cache misses by
> orphaning an skb just before it is handed to a device for transmit thus
> avoiding the case where the orphaning occurs on a different CPU.
>
> In the case of bonded devices this has the unfortunate side-effect of
> breaking down flow control allowing a socket to send UDP packets as fast as
> the CPU will allow. This is particularly undesirable in virtualised
> network environments.
>
> This patch introduces ethtool control of early orphaning.
> It remains on by default by it now may be disabled on a per-interface basis.
>
> I have implemented this as a generic flag.
> As it seems to be the first generic flag that requires
> no driver awareness I also supplied a default flag handler.
> I am unsure if any aspect of this approach is acceptable.
I'm not convinced that this belongs in the ethtool API. It doesn't seem
to have anything to do with hardware or driver behaviour. The flag
belongs in priv_flags, not features.
But if it is to be a feature flag...
[...]
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 1774178..f63bdce 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
[...]
> @@ -157,6 +158,13 @@ int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
> }
> EXPORT_SYMBOL(ethtool_op_set_flags);
>
> +static int ethtool_op_set_flags_early_orphan(struct net_device *dev, u32 data)
> +{
> + dev->features = ((dev->features & ~NETIF_F_EARLY_ORPHAN) |
> + (data & NETIF_F_EARLY_ORPHAN));
> + return 0;
this needs to check that no unsupported flags are set, i.e.
return ethtool_op_set_flags(dev, data, NETIF_F_EARLY_ORPHAN);
> +}
> +
> void ethtool_ntuple_flush(struct net_device *dev)
> {
> struct ethtool_rx_ntuple_flow_spec_container *fsc, *f;
> @@ -1644,7 +1652,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
> break;
> case ETHTOOL_SFLAGS:
> rc = ethtool_set_value(dev, useraddr,
> - dev->ethtool_ops->set_flags);
> + dev->ethtool_ops->set_flags ?
> + dev->ethtool_ops->set_flags :
> + ethtool_op_set_flags_early_orphan);
[...]
and this fallback needs to be done further up along with ETHTOOL_DRVINFO
so that it doesn't depend on the driver setting dev->ethtool_ops at all.
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] rfc: ethtool: early-orphan control (user-space)
From: Ben Hutchings @ 2010-12-11 4:44 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, Eric Dumazet
In-Reply-To: <1292040838-10579-1-git-send-email-horms@verge.net.au>
On Sat, 2010-12-11 at 13:13 +0900, Simon Horman wrote:
[...]
> diff --git a/ethtool.c b/ethtool.c
> index 239912b..579b4e4 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -163,7 +163,8 @@ static struct option {
> " [ xcvr internal|external ]\n"
> " [ wol p|u|m|b|a|g|s|d... ]\n"
> " [ sopass %x:%x:%x:%x:%x:%x ]\n"
> - " [ msglvl %d | msglvl type on|off ... ]\n" },
> + " [ msglvl %d | msglvl type on|off ... ]\n"
> + " [ early-orphan on|off ]\n" },
> { "-a", "--show-pause", MODE_GPAUSE, "Show pause options" },
> { "-A", "--pause", MODE_SPAUSE, "Set pause options",
> " [ autoneg on|off ]\n"
> @@ -411,6 +412,10 @@ static int msglvl_changed;
> static u32 msglvl_wanted = 0;
> static u32 msglvl_mask = 0;
>
> +static u32 generic_flags_changed;
> +static u32 generic_flags_wanted = 0;
> +static u32 generic_flags_mask = 0;
> +
> static enum {
> ONLINE=0,
> OFFLINE,
> @@ -608,6 +613,11 @@ static struct cmdline_info cmdline_msglvl[] = {
> NETIF_MSG_WOL, &msglvl_mask },
> };
>
> +static struct cmdline_info cmdline_generic_flags[] = {
> + { "early-orphan", CMDL_FLAG, &generic_flags_wanted, NULL,
> + ETH_FLAG_EARLY_ORPHAN, &generic_flags_mask },
> +};
> +
> static long long
> get_int_range(char *str, int base, long long min, long long max)
> {
> @@ -1125,7 +1135,13 @@ static void parse_cmdline(int argc, char **argp)
> }
> break;
> }
> - show_usage(1);
> + parse_generic_cmdline(
> + argc, argp, i,
> + &generic_flags_changed,
> + cmdline_generic_flags,
> + ARRAY_SIZE(cmdline_generic_flags));
> + i = argc;
> + break;
This seems to introduce an order-dependency which doesn't exist with any
of the other keyword arguments after -s.
[...]
> + if (generic_flags_changed) {
> + printf("generic flags changed\n");
> + struct ethtool_value edata;
> +
> + edata.cmd = ETHTOOL_GFLAGS;
> + edata.data = 0;
> + ifr->ifr_data = (caddr_t)&edata;
> + err = ioctl(fd, SIOCETHTOOL, ifr);
> + if (err) {
> + perror("Cannot get device flag settings");
> + return 91;
> + }
> +
> + edata.cmd = ETHTOOL_SFLAGS;
> + edata.data = ((edata.data & ~generic_flags_mask) |
> + generic_flags_wanted);
> +
> + err = ioctl(fd, SIOCETHTOOL, ifr);
> + if (err)
> + perror("Cannot set device flag settings");
> + }
> return 0;
> }
[...]
This has a silent failure case (silent to any script checking the exit
code, anyway).
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH] fix bug in Ethernet Channel Bonding Driver
From: Hillf Danton @ 2010-12-11 4:54 UTC (permalink / raw)
To: netdev, linux-kernel
The returned slave is incorrect, if the net device under check is not
charged yet by the master.
Signed-off-by: Hillf Danton <dhillf@gmail.com>
---
--- a/drivers/net/bonding/bonding.h 2010-11-01 19:54:12.000000000 +0800
+++ b/drivers/net/bonding/bonding.h 2010-12-11 12:48:00.000000000 +0800
@@ -273,11 +273,11 @@ static inline struct slave *bond_get_sla
bond_for_each_slave(bond, slave, i) {
if (slave->dev == slave_dev) {
- break;
+ return slave;
}
}
- return slave;
+ return 0;
}
static inline struct bonding *bond_get_bond_by_slave(struct slave *slave)
^ permalink raw reply
* Re: [PATCH] rfc: ethtool: early-orphan control
From: Simon Horman @ 2010-12-11 5:04 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, Eric Dumazet
In-Reply-To: <1292042278.3136.14.camel@localhost>
On Sat, Dec 11, 2010 at 04:37:58AM +0000, Ben Hutchings wrote:
> On Sat, 2010-12-11 at 13:13 +0900, Simon Horman wrote:
> > Early orphaning is an optimisation which avoids unnecessary cache misses by
> > orphaning an skb just before it is handed to a device for transmit thus
> > avoiding the case where the orphaning occurs on a different CPU.
> >
> > In the case of bonded devices this has the unfortunate side-effect of
> > breaking down flow control allowing a socket to send UDP packets as fast as
> > the CPU will allow. This is particularly undesirable in virtualised
> > network environments.
> >
> > This patch introduces ethtool control of early orphaning.
> > It remains on by default by it now may be disabled on a per-interface basis.
> >
> > I have implemented this as a generic flag.
> > As it seems to be the first generic flag that requires
> > no driver awareness I also supplied a default flag handler.
> > I am unsure if any aspect of this approach is acceptable.
>
> I'm not convinced that this belongs in the ethtool API. It doesn't seem
> to have anything to do with hardware or driver behaviour. The flag
> belongs in priv_flags, not features.
Ok, I have no objection to it going in priv_flags so long
as it can be exposed to user-space in some sensible fashion.
Do you have any thoughts on how best to achieve that?
> But if it is to be a feature flag...
>
> [...]
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 1774178..f63bdce 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> [...]
> > @@ -157,6 +158,13 @@ int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported)
> > }
> > EXPORT_SYMBOL(ethtool_op_set_flags);
> >
> > +static int ethtool_op_set_flags_early_orphan(struct net_device *dev, u32 data)
> > +{
> > + dev->features = ((dev->features & ~NETIF_F_EARLY_ORPHAN) |
> > + (data & NETIF_F_EARLY_ORPHAN));
> > + return 0;
>
> this needs to check that no unsupported flags are set, i.e.
>
> return ethtool_op_set_flags(dev, data, NETIF_F_EARLY_ORPHAN);
I thought that I could ensure that by using NETIF_F_EARLY_ORPHAN
as the mask as I have above.
I think that in order for your suggestion to work we
need to mask out the non-flags_dup_features in the supported
check in ethtool_op_set_flags() or use:
return ethtool_op_set_flags(dev, data, dev->features & NETIF_F_EARLY_ORPHAN);
Although NETIF_F_EARLY_ORPHAN isn't needed there due to the
exception I added for it to the supported check in ethtool_op_set_flags().
>
> > +}
> > +
> > void ethtool_ntuple_flush(struct net_device *dev)
> > {
> > struct ethtool_rx_ntuple_flow_spec_container *fsc, *f;
> > @@ -1644,7 +1652,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
> > break;
> > case ETHTOOL_SFLAGS:
> > rc = ethtool_set_value(dev, useraddr,
> > - dev->ethtool_ops->set_flags);
> > + dev->ethtool_ops->set_flags ?
> > + dev->ethtool_ops->set_flags :
> > + ethtool_op_set_flags_early_orphan);
> [...]
>
> and this fallback needs to be done further up along with ETHTOOL_DRVINFO
> so that it doesn't depend on the driver setting dev->ethtool_ops at all.
Thanks, got it.
^ permalink raw reply
* Re: [patch] isdn: return -EFAULT if copy_from_user() fails
From: Dan Carpenter @ 2010-12-11 5:07 UTC (permalink / raw)
To: David Miller; +Cc: nikai, isdn, netdev, kernel-janitors
In-Reply-To: <20101210.165259.22054320.davem@davemloft.net>
> >>> Ahem, we're printing this return value:
> >>>
Doh! Thanks for that guys.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] rfc: ethtool: early-orphan control (user-space)
From: Simon Horman @ 2010-12-11 5:10 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, Eric Dumazet
In-Reply-To: <1292042650.3136.20.camel@localhost>
On Sat, Dec 11, 2010 at 04:44:10AM +0000, Ben Hutchings wrote:
> On Sat, 2010-12-11 at 13:13 +0900, Simon Horman wrote:
> [...]
> > diff --git a/ethtool.c b/ethtool.c
> > index 239912b..579b4e4 100644
> > --- a/ethtool.c
> > +++ b/ethtool.c
> > @@ -163,7 +163,8 @@ static struct option {
> > " [ xcvr internal|external ]\n"
> > " [ wol p|u|m|b|a|g|s|d... ]\n"
> > " [ sopass %x:%x:%x:%x:%x:%x ]\n"
> > - " [ msglvl %d | msglvl type on|off ... ]\n" },
> > + " [ msglvl %d | msglvl type on|off ... ]\n"
> > + " [ early-orphan on|off ]\n" },
> > { "-a", "--show-pause", MODE_GPAUSE, "Show pause options" },
> > { "-A", "--pause", MODE_SPAUSE, "Set pause options",
> > " [ autoneg on|off ]\n"
> > @@ -411,6 +412,10 @@ static int msglvl_changed;
> > static u32 msglvl_wanted = 0;
> > static u32 msglvl_mask = 0;
> >
> > +static u32 generic_flags_changed;
> > +static u32 generic_flags_wanted = 0;
> > +static u32 generic_flags_mask = 0;
> > +
> > static enum {
> > ONLINE=0,
> > OFFLINE,
> > @@ -608,6 +613,11 @@ static struct cmdline_info cmdline_msglvl[] = {
> > NETIF_MSG_WOL, &msglvl_mask },
> > };
> >
> > +static struct cmdline_info cmdline_generic_flags[] = {
> > + { "early-orphan", CMDL_FLAG, &generic_flags_wanted, NULL,
> > + ETH_FLAG_EARLY_ORPHAN, &generic_flags_mask },
> > +};
> > +
> > static long long
> > get_int_range(char *str, int base, long long min, long long max)
> > {
> > @@ -1125,7 +1135,13 @@ static void parse_cmdline(int argc, char **argp)
> > }
> > break;
> > }
> > - show_usage(1);
> > + parse_generic_cmdline(
> > + argc, argp, i,
> > + &generic_flags_changed,
> > + cmdline_generic_flags,
> > + ARRAY_SIZE(cmdline_generic_flags));
> > + i = argc;
> > + break;
>
> This seems to introduce an order-dependency which doesn't exist with any
> of the other keyword arguments after -s.
Perhaps it would be better to just open-code the parsing
as per the rest of parse_generic_cmdline()?
> [...]
> > + if (generic_flags_changed) {
> > + printf("generic flags changed\n");
> > + struct ethtool_value edata;
> > +
> > + edata.cmd = ETHTOOL_GFLAGS;
> > + edata.data = 0;
> > + ifr->ifr_data = (caddr_t)&edata;
> > + err = ioctl(fd, SIOCETHTOOL, ifr);
> > + if (err) {
> > + perror("Cannot get device flag settings");
> > + return 91;
> > + }
> > +
> > + edata.cmd = ETHTOOL_SFLAGS;
> > + edata.data = ((edata.data & ~generic_flags_mask) |
> > + generic_flags_wanted);
> > +
> > + err = ioctl(fd, SIOCETHTOOL, ifr);
> > + if (err)
> > + perror("Cannot set device flag settings");
> > + }
> > return 0;
> > }
> [...]
>
> This has a silent failure case (silent to any script checking the exit
> code, anyway).
Oops, sorry about that. I think that it should be:
err = ioctl(fd, SIOCETHTOOL, ifr);
if (err) {
perror("Cannot set device flag settings");
return 92;
}
^ permalink raw reply
* Re: [net-next-2.6 03/27] Documentation/networking/igb.txt: update documentation
From: Jeff Kirsher @ 2010-12-11 5:25 UTC (permalink / raw)
To: Ben Hutchings; +Cc: davem, davem, netdev, gospo, bphilips
In-Reply-To: <1291996224.11673.2.camel@bwh-desktop>
On Fri, Dec 10, 2010 at 07:50, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Fri, 2010-12-10 at 01:50 -0800, Jeff Kirsher wrote:
>> Update Intel Wired LAN igb documentation.
>>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>> Documentation/networking/igb.txt | 22 +++++++++++++++++++---
>> 1 files changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/networking/igb.txt b/Documentation/networking/igb.txt
>> index ab2d718..393bdb7 100644
>> --- a/Documentation/networking/igb.txt
>> +++ b/Documentation/networking/igb.txt
>> @@ -36,6 +36,7 @@ Default Value: 0
>> This parameter adds support for SR-IOV. It causes the driver to spawn up to
>> max_vfs worth of virtual function.
>>
>> +
>> Additional Configurations
>> =========================
>>
>> @@ -60,7 +61,8 @@ Additional Configurations
>> Ethtool
>> -------
>> The driver utilizes the ethtool interface for driver configuration and
>> - diagnostics, as well as displaying statistical information.
>> + diagnostics, as well as displaying statistical information. The latest
>> + version of Ethtool can be found at:
>>
>> http://sourceforge.net/projects/gkernel.
>
> Please update this to:
> http://ftp.kernel.org/pub/software/network/ethtool/
>
>> @@ -103,8 +105,8 @@ Additional Configurations
>>
>> NOTE: You need to have inet_lro enabled via either the CONFIG_INET_LRO or
>> CONFIG_INET_LRO_MODULE kernel config option. Additionally, if
>> - CONFIG_INET_LRO_MODULE is used, the inet_lro module needs to be loaded
>> - before the igb driver.
>> + CONFIG_INET_LRO_MODULE is used, the inet_lro module needs to be loaded before
>> + the igb driver.
>
> This should be removed as you don't use inet_lro any more.
>
>> You can verify that the driver is using LRO by looking at these counters in
>> Ethtool:
>> @@ -116,6 +118,20 @@ Additional Configurations
>>
>> NOTE: IPv6 and UDP are not supported by LRO.
>>
>> + MAC and VLAN anti-spoofing feature
>> + ----------------------------------
>> + When a malicious driver attempts to send a spoofed packet, it is dropped by
>> + the hardware and not transmitted. An interrupt is sent to the PF driver
>> + notifying it of the spoof attempt.
>> +
>> + When a spoofed packet is detected the PF driver will send the following
>> + message to the system log (displayed by the "dmesg" command):
>> +
>> + Spoof event(s) detected on VF(n)
>> +
>> + Where n=the VF that attempted to do the spoofing.
>
> I can't see that message in the PF driver code; does this actually apply
> to the in-tree driver? Also I hope this is rate-limited.
>
> Ben.
>
For once the documentation was ahead of the code... :) I will update
the patch, and I have updated the anti-spoofing patch we have in
testing to also update the documentation at the same time the code is
updated.
--
Cheers,
Jeff
^ permalink raw reply
* Re: [net-next-2.6 03/27] Documentation/networking/igb.txt: update documentation
From: Jeff Kirsher @ 2010-12-11 5:26 UTC (permalink / raw)
To: David Miller; +Cc: bhutchings, davem, netdev, gospo, bphilips
In-Reply-To: <20101210.122149.232731880.davem@davemloft.net>
On Fri, Dec 10, 2010 at 12:21, David Miller <davem@davemloft.net> wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Fri, 10 Dec 2010 15:50:24 +0000
>
>> On Fri, 2010-12-10 at 01:50 -0800, Jeff Kirsher wrote:
>>> Update Intel Wired LAN igb documentation.
>>>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> ---
>>> Documentation/networking/igb.txt | 22 +++++++++++++++++++---
>>> 1 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/networking/igb.txt b/Documentation/networking/igb.txt
>>> index ab2d718..393bdb7 100644
>>> --- a/Documentation/networking/igb.txt
>>> +++ b/Documentation/networking/igb.txt
>>> @@ -36,6 +36,7 @@ Default Value: 0
>>> This parameter adds support for SR-IOV. It causes the driver to spawn up to
>>> max_vfs worth of virtual function.
>>>
>>> +
>>> Additional Configurations
>>> =========================
>>>
>>> @@ -60,7 +61,8 @@ Additional Configurations
>>> Ethtool
>>> -------
>>> The driver utilizes the ethtool interface for driver configuration and
>>> - diagnostics, as well as displaying statistical information.
>>> + diagnostics, as well as displaying statistical information. The latest
>>> + version of Ethtool can be found at:
>>>
>>> http://sourceforge.net/projects/gkernel.
>>
>> Please update this to:
>> http://ftp.kernel.org/pub/software/network/ethtool/
>
> Indeed.
>
> Intel folks, please address all of Ben's feedback and respin this
> pull request, thanks.
> --
>
Re-spinning the pull request now.
--
Cheers,
Jeff
^ permalink raw reply
* Re: [PATCH] rfc: ethtool: early-orphan control
From: Simon Horman @ 2010-12-11 5:39 UTC (permalink / raw)
To: Ben Hutchings; +Cc: netdev, Eric Dumazet
In-Reply-To: <20101211050447.GC32453@verge.net.au>
On Sat, Dec 11, 2010 at 02:04:47PM +0900, Simon Horman wrote:
> On Sat, Dec 11, 2010 at 04:37:58AM +0000, Ben Hutchings wrote:
> > On Sat, 2010-12-11 at 13:13 +0900, Simon Horman wrote:
> > > Early orphaning is an optimisation which avoids unnecessary cache misses by
> > > orphaning an skb just before it is handed to a device for transmit thus
> > > avoiding the case where the orphaning occurs on a different CPU.
> > >
> > > In the case of bonded devices this has the unfortunate side-effect of
> > > breaking down flow control allowing a socket to send UDP packets as fast as
> > > the CPU will allow. This is particularly undesirable in virtualised
> > > network environments.
> > >
> > > This patch introduces ethtool control of early orphaning.
> > > It remains on by default by it now may be disabled on a per-interface basis.
> > >
> > > I have implemented this as a generic flag.
> > > As it seems to be the first generic flag that requires
> > > no driver awareness I also supplied a default flag handler.
> > > I am unsure if any aspect of this approach is acceptable.
> >
> > I'm not convinced that this belongs in the ethtool API. It doesn't seem
> > to have anything to do with hardware or driver behaviour. The flag
> > belongs in priv_flags, not features.
>
> Ok, I have no objection to it going in priv_flags so long
> as it can be exposed to user-space in some sensible fashion.
> Do you have any thoughts on how best to achieve that?
Sorry, I realise that was a pretty silly question
as I now see ETHTOOL_GPFLAGS and ETHTOOL_SPFLAGS.
^ permalink raw reply
* Re: [PATCH] rfc: ethtool: early-orphan control
From: Ben Hutchings @ 2010-12-11 5:46 UTC (permalink / raw)
To: Simon Horman; +Cc: netdev, Eric Dumazet
In-Reply-To: <20101211053945.GA2101@verge.net.au>
On Sat, 2010-12-11 at 14:39 +0900, Simon Horman wrote:
> On Sat, Dec 11, 2010 at 02:04:47PM +0900, Simon Horman wrote:
> > On Sat, Dec 11, 2010 at 04:37:58AM +0000, Ben Hutchings wrote:
> > > On Sat, 2010-12-11 at 13:13 +0900, Simon Horman wrote:
> > > > Early orphaning is an optimisation which avoids unnecessary cache misses by
> > > > orphaning an skb just before it is handed to a device for transmit thus
> > > > avoiding the case where the orphaning occurs on a different CPU.
> > > >
> > > > In the case of bonded devices this has the unfortunate side-effect of
> > > > breaking down flow control allowing a socket to send UDP packets as fast as
> > > > the CPU will allow. This is particularly undesirable in virtualised
> > > > network environments.
> > > >
> > > > This patch introduces ethtool control of early orphaning.
> > > > It remains on by default by it now may be disabled on a per-interface basis.
> > > >
> > > > I have implemented this as a generic flag.
> > > > As it seems to be the first generic flag that requires
> > > > no driver awareness I also supplied a default flag handler.
> > > > I am unsure if any aspect of this approach is acceptable.
> > >
> > > I'm not convinced that this belongs in the ethtool API. It doesn't seem
> > > to have anything to do with hardware or driver behaviour. The flag
> > > belongs in priv_flags, not features.
> >
> > Ok, I have no objection to it going in priv_flags so long
> > as it can be exposed to user-space in some sensible fashion.
> > Do you have any thoughts on how best to achieve that?
>
> Sorry, I realise that was a pretty silly question
> as I now see ETHTOOL_GPFLAGS and ETHTOOL_SPFLAGS.
Not a silly question. The ETHTOOL_{G,S}PFLAGS commands are for
driver-specific flags while net_device::priv_flags is used by the
networking core and some special drivers (IFF_802_1Q_VLAN etc. in
<linux/if.h>).
Ben.
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH 08/27] e1000: fix return value not set on error
From: Jeff Kirsher @ 2010-12-11 6:17 UTC (permalink / raw)
To: davem, davem
Cc: netdev, Dean Nelson, bphilips, Jesse Brandeburg, Jeff Kirsher,
gospo, stable
In-Reply-To: <1292048241-22026-1-git-send-email-jeffrey.t.kirsher@intel.com>
From: Dean Nelson <dnelson@redhat.com>
Dean noticed that 'err' wasn't being set when the "goto err_dma"
statement is executed in the following hunk from the commit. It's value
will be zero as a result of a successful call to e1000_init_hw_struct().
This patch changes the error condition to be correctly propagated.
CC: stable@kernel.org
Signed-off-by: Dean Nelson <dnelson@redhat.com>
Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
Tested-by: Emil Tantilov <emil.s.tantilov@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
drivers/net/e1000/e1000_main.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 06c7d1c..491bf2a 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -971,11 +971,13 @@ static int __devinit e1000_probe(struct pci_dev *pdev,
*/
dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64));
pci_using_dac = 1;
- } else if (!dma_set_mask(&pdev->dev, DMA_BIT_MASK(32))) {
- dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
} else {
- pr_err("No usable DMA config, aborting\n");
- goto err_dma;
+ err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+ if (err) {
+ pr_err("No usable DMA config, aborting\n");
+ goto err_dma;
+ }
+ dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
}
netdev->netdev_ops = &e1000_netdev_ops;
--
1.7.3.2
^ permalink raw reply related
* [net-next 00/27][pull-request] Intel Wired LAN Driver Updates
From: Jeff Kirsher @ 2010-12-11 6:16 UTC (permalink / raw)
To: davem, davem; +Cc: Jeff Kirsher, netdev, gospo, bphilips
Here are a batch of fixes and cleanups intended for 2.6.38.
v2- updated igb.txt and e1000e (patch 27) based on feedback from
Ben Hutchings and Joe Perches.
The following changes since commit:
commit a5d62a149bb8f5359aff7ed7dce339752fbabfd9
Author: David S. Miller <davem@davemloft.net>
Date: Fri Dec 10 16:49:24 2010 -0800
isdn: Fix printed out copy_from_user() return value after previous change
are available in the git repository at:
master.kernel.org:/pub/scm/linux/kernel/git/jkirsher/net-next-2.6.git master
Bruce Allan (11):
e1000e: 82571-based mezzanine card can fail ethtool link test
e1000e: 82574/82583 performance improvement
e1000e: 82577/8 must acquire h/w semaphore before workaround
e1000e: 82571 Serdes can fail to get link
e1000e: 82577/8/9 mis-configured OEM bits during S0->Sx
e1000e: 82579 PHY incorrectly identified during init
e1000e: support new PBA format from EEPROM
e1000e: prevent null ptr dereference in e1000_tx_queue()
e1000e: minor error message corrections
e1000e: static analysis tools complain of a possible null ptr p
dereference
e1000e: increment the driver version
Carolyn Wyborny (1):
igb: Add new function to read part number from EEPROM in string
format
Dean Nelson (1):
e1000: fix return value not set on error
Don Skidmore (3):
ixgbe: fix X540 phy id to correct value
ixgbe: fix X540 to use it's own info struct
ixgbe: cleanup string function calls to use bound checking versions.
Emil Tantilov (2):
ixgb: Don't check for vlan group on transmit
ixgbe: fix ntuple support
Holger Eitzenberger (1):
e1000e: fix double initialization in blink path
Jeff Kirsher (8):
Documentation/networking/e1000.txt: Update documentation
Documentation/networking/e1000e.txt: Update documentation
Documentation/networking/igb.txt: update documentation
Documentation/networking/igbvf.txt: Update documentation
Documentation/networking/ixgbe.txt: Update ixgbe documentation
Documentation/networking/ixgbevf.txt: Update documentation
MAINTAINERS: Update Intel Wired LAN info
Intel Wired LAN drivers: Use static const
Documentation/networking/e1000.txt | 10 +-
Documentation/networking/e1000e.txt | 40 ++++---
Documentation/networking/igb.txt | 31 +-----
Documentation/networking/igbvf.txt | 4 +-
Documentation/networking/ixgbe.txt | 211 ++++++++++++++++++++++------------
Documentation/networking/ixgbevf.txt | 4 -
MAINTAINERS | 2 +
drivers/net/e1000/e1000_hw.c | 20 ++--
drivers/net/e1000/e1000_main.c | 10 +-
drivers/net/e1000/e1000_param.c | 13 +-
drivers/net/e1000e/82571.c | 46 ++++++--
drivers/net/e1000e/defines.h | 8 +-
drivers/net/e1000e/e1000.h | 4 +-
drivers/net/e1000e/ethtool.c | 13 ++-
drivers/net/e1000e/ich8lan.c | 18 ++-
drivers/net/e1000e/lib.c | 135 ++++++++++++++++++----
drivers/net/e1000e/netdev.c | 21 ++--
drivers/net/e1000e/param.c | 2 +-
drivers/net/e1000e/phy.c | 25 +++-
drivers/net/igb/e1000_defines.h | 7 +
drivers/net/igb/e1000_nvm.c | 93 ++++++++++++++-
drivers/net/igb/e1000_nvm.h | 2 +
drivers/net/igb/e1000_phy.c | 11 +-
drivers/net/igb/igb_main.c | 11 +-
drivers/net/ixgb/ixgb_main.c | 51 +++++----
drivers/net/ixgb/ixgb_param.c | 21 ++--
drivers/net/ixgbe/ixgbe_ethtool.c | 31 +++--
drivers/net/ixgbe/ixgbe_main.c | 32 +++---
drivers/net/ixgbe/ixgbe_phy.c | 2 +-
drivers/net/ixgbe/ixgbe_type.h | 2 +-
drivers/net/ixgbe/ixgbe_x540.c | 4 +-
drivers/net/ixgbevf/ethtool.c | 18 ++-
32 files changed, 608 insertions(+), 294 deletions(-)
--
1.7.3.2
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox