* Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: Ben Hutchings @ 2012-07-30 19:19 UTC (permalink / raw)
To: KY Srinivasan
Cc: Olaf Hering, gregkh@linuxfoundation.org,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
apw@canonical.com, netdev@vger.kernel.org
In-Reply-To: <426367E2313C2449837CD2DE46E7EAF9236AA7DB@SN2PRD0310MB382.namprd03.prod.outlook.com>
On Mon, Jul 30, 2012 at 06:32:15PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Olaf Hering [mailto:olaf@aepfle.de]
> > Sent: Monday, July 30, 2012 2:03 PM
> > To: KY Srinivasan
> > Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> > ben@decadent.org.uk
> > Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> > KVP_OP_SET_IP_INFO
> >
> > On Tue, Jul 24, K. Y. Srinivasan wrote:
> >
> > > + /*
> > > + * Set the configuration for the specified interface with
> > > + * the information provided. Since there is no standard
> > > + * way to configure an interface, we will have an external
> > > + * script that does the job of configuring the interface and
> > > + * flushing the configuration.
> > > + *
> > > + * The parameters passed to this external script are:
> > > + * 1. A configuration file that has the specified configuration.
> >
> > Maybe this should be written as 'A info file that has the requested
> > network configuration' or something like that.
>
> That is the idea. This configuration file simply reflects all the
> information we have perhaps with some additional constant
> information. The script is free to ignore what it does not need.
[...]
This does not strike me as a sensible interface. If scripts are
'free to ignore' information then the KVP interface becomes unreliable
as a means for managing networking on Linux guests. I would suggest
that at the least the script should be able to report that it did not
recognise some parts of the configuration. This would be logged
and/or reported back to the hypervisor.
(This is separate from the issue of constant configuration lines;
for some distributions the script might recognise but ignore them
because they have no use on that distribution. I don't see the
point in constant lines, but they don't seem to result in any
unreliability.)
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert Camus
^ permalink raw reply
* RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: KY Srinivasan @ 2012-07-30 19:12 UTC (permalink / raw)
To: Olaf Hering
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, apw@canonical.com,
netdev@vger.kernel.org, ben@decadent.org.uk
In-Reply-To: <20120730173323.GA533@aepfle.de>
> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Monday, July 30, 2012 1:33 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> ben@decadent.org.uk
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
>
> On Tue, Jul 24, K. Y. Srinivasan wrote:
>
> > +static char *kvp_get_if_name(char *guid)
> > +{
> > + DIR *dir;
> > + struct dirent *entry;
> > + FILE *file;
> > + char *p, *q, *x;
> > + char *if_name = NULL;
> > + char buf[256];
> > + char *kvp_net_dir = "/sys/class/net/";
> > + char dev_id[100];
>
> Perhaps this can be written as char dev_id[100] = "short string";?
Why?
>
> > +
> > + dir = opendir(kvp_net_dir);
> > + if (dir == NULL)
> > + return NULL;
> > +
> > + memset(dev_id, 0, sizeof(dev_id));
> > + strcat(dev_id, kvp_net_dir);
> > + q = dev_id + strlen(kvp_net_dir);
> > +
> > + while ((entry = readdir(dir)) != NULL) {
> > + /*
> > + * Set the state for the next pass.
> > + */
> > + *q = '\0';
> > + strcat(dev_id, entry->d_name);
> > + strcat(dev_id, "/device/device_id");
>
> Maybe this (and other) strcat should be changed to snprintf?
Already done.
>
> > +
> > + file = fopen(dev_id, "r");
> > + if (file == NULL)
> > + continue;
> > +
> > + p = fgets(buf, sizeof(buf), file);
> > + if (p) {
> > + x = strchr(p, '\n');
> > + if (x)
> > + *x = '\0';
> > +
> > + if (!strcmp(p, guid)) {
> > + /*
> > + * Found the guid match; return the interface
> > + * name. The caller will free the memory.
> > + */
> > + if_name = strdup(entry->d_name);
> > + break;
>
> This will leave 'file' open.
> I have seen it in some other place as well.
I will audit all instances and fix it.
>
> > + }
> > + }
> > + fclose(file);
> > + }
> > +
> > + closedir(dir);
> > + return if_name;
> > +}
> > +
> > +/*
> > + * Retrieve the MAC address given the interface name.
> > + */
> > +
> > +static char *kvp_if_name_to_mac(char *if_name)
> > +{
> > + FILE *file;
> > + char *p, *x;
> > + char buf[256];
> > + char addr_file[100];
> > + int i;
> > + char *mac_addr = NULL;
> > +
> > + memset(addr_file, 0, sizeof(addr_file));
> > + strcat(addr_file, "/sys/class/net/");
> > + strcat(addr_file, if_name);
> > + strcat(addr_file, "/address");
>
> snprintf?
Already fixed.
>
> > +
> > + file = fopen(addr_file, "r");
> > + if (file == NULL)
> > + return NULL;
> > +
> > + p = fgets(buf, sizeof(buf), file);
> > + if (p) {
> > + x = strchr(p, '\n');
> > + if (x)
> > + *x = '\0';
> > + for (i = 0; i < strlen(p); i++)
> > + p[i] = toupper(p[i]);
> > + mac_addr = strdup(p);
> > + }
> > +
> > + fclose(file);
> > + return mac_addr;
> > +}
> > +
> > +
> > static void kvp_process_ipconfig_file(char *cmd,
> > char *config_buf, int len,
> > int element_size, int offset)
> > @@ -800,6 +910,314 @@ getaddr_done:
> > }
> >
> >
> > +static int expand_ipv6(char *addr, int type)
> > +{
> > + int ret;
> > + struct in6_addr v6_addr;
> > +
> > + ret = inet_pton(AF_INET6, addr, &v6_addr);
> > +
> > + if (ret != 1) {
> > + if (type == NETMASK)
> > + return 1;
> > + return 0;
> > + }
> > +
> > + sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
> > + "%02x%02x:%02x%02x:%02x%02x",
> > + (int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],
> > + (int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],
> > + (int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],
> > + (int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],
> > + (int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],
> > + (int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],
> > + (int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],
> > + (int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);
> > +
> > + return 1;
> > +
> > +}
> > +
> > +static int is_ipv4(char *addr)
> > +{
> > + int ret;
> > + struct in_addr ipv4_addr;
> > +
> > + ret = inet_pton(AF_INET, addr, &ipv4_addr);
> > +
> > + if (ret == 1)
> > + return 1;
> > + return 0;
> > +}
> > +
> > +static int parse_ip_val_buffer(char *in_buf, int *offset,
> > + char *out_buf, int out_len)
> > +{
> > + char *x;
> > + char *start;
> > +
> > + /*
> > + * in_buf has sequence of characters that are seperated by
> > + * the character ';'. The last sequence does not have the
> > + * terminating ";" character.
> > + */
> > + start = in_buf + *offset;
> > +
> > + x = strchr(start, ';');
> > + if (x)
> > + *x = 0;
> > + else
> > + x = start + strlen(start);
> > +
> > + if (strlen(start) != 0) {
> > + int i = 0;
> > + /*
> > + * Get rid of leading spaces.
> > + */
> > + while (start[i] == ' ')
> > + i++;
> > +
> > + if ((x - start) <= out_len) {
> > + strcpy(out_buf, (start + i));
> > + *offset += (x - start) + 1;
> > + return 1;
> > + }
> > + }
> > + return 0;
> > +}
> > +
> > +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> > +{
> > + char str[256];
> > + int error;
> > +
> > + memset(str, 0, sizeof(str));
> > + strcat(str, s1);
> > + if (s2 != NULL)
> > + strcat(str, s2);
> > + strcat(str, "=");
> > + strcat(str, s3);
> > + strcat(str, "\n");
> > +
> > + error = fputs(str, f);
> > + if (error == EOF)
> > + return HV_E_FAIL;
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static int process_ip_string(FILE *f, char *ip_string, int type)
> > +{
> > + int error = 0;
> > + char addr[INET6_ADDRSTRLEN];
> > + int i = 0;
> > + int j = 0;
> > + char str[256];
> > + char sub_str[10];
> > + int offset = 0;
> > +
> > + memset(addr, 0, sizeof(addr));
> > +
> > + while (parse_ip_val_buffer(ip_string, &offset, addr,
> > + (MAX_IP_ADDR_SIZE * 2))) {
> > + memset(sub_str, 0, sizeof(sub_str));
> > + memset(str, 0, sizeof(str));
> > +
> > + if (is_ipv4(addr)) {
> > + switch (type) {
> > + case IPADDR:
> > + strcat(str, "IPADDR");
> > + break;
> > + case NETMASK:
> > + strcat(str, "NETMASK");
> > + break;
> > + case GATEWAY:
> > + strcat(str, "GATEWAY");
> > + break;
> > + case DNS:
> > + strcat(str, "DNS");
> > + break;
> > + }
> > + if (i != 0) {
> > + if (type != DNS)
> > + sprintf(sub_str, "_%d", i++);
> > + else
> > + sprintf(sub_str, "%d", ++i);
> > + } else if (type == DNS) {
> > + sprintf(sub_str, "%d", ++i);
> > + }
> > +
> > +
> > + } else if (expand_ipv6(addr, type)) {
> > + switch (type) {
> > + case IPADDR:
> > + strcat(str, "IPV6ADDR");
> > + break;
> > + case NETMASK:
> > + strcat(str, "IPV6NETMASK");
> > + break;
> > + case GATEWAY:
> > + strcat(str, "IPV6_DEFAULTGW");
> > + break;
> > + case DNS:
> > + strcat(str, "DNS");
> > + break;
> > + }
> > + if ((j != 0) || (type == DNS)) {
> > + if (type != DNS)
> > + sprintf(sub_str, "_%d", j++);
> > + else
> > + sprintf(sub_str, "%d", ++i);
> > + } else if (type == DNS) {
> > + sprintf(sub_str, "%d", ++i);
> > + }
> > + } else {
> > + return HV_INVALIDARG;
> > + }
> > +
> > + error = kvp_write_file(f, str, sub_str, addr);
> > + if (error)
> > + return error;
> > + memset(addr, 0, sizeof(addr));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value
> *new_val)
> > +{
> > + int error = 0;
> > + char if_file[50];
> > + FILE *file;
> > + char cmd[512];
> > + char *mac_addr;
> > +
> > + /*
> > + * Set the configuration for the specified interface with
> > + * the information provided. Since there is no standard
> > + * way to configure an interface, we will have an external
> > + * script that does the job of configuring the interface and
> > + * flushing the configuration.
> > + *
> > + * The parameters passed to this external script are:
> > + * 1. A configuration file that has the specified configuration.
> > + *
> > + * We will embed the name of the interface in the configuration
> > + * file: ifcfg-ethx (where ethx is the interface name).
> > + *
> > + * Here is the format of the ip configuration file:
> > + *
> > + * HWADDR=macaddr
> > + * BOOTPROTO=dhcp (dhcp enabled for the interface)
> > + * NM_CONTROLLED=no (this interface will not be controlled by NM)
> > + * PEERDNS=yes
> > + * IPADDR_x=ipaddr
> > + * NETMASK_x=netmask
> > + * GATEWAY_x=gateway
> > + * DNSx=dns
> > + *
> > + * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> > + * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> > + * IPV6NETMASK.
> > + */
> > +
> > + memset(if_file, 0, sizeof(if_file));
> > + strcat(if_file, "/var/opt/hyperv/ifcfg-");
> > + strcat(if_file, if_name);
> > +
> > + file = fopen(if_file, "w");
> > +
> > + if (file == NULL) {
> > + syslog(LOG_ERR, "Failed to open config file");
> > + return HV_E_FAIL;
> > + }
> > +
> > + /*
> > + * First write out the MAC address.
> > + */
> > +
> > + mac_addr = kvp_if_name_to_mac(if_name);
> > + if (mac_addr == NULL) {
> > + error = HV_E_FAIL;
> > + goto setval_error;
> > + }
> > +
> > + error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
> > + if (error)
> > + goto setval_error;
> > +
> > + error = kvp_write_file(file, "PEERDNS", NULL, "yes");
> > + if (error)
> > + goto setval_error;
> > +
> > + if (new_val->dhcp_enabled) {
> > + error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
> > + if (error)
> > + goto setval_error;
> > +
> > + /*
> > + * We are done!.
> > + */
> > + goto setval_done;
> > + }
> > +
> > + /*
> > + * Write the configuration for ipaddress, netmask, gateway and
> > + * name servers.
> > + */
> > +
> > + error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> > + if (error)
> > + goto setval_error;
> > +
> > + error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> > + if (error)
> > + goto setval_error;
> > +
> > +setval_done:
> > + free(mac_addr);
> > + fclose(file);
> > +
> > + /*
> > + * Now that we have populated the configuration file,
> > + * invoke the external script to do its magic.
> > + */
> > +
> > + memset(cmd, 0, sizeof(cmd));
> > + strcat(cmd, "/sbin/hv_set_ifconfig ");
> > + strcat(cmd, if_file);
>
> The new patch should use "%s %s", not "%s%s" as format string.
>
>
>
Thanks Olaf,
K. Y
^ permalink raw reply
* v3.5: NETDEV WATCHDOG: eth0 (r8169): transmit queue 0 timed out
From: George Spelvin @ 2012-07-30 19:02 UTC (permalink / raw)
To: netdev; +Cc: linux
This actually isn't new to 3.5, but I figured I'd mention it again
anyway. The machine serves firewalll duty. A quad-tulip fast ethernet
card serves the external interfaces and DMZ, while an on-board Realtek
gigabit interface serves the main LAN. The latter is hitting a transmit
queue timeout.
02:00.0 Ethernet controller [0200]: Realtek Semiconductor Co., Ltd. RTL8111/8168B PCI Express Gigabit Ethernet controller [10ec:8168] (rev 01)
Soon after startup, the following WARN_ONCE triggers:
r8169 0000:02:00.0: inside: link down
r8169 0000:02:00.0: inside: link down
IPv6: ADDRCONF(NETDEV_UP): inside: link is not ready
r8169 0000:02:00.0: inside: link up
IPv6: ADDRCONF(NETDEV_CHANGE): inside: link becomes ready
net dmz: Setting full-duplex based on MII#1 link partner capability of 45e1
net cable: Setting full-duplex based on MII#1 link partner capability of 41e1
postgres (5652): /proc/5652/oom_adj is deprecated, please use /proc/5652/oom_score_adj instead.
pps pps0: new PPS source serial3 at ID 0
pps pps0: source "/dev/ttyS3" added
pps pps1: new PPS source serial4 at ID 1
pps pps1: source "/dev/ttyS4" added
device cable entered promiscuous mode
device dmz entered promiscuous mode
nf_conntrack: automatic helper assignment is deprecated and it will be removed soon. Use the iptables CT target to attach helpers instead.
ntop[6305]: segfault at 0 ip 00000000f750644b sp 00000000f01fbd50 error 4 in libntop-4.99.3.so[f74bf000+7e000]
device cable left promiscuous mode
device dmz left promiscuous mode
------------[ cut here ]------------
WARNING: at net/sched/sch_generic.c:255 dev_watchdog+0xe9/0x15c()
Hardware name: MS-7376
NETDEV WATCHDOG: inside (r8169): transmit queue 0 timed out
Pid: 0, comm: swapper/3 Not tainted 3.5.0-00020-g7bd6501 #160
Call Trace:
<IRQ> [<ffffffff8131d16e>] ? dev_watchdog+0xe9/0x15c
[<ffffffff810254bd>] ? warn_slowpath_common+0x71/0x85
[<ffffffff8131d085>] ? netif_tx_lock+0x7a/0x7a
[<ffffffff81025535>] ? warn_slowpath_fmt+0x45/0x4a
[<ffffffff8103ce9c>] ? hrtimer_interrupt+0x100/0x1a4
[<ffffffff8131d072>] ? netif_tx_lock+0x67/0x7a
[<ffffffff8131d16e>] ? dev_watchdog+0xe9/0x15c
[<ffffffff8104e764>] ? clockevents_program_event+0x9a/0xb6
[<ffffffff8102e9bc>] ? run_timer_softirq+0x17e/0x20b
[<ffffffff8102a945>] ? __do_softirq+0x80/0x102
[<ffffffff81411d4c>] ? call_softirq+0x1c/0x30
[<ffffffff810032f4>] ? do_softirq+0x2c/0x60
[<ffffffff8102ab78>] ? irq_exit+0x3a/0x91
[<ffffffff81003141>] ? do_IRQ+0x81/0x97
[<ffffffff81410367>] ? common_interrupt+0x67/0x67
<EOI> [<ffffffff81007d63>] ? default_idle+0x1e/0x32
[<ffffffff81007e87>] ? amd_e400_idle+0xb7/0xd4
[<ffffffff81008501>] ? cpu_idle+0x58/0x98
---[ end trace 9ca7e03889ac2abb ]---
r8169 0000:02:00.0: inside: link up
udevd[30490]: starting version 175
UDP: short packet: From 151.60.217.248:59748 27692/73 to 71.41.210.146:43935
r8169 0000:02:00.0: inside: link up
There are a few local patches, but only two are anywhere close to the
network interface, and they are test patches designed to fix this.
Should I get rid of them?
It's a production system, so I don't like to bounce it too often, but I can
test after hours.
One local patch I've thought about writing is something to count
how often this happens (probably in sysfs).
commit 526ef2edd6ffb8f529256a033ca3a266530739d3
Author: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed May 23 22:18:35 2012 +0200
r8169: avoid clearing the end of Tx descriptor ring marker bit.
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d7a04e0..9c35130 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5346,7 +5346,7 @@ static void rtl8169_unmap_tx_skb(struct device *d, struct ring_info *tx_skb,
dma_unmap_single(d, le64_to_cpu(desc->addr), len, DMA_TO_DEVICE);
- desc->opts1 = 0x00;
+ desc->opts1 &= cpu_to_le32(RingEnd);
desc->opts2 = 0x00;
desc->addr = 0x00;
tx_skb->len = 0;
commit 7bd6501f52c2c0382b41fe77b9bd003ff768a44c
Author: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed May 23 23:21:13 2012 +0200
r8169: TxPoll hack rework.
I don't want to try and convince myself that it is completely safe to
issue a TxPoll request from the NAPI handler right in the middle of
a start_xmit, whence the tx_lock.
Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 9c35130..be5cd33 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5545,19 +5545,20 @@ static netdev_tx_t rtl8169_start_xmit(struct sk_buff *skb,
status = opts[0] | len | (RingEnd * !((entry + 1) % NUM_TX_DESC));
txd->opts1 = cpu_to_le32(status);
+ smp_wmb();
+
tp->cur_tx += frags + 1;
- wmb();
+ smp_wmb();
RTL_W8(TxPoll, NPQ);
mmiowb();
if (!TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
- /* Avoid wrongly optimistic queue wake-up: rtl_tx thread must
- * not miss a ring update when it notices a stopped queue.
- */
- smp_wmb();
+ /* rtl_tx thread must not miss a ring update when it notices
+ * a stopped queue. The TxPoll hack requires the smp_wmb
+ * above so we can go ahead. */
netif_stop_queue(dev);
/* Sync with rtl_tx:
* - publish queue status and cur_tx ring index (write barrier)
@@ -5641,22 +5642,36 @@ struct rtl_txc {
static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
{
struct rtl8169_stats *tx_stats = &tp->tx_stats;
- unsigned int dirty_tx, tx_left;
+ unsigned int dirty_tx, cur_tx;
struct rtl_txc txc = { 0, 0 };
dirty_tx = tp->dirty_tx;
- smp_rmb();
- tx_left = tp->cur_tx - dirty_tx;
-
- while (tx_left > 0) {
+xmit_race:
+ for (cur_tx = tp->cur_tx; dirty_tx != cur_tx; dirty_tx++) {
unsigned int entry = dirty_tx % NUM_TX_DESC;
struct ring_info *tx_skb = tp->tx_skb + entry;
u32 status;
- rmb();
status = le32_to_cpu(tp->TxDescArray[entry].opts1);
- if (status & DescOwn)
+
+ /* 8168 (only ?) hack: TxPoll requests are lost when the Tx
+ * packets are too close. Let's kick an extra TxPoll request
+ * when a burst of start_xmit activity is detected (if it is
+ * not detected, it is slow enough).
+ * The NPQ bit is cleared automatically by the chipset.
+ * The code assumes that the chipset is sane enough to clear
+ * it at a sensible time.*/
+ if (unlikely(status & DescOwn)) {
+ void __iomem *ioaddr = tp->mmio_addr;
+
+ if (!(RTL_R8(TxPoll) & NPQ)) {
+ netif_tx_lock(dev);
+ RTL_W8(TxPoll, NPQ);
+ netif_tx_unlock(dev);
+ goto done;
+ }
break;
+ }
rtl8169_unmap_tx_skb(&tp->pci_dev->dev, tx_skb,
tp->TxDescArray + entry);
@@ -5668,10 +5683,15 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
dev_kfree_skb(skb);
tx_skb->skb = NULL;
}
- dirty_tx++;
- tx_left--;
}
+ /* Rationale: if chipset stopped DMAing, enforce TxPoll write either
+ * here or in start_xmit. If chipset is still DMAing, this code
+ * will be run later anyway. */
+ smp_mb();
+ if (cur_tx != tp->cur_tx)
+ goto xmit_race;
+done:
u64_stats_update_begin(&tx_stats->syncp);
tx_stats->packets += txc.packets;
tx_stats->bytes += txc.bytes;
@@ -5693,17 +5713,6 @@ static void rtl_tx(struct net_device *dev, struct rtl8169_private *tp)
TX_FRAGS_READY_FOR(tp, MAX_SKB_FRAGS)) {
netif_wake_queue(dev);
}
- /*
- * 8168 hack: TxPoll requests are lost when the Tx packets are
- * too close. Let's kick an extra TxPoll request when a burst
- * of start_xmit activity is detected (if it is not detected,
- * it is slow enough). -- FR
- */
- if (tp->cur_tx != dirty_tx) {
- void __iomem *ioaddr = tp->mmio_addr;
-
- RTL_W8(TxPoll, NPQ);
- }
}
}
^ permalink raw reply related
* [PATCH] bridge: make port attributes const
From: Stephen Hemminger @ 2012-07-30 18:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Simple table that can be marked const.
Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
---
net/bridge/br_sysfs_br.c | 136 +++++++++++++++++++++++------------------------
net/bridge/br_sysfs_if.c | 6 +-
3 files changed, 73 insertions(+), 71 deletions(-)
--- a/net/bridge/br_sysfs_if.c 2012-07-19 15:35:29.062250368 -0700
+++ b/net/bridge/br_sysfs_if.c 2012-07-19 15:36:10.843427691 -0700
@@ -27,7 +27,7 @@ struct brport_attribute {
};
#define BRPORT_ATTR(_name,_mode,_show,_store) \
-struct brport_attribute brport_attr_##_name = { \
+const struct brport_attribute brport_attr_##_name = { \
.attr = {.name = __stringify(_name), \
.mode = _mode }, \
.show = _show, \
@@ -164,7 +164,7 @@ static BRPORT_ATTR(multicast_router, S_I
store_multicast_router);
#endif
-static struct brport_attribute *brport_attrs[] = {
+static const struct brport_attribute *brport_attrs[] = {
&brport_attr_path_cost,
&brport_attr_priority,
&brport_attr_port_id,
@@ -241,7 +241,7 @@ const struct sysfs_ops brport_sysfs_ops
int br_sysfs_addif(struct net_bridge_port *p)
{
struct net_bridge *br = p->br;
- struct brport_attribute **a;
+ const struct brport_attribute **a;
int err;
err = sysfs_create_link(&p->kobj, &br->dev->dev.kobj,
^ permalink raw reply
* RE: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: KY Srinivasan @ 2012-07-30 18:32 UTC (permalink / raw)
To: Olaf Hering
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, apw@canonical.com,
netdev@vger.kernel.org, ben@decadent.org.uk
In-Reply-To: <20120730180326.GB533@aepfle.de>
> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Monday, July 30, 2012 2:03 PM
> To: KY Srinivasan
> Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; apw@canonical.com; netdev@vger.kernel.org;
> ben@decadent.org.uk
> Subject: Re: [PATCH 13/17] Tools: hv: Implement the KVP verb -
> KVP_OP_SET_IP_INFO
>
> On Tue, Jul 24, K. Y. Srinivasan wrote:
>
> > + /*
> > + * Set the configuration for the specified interface with
> > + * the information provided. Since there is no standard
> > + * way to configure an interface, we will have an external
> > + * script that does the job of configuring the interface and
> > + * flushing the configuration.
> > + *
> > + * The parameters passed to this external script are:
> > + * 1. A configuration file that has the specified configuration.
>
> Maybe this should be written as 'A info file that has the requested
> network configuration' or something like that.
That is the idea. This configuration file simply reflects all the information we have
perhaps with some additional constant information. The script is free to ignore what
it does not need.
>
> > + *
> > + * We will embed the name of the interface in the configuration
> > + * file: ifcfg-ethx (where ethx is the interface name).
>
> I think the intention here is to use the generated file as is. Depending
> on the distro in the guest the file may need some processing. So I think
> the actual interface name should also be part of the file.
That is not the intention although on some distros, the format of this file
may be closer to the distro specific configuration file than others. I will
however include the name of the interface in the file as well.
>
> > + *
> > + * Here is the format of the ip configuration file:
> > + *
> > + * HWADDR=macaddr
> > + * BOOTPROTO=dhcp (dhcp enabled for the interface)
>
> While BOOTPROTO= is used in current network config files, its meaning
> there is unrelated to what its meant here. Here it means DHCP=yes/no, so
> I think the file should contain just that. And as the code is written
> now BOOTPROTO= is optional, which is not mentioned in the comment.
I will fix this.
>
> > + * NM_CONTROLLED=no (this interface will not be controlled by NM)
>
> I think this is not up to kvp_deamon to decide what controls the
> interface. Maybe one day NM is sufficiently advanced technology that it
> can cope with such requests?
> The helper script should decide if the NM flag should be written to the
> final config file.
As I noted earlier, the external script can choose to interpret the contents of this
file in a way that it makes sense for the distribution. Maybe, I will get rid of all the
constant information.
>
> > + * PEERDNS=yes
> > + * IPADDR_x=ipaddr
> > + * NETMASK_x=netmask
> > + * GATEWAY_x=gateway
>
> This should mention that its either IPADDR=ipaddr1 or 'IPADDR=ipaddr1;
> IPADDR_1=ipaddr2'
>
> > + * DNSx=dns
>
> This should mention that its either DNS=ipaddr1 or 'DNS=ipaddr1;
> DNS1=ipaddr2'
I take it that you want the comments to be more explicit on the format.
>
> Olaf
>
>
Thank you Olaf,
K. Y
^ permalink raw reply
* Re: [RFC v2 2/2] net: Add support for NTB virtual ethernet device
From: Jon Mason @ 2012-07-30 18:19 UTC (permalink / raw)
To: Jiri Pirko; +Cc: linux-kernel, netdev, linux-pci, Dave Jiang
In-Reply-To: <20120730140216.GA1550@minipsycho.orion>
On Mon, Jul 30, 2012 at 04:02:16PM +0200, Jiri Pirko wrote:
> Mon, Jul 30, 2012 at 02:26:34AM CEST, jon.mason@intel.com wrote:
> >+static int __devinit ntb_netdev_probe(struct pci_dev *pdev)
> >+{
> >+ struct net_device *ndev;
> >+ struct ntb_netdev *dev;
> >+ int rc;
> >+
> >+ ndev = alloc_etherdev(sizeof(struct ntb_netdev));
> >+ if (!ndev)
> >+ return -ENOMEM;
> >+
> >+ dev = netdev_priv(ndev);
> >+ dev->ndev = ndev;
> >+ dev->pdev = pdev;
> >+ BUG_ON(!dev->pdev);
> >+ ndev->features = NETIF_F_HIGHDMA;
> >+
> >+ //ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> ^^ I guess you forgot to un-comment this.
Oops. An easy fix. Anything else?
Thanks,
Jon
^ permalink raw reply
* Re: [RFC v2 1/2] PCI-Express Non-Transparent Bridge Support
From: Jon Mason @ 2012-07-30 18:15 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-kernel, netdev, linux-pci, Dave Jiang
In-Reply-To: <CAErSpo4p_hxvCeVGqiTXVq3itXepheQZerpqRHwLvG6A7R2Lcg@mail.gmail.com>
On Mon, Jul 30, 2012 at 10:50:13AM -0600, Bjorn Helgaas wrote:
> On Sun, Jul 29, 2012 at 6:26 PM, Jon Mason <jon.mason@intel.com> wrote:
> > A PCI-Express non-transparent bridge (NTB) is a point-to-point PCIe bus
> > connecting 2 systems, providing electrical isolation between the two subsystems.
> > A non-transparent bridge is functionally similar to a transparent bridge except
> > that both sides of the bridge have their own independent address domains. The
> > host on one side of the bridge will not have the visibility of the complete
> > memory or I/O space on the other side of the bridge. To communicate across the
> > non-transparent bridge, each NTB endpoint has one (or more) apertures exposed to
> > the local system. Writes to these apertures are mirrored to memory on the
> > remote system. Communications can also occur through the use of doorbell
> > registers that initiate interrupts to the alternate domain, and scratch-pad
> > registers accessible from both sides.
> >
> > The NTB device driver is needed to configure these memory windows, doorbell, and
> > scratch-pad registers as well as use them in such a way as they can be turned
> > into a viable communication channel to the remote system. ntb_hw.[ch]
> > determines the usage model (NTB to NTB or NTB to Root Port) and abstracts away
> > the underlying hardware to provide access and a common interface to the doorbell
> > registers, scratch pads, and memory windows. These hardware interfaces are
> > exported so that other, non-mainlined kernel drivers can access these.
> > ntb_transport.[ch] also uses the exported interfaces in ntb_hw.[ch] to setup a
> > communication channel(s) and provide a reliable way of transferring data from
> > one side to the other, which it then exports so that "client" drivers can access
> > them. These client drivers are used to provide a standard kernel interface
> > (i.e., Ethernet device) to NTB, such that Linux can transfer data from one
> > system to the other in a standard way.
> >
> > Signed-off-by: Jon Mason <jon.mason@intel.com>
> > ---
> > MAINTAINERS | 6 +
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/ntb/Kconfig | 13 +
> > drivers/ntb/Makefile | 3 +
> > drivers/ntb/ntb_hw.c | 1178 ++++++++++++++++++++++++++++++++++++
> > drivers/ntb/ntb_hw.h | 206 +++++++
> > drivers/ntb/ntb_regs.h | 150 +++++
> > drivers/ntb/ntb_transport.c | 1387 +++++++++++++++++++++++++++++++++++++++++++
> > include/linux/ntb.h | 92 +++
>
> Where will drivers for non-Intel NTBs fit in this hierarchy? It seems
> a bit presumptuous to claim the generic "ntb" names just for Intel
> devices.
I've tried to make it all generic enough that non-Intel NTBs should plug in with
minimal changes to ntb_hw.c. If their design is too divergent, then a slight
redesign of ntb_hw.c might be necessary. But from what I've seen of other
designs on the internet, they appear to be extremely similar. The transport and
client drivers were written with the hardware abstracted away as much as
possible to prevent the need to modify it for different hardware. If there is
anything which is Intel hardware specific, I'd be happy to change it to make it
more generic.
Thanks,
Jon
^ permalink raw reply
* Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: Olaf Hering @ 2012-07-30 18:03 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, devel, apw, netdev, ben
In-Reply-To: <1343145701-3691-13-git-send-email-kys@microsoft.com>
On Tue, Jul 24, K. Y. Srinivasan wrote:
> + /*
> + * Set the configuration for the specified interface with
> + * the information provided. Since there is no standard
> + * way to configure an interface, we will have an external
> + * script that does the job of configuring the interface and
> + * flushing the configuration.
> + *
> + * The parameters passed to this external script are:
> + * 1. A configuration file that has the specified configuration.
Maybe this should be written as 'A info file that has the requested
network configuration' or something like that.
> + *
> + * We will embed the name of the interface in the configuration
> + * file: ifcfg-ethx (where ethx is the interface name).
I think the intention here is to use the generated file as is. Depending
on the distro in the guest the file may need some processing. So I think
the actual interface name should also be part of the file.
> + *
> + * Here is the format of the ip configuration file:
> + *
> + * HWADDR=macaddr
> + * BOOTPROTO=dhcp (dhcp enabled for the interface)
While BOOTPROTO= is used in current network config files, its meaning
there is unrelated to what its meant here. Here it means DHCP=yes/no, so
I think the file should contain just that. And as the code is written
now BOOTPROTO= is optional, which is not mentioned in the comment.
> + * NM_CONTROLLED=no (this interface will not be controlled by NM)
I think this is not up to kvp_deamon to decide what controls the
interface. Maybe one day NM is sufficiently advanced technology that it
can cope with such requests?
The helper script should decide if the NM flag should be written to the
final config file.
> + * PEERDNS=yes
> + * IPADDR_x=ipaddr
> + * NETMASK_x=netmask
> + * GATEWAY_x=gateway
This should mention that its either IPADDR=ipaddr1 or 'IPADDR=ipaddr1;
IPADDR_1=ipaddr2'
> + * DNSx=dns
This should mention that its either DNS=ipaddr1 or 'DNS=ipaddr1;
DNS1=ipaddr2'
Olaf
^ permalink raw reply
* Re: [PATCH 13/17] Tools: hv: Implement the KVP verb - KVP_OP_SET_IP_INFO
From: Olaf Hering @ 2012-07-30 17:33 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: gregkh, linux-kernel, netdev, apw, devel, ben
In-Reply-To: <1343145701-3691-13-git-send-email-kys@microsoft.com>
On Tue, Jul 24, K. Y. Srinivasan wrote:
> +static char *kvp_get_if_name(char *guid)
> +{
> + DIR *dir;
> + struct dirent *entry;
> + FILE *file;
> + char *p, *q, *x;
> + char *if_name = NULL;
> + char buf[256];
> + char *kvp_net_dir = "/sys/class/net/";
> + char dev_id[100];
Perhaps this can be written as char dev_id[100] = "short string";?
> +
> + dir = opendir(kvp_net_dir);
> + if (dir == NULL)
> + return NULL;
> +
> + memset(dev_id, 0, sizeof(dev_id));
> + strcat(dev_id, kvp_net_dir);
> + q = dev_id + strlen(kvp_net_dir);
> +
> + while ((entry = readdir(dir)) != NULL) {
> + /*
> + * Set the state for the next pass.
> + */
> + *q = '\0';
> + strcat(dev_id, entry->d_name);
> + strcat(dev_id, "/device/device_id");
Maybe this (and other) strcat should be changed to snprintf?
> +
> + file = fopen(dev_id, "r");
> + if (file == NULL)
> + continue;
> +
> + p = fgets(buf, sizeof(buf), file);
> + if (p) {
> + x = strchr(p, '\n');
> + if (x)
> + *x = '\0';
> +
> + if (!strcmp(p, guid)) {
> + /*
> + * Found the guid match; return the interface
> + * name. The caller will free the memory.
> + */
> + if_name = strdup(entry->d_name);
> + break;
This will leave 'file' open.
I have seen it in some other place as well.
> + }
> + }
> + fclose(file);
> + }
> +
> + closedir(dir);
> + return if_name;
> +}
> +
> +/*
> + * Retrieve the MAC address given the interface name.
> + */
> +
> +static char *kvp_if_name_to_mac(char *if_name)
> +{
> + FILE *file;
> + char *p, *x;
> + char buf[256];
> + char addr_file[100];
> + int i;
> + char *mac_addr = NULL;
> +
> + memset(addr_file, 0, sizeof(addr_file));
> + strcat(addr_file, "/sys/class/net/");
> + strcat(addr_file, if_name);
> + strcat(addr_file, "/address");
snprintf?
> +
> + file = fopen(addr_file, "r");
> + if (file == NULL)
> + return NULL;
> +
> + p = fgets(buf, sizeof(buf), file);
> + if (p) {
> + x = strchr(p, '\n');
> + if (x)
> + *x = '\0';
> + for (i = 0; i < strlen(p); i++)
> + p[i] = toupper(p[i]);
> + mac_addr = strdup(p);
> + }
> +
> + fclose(file);
> + return mac_addr;
> +}
> +
> +
> static void kvp_process_ipconfig_file(char *cmd,
> char *config_buf, int len,
> int element_size, int offset)
> @@ -800,6 +910,314 @@ getaddr_done:
> }
>
>
> +static int expand_ipv6(char *addr, int type)
> +{
> + int ret;
> + struct in6_addr v6_addr;
> +
> + ret = inet_pton(AF_INET6, addr, &v6_addr);
> +
> + if (ret != 1) {
> + if (type == NETMASK)
> + return 1;
> + return 0;
> + }
> +
> + sprintf(addr, "%02x%02x:%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
> + "%02x%02x:%02x%02x:%02x%02x",
> + (int)v6_addr.s6_addr[0], (int)v6_addr.s6_addr[1],
> + (int)v6_addr.s6_addr[2], (int)v6_addr.s6_addr[3],
> + (int)v6_addr.s6_addr[4], (int)v6_addr.s6_addr[5],
> + (int)v6_addr.s6_addr[6], (int)v6_addr.s6_addr[7],
> + (int)v6_addr.s6_addr[8], (int)v6_addr.s6_addr[9],
> + (int)v6_addr.s6_addr[10], (int)v6_addr.s6_addr[11],
> + (int)v6_addr.s6_addr[12], (int)v6_addr.s6_addr[13],
> + (int)v6_addr.s6_addr[14], (int)v6_addr.s6_addr[15]);
> +
> + return 1;
> +
> +}
> +
> +static int is_ipv4(char *addr)
> +{
> + int ret;
> + struct in_addr ipv4_addr;
> +
> + ret = inet_pton(AF_INET, addr, &ipv4_addr);
> +
> + if (ret == 1)
> + return 1;
> + return 0;
> +}
> +
> +static int parse_ip_val_buffer(char *in_buf, int *offset,
> + char *out_buf, int out_len)
> +{
> + char *x;
> + char *start;
> +
> + /*
> + * in_buf has sequence of characters that are seperated by
> + * the character ';'. The last sequence does not have the
> + * terminating ";" character.
> + */
> + start = in_buf + *offset;
> +
> + x = strchr(start, ';');
> + if (x)
> + *x = 0;
> + else
> + x = start + strlen(start);
> +
> + if (strlen(start) != 0) {
> + int i = 0;
> + /*
> + * Get rid of leading spaces.
> + */
> + while (start[i] == ' ')
> + i++;
> +
> + if ((x - start) <= out_len) {
> + strcpy(out_buf, (start + i));
> + *offset += (x - start) + 1;
> + return 1;
> + }
> + }
> + return 0;
> +}
> +
> +static int kvp_write_file(FILE *f, char *s1, char *s2, char *s3)
> +{
> + char str[256];
> + int error;
> +
> + memset(str, 0, sizeof(str));
> + strcat(str, s1);
> + if (s2 != NULL)
> + strcat(str, s2);
> + strcat(str, "=");
> + strcat(str, s3);
> + strcat(str, "\n");
> +
> + error = fputs(str, f);
> + if (error == EOF)
> + return HV_E_FAIL;
> +
> + return 0;
> +}
> +
> +
> +static int process_ip_string(FILE *f, char *ip_string, int type)
> +{
> + int error = 0;
> + char addr[INET6_ADDRSTRLEN];
> + int i = 0;
> + int j = 0;
> + char str[256];
> + char sub_str[10];
> + int offset = 0;
> +
> + memset(addr, 0, sizeof(addr));
> +
> + while (parse_ip_val_buffer(ip_string, &offset, addr,
> + (MAX_IP_ADDR_SIZE * 2))) {
> + memset(sub_str, 0, sizeof(sub_str));
> + memset(str, 0, sizeof(str));
> +
> + if (is_ipv4(addr)) {
> + switch (type) {
> + case IPADDR:
> + strcat(str, "IPADDR");
> + break;
> + case NETMASK:
> + strcat(str, "NETMASK");
> + break;
> + case GATEWAY:
> + strcat(str, "GATEWAY");
> + break;
> + case DNS:
> + strcat(str, "DNS");
> + break;
> + }
> + if (i != 0) {
> + if (type != DNS)
> + sprintf(sub_str, "_%d", i++);
> + else
> + sprintf(sub_str, "%d", ++i);
> + } else if (type == DNS) {
> + sprintf(sub_str, "%d", ++i);
> + }
> +
> +
> + } else if (expand_ipv6(addr, type)) {
> + switch (type) {
> + case IPADDR:
> + strcat(str, "IPV6ADDR");
> + break;
> + case NETMASK:
> + strcat(str, "IPV6NETMASK");
> + break;
> + case GATEWAY:
> + strcat(str, "IPV6_DEFAULTGW");
> + break;
> + case DNS:
> + strcat(str, "DNS");
> + break;
> + }
> + if ((j != 0) || (type == DNS)) {
> + if (type != DNS)
> + sprintf(sub_str, "_%d", j++);
> + else
> + sprintf(sub_str, "%d", ++i);
> + } else if (type == DNS) {
> + sprintf(sub_str, "%d", ++i);
> + }
> + } else {
> + return HV_INVALIDARG;
> + }
> +
> + error = kvp_write_file(f, str, sub_str, addr);
> + if (error)
> + return error;
> + memset(addr, 0, sizeof(addr));
> + }
> +
> + return 0;
> +}
> +
> +static int kvp_set_ip_info(char *if_name, struct hv_kvp_ipaddr_value *new_val)
> +{
> + int error = 0;
> + char if_file[50];
> + FILE *file;
> + char cmd[512];
> + char *mac_addr;
> +
> + /*
> + * Set the configuration for the specified interface with
> + * the information provided. Since there is no standard
> + * way to configure an interface, we will have an external
> + * script that does the job of configuring the interface and
> + * flushing the configuration.
> + *
> + * The parameters passed to this external script are:
> + * 1. A configuration file that has the specified configuration.
> + *
> + * We will embed the name of the interface in the configuration
> + * file: ifcfg-ethx (where ethx is the interface name).
> + *
> + * Here is the format of the ip configuration file:
> + *
> + * HWADDR=macaddr
> + * BOOTPROTO=dhcp (dhcp enabled for the interface)
> + * NM_CONTROLLED=no (this interface will not be controlled by NM)
> + * PEERDNS=yes
> + * IPADDR_x=ipaddr
> + * NETMASK_x=netmask
> + * GATEWAY_x=gateway
> + * DNSx=dns
> + *
> + * IPV6 addresses will be tagged as IPV6ADDR, IPV6 gateway will be
> + * tagged as IPV6_DEFAULTGW and IPV6 NETMASK will be tagged as
> + * IPV6NETMASK.
> + */
> +
> + memset(if_file, 0, sizeof(if_file));
> + strcat(if_file, "/var/opt/hyperv/ifcfg-");
> + strcat(if_file, if_name);
> +
> + file = fopen(if_file, "w");
> +
> + if (file == NULL) {
> + syslog(LOG_ERR, "Failed to open config file");
> + return HV_E_FAIL;
> + }
> +
> + /*
> + * First write out the MAC address.
> + */
> +
> + mac_addr = kvp_if_name_to_mac(if_name);
> + if (mac_addr == NULL) {
> + error = HV_E_FAIL;
> + goto setval_error;
> + }
> +
> + error = kvp_write_file(file, "HWADDR", NULL, mac_addr);
> + if (error)
> + goto setval_error;
> +
> + error = kvp_write_file(file, "ONBOOT", NULL, "yes");
> + if (error)
> + goto setval_error;
> +
> + error = kvp_write_file(file, "IPV6INIT", NULL, "yes");
> + if (error)
> + goto setval_error;
> +
> + error = kvp_write_file(file, "NM_CONTROLLED", NULL, "no");
> + if (error)
> + goto setval_error;
> +
> + error = kvp_write_file(file, "PEERDNS", NULL, "yes");
> + if (error)
> + goto setval_error;
> +
> + if (new_val->dhcp_enabled) {
> + error = kvp_write_file(file, "BOOTPROTO", NULL, "dhcp");
> + if (error)
> + goto setval_error;
> +
> + /*
> + * We are done!.
> + */
> + goto setval_done;
> + }
> +
> + /*
> + * Write the configuration for ipaddress, netmask, gateway and
> + * name servers.
> + */
> +
> + error = process_ip_string(file, (char *)new_val->ip_addr, IPADDR);
> + if (error)
> + goto setval_error;
> +
> + error = process_ip_string(file, (char *)new_val->sub_net, NETMASK);
> + if (error)
> + goto setval_error;
> +
> + error = process_ip_string(file, (char *)new_val->gate_way, GATEWAY);
> + if (error)
> + goto setval_error;
> +
> + error = process_ip_string(file, (char *)new_val->dns_addr, DNS);
> + if (error)
> + goto setval_error;
> +
> +setval_done:
> + free(mac_addr);
> + fclose(file);
> +
> + /*
> + * Now that we have populated the configuration file,
> + * invoke the external script to do its magic.
> + */
> +
> + memset(cmd, 0, sizeof(cmd));
> + strcat(cmd, "/sbin/hv_set_ifconfig ");
> + strcat(cmd, if_file);
The new patch should use "%s %s", not "%s%s" as format string.
^ permalink raw reply
* Re: [PATCH net 1/2] tcp: Limit number of segments generated by GSO per skb
From: Eric Dumazet @ 2012-07-30 17:31 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, linux-net-drivers
In-Reply-To: <1343668602.2667.6.camel@bwh-desktop.uk.solarflarecom.com>
On Mon, 2012-07-30 at 18:16 +0100, Ben Hutchings wrote:
> A peer (or local user) may cause TCP to use a nominal MSS of as little
> as 88 (actual MSS of 76 with timestamps). Given that we have a
> sufficiently prodigious local sender and the peer ACKs quickly enough,
> it is nevertheless possible to grow the window for such a connection
> to the point that we will try to send just under 64K at once. This
> results in a single skb that expands to 861 segments.
>
> In some drivers with TSO support, such an skb will require hundreds of
> DMA descriptors; a substantial fraction of a TX ring or even more than
> a full ring. The TX queue selected for the skb may stall and trigger
> the TX watchdog repeatedly (since the problem skb will be retried
> after the TX reset). This particularly affects sfc, for which the
> issue is designated as CVE-2012-3412. However it may be that some
> hardware or firmware also fails to handle such an extreme TSO request
> correctly.
>
> Therefore, limit the number of segments per skb to 100. This should
> make no difference to behaviour unless the actual MSS is less than
> about 700.
>
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
Hmm, isnt GRO path also vulnerable ?
An alternative would be to drop such frames in the ndo_start_xmit(), and
cap sk->sk_gso_max_size (since skb are no longer orphaned...)
Or you could introduce a new wk->sk_gso_max_segments, that your sfc
driver sets to whatever limit ?
^ permalink raw reply
* Re: [PATCH net 1/2] tcp: Limit number of segments generated by GSO per skb
From: Ben Greear @ 2012-07-30 17:23 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev, linux-net-drivers
In-Reply-To: <1343668602.2667.6.camel@bwh-desktop.uk.solarflarecom.com>
On 07/30/2012 10:16 AM, Ben Hutchings wrote:
> A peer (or local user) may cause TCP to use a nominal MSS of as little
> as 88 (actual MSS of 76 with timestamps). Given that we have a
> sufficiently prodigious local sender and the peer ACKs quickly enough,
> it is nevertheless possible to grow the window for such a connection
> to the point that we will try to send just under 64K at once. This
> results in a single skb that expands to 861 segments.
>
> In some drivers with TSO support, such an skb will require hundreds of
> DMA descriptors; a substantial fraction of a TX ring or even more than
> a full ring. The TX queue selected for the skb may stall and trigger
> the TX watchdog repeatedly (since the problem skb will be retried
> after the TX reset). This particularly affects sfc, for which the
> issue is designated as CVE-2012-3412. However it may be that some
> hardware or firmware also fails to handle such an extreme TSO request
> correctly.
>
> Therefore, limit the number of segments per skb to 100. This should
> make no difference to behaviour unless the actual MSS is less than
> about 700.
Please do not do this...or at least allow over-rides. We love
the trick of seting very small MSS and making the NICs generate
huge numbers of small TCP frames with efficient user-space
logic. We use this for stateful TCP load testing when high
numbers of tcp packets-per-second is desired.
Intel NICs, including 10G, work just fine with minimal MSS
in this scenario.
Thanks,
Ben
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* [PATCH net 2/2] sfc: Correct the minimum TX queue size
From: Ben Hutchings @ 2012-07-30 17:17 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1343668498.2667.5.camel@bwh-desktop.uk.solarflarecom.com>
Currently an skb requiring TSO may not fit within a minimum-size TX
queue. The TX queue selected for the skb may stall and trigger the TX
watchdog repeatedly (since the problem skb will be retried after the
TX reset). This issue is designated as CVE-2012-3412.
The preceding change 'tcp: Limit number of segments generated by GSO
per skb' fixed this for the default queue size. Increase the minimum
to allow for 2 worst-case skbs, such that there will definitely be
space to add an skb after we wake a queue.
To avoid invalidating existing configurations, change
efx_ethtool_set_ringparam() to fix up values that are too small rather
than returning -EINVAL.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/ethernet/sfc/efx.c | 5 +++++
drivers/net/ethernet/sfc/efx.h | 11 +++++++----
drivers/net/ethernet/sfc/ethtool.c | 16 +++++++++++-----
drivers/net/ethernet/sfc/tx.c | 20 ++++++++++++++++++++
4 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 70554a1..a5b5a75 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -1503,6 +1503,11 @@ static int efx_probe_all(struct efx_nic *efx)
goto fail2;
}
+ BUILD_BUG_ON(EFX_DEFAULT_DMAQ_SIZE < EFX_RXQ_MIN_ENT);
+ if (WARN_ON(EFX_DEFAULT_DMAQ_SIZE < EFX_TXQ_MIN_ENT(efx))) {
+ rc = -EINVAL;
+ goto fail3;
+ }
efx->rxq_entries = efx->txq_entries = EFX_DEFAULT_DMAQ_SIZE;
rc = efx_probe_filters(efx);
diff --git a/drivers/net/ethernet/sfc/efx.h b/drivers/net/ethernet/sfc/efx.h
index be8f915..d95309c 100644
--- a/drivers/net/ethernet/sfc/efx.h
+++ b/drivers/net/ethernet/sfc/efx.h
@@ -30,6 +30,7 @@ extern netdev_tx_t
efx_enqueue_skb(struct efx_tx_queue *tx_queue, struct sk_buff *skb);
extern void efx_xmit_done(struct efx_tx_queue *tx_queue, unsigned int index);
extern int efx_setup_tc(struct net_device *net_dev, u8 num_tc);
+extern unsigned int efx_tx_max_skb_descs(struct efx_nic *efx);
/* RX */
extern int efx_probe_rx_queue(struct efx_rx_queue *rx_queue);
@@ -52,10 +53,12 @@ extern void efx_schedule_slow_fill(struct efx_rx_queue *rx_queue);
#define EFX_MAX_EVQ_SIZE 16384UL
#define EFX_MIN_EVQ_SIZE 512UL
-/* The smallest [rt]xq_entries that the driver supports. Callers of
- * efx_wake_queue() assume that they can subsequently send at least one
- * skb. Falcon/A1 may require up to three descriptors per skb_frag. */
-#define EFX_MIN_RING_SIZE (roundup_pow_of_two(2 * 3 * MAX_SKB_FRAGS))
+/* The smallest [rt]xq_entries that the driver supports. RX minimum
+ * is a bit arbitrary. For TX, we must have space for at least 2
+ * TSO skbs.
+ */
+#define EFX_RXQ_MIN_ENT 128U
+#define EFX_TXQ_MIN_ENT(efx) (2 * efx_tx_max_skb_descs(efx))
/* Filters */
extern int efx_probe_filters(struct efx_nic *efx);
diff --git a/drivers/net/ethernet/sfc/ethtool.c b/drivers/net/ethernet/sfc/ethtool.c
index 10536f9..8cba2df 100644
--- a/drivers/net/ethernet/sfc/ethtool.c
+++ b/drivers/net/ethernet/sfc/ethtool.c
@@ -680,21 +680,27 @@ static int efx_ethtool_set_ringparam(struct net_device *net_dev,
struct ethtool_ringparam *ring)
{
struct efx_nic *efx = netdev_priv(net_dev);
+ u32 txq_entries;
if (ring->rx_mini_pending || ring->rx_jumbo_pending ||
ring->rx_pending > EFX_MAX_DMAQ_SIZE ||
ring->tx_pending > EFX_MAX_DMAQ_SIZE)
return -EINVAL;
- if (ring->rx_pending < EFX_MIN_RING_SIZE ||
- ring->tx_pending < EFX_MIN_RING_SIZE) {
+ if (ring->rx_pending < EFX_RXQ_MIN_ENT) {
netif_err(efx, drv, efx->net_dev,
- "TX and RX queues cannot be smaller than %ld\n",
- EFX_MIN_RING_SIZE);
+ "RX queues cannot be smaller than %u\n",
+ EFX_RXQ_MIN_ENT);
return -EINVAL;
}
- return efx_realloc_channels(efx, ring->rx_pending, ring->tx_pending);
+ txq_entries = max(ring->tx_pending, EFX_TXQ_MIN_ENT(efx));
+ if (txq_entries != ring->tx_pending)
+ netif_warn(efx, drv, efx->net_dev,
+ "increasing TX queue size to minimum of %u\n",
+ txq_entries);
+
+ return efx_realloc_channels(efx, ring->rx_pending, txq_entries);
}
static int efx_ethtool_set_pauseparam(struct net_device *net_dev,
diff --git a/drivers/net/ethernet/sfc/tx.c b/drivers/net/ethernet/sfc/tx.c
index 9b225a7..814bd17 100644
--- a/drivers/net/ethernet/sfc/tx.c
+++ b/drivers/net/ethernet/sfc/tx.c
@@ -15,6 +15,7 @@
#include <linux/ipv6.h>
#include <linux/slab.h>
#include <net/ipv6.h>
+#include <net/tcp.h>
#include <linux/if_ether.h>
#include <linux/highmem.h>
#include "net_driver.h"
@@ -119,6 +120,25 @@ efx_max_tx_len(struct efx_nic *efx, dma_addr_t dma_addr)
return len;
}
+unsigned int efx_tx_max_skb_descs(struct efx_nic *efx)
+{
+ /* Header and payload descriptor for each output segment, plus
+ * one for every input fragment boundary within a segment
+ */
+ unsigned int max_descs = TCP_MAX_GSO_SEGS * 2 + MAX_SKB_FRAGS;
+
+ /* Possibly one more per segment for the alignment workaround */
+ if (EFX_WORKAROUND_5391(efx))
+ max_descs += TCP_MAX_GSO_SEGS;
+
+ /* Possibly more for PCIe page boundaries within input fragments */
+ if (PAGE_SIZE > EFX_PAGE_SIZE)
+ max_descs += max_t(unsigned int, MAX_SKB_FRAGS,
+ DIV_ROUND_UP(GSO_MAX_SIZE, EFX_PAGE_SIZE));
+
+ return max_descs;
+}
+
/*
* Add a socket buffer to a TX queue
*
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net 1/2] tcp: Limit number of segments generated by GSO per skb
From: Ben Hutchings @ 2012-07-30 17:16 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
In-Reply-To: <1343668498.2667.5.camel@bwh-desktop.uk.solarflarecom.com>
A peer (or local user) may cause TCP to use a nominal MSS of as little
as 88 (actual MSS of 76 with timestamps). Given that we have a
sufficiently prodigious local sender and the peer ACKs quickly enough,
it is nevertheless possible to grow the window for such a connection
to the point that we will try to send just under 64K at once. This
results in a single skb that expands to 861 segments.
In some drivers with TSO support, such an skb will require hundreds of
DMA descriptors; a substantial fraction of a TX ring or even more than
a full ring. The TX queue selected for the skb may stall and trigger
the TX watchdog repeatedly (since the problem skb will be retried
after the TX reset). This particularly affects sfc, for which the
issue is designated as CVE-2012-3412. However it may be that some
hardware or firmware also fails to handle such an extreme TSO request
correctly.
Therefore, limit the number of segments per skb to 100. This should
make no difference to behaviour unless the actual MSS is less than
about 700.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
include/net/tcp.h | 3 +++
net/ipv4/tcp.c | 4 +++-
net/ipv4/tcp_output.c | 17 +++++++++--------
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index e19124b..098a2d0 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -70,6 +70,9 @@ extern void tcp_time_wait(struct sock *sk, int state, int timeo);
/* The least MTU to use for probing */
#define TCP_BASE_MSS 512
+/* Maximum number of segments we may require GSO to generate from an skb. */
+#define TCP_MAX_GSO_SEGS 100
+
/* After receiving this amount of duplicate ACKs fast retransmit starts. */
#define TCP_FASTRETRANS_THRESH 3
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index e7e6eea..51d8daf 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -811,7 +811,9 @@ static unsigned int tcp_xmit_size_goal(struct sock *sk, u32 mss_now,
old_size_goal + mss_now > xmit_size_goal)) {
xmit_size_goal = old_size_goal;
} else {
- tp->xmit_size_goal_segs = xmit_size_goal / mss_now;
+ tp->xmit_size_goal_segs =
+ min_t(u32, xmit_size_goal / mss_now,
+ TCP_MAX_GSO_SEGS);
xmit_size_goal = tp->xmit_size_goal_segs * mss_now;
}
}
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 33cd065..c86c288 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1522,21 +1522,21 @@ static void tcp_cwnd_validate(struct sock *sk)
* when we would be allowed to send the split-due-to-Nagle skb fully.
*/
static unsigned int tcp_mss_split_point(const struct sock *sk, const struct sk_buff *skb,
- unsigned int mss_now, unsigned int cwnd)
+ unsigned int mss_now, unsigned int max_segs)
{
const struct tcp_sock *tp = tcp_sk(sk);
- u32 needed, window, cwnd_len;
+ u32 needed, window, max_len;
window = tcp_wnd_end(tp) - TCP_SKB_CB(skb)->seq;
- cwnd_len = mss_now * cwnd;
+ max_len = mss_now * max_segs;
- if (likely(cwnd_len <= window && skb != tcp_write_queue_tail(sk)))
- return cwnd_len;
+ if (likely(max_len <= window && skb != tcp_write_queue_tail(sk)))
+ return max_len;
needed = min(skb->len, window);
- if (cwnd_len <= needed)
- return cwnd_len;
+ if (max_len <= needed)
+ return max_len;
return needed - needed % mss_now;
}
@@ -1999,7 +1999,8 @@ static bool tcp_write_xmit(struct sock *sk, unsigned int mss_now, int nonagle,
limit = mss_now;
if (tso_segs > 1 && !tcp_urg_mode(tp))
limit = tcp_mss_split_point(sk, skb, mss_now,
- cwnd_quota);
+ min(cwnd_quota,
+ TCP_MAX_GSO_SEGS));
if (skb->len > limit &&
unlikely(tso_fragment(sk, skb, limit, mss_now, gfp)))
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net 0/2] Prevent extreme TSO parameters from stalling TX queues
From: Ben Hutchings @ 2012-07-30 17:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers
The following changes fix a potential DoS by peers or local users on
network interfaces using the sfc driver (and possibly others) with TSO
enabled (as it is by default). Please apply them to the net tree and
your stable update queue.
Ben.
Ben Hutchings (2):
tcp: Limit number of segments generated by GSO per skb
sfc: Correct the minimum TX queue size
drivers/net/ethernet/sfc/efx.c | 5 +++++
drivers/net/ethernet/sfc/efx.h | 11 +++++++----
drivers/net/ethernet/sfc/ethtool.c | 16 +++++++++++-----
drivers/net/ethernet/sfc/tx.c | 20 ++++++++++++++++++++
include/net/tcp.h | 3 +++
net/ipv4/tcp.c | 4 +++-
net/ipv4/tcp_output.c | 17 +++++++++--------
7 files changed, 58 insertions(+), 18 deletions(-)
--
1.7.7.6
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* [PATCH] ipv4: remove rt_cache_rebuild_count
From: Eric Dumazet @ 2012-07-30 17:14 UTC (permalink / raw)
To: David Miller; +Cc: netdev
From: Eric Dumazet <edumazet@google.com>
After IP route cache removal, rt_cache_rebuild_count is no longer
used.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
Documentation/networking/ip-sysctl.txt | 6 ------
include/net/netns/ipv4.h | 2 --
net/ipv4/sysctl_net_ipv4.c | 11 -----------
3 files changed, 19 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 406a522..ca447b3 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -48,12 +48,6 @@ min_adv_mss - INTEGER
The advertised MSS depends on the first hop route MTU, but will
never be lower than this setting.
-rt_cache_rebuild_count - INTEGER
- The per net-namespace route cache emergency rebuild threshold.
- Any net-namespace having its route cache rebuilt due to
- a hash bucket chain being too long more than this many times
- will have its route caching disabled
-
IP Fragmentation:
ipfrag_high_thresh - INTEGER
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 0ffb8e3..1474dd6 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -61,8 +61,6 @@ struct netns_ipv4 {
int sysctl_icmp_ratelimit;
int sysctl_icmp_ratemask;
int sysctl_icmp_errors_use_inbound_ifaddr;
- int sysctl_rt_cache_rebuild_count;
- int current_rt_cache_rebuild_count;
unsigned int sysctl_ping_group_range[2];
long sysctl_tcp_mem[3];
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 5840c32..4b6487a 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -784,13 +784,6 @@ static struct ctl_table ipv4_net_table[] = {
.proc_handler = proc_dointvec
},
{
- .procname = "rt_cache_rebuild_count",
- .data = &init_net.ipv4.sysctl_rt_cache_rebuild_count,
- .maxlen = sizeof(int),
- .mode = 0644,
- .proc_handler = proc_dointvec
- },
- {
.procname = "ping_group_range",
.data = &init_net.ipv4.sysctl_ping_group_range,
.maxlen = sizeof(init_net.ipv4.sysctl_ping_group_range),
@@ -829,8 +822,6 @@ static __net_init int ipv4_sysctl_init_net(struct net *net)
table[5].data =
&net->ipv4.sysctl_icmp_ratemask;
table[6].data =
- &net->ipv4.sysctl_rt_cache_rebuild_count;
- table[7].data =
&net->ipv4.sysctl_ping_group_range;
}
@@ -842,8 +833,6 @@ static __net_init int ipv4_sysctl_init_net(struct net *net)
net->ipv4.sysctl_ping_group_range[0] = 1;
net->ipv4.sysctl_ping_group_range[1] = 0;
- net->ipv4.sysctl_rt_cache_rebuild_count = 4;
-
tcp_init_mem(net);
net->ipv4.ipv4_hdr = register_net_sysctl(net, "net/ipv4", table);
^ permalink raw reply related
* Re: [RFC v2 1/2] PCI-Express Non-Transparent Bridge Support
From: Bjorn Helgaas @ 2012-07-30 16:50 UTC (permalink / raw)
To: Jon Mason; +Cc: linux-kernel, netdev, linux-pci, Dave Jiang
In-Reply-To: <1343607994-32415-2-git-send-email-jon.mason@intel.com>
On Sun, Jul 29, 2012 at 6:26 PM, Jon Mason <jon.mason@intel.com> wrote:
> A PCI-Express non-transparent bridge (NTB) is a point-to-point PCIe bus
> connecting 2 systems, providing electrical isolation between the two subsystems.
> A non-transparent bridge is functionally similar to a transparent bridge except
> that both sides of the bridge have their own independent address domains. The
> host on one side of the bridge will not have the visibility of the complete
> memory or I/O space on the other side of the bridge. To communicate across the
> non-transparent bridge, each NTB endpoint has one (or more) apertures exposed to
> the local system. Writes to these apertures are mirrored to memory on the
> remote system. Communications can also occur through the use of doorbell
> registers that initiate interrupts to the alternate domain, and scratch-pad
> registers accessible from both sides.
>
> The NTB device driver is needed to configure these memory windows, doorbell, and
> scratch-pad registers as well as use them in such a way as they can be turned
> into a viable communication channel to the remote system. ntb_hw.[ch]
> determines the usage model (NTB to NTB or NTB to Root Port) and abstracts away
> the underlying hardware to provide access and a common interface to the doorbell
> registers, scratch pads, and memory windows. These hardware interfaces are
> exported so that other, non-mainlined kernel drivers can access these.
> ntb_transport.[ch] also uses the exported interfaces in ntb_hw.[ch] to setup a
> communication channel(s) and provide a reliable way of transferring data from
> one side to the other, which it then exports so that "client" drivers can access
> them. These client drivers are used to provide a standard kernel interface
> (i.e., Ethernet device) to NTB, such that Linux can transfer data from one
> system to the other in a standard way.
>
> Signed-off-by: Jon Mason <jon.mason@intel.com>
> ---
> MAINTAINERS | 6 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/ntb/Kconfig | 13 +
> drivers/ntb/Makefile | 3 +
> drivers/ntb/ntb_hw.c | 1178 ++++++++++++++++++++++++++++++++++++
> drivers/ntb/ntb_hw.h | 206 +++++++
> drivers/ntb/ntb_regs.h | 150 +++++
> drivers/ntb/ntb_transport.c | 1387 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/ntb.h | 92 +++
Where will drivers for non-Intel NTBs fit in this hierarchy? It seems
a bit presumptuous to claim the generic "ntb" names just for Intel
devices.
^ permalink raw reply
* [PATCH] net: batch nf_conntrack_net_exit
From: Vladimir Davydov @ 2012-07-30 15:16 UTC (permalink / raw)
To: David S. Miller; +Cc: Vladimir Davydov, netdev, linux-kernel
The patch introduces nf_conntrack_cleanup_list(), which cleanups
nf_conntracks for a list of netns and calls synchronize_net() only
once for them all.
---
include/net/netfilter/nf_conntrack_core.h | 10 +++++++++-
net/netfilter/nf_conntrack_core.c | 21 +++++++++++++--------
net/netfilter/nf_conntrack_standalone.c | 14 +++++++++-----
3 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/include/net/netfilter/nf_conntrack_core.h b/include/net/netfilter/nf_conntrack_core.h
index d8f5b9f..f53b855 100644
--- a/include/net/netfilter/nf_conntrack_core.h
+++ b/include/net/netfilter/nf_conntrack_core.h
@@ -26,7 +26,15 @@ extern unsigned int nf_conntrack_in(struct net *net,
struct sk_buff *skb);
extern int nf_conntrack_init(struct net *net);
-extern void nf_conntrack_cleanup(struct net *net);
+extern void nf_conntrack_cleanup_list(struct list_head *net_exit_list);
+
+static inline void nf_conntrack_cleanup(struct net *net)
+{
+ LIST_HEAD(single);
+
+ list_add(&net->exit_list, &single);
+ nf_conntrack_cleanup_list(&single);
+}
extern int nf_conntrack_proto_init(struct net *net);
extern void nf_conntrack_proto_fini(struct net *net);
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index cf48755..afa62f7 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -1363,21 +1363,26 @@ static void nf_conntrack_cleanup_net(struct net *net)
/* Mishearing the voices in his head, our hero wonders how he's
supposed to kill the mall. */
-void nf_conntrack_cleanup(struct net *net)
+void nf_conntrack_cleanup_list(struct list_head *net_exit_list)
{
- if (net_eq(net, &init_net))
- RCU_INIT_POINTER(ip_ct_attach, NULL);
+ struct net *net;
+
+ list_for_each_entry(net, net_exit_list, exit_list)
+ if (net_eq(net, &init_net))
+ RCU_INIT_POINTER(ip_ct_attach, NULL);
/* This makes sure all current packets have passed through
netfilter framework. Roll on, two-stage module
delete... */
synchronize_net();
- nf_conntrack_proto_fini(net);
- nf_conntrack_cleanup_net(net);
- if (net_eq(net, &init_net)) {
- RCU_INIT_POINTER(nf_ct_destroy, NULL);
- nf_conntrack_cleanup_init_net();
+ list_for_each_entry(net, net_exit_list, exit_list) {
+ nf_conntrack_proto_fini(net);
+ nf_conntrack_cleanup_net(net);
+ if (net_eq(net, &init_net)) {
+ RCU_INIT_POINTER(nf_ct_destroy, NULL);
+ nf_conntrack_cleanup_init_net();
+ }
}
}
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index 9b39432..8f60708 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -551,16 +551,20 @@ out_init:
return ret;
}
-static void nf_conntrack_net_exit(struct net *net)
+static void nf_conntrack_net_exit(struct list_head *net_exit_list)
{
- nf_conntrack_standalone_fini_sysctl(net);
- nf_conntrack_standalone_fini_proc(net);
- nf_conntrack_cleanup(net);
+ struct net *net;
+
+ list_for_each_entry(net, net_exit_list, exit_list) {
+ nf_conntrack_standalone_fini_sysctl(net);
+ nf_conntrack_standalone_fini_proc(net);
+ }
+ nf_conntrack_cleanup_list(net_exit_list);
}
static struct pernet_operations nf_conntrack_net_ops = {
.init = nf_conntrack_net_init,
- .exit = nf_conntrack_net_exit,
+ .exit_batch = nf_conntrack_net_exit,
};
static int __init nf_conntrack_standalone_init(void)
--
1.7.1
^ permalink raw reply related
* Re: [RFC v2 2/2] net: Add support for NTB virtual ethernet device
From: Jiri Pirko @ 2012-07-30 14:02 UTC (permalink / raw)
To: Jon Mason; +Cc: linux-kernel, netdev, linux-pci, Dave Jiang
In-Reply-To: <1343607994-32415-3-git-send-email-jon.mason@intel.com>
Mon, Jul 30, 2012 at 02:26:34AM CEST, jon.mason@intel.com wrote:
>+static int __devinit ntb_netdev_probe(struct pci_dev *pdev)
>+{
>+ struct net_device *ndev;
>+ struct ntb_netdev *dev;
>+ int rc;
>+
>+ ndev = alloc_etherdev(sizeof(struct ntb_netdev));
>+ if (!ndev)
>+ return -ENOMEM;
>+
>+ dev = netdev_priv(ndev);
>+ dev->ndev = ndev;
>+ dev->pdev = pdev;
>+ BUG_ON(!dev->pdev);
>+ ndev->features = NETIF_F_HIGHDMA;
>+
>+ //ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
^^ I guess you forgot to un-comment this.
^ permalink raw reply
* Re: [PATCH] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie @ 2012-07-30 13:33 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: Wolfgang Grandegger, linux-can, netdev
In-Reply-To: <50166BF2.9000700@pengutronix.de>
Hi Marc,
On Mon, Jul 30, 2012 at 01:11:46PM +0200, Marc Kleine-Budde wrote:
> On 07/30/2012 07:32 AM, Olivier Sobrie wrote:
> > This driver provides support for several Kvaser CAN/USB devices.
> > Such kind of devices supports up to three can network interfaces.
> >
> > It has been tested with a Kvaser USB Leaf Light (one network interface)
> > connected to a pch_can interface.
> > The firmware version of the Kvaser device was 2.5.205.
>
> Please add linux-usb@vger.kernel.org to Cc for review of the USB part.
Ok I'll do it when I send the new version of the patch.
But it might be a good idea to add an entry in the MAINTAINERS file so
that when someone sends a patch they are aware of this when the
get_maintainer script is invoked.
> Please combine .h and .c file. Please make use of netdev_LEVEL() for
> error printing, not dev_LEVEL().
I'll combine the .c and .h.
I used the netdev_LEVEL() everywhere it was possible. It requires to
have access to a pointer to netdev which is not always possible;
that's the reason why I used dev_LEVEL().
>
> Please review if all members of the struct kvaser_msg are properly
> aligned. You never access the struct kvaser_msg_* members directly, as
> they are unaligned. Please check for le16 and le32 access. You missed to
> convert the bitrate.
Indeed. Thanks. I'll check if I didn't missed another one.
>
> Please check if your driver survives hot-unplugging while sending and
> receiving CAN frames at maximum laod.
I tested this with two Kvaser sending frames with "cangen can0 -g 0 -i"
never saw a crash.
>
> More comments inline,
> regards, Marc
>
> > List of Kvaser devices supported by the driver:
> > - Kvaser Leaf prototype (P010v2 and v3)
> > - Kvaser Leaf Light (P010v3)
> > - Kvaser Leaf Professional HS
> > - Kvaser Leaf SemiPro HS
> > - Kvaser Leaf Professional LS
> > - Kvaser Leaf Professional SWC
> > - Kvaser Leaf Professional LIN
> > - Kvaser Leaf SemiPro LS
> > - Kvaser Leaf SemiPro SWC
> > - Kvaser Memorator II, Prototype
> > - Kvaser Memorator II HS/HS
> > - Kvaser USBcan Professional HS/HS
> > - Kvaser Leaf Light GI
> > - Kvaser Leaf Professional HS (OBD-II connector)
> > - Kvaser Memorator Professional HS/LS
> > - Kvaser Leaf Light "China"
> > - Kvaser BlackBird SemiPro
> > - Kvaser OEM Mercury
> > - Kvaser OEM Leaf
> > - Kvaser USBcan R
> >
> > Signed-off-by: Olivier Sobrie <olivier@sobrie.be>
> > ---
> > drivers/net/can/usb/Kconfig | 33 ++
> > drivers/net/can/usb/Makefile | 1 +
> > drivers/net/can/usb/kvaser_usb.c | 1062 ++++++++++++++++++++++++++++++++++++++
> > drivers/net/can/usb/kvaser_usb.h | 237 +++++++++
> > 4 files changed, 1333 insertions(+)
> > create mode 100644 drivers/net/can/usb/kvaser_usb.c
> > create mode 100644 drivers/net/can/usb/kvaser_usb.h
> >
> > diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
> > index 0a68768..578955f 100644
> > --- a/drivers/net/can/usb/Kconfig
> > +++ b/drivers/net/can/usb/Kconfig
> > @@ -13,6 +13,39 @@ config CAN_ESD_USB2
> > This driver supports the CAN-USB/2 interface
> > from esd electronic system design gmbh (http://www.esd.eu).
> >
> > +config CAN_KVASER_USB
> > + tristate "Kvaser CAN/USB interface"
> > + ---help---
> > + This driver adds support for Kvaser CAN/USB devices like Kvaser
> > + Leaf Light.
> > +
> > + The driver gives support for the following devices:
> > + - Kvaser Leaf prototype (P010v2 and v3)
> > + - Kvaser Leaf Light (P010v3)
> > + - Kvaser Leaf Professional HS
> > + - Kvaser Leaf SemiPro HS
> > + - Kvaser Leaf Professional LS
> > + - Kvaser Leaf Professional SWC
> > + - Kvaser Leaf Professional LIN
> > + - Kvaser Leaf SemiPro LS
> > + - Kvaser Leaf SemiPro SWC
> > + - Kvaser Memorator II, Prototype
> > + - Kvaser Memorator II HS/HS
> > + - Kvaser USBcan Professional HS/HS
> > + - Kvaser Leaf Light GI
> > + - Kvaser Leaf Professional HS (OBD-II connector)
> > + - Kvaser Memorator Professional HS/LS
> > + - Kvaser Leaf Light "China"
> > + - Kvaser BlackBird SemiPro
> > + - Kvaser OEM Mercury
> > + - Kvaser OEM Leaf
> > + - Kvaser USBcan R
> > +
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called kvaser_usb.
> > +
> > config CAN_PEAK_USB
> > tristate "PEAK PCAN-USB/USB Pro interfaces"
> > ---help---
> > diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
> > index da6d1d3..80a2ee4 100644
> > --- a/drivers/net/can/usb/Makefile
> > +++ b/drivers/net/can/usb/Makefile
> > @@ -4,6 +4,7 @@
> >
> > obj-$(CONFIG_CAN_EMS_USB) += ems_usb.o
> > obj-$(CONFIG_CAN_ESD_USB2) += esd_usb2.o
> > +obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb.o
> > obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
> >
> > ccflags-$(CONFIG_CAN_DEBUG_DEVICES) := -DDEBUG
> > diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
> > new file mode 100644
> > index 0000000..4965480
> > --- /dev/null
> > +++ b/drivers/net/can/usb/kvaser_usb.c
> > @@ -0,0 +1,1062 @@
> > +/*
>
> Please add a license statement and probably your copyright:
>
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License as
> * published by the Free Software Foundation version 2.
>
> You also should copy the copyright from the drivers you used:
>
> > + * Parts of this driver are based on the following:
> > + * - Kvaser linux leaf driver (version 4.78)
>
> I just downloaded their driver and noticed that it's quite sparse on
> stating the license the code is released under.
> "doc/HTMLhelp/copyright.htx" is quite restrictive, the word GPL occurs 3
> times, all in MODULE_LICENSE("GPL"). Running modinfo on the usbcan.ko
> shows "license: GPL"
I'll add the license statement.
In fact it's the leaf.ko which is used for this device and it is under
GPL as modinfo said.
>
> > + * - CAN driver for esd CAN-USB/2
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/completion.h>
> > +#include <linux/module.h>
> > +#include <linux/netdevice.h>
> > +#include <linux/usb.h>
> > +
> > +#include <linux/can.h>
> > +#include <linux/can/dev.h>
> > +#include <linux/can/error.h>
> > +
> > +#include "kvaser_usb.h"
> > +
> > +struct kvaser_usb_tx_urb_context {
> > + struct kvaser_usb_net_priv *priv;
>
> Huh - how does this work without forward declaration?
It works.
"In C and C++ it is possible to declare pointers to structs before
declaring their struct layout, provided the pointers are not
dereferenced--this is known as forward declaration."
See http://www.linuxtopia.org/online_books/an_introduction_to_gcc/gccintro_94.html
>
> > + u32 echo_index;
> > + int dlc;
> > +};
> > +
> > +struct kvaser_usb {
> > + struct usb_device *udev;
> > + struct kvaser_usb_net_priv *nets[MAX_NET_DEVICES];
> > +
> > + struct usb_anchor rx_submitted;
> > +
> > + u32 fw_version;
> > + unsigned int nchannels;
> > +
> > + bool rxinitdone;
> > +};
> > +
> > +struct kvaser_usb_net_priv {
> > + struct can_priv can;
> > +
> > + atomic_t active_tx_urbs;
> > + struct usb_anchor tx_submitted;
> > + struct kvaser_usb_tx_urb_context tx_contexts[MAX_TX_URBS];
> > +
> > + int open_time;
>
> please remove open_time
Ok.
>
> > + struct completion start_stop_comp;
> > +
> > + struct kvaser_usb *dev;
> > + struct net_device *netdev;
> > + int channel;
> > + struct can_berr_counter bec;
> > +};
> > +
> > +static struct usb_device_id kvaser_usb_table[] = {
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LS_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_SWC_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_LIN_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_LS_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_SPRO_SWC_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_DEVEL_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSHS_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_UPRO_HSHS_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_GI_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_OBDII_PRODUCT_ID),
> > + .driver_info = KVASER_HAS_SILENT_MODE },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMO2_HSLS_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_CH_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_BLACKBIRD_SPRO_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_MERCURY_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_OEM_LEAF_PRODUCT_ID) },
> > + { USB_DEVICE(KVASER_VENDOR_ID, USB_CAN_R_PRODUCT_ID) },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
> > +
> > +static int kvaser_usb_send_msg(const struct kvaser_usb *dev,
> > + struct kvaser_msg *msg)
> inline?
Ok.
> > +{
> > + int actual_len;
> > +
> > + return usb_bulk_msg(dev->udev, usb_sndbulkpipe(dev->udev, 1),
> ^
> Can you please introduce a #define for this.
Ok. No problem.
>
> > + msg, msg->len, &actual_len, USB_SEND_TIMEOUT);
> > +}
> > +
> > +static int kvaser_usb_wait_msg(const struct kvaser_usb *dev, u8 id,
> > + struct kvaser_msg *msg)
> > +{
> > + struct kvaser_msg *tmp;
> > + void *buf;
> > + int actual_len;
> > + int err;
> > + int pos = 0;
> > +
> > + buf = kzalloc(RX_BUFFER_SIZE, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + err = usb_bulk_msg(dev->udev, usb_rcvbulkpipe(dev->udev, 129),
> ^^^
> dito
Ok too.
>
> > + buf, RX_BUFFER_SIZE, &actual_len,
> > + USB_RECEIVE_TIMEOUT);
> > + if (err < 0) {
> > + kfree(buf);
> > + return err;
> > + }
> > +
> > + while (pos < actual_len) {
>
> Please check that pos + sizeof(*msg) is < actual_len, as you fill access
> it later.
I'll instead perform a check on 'pos + tmp->len < actual_len' and copy
only tmp->len instead of sizeof(*msg).
Thanks.
>
> > + tmp = buf + pos;
> > +
> > + if (!tmp->len)
> > + break;
> > +
> > + if (tmp->id == id) {
> > + memcpy(msg, tmp, sizeof(*msg));
> > + kfree(buf);
> > + return 0;
> > + }
> > +
> > + pos += tmp->len;
> > + }
> > +
> > + kfree(buf);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int kvaser_usb_send_simple_msg(const struct kvaser_usb *dev,
> > + u8 msg_id, int channel)
> > +{
> > + struct kvaser_msg msg;
> > + int err;
> > +
> > + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_simple);
> > + msg.id = msg_id;
> > + msg.u.simple.channel = channel;
> > + msg.u.simple.tid = 0xff;
>
> Please use C99 struct initializer.
>
> struct kvaser_msg msg = {
> .len = ,
> .id =,
> };
Ok.
>
>
> > +
> > + err = kvaser_usb_send_msg(dev, &msg);
>
> just:
> return err;
Ok.
>
> > + if (err)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
> > +{
> > + struct kvaser_msg msg;
> > + int err;
> > +
> > + err = kvaser_usb_send_simple_msg(dev, CMD_GET_SOFTWARE_INFO, 0);
> > + if (err)
> > + return err;
> > +
> > + err = kvaser_usb_wait_msg(dev, CMD_GET_SOFTWARE_INFO_REPLY, &msg);
> > + if (err)
> > + return err;
> > +
> > + dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
> > +{
> > + struct kvaser_msg msg;
> > + int err;
> > +
> > + err = kvaser_usb_send_simple_msg(dev, CMD_GET_CARD_INFO, 0);
> > + if (err)
> > + return err;
> > +
> > + err = kvaser_usb_wait_msg(dev, CMD_GET_CARD_INFO_REPLY, &msg);
> > + if (err)
> > + return err;
> > +
> > + dev->nchannels = msg.u.cardinfo.nchannels;
> > +
> > + return 0;
> > +}
> > +
> > +static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct net_device_stats *stats;
> > + struct kvaser_usb_tx_urb_context *context;
> > + struct kvaser_usb_net_priv *priv;
> > + u8 channel = msg->u.tx_acknowledge.channel;
> > + u8 tid = msg->u.tx_acknowledge.tid;
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
>
> can you do a check for (channel >= dev->nchannels), in a central place?
> e.g. kvaser_usb_handle_message()?
The problem is that channel is not always at the same place in the
messages I get from the hardware. 'tid' and 'channel' are inverted for
tx and rx frames.
e.g.
struct kvaser_msg_tx_can {
u8 channel;
u8 tid;
u8 msg[14];
u8 padding;
u8 flags;
} __packed;
struct kvaser_msg_busparams {
u8 tid;
u8 channel;
__le32 bitrate;
u8 tseg1;
u8 tseg2;
u8 sjw;
u8 no_samp;
} __packed;
>
> > +
> > + priv = dev->nets[channel];
> > +
> > + if (!netif_device_present(priv->netdev))
> > + return;
> > +
> > + stats = &priv->netdev->stats;
> > +
> > + context = &priv->tx_contexts[tid % MAX_TX_URBS];
> > +
> > + /*
> > + * It looks like the firmware never sets the flags field of the
> > + * tx_acknowledge frame and never reports a transmit failure.
> > + * If the can message can't be transmited (e.g. incompatible
> > + * bitrates), a frame CMD_CAN_ERROR_EVENT is sent (with a null
> > + * tid) and the firmware tries to transmit again the packet until
> > + * it succeeds. Once the packet is successfully transmitted, then
> > + * the tx_acknowledge frame is sent.
> > + */
> > +
> > + stats->tx_packets++;
> > + stats->tx_bytes += context->dlc;
> > + can_get_echo_skb(priv->netdev, context->echo_index);
> > +
> > + context->echo_index = MAX_TX_URBS;
> > + atomic_dec(&priv->active_tx_urbs);
> > +
> > + netif_wake_queue(priv->netdev);
> > +}
> > +
> > +static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct net_device_stats *stats;
> > + struct kvaser_usb_net_priv *priv;
> > + u8 channel, status, txerr, rxerr;
> > +
> > + if (msg->id == CMD_CAN_ERROR_EVENT) {
> > + channel = msg->u.error_event.channel;
> > + status = msg->u.error_event.status;
> > + txerr = msg->u.error_event.tx_errors_count;
> > + rxerr = msg->u.error_event.rx_errors_count;
> > + } else {
> > + channel = msg->u.chip_state_event.channel;
> > + status = msg->u.chip_state_event.status;
> > + txerr = msg->u.chip_state_event.tx_errors_count;
> > + rxerr = msg->u.chip_state_event.rx_errors_count;
> > + }
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > + stats = &priv->netdev->stats;
> > +
> > + skb = alloc_can_err_skb(priv->netdev, &cf);
> > + if (!skb) {
> > + stats->rx_dropped++;
> > + return;
> > + }
> > +
> > + if ((status & M16C_STATE_BUS_OFF) ||
> > + (status & M16C_STATE_BUS_RESET)) {
> > + priv->can.state = CAN_STATE_BUS_OFF;
> > + cf->can_id |= CAN_ERR_BUSOFF;
> > + can_bus_off(priv->netdev);
> > + } else if (status & M16C_STATE_BUS_ERROR) {
> > + priv->can.state = CAN_STATE_ERROR_WARNING;
> > + priv->can.can_stats.error_warning++;
> > + } else if (status & M16C_STATE_BUS_PASSIVE) {
> > + priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > + priv->can.can_stats.error_passive++;
> > + } else {
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + cf->can_id |= CAN_ERR_PROT;
> > + cf->data[2] = CAN_ERR_PROT_ACTIVE;
> > + }
> > +
> > + if (msg->id == CMD_CAN_ERROR_EVENT) {
> > + u8 error_factor = msg->u.error_event.error_factor;
> > +
> > + priv->can.can_stats.bus_error++;
> > + stats->rx_errors++;
> > +
> > + cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > + if ((priv->can.state == CAN_STATE_ERROR_WARNING) ||
> > + (priv->can.state == CAN_STATE_ERROR_PASSIVE)) {
> > + cf->data[1] = (txerr > rxerr) ?
> > + CAN_ERR_CRTL_TX_PASSIVE
> > + : CAN_ERR_CRTL_RX_PASSIVE;
> > + }
> > +
> > + if (error_factor & M16C_EF_ACKE)
> > + cf->data[3] |= (CAN_ERR_PROT_LOC_ACK |
> > + CAN_ERR_PROT_LOC_ACK_DEL);
> > + if (error_factor & M16C_EF_CRCE)
> > + cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
> > + CAN_ERR_PROT_LOC_CRC_DEL);
> > + if (error_factor & M16C_EF_FORME)
> > + cf->data[2] |= CAN_ERR_PROT_FORM;
> > + if (error_factor & M16C_EF_STFE)
> > + cf->data[2] |= CAN_ERR_PROT_STUFF;
> > + if (error_factor & M16C_EF_BITE0)
> > + cf->data[2] |= CAN_ERR_PROT_BIT0;
> > + if (error_factor & M16C_EF_BITE1)
> > + cf->data[2] |= CAN_ERR_PROT_BIT1;
> > + if (error_factor & M16C_EF_TRE)
> > + cf->data[2] |= CAN_ERR_PROT_TX;
> > + }
> > +
> > + cf->data[6] = txerr;
> > + cf->data[7] = rxerr;
> > +
> > + netif_rx(skb);
> > +
> > + priv->bec.txerr = txerr;
> > + priv->bec.rxerr = rxerr;
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
> > +static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct kvaser_usb_net_priv *priv;
> > + struct can_frame *cf;
> > + struct sk_buff *skb;
> > + struct net_device_stats *stats;
> > + u8 channel = msg->u.rx_can.channel;
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > + stats = &priv->netdev->stats;
> > +
> > + skb = alloc_can_skb(priv->netdev, &cf);
> > + if (skb == NULL) {
> > + stats->tx_dropped++;
> > + return;
> > + }
> > +
> > + cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
> > + (msg->u.rx_can.msg[1] & 0x3f);
> > + cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
> > +
> > + if (msg->id == CMD_RX_EXT_MESSAGE) {
> > + cf->can_id <<= 18;
> > + cf->can_id += ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
> |=
>
> is more appropriate here
Ok.
>
> > + ((msg->u.rx_can.msg[3] & 0xff) << 6) |
> > + (msg->u.rx_can.msg[4] & 0x3f);
> > + cf->can_id |= CAN_EFF_FLAG;
> > + }
> > +
> > + if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME) {
> > + cf->can_id |= CAN_RTR_FLAG;
> > + } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
> > + MSG_FLAG_NERR)) {
> > + cf->can_id |= CAN_ERR_FLAG;
> > + cf->can_dlc = CAN_ERR_DLC;
>
> What kind of error is this? Can you set cf->data? What about the
> original cd->can_id? What about the stats->rx_*error* stats?
Good question I've to take a look to this.
>
> > + } else if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
> > + cf->can_id = CAN_ERR_FLAG | CAN_ERR_CRTL;
> > + cf->can_dlc = CAN_ERR_DLC;
> > + cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > +
> > + stats->rx_over_errors++;
> > + stats->rx_errors++;
> > + } else if (!msg->u.rx_can.flag) {
> > + memcpy(cf->data, &msg->u.rx_can.msg[6], cf->can_dlc);
> > + } else {
> > + kfree_skb(skb);
> > + return;
> > + }
> > +
> > + netif_rx(skb);
> > +
> > + stats->rx_packets++;
> > + stats->rx_bytes += cf->can_dlc;
> > +}
> > +
> > +static void kvaser_usb_start_stop_chip_reply(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + struct kvaser_usb_net_priv *priv;
> > + u8 channel = msg->u.simple.channel;
> > +
> > + if (channel >= dev->nchannels) {
> > + dev_err(dev->udev->dev.parent,
> > + "Invalid channel number (%d)\n", channel);
> > + return;
> > + }
> > +
> > + priv = dev->nets[channel];
> > +
> > + complete(&priv->start_stop_comp);
> > +}
> > +
> > +static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
> > + const struct kvaser_msg *msg)
> > +{
> > + switch (msg->id) {
> > + case CMD_START_CHIP_REPLY:
> > + case CMD_STOP_CHIP_REPLY:
> > + kvaser_usb_start_stop_chip_reply(dev, msg);
> > + break;
> > +
> > + case CMD_RX_STD_MESSAGE:
> > + case CMD_RX_EXT_MESSAGE:
> > + kvaser_usb_rx_can_msg(dev, msg);
> > + break;
> > +
> > + case CMD_CHIP_STATE_EVENT:
> > + case CMD_CAN_ERROR_EVENT:
> > + kvaser_usb_rx_error(dev, msg);
> > + break;
> > +
> > + case CMD_TX_ACKNOWLEDGE:
> > + kvaser_usb_tx_acknowledge(dev, msg);
> > + break;
> > +
> > + default:
> > + dev_warn(dev->udev->dev.parent,
> > + "Unhandled message (%d)\n", msg->id);
> > + break;
> > + }
> > +}
> > +
> > +static void kvaser_usb_read_bulk_callback(struct urb *urb)
> > +{
> > + struct kvaser_usb *dev = urb->context;
> > + struct kvaser_msg *msg;
> > + int pos = 0;
> > + int err, i;
> > +
> > + switch (urb->status) {
> > + case 0:
> > + break;
> > + case -ENOENT:
> > + case -ESHUTDOWN:
> > + return;
> > + default:
> > + dev_info(dev->udev->dev.parent, "Rx URB aborted (%d)\n",
> > + urb->status);
> > + goto resubmit_urb;
> > + }
> > +
> > + while (pos < urb->actual_length) {
>
> please check here for pos + sizeof(*msg), too
Same as above.
>
> > + msg = urb->transfer_buffer + pos;
> > +
> > + if (!msg->len)
> > + break;
> > +
> > + kvaser_usb_handle_message(dev, msg);
> > +
> > + if (pos > urb->actual_length) {
> > + dev_err(dev->udev->dev.parent, "Format error\n");
> > + break;
> > + }
> > +
> > + pos += msg->len;
> > + }
> > +
> > +resubmit_urb:
> > + usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 129),
> ^^^
>
> use #define
Ok.
>
> > + urb->transfer_buffer, RX_BUFFER_SIZE,
> > + kvaser_usb_read_bulk_callback, dev);
> > +
> > + err = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (err == -ENODEV) {
> > + for (i = 0; i < dev->nchannels; i++) {
> > + if (!dev->nets[i])
> > + continue;
> > +
> > + netif_device_detach(dev->nets[i]->netdev);
> > + }
> > + } else if (err) {
> > + dev_err(dev->udev->dev.parent,
> > + "Failed resubmitting read bulk urb: %d\n", err);
> > + }
> > +
> > + return;
> > +}
> > +
> > +static int kvaser_usb_setup_rx_urbs(struct kvaser_usb *dev)
> > +{
> > + int i, err = 0;
> > +
> > + if (dev->rxinitdone)
> > + return 0;
> > +
> > + for (i = 0; i < MAX_RX_URBS; i++) {
> > + struct urb *urb = NULL;
> > + u8 *buf = NULL;
> > +
> > + urb = usb_alloc_urb(0, GFP_KERNEL);
> > + if (!urb) {
> > + dev_warn(dev->udev->dev.parent,
> > + "No memory left for URBs\n");
> > + err = -ENOMEM;
> > + break;
> > + }
> > +
> > + buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE,
> > + GFP_KERNEL, &urb->transfer_dma);
> > + if (!buf) {
> > + dev_warn(dev->udev->dev.parent,
> > + "No memory left for USB buffer\n");
> > + usb_free_urb(urb);
> > + err = -ENOMEM;
> > + break;
> > + }
> > +
> > + usb_fill_bulk_urb(urb, dev->udev,
> > + usb_rcvbulkpipe(dev->udev, 129),
>
> use #define
Ok.
>
> > + buf, RX_BUFFER_SIZE,
> > + kvaser_usb_read_bulk_callback, dev);
> > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > + usb_anchor_urb(urb, &dev->rx_submitted);
> > +
> > + err = usb_submit_urb(urb, GFP_KERNEL);
> > + if (err) {
> > + usb_unanchor_urb(urb);
> > + usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
> > + urb->transfer_dma);
> > + break;
> > + }
> > +
> > + usb_free_urb(urb);
> > + }
> > +
> > + if (i == 0) {
> > + dev_warn(dev->udev->dev.parent,
> > + "Cannot setup read URBs, error %d\n", err);
> > + return err;
> > + } else if (i < MAX_RX_URBS) {
> > + dev_warn(dev->udev->dev.parent,
> > + "RX performances may be slow\n");
> > + }
> > +
> > + dev->rxinitdone = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_set_opt_mode(const struct kvaser_usb_net_priv *priv)
> > +{
> > + struct kvaser_msg msg;
> > +
> > + memset(&msg, 0x00, sizeof(msg));
> > + msg.id = CMD_SET_CTRL_MODE;
> > + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_ctrl_mode);
> > + msg.u.ctrl_mode.tid = 0xff;
> > + msg.u.ctrl_mode.channel = priv->channel;
>
> please use C99 struct initializers
Ok.
>
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > + msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_SILENT;
> > + else
> > + msg.u.ctrl_mode.ctrl_mode = KVASER_CTRL_MODE_NORMAL;
> > +
> > + return kvaser_usb_send_msg(priv->dev, &msg);
> > +}
> > +
> > +static int kvaser_usb_start_chip(struct kvaser_usb_net_priv *priv)
> > +{
> > + int err;
> > +
> > + init_completion(&priv->start_stop_comp);
> > +
> > + err = kvaser_usb_send_simple_msg(priv->dev, CMD_START_CHIP,
> > + priv->channel);
> > + if (err)
> > + return err;
> > +
> > + if (!wait_for_completion_timeout(&priv->start_stop_comp,
> > + msecs_to_jiffies(START_TIMEOUT)))
> > + return -ETIMEDOUT;
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_open(struct net_device *netdev)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > + struct kvaser_usb *dev = priv->dev;
> > + int err;
> > +
> > + err = open_candev(netdev);
> > + if (err)
> > + return err;
> > +
> > + err = kvaser_usb_setup_rx_urbs(dev);
> > + if (err)
> > + return err;
> > +
> > + err = kvaser_usb_set_opt_mode(priv);
> > + if (err)
> > + return err;
> > +
> > + err = kvaser_usb_start_chip(priv);
> > + if (err) {
> > + netdev_warn(netdev, "Cannot start device, error %d\n", err);
> > + close_candev(netdev);
> > + return err;
> > + }
> > +
> > + priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > + priv->open_time = jiffies;
> > + netif_start_queue(netdev);
> > +
> > + return 0;
> > +}
> > +
> > +static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
> > +{
> > + int i;
> > +
> > + usb_kill_anchored_urbs(&priv->tx_submitted);
> > + atomic_set(&priv->active_tx_urbs, 0);
> > +
> > + for (i = 0; i < MAX_TX_URBS; i++)
> ARRAY_SIZE(priv->tx_contexts) instead of MAX_TX_URBS
Ok.
> > + priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> > +}
> > +
> > +static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
> > +{
> > + int i;
> > +
> > + usb_kill_anchored_urbs(&dev->rx_submitted);
> > +
> > + for (i = 0; i < MAX_NET_DEVICES; i++) {
> ARRAY_SIZE()
> > + struct kvaser_usb_net_priv *priv = dev->nets[i];
> > +
> > + if (priv)
> > + kvaser_usb_unlink_tx_urbs(priv);
> > + }
> > +}
> > +
> > +static int kvaser_usb_stop_chip(struct kvaser_usb_net_priv *priv)
> > +{
> > + int err;
> > +
> > + init_completion(&priv->start_stop_comp);
> > +
> > + err = kvaser_usb_send_simple_msg(priv->dev, CMD_STOP_CHIP,
> > + priv->channel);
> > + if (err)
> > + return err;
> > +
> > + if (!wait_for_completion_timeout(&priv->start_stop_comp,
> > + msecs_to_jiffies(STOP_TIMEOUT)))
> > + return -ETIMEDOUT;
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_flush_queue(struct kvaser_usb_net_priv *priv)
> > +{
> > + struct kvaser_msg msg;
> > +
> > + memset(&msg, 0x00, sizeof(msg));
> > + msg.id = CMD_FLUSH_QUEUE;
> > + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_flush_queue);
> > + msg.u.flush_queue.channel = priv->channel;
> C99 initialziers, please
Ok.
> > +
> > + return kvaser_usb_send_msg(priv->dev, &msg);
> > +}
> > +
> > +static int kvaser_usb_close(struct net_device *netdev)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > + int err;
> > +
> > + netif_stop_queue(netdev);
> > +
> > + err = kvaser_usb_flush_queue(priv);
> > + if (err)
> > + netdev_warn(netdev, "Cannot flush queue, error %d\n", err);
> > +
> > + err = kvaser_usb_stop_chip(priv);
> > + if (err) {
> > + netdev_warn(netdev, "Cannot stop device, error %d\n", err);
> > + return err;
> > + }
> > +
> > + kvaser_usb_unlink_tx_urbs(priv);
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > + close_candev(priv->netdev);
> > + priv->open_time = 0;
> > +
> > + return 0;
> > +}
> > +
> > +static void kvaser_usb_write_bulk_callback(struct urb *urb)
> > +{
> > + struct kvaser_usb_tx_urb_context *context = urb->context;
> > + struct kvaser_usb_net_priv *priv;
> > + struct net_device *netdev;
> > +
> > + if (WARN_ON(!context))
> > + return;
> > +
> > + priv = context->priv;
> > + netdev = priv->netdev;
> > +
> > + usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> > + urb->transfer_buffer, urb->transfer_dma);
> > +
> > + if (!netif_device_present(netdev))
> > + return;
> > +
> > + if (urb->status)
> > + netdev_info(netdev, "Tx URB aborted (%d)\n", urb->status);
> > +
> > + netdev->trans_start = jiffies;
>
> Is trans_start needed? at least for non-usb devices it works without.
I don't know, I'll try to figure this out.
I see it's used in the two others CAN/USB drivers, 'ems_usb.c' and
'esd_usb2.c'
>
> > +}
> > +
> > +static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> > + struct net_device *netdev)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > + struct kvaser_usb *dev = priv->dev;
> > + struct net_device_stats *stats = &netdev->stats;
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > + struct kvaser_usb_tx_urb_context *context = NULL;
> > + struct urb *urb;
> > + void *buf;
> > + struct kvaser_msg *msg;
> > + int i, err;
> > + int ret = NETDEV_TX_OK;
> > +
> > + if (can_dropped_invalid_skb(netdev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + urb = usb_alloc_urb(0, GFP_ATOMIC);
> > + if (!urb) {
> > + netdev_err(netdev, "No memory left for URBs\n");
> > + stats->tx_dropped++;
> > + dev_kfree_skb(skb);
> > + goto nourbmem;
> > + }
> > +
> > + buf = usb_alloc_coherent(dev->udev, sizeof(struct kvaser_msg),
> > + GFP_ATOMIC, &urb->transfer_dma);
> > + if (!buf) {
> > + netdev_err(netdev, "No memory left for USB buffer\n");
> > + stats->tx_dropped++;
> > + dev_kfree_skb(skb);
> > + goto nobufmem;
> > + }
> > +
> > + msg = (struct kvaser_msg *)buf;
> > + msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
> > + msg->u.tx_can.flags = 0;
> > + msg->u.tx_can.channel = priv->channel;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + msg->id = CMD_TX_EXT_MESSAGE;
> > + msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
> > + msg->u.tx_can.msg[1] = (cf->can_id >> 18) & 0x3f;
> > + msg->u.tx_can.msg[2] = (cf->can_id >> 14) & 0x0f;
> > + msg->u.tx_can.msg[3] = (cf->can_id >> 6) & 0xff;
> > + msg->u.tx_can.msg[4] = cf->can_id & 0x3f;
> > + } else {
> > + msg->id = CMD_TX_STD_MESSAGE;
> > + msg->u.tx_can.msg[0] = (cf->can_id >> 6) & 0x1f;
> > + msg->u.tx_can.msg[1] = cf->can_id & 0x3f;
> > + }
> > +
> > + msg->u.tx_can.msg[5] = cf->can_dlc;
> > + memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
> > +
> > + for (i = 0; i < MAX_TX_URBS; i++) {
> ARRAY_SIZE
Ok.
> > + if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> > + context = &priv->tx_contexts[i];
> > + break;
> > + }
> > + }
> > +
> > + if (!context) {
> > + netdev_warn(netdev, "cannot find free context\n");
> > + ret = NETDEV_TX_BUSY;
> > + goto releasebuf;
> > + }
> > +
> > + context->priv = priv;
> > + context->echo_index = i;
> > + context->dlc = cf->can_dlc;
> > +
> > + msg->u.tx_can.tid = context->echo_index;
> > +
> > + usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 1),
> > + buf, msg->len,
> > + kvaser_usb_write_bulk_callback, context);
> > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > + usb_anchor_urb(urb, &priv->tx_submitted);
> > +
> > + can_put_echo_skb(skb, netdev, context->echo_index);
> > +
> > + atomic_inc(&priv->active_tx_urbs);
> > +
> > + if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
> > + netif_stop_queue(netdev);
> > +
> > + err = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (unlikely(err)) {
> > + can_free_echo_skb(netdev, context->echo_index);
> > +
> > + atomic_dec(&priv->active_tx_urbs);
> > + usb_unanchor_urb(urb);
> > +
> > + stats->tx_dropped++;
> > +
> > + if (err == -ENODEV)
> > + netif_device_detach(netdev);
> > + else
> > + netdev_warn(netdev, "Failed tx_urb %d\n", err);
> > +
> > + goto releasebuf;
> > + }
> > +
> > + netdev->trans_start = jiffies;
> > +
> > + usb_free_urb(urb);
> > +
> > + return NETDEV_TX_OK;
> > +
> > +releasebuf:
> > + usb_free_coherent(dev->udev, sizeof(struct kvaser_msg),
> > + buf, urb->transfer_dma);
> > +nobufmem:
> > + usb_free_urb(urb);
> > +nourbmem:
> > + return ret;
> > +}
> > +
> > +static const struct net_device_ops kvaser_usb_netdev_ops = {
> > + .ndo_open = kvaser_usb_open,
> > + .ndo_stop = kvaser_usb_close,
> > + .ndo_start_xmit = kvaser_usb_start_xmit,
> > +};
> > +
> > +static struct can_bittiming_const kvaser_usb_bittiming_const = {
> > + .name = "kvaser_usb",
> > + .tseg1_min = KVASER_USB_TSEG1_MIN,
> > + .tseg1_max = KVASER_USB_TSEG1_MAX,
> > + .tseg2_min = KVASER_USB_TSEG2_MIN,
> > + .tseg2_max = KVASER_USB_TSEG2_MAX,
> > + .sjw_max = KVASER_USB_SJW_MAX,
> > + .brp_min = KVASER_USB_BRP_MIN,
> > + .brp_max = KVASER_USB_BRP_MAX,
> > + .brp_inc = KVASER_USB_BRP_INC,
> > +};
> > +
> > +static int kvaser_usb_set_bittiming(struct net_device *netdev)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > + struct can_bittiming *bt = &priv->can.bittiming;
> > + struct kvaser_usb *dev = priv->dev;
> > + struct kvaser_msg msg;
> > +
> > + msg.id = CMD_SET_BUS_PARAMS;
> > + msg.len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_busparams);
> > + msg.u.busparams.channel = priv->channel;
> > + msg.u.busparams.tid = 0xff;
> > + msg.u.busparams.bitrate = bt->bitrate;
>
> bitrate is le32
Indeed ! I'll fix this.
>
> > + msg.u.busparams.sjw = bt->sjw;
> > + msg.u.busparams.tseg1 = bt->prop_seg + bt->phase_seg1;
> > + msg.u.busparams.tseg2 = bt->phase_seg2;
>
> C99 initializers, please
>
> > +
> > + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> > + msg.u.busparams.no_samp = 3;
> > + else
> > + msg.u.busparams.no_samp = 1;
> > +
> > + return kvaser_usb_send_msg(dev, &msg);
> > +}
> > +
> > +static int kvaser_usb_set_mode(struct net_device *netdev,
> > + enum can_mode mode)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > +
> > + if (!priv->open_time)
> > + return -EINVAL;
> > +
> > + switch (mode) {
> > + case CAN_MODE_START:
> > + if (netif_queue_stopped(netdev))
> > + netif_wake_queue(netdev);
>
> No need to restart your USB device?
No. I don't think so.
The module continuously tries to transmit the frame and isn't stopped.
So there is no need to restart it if it has been explicitely stopped.
When it cannot transmit, the module try again and sends continuously
CMD_CAN_ERROR_EVENT frames until it succeeds to transmit the frame.
If the device is stopped with the command CMD_STOP_CHIP then it stops
sending these CMD_CAN_ERROR_EVENT.
Should I handle this in another manner?
>
> > + break;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_get_berr_counter(const struct net_device *netdev,
> > + struct can_berr_counter *bec)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > +
> > + bec->txerr = priv->bec.txerr;
> > + bec->rxerr = priv->bec.rxerr;
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_init_one(struct usb_interface *intf,
> > + const struct usb_device_id *id, int channel)
> > +{
> > + struct kvaser_usb *dev = usb_get_intfdata(intf);
> > + struct net_device *netdev;
> > + struct kvaser_usb_net_priv *priv;
> > + int i, err;
> > +
> > + netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> > + if (!netdev) {
> > + dev_err(&intf->dev, "Cannot alloc candev\n");
> > + return -ENOMEM;
> > + }
> > +
> > + priv = netdev_priv(netdev);
> > +
> > + init_usb_anchor(&priv->tx_submitted);
> > + atomic_set(&priv->active_tx_urbs, 0);
> > +
> > + for (i = 0; i < MAX_TX_URBS; i++)
> > + priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> > +
> > + priv->dev = dev;
> > + priv->netdev = netdev;
> > + priv->channel = channel;
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > + priv->can.clock.freq = CAN_USB_CLOCK;
> > + priv->can.bittiming_const = &kvaser_usb_bittiming_const;
> > + priv->can.do_set_bittiming = kvaser_usb_set_bittiming;
> > + priv->can.do_set_mode = kvaser_usb_set_mode;
> > + priv->can.do_get_berr_counter = kvaser_usb_get_berr_counter;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> > + if (id->driver_info & KVASER_HAS_SILENT_MODE)
> > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
> > +
> > + netdev->flags |= IFF_ECHO;
> > +
> > + netdev->netdev_ops = &kvaser_usb_netdev_ops;
> > +
> > + SET_NETDEV_DEV(netdev, &intf->dev);
> > +
> > + err = register_candev(netdev);
> > + if (err) {
> > + dev_err(&intf->dev, "Failed to register can device\n");
> > + free_candev(netdev);
> > + return err;
> > + }
> > +
> > + dev->nets[channel] = priv;
> > + netdev_info(netdev, "device %s registered\n", netdev->name);
>
> netdev_info should take care of printing the device's name.
Ok.
> > +
> > + return 0;
> > +}
> > +
> > +static int kvaser_usb_probe(struct usb_interface *intf,
> > + const struct usb_device_id *id)
> > +{
> > + struct kvaser_usb *dev;
> > + int err = -ENOMEM;
> > + int i;
> > +
> > + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>
> Who will free dev on driver unload? Please make use of devm_kzalloc().
Ok. kfree is missing is disconnect().
I'll replace it by devm_kzalloc() and devm_free().
>
> > + if (!dev)
> > + return -ENOMEM;
> > +
> > + dev->udev = interface_to_usbdev(intf);
> > +
> > + init_usb_anchor(&dev->rx_submitted);
> > +
> > + usb_set_intfdata(intf, dev);
> > +
> > + if (kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, 0)) {
> > + dev_err(&intf->dev, "Cannot reset kvaser\n");
> > + goto error;
> > + }
> > +
> > + if (kvaser_usb_get_software_info(dev)) {
> > + dev_err(&intf->dev, "Cannot get software infos\n");
> > + goto error;
> > + }
> > +
> > + if (kvaser_usb_get_card_info(dev)) {
> > + dev_err(&intf->dev, "Cannot get card infos\n");
> > + goto error;
> > + }
> > +
> > + dev_dbg(&intf->dev, "Firmware version: %d.%d.%d\n",
> > + ((dev->fw_version >> 24) & 0xff),
> > + ((dev->fw_version >> 16) & 0xff),
> > + (dev->fw_version & 0xffff));
> > +
> > + for (i = 0; i < dev->nchannels; i++)
> > + kvaser_usb_init_one(intf, id, i);
> > +
> > + return 0;
> > +
> > +error:
> > + kfree(dev);
> > + return err;
> > +}
> > +
> > +static void kvaser_usb_disconnect(struct usb_interface *intf)
> > +{
> > + struct kvaser_usb *dev = usb_get_intfdata(intf);
> > + int i;
> > +
> > + usb_set_intfdata(intf, NULL);
> > +
> > + if (!dev)
> > + return;
> > +
> > + for (i = 0; i < dev->nchannels; i++) {
> > + if (!dev->nets[i])
> > + continue;
> > +
> > + unregister_netdev(dev->nets[i]->netdev);
> > + free_candev(dev->nets[i]->netdev);
> > + }
> > +
> > + kvaser_usb_unlink_all_urbs(dev);
> > +}
> > +
> > +static struct usb_driver kvaser_usb_driver = {
> > + .name = "kvaser_usb",
> > + .probe = kvaser_usb_probe,
> > + .disconnect = kvaser_usb_disconnect,
> > + .id_table = kvaser_usb_table
> > +};
> > +
> > +module_usb_driver(kvaser_usb_driver);
> > +
> > +MODULE_AUTHOR("Olivier Sobrie <olivier@sobrie.be>");
> > +MODULE_DESCRIPTION("Can driver for Kvaser CAN/USB devices");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/net/can/usb/kvaser_usb.h b/drivers/net/can/usb/kvaser_usb.h
> > new file mode 100644
> > index 0000000..8e0b6ab
> > --- /dev/null
> > +++ b/drivers/net/can/usb/kvaser_usb.h
> > @@ -0,0 +1,237 @@
> > +#ifndef _KVASER_USB_H_
> > +#define _KVASER_USB_H_
> > +
> > +#define MAX_TX_URBS 16
> Please no tab between define and macro name
Ok I didn't know it was not allowed... checkpatch didn't complain.
> > +#define MAX_RX_URBS 4
> > +#define START_TIMEOUT 1000
> > +#define STOP_TIMEOUT 1000
> > +#define USB_SEND_TIMEOUT 1000
> > +#define USB_RECEIVE_TIMEOUT 1000
> > +#define RX_BUFFER_SIZE 3072
> > +#define CAN_USB_CLOCK 8000000
> > +#define MAX_NET_DEVICES 3
> > +
> > +/* Kvaser USB devices */
> > +#define KVASER_VENDOR_ID 0x0bfd
> > +#define USB_LEAF_DEVEL_PRODUCT_ID 10
> > +#define USB_LEAF_LITE_PRODUCT_ID 11
> > +#define USB_LEAF_PRO_PRODUCT_ID 12
> > +#define USB_LEAF_SPRO_PRODUCT_ID 14
> > +#define USB_LEAF_PRO_LS_PRODUCT_ID 15
> > +#define USB_LEAF_PRO_SWC_PRODUCT_ID 16
> > +#define USB_LEAF_PRO_LIN_PRODUCT_ID 17
> > +#define USB_LEAF_SPRO_LS_PRODUCT_ID 18
> > +#define USB_LEAF_SPRO_SWC_PRODUCT_ID 19
> > +#define USB_MEMO2_DEVEL_PRODUCT_ID 22
> > +#define USB_MEMO2_HSHS_PRODUCT_ID 23
> > +#define USB_UPRO_HSHS_PRODUCT_ID 24
> > +#define USB_LEAF_LITE_GI_PRODUCT_ID 25
> > +#define USB_LEAF_PRO_OBDII_PRODUCT_ID 26
> > +#define USB_MEMO2_HSLS_PRODUCT_ID 27
> > +#define USB_LEAF_LITE_CH_PRODUCT_ID 28
> > +#define USB_BLACKBIRD_SPRO_PRODUCT_ID 29
> > +#define USB_OEM_MERCURY_PRODUCT_ID 34
> > +#define USB_OEM_LEAF_PRODUCT_ID 35
> > +#define USB_CAN_R_PRODUCT_ID 39
> > +
> > +/* USB devices features */
> > +#define KVASER_HAS_SILENT_MODE (1 << 0)
> pleae use BIT(0)
> > +
> > +/* Message header size */
> > +#define MSG_HEADER_LEN 2
> > +
> > +/* Can message flags */
> > +#define MSG_FLAG_ERROR_FRAME (1 << 0)
> > +#define MSG_FLAG_OVERRUN (1 << 1)
> > +#define MSG_FLAG_NERR (1 << 2)
> > +#define MSG_FLAG_WAKEUP (1 << 3)
> > +#define MSG_FLAG_REMOTE_FRAME (1 << 4)
> > +#define MSG_FLAG_RESERVED (1 << 5)
> > +#define MSG_FLAG_TX_ACK (1 << 6)
> > +#define MSG_FLAG_TX_REQUEST (1 << 7)
> > +
> > +/* Can states */
> > +#define M16C_STATE_BUS_RESET (1 << 0)
> > +#define M16C_STATE_BUS_ERROR (1 << 4)
> > +#define M16C_STATE_BUS_PASSIVE (1 << 5)
> > +#define M16C_STATE_BUS_OFF (1 << 6)
> > +
> > +/* Can msg ids */
> > +#define CMD_RX_STD_MESSAGE 12
> > +#define CMD_TX_STD_MESSAGE 13
> > +#define CMD_RX_EXT_MESSAGE 14
> > +#define CMD_TX_EXT_MESSAGE 15
> > +#define CMD_SET_BUS_PARAMS 16
> > +#define CMD_GET_BUS_PARAMS 17
> > +#define CMD_GET_BUS_PARAMS_REPLY 18
> > +#define CMD_GET_CHIP_STATE 19
> > +#define CMD_CHIP_STATE_EVENT 20
> > +#define CMD_SET_CTRL_MODE 21
> > +#define CMD_GET_CTRL_MODE 22
> > +#define CMD_GET_CTRL_MODE_REPLY 23
> > +#define CMD_RESET_CHIP 24
> > +#define CMD_RESET_CHIP_REPLY 25
> > +#define CMD_START_CHIP 26
> > +#define CMD_START_CHIP_REPLY 27
> > +#define CMD_STOP_CHIP 28
> > +#define CMD_STOP_CHIP_REPLY 29
> > +#define CMD_GET_CARD_INFO2 32
> > +#define CMD_GET_CARD_INFO 34
> > +#define CMD_GET_CARD_INFO_REPLY 35
> > +#define CMD_GET_SOFTWARE_INFO 38
> > +#define CMD_GET_SOFTWARE_INFO_REPLY 39
> > +#define CMD_ERROR_EVENT 45
> > +#define CMD_FLUSH_QUEUE 48
> > +#define CMD_TX_ACKNOWLEDGE 50
> > +#define CMD_CAN_ERROR_EVENT 51
> > +#define CMD_USB_THROTTLE 77
> > +
> > +/* error factors */
> > +#define M16C_EF_ACKE (1 << 0)
> > +#define M16C_EF_CRCE (1 << 1)
> > +#define M16C_EF_FORME (1 << 2)
> > +#define M16C_EF_STFE (1 << 3)
> > +#define M16C_EF_BITE0 (1 << 4)
> > +#define M16C_EF_BITE1 (1 << 5)
> > +#define M16C_EF_RCVE (1 << 6)
> > +#define M16C_EF_TRE (1 << 7)
> > +
> > +/* bittiming parameters */
> > +#define KVASER_USB_TSEG1_MIN 1
> > +#define KVASER_USB_TSEG1_MAX 16
> > +#define KVASER_USB_TSEG2_MIN 1
> > +#define KVASER_USB_TSEG2_MAX 8
> > +#define KVASER_USB_SJW_MAX 4
> > +#define KVASER_USB_BRP_MIN 1
> > +#define KVASER_USB_BRP_MAX 64
> > +#define KVASER_USB_BRP_INC 1
> > +
> > +/* ctrl modes */
> > +#define KVASER_CTRL_MODE_NORMAL 1
> > +#define KVASER_CTRL_MODE_SILENT 2
> > +#define KVASER_CTRL_MODE_SELFRECEPTION 3
> > +#define KVASER_CTRL_MODE_OFF 4
> > +
> > +struct kvaser_msg_simple {
> > + u8 tid;
> > + u8 channel;
> > +} __packed;
> > +
> > +struct kvaser_msg_cardinfo {
> > + u8 tid;
> > + u8 nchannels;
> > + __le32 serial_number;
> > + __le32 padding;
> > + __le32 clock_resolution;
> > + __le32 mfgdate;
> > + u8 ean[8];
> > + u8 hw_revision;
> > + u8 usb_hs_mode;
> > + __le16 padding2;
> > +} __packed;
> > +
> > +struct kvaser_msg_cardinfo2 {
> > + u8 tid;
> > + u8 channel;
> > + u8 pcb_id[24];
> > + __le32 oem_unlock_code;
> > +} __packed;
> > +
> > +struct kvaser_msg_softinfo {
> > + u8 tid;
> > + u8 channel;
> > + __le32 sw_options;
> > + __le32 fw_version;
> > + __le16 max_outstanding_tx;
> > + __le16 padding[9];
> > +} __packed;
> > +
> > +struct kvaser_msg_busparams {
> > + u8 tid;
> > + u8 channel;
> > + __le32 bitrate;
> > + u8 tseg1;
> > + u8 tseg2;
> > + u8 sjw;
> > + u8 no_samp;
> > +} __packed;
> > +
> > +struct kvaser_msg_tx_can {
> > + u8 channel;
> > + u8 tid;
> > + u8 msg[14];
> > + u8 padding;
> > + u8 flags;
> > +} __packed;
> > +
> > +struct kvaser_msg_rx_can {
> > + u8 channel;
> > + u8 flag;
> > + __le16 time[3];
> > + u8 msg[14];
> > +} __packed;
> > +
> > +struct kvaser_msg_chip_state_event {
> > + u8 tid;
> > + u8 channel;
> > + __le16 time[3];
> > + u8 tx_errors_count;
> > + u8 rx_errors_count;
> > + u8 status;
> > + u8 padding[3];
> > +} __packed;
> > +
> > +struct kvaser_msg_tx_acknowledge {
> > + u8 channel;
> > + u8 tid;
> > + __le16 time[3];
> > + u8 flags;
> > + u8 time_offset;
> > +} __packed;
> > +
> > +struct kvaser_msg_error_event {
> > + u8 tid;
> > + u8 flags;
> > + __le16 time[3];
> > + u8 channel;
> > + u8 padding;
> > + u8 tx_errors_count;
> > + u8 rx_errors_count;
> > + u8 status;
> > + u8 error_factor;
> > +} __packed;
> > +
> > +struct kvaser_msg_ctrl_mode {
> > + u8 tid;
> > + u8 channel;
> > + u8 ctrl_mode;
> > + u8 padding[3];
> > +} __packed;
> > +
> > +struct kvaser_msg_flush_queue {
> > + u8 tid;
> > + u8 channel;
> > + u8 flags;
> > + u8 padding[3];
> > +} __packed;
> > +
> > +struct kvaser_msg {
> > + u8 len;
> > + u8 id;
> > + union {
> > + struct kvaser_msg_simple simple;
> > + struct kvaser_msg_cardinfo cardinfo;
> > + struct kvaser_msg_cardinfo2 cardinfo2;
> > + struct kvaser_msg_softinfo softinfo;
> > + struct kvaser_msg_busparams busparams;
> > + struct kvaser_msg_tx_can tx_can;
> > + struct kvaser_msg_rx_can rx_can;
> > + struct kvaser_msg_chip_state_event chip_state_event;
> > + struct kvaser_msg_tx_acknowledge tx_acknowledge;
> > + struct kvaser_msg_error_event error_event;
> > + struct kvaser_msg_ctrl_mode ctrl_mode;
> > + struct kvaser_msg_ctrl_mode flush_queue;
> > + } u;
> > +} __packed;
> > +
> > +#endif
> >
>
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
Thanks for the review.
Regards,
--
Olivier
^ permalink raw reply
* Re: [net-next RFC V5 4/5] virtio_net: multiqueue support
From: Sasha Levin @ 2012-07-30 13:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: krkumar2, habanero, kvm, netdev, mashirle, linux-kernel,
virtualization, edumazet, tahm, jwhan, davem, sri
In-Reply-To: <20120729094451.GB8977@redhat.com>
On 07/29/2012 11:44 AM, Michael S. Tsirkin wrote:
> On Sat, Jul 21, 2012 at 02:02:58PM +0200, Sasha Levin wrote:
>> On 07/20/2012 03:40 PM, Michael S. Tsirkin wrote:
>>>> - err = init_vqs(vi);
>>>>> + if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
>>>>> + vi->has_cvq = true;
>>>>> +
>>> How about we disable multiqueue if there's no cvq?
>>> Will make logic a bit simpler, won't it?
>>
>> multiqueues don't really depend on cvq. Does this added complexity really justifies adding an artificial limit?
>
> Well !cvq support is a legacy feature: the reason we support it
> in driver is to avoid breaking on old hosts. Adding more code to that
> path just doesn't make much sense since old hosts won't have mq.
Is it really a legacy feature? The spec suggests that its an optional queue which is not necessary for the operation of the device.
Which is why we never implemented it in lkvm - we weren't interested in any of the features it provided at that time and we could provide high performance with vhost support even without it.
^ permalink raw reply
* Re: [PATCH net,1/1] hyperv: Add support for setting MAC from within guests
From: Olaf Hering @ 2012-07-30 12:39 UTC (permalink / raw)
To: Haiyang Zhang; +Cc: davem, netdev, kys, linux-kernel, devel
In-Reply-To: <1341940762-18307-1-git-send-email-haiyangz@microsoft.com>
On Tue, Jul 10, Haiyang Zhang wrote:
> diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
> index 981ebb1..fbf5394 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -47,6 +48,7 @@ struct rndis_request {
> struct hv_page_buffer buf;
> /* FIXME: We assumed a fixed size request here. */
> struct rndis_message request_msg;
> + u8 ext[100];
This array is not referenced in the patch.
Please add a comment to the code what the purpose of this array is, and
why its size is 100 bytes.
Thanks.
Olaf
^ permalink raw reply
* Re: [PATCH 1/2] net: Allow to create links with given ifindex
From: Eric W. Biederman @ 2012-07-30 12:33 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Pavel Emelyanov, Linux Netdev List, David Miller
In-Reply-To: <1343649062.21269.23.camel@edumazet-glaptop>
Eric Dumazet <eric.dumazet@gmail.com> writes:
> On Mon, 2012-07-30 at 03:49 -0700, Eric W. Biederman wrote:
>> Pavel Emelyanov <xemul@parallels.com> writes:
>>
>> > Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
>> > is not zero. I propose to allow requesting ifindices on link creation. This
>> > is required by the checkpoint-restore to correctly restore a net namespace
>> > (i.e. -- a container). The question what to do with pre-created devices such
>> > as lo or sit fbdev is open, but for manually created devices this can be
>> > solved by this patch.
>>
>> Have you walked through and found the locations where we still rely on
>> ifindex being globally unique?
>>
>> Last time I was working in this area there were serveral places where
>> things were indexed by just the interface index.
>
> Really ? This would be very strange.
There at least were places that used oif or iff without being pernet
last time I was working on this.
It was never code that I understood particularly well so my memory of
what that code is, is unfortunately fuzzy.
> AFAIK dev_new_index() is always called, even in the
> dev_change_net_namespace() case if there is a conflict.
Except we never have a conflict because it takes an absurd number of
network devices to cause a 32bit counter to wrap.
> And dev_new_index() could use a pernet net->ifindex instead of a
> shared/static one.
Yes. I made all of the core changes, and held back on making
dev_new_index() use a pernet net->ifindex because of a couple of problem
cases.
It has been a long time and those cases might have been fixed.
I'm not seeing anything obvious in the network stack with a quick skim,
but before we start relying on the property that interface indicies are
not globally unique I expect an good hard look at the networking stack
to see if any of those cases where there were problems still exist.
Eric
^ permalink raw reply
* Re: [net-next,v3] GRE over IPv6
From: Timo Teras @ 2012-07-30 12:26 UTC (permalink / raw)
To: Kozlov Dmitry; +Cc: netdev
In-Reply-To: <4682082.2P7rpuTCdd@dima>
On Mon, 30 Jul 2012 15:52:46 +0400 Kozlov Dmitry <xeb@mail.ru> wrote:
> On Monday 30 July 2012 14:38:06 Timo Teras wrote:
> > On Sat, 28 Jul 2012 22:12:42 -0000 xeb@mail.ru wrote:
> > > GRE over IPv6 implementation.
> > >
> > > Signed-off-by: Dmitry Kozlov <xeb@mail.ru>
> > >
> > > ---
> > > Changes:
> > > Initialize nt->dev before calling ip6gre_tnl_link_config in
> > > ip6gre_newlink.
> > > Add missing ip6gre.c
> > >
> > > include/linux/if_arp.h | 1 +
> > > include/linux/if_tunnel.h | 3 +
> > > include/linux/ip6_tunnel.h | 18 +
> > > include/net/ip6_tunnel.h | 40 +-
> > > include/net/ipv6.h | 1 +
> > > net/ipv6/Kconfig | 16 +
> > > net/ipv6/Makefile | 1 +
> > > net/ipv6/ip6_gre.c | 1817
> > >
> > > ++++++++++++++++++++++++++++++++++++++++++++
> > > net/ipv6/ip6_tunnel.c | 86 ++- 9 files changed, 1958
> > > insertions(+), 25 deletions(-)
> >
> > Would it be possible and/or feasible to instead modify ip_gre to
> > support also ipv6 as outer protocol?
> >
> > It already has ipv6 stuff in it for the inner protocol support. And
> > it would avoid duplicating most of the code.
> >
> > And I would especially love that approach, since I could then on
> > per-target basis say if it should be contacted with IPv4 or IPv6.
> > As in:
> >
> > ip addr add 10.0.0.1/24 dev gre1
> > ip neigh add 10.0.0.2 lladdr 192.168.x.x dev gre1 nud permanent
> > ip neigh add 10.0.0.3 lladdr fe80::xxxx:xxxx:xxxx:xxxx/64 dev
> > gre1 nud permanent
>
> Sounds good, but it involves too many if/else because there are much
> ipv4 and ipv6 specifics and code will be unreadable. I see only
> shared part is tunnel initialization and managing code. Tunnel
> lookup, receive and transmit parts are mostly different.
Hmm... And thinking more, it looks like various other places need lot
of tuning; e.g. tunnel might need multiple local address bindings
which is not nice.
And now that I checked, seems Cisco also needs separate tunnel
interfaces for "over IPv4" and "over IPv6" targets. That's rather
inconvenient, but seems to be how things are.
^ permalink raw reply
* Re: [net-next,v3] GRE over IPv6
From: Kozlov Dmitry @ 2012-07-30 11:52 UTC (permalink / raw)
To: Timo Teras; +Cc: netdev
In-Reply-To: <20120730143806.41f759fd@vostro>
On Monday 30 July 2012 14:38:06 Timo Teras wrote:
> On Sat, 28 Jul 2012 22:12:42 -0000 xeb@mail.ru wrote:
> > GRE over IPv6 implementation.
> >
> > Signed-off-by: Dmitry Kozlov <xeb@mail.ru>
> >
> > ---
> > Changes:
> > Initialize nt->dev before calling ip6gre_tnl_link_config in
> > ip6gre_newlink.
> > Add missing ip6gre.c
> >
> > include/linux/if_arp.h | 1 +
> > include/linux/if_tunnel.h | 3 +
> > include/linux/ip6_tunnel.h | 18 +
> > include/net/ip6_tunnel.h | 40 +-
> > include/net/ipv6.h | 1 +
> > net/ipv6/Kconfig | 16 +
> > net/ipv6/Makefile | 1 +
> > net/ipv6/ip6_gre.c | 1817
> >
> > ++++++++++++++++++++++++++++++++++++++++++++
> > net/ipv6/ip6_tunnel.c | 86 ++- 9 files changed, 1958
> > insertions(+), 25 deletions(-)
>
> Would it be possible and/or feasible to instead modify ip_gre to support
> also ipv6 as outer protocol?
>
> It already has ipv6 stuff in it for the inner protocol support. And it
> would avoid duplicating most of the code.
>
> And I would especially love that approach, since I could then on
> per-target basis say if it should be contacted with IPv4 or IPv6. As in:
>
> ip addr add 10.0.0.1/24 dev gre1
> ip neigh add 10.0.0.2 lladdr 192.168.x.x dev gre1 nud permanent
> ip neigh add 10.0.0.3 lladdr fe80::xxxx:xxxx:xxxx:xxxx/64 dev gre1 nud
> permanent
Sounds good, but it involves too many if/else because there are much ipv4 and ipv6 specifics and code will be unreadable.
I see only shared part is tunnel initialization and managing code.
Tunnel lookup, receive and transmit parts are mostly different.
^ permalink raw reply
* Re: [PATCH 1/2] net: Allow to create links with given ifindex
From: Eric Dumazet @ 2012-07-30 11:51 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Pavel Emelyanov, Linux Netdev List, David Miller
In-Reply-To: <87pq7dh6b2.fsf@xmission.com>
On Mon, 2012-07-30 at 03:49 -0700, Eric W. Biederman wrote:
> Pavel Emelyanov <xemul@parallels.com> writes:
>
> > Currently the RTM_NEWLINK results in -EOPNOTSUPP if the ifinfomsg->ifi_index
> > is not zero. I propose to allow requesting ifindices on link creation. This
> > is required by the checkpoint-restore to correctly restore a net namespace
> > (i.e. -- a container). The question what to do with pre-created devices such
> > as lo or sit fbdev is open, but for manually created devices this can be
> > solved by this patch.
>
> Have you walked through and found the locations where we still rely on
> ifindex being globally unique?
>
> Last time I was working in this area there were serveral places where
> things were indexed by just the interface index.
Really ? This would be very strange.
AFAIK dev_new_index() is always called, even in the
dev_change_net_namespace() case if there is a conflict.
And dev_new_index() could use a pernet net->ifindex instead of a
shared/static one.
^ 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