* Re: [PATCH] can: mcp251x: add error check when wq alloc failed
From: Sean Nyekjaer @ 2019-06-26 6:26 UTC (permalink / raw)
To: Willem de Bruijn, Weitao Hou
Cc: Wolfgang Grandegger, Marc Kleine-Budde, David Miller,
Greg Kroah-Hartman, allison, tglx, linux-can, Network Development,
linux-kernel
In-Reply-To: <CA+FuTSegsUvPSWX+CZuafSD32Sx+xJmYPiQ92geDNqAe8_JGrQ@mail.gmail.com>
On 25/06/2019 16.03, Willem de Bruijn wrote:
> On Tue, Jun 25, 2019 at 8:51 AM Weitao Hou <houweitaoo@gmail.com> wrote:
>>
>> add error check when workqueue alloc failed, and remove
>> redundant code to make it clear
>>
>> Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
>
> Acked-by: Willem de Bruijn <willemb@google.com>
>
Tested-by: Sean Nyekjaer <sean@geanix.com>
^ permalink raw reply
* Re: [for-next V2 05/10] linux/dim: Rename externally used net_dim members
From: Tal Gilboa @ 2019-06-26 6:38 UTC (permalink / raw)
To: Sagi Grimberg, Saeed Mahameed, David S. Miller, Doug Ledford,
Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org
In-Reply-To: <c97bbab4-13a9-b9e1-69f2-d4aba43e1c06@grimberg.me>
On 6/26/2019 12:57 AM, Sagi Grimberg wrote:
> Question, do any other nics use or plan to use this?
Yes, see the changed files list under drivers/net for existing usage.
>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply
* RE: [EXT] Re: [PATCH net-next 4/4] qed: Add devlink support for configuration attributes.
From: Sudarsana Reddy Kalluru @ 2019-06-26 6:46 UTC (permalink / raw)
To: Jiri Pirko
Cc: Jakub Kicinski, davem@davemloft.net, netdev@vger.kernel.org,
Michal Kalderon, Ariel Elior
In-Reply-To: <20190620133748.GD2504@nanopsycho>
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, June 20, 2019 7:08 PM
> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>; davem@davemloft.net;
> netdev@vger.kernel.org; Michal Kalderon <mkalderon@marvell.com>; Ariel
> Elior <aelior@marvell.com>
> Subject: Re: [EXT] Re: [PATCH net-next 4/4] qed: Add devlink support for
> configuration attributes.
>
> Thu, Jun 20, 2019 at 02:09:29PM CEST, skalluru@marvell.com wrote:
> >> -----Original Message-----
> >> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> >> Sent: Tuesday, June 18, 2019 4:24 AM
> >> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> >> Cc: davem@davemloft.net; netdev@vger.kernel.org; Michal Kalderon
> >> <mkalderon@marvell.com>; Ariel Elior <aelior@marvell.com>; Jiri Pirko
> >> <jiri@resnulli.us>
> >> Subject: [EXT] Re: [PATCH net-next 4/4] qed: Add devlink support for
> >> configuration attributes.
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - On Mon, 17 Jun 2019 04:45:28 -0700, Sudarsana Reddy Kalluru wrote:
> >> > This patch adds implementation for devlink callbacks for reading/
> >> > configuring the device attributes.
> >> >
> >> > Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> >> > Signed-off-by: Ariel Elior <aelior@marvell.com>
> >>
> >> You need to provide documentation for your parameters, plus some of
> >> them look like they should potentially be port params, not device params.
> >
> >Thanks a lot for your review. Will add the required documentation. In case
> of Marvell adapter, any of the device/adapter/port parameters can be
> read/configurable via any PF (ethdev) on the port. Hence adding the
> commands at device level. Hope this is fine.
>
> No it is not. Port param should be port param.
>
> Also please be careful not to add any generic param as driver specific.
>
> Thanks!
Hi,
Could you please with my query on the devlink-port-params implementation. [had sent the same query earlier to jiri@mellanox.com (based on the copyright info)].
Kernel seem to be invoking the driver devlink callbacks (registered via DEVLINK_PARAM_DRIVER) only when the associated parameter is published via devlink_params_publish(). callnback invocation path,
devlink_nl_param_fill()
{
if (!param_item->published)
continue;
ctx.cmode = i;
err = devlink_param_get(devlink, param, &ctx);
}
The API devlink_params_publish() publishes only the devlink-dev parameters (i.e., registered via devlink_params_register()), not the devlink-port params which are registered via devlink_port_params_register(). I couldn't find any other interface for publishing the devlink-port-params.
I have manually verified setting the published flag for port-params (as in below) and, observed that kernel correctly invokes the callbacks of devlink-port-params.
list_for_each_entry(param_item, &dl_port.param_list, list) {
param_item->published = true;
}
Please let me know if I'm missing something here or, it's a missing functionality in the kernel.
Thanks,
Sudarsana
^ permalink raw reply
* Re: [PATCH bpf-next v9 02/10] bpf: Add eBPF program subtype and is_valid_subtype() verifier
From: Mickaël Salaün @ 2019-06-26 7:33 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: LKML, Aleksa Sarai, Alexander Viro, Alexei Starovoitov,
Andrew Morton, Andy Lutomirski, Arnaldo Carvalho de Melo,
Casey Schaufler, Daniel Borkmann, David Drysdale,
David S . Miller, Eric W . Biederman, James Morris, Jann Horn,
John Johansen, Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Paul Moore, Sargun Dhillon,
Serge E . Hallyn, Shuah Khan, Stephen Smalley, Tejun Heo,
Tetsuo Handa, Thomas Graf, Tycho Andersen, Will Drewry,
Kernel Hardening, Linux API, Linux-Fsdevel, LSM List,
Network Development
In-Reply-To: <CAADnVQ+Twio22VSi21RR5TY1Zm-1xRTGmREcXLSs5Jv-KWGTiw@mail.gmail.com>
On 26/06/2019 01:02, Alexei Starovoitov wrote:
> On Tue, Jun 25, 2019 at 3:04 PM Mickaël Salaün <mic@digikod.net> wrote:
>>
>> The goal of the program subtype is to be able to have different static
>> fine-grained verifications for a unique program type.
>>
>> The struct bpf_verifier_ops gets a new optional function:
>> is_valid_subtype(). This new verifier is called at the beginning of the
>> eBPF program verification to check if the (optional) program subtype is
>> valid.
>>
>> The new helper bpf_load_program_xattr() enables to verify a program with
>> subtypes.
>>
>> For now, only Landlock eBPF programs are using a program subtype (see
>> next commits) but this could be used by other program types in the
>> future.
>>
>> Signed-off-by: Mickaël Salaün <mic@digikod.net>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>> Cc: David S. Miller <davem@davemloft.net>
>> Link: https://lkml.kernel.org/r/20160827205559.GA43880@ast-mbp.thefacebook.com
>> ---
>>
>> Changes since v8:
>> * use bpf_load_program_xattr() instead of bpf_load_program() and add
>> bpf_verify_program_xattr() to deal with subtypes
>> * remove put_extra() since there is no more "previous" field (for now)
>>
>> Changes since v7:
>> * rename LANDLOCK_SUBTYPE_* to LANDLOCK_*
>> * move subtype in bpf_prog_aux and use only one bit for has_subtype
>> (suggested by Alexei Starovoitov)
>
> sorry to say, but I don't think the landlock will ever land,
> since posting huge patches once a year is missing a lot of development
> that is happening during that time.
You're right that it's been a while since the last patch set, but the
main reasons behind this was a lack of feedback (probably because of the
size of the patch set, which is now reduce to a consistent minimum), the
rework needed to address everyone's concern (Landlock modify kernel
components from different maintainers), and above all, the LSM stacking
infrastructure which was quite beefy and then took some time to land:
https://lore.kernel.org/lkml/50db058a-7dde-441b-a7f9-f6837fe8b69f@schaufler-ca.com/
This stacking infrastructure was required to have a useful version of
Landlock (which is used as a use case example), and it was released with
Linux v5.1 (last month). Now, I think everything is finally ready to
move forward.
> This 2/10 patch is an example.
> subtype concept was useful 2 years ago when v6 was posted.
> Since then bpf developers faced very similar problem in other parts
> and it was solved with 'expected_attach_type' field.
> See commit 5e43f899b03a ("bpf: Check attach type at prog load time")
> dated March 2018.
I saw this nice feature but I wasn't sure if it was the right field to
use. Indeed, I need more than a "type", but also some values (triggers)
as shown by this patch. What do you suggest?
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-26 7:41 UTC (permalink / raw)
To: Vladimir Oltean
Cc: René van Dorst, sean.wang, Florian Fainelli, David S. Miller,
matthias.bgg, Andrew Lunn, Vivien Didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <CA+h21hq_w8-96ehKYxcziSq1TjOjoKduZ+pB3umBfjODaKWd+A@mail.gmail.com>
On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote:
> On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > Hi Russell,
> > > > >
> > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > 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?
> > > >
> > > > Yes. mac_config() should not touch it; it is not always in a defined
> > > > state. For example, if you set modes via ethtool (the
> > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > zero irrespective of the true link state.
> > > >
> > >
> > > Experimentally, state->link is zero at the same time as state->speed
> > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > AN. What is your suggestion? Should I proceed to try and configure the
> > > MAC for SPEED_UNKNOWN?
> >
> > What would you have done with a PHY when the link is down, what speed
> > would you have configured in the phylib adjust_link callback? phylib
> > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> >
>
> With phylib, I'd make the driver ignore the speed and do nothing.
> With phylink, I'd make the core not call mac_config.
> But what happened is I saw phylink call mac_config anyway, said
> 'weird' and proceeded to ignore it as I would have for phylib.
> I'm just not understanding your position - it seems like you're
> implying there's a bug in phylink and the function call with
> MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> place, which is what I wanted to confirm.
It is not a bug. It is a request to configure the MAC, and what it's
saying is "we don't know what speed and/or duplex".
Take for instance when the network adapter is brought up initially.
The link is most likely down, but we should configure the initial MAC
operating parameters (such as the PHY interface). Phylink makes a
mac_config() call with the speed and duplex set to UNKNOWN.
Using your theory, we shouldn't be making that call. In which case,
MAC drivers aren't going to initially configure their interface
settings.
_That_ would be a bug.
> > It's unlikely that it would switch between SGMII and USXGMII
> > dynamically, as USXGMII supports speeds from 10G down to 10M.
> >
> > Where interface mode switching tends to be used is with modes such
> > as 10GBASE-R, which doesn't support anything except 10G. In order
> > for the PHY to operate at slower speeds, it has a few options:
> >
> > 1) perform rate adaption.
> > 2) dynamically switch interface type to an interface type that
> > supports the desired speed.
> > 3) just not support slower speeds.
> >
>
> So am I reading this correctly - it kind of makes sense for gigabit
> MAC drivers to not check for the MII interface changing protocol?
Again, that's incorrect in the general case. Gigabit includes SGMII
and 802.3z PHY protocols which need to be switched between for SFPs.
--
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 net-next 00/11] net: hns3: some code optimizations & bugfixes
From: tanhuazhong @ 2019-06-26 7:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, salil.mehta, yisen.zhuang, linuxarm
In-Reply-To: <20190622.095323.1495992426494142587.davem@davemloft.net>
On 2019/6/22 21:53, David Miller wrote:
> From: Huazhong Tan <tanhuazhong@huawei.com>
> Date: Thu, 20 Jun 2019 16:52:34 +0800
>
>> This patch-set includes code optimizations and bugfixes for
>> the HNS3 ethernet controller driver.
>>
>> [patch 1/11] fixes a selftest issue when doing autoneg.
>>
>> [patch 2/11 - 3-11] adds two code optimizations about VLAN issue.
>>
>> [patch 4/11] restores the MAC autoneg state after reset.
>>
>> [patch 5/11 - 8/11] adds some code optimizations and bugfixes about
>> HW errors handling.
>>
>> [patch 9/11 - 11/11] fixes some issues related to driver loading and
>> unloading.
>
> Series applied, thanks.
>
Hi, david, has this patchset merged into net-next, why I cannot see it
after pulling net-next? Or is there some problem about this patchset I
have missed?
> .
>
^ permalink raw reply
* RE: [for-next V2 10/10] RDMA/core: Provide RDMA DIM support for ULPs
From: Idan Burstein @ 2019-06-26 7:56 UTC (permalink / raw)
To: Sagi Grimberg, Saeed Mahameed, David S. Miller, Doug Ledford,
Jason Gunthorpe
Cc: Leon Romanovsky, Or Gerlitz, Tal Gilboa, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, Yamin Friedman, Max Gurtovoy
In-Reply-To: <adb3687a-6db3-b1a4-cd32-8b4889550c81@grimberg.me>
" Please don't. This is a bad choice to opt it in by default."
I disagree here. I'd prefer Linux to have good out of the box experience (e.g. reach 100G in 4K NVMeOF on Intel servers) with the default parameters. Especially since Yamin have shown it is beneficial / not hurting in terms of performance for variety of use cases. The whole concept of DIM is that it adapts to the workload requirements in terms of bandwidth and latency.
Moreover, net-dim is enabled by default, I don't see why RDMA is different.
-----Original Message-----
From: linux-rdma-owner@vger.kernel.org <linux-rdma-owner@vger.kernel.org> On Behalf Of Sagi Grimberg
Sent: Wednesday, June 26, 2019 12:14 AM
To: Saeed Mahameed <saeedm@mellanox.com>; David S. Miller <davem@davemloft.net>; Doug Ledford <dledford@redhat.com>; Jason Gunthorpe <jgg@mellanox.com>
Cc: Leon Romanovsky <leonro@mellanox.com>; Or Gerlitz <ogerlitz@mellanox.com>; Tal Gilboa <talgi@mellanox.com>; netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Yamin Friedman <yaminf@mellanox.com>; Max Gurtovoy <maxg@mellanox.com>
Subject: Re: [for-next V2 10/10] RDMA/core: Provide RDMA DIM support for ULPs
> +static int ib_poll_dim_handler(struct irq_poll *iop, int budget) {
> + struct ib_cq *cq = container_of(iop, struct ib_cq, iop);
> + struct dim *dim = cq->dim;
> + int completed;
> +
> + completed = __ib_process_cq(cq, budget, cq->wc, IB_POLL_BATCH);
> + if (completed < budget) {
> + irq_poll_complete(&cq->iop);
> + if (ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
> + irq_poll_sched(&cq->iop);
> + }
> +
> + rdma_dim(dim, completed);
Why duplicate the entire thing for a one-liner?
> +
> + return completed;
> +}
> +
> static void ib_cq_completion_softirq(struct ib_cq *cq, void *private)
> {
> irq_poll_sched(&cq->iop);
> @@ -105,14 +157,18 @@ static void ib_cq_completion_softirq(struct
> ib_cq *cq, void *private)
>
> static void ib_cq_poll_work(struct work_struct *work)
> {
> - struct ib_cq *cq = container_of(work, struct ib_cq, work);
> + struct ib_cq *cq = container_of(work, struct ib_cq,
> + work);
Why was that changed?
> int completed;
>
> completed = __ib_process_cq(cq, IB_POLL_BUDGET_WORKQUEUE, cq->wc,
> IB_POLL_BATCH);
> +
newline?
> if (completed >= IB_POLL_BUDGET_WORKQUEUE ||
> ib_req_notify_cq(cq, IB_POLL_FLAGS) > 0)
> queue_work(cq->comp_wq, &cq->work);
> + else if (cq->dim)
> + rdma_dim(cq->dim, completed);
> }
>
> static void ib_cq_completion_workqueue(struct ib_cq *cq, void
> *private) @@ -166,6 +222,8 @@ struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
> rdma_restrack_set_task(&cq->res, caller);
> rdma_restrack_kadd(&cq->res);
>
> + rdma_dim_init(cq);
> +
> switch (cq->poll_ctx) {
> case IB_POLL_DIRECT:
> cq->comp_handler = ib_cq_completion_direct; @@ -173,7 +231,13 @@
> struct ib_cq *__ib_alloc_cq_user(struct ib_device *dev, void *private,
> case IB_POLL_SOFTIRQ:
> cq->comp_handler = ib_cq_completion_softirq;
>
> - irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ, ib_poll_handler);
> + if (cq->dim) {
> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
> + ib_poll_dim_handler);
> + } else
> + irq_poll_init(&cq->iop, IB_POLL_BUDGET_IRQ,
> + ib_poll_handler);
> +
> ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
> break;
> case IB_POLL_WORKQUEUE:
> @@ -226,6 +290,9 @@ void ib_free_cq_user(struct ib_cq *cq, struct ib_udata *udata)
> WARN_ON_ONCE(1);
> }
>
> + if (cq->dim)
> + cancel_work_sync(&cq->dim->work);
> + kfree(cq->dim);
> kfree(cq->wc);
> rdma_restrack_del(&cq->res);
> ret = cq->device->ops.destroy_cq(cq, udata); diff --git
> a/drivers/infiniband/hw/mlx5/main.c
> b/drivers/infiniband/hw/mlx5/main.c
> index abac70ad5c7c..b1b45dbe24a5 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -6305,6 +6305,8 @@ static int mlx5_ib_stage_caps_init(struct mlx5_ib_dev *dev)
> MLX5_CAP_GEN(dev->mdev, disable_local_lb_mc)))
> mutex_init(&dev->lb.mutex);
>
> + dev->ib_dev.use_cq_dim = true;
> +
Please don't. This is a bad choice to opt it in by default.
^ permalink raw reply
* Re: [PATCH net-next] net: ipvlan: forward ingress packet to slave's l2 in l3s mode
From: Paolo Abeni @ 2019-06-26 8:16 UTC (permalink / raw)
To: Zhiyuan Hou, davem, idosch, daniel, petrm, jiri, tglx, linmiaohe
Cc: zhabin, caspar, netdev, linux-kernel
In-Reply-To: <20190625064208.2256-1-zhiyuan2048@linux.alibaba.com>
Hi,
On Tue, 2019-06-25 at 14:42 +0800, Zhiyuan Hou wrote:
> In ipvlan l3s mode, ingress packet is switched to slave interface and
> delivers to l4 stack. This may cause two problems:
>
> 1. When slave is in an ns different from master, the behavior of stack
> in slave ns may cause confusion for users. For example, iptables, tc,
> and other l2/l3 functions are not available for ingress packet.
>
> 2. l3s mode is not used for tap device, and cannot support ipvtap. But
> in VM or container based VM cases, tap device is a very common device.
>
> In l3s mode's input nf_hook, this patch calles the skb_forward_dev() to
> forward ingress packet to slave and uses nf_conntrack_confirm() to make
> conntrack work with new mode.
>
> Signed-off-by: Zha Bin <zhabin@linux.alibaba.com>
> Signed-off-by: Zhiyuan Hou <zhiyuan2048@linux.alibaba.com>
> ---
> drivers/net/ipvlan/ipvlan.h | 9 ++++++++-
> drivers/net/ipvlan/ipvlan_l3s.c | 16 ++++++++++++++--
> 2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ipvlan/ipvlan.h b/drivers/net/ipvlan/ipvlan.h
> index 3837c897832e..48c814e24c3f 100644
> --- a/drivers/net/ipvlan/ipvlan.h
> +++ b/drivers/net/ipvlan/ipvlan.h
> @@ -172,6 +172,14 @@ void ipvlan_link_delete(struct net_device *dev, struct list_head *head);
> void ipvlan_link_setup(struct net_device *dev);
> int ipvlan_link_register(struct rtnl_link_ops *ops);
> #ifdef CONFIG_IPVLAN_L3S
> +
> +#include <net/netfilter/nf_conntrack_core.h>
> +
> +static inline int ipvlan_confirm_conntrack(struct sk_buff *skb)
> +{
> + return nf_conntrack_confirm(skb);
> +}
> +
> int ipvlan_l3s_register(struct ipvl_port *port);
> void ipvlan_l3s_unregister(struct ipvl_port *port);
> void ipvlan_migrate_l3s_hook(struct net *oldnet, struct net *newnet);
> @@ -206,5 +214,4 @@ static inline bool netif_is_ipvlan_port(const struct net_device *dev)
> {
> return rcu_access_pointer(dev->rx_handler) == ipvlan_handle_frame;
> }
> -
> #endif /* __IPVLAN_H */
> diff --git a/drivers/net/ipvlan/ipvlan_l3s.c b/drivers/net/ipvlan/ipvlan_l3s.c
> index 943d26cbf39f..ed210002f593 100644
> --- a/drivers/net/ipvlan/ipvlan_l3s.c
> +++ b/drivers/net/ipvlan/ipvlan_l3s.c
> @@ -95,14 +95,26 @@ static unsigned int ipvlan_nf_input(void *priv, struct sk_buff *skb,
> {
> struct ipvl_addr *addr;
> unsigned int len;
> + int ret = NF_ACCEPT;
> + bool success;
>
> addr = ipvlan_skb_to_addr(skb, skb->dev);
> if (!addr)
> goto out;
>
> - skb->dev = addr->master->dev;
> len = skb->len + ETH_HLEN;
> - ipvlan_count_rx(addr->master, len, true, false);
> +
> + ret = ipvlan_confirm_conntrack(skb);
> + if (ret != NF_ACCEPT) {
> + ipvlan_count_rx(addr->master, len, false, false);
> + goto out;
> + }
> +
> + skb_push_rcsum(skb, ETH_HLEN);
> + success = dev_forward_skb(addr->master->dev, skb) == NET_RX_SUCCESS;
This looks weird to me: if I read the code correctly, the skb will
traverse twice NF_INET_LOCAL_IN, once due to the l3s hooking and
another one due to dev_forward_skb().
Also, tc ingreess, etc will run after the first traversing of
NF_INET_LOCAL_IN.
All in all I think that if full l2 processing is required, a different
mode or a different virtual device should be used.
Cheers,
Paolo
^ permalink raw reply
* RE: [EXT] [PATCH V2] bnx2x: Prevent ptp_task to be rescheduled indefinitely
From: Sudarsana Reddy Kalluru @ 2019-06-26 8:25 UTC (permalink / raw)
To: Guilherme Piccoli, jay.vosburgh@canonical.com
Cc: GR-everest-linux-l2, netdev@vger.kernel.org, Ariel Elior
In-Reply-To: <CAHD1Q_y5wWqOkPaC+JsuGMfBHbwPHbQF93Y-+06Nck=HKrif2g@mail.gmail.com>
> -----Original Message-----
> From: Guilherme Piccoli <gpiccoli@canonical.com>
> Sent: Wednesday, June 26, 2019 1:56 AM
> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>;
> jay.vosburgh@canonical.com
> Cc: GR-everest-linux-l2 <GR-everest-linux-l2@marvell.com>;
> netdev@vger.kernel.org; Ariel Elior <aelior@marvell.com>
> Subject: Re: [EXT] [PATCH V2] bnx2x: Prevent ptp_task to be rescheduled
> indefinitely
>
> 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.
The register value of zero indicates there is no pending Tx timestamp to be read by the driver.
FW writes/latches the Tx timestamp for PTP event packet in this register. And it does the latching only if the register is free.
In this case user/app look to be requesting the Timestamp (via skb->tx_flags) for non-ptp Tx packet. In the Tx path, driver schedules a thread for reading the Tx timestamp,
bnx2x_start_xmit()
{
if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP))
schedule_work(&bp->ptp_task);
}
FW seem to be not timestamping the packet at all and driver is indefinitely waiting for it.
>
> 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
* [PATCH net-next v2] libbpf: add xsk_ring_prod__nb_free() function
From: Eelco Chaudron @ 2019-06-26 8:27 UTC (permalink / raw)
To: netdev
Cc: ast, daniel, kafai, songliubraving, yhs, andrii.nakryiko,
magnus.karlsson
When an AF_XDP application received X packets, it does not mean X
frames can be stuffed into the producer ring. To make it easier for
AF_XDP applications this API allows them to check how many frames can
be added into the ring.
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v1 -> v2
- Renamed xsk_ring_prod__free() to xsk_ring_prod__nb_free()
- Add caching so it will only touch global state when needed
tools/lib/bpf/xsk.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
index 82ea71a0f3ec..6acb81102346 100644
--- a/tools/lib/bpf/xsk.h
+++ b/tools/lib/bpf/xsk.h
@@ -76,11 +76,11 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
return &descs[idx & rx->mask];
}
-static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
+static inline __u32 xsk_prod__nb_free(struct xsk_ring_prod *r, __u32 nb)
{
__u32 free_entries = r->cached_cons - r->cached_prod;
- if (free_entries >= nb)
+ if (free_entries >= nb && nb != 0)
return free_entries;
/* Refresh the local tail pointer.
@@ -110,7 +110,7 @@ static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
size_t nb, __u32 *idx)
{
- if (xsk_prod_nb_free(prod, nb) < nb)
+ if (xsk_prod__nb_free(prod, nb) < nb)
return 0;
*idx = prod->cached_prod;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next v2] libbpf: add xsk_ring_prod__nb_free() function
From: Eelco Chaudron @ 2019-06-26 8:30 UTC (permalink / raw)
To: netdev
Cc: ast, daniel, kafai, songliubraving, yhs, andrii.nakryiko,
magnus.karlsson
In-Reply-To: <d4692ea57ba7a3fe33549fc6222fb8aea5a4225e.1561537432.git.echaudro@redhat.com>
Please ignore, this was supposed to be bpf-next not net-next :(
On 26 Jun 2019, at 10:27, Eelco Chaudron wrote:
> When an AF_XDP application received X packets, it does not mean X
> frames can be stuffed into the producer ring. To make it easier for
> AF_XDP applications this API allows them to check how many frames can
> be added into the ring.
>
> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>
> v1 -> v2
> - Renamed xsk_ring_prod__free() to xsk_ring_prod__nb_free()
> - Add caching so it will only touch global state when needed
>
> tools/lib/bpf/xsk.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
> index 82ea71a0f3ec..6acb81102346 100644
> --- a/tools/lib/bpf/xsk.h
> +++ b/tools/lib/bpf/xsk.h
> @@ -76,11 +76,11 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons
> *rx, __u32 idx)
> return &descs[idx & rx->mask];
> }
>
> -static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32
> nb)
> +static inline __u32 xsk_prod__nb_free(struct xsk_ring_prod *r, __u32
> nb)
> {
> __u32 free_entries = r->cached_cons - r->cached_prod;
>
> - if (free_entries >= nb)
> + if (free_entries >= nb && nb != 0)
> return free_entries;
>
> /* Refresh the local tail pointer.
> @@ -110,7 +110,7 @@ static inline __u32 xsk_cons_nb_avail(struct
> xsk_ring_cons *r, __u32 nb)
> static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod
> *prod,
> size_t nb, __u32 *idx)
> {
> - if (xsk_prod_nb_free(prod, nb) < nb)
> + if (xsk_prod__nb_free(prod, nb) < nb)
> return 0;
>
> *idx = prod->cached_prod;
> --
> 2.20.1
^ permalink raw reply
* [PATCH net] sctp: not bind the socket in sctp_connect
From: Xin Long @ 2019-06-26 8:31 UTC (permalink / raw)
To: network dev, linux-sctp
Cc: davem, Marcelo Ricardo Leitner, Neil Horman, syzkaller-bugs
Now when sctp_connect() is called with a wrong sa_family, it binds
to a port but doesn't set bp->port, then sctp_get_af_specific will
return NULL and sctp_connect() returns -EINVAL.
Then if sctp_bind() is called to bind to another port, the last
port it has bound will leak due to bp->port is NULL by then.
sctp_connect() doesn't need to bind ports, as later __sctp_connect
will do it if bp->port is NULL. So remove it from sctp_connect().
While at it, remove the unnecessary sockaddr.sa_family len check
as it's already done in sctp_inet_connect.
Fixes: 644fbdeacf1d ("sctp: fix the issue that flags are ignored when using kernel_connect")
Reported-by: syzbot+079bf326b38072f849d9@syzkaller.appspotmail.com
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/sctp/socket.c | 24 +++---------------------
1 file changed, 3 insertions(+), 21 deletions(-)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 39ea0a3..f33aa9e 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4816,35 +4816,17 @@ static int sctp_setsockopt(struct sock *sk, int level, int optname,
static int sctp_connect(struct sock *sk, struct sockaddr *addr,
int addr_len, int flags)
{
- struct inet_sock *inet = inet_sk(sk);
struct sctp_af *af;
- int err = 0;
+ int err = -EINVAL;
lock_sock(sk);
-
pr_debug("%s: sk:%p, sockaddr:%p, addr_len:%d\n", __func__, sk,
addr, addr_len);
- /* We may need to bind the socket. */
- if (!inet->inet_num) {
- if (sk->sk_prot->get_port(sk, 0)) {
- release_sock(sk);
- return -EAGAIN;
- }
- inet->inet_sport = htons(inet->inet_num);
- }
-
/* Validate addr_len before calling common connect/connectx routine. */
- af = addr_len < offsetofend(struct sockaddr, sa_family) ? NULL :
- sctp_get_af_specific(addr->sa_family);
- if (!af || addr_len < af->sockaddr_len) {
- err = -EINVAL;
- } else {
- /* Pass correct addr len to common routine (so it knows there
- * is only one address being passed.
- */
+ af = sctp_get_af_specific(addr->sa_family);
+ if (af && addr_len >= af->sockaddr_len)
err = __sctp_connect(sk, addr, af->sockaddr_len, flags, NULL);
- }
release_sock(sk);
return err;
--
2.1.0
^ permalink raw reply related
* [PATCH bpf-next v2] libbpf: add xsk_ring_prod__nb_free() function
From: Eelco Chaudron @ 2019-06-26 8:33 UTC (permalink / raw)
To: netdev
Cc: ast, daniel, kafai, songliubraving, yhs, andrii.nakryiko,
magnus.karlsson
When an AF_XDP application received X packets, it does not mean X
frames can be stuffed into the producer ring. To make it easier for
AF_XDP applications this API allows them to check how many frames can
be added into the ring.
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v1 -> v2
- Renamed xsk_ring_prod__free() to xsk_ring_prod__nb_free()
- Add caching so it will only touch global state when needed
tools/lib/bpf/xsk.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
index 82ea71a0f3ec..6acb81102346 100644
--- a/tools/lib/bpf/xsk.h
+++ b/tools/lib/bpf/xsk.h
@@ -76,11 +76,11 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
return &descs[idx & rx->mask];
}
-static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
+static inline __u32 xsk_prod__nb_free(struct xsk_ring_prod *r, __u32 nb)
{
__u32 free_entries = r->cached_cons - r->cached_prod;
- if (free_entries >= nb)
+ if (free_entries >= nb && nb != 0)
return free_entries;
/* Refresh the local tail pointer.
@@ -110,7 +110,7 @@ static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
size_t nb, __u32 *idx)
{
- if (xsk_prod_nb_free(prod, nb) < nb)
+ if (xsk_prod__nb_free(prod, nb) < nb)
return 0;
*idx = prod->cached_prod;
--
2.20.1
^ permalink raw reply related
* XDP multi-buffer incl. jumbo-frames (Was: [RFC V1 net-next 1/1] net: ena: implement XDP drop support)
From: Jesper Dangaard Brouer @ 2019-06-26 8:38 UTC (permalink / raw)
To: Machulsky, Zorik
Cc: Jubran, Samih, davem@davemloft.net, netdev@vger.kernel.org,
Woodhouse, David, Matushevsky, Alexander, Bshara, Saeed,
Wilson, Matt, Liguori, Anthony, Bshara, Nafea, Tzalik, Guy,
Belgazal, Netanel, Saidi, Ali, Herrenschmidt, Benjamin,
Kiyanovski, Arthur, Daniel Borkmann, brouer,
Toke Høiland-Jørgensen, Ilias Apalodimas,
Alexei Starovoitov, Jakub Kicinski, xdp-newbies@vger.kernel.org
In-Reply-To: <A658E65E-93D2-4F10-823D-CC25B081C1B7@amazon.com>
On Tue, 25 Jun 2019 03:19:22 +0000
"Machulsky, Zorik" <zorik@amazon.com> wrote:
> On 6/23/19, 7:21 AM, "Jesper Dangaard Brouer" <brouer@redhat.com> wrote:
>
> On Sun, 23 Jun 2019 10:06:49 +0300 <sameehj@amazon.com> wrote:
>
> > This commit implements the basic functionality of drop/pass logic in the
> > ena driver.
>
> Usually we require a driver to implement all the XDP return codes,
> before we accept it. But as Daniel and I discussed with Zorik during
> NetConf[1], we are going to make an exception and accept the driver
> if you also implement XDP_TX.
>
> As we trust that Zorik/Amazon will follow and implement XDP_REDIRECT
> later, given he/you wants AF_XDP support which requires XDP_REDIRECT.
>
> Jesper, thanks for your comments and very helpful discussion during
> NetConf! That's the plan, as we agreed. From our side I would like to
> reiterate again the importance of multi-buffer support by xdp frame.
> We would really prefer not to see our MTU shrinking because of xdp
> support.
Okay we really need to make a serious attempt to find a way to support
multi-buffer packets with XDP. With the important criteria of not
hurting performance of the single-buffer per packet design.
I've created a design document[2], that I will update based on our
discussions: [2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp-multi-buffer01-design.org
The use-case that really convinced me was Eric's packet header-split.
Lets refresh: Why XDP don't have multi-buffer support:
XDP is designed for maximum performance, which is why certain driver-level
use-cases were not supported, like multi-buffer packets (like jumbo-frames).
As it e.g. complicated the driver RX-loop and memory model handling.
The single buffer per packet design, is also tied into eBPF Direct-Access
(DA) to packet data, which can only be allowed if the packet memory is in
contiguous memory. This DA feature is essential for XDP performance.
One way forward is to define that XDP only get access to the first
packet buffer, and it cannot see subsequent buffers. For XDP_TX and
XDP_REDIRECT to work then XDP still need to carry pointers (plus
len+offset) to the other buffers, which is 16 bytes per extra buffer.
> [1] http://vger.kernel.org/netconf2019.html
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH net-next] net: ethtool: Allow parsing ETHER_FLOW types when using flow_rule
From: Maxime Chevallier @ 2019-06-26 8:44 UTC (permalink / raw)
To: davem, Pablo Neira Ayuso, Florian Fainelli, Jiri Pirko,
Jakub Kicinski
Cc: Maxime Chevallier, netdev, linux-kernel, Antoine Tenart,
thomas.petazzoni
When parsing an ethtool_rx_flow_spec, users can specify an ethernet flow
which could contain matches based on the ethernet header, such as the
MAC address, the VLAN tag or the ethertype.
Only the ethtype field is specific to the ether flow, the MAC and vlan
fields are processed using the special FLOW_EXT and FLOW_MAC_EXT flags.
Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
net/core/ethtool.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 4d1011b2e24f..01ceba556341 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -2883,6 +2883,18 @@ ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input)
match->mask.basic.n_proto = htons(0xffff);
switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS)) {
+ case ETHER_FLOW: {
+ const struct ethhdr *ether_spec, *ether_m_spec;
+
+ ether_spec = &fs->h_u.ether_spec;
+ ether_m_spec = &fs->m_u.ether_spec;
+
+ if (ether_m_spec->h_proto) {
+ match->key.basic.n_proto = ether_spec->h_proto;
+ match->mask.basic.n_proto = ether_m_spec->h_proto;
+ }
+ }
+ break;
case TCP_V4_FLOW:
case UDP_V4_FLOW: {
const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Vladimir Oltean @ 2019-06-26 8:46 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: René van Dorst, sean.wang, Florian Fainelli, David S. Miller,
matthias.bgg, Andrew Lunn, Vivien Didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <20190626074158.odyrgzie7sv4ovtn@shell.armlinux.org.uk>
On Wed, 26 Jun 2019 at 10:42, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote:
> > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > > Hi Russell,
> > > > > >
> > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > > 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?
> > > > >
> > > > > Yes. mac_config() should not touch it; it is not always in a defined
> > > > > state. For example, if you set modes via ethtool (the
> > > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > > zero irrespective of the true link state.
> > > > >
> > > >
> > > > Experimentally, state->link is zero at the same time as state->speed
> > > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > > AN. What is your suggestion? Should I proceed to try and configure the
> > > > MAC for SPEED_UNKNOWN?
> > >
> > > What would you have done with a PHY when the link is down, what speed
> > > would you have configured in the phylib adjust_link callback? phylib
> > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> > >
> >
> > With phylib, I'd make the driver ignore the speed and do nothing.
> > With phylink, I'd make the core not call mac_config.
> > But what happened is I saw phylink call mac_config anyway, said
> > 'weird' and proceeded to ignore it as I would have for phylib.
> > I'm just not understanding your position - it seems like you're
> > implying there's a bug in phylink and the function call with
> > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> > place, which is what I wanted to confirm.
>
> It is not a bug. It is a request to configure the MAC, and what it's
> saying is "we don't know what speed and/or duplex".
>
> Take for instance when the network adapter is brought up initially.
> The link is most likely down, but we should configure the initial MAC
> operating parameters (such as the PHY interface). Phylink makes a
> mac_config() call with the speed and duplex set to UNKNOWN.
>
> Using your theory, we shouldn't be making that call. In which case,
> MAC drivers aren't going to initially configure their interface
> settings.
>
> _That_ would be a bug.
>
So you're saying that:
- state->link should not be checked, because it is not guaranteed to be valid
- state->speed, state->duplex, state->pause *should* be checked,
because it is not guaranteed to be valid
Is state->interface always valid?
I don't think I follow the pattern here. Or shouldn't I check speed,
duplex and pause either, and try to pass the MAC UNKNOWN values,
inevitably failing at some point? Do Marvell MACs have an UNKNOWN
setting?
> > It's unlikely that it would switch between SGMII and USXGMII
> > > dynamically, as USXGMII supports speeds from 10G down to 10M.
> > >
> > > Where interface mode switching tends to be used is with modes such
> > > as 10GBASE-R, which doesn't support anything except 10G. In order
> > > for the PHY to operate at slower speeds, it has a few options:
> > >
> > > 1) perform rate adaption.
> > > 2) dynamically switch interface type to an interface type that
> > > supports the desired speed.
> > > 3) just not support slower speeds.
> > >
> >
> > So am I reading this correctly - it kind of makes sense for gigabit
> > MAC drivers to not check for the MII interface changing protocol?
>
> Again, that's incorrect in the general case. Gigabit includes SGMII
> and 802.3z PHY protocols which need to be switched between for SFPs.
>
> --
> 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
Thanks,
-Vladimir
^ permalink raw reply
* Re: [PATCH net-next] net: ethtool: Allow parsing ETHER_FLOW types when using flow_rule
From: Pablo Neira Ayuso @ 2019-06-26 8:58 UTC (permalink / raw)
To: Maxime Chevallier
Cc: davem, Florian Fainelli, Jiri Pirko, Jakub Kicinski, netdev,
linux-kernel, Antoine Tenart, thomas.petazzoni
In-Reply-To: <20190626084403.17749-1-maxime.chevallier@bootlin.com>
On Wed, Jun 26, 2019 at 10:44:03AM +0200, Maxime Chevallier wrote:
> When parsing an ethtool_rx_flow_spec, users can specify an ethernet flow
> which could contain matches based on the ethernet header, such as the
> MAC address, the VLAN tag or the ethertype.
>
> Only the ethtype field is specific to the ether flow, the MAC and vlan
> fields are processed using the special FLOW_EXT and FLOW_MAC_EXT flags.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> net/core/ethtool.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 4d1011b2e24f..01ceba556341 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -2883,6 +2883,18 @@ ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input)
> match->mask.basic.n_proto = htons(0xffff);
>
> switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS)) {
> + case ETHER_FLOW: {
> + const struct ethhdr *ether_spec, *ether_m_spec;
> +
> + ether_spec = &fs->h_u.ether_spec;
> + ether_m_spec = &fs->m_u.ether_spec;
> +
> + if (ether_m_spec->h_proto) {
> + match->key.basic.n_proto = ether_spec->h_proto;
> + match->mask.basic.n_proto = ether_m_spec->h_proto;
> + }
I see some drivers in the tree also interpret the h_source and h_dest
fields?
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/sfc/falcon/ethtool.c#L1182
Probably good to address this in this patch too?
Thanks.
^ permalink raw reply
* RE: [PATCH net-next 01/16] qlge: Remove irq_cnt
From: Manish Chopra @ 2019-06-26 8:59 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <20190617074858.32467-1-bpoirier@suse.com>
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 01/16] qlge: Remove irq_cnt
>
> qlge uses an irq enable/disable refcounting scheme that is:
> * poorly implemented
> Uses a spin_lock to protect accesses to the irq_cnt atomic variable
> * buggy
> Breaks when there is not a 1:1 sequence of irq - napi_poll, such as
> when using SO_BUSY_POLL.
> * unnecessary
> The purpose or irq_cnt is to reduce irq control writes when
> multiple work items result from one irq: the irq is re-enabled
> after all work is done.
> Analysis of the irq handler shows that there is only one case where
> there might be two workers scheduled at once, and those have
> separate irq masking bits.
I believe you are talking about here for asic error recovery worker and MPI worker.
Which separate IRQ masking bits are you referring here ?
> static int ql_validate_flash(struct ql_adapter *qdev, u32 size, const char *str)
> @@ -2500,21 +2451,22 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> u32 var;
> int work_done = 0;
>
> - spin_lock(&qdev->hw_lock);
> - if (atomic_read(&qdev->intr_context[0].irq_cnt)) {
> - netif_printk(qdev, intr, KERN_DEBUG, qdev->ndev,
> - "Shared Interrupt, Not ours!\n");
> - spin_unlock(&qdev->hw_lock);
> - return IRQ_NONE;
> - }
> - spin_unlock(&qdev->hw_lock);
> + /* Experience shows that when using INTx interrupts, the device does
> + * not always auto-mask the interrupt.
> + * When using MSI mode, the interrupt must be explicitly disabled
> + * (even though it is auto-masked), otherwise a later command to
> + * enable it is not effective.
> + */
> + if (!test_bit(QL_MSIX_ENABLED, &qdev->flags))
> + ql_disable_completion_interrupt(qdev, 0);
Current code is disabling completion interrupt in case of MSI-X zeroth vector.
This change will break that behavior. Shouldn't zeroth vector in case of MSI-X also disable completion interrupt ?
If not, please explain why ?
>
> - var = ql_disable_completion_interrupt(qdev, intr_context->intr);
> + var = ql_read32(qdev, STS);
>
> /*
> * Check for fatal error.
> */
> if (var & STS_FE) {
> + ql_disable_completion_interrupt(qdev, 0);
Why need to do it again here ? if before this it can disable completion interrupt for INT-X case and MSI-X zeroth vector case ?
> ql_queue_asic_error(qdev);
> netdev_err(qdev->ndev, "Got fatal error, STS = %x.\n", var);
> var = ql_read32(qdev, ERR_STS);
> @@ -2534,7 +2486,6 @@ static irqreturn_t qlge_isr(int irq, void *dev_id)
> */
> netif_err(qdev, intr, qdev->ndev,
> "Got MPI processor interrupt.\n");
> - ql_disable_completion_interrupt(qdev, intr_context->intr);
Why disable interrupt is not required here ? While in case of Fatal error case above ql_disable_completion_interrupt() is being called ?
Also, in case of MSI-X zeroth vector it will not disable completion interrupt but at the end, it will end of qlge_isr() enabling completion interrupt.
Seems like disabling and enabling might not be in sync in case of MSI-X zeroth vector.
^ permalink raw reply
* Re: [PATCH nf-next v2 1/2] netfilter: nft_meta: add NFT_META_BRI_PVID support
From: Pablo Neira Ayuso @ 2019-06-26 9:01 UTC (permalink / raw)
To: wenxu; +Cc: fw, netfilter-devel, netdev
In-Reply-To: <1560993460-25569-1-git-send-email-wenxu@ucloud.cn>
On Thu, Jun 20, 2019 at 09:17:39AM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> nft add table bridge firewall
> nft add chain bridge firewall zones { type filter hook prerouting priority - 300 \; }
> nft add rule bridge firewall zones counter ct zone set vlan id map { 100 : 1, 200 : 2 }
>
> As above set the bridge port with pvid, the received packet don't contain
> the vlan tag which means the packet should belong to vlan 200 through pvid.
> With this pacth user can get the pvid of bridge ports.
>
> So add the following rule for as the first rule in the chain of zones.
>
> nft add rule bridge firewall zones counter meta brvlan set meta brpvid
Applied, thanks.
^ permalink raw reply
* Re: [PATCH nf-next v2 2/2] netfilter: nft_meta: Add NFT_META_BRI_VLAN support
From: Pablo Neira Ayuso @ 2019-06-26 9:01 UTC (permalink / raw)
To: wenxu; +Cc: fw, netfilter-devel, netdev
In-Reply-To: <1560993460-25569-2-git-send-email-wenxu@ucloud.cn>
On Thu, Jun 20, 2019 at 09:17:40AM +0800, wenxu@ucloud.cn wrote:
> From: wenxu <wenxu@ucloud.cn>
>
> nft add table bridge firewall
> nft add chain bridge firewall zones { type filter hook prerouting priority - 300 \; }
> nft add rule bridge firewall zones counter ct zone set vlan id map { 100 : 1, 200 : 2 }
>
> As above set the bridge port with pvid, the received packet don't contain
> the vlan tag which means the packet should belong to vlan 200 through pvid.
> With this pacth user can set the pvid in the prerouting hook before set zone
> id and conntrack.
>
> So add the following rule for as the first rule in the chain of zones.
>
> nft add rule bridge firewall zones counter meta brvlan set meta brpvid
Also applied, thanks.
^ permalink raw reply
* Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-06-26 9:04 UTC (permalink / raw)
To: Vladimir Oltean
Cc: René van Dorst, sean.wang, Florian Fainelli, David S. Miller,
matthias.bgg, Andrew Lunn, Vivien Didelot, frank-w, netdev,
linux-mediatek, linux-mips
In-Reply-To: <CA+h21hpkjHD07-o7W-5sUf+FqEeks17_W6VUROSDzdGokFvNWQ@mail.gmail.com>
On Wed, Jun 26, 2019 at 11:46:15AM +0300, Vladimir Oltean wrote:
> On Wed, 26 Jun 2019 at 10:42, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote:
> > > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > > > <linux@armlinux.org.uk> wrote:
> > > > > >
> > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > > > Hi Russell,
> > > > > > >
> > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > > > 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?
> > > > > >
> > > > > > Yes. mac_config() should not touch it; it is not always in a defined
> > > > > > state. For example, if you set modes via ethtool (the
> > > > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > > > zero irrespective of the true link state.
> > > > > >
> > > > >
> > > > > Experimentally, state->link is zero at the same time as state->speed
> > > > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > > > AN. What is your suggestion? Should I proceed to try and configure the
> > > > > MAC for SPEED_UNKNOWN?
> > > >
> > > > What would you have done with a PHY when the link is down, what speed
> > > > would you have configured in the phylib adjust_link callback? phylib
> > > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> > > >
> > >
> > > With phylib, I'd make the driver ignore the speed and do nothing.
> > > With phylink, I'd make the core not call mac_config.
> > > But what happened is I saw phylink call mac_config anyway, said
> > > 'weird' and proceeded to ignore it as I would have for phylib.
> > > I'm just not understanding your position - it seems like you're
> > > implying there's a bug in phylink and the function call with
> > > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> > > place, which is what I wanted to confirm.
> >
> > It is not a bug. It is a request to configure the MAC, and what it's
> > saying is "we don't know what speed and/or duplex".
> >
> > Take for instance when the network adapter is brought up initially.
> > The link is most likely down, but we should configure the initial MAC
> > operating parameters (such as the PHY interface). Phylink makes a
> > mac_config() call with the speed and duplex set to UNKNOWN.
> >
> > Using your theory, we shouldn't be making that call. In which case,
> > MAC drivers aren't going to initially configure their interface
> > settings.
> >
> > _That_ would be a bug.
> >
>
> So you're saying that:
> - state->link should not be checked, because it is not guaranteed to be valid
state->link is undefined.
> - state->speed, state->duplex, state->pause *should* be checked,
These will always be valid for FIXED and PHY modes, but _may_ be
UNKNOWN, meaning phylink does not have any information about what
the speed should be.
speed and duplex are not defined for inband modes, since the purpose
of inband modes is to communicate this information through... inband
information, which the MAC driver already has access to. pause is
a different matter because it is present in some inband modes but
not others.
Which fields may be examined are now documented in the phylink
documentation in mainline kernels.
> Is state->interface always valid?
Yes.
> I don't think I follow the pattern here. Or shouldn't I check speed,
> duplex and pause either, and try to pass the MAC UNKNOWN values,
> inevitably failing at some point? Do Marvell MACs have an UNKNOWN
> setting?
Why do you think that just because state->speed is SPEED_UNKNOWN you
have to dream up some weird "unknown" value for the MAC? Default it
to something sensible, just like you would do if phylib reports
SPEED_UNKNOWN during link-down. I really don't get what the problem
is here.
--
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: [bpf-next v2 08/10] bpf: Implement bpf_prog_test_run for perf event programs
From: Krzesimir Nowak @ 2019-06-26 9:10 UTC (permalink / raw)
To: Stanislav Fomichev
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: <20190625201220.GC10487@mini-arch>
On Tue, Jun 25, 2019 at 10:12 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> 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.
My impression about net/bpf/test_run.c was that it was a collection of
helpers for test runs of the network-related BPF programs, because
they are so similar to each other. So kernel/trace/bpf_trace.c looked
like an obvious place for the test_run implementation since other perf
trace BPF stuff was already there.
And about bpf_ctx_init - looks useful as it seems to me that it
handles the scenario where the size of the ctx struct grows, but still
allows passing older version of the struct (thus smaller) from
userspace for compatibility. Maybe that checking and copying part of
the function could be moved into some non-static helper function, so I
could use it and still skip the need for allocating memory for the
context?
>
> > + 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?
Two, mostly. One was that it is a static function and my code was
elsewhere. Second was that it does some cgroup storage setup and I'm
not sure if the perf event BPF program needs that.
>
> > + 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?
Reading the pe_prog_is_valid_access function tells me that it's not
possible - the only type of valid access is read. So maybe I should be
stricter about the requirements for the data_out and ctx_out sizes
(should be zero or return -EINVAL).
>
> > + 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.
Yeah, I was wondering about it. These changes are here to avoid
breaking the tests, since perf event program can actually be run now
and the test_run for perf event required certain sizes for ctx and
data.
So, I will either move them to a separate patch or rework the test_run
for perf event to accept the size between 0 and sizeof(struct
something), so the changes in tests maybe will not be necessary.
>
> > @@ -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
> >
--
Kinvolk GmbH | Adalbertstr.6a, 10999 Berlin | tel: +491755589364
Geschäftsführer/Directors: Alban Crequy, Chris Kühl, Iago López Galeiras
Registergericht/Court of registration: Amtsgericht Charlottenburg
Registernummer/Registration number: HRB 171414 B
Ust-ID-Nummer/VAT ID number: DE302207000
^ permalink raw reply
* RE: [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag
From: Manish Chopra @ 2019-06-26 9:12 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <20190617074858.32467-2-bpoirier@suse.com>
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [PATCH net-next 02/16] qlge: Remove page_chunk.last_flag
>
> As already done in ql_get_curr_lchunk(), this member can be replaced by a
> simple test.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 1 -
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 13 +++++--------
> 2 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 5d9a36deda08..0a156a95e981 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1363,7 +1363,6 @@ struct page_chunk {
> char *va; /* virt addr for this chunk */
> u64 map; /* mapping for master */
> unsigned int offset; /* offset for this chunk */
> - unsigned int last_flag; /* flag set for last chunk in page */
> };
>
> struct bq_desc {
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 0bfbe11db795..038a6bfc79c7 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -1077,11 +1077,9 @@ static int ql_get_next_chunk(struct ql_adapter
> *qdev, struct rx_ring *rx_ring,
> rx_ring->pg_chunk.offset += rx_ring->lbq_buf_size;
> if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
> rx_ring->pg_chunk.page = NULL;
> - lbq_desc->p.pg_chunk.last_flag = 1;
> } else {
> rx_ring->pg_chunk.va += rx_ring->lbq_buf_size;
> get_page(rx_ring->pg_chunk.page);
> - lbq_desc->p.pg_chunk.last_flag = 0;
> }
> return 0;
> }
> @@ -2778,6 +2776,8 @@ static int ql_alloc_tx_resources(struct ql_adapter
> *qdev,
>
> static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring
> *rx_ring) {
> + unsigned int last_offset = ql_lbq_block_size(qdev) -
> + rx_ring->lbq_buf_size;
> struct bq_desc *lbq_desc;
>
> uint32_t curr_idx, clean_idx;
> @@ -2787,13 +2787,10 @@ static void ql_free_lbq_buffers(struct ql_adapter
> *qdev, struct rx_ring *rx_ring
> while (curr_idx != clean_idx) {
> lbq_desc = &rx_ring->lbq[curr_idx];
>
> - if (lbq_desc->p.pg_chunk.last_flag) {
> - pci_unmap_page(qdev->pdev,
> - lbq_desc->p.pg_chunk.map,
> - ql_lbq_block_size(qdev),
> + if (lbq_desc->p.pg_chunk.offset == last_offset)
> + pci_unmap_page(qdev->pdev, lbq_desc-
> >p.pg_chunk.map,
> + ql_lbq_block_size(qdev),
> PCI_DMA_FROMDEVICE);
> - lbq_desc->p.pg_chunk.last_flag = 0;
> - }
>
> put_page(lbq_desc->p.pg_chunk.page);
> lbq_desc->p.pg_chunk.page = NULL;
> --
> 2.21.0
Acked-by: Manish Chopra <manishc@marvell.com>
^ permalink raw reply
* Re: [PATCH net-next] net: ethtool: Allow parsing ETHER_FLOW types when using flow_rule
From: Maxime Chevallier @ 2019-06-26 9:23 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: davem, Florian Fainelli, Jiri Pirko, Jakub Kicinski, netdev,
linux-kernel, Antoine Tenart, thomas.petazzoni
In-Reply-To: <20190626085846.ax277ojvyp5k3abt@salvia>
Hi Pablo,
On Wed, 26 Jun 2019 10:58:46 +0200
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>On Wed, Jun 26, 2019 at 10:44:03AM +0200, Maxime Chevallier wrote:
>> When parsing an ethtool_rx_flow_spec, users can specify an ethernet flow
>> which could contain matches based on the ethernet header, such as the
>> MAC address, the VLAN tag or the ethertype.
>>
>> Only the ethtype field is specific to the ether flow, the MAC and vlan
>> fields are processed using the special FLOW_EXT and FLOW_MAC_EXT flags.
>>
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> ---
>> net/core/ethtool.c | 12 ++++++++++++
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
>> index 4d1011b2e24f..01ceba556341 100644
>> --- a/net/core/ethtool.c
>> +++ b/net/core/ethtool.c
>> @@ -2883,6 +2883,18 @@ ethtool_rx_flow_rule_create(const struct ethtool_rx_flow_spec_input *input)
>> match->mask.basic.n_proto = htons(0xffff);
>>
>> switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_EXT | FLOW_RSS)) {
>> + case ETHER_FLOW: {
>> + const struct ethhdr *ether_spec, *ether_m_spec;
>> +
>> + ether_spec = &fs->h_u.ether_spec;
>> + ether_m_spec = &fs->m_u.ether_spec;
>> +
>> + if (ether_m_spec->h_proto) {
>> + match->key.basic.n_proto = ether_spec->h_proto;
>> + match->mask.basic.n_proto = ether_m_spec->h_proto;
>> + }
>
>I see some drivers in the tree also interpret the h_source and h_dest
>fields?
Ah yes you're right. I assumed these fields were specific to the
FLOW_MAC_EXT flags, but by looking into the ethtool code, it seems we
do need to handle the h_source and h_dest fields.
I'll respin with these fields added.
Thanks for the review,
Maxime
^ permalink raw reply
* RE: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
From: Manish Chopra @ 2019-06-26 9:24 UTC (permalink / raw)
To: Benjamin Poirier, GR-Linux-NIC-Dev, netdev@vger.kernel.org
In-Reply-To: <20190617074858.32467-3-bpoirier@suse.com>
> -----Original Message-----
> From: Benjamin Poirier <bpoirier@suse.com>
> Sent: Monday, June 17, 2019 1:19 PM
> To: Manish Chopra <manishc@marvell.com>; GR-Linux-NIC-Dev <GR-Linux-
> NIC-Dev@marvell.com>; netdev@vger.kernel.org
> Subject: [EXT] [PATCH net-next 03/16] qlge: Deduplicate lbq_buf_size
>
> External Email
>
> ----------------------------------------------------------------------
> lbq_buf_size is duplicated to every rx_ring structure whereas lbq_buf_order is
> present once in the ql_adapter structure. All rings use the same buf size, keep
> only one copy of it. Also factor out the calculation of lbq_buf_size instead of
> having two copies.
>
> Signed-off-by: Benjamin Poirier <bpoirier@suse.com>
> ---
> drivers/net/ethernet/qlogic/qlge/qlge.h | 2 +-
> drivers/net/ethernet/qlogic/qlge/qlge_dbg.c | 2 +-
> drivers/net/ethernet/qlogic/qlge/qlge_main.c | 61 +++++++++-----------
> 3 files changed, 28 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge.h
> b/drivers/net/ethernet/qlogic/qlge/qlge.h
> index 0a156a95e981..ba61b4559dd6 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge.h
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge.h
> @@ -1433,7 +1433,6 @@ struct rx_ring {
> /* Large buffer queue elements. */
> u32 lbq_len; /* entry count */
> u32 lbq_size; /* size in bytes of queue */
> - u32 lbq_buf_size;
> void *lbq_base;
> dma_addr_t lbq_base_dma;
> void *lbq_base_indirect;
> @@ -2108,6 +2107,7 @@ struct ql_adapter {
> struct rx_ring rx_ring[MAX_RX_RINGS];
> struct tx_ring tx_ring[MAX_TX_RINGS];
> unsigned int lbq_buf_order;
> + u32 lbq_buf_size;
>
> int rx_csum;
> u32 default_rx_queue;
> diff --git a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> index 31389ab8bdf7..46599d74c6fb 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_dbg.c
> @@ -1630,6 +1630,7 @@ void ql_dump_qdev(struct ql_adapter *qdev)
> DUMP_QDEV_FIELD(qdev, "0x%08x", xg_sem_mask);
> DUMP_QDEV_FIELD(qdev, "0x%08x", port_link_up);
> DUMP_QDEV_FIELD(qdev, "0x%08x", port_init);
> + DUMP_QDEV_FIELD(qdev, "%u", lbq_buf_size);
> }
> #endif
>
> @@ -1774,7 +1775,6 @@ void ql_dump_rx_ring(struct rx_ring *rx_ring)
> pr_err("rx_ring->lbq_curr_idx = %d\n", rx_ring->lbq_curr_idx);
> pr_err("rx_ring->lbq_clean_idx = %d\n", rx_ring->lbq_clean_idx);
> pr_err("rx_ring->lbq_free_cnt = %d\n", rx_ring->lbq_free_cnt);
> - pr_err("rx_ring->lbq_buf_size = %d\n", rx_ring->lbq_buf_size);
>
> pr_err("rx_ring->sbq_base = %p\n", rx_ring->sbq_base);
> pr_err("rx_ring->sbq_base_dma = %llx\n", diff --git
> a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> index 038a6bfc79c7..9df06ad3fb93 100644
> --- a/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> +++ b/drivers/net/ethernet/qlogic/qlge/qlge_main.c
> @@ -995,15 +995,14 @@ static struct bq_desc *ql_get_curr_lchunk(struct
> ql_adapter *qdev,
> struct bq_desc *lbq_desc = ql_get_curr_lbuf(rx_ring);
>
> pci_dma_sync_single_for_cpu(qdev->pdev,
> - dma_unmap_addr(lbq_desc,
> mapaddr),
> - rx_ring->lbq_buf_size,
> - PCI_DMA_FROMDEVICE);
> + dma_unmap_addr(lbq_desc, mapaddr),
> + qdev->lbq_buf_size,
> PCI_DMA_FROMDEVICE);
>
> /* If it's the last chunk of our master page then
> * we unmap it.
> */
> - if ((lbq_desc->p.pg_chunk.offset + rx_ring->lbq_buf_size)
> - == ql_lbq_block_size(qdev))
> + if (lbq_desc->p.pg_chunk.offset + qdev->lbq_buf_size ==
> + ql_lbq_block_size(qdev))
> pci_unmap_page(qdev->pdev,
> lbq_desc->p.pg_chunk.map,
> ql_lbq_block_size(qdev),
> @@ -1074,11 +1073,11 @@ static int ql_get_next_chunk(struct ql_adapter
> *qdev, struct rx_ring *rx_ring,
> /* Adjust the master page chunk for next
> * buffer get.
> */
> - rx_ring->pg_chunk.offset += rx_ring->lbq_buf_size;
> + rx_ring->pg_chunk.offset += qdev->lbq_buf_size;
> if (rx_ring->pg_chunk.offset == ql_lbq_block_size(qdev)) {
> rx_ring->pg_chunk.page = NULL;
> } else {
> - rx_ring->pg_chunk.va += rx_ring->lbq_buf_size;
> + rx_ring->pg_chunk.va += qdev->lbq_buf_size;
> get_page(rx_ring->pg_chunk.page);
> }
> return 0;
> @@ -1110,12 +1109,12 @@ static void ql_update_lbq(struct ql_adapter
> *qdev, struct rx_ring *rx_ring)
> lbq_desc->p.pg_chunk.offset;
> dma_unmap_addr_set(lbq_desc, mapaddr, map);
> dma_unmap_len_set(lbq_desc, maplen,
> - rx_ring->lbq_buf_size);
> + qdev->lbq_buf_size);
> *lbq_desc->addr = cpu_to_le64(map);
>
> pci_dma_sync_single_for_device(qdev->pdev, map,
> - rx_ring->lbq_buf_size,
> - PCI_DMA_FROMDEVICE);
> + qdev->lbq_buf_size,
> + PCI_DMA_FROMDEVICE);
> clean_idx++;
> if (clean_idx == rx_ring->lbq_len)
> clean_idx = 0;
> @@ -1880,8 +1879,7 @@ static struct sk_buff *ql_build_rx_skb(struct
> ql_adapter *qdev,
> }
> do {
> lbq_desc = ql_get_curr_lchunk(qdev, rx_ring);
> - size = (length < rx_ring->lbq_buf_size) ? length :
> - rx_ring->lbq_buf_size;
> + size = min(length, qdev->lbq_buf_size);
>
> netif_printk(qdev, rx_status, KERN_DEBUG, qdev-
> >ndev,
> "Adding page %d to skb for %d bytes.\n",
> @@ -2776,12 +2774,12 @@ static int ql_alloc_tx_resources(struct ql_adapter
> *qdev,
>
> static void ql_free_lbq_buffers(struct ql_adapter *qdev, struct rx_ring
> *rx_ring) {
> - unsigned int last_offset = ql_lbq_block_size(qdev) -
> - rx_ring->lbq_buf_size;
> + unsigned int last_offset;
> struct bq_desc *lbq_desc;
>
> uint32_t curr_idx, clean_idx;
>
> + last_offset = ql_lbq_block_size(qdev) - qdev->lbq_buf_size;
> curr_idx = rx_ring->lbq_curr_idx;
> clean_idx = rx_ring->lbq_clean_idx;
> while (curr_idx != clean_idx) {
> @@ -3149,8 +3147,8 @@ static int ql_start_rx_ring(struct ql_adapter *qdev,
> struct rx_ring *rx_ring)
> } while (page_entries < MAX_DB_PAGES_PER_BQ(rx_ring-
> >lbq_len));
> cqicb->lbq_addr =
> cpu_to_le64(rx_ring->lbq_base_indirect_dma);
> - bq_len = (rx_ring->lbq_buf_size == 65536) ? 0 :
> - (u16) rx_ring->lbq_buf_size;
> + bq_len = (qdev->lbq_buf_size == 65536) ? 0 :
> + (u16)qdev->lbq_buf_size;
> cqicb->lbq_buf_size = cpu_to_le16(bq_len);
> bq_len = (rx_ring->lbq_len == 65536) ? 0 :
> (u16) rx_ring->lbq_len;
> @@ -4048,16 +4046,21 @@ static int qlge_close(struct net_device *ndev)
> return 0;
> }
>
> +static void qlge_set_lb_size(struct ql_adapter *qdev) {
> + if (qdev->ndev->mtu <= 1500)
> + qdev->lbq_buf_size = LARGE_BUFFER_MIN_SIZE;
> + else
> + qdev->lbq_buf_size = LARGE_BUFFER_MAX_SIZE;
> + qdev->lbq_buf_order = get_order(qdev->lbq_buf_size); }
> +
> static int ql_configure_rings(struct ql_adapter *qdev) {
> int i;
> struct rx_ring *rx_ring;
> struct tx_ring *tx_ring;
> int cpu_cnt = min(MAX_CPUS, (int)num_online_cpus());
> - unsigned int lbq_buf_len = (qdev->ndev->mtu > 1500) ?
> - LARGE_BUFFER_MAX_SIZE : LARGE_BUFFER_MIN_SIZE;
> -
> - qdev->lbq_buf_order = get_order(lbq_buf_len);
>
> /* In a perfect world we have one RSS ring for each CPU
> * and each has it's own vector. To do that we ask for @@ -4105,7
> +4108,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
> rx_ring->lbq_len = NUM_LARGE_BUFFERS;
> rx_ring->lbq_size =
> rx_ring->lbq_len * sizeof(__le64);
> - rx_ring->lbq_buf_size = (u16)lbq_buf_len;
> rx_ring->sbq_len = NUM_SMALL_BUFFERS;
> rx_ring->sbq_size =
> rx_ring->sbq_len * sizeof(__le64); @@ -4121,7
> +4123,6 @@ static int ql_configure_rings(struct ql_adapter *qdev)
> rx_ring->cq_len * sizeof(struct ql_net_rsp_iocb);
> rx_ring->lbq_len = 0;
> rx_ring->lbq_size = 0;
> - rx_ring->lbq_buf_size = 0;
> rx_ring->sbq_len = 0;
> rx_ring->sbq_size = 0;
> rx_ring->sbq_buf_size = 0;
> @@ -4140,6 +4141,7 @@ static int qlge_open(struct net_device *ndev)
> if (err)
> return err;
>
> + qlge_set_lb_size(qdev);
> err = ql_configure_rings(qdev);
> if (err)
> return err;
> @@ -4161,9 +4163,7 @@ static int qlge_open(struct net_device *ndev)
>
> static int ql_change_rx_buffers(struct ql_adapter *qdev) {
> - struct rx_ring *rx_ring;
> - int i, status;
> - u32 lbq_buf_len;
> + int status;
>
> /* Wait for an outstanding reset to complete. */
> if (!test_bit(QL_ADAPTER_UP, &qdev->flags)) { @@ -4186,16 +4186,7
> @@ static int ql_change_rx_buffers(struct ql_adapter *qdev)
> if (status)
> goto error;
>
> - /* Get the new rx buffer size. */
> - lbq_buf_len = (qdev->ndev->mtu > 1500) ?
> - LARGE_BUFFER_MAX_SIZE : LARGE_BUFFER_MIN_SIZE;
> - qdev->lbq_buf_order = get_order(lbq_buf_len);
> -
> - for (i = 0; i < qdev->rss_ring_count; i++) {
> - rx_ring = &qdev->rx_ring[i];
> - /* Set the new size. */
> - rx_ring->lbq_buf_size = lbq_buf_len;
> - }
> + qlge_set_lb_size(qdev);
>
> status = ql_adapter_up(qdev);
> if (status)
> --
> 2.21.0
Not sure if this change is really required, I think fields relevant to rx_ring should be present in the rx_ring structure.
There are various other fields like "lbq_len" and "lbq_size" which would be same for all rx rings but still under the relevant rx_ring structure.
^ 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