* Re: [PATCH -next] qlcnic: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 14:18 UTC (permalink / raw)
To: Ben Hutchings
Cc: Amit Salecha, netdev@vger.kernel.org, Amerigo Wang,
Anirban Chakraborty
In-Reply-To: <1277731840.2089.8.camel@achroite.uk.solarflarecom.com>
On Mon, 28 Jun 2010 14:30:40 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:
> > It would be more useful to add a supported_flags parameter to
> > ethtool_op_set_flags() so it can check the requested flags against the
> > driver/hardware capabilities.
>
> I also just noticed that ethtool.h says set_flags() will return -EINVAL
> for unsupported values. The current implementations variously return
> -EINVAL or -EOPNOTSUPP.
We need to unify ... , I prefer EOPNOTSUPP since this is returned now
if .set_flags == NULL. That will require to change a comment in
ethtool.h and some drivers, I will take a look at it.
Stanislaw
^ permalink raw reply
* Re: [PATCH -next] qlcnic: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 14:14 UTC (permalink / raw)
To: Amit Salecha, Ben Hutchings
Cc: netdev@vger.kernel.org, Amerigo Wang, Anirban Chakraborty
In-Reply-To: <99737F4847ED0A48AECC9F4A1974A4B80F82CB7D4E@MNEXMB2.qlogic.org>
On Mon, 28 Jun 2010 08:09:18 -0500
Amit Salecha <amit.salecha@qlogic.com> wrote:
> >> I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
> I don't know what it will buy you.
>
> Why don't you submit separate patch for below hunk, after your EOPNOTSUPP in ethtool_op_set_flags get accepted.
To do not brake things between patches, I plan to post one patch
touching every ethtool_op_set_flags() call in every driver (see below).
Hence removing that function where possible will make my work easier.
Beside I think toggling NETIF_F_FLAG bits directly is just simpler
than calling ethtool_op_set_flags() only for that purpose.
On Mon, 28 Jun 2010 14:16:51 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:
> > Yes, I did not describe that change in the changelog. I want to
> > remove such usage of ethtool_op_set_flags() for my furher patches, where
> > I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
>
> You might as well remove ethtool_op_set_flags() in that case, as this is
> equivalent to the behaviour when ethtool_ops::set_flags is NULL.
In case of qlcnic we change LRO settings, so removing it here is wrong.
> It would be more useful to add a supported_flags parameter to
> ethtool_op_set_flags() so it can check the requested flags against the
> driver/hardware capabilities.
My plan is something like that:
static const struct ethtool_ops my_ethtool_ops = {
.get_flags = ethtool_op_get_flags,
.set_flags = ethtool_op_set_flags,
.supported_flags = ETH_FLAG_LRO
}
Plus op->supported_flags check in ethtool_op_set_flags. That will allow
to define flags per driver. There is also possible to add supported_flags
to netdev, but I would like to avoid that - in such case drivers can use
custom .set_flags function.
Stanislaw
^ permalink raw reply
* Re: [PATCH -next] qlcnic: fail when try to setup unsupported features
From: Ben Hutchings @ 2010-06-28 13:30 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Amit Salecha, netdev@vger.kernel.org, Amerigo Wang,
Anirban Chakraborty
In-Reply-To: <1277731011.2089.1.camel@achroite.uk.solarflarecom.com>
On Mon, 2010-06-28 at 14:16 +0100, Ben Hutchings wrote:
> On Mon, 2010-06-28 at 14:58 +0200, Stanislaw Gruszka wrote:
> > On Mon, 28 Jun 2010 07:36:04 -0500
> > Amit Salecha <amit.salecha@qlogic.com> wrote:
> >
> > > - ethtool_op_set_flags(netdev, data);
> > > -
> > > - hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
> > > + if (data & ETH_FLAG_LRO) {
> > > + hw_lro = QLCNIC_LRO_ENABLED;
> > > + netdev->features |= NETIF_F_LRO;
> > > + } else {
> > > + hw_lro = 0;
> > > + netdev->features &= ~NETIF_F_LRO;
> > > + }
> > >
> > > Above hunk is unnecessary.
> >
> > Yes, I did not describe that change in the changelog. I want to
> > remove such usage of ethtool_op_set_flags() for my furher patches, where
> > I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
>
> You might as well remove ethtool_op_set_flags() in that case, as this is
> equivalent to the behaviour when ethtool_ops::set_flags is NULL.
>
> It would be more useful to add a supported_flags parameter to
> ethtool_op_set_flags() so it can check the requested flags against the
> driver/hardware capabilities.
I also just noticed that ethtool.h says set_flags() will return -EINVAL
for unsupported values. The current implementations variously return
-EINVAL or -EOPNOTSUPP.
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 -next] qlcnic: fail when try to setup unsupported features
From: Ben Hutchings @ 2010-06-28 13:16 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: Amit Salecha, netdev@vger.kernel.org, Amerigo Wang,
Anirban Chakraborty
In-Reply-To: <20100628145819.74d22d5f@dhcp-lab-109.englab.brq.redhat.com>
On Mon, 2010-06-28 at 14:58 +0200, Stanislaw Gruszka wrote:
> On Mon, 28 Jun 2010 07:36:04 -0500
> Amit Salecha <amit.salecha@qlogic.com> wrote:
>
> > - ethtool_op_set_flags(netdev, data);
> > -
> > - hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
> > + if (data & ETH_FLAG_LRO) {
> > + hw_lro = QLCNIC_LRO_ENABLED;
> > + netdev->features |= NETIF_F_LRO;
> > + } else {
> > + hw_lro = 0;
> > + netdev->features &= ~NETIF_F_LRO;
> > + }
> >
> > Above hunk is unnecessary.
>
> Yes, I did not describe that change in the changelog. I want to
> remove such usage of ethtool_op_set_flags() for my furher patches, where
> I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
You might as well remove ethtool_op_set_flags() in that case, as this is
equivalent to the behaviour when ethtool_ops::set_flags is NULL.
It would be more useful to add a supported_flags parameter to
ethtool_op_set_flags() so it can check the requested flags against the
driver/hardware capabilities.
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 -next] qlcnic: fail when try to setup unsupported features
From: Amit Salecha @ 2010-06-28 13:09 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: netdev@vger.kernel.org, Amerigo Wang, Anirban Chakraborty
In-Reply-To: <20100628145819.74d22d5f@dhcp-lab-109.englab.brq.redhat.com>
>> I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
I don't know what it will buy you.
Why don't you submit separate patch for below hunk, after your EOPNOTSUPP in ethtool_op_set_flags get accepted.
BTW, thanks of this fix.
-----Original Message-----
From: Stanislaw Gruszka [mailto:sgruszka@redhat.com]
Sent: Monday, June 28, 2010 6:28 PM
To: Amit Salecha
Cc: netdev@vger.kernel.org; Amerigo Wang; Anirban Chakraborty
Subject: Re: [PATCH -next] qlcnic: fail when try to setup unsupported features
On Mon, 28 Jun 2010 07:36:04 -0500
Amit Salecha <amit.salecha@qlogic.com> wrote:
> - ethtool_op_set_flags(netdev, data);
> -
> - hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
> + if (data & ETH_FLAG_LRO) {
> + hw_lro = QLCNIC_LRO_ENABLED;
> + netdev->features |= NETIF_F_LRO;
> + } else {
> + hw_lro = 0;
> + netdev->features &= ~NETIF_F_LRO;
> + }
>
> Above hunk is unnecessary.
Yes, I did not describe that change in the changelog. I want to
remove such usage of ethtool_op_set_flags() for my furher patches, where
I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
Stanislaw
^ permalink raw reply
* Re: [PATCH -next] qlcnic: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 12:58 UTC (permalink / raw)
To: Amit Salecha; +Cc: netdev@vger.kernel.org, Amerigo Wang, Anirban Chakraborty
In-Reply-To: <99737F4847ED0A48AECC9F4A1974A4B80F82CB7D46@MNEXMB2.qlogic.org>
On Mon, 28 Jun 2010 07:36:04 -0500
Amit Salecha <amit.salecha@qlogic.com> wrote:
> - ethtool_op_set_flags(netdev, data);
> -
> - hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
> + if (data & ETH_FLAG_LRO) {
> + hw_lro = QLCNIC_LRO_ENABLED;
> + netdev->features |= NETIF_F_LRO;
> + } else {
> + hw_lro = 0;
> + netdev->features &= ~NETIF_F_LRO;
> + }
>
> Above hunk is unnecessary.
Yes, I did not describe that change in the changelog. I want to
remove such usage of ethtool_op_set_flags() for my furher patches, where
I plan to add return EOPNOTSUPP to ethtool_op_set_flags().
Stanislaw
^ permalink raw reply
* [PATCH] bridge: add per bridge device controls for invoking iptables
From: kaber @ 2010-06-28 12:47 UTC (permalink / raw)
To: shemminger; +Cc: netdev
From: Patrick McHardy <kaber@trash.net>
Support more fine grained control of bridge netfilter iptables invocation
by adding seperate brnf_call_*tables parameters for each device using the
sysfs interface. Packets are passed to layer 3 netfilter when either the
global parameter or the per bridge parameter is enabled.
Signed-off-by: Patrick McHardy <kaber@trash.net>
---
net/bridge/br_netfilter.c | 31 ++++++++++++++-----
net/bridge/br_private.h | 3 ++
net/bridge/br_sysfs_br.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 97 insertions(+), 9 deletions(-)
diff --git a/net/bridge/br_netfilter.c b/net/bridge/br_netfilter.c
index 4442099..236a054 100644
--- a/net/bridge/br_netfilter.c
+++ b/net/bridge/br_netfilter.c
@@ -55,6 +55,9 @@ static int brnf_call_arptables __read_mostly = 1;
static int brnf_filter_vlan_tagged __read_mostly = 0;
static int brnf_filter_pppoe_tagged __read_mostly = 0;
#else
+#define brnf_call_iptables 1
+#define brnf_call_ip6tables 1
+#define brnf_call_arptables 1
#define brnf_filter_vlan_tagged 0
#define brnf_filter_pppoe_tagged 0
#endif
@@ -545,25 +548,30 @@ static unsigned int br_nf_pre_routing(unsigned int hook, struct sk_buff *skb,
const struct net_device *out,
int (*okfn)(struct sk_buff *))
{
+ struct net_bridge_port *p;
+ struct net_bridge *br;
struct iphdr *iph;
__u32 len = nf_bridge_encap_header_len(skb);
if (unlikely(!pskb_may_pull(skb, len)))
goto out;
+ p = rcu_dereference(in->br_port);
+ if (p == NULL)
+ goto out;
+ br = p->br;
+
if (skb->protocol == htons(ETH_P_IPV6) || IS_VLAN_IPV6(skb) ||
IS_PPPOE_IPV6(skb)) {
-#ifdef CONFIG_SYSCTL
- if (!brnf_call_ip6tables)
+ if (!brnf_call_ip6tables && !br->nf_call_ip6tables)
return NF_ACCEPT;
-#endif
+
nf_bridge_pull_encap_header_rcsum(skb);
return br_nf_pre_routing_ipv6(hook, skb, in, out, okfn);
}
-#ifdef CONFIG_SYSCTL
- if (!brnf_call_iptables)
+
+ if (!brnf_call_iptables && !br->nf_call_iptables)
return NF_ACCEPT;
-#endif
if (skb->protocol != htons(ETH_P_IP) && !IS_VLAN_IP(skb) &&
!IS_PPPOE_IP(skb))
@@ -716,12 +724,17 @@ static unsigned int br_nf_forward_arp(unsigned int hook, struct sk_buff *skb,
const struct net_device *out,
int (*okfn)(struct sk_buff *))
{
+ struct net_bridge_port *p;
+ struct net_bridge *br;
struct net_device **d = (struct net_device **)(skb->cb);
-#ifdef CONFIG_SYSCTL
- if (!brnf_call_arptables)
+ p = rcu_dereference(out->br_port);
+ if (p == NULL)
+ return NF_ACCEPT;
+ br = p->br;
+
+ if (!brnf_call_arptables && !br->nf_call_arptables)
return NF_ACCEPT;
-#endif
if (skb->protocol != htons(ETH_P_ARP)) {
if (!IS_VLAN_ARP(skb))
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 0f4a74b..2ff13a3 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -164,6 +164,9 @@ struct net_bridge
unsigned long feature_mask;
#ifdef CONFIG_BRIDGE_NETFILTER
struct rtable fake_rtable;
+ bool nf_call_iptables;
+ bool nf_call_ip6tables;
+ bool nf_call_arptables;
#endif
unsigned long flags;
#define BR_SET_MAC_ADDR 0x00000001
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 486b8f3..5c1e555 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -611,6 +611,73 @@ static DEVICE_ATTR(multicast_startup_query_interval, S_IRUGO | S_IWUSR,
show_multicast_startup_query_interval,
store_multicast_startup_query_interval);
#endif
+#ifdef CONFIG_BRIDGE_NETFILTER
+static ssize_t show_nf_call_iptables(
+ struct device *d, struct device_attribute *attr, char *buf)
+{
+ struct net_bridge *br = to_bridge(d);
+ return sprintf(buf, "%u\n", br->nf_call_iptables);
+}
+
+static int set_nf_call_iptables(struct net_bridge *br, unsigned long val)
+{
+ br->nf_call_iptables = val ? true : false;
+ return 0;
+}
+
+static ssize_t store_nf_call_iptables(
+ struct device *d, struct device_attribute *attr, const char *buf,
+ size_t len)
+{
+ return store_bridge_parm(d, buf, len, set_nf_call_iptables);
+}
+static DEVICE_ATTR(nf_call_iptables, S_IRUGO | S_IWUSR,
+ show_nf_call_iptables, store_nf_call_iptables);
+
+static ssize_t show_nf_call_ip6tables(
+ struct device *d, struct device_attribute *attr, char *buf)
+{
+ struct net_bridge *br = to_bridge(d);
+ return sprintf(buf, "%u\n", br->nf_call_ip6tables);
+}
+
+static int set_nf_call_ip6tables(struct net_bridge *br, unsigned long val)
+{
+ br->nf_call_ip6tables = val ? true : false;
+ return 0;
+}
+
+static ssize_t store_nf_call_ip6tables(
+ struct device *d, struct device_attribute *attr, const char *buf,
+ size_t len)
+{
+ return store_bridge_parm(d, buf, len, set_nf_call_ip6tables);
+}
+static DEVICE_ATTR(nf_call_ip6tables, S_IRUGO | S_IWUSR,
+ show_nf_call_ip6tables, store_nf_call_ip6tables);
+
+static ssize_t show_nf_call_arptables(
+ struct device *d, struct device_attribute *attr, char *buf)
+{
+ struct net_bridge *br = to_bridge(d);
+ return sprintf(buf, "%u\n", br->nf_call_arptables);
+}
+
+static int set_nf_call_arptables(struct net_bridge *br, unsigned long val)
+{
+ br->nf_call_arptables = val ? true : false;
+ return 0;
+}
+
+static ssize_t store_nf_call_arptables(
+ struct device *d, struct device_attribute *attr, const char *buf,
+ size_t len)
+{
+ return store_bridge_parm(d, buf, len, set_nf_call_arptables);
+}
+static DEVICE_ATTR(nf_call_arptables, S_IRUGO | S_IWUSR,
+ show_nf_call_arptables, store_nf_call_arptables);
+#endif
static struct attribute *bridge_attrs[] = {
&dev_attr_forward_delay.attr,
@@ -645,6 +712,11 @@ static struct attribute *bridge_attrs[] = {
&dev_attr_multicast_query_response_interval.attr,
&dev_attr_multicast_startup_query_interval.attr,
#endif
+#ifdef CONFIG_BRIDGE_NETFILTER
+ &dev_attr_nf_call_iptables.attr,
+ &dev_attr_nf_call_ip6tables.attr,
+ &dev_attr_nf_call_arptables.attr,
+#endif
NULL
};
--
1.7.0.4
^ permalink raw reply related
* RE: [PATCH -next] qlcnic: fail when try to setup unsupported features
From: Amit Salecha @ 2010-06-28 12:36 UTC (permalink / raw)
To: Stanislaw Gruszka, netdev@vger.kernel.org
Cc: Amerigo Wang, Anirban Chakraborty
In-Reply-To: <20100628113134.0c5208b0@dhcp-lab-109.englab.brq.redhat.com>
- ethtool_op_set_flags(netdev, data);
-
- hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
+ if (data & ETH_FLAG_LRO) {
+ hw_lro = QLCNIC_LRO_ENABLED;
+ netdev->features |= NETIF_F_LRO;
+ } else {
+ hw_lro = 0;
+ netdev->features &= ~NETIF_F_LRO;
+ }
Above hunk is unnecessary.
-Amit
-----Original Message-----
From: Stanislaw Gruszka [mailto:sgruszka@redhat.com]
Sent: Monday, June 28, 2010 3:02 PM
To: netdev@vger.kernel.org
Cc: Amerigo Wang; Amit Salecha; Anirban Chakraborty
Subject: [PATCH -next] qlcnic: fail when try to setup unsupported features
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/qlcnic/qlcnic_ethtool.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
index d4e803e..b9d5acb 100644
--- a/drivers/net/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c
@@ -983,12 +983,19 @@ static int qlcnic_set_flags(struct net_device *netdev, u32 data)
struct qlcnic_adapter *adapter = netdev_priv(netdev);
int hw_lro;
+ if (data & ~ETH_FLAG_LRO)
+ return -EOPNOTSUPP;
+
if (!(adapter->capabilities & QLCNIC_FW_CAPABILITY_HW_LRO))
return -EINVAL;
- ethtool_op_set_flags(netdev, data);
-
- hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
+ if (data & ETH_FLAG_LRO) {
+ hw_lro = QLCNIC_LRO_ENABLED;
+ netdev->features |= NETIF_F_LRO;
+ } else {
+ hw_lro = 0;
+ netdev->features &= ~NETIF_F_LRO;
+ }
if (qlcnic_config_hw_lro(adapter, hw_lro))
return -EIO;
--
1.5.5.6
^ permalink raw reply related
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: Antonio Ospite @ 2010-06-28 11:14 UTC (permalink / raw)
To: Alan Ott
Cc: Marcel Holtmann, David S Miller, Jiri Kosina, Michael Poole,
Bastien Nocera, Eric Dumazet, linux-bluetooth, linux-kernel,
netdev
In-Reply-To: <1276467601-9066-1-git-send-email-alan@signal11.us>
[-- Attachment #1: Type: text/plain, Size: 8758 bytes --]
On Sun, 13 Jun 2010 18:20:01 -0400
Alan Ott <alan@signal11.us> wrote:
> This patch adds support or getting and setting feature reports for bluetooth
> HID devices from HIDRAW.
>
> Signed-off-by: Alan Ott <alan@signal11.us>
> ---
Ping.
> net/bluetooth/hidp/core.c | 121 +++++++++++++++++++++++++++++++++++++++++++--
> net/bluetooth/hidp/hidp.h | 8 +++
> 2 files changed, 125 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index bfe641b..0f068a0 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -36,6 +36,7 @@
> #include <linux/file.h>
> #include <linux/init.h>
> #include <linux/wait.h>
> +#include <linux/mutex.h>
> #include <net/sock.h>
>
> #include <linux/input.h>
> @@ -313,6 +314,93 @@ static int hidp_send_report(struct hidp_session *session, struct hid_report *rep
> return hidp_queue_report(session, buf, rsize);
> }
>
> +static int hidp_get_raw_report(struct hid_device *hid,
> + unsigned char report_number,
> + unsigned char *data, size_t count,
> + unsigned char report_type)
> +{
> + struct hidp_session *session = hid->driver_data;
> + struct sk_buff *skb;
> + size_t len;
> + int numbered_reports = hid->report_enum[report_type].numbered;
> +
> + switch (report_type) {
> + case HID_FEATURE_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> + break;
> + case HID_INPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_INPUT;
> + break;
> + case HID_OUTPUT_REPORT:
> + report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_OUPUT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (mutex_lock_interruptible(&session->report_mutex))
> + return -ERESTARTSYS;
> +
> + /* Set up our wait, and send the report request to the device. */
> + session->waiting_report_type = report_type & HIDP_DATA_RTYPE_MASK;
> + session->waiting_report_number = numbered_reports ? report_number : -1;
> + set_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + data[0] = report_number;
> + if (hidp_send_ctrl_message(hid->driver_data, report_type, data, 1))
> + goto err_eio;
> +
> + /* Wait for the return of the report. The returned report
> + gets put in session->report_return. */
> + while (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + int res;
> +
> + res = wait_event_interruptible_timeout(session->report_queue,
> + !test_bit(HIDP_WAITING_FOR_RETURN, &session->flags),
> + 5*HZ);
> + if (res == 0) {
> + /* timeout */
> + goto err_eio;
> + }
> + if (res < 0) {
> + /* signal */
> + goto err_restartsys;
> + }
> + }
> +
> + skb = session->report_return;
> + if (skb) {
> + if (numbered_reports) {
> + /* Strip off the report number. */
> + size_t rpt_len = skb->len-1;
> + len = rpt_len < count ? rpt_len : count;
> + memcpy(data, skb->data+1, len);
> + } else {
> + len = skb->len < count ? skb->len : count;
> + memcpy(data, skb->data, len);
> + }
> +
> + kfree_skb(skb);
> + session->report_return = NULL;
> + } else {
> + /* Device returned a HANDSHAKE, indicating protocol error. */
> + len = -EIO;
> + }
> +
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> +
> + return len;
> +
> +err_restartsys:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -ERESTARTSYS;
> +err_eio:
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + mutex_unlock(&session->report_mutex);
> + return -EIO;
> +}
> +
> static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
> unsigned char report_type)
> {
> @@ -367,6 +455,10 @@ static void hidp_process_handshake(struct hidp_session *session,
> case HIDP_HSHK_ERR_INVALID_REPORT_ID:
> case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
> case HIDP_HSHK_ERR_INVALID_PARAMETER:
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags)) {
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> /* FIXME: Call into SET_ GET_ handlers here */
> break;
>
> @@ -403,9 +495,11 @@ static void hidp_process_hid_control(struct hidp_session *session,
> }
> }
>
> -static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> +/* Returns true if the passed-in skb should be freed by the caller. */
> +static int hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> unsigned char param)
> {
> + int done_with_skb = 1;
> BT_DBG("session %p skb %p len %d param 0x%02x", session, skb, skb->len, param);
>
> switch (param) {
> @@ -417,7 +511,6 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
>
> if (session->hid)
> hid_input_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len, 0);
> -
> break;
>
> case HIDP_DATA_RTYPE_OTHER:
> @@ -429,12 +522,27 @@ static void hidp_process_data(struct hidp_session *session, struct sk_buff *skb,
> __hidp_send_ctrl_message(session,
> HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER, NULL, 0);
> }
> +
> + if (test_bit(HIDP_WAITING_FOR_RETURN, &session->flags) &&
> + param == session->waiting_report_type) {
> + if (session->waiting_report_number < 0 ||
> + session->waiting_report_number == skb->data[0]) {
> + /* hidp_get_raw_report() is waiting on this report. */
> + session->report_return = skb;
> + done_with_skb = 0;
> + clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
> + wake_up_interruptible(&session->report_queue);
> + }
> + }
> +
> + return done_with_skb;
> }
>
> static void hidp_recv_ctrl_frame(struct hidp_session *session,
> struct sk_buff *skb)
> {
> unsigned char hdr, type, param;
> + int free_skb = 1;
>
> BT_DBG("session %p skb %p len %d", session, skb, skb->len);
>
> @@ -454,7 +562,7 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
>
> case HIDP_TRANS_DATA:
> - hidp_process_data(session, skb, param);
> + free_skb = hidp_process_data(session, skb, param);
> break;
>
> default:
> @@ -463,7 +571,8 @@ static void hidp_recv_ctrl_frame(struct hidp_session *session,
> break;
> }
>
> - kfree_skb(skb);
> + if (free_skb)
> + kfree_skb(skb);
> }
>
> static void hidp_recv_intr_frame(struct hidp_session *session,
> @@ -797,6 +906,7 @@ static int hidp_setup_hid(struct hidp_session *session,
> hid->dev.parent = hidp_get_device(session);
> hid->ll_driver = &hidp_hid_driver;
>
> + hid->hid_get_raw_report = hidp_get_raw_report;
> hid->hid_output_raw_report = hidp_output_raw_report;
>
> err = hid_add_device(hid);
> @@ -857,6 +967,9 @@ int hidp_add_connection(struct hidp_connadd_req *req, struct socket *ctrl_sock,
> skb_queue_head_init(&session->ctrl_transmit);
> skb_queue_head_init(&session->intr_transmit);
>
> + mutex_init(&session->report_mutex);
> + init_waitqueue_head(&session->report_queue);
> +
> session->flags = req->flags & (1 << HIDP_BLUETOOTH_VENDOR_ID);
> session->idle_to = req->idle_to;
>
> diff --git a/net/bluetooth/hidp/hidp.h b/net/bluetooth/hidp/hidp.h
> index 8d934a1..00e71dd 100644
> --- a/net/bluetooth/hidp/hidp.h
> +++ b/net/bluetooth/hidp/hidp.h
> @@ -80,6 +80,7 @@
> #define HIDP_VIRTUAL_CABLE_UNPLUG 0
> #define HIDP_BOOT_PROTOCOL_MODE 1
> #define HIDP_BLUETOOTH_VENDOR_ID 9
> +#define HIDP_WAITING_FOR_RETURN 10
>
> struct hidp_connadd_req {
> int ctrl_sock; // Connected control socket
> @@ -154,6 +155,13 @@ struct hidp_session {
> struct sk_buff_head ctrl_transmit;
> struct sk_buff_head intr_transmit;
>
> + /* Used in hidp_get_raw_report() */
> + int waiting_report_type; /* HIDP_DATA_RTYPE_* */
> + int waiting_report_number; /* -1 for not numbered */
> + struct mutex report_mutex;
> + struct sk_buff *report_return;
> + wait_queue_head_t report_queue;
> +
> /* Report descriptor */
> __u8 *rd_data;
> uint rd_size;
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: b44: Reset due to FIFO overflow.
From: Eric Dumazet @ 2010-06-28 11:11 UTC (permalink / raw)
To: James Courtier-Dutton; +Cc: netdev
In-Reply-To: <AANLkTik6YBO7bVAy0NjdQGomYBsY-e8PqZ3-Je_vyx7B@mail.gmail.com>
Le lundi 28 juin 2010 à 11:24 +0100, James Courtier-Dutton a écrit :
> On 28 June 2010 10:13, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Hi
> >
> > Problem is we dont know if a Receive Fifo overflow is a minor or major
> > indication from b44 chip.
> >
> > A minor indication would be : Chip tells us one or more frame were lost.
> > No special action needed from driver.
> >
> > A major indication (as of current implemented in b44 driver) is :
> > I am completely out of order and need a reset. Please do it.
> >
> > Patch to switch from major to minor indication is easy, but we dont know
> > if its valid or not.
> >
> > diff --git a/drivers/net/b44.h b/drivers/net/b44.h
> > index e1905a4..514dc3a 100644
> > --- a/drivers/net/b44.h
> > +++ b/drivers/net/b44.h
> > @@ -42,7 +42,7 @@
> > #define ISTAT_EMAC 0x04000000 /* EMAC Interrupt */
> > #define ISTAT_MII_WRITE 0x08000000 /* MII Write Interrupt */
> > #define ISTAT_MII_READ 0x10000000 /* MII Read Interrupt */
> > -#define ISTAT_ERRORS (ISTAT_DSCE|ISTAT_DATAE|ISTAT_DPE|ISTAT_RDU|ISTAT_RFO|ISTAT_TFU)
> > +#define ISTAT_ERRORS (ISTAT_DSCE|ISTAT_DATAE|ISTAT_DPE|ISTAT_RDU|ISTAT_TFU)
> > #define B44_IMASK 0x0024UL /* Interrupt Mask */
> > #define IMASK_DEF (ISTAT_ERRORS | ISTAT_TO | ISTAT_RX | ISTAT_TX)
> > #define B44_GPTIMER 0x0028UL /* General Purpose Timer */
> >
> >
> >
>
> Ok, are you saying that all I have to do is apply this patch,
> reproduce the problem condition, and if it recovers OK, then we can go
> with this fix?
> If so, I will try it out after work.
>
Yes, please try the patch and tell us what happens.
Note : It can be better, it can be worse.
It can work on your b44 chip, and freeze another computer with another
b44 chip. Use at your own risk.
> I will probably add a printk in before the ISTAT_ERRORS test, to
> inform me when that ISTAT_RFO has actually happened.
>
In this case, you also need to add ISTAT_RFO to IMASK_DEF. My patch
completely ignores ISTAT_RFO and NIC wont generate an interrupt for this
event.
> But is doing nothing the right thing?
If chip only signals an overflow and can revover, its pretty like losing
a frame on a busy network. It happens and you dont have a report for
this.
> I would have thought that one would have to at least start and stop
> the FIFO in order for the write/read pointers to be in the correct
> positions or at least change the read pointer to do the equivalent of
> flush the buffer.
All this depends on hardware bits I dont have access to.
> Is there any of this sort of control over the FIFO possible?
>
Probably, but I guess if broadcam guys did not implement it, they
probably have a good reason :)
^ permalink raw reply
* Re: b44: Reset due to FIFO overflow.
From: Eric Dumazet @ 2010-06-28 11:09 UTC (permalink / raw)
To: James Courtier-Dutton; +Cc: Mitchell Erblich, netdev
In-Reply-To: <AANLkTimK4mGdsSq206aqfusXPvnQczbYDlOWSYXAbOQJ@mail.gmail.com>
Le lundi 28 juin 2010 à 11:17 +0100, James Courtier-Dutton a écrit :
> On 28 June 2010 11:00, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> > Problem is we receive a spike of RX network frames (possibly UDP or some
> > other RX only trafic), and chip raises an RX fifo overflow _error_
> > indication.
> >
>
> The cause of the RX overflow is in my case is TCP.
> It is reproducible in mythtv.
> While watching LiveTV, press "s" for the program guide.
> The program guide is implemented into mythtv by a SQL query that
> results in a large response.
> The kernel is probably not servicing the RX FIFO quickly enough due to
> it being busy doing something else. In this case, probably a video
> mode switch.
>
Thats strange, b44 has a big RX ring... and tcp sender should wait for
ACK...
> > Some hardware are buggy enough that such error indication is fatal and
> > _require_ hardware reset. Thats life. I suspect b44 driver doing a full
> > reset is not a random guess from driver author, but to avoid a complete
> > NIC lockup.
> >
>
> Interesting, which hardware, apart from the b44, is it that "requires"
> a hardware reset after a RX FIFO overflow.
Just take a look at some net drivers and you'll see some of them have
this requirement.
rtl8169_rx_interrupt()
...
if (status & RxFOVF) {
rtl8169_schedule_work(dev, rtl8169_reset_task);
dev->stats.rx_fifo_errors++;
}
^ permalink raw reply
* Re: [PATCH -next] bnx2x: fail when try to setup unsupported features
From: Eilon Greenstein @ 2010-06-28 11:07 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: netdev@vger.kernel.org, Amerigo Wang
In-Reply-To: <20100628112811.29881285@dhcp-lab-109.englab.brq.redhat.com>
On Mon, 2010-06-28 at 02:28 -0700, Stanislaw Gruszka wrote:
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
Thanks Stanislaw!
> ---
> drivers/net/bnx2x_main.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
> index 57ff5b3..0809f6c 100644
> --- a/drivers/net/bnx2x_main.c
> +++ b/drivers/net/bnx2x_main.c
> @@ -10982,6 +10982,9 @@ static int bnx2x_set_flags(struct net_device *dev, u32 data)
> int changed = 0;
> int rc = 0;
>
> + if (data & ~(ETH_FLAG_LRO | ETH_FLAG_RXHASH))
> + return -EOPNOTSUPP;
> +
> if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
> printk(KERN_ERR "Handling parity error recovery. Try again later\n");
> return -EAGAIN;
^ permalink raw reply
* Re: [RFC][BUG-FIX] the problem of checksum checking in UDP protocol
From: Shan Wei @ 2010-06-28 10:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Ronciak, John, netdev
In-Reply-To: <1277530086.2481.15.camel@edumazet-laptop>
Eric Dumazet wrote, at 06/26/2010 01:28 PM:
>> (This patch is not complete, it's just for my idea.)
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 1dd1aff..47f7e86 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -723,6 +723,10 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
>> if (ulen < skb->len) {
>> if (pskb_trim_rcsum(skb, ulen))
>> goto short_packet;
>> +
>> + if (skb_csum_unnecessary(skb))
>> + skb->ip_summed = CHECKSUM_NONE;
>> +
>> saddr = &ipv6_hdr(skb)->saddr;
>> daddr = &ipv6_hdr(skb)->daddr;
>> uh = udp_hdr(skb);
>>
>
> I really dont know if this fix is the right one.
>
> pskb_trim_rcsum() already contains a check. Should this check be changed
> to include yours ?
Oh..... I don't think so.
pskb_trim_rcsum() is also used when IPv4/IPv6 protocol receiving packets
and reassembling fragments. IP protocol does the right check and should
trust CHECKSUM_UNNECESSARY flag that drivers set, So we no need to
change IP protocol.
If we add the skb_csum_unnecessary(skb) check into pskb_trim_rcsum() and
reset ip_summed with CHECKSUM_NONE, the checksum check that NIC hardward
has done is wasted.
Only for UDP protocol over IPv4/IPv6, and length parameter is lower than
skb->len, We reset ip_summed with CHECKSUM_NONE.
--
Best Regards
-----
Shan Wei
^ permalink raw reply
* Re: b44: Reset due to FIFO overflow.
From: James Courtier-Dutton @ 2010-06-28 10:24 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1277716394.4235.235.camel@edumazet-laptop>
On 28 June 2010 10:13, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Hi
>
> Problem is we dont know if a Receive Fifo overflow is a minor or major
> indication from b44 chip.
>
> A minor indication would be : Chip tells us one or more frame were lost.
> No special action needed from driver.
>
> A major indication (as of current implemented in b44 driver) is :
> I am completely out of order and need a reset. Please do it.
>
> Patch to switch from major to minor indication is easy, but we dont know
> if its valid or not.
>
> diff --git a/drivers/net/b44.h b/drivers/net/b44.h
> index e1905a4..514dc3a 100644
> --- a/drivers/net/b44.h
> +++ b/drivers/net/b44.h
> @@ -42,7 +42,7 @@
> #define ISTAT_EMAC 0x04000000 /* EMAC Interrupt */
> #define ISTAT_MII_WRITE 0x08000000 /* MII Write Interrupt */
> #define ISTAT_MII_READ 0x10000000 /* MII Read Interrupt */
> -#define ISTAT_ERRORS (ISTAT_DSCE|ISTAT_DATAE|ISTAT_DPE|ISTAT_RDU|ISTAT_RFO|ISTAT_TFU)
> +#define ISTAT_ERRORS (ISTAT_DSCE|ISTAT_DATAE|ISTAT_DPE|ISTAT_RDU|ISTAT_TFU)
> #define B44_IMASK 0x0024UL /* Interrupt Mask */
> #define IMASK_DEF (ISTAT_ERRORS | ISTAT_TO | ISTAT_RX | ISTAT_TX)
> #define B44_GPTIMER 0x0028UL /* General Purpose Timer */
>
>
>
Ok, are you saying that all I have to do is apply this patch,
reproduce the problem condition, and if it recovers OK, then we can go
with this fix?
If so, I will try it out after work.
I will probably add a printk in before the ISTAT_ERRORS test, to
inform me when that ISTAT_RFO has actually happened.
But is doing nothing the right thing?
I would have thought that one would have to at least start and stop
the FIFO in order for the write/read pointers to be in the correct
positions or at least change the read pointer to do the equivalent of
flush the buffer.
Is there any of this sort of control over the FIFO possible?
Kind Regards
James
^ permalink raw reply
* Re: b44: Reset due to FIFO overflow.
From: James Courtier-Dutton @ 2010-06-28 10:17 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Mitchell Erblich, netdev
In-Reply-To: <1277719251.4235.306.camel@edumazet-laptop>
On 28 June 2010 11:00, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
> Problem is we receive a spike of RX network frames (possibly UDP or some
> other RX only trafic), and chip raises an RX fifo overflow _error_
> indication.
>
The cause of the RX overflow is in my case is TCP.
It is reproducible in mythtv.
While watching LiveTV, press "s" for the program guide.
The program guide is implemented into mythtv by a SQL query that
results in a large response.
The kernel is probably not servicing the RX FIFO quickly enough due to
it being busy doing something else. In this case, probably a video
mode switch.
> Some hardware are buggy enough that such error indication is fatal and
> _require_ hardware reset. Thats life. I suspect b44 driver doing a full
> reset is not a random guess from driver author, but to avoid a complete
> NIC lockup.
>
Interesting, which hardware, apart from the b44, is it that "requires"
a hardware reset after a RX FIFO overflow.
Now, it is true that the driver for the b44 is just as bad on windows,
but there the recovery requires a windows reboot!
Kind Regards
James
^ permalink raw reply
* nat bypass
From: ratheesh k @ 2010-06-28 10:13 UTC (permalink / raw)
To: Netfilter mailing list, netdev
Hi,
A -------> R ------->S
I have a linux machine A is connected to Linux machine R . Machine R
is having two network interfaces and acting as a router .
It has a dhcp server running . It will assign ip in 192.168.1.0/24
subnet to all machine connected on lan side ( A is connected also in
lan side ) . Wan side of R is connected to HTTP server S . There is
also a DHCP server running on S to assign ip in 10.232.18.0/24 subnet
. Is there any way , in which NAT should be bypassed to get ip from
DHCP server running on S . My question is : How can A will get an ip
from 10.232.18.0/24 pool ip .?
ebtables is an option ? How can we make it ?
Is there any other optimal way ?
Thanks,
Ratheesh
^ permalink raw reply
* [PATCHv2] vhost-net: add dhclient work-around from userspace
From: Michael S. Tsirkin @ 2010-06-28 10:08 UTC (permalink / raw)
To: Michael S. Tsirkin, Aristeu Rozanski, Herbert Xu, Juan Quintela,
David S. Miller
Userspace virtio server has the following hack
so guests rely on it, and we have to replicate it, too:
Use port number to detect incoming IPv4 DHCP response packets,
and fill in the checksum for these.
The issue we are solving is that on linux guests, some apps
that use recvmsg with AF_PACKET sockets, don't know how to
handle CHECKSUM_PARTIAL;
The interface to return the relevant information was added
in 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
and older userspace does not use it.
One important user of recvmsg with AF_PACKET is dhclient,
so we add a work-around just for DHCP.
Don't bother applying the hack to IPv6 as userspace virtio does not
have a work-around for that - let's hope guests will do the right
thing wrt IPv6.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Dave, I'm going to put this patch on the vhost tree,
no need for you to bother merging it - you'll get
it with a pull request.
drivers/vhost/net.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 43 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index cc19595..03bba6a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -24,6 +24,10 @@
#include <linux/if_tun.h>
#include <linux/if_macvlan.h>
+#include <linux/ip.h>
+#include <linux/udp.h>
+#include <linux/netdevice.h>
+
#include <net/sock.h>
#include "vhost.h"
@@ -186,6 +190,44 @@ static void handle_tx(struct vhost_net *net)
unuse_mm(net->dev.mm);
}
+static int peek_head(struct sock *sk)
+{
+ struct sk_buff *skb;
+
+ lock_sock(sk);
+ skb = skb_peek(&sk->sk_receive_queue);
+ if (unlikely(!skb)) {
+ release_sock(sk);
+ return 0;
+ }
+ /* Userspace virtio server has the following hack so
+ * guests rely on it, and we have to replicate it, too: */
+ /* Use port number to detect incoming IPv4 DHCP response packets,
+ * and fill in the checksum. */
+
+ /* The issue we are solving is that on linux guests, some apps
+ * that use recvmsg with AF_PACKET sockets, don't know how to
+ * handle CHECKSUM_PARTIAL;
+ * The interface to return the relevant information was added in
+ * 8dc4194474159660d7f37c495e3fc3f10d0db8cc,
+ * and older userspace does not use it.
+ * One important user of recvmsg with AF_PACKET is dhclient,
+ * so we add a work-around just for DHCP. */
+ if (skb->ip_summed == CHECKSUM_PARTIAL &&
+ skb_headlen(skb) >= skb_transport_offset(skb) +
+ sizeof(struct udphdr) &&
+ udp_hdr(skb)->dest == htons(68) &&
+ skb_network_header_len(skb) >= sizeof(struct iphdr) &&
+ ip_hdr(skb)->protocol == IPPROTO_UDP &&
+ skb->protocol == htons(ETH_P_IP)) {
+ skb_checksum_help(skb);
+ /* Restore ip_summed value: tun passes it to user. */
+ skb->ip_summed = CHECKSUM_PARTIAL;
+ }
+ release_sock(sk);
+ return 1;
+}
+
/* Expects to be always run from workqueue - which acts as
* read-size critical section for our kind of RCU. */
static void handle_rx(struct vhost_net *net)
@@ -222,7 +264,7 @@ static void handle_rx(struct vhost_net *net)
vq_log = unlikely(vhost_has_feature(&net->dev, VHOST_F_LOG_ALL)) ?
vq->log : NULL;
- for (;;) {
+ while (peek_head(sock->sk)) {
head = vhost_get_vq_desc(&net->dev, vq, vq->iov,
ARRAY_SIZE(vq->iov),
&out, &in,
--
1.7.1.12.g42b7f
^ permalink raw reply related
* Re: b44: Reset due to FIFO overflow.
From: Eric Dumazet @ 2010-06-28 10:00 UTC (permalink / raw)
To: Mitchell Erblich; +Cc: James Courtier-Dutton, netdev
In-Reply-To: <5E7CE189-1002-4723-ACB2-D537B71BA5F3@earthlink.net>
Le lundi 28 juin 2010 à 02:33 -0700, Mitchell Erblich a écrit :
>
> Why wouldn't the ability to recv frames after a Recv FIFO overflow
> indicate that a reset is NOT required?
>
Do you have datasheets of all b44 chips ? I dont.
> Thus, should't it be an indication of congestion if associated with a single
> flow and either speed up (reduce latency to service) the recv side or
> slow down the xmit side?
I dont understand what you are saying. xmit is not the problem here.
And driver is flow agnostic. Its well before network stack.
Problem is we receive a spike of RX network frames (possibly UDP or some
other RX only trafic), and chip raises an RX fifo overflow _error_
indication.
Some hardware are buggy enough that such error indication is fatal and
_require_ hardware reset. Thats life. I suspect b44 driver doing a full
reset is not a random guess from driver author, but to avoid a complete
NIC lockup.
Refs:
http://bugzilla.kernel.org/show_bug.cgi?id=7696
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=216338
commit 5fc7d61aee1a7f7d3448f8fbccaa93371ebeecb0
^ permalink raw reply
* Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
From: Michael S. Tsirkin @ 2010-06-28 10:00 UTC (permalink / raw)
To: Xin, Xiaohui
Cc: Herbert Xu, Dong, Eddie, Stephen Hemminger,
netdev@vger.kernel.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, mingo@elte.hu, davem@davemloft.net,
jdike@linux.intel.com
In-Reply-To: <F2E9EB7348B8264F86B6AB8151CE2D7916B0129230@shsmsx502.ccr.corp.intel.com>
On Mon, Jun 28, 2010 at 05:56:07PM +0800, Xin, Xiaohui wrote:
> >-----Original Message-----
> >From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
> >Sent: Sunday, June 27, 2010 2:15 PM
> >To: Dong, Eddie
> >Cc: Xin, Xiaohui; Stephen Hemminger; netdev@vger.kernel.org; kvm@vger.kernel.org;
> >linux-kernel@vger.kernel.org; mst@redhat.com; mingo@elte.hu; davem@davemloft.net;
> >jdike@linux.intel.com
> >Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
> >
> >On Fri, Jun 25, 2010 at 09:03:46AM +0800, Dong, Eddie wrote:
> >>
> >> In current patch, each SKB for the assigned device (SRIOV VF or NIC or a complete
> >queue pairs) uses the buffer from guest, so it eliminates copy completely in software and
> >requires hardware to do so. If we can have an additonal place to store the buffer per skb (may
> >cause copy later on), we can do copy later on or re-post the buffer to assigned NIC driver
> >later on. But that may be not very clean either :(
> >
> >OK, if I understand you correctly then I don't think have a
> >problem. With your current patch-set you have exactly the same
> >situation when the skb->data is reallocated as a kernel buffer.
> >
>
> When will skb->data to be reallocated? May you point me the code path?
>
> >This is OK because as you correctly argue, it is a rare situation.
> >
> >With my proposal you will need to get this extra external buffer
> >in even less cases, because you'd only need to do it if the skb
> >head grows, which only happens if it becomes encapsulated.
> >So let me explain it in a bit more detail:
> >
> >Our packet starts out as a purely non-linear skb, i.e., skb->head
> >contains nothing and all the page frags come from the guest.
> >
> >During host processing we may pull data into skb->head but the
> >first frag will remain unless we pull all of it. If we did do
> >that then you would have a free external buffer anyway.
> >
> >Now in the common case the header may be modified or pulled, but
> >it very rarely grows. So you can just copy the header back into
> >the first frag just before we give it to the guest.
> >
> Since the data is still there, so recompute the page offset and size is ok, right?
Question: can devices use parts of the same page
in frags of different skbs (or for other purposes)? If they do,
we'll corrupt that memory if we try to stick the header there.
We have another option, reserve some buffers
posted by guest and use them if we need to copy
the header. This seems the most straight-forward to me.
> >Only in the case where the packet header grows (e.g., encapsulation)
> >would you need to get an extra external buffer.
> >
> >Cheers,
> >--
> >Email: Herbert Xu <herbert@gondor.apana.org.au>
> >Home Page: http://gondor.apana.org.au/~herbert/
> >PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* RE: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
From: Xin, Xiaohui @ 2010-06-28 9:56 UTC (permalink / raw)
To: Herbert Xu, Dong, Eddie
Cc: Stephen Hemminger, netdev@vger.kernel.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, mst@redhat.com, mingo@elte.hu,
davem@davemloft.net, jdike@linux.intel.com
In-Reply-To: <20100627061455.GA16782@gondor.apana.org.au>
>-----Original Message-----
>From: Herbert Xu [mailto:herbert@gondor.apana.org.au]
>Sent: Sunday, June 27, 2010 2:15 PM
>To: Dong, Eddie
>Cc: Xin, Xiaohui; Stephen Hemminger; netdev@vger.kernel.org; kvm@vger.kernel.org;
>linux-kernel@vger.kernel.org; mst@redhat.com; mingo@elte.hu; davem@davemloft.net;
>jdike@linux.intel.com
>Subject: Re: [RFC PATCH v7 01/19] Add a new structure for skb buffer from external.
>
>On Fri, Jun 25, 2010 at 09:03:46AM +0800, Dong, Eddie wrote:
>>
>> In current patch, each SKB for the assigned device (SRIOV VF or NIC or a complete
>queue pairs) uses the buffer from guest, so it eliminates copy completely in software and
>requires hardware to do so. If we can have an additonal place to store the buffer per skb (may
>cause copy later on), we can do copy later on or re-post the buffer to assigned NIC driver
>later on. But that may be not very clean either :(
>
>OK, if I understand you correctly then I don't think have a
>problem. With your current patch-set you have exactly the same
>situation when the skb->data is reallocated as a kernel buffer.
>
When will skb->data to be reallocated? May you point me the code path?
>This is OK because as you correctly argue, it is a rare situation.
>
>With my proposal you will need to get this extra external buffer
>in even less cases, because you'd only need to do it if the skb
>head grows, which only happens if it becomes encapsulated.
>So let me explain it in a bit more detail:
>
>Our packet starts out as a purely non-linear skb, i.e., skb->head
>contains nothing and all the page frags come from the guest.
>
>During host processing we may pull data into skb->head but the
>first frag will remain unless we pull all of it. If we did do
>that then you would have a free external buffer anyway.
>
>Now in the common case the header may be modified or pulled, but
>it very rarely grows. So you can just copy the header back into
>the first frag just before we give it to the guest.
>
Since the data is still there, so recompute the page offset and size is ok, right?
>Only in the case where the packet header grows (e.g., encapsulation)
>would you need to get an extra external buffer.
>
>Cheers,
>--
>Email: Herbert Xu <herbert@gondor.apana.org.au>
>Home Page: http://gondor.apana.org.au/~herbert/
>PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH -next] netxen: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 9:33 UTC (permalink / raw)
To: netdev; +Cc: Amerigo Wang, Amit Kumar Salecha
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/netxen/netxen_nic_ethtool.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/netxen/netxen_nic_ethtool.c b/drivers/net/netxen/netxen_nic_ethtool.c
index 20f7c58..6d94ee5 100644
--- a/drivers/net/netxen/netxen_nic_ethtool.c
+++ b/drivers/net/netxen/netxen_nic_ethtool.c
@@ -887,12 +887,19 @@ static int netxen_nic_set_flags(struct net_device *netdev, u32 data)
struct netxen_adapter *adapter = netdev_priv(netdev);
int hw_lro;
+ if (data & ~ETH_FLAG_LRO)
+ return -EOPNOTSUPP;
+
if (!(adapter->capabilities & NX_FW_CAPABILITY_HW_LRO))
return -EINVAL;
- ethtool_op_set_flags(netdev, data);
-
- hw_lro = (data & ETH_FLAG_LRO) ? NETXEN_NIC_LRO_ENABLED : 0;
+ if (data & ETH_FLAG_LRO) {
+ hw_lro = NETXEN_NIC_LRO_ENABLED;
+ netdev->features |= NETIF_F_LRO;
+ } else {
+ hw_lro = 0;
+ netdev->features &= ~NETIF_F_LRO;
+ }
if (netxen_config_hw_lro(adapter, hw_lro))
return -EIO;
--
1.5.5.6
^ permalink raw reply related
* Re: b44: Reset due to FIFO overflow.
From: Mitchell Erblich @ 2010-06-28 9:33 UTC (permalink / raw)
To: Eric Dumazet; +Cc: James Courtier-Dutton, netdev
In-Reply-To: <1277716394.4235.235.camel@edumazet-laptop>
On Jun 28, 2010, at 2:13 AM, Eric Dumazet wrote:
> Le lundi 28 juin 2010 à 08:41 +0100, James Courtier-Dutton a écrit :
>> Hi,
>>
>> Reference:
>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/279102
>>
>> I can see this bug and can reproduce it 100% on demand.
>> The problem seems to be that when the b44 has a incoming FIFO buffer
>> overflow, it resets the entire card, dis-associates with the access
>> point and therefore takes some time before it can pass traffic again.
>> Can anyone point me to some code that would just recover the FIFO
>> instead of reset the entire card?
>>
>> I am a kernel developer, but I don't have any data sheets on this card
>> so was hoping someone with more knowledge of its workings, could help
>> me.
>>
>> I can then test it, and see if it is a good fix or not.
>>
>
> Hi
>
> Problem is we dont know if a Receive Fifo overflow is a minor or major
> indication from b44 chip.
>
> A minor indication would be : Chip tells us one or more frame were lost.
> No special action needed from driver.
>
> A major indication (as of current implemented in b44 driver) is :
> I am completely out of order and need a reset. Please do it.
>
> Patch to switch from major to minor indication is easy, but we dont know
> if its valid or not.
>
> diff --git a/drivers/net/b44.h b/drivers/net/b44.h
> index e1905a4..514dc3a 100644
> --- a/drivers/net/b44.h
> +++ b/drivers/net/b44.h
> @@ -42,7 +42,7 @@
> #define ISTAT_EMAC 0x04000000 /* EMAC Interrupt */
> #define ISTAT_MII_WRITE 0x08000000 /* MII Write Interrupt */
> #define ISTAT_MII_READ 0x10000000 /* MII Read Interrupt */
> -#define ISTAT_ERRORS (ISTAT_DSCE|ISTAT_DATAE|ISTAT_DPE|ISTAT_RDU|ISTAT_RFO|ISTAT_TFU)
> +#define ISTAT_ERRORS (ISTAT_DSCE|ISTAT_DATAE|ISTAT_DPE|ISTAT_RDU|ISTAT_TFU)
> #define B44_IMASK 0x0024UL /* Interrupt Mask */
> #define IMASK_DEF (ISTAT_ERRORS | ISTAT_TO | ISTAT_RX | ISTAT_TX)
> #define B44_GPTIMER 0x0028UL /* General Purpose Timer */
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Why wouldn't the ability to recv frames after a Recv FIFO overflow
indicate that a reset is NOT required?
Thus, should't it be an indication of congestion if associated with a single
flow and either speed up (reduce latency to service) the recv side or
slow down the xmit side?
Mitchell Erblich
^ permalink raw reply
* [PATCH -next] qlcnic: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 9:31 UTC (permalink / raw)
To: netdev; +Cc: Amerigo Wang, Amit Kumar Salecha, Anirban Chakraborty
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/qlcnic/qlcnic_ethtool.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c
index d4e803e..b9d5acb 100644
--- a/drivers/net/qlcnic/qlcnic_ethtool.c
+++ b/drivers/net/qlcnic/qlcnic_ethtool.c
@@ -983,12 +983,19 @@ static int qlcnic_set_flags(struct net_device *netdev, u32 data)
struct qlcnic_adapter *adapter = netdev_priv(netdev);
int hw_lro;
+ if (data & ~ETH_FLAG_LRO)
+ return -EOPNOTSUPP;
+
if (!(adapter->capabilities & QLCNIC_FW_CAPABILITY_HW_LRO))
return -EINVAL;
- ethtool_op_set_flags(netdev, data);
-
- hw_lro = (data & ETH_FLAG_LRO) ? QLCNIC_LRO_ENABLED : 0;
+ if (data & ETH_FLAG_LRO) {
+ hw_lro = QLCNIC_LRO_ENABLED;
+ netdev->features |= NETIF_F_LRO;
+ } else {
+ hw_lro = 0;
+ netdev->features &= ~NETIF_F_LRO;
+ }
if (qlcnic_config_hw_lro(adapter, hw_lro))
return -EIO;
--
1.5.5.6
^ permalink raw reply related
* [PATCH -next] vmxnet3: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 9:29 UTC (permalink / raw)
To: netdev; +Cc: Amerigo Wang, Shreyas Bhatewara
Return EOPNOTSUPP in ethtool_ops->set_flags.
Fix coding style while at it.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/vmxnet3/vmxnet3_ethtool.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/vmxnet3/vmxnet3_ethtool.c b/drivers/net/vmxnet3/vmxnet3_ethtool.c
index 3935c44..8a71a21 100644
--- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
+++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
@@ -276,16 +276,21 @@ vmxnet3_get_strings(struct net_device *netdev, u32 stringset, u8 *buf)
}
static u32
-vmxnet3_get_flags(struct net_device *netdev) {
+vmxnet3_get_flags(struct net_device *netdev)
+{
return netdev->features;
}
static int
-vmxnet3_set_flags(struct net_device *netdev, u32 data) {
+vmxnet3_set_flags(struct net_device *netdev, u32 data)
+{
struct vmxnet3_adapter *adapter = netdev_priv(netdev);
u8 lro_requested = (data & ETH_FLAG_LRO) == 0 ? 0 : 1;
u8 lro_present = (netdev->features & NETIF_F_LRO) == 0 ? 0 : 1;
+ if (data & ~ETH_FLAG_LRO)
+ return -EOPNOTSUPP;
+
if (lro_requested ^ lro_present) {
/* toggle the LRO feature*/
netdev->features ^= NETIF_F_LRO;
--
1.5.5.6
^ permalink raw reply related
* [PATCH -next] bnx2x: fail when try to setup unsupported features
From: Stanislaw Gruszka @ 2010-06-28 9:28 UTC (permalink / raw)
To: netdev; +Cc: Amerigo Wang, Eilon Greenstein
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/bnx2x_main.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bnx2x_main.c b/drivers/net/bnx2x_main.c
index 57ff5b3..0809f6c 100644
--- a/drivers/net/bnx2x_main.c
+++ b/drivers/net/bnx2x_main.c
@@ -10982,6 +10982,9 @@ static int bnx2x_set_flags(struct net_device *dev, u32 data)
int changed = 0;
int rc = 0;
+ if (data & ~(ETH_FLAG_LRO | ETH_FLAG_RXHASH))
+ return -EOPNOTSUPP;
+
if (bp->recovery_state != BNX2X_RECOVERY_DONE) {
printk(KERN_ERR "Handling parity error recovery. Try again later\n");
return -EAGAIN;
--
1.5.5.6
^ permalink raw reply related
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