* Re: [PATCH v4 net-next 0/4] net: ethernet: ti: cpsw: Add XDP support
From: David Miller @ 2019-06-25 20:46 UTC (permalink / raw)
To: ivan.khoronzhuk
Cc: grygorii.strashko, hawk, brouer, saeedm, leon, ast, linux-kernel,
linux-omap, xdp-newbies, ilias.apalodimas, netdev, daniel,
jakub.kicinski, john.fastabend
In-Reply-To: <20190625175948.24771-1-ivan.khoronzhuk@linaro.org>
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Tue, 25 Jun 2019 20:59:44 +0300
> This patchset adds XDP support for TI cpsw driver and base it on
> page_pool allocator. It was verified on af_xdp socket drop,
> af_xdp l2f, ebpf XDP_DROP, XDP_REDIRECT, XDP_PASS, XDP_TX.
So happy to watch the progress of this patch set.
Jesper and others, please review.
Thank you.
^ permalink raw reply
* Re: [net-next 1/1] tipc: rename function msg_get_wrapped() to msg_inner_hdr()
From: David Miller @ 2019-06-25 20:43 UTC (permalink / raw)
To: jon.maloy
Cc: netdev, gordan.mihaljevic, tung.q.nguyen, hoang.h.le, canh.d.luu,
ying.xue, tipc-discussion
In-Reply-To: <1561484220-22814-1-git-send-email-jon.maloy@ericsson.com>
From: Jon Maloy <jon.maloy@ericsson.com>
Date: Tue, 25 Jun 2019 19:37:00 +0200
> We rename the inline function msg_get_wrapped() to the more
> comprehensible msg_inner_hdr().
>
> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Applied, thanks Jon.
^ permalink raw reply
* Re: [PATCH net-next] Revert "net: ena: ethtool: add extra properties retrieval via get_priv_flags"
From: David Miller @ 2019-06-25 20:43 UTC (permalink / raw)
To: jakub.kicinski
Cc: netdev, oss-drivers, nafea, dwmw2, sameehj, zorik, saeedb,
netanel
In-Reply-To: <20190625165956.19278-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue, 25 Jun 2019 09:59:56 -0700
> This reverts commit 315c28d2b714 ("net: ena: ethtool: add extra properties retrieval via get_priv_flags").
>
> As discussed at netconf and on the mailing list we can't allow
> for the the abuse of private flags for exposing arbitrary device
> labels.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Amazon folks I think this is entirely reasonable, please ACK.
Thanks.
^ permalink raw reply
* Re: [PATCH net] vrf: reset rt_iif for recirculated mcast out pkts
From: Stephen Suryaputra @ 2019-06-25 20:42 UTC (permalink / raw)
To: David Ahern; +Cc: netdev
In-Reply-To: <f0a47b5d-6477-9a6a-cf5d-6e13f0b4acdc@gmail.com>
On Tue, Jun 25, 2019 at 4:22 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 6/25/19 4:33 AM, Stephen Suryaputra wrote:
> > @@ -363,10 +376,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
> > #endif
> > ) {
> > struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
> > - if (newskb)
> > + if (newskb) {
> > + /* Reset rt_iif so that inet_iif() will return
> > + * skb->dev->ifIndex which is the VRF device for
> > + * socket lookup. Setting this to VRF ifindex
> > + * causes ipi_ifindex in in_pktinfo to be
> > + * overwritten, see ipv4_pktinfo_prepare().
> > + */
> > + if (netif_is_l3_slave(dev))
>
> seems like the rt_iif is a problem for recirculated mcast packets in
> general, not just ones tied to a VRF.
It seems so to me too but I wonder why this hasn't been seen...
Can I get more feedbacks on this? If there is an agreement to fix this
generally, I will remove the if clause and respin the patch with more
accurate changelog.
>
> > + ip_mc_reset_rt_iif(net, rt, newskb);
> > +
> > NF_HOOK(NFPROTO_IPV4, NF_INET_POST_ROUTING,
> > net, sk, newskb, NULL, newskb->dev,
> > ip_mc_finish_output);
> > + }
> > }
> >
> > /* Multicasts with ttl 0 must not go beyond the host */
Thanks.
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Andrew Lunn @ 2019-06-25 20:41 UTC (permalink / raw)
To: Daniel Santos
Cc: Russell King - ARM Linux admin, René van Dorst, sean.wang,
f.fainelli, davem, matthias.bgg, vivien.didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <4fc51dc4-0eec-30d7-86d1-3404819cf6fe@pobox.com>
On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
> On 6/25/19 2:02 PM, Andrew Lunn wrote:
> >> But will there still be a mechanism to ignore link partner's advertising
> >> and force these parameters?
> > >From man 1 ethtool:
> >
> > -a --show-pause
> > Queries the specified Ethernet device for pause parameter information.
> >
> > -A --pause
> > Changes the pause parameters of the specified Ethernet device.
> >
> > autoneg on|off
> > Specifies whether pause autonegotiation should be enabled.
> >
> > rx on|off
> > Specifies whether RX pause should be enabled.
> >
> > tx on|off
> > Specifies whether TX pause should be enabled.
> >
> > You need to check the driver to see if it actually implements this
> > ethtool call, but that is how it should be configured.
> >
> > Andrew
> >
> Thank you Andrew,
>
> So in this context, my question is the difference between "enabling" and
> "forcing". Here's that register for the mt7620 (which has an mt7530 on
> its die): https://imgur.com/a/pTk0668 I believe this is also what René
> is seeking clarity on?
Lets start with normal operation. If the MAC supports pause or asym
pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
phylib will then configure the PHY to advertise pause as appropriate.
Once auto-neg has completed, the results of the negotiation are set in
phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
then used to tell the MAC about the autoneg results. The MAC should be
programmed using the values in phdev->pause and phydev->asym_pause.
For ethtool, the MAC driver needs to implement .get_pauseparam and
.set_pauseparam. The set_pauseparam needs to validate the settings,
using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
tell the PHY about the new configuration. This will trigger a new
auto-neg if auto-neg is enabled, and the results will be passed back
in the usual way. If auto-neg is disabled, or pause auto-neg is
disabled, the MAC should configure pause directly based on the
settings passed.
Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
want the MAC directly talking to the PHY. Bad things will happen.
Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
phydev->asym_pause.
The same idea applies when using phylink.
Andrew
^ permalink raw reply
* Re: [net-next 1/1] tipc: eliminate unnecessary skb expansion during retransmission
From: David Miller @ 2019-06-25 20:40 UTC (permalink / raw)
To: jon.maloy
Cc: netdev, gordan.mihaljevic, tung.q.nguyen, hoang.h.le, canh.d.luu,
ying.xue, tipc-discussion
In-Reply-To: <1561478893-31371-1-git-send-email-jon.maloy@ericsson.com>
From: Jon Maloy <jon.maloy@ericsson.com>
Date: Tue, 25 Jun 2019 18:08:13 +0200
> We increase the allocated headroom for the buffer copies to be
> retransmitted. This eliminates the need for the lower stack levels
> (UDP/IP/L2) to expand the headroom in order to add their own headers.
>
> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Applied.
^ permalink raw reply
* Re: [PATCH net] vrf: reset rt_iif for recirculated mcast out pkts
From: David Ahern @ 2019-06-25 20:36 UTC (permalink / raw)
To: Stephen Suryaputra, netdev
In-Reply-To: <f0a47b5d-6477-9a6a-cf5d-6e13f0b4acdc@gmail.com>
On 6/25/19 2:22 PM, David Ahern wrote:
> On 6/25/19 4:33 AM, Stephen Suryaputra wrote:
>> @@ -363,10 +376,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>> #endif
>> ) {
>> struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
>> - if (newskb)
>> + if (newskb) {
>> + /* Reset rt_iif so that inet_iif() will return
>> + * skb->dev->ifIndex which is the VRF device for
>> + * socket lookup. Setting this to VRF ifindex
>> + * causes ipi_ifindex in in_pktinfo to be
>> + * overwritten, see ipv4_pktinfo_prepare().
>> + */
>> + if (netif_is_l3_slave(dev))
>
> seems like the rt_iif is a problem for recirculated mcast packets in
> general, not just ones tied to a VRF.
>
>> + ip_mc_reset_rt_iif(net, rt, newskb);
>> +
>> NF_HOOK(NFPROTO_IPV4, NF_INET_POST_ROUTING,
>> net, sk, newskb, NULL, newskb->dev,
>> ip_mc_finish_output);
>> + }
>> }
>>
>> /* Multicasts with ttl 0 must not go beyond the host */
Also, wouldn't this problem apply to broadcast packets as well if they
get recirculated back up the stack? Maybe then the reset_rt_iif needs to
be done in ip_mc_finish_output.
^ permalink raw reply
* Re: [net-next 1/1] tipc: simplify stale link failure criteria
From: David Miller @ 2019-06-25 20:34 UTC (permalink / raw)
To: jon.maloy
Cc: netdev, gordan.mihaljevic, tung.q.nguyen, hoang.h.le, canh.d.luu,
ying.xue, tipc-discussion
In-Reply-To: <1561477003-25362-1-git-send-email-jon.maloy@ericsson.com>
From: Jon Maloy <jon.maloy@ericsson.com>
Date: Tue, 25 Jun 2019 17:36:43 +0200
> In commit a4dc70d46cf1 ("tipc: extend link reset criteria for stale
> packet retransmission") we made link retransmission failure events
> dependent on the link tolerance, and not only of the number of failed
> retransmission attempts, as we did earlier. This works well. However,
> keeping the original, additional criteria of 99 failed retransmissions
> is now redundant, and may in some cases lead to failure detection
> times in the order of minutes instead of the expected 1.5 sec link
> tolerance value.
>
> We now remove this criteria altogether.
>
> Acked-by: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/1] mlx5: Fix build when CONFIG_MLX5_EN_RXNFC is disabled
From: David Miller @ 2019-06-25 20:34 UTC (permalink / raw)
To: jes.sorensen; +Cc: saeedm, netdev, kernel-team, jsorensen
In-Reply-To: <20190625152708.23729-2-Jes.Sorensen@gmail.com>
From: Jes Sorensen <jes.sorensen@gmail.com>
Date: Tue, 25 Jun 2019 11:27:08 -0400
> From: Jes Sorensen <jsorensen@fb.com>
>
> The previous patch broke the build with a static declaration for
> a public function.
>
> Fixes: 8f0916c6dc5c (net/mlx5e: Fix ethtool rxfh commands when CONFIG_MLX5_EN_RXNFC is disabled)
> Signed-off-by: Jes Sorensen <jsorensen@fb.com>
Saeed, I'm assuming I will get this via your next pull request once things
are sorted.
Thanks.
^ permalink raw reply
* Re: [PATCH net] vrf: reset rt_iif for recirculated mcast out pkts
From: David Miller @ 2019-06-25 20:31 UTC (permalink / raw)
To: ssuryaextr; +Cc: netdev, dsahern
In-Reply-To: <20190625103359.31102-1-ssuryaextr@gmail.com>
From: Stephen Suryaputra <ssuryaextr@gmail.com>
Date: Tue, 25 Jun 2019 06:33:59 -0400
> Multicast egress packets has skb_rtable(skb)->rt_iif set to the oif.
> Depending on the socket, these packets might be recirculated back as
> input and raw sockets that are opened for them are bound to the VRF. But
> since skb_rtable(skb) is set and its rt_iif is non-zero, inet_iif()
> function returns rt_iif instead of skb_iif (the VRF netdev). Hence, the
> socket lookup fails.
>
> Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
David A., please review.
Thank you.
^ permalink raw reply
* Re: selftests: bpf: test_libbpf.sh failed at file test_l4lb.o
From: Alexei Starovoitov @ 2019-06-25 20:27 UTC (permalink / raw)
To: Dan Rue
Cc: Andrii Nakryiko, Naresh Kamboju,
open list:KERNEL SELFTEST FRAMEWORK, bpf, Netdev, open list, Xdp,
David S. Miller, Daniel Borkmann, Alexei Starovoitov, Martin Lau,
Yonghong Song, john fastabend, Jesper Dangaard Brouer,
Jakub Kicinski, Shuah Khan
In-Reply-To: <20190625153159.5utnn36dgku5545n@xps.therub.org>
On Tue, Jun 25, 2019 at 8:32 AM Dan Rue <dan.rue@linaro.org> wrote:
>
> On Mon, Jun 24, 2019 at 12:58:15PM -0700, Alexei Starovoitov wrote:
> > On Mon, Jun 24, 2019 at 12:53 PM Dan Rue <dan.rue@linaro.org> wrote:
> > >
> > > I would say if it's not possible to check at runtime, and it requires
> > > clang 9.0, that this test should not be enabled by default.
> >
> > The latest clang is the requirement.
> > If environment has old clang or no clang at all these tests will be failing.
>
> Hi Alexei!
>
> I'm not certain if I'm interpreting you as you intended, but it sounds
> like you're telling me that if the test build environment does not use
> 'latest clang' (i guess latest as of today?), that these tests will
> fail, and that is how it is going to be. If I have that wrong, please
> correct me and disregard the rest of my message.
>
> Please understand where we are coming from. We (and many others) run
> thousands of tests from a lot of test frameworks, and so our environment
> often has mutually exclusive requirements when it comes to things like
> toolchain selection.
>
> We believe, strongly, that a test should not emit a "fail" for a missing
> requirement. Fail is a serious thing, and should be reserved for an
> actual issue that needs to be investigated, reported, and fixed.
>
> This is how we treat test failures - we investigate, report, and fix
> them when possible. When they're not real failures, we waste our time
> (and yours, in this case).
>
> By adding the tests to TEST_GEN_PROGS, you're adding them to the general
> test set that those of us running test farms try to run continuously
> across a wide range of hardware environments and kernel branches.
you run the latest selftests/bpf on the latest kernel, right?
If not than selftests/bpf is not for your setup.
In the past people argued that selftests/bpf should check
features of the kernel and skip when features are not found.
My answer to that was always the same: such changes to selftests
for older kernels need to live out of tree.
selftests/bpf are one to one to the latest kernel.
Often kernel commit X will break selftests and they're fixed
in the commit X+1.
clang, pahole, bpftool, iproute2 provide those features for the kernel.
In other words new kernel features rely on new clang and
other tools and selftests are testing those latest kernel features.
Without new clang many new features cannot be tested exhaustively.
datasec and btf are just few examples.
Hence if your test farm cannot install the latest clang, pahole, etc then
I recommend not to run selftest/bpf.
^ permalink raw reply
* [PATCH bpf-next] libbpf: fix max() type mismatch for 32bit
From: Ivan Khoronzhuk @ 2019-06-25 20:27 UTC (permalink / raw)
To: ast, netdev; +Cc: daniel, bpf, linux-kernel, Ivan Khoronzhuk
It fixes build error for 32bit caused by type mismatch
size_t/unsigned long.
Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
---
tools/lib/bpf/libbpf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 68f45a96769f..5186b7710430 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -778,7 +778,7 @@ static struct bpf_map *bpf_object__add_map(struct bpf_object *obj)
if (obj->nr_maps < obj->maps_cap)
return &obj->maps[obj->nr_maps++];
- new_cap = max(4ul, obj->maps_cap * 3 / 2);
+ new_cap = max((size_t)4, obj->maps_cap * 3 / 2);
new_maps = realloc(obj->maps, new_cap * sizeof(*obj->maps));
if (!new_maps) {
pr_warning("alloc maps for object failed\n");
--
2.17.1
^ permalink raw reply related
* Re: [RFC PATCH bpf-next] RV32G eBPF JIT
From: Luke Nelson @ 2019-06-25 20:26 UTC (permalink / raw)
To: Jiong Wang
Cc: Luke Nelson, Xi Wang, Palmer Dabbelt, Albert Ou,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Björn Töpel, linux-riscv, linux-kernel,
netdev, bpf
In-Reply-To: <87h88f9bm3.fsf@netronome.com>
On Mon, Jun 24, 2019 at 9:45 AM Jiong Wang <jiong.wang@netronome.com> wrote:
>
> Looks to me 32-bit optimization is not enabled.
>
> If you define bpf_jit_needs_zext to return true
>
> bool bpf_jit_needs_zext(void)
> {
> return true;
> }
>
> Then you don't need to zero high 32-bit when writing 32-bit sub-register
> and you just need to implement the explicit zero extension insn which is a
> special variant of BPF_MOV. This can save quite a few instructions. RV64
> and arches like arm has implemented this, please search
> "aux->verifier_zext".
>
> And there is a doc for this optimization:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/Documentation/bpf/bpf_design_QA.rst#n168
Thanks for the pointer. I'll add this optimization before sending out
an updated version of the patch.
– Luke
^ permalink raw reply
* Re: [EXT] [PATCH V2] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Guilherme Piccoli @ 2019-06-25 20:25 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru, jay.vosburgh@canonical.com
Cc: GR-everest-linux-l2, netdev@vger.kernel.org, Ariel Elior
In-Reply-To: <CAHD1Q_y7v5fVeDRT+KDimQ-RBJMujMCL2DPvdBh==YEJ3+2ZaQ@mail.gmail.com>
Sudarsana, let me ask you something: why does the register is reading
value 0x0 always in the
TX timestamp routine if the RX filter is set to None? This is the main
cause of the thread reschedule
thing.
Of course this thread thing is important to fix, but I was discussing
with my leader here and we
are curious on the reasoning the register is getting 0x0.
Thanks in advance,
Guilherme
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Vladimir Oltean @ 2019-06-25 20:24 UTC (permalink / raw)
To: Russell King - ARM Linux admin, René van Dorst
Cc: sean.wang, f.fainelli, davem, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190624153950.hdsuhrvfd77heyor@shell.armlinux.org.uk>
Hi Russell,
On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> Hi,
>
> On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>> drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>> drivers/net/dsa/mt7530.h | 9 ++
>> 2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>> #include <linux/of_mdio.h>
>> #include <linux/of_net.h>
>> #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>> #include <linux/regmap.h>
>> #include <linux/regulator/consumer.h>
>> #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
>> return ARRAY_SIZE(mt7530_mib);
>> }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> - struct phy_device *phydev)
>> -{
>> - struct mt7530_priv *priv = ds->priv;
>> -
>> - if (phy_is_pseudo_fixed_link(phydev)) {
>> - dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> - phydev->interface);
>> -
>> - /* Setup TX circuit incluing relevant PAD and driving */
>> - mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> - if (priv->id == ID_MT7530) {
>> - /* Setup RX circuit, relevant PAD and driving on the
>> - * host which must be placed after the setup on the
>> - * device side is all finished.
>> - */
>> - mt7623_pad_clk_setup(ds);
>> - }
>> - } else {
>> - u16 lcl_adv = 0, rmt_adv = 0;
>> - u8 flowctrl;
>> - u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> - switch (phydev->speed) {
>> - case SPEED_1000:
>> - mcr |= PMCR_FORCE_SPEED_1000;
>> - break;
>> - case SPEED_100:
>> - mcr |= PMCR_FORCE_SPEED_100;
>> - break;
>> - }
>> -
>> - if (phydev->link)
>> - mcr |= PMCR_FORCE_LNK;
>> -
>> - if (phydev->duplex) {
>> - mcr |= PMCR_FORCE_FDX;
>> -
>> - if (phydev->pause)
>> - rmt_adv = LPA_PAUSE_CAP;
>> - if (phydev->asym_pause)
>> - rmt_adv |= LPA_PAUSE_ASYM;
>> -
>> - lcl_adv = linkmode_adv_to_lcl_adv_t(
>> - phydev->advertising);
>> - flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> -
>> - if (flowctrl & FLOW_CTRL_TX)
>> - mcr |= PMCR_TX_FC_EN;
>> - if (flowctrl & FLOW_CTRL_RX)
>> - mcr |= PMCR_RX_FC_EN;
>> - }
>> - mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> - }
>> -}
>> -
>> static int
>> mt7530_cpu_port_enable(struct mt7530_priv *priv,
>> int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>> return 0;
>> }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + const struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> + PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 is not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>> + goto unsupported;
>> +
>> + /* Setup TX circuit incluing relevant PAD and driving */
>> + mt7530_pad_clk_setup(ds, state->interface);
>> +
>> + if (priv->id == ID_MT7530) {
>> + /* Setup RX circuit, relevant PAD and driving on the
>> + * host which must be placed after the setup on the
>> + * device side is all finished.
>> + */
>> + mt7623_pad_clk_setup(ds);
>> + }
>> + break;
>> + default:
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> + mcr |= PMCR_FORCE_MODE;
>> +
>> + if (state->speed == SPEED_1000)
>> + mcr |= PMCR_FORCE_SPEED_1000;
>> + if (state->speed == SPEED_100)
>> + mcr |= PMCR_FORCE_SPEED_100;
>> + if (state->duplex == DUPLEX_FULL)
>> + mcr |= PMCR_FORCE_FDX;
>> + if (state->link || mode == MLO_AN_FIXED)
>> + mcr |= PMCR_FORCE_LNK;
>
> This should be removed - state->link is not for use in mac_config.
> Even in fixed mode, the link can be brought up/down by means of a
> gpio, and this should be dealt with via the mac_link_* functions.
>
What do you mean exactly that state->link is not for use, is that true
in general?
In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
(!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument
for ports that are BR_STATE_DISABLED. Is that normal?
>> + if (state->pause || phylink_test(state->advertising, Pause))
>> + mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> + if (state->pause & MLO_PAUSE_TX)
>> + mcr |= PMCR_TX_FC_EN;
>> + if (state->pause & MLO_PAUSE_RX)
>> + mcr |= PMCR_RX_FC_EN;
>
> This is clearly wrong - if any bit in state->pause is set, then we
> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set. If we have Pause
> Pause set in the advertising mask, then both are set. This doesn't
> seem right - are these bits setting the advertisement, or are they
> telling the MAC to use flow control?
>
>> + }
>> +
>> + mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> + return;
>> +
>> +unsupported:
>> + dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> + __func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface)
>> +{
>> + /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> + unsigned int mode,
>> + phy_interface_t interface,
>> + struct phy_device *phydev)
>> +{
>> + /* Do nothing */
>> +}
>
> These two are where you should be forcing the link up or down if
> required (basically, inband modes should let the link come up/down
> irrespective of these functions, otherwise it should be forced.)
>
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> + unsigned long *supported,
>> + struct phylink_link_state *state)
>> +{
>> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> + switch (port) {
>> + case 0: /* Internal phy */
>> + case 1:
>> + case 2:
>> + case 3:
>> + case 4:
>> + if (state->interface != PHY_INTERFACE_MODE_NA &&
>> + state->interface != PHY_INTERFACE_MODE_GMII)
>> + goto unsupported;
>> + break;
>> + /* case 5: Port 5 not supported! */
>> + case 6: /* 1st cpu port */
>> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> + state->interface != PHY_INTERFACE_MODE_TRGMII)
>
> PHY_INTERFACE_MODE_NA ?
>
>> + goto unsupported;
>> + break;
>> + default:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> + return;
>> + }
>> +
>> + phylink_set(mask, Autoneg);
>> + phylink_set(mask, Pause);
>> + phylink_set(mask, Asym_Pause);
>> + phylink_set(mask, MII);
>> +
>> + phylink_set(mask, 10baseT_Half);
>> + phylink_set(mask, 10baseT_Full);
>> + phylink_set(mask, 100baseT_Half);
>> + phylink_set(mask, 100baseT_Full);
>> + phylink_set(mask, 1000baseT_Full);
>> + phylink_set(mask, 1000baseT_Half);
>
> You seem to be missing phylink_set_port_modes() here.
>
>> +
>> + linkmode_and(supported, supported, mask);
>> + linkmode_and(state->advertising, state->advertising, mask);
>> + return;
>> +
>> +unsupported:
>> + linkmode_zero(supported);
>> + dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> + __func__, state->interface, phy_modes(state->interface));
>
> Not a good idea to print this at error level; sometimes we just probe
> for support.
>
> Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
> interface mode that the MAC can't support - we detect that by the
> validation failing, and printing a more meaningful message in phylink
> itself.
>
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> + struct phylink_link_state *state)
>> +{
>> + struct mt7530_priv *priv = ds->priv;
>> + u32 pmsr;
>> +
>> + if (port < 0 || port >= MT7530_NUM_PORTS)
>> + return -EINVAL;
>> +
>> + pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> + state->link = (pmsr & PMSR_LINK);
>> + state->an_complete = state->link;
>> + state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> + switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> + case 0:
>> + state->speed = SPEED_10;
>> + break;
>> + case PMSR_SPEED_100:
>> + state->speed = SPEED_100;
>> + break;
>> + case PMSR_SPEED_1000:
>> + state->speed = SPEED_1000;
>> + break;
>> + default:
>> + state->speed = SPEED_UNKNOWN;
>> + break;
>> + }
>> +
>> + state->pause = 0;
>> + if (pmsr & PMSR_RX_FC)
>> + state->pause |= MLO_PAUSE_RX;
>> + if (pmsr & PMSR_TX_FC)
>> + state->pause |= MLO_PAUSE_TX;
>> +
>> + return 1;
>> +}
>> +
>> static const struct dsa_switch_ops mt7530_switch_ops = {
>> .get_tag_protocol = mtk_get_tag_protocol,
>> .setup = mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>> .phy_write = mt7530_phy_write,
>> .get_ethtool_stats = mt7530_get_ethtool_stats,
>> .get_sset_count = mt7530_get_sset_count,
>> - .adjust_link = mt7530_adjust_link,
>> .port_enable = mt7530_port_enable,
>> .port_disable = mt7530_port_disable,
>> .port_stp_state_set = mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>> .port_vlan_prepare = mt7530_port_vlan_prepare,
>> .port_vlan_add = mt7530_port_vlan_add,
>> .port_vlan_del = mt7530_port_vlan_del,
>> + .phylink_validate = mt7530_phylink_validate,
>> + .phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> + .phylink_mac_config = mt7530_phylink_mac_config,
>> + .phylink_mac_link_down = mt7530_phylink_mac_link_down,
>> + .phylink_mac_link_up = mt7530_phylink_mac_link_up,
>> };
>>
>> static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>> #define PMCR_FORCE_SPEED_100 BIT(2)
>> #define PMCR_FORCE_FDX BIT(1)
>> #define PMCR_FORCE_LNK BIT(0)
>> +#define PMCR_FORCE_LNK_DOWN PMCR_FORCE_MODE
>> #define PMCR_COMMON_LINK (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>> PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>> PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>> PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>> #define MT7530_PMSR_P(x) (0x3008 + (x) * 0x100)
>> +#define PMSR_EEE1G BIT(7)
>> +#define PMSR_EEE100M BIT(6)
>> +#define PMSR_RX_FC BIT(5)
>> +#define PMSR_TX_FC BIT(4)
>> +#define PMSR_SPEED_1000 BIT(3)
>> +#define PMSR_SPEED_100 BIT(2)
>> +#define PMSR_DPX BIT(1)
>> +#define PMSR_LINK BIT(0)
>>
>> /* Register for MIB */
>> #define MT7530_PORT_MIB_COUNTER(x) (0x4000 + (x) * 0x100)
>> --
>> 2.20.1
>>
>>
>
Regards,
-Vladimir
^ permalink raw reply
* Re: [PATCH rdma-next v1 11/12] IB/mlx5: Implement DEVX dispatching event
From: Jason Gunthorpe @ 2019-06-25 20:23 UTC (permalink / raw)
To: Yishai Hadas
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, linux-netdev
In-Reply-To: <a2380ea6-4542-c72c-96f7-e68786847ccc@dev.mellanox.co.il>
On Tue, Jun 25, 2019 at 05:41:49PM +0300, Yishai Hadas wrote:
> > > > Why don't we return EIO as soon as is-destroyed happens? What is the
> > > > point of flushing out the accumulated events?
> > >
> > > It follows the above uverb code/logic that returns existing events even in
> > > that case, also the async command events in this file follows that logic, I
> > > suggest to stay consistent.
> >
> > Don't follow broken uverbs stuff...
>
> May it be that there is some event that we still want to deliver post
> unbind/hot-unplug ? for example IB_EVENT_DEVICE_FATAL in uverbs and others
> from the driver code.
EIO is DEVICE_FATAL.
> Not sure that we want to change this logic.
> What do you think ?
I think this code should exit immediately with EIO if the device is
disassociated.
> > > > Maybe the event should be re-added on error? Tricky.
> > >
> > > What will happen if another copy_to_user may then fail again (loop ?) ...
> > > not sure that we want to get into this tricky handling ...
> > >
> > > As of above, It follows the logic from uverbs at that area.
> > > https://elixir.bootlin.com/linux/latest/source/drivers/infiniband/core/uverbs_main.c#L267
> >
> > again it is wrong...
> >
> > There is no loop if you just stick the item back on the head of the
> > list and exit, which is probably the right thing to do..
> >
>
> What if copy_to_user() will fail again just later on ? we might end-up with
> loop of read(s) that always find an event as it was put back.
That is clearly an application bug and is not the concern of the
kernel..
> I suggest to leave this flow as it's now, at least for this series
> submission.
>
> Agree ?
I don't think you can actually fix this, so maybe we have to leave
it. But add a comment explaining
Jason
^ permalink raw reply
* Re: [PATCH net] vrf: reset rt_iif for recirculated mcast out pkts
From: David Ahern @ 2019-06-25 20:22 UTC (permalink / raw)
To: Stephen Suryaputra, netdev
In-Reply-To: <20190625103359.31102-1-ssuryaextr@gmail.com>
On 6/25/19 4:33 AM, Stephen Suryaputra wrote:
> @@ -363,10 +376,20 @@ int ip_mc_output(struct net *net, struct sock *sk, struct sk_buff *skb)
> #endif
> ) {
> struct sk_buff *newskb = skb_clone(skb, GFP_ATOMIC);
> - if (newskb)
> + if (newskb) {
> + /* Reset rt_iif so that inet_iif() will return
> + * skb->dev->ifIndex which is the VRF device for
> + * socket lookup. Setting this to VRF ifindex
> + * causes ipi_ifindex in in_pktinfo to be
> + * overwritten, see ipv4_pktinfo_prepare().
> + */
> + if (netif_is_l3_slave(dev))
seems like the rt_iif is a problem for recirculated mcast packets in
general, not just ones tied to a VRF.
> + ip_mc_reset_rt_iif(net, rt, newskb);
> +
> NF_HOOK(NFPROTO_IPV4, NF_INET_POST_ROUTING,
> net, sk, newskb, NULL, newskb->dev,
> ip_mc_finish_output);
> + }
> }
>
> /* Multicasts with ttl 0 must not go beyond the host */
^ permalink raw reply
* Re: [PATCH bpf] bpf: fix BPF_ALU32 | BPF_ARSH on BE arches
From: Song Liu @ 2019-06-25 20:15 UTC (permalink / raw)
To: Jiong Wang
Cc: Alexei Starovoitov, Daniel Borkmann, yauheni.kaliuta, bpf,
Networking, oss-drivers
In-Reply-To: <1561480910-23543-1-git-send-email-jiong.wang@netronome.com>
On Tue, Jun 25, 2019 at 12:31 PM Jiong Wang <jiong.wang@netronome.com> wrote:
>
> Yauheni reported the following code do not work correctly on BE arches:
>
> ALU_ARSH_X:
> DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
> CONT;
> ALU_ARSH_K:
> DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
> CONT;
>
> and are causing failure of test_verifier test 'arsh32 on imm 2' on BE
> arches.
>
> The code is taking address and interpreting memory directly, so is not
> endianness neutral. We should instead perform standard C type casting on
> the variable. A u64 to s32 conversion will drop the high 32-bit and reserve
> the low 32-bit as signed integer, this is all we want.
>
> Fixes: 2dc6b100f928 ("bpf: interpreter support BPF_ALU | BPF_ARSH")
> Reported-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> Signed-off-by: Jiong Wang <jiong.wang@netronome.com>
Acked-by: Song Liu <songliubraving@fb.com>
I guess we need:
Cc: <stable@vger.kernel.org> #v5.0+
> ---
> kernel/bpf/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index 080e2bb..f2148db 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -1364,10 +1364,10 @@ static u64 ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
> insn++;
> CONT;
> ALU_ARSH_X:
> - DST = (u64) (u32) ((*(s32 *) &DST) >> SRC);
> + DST = (u64) (u32) (((s32) DST) >> SRC);
> CONT;
> ALU_ARSH_K:
> - DST = (u64) (u32) ((*(s32 *) &DST) >> IMM);
> + DST = (u64) (u32) (((s32) DST) >> IMM);
> CONT;
> ALU64_ARSH_X:
> (*(s64 *) &DST) >>= SRC;
> --
> 2.7.4
>
^ permalink raw reply
* Re: [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Stanislav Fomichev @ 2019-06-25 20:12 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: netdev, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kernel, bpf
In-Reply-To: <20190625194215.14927-9-krzesimir@kinvolk.io>
On 06/25, Krzesimir Nowak wrote:
> As an input, test run for perf event program takes struct
> bpf_perf_event_data as ctx_in and struct bpf_perf_event_value as
> data_in. For an output, it basically ignores ctx_out and data_out.
>
> The implementation sets an instance of struct bpf_perf_event_data_kern
> in such a way that the BPF program reading data from context will
> receive what we passed to the bpf prog test run in ctx_in. Also BPF
> program can call bpf_perf_prog_read_value to receive what was passed
> in data_in.
>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
> kernel/trace/bpf_trace.c | 107 ++++++++++++++++++
> .../bpf/verifier/perf_event_sample_period.c | 8 ++
> 2 files changed, 115 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index c102c240bb0b..2fa49ea8a475 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -16,6 +16,8 @@
>
> #include <asm/tlb.h>
>
> +#include <trace/events/bpf_test_run.h>
> +
> #include "trace_probe.h"
> #include "trace.h"
>
> @@ -1160,7 +1162,112 @@ const struct bpf_verifier_ops perf_event_verifier_ops = {
> .convert_ctx_access = pe_prog_convert_ctx_access,
> };
>
> +static int pe_prog_test_run(struct bpf_prog *prog,
> + const union bpf_attr *kattr,
> + union bpf_attr __user *uattr)
> +{
> + void __user *ctx_in = u64_to_user_ptr(kattr->test.ctx_in);
> + void __user *data_in = u64_to_user_ptr(kattr->test.data_in);
> + u32 data_size_in = kattr->test.data_size_in;
> + u32 ctx_size_in = kattr->test.ctx_size_in;
> + u32 repeat = kattr->test.repeat;
> + u32 retval = 0, duration = 0;
> + int err = -EINVAL;
> + u64 time_start, time_spent = 0;
> + int i;
> + struct perf_sample_data sample_data = {0, };
> + struct perf_event event = {0, };
> + struct bpf_perf_event_data_kern real_ctx = {0, };
> + struct bpf_perf_event_data fake_ctx = {0, };
> + struct bpf_perf_event_value value = {0, };
> +
> + if (ctx_size_in != sizeof(fake_ctx))
> + goto out;
> + if (data_size_in != sizeof(value))
> + goto out;
> +
> + if (copy_from_user(&fake_ctx, ctx_in, ctx_size_in)) {
> + err = -EFAULT;
> + goto out;
> + }
Move this to net/bpf/test_run.c? I have a bpf_ctx_init helper to deal
with ctx input, might save you some code above wrt ctx size/etc.
> + if (copy_from_user(&value, data_in, data_size_in)) {
> + err = -EFAULT;
> + goto out;
> + }
> +
> + real_ctx.regs = &fake_ctx.regs;
> + real_ctx.data = &sample_data;
> + real_ctx.event = &event;
> + perf_sample_data_init(&sample_data, fake_ctx.addr,
> + fake_ctx.sample_period);
> + event.cpu = smp_processor_id();
> + event.oncpu = -1;
> + event.state = PERF_EVENT_STATE_OFF;
> + local64_set(&event.count, value.counter);
> + event.total_time_enabled = value.enabled;
> + event.total_time_running = value.running;
> + /* make self as a leader - it is used only for checking the
> + * state field
> + */
> + event.group_leader = &event;
> +
> + /* slightly changed copy pasta from bpf_test_run() in
> + * net/bpf/test_run.c
> + */
> + if (!repeat)
> + repeat = 1;
> +
> + rcu_read_lock();
> + preempt_disable();
> + time_start = ktime_get_ns();
> + for (i = 0; i < repeat; i++) {
Any reason for not using bpf_test_run?
> + retval = BPF_PROG_RUN(prog, &real_ctx);
> +
> + if (signal_pending(current)) {
> + err = -EINTR;
> + preempt_enable();
> + rcu_read_unlock();
> + goto out;
> + }
> +
> + if (need_resched()) {
> + time_spent += ktime_get_ns() - time_start;
> + preempt_enable();
> + rcu_read_unlock();
> +
> + cond_resched();
> +
> + rcu_read_lock();
> + preempt_disable();
> + time_start = ktime_get_ns();
> + }
> + }
> + time_spent += ktime_get_ns() - time_start;
> + preempt_enable();
> + rcu_read_unlock();
> +
> + do_div(time_spent, repeat);
> + duration = time_spent > U32_MAX ? U32_MAX : (u32)time_spent;
> + /* end of slightly changed copy pasta from bpf_test_run() in
> + * net/bpf/test_run.c
> + */
> +
> + if (copy_to_user(&uattr->test.retval, &retval, sizeof(retval))) {
> + err = -EFAULT;
> + goto out;
> + }
> + if (copy_to_user(&uattr->test.duration, &duration, sizeof(duration))) {
> + err = -EFAULT;
> + goto out;
> + }
Can BPF program modify fake_ctx? Do we need/want to copy it back?
> + err = 0;
> +out:
> + trace_bpf_test_finish(&err);
> + return err;
> +}
> +
> const struct bpf_prog_ops perf_event_prog_ops = {
> + .test_run = pe_prog_test_run,
> };
>
> static DEFINE_MUTEX(bpf_event_mutex);
> diff --git a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> index 471c1a5950d8..16e9e5824d14 100644
> --- a/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
> +++ b/tools/testing/selftests/bpf/verifier/perf_event_sample_period.c
This should probably go in another patch.
> @@ -13,6 +13,8 @@
> },
> .result = ACCEPT,
> .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> + .ctx_len = sizeof(struct bpf_perf_event_data),
> + .data_len = sizeof(struct bpf_perf_event_value),
> },
> {
> "check bpf_perf_event_data->sample_period half load permitted",
> @@ -29,6 +31,8 @@
> },
> .result = ACCEPT,
> .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> + .ctx_len = sizeof(struct bpf_perf_event_data),
> + .data_len = sizeof(struct bpf_perf_event_value),
> },
> {
> "check bpf_perf_event_data->sample_period word load permitted",
> @@ -45,6 +49,8 @@
> },
> .result = ACCEPT,
> .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> + .ctx_len = sizeof(struct bpf_perf_event_data),
> + .data_len = sizeof(struct bpf_perf_event_value),
> },
> {
> "check bpf_perf_event_data->sample_period dword load permitted",
> @@ -56,4 +62,6 @@
> },
> .result = ACCEPT,
> .prog_type = BPF_PROG_TYPE_PERF_EVENT,
> + .ctx_len = sizeof(struct bpf_perf_event_data),
> + .data_len = sizeof(struct bpf_perf_event_value),
> },
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH iproute2] ip/iptoken: fix dump error when ipv6 disabled
From: Stephen Hemminger @ 2019-06-25 20:11 UTC (permalink / raw)
To: Hangbin Liu
Cc: netdev, Daniel Borkmann, Phil Sutter, David Ahern, Andrea Claudi
In-Reply-To: <20190625093550.7804-1-liuhangbin@gmail.com>
On Tue, 25 Jun 2019 17:35:50 +0800
Hangbin Liu <liuhangbin@gmail.com> wrote:
> When we disable IPv6 from the start up (ipv6.disable=1), there will be
> no IPv6 route info in the dump message. If we return -1 when
> ifi->ifi_family != AF_INET6, we will get error like
>
> $ ip token list
> Dump terminated
>
> which will make user feel confused. There is no need to return -1 if the
> dump message not match. Return 0 is enough.
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> ip/iptoken.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/ip/iptoken.c b/ip/iptoken.c
> index f1194c3e..dfd22734 100644
> --- a/ip/iptoken.c
> +++ b/ip/iptoken.c
> @@ -59,13 +59,9 @@ static int print_token(struct nlmsghdr *n, void *arg)
> if (len < 0)
> return -1;
>
> - if (ifi->ifi_family != AF_INET6)
> - return -1;
> - if (ifi->ifi_index == 0)
> - return -1;
> - if (ifindex > 0 && ifi->ifi_index != ifindex)
> - return 0;
> - if (ifi->ifi_flags & (IFF_LOOPBACK | IFF_NOARP))
> + if (ifi->ifi_family != AF_INET6 || ifi->ifi_index == 0 ||
> + (ifindex > 0 && ifi->ifi_index != ifindex) ||
> + (ifi->ifi_flags & (IFF_LOOPBACK | IFF_NOARP)))
> return 0;
Please don't combine all the conditions, it is simpler as:
if (ifi->ifi_family != AF_INET6)
return 0;
^ permalink raw reply
* Re: [PATCH bpf-next v4 2/2] bpf: Add selftests for bpf_perf_event_output
From: Song Liu @ 2019-06-25 20:11 UTC (permalink / raw)
To: allanzhang
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Networking, bpf, open list
In-Reply-To: <20190625172717.158613-3-allanzhang@google.com>
On Tue, Jun 25, 2019 at 12:45 PM allanzhang <allanzhang@google.com> wrote:
>
> Software event output is only enabled by a few prog types.
> This test is to ensure that all supported types are enbled for
> bpf_perf_event_output sucessfully.
Please fix these typos highlighted by Daniel.
enbled
sucessfully
>
> v4:
> * Reformating log message
> v3:
> * Reformating log message
> v2:
> * Reformating log message
>
> Signed-off-by: allanzhang <allanzhang@google.com>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 2 files changed, 126 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index c5514daf8865..901a188e1eea 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -50,7 +50,7 @@
> #define MAX_INSNS BPF_MAXINSNS
> #define MAX_TEST_INSNS 1000000
> #define MAX_FIXUPS 8
> -#define MAX_NR_MAPS 18
> +#define MAX_NR_MAPS 19
> #define MAX_TEST_RUNS 8
> #define POINTER_VALUE 0xcafe4all
> #define TEST_DATA_LEN 64
> @@ -84,6 +84,7 @@ struct bpf_test {
> int fixup_map_array_wo[MAX_FIXUPS];
> int fixup_map_array_small[MAX_FIXUPS];
> int fixup_sk_storage_map[MAX_FIXUPS];
> + int fixup_map_event_output[MAX_FIXUPS];
> const char *errstr;
> const char *errstr_unpriv;
> uint32_t retval, retval_unpriv, insn_processed;
> @@ -604,6 +605,28 @@ static int create_sk_storage_map(void)
> return fd;
> }
>
> +static int create_event_output_map(void)
> +{
> + struct bpf_create_map_attr attr = {
> + .name = "test_map",
> + .map_type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> + .key_size = 4,
> + .value_size = 4,
> + .max_entries = 1,
> + };
> + int fd, btf_fd;
> +
> + btf_fd = load_btf();
> + if (btf_fd < 0)
> + return -1;
> + attr.btf_fd = btf_fd;
> + fd = bpf_create_map_xattr(&attr);
> + close(attr.btf_fd);
> + if (fd < 0)
> + printf("Failed to create event_output\n");
> + return fd;
> +}
> +
> static char bpf_vlog[UINT_MAX >> 8];
>
> static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> @@ -627,6 +650,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> int *fixup_map_array_wo = test->fixup_map_array_wo;
> int *fixup_map_array_small = test->fixup_map_array_small;
> int *fixup_sk_storage_map = test->fixup_sk_storage_map;
> + int *fixup_map_event_output = test->fixup_map_event_output;
>
> if (test->fill_helper) {
> test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
> @@ -788,6 +812,13 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
> fixup_sk_storage_map++;
> } while (*fixup_sk_storage_map);
> }
> + if (*fixup_map_event_output) {
> + map_fds[18] = create_event_output_map();
> + do {
> + prog[*fixup_map_event_output].imm = map_fds[18];
> + fixup_map_event_output++;
> + } while (*fixup_map_event_output);
> + }
> }
>
> static int set_admin(bool admin)
> diff --git a/tools/testing/selftests/bpf/verifier/event_output.c b/tools/testing/selftests/bpf/verifier/event_output.c
> new file mode 100644
> index 000000000000..b25eabcfaa56
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/event_output.c
> @@ -0,0 +1,94 @@
> +/* instructions used to output a skb based software event, produced
> + * from code snippet:
> +struct TMP {
> + uint64_t tmp;
> +} tt;
> +tt.tmp = 5;
> +bpf_perf_event_output(skb, &connection_tracking_event_map, 0,
> + &tt, sizeof(tt));
> +return 1;
> +
> +the bpf assembly from llvm is:
> + 0: b7 02 00 00 05 00 00 00 r2 = 5
> + 1: 7b 2a f8 ff 00 00 00 00 *(u64 *)(r10 - 8) = r2
> + 2: bf a4 00 00 00 00 00 00 r4 = r10
> + 3: 07 04 00 00 f8 ff ff ff r4 += -8
> + 4: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0ll
> + 6: b7 03 00 00 00 00 00 00 r3 = 0
> + 7: b7 05 00 00 08 00 00 00 r5 = 8
> + 8: 85 00 00 00 19 00 00 00 call 25
> + 9: b7 00 00 00 01 00 00 00 r0 = 1
> + 10: 95 00 00 00 00 00 00 00 exit
> +
> + The reason I put the code here instead of fill_helpers is that map fixup is
> + against the insns, instead of filled prog.
> +*/
Please prefix every line in the comment section with space_star_space: " * ".
This makes it obvious that this is a comment.
Thanks,
Song
> +
> +#define __PERF_EVENT_INSNS__ \
> + BPF_MOV64_IMM(BPF_REG_2, 5), \
> + BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -8), \
> + BPF_MOV64_REG(BPF_REG_4, BPF_REG_10), \
> + BPF_ALU64_IMM(BPF_ADD, BPF_REG_4, -8), \
> + BPF_LD_MAP_FD(BPF_REG_2, 0), \
> + BPF_MOV64_IMM(BPF_REG_3, 0), \
> + BPF_MOV64_IMM(BPF_REG_5, 8), \
> + BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, \
> + BPF_FUNC_perf_event_output), \
> + BPF_MOV64_IMM(BPF_REG_0, 1), \
> + BPF_EXIT_INSN(),
> +{
> + "perfevent for sockops",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCK_OPS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for tc",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SCHED_CLS,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for lwt out",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_LWT_OUT,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for xdp",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_XDP,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for socket filter",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SOCKET_FILTER,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for sk_skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_SK_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> +{
> + "perfevent for cgroup skb",
> + .insns = { __PERF_EVENT_INSNS__ },
> + .prog_type = BPF_PROG_TYPE_CGROUP_SKB,
> + .fixup_map_event_output = { 4 },
> + .result = ACCEPT,
> + .retval = 1,
> +},
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [bpf-next v2 03/10] selftests/bpf: Avoid another case of errno clobbering
From: Stanislav Fomichev @ 2019-06-25 20:08 UTC (permalink / raw)
To: Krzesimir Nowak
Cc: netdev, Alban Crequy, Iago López Galeiras,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, linux-kernel, bpf, Stanislav Fomichev
In-Reply-To: <20190625194215.14927-4-krzesimir@kinvolk.io>
On 06/25, Krzesimir Nowak wrote:
> Commit 8184d44c9a57 ("selftests/bpf: skip verifier tests for
> unsupported program types") added a check for an unsupported program
> type. The function doing it changes errno, so test_verifier should
> save it before calling it if test_verifier wants to print a reason why
> verifying a BPF program of a supported type failed.
>
> Fixes: 8184d44c9a57 ("selftests/bpf: skip verifier tests for unsupported program types")
> Cc: Stanislav Fomichev <sdf@google.com>
> Signed-off-by: Krzesimir Nowak <krzesimir@kinvolk.io>
> ---
> tools/testing/selftests/bpf/test_verifier.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
> index 12589da13487..779e30b96ded 100644
> --- a/tools/testing/selftests/bpf/test_verifier.c
> +++ b/tools/testing/selftests/bpf/test_verifier.c
> @@ -867,6 +867,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> int fixup_skips;
> __u32 pflags;
> int i, err;
> + int saved_errno;
Reverse Christmas tree. Otherwise LGTM.
>
> for (i = 0; i < MAX_NR_MAPS; i++)
> map_fds[i] = -1;
> @@ -894,6 +895,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> pflags |= BPF_F_ANY_ALIGNMENT;
> fd_prog = bpf_verify_program(prog_type, prog, prog_len, pflags,
> "GPL", 0, bpf_vlog, sizeof(bpf_vlog), 4);
> + saved_errno = errno;
> if (fd_prog < 0 && !bpf_probe_prog_type(prog_type, 0)) {
> printf("SKIP (unsupported program type %d)\n", prog_type);
> skips++;
> @@ -910,7 +912,7 @@ static void do_test_single(struct bpf_test *test, bool unpriv,
> if (expected_ret == ACCEPT) {
> if (fd_prog < 0) {
> printf("FAIL\nFailed to load prog '%s'!\n",
> - strerror(errno));
> + strerror(saved_errno));
> goto fail_log;
> }
> #ifndef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/2] bpf: Allow bpf_skb_event_output for more prog types
From: Song Liu @ 2019-06-25 20:06 UTC (permalink / raw)
To: allanzhang
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, David S. Miller, Networking, bpf, open list
In-Reply-To: <20190625172717.158613-1-allanzhang@google.com>
On Tue, Jun 25, 2019 at 12:45 PM allanzhang <allanzhang@google.com> wrote:
>
> Software event output is only enabled by a few prog types right now (TC,
> LWT out, XDP, sockops). Many other skb based prog types need
> bpf_skb_event_output to produce software event.
>
> Added socket_filter, cg_skb, sk_skb prog types to generate sw event.
>
> allanzhang (2):
> bpf: Allow bpf_skb_event_output for a few prog types
> bpf: Add selftests for bpf_perf_event_output
I am not sure whether this is caused by delay in the mailing list or something
else. But it appears to me that you are ignoring some of the feedback. Please
pay more attention to these feedback.
Please include changes "v1, xxx, v2, xxx, .." in the cover letter, but not the
commit log itself. In other words, include that in 0/2, but not in 1/2 or 2/2.
Thanks,
Song
>
> net/core/filter.c | 6 ++
> tools/testing/selftests/bpf/test_verifier.c | 33 ++++++-
> .../selftests/bpf/verifier/event_output.c | 94 +++++++++++++++++++
> 3 files changed, 132 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/event_output.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [PATCH v4 rdma-next 2/3] RDMA/qedr: Add doorbell overflow recovery support
From: Jason Gunthorpe @ 2019-06-25 20:04 UTC (permalink / raw)
To: Michal Kalderon
Cc: ariel.elior@marvell.com, dledford@redhat.com,
linux-rdma@vger.kernel.org, davem@davemloft.net,
netdev@vger.kernel.org
In-Reply-To: <20190624102809.8793-3-michal.kalderon@marvell.com>
On Mon, Jun 24, 2019 at 01:28:08PM +0300, Michal Kalderon wrote:
> +/* Map the kernel doorbell recovery memory entry */
> +int qedr_mmap_db_rec(struct vm_area_struct *vma)
> +{
> + unsigned long len = vma->vm_end - vma->vm_start;
> +
> + return remap_pfn_range(vma, vma->vm_start,
> + vma->vm_pgoff,
> + len, vma->vm_page_prot);
> +}
> +
> int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> {
> struct qedr_ucontext *ucontext = get_qedr_ucontext(context);
> @@ -390,6 +446,8 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> unsigned long phys_addr = vma->vm_pgoff << PAGE_SHIFT;
> unsigned long len = (vma->vm_end - vma->vm_start);
> unsigned long dpi_start;
> + struct qedr_mm *mm;
> + int rc;
>
> dpi_start = dev->db_phys_addr + (ucontext->dpi * ucontext->dpi_size);
>
> @@ -405,29 +463,28 @@ int qedr_mmap(struct ib_ucontext *context, struct vm_area_struct *vma)
> return -EINVAL;
> }
>
> - if (!qedr_search_mmap(ucontext, phys_addr, len)) {
> - DP_ERR(dev, "failed mmap, vm_pgoff=0x%lx is not authorized\n",
> + mm = qedr_remove_mmap(ucontext, phys_addr, len);
> + if (!mm) {
> + DP_ERR(dev, "failed to remove mmap, vm_pgoff=0x%lx\n",
> vma->vm_pgoff);
> return -EINVAL;
>
This is so gross, please follow the pattern other drivers use for
managing the mmap cookie
In fact I am sick of seeing drivers wrongly re-implement this, so you
now get the job to make some proper core helpers to manage mmap
cookies for drivers.
The EFA driver is probably the best example, I suggest you move that
code to a common file in ib-core and use it here instead of redoing
yet again another broken version.
siw has another copy of basically the same thing.
> +static int qedr_init_user_db_rec(struct ib_udata *udata,
> + struct qedr_dev *dev, struct qedr_userq *q,
> + bool requires_db_rec)
> +{
> + struct qedr_ucontext *uctx =
> + rdma_udata_to_drv_context(udata, struct qedr_ucontext,
> + ibucontext);
> +
> + /* Aborting for non doorbell userqueue (SRQ) or non-supporting lib */
> + if (requires_db_rec == 0 || !uctx->db_rec)
> + return 0;
> +
> + /* Allocate a page for doorbell recovery, add to mmap ) */
> + q->db_rec_data = (void *)get_zeroed_page(GFP_KERNEL);
Pages obtained by get_zeroed_page shuld not be inserted by
remap_pfn_range, those cases need to use vm_insert_page instead.
> struct qedr_alloc_ucontext_resp {
> __aligned_u64 db_pa;
> @@ -74,6 +83,7 @@ struct qedr_create_cq_uresp {
> __u32 db_offset;
> __u16 icid;
> __u16 reserved;
> + __u64 db_rec_addr;
> };
All uapi u64s need to be __aligned_u64 in this file.
> +/* doorbell recovery entry allocated and populated by userspace doorbelling
> + * entities and mapped to kernel. Kernel uses this to register doorbell
> + * information with doorbell drop recovery mechanism.
> + */
> +struct qedr_user_db_rec {
> + __aligned_u64 db_data; /* doorbell data */
> +};
like this one :\
Jason
^ permalink raw reply
* Re: hard-coded limit on unresolved multicast route cache in ipv4/ipmr.c causes slow, unreliable creation of multicast routes on busy networks
From: David Miller @ 2019-06-25 20:03 UTC (permalink / raw)
To: liuhangbin; +Cc: sukumarg1973, karn, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20190625061507.GG18865@dhcp-12-139.nay.redhat.com>
From: Hangbin Liu <liuhangbin@gmail.com>
Date: Tue, 25 Jun 2019 14:15:07 +0800
> On Tue, Dec 18, 2018 at 09:55:45PM -0800, David Miller wrote:
>> From: Sukumar Gopalakrishnan <sukumarg1973@gmail.com>
>> Date: Wed, 19 Dec 2018 10:57:02 +0530
>>
>> > Hi David,
>> >
>> > There are two patch for this issue:
>> > 1) Your changes which removes cache_resolve_queue_len
>> > 2) Hangbin's changes which make cache_resolve_queue_len configurable.
>> >
>> > Which one will be chosen for this issue ?
>>
>> I do plan to look into this, sorry for taking so long.
>>
>> Right now I am overwhelmed preparing for the next merge window and
>> synchronizing with other developers for that.
>>
>> Please be patient.
>
> Hi David,
>
> Any progress for this issue?
I have absolutely no context from a discussion that happened back in Dec 2018
If it is important to you, please restart the discussion with a new mailing list
posting restating the problem from the beginning and reiterating all of the
points and arguments that have been made thus far.
^ 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