* [PATCH 2/6] Drivers: net: hyperv: Cleanup the send path
2014-03-06 11:13 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O K. Y. Srinivasan
@ 2014-03-06 11:13 ` K. Y. Srinivasan
2014-03-06 19:30 ` David Miller
2014-03-06 11:13 ` [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host K. Y. Srinivasan
` (4 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: K. Y. Srinivasan @ 2014-03-06 11:13 UTC (permalink / raw)
To: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
In preparation for enabling offloads, cleanup the send path.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 7 +---
drivers/net/hyperv/netvsc_drv.c | 87 +++++++++++++++++++++++++++++-------
drivers/net/hyperv/rndis_filter.c | 66 ----------------------------
3 files changed, 71 insertions(+), 89 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 39fc230..694bf7c 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -73,7 +73,7 @@ struct hv_netvsc_packet {
} completion;
/* This points to the memory after page_buf */
- void *extension;
+ struct rndis_message *rndis_msg;
u32 total_data_buflen;
/* Points to the send/receive buffer where the ethernet frame is */
@@ -126,11 +126,6 @@ void rndis_filter_device_remove(struct hv_device *dev);
int rndis_filter_receive(struct hv_device *dev,
struct hv_netvsc_packet *pkt);
-
-
-int rndis_filter_send(struct hv_device *dev,
- struct hv_netvsc_packet *pkt);
-
int rndis_filter_set_packet_filter(struct rndis_device *dev, u32 new_filter);
int rndis_filter_set_device_mac(struct hv_device *hdev, char *mac);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index afa9c53..e812529 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -128,6 +128,28 @@ static int netvsc_close(struct net_device *net)
return ret;
}
+
+static void *init_ppi_data(struct rndis_message *msg, u32 ppi_size,
+ int pkt_type)
+{
+ struct rndis_packet *rndis_pkt;
+ struct rndis_per_packet_info *ppi;
+
+ rndis_pkt = &msg->msg.pkt;
+ rndis_pkt->data_offset += ppi_size;
+
+ ppi = (struct rndis_per_packet_info *)((ulong)rndis_pkt +
+ rndis_pkt->per_pkt_info_offset + rndis_pkt->per_pkt_info_len);
+
+ ppi->size = ppi_size;
+ ppi->type = pkt_type;
+ ppi->ppi_offset = sizeof(struct rndis_per_packet_info);
+
+ rndis_pkt->per_pkt_info_len += ppi_size;
+
+ return ppi;
+}
+
static void netvsc_xmit_completion(void *context)
{
struct hv_netvsc_packet *packet = (struct hv_netvsc_packet *)context;
@@ -173,7 +195,7 @@ static u32 fill_pg_buf(struct page *page, u32 offset, u32 len,
return j + 1;
}
-static void init_page_array(void *hdr, u32 len, struct sk_buff *skb,
+static u32 init_page_array(void *hdr, u32 len, struct sk_buff *skb,
struct hv_page_buffer *pb)
{
u32 slots_used = 0;
@@ -204,6 +226,7 @@ static void init_page_array(void *hdr, u32 len, struct sk_buff *skb,
frag->page_offset,
skb_frag_size(frag), &pb[slots_used]);
}
+ return slots_used;
}
static int count_skb_frag_slots(struct sk_buff *skb)
@@ -241,15 +264,20 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
struct net_device_context *net_device_ctx = netdev_priv(net);
struct hv_netvsc_packet *packet;
int ret;
- unsigned int num_data_pages;
+ unsigned int num_data_pgs;
+ struct rndis_message *rndis_msg;
+ struct rndis_packet *rndis_pkt;
+ u32 rndis_msg_size;
+ bool isvlan;
+ struct rndis_per_packet_info *ppi;
/*
* We will atmost need two pages to describe the rndis
* header. We can only transmit MAX_PAGE_BUFFER_COUNT number
* of pages in a single packet.
*/
- num_data_pages = netvsc_get_slots(skb) + 2;
- if (num_data_pages > MAX_PAGE_BUFFER_COUNT) {
+ num_data_pgs = netvsc_get_slots(skb) + 2;
+ if (num_data_pgs > MAX_PAGE_BUFFER_COUNT) {
netdev_err(net, "Packet too big: %u\n", skb->len);
dev_kfree_skb(skb);
net->stats.tx_dropped++;
@@ -258,7 +286,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
/* Allocate a netvsc packet based on # of frags. */
packet = kzalloc(sizeof(struct hv_netvsc_packet) +
- (num_data_pages * sizeof(struct hv_page_buffer)) +
+ (num_data_pgs * sizeof(struct hv_page_buffer)) +
sizeof(struct rndis_message) +
NDIS_VLAN_PPI_SIZE, GFP_ATOMIC);
if (!packet) {
@@ -272,26 +300,51 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
packet->vlan_tci = skb->vlan_tci;
- packet->extension = (void *)(unsigned long)packet +
- sizeof(struct hv_netvsc_packet) +
- (num_data_pages * sizeof(struct hv_page_buffer));
-
- /* If the rndis msg goes beyond 1 page, we will add 1 later */
- packet->page_buf_cnt = num_data_pages - 1;
-
- /* Initialize it from the skb */
+ packet->is_data_pkt = true;
packet->total_data_buflen = skb->len;
- /* Start filling in the page buffers starting after RNDIS buffer. */
- init_page_array(NULL, 0, skb, &packet->page_buf[1]);
+ packet->rndis_msg = (struct rndis_message *)((unsigned long)packet +
+ sizeof(struct hv_netvsc_packet) +
+ (num_data_pgs * sizeof(struct hv_page_buffer)));
/* Set the completion routine */
packet->completion.send.send_completion = netvsc_xmit_completion;
packet->completion.send.send_completion_ctx = packet;
packet->completion.send.send_completion_tid = (unsigned long)skb;
- ret = rndis_filter_send(net_device_ctx->device_ctx,
- packet);
+ isvlan = packet->vlan_tci & VLAN_TAG_PRESENT;
+
+ /* Add the rndis header */
+ rndis_msg = packet->rndis_msg;
+ rndis_msg->ndis_msg_type = RNDIS_MSG_PACKET;
+ rndis_msg->msg_len = packet->total_data_buflen;
+ rndis_pkt = &rndis_msg->msg.pkt;
+ rndis_pkt->data_offset = sizeof(struct rndis_packet);
+ rndis_pkt->data_len = packet->total_data_buflen;
+ rndis_pkt->per_pkt_info_offset = sizeof(struct rndis_packet);
+
+ rndis_msg_size = RNDIS_MESSAGE_SIZE(struct rndis_packet);
+
+ if (isvlan) {
+ struct ndis_pkt_8021q_info *vlan;
+
+ rndis_msg_size += NDIS_VLAN_PPI_SIZE;
+ ppi = init_ppi_data(rndis_msg, NDIS_VLAN_PPI_SIZE,
+ IEEE_8021Q_INFO);
+ vlan = (struct ndis_pkt_8021q_info *)((ulong)ppi +
+ ppi->ppi_offset);
+ vlan->vlanid = packet->vlan_tci & VLAN_VID_MASK;
+ vlan->pri = (packet->vlan_tci & VLAN_PRIO_MASK) >>
+ VLAN_PRIO_SHIFT;
+ }
+
+ /* Start filling in the page buffers with the rndis hdr */
+ rndis_msg->msg_len += rndis_msg_size;
+ packet->page_buf_cnt = init_page_array(rndis_msg, rndis_msg_size,
+ skb, &packet->page_buf[0]);
+
+ ret = netvsc_send(net_device_ctx->device_ctx, packet);
+
if (ret == 0) {
net->stats.tx_bytes += skb->len;
net->stats.tx_packets++;
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 6a9f602..dcbf144e 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -910,69 +910,3 @@ int rndis_filter_close(struct hv_device *dev)
return rndis_filter_close_device(nvdev->extension);
}
-
-int rndis_filter_send(struct hv_device *dev,
- struct hv_netvsc_packet *pkt)
-{
- struct rndis_message *rndis_msg;
- struct rndis_packet *rndis_pkt;
- u32 rndis_msg_size;
- bool isvlan = pkt->vlan_tci & VLAN_TAG_PRESENT;
-
- /* Add the rndis header */
- rndis_msg = (struct rndis_message *)pkt->extension;
-
- rndis_msg_size = RNDIS_MESSAGE_SIZE(struct rndis_packet);
- if (isvlan)
- rndis_msg_size += NDIS_VLAN_PPI_SIZE;
-
- rndis_msg->ndis_msg_type = RNDIS_MSG_PACKET;
- rndis_msg->msg_len = pkt->total_data_buflen +
- rndis_msg_size;
-
- rndis_pkt = &rndis_msg->msg.pkt;
- rndis_pkt->data_offset = sizeof(struct rndis_packet);
- if (isvlan)
- rndis_pkt->data_offset += NDIS_VLAN_PPI_SIZE;
- rndis_pkt->data_len = pkt->total_data_buflen;
-
- if (isvlan) {
- struct rndis_per_packet_info *ppi;
- struct ndis_pkt_8021q_info *vlan;
-
- rndis_pkt->per_pkt_info_offset = sizeof(struct rndis_packet);
- rndis_pkt->per_pkt_info_len = NDIS_VLAN_PPI_SIZE;
-
- ppi = (struct rndis_per_packet_info *)((ulong)rndis_pkt +
- rndis_pkt->per_pkt_info_offset);
- ppi->size = NDIS_VLAN_PPI_SIZE;
- ppi->type = IEEE_8021Q_INFO;
- ppi->ppi_offset = sizeof(struct rndis_per_packet_info);
-
- vlan = (struct ndis_pkt_8021q_info *)((ulong)ppi +
- ppi->ppi_offset);
- vlan->vlanid = pkt->vlan_tci & VLAN_VID_MASK;
- vlan->pri = (pkt->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
- }
-
- pkt->is_data_pkt = true;
- pkt->page_buf[0].pfn = virt_to_phys(rndis_msg) >> PAGE_SHIFT;
- pkt->page_buf[0].offset =
- (unsigned long)rndis_msg & (PAGE_SIZE-1);
- pkt->page_buf[0].len = rndis_msg_size;
-
- /* Add one page_buf if the rndis msg goes beyond page boundary */
- if (pkt->page_buf[0].offset + rndis_msg_size > PAGE_SIZE) {
- int i;
- for (i = pkt->page_buf_cnt; i > 1; i--)
- pkt->page_buf[i] = pkt->page_buf[i-1];
- pkt->page_buf_cnt++;
- pkt->page_buf[0].len = PAGE_SIZE - pkt->page_buf[0].offset;
- pkt->page_buf[1].pfn = virt_to_phys((void *)((ulong)
- rndis_msg + pkt->page_buf[0].len)) >> PAGE_SHIFT;
- pkt->page_buf[1].offset = 0;
- pkt->page_buf[1].len = rndis_msg_size - pkt->page_buf[0].len;
- }
-
- return netvsc_send(dev, pkt);
-}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 2/6] Drivers: net: hyperv: Cleanup the send path
2014-03-06 11:13 ` [PATCH 2/6] Drivers: net: hyperv: Cleanup the send path K. Y. Srinivasan
@ 2014-03-06 19:30 ` David Miller
0 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2014-03-06 19:30 UTC (permalink / raw)
To: kys; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel
From: "K. Y. Srinivasan" <kys@microsoft.com>
Date: Thu, 6 Mar 2014 03:13:06 -0800
> In preparation for enabling offloads, cleanup the send path.
>
> Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
> drivers/net/hyperv/hyperv_net.h | 7 +---
> drivers/net/hyperv/netvsc_drv.c | 87 +++++++++++++++++++++++++++++-------
> drivers/net/hyperv/rndis_filter.c | 66 ----------------------------
> 3 files changed, 71 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
> index 39fc230..694bf7c 100644
> --- a/drivers/net/hyperv/hyperv_net.h
> +++ b/drivers/net/hyperv/hyperv_net.h
> @@ -73,7 +73,7 @@ struct hv_netvsc_packet {
> } completion;
>
> /* This points to the memory after page_buf */
> - void *extension;
> + struct rndis_message *rndis_msg;
>
> u32 total_data_buflen;
> /* Points to the send/receive buffer where the ethernet frame is */
> @@ -126,11 +126,6 @@ void rndis_filter_device_remove(struct hv_device *dev);
> int rndis_filter_receive(struct hv_device *dev,
> struct hv_netvsc_packet *pkt);
>
> -
> -
> -int rndis_filter_send(struct hv_device *dev,
> - struct hv_netvsc_packet *pkt);
> -
> int rndis_filter_set_packet_filter(struct rndis_device *dev, u32 new_filter);
> int rndis_filter_set_device_mac(struct hv_device *hdev, char *mac);
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index afa9c53..e812529 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -128,6 +128,28 @@ static int netvsc_close(struct net_device *net)
> return ret;
> }
>
> +
> +static void *init_ppi_data(struct rndis_message *msg, u32 ppi_size,
One empty line between functions is sufficient.
> @@ -173,7 +195,7 @@ static u32 fill_pg_buf(struct page *page, u32 offset, u32 len,
> return j + 1;
> }
>
> -static void init_page_array(void *hdr, u32 len, struct sk_buff *skb,
> +static u32 init_page_array(void *hdr, u32 len, struct sk_buff *skb,
> struct hv_page_buffer *pb)
When you adjust where the openning parenthesis occurs in a function
definition or call, you must adjust the indentation of arguments that
occur on the following lines.
In particular, such arguments must start at exactly the first column
after the openning parenthesis. You must use the appropriate number
of TAB and space characters necessary to achieve this.
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host
2014-03-06 11:13 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O K. Y. Srinivasan
2014-03-06 11:13 ` [PATCH 2/6] Drivers: net: hyperv: Cleanup the send path K. Y. Srinivasan
@ 2014-03-06 11:13 ` K. Y. Srinivasan
2014-03-06 19:31 ` David Miller
2014-03-06 19:36 ` Dan Carpenter
2014-03-06 11:13 ` [PATCH 4/6] Drivers: net: hyperv: Enable receive side IP checksum offload K. Y. Srinivasan
` (3 subsequent siblings)
5 siblings, 2 replies; 30+ messages in thread
From: K. Y. Srinivasan @ 2014-03-06 11:13 UTC (permalink / raw)
To: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
Prior to enabling guest side offloads, enable the offloads on the host.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 55 ++++++++++++++++++++++++
drivers/net/hyperv/rndis_filter.c | 83 +++++++++++++++++++++++++++++++++++++
2 files changed, 138 insertions(+), 0 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 694bf7c..8bc4e76 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -721,6 +721,61 @@ struct ndis_pkt_8021q_info {
};
};
+struct ndis_oject_header {
+ u8 type;
+ u8 revision;
+ u16 size;
+};
+
+#define NDIS_OBJECT_TYPE_DEFAULT 0x80
+#define NDIS_OFFLOAD_PARAMETERS_REVISION_3 3
+#define NDIS_OFFLOAD_PARAMETERS_NO_CHANGE 0
+#define NDIS_OFFLOAD_PARAMETERS_LSOV2_DISABLED 1
+#define NDIS_OFFLOAD_PARAMETERS_LSOV2_ENABLED 2
+#define NDIS_OFFLOAD_PARAMETERS_LSOV1_ENABLED 2
+#define NDIS_OFFLOAD_PARAMETERS_RSC_DISABLED 1
+#define NDIS_OFFLOAD_PARAMETERS_RSC_ENABLED 2
+#define NDIS_OFFLOAD_PARAMETERS_TX_RX_DISABLED 1
+#define NDIS_OFFLOAD_PARAMETERS_TX_ENABLED_RX_DISABLED 2
+#define NDIS_OFFLOAD_PARAMETERS_RX_ENABLED_TX_DISABLED 3
+#define NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED 4
+
+/*
+ * New offload OIDs for NDIS 6
+ */
+#define OID_TCP_OFFLOAD_CURRENT_CONFIG 0xFC01020B /* query only */
+#define OID_TCP_OFFLOAD_PARAMETERS 0xFC01020C /* set only */
+#define OID_TCP_OFFLOAD_HARDWARE_CAPABILITIES 0xFC01020D/* query only */
+#define OID_TCP_CONNECTION_OFFLOAD_CURRENT_CONFIG 0xFC01020E /* query only */
+#define OID_TCP_CONNECTION_OFFLOAD_HARDWARE_CAPABILITIES 0xFC01020F /* query */
+#define OID_OFFLOAD_ENCAPSULATION 0x0101010A /* set/query */
+
+struct ndis_offload_params {
+ struct ndis_oject_header header;
+ u8 ip_v4_csum;
+ u8 tcp_ip_v4_csum;
+ u8 udp_ip_v4_csum;
+ u8 tcp_ip_v6_csum;
+ u8 udp_ip_v6_csum;
+ u8 lso_v1;
+ u8 ip_sec_v1;
+ u8 lso_v2_ipv4;
+ u8 lso_v2_ipv6;
+ u8 tcp_connection_ip_v4;
+ u8 tcp_connection_ip_v6;
+ u32 flags;
+ u8 ip_sec_v2;
+ u8 ip_sec_v2_ip_v4;
+ struct {
+ u8 rsc_ip_v4;
+ u8 rsc_ip_v6;
+ };
+ struct {
+ u8 encapsulated_packet_task_offload;
+ u8 encapsulation_types;
+ };
+};
+
#define NDIS_VLAN_PPI_SIZE (sizeof(struct rndis_per_packet_info) + \
sizeof(struct ndis_pkt_8021q_info))
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index dcbf144e..9e257ac 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -627,6 +627,62 @@ cleanup:
return ret;
}
+int rndis_filter_set_offload_params(struct hv_device *hdev,
+ struct ndis_offload_params *req_offloads)
+{
+ struct netvsc_device *nvdev = hv_get_drvdata(hdev);
+ struct rndis_device *rdev = nvdev->extension;
+ struct net_device *ndev = nvdev->ndev;
+ struct rndis_request *request;
+ struct rndis_set_request *set;
+ struct ndis_offload_params *offload_params;
+ struct rndis_set_complete *set_complete;
+ u32 extlen = sizeof(struct ndis_offload_params);
+ int ret, t;
+
+ request = get_rndis_request(rdev, RNDIS_MSG_SET,
+ RNDIS_MESSAGE_SIZE(struct rndis_set_request) + extlen);
+ if (!request)
+ return -ENOMEM;
+
+ set = &request->request_msg.msg.set_req;
+ set->oid = OID_TCP_OFFLOAD_PARAMETERS;
+ set->info_buflen = extlen;
+ set->info_buf_offset = sizeof(struct rndis_set_request);
+ set->dev_vc_handle = 0;
+
+ offload_params = (struct ndis_offload_params *)((ulong)set +
+ set->info_buf_offset);
+ *offload_params = *req_offloads;
+ offload_params->header.type = NDIS_OBJECT_TYPE_DEFAULT;
+ offload_params->header.revision = NDIS_OFFLOAD_PARAMETERS_REVISION_3;
+ offload_params->header.size = extlen;
+
+ ret = rndis_filter_send_request(rdev, request);
+ if (ret != 0)
+ goto cleanup;
+
+ t = wait_for_completion_timeout(&request->wait_event, 5*HZ);
+ if (t == 0) {
+ netdev_err(ndev, "timeout before we got aOFFLOAD set response...\n");
+ /*
+ * can't put_rndis_request, since we may still receive a
+ * send-completion.
+ */
+ return -EBUSY;
+ } else {
+ set_complete = &request->response_msg.msg.set_complete;
+ if (set_complete->status != RNDIS_STATUS_SUCCESS) {
+ netdev_err(ndev, "Fail to set MAC on host side:0x%x\n",
+ set_complete->status);
+ ret = -EINVAL;
+ }
+ }
+
+cleanup:
+ put_rndis_request(rdev, request);
+ return ret;
+}
static int rndis_filter_query_device_link_status(struct rndis_device *dev)
{
@@ -826,6 +882,7 @@ int rndis_filter_device_add(struct hv_device *dev,
struct netvsc_device *net_device;
struct rndis_device *rndis_device;
struct netvsc_device_info *device_info = additional_info;
+ struct ndis_offload_params offloads;
rndis_device = get_rndis_device();
if (!rndis_device)
@@ -865,6 +922,28 @@ int rndis_filter_device_add(struct hv_device *dev,
memcpy(device_info->mac_adr, rndis_device->hw_mac_adr, ETH_ALEN);
+ /*
+ * Turn on the offloads; the host supports all of the relevant
+ * offloads.
+ */
+ memset(&offloads, 0, sizeof(struct ndis_offload_params));
+ /*
+ * A value of zero means "no change"; now turn on what we
+ * want.
+ */
+ offloads.ip_v4_csum = NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED;
+ offloads.tcp_ip_v4_csum = NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED;
+ offloads.udp_ip_v4_csum = NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED;
+ offloads.tcp_ip_v6_csum = NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED;
+ offloads.udp_ip_v6_csum = NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED;
+ offloads.lso_v2_ipv4 = NDIS_OFFLOAD_PARAMETERS_LSOV2_ENABLED;
+
+
+ ret = rndis_filter_set_offload_params(dev, &offloads);
+ if (ret)
+ goto err_dev_remv;
+
+
rndis_filter_query_device_link_status(rndis_device);
device_info->link_state = rndis_device->link_state;
@@ -874,6 +953,10 @@ int rndis_filter_device_add(struct hv_device *dev,
device_info->link_state ? "down" : "up");
return ret;
+
+err_dev_remv:
+ rndis_filter_device_remove(dev);
+ return ret;
}
void rndis_filter_device_remove(struct hv_device *dev)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host
2014-03-06 11:13 ` [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host K. Y. Srinivasan
@ 2014-03-06 19:31 ` David Miller
2014-03-06 19:36 ` Dan Carpenter
1 sibling, 0 replies; 30+ messages in thread
From: David Miller @ 2014-03-06 19:31 UTC (permalink / raw)
To: kys; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel
From: "K. Y. Srinivasan" <kys@microsoft.com>
Date: Thu, 6 Mar 2014 03:13:07 -0800
> + /*
> + * can't put_rndis_request, since we may still receive a
> + * send-completion.
> + */
Please fix the formatting of this comment.
> + /*
> + * Turn on the offloads; the host supports all of the relevant
> + * offloads.
> + */
Likewise.
> + /*
> + * A value of zero means "no change"; now turn on what we
> + * want.
> + */
Likewise.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host
2014-03-06 11:13 ` [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host K. Y. Srinivasan
2014-03-06 19:31 ` David Miller
@ 2014-03-06 19:36 ` Dan Carpenter
1 sibling, 0 replies; 30+ messages in thread
From: Dan Carpenter @ 2014-03-06 19:36 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel, davem
On Thu, Mar 06, 2014 at 03:13:07AM -0800, K. Y. Srinivasan wrote:
> + offload_params = (struct ndis_offload_params *)((ulong)set +
> + set->info_buf_offset);
It's a bit simpler to do pointer math like this:
offload_params = (void *)set + set->info_buf_offset;
regards,
dan carpenter
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 4/6] Drivers: net: hyperv: Enable receive side IP checksum offload
2014-03-06 11:13 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O K. Y. Srinivasan
2014-03-06 11:13 ` [PATCH 2/6] Drivers: net: hyperv: Cleanup the send path K. Y. Srinivasan
2014-03-06 11:13 ` [PATCH 3/6] Drivers: net: hyperv: Enable offloads on the host K. Y. Srinivasan
@ 2014-03-06 11:13 ` K. Y. Srinivasan
2014-03-06 19:31 ` David Miller
2014-03-06 11:13 ` [PATCH 5/6] Drivers: net: hyperv: Enable send side " K. Y. Srinivasan
` (2 subsequent siblings)
5 siblings, 1 reply; 30+ messages in thread
From: K. Y. Srinivasan @ 2014-03-06 11:13 UTC (permalink / raw)
To: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
Enable receive side checksum offload.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 33 ++++++++++++++++++++++++++++++++-
drivers/net/hyperv/netvsc_drv.c | 20 ++++++++++++++++----
drivers/net/hyperv/rndis_filter.c | 4 +++-
3 files changed, 51 insertions(+), 6 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 8bc4e76..faeb746 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -30,6 +30,7 @@
/* Fwd declaration */
struct hv_netvsc_packet;
+struct ndis_tcp_ip_checksum_info;
/* Represent the xfer page packet which contains 1 or more netvsc packet */
struct xferpage_packet {
@@ -117,7 +118,8 @@ int netvsc_send(struct hv_device *device,
void netvsc_linkstatus_callback(struct hv_device *device_obj,
unsigned int status);
int netvsc_recv_callback(struct hv_device *device_obj,
- struct hv_netvsc_packet *packet);
+ struct hv_netvsc_packet *packet,
+ struct ndis_tcp_ip_checksum_info *csum_info);
int rndis_filter_open(struct hv_device *dev);
int rndis_filter_close(struct hv_device *dev);
int rndis_filter_device_add(struct hv_device *dev,
@@ -776,9 +778,38 @@ struct ndis_offload_params {
};
};
+struct ndis_tcp_ip_checksum_info {
+ union {
+ struct {
+ u32 is_ipv4:1;
+ u32 is_ipv6:1;
+ u32 tcp_checksum:1;
+ u32 udp_checksum:1;
+ u32 ip_header_checksum:1;
+ u32 reserved:11;
+ u32 tcp_header_offset:10;
+ } transmit;
+ struct {
+ u32 tcp_checksum_failed:1;
+ u32 udp_checksum_failed:1;
+ u32 ip_checksum_failed:1;
+ u32 tcp_checksum_succeeded:1;
+ u32 udp_checksum_succeeded:1;
+ u32 ip_checksum_succeeded:1;
+ u32 loopback:1;
+ u32 tcp_checksum_value_invalid:1;
+ u32 ip_checksum_value_invalid:1;
+ } receive;
+ u32 value;
+ };
+};
+
#define NDIS_VLAN_PPI_SIZE (sizeof(struct rndis_per_packet_info) + \
sizeof(struct ndis_pkt_8021q_info))
+#define NDIS_CSUM_PPI_SIZE (sizeof(struct rndis_per_packet_info) + \
+ sizeof(struct ndis_tcp_ip_checksum_info))
+
/* Format of Information buffer passed in a SetRequest for the OID */
/* OID_GEN_RNDIS_CONFIG_PARAMETER. */
struct rndis_config_parameter_info {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index e812529..98562fc 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -394,7 +394,8 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj,
* "wire" on the specified device.
*/
int netvsc_recv_callback(struct hv_device *device_obj,
- struct hv_netvsc_packet *packet)
+ struct hv_netvsc_packet *packet,
+ struct ndis_tcp_ip_checksum_info *csum_info)
{
struct net_device *net;
struct sk_buff *skb;
@@ -421,7 +422,18 @@ int netvsc_recv_callback(struct hv_device *device_obj,
packet->total_data_buflen);
skb->protocol = eth_type_trans(skb, net);
- skb->ip_summed = CHECKSUM_NONE;
+ if (csum_info) {
+ /*
+ * We only look at the IP checksum here.
+ * Should we be dropping the packet if checksum
+ * failed? How do we deal with other checksums - TCP/UDP?
+ */
+ if (csum_info->receive.ip_checksum_succeeded)
+ skb->ip_summed = CHECKSUM_UNNECESSARY;
+ else
+ skb->ip_summed = CHECKSUM_NONE;
+ }
+
if (packet->vlan_tci & VLAN_TAG_PRESENT)
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
packet->vlan_tci);
@@ -581,8 +593,8 @@ static int netvsc_probe(struct hv_device *dev,
net->netdev_ops = &device_ops;
/* TODO: Add GSO and Checksum offload */
- net->hw_features = NETIF_F_SG;
- net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG;
+ net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG;
+ net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG | NETIF_F_RXCSUM;
SET_ETHTOOL_OPS(net, ðtool_ops);
SET_NETDEV_DEV(net, &dev->device);
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index 9e257ac..9cbe666 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -370,6 +370,7 @@ static void rndis_filter_receive_data(struct rndis_device *dev,
struct rndis_packet *rndis_pkt;
u32 data_offset;
struct ndis_pkt_8021q_info *vlan;
+ struct ndis_tcp_ip_checksum_info *csum_info;
rndis_pkt = &msg->msg.pkt;
@@ -408,7 +409,8 @@ static void rndis_filter_receive_data(struct rndis_device *dev,
pkt->vlan_tci = 0;
}
- netvsc_recv_callback(dev->net_dev->dev, pkt);
+ csum_info = rndis_get_ppi(rndis_pkt, TCPIP_CHKSUM_PKTINFO);
+ netvsc_recv_callback(dev->net_dev->dev, pkt, csum_info);
}
int rndis_filter_receive(struct hv_device *dev,
--
1.7.4.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum offload
2014-03-06 11:13 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O K. Y. Srinivasan
` (2 preceding siblings ...)
2014-03-06 11:13 ` [PATCH 4/6] Drivers: net: hyperv: Enable receive side IP checksum offload K. Y. Srinivasan
@ 2014-03-06 11:13 ` K. Y. Srinivasan
2014-03-06 19:33 ` David Miller
2014-03-09 18:53 ` Ben Hutchings
2014-03-06 11:13 ` [PATCH 6/6] Drivers: net: hyperv: Enable large send offload K. Y. Srinivasan
2014-03-06 19:29 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O David Miller
5 siblings, 2 replies; 30+ messages in thread
From: K. Y. Srinivasan @ 2014-03-06 11:13 UTC (permalink / raw)
To: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
Enable send side checksum offload.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 70 +++++++++++++++++++++++++++++++++++++-
1 files changed, 68 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 98562fc..e8159a8 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -259,6 +259,36 @@ static int netvsc_get_slots(struct sk_buff *skb)
return slots + frag_slots;
}
+bool get_net_transport_info(struct sk_buff *skb, bool *is_v4,
+ bool *is_tcp, bool *is_udp, u32 *trans_off)
+{
+ *is_tcp = false;
+ *is_udp = false;
+
+ if ((eth_hdr(skb)->h_proto != htons(ETH_P_IP)) &&
+ (eth_hdr(skb)->h_proto != htons(ETH_P_IPV6))) {
+ return false;
+ }
+
+ *trans_off = skb_transport_offset(skb);
+
+ if ((eth_hdr(skb)->h_proto == htons(ETH_P_IP))) {
+ struct iphdr *iphdr = ip_hdr(skb);
+ *is_v4 = true;
+ if (iphdr->protocol == IPPROTO_TCP)
+ *is_tcp = true;
+ else if (iphdr->protocol == IPPROTO_UDP)
+ *is_udp = true;
+ } else {
+ *is_v4 = false;
+ if (ipv6_hdr(skb)->nexthdr == IPPROTO_TCP)
+ *is_tcp = true;
+ else if (ipv6_hdr(skb)->nexthdr == IPPROTO_UDP)
+ *is_udp = true;
+ }
+ return true;
+}
+
static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
@@ -270,6 +300,12 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
u32 rndis_msg_size;
bool isvlan;
struct rndis_per_packet_info *ppi;
+ struct ndis_tcp_ip_checksum_info *csum_info;
+ int hdr_offset;
+ bool is_v4;
+ bool is_tcp;
+ bool is_udp;
+
/*
* We will atmost need two pages to describe the rndis
@@ -338,6 +374,35 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
VLAN_PRIO_SHIFT;
}
+ if (!get_net_transport_info(skb, &is_v4, &is_tcp, &is_udp, &hdr_offset))
+ goto do_send;
+ /*
+ * Setup the sendside checksum offload only if this is not a
+ * GSO packet.
+ */
+ if (skb_is_gso(skb))
+ goto do_send;
+
+ rndis_msg_size += NDIS_CSUM_PPI_SIZE;
+ ppi = init_ppi_data(rndis_msg, NDIS_CSUM_PPI_SIZE,
+ TCPIP_CHKSUM_PKTINFO);
+
+ csum_info = (struct ndis_tcp_ip_checksum_info *)((ulong)ppi +
+ ppi->ppi_offset);
+
+ if (is_v4)
+ csum_info->transmit.is_ipv4 = 1;
+ else
+ csum_info->transmit.is_ipv6 = 1;
+
+ if (is_tcp) {
+ csum_info->transmit.tcp_checksum = 1;
+ csum_info->transmit.tcp_header_offset = hdr_offset;
+ } else if (is_udp) {
+ csum_info->transmit.udp_checksum = 1;
+ }
+
+do_send:
/* Start filling in the page buffers with the rndis hdr */
rndis_msg->msg_len += rndis_msg_size;
packet->page_buf_cnt = init_page_array(rndis_msg, rndis_msg_size,
@@ -593,8 +658,9 @@ static int netvsc_probe(struct hv_device *dev,
net->netdev_ops = &device_ops;
/* TODO: Add GSO and Checksum offload */
- net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG;
- net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG | NETIF_F_RXCSUM;
+ net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_IP_CSUM;
+ net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG | NETIF_F_RXCSUM |
+ NETIF_F_IP_CSUM;
SET_ETHTOOL_OPS(net, ðtool_ops);
SET_NETDEV_DEV(net, &dev->device);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum offload
2014-03-06 11:13 ` [PATCH 5/6] Drivers: net: hyperv: Enable send side " K. Y. Srinivasan
@ 2014-03-06 19:33 ` David Miller
2014-03-06 20:29 ` KY Srinivasan
2014-03-09 18:53 ` Ben Hutchings
1 sibling, 1 reply; 30+ messages in thread
From: David Miller @ 2014-03-06 19:33 UTC (permalink / raw)
To: kys; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel
From: "K. Y. Srinivasan" <kys@microsoft.com>
Date: Thu, 6 Mar 2014 03:13:09 -0800
> +bool get_net_transport_info(struct sk_buff *skb, bool *is_v4,
> + bool *is_tcp, bool *is_udp, u32 *trans_off)
> +{
Returning so many values like this is awkward, at best.
Why not return a well defined bitmask:
#define TRANSPORT_INFO_SUCCESS 0x00000001
#define TRANSPORT_INFO_TCP 0x00000002
#define TRANSPORT_INFO_UDP 0x00000004
Then the only value you have to return by reference is trans_off.
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum offload
2014-03-06 19:33 ` David Miller
@ 2014-03-06 20:29 ` KY Srinivasan
2014-03-06 20:48 ` David Miller
0 siblings, 1 reply; 30+ messages in thread
From: KY Srinivasan @ 2014-03-06 20:29 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, olaf@aepfle.de, apw@canonical.com,
jasowang@redhat.com
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Friday, March 7, 2014 1:04 AM
> To: KY Srinivasan
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum
> offload
>
> From: "K. Y. Srinivasan" <kys@microsoft.com>
> Date: Thu, 6 Mar 2014 03:13:09 -0800
>
> > +bool get_net_transport_info(struct sk_buff *skb, bool *is_v4,
> > + bool *is_tcp, bool *is_udp, u32 *trans_off) {
>
> Returning so many values like this is awkward, at best.
>
> Why not return a well defined bitmask:
>
> #define TRANSPORT_INFO_SUCCESS 0x00000001
> #define TRANSPORT_INFO_TCP 0x00000002
> #define TRANSPORT_INFO_UDP 0x00000004
>
> Then the only value you have to return by reference is trans_off.
I also need information on the IP version as well. If it is ok with you, I will use the return
value to get information on the network protocol and return information on transport via
reference - protocol and the offset.
Regards,
K. Y
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum offload
2014-03-06 20:29 ` KY Srinivasan
@ 2014-03-06 20:48 ` David Miller
2014-03-06 21:00 ` KY Srinivasan
0 siblings, 1 reply; 30+ messages in thread
From: David Miller @ 2014-03-06 20:48 UTC (permalink / raw)
To: kys; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel
From: KY Srinivasan <kys@microsoft.com>
Date: Thu, 6 Mar 2014 20:29:13 +0000
>
>
>> -----Original Message-----
>> From: David Miller [mailto:davem@davemloft.net]
>> Sent: Friday, March 7, 2014 1:04 AM
>> To: KY Srinivasan
>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
>> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
>> jasowang@redhat.com
>> Subject: Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum
>> offload
>>
>> From: "K. Y. Srinivasan" <kys@microsoft.com>
>> Date: Thu, 6 Mar 2014 03:13:09 -0800
>>
>> > +bool get_net_transport_info(struct sk_buff *skb, bool *is_v4,
>> > + bool *is_tcp, bool *is_udp, u32 *trans_off) {
>>
>> Returning so many values like this is awkward, at best.
>>
>> Why not return a well defined bitmask:
>>
>> #define TRANSPORT_INFO_SUCCESS 0x00000001
>> #define TRANSPORT_INFO_TCP 0x00000002
>> #define TRANSPORT_INFO_UDP 0x00000004
>>
>> Then the only value you have to return by reference is trans_off.
>
> I also need information on the IP version as well. If it is ok with you, I will use the return
> value to get information on the network protocol and return information on transport via
> reference - protocol and the offset.
Just add "TRANSPORT_INFO_IPv4" as another bitmask in the return value.
^ permalink raw reply [flat|nested] 30+ messages in thread* RE: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum offload
2014-03-06 20:48 ` David Miller
@ 2014-03-06 21:00 ` KY Srinivasan
0 siblings, 0 replies; 30+ messages in thread
From: KY Srinivasan @ 2014-03-06 21:00 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, olaf@aepfle.de, apw@canonical.com,
jasowang@redhat.com
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Friday, March 7, 2014 2:19 AM
> To: KY Srinivasan
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> jasowang@redhat.com
> Subject: Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum
> offload
>
> From: KY Srinivasan <kys@microsoft.com>
> Date: Thu, 6 Mar 2014 20:29:13 +0000
>
> >
> >
> >> -----Original Message-----
> >> From: David Miller [mailto:davem@davemloft.net]
> >> Sent: Friday, March 7, 2014 1:04 AM
> >> To: KY Srinivasan
> >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> >> devel@linuxdriverproject.org; olaf@aepfle.de; apw@canonical.com;
> >> jasowang@redhat.com
> >> Subject: Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side
> >> checksum offload
> >>
> >> From: "K. Y. Srinivasan" <kys@microsoft.com>
> >> Date: Thu, 6 Mar 2014 03:13:09 -0800
> >>
> >> > +bool get_net_transport_info(struct sk_buff *skb, bool *is_v4,
> >> > + bool *is_tcp, bool *is_udp, u32 *trans_off) {
> >>
> >> Returning so many values like this is awkward, at best.
> >>
> >> Why not return a well defined bitmask:
> >>
> >> #define TRANSPORT_INFO_SUCCESS 0x00000001
> >> #define TRANSPORT_INFO_TCP 0x00000002
> >> #define TRANSPORT_INFO_UDP 0x00000004
> >>
> >> Then the only value you have to return by reference is trans_off.
> >
> > I also need information on the IP version as well. If it is ok with
> > you, I will use the return value to get information on the network
> > protocol and return information on transport via reference - protocol and
> the offset.
>
> Just add "TRANSPORT_INFO_IPv4" as another bitmask in the return value.
The network information is independent of the transport information - I can partition the
return value to encode all the possible network and transport combination and subsequently
parse the network and transport information. I will go ahead and code it up this way.
Regards,
K. Y
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 5/6] Drivers: net: hyperv: Enable send side checksum offload
2014-03-06 11:13 ` [PATCH 5/6] Drivers: net: hyperv: Enable send side " K. Y. Srinivasan
2014-03-06 19:33 ` David Miller
@ 2014-03-09 18:53 ` Ben Hutchings
1 sibling, 0 replies; 30+ messages in thread
From: Ben Hutchings @ 2014-03-09 18:53 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel, davem
[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]
On Thu, 2014-03-06 at 03:13 -0800, K. Y. Srinivasan wrote:
[...]
> @@ -593,8 +658,9 @@ static int netvsc_probe(struct hv_device *dev,
> net->netdev_ops = &device_ops;
>
> /* TODO: Add GSO and Checksum offload */
> - net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG;
> - net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG | NETIF_F_RXCSUM;
> + net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_IP_CSUM;
> + net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG | NETIF_F_RXCSUM |
> + NETIF_F_IP_CSUM;
Missing NETIF_F_IPV6_CSUM?
Ben.
> SET_ETHTOOL_OPS(net, ðtool_ops);
> SET_NETDEV_DEV(net, &dev->device);
--
Ben Hutchings
I say we take off; nuke the site from orbit. It's the only way to be sure.
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
[-- Attachment #2: Type: text/plain, Size: 169 bytes --]
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 6/6] Drivers: net: hyperv: Enable large send offload
2014-03-06 11:13 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O K. Y. Srinivasan
` (3 preceding siblings ...)
2014-03-06 11:13 ` [PATCH 5/6] Drivers: net: hyperv: Enable send side " K. Y. Srinivasan
@ 2014-03-06 11:13 ` K. Y. Srinivasan
2014-03-06 19:34 ` David Miller
2014-03-06 19:29 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O David Miller
5 siblings, 1 reply; 30+ messages in thread
From: K. Y. Srinivasan @ 2014-03-06 11:13 UTC (permalink / raw)
To: davem, netdev, linux-kernel, devel, olaf, apw, jasowang
Enable segmentation offload.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 40 +++++++++++++++++++++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 38 +++++++++++++++++++++++++++++++++---
2 files changed, 74 insertions(+), 4 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index faeb746..ae5a899 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -742,6 +742,10 @@ struct ndis_oject_header {
#define NDIS_OFFLOAD_PARAMETERS_RX_ENABLED_TX_DISABLED 3
#define NDIS_OFFLOAD_PARAMETERS_TX_RX_ENABLED 4
+#define NDIS_TCP_LARGE_SEND_OFFLOAD_V2_TYPE 1
+#define NDIS_TCP_LARGE_SEND_OFFLOAD_IPV4 0
+#define NDIS_TCP_LARGE_SEND_OFFLOAD_IPV6 1
+
/*
* New offload OIDs for NDIS 6
*/
@@ -804,12 +808,48 @@ struct ndis_tcp_ip_checksum_info {
};
};
+struct ndis_tcp_lso_info {
+ union {
+ struct {
+ u32 unused:30;
+ u32 type:1;
+ u32 reserved2:1;
+ } transmit;
+ struct {
+ u32 mss:20;
+ u32 tcp_header_offset:10;
+ u32 type:1;
+ u32 reserved2:1;
+ } lso_v1_transmit;
+ struct {
+ u32 tcp_payload:30;
+ u32 type:1;
+ u32 reserved2:1;
+ } lso_v1_transmit_complete;
+ struct {
+ u32 mss:20;
+ u32 tcp_header_offset:10;
+ u32 type:1;
+ u32 ip_version:1;
+ } lso_v2_transmit;
+ struct {
+ u32 reserved:30;
+ u32 type:1;
+ u32 reserved2:1;
+ } lso_v2_transmit_complete;
+ u32 value;
+ };
+};
+
#define NDIS_VLAN_PPI_SIZE (sizeof(struct rndis_per_packet_info) + \
sizeof(struct ndis_pkt_8021q_info))
#define NDIS_CSUM_PPI_SIZE (sizeof(struct rndis_per_packet_info) + \
sizeof(struct ndis_tcp_ip_checksum_info))
+#define NDIS_LSO_PPI_SIZE (sizeof(struct rndis_per_packet_info) + \
+ sizeof(struct ndis_tcp_lso_info))
+
/* Format of Information buffer passed in a SetRequest for the OID */
/* OID_GEN_RNDIS_CONFIG_PARAMETER. */
struct rndis_config_parameter_info {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index e8159a8..9f1f995 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -301,6 +301,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
bool isvlan;
struct rndis_per_packet_info *ppi;
struct ndis_tcp_ip_checksum_info *csum_info;
+ struct ndis_tcp_lso_info *lso_info;
int hdr_offset;
bool is_v4;
bool is_tcp;
@@ -381,7 +382,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
* GSO packet.
*/
if (skb_is_gso(skb))
- goto do_send;
+ goto do_lso;
rndis_msg_size += NDIS_CSUM_PPI_SIZE;
ppi = init_ppi_data(rndis_msg, NDIS_CSUM_PPI_SIZE,
@@ -401,6 +402,35 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
} else if (is_udp) {
csum_info->transmit.udp_checksum = 1;
}
+ goto do_send;
+
+do_lso:
+ rndis_msg_size += NDIS_LSO_PPI_SIZE;
+ ppi = init_ppi_data(rndis_msg, NDIS_LSO_PPI_SIZE,
+ TCP_LARGESEND_PKTINFO);
+
+ lso_info = (struct ndis_tcp_lso_info *)((ulong)ppi +
+ ppi->ppi_offset);
+
+ lso_info->lso_v2_transmit.type = NDIS_TCP_LARGE_SEND_OFFLOAD_V2_TYPE;
+ if (is_v4) {
+ lso_info->lso_v2_transmit.ip_version =
+ NDIS_TCP_LARGE_SEND_OFFLOAD_IPV4;
+ ip_hdr(skb)->tot_len = 0;
+ ip_hdr(skb)->check = 0;
+ tcp_hdr(skb)->check =
+ ~csum_tcpudp_magic(ip_hdr(skb)->saddr,
+ ip_hdr(skb)->daddr, 0, IPPROTO_TCP, 0);
+ } else {
+ lso_info->lso_v2_transmit.ip_version =
+ NDIS_TCP_LARGE_SEND_OFFLOAD_IPV6;
+ ipv6_hdr(skb)->payload_len = 0;
+ tcp_hdr(skb)->check =
+ ~csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
+ &ipv6_hdr(skb)->daddr, 0, IPPROTO_TCP, 0);
+ }
+ lso_info->lso_v2_transmit.tcp_header_offset = hdr_offset;
+ lso_info->lso_v2_transmit.mss = skb_shinfo(skb)->gso_size;
do_send:
/* Start filling in the page buffers with the rndis hdr */
@@ -657,10 +687,10 @@ static int netvsc_probe(struct hv_device *dev,
net->netdev_ops = &device_ops;
- /* TODO: Add GSO and Checksum offload */
- net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_IP_CSUM;
+ net->hw_features = NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_IP_CSUM |
+ NETIF_F_TSO;
net->features = NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_SG | NETIF_F_RXCSUM |
- NETIF_F_IP_CSUM;
+ NETIF_F_IP_CSUM | NETIF_F_TSO;
SET_ETHTOOL_OPS(net, ðtool_ops);
SET_NETDEV_DEV(net, &dev->device);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 6/6] Drivers: net: hyperv: Enable large send offload
2014-03-06 11:13 ` [PATCH 6/6] Drivers: net: hyperv: Enable large send offload K. Y. Srinivasan
@ 2014-03-06 19:34 ` David Miller
0 siblings, 0 replies; 30+ messages in thread
From: David Miller @ 2014-03-06 19:34 UTC (permalink / raw)
To: kys; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel
From: "K. Y. Srinivasan" <kys@microsoft.com>
Date: Thu, 6 Mar 2014 03:13:10 -0800
> + ppi = init_ppi_data(rndis_msg, NDIS_LSO_PPI_SIZE,
> + TCP_LARGESEND_PKTINFO);
This is not indented properly, "TCP_LARGESEND_PKTINFO);" should start at the
first column after the openning parenthesis of "init_ppi_data(".
> + ~csum_tcpudp_magic(ip_hdr(skb)->saddr,
> + ip_hdr(skb)->daddr, 0, IPPROTO_TCP, 0);
Likewise.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O
2014-03-06 11:13 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O K. Y. Srinivasan
` (4 preceding siblings ...)
2014-03-06 11:13 ` [PATCH 6/6] Drivers: net: hyperv: Enable large send offload K. Y. Srinivasan
@ 2014-03-06 19:29 ` David Miller
2014-03-06 23:28 ` [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration Joe Perches
5 siblings, 1 reply; 30+ messages in thread
From: David Miller @ 2014-03-06 19:29 UTC (permalink / raw)
To: kys; +Cc: olaf, netdev, jasowang, linux-kernel, apw, devel
From: "K. Y. Srinivasan" <kys@microsoft.com>
Date: Thu, 6 Mar 2014 03:13:05 -0800
> @@ -140,21 +140,125 @@ static void netvsc_xmit_completion(void *context)
> dev_kfree_skb_any(skb);
> }
>
> +static u32 fill_pg_buf(struct page *page, u32 offset, u32 len,
> + struct hv_page_buffer *pb)
> +{
> + int j = 0;
> + /*
> + * Deal with compund pages by ignoring unused part
> + * of the page.
> + */
> + page += offset >> PAGE_SHIFT;
> + offset &= ~PAGE_MASK;
Please put an empty line between local variable declarations and
the code.
Please format comments:
/* Like
* this.
*/
> + while (len > 0) {
> + unsigned long bytes;
> + bytes = PAGE_SIZE - offset;
Empty line between local variable declarations and code.
> + /*
> + * The packet is laid out thus:
> + * 1. hdr
> + * 2. skb linear data
> + * 3. skb fragment data
> + */
Fix comment formatting as explained above.
> + if (hdr != NULL) {
> + slots_used += fill_pg_buf(virt_to_page(hdr),
> + offset_in_page(hdr),
> + len, &pb[slots_used]);
> + }
Single statment basic blocks do not need surrounding braces.
> + /*
> + * We will atmost need two pages to describe the rndis
> + * header. We can only transmit MAX_PAGE_BUFFER_COUNT number
> + * of pages in a single packet.
> + */
Comment formatting.
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration
2014-03-06 19:29 ` [PATCH 1/6] Drivers: net: hyperv: Enable scatter gather I/O David Miller
@ 2014-03-06 23:28 ` Joe Perches
2014-03-06 23:35 ` Andrew Morton
2014-03-07 7:54 ` [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration Dan Carpenter
0 siblings, 2 replies; 30+ messages in thread
From: Joe Perches @ 2014-03-06 23:28 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Miller, kys, netdev, linux-kernel, devel, apw
Networking prefers this style, so warn when it's not used.
Networking uses:
void foo(int bar)
{
int baz;
code...
}
not
void foo(int bar)
{
int baz;
code...
}
There are a limited number of false positives when using
macros to declare variables like:
WARNING: networking uses a blank line after declarations
#330: FILE: net/ipv4/inet_hashtables.c:330:
+ int dif = sk->sk_bound_dev_if;
+ INET_ADDR_COOKIE(acookie, saddr, daddr)
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3d3ef2f..a6e3048 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2248,6 +2248,21 @@ sub process {
"networking block comments put the trailing */ on a separate line\n" . $herecurr);
}
+# check for missing blank lines after declarations
+ if ($realfile =~ m@^(drivers/net/|net/)@ &&
+ $prevline =~ /^\+\s+$Declare\s+$Ident/ &&
+ !($prevline =~ /(?:$Compare|$Assignment|$Operators)\s*$/ ||
+ $prevline =~ /(?:\{\s*|\\)$/) && #extended lines
+ $sline =~ /^\+\s+/ && #Not at char 1
+ !($sline =~ /^\+\s+$Declare/ ||
+ $sline =~ /^\+\s+$Ident\s+$Ident/ || #eg: typedef foo
+ $sline =~ /^\+\s+(?:union|struct|enum|typedef)\b/ ||
+ $sline =~ /^\+\s+(?:$|[\{\}\.\#\"\?\:\(])/ ||
+ $sline =~ /^\+\s+\(?\s*(?:$Compare|$Assignment|$Operators)/)) {
+ WARN("SPACING",
+ "networking uses a blank line after declarations\n" . $hereprev);
+ }
+
# check for spaces at the beginning of a line.
# Exceptions:
# 1) within comments
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration
2014-03-06 23:28 ` [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration Joe Perches
@ 2014-03-06 23:35 ` Andrew Morton
2014-03-06 23:42 ` Joe Perches
2014-03-07 7:54 ` [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration Dan Carpenter
1 sibling, 1 reply; 30+ messages in thread
From: Andrew Morton @ 2014-03-06 23:35 UTC (permalink / raw)
To: Joe Perches; +Cc: David Miller, kys, netdev, linux-kernel, devel, apw
On Thu, 06 Mar 2014 15:28:40 -0800 Joe Perches <joe@perches.com> wrote:
> Networking prefers this style, so warn when it's not used.
>
> Networking uses:
>
> void foo(int bar)
> {
> int baz;
>
> code...
> }
>
> not
>
> void foo(int bar)
> {
> int baz;
> code...
> }
>
> There are a limited number of false positives when using
> macros to declare variables like:
>
> WARNING: networking uses a blank line after declarations
> #330: FILE: net/ipv4/inet_hashtables.c:330:
> + int dif = sk->sk_bound_dev_if;
> + INET_ADDR_COOKIE(acookie, saddr, daddr)
um wait wut wot.
*All* kernel code uses blank line between end-of-locals and
start-of-code. Or if it doesn't it should, thwap.
Why are we special-casing net/?
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration
2014-03-06 23:35 ` Andrew Morton
@ 2014-03-06 23:42 ` Joe Perches
2014-03-06 23:55 ` Andrew Morton
0 siblings, 1 reply; 30+ messages in thread
From: Joe Perches @ 2014-03-06 23:42 UTC (permalink / raw)
To: Andrew Morton; +Cc: David Miller, kys, netdev, linux-kernel, devel, apw
On Thu, 2014-03-06 at 15:35 -0800, Andrew Morton wrote:
> On Thu, 06 Mar 2014 15:28:40 -0800 Joe Perches <joe@perches.com> wrote:
> > Networking prefers this style, so warn when it's not used.
> > void foo(int bar)
> > {
> > int baz;
> >
> > code...
> > }
> >
> > not
> >
> > void foo(int bar)
> > {
> > int baz;
> > code...
> > }
> >
> > There are a limited number of false positives when using
> > macros to declare variables like:
> >
> > WARNING: networking uses a blank line after declarations
> > #330: FILE: net/ipv4/inet_hashtables.c:330:
> > + int dif = sk->sk_bound_dev_if;
> > + INET_ADDR_COOKIE(acookie, saddr, daddr)
>
> um wait wut wot.
>
> *All* kernel code uses blank line between end-of-locals and
> start-of-code. Or if it doesn't it should, thwap.
> Why are we special-casing net/?
It's easy enough to remove the path test, but it's
not in CodingStyle and David seems to be the one
that makes the effort to correct people about it.
I don't care one way or another.
I'm just trying to get fewer rejections for people
that use checkpatch.
cheers, Joe
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration
2014-03-06 23:42 ` Joe Perches
@ 2014-03-06 23:55 ` Andrew Morton
2014-03-07 0:11 ` [PATCH] checkpatch: Always warn on missing blank line after variable declaration block Joe Perches
0 siblings, 1 reply; 30+ messages in thread
From: Andrew Morton @ 2014-03-06 23:55 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, linux-kernel, apw, devel, David Miller
On Thu, 06 Mar 2014 15:42:30 -0800 Joe Perches <joe@perches.com> wrote:
> On Thu, 2014-03-06 at 15:35 -0800, Andrew Morton wrote:
> > On Thu, 06 Mar 2014 15:28:40 -0800 Joe Perches <joe@perches.com> wrote:
> > > Networking prefers this style, so warn when it's not used.
> > > void foo(int bar)
> > > {
> > > int baz;
> > >
> > > code...
> > > }
> > >
> > > not
> > >
> > > void foo(int bar)
> > > {
> > > int baz;
> > > code...
> > > }
> > >
> > > There are a limited number of false positives when using
> > > macros to declare variables like:
> > >
> > > WARNING: networking uses a blank line after declarations
> > > #330: FILE: net/ipv4/inet_hashtables.c:330:
> > > + int dif = sk->sk_bound_dev_if;
> > > + INET_ADDR_COOKIE(acookie, saddr, daddr)
> >
> > um wait wut wot.
> >
> > *All* kernel code uses blank line between end-of-locals and
> > start-of-code. Or if it doesn't it should, thwap.
> > Why are we special-casing net/?
>
> It's easy enough to remove the path test, but it's
> not in CodingStyle and David seems to be the one
> that makes the effort to correct people about it.
>
> I don't care one way or another.
>
> I'm just trying to get fewer rejections for people
> that use checkpatch.
mutter.
OK, let's do this for now. Could you please cook up a followon patch
which makes this kernel-wide? I'll play with that for a while then I'll
decide how much I feel like irritating people.
^ permalink raw reply [flat|nested] 30+ messages in thread* [PATCH] checkpatch: Always warn on missing blank line after variable declaration block
2014-03-06 23:55 ` Andrew Morton
@ 2014-03-07 0:11 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2014-03-07 0:11 UTC (permalink / raw)
To: Andrew Morton; +Cc: netdev, linux-kernel, apw, devel, David Miller
Make the test system wide, modify the message too.
Signed-off-by: Joe Perches <joe@perches.com>
---
> OK, let's do this for now. Could you please cook up a followon patch
> which makes this kernel-wide? I'll play with that for a while then I'll
> decide how much I feel like irritating people.
scripts/checkpatch.pl | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a6e3048..37a94f1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2249,8 +2249,7 @@ sub process {
}
# check for missing blank lines after declarations
- if ($realfile =~ m@^(drivers/net/|net/)@ &&
- $prevline =~ /^\+\s+$Declare\s+$Ident/ &&
+ if ($prevline =~ /^\+\s+$Declare\s+$Ident/ &&
!($prevline =~ /(?:$Compare|$Assignment|$Operators)\s*$/ ||
$prevline =~ /(?:\{\s*|\\)$/) && #extended lines
$sline =~ /^\+\s+/ && #Not at char 1
@@ -2260,7 +2259,7 @@ sub process {
$sline =~ /^\+\s+(?:$|[\{\}\.\#\"\?\:\(])/ ||
$sline =~ /^\+\s+\(?\s*(?:$Compare|$Assignment|$Operators)/)) {
WARN("SPACING",
- "networking uses a blank line after declarations\n" . $hereprev);
+ "Missing a blank line after declarations\n" . $hereprev);
}
# check for spaces at the beginning of a line.
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration
2014-03-06 23:28 ` [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration Joe Perches
2014-03-06 23:35 ` Andrew Morton
@ 2014-03-07 7:54 ` Dan Carpenter
2014-03-07 9:30 ` Joe Perches
1 sibling, 1 reply; 30+ messages in thread
From: Dan Carpenter @ 2014-03-07 7:54 UTC (permalink / raw)
To: Joe Perches; +Cc: netdev, linux-kernel, apw, devel, Andrew Morton, David Miller
How many warnings does this generate does this generate when you run it
across the whole tree?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration
2014-03-07 7:54 ` [PATCH] checkpatch: net and drivers/net: Warn on missing blank line after variable declaration Dan Carpenter
@ 2014-03-07 9:30 ` Joe Perches
2014-03-10 16:02 ` Treewide frequency of various checkpatch messages Joe Perches
0 siblings, 1 reply; 30+ messages in thread
From: Joe Perches @ 2014-03-07 9:30 UTC (permalink / raw)
To: Dan Carpenter
Cc: netdev, linux-kernel, apw, devel, Andrew Morton, David Miller
On Fri, 2014-03-07 at 10:54 +0300, Dan Carpenter wrote:
> How many warnings does this generate does this generate when you run it
> across the whole tree?
A lot.
Check back with me after the week or so
it'll take to run on this little netboook.
Try this with both patches applied if you want
to know yourself...
$ git ls-files | grep "\.[ch]$"| \
xargs ./scripts/checkpatch.pl -f --strict --types=spacing --emacs --terse | \
grep "Missing a blank"
^ permalink raw reply [flat|nested] 30+ messages in thread
* Treewide frequency of various checkpatch messages
2014-03-07 9:30 ` Joe Perches
@ 2014-03-10 16:02 ` Joe Perches
2014-03-10 16:50 ` Greg KH
0 siblings, 1 reply; 30+ messages in thread
From: Joe Perches @ 2014-03-10 16:02 UTC (permalink / raw)
To: Dan Carpenter
Cc: netdev, linux-kernel, apw, devel, Andrew Morton, David Miller
On Fri, 2014-03-07 at 01:30 -0800, Joe Perches wrote:
> On Fri, 2014-03-07 at 10:54 +0300, Dan Carpenter wrote:
(a question about a new message warning of a missing
blank line between variable declaration blocks and
code in a function)
> > How many warnings does this generate does this generate when you run it
> > across the whole tree?
> A lot.
Turns out it's 20,210 and it's the 14th
most common checkpatch message type.
14 20210 WARNING:SPACING: Missing a blank line after declarations
> Check back with me after the week or so
> it'll take to run on this little netboook.
I ran a variant of this over the weekend
over next-20140306. It just finished.
$ git ls-files | grep "\.[ch]$"| \
while read file ; do \
./scripts/checkpatch.pl -f --strict --show-types --no-summary --emacs --terse $file ; \
done | \
tee -a checkpatch.all
Here are the top 100 types of messages:
(for 2377922 total LOC)
1 241524 WARNING:LONG_LINE: line over 80 characters
2 238492 ERROR:SPACING: space required after that ',' (ctx:VxV)
3 152039 WARNING:LEADING_SPACE: please, no spaces at the start of a line
4 117364 CHECK:CAMELCASE: Avoid CamelCase
5 67761 CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
6 53673 ERROR:CODE_INDENT: code indent should use tabs where possible
7 30981 ERROR:TRAILING_WHITESPACE: trailing whitespace
8 30443 ERROR:C99_COMMENTS: do not use C99 // comments
9 26928 WARNING:SPACE_BEFORE_TAB: please, no space before tabs
10 25546 CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
11 25384 WARNING:SPACING: space prohibited between function name and open parenthesis '('
12 25209 WARNING:SPLIT_STRING: quoted string split across lines
13 21410 WARNING:NETWORKING_BLOCK_COMMENT_STYLE: networking block comments don't use an empty /* line, use /* Comment...
14 20210 WARNING:SPACING: Missing a blank line after declarations
15 19881 CHECK:SPACING: No space is necessary after a cast
16 18045 ERROR:SPACING: space prohibited after that open parenthesis '('
17 17009 ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parenthesis
18 16674 ERROR:SPACING: space prohibited before that close parenthesis ')'
19 14364 ERROR:SPACING: space required before the open parenthesis '('
20 13331 ERROR:TRAILING_STATEMENTS: trailing statements should be on next line
21 12619 CHECK:BRACES: braces {} should be used on all arms of this statement
22 12605 CHECK:FSF_MAILING_ADDRESS: Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The
23 11194 ERROR:SPACING: spaces prohibited around that ':' (ctx:WxW)
24 10192 ERROR:OPEN_BRACE: that open brace { should be on the previous line
25 9941 WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: netdev]_err([subsystem]dev, ... then dev_err(dev, ... then pr_err(... to printk(KERN_ERR ...
26 9355 CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
27 9215 WARNING:NEW_TYPEDEFS: do not add new typedefs
28 8733 ERROR:POINTER_LOCATION: "foo * bar" should be "foo *bar"
29 8129 WARNING:PRINTK_WITHOUT_KERN_LEVEL: printk() should include KERN_ facility level
30 7676 WARNING:BRACES: braces {} are not necessary for single statement blocks
31 7339 CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the previous line
32 7247 ERROR:ASSIGN_IN_IF: do not use assignment in if condition
33 6918 WARNING:NETWORKING_BLOCK_COMMENT_STYLE: networking block comments put the trailing */ on a separate line
34 6750 ERROR:SPACING: spaces required around that '=' (ctx:VxV)
35 6446 WARNING:SPACING: space prohibited before semicolon
36 6289 WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: netdev]_info([subsystem]dev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
37 6276 CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
38 6204 WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
39 5921 CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
40 5262 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE: unnecessary whitespace before a quoted newline
41 5050 WARNING:NETWORKING_BLOCK_COMMENT_STYLE: networking block comments start with * on subsequent lines
42 4740 WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: netdev]_warn([subsystem]dev, ... then dev_warn(dev, ... then pr_warn(... to printk(KERN_WARNING ...
43 4409 WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: netdev]_dbg([subsystem]dev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
44 4248 ERROR:SPACING: space required after that close brace '}'
45 4237 ERROR:POINTER_LOCATION: "foo* bar" should be "foo *bar"
46 4224 WARNING:EXPORT_SYMBOL: EXPORT_SYMBOL(foo); should immediately follow its function/variable
47 4164 ERROR:RETURN_PARENTHESES: return is not a function, parentheses are not required
48 3734 CHECK:ALLOC_SIZEOF_STRUCT: Prefer
49 3651 CHECK:USLEEP_RANGE: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt
50 3597 WARNING:AVOID_EXTERNS: externs should be avoided in .c files
51 3515 WARNING:SPACING: Unnecessary space before function pointer arguments
52 3241 ERROR:ELSE_AFTER_BRACE: else should follow close brace '}'
53 2995 WARNING:MEMORY_BARRIER: memory barrier without comment
54 2713 ERROR:SPACING: space prohibited before that close square bracket ']'
55 2337 ERROR:POINTER_LOCATION: "(foo*)" should be "(foo *)"
56 2329 CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without comment
57 2298 WARNING:PREFER_PACKED: __packed is preferred over __attribute__((packed))
58 2180 ERROR:OPEN_BRACE: open brace '{' following struct go on the same line
59 2032 CHECK:REDUNDANT_CODE: if this code is redundant consider removing it
60 2017 ERROR:SPACING: space prohibited after that open square bracket '['
61 1945 WARNING:BRACES: braces {} are not necessary for any arm of this statement
62 1898 WARNING:MSLEEP: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.txt
63 1884 WARNING:MISSING_BREAK: Possible switch case/default not preceeded by break or fallthrough comment
64 1869 WARNING:CONSTANT_CONVERSION: __constant_cpu_to_le32 should be cpu_to_le32
65 1855 WARNING:PREFER_ETHER_ADDR_COPY: Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)
66 1800 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 12)
67 1779 ERROR:SPACING: space required after that ',' (ctx:VxO)
68 1773 ERROR:BRACKET_SPACE: space prohibited before open square bracket '['
69 1664 WARNING:INDENTED_LABEL: labels should not be indented
70 1620 ERROR:SPACING: need consistent spacing around '%' (ctx:WxV)
71 1594 CHECK:SPACING: spaces required around that ':' (ctx:VxV)
72 1519 ERROR:SPACING: space required before the open brace '{'
73 1414 WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be static const char * const
74 1392 ERROR:SPACING: space required after that ';' (ctx:VxV)
75 1376 WARNING:PREFER_SEQ_PUTS: Prefer seq_puts to seq_printf
76 1363 WARNING:DEEP_INDENTATION: Too many leading tabs - consider code refactoring
77 1328 CHECK:UNCOMMENTED_DEFINITION: struct mutex definition without comment
78 1316 ERROR:INITIALISED_STATIC: do not initialise statics to 0 or NULL
79 1287 WARNING:LINE_CONTINUATIONS: Avoid unnecessary line continuations
80 1239 ERROR:SPACING: spaces required around that '<' (ctx:VxV)
81 1181 ERROR:SWITCH_CASE_INDENT_LEVEL: switch and case should be at the same indent
82 1129 ERROR:MULTISTATEMENT_MACRO_USE_DO_WHILE: Macros with multiple statements should be enclosed in a do - while loop
83 1036 WARNING:PREFER_PR_LEVEL: Prefer pr_warn(... to pr_warning(...
84 942 ERROR:SPACING: spaces required around that '==' (ctx:VxV)
85 941 WARNING:INCLUDE_LINUX: Use #include <linux/io.h> instead of <asm/io.h>
86 906 WARNING:CONSIDER_KSTRTO: simple_strtoul is obsolete, use kstrtoul instead
87 836 ERROR:SPACING: spaces prohibited around that ':' (ctx:WxV)
88 805 WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: netdev]_notice([subsystem]dev, ... then dev_notice(dev, ... then pr_notice(... to printk(KERN_NOTICE ..
89 803 ERROR:SPACING: space prohibited after that '!' (ctx:BxW)
90 787 ERROR:SPACING: space required before that '-' (ctx:OxV)
91 776 ERROR:SPACING: spaces required around that ':' (ctx:VxW)
92 765 WARNING:INCLUDE_LINUX: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>
93 759 CHECK:INVALID_UTF8: Invalid UTF-8, patch and commit message should be encoded in UTF-8
94 754 ERROR:SPACING: space required before that '&' (ctx:OxV)
95 723 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO: Single statement macros should not use a do {} while (0) loop
96 710 CHECK:ARCH_INCLUDE_LINUX: Consider using #include <linux/io.h> instead of <asm/io.h>
97 707 ERROR:SPACING: spaces required around that '=' (ctx:VxW)
98 691 WARNING:CVS_KEYWORD: CVS style keyword markers, these will _not_ be updated
99 644 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (2, 4)
100 641 ERROR:OPEN_BRACE: open brace '{' following function declarations go on the next line
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Treewide frequency of various checkpatch messages
2014-03-10 16:02 ` Treewide frequency of various checkpatch messages Joe Perches
@ 2014-03-10 16:50 ` Greg KH
2014-03-10 18:48 ` Joe Perches
0 siblings, 1 reply; 30+ messages in thread
From: Greg KH @ 2014-03-10 16:50 UTC (permalink / raw)
To: Joe Perches
Cc: netdev, linux-kernel, apw, devel, Andrew Morton, David Miller,
Dan Carpenter
On Mon, Mar 10, 2014 at 09:02:26AM -0700, Joe Perches wrote:
> On Fri, 2014-03-07 at 01:30 -0800, Joe Perches wrote:
> > On Fri, 2014-03-07 at 10:54 +0300, Dan Carpenter wrote:
> (a question about a new message warning of a missing
> blank line between variable declaration blocks and
> code in a function)
> > > How many warnings does this generate does this generate when you run it
> > > across the whole tree?
> > A lot.
>
> Turns out it's 20,210 and it's the 14th
> most common checkpatch message type.
>
> 14 20210 WARNING:SPACING: Missing a blank line after declarations
I think it's still worthwhile to clean up.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Treewide frequency of various checkpatch messages
2014-03-10 16:50 ` Greg KH
@ 2014-03-10 18:48 ` Joe Perches
2014-03-10 19:33 ` Greg KH
0 siblings, 1 reply; 30+ messages in thread
From: Joe Perches @ 2014-03-10 18:48 UTC (permalink / raw)
To: Greg KH
Cc: Dan Carpenter, netdev, linux-kernel, apw, devel, Andrew Morton,
David Miller
On Mon, 2014-03-10 at 09:50 -0700, Greg KH wrote:
> On Mon, Mar 10, 2014 at 09:02:26AM -0700, Joe Perches wrote:
> > On Fri, 2014-03-07 at 01:30 -0800, Joe Perches wrote:
> > > On Fri, 2014-03-07 at 10:54 +0300, Dan Carpenter wrote:
> > (a question about a new message warning of a missing
> > blank line between variable declaration blocks and
> > code in a function)
> > > > How many warnings does this generate does this generate when you run it
> > > > across the whole tree?
> > > A lot.
> >
> > Turns out it's 20,210 and it's the 14th
> > most common checkpatch message type.
> >
> > 14 20210 WARNING:SPACING: Missing a blank line after declarations
>
> I think it's still worthwhile to clean up.
Maybe.
Luckily, <smile> I don't have to deal with the
patches that would be generated by this message.
Some people are going to view patches for this as
useless noise.
Couple of things:
It's kind of interesting how the messages vary by
subsystem. Let me know if you want any breakdowns.
And there are a small number of false positives for
this "Missing a blank line" test with declarations
like:
typedef *foo;
DECLARE_BITMAP(foo);
__DECL_REG(foo);
LIST_HEAD(foo);
So there could be a minor improvement to the test.
I looked at some of the results using:
This sort of match stands out a bit:
---> arch/tile/lib/spinlock_32.c:68:
{
u32 iterations = 0;
while (arch_spin_is_locked(lock))
delay_backoff(iterations++);
}
Instances like this may be fine, but adding blank
lines to very short functions with a single
declaration just adds to the overall line count.
I've no strong opinion of the need to write code
like:
{
u32 iterations = 0;
while (arch_spin_is_locked(lock))
delay_backoff(iterations++);
}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Treewide frequency of various checkpatch messages
2014-03-10 18:48 ` Joe Perches
@ 2014-03-10 19:33 ` Greg KH
2014-03-10 20:11 ` Joe Perches
0 siblings, 1 reply; 30+ messages in thread
From: Greg KH @ 2014-03-10 19:33 UTC (permalink / raw)
To: Joe Perches
Cc: netdev, linux-kernel, apw, devel, Andrew Morton, David Miller,
Dan Carpenter
On Mon, Mar 10, 2014 at 11:48:50AM -0700, Joe Perches wrote:
> On Mon, 2014-03-10 at 09:50 -0700, Greg KH wrote:
> > On Mon, Mar 10, 2014 at 09:02:26AM -0700, Joe Perches wrote:
> > > On Fri, 2014-03-07 at 01:30 -0800, Joe Perches wrote:
> > > > On Fri, 2014-03-07 at 10:54 +0300, Dan Carpenter wrote:
> > > (a question about a new message warning of a missing
> > > blank line between variable declaration blocks and
> > > code in a function)
> > > > > How many warnings does this generate does this generate when you run it
> > > > > across the whole tree?
> > > > A lot.
> > >
> > > Turns out it's 20,210 and it's the 14th
> > > most common checkpatch message type.
> > >
> > > 14 20210 WARNING:SPACING: Missing a blank line after declarations
> >
> > I think it's still worthwhile to clean up.
>
> Maybe.
>
> Luckily, <smile> I don't have to deal with the
> patches that would be generated by this message.
>
> Some people are going to view patches for this as
> useless noise.
That's true for all checkpatch cleanups :)
> Couple of things:
>
> It's kind of interesting how the messages vary by
> subsystem. Let me know if you want any breakdowns.
>
> And there are a small number of false positives for
> this "Missing a blank line" test with declarations
> like:
>
> typedef *foo;
> DECLARE_BITMAP(foo);
> __DECL_REG(foo);
> LIST_HEAD(foo);
>
> So there could be a minor improvement to the test.
>
> I looked at some of the results using:
>
> This sort of match stands out a bit:
>
> ---> arch/tile/lib/spinlock_32.c:68:
> {
> u32 iterations = 0;
> while (arch_spin_is_locked(lock))
> delay_backoff(iterations++);
> }
>
> Instances like this may be fine, but adding blank
> lines to very short functions with a single
> declaration just adds to the overall line count.
>
> I've no strong opinion of the need to write code
> like:
>
> {
> u32 iterations = 0;
>
> while (arch_spin_is_locked(lock))
> delay_backoff(iterations++);
> }
I wonder if there's a way to "count" the size of the function, and only
complain if it's longer than 4-5 lines long?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Treewide frequency of various checkpatch messages
2014-03-10 19:33 ` Greg KH
@ 2014-03-10 20:11 ` Joe Perches
0 siblings, 0 replies; 30+ messages in thread
From: Joe Perches @ 2014-03-10 20:11 UTC (permalink / raw)
To: Greg KH
Cc: netdev, linux-kernel, apw, devel, Andrew Morton, David Miller,
Dan Carpenter
On Mon, 2014-03-10 at 12:33 -0700, Greg KH wrote:
> On Mon, Mar 10, 2014 at 11:48:50AM -0700, Joe Perches wrote:
[]
> > I've no strong opinion of the need to write code
> > like:
> >
> > {
> > u32 iterations = 0;
> >
> > while (arch_spin_is_locked(lock))
> > delay_backoff(iterations++);
> > }
>
> I wonder if there's a way to "count" the size of the function, and only
> complain if it's longer than 4-5 lines long?
No, there's isn't really.
checkpatch works on patch hunks.
The function scope isn't necessarily visible.
^ permalink raw reply [flat|nested] 30+ messages in thread