* [PATCH ipsec] xfrm: Fix inbound traffic via XFRM interfaces across network namespaces
From: Tobias Brunner @ 2019-02-18 9:49 UTC (permalink / raw)
To: Steffen Klassert; +Cc: netdev
After moving an XFRM interface to another namespace it stays associated
with the original namespace (net in `struct xfrm_if` and the list keyed
with `xfrmi_net_id`), allowing processes in the new namespace to use
SAs/policies that were created in the original namespace. For instance,
this allows a keying daemon in one namespace to establish IPsec SAs for
other namespaces without processes there having access to the keys or IKE
credentials.
This worked fine for outbound traffic, however, for inbound traffic the
lookup for the interfaces and the policies used the incorrect namespace
(the one the XFRM interface was moved to).
Fixes: f203b76d7809 ("xfrm: Add virtual xfrm interfaces")
Signed-off-by: Tobias Brunner <tobias@strongswan.org>
---
net/xfrm/xfrm_interface.c | 4 ++--
net/xfrm/xfrm_policy.c | 4 +++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 6be8c7df15bb..dbb3c1945b5c 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -76,10 +76,10 @@ static struct xfrm_if *xfrmi_decode_session(struct sk_buff *skb)
int ifindex;
struct xfrm_if *xi;
- if (!skb->dev)
+ if (!secpath_exists(skb) || !skb->dev)
return NULL;
- xfrmn = net_generic(dev_net(skb->dev), xfrmi_net_id);
+ xfrmn = net_generic(xs_net(xfrm_input_state(skb)), xfrmi_net_id);
ifindex = skb->dev->ifindex;
for_each_xfrmi_rcu(xfrmn->xfrmi[0], xi) {
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index ba0a4048c846..8d1a898d0ba5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3314,8 +3314,10 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
if (ifcb) {
xi = ifcb->decode_session(skb);
- if (xi)
+ if (xi) {
if_id = xi->p.if_id;
+ net = xi->net;
+ }
}
rcu_read_unlock();
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next v4 05/17] net: sched: traverse chains in block with tcf_get_next_chain()
From: Vlad Buslov @ 2019-02-18 10:07 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAM_iQpUnmKPh+RCi-JyouKz7PrAiWPp30Ro2qZFrR=MjVHogHA@mail.gmail.com>
Hi Cong,
Thanks for reviewing!
On Fri 15 Feb 2019 at 22:21, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> (Sorry for joining this late.)
>
> On Mon, Feb 11, 2019 at 12:56 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> @@ -2432,7 +2474,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
>> index_start = cb->args[0];
>> index = 0;
>>
>> - list_for_each_entry(chain, &block->chain_list, list) {
>> + for (chain = __tcf_get_next_chain(block, NULL);
>> + chain;
>> + chain_prev = chain,
>> + chain = __tcf_get_next_chain(block, chain),
>> + tcf_chain_put(chain_prev)) {
>
> Why do you want to take the block->lock in each iteration
> of the loop rather than taking once for the whole loop?
This loop calls classifier ops callback in tc_chain_fill_node(). I don't
call any classifier ops callbacks while holding block or chain lock in
this change because the goal is to achieve fine-grained locking for data
structures used by filter update path. Locking per-block or per-chain is
much coarser than taking reference counters to parent structures and
allowing classifiers to implement their own locking.
In this case call to ops->tmplt_dump() is probably quite fast and its
execution time doesn't depend on number of filters on the classifier, so
releasing block->lock on each iteration doesn't provide much benefit, if
at all. However, it is easier for me to reason about locking correctness
in this refactoring by following a simple rule that no locks (besides
rtnl mutex) can be held when calling classifier ops callbacks.
^ permalink raw reply
* Re: [PATCH bpf-next v4 0/2] libbpf: adding AF_XDP support
From: Magnus Karlsson @ 2019-02-18 10:09 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jesper Dangaard Brouer, Jonathan Lemon, Magnus Karlsson,
Björn Töpel, ast, Network Development, Jakub Kicinski,
Björn Töpel, Zhang, Qi Z, xiaolong.ye,
xdp-newbies@vger.kernel.org
In-Reply-To: <5ed22245-fe6b-14a9-9c93-f039828a02b6@iogearbox.net>
On Mon, Feb 18, 2019 at 10:38 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 02/18/2019 09:20 AM, Magnus Karlsson wrote:
> > On Fri, Feb 15, 2019 at 5:48 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>
> >> On 02/13/2019 12:55 PM, Jesper Dangaard Brouer wrote:
> >>> On Wed, 13 Feb 2019 12:32:47 +0100
> >>> Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
> >>>> On Mon, Feb 11, 2019 at 9:44 PM Jonathan Lemon <jonathan.lemon@gmail.com> wrote:
> >>>>> On 8 Feb 2019, at 5:05, Magnus Karlsson wrote:
> >>>>>
> >>>>>> This patch proposes to add AF_XDP support to libbpf. The main reason
> >>>>>> for this is to facilitate writing applications that use AF_XDP by
> >>>>>> offering higher-level APIs that hide many of the details of the AF_XDP
> >>>>>> uapi. This is in the same vein as libbpf facilitates XDP adoption by
> >>>>>> offering easy-to-use higher level interfaces of XDP
> >>>>>> functionality. Hopefully this will facilitate adoption of AF_XDP, make
> >>>>>> applications using it simpler and smaller, and finally also make it
> >>>>>> possible for applications to benefit from optimizations in the AF_XDP
> >>>>>> user space access code. Previously, people just copied and pasted the
> >>>>>> code from the sample application into their application, which is not
> >>>>>> desirable.
> >>>>>
> >>>>> I like the idea of encapsulating the boilerplate logic in a library.
> >>>>>
> >>>>> I do think there is an important missing piece though - there should be
> >>>>> some code which queries the netdev for how many queues are attached, and
> >>>>> create the appropriate number of umem/AF_XDP sockets.
> >>>>>
> >>>>> I ran into this issue when testing the current AF_XDP code - on my test
> >>>>> boxes, the mlx5 card has 55 channels (aka queues), so when the test program
> >>>>> binds only to channel 0, nothing works as expected, since not all traffic
> >>>>> is being intercepted. While obvious in hindsight, this took a while to
> >>>>> track down.
> >>>>
> >>>> Yes, agreed. You are not the first one to stumble upon this problem
> >>>> :-). Let me think a little bit on how to solve this in a good way. We
> >>>> need this to be simple and intuitive, as you say.
> >>>
> >>> I see people hitting this with AF_XDP all the time... I had some
> >>> backup-slides[2] in our FOSDEM presentation[1] that describe the issue,
> >>> give the performance reason why and propose a workaround.
> >>
> >> Magnus, I presume you're going to address this for the initial libbpf merge
> >> since the plan is to make it easier to consume for users?
> >
> > I think the first thing we need is education and documentation. Have a
> > FAQ or "common mistakes" section in the Documentation. And of course,
> > sending Jesper around the world reminding people about this ;-).
> >
> > To address this on a libbpf interface level, I think the best way is
> > to reprogram the NIC to send all traffic to the queue that you
> > provided in the xsk_socket__create call. This "set up NIC routing"
> > behavior can then be disable with a flag, just as the XDP program
> > loading can be disabled. The standard config of xsk_socket__create
> > will then set up as many things for the user as possible just to get
> > up and running quickly. More advanced users can then disable parts of
> > it to gain more flexibility. Does this sound OK? Do not want to go the
> > route of polling multiple sockets and aggregating the traffic as this
> > will have significant negative performance implications.
>
> I think that is fine, I would probably make this one a dedicated API call
> in order to have some more flexibility than just simple flag. E.g. once
> nfp AF_XDP support lands at some point, I could imagine that this call
> resp. a drop-in replacement API call for more advanced steering could
> also take an offloaded BPF prog fd, for example, which then would program
> the steering on the NIC [0]. Seems at least there's enough complexity on
> its own to have a dedicated API for it. Thoughts?
I agree that there is probably enough complexity to warrant adding a
higher level API to deal with this problem (flow steering). But there
are likely a number of cases we have not thought that would complicate
it even further. This is why I suggest that this functionality should
be in its own patch set that I can devote some time and thought to.
IMO, the current patch set and functionality does already lower the
bar of entry significantly and has a value even without hiding or
controlling the steering of traffic. What I would like to do in this
patch set is to add a FAQ section in
Documentation/networking/af_xdp.rst explaining this problem. Something
like: "Q: Why am I not seeing any traffic? A: Check these four
things.....". Could add some text in the libbpf README referring to
this document also. Opinions?
Thanks: Magnus
> Thanks,
> Daniel
>
> [0] https://patchwork.ozlabs.org/cover/910614/
^ permalink raw reply
* [PATCH v2 1/2] net: phy: at803x: dont inline helpers
From: Vinod Koul @ 2019-02-18 10:18 UTC (permalink / raw)
To: David S Miller
Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez
Some helpers were inlined, but makes more sense to allow compiler
to do the right optiomazations instead, so remove inline for
at803x_disable_rx_delay() and at803x_disable_tx_delay()
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/net/phy/at803x.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 8ff12938ab47..c6e7d800fd7a 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -110,13 +110,13 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
return phy_write(phydev, AT803X_DEBUG_DATA, val);
}
-static inline int at803x_disable_rx_delay(struct phy_device *phydev)
+static int at803x_disable_rx_delay(struct phy_device *phydev)
{
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
AT803X_DEBUG_RX_CLK_DLY_EN, 0);
}
-static inline int at803x_disable_tx_delay(struct phy_device *phydev)
+static int at803x_disable_tx_delay(struct phy_device *phydev)
{
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
AT803X_DEBUG_TX_CLK_DLY_EN, 0);
--
2.20.1
^ permalink raw reply related
* [PATCH v2 2/2] net: phy: at803x: disable delay only for RGMII mode
From: Vinod Koul @ 2019-02-18 10:18 UTC (permalink / raw)
To: David S Miller
Cc: linux-arm-msm, Bjorn Andersson, Vinod Koul, netdev, Niklas Cassel,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez
In-Reply-To: <20190218101853.4290-1-vkoul@kernel.org>
Per "Documentation/devicetree/bindings/net/ethernet.txt" RGMII mode
should not have delay in PHY wheras RGMII_ID and RGMII_RXID/RGMII_TXID
can have delay in phy.
So disable the delay only for RGMII mode and enable for other modes.
Also treat the default case as disabled delays.
Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++----------
1 file changed, 36 insertions(+), 11 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c6e7d800fd7a..dc1b13f7fc12 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -110,6 +110,18 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
return phy_write(phydev, AT803X_DEBUG_DATA, val);
}
+static int at803x_enable_rx_delay(struct phy_device *phydev)
+{
+ return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
+ AT803X_DEBUG_RX_CLK_DLY_EN);
+}
+
+static int at803x_enable_tx_delay(struct phy_device *phydev)
+{
+ return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
+ AT803X_DEBUG_TX_CLK_DLY_EN);
+}
+
static int at803x_disable_rx_delay(struct phy_device *phydev)
{
return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
@@ -255,23 +267,36 @@ static int at803x_config_init(struct phy_device *phydev)
if (ret < 0)
return ret;
- if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII) {
- ret = at803x_disable_rx_delay(phydev);
+ /* The hardware register default is RX and TX delay enabled, so lets
+ * first disable the RX and TX delays in phy and enable them based
+ * on the mode selected
+ */
+ ret = at803x_disable_rx_delay(phydev);
+ if (ret < 0)
+ return ret;
+ ret = at803x_disable_tx_delay(phydev);
+ if (ret < 0)
+ return ret;
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+ /* If RGMII_ID or RGMII_RXID are specified enable RX delay,
+ * otherwise keep it disabled
+ */
+ ret = at803x_enable_rx_delay(phydev);
if (ret < 0)
return ret;
}
- if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
- phydev->interface == PHY_INTERFACE_MODE_RGMII) {
- ret = at803x_disable_tx_delay(phydev);
- if (ret < 0)
- return ret;
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+ /* If RGMII_ID or RGMII_TXID are specified enable TX delay,
+ * otherwise keep it disabled
+ */
+ ret = at803x_enable_tx_delay(phydev);
}
- return 0;
+ return ret;
}
static int at803x_ack_interrupt(struct phy_device *phydev)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] lib/test_rhashtable: fix spelling mistake "existant" -> "existent"
From: Herbert Xu @ 2019-02-18 10:23 UTC (permalink / raw)
To: Colin King; +Cc: Thomas Graf, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190217225209.17536-1-colin.king@canonical.com>
On Sun, Feb 17, 2019 at 10:52:09PM +0000, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There are spelling mistakes in warning macro messages. Fix them.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [v3 PATCH 0/4] mac80211: Fix incorrect usage of rhashtable walk API
From: Herbert Xu @ 2019-02-18 10:25 UTC (permalink / raw)
To: Johannes Berg
Cc: David Miller, linux-wireless, netdev, j, tgraf, Julia Lawall
In-Reply-To: <52bdb66f7e9e26ce9225ae11e37e22766d73694d.camel@sipsolutions.net>
On Fri, Feb 15, 2019 at 01:21:55PM +0100, Johannes Berg wrote:
>
> Applied those now, I'll send a pull request to Dave with them. Once that
> trickles back into net-next I'll apply the third patch (it doesn't apply
> without the others), and then Dave you can take the rhashtable one.
Thanks Johannes.
> Let me know if you'd prefer I take the rhashtable one through my tree,
> which really would be only so you don't have to track the dependency.
I don't mind.
> NB: it'd be easier in patchwork if you tagged all the patches with v3 in
> their own PATCH tag, or put the "v3" tag into the actual subject (not
> the "[PATCH 0/4]" tag because evidently patchwork drops the tags and
> doesn't track them for the *series* just each *patch* ... so with what
> you did nothing is visible in patchwork, even just appending "(v3)" to
> the subject of the cover letter would've fixed that...
Noted.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] net/mlx4_en: fix spelling mistake: "quiting" -> "quitting"
From: Dan Carpenter @ 2019-02-18 10:25 UTC (permalink / raw)
To: Tariq Toukan
Cc: Colin King, David S . Miller, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, kernel-janitors@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <2a1a6636-7795-addb-d7b7-711a23dbafa5@mellanox.com>
On Mon, Feb 18, 2019 at 09:37:22AM +0000, Tariq Toukan wrote:
>
>
> On 2/18/2019 1:03 AM, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > There is a spelling mistake in a en_err error message. Fix it.
> >
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > index 6b88881b8e35..c1438ae52a11 100644
> > --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c
> > @@ -3360,7 +3360,7 @@ int mlx4_en_init_netdev(struct mlx4_en_dev *mdev, int port,
> > dev->addr_len = ETH_ALEN;
> > mlx4_en_u64_to_mac(dev->dev_addr, mdev->dev->caps.def_mac[priv->port]);
> > if (!is_valid_ether_addr(dev->dev_addr)) {
> > - en_err(priv, "Port: %d, invalid mac burned: %pM, quiting\n",
> > + en_err(priv, "Port: %d, invalid mac burned: %pM, quitting\n",
> > priv->port, dev->dev_addr);
> > err = -EINVAL;
> > goto out;
> >
>
> Hi Colin, thanks for your patch.
>
> Reviewed-by: Tariq Toukan <tariqt@mellanox.com>
>
> I would suggest adding a Fixes line, but looking into the history of the
> typo, it went through many patches that modified this line but preserved
> the typo.
> Actually, it dates back to the very first commit that introduces mlx4
> driver:
>
> Patches history:
> 2b3ddf27f48c net/mlx4_core: Replace VF zero mac with random mac in mlx4_core
> ef96f7d46ad8 net/mlx4_en: Handle unassigned VF MAC address correctly
> 6bbb6d99f3d2 net/mlx4_en: Optimize Rx fast path filter checks
> 453a60827735 mlx4_en: Giving interface name in debug messages
> c27a02cd94d6 mlx4_en: Add driver for Mellanox ConnectX 10GbE NIC
>
> I'm not sure what the "Fixes:" policy is in these cases.
I wouldn't necessarily put a Fixes tag on this, because does fixing the
spelling really count as a bugfix? It's borderline whether it's a fix
or a cleanup.
regards,
daan carpenter
^ permalink raw reply
* Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Antoine Tenart @ 2019-02-18 10:26 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Antoine Tenart, davem, netdev, linux-kernel, thomas.petazzoni,
maxime.chevallier, gregory.clement, miquel.raynal, nadavh,
stefanc, ymarkman, mw
In-Reply-To: <20190215171224.sjfrid5csseywuks@shell.armlinux.org.uk>
Hi Russell,
On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote:
> > The documentation advises to set the XPCS in reset while reconfiguring
> > the serdes lanes. This seems to be a good thing to do, but the PPv2
> > driver wasn't doing it. This patch fixes it.
>
> Hmm. That statment seems to have some ambiguity in it - we do two
> "reconfigurations" - one may be upon initialisation, where the lane
> is already configured for 10Gbase-KR, and we're re-initialising it
> for the same mode. The other case is when we're switching between
> 10Gbase-KR and SGMII, or as will be the case with 2.5G support for
> the Alaska PHYs, 2500base-X.
The configuration at the lane at boot time is dependent to the
firmware or bootloader configuration. On the mcbin, the lane may be
configured in 10Gbase-KR, but it could be configured in SGMII as well.
The configuration upon initialization and the re-configuration are quite
similar then, as we might change mode as well at boot time.
You're right in that we might be re-configuring the lane for the same
exact mode at boot time, if it was already configured in the same mode.
> Does this apply to reconfiguration of the serdes lane between
> 10Gbase-KR and slower modes?
This applies only when configuring a line in a 10G mode,
mvpp22_gop_init_10gkr isn't called otherwise.
When switching to an non-10G mode we might want to put the XPCS in reset
though, which is not done today with this patch.
Thanks,
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v2 1/2] net: phy: at803x: dont inline helpers
From: Niklas Cassel @ 2019-02-18 10:28 UTC (permalink / raw)
To: Vinod Koul
Cc: David S Miller, linux-arm-msm, Bjorn Andersson, netdev,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez
In-Reply-To: <20190218101853.4290-1-vkoul@kernel.org>
On Mon, Feb 18, 2019 at 03:48:52PM +0530, Vinod Koul wrote:
> Some helpers were inlined, but makes more sense to allow compiler
> to do the right optiomazations instead, so remove inline for
> at803x_disable_rx_delay() and at803x_disable_tx_delay()
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/net/phy/at803x.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 8ff12938ab47..c6e7d800fd7a 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -110,13 +110,13 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
> return phy_write(phydev, AT803X_DEBUG_DATA, val);
> }
>
> -static inline int at803x_disable_rx_delay(struct phy_device *phydev)
> +static int at803x_disable_rx_delay(struct phy_device *phydev)
> {
> return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> AT803X_DEBUG_RX_CLK_DLY_EN, 0);
> }
>
> -static inline int at803x_disable_tx_delay(struct phy_device *phydev)
> +static int at803x_disable_tx_delay(struct phy_device *phydev)
> {
> return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
> AT803X_DEBUG_TX_CLK_DLY_EN, 0);
> --
> 2.20.1
>
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
^ permalink raw reply
* Re: [PATCH v2 2/2] net: phy: at803x: disable delay only for RGMII mode
From: Niklas Cassel @ 2019-02-18 10:28 UTC (permalink / raw)
To: Vinod Koul
Cc: David S Miller, linux-arm-msm, Bjorn Andersson, netdev,
Andrew Lunn, Florian Fainelli, Nori, Sekhar, Peter Ujfalusi,
Marc Gonzalez
In-Reply-To: <20190218101853.4290-2-vkoul@kernel.org>
On Mon, Feb 18, 2019 at 03:48:53PM +0530, Vinod Koul wrote:
> Per "Documentation/devicetree/bindings/net/ethernet.txt" RGMII mode
> should not have delay in PHY wheras RGMII_ID and RGMII_RXID/RGMII_TXID
> can have delay in phy.
>
> So disable the delay only for RGMII mode and enable for other modes.
> Also treat the default case as disabled delays.
>
> Fixes: cd28d1d6e52e: ("net: phy: at803x: Disable phy delay for RGMII mode")
> Reported-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/net/phy/at803x.c | 47 ++++++++++++++++++++++++++++++----------
> 1 file changed, 36 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index c6e7d800fd7a..dc1b13f7fc12 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -110,6 +110,18 @@ static int at803x_debug_reg_mask(struct phy_device *phydev, u16 reg,
> return phy_write(phydev, AT803X_DEBUG_DATA, val);
> }
>
> +static int at803x_enable_rx_delay(struct phy_device *phydev)
> +{
> + return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0, 0,
> + AT803X_DEBUG_RX_CLK_DLY_EN);
> +}
> +
> +static int at803x_enable_tx_delay(struct phy_device *phydev)
> +{
> + return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
> + AT803X_DEBUG_TX_CLK_DLY_EN);
> +}
> +
> static int at803x_disable_rx_delay(struct phy_device *phydev)
> {
> return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
> @@ -255,23 +267,36 @@ static int at803x_config_init(struct phy_device *phydev)
> if (ret < 0)
> return ret;
>
> - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> - phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> - ret = at803x_disable_rx_delay(phydev);
> + /* The hardware register default is RX and TX delay enabled, so lets
> + * first disable the RX and TX delays in phy and enable them based
> + * on the mode selected
> + */
> + ret = at803x_disable_rx_delay(phydev);
> + if (ret < 0)
> + return ret;
> + ret = at803x_disable_tx_delay(phydev);
> + if (ret < 0)
> + return ret;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> + /* If RGMII_ID or RGMII_RXID are specified enable RX delay,
> + * otherwise keep it disabled
> + */
> + ret = at803x_enable_rx_delay(phydev);
> if (ret < 0)
> return ret;
> }
>
> - if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> - phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> - phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> - ret = at803x_disable_tx_delay(phydev);
> - if (ret < 0)
> - return ret;
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + /* If RGMII_ID or RGMII_TXID are specified enable TX delay,
> + * otherwise keep it disabled
> + */
> + ret = at803x_enable_tx_delay(phydev);
> }
>
> - return 0;
> + return ret;
> }
>
> static int at803x_ack_interrupt(struct phy_device *phydev)
> --
> 2.20.1
>
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
^ permalink raw reply
* [PATCH net] mac80211: mesh: fix missing unlock on error in table_path_del()
From: Johannes Berg @ 2019-02-18 10:29 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: Herbert Xu, Wei Yongjun
From: Wei Yongjun <weiyongjun1@huawei.com>
spin_lock_bh() is used in table_path_del() but rcu_read_unlock()
is used for unlocking. Fix it by using spin_unlock_bh() instead
of rcu_read_unlock() in the error handling case.
Fixes: b4c3fbe63601 ("mac80211: Use linked list instead of rhashtable walk for mesh tables")
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Dave, can you take this directly? I see little value in sending a
pull request for this one patch (but if you prefer can do so.)
---
net/mac80211/mesh_pathtbl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/mesh_pathtbl.c b/net/mac80211/mesh_pathtbl.c
index c3a7396fb955..88a6d5e18ccc 100644
--- a/net/mac80211/mesh_pathtbl.c
+++ b/net/mac80211/mesh_pathtbl.c
@@ -627,7 +627,7 @@ static int table_path_del(struct mesh_table *tbl,
spin_lock_bh(&tbl->walk_lock);
mpath = rhashtable_lookup_fast(&tbl->rhead, addr, mesh_rht_params);
if (!mpath) {
- rcu_read_unlock();
+ spin_unlock_bh(&tbl->walk_lock);
return -ENXIO;
}
--
2.17.2
^ permalink raw reply related
* Re: [PATCH net-next 01/13] net: mvpp2: do not call phylink_mac_change if there is no event
From: Antoine Tenart @ 2019-02-18 10:40 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Antoine Tenart, davem, netdev, linux-kernel, thomas.petazzoni,
maxime.chevallier, gregory.clement, miquel.raynal, nadavh,
stefanc, ymarkman, mw
In-Reply-To: <20190215170553.ur725djxoudtnjne@shell.armlinux.org.uk>
Hi Russell,
On Fri, Feb 15, 2019 at 05:05:53PM +0000, Russell King - ARM Linux admin wrote:
> On Fri, Feb 15, 2019 at 04:32:29PM +0100, Antoine Tenart wrote:
> > This patch makes the link interrupt handler to avoid calling
> > phylink_mac_change when there are no event.
>
> The reasoning being?
The interrupt can fire for a number of reasons, some of which may not be
related to changes in the MAC state (it can fire upon errors or counter
wraparound).
This interrupt handler currently checks if an event occurred bu checking
if the cause of the interrupt is a link change. I wanted to extend this
logic to phylink to avoid having two link change handlers (phylink and
!phylink) being called in different ways.
Anyway, the only cause unmask so far is the link one, so it won't make
any difference so far. We can probably drop this one for now.
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Russell King - ARM Linux admin @ 2019-02-18 10:43 UTC (permalink / raw)
To: Antoine Tenart
Cc: davem, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier,
gregory.clement, miquel.raynal, nadavh, stefanc, ymarkman, mw
In-Reply-To: <20190218102630.GA3784@kwain>
On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote:
> Hi Russell,
>
> On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote:
> > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote:
> > > The documentation advises to set the XPCS in reset while reconfiguring
> > > the serdes lanes. This seems to be a good thing to do, but the PPv2
> > > driver wasn't doing it. This patch fixes it.
> >
> > Hmm. That statment seems to have some ambiguity in it - we do two
> > "reconfigurations" - one may be upon initialisation, where the lane
> > is already configured for 10Gbase-KR, and we're re-initialising it
> > for the same mode. The other case is when we're switching between
> > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for
> > the Alaska PHYs, 2500base-X.
>
> The configuration at the lane at boot time is dependent to the
> firmware or bootloader configuration. On the mcbin, the lane may be
> configured in 10Gbase-KR, but it could be configured in SGMII as well.
> The configuration upon initialization and the re-configuration are quite
> similar then, as we might change mode as well at boot time.
>
> You're right in that we might be re-configuring the lane for the same
> exact mode at boot time, if it was already configured in the same mode.
>
> > Does this apply to reconfiguration of the serdes lane between
> > 10Gbase-KR and slower modes?
>
> This applies only when configuring a line in a 10G mode,
> mvpp22_gop_init_10gkr isn't called otherwise.
>
> When switching to an non-10G mode we might want to put the XPCS in reset
> though, which is not done today with this patch.
I'm merely pointing out the discrepency between the commit message and
what is actually being done. I'm not particularly concerned about what
happens at boot.
We have four different transitions a port can go through, all of which
reconfigure the serdes lanes:
1. 10gkr -> 10gkr
2. 10gkr -> non-10gkr
3. non-10gkr -> non-10gkr
4. non-10gkr -> 10gkr
With this patch, XPCS is only placed into reset during the
reconfiguration for cases 1 and 4. Case 3 doesn't matter (the XPCS
should already be in reset right?) Case 2 isn't covered, and this
leaves a rather big hole.
It seems to me that if the documentation states that the XPCS needs to
be placed in reset while the serdes is reconfigured, then what should
be happening is:
- at boot, place the XPCS into reset.
- when we configure for 10gkr, release the reset once we've finished
configuring the serdes.
- when we configure away from 10gkr, place the XPCS back into reset
before configuring the serdes.
Merely placing the XPCS into reset while we configure the serdes for
10gkr doesn't seem to be "fixing" the driver to conform to your commit
message opening statement.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [RFC] net: dsa: qca8k: implement rgmii-id mode
From: Vinod Koul @ 2019-02-18 10:45 UTC (permalink / raw)
To: Andrew Lunn
Cc: Michal Vokáč, David S. Miller, netdev,
linux-kernel@vger.kernel.org, Florian Fainelli
In-Reply-To: <20190215152356.GP708@lunn.ch>
On 15-02-19, 16:23, Andrew Lunn wrote:
> On Fri, Feb 15, 2019 at 04:01:08PM +0100, Michal Vokáč wrote:
> > Hi,
> >
> > networking on my boards [1], which are currently in linux-next, suddently
> > stopped working. I tracked it down to this commit 5ecdd77c61c8 ("net: dsa:
> > qca8k: disable delay for RGMII mode") [2].
> >
> > So I think the rgmii-id mode is obviously needed in my case.
> > I was able to find a couple drivers that read tx/rx-delay or
> > tx/rx-internal-delay from device tree. Namely:
> >
> > drivers/net/ethernet/apm/xgene/xgene_enet_main.c
> > drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> > drivers/net/ethernet/stmicro/stmmac/dwmac-meson8b.c
> > drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> > drivers/net/phy/dp83867.c
> >
> > I would appreciate any hints how to add similar function to qca8k driver
> > if that is the correct way to go. Can I take some of the above mentioned
> > drivers as a good example for that? How should the binding look like?
> >
> > I would expect something like this:
> >
> > switch@0 {
> > compatible = "qca,qca8334";
> > reg = <0>;
> >
> > switch_ports: ports {
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > ethphy0: port@0 {
> > reg = <0>;
> > label = "cpu";
> > phy-mode = "rgmii-id";
> > qca,tx-delay = <3>;
> > qca,rx-delay = <3>;
> > ethernet = <&fec>;
> > };
>
> Hi Michal
>
> Your submission used:
>
> + ethphy0: port@0 {
> + reg = <0>;
> + label = "cpu";
> + phy-mode = "rgmii";
> + ethernet = <&fec>;
> +
> + fixed-link {
> + speed = <1000>;
> + full-duplex;
> + };
> + };
>
> This is good. If you have a fixed-link you can pass a phy-mode.
>
> The comment that was removed was:
>
> - /* According to the datasheet, RGMII delay is enabled through
> - * PORT5_PAD_CTRL for all ports, rather than individual port
> - * registers
> - */
>
> Is it possible to enable delays per port? Ideally, you want to enable
> delays for just selected ports. Add another case for
> PHY_INTERFACE_MODE_RGMII_ID to enable the delays.
In the hindsight I should not have removed the comment, let me ressurect
that as well as add handling of the RGMII modes...
Please do test
Thanks
--
~Vinod
^ permalink raw reply
* Re: Possible bug into DSA2 code.
From: Rodolfo Giometti @ 2019-02-18 10:38 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn; +Cc: Vivien Didelot, David S. Miller, netdev
In-Reply-To: <24527f56-41dd-f257-0f5e-c568cb80881e@gmail.com>
On 11/02/2019 20:13, Florian Fainelli wrote:
> Does that work:
>
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index a1917025e155..54cf6a5c865d 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -368,6 +368,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
> if (err)
> return err;
>
> + if (ds->slave_mii_bus && (ds->ops->phy_read || ds->ops->phy_write))
> + return -EINVAL;
> +
> if (!ds->slave_mii_bus && ds->ops->phy_read) {
> ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> if (!ds->slave_mii_bus)
> diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
> index cb42939db776..0796c6213be6 100644
> --- a/net/dsa/legacy.c
> +++ b/net/dsa/legacy.c
> @@ -176,6 +176,9 @@ static int dsa_switch_setup_one(struct dsa_switch *ds,
> if (ret)
> return ret;
>
> + if (ds->slave_mii_bus && (ops->phy_read || ops->phy_write))
> + return -EINVAL;
> +
> if (!ds->slave_mii_bus && ops->phy_read) {
> ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> if (!ds->slave_mii_bus)
OK, now probing of mv88e6085 fails!
[ 42.745004] mv88e6085: probe of d0032004.mdio-mii:01 failed with error -22
Now I suppose mv88e6085's driver should set ds->slave_mii_bus as follow:
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 54a5b660640a..bb46ebbb2bb8 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -2399,7 +2399,7 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
int i;
chip->ds = ds;
- ds->slave_mii_bus = mv88e6xxx_default_mdio_bus(chip);
+ ds->slave_mii_bus = NULL;
mutex_lock(&chip->reg_lock);
Is that right?
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com
Linux Device Driver giometti@linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
^ permalink raw reply related
* Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Russell King - ARM Linux admin @ 2019-02-18 10:47 UTC (permalink / raw)
To: Antoine Tenart
Cc: davem, netdev, linux-kernel, thomas.petazzoni, maxime.chevallier,
gregory.clement, miquel.raynal, nadavh, stefanc, ymarkman, mw
In-Reply-To: <20190218104302.bp6ccmpkt26dflyx@shell.armlinux.org.uk>
On Mon, Feb 18, 2019 at 10:43:02AM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote:
> > Hi Russell,
> >
> > On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote:
> > > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote:
> > > > The documentation advises to set the XPCS in reset while reconfiguring
> > > > the serdes lanes. This seems to be a good thing to do, but the PPv2
> > > > driver wasn't doing it. This patch fixes it.
> > >
> > > Hmm. That statment seems to have some ambiguity in it - we do two
> > > "reconfigurations" - one may be upon initialisation, where the lane
> > > is already configured for 10Gbase-KR, and we're re-initialising it
> > > for the same mode. The other case is when we're switching between
> > > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for
> > > the Alaska PHYs, 2500base-X.
> >
> > The configuration at the lane at boot time is dependent to the
> > firmware or bootloader configuration. On the mcbin, the lane may be
> > configured in 10Gbase-KR, but it could be configured in SGMII as well.
> > The configuration upon initialization and the re-configuration are quite
> > similar then, as we might change mode as well at boot time.
> >
> > You're right in that we might be re-configuring the lane for the same
> > exact mode at boot time, if it was already configured in the same mode.
> >
> > > Does this apply to reconfiguration of the serdes lane between
> > > 10Gbase-KR and slower modes?
> >
> > This applies only when configuring a line in a 10G mode,
> > mvpp22_gop_init_10gkr isn't called otherwise.
> >
> > When switching to an non-10G mode we might want to put the XPCS in reset
> > though, which is not done today with this patch.
>
> I'm merely pointing out the discrepency between the commit message and
> what is actually being done. I'm not particularly concerned about what
> happens at boot.
>
> We have four different transitions a port can go through, all of which
> reconfigure the serdes lanes:
>
> 1. 10gkr -> 10gkr
> 2. 10gkr -> non-10gkr
> 3. non-10gkr -> non-10gkr
> 4. non-10gkr -> 10gkr
>
> With this patch, XPCS is only placed into reset during the
> reconfiguration for cases 1 and 4. Case 3 doesn't matter (the XPCS
> should already be in reset right?) Case 2 isn't covered, and this
> leaves a rather big hole.
>
> It seems to me that if the documentation states that the XPCS needs to
> be placed in reset while the serdes is reconfigured, then what should
> be happening is:
>
> - at boot, place the XPCS into reset.
> - when we configure for 10gkr, release the reset once we've finished
> configuring the serdes.
> - when we configure away from 10gkr, place the XPCS back into reset
> before configuring the serdes.
>
> Merely placing the XPCS into reset while we configure the serdes for
> 10gkr doesn't seem to be "fixing" the driver to conform to your commit
> message opening statement.
Another case that needs to be considered: if the XPCS should be placed
into reset while reconfiguring the serdes lanes, is the same treatment
needed for the GMAC?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH RESEND 0/3] Add quirk for reading BD_ADDR from fwnode property
From: Marcel Holtmann @ 2019-02-18 10:48 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: Johan Hedberg, David S. Miller, Loic Poulain, linux-bluetooth,
linux-kernel, netdev, Balakrishna Godavarthi
In-Reply-To: <20190131221021.176809-1-mka@chromium.org>
Hi Matthias,
> [initial post: https://lore.kernel.org/patchwork/cover/1028184/]
>
> On some systems the Bluetooth Device Address (BD_ADDR) isn't stored
> on the Bluetooth chip itself. One way to configure the address is
> through the device tree (patched in by the bootloader). The btqcomsmd
> driver is an example, it can read the address from the DT property
> 'local-bd-address'.
>
> To avoid redundant open-coded reading of 'local-bd-address' and error
> handling this series adds the quirk HCI_QUIRK_USE_BDADDR_PROPERTY to
> retrieve the BD address of a device from the DT and adapts the
> btqcomsmd and hci_qca drivers to use this quirk.
>
> Matthias Kaehlcke (3):
> Bluetooth: Add quirk for reading BD_ADDR from fwnode property
> Bluetooth: btqcomsmd: use HCI_QUIRK_USE_BDADDR_PROPERTY
> Bluetooth: hci_qca: Set HCI_QUIRK_USE_BDADDR_PROPERTY for wcn3990
>
> drivers/bluetooth/btqcomsmd.c | 29 +++--------------------
> drivers/bluetooth/hci_qca.c | 1 +
> include/net/bluetooth/hci.h | 12 ++++++++++
> net/bluetooth/hci_core.c | 43 +++++++++++++++++++++++++++++++++++
> net/bluetooth/mgmt.c | 6 +++--
> 5 files changed, 63 insertions(+), 28 deletions(-)
I am getting compiler warnings when trying to apply this set:
CC drivers/bluetooth/btqcomsmd.o
drivers/bluetooth/btqcomsmd.c: In function ‘btqcomsmd_setup’:
drivers/bluetooth/btqcomsmd.c:120:6: warning: unused variable ‘err’ [-Wunused-variable]
int err;
^~~
drivers/bluetooth/btqcomsmd.c:118:20: warning: unused variable ‘btq’ [-Wunused-variable]
struct btqcomsmd *btq = hci_get_drvdata(hdev);
^~~
Regards
Marcel
^ permalink raw reply
* Re: linux-next: manual merge of the net-next tree with the rdma tree
From: Leon Romanovsky @ 2019-02-18 10:48 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, Networking, Doug Ledford, Jason Gunthorpe,
Linux Next Mailing List, Linux Kernel Mailing List, Mark Bloch,
Bodong Wang, Saeed Mahameed
In-Reply-To: <20190218110549.7f22feda@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]
On Mon, Feb 18, 2019 at 11:05:49AM +1100, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the net-next tree got a conflict in:
>
> drivers/infiniband/hw/mlx5/ib_rep.c
>
> between commits:
>
> 459cc69fa4c1 ("RDMA: Provide safe ib_alloc_device() function")
> fc9e4477f924 ("RDMA/mlx5: Fix memory leak in case we fail to add an IB device")
>
> from the rdma tree and commit:
>
> f0666f1f22b5 ("IB/mlx5: Use unified register/load function for uplink and VF vports")
>
> from the net-next tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
Thanks a lot for your resolution.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Antoine Tenart @ 2019-02-18 10:50 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Antoine Tenart, davem, netdev, linux-kernel, thomas.petazzoni,
maxime.chevallier, gregory.clement, miquel.raynal, nadavh,
stefanc, ymarkman, mw
In-Reply-To: <20190218104302.bp6ccmpkt26dflyx@shell.armlinux.org.uk>
Russell,
On Mon, Feb 18, 2019 at 10:43:02AM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Feb 18, 2019 at 11:26:30AM +0100, Antoine Tenart wrote:
> > On Fri, Feb 15, 2019 at 05:12:24PM +0000, Russell King - ARM Linux admin wrote:
> > > On Fri, Feb 15, 2019 at 04:32:38PM +0100, Antoine Tenart wrote:
> > > > The documentation advises to set the XPCS in reset while reconfiguring
> > > > the serdes lanes. This seems to be a good thing to do, but the PPv2
> > > > driver wasn't doing it. This patch fixes it.
> > >
> > > Hmm. That statment seems to have some ambiguity in it - we do two
> > > "reconfigurations" - one may be upon initialisation, where the lane
> > > is already configured for 10Gbase-KR, and we're re-initialising it
> > > for the same mode. The other case is when we're switching between
> > > 10Gbase-KR and SGMII, or as will be the case with 2.5G support for
> > > the Alaska PHYs, 2500base-X.
> >
> > The configuration at the lane at boot time is dependent to the
> > firmware or bootloader configuration. On the mcbin, the lane may be
> > configured in 10Gbase-KR, but it could be configured in SGMII as well.
> > The configuration upon initialization and the re-configuration are quite
> > similar then, as we might change mode as well at boot time.
> >
> > You're right in that we might be re-configuring the lane for the same
> > exact mode at boot time, if it was already configured in the same mode.
> >
> > > Does this apply to reconfiguration of the serdes lane between
> > > 10Gbase-KR and slower modes?
> >
> > This applies only when configuring a line in a 10G mode,
> > mvpp22_gop_init_10gkr isn't called otherwise.
> >
> > When switching to an non-10G mode we might want to put the XPCS in reset
> > though, which is not done today with this patch.
>
> I'm merely pointing out the discrepency between the commit message and
> what is actually being done. I'm not particularly concerned about what
> happens at boot.
>
> We have four different transitions a port can go through, all of which
> reconfigure the serdes lanes:
>
> 1. 10gkr -> 10gkr
> 2. 10gkr -> non-10gkr
> 3. non-10gkr -> non-10gkr
> 4. non-10gkr -> 10gkr
>
> With this patch, XPCS is only placed into reset during the
> reconfiguration for cases 1 and 4. Case 3 doesn't matter (the XPCS
> should already be in reset right?) Case 2 isn't covered, and this
> leaves a rather big hole.
>
> It seems to me that if the documentation states that the XPCS needs to
> be placed in reset while the serdes is reconfigured, then what should
> be happening is:
>
> - at boot, place the XPCS into reset.
> - when we configure for 10gkr, release the reset once we've finished
> configuring the serdes.
> - when we configure away from 10gkr, place the XPCS back into reset
> before configuring the serdes.
>
> Merely placing the XPCS into reset while we configure the serdes for
> 10gkr doesn't seem to be "fixing" the driver to conform to your commit
> message opening statement.
That's right, we should put the XPCS in reset when booting and when
switching away from a 10G mode. I'll fix that in v2.
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Antoine Tenart @ 2019-02-18 10:52 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Antoine Tenart, davem, netdev, linux-kernel, thomas.petazzoni,
maxime.chevallier, gregory.clement, miquel.raynal, nadavh,
stefanc, ymarkman, mw
In-Reply-To: <20190218104757.be63b5ft5jpongn3@shell.armlinux.org.uk>
Russell,
On Mon, Feb 18, 2019 at 10:47:57AM +0000, Russell King - ARM Linux admin wrote:
>
> Another case that needs to be considered: if the XPCS should be placed
> into reset while reconfiguring the serdes lanes, is the same treatment
> needed for the GMAC?
That's something I wanted to check as well. I don't know for the GMAC,
while I'm sure the documentation explicitly state to put the XPCS in
reset when reconfiguring the lanes.
Antoine
--
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH net-next v4 07/17] net: sched: protect filter_chain list with filter_chain_lock mutex
From: Vlad Buslov @ 2019-02-18 11:06 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAM_iQpVJ69Uib2Y4GqVZ4eHd25JrUZCa=d4ZGq4WCd+Q4vVi1g@mail.gmail.com>
On Fri 15 Feb 2019 at 22:35, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 11, 2019 at 12:56 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> +#ifdef CONFIG_PROVE_LOCKING
>> +static inline bool lockdep_tcf_chain_is_locked(struct tcf_chain *chain)
>> +{
>> + return lockdep_is_held(&chain->filter_chain_lock);
>> +}
>> +#else
>> +static inline bool lockdep_tcf_chain_is_locked(struct tcf_block *chain)
>> +{
>> + return true;
>> +}
>> +#endif /* #ifdef CONFIG_PROVE_LOCKING */
>> +
>> +#define tcf_chain_dereference(p, chain) \
>> + rcu_dereference_protected(p, lockdep_tcf_chain_is_locked(chain))
>
>
> Are you sure you need this #ifdef CONFIG_PROVE_LOCKING?
> rcu_dereference_protected() should already test CONFIG_PROVE_RCU.
>
> Ditto for tcf_proto_dereference().
I implemented these macro same way as rtnl_dereference() is implemented,
which they are intended to substitute.
After removing them I get following compilation error with
CONFIG_PROVE_LOCKING disabled:
./include/net/sch_generic.h: In function ‘lockdep_tcf_chain_is_locked’:
./include/net/sch_generic.h:404:9: error: implicit declaration of function ‘lockdep_is_held’; did you mean ‘lockdep_rtnl_is_held’? [-Werror=implicit-function-declaration]
return lockdep_is_held(&chain->filter_chain_lock);
^~~~~~~~~~~~~~~
lockdep_rtnl_is_held
^ permalink raw reply
* RE: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while reconfiguring the serdes lanes
From: Stefan Chulski @ 2019-02-18 11:08 UTC (permalink / raw)
To: Antoine Tenart, Russell King - ARM Linux admin
Cc: davem@davemloft.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, thomas.petazzoni@bootlin.com,
maxime.chevallier@bootlin.com, gregory.clement@bootlin.com,
miquel.raynal@bootlin.com, Nadav Haklai, Yan Markman,
mw@semihalf.com
In-Reply-To: <20190218105222.GD3784@kwain>
> -----Original Message-----
> From: Antoine Tenart <antoine.tenart@bootlin.com>
> Sent: Monday, February 18, 2019 12:52 PM
> To: Russell King - ARM Linux admin <linux@armlinux.org.uk>
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; davem@davemloft.net;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> thomas.petazzoni@bootlin.com; maxime.chevallier@bootlin.com;
> gregory.clement@bootlin.com; miquel.raynal@bootlin.com; Nadav Haklai
> <nadavh@marvell.com>; Stefan Chulski <stefanc@marvell.com>; Yan
> Markman <ymarkman@marvell.com>; mw@semihalf.com
> Subject: [EXT] Re: [PATCH net-next 10/13] net: mvpp2: reset the XPCS while
> reconfiguring the serdes lanes
>
> External Email
>
> ----------------------------------------------------------------------
> Russell,
>
> On Mon, Feb 18, 2019 at 10:47:57AM +0000, Russell King - ARM Linux admin
> wrote:
> >
> > Another case that needs to be considered: if the XPCS should be placed
> > into reset while reconfiguring the serdes lanes, is the same treatment
> > needed for the GMAC?
>
> That's something I wanted to check as well. I don't know for the GMAC, while
> I'm sure the documentation explicitly state to put the XPCS in reset when
> reconfiguring the lanes.
HW recommendation upon Serdes reconfiguration are the following:
1. Disable port(CTRL0_REG - in XLG/GMAC)
2. Put port in reset (both XLG/GMAC)
3. For KR - put in reset MPCS (MAC control clock, RX SD clock, TX SD clock), XPSC is RXAUI/XAUI clock domain
4. Power down Serdes lane
Do reconfiguration of Serdes.
5. Enable Serdes lane
6. Disable MPCS reset for KR
7. Disable port reset (both XLG/GMAC)
8. Enable port (both XLG/GMAC)
Stefan,
Best Regards.
^ permalink raw reply
* Re: [PATCH net-next v4 10/17] net: sched: refactor tp insert/delete for concurrent execution
From: Vlad Buslov @ 2019-02-18 11:19 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAM_iQpVdO5G9a+C4FecORHOuO7uKu6=fqKa-CUTyFMd_heyU+Q@mail.gmail.com>
On Fri 15 Feb 2019 at 23:17, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 11, 2019 at 12:56 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> +static bool tcf_proto_is_empty(struct tcf_proto *tp)
>> +{
>> + struct tcf_walker walker = { .fn = walker_noop, };
>> +
>> + if (tp->ops->walk) {
>> + tp->ops->walk(tp, &walker);
>> + return !walker.stop;
>> + }
>> + return true;
>> +}
>> +
>> +static bool tcf_proto_check_delete(struct tcf_proto *tp)
>> +{
>> + spin_lock(&tp->lock);
>> + if (tcf_proto_is_empty(tp))
>> + tp->deleting = true;
>> + spin_unlock(&tp->lock);
>> + return tp->deleting;
>
> If you use this spinlock for walking each tp data structure,
> why it is not needed for adding to/deleting filters from each
> tp?
This lock is intended to be used by unlocked classifiers and I use it in
my following flower patch set extensively. Classifiers that do not set
'unlocked' flag continue to rely on rtnl lock for synchronization.
^ permalink raw reply
* Re: [PATCH bpf-next v4 1/2] libbpf: add support for using AF_XDP sockets
From: Maciej Fijalkowski @ 2019-02-18 11:21 UTC (permalink / raw)
To: Magnus Karlsson
Cc: Daniel Borkmann, Magnus Karlsson, Björn Töpel, ast,
Network Development, Jakub Kicinski, Björn Töpel,
Zhang, Qi Z, Jesper Dangaard Brouer, xiaolong.ye
In-Reply-To: <CAJ8uoz1xjv8OBAn==S3EBiGvTqp_dd_t7YQqp2TOSR6JSOdrnA@mail.gmail.com>
On Mon, 18 Feb 2019 09:59:30 +0100
Magnus Karlsson <magnus.karlsson@gmail.com> wrote:
> On Fri, Feb 15, 2019 at 6:09 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >
> > On 02/08/2019 02:05 PM, Magnus Karlsson wrote:
> > > This commit adds AF_XDP support to libbpf. The main reason for this is
> > > to facilitate writing applications that use AF_XDP by offering
> > > higher-level APIs that hide many of the details of the AF_XDP
> > > uapi. This is in the same vein as libbpf facilitates XDP adoption by
> > > offering easy-to-use higher level interfaces of XDP
> > > functionality. Hopefully this will facilitate adoption of AF_XDP, make
> > > applications using it simpler and smaller, and finally also make it
> > > possible for applications to benefit from optimizations in the AF_XDP
> > > user space access code. Previously, people just copied and pasted the
> > > code from the sample application into their application, which is not
> > > desirable.
> > >
> > > The interface is composed of two parts:
> > >
> > > * Low-level access interface to the four rings and the packet
> > > * High-level control plane interface for creating and setting
> > > up umems and af_xdp sockets as well as a simple XDP program.
> > >
> > > Tested-by: Björn Töpel <bjorn.topel@intel.com>
> > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > [...]
> > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > > new file mode 100644
> > > index 0000000..a982a76
> > > --- /dev/null
> > > +++ b/tools/lib/bpf/xsk.c
> > > @@ -0,0 +1,742 @@
> > > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
> > > +
> > > +/*
> > > + * AF_XDP user-space access library.
> > > + *
> > > + * Copyright(c) 2018 - 2019 Intel Corporation.
> > > + *
> > > + * Author(s): Magnus Karlsson <magnus.karlsson@intel.com>
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <unistd.h>
> > > +#include <arpa/inet.h>
> > > +#include <asm/barrier.h>
> > > +#include <linux/compiler.h>
> > > +#include <linux/filter.h>
> > > +#include <linux/if_ether.h>
> > > +#include <linux/if_link.h>
> > > +#include <linux/if_packet.h>
> > > +#include <linux/if_xdp.h>
> > > +#include <linux/rtnetlink.h>
> > > +#include <net/if.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/socket.h>
> > > +#include <sys/types.h>
> > > +
> > > +#include "bpf.h"
> > > +#include "libbpf.h"
> > > +#include "libbpf_util.h"
> > > +#include "nlattr.h"
> > > +#include "xsk.h"
> > > +
> > > +#ifndef SOL_XDP
> > > + #define SOL_XDP 283
> > > +#endif
> > > +
> > > +#ifndef AF_XDP
> > > + #define AF_XDP 44
> > > +#endif
> > > +
> > > +#ifndef PF_XDP
> > > + #define PF_XDP AF_XDP
> > > +#endif
> > > +
> > > +struct xsk_umem {
> > > + struct xsk_ring_prod *fill;
> > > + struct xsk_ring_cons *comp;
> > > + char *umem_area;
> > > + struct xsk_umem_config config;
> > > + int fd;
> > > + int refcount;
> > > +};
> > > +
> > > +struct xsk_socket {
> > > + struct xsk_ring_cons *rx;
> > > + struct xsk_ring_prod *tx;
> > > + __u64 outstanding_tx;
> > > + struct xsk_umem *umem;
> > > + struct xsk_socket_config config;
> > > + int fd;
> > > + int xsks_map;
> > > + int ifindex;
> > > + int prog_fd;
> > > + int qidconf_map_fd;
> > > + int xsks_map_fd;
> > > + __u32 queue_id;
> > > +};
> > > +
> > > +struct xsk_nl_info {
> > > + bool xdp_prog_attached;
> > > + int ifindex;
> > > + int fd;
> > > +};
> > > +
> > > +#define MAX_QUEUES 128
> >
> > Why is this a fixed constant here, shouldn't this be dynamic due to being NIC
> > specific anyway?
>
> It was only here for simplicity. If a NIC had more queues, it would
> require a recompile of the lib. Obviously, not desirable in a distro.
> What I could do is to read the max "combined" queues (pre-set maximum
> in the ethtool output) from the same interface as ethool uses and size
> the array after that. Or is there a simpler way? What to do if the NIC
> does not have a "combined", or is there no such NIC (seems the common
> HW ones set this)?
>
> > [...]
> > > +void *xsk_umem__get_data(struct xsk_umem *umem, __u64 addr)
> > > +{
> > > + return &((char *)(umem->umem_area))[addr];
> > > +}
> >
> > There's also a xsk_umem__get_data_raw() doing the same. Why having both, resp.
> > when to choose which? ;)
>
> There is enough to have the xsk_umem__get_data_raw() function.
> xsk_umem__get_data() is just a convenience function for which the
> application does not have to store the beginning of the umem. But as
> the application always has to provide this anyway in the
> xsk_umem__create() function, it might as well store this pointer. I
> will delete xsk_umem__get_data() and rename xsk_umem__get_data_raw()
> to xsk_umem__get_data().
>
> > > +int xsk_umem__fd(const struct xsk_umem *umem)
> > > +{
> > > + return umem ? umem->fd : -EINVAL;
> > > +}
> > > +
> > > +int xsk_socket__fd(const struct xsk_socket *xsk)
> > > +{
> > > + return xsk ? xsk->fd : -EINVAL;
> > > +}
> > > +
> > > +static bool xsk_page_aligned(void *buffer)
> > > +{
> > > + unsigned long addr = (unsigned long)buffer;
> > > +
> > > + return !(addr & (getpagesize() - 1));
> > > +}
> > > +
> > > +static void xsk_set_umem_config(struct xsk_umem_config *cfg,
> > > + const struct xsk_umem_config *usr_cfg)
> > > +{
> > > + if (!usr_cfg) {
> > > + cfg->fill_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
> > > + cfg->comp_size = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> > > + cfg->frame_size = XSK_UMEM__DEFAULT_FRAME_SIZE;
> > > + cfg->frame_headroom = XSK_UMEM__DEFAULT_FRAME_HEADROOM;
> > > + return;
> > > + }
> > > +
> > > + cfg->fill_size = usr_cfg->fill_size;
> > > + cfg->comp_size = usr_cfg->comp_size;
> > > + cfg->frame_size = usr_cfg->frame_size;
> > > + cfg->frame_headroom = usr_cfg->frame_headroom;
> >
> > Just optional nit, might be a bit nicer to have it in this form:
> >
> > cfg->fill_size = usr_cfg ? usr_cfg->fill_size :
> > XSK_RING_PROD__DEFAULT_NUM_DESCS;
>
> I actually think the current form is clearer when there are multiple
> lines. If there was only one line, I would agree with you.
>
> > > +}
> > > +
> > > +static void xsk_set_xdp_socket_config(struct xsk_socket_config *cfg,
> > > + const struct xsk_socket_config *usr_cfg)
> > > +{
> > > + if (!usr_cfg) {
> > > + cfg->rx_size = XSK_RING_CONS__DEFAULT_NUM_DESCS;
> > > + cfg->tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS;
> > > + cfg->libbpf_flags = 0;
> > > + cfg->xdp_flags = 0;
> > > + cfg->bind_flags = 0;
> > > + return;
> > > + }
> > > +
> > > + cfg->rx_size = usr_cfg->rx_size;
> > > + cfg->tx_size = usr_cfg->tx_size;
> > > + cfg->libbpf_flags = usr_cfg->libbpf_flags;
> > > + cfg->xdp_flags = usr_cfg->xdp_flags;
> > > + cfg->bind_flags = usr_cfg->bind_flags;
> >
> > (Ditto)
> >
> > > +}
> > > +
> > > +int xsk_umem__create(struct xsk_umem **umem_ptr, void *umem_area, __u64 size,
> > > + struct xsk_ring_prod *fill, struct xsk_ring_cons *comp,
> > > + const struct xsk_umem_config *usr_config)
> > > +{
> > > + struct xdp_mmap_offsets off;
> > > + struct xdp_umem_reg mr;
> > > + struct xsk_umem *umem;
> > > + socklen_t optlen;
> > > + void *map;
> > > + int err;
> > > +
> > > + if (!umem_area || !umem_ptr || !fill || !comp)
> > > + return -EFAULT;
> > > + if (!size && !xsk_page_aligned(umem_area))
> > > + return -EINVAL;
> > > +
> > > + umem = calloc(1, sizeof(*umem));
> > > + if (!umem)
> > > + return -ENOMEM;
> > > +
> > > + umem->fd = socket(AF_XDP, SOCK_RAW, 0);
> > > + if (umem->fd < 0) {
> > > + err = -errno;
> > > + goto out_umem_alloc;
> > > + }
> > > +
> > > + umem->umem_area = umem_area;
> > > + xsk_set_umem_config(&umem->config, usr_config);
> > > +
> > > + mr.addr = (uintptr_t)umem_area;
> > > + mr.len = size;
> > > + mr.chunk_size = umem->config.frame_size;
> > > + mr.headroom = umem->config.frame_headroom;
> > > +
> > > + err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_REG, &mr, sizeof(mr));
> > > + if (err) {
> > > + err = -errno;
> > > + goto out_socket;
> > > + }
> > > + err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_FILL_RING,
> > > + &umem->config.fill_size,
> > > + sizeof(umem->config.fill_size));
> > > + if (err) {
> > > + err = -errno;
> > > + goto out_socket;
> > > + }
> > > + err = setsockopt(umem->fd, SOL_XDP, XDP_UMEM_COMPLETION_RING,
> > > + &umem->config.comp_size,
> > > + sizeof(umem->config.comp_size));
> > > + if (err) {
> > > + err = -errno;
> > > + goto out_socket;
> > > + }
> > > +
> > > + optlen = sizeof(off);
> > > + err = getsockopt(umem->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen);
> > > + if (err) {
> > > + err = -errno;
> > > + goto out_socket;
> > > + }
> > > +
> > > + map = xsk_mmap(NULL, off.fr.desc +
> > > + umem->config.fill_size * sizeof(__u64),
> > > + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE,
> > > + umem->fd, XDP_UMEM_PGOFF_FILL_RING);
> > > + if (map == MAP_FAILED) {
> > > + err = -errno;
> > > + goto out_socket;
> > > + }
> > > +
> > > + umem->fill = fill;
> > > + fill->mask = umem->config.fill_size - 1;
> > > + fill->size = umem->config.fill_size;
> > > + fill->producer = map + off.fr.producer;
> > > + fill->consumer = map + off.fr.consumer;
> > > + fill->ring = map + off.fr.desc;
> > > + fill->cached_cons = umem->config.fill_size;
> > > +
> > > + map = xsk_mmap(NULL,
> > > + off.cr.desc + umem->config.comp_size * sizeof(__u64),
> > > + PROT_READ | PROT_WRITE, MAP_SHARED | MAP_POPULATE,
> > > + umem->fd, XDP_UMEM_PGOFF_COMPLETION_RING);
> > > + if (map == MAP_FAILED) {
> > > + err = -errno;
> > > + goto out_mmap;
> > > + }
> > > +
> > > + umem->comp = comp;
> > > + comp->mask = umem->config.comp_size - 1;
> > > + comp->size = umem->config.comp_size;
> > > + comp->producer = map + off.cr.producer;
> > > + comp->consumer = map + off.cr.consumer;
> > > + comp->ring = map + off.cr.desc;
> > > +
> > > + *umem_ptr = umem;
> > > + return 0;
> > > +
> > > +out_mmap:
> > > + munmap(umem->fill,
> > > + off.fr.desc + umem->config.fill_size * sizeof(__u64));
> > > +out_socket:
> > > + close(umem->fd);
> > > +out_umem_alloc:
> > > + free(umem);
> > > + return err;
> > > +}
> > > +
> > > +static int xsk_parse_nl(void *cookie, void *msg, struct nlattr **tb)
> > > +{
> > > + struct nlattr *tb_parsed[IFLA_XDP_MAX + 1];
> > > + struct xsk_nl_info *nl_info = cookie;
> > > + struct ifinfomsg *ifinfo = msg;
> > > + unsigned char mode;
> > > + int err;
> > > +
> > > + if (nl_info->ifindex && nl_info->ifindex != ifinfo->ifi_index)
> > > + return 0;
> > > +
> > > + if (!tb[IFLA_XDP])
> > > + return 0;
> > > +
> > > + err = libbpf_nla_parse_nested(tb_parsed, IFLA_XDP_MAX, tb[IFLA_XDP],
> > > + NULL);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (!tb_parsed[IFLA_XDP_ATTACHED] || !tb_parsed[IFLA_XDP_FD])
> > > + return 0;
> > > +
> > > + mode = libbpf_nla_getattr_u8(tb_parsed[IFLA_XDP_ATTACHED]);
> > > + if (mode == XDP_ATTACHED_NONE)
> > > + return 0;
> > > +
> > > + nl_info->xdp_prog_attached = true;
> > > + nl_info->fd = libbpf_nla_getattr_u32(tb_parsed[IFLA_XDP_FD]);
> >
> > Hm, I don't think this works if I read the intention of this helper correctly.
> >
> > IFLA_XDP_FD is never set for retrieving the prog from the kernel. So the
> > above is a bug.
> >
> > We also have bpf_get_link_xdp_id(). This should probably just be reused in
> > this context here.
>
> If bpf_get_link_xdp_id() will fit my bill, I will happily use it. I
> will check it out and hopefully I can drop all this code. Thanks.
>
I see that all you need to know is whether there's already attached XDP program
to xsk socket's related interface, no?
If so, then within the xsk_setup_xdp_prog, you could do something like:
u32 prog_id = 0;
bpf_get_link_xdp_id(xsk->ifindex, &prog_id, xsk->config.xdp_flags);
if (!prog_id) {
// create maps
// load xdp prog
} else {
xsk->fd = prog_id;
}
xsk_update_bpf_maps(xsk, true, xsk->fd);
If that's ok then xsk_xdp_prog_attached and xsk_parse_nl could be dropped.
> > > + return 0;
> > > +}
> > > +
> > > +static bool xsk_xdp_prog_attached(struct xsk_socket *xsk)
> > > +{
> > > + struct xsk_nl_info nl_info;
> > > + unsigned int nl_pid;
> > > + char err_buf[256];
> > > + int sock, err;
> > > +
> > > + sock = libbpf_netlink_open(&nl_pid);
> > > + if (sock < 0)
> > > + return false;
> > > +
> > > + nl_info.xdp_prog_attached = false;
> > > + nl_info.ifindex = xsk->ifindex;
> > > + nl_info.fd = -1;
> > > +
> > > + err = libbpf_nl_get_link(sock, nl_pid, xsk_parse_nl, &nl_info);
> > > + if (err) {
> > > + libbpf_strerror(err, err_buf, sizeof(err_buf));
> > > + pr_warning("Error:\n%s\n", err_buf);
> > > + close(sock);
> > > + return false;
> > > + }
> > > +
> > > + close(sock);
> > > + xsk->prog_fd = nl_info.fd;
> > > + return nl_info.xdp_prog_attached;
> > > +}
> >
> > (See bpf_get_link_xdp_id().)
> >
> > > +
> > > +static int xsk_load_xdp_prog(struct xsk_socket *xsk)
> > > +{
> > > + char bpf_log_buf[BPF_LOG_BUF_SIZE];
> > > + int err, prog_fd;
> > > +
> > > + /* This is the C-program:
> > > + * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx)
> > > + * {
> > > + * int *qidconf, index = ctx->rx_queue_index;
> > [...]
> > > +
> > > +int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> > > + __u32 queue_id, struct xsk_umem *umem,
> > > + struct xsk_ring_cons *rx, struct xsk_ring_prod *tx,
> > > + const struct xsk_socket_config *usr_config)
> > > +{
> > > + struct sockaddr_xdp sxdp = {};
> > > + struct xdp_mmap_offsets off;
> > > + struct xsk_socket *xsk;
> > > + socklen_t optlen;
> > > + void *map;
> > > + int err;
> > > +
> > > + if (!umem || !xsk_ptr || !rx || !tx)
> > > + return -EFAULT;
> > > +
> > > + if (umem->refcount) {
> > > + pr_warning("Error: shared umems not supported by libbpf.\n");
> > > + return -EBUSY;
> > > + }
> > > +
> > > + xsk = calloc(1, sizeof(*xsk));
> > > + if (!xsk)
> > > + return -ENOMEM;
> > > +
> > > + if (umem->refcount++ > 0) {
> >
> > Should this refcount rather be atomic actually?
>
> Neither our config nor data plane interfaces are reentrant for
> performance reasons. Any concurrency has to be handled explicitly on
> the application level. This so that it only penalizes apps that really
> need this.
>
> Thanks for all your reviews: Magnus
>
> > > + xsk->fd = socket(AF_XDP, SOCK_RAW, 0);
> > > + if (xsk->fd < 0) {
> > > + err = -errno;
> > > + goto out_xsk_alloc;
> > > + }
> > > + } else {
> > > + xsk->fd = umem->fd;
> > > + }
> > > +
> > > + xsk->outstanding_tx = 0;
> > > + xsk->queue_id = queue_id;
> > > + xsk->umem = umem;
> > > + xsk->ifindex = if_nametoindex(ifname);
> > > + if (!xsk->ifindex) {
> > > + err = -errno;
> > > + goto out_socket;
> > > + }
> > > +
> > > + xsk_set_xdp_socket_config(&xsk->config, usr_config);
> > [...]
^ 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