* Re: [PATCH net-next v2 0/7] net: dsa: mv88e6xxx: ops cosmetics
From: David Miller @ 2017-06-09 16:11 UTC (permalink / raw)
To: vivien.didelot; +Cc: netdev, linux-kernel, kernel, f.fainelli, andrew
In-Reply-To: <20170608223414.594-1-vivien.didelot@savoirfairelinux.com>
From: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Date: Thu, 8 Jun 2017 18:34:07 -0400
> This patchset brings no functional changes. It is a first step in a
> bigger cosmetics change to the driver. It simplifies print messages and
> polishes data types and chip operations.
>
> The next patchs will only prefix and document the port registers macros.
>
> Changes in v2:
> - KISS and simply use dev_* since chip->ds may not be initialized
> - add reviewers tags
Series applied, thank you.
^ permalink raw reply
* [PATCH net-next v2] bonding: warn user when 802.3ad speed is unknown
From: Nicolas Dichtel @ 2017-06-09 15:58 UTC (permalink / raw)
To: andrew
Cc: netdev, davem, j.vosburgh, vfalico, andy, thibaut.collet,
linville, Nicolas Dichtel
In-Reply-To: <20170609145932.GG20756@lunn.ch>
Goal is to advertise the user when ethtool speeds and 802.3ad speeds are
desynchronized.
When this case happens, the kernel needs to be patched.
Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
---
Here is another proposal. Comments are welcome.
v2: use pr_warn_once()
warn only when speed != SPEED_UNKNOWN
drivers/net/bonding/bond_3ad.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index b44a6aeb346d..165a8009c640 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -322,6 +322,11 @@ static u16 __get_link_speed(struct port *port)
default:
/* unknown speed value from ethtool. shouldn't happen */
+ if (slave->speed != SPEED_UNKNOWN)
+ pr_warn_once("%s: unknown ethtool speed (%d) for port %d (set it to 0)\n",
+ slave->bond->dev->name,
+ slave->speed,
+ port->actor_port_number);
speed = 0;
break;
}
--
2.8.1
^ permalink raw reply related
* Re: [PATCH net] ipv4: igmp: fix a use after free
From: Eric Dumazet @ 2017-06-09 15:56 UTC (permalink / raw)
To: Xin Long; +Cc: Cong Wang, Andrey Konovalov, David S. Miller, netdev
In-Reply-To: <CADvbK_cv+yfZpa7y1+5a9mntTbkTmEu6YyR7pbHGAtH+5+RLRA@mail.gmail.com>
On Fri, 2017-06-09 at 14:24 +0800, Xin Long wrote:
> On Fri, Jun 9, 2017 at 8:59 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> > On Thu, Jun 8, 2017 at 1:33 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> I mentioned (in https://lkml.org/lkml/2017/5/31/619 ) that we might need
> >> to defer freeing after rcu grace period but for some reason decided it
> >> was not needed.
> Yes, this one could fix it.
>
> >
> > This one makes sense, it is the second time I saw the use-after-free
> > in igmp code, both are because we don't respect the RCU rule to free
> > an element in the list.
> >
> >>
> >> What about :
> >
> > But not sure if all ip_ma_put() callers want ip_mc_clear_src().
> If that's problem, there may be another way:
>
> leave ip_mc_clear_src as it is, just add pmc->lock to protect this call.
>
> this use-after-free was actually caused by using pmc->sources/tomb
> in add_grec while ip_mc_clear_src is freeing them. add_grec is already
> under pmc->lock, so to add pmc->lock for ip_mc_clear_src should be
> enough to protect the list pmc->sources/tomb.
>
> wdyt ?
This would we weird.
When we free skb components, we do not grab a spinlock.
When we free something, just make sure we must be the last user of it.
RCU rules -> Must respect RCU grace period before delete.
No need for extra spinlock.
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] udp: try to avoid 2 cache miss on dequeue
From: Paolo Abeni @ 2017-06-09 15:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, edumazet
In-Reply-To: <20170607.101056.833234534371053047.davem@davemloft.net>
Hi,
On Wed, 2017-06-07 at 10:10 -0400, David Miller wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Wed, 07 Jun 2017 09:56:45 +0200
>
> > Hi David,
> >
> > On Tue, 2017-06-06 at 16:23 +0200, Paolo Abeni wrote:
> >> when udp_recvmsg() is executed, on x86_64 and other archs, most skb
> >> fields are on cold cachelines.
> >> If the skb are linear and the kernel don't need to compute the udp
> >> csum, only a handful of skb fields are required by udp_recvmsg().
> >> Since we already use skb->dev_scratch to cache hot data, and
> >> there are 32 bits unused on 64 bit archs, use such field to cache
> >> as much data as we can, and try to prefetch on dequeue the relevant
> >> fields that are left out.
> >>
> >> This can save up to 2 cache miss per packet.
> >>
> >> v1 -> v2:
> >> - changed udp_dev_scratch fields types to u{32,16} variant,
> >> replaced bitfield with bool
> >>
> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> >
> > Can you please keep on-hold this series a little time? the lkp-robot
> > just reported a performance regression on v1 which I have still to
> > investigate. I can't look at it really soon, but I expect the same
> > should apply to v2.
> >
> > It sounds quite weird to me, since the bisected patch touches the UDP
> > code only and the regression is on apachebench.
>
> Hmmm, DNS lookups?
>
> Thanks for looking into this.
I spent a little time trying to reproduce the regression. There are not
DNS requests during the test, because it's done against the loopback
address (verified with perf probe on UDP code).
I collected several samples for both the patched and vanilla kernels,
and I measured a lot of variance (while using the same kernel) - well
above 21% - and a similar results distribution when comparing vanilla
to patched kernel.
I notified the lkp ML of the above, and I think this is actually a
test-suite artifact.
I'll re-submit v3 unchanged, if there are no objections.
Cheers,
Paolo
^ permalink raw reply
* Re: [PATCH net-next 2/2] bpf: Fix test_obj_id.c for llvm 5.0
From: Yonghong Song @ 2017-06-09 15:40 UTC (permalink / raw)
To: Martin KaFai Lau, netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <20170609053017.2909855-2-kafai@fb.com>
On 6/8/17 10:30 PM, Martin KaFai Lau wrote:
> llvm 5.0 does not like the section name and the function name
> to be the same:
>
> clang -I. -I./include/uapi -I../../../include/uapi \
> -I../../../../samples/bpf/ \
> -Wno-compare-distinct-pointer-types \
> -O2 -target bpf -c \
> linux/tools/testing/selftests/bpf/test_obj_id.c -o \
> linux/tools/testing/selftests/bpf/test_obj_id.o
> fatal error: error in backend: 'test_prog_id' label emitted multiple times to
> assembly file
> clang-5.0: error: clang frontend command failed with exit code 70 (use -v to
> see invocation)
> clang version 5.0.0 (trunk 304326) (llvm/trunk 304329)
>
> This patch makes changes to the section name and the function name.
>
> Fixes: 95b9afd3987f ("bpf: Test for bpf ID")
> Reported-by: Alexei Starovoitov <ast@fb.com>
> Reported-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> tools/testing/selftests/bpf/test_obj_id.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_obj_id.c b/tools/testing/selftests/bpf/test_obj_id.c
> index d8723aaf827a..880d2963b472 100644
> --- a/tools/testing/selftests/bpf/test_obj_id.c
> +++ b/tools/testing/selftests/bpf/test_obj_id.c
> @@ -23,8 +23,8 @@ struct bpf_map_def SEC("maps") test_map_id = {
> .max_entries = 1,
> };
>
> -SEC("test_prog_id")
> -int test_prog_id(struct __sk_buff *skb)
> +SEC("test_obj_id_dummy")
> +int test_obj_id(struct __sk_buff *skb)
> {
> __u32 key = 0;
> __u64 *value;
>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH net-next 1/2] bpf: Fix test_bpf_obj_id() when the bpf_jit_enable sysctl is diabled
From: Yonghong Song @ 2017-06-09 15:39 UTC (permalink / raw)
To: Martin KaFai Lau, netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team
In-Reply-To: <20170609053017.2909855-1-kafai@fb.com>
On 6/8/17 10:30 PM, Martin KaFai Lau wrote:
> test_bpf_obj_id() should not expect a non zero jited_prog_len
> to be returned by bpf_obj_get_info_by_fd() when
> net.core.bpf_jit_enable is 0.
>
> The patch checks for net.core.bpf_jit_enable and
> has different expectation on jited_prog_len.
>
> This patch also removes the pwd.h header which I forgot
> to remove after making changes.
>
> Fixes: 95b9afd3987f ("bpf: Test for bpf ID")
> Reported-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Martin KaFai Lau <kafai@fb.com>
> ---
> tools/testing/selftests/bpf/test_progs.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 8189bfc7e277..fec13ab84fca 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -23,7 +23,7 @@ typedef __u16 __sum16;
> #include <sys/wait.h>
> #include <sys/resource.h>
> #include <sys/types.h>
> -#include <pwd.h>
> +#include <fcntl.h>
>
> #include <linux/bpf.h>
> #include <linux/err.h>
> @@ -297,6 +297,7 @@ static void test_bpf_obj_id(void)
> const __u32 array_key = 0;
> const int nr_iters = 2;
> const char *file = "./test_obj_id.o";
> + const char *jit_sysctl = "/proc/sys/net/core/bpf_jit_enable";
>
> struct bpf_object *objs[nr_iters];
> int prog_fds[nr_iters], map_fds[nr_iters];
> @@ -305,9 +306,18 @@ static void test_bpf_obj_id(void)
> struct bpf_map_info map_infos[nr_iters + 1];
> char jited_insns[128], xlated_insns[128];
> __u32 i, next_id, info_len, nr_id_found, duration = 0;
> - int err = 0;
> + int sysctl_fd, jit_enabled = 0, err = 0;
> __u64 array_value;
>
> + sysctl_fd = open(jit_sysctl, 0, O_RDONLY);
> + if (sysctl_fd != -1) {
> + char tmpc;
> +
> + if (read(sysctl_fd, &tmpc, sizeof(tmpc)) == 1)
> + jit_enabled = (tmpc != '0');
> + close(sysctl_fd);
> + }
> +
> err = bpf_prog_get_fd_by_id(0);
> CHECK(err >= 0 || errno != ENOENT,
> "get-fd-by-notexist-prog-id", "err %d errno %d\n", err, errno);
> @@ -339,13 +349,14 @@ static void test_bpf_obj_id(void)
> if (CHECK(err ||
> prog_infos[i].type != BPF_PROG_TYPE_SOCKET_FILTER ||
> info_len != sizeof(struct bpf_prog_info) ||
> - !prog_infos[i].jited_prog_len ||
> + (jit_enabled && !prog_infos[i].jited_prog_len) ||
> !prog_infos[i].xlated_prog_len,
> "get-prog-info(fd)",
> - "err %d errno %d i %d type %d(%d) info_len %u(%lu) jited_prog_len %u xlated_prog_len %u\n",
> + "err %d errno %d i %d type %d(%d) info_len %u(%lu) jit_enabled %d jited_prog_len %u xlated_prog_len %u\n",
> err, errno, i,
> prog_infos[i].type, BPF_PROG_TYPE_SOCKET_FILTER,
> info_len, sizeof(struct bpf_prog_info),
> + jit_enabled,
> prog_infos[i].jited_prog_len,
> prog_infos[i].xlated_prog_len))
> goto done;
>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* [PATCH] sh_eth: add support to change MTU
From: Niklas Söderlund @ 2017-06-09 15:30 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: netdev, linux-renesas-soc, Niklas Söderlund
The hardware supports the MTU to be changed and the driver it self is
somewhat prepared to support this. This patch hooks up the callbacks to
be able to change the MTU from user-space.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
Based on v4.12-rc1 and tested on Renesas R-Car Koelsch M2.
Test procedure:
1. On host set MTU to something large (9000) was used for this test.
2. On target set MTU to something other then 1500, in this test the max
MTU of 1978 is used.
3. Send ping with large payload and observe that it works.
ping -M do -s 1954 <target>
The reason for 1954 instead of 1982 is two fold:
1. On Linux (different on Mac IIRC) the ICMP/ping implementation
doesn’t encapsulate the 28 byte ICMP (8) + IP (20).
2. The driver internally reserve 4 bytes of transmission buffer for
an optional VLAN header (4). And since no VLAN is used in this
setup the additional 4 bytes can carry data.
4. For extra verification the packet flow is inspected using tcpdump to
verify that there is no packet fragmentation.
drivers/net/ethernet/renesas/sh_eth.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index f68c4db656eda846..da41eda7bfada6b9 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2558,6 +2558,17 @@ static int sh_eth_do_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
return phy_mii_ioctl(phydev, rq, cmd);
}
+static int sh_eth_change_mtu(struct net_device *dev, int new_mtu)
+{
+ if (netif_running(dev))
+ return -EBUSY;
+
+ dev->mtu = new_mtu;
+ netdev_update_features(dev);
+
+ return 0;
+}
+
/* For TSU_POSTn. Please refer to the manual about this (strange) bitfields */
static void *sh_eth_tsu_get_post_reg_offset(struct sh_eth_private *mdp,
int entry)
@@ -3029,6 +3040,7 @@ static const struct net_device_ops sh_eth_netdev_ops = {
.ndo_set_rx_mode = sh_eth_set_rx_mode,
.ndo_tx_timeout = sh_eth_tx_timeout,
.ndo_do_ioctl = sh_eth_do_ioctl,
+ .ndo_change_mtu = sh_eth_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = eth_mac_addr,
};
@@ -3043,6 +3055,7 @@ static const struct net_device_ops sh_eth_netdev_ops_tsu = {
.ndo_vlan_rx_kill_vid = sh_eth_vlan_rx_kill_vid,
.ndo_tx_timeout = sh_eth_tx_timeout,
.ndo_do_ioctl = sh_eth_do_ioctl,
+ .ndo_change_mtu = sh_eth_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = eth_mac_addr,
};
@@ -3171,6 +3184,13 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
}
sh_eth_set_default_cpu_data(mdp->cd);
+ /* Datasheet states max MTU should be 2048 but due to the
+ * aliment calculations in sh_eth_ring_init() the practical
+ * MTU is a bit less. Maybe this can be optimized some more.
+ */
+ ndev->max_mtu = 2000 - (ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN);
+ ndev->min_mtu = ETH_MIN_MTU;
+
/* set function */
if (mdp->cd->tsu)
ndev->netdev_ops = &sh_eth_netdev_ops_tsu;
--
2.13.1
^ permalink raw reply related
* Re: [PATCH net v2] net: ipmr: Fix some mroute forwarding issues in vrf's
From: Donald Sharp @ 2017-06-09 15:20 UTC (permalink / raw)
To: David Ahern
Cc: netdev, Thomas Winter, Nikolay Aleksandrov, Yotam Gigi,
Ido Schimmel, Roopa Prabhu
In-Reply-To: <212ffd99-b2aa-3fad-b042-2b4b0435223d@gmail.com>
I'll change it over to this way. No problem.
donald
On Fri, Jun 9, 2017 at 10:54 AM, David Ahern <dsahern@gmail.com> wrote:
> On 6/9/17 8:22 AM, Donald Sharp wrote:
>> @@ -988,7 +988,16 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt,
>>
>> rtnl_unicast(skb, net, NETLINK_CB(skb).portid);
>> } else {
>> - ip_mr_forward(net, mrt, skb, c, 0);
>> + struct net_device *dev = skb->dev;
>> +
>> + if (netif_is_l3_master(dev)) {
>> + dev = __dev_get_by_index(net, IPCB(skb)->iif);
>> + if (!dev) {
>> + kfree_skb(skb);
>> + continue;
>> + }
>> + }
>> + ip_mr_forward(net, mrt, dev, skb, c, 0);
>> }
>> }
>> }
>
> What about changing ipmr_cache_unresolved to take the dev it looked up
> already and then have ipmr_cache_unresolved reset skb->dev to it (and
> reset skb->skb_iff to dev->ifindex) when queuing to the unresolved list?
> Since this path does not have a local delivery, resetting the skb->dev
> will be fine and it avoids this second lookup using IPCB(skb)->iif:
>
> @@ -1073,7 +1073,7 @@ static int ipmr_cache_report(struct mr_table *mrt,
>
> /* Queue a packet for resolution. It gets locked cache entry! */
> static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
> - struct sk_buff *skb)
> + struct sk_buff *skb, struct net_device
> *dev)
> {
> const struct iphdr *iph = ip_hdr(skb);
> struct mfc_cache *c;
> @@ -1130,6 +1130,10 @@ static int ipmr_cache_unresolved(struct mr_table
> *mrt, vifi_t vifi,
> kfree_skb(skb);
> err = -ENOBUFS;
> } else {
> + if (dev) {
> + skb->dev = dev;
> + skb->skb_iif = dev->ifindex;
> + }
> skb_queue_tail(&c->mfc_un.unres.unresolved, skb);
> err = 0;
> }
>
>
> Combined with Thomas' earlier change this check in ip_mr_forward becomes:
>
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 9374b99c7c17..1393a4d18a9a 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -1853,13 +1853,7 @@ static void ip_mr_forward(struct net *net, struct
> mr_table *mrt,
> }
>
> /* Wrong interface: drop packet and (maybe) send PIM assert. */
> - if (mrt->vif_table[vif].dev != skb->dev) {
> - struct net_device *mdev;
> -
> - mdev = l3mdev_master_dev_rcu(mrt->vif_table[vif].dev);
> - if (mdev == skb->dev)
> - goto forward;
> -
> + if (mrt->vif_table[vif].dev != dev) {
> if (rt_is_output_route(skb_rtable(skb))) {
> /* It is our own packet, looped back.
> * Very complicated situation...
^ permalink raw reply
* Re: [PATCH v2 7/8] net: mvmdio: add xmdio support
From: Andrew Lunn @ 2017-06-09 15:03 UTC (permalink / raw)
To: Antoine Tenart
Cc: Florian Fainelli, davem, jason, gregory.clement,
sebastian.hesselbarth, thomas.petazzoni, mw, linux, netdev,
linux-arm-kernel
In-Reply-To: <20170609145630.GE31464@kwain>
> There are two busses, one generating c22 transactions and one generating
> c45 transactions. Each bus has its own MDC/MDIO pins.
O.K. That is what i wanted to know. So we want two completely separate
device tree bindings, busses registered with Linux, etc.
Thanks for clarification.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown
From: Andrew Lunn @ 2017-06-09 14:59 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: davem, netdev, j.vosburgh, vfalico, andy, thibaut.collet,
linville
In-Reply-To: <fcca67e0-a170-6dd7-dcbb-7a0ecc0453d5@6wind.com>
> > I just wonder if this should be rate limited?
> So using net_warn_ratelimited()?
>
> Displaying this message continuously in the log, even at a low rate, seems not
> the best things to do. The first time seems enough, but it would require more
> code for that. Is it not over-engineering?
Maybe look at the _once functions, dev_warn_once, printk_once.
> The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that.
Me neither.
Andrew
^ permalink raw reply
* Re: [PATCH v2 7/8] net: mvmdio: add xmdio support
From: Antoine Tenart @ 2017-06-09 14:56 UTC (permalink / raw)
To: Andrew Lunn
Cc: thomas.petazzoni, Florian Fainelli, jason, netdev, Antoine Tenart,
linux, gregory.clement, mw, davem, linux-arm-kernel,
sebastian.hesselbarth
In-Reply-To: <20170609144936.GF20756@lunn.ch>
[-- Attachment #1.1: Type: text/plain, Size: 2436 bytes --]
Hi Andrew,
On Fri, Jun 09, 2017 at 04:49:36PM +0200, Andrew Lunn wrote:
> On Fri, Jun 09, 2017 at 04:09:22PM +0200, Antoine Tenart wrote:
> >
> > The MDIO/xMDIO registers are embedded into the network controller. The
> > mvmdio driver was created at first to abstract this functionality
> > outside the network controller driver because it is shared between all
> > ports and used in different IPs. So it's not really devices per say.
> >
> > Looking at the datasheet/schematics there are two hardware buses, one
> > for c22 and one for c45. So we should keep two separate nodes to
> > describe the two interfaces. From what I read c45 is backward
> > compatible with c22 so the xSMI interface should be capable to speak to
> > c22 PHYs as well.
>
> The on the wire protocol of c45 is backwards compatible with c22, in
> that a c22 device will not get confused by a c45 transaction on the
> bus. A c22 device will just ignore it. You cannot talk to a c22 device
> using c45.
I see.
> From what you are saying, you have a hardware block generating c45
> transactions and a hardware block which generates c22 transactions.
> What is not clear to me is if these two hardware blocks are using the
> same physical MDC/MDIO pins, i.e. there is just one MDIO bus to the
> outside world, or are there two busses?
There are two busses, one generating c22 transactions and one generating
c45 transactions. Each bus has its own MDC/MDIO pins.
> > And by looking at the datasheet this seems possible, although it's
> > not completely clear. But anyway this wouldn't impact the dt
> > bindings.
>
> What i'm worried about is there being one set of MDC/MDIO lines. You
> should not expose that to linux as two mdio busses. It is one bus.
>
> The phylib will poll each phy on the bus once per second to check its
> state. The phylib serialises the read/writes at a bus level. So if you
> have one physical bus registered as two logical bus, at some point you
> are going to get simultaneous read/writes, and you are going to need a
> mutex low down to serialise this for physical bus access.
>
> And in the end, this would affect the dt binding. If it is one
> physical bus, you want one binding representing both c22 and c45
> transactions.
Of course. See above.
Thanks!
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net v2] net: ipmr: Fix some mroute forwarding issues in vrf's
From: David Ahern @ 2017-06-09 14:54 UTC (permalink / raw)
To: Donald Sharp, netdev, Thomas.Winter, nikolay, yotamg, idosch,
roopa
In-Reply-To: <20170609142259.24423-1-sharpd@cumulusnetworks.com>
On 6/9/17 8:22 AM, Donald Sharp wrote:
> @@ -988,7 +988,16 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt,
>
> rtnl_unicast(skb, net, NETLINK_CB(skb).portid);
> } else {
> - ip_mr_forward(net, mrt, skb, c, 0);
> + struct net_device *dev = skb->dev;
> +
> + if (netif_is_l3_master(dev)) {
> + dev = __dev_get_by_index(net, IPCB(skb)->iif);
> + if (!dev) {
> + kfree_skb(skb);
> + continue;
> + }
> + }
> + ip_mr_forward(net, mrt, dev, skb, c, 0);
> }
> }
> }
What about changing ipmr_cache_unresolved to take the dev it looked up
already and then have ipmr_cache_unresolved reset skb->dev to it (and
reset skb->skb_iff to dev->ifindex) when queuing to the unresolved list?
Since this path does not have a local delivery, resetting the skb->dev
will be fine and it avoids this second lookup using IPCB(skb)->iif:
@@ -1073,7 +1073,7 @@ static int ipmr_cache_report(struct mr_table *mrt,
/* Queue a packet for resolution. It gets locked cache entry! */
static int ipmr_cache_unresolved(struct mr_table *mrt, vifi_t vifi,
- struct sk_buff *skb)
+ struct sk_buff *skb, struct net_device
*dev)
{
const struct iphdr *iph = ip_hdr(skb);
struct mfc_cache *c;
@@ -1130,6 +1130,10 @@ static int ipmr_cache_unresolved(struct mr_table
*mrt, vifi_t vifi,
kfree_skb(skb);
err = -ENOBUFS;
} else {
+ if (dev) {
+ skb->dev = dev;
+ skb->skb_iif = dev->ifindex;
+ }
skb_queue_tail(&c->mfc_un.unres.unresolved, skb);
err = 0;
}
Combined with Thomas' earlier change this check in ip_mr_forward becomes:
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 9374b99c7c17..1393a4d18a9a 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1853,13 +1853,7 @@ static void ip_mr_forward(struct net *net, struct
mr_table *mrt,
}
/* Wrong interface: drop packet and (maybe) send PIM assert. */
- if (mrt->vif_table[vif].dev != skb->dev) {
- struct net_device *mdev;
-
- mdev = l3mdev_master_dev_rcu(mrt->vif_table[vif].dev);
- if (mdev == skb->dev)
- goto forward;
-
+ if (mrt->vif_table[vif].dev != dev) {
if (rt_is_output_route(skb_rtable(skb))) {
/* It is our own packet, looped back.
* Very complicated situation...
^ permalink raw reply related
* Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown
From: Andy Gospodarek @ 2017-06-09 14:51 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: Andrew Lunn, davem, netdev, j.vosburgh, vfalico, thibaut.collet,
linville
In-Reply-To: <fcca67e0-a170-6dd7-dcbb-7a0ecc0453d5@6wind.com>
On Fri, Jun 09, 2017 at 04:39:06PM +0200, Nicolas Dichtel wrote:
> Le 09/06/2017 à 16:23, Andrew Lunn a écrit :
> > On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote:
> >> Make it explicit in the log.
> >>
> >> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> >> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
> >
> > Hi Nicolas
> Hi Andrew,
>
> >
> > How often is this called? It seems like it is used by
> Good question! ;-)
>
> > ad_port_selection_logic, which has the comment:
> >
> > is called in the inititalization (after all the handshkes),
> > and after every lacpdu receive (if selected is off)
> >
> > I just wonder if this should be rate limited?
> So using net_warn_ratelimited()?
Why not just use pr_warn_once()?
It may go unnoticed at first, but that seems like a decent option compared to
spamming log files.
> Displaying this message continuously in the log, even at a low rate, seems not
> the best things to do. The first time seems enough, but it would require more
> code for that. Is it not over-engineering?
> The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that.
>
^ permalink raw reply
* Re: [PATCH v2 7/8] net: mvmdio: add xmdio support
From: Andrew Lunn @ 2017-06-09 14:49 UTC (permalink / raw)
To: Antoine Tenart
Cc: thomas.petazzoni, Florian Fainelli, jason, netdev, linux,
gregory.clement, mw, davem, linux-arm-kernel,
sebastian.hesselbarth
In-Reply-To: <20170609140922.GD31464@kwain>
On Fri, Jun 09, 2017 at 04:09:22PM +0200, Antoine Tenart wrote:
> Hi Andrew,
>
> On Fri, Jun 09, 2017 at 03:26:24PM +0200, Andrew Lunn wrote:
> > On Fri, Jun 09, 2017 at 10:25:41AM +0200, Antoine Tenart wrote:
> > > On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> > > >
> > > > If I get this right, the xMDIO controller is actually a superset of the
> > > > MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> > > > preform C45 accesses?
> > >
> > > This is also a mistake. It's not a superset as the register offsets are
> > > different. So we can't use the same smi operations in both cases. We
> > > would need dedicated xmdio smi operations, but I don't think there is a
> > > board where a c22 PHY is connected to the xMDIO interface.
> >
> > Is it described as one device, which can do c22 via one set of
> > registers, and c45 by another set of registers?
> >
> > Or are there two separate devices. In particular, does each device
> > have there own MDC and MDIO pins? Do we have an C22 only device/bus,
> > and a C45 only device/bus?
>
> The MDIO/xMDIO registers are embedded into the network controller. The
> mvmdio driver was created at first to abstract this functionality
> outside the network controller driver because it is shared between all
> ports and used in different IPs. So it's not really devices per say.
>
> Looking at the datasheet/schematics there are two hardware buses, one
> for c22 and one for c45. So we should keep two separate nodes to
> describe the two interfaces. From what I read c45 is backward
> compatible with c22 so the xSMI interface should be capable to speak to
> c22 PHYs as well.
The on the wire protocol of c45 is backwards compatible with c22, in
that a c22 device will not get confused by a c45 transaction on the
bus. A c22 device will just ignore it. You cannot talk to a c22 device
using c45.
>From what you are saying, you have a hardware block generating c45
transactions and a hardware block which generates c22 transactions.
What is not clear to me is if these two hardware blocks are using the
same physical MDC/MDIO pins, i.e. there is just one MDIO bus to the
outside world, or are there two busses?
> And by looking at the datasheet this seems possible, although it's
> not completely clear. But anyway this wouldn't impact the dt
> bindings.
What i'm worried about is there being one set of MDC/MDIO lines. You
should not expose that to linux as two mdio busses. It is one bus.
The phylib will poll each phy on the bus once per second to check its
state. The phylib serialises the read/writes at a bus level. So if you
have one physical bus registered as two logical bus, at some point you
are going to get simultaneous read/writes, and you are going to need a
mutex low down to serialise this for physical bus access.
And in the end, this would affect the dt binding. If it is one
physical bus, you want one binding representing both c22 and c45
transactions.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown
From: Nicolas Dichtel @ 2017-06-09 14:39 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, netdev, j.vosburgh, vfalico, andy, thibaut.collet,
linville
In-Reply-To: <20170609142359.GD20756@lunn.ch>
Le 09/06/2017 à 16:23, Andrew Lunn a écrit :
> On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote:
>> Make it explicit in the log.
>>
>> Suggested-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>
> Hi Nicolas
Hi Andrew,
>
> How often is this called? It seems like it is used by
Good question! ;-)
> ad_port_selection_logic, which has the comment:
>
> is called in the inititalization (after all the handshkes),
> and after every lacpdu receive (if selected is off)
>
> I just wonder if this should be rate limited?
So using net_warn_ratelimited()?
Displaying this message continuously in the log, even at a low rate, seems not
the best things to do. The first time seems enough, but it would require more
code for that. Is it not over-engineering?
The ideal solution would be a BUILD_BUG_ON(), but I don't see how to make that.
Regards,
Nicolas
^ permalink raw reply
* Re: [PATCH] net: Fix inconsistent teardown and release of private netdev state.
From: Johannes Berg @ 2017-06-09 14:33 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <20170609.101733.1675759409997288763.davem@davemloft.net>
On Fri, 2017-06-09 at 10:17 -0400, David Miller wrote:
>
> > I guess this must mean that that all others are dealing with the
> > problem "manually", right? Perhaps needs_free_netdev isn't all that
> > necessary then?
>
> Yeah, the major two modes of operation are manual freeing of the
> netdev (usually at module unload time or similar) or via the
> destructor mechanisms.
Ok, thanks.
> > I think you introduced a bug though - isn't this needed?
> >
> > --- a/net/mac80211/iface.c
> > +++ b/net/mac80211/iface.c
> > @@ -1816,6 +1816,7 @@ int ieee80211_if_add(struct ieee80211_local
> *local, const char *name,
> > ret = dev_alloc_name(ndev, ndev->name);
> > if (ret < 0) {
> > ieee80211_if_free(ndev);
> > + free_netdev(ndev);
> > return ret;
> > }
> >
> > There was another caller of ieee80211_if_free() which you modified,
> and
> > thus needs the free_netdev() that you removed from it.
>
> I think you are right. The pattern is that when something fails
> after allocating the netdev, but before registering it, that code
> must free the netdev itself.
Right. Do you want me to put that into my tree? I could do it tonight,
or perhaps only Monday though.
> > Would an alternative have been to not use (priv_)destructor here,
> and
> > just free all the data after unregister_netdevice(_many)? This sets
> the
> > reg_state to NETREG_UNREGISTERING, so sysfs can no longer look at
> the
> > stats afterwards, and it's been unlisted (unlist_netdevice) so
> can't be
> > reached through any other means either.
> >
> > IOW, this would also work and fix the bug above along the way?
> >
> > diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> > index 915d7e1b4545..23df973d5181 100644
> > --- a/net/mac80211/iface.c
> > +++ b/net/mac80211/iface.c
> ...
> > @@ -1932,6 +1931,7 @@ void ieee80211_if_remove(struct
> ieee80211_sub_if_data *sdata)
> >
> > if (sdata->dev) {
> > unregister_netdevice(sdata->dev);
> > + ieee80211_if_free(sdata->dev);
> > } else {
> > cfg80211_unregister_wdev(&sdata->wdev);
> > ieee80211_teardown_sdata(sdata);
>
> Unless I misunderstood something, unregister_netdevice() here will
> invoke the ->priv_destructor() and thus now it will be invoked twice?
I think you missed that I removed priv_destructor?
johannes
^ permalink raw reply
* [PATCH] l2tp: cast l2tp traffic counter to unsigned
From: Dominik Heidler @ 2017-06-09 14:29 UTC (permalink / raw)
To: netdev, linux-kernel; +Cc: Dominik Heidler, Tom Parkin
This fixes a counter problem on 32bit systems:
When the rx_bytes counter reached 2 GiB, it jumpd to (2^64 Bytes - 2GiB) Bytes.
rtnl_link_stats64 has __u64 type and atomic_long_read returns
atomic_long_t which is signed. Due to the conversation
we get an incorrect value on 32bit systems if the MSB of
the atomic_long_t value is set.
CC: Tom Parkin <tparkin@katalix.com>
Fixes: 7b7c0719cd7a ("l2tp: avoid deadlock in l2tp stats update")
Signed-off-by: Dominik Heidler <dheidler@suse.de>
---
net/l2tp/l2tp_eth.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/net/l2tp/l2tp_eth.c b/net/l2tp/l2tp_eth.c
index 8b21af7321b9..668a75e002e9 100644
--- a/net/l2tp/l2tp_eth.c
+++ b/net/l2tp/l2tp_eth.c
@@ -114,12 +114,13 @@ static void l2tp_eth_get_stats64(struct net_device *dev,
{
struct l2tp_eth *priv = netdev_priv(dev);
- stats->tx_bytes = atomic_long_read(&priv->tx_bytes);
- stats->tx_packets = atomic_long_read(&priv->tx_packets);
- stats->tx_dropped = atomic_long_read(&priv->tx_dropped);
- stats->rx_bytes = atomic_long_read(&priv->rx_bytes);
- stats->rx_packets = atomic_long_read(&priv->rx_packets);
- stats->rx_errors = atomic_long_read(&priv->rx_errors);
+ stats->tx_bytes = (unsigned long) atomic_long_read(&priv->tx_bytes);
+ stats->tx_packets = (unsigned long) atomic_long_read(&priv->tx_packets);
+ stats->tx_dropped = (unsigned long) atomic_long_read(&priv->tx_dropped);
+ stats->rx_bytes = (unsigned long) atomic_long_read(&priv->rx_bytes);
+ stats->rx_packets = (unsigned long) atomic_long_read(&priv->rx_packets);
+ stats->rx_errors = (unsigned long) atomic_long_read(&priv->rx_errors);
+
}
static const struct net_device_ops l2tp_eth_netdev_ops = {
--
2.13.0
^ permalink raw reply related
* Re: [PATCH net v2] net: ipmr: Fix some mroute forwarding issues in vrf's
From: Andrew Lunn @ 2017-06-09 14:28 UTC (permalink / raw)
To: Donald Sharp
Cc: netdev, dsahern, Thomas.Winter, nikolay, yotamg, idosch, roopa
In-Reply-To: <20170609142259.24423-1-sharpd@cumulusnetworks.com>
> static void ip_mr_forward(struct net *net, struct mr_table *mrt,
> - struct sk_buff *skb, struct mfc_cache *cache,
> - int local);
> + struct net_device *dev, struct sk_buff *skb,
> + struct mfc_cache *cache, int local);
You appear to have spaces here, not tabs.
Andrew
^ permalink raw reply
* Re: [PATCH net-next] bonding: warn user when 802.3ad speed is unknown
From: Andrew Lunn @ 2017-06-09 14:23 UTC (permalink / raw)
To: Nicolas Dichtel
Cc: davem, netdev, j.vosburgh, vfalico, andy, thibaut.collet,
linville
In-Reply-To: <1497015283-1729-1-git-send-email-nicolas.dichtel@6wind.com>
On Fri, Jun 09, 2017 at 03:34:43PM +0200, Nicolas Dichtel wrote:
> Make it explicit in the log.
>
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Hi Nicolas
How often is this called? It seems like it is used by
ad_port_selection_logic, which has the comment:
is called in the inititalization (after all the handshkes),
and after every lacpdu receive (if selected is off)
I just wonder if this should be rate limited?
Andrew
^ permalink raw reply
* [PATCH net v2] net: ipmr: Fix some mroute forwarding issues in vrf's
From: Donald Sharp @ 2017-06-09 14:22 UTC (permalink / raw)
To: netdev, dsahern, Thomas.Winter, nikolay, yotamg, idosch, roopa
This patch fixes two issues:
1) When forwarding on *,G mroutes that are in a vrf, the
kernel was dropping information about the actual incoming
interface when calling ip_mr_forward from ip_mr_input.
This caused ip_mr_forward to send the multicast packet
back out the incoming interface. Fix this by
modifying ip_mr_forward to be handed the correctly
resolved dev.
2) When a unresolved cache entry is created we store
the incoming skb on the unresolved cache entry and
upon mroute resolution from the user space daemon,
we attempt to forward the packet. Again we were
not resolving to the correct incoming device for
a vrf scenario, before calling ip_mr_forward.
Fix this by resolving to the correct interface
and calling ip_mr_forward with the result.
Signed-off-by: Donald Sharp <sharpd@cumulusnetworks.com>
---
v2: Fixed title
net/ipv4/ipmr.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 551de4d..559009e 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -101,8 +101,8 @@ static struct mr_table *ipmr_new_table(struct net *net, u32 id);
static void ipmr_free_table(struct mr_table *mrt);
static void ip_mr_forward(struct net *net, struct mr_table *mrt,
- struct sk_buff *skb, struct mfc_cache *cache,
- int local);
+ struct net_device *dev, struct sk_buff *skb,
+ struct mfc_cache *cache, int local);
static int ipmr_cache_report(struct mr_table *mrt,
struct sk_buff *pkt, vifi_t vifi, int assert);
static int __ipmr_fill_mroute(struct mr_table *mrt, struct sk_buff *skb,
@@ -988,7 +988,16 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt,
rtnl_unicast(skb, net, NETLINK_CB(skb).portid);
} else {
- ip_mr_forward(net, mrt, skb, c, 0);
+ struct net_device *dev = skb->dev;
+
+ if (netif_is_l3_master(dev)) {
+ dev = __dev_get_by_index(net, IPCB(skb)->iif);
+ if (!dev) {
+ kfree_skb(skb);
+ continue;
+ }
+ }
+ ip_mr_forward(net, mrt, dev, skb, c, 0);
}
}
}
@@ -1828,10 +1837,10 @@ static int ipmr_find_vif(struct mr_table *mrt, struct net_device *dev)
/* "local" means that we should preserve one skb (for local delivery) */
static void ip_mr_forward(struct net *net, struct mr_table *mrt,
- struct sk_buff *skb, struct mfc_cache *cache,
- int local)
+ struct net_device *dev, struct sk_buff *skb,
+ struct mfc_cache *cache, int local)
{
- int true_vifi = ipmr_find_vif(mrt, skb->dev);
+ int true_vifi = ipmr_find_vif(mrt, dev);
int psend = -1;
int vif, ct;
@@ -1853,11 +1862,11 @@ static void ip_mr_forward(struct net *net, struct mr_table *mrt,
}
/* Wrong interface: drop packet and (maybe) send PIM assert. */
- if (mrt->vif_table[vif].dev != skb->dev) {
+ if (mrt->vif_table[vif].dev != dev) {
struct net_device *mdev;
mdev = l3mdev_master_dev_rcu(mrt->vif_table[vif].dev);
- if (mdev == skb->dev)
+ if (mdev == dev)
goto forward;
if (rt_is_output_route(skb_rtable(skb))) {
@@ -2064,7 +2073,7 @@ int ip_mr_input(struct sk_buff *skb)
}
read_lock(&mrt_lock);
- ip_mr_forward(net, mrt, skb, cache, local);
+ ip_mr_forward(net, mrt, dev, skb, cache, local);
read_unlock(&mrt_lock);
if (local)
--
2.9.4
^ permalink raw reply related
* [PATCH net 2/2] bnx2x: Don't post statistics to malicious VFs
From: Yuval Mintz @ 2017-06-09 14:17 UTC (permalink / raw)
To: davem, netdev; +Cc: mschmidt, Yuval Mintz
In-Reply-To: <20170609141702.20688-1-Yuval.Mintz@cavium.com>
Once firmware indicates that a given VF is malicious and until
that VF passes an FLR all bets are off - PF can't know anything
is happening to the VF [since VF can't communicate anything to its PF].
But PF is currently still periodically asking device to collect
statistics for the VF which might in turn fill logs by IOMMU blocking
memory access done by the VF's PCI function [in the case VF has unmapped
its buffers].
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 13 ++++++++++++-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h | 1 +
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
index 870ea00..9ca994d 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
@@ -901,6 +901,8 @@ static void bnx2x_vf_flr(struct bnx2x *bp, struct bnx2x_virtf *vf)
/* release VF resources */
bnx2x_vf_free_resc(bp, vf);
+ vf->malicious = false;
+
/* re-open the mailbox */
bnx2x_vf_enable_mbx(bp, vf->abs_vfid);
return;
@@ -1822,9 +1824,11 @@ int bnx2x_iov_eq_sp_event(struct bnx2x *bp, union event_ring_elem *elem)
vf->abs_vfid, qidx);
bnx2x_vf_handle_rss_update_eqe(bp, vf);
case EVENT_RING_OPCODE_VF_FLR:
- case EVENT_RING_OPCODE_MALICIOUS_VF:
/* Do nothing for now */
return 0;
+ case EVENT_RING_OPCODE_MALICIOUS_VF:
+ vf->malicious = true;
+ return 0;
}
return 0;
@@ -1905,6 +1909,13 @@ void bnx2x_iov_adjust_stats_req(struct bnx2x *bp)
continue;
}
+ if (vf->malicious) {
+ DP_AND((BNX2X_MSG_IOV | BNX2X_MSG_STATS),
+ "vf %d malicious so no stats for it\n",
+ vf->abs_vfid);
+ continue;
+ }
+
DP_AND((BNX2X_MSG_IOV | BNX2X_MSG_STATS),
"add addresses for vf %d\n", vf->abs_vfid);
for_each_vfq(vf, j) {
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h
index 888d0b6..53466f6 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h
@@ -141,6 +141,7 @@ struct bnx2x_virtf {
#define VF_RESET 3 /* VF FLR'd, pending cleanup */
bool flr_clnup_stage; /* true during flr cleanup */
+ bool malicious; /* true if FW indicated so, until FLR */
/* dma */
dma_addr_t fw_stat_map;
--
2.9.4
^ permalink raw reply related
* Re: [PATCH] net: Fix inconsistent teardown and release of private netdev state.
From: David Miller @ 2017-06-09 14:17 UTC (permalink / raw)
To: johannes; +Cc: netdev
In-Reply-To: <1497012325.2424.12.camel@sipsolutions.net>
From: Johannes Berg <johannes@sipsolutions.net>
Date: Fri, 09 Jun 2017 14:45:25 +0200
> Under drivers/ in general, I count more than 400 calls to
> register_netdev[ice](), but only 39 that set the new needs_free_netdev.
> Some will overlap and have the same ops, but still, that's a rather
> small portion of them. The logic means those that don't set
> needs_free_netdev can't really use the new priv_destructor (previously
> destructor), I think? It seems to me that priv_destructor should imply
> needs_free_netdev (though not necessarily the other way around).
>
> I guess this must mean that that all others are dealing with the
> problem "manually", right? Perhaps needs_free_netdev isn't all that
> necessary then?
Yeah, the major two modes of operation are manual freeing of the
netdev (usually at module unload time or similar) or via the
destructor mechanisms.
> I think you introduced a bug though - isn't this needed?
>
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -1816,6 +1816,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
> ret = dev_alloc_name(ndev, ndev->name);
> if (ret < 0) {
> ieee80211_if_free(ndev);
> + free_netdev(ndev);
> return ret;
> }
>
> There was another caller of ieee80211_if_free() which you modified, and
> thus needs the free_netdev() that you removed from it.
I think you are right. The pattern is that when something fails
after allocating the netdev, but before registering it, that code
must free the netdev itself.
> Would an alternative have been to not use (priv_)destructor here, and
> just free all the data after unregister_netdevice(_many)? This sets the
> reg_state to NETREG_UNREGISTERING, so sysfs can no longer look at the
> stats afterwards, and it's been unlisted (unlist_netdevice) so can't be
> reached through any other means either.
>
> IOW, this would also work and fix the bug above along the way?
>
> diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
> index 915d7e1b4545..23df973d5181 100644
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
...
> @@ -1932,6 +1931,7 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
>
> if (sdata->dev) {
> unregister_netdevice(sdata->dev);
> + ieee80211_if_free(sdata->dev);
> } else {
> cfg80211_unregister_wdev(&sdata->wdev);
> ieee80211_teardown_sdata(sdata);
Unless I misunderstood something, unregister_netdevice() here will
invoke the ->priv_destructor() and thus now it will be invoked twice?
^ permalink raw reply
* [PATCH net 1/2] bnx2x: Allow vfs to disable txvlan offload
From: Yuval Mintz @ 2017-06-09 14:17 UTC (permalink / raw)
To: davem, netdev; +Cc: mschmidt, Yuval Mintz
In-Reply-To: <20170609141702.20688-1-Yuval.Mintz@cavium.com>
VF clients are configured as enforced, meaning firmware is validating
the correctness of their ethertype/vid during transmission.
Once txvlan is disabled, VF would start getting SKBs for transmission
here vlan is on the payload - but it'll pass the packet's ethertype
instead of the vid, leading to firmware declaring it as malicious.
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 5f49334..f619c4c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -3883,15 +3883,26 @@ netdev_tx_t bnx2x_start_xmit(struct sk_buff *skb, struct net_device *dev)
/* when transmitting in a vf, start bd must hold the ethertype
* for fw to enforce it
*/
+ u16 vlan_tci = 0;
#ifndef BNX2X_STOP_ON_ERROR
- if (IS_VF(bp))
+ if (IS_VF(bp)) {
#endif
- tx_start_bd->vlan_or_ethertype =
- cpu_to_le16(ntohs(eth->h_proto));
+ /* Still need to consider inband vlan for enforced */
+ if (__vlan_get_tag(skb, &vlan_tci)) {
+ tx_start_bd->vlan_or_ethertype =
+ cpu_to_le16(ntohs(eth->h_proto));
+ } else {
+ tx_start_bd->bd_flags.as_bitfield |=
+ (X_ETH_INBAND_VLAN <<
+ ETH_TX_BD_FLAGS_VLAN_MODE_SHIFT);
+ tx_start_bd->vlan_or_ethertype =
+ cpu_to_le16(vlan_tci);
+ }
#ifndef BNX2X_STOP_ON_ERROR
- else
+ } else {
/* used by FW for packet accounting */
tx_start_bd->vlan_or_ethertype = cpu_to_le16(pkt_prod);
+ }
#endif
}
--
2.9.4
^ permalink raw reply related
* [PATCH net 0/2] bnx2x: Fix malicious VFs indication
From: Yuval Mintz @ 2017-06-09 14:17 UTC (permalink / raw)
To: davem, netdev; +Cc: mschmidt, Yuval Mintz
It was discovered that for a VF there's a simple [yet uncommon] scenario
which would cause device firmware to declare that VF as malicious -
Add a vlan interface on top of a VF and disable txvlan offloading for
that VF [causing VF to transmit packets where vlan is on payload].
Patch #1 corrects driver transmission to prevent this issue.
Patch #2 is a by-product correcting PF behavior once a VF is declared
malicious.
Dave,
Please consider applying these to 'net'.
Thanks,
Yuval
Yuval Mintz (2):
bnx2x: Allow vfs to disable txvlan offload
bnx2x: Don't post statistics to malicious VFs
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 19 +++++++++++++++----
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 13 ++++++++++++-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h | 1 +
3 files changed, 28 insertions(+), 5 deletions(-)
--
2.9.4
^ permalink raw reply
* [PATCH v2 net-next 7/8] qed: Call rx_release_cb() when flushing LL2
From: Yuval Mintz @ 2017-06-09 14:13 UTC (permalink / raw)
To: netdev, davem; +Cc: linux-rdma, Michal.Kalderon, Yuval Mintz
In-Reply-To: <20170609141325.20547-1-Yuval.Mintz@cavium.com>
Driver to inform the connection owner that the its buffers are being
released as part of a flush.
Signed-off-by: Yuval Mintz <Yuval.Mintz@cavium.com>
---
drivers/net/ethernet/qlogic/qed/qed_ll2.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_ll2.c b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
index 2a68fb9..c6172a77e 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_ll2.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_ll2.c
@@ -525,10 +525,6 @@ static void qed_ll2_rxq_flush(struct qed_hwfn *p_hwfn, u8 connection_handle)
p_rx = &p_ll2_conn->rx_queue;
while (!list_empty(&p_rx->active_descq)) {
- dma_addr_t rx_buf_addr;
- void *cookie;
- bool b_last;
-
p_pkt = list_first_entry(&p_rx->active_descq,
struct qed_ll2_rx_packet, list_entry);
if (!p_pkt)
@@ -543,10 +539,15 @@ static void qed_ll2_rxq_flush(struct qed_hwfn *p_hwfn, u8 connection_handle)
qed_ooo_put_free_buffer(p_hwfn, p_hwfn->p_ooo_info,
p_buffer);
} else {
- rx_buf_addr = p_pkt->rx_buf_addr;
- cookie = p_pkt->cookie;
+ dma_addr_t rx_buf_addr = p_pkt->rx_buf_addr;
+ void *cookie = p_pkt->cookie;
+ bool b_last;
b_last = list_empty(&p_rx->active_descq);
+ p_ll2_conn->cbs.rx_release_cb(p_ll2_conn->cbs.cookie,
+ p_ll2_conn->my_id,
+ cookie,
+ rx_buf_addr, b_last);
}
}
}
--
2.9.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox