* [PATCH v4 3/4] dt-bindings: can: m_can: Document new can transceiver binding
From: Franklin S Cooper Jr @ 2017-08-10 0:59 UTC (permalink / raw)
To: linux-kernel, devicetree, netdev, linux-can, wg, mkl, robh+dt,
quentin.schulz, dev.kurt, andrew, sergei.shtylyov, socketcan
Cc: Franklin S Cooper Jr
In-Reply-To: <20170810005916.27163-1-fcooper@ti.com>
Add information regarding can-transceiver binding. This is especially
important for MCAN since the IP allows CAN FD mode to run significantly
faster than what most transceivers are capable of.
Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Drop unit address.
Switch from using fixed-transceiver to can-transceiver
Indicate that can-transceiver is an optional subnode not a property.
Documentation/devicetree/bindings/net/can/m_can.txt | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index 9e33177..ee90aac 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -43,6 +43,11 @@ Required properties:
Please refer to 2.4.1 Message RAM Configuration in
Bosch M_CAN user manual for details.
+Optional Subnode:
+- can-transceiver : Can-transceiver subnode describing maximum speed
+ that can be used for CAN/CAN-FD modes. See
+ Documentation/devicetree/bindings/net/can/can-transceiver.txt
+ for details.
Example:
SoC dtsi:
m_can1: can@020e8000 {
@@ -64,4 +69,8 @@ Board dts:
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_m_can1>;
status = "enabled";
+
+ can-transceiver@ {
+ max-bitrate = <5000000>;
+ };
};
--
2.9.4.dirty
^ permalink raw reply related
* [PATCH v4 2/4] dt-bindings: can: can-transceiver: Document new binding
From: Franklin S Cooper Jr @ 2017-08-10 0:59 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-can-u79uwXL29TY76Z2rM5mHXA, wg-5Yr1BZd7O62+XT7JhA+gdA,
mkl-bIcnvbaLZ9MEGnE8C9+IrQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
dev.kurt-yI9piX4KPfawT/RRk36CISFp6vIno51x, andrew-g2DYL2Zd6BY,
sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8,
socketcan-fJ+pQTUTwRTk1uMJSBkQmQ
Cc: Franklin S Cooper Jr
In-Reply-To: <20170810005916.27163-1-fcooper-l0cyMroinI0@public.gmane.org>
Add documentation to describe usage of the new can-transceiver binding.
This new binding is applicable for any CAN device therefore it exists as
its own document.
Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
---
Version 4 changes:
Drop unit address.
Switch from using fixed-transceiver to can-transceiver
.../bindings/net/can/can-transceiver.txt | 24 ++++++++++++++++++++++
1 file changed, 24 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt
diff --git a/Documentation/devicetree/bindings/net/can/can-transceiver.txt b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
new file mode 100644
index 0000000..2c31dc0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/can-transceiver.txt
@@ -0,0 +1,24 @@
+Generic CAN transceiver Device Tree binding
+------------------------------
+
+CAN transceiver typically limits the max speed in standard CAN and CAN FD
+modes. Typically these limitations are static and the transceivers themselves
+provide no way to detect this limitation at runtime. For this situation,
+the "can-transceiver" node can be used.
+
+Required Properties:
+ max-bitrate: a positive non 0 value that determines the max
+ speed that CAN/CAN-FD can run. Any other value
+ will be ignored.
+
+Examples:
+
+Based on Texas Instrument's TCAN1042HGV CAN Transceiver
+
+m_can0 {
+ ....
+ can-transceiver@ {
+ max-bitrate = <5000000>;
+ };
+ ...
+};
--
2.9.4.dirty
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v4 1/4] can: dev: Add support for limiting configured bitrate
From: Franklin S Cooper Jr @ 2017-08-10 0:59 UTC (permalink / raw)
To: linux-kernel, devicetree, netdev, linux-can, wg, mkl, robh+dt,
quentin.schulz, dev.kurt, andrew, sergei.shtylyov, socketcan
Cc: Franklin S Cooper Jr
In-Reply-To: <20170810005916.27163-1-fcooper@ti.com>
Various CAN or CAN-FD IP may be able to run at a faster rate than
what the transceiver the CAN node is connected to. This can lead to
unexpected errors. However, CAN transceivers typically have fixed
limitations and provide no means to discover these limitations at
runtime. Therefore, add support for a can-transceiver node that
can be reused by other CAN peripheral drivers to determine for both
CAN and CAN-FD what the max bitrate that can be used. If the user
tries to configure CAN to pass these maximum bitrates it will throw
an error.
Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
Version 4 changes:
Used can-transceiver instead of fixed-transceiver.
drivers/net/can/dev.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/can/dev.h | 5 +++++
2 files changed, 55 insertions(+)
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 365a8cc..372108f 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -27,6 +27,7 @@
#include <linux/can/skb.h>
#include <linux/can/netlink.h>
#include <linux/can/led.h>
+#include <linux/of.h>
#include <net/rtnetlink.h>
#define MOD_DESC "CAN device driver interface"
@@ -814,6 +815,39 @@ int open_candev(struct net_device *dev)
}
EXPORT_SYMBOL_GPL(open_candev);
+#ifdef CONFIG_OF
+/*
+ * Common function that can be used to understand the limitation of
+ * a transceiver when it provides no means to determine these limitations
+ * at runtime.
+ */
+void of_can_transceiver(struct net_device *dev)
+{
+ struct device_node *dn;
+ struct can_priv *priv = netdev_priv(dev);
+ struct device_node *np;
+ unsigned int max_bitrate;
+ int ret;
+
+ np = dev->dev.parent->of_node;
+
+ dn = of_get_child_by_name(np, "can-transceiver");
+ if (!dn)
+ return;
+
+ max_bitrate = 0;
+ ret = of_property_read_u32(dn, "max-bitrate", &max_bitrate);
+
+ if (max_bitrate > 0) {
+ priv->max_bitrate = max_bitrate;
+ priv->is_bitrate_limited = true;
+ } else if (ret != -EINVAL) {
+ netdev_warn(dev, "Invalid value for transceiver max bitrate\n");
+ }
+}
+EXPORT_SYMBOL(of_can_transceiver);
+#endif
+
/*
* Common close function for cleanup before the device gets closed.
*
@@ -913,6 +947,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->is_bitrate_limited &&
+ bt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->bittiming, &bt, sizeof(bt));
if (priv->do_set_bittiming) {
@@ -997,6 +1039,14 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
priv->data_bitrate_const_cnt);
if (err)
return err;
+
+ if (priv->is_bitrate_limited &&
+ dbt.bitrate > priv->max_bitrate) {
+ netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
+ priv->max_bitrate);
+ return -EINVAL;
+ }
+
memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
if (priv->do_set_data_bittiming) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 141b05a..5519f59 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -47,6 +47,9 @@ struct can_priv {
unsigned int data_bitrate_const_cnt;
struct can_clock clock;
+ unsigned int max_bitrate;
+ bool is_bitrate_limited;
+
enum can_state state;
/* CAN controller features - see include/uapi/linux/can/netlink.h */
@@ -165,6 +168,8 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
void can_free_echo_skb(struct net_device *dev, unsigned int idx);
+void of_can_transceiver(struct net_device *dev);
+
struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
struct sk_buff *alloc_canfd_skb(struct net_device *dev,
struct canfd_frame **cfd);
--
2.9.4.dirty
^ permalink raw reply related
* [PATCH v4 0/4] can: Support transceiver based bit rate limit
From: Franklin S Cooper Jr @ 2017-08-10 0:59 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-can-u79uwXL29TY76Z2rM5mHXA, wg-5Yr1BZd7O62+XT7JhA+gdA,
mkl-bIcnvbaLZ9MEGnE8C9+IrQ, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
quentin.schulz-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
dev.kurt-yI9piX4KPfawT/RRk36CISFp6vIno51x, andrew-g2DYL2Zd6BY,
sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8,
socketcan-fJ+pQTUTwRTk1uMJSBkQmQ
Cc: Franklin S Cooper Jr
Add a new generic binding that CAN drivers can be used to specify the max
bit rate supported by a transceiver. This is useful since in some instances
since the maximum speeds may be limited by the transceiver used. However,
transceivers may not provide a means to determine this limitation at
runtime. Therefore, create a new binding that mimics "fixed-link" that
allows a user to hardcode the max speeds that can be used.
Also add support for this new binding in the MCAN driver.
Note this is an optional subnode so even if a driver adds support for
parsing can-transceiver the user does not have to define it in their
device tree.
Version 4 changes:
Switch from fixed-transceiver to can-transceiver
Drop unit address that snuck back in again.
Indicate that can-transceiver is a subnode and not a property in
documentation
Version 3 changes:
Switch from having two "max bitrates" to one universal bitrate.
Version 2 changes:
Rename function
Define proper variable default
Drop unit address
Move check to changelink function
Reword commit message
Reword documentation
Franklin S Cooper Jr (4):
can: dev: Add support for limiting configured bitrate
dt-bindings: can: can-transceiver: Document new binding
dt-bindings: can: m_can: Document new can transceiver binding
can: m_can: Add call to of_can_transceiver
.../bindings/net/can/can-transceiver.txt | 24 +++++++++++
.../devicetree/bindings/net/can/m_can.txt | 9 ++++
drivers/net/can/dev.c | 50 ++++++++++++++++++++++
drivers/net/can/m_can/m_can.c | 2 +
include/linux/can/dev.h | 5 +++
5 files changed, 90 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/can/can-transceiver.txt
--
2.9.4.dirty
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482
From: David Miller @ 2017-08-10 0:52 UTC (permalink / raw)
To: rao.shoaib; +Cc: codesoldier1, hkchu, ycheng, kuznet, netdev
In-Reply-To: <19024bb3-c06b-d004-5527-e4c54af66003@oracle.com>
From: Rao Shoaib <rao.shoaib@oracle.com>
Date: Wed, 9 Aug 2017 17:47:57 -0700
>
>
> On 08/09/2017 05:30 PM, David Miller wrote:
>> From: Joe Smith <codesoldier1@gmail.com>
>> Date: Wed, 9 Aug 2017 17:20:32 -0700
>>
>>> Making Linux conform to standards and behavior that is logical seems
>>> like a good enough reason.
>> That's an awesome attitude to have when we're implementing something
>> new and don't have the facility already.
>>
>> But when we have something already the only important consideration is
>> not breaking existing apps which rely on that behavior.
>>
>> That is much, much, more important than standards compliance.
>>
>> If users are confused, just fix the documentation.
> David,
>
> If it was just confusion than sure fixing the documentation is
> fine. What if the logic is incorrect, does not conform to the standard
> that is says it is implementing and easy to fix with little or no risk
> of breakage.
>
> The proposed patch changes a feature that no one uses. It also imposes
> the relation ship between keepalive and timeout values that is
> required by the RFC and make sense.
>
> You are the final authority, if you say we should just fix the
> documentation than that is fine.
I want to hear more about what hkchu and ycheng have to say about
this.
^ permalink raw reply
* Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482
From: Rao Shoaib @ 2017-08-10 0:47 UTC (permalink / raw)
To: David Miller, codesoldier1; +Cc: hkchu, ycheng, kuznet, netdev
In-Reply-To: <20170809.173032.660035274684914457.davem@davemloft.net>
On 08/09/2017 05:30 PM, David Miller wrote:
> From: Joe Smith <codesoldier1@gmail.com>
> Date: Wed, 9 Aug 2017 17:20:32 -0700
>
>> Making Linux conform to standards and behavior that is logical seems
>> like a good enough reason.
> That's an awesome attitude to have when we're implementing something
> new and don't have the facility already.
>
> But when we have something already the only important consideration is
> not breaking existing apps which rely on that behavior.
>
> That is much, much, more important than standards compliance.
>
> If users are confused, just fix the documentation.
David,
If it was just confusion than sure fixing the documentation is fine.
What if the logic is incorrect, does not conform to the standard that is
says it is implementing and easy to fix with little or no risk of breakage.
The proposed patch changes a feature that no one uses. It also imposes
the relation ship between keepalive and timeout values that is required
by the RFC and make sense.
You are the final authority, if you say we should just fix the
documentation than that is fine.
Shoaib
^ permalink raw reply
* [PATCH net-next 10/10] netvsc: keep track of some non-fatal overload conditions
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
Add ethtool statistics for case where send chimmeny buffer is
exhausted and driver has to fall back to doing scatter/gather
send. Also, add statistic for case where ring buffer is full and
receive completions are delayed.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 2 ++
drivers/net/hyperv/netvsc.c | 19 +++++++++++++------
drivers/net/hyperv/netvsc_drv.c | 2 ++
3 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 4ccfc335a356..345786321879 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -680,6 +680,8 @@ struct netvsc_ethtool_stats {
unsigned long tx_no_space;
unsigned long tx_too_big;
unsigned long tx_busy;
+ unsigned long tx_send_full;
+ unsigned long rx_comp_busy;
};
struct netvsc_vf_pcpu_stats {
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 5b3a2b8fa77e..938c9f3d2ea6 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -882,7 +882,9 @@ int netvsc_send(struct net_device_context *ndev_ctx,
} else if (pktlen + net_device->pkt_align <
net_device->send_section_size) {
section_index = netvsc_get_next_send_section(net_device);
- if (section_index != NETVSC_INVALID_INDEX) {
+ if (unlikely(section_index == NETVSC_INVALID_INDEX)) {
+ ++ndev_ctx->eth_stats.tx_send_full;
+ } else {
move_pkt_msd(&msd_send, &msd_skb, msdp);
msd_len = 0;
}
@@ -948,9 +950,10 @@ int netvsc_send(struct net_device_context *ndev_ctx,
}
/* Send pending recv completions */
-static int send_recv_completions(struct netvsc_channel *nvchan)
+static int send_recv_completions(struct net_device *ndev,
+ struct netvsc_device *nvdev,
+ struct netvsc_channel *nvchan)
{
- struct netvsc_device *nvdev = nvchan->net_device;
struct multi_recv_comp *mrc = &nvchan->mrc;
struct recv_comp_msg {
struct nvsp_message_header hdr;
@@ -968,8 +971,12 @@ static int send_recv_completions(struct netvsc_channel *nvchan)
msg.status = rcd->status;
ret = vmbus_sendpacket(nvchan->channel, &msg, sizeof(msg),
rcd->tid, VM_PKT_COMP, 0);
- if (unlikely(ret))
+ if (unlikely(ret)) {
+ struct net_device_context *ndev_ctx = netdev_priv(ndev);
+
+ ++ndev_ctx->eth_stats.rx_comp_busy;
return ret;
+ }
if (++mrc->first == nvdev->recv_completion_cnt)
mrc->first = 0;
@@ -1010,7 +1017,7 @@ static void enq_receive_complete(struct net_device *ndev,
recv_comp_slot_avail(nvdev, mrc, &filled, &avail);
if (unlikely(filled > NAPI_POLL_WEIGHT)) {
- send_recv_completions(nvchan);
+ send_recv_completions(ndev, nvdev, nvchan);
recv_comp_slot_avail(nvdev, mrc, &filled, &avail);
}
@@ -1193,7 +1200,7 @@ int netvsc_poll(struct napi_struct *napi, int budget)
* then re-enable host interrupts
* and reschedule if ring is not empty.
*/
- if (send_recv_completions(nvchan) == 0 &&
+ if (send_recv_completions(ndev, net_device, nvchan) == 0 &&
work_done < budget &&
napi_complete_done(napi, work_done) &&
hv_end_read(&channel->inbound)) {
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 873c83a66cc2..b33f0507c373 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1112,6 +1112,8 @@ static const struct {
{ "tx_no_space", offsetof(struct netvsc_ethtool_stats, tx_no_space) },
{ "tx_too_big", offsetof(struct netvsc_ethtool_stats, tx_too_big) },
{ "tx_busy", offsetof(struct netvsc_ethtool_stats, tx_busy) },
+ { "tx_send_full", offsetof(struct netvsc_ethtool_stats, tx_send_full) },
+ { "rx_comp_busy", offsetof(struct netvsc_ethtool_stats, rx_comp_busy) },
}, vf_stats[] = {
{ "vf_rx_packets", offsetof(struct netvsc_vf_pcpu_stats, rx_packets) },
{ "vf_rx_bytes", offsetof(struct netvsc_vf_pcpu_stats, rx_bytes) },
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 07/10] netvsc: remove unnecessary cast of void pointer
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
Assignment to a typed pointer is sufficient in C.
No cast is needed.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 3219d2e8918f..9f89de17b5fa 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -523,9 +523,9 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
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 *)((void *)ppi +
- ppi->ppi_offset);
+ IEEE_8021Q_INFO);
+
+ vlan = (void *)ppi + ppi->ppi_offset;
vlan->vlanid = skb->vlan_tci & VLAN_VID_MASK;
vlan->pri = (skb->vlan_tci & VLAN_PRIO_MASK) >>
VLAN_PRIO_SHIFT;
@@ -538,8 +538,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
ppi = init_ppi_data(rndis_msg, NDIS_LSO_PPI_SIZE,
TCP_LARGESEND_PKTINFO);
- lso_info = (struct ndis_tcp_lso_info *)((void *)ppi +
- ppi->ppi_offset);
+ lso_info = (void *)ppi + ppi->ppi_offset;
lso_info->lso_v2_transmit.type = NDIS_TCP_LARGE_SEND_OFFLOAD_V2_TYPE;
if (skb->protocol == htons(ETH_P_IP)) {
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 05/10] netvsc: no need to allocate send/receive on numa node
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
The send and receive buffers are both per-device (not per-channel).
The associated NUMA node is a property of the CPU which is per-channel
therefore it makes no sense to force the receive/send buffer to be
allocated on a particular node (since it is a shared resource).
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index c842265d1df9..91b0674cfdb0 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -245,20 +245,13 @@ int netvsc_alloc_recv_comp_ring(struct netvsc_device *net_device, u32 q_idx)
static int netvsc_init_buf(struct hv_device *device,
struct netvsc_device *net_device)
{
- int ret = 0;
- struct nvsp_message *init_packet;
struct nvsp_1_message_send_receive_buffer_complete *resp;
- struct net_device *ndev;
+ struct net_device *ndev = hv_get_drvdata(device);
+ struct nvsp_message *init_packet;
size_t map_words;
- int node;
-
- ndev = hv_get_drvdata(device);
-
- node = cpu_to_node(device->channel->target_cpu);
- net_device->recv_buf = vzalloc_node(net_device->recv_buf_size, node);
- if (!net_device->recv_buf)
- net_device->recv_buf = vzalloc(net_device->recv_buf_size);
+ int ret = 0;
+ net_device->recv_buf = vzalloc(net_device->recv_buf_size);
if (!net_device->recv_buf) {
netdev_err(ndev, "unable to allocate receive "
"buffer of size %d\n", net_device->recv_buf_size);
@@ -339,9 +332,7 @@ static int netvsc_init_buf(struct hv_device *device,
goto cleanup;
/* Now setup the send buffer. */
- net_device->send_buf = vzalloc_node(net_device->send_buf_size, node);
- if (!net_device->send_buf)
- net_device->send_buf = vzalloc(net_device->send_buf_size);
+ net_device->send_buf = vzalloc(net_device->send_buf_size);
if (!net_device->send_buf) {
netdev_err(ndev, "unable to allocate send "
"buffer of size %d\n", net_device->send_buf_size);
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 03/10] netvsc: propagate MAC address change to VF slave
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
If VF is slaved to synthetic device, then any change to netvsc
MAC address should be propagated to the slave device.
If slave device doesn't support MAC address change then it
should also be an error to attempt to change synthetic NIC MAC
address.
It also fixes the error unwind in the original code.
If give a bad address, the old code would change the device
MAC address anyway.
Reviewed-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index e059375a6d8c..07015b1c42c6 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1053,27 +1053,31 @@ static void netvsc_get_stats64(struct net_device *net,
static int netvsc_set_mac_addr(struct net_device *ndev, void *p)
{
struct net_device_context *ndc = netdev_priv(ndev);
+ struct net_device *vf_netdev = rtnl_dereference(ndc->vf_netdev);
struct netvsc_device *nvdev = rtnl_dereference(ndc->nvdev);
struct sockaddr *addr = p;
- char save_adr[ETH_ALEN];
- unsigned char save_aatype;
int err;
- memcpy(save_adr, ndev->dev_addr, ETH_ALEN);
- save_aatype = ndev->addr_assign_type;
-
- err = eth_mac_addr(ndev, p);
- if (err != 0)
+ err = eth_prepare_mac_addr_change(ndev, p);
+ if (err)
return err;
if (!nvdev)
return -ENODEV;
+ if (vf_netdev) {
+ err = dev_set_mac_address(vf_netdev, addr);
+ if (err)
+ return err;
+ }
+
err = rndis_filter_set_device_mac(nvdev, addr->sa_data);
- if (err != 0) {
- /* roll back to saved MAC */
- memcpy(ndev->dev_addr, save_adr, ETH_ALEN);
- ndev->addr_assign_type = save_aatype;
+ if (!err) {
+ eth_commit_mac_addr_change(ndev, p);
+ } else if (vf_netdev) {
+ /* rollback change on VF */
+ memcpy(addr->sa_data, ndev->dev_addr, ETH_ALEN);
+ dev_set_mac_address(vf_netdev, addr);
}
return err;
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 01/10] netvsc: delay setup of VF device
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
When VF device is discovered, delay bring it automatically up in
order to allow userspace to some simple changes (like renaming).
Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 2 +-
drivers/net/hyperv/netvsc_drv.c | 15 ++++++++-------
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index d1ea99a12cf2..f620c90307ed 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -723,7 +723,7 @@ struct net_device_context {
/* State to manage the associated VF interface. */
struct net_device __rcu *vf_netdev;
struct netvsc_vf_pcpu_stats __percpu *vf_stats;
- struct work_struct vf_takeover;
+ struct delayed_work vf_takeover;
/* 1: allocated, serial number is valid. 0: not allocated */
u32 vf_alloc;
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index eb0023f55fe1..e059375a6d8c 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -47,6 +47,7 @@
#define RING_SIZE_MIN 64
#define LINKCHANGE_INT (2 * HZ)
+#define VF_TAKEOVER_INT (HZ / 10)
static int ring_size = 128;
module_param(ring_size, int, S_IRUGO);
@@ -1559,7 +1560,9 @@ static int netvsc_vf_join(struct net_device *vf_netdev,
/* set slave flag before open to prevent IPv6 addrconf */
vf_netdev->flags |= IFF_SLAVE;
- schedule_work(&ndev_ctx->vf_takeover);
+ schedule_delayed_work(&ndev_ctx->vf_takeover, VF_TAKEOVER_INT);
+
+ call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
netdev_info(vf_netdev, "joined to %s\n", ndev->name);
return 0;
@@ -1575,8 +1578,6 @@ static void __netvsc_vf_setup(struct net_device *ndev,
{
int ret;
- call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
-
/* Align MTU of VF with master */
ret = dev_set_mtu(vf_netdev, ndev->mtu);
if (ret)
@@ -1597,12 +1598,12 @@ static void __netvsc_vf_setup(struct net_device *ndev,
static void netvsc_vf_setup(struct work_struct *w)
{
struct net_device_context *ndev_ctx
- = container_of(w, struct net_device_context, vf_takeover);
+ = container_of(w, struct net_device_context, vf_takeover.work);
struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
struct net_device *vf_netdev;
if (!rtnl_trylock()) {
- schedule_work(w);
+ schedule_delayed_work(&ndev_ctx->vf_takeover, 0);
return;
}
@@ -1706,7 +1707,7 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
return NOTIFY_DONE;
net_device_ctx = netdev_priv(ndev);
- cancel_work_sync(&net_device_ctx->vf_takeover);
+ cancel_delayed_work_sync(&net_device_ctx->vf_takeover);
netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name);
@@ -1748,7 +1749,7 @@ static int netvsc_probe(struct hv_device *dev,
spin_lock_init(&net_device_ctx->lock);
INIT_LIST_HEAD(&net_device_ctx->reconfig_events);
- INIT_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup);
+ INIT_DELAYED_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup);
net_device_ctx->vf_stats
= netdev_alloc_pcpu_stats(struct netvsc_vf_pcpu_stats);
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 00/10] netvsc: minor fixes and improvements
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
These are non-critical bug fixes, related to functionality now in net-next.
1. delaying the automatic bring up of VF device to allow udev to change name.
2. performance improvement
3. handle MAC address change with VF; mostly propogate the error that VF gives.
4. minor cleanups
5. allow setting send/receive buffer size with ethtool.
Stephen Hemminger (10):
netvsc: delay setup of VF device
netvsc: don't signal host twice if empty
netvsc: propagate MAC address change to VF slave
netvsc: check error return when restoring channels and mtu
netvsc: no need to allocate send/receive on numa node
netvsc: whitespace cleanup
netvsc: remove unnecessary cast of void pointer
netvsc: remove unnecessary check for NULL hdr
netvsc: allow controlling send/recv buffer size
netvsc: keep track of some non-fatal overload conditions
drivers/net/hyperv/hyperv_net.h | 13 ++-
drivers/net/hyperv/netvsc.c | 106 ++++++++++-----------
drivers/net/hyperv/netvsc_drv.c | 203 ++++++++++++++++++++++++++++++++--------
3 files changed, 227 insertions(+), 95 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH net-next 09/10] netvsc: allow controlling send/recv buffer size
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
Control the size of the buffer areas via ethtool ring settings.
They aren't really traditional hardware rings, but host API breaks
receive and send buffer into chunks. The final size of the chunks are
controlled by the host.
The default value of send and receive buffer area for host DMA
is much larger than it needs to be. Experimentation shows that
4M receive and 1M send is sufficient.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/hyperv_net.h | 9 ++--
drivers/net/hyperv/netvsc.c | 70 +++++++++++++-----------
drivers/net/hyperv/netvsc_drv.c | 117 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 157 insertions(+), 39 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index f620c90307ed..4ccfc335a356 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -148,6 +148,8 @@ struct netvsc_device_info {
unsigned char mac_adr[ETH_ALEN];
int ring_size;
u32 num_chn;
+ u32 send_sections;
+ u32 recv_sections;
};
enum rndis_device_state {
@@ -634,12 +636,12 @@ struct nvsp_message {
#define NETVSC_SEND_BUFFER_SIZE (1024 * 1024 * 15) /* 15MB */
#define NETVSC_INVALID_INDEX -1
+#define NETVSC_SEND_SECTION_SIZE 6144
+#define NETVSC_RECV_SECTION_SIZE 1728
#define NETVSC_RECEIVE_BUFFER_ID 0xcafe
#define NETVSC_SEND_BUFFER_ID 0
-#define NETVSC_PACKET_SIZE 4096
-
#define VRSS_SEND_TAB_SIZE 16 /* must be power of 2 */
#define VRSS_CHANNEL_MAX 64
#define VRSS_CHANNEL_DEFAULT 8
@@ -754,14 +756,13 @@ struct netvsc_device {
/* Receive buffer allocated by us but manages by NetVSP */
void *recv_buf;
- u32 recv_buf_size;
u32 recv_buf_gpadl_handle;
u32 recv_section_cnt;
+ u32 recv_section_size;
u32 recv_completion_cnt;
/* Send buffer allocated by us */
void *send_buf;
- u32 send_buf_size;
u32 send_buf_gpadl_handle;
u32 send_section_cnt;
u32 send_section_size;
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 91b0674cfdb0..5b3a2b8fa77e 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -75,6 +75,10 @@ static struct netvsc_device *alloc_net_device(void)
atomic_set(&net_device->open_cnt, 0);
net_device->max_pkt = RNDIS_MAX_PKT_DEFAULT;
net_device->pkt_align = RNDIS_PKT_ALIGN_DEFAULT;
+
+ net_device->recv_section_size = NETVSC_RECV_SECTION_SIZE;
+ net_device->send_section_size = NETVSC_SEND_SECTION_SIZE;
+
init_completion(&net_device->channel_init_wait);
return net_device;
@@ -142,6 +146,7 @@ static void netvsc_destroy_buf(struct hv_device *device)
"revoke receive buffer to netvsp\n");
return;
}
+ net_device->recv_section_cnt = 0;
}
/* Teardown the gpadl on the vsp end */
@@ -172,7 +177,7 @@ static void netvsc_destroy_buf(struct hv_device *device)
* NVSP_MSG1_TYPE_SEND_SEND_BUF msg) therefore, we need
* to send a revoke msg here
*/
- if (net_device->send_section_size) {
+ if (net_device->send_section_cnt) {
/* Send the revoke receive buffer */
revoke_packet = &net_device->revoke_packet;
memset(revoke_packet, 0, sizeof(struct nvsp_message));
@@ -204,6 +209,7 @@ static void netvsc_destroy_buf(struct hv_device *device)
"revoke send buffer to netvsp\n");
return;
}
+ net_device->send_section_cnt = 0;
}
/* Teardown the gpadl on the vsp end */
if (net_device->send_buf_gpadl_handle) {
@@ -243,18 +249,25 @@ int netvsc_alloc_recv_comp_ring(struct netvsc_device *net_device, u32 q_idx)
}
static int netvsc_init_buf(struct hv_device *device,
- struct netvsc_device *net_device)
+ struct netvsc_device *net_device,
+ const struct netvsc_device_info *device_info)
{
struct nvsp_1_message_send_receive_buffer_complete *resp;
struct net_device *ndev = hv_get_drvdata(device);
struct nvsp_message *init_packet;
+ unsigned int buf_size;
size_t map_words;
int ret = 0;
- net_device->recv_buf = vzalloc(net_device->recv_buf_size);
+ /* Get receive buffer area. */
+ buf_size = device_info->recv_sections * net_device->recv_section_size;
+ buf_size = roundup(buf_size, PAGE_SIZE);
+
+ net_device->recv_buf = vzalloc(buf_size);
if (!net_device->recv_buf) {
- netdev_err(ndev, "unable to allocate receive "
- "buffer of size %d\n", net_device->recv_buf_size);
+ netdev_err(ndev,
+ "unable to allocate receive buffer of size %u\n",
+ buf_size);
ret = -ENOMEM;
goto cleanup;
}
@@ -265,7 +278,7 @@ static int netvsc_init_buf(struct hv_device *device,
* than the channel to establish the gpadl handle.
*/
ret = vmbus_establish_gpadl(device->channel, net_device->recv_buf,
- net_device->recv_buf_size,
+ buf_size,
&net_device->recv_buf_gpadl_handle);
if (ret != 0) {
netdev_err(ndev,
@@ -311,31 +324,31 @@ static int netvsc_init_buf(struct hv_device *device,
resp->num_sections, resp->sections[0].sub_alloc_size,
resp->sections[0].num_sub_allocs);
- net_device->recv_section_cnt = resp->num_sections;
-
- /*
- * For 1st release, there should only be 1 section that represents the
- * entire receive buffer
- */
- if (net_device->recv_section_cnt != 1 ||
- resp->sections[0].offset != 0) {
+ /* There should only be one section for the entire receive buffer */
+ if (resp->num_sections != 1 || resp->sections[0].offset != 0) {
ret = -EINVAL;
goto cleanup;
}
+ net_device->recv_section_size = resp->sections[0].sub_alloc_size;
+ net_device->recv_section_cnt = resp->sections[0].num_sub_allocs;
+
/* Setup receive completion ring */
net_device->recv_completion_cnt
- = round_up(resp->sections[0].num_sub_allocs + 1,
+ = round_up(net_device->recv_section_cnt + 1,
PAGE_SIZE / sizeof(u64));
ret = netvsc_alloc_recv_comp_ring(net_device, 0);
if (ret)
goto cleanup;
/* Now setup the send buffer. */
- net_device->send_buf = vzalloc(net_device->send_buf_size);
+ buf_size = device_info->send_sections * net_device->send_section_size;
+ buf_size = round_up(buf_size, PAGE_SIZE);
+
+ net_device->send_buf = vzalloc(buf_size);
if (!net_device->send_buf) {
- netdev_err(ndev, "unable to allocate send "
- "buffer of size %d\n", net_device->send_buf_size);
+ netdev_err(ndev, "unable to allocate send buffer of size %u\n",
+ buf_size);
ret = -ENOMEM;
goto cleanup;
}
@@ -345,7 +358,7 @@ static int netvsc_init_buf(struct hv_device *device,
* than the channel to establish the gpadl handle.
*/
ret = vmbus_establish_gpadl(device->channel, net_device->send_buf,
- net_device->send_buf_size,
+ buf_size,
&net_device->send_buf_gpadl_handle);
if (ret != 0) {
netdev_err(ndev,
@@ -390,10 +403,8 @@ static int netvsc_init_buf(struct hv_device *device,
net_device->send_section_size = init_packet->msg.
v1_msg.send_send_buf_complete.section_size;
- /* Section count is simply the size divided by the section size.
- */
- net_device->send_section_cnt =
- net_device->send_buf_size / net_device->send_section_size;
+ /* Section count is simply the size divided by the section size. */
+ net_device->send_section_cnt = buf_size / net_device->send_section_size;
netdev_dbg(ndev, "Send section size: %d, Section count:%d\n",
net_device->send_section_size, net_device->send_section_cnt);
@@ -471,7 +482,8 @@ static int negotiate_nvsp_ver(struct hv_device *device,
}
static int netvsc_connect_vsp(struct hv_device *device,
- struct netvsc_device *net_device)
+ struct netvsc_device *net_device,
+ const struct netvsc_device_info *device_info)
{
const u32 ver_list[] = {
NVSP_PROTOCOL_VERSION_1, NVSP_PROTOCOL_VERSION_2,
@@ -521,14 +533,8 @@ static int netvsc_connect_vsp(struct hv_device *device,
if (ret != 0)
goto cleanup;
- /* Post the big receive buffer to NetVSP */
- if (net_device->nvsp_version <= NVSP_PROTOCOL_VERSION_2)
- net_device->recv_buf_size = NETVSC_RECEIVE_BUFFER_SIZE_LEGACY;
- else
- net_device->recv_buf_size = NETVSC_RECEIVE_BUFFER_SIZE;
- net_device->send_buf_size = NETVSC_SEND_BUFFER_SIZE;
- ret = netvsc_init_buf(device, net_device);
+ ret = netvsc_init_buf(device, net_device, device_info);
cleanup:
return ret;
@@ -1284,7 +1290,7 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
rcu_assign_pointer(net_device_ctx->nvdev, net_device);
/* Connect with the NetVsp */
- ret = netvsc_connect_vsp(device, net_device);
+ ret = netvsc_connect_vsp(device, net_device, device_info);
if (ret != 0) {
netdev_err(ndev,
"unable to connect to NetVSP - %d\n", ret);
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 7b465e40869b..873c83a66cc2 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -45,7 +45,12 @@
#include "hyperv_net.h"
-#define RING_SIZE_MIN 64
+#define RING_SIZE_MIN 64
+#define NETVSC_MIN_TX_SECTIONS 10
+#define NETVSC_DEFAULT_TX 192 /* ~1M */
+#define NETVSC_MIN_RX_SECTIONS 10 /* ~64K */
+#define NETVSC_DEFAULT_RX 2048 /* ~4M */
+
#define LINKCHANGE_INT (2 * HZ)
#define VF_TAKEOVER_INT (HZ / 10)
@@ -831,11 +836,13 @@ static int netvsc_set_channels(struct net_device *net,
if (was_opened)
rndis_filter_close(nvdev);
- rndis_filter_device_remove(dev, nvdev);
-
memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = count;
device_info.ring_size = ring_size;
+ device_info.send_sections = nvdev->send_section_cnt;
+ device_info.recv_sections = nvdev->recv_section_cnt;
+
+ rndis_filter_device_remove(dev, nvdev);
nvdev = rndis_filter_device_add(dev, &device_info);
if (!IS_ERR(nvdev)) {
@@ -947,6 +954,8 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
memset(&device_info, 0, sizeof(device_info));
device_info.ring_size = ring_size;
device_info.num_chn = nvdev->num_chn;
+ device_info.send_sections = nvdev->send_section_cnt;
+ device_info.recv_sections = nvdev->recv_section_cnt;
rndis_filter_device_remove(hdev, nvdev);
@@ -1351,6 +1360,104 @@ static int netvsc_set_rxfh(struct net_device *dev, const u32 *indir,
return rndis_filter_set_rss_param(rndis_dev, key, ndev->num_chn);
}
+/* Hyper-V RNDIS protocol does not have ring in the HW sense.
+ * It does have pre-allocated receive area which is divided into sections.
+ */
+static void __netvsc_get_ringparam(struct netvsc_device *nvdev,
+ struct ethtool_ringparam *ring)
+{
+ u32 max_buf_size;
+
+ ring->rx_pending = nvdev->recv_section_cnt;
+ ring->tx_pending = nvdev->send_section_cnt;
+
+ if (nvdev->nvsp_version <= NVSP_PROTOCOL_VERSION_2)
+ max_buf_size = NETVSC_RECEIVE_BUFFER_SIZE_LEGACY;
+ else
+ max_buf_size = NETVSC_RECEIVE_BUFFER_SIZE;
+
+ ring->rx_max_pending = max_buf_size / nvdev->recv_section_size;
+ ring->tx_max_pending = NETVSC_SEND_BUFFER_SIZE
+ / nvdev->send_section_size;
+}
+
+static void netvsc_get_ringparam(struct net_device *ndev,
+ struct ethtool_ringparam *ring)
+{
+ struct net_device_context *ndevctx = netdev_priv(ndev);
+ struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
+
+ if (!nvdev)
+ return;
+
+ __netvsc_get_ringparam(nvdev, ring);
+}
+
+static int netvsc_set_ringparam(struct net_device *ndev,
+ struct ethtool_ringparam *ring)
+{
+ struct net_device_context *ndevctx = netdev_priv(ndev);
+ struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
+ struct hv_device *hdev = ndevctx->device_ctx;
+ struct netvsc_device_info device_info;
+ struct ethtool_ringparam orig;
+ u32 new_tx, new_rx;
+ bool was_opened;
+ int ret = 0;
+
+ if (!nvdev || nvdev->destroy)
+ return -ENODEV;
+
+ memset(&orig, 0, sizeof(orig));
+ __netvsc_get_ringparam(nvdev, &orig);
+
+ new_tx = clamp_t(u32, ring->tx_pending,
+ NETVSC_MIN_TX_SECTIONS, orig.tx_max_pending);
+ new_rx = clamp_t(u32, ring->rx_pending,
+ NETVSC_MIN_RX_SECTIONS, orig.rx_max_pending);
+
+ if (new_tx == orig.tx_pending &&
+ new_rx == orig.rx_pending)
+ return 0; /* no change */
+
+ memset(&device_info, 0, sizeof(device_info));
+ device_info.num_chn = nvdev->num_chn;
+ device_info.ring_size = ring_size;
+ device_info.send_sections = new_tx;
+ device_info.recv_sections = new_rx;
+
+ netif_device_detach(ndev);
+ was_opened = rndis_filter_opened(nvdev);
+ if (was_opened)
+ rndis_filter_close(nvdev);
+
+ rndis_filter_device_remove(hdev, nvdev);
+
+ nvdev = rndis_filter_device_add(hdev, &device_info);
+ if (IS_ERR(nvdev)) {
+ ret = PTR_ERR(nvdev);
+
+ device_info.send_sections = orig.tx_pending;
+ device_info.recv_sections = orig.rx_pending;
+ nvdev = rndis_filter_device_add(hdev, &device_info);
+ if (IS_ERR(nvdev)) {
+ netdev_err(ndev, "restoring ringparam failed: %ld\n",
+ PTR_ERR(nvdev));
+ return ret;
+ }
+ }
+
+ if (was_opened)
+ rndis_filter_open(nvdev);
+ netif_device_attach(ndev);
+
+ /* We may have missed link change notifications */
+ ndevctx->last_reconfig = 0;
+ schedule_delayed_work(&ndevctx->dwork, 0);
+
+ return ret;
+}
+
static const struct ethtool_ops ethtool_ops = {
.get_drvinfo = netvsc_get_drvinfo,
.get_link = ethtool_op_get_link,
@@ -1367,6 +1474,8 @@ static const struct ethtool_ops ethtool_ops = {
.set_rxfh = netvsc_set_rxfh,
.get_link_ksettings = netvsc_get_link_ksettings,
.set_link_ksettings = netvsc_set_link_ksettings,
+ .get_ringparam = netvsc_get_ringparam,
+ .set_ringparam = netvsc_set_ringparam,
};
static const struct net_device_ops device_ops = {
@@ -1782,6 +1891,8 @@ static int netvsc_probe(struct hv_device *dev,
memset(&device_info, 0, sizeof(device_info));
device_info.ring_size = ring_size;
device_info.num_chn = VRSS_CHANNEL_DEFAULT;
+ device_info.send_sections = NETVSC_DEFAULT_TX;
+ device_info.recv_sections = NETVSC_DEFAULT_RX;
nvdev = rndis_filter_device_add(dev, &device_info);
if (IS_ERR(nvdev)) {
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 08/10] netvsc: remove unnecessary check for NULL hdr
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
The function init_page_array is always called with a valid pointer
to RNDIS header. No check for NULL is needed.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 9f89de17b5fa..7b465e40869b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -333,10 +333,9 @@ static u32 init_page_array(void *hdr, u32 len, struct sk_buff *skb,
* 2. skb linear data
* 3. skb fragment data
*/
- if (hdr != NULL)
- slots_used += fill_pg_buf(virt_to_page(hdr),
- offset_in_page(hdr),
- len, &pb[slots_used]);
+ slots_used += fill_pg_buf(virt_to_page(hdr),
+ offset_in_page(hdr),
+ len, &pb[slots_used]);
packet->rmsg_size = len;
packet->rmsg_pgcnt = slots_used;
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 06/10] netvsc: whitespace cleanup
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
Fix some minor indentation issues.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index c7391889938b..3219d2e8918f 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -165,7 +165,7 @@ static int netvsc_close(struct net_device *net)
}
static void *init_ppi_data(struct rndis_message *msg, u32 ppi_size,
- int pkt_type)
+ int pkt_type)
{
struct rndis_packet *rndis_pkt;
struct rndis_per_packet_info *ppi;
@@ -286,7 +286,7 @@ static u16 netvsc_select_queue(struct net_device *ndev, struct sk_buff *skb,
}
static u32 fill_pg_buf(struct page *page, u32 offset, u32 len,
- struct hv_page_buffer *pb)
+ struct hv_page_buffer *pb)
{
int j = 0;
@@ -626,6 +626,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
++net_device_ctx->eth_stats.tx_no_memory;
goto drop;
}
+
/*
* netvsc_linkstatus_callback - Link up/down notification
*/
@@ -649,8 +650,8 @@ void netvsc_linkstatus_callback(struct hv_device *device_obj,
if (indicate->status == RNDIS_STATUS_LINK_SPEED_CHANGE) {
u32 speed;
- speed = *(u32 *)((void *)indicate + indicate->
- status_buf_offset) / 10000;
+ speed = *(u32 *)((void *)indicate
+ + indicate->status_buf_offset) / 10000;
ndev_ctx->speed = speed;
return;
}
@@ -1018,7 +1019,7 @@ static void netvsc_get_stats64(struct net_device *net,
struct net_device_context *ndev_ctx = netdev_priv(net);
struct netvsc_device *nvdev = rcu_dereference_rtnl(ndev_ctx->nvdev);
struct netvsc_vf_pcpu_stats vf_tot;
- int i;
+ int i;
if (!nvdev)
return;
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 04/10] netvsc: check error return when restoring channels and mtu
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
If setting new values fails, and the attempt to restore original
settings fails. Then log an error and leave device down.
This should never happen, but if it does don't go down in flames.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 07015b1c42c6..c7391889938b 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -845,7 +845,13 @@ static int netvsc_set_channels(struct net_device *net,
} else {
ret = PTR_ERR(nvdev);
device_info.num_chn = orig;
- rndis_filter_device_add(dev, &device_info);
+ nvdev = rndis_filter_device_add(dev, &device_info);
+
+ if (IS_ERR(nvdev)) {
+ netdev_err(net, "restoring channel setting failed: %ld\n",
+ PTR_ERR(nvdev));
+ return ret;
+ }
}
if (was_opened)
@@ -953,10 +959,16 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
/* Attempt rollback to original MTU */
ndev->mtu = orig_mtu;
- rndis_filter_device_add(hdev, &device_info);
+ nvdev = rndis_filter_device_add(hdev, &device_info);
if (vf_netdev)
dev_set_mtu(vf_netdev, orig_mtu);
+
+ if (IS_ERR(nvdev)) {
+ netdev_err(ndev, "restoring mtu failed: %ld\n",
+ PTR_ERR(nvdev));
+ return ret;
+ }
}
if (was_opened)
--
2.11.0
^ permalink raw reply related
* [PATCH net-next 02/10] netvsc: don't signal host twice if empty
From: Stephen Hemminger @ 2017-08-10 0:46 UTC (permalink / raw)
To: kys, haiyangz, sthemmin; +Cc: devel, netdev
In-Reply-To: <20170810004612.22163-1-sthemmin@microsoft.com>
When hv_pkt_iter_next() returns NULL, it has already called
hv_pkt_iter_close(). Calling it twice can lead to extra host signal.
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
drivers/net/hyperv/netvsc.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 208f03aa83de..c842265d1df9 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -1190,10 +1190,6 @@ int netvsc_poll(struct napi_struct *napi, int budget)
nvchan->desc = hv_pkt_iter_next(channel, nvchan->desc);
}
- /* if ring is empty, signal host */
- if (!nvchan->desc)
- hv_pkt_iter_close(channel);
-
/* If send of pending receive completions suceeded
* and did not exhaust NAPI budget this time
* and not doing busy poll
--
2.11.0
^ permalink raw reply related
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: John Stultz @ 2017-08-10 0:44 UTC (permalink / raw)
To: Wei Wang
Cc: Cong Wang, lkml, Network Development, Linux USB List,
David S. Miller, Felipe Balbi
In-Reply-To: <CAEA6p_AL8uUghtZgp4OmqygcFs-Jiy286P5r6tRd+yeSWxR3KA@mail.gmail.com>
On Wed, Aug 9, 2017 at 5:36 PM, Wei Wang <weiwan@google.com> wrote:
>
> Does your USB adapter get an IPv6 address?
Yes, it does.
> If you see the problem starts to happen on commit
> 9514528d92d4cbe086499322370155ed69f5d06c, could you try reverting all
> the following commits:
> (from new to old)
> 1eb04e7c9e63 net: reorder all the dst flags
> a4c2fd7f7891 net: remove DST_NOCACHE flag
> b2a9c0ed75a3 net: remove DST_NOGC flag
> 5b7c9a8ff828 net: remove dst gc related code
> db916649b5dd ipv6: get rid of icmp6 dst garbage collector
> 587fea741134 ipv6: mark DST_NOGC and remove the operation of dst_free()
> ad65a2f05695 ipv6: call dst_hold_safe() properly
> 9514528d92d4 ipv6: call dst_dev_put() properly
>
> and try if it starts to work?
I'll give that a shot!
Thanks so much for the help! I really appreciate it!
-john
^ permalink raw reply
* Re: unregister_netdevice: waiting for eth0 to become free. Usage count = 1
From: Wei Wang @ 2017-08-10 0:36 UTC (permalink / raw)
To: John Stultz
Cc: Cong Wang, lkml, Network Development, Linux USB List,
David S. Miller, Felipe Balbi
In-Reply-To: <CALAqxLXv60ReJu26omM6p=ZboEm-Z-3w+q0JcxLXDqqK5ohNoQ@mail.gmail.com>
On Wed, Aug 9, 2017 at 4:44 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Wed, Aug 9, 2017 at 4:34 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> (Cc'ing Wei whose commit was blamed)
>>
>> On Mon, Aug 7, 2017 at 2:15 PM, John Stultz <john.stultz@linaro.org> wrote:
>>> On Mon, Aug 7, 2017 at 2:05 PM, John Stultz <john.stultz@linaro.org> wrote:
>>>> So, with recent testing with my HiKey board, I've been noticing some
>>>> quirky behavior with my USB eth adapter.
>>>>
>>>> Basically, pluging the usb eth adapter in and then removing it, when
>>>> plugging it back in I often find that its not detected, and the system
>>>> slowly spits out the following message over and over:
>>>> unregister_netdevice: waiting for eth0 to become free. Usage count = 1
>>>
>>> The other bit is that after this starts printing, the board will no
>>> longer reboot (it hangs continuing to occasionally print the above
>>> message), and I have to manually reset the device.
>>>
>>
>> So this warning is not temporarily shown but lasts until a reboot,
>> right? If so it is a dst refcnt leak.
>
> Correct, once I get into the state it lasts until a reboot.
>
>> How reproducible is it for you? From my reading, it seems always
>> reproduced when you unplug and plug your usb eth interface?
>> Is there anything else involved? For example, network namespace.
>
> So with 4.13-rc3/4 I seem to trigger it easily, often with the first
> unplug of the USB eth adapter.
>
> But as I get back closer to 4.12, it seemingly becomes harder to
> trigger, but sometimes still happens.
>
> So far, I've not been able to trigger it with 4.12.
>
> I don't think network namespaces are involved? Though its out of my
> area, so AOSP may be using them these days. Is there a simple way to
> check?
>
> I'll also do another bisection to see if the bad point moves back any further.
>
> thanks
> -john
Hi John,
Does your USB adapter get an IPv6 address?
If you see the problem starts to happen on commit
9514528d92d4cbe086499322370155ed69f5d06c, could you try reverting all
the following commits:
(from new to old)
1eb04e7c9e63 net: reorder all the dst flags
a4c2fd7f7891 net: remove DST_NOCACHE flag
b2a9c0ed75a3 net: remove DST_NOGC flag
5b7c9a8ff828 net: remove dst gc related code
db916649b5dd ipv6: get rid of icmp6 dst garbage collector
587fea741134 ipv6: mark DST_NOGC and remove the operation of dst_free()
ad65a2f05695 ipv6: call dst_hold_safe() properly
9514528d92d4 ipv6: call dst_dev_put() properly
and try if it starts to work?
By only reverting 9514528d92d4 definitely won't work as all the later
commits depend on this one.
Thanks a lot.
Wei
^ permalink raw reply
* Re: [PATCH iproute2 master] bpf: unbreak libelf linkage for bpf obj loader
From: Stephen Hemminger @ 2017-08-10 0:35 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev
In-Reply-To: <8ddcce8a9564e9c4a265db790002d6cd5ba8f353.1502316870.git.daniel@iogearbox.net>
On Thu, 10 Aug 2017 00:15:41 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:
> Commit 69fed534a533 ("change how Config is used in Makefile's") moved
> HAVE_MNL specific CFLAGS/LDLIBS for building with libmnl out of the
> top level Makefile into sub-Makefiles. However, it also removed the
> HAVE_ELF specific CFLAGS/LDLIBS entirely, which breaks the BPF object
> loader for tc and ip with "No ELF library support compiled in." despite
> having libelf detected in configure script. Fix it similarly as in
> 69fed534a533 for HAVE_ELF.
>
> Fixes: 69fed534a533 ("change how Config is used in Makefile's")
> Reported-by: Jeffrey Panneman <jeffrey.panneman@tno.nl>
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Thanks, but I am thinking maybe a better solution long term would be to
move all the package specific stuff into the generated Config file.
That way only the generation shell script would have to change.
Also, all the flags should probably be using pkg-config to get the values.
^ permalink raw reply
* Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482
From: Rao Shoaib @ 2017-08-10 0:31 UTC (permalink / raw)
To: Joe Smith, Jerry Chu
Cc: Yuchung Cheng, David Miller, Alexey Kuznetsov, netdev
In-Reply-To: <CABGNeczGSsL6xj-SkXQYTWXXe+mJoyhvrQxuLkiO27pW0y8MtA@mail.gmail.com>
On 08/09/2017 05:20 PM, Joe Smith wrote:
> On Wed, Aug 9, 2017 at 4:52 PM, Jerry Chu <hkchu@google.com> wrote:
>> [try to recover from long lost memory]
>>
>> On Tue, Aug 8, 2017 at 10:25 AM, Yuchung Cheng <ycheng@google.com> wrote:
>>> On Mon, Aug 7, 2017 at 11:16 AM, Rao Shoaib <rao.shoaib@oracle.com> wrote:
>>>> Change from version 0: Rationale behind the change:
>>>>
>>>> The man page for tcp(7) states
>>>>
>>>> when used with the TCP keepalive (SO_KEEPALIVE) option, TCP_USER_TIMEOUT will
>>>> override keepalive to determine when to close a connection due to keepalive
>>>> failure.
>>>>
>>>> This is ambigious at best. user expectation is most likely that the connection
>>>> will be reset after TCP_USER_TIMEOUT milliseconds of inactivity.
>>> ccing the original author Jerry Chu who can tell more.
>>>
>> There was a reason for the above otherwise I wouldn't have explicitly
>> spelled it out in
>> my commit msg. But unfortunately it was seven years ago and I can't
>> remember why.
>> It could range from micro-optimization (saving a syscall() because
>> this facility was
>> used by servers handling millions of Android clients) to something more critical
>> but I can't remember.
> The issue is that the man page is ambiguous and does not conform to
> any standard.
> Whether RFC 5482 is in little use or not that was cited as the basis
> of this change and
> I want to change the behavior to conform to it as users are confused.
>
> I doubt that saving a syscall is of any benefit when the connection
> has been idle for 2hrs. If anything the user expects the keep alive
> probes to start after TCP_USER_TIMEOUT of inactivity. In which case
> keep alive should be adjusted.
>
>>>> The code however waits for the keepalive to kick-in (default 2hrs) and than
>>>> after one failure resets the conenction.
>>>>
>>>> What is the rationale for that ? The same effect can be obtained by simply
>>>> changing the value of tcp_keep_alive_probes.
>>>>
>>>> Since the TCP_USER_TIMEOUT option was added based on RFC 5482 we need to follow
>>>> the RFC. Which states
>> Well the patch has little to do with RFC5482 other than borrowing the name, and
>> also conveniently providing a mechanism for RFC5482 apps to program the local
>> timeout value. As far as I knew back when I worked on the patch, RFC5482 was
>> under little use (told directly by Lars).
>>
>> Your proposed change may not be unreasonable but my fear is it may
>> cause breakage
>> on apps that depend on "TCP_USER_TIMEOUT will overtake keepalive to determine
>> when to close a connection due to keepalive failure". What is your
>> case for "RFC5482
>> compliance" after all? I know the TCP_USER_TIMEOUT option has been very popular
>> among apps since its inception.
> The only use of TCP_USER_TIMEOUT has been for flushing unacknowledged
> data (evident from all the fixes). That behavior is not being touched.
>
> Making Linux conform to standards and behavior that is logical seems
> like a good enough reason. Mixing keep alive and TCP_USER_TIMEOUT does
> not make any sense. I doubt very much if this change will break
> anything but if it does than we need to see why that is needed and
> implement a proper fix and document it.
>
>
The behavior for the main use case has been previously changed as part
of bug fixes. This is a very low risk change and makes the code logical
and clean.
Shoaib
^ permalink raw reply
* Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482
From: David Miller @ 2017-08-10 0:30 UTC (permalink / raw)
To: codesoldier1; +Cc: hkchu, ycheng, rao.shoaib, kuznet, netdev
In-Reply-To: <CABGNeczGSsL6xj-SkXQYTWXXe+mJoyhvrQxuLkiO27pW0y8MtA@mail.gmail.com>
From: Joe Smith <codesoldier1@gmail.com>
Date: Wed, 9 Aug 2017 17:20:32 -0700
> Making Linux conform to standards and behavior that is logical seems
> like a good enough reason.
That's an awesome attitude to have when we're implementing something
new and don't have the facility already.
But when we have something already the only important consideration is
not breaking existing apps which rely on that behavior.
That is much, much, more important than standards compliance.
If users are confused, just fix the documentation.
^ permalink raw reply
* Re: [PATCH v2 net-next 0/7] rtnetlink: allow selected handlers to run without rtnl
From: David Miller @ 2017-08-10 0:21 UTC (permalink / raw)
To: fw; +Cc: netdev
In-Reply-To: <20170809184153.16700-1-fw@strlen.de>
From: Florian Westphal <fw@strlen.de>
Date: Wed, 9 Aug 2017 20:41:46 +0200
> Changes since v1:
> In patch 6, don't make ipv6 route handlers lockless, they all have
> assumptions on rtnl being held. Other patches are unchanged.
>
> The RTNL mutex is used to serialize both rtnetlink calls and
> dump requests.
> Its also used to protect other things such as the list of current
> net namespaces.
>
> Unfortunately RTNL mutex is a performance issue, e.g. a cpu adding an
> ip address prevents other cpus from seemingly unrelated tasks such as
> dumping tc classifiers or doing rtnetlink route lookups.
>
> This patch set adds basic infrastructure to start pushing the rtnl lock
> down to those places that need it, or even elide it entirely in some cases.
>
> Subsystems can now indicate that their doit() callback can run without
> RTNL mutex, such callbacks can then run in parallel.
>
> This will obviously need a lot of followup work; all current
> users need to be audited/changed to benefit from this.
> Initial no-rtnl spot is netns new/getid.
>
> We have various 'get' handlers that are also a tempting target,
> however, several of these depend on rtnl mutex to prevent information
> from changing while objects are being read by rtnl handlers; however,
> it doesn't appear impossible to change this.
>
> Dumps are another problem entirely, see
> commit 2907c35ff64708065 ("net: hold rtnl again in dump callbacks"),
> this patchset doesn't touch dump requests.
Ok series applied, let's see where this goes :-)
^ permalink raw reply
* Re: [PATCH v4 05/12] Documentation: net: phy: Add phy-is-internal binding
From: Andrew Lunn @ 2017-08-10 0:20 UTC (permalink / raw)
To: Florian Fainelli
Cc: David Wu, davem, heiko, robh+dt, mark.rutland, catalin.marinas,
will.deacon, olof, linux, arnd, peppe.cavallaro, alexandre.torgue,
huangtao, hwg, netdev, linux-arm-kernel, linux-rockchip,
devicetree, linux-kernel
In-Reply-To: <778B0470-8ABD-46AD-A63D-F0CF4911C800@gmail.com>
On Wed, Aug 09, 2017 at 03:47:34PM -0700, Florian Fainelli wrote:
> On August 9, 2017 5:10:30 AM PDT, David Wu <david.wu@rock-chips.com> wrote:
> >Add the documentation for internal phy. A boolean property
> >indicates that a internal phy will be used.
> >
> >Signed-off-by: David Wu <david.wu@rock-chips.com>
> >---
> > Documentation/devicetree/bindings/net/phy.txt | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/Documentation/devicetree/bindings/net/phy.txt
> >b/Documentation/devicetree/bindings/net/phy.txt
> >index b558576..942c892 100644
> >--- a/Documentation/devicetree/bindings/net/phy.txt
> >+++ b/Documentation/devicetree/bindings/net/phy.txt
> >@@ -52,6 +52,9 @@ Optional Properties:
> > Mark the corresponding energy efficient ethernet mode as broken and
> > request the ethernet to stop advertising it.
> >
> >+- phy-is-internal: If set, indicates that phy will connect to the MAC
> >as a
> >+ internal phy.
>
> Something along the lines of:
>
> If set, indicates that the PHY is integrated into the same physical package as the Ethernet MAC.
Hi Florian, David.
I'm happy with the property name. But i think the text needs more
description. We deal with Ethernet switches with integrated PHYs. Yet
for us, this property is unneeded.
Seeing this property means some bit of software needs to ensure the
internal PHY should be used, when given the choice between an internal
and external PHY. So i would say something like:
If set, indicates that the PHY is integrated into the same
physical package as the Ethernet MAC. If needed, muxers should be
configured to ensure the internal PHY is used. The absence of this
property indicates the muxers should be configured so that the
external PHY is used.
This last part is important. If the bootloader has set the internal
PHY to be used, you need to reset it. Otherwise we are going to get
into a mess sometime later and need to add a phy-is-external property.
Andrew
^ permalink raw reply
* Re: [PATCH v1 net] TCP_USER_TIMEOUT and tcp_keepalive should conform to RFC5482
From: Joe Smith @ 2017-08-10 0:20 UTC (permalink / raw)
To: Jerry Chu
Cc: Yuchung Cheng, Rao Shoaib, David Miller, Alexey Kuznetsov, netdev
In-Reply-To: <CAPshTCgaYVPEbZxDr9sUz7OFibweQn6fwRtP66cgw5eNrHUOWA@mail.gmail.com>
On Wed, Aug 9, 2017 at 4:52 PM, Jerry Chu <hkchu@google.com> wrote:
> [try to recover from long lost memory]
>
> On Tue, Aug 8, 2017 at 10:25 AM, Yuchung Cheng <ycheng@google.com> wrote:
>> On Mon, Aug 7, 2017 at 11:16 AM, Rao Shoaib <rao.shoaib@oracle.com> wrote:
>>> Change from version 0: Rationale behind the change:
>>>
>>> The man page for tcp(7) states
>>>
>>> when used with the TCP keepalive (SO_KEEPALIVE) option, TCP_USER_TIMEOUT will
>>> override keepalive to determine when to close a connection due to keepalive
>>> failure.
>>>
>>> This is ambigious at best. user expectation is most likely that the connection
>>> will be reset after TCP_USER_TIMEOUT milliseconds of inactivity.
>> ccing the original author Jerry Chu who can tell more.
>>
>
> There was a reason for the above otherwise I wouldn't have explicitly
> spelled it out in
> my commit msg. But unfortunately it was seven years ago and I can't
> remember why.
> It could range from micro-optimization (saving a syscall() because
> this facility was
> used by servers handling millions of Android clients) to something more critical
> but I can't remember.
The issue is that the man page is ambiguous and does not conform to
any standard.
Whether RFC 5482 is in little use or not that was cited as the basis
of this change and
I want to change the behavior to conform to it as users are confused.
I doubt that saving a syscall is of any benefit when the connection
has been idle for 2hrs. If anything the user expects the keep alive
probes to start after TCP_USER_TIMEOUT of inactivity. In which case
keep alive should be adjusted.
>
>>>
>>> The code however waits for the keepalive to kick-in (default 2hrs) and than
>>> after one failure resets the conenction.
>>>
>>> What is the rationale for that ? The same effect can be obtained by simply
>>> changing the value of tcp_keep_alive_probes.
>>>
>>> Since the TCP_USER_TIMEOUT option was added based on RFC 5482 we need to follow
>>> the RFC. Which states
>
> Well the patch has little to do with RFC5482 other than borrowing the name, and
> also conveniently providing a mechanism for RFC5482 apps to program the local
> timeout value. As far as I knew back when I worked on the patch, RFC5482 was
> under little use (told directly by Lars).
>
> Your proposed change may not be unreasonable but my fear is it may
> cause breakage
> on apps that depend on "TCP_USER_TIMEOUT will overtake keepalive to determine
> when to close a connection due to keepalive failure". What is your
> case for "RFC5482
> compliance" after all? I know the TCP_USER_TIMEOUT option has been very popular
> among apps since its inception.
The only use of TCP_USER_TIMEOUT has been for flushing unacknowledged
data (evident from all the fixes). That behavior is not being touched.
Making Linux conform to standards and behavior that is logical seems
like a good enough reason. Mixing keep alive and TCP_USER_TIMEOUT does
not make any sense. I doubt very much if this change will break
anything but if it does than we need to see why that is needed and
implement a proper fix and document it.
Shoaib
>
>>>
>>> 4.2 TCP keep-Alives:
>>> Some TCP implementations, such as those in BSD systems, use a
>>> different abort policy for TCP keep-alives than for user data. Thus,
>>> the TCP keep-alive mechanism might abort a connection that would
>>> otherwise have survived the transient period without connectivity.
>>> Therefore, if a connection that enables keep-alives is also using the
>>> TCP User Timeout Option, then the keep-alive timer MUST be set to a
>>> value larger than that of the adopted USER TIMEOUT.
>>>
>>> This patch enforces the MUST and also dis-associates user timeout from keep
>>> alive. A man page patch will be submitted separately.
>>>
>>> Signed-off-by: Rao Shoaib <rao.shoaib@oracle.com>
>>> ---
>>> net/ipv4/tcp.c | 10 ++++++++--
>>> net/ipv4/tcp_timer.c | 9 +--------
>>> 2 files changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
>>> index 71ce33d..f2af44d 100644
>>> --- a/net/ipv4/tcp.c
>>> +++ b/net/ipv4/tcp.c
>>> @@ -2628,7 +2628,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>>> break;
>>>
>>> case TCP_KEEPIDLE:
>>> - if (val < 1 || val > MAX_TCP_KEEPIDLE)
>>> + /* Per RFC5482 keepalive_time must be > user_timeout */
>>> + if (val < 1 || val > MAX_TCP_KEEPIDLE ||
>>> + ((val * HZ) <= icsk->icsk_user_timeout))
>>> err = -EINVAL;
>>> else {
>>> tp->keepalive_time = val * HZ;
>>> @@ -2724,8 +2726,12 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>>> case TCP_USER_TIMEOUT:
>>> /* Cap the max time in ms TCP will retry or probe the window
>>> * before giving up and aborting (ETIMEDOUT) a connection.
>>> + * Per RFC5482 TCP user timeout must be < keepalive_time.
>>> + * If the default value changes later -- all bets are off.
>>> */
>>> - if (val < 0)
>>> + if (val < 0 || (tp->keepalive_time &&
>>> + tp->keepalive_time <= msecs_to_jiffies(val)) ||
>>> + net->ipv4.sysctl_tcp_keepalive_time <= msecs_to_jiffies(val))
>>> err = -EINVAL;
>>> else
>>> icsk->icsk_user_timeout = msecs_to_jiffies(val);
>>> diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
>>> index c0feeee..d39fe60 100644
>>> --- a/net/ipv4/tcp_timer.c
>>> +++ b/net/ipv4/tcp_timer.c
>>> @@ -664,14 +664,7 @@ static void tcp_keepalive_timer (unsigned long data)
>>> elapsed = keepalive_time_elapsed(tp);
>>>
>>> if (elapsed >= keepalive_time_when(tp)) {
>>> - /* If the TCP_USER_TIMEOUT option is enabled, use that
>>> - * to determine when to timeout instead.
>>> - */
>>> - if ((icsk->icsk_user_timeout != 0 &&
>>> - elapsed >= icsk->icsk_user_timeout &&
>>> - icsk->icsk_probes_out > 0) ||
>>> - (icsk->icsk_user_timeout == 0 &&
>>> - icsk->icsk_probes_out >= keepalive_probes(tp))) {
>>> + if (icsk->icsk_probes_out >= keepalive_probes(tp)) {
>>> tcp_send_active_reset(sk, GFP_ATOMIC);
>>> tcp_write_err(sk);
>>> goto out;
>>> --
>>> 2.7.4
>>>
--
JS
^ 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