* Re: [RFC bpf-next 4/4] tools/bpf: handle EOPNOTSUPP when map lookup is failed
From: Jakub Kicinski @ 2018-09-20 15:59 UTC (permalink / raw)
To: Prashant Bhole
Cc: Alexei Starovoitov, Daniel Borkmann, Quentin Monnet,
David S . Miller, netdev
In-Reply-To: <6fb405e9-fe02-17c6-90e6-cf9c73ca7693@lab.ntt.co.jp>
On Thu, 20 Sep 2018 14:04:19 +0900, Prashant Bhole wrote:
> On 9/20/2018 12:29 AM, Jakub Kicinski wrote:
> > On Wed, 19 Sep 2018 16:51:43 +0900, Prashant Bhole wrote:
> >> Let's add a check for EOPNOTSUPP error when map lookup is failed.
> >> Also in case map doesn't support lookup, the output of map dump is
> >> changed from "can't lookup element" to "lookup not supported for
> >> this map".
> >>
> >> Patch adds function print_entry_error() function to print the error
> >> value.
> >>
> >> Following example dumps a map which does not support lookup.
> >>
> >> Output before:
> >> root# bpftool map -jp dump id 40
> >> [
> >> "key": ["0x0a","0x00","0x00","0x00"
> >> ],
> >> "value": {
> >> "error": "can\'t lookup element"
> >> },
> >> "key": ["0x0b","0x00","0x00","0x00"
> >> ],
> >> "value": {
> >> "error": "can\'t lookup element"
> >> }
> >> ]
> >>
> >> root# bpftool map dump id 40
> >> can't lookup element with key:
> >> 0a 00 00 00
> >> can't lookup element with key:
> >> 0b 00 00 00
> >> Found 0 elements
> >>
> >> Output after changes:
> >> root# bpftool map dump -jp id 45
> >> [
> >> "key": ["0x0a","0x00","0x00","0x00"
> >> ],
> >> "value": {
> >> "error": "lookup not supported for this map"
> >> },
> >> "key": ["0x0b","0x00","0x00","0x00"
> >> ],
> >> "value": {
> >> "error": "lookup not supported for this map"
> >> }
> >> ]
> >>
> >> root# bpftool map dump id 45
> >> key:
> >> 0a 00 00 00
> >> value:
> >> lookup not supported for this map
> >> key:
> >> 0b 00 00 00
> >> value:
> >> lookup not supported for this map
> >> Found 0 elements
> >
> > Nice improvement, thanks for the changes! I wonder what your thoughts
> > would be on just printing some form of "lookup not supported for this
> > map" only once? It seems slightly like repeated information - if
> > lookup is not supported for one key it likely won't be for other keys
> > too, so we could shorten the output. Would that make sense?
> >
> >> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> >> ---
> >> tools/bpf/bpftool/main.h | 5 +++++
> >> tools/bpf/bpftool/map.c | 35 ++++++++++++++++++++++++++++++-----
> >> 2 files changed, 35 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> >> index 40492cdc4e53..1a8c683f949b 100644
> >> --- a/tools/bpf/bpftool/main.h
> >> +++ b/tools/bpf/bpftool/main.h
> >> @@ -46,6 +46,11 @@
> >>
> >> #include "json_writer.h"
> >>
> >> +#define ERR_CANNOT_LOOKUP \
> >> + "can't lookup element"
> >> +#define ERR_LOOKUP_NOT_SUPPORTED \
> >> + "lookup not supported for this map"
> >
> > Do we need these? Are we going to reused them in more parts of the
> > code?
>
> These are used only once. These can be used in do_lookup(). Currently
> do_lookup() prints strerror(errno) when lookup is failed. Shall I change
> that do_lookup() output?
I actually prefer to stick to strerror(), the standard errors more
clearly correlate with what happened in my mind (i.e. "Operation not
supported" == kernel sent EOPNOTSUPP). strerror() may also print in
local language if translation/localization matters.
We could even use strerr() in dump_map_elem() but up to you. The one
in do_lookup() I'd prefer to leave be ;)
^ permalink raw reply
* [RFC PATCH] net: nixge: Address compiler warnings when building for i386
From: Moritz Fischer @ 2018-09-20 21:55 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, keescook, davem, moritz.fischer, f.fainelli,
Moritz Fischer
From: Moritz Fischer <mdf@kernel.org>
Address compiler warnings reported by kbuild autobuilders
when building for i386 as a result of dma_addr_t size on
different architectures.
warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
Hi all,
got an angry email from the kbuild bot.
This is an attempt at fixing its complaints. I'm travelling this week
so I don't have access to test this on hardware but maybe someone
can shoot a hole into this already.
I have *only build-tested* this for arm/arm64/x86/x86_64 and it seemed
to be happy.
Comments?
Thanks for your feedback
- Moritz
---
drivers/net/ethernet/ni/nixge.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/ni/nixge.c b/drivers/net/ethernet/ni/nixge.c
index 74cf52e3fb09..0611f2335b4a 100644
--- a/drivers/net/ethernet/ni/nixge.c
+++ b/drivers/net/ethernet/ni/nixge.c
@@ -127,8 +127,8 @@ struct nixge_hw_dma_bd {
#ifdef CONFIG_PHYS_ADDR_T_64BIT
#define nixge_hw_dma_bd_set_addr(bd, field, addr) \
do { \
- (bd)->field##_lo = lower_32_bits(((u64)addr)); \
- (bd)->field##_hi = upper_32_bits(((u64)addr)); \
+ (bd)->field##_lo = lower_32_bits((addr)); \
+ (bd)->field##_hi = upper_32_bits((addr)); \
} while (0)
#else
#define nixge_hw_dma_bd_set_addr(bd, field, addr) \
@@ -251,7 +251,7 @@ static void nixge_hw_dma_bd_release(struct net_device *ndev)
NIXGE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
- skb = (struct sk_buff *)
+ skb = (struct sk_buff *)(uintptr_t)
nixge_hw_dma_bd_get_addr(&priv->rx_bd_v[i],
sw_id_offset);
dev_kfree_skb(skb);
@@ -323,7 +323,7 @@ static int nixge_hw_dma_bd_init(struct net_device *ndev)
if (!skb)
goto out;
- nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], skb);
+ nixge_hw_dma_bd_set_offset(&priv->rx_bd_v[i], (uintptr_t)skb);
phys = dma_map_single(ndev->dev.parent, skb->data,
NIXGE_MAX_JUMBO_FRAME_SIZE,
DMA_FROM_DEVICE);
@@ -601,8 +601,8 @@ static int nixge_recv(struct net_device *ndev, int budget)
tail_p = priv->rx_bd_p + sizeof(*priv->rx_bd_v) *
priv->rx_bd_ci;
- skb = (struct sk_buff *)nixge_hw_dma_bd_get_addr(cur_p,
- sw_id_offset);
+ skb = (struct sk_buff *)(uintptr_t)
+ nixge_hw_dma_bd_get_addr(cur_p, sw_id_offset);
length = cur_p->status & XAXIDMA_BD_STS_ACTUAL_LEN_MASK;
if (length > NIXGE_MAX_JUMBO_FRAME_SIZE)
@@ -643,7 +643,7 @@ static int nixge_recv(struct net_device *ndev, int budget)
nixge_hw_dma_bd_set_phys(cur_p, cur_phys);
cur_p->cntrl = NIXGE_MAX_JUMBO_FRAME_SIZE;
cur_p->status = 0;
- nixge_hw_dma_bd_set_offset(cur_p, new_skb);
+ nixge_hw_dma_bd_set_offset(cur_p, (uintptr_t)new_skb);
++priv->rx_bd_ci;
priv->rx_bd_ci %= RX_BD_NUM;
--
2.18.0
^ permalink raw reply related
* Re: [PATCH net] netfilter: bridge: Don't sabotage nf_hook calls from an l3mdev
From: Pablo Neira Ayuso @ 2018-09-20 16:25 UTC (permalink / raw)
To: dsahern; +Cc: netdev, netfilter-devel, ndsouza, idosch, fw, David Ahern
In-Reply-To: <20180917152036.30674-1-dsahern@kernel.org>
On Mon, Sep 17, 2018 at 08:20:36AM -0700, dsahern@kernel.org wrote:
> From: David Ahern <dsahern@gmail.com>
>
> For starters, the bridge netfilter code registers operations that
> are invoked any time nh_hook is called. Specifically, ip_sabotage_in
> watches for nested calls for NF_INET_PRE_ROUTING when a bridge is in
> the stack.
>
> Packet wise, the bridge netfilter hook runs first. br_nf_pre_routing
> allocates nf_bridge, sets in_prerouting to 1 and calls NF_HOOK for
> NF_INET_PRE_ROUTING. It's finish function, br_nf_pre_routing_finish,
> then resets in_prerouting flag to 0 and the packet continues up the
> stack. The packet eventually makes it to the VRF driver and it invokes
> nf_hook for NF_INET_PRE_ROUTING in case any rules have been added against
> the vrf device.
>
> Because of the registered operations the call to nf_hook causes
> ip_sabotage_in to be invoked. That function sees the nf_bridge on the
> skb and that in_prerouting is not set. Thinking it is an invalid nested
> call it steals (drops) the packet.
>
> Update ip_sabotage_in to recognize that the bridge or one of its upper
> devices (e.g., vlan) can be enslaved to a VRF (L3 master device) and
> allow the packet to go through the nf_hook a second time.
Applied.
^ permalink raw reply
* Re: [PATCH] net: netronome: remove redundant continue
From: Jakub Kicinski @ 2018-09-20 16:38 UTC (permalink / raw)
To: zhong jiang; +Cc: davem, dirk.vandermerwe, simon.horman, netdev, linux-kernel
In-Reply-To: <1537430541-37817-1-git-send-email-zhongjiang@huawei.com>
On Thu, 20 Sep 2018 16:02:21 +0800, zhong jiang wrote:
> The continue will not truely skip any code. hence it is safe to
> remove it.
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
I think this came up during review at some point. I still prefer to
keep the continue. The body of the loop performs initialization of
objects, if an object is removed we shouldn't carry on with the body.
It's easy to miss that the object got freed otherwise, there is no
error being set and no warning printed...
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
> index 0b1ac9c..50d7b58 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_main.c
> @@ -230,10 +230,8 @@ static void nfp_net_pf_free_vnics(struct nfp_pf *pf)
> ctrl_bar += NFP_PF_CSR_SLICE_SIZE;
>
> /* Kill the vNIC if app init marked it as invalid */
> - if (nn->port && nn->port->type == NFP_PORT_INVALID) {
> + if (nn->port && nn->port->type == NFP_PORT_INVALID)
> nfp_net_pf_free_vnic(pf, nn);
> - continue;
> - }
> }
>
> if (list_empty(&pf->vnics))
^ permalink raw reply
* [PATCH] bna: Remove unnecessary self assignment
From: Nathan Chancellor @ 2018-09-20 22:24 UTC (permalink / raw)
To: Rasesh Mody, Sudarsana Kalluru, Dept-GELinuxNICDev,
David S. Miller
Cc: netdev, linux-kernel, Nathan Chancellor
Clang warns when a variable is assigned to itself.
drivers/net/ethernet/brocade/bna/bna_enet.c:1800:9: warning: explicitly
assigning value of variable of type 'int' to itself [-Wself-assign]
for (i = i; i < (bna->ioceth.attr.num_ucmac * 2); i++)
~ ^ ~
drivers/net/ethernet/brocade/bna/bna_enet.c:1835:9: warning: explicitly
assigning value of variable of type 'int' to itself [-Wself-assign]
for (i = i; i < (bna->ioceth.attr.num_mcmac * 2); i++)
~ ^ ~
2 warnings generated.
Link: https://github.com/ClangBuiltLinux/linux/issues/110
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/ethernet/brocade/bna/bna_enet.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/brocade/bna/bna_enet.c b/drivers/net/ethernet/brocade/bna/bna_enet.c
index bba81735ce87..6d2d4527357c 100644
--- a/drivers/net/ethernet/brocade/bna/bna_enet.c
+++ b/drivers/net/ethernet/brocade/bna/bna_enet.c
@@ -1797,7 +1797,7 @@ bna_ucam_mod_init(struct bna_ucam_mod *ucam_mod, struct bna *bna,
/* A separate queue to allow synchronous setting of a list of MACs */
INIT_LIST_HEAD(&ucam_mod->del_q);
- for (i = i; i < (bna->ioceth.attr.num_ucmac * 2); i++)
+ for (; i < (bna->ioceth.attr.num_ucmac * 2); i++)
list_add_tail(&ucam_mod->ucmac[i].qe, &ucam_mod->del_q);
ucam_mod->bna = bna;
@@ -1832,7 +1832,7 @@ bna_mcam_mod_init(struct bna_mcam_mod *mcam_mod, struct bna *bna,
/* A separate queue to allow synchronous setting of a list of MACs */
INIT_LIST_HEAD(&mcam_mod->del_q);
- for (i = i; i < (bna->ioceth.attr.num_mcmac * 2); i++)
+ for (; i < (bna->ioceth.attr.num_mcmac * 2); i++)
list_add_tail(&mcam_mod->mcmac[i].qe, &mcam_mod->del_q);
mcam_mod->bna = bna;
--
2.19.0
^ permalink raw reply related
* Re: [PATCH rdma-next 1/5] RDMA/core: Provide getter and setter to access IB device name
From: Leon Romanovsky @ 2018-09-20 16:40 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, linux-s390, Ursula Braun,
David S. Miller, netdev, Selvin Xavier, Steve Wise, Lijun Ou,
Shiraz Saleem, Ariel Elior, Christian Benvenuti, Adit Ranadive,
Dennis Dalessandro
In-Reply-To: <20180920151541.GC30219@mellanox.com>
[-- Attachment #1: Type: text/plain, Size: 2700 bytes --]
On Thu, Sep 20, 2018 at 09:15:41AM -0600, Jason Gunthorpe wrote:
> On Thu, Sep 20, 2018 at 02:21:58PM +0300, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@mellanox.com>
> >
> > Prepare IB device name field to rename operation by ensuring that all
> > accesses to it are protected with lock and users don't see part of name.
>
> Oh dear, no, that isn't going to work, there is too much stuff using
> dev_name.. Did you read the comment on device_rename??
>
> https://elixir.bootlin.com/linux/v4.19-rc4/source/drivers/base/core.c#L2715
Yes, I read, it was mentioned in the cover letter.
>
> > The protection is done with global device_lock because it is used in
> > allocation and deallocation phases. At this stage, this lock is not
> > busy and easily can be moved to be per-device, once it will be needed.
> >
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
> > include/rdma/ib_verbs.h | 8 +++++++-
> > 2 files changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index 5a680a88aa87..3270cde6d806 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -170,6 +170,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
> > return NULL;
> > }
> >
> > +void ib_device_get_name(struct ib_device *ibdev, char *name)
> > +{
> > + down_read(&lists_rwsem);
> > + strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
> > + up_read(&lists_rwsem);
> > +}
> > +EXPORT_SYMBOL(ib_device_get_name);
>
> I think we have to follow netdev and just rely on device_rename()
> being 'good enough'.
>
> Switch everything to use dev_name()/etc rather than try and do
> something like this so the responsibility is on the device core to
> keep this working, not us.
>
> Turns out I have a series for that for unrelated reasons..
And what should I do now with this knowledge?
>
> > static int alloc_name(char *name)
> > {
> > unsigned long *inuse;
> > @@ -202,6 +210,21 @@ static int alloc_name(char *name)
> > return 0;
> > }
> >
> > +int ib_device_alloc_name(struct ib_device *ibdev, const char *pattern)
> > +{
> > + int ret = 0;
> > +
> > + mutex_lock(&device_mutex);
> > + strlcpy(ibdev->name, pattern, IB_DEVICE_NAME_MAX);
> > + if (strchr(ibdev->name, '%'))
> > + ret = alloc_name(ibdev->name);
> > +
> > + mutex_unlock(&device_mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(ib_device_alloc_name);
>
> Can't call alloc_name() without also adding to the list, this will
> allow duplicates.
I planned to change it in the future by moving to different name scheme
with unique naming.
>
> Jason
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH rdma-next 1/5] RDMA/core: Provide getter and setter to access IB device name
From: Jason Gunthorpe @ 2018-09-20 16:46 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, RDMA mailing list, linux-s390, Ursula Braun,
David S. Miller, netdev, Selvin Xavier, Steve Wise, Lijun Ou,
Shiraz Saleem, Ariel Elior, Christian Benvenuti, Adit Ranadive,
Dennis Dalessandro
In-Reply-To: <20180920164039.GM3519@mtr-leonro.mtl.com>
On Thu, Sep 20, 2018 at 07:40:39PM +0300, Leon Romanovsky wrote:
> > > The protection is done with global device_lock because it is used in
> > > allocation and deallocation phases. At this stage, this lock is not
> > > busy and easily can be moved to be per-device, once it will be needed.
> > >
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > drivers/infiniband/core/device.c | 24 +++++++++++++++++++++++-
> > > include/rdma/ib_verbs.h | 8 +++++++-
> > > 2 files changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > > index 5a680a88aa87..3270cde6d806 100644
> > > +++ b/drivers/infiniband/core/device.c
> > > @@ -170,6 +170,14 @@ static struct ib_device *__ib_device_get_by_name(const char *name)
> > > return NULL;
> > > }
> > >
> > > +void ib_device_get_name(struct ib_device *ibdev, char *name)
> > > +{
> > > + down_read(&lists_rwsem);
> > > + strlcpy(name, ibdev->name, IB_DEVICE_NAME_MAX);
> > > + up_read(&lists_rwsem);
> > > +}
> > > +EXPORT_SYMBOL(ib_device_get_name);
> >
> > I think we have to follow netdev and just rely on device_rename()
> > being 'good enough'.
> >
> > Switch everything to use dev_name()/etc rather than try and do
> > something like this so the responsibility is on the device core to
> > keep this working, not us.
> >
> > Turns out I have a series for that for unrelated reasons..
>
> And what should I do now with this knowledge?
Maybe I can send it today..
Jason
^ permalink raw reply
* Re: [PATCH v2 0/2] hv_netvsc: associate VF and PV device by serial number
From: Stephen Hemminger @ 2018-09-20 16:50 UTC (permalink / raw)
To: Lorenzo Pieralisi; +Cc: kys, haiyangz, sthemmin, devel, netdev, linux-pci
In-Reply-To: <20180920141820.GD838@e107981-ln.cambridge.arm.com>
On Thu, 20 Sep 2018 15:18:20 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Fri, Sep 14, 2018 at 12:54:55PM -0700, Stephen Hemminger wrote:
> > The Hyper-V implementation of PCI controller has concept of 32 bit serial number
> > (not to be confused with PCI-E serial number). This value is sent in the protocol
> > from the host to indicate SR-IOV VF device is attached to a synthetic NIC.
> >
> > Using the serial number (instead of MAC address) to associate the two devices
> > avoids lots of potential problems when there are duplicate MAC addresses from
> > tunnels or layered devices.
> >
> > The patch set is broken into two parts, one is for the PCI controller
> > and the other is for the netvsc device. Normally, these go through different
> > trees but sending them together here for better review. The PCI changes
> > were submitted previously, but the main review comment was "why do you
> > need this?". This is why.
>
> The question was more whether we should convert this serial number into
> a PCI slot number (that has user space visibility and that is what you are
> after) to improve the current matching, I do not question why you need
> it, just for the records.
The name slot is way overloaded in this context.
There is
windows slot number which comes from Hyperv
pci address slot which pci-hyperv sets from windows slot
pci slot api value which for normal devices comes from ACPI
this patch gets it from serial number
The netvsc driver needed to be able to find a PCI device based on the serial
number. The serial number was not visible in any current PCI-hyperv controller
values. The windows slot (wslot) is not the same the serial number.
^ permalink raw reply
* Re: [PATCH net-next 4/5] ipv6: do not drop vrf udp multicast packets
From: Mike Manning @ 2018-09-20 16:50 UTC (permalink / raw)
To: Paolo Abeni, netdev; +Cc: Dewi Morgan
In-Reply-To: <09797df92f76885b0e3244a3aca68dbf07a09a3a.camel@redhat.com>
On 20/09/2018 14:02, Paolo Abeni wrote:
> Hi,
>
> On Thu, 2018-09-20 at 09:58 +0100, Mike Manning wrote:
>> diff --git a/net/ipv6/ip6_input.c b/net/ipv6/ip6_input.c
>> index 108f5f88ec98..fc60f297d95b 100644
>> --- a/net/ipv6/ip6_input.c
>> +++ b/net/ipv6/ip6_input.c
>> @@ -325,9 +325,12 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk
>> {
>> const struct inet6_protocol *ipprot;
>> struct inet6_dev *idev;
>> + struct net_device *dev;
>> unsigned int nhoff;
>> + int sdif = inet6_sdif(skb);
>> int nexthdr;
>> bool raw;
>> + bool deliver;
>> bool have_final = false;
> Please, try instead to sort the variable in reverse x-mas tree order.
Will do.
>>
>> /*
>> @@ -371,9 +374,27 @@ static int ip6_input_finish(struct net *net, struct sock *sk, struct sk_buff *sk
>> skb_postpull_rcsum(skb, skb_network_header(skb),
>> skb_network_header_len(skb));
>> hdr = ipv6_hdr(skb);
>> - if (ipv6_addr_is_multicast(&hdr->daddr) &&
>> - !ipv6_chk_mcast_addr(skb->dev, &hdr->daddr,
>> - &hdr->saddr) &&
>> +
>> + /* skb->dev passed may be master dev for vrfs. */
>> + if (sdif) {
>> + rcu_read_lock();
> AFAICS, the rcu lock is already acquired at the beginning of
> ip6_input_finish(), not need to acquire it here again.
Nice catch, I will remove this.
> + dev = dev_get_by_index_rcu(dev_net(skb->dev),
>> + sdif);
>> + if (!dev) {
>> + rcu_read_unlock();
>> + kfree_skb(skb);
>> + return -ENODEV;
>> + }
>> + } else {
>> + dev = skb->dev;
> The above fragment of code is a recurring pattern in this series,
> perhaps adding an helper for it would reduce code duplication ?
This pattern of checking the secondary device index is used only twice, both in this file.
But with now one instance having the rcu lock handling, and the other not, I cannot refactor this.
>
> Cheers,
>
> Paolo
>
Thanks for the review! I will wait for further comments before producing a v1 of the series.
Regards, Mike
^ permalink raw reply
* [PATCH] net: fddi: skfp: Remove unused function
From: Nathan Chancellor @ 2018-09-20 22:36 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel, Nathan Chancellor
Clang warns when a variable is assigned to itself.
drivers/net/fddi/skfp/pcmplc.c:1257:6: warning: explicitly assigning
value of variable of type 'int' to itself [-Wself-assign]
phy = phy ; on_off = on_off ;
~~~ ^ ~~~
drivers/net/fddi/skfp/pcmplc.c:1257:21: warning: explicitly assigning
value of variable of type 'int' to itself [-Wself-assign]
phy = phy ; on_off = on_off ;
~~~~~~ ^ ~~~~~~
2 warnings generated.
Turns out this entire function doesn't actually do anything since
SK_UNUSED is just casting the pointer to void. Remove it to silence
this Clang warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/128
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/fddi/skfp/ecm.c | 3 ---
drivers/net/fddi/skfp/h/cmtdef.h | 1 -
drivers/net/fddi/skfp/pcmplc.c | 11 -----------
3 files changed, 15 deletions(-)
diff --git a/drivers/net/fddi/skfp/ecm.c b/drivers/net/fddi/skfp/ecm.c
index eee9ba91346a..c813e462183d 100644
--- a/drivers/net/fddi/skfp/ecm.c
+++ b/drivers/net/fddi/skfp/ecm.c
@@ -30,7 +30,6 @@
*
* The following external HW dependent functions are referenced :
* sm_pm_bypass_req()
- * sm_pm_ls_latch()
* sm_pm_get_ls()
*
* The following HW dependent events are required :
@@ -356,8 +355,6 @@ static void ecm_fsm(struct s_smc *smc, int cmd)
*/
start_ecm_timer(smc,smc->s.ecm_check_poll,0) ;
smc->e.ecm_line_state = TRUE ; /* flag to pcm: report Q/HLS */
- (void) sm_pm_ls_latch(smc,PA,1) ; /* enable line state latch */
- (void) sm_pm_ls_latch(smc,PB,1) ; /* enable line state latch */
ACTIONS_DONE() ;
break ;
case EC6_CHECK :
diff --git a/drivers/net/fddi/skfp/h/cmtdef.h b/drivers/net/fddi/skfp/h/cmtdef.h
index 5d6891154367..a12f464941ed 100644
--- a/drivers/net/fddi/skfp/h/cmtdef.h
+++ b/drivers/net/fddi/skfp/h/cmtdef.h
@@ -513,7 +513,6 @@ void pcm_status_state(struct s_smc *smc, int np, int *type, int *state,
void plc_config_mux(struct s_smc *smc, int mux);
void sm_lem_evaluate(struct s_smc *smc);
void mac_update_counter(struct s_smc *smc);
-void sm_pm_ls_latch(struct s_smc *smc, int phy, int on_off);
void sm_ma_control(struct s_smc *smc, int mode);
void sm_mac_check_beacon_claim(struct s_smc *smc);
void config_mux(struct s_smc *smc, int mux);
diff --git a/drivers/net/fddi/skfp/pcmplc.c b/drivers/net/fddi/skfp/pcmplc.c
index a9ecf923f63d..6ef44c480bd5 100644
--- a/drivers/net/fddi/skfp/pcmplc.c
+++ b/drivers/net/fddi/skfp/pcmplc.c
@@ -30,7 +30,6 @@
* The following external HW dependent functions are referenced :
* sm_pm_control()
* sm_ph_linestate()
- * sm_pm_ls_latch()
*
* The following HW dependent events are required :
* PC_QLS
@@ -1248,16 +1247,6 @@ static void sm_ph_lem_stop(struct s_smc *smc, int np)
CLEAR(PLC(np,PL_INTR_MASK),PL_LE_CTR) ;
}
-/* ARGSUSED */
-void sm_pm_ls_latch(struct s_smc *smc, int phy, int on_off)
-/* int on_off; en- or disable ident. ls */
-{
- SK_UNUSED(smc) ;
-
- phy = phy ; on_off = on_off ;
-}
-
-
/*
* PCM pseudo code
* receive actions are called AFTER the bit n is received,
--
2.19.0
^ permalink raw reply related
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Song Liu @ 2018-09-20 22:42 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Eric Dumazet, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <2ca0e823642d232092017f66e8151652e22e74a1.camel@intel.com>
> On Sep 20, 2018, at 2:01 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>
> On Thu, 2018-09-20 at 13:35 -0700, Eric Dumazet wrote:
>> On 09/20/2018 12:01 PM, Song Liu wrote:
>>> The NIC driver should only enable interrupts when napi_complete_done()
>>> returns true. This patch adds the check for ixgbe.
>>>
>>> Cc: stable@vger.kernel.org # 4.10+
>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> Suggested-by: Eric Dumazet <edumazet@google.com>
>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>> ---
>>
>>
>> Well, unfortunately we do not know why this is needed,
>> this is why I have not yet sent this patch formally.
>>
>> netpoll has correct synchronization :
>>
>> poll_napi() places into napi->poll_owner current cpu number before
>> calling poll_one_napi()
>>
>> netpoll_poll_lock() does also use napi->poll_owner
>>
>> When netpoll calls ixgbe poll() method, it passed a budget of 0,
>> meaning napi_complete_done() is not called.
>>
>> As long as we can not explain the problem properly in the changelog,
>> we should investigate, otherwise we will probably see coming dozens of
>> patches
>> trying to fix a 'potential hazard'.
>
> Agreed, which is why I have our validation and developers looking into it,
> while we test the current patch from Song.
I figured out what is the issue here. And I have a proposal to fix it. I
have verified that this fixes the issue in our tests. But Alexei suggests
that it may not be the right way to fix.
Here is what happened:
netpoll tries to send skb with netpoll_start_xmit(). If that fails, it
calls netpoll_poll_dev(), which calls ndo_poll_controller(). Then, in
the driver, ndo_poll_controller() calls napi_schedule() for ALL NAPIs
within the same NIC.
This is problematic, because at the end napi_schedule() calls:
____napi_schedule(this_cpu_ptr(&softnet_data), n);
which attached these NAPIs to softnet_data on THIS CPU. This is done
via napi->poll_list.
Then suddenly ksoftirqd on this CPU owns multiple NAPIs. And it will
not give up the ownership until it calls napi_complete_done(). However,
for a very busy server, we usually use 16 CPUs to poll NAPI, so this
CPU can easily be overloaded. And as a result, each call of napi->poll()
will hit budget (of 64), and it will not call napi_complete_done(),
and the NAPI stays in the poll_list of this CPU.
When this happens, the host usually cannot get out of this state until
we throttle/stop client traffic.
I am pretty confident this is what happened. Please let me know if
anything above doesn't make sense.
Here is my proposal to fix it: Instead of polling all NAPIs within one
NIC, I would have netpoll to only poll the NAPI that will free space
for netpoll_start_xmit(). I attached my two RFC patches to the end of
this email.
I chatted with Alexei about this. He think polling only one NAPI may
not guarantee netpoll make progress with the TX queue we are aiming
for. Also, the bigger problem may be the fact that NAPIs could get
pinned to one CPU and cannot get released.
At this point, I really don't know what is the best way to fix this.
I will also work on a repro with netperf.
Please let me know your suggestions.
Thanks,
Song
========================= RFCs ============================
>From 7b6d1d0e21bc7a0034dbcacfdaaec95a0e5f5b14 Mon Sep 17 00:00:00 2001
From: Song Liu <songliubraving@fb.com>
Date: Thu, 20 Sep 2018 13:09:26 -0700
Subject: [RFC net 1/2] net: netpoll: when failed to send a message, only poll
the target NAPI
Currently, netpoll polls all NAPIs in the NIC if it fails to send data to
one TX queue. This is not necessary and sometimes problematic. This is
because ndo_poll_controller() calls napi_schedule() for all NAPIs in the
NIC, and attach available NAPIs to this_cpu's softnet_data->poll_list.
Modern highend NIC has tens of NAPIs, and requires multiple CPUs to poll
the data from the queues. Attaching multiple NAPIs to one CPU's poll_list
fan-in softirq work of multiple CPUs to one CPU. As a result, we see one
CPU pegged in ksoftirqd, while other CPUs cannot get assgined work.
This patch fixes this issue by only polls target NAPI when send fails.
New hook ndo_netpoll_get_napi() is added for the driver to map busy
TX queue to the NAPI to poll. If the driver implements this hook, netpoll
will only poll the NAPI of the target TX queue.
Signed-off-by: Song Liu <songliubraving@fb.com>
---
include/linux/netdevice.h | 5 +++++
net/core/netpoll.c | 30 ++++++++++++++++++++++++------
2 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 6a7ac01fa605..576414b150ff 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1033,6 +1033,9 @@ struct dev_ifalias {
*
* void (*ndo_poll_controller)(struct net_device *dev);
* Run napi_schedule() for all NAPI within this device.
+ * struct napi_struct* (*ndo_netpoll_get_napi)(struct net_device *dev,
+ * struct netdev_queue *txq);
+ * Returns the NAPI to poll for the given TX queue.
*
* SR-IOV management functions.
* int (*ndo_set_vf_mac)(struct net_device *dev, int vf, u8* mac);
@@ -1267,6 +1270,8 @@ struct net_device_ops {
__be16 proto, u16 vid);
#ifdef CONFIG_NET_POLL_CONTROLLER
void (*ndo_poll_controller)(struct net_device *dev);
+ struct napi_struct* (*ndo_netpoll_get_napi)(struct net_device *dev,
+ struct netdev_queue *txq);
int (*ndo_netpoll_setup)(struct net_device *dev,
struct netpoll_info *info);
void (*ndo_netpoll_cleanup)(struct net_device *dev);
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 57557a6a950c..218a46acbdfc 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -187,7 +187,7 @@ static void poll_napi(struct net_device *dev)
}
}
-static void netpoll_poll_dev(struct net_device *dev)
+static void netpoll_poll_dev(struct net_device *dev, struct napi_struct *napi)
{
const struct net_device_ops *ops;
struct netpoll_info *ni = rcu_dereference_bh(dev->npinfo);
@@ -210,10 +210,23 @@ static void netpoll_poll_dev(struct net_device *dev)
return;
}
- /* Process pending work on NIC */
- ops->ndo_poll_controller(dev);
+ if (napi) {
+ int cpu = smp_processor_id();
- poll_napi(dev);
+ /* If we know which NAPI to poll, just poll it. */
+ napi_schedule(napi);
+ if (cmpxchg(&napi->poll_owner, -1, cpu) == -1) {
+ poll_one_napi(napi);
+ smp_store_release(&napi->poll_owner, -1);
+ }
+ } else {
+ /* If we are not sure which NAPI to poll, process all
+ * pending work on NIC.
+ */
+ ops->ndo_poll_controller(dev);
+
+ poll_napi(dev);
+ }
up(&ni->dev_lock);
@@ -303,7 +316,7 @@ static struct sk_buff *find_skb(struct netpoll *np, int len, int reserve)
if (!skb) {
if (++count < 10) {
- netpoll_poll_dev(np->dev);
+ netpoll_poll_dev(np->dev, NULL);
goto repeat;
}
return NULL;
@@ -345,8 +358,13 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
/* don't get messages out of order, and no recursion */
if (skb_queue_len(&npinfo->txq) == 0 && !netpoll_owner_active(dev)) {
struct netdev_queue *txq;
+ struct napi_struct *napi;
txq = netdev_pick_tx(dev, skb, NULL);
+ if (dev->netdev_ops->ndo_netpoll_get_napi)
+ napi = dev->netdev_ops->ndo_netpoll_get_napi(dev, txq);
+ else
+ napi = NULL;
/* try until next clock tick */
for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
@@ -363,7 +381,7 @@ void netpoll_send_skb_on_dev(struct netpoll *np, struct sk_buff *skb,
}
/* tickle device maybe there is some cleanup */
- netpoll_poll_dev(np->dev);
+ netpoll_poll_dev(np->dev, napi);
udelay(USEC_PER_POLL);
}
--
2.17.1
>From 9f6bd53cf011820054802f17ede2f73de0ec7d41 Mon Sep 17 00:00:00 2001
From: Song Liu <songliubraving@fb.com>
Date: Thu, 20 Sep 2018 15:08:33 -0700
Subject: [RFC net 2/2] net: bnxt: add ndo_netpoll_get_napi
Signed-off-by: Song Liu <songliubraving@fb.com>
---
drivers/net/ethernet/broadcom/bnxt/bnxt.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 177587f9c3f1..0a82a61a16d9 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -7685,6 +7685,20 @@ static void bnxt_poll_controller(struct net_device *dev)
napi_schedule(&txr->bnapi->napi);
}
}
+
+static struct napi_struct* bnxt_netpoll_get_napi(struct net_device *dev,
+ struct netdev_queue *txq)
+{
+ struct bnxt *bp = netdev_priv(dev);
+ int i;
+
+ for (i = 0; i < bp->tx_nr_rings; i++) {
+ struct bnxt_tx_ring_info *txr = &bp->tx_ring[i];
+ if (txq == netdev_get_tx_queue(dev, txr->txq_index))
+ return &txr->bnapi->napi;
+ }
+ return NULL;
+}
#endif
static void bnxt_timer(struct timer_list *t)
@@ -8522,6 +8536,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
#endif
#ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller = bnxt_poll_controller,
+ .ndo_netpoll_get_napi = bnxt_netpoll_get_napi,
#endif
.ndo_setup_tc = bnxt_setup_tc,
#ifdef CONFIG_RFS_ACCEL
^ permalink raw reply related
* Re: [PATCH 3/3] ipv4: initialize ra_mutex in inet_init_net()
From: Cong Wang @ 2018-09-20 17:01 UTC (permalink / raw)
To: Kirill Tkhai
Cc: David Miller, Hideaki YOSHIFUJI, avagin, Eric Dumazet,
Eric W. Biederman, Linux Kernel Network Developers
In-Reply-To: <153743472937.21312.592412049853708197.stgit@localhost.localdomain>
On Thu, Sep 20, 2018 at 2:12 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> From: Cong Wang <xiyou.wangcong@gmail.com>
>
> ra_mutex is a IPv4 specific mutex, it is inside struct netns_ipv4,
> but its initialization is in the generic netns code, setup_net().
>
> Move it to IPv4 specific net init code, inet_init_net().
>
> Fixes: d9ff3049739e ("net: Replace ip_ra_lock with per-net mutex")
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: Kirill Tkhai <ktkhai@virtuozzo.com>
I regret for wasting my time on this, so:
Nacked-by: Cong Wang <xiyou.wangcong@gmail.com>
Let's just leave the current code as it is.
^ permalink raw reply
* Re: [PATCH 2/3] net: Register af_inet_ops earlier
From: Cong Wang @ 2018-09-20 17:03 UTC (permalink / raw)
To: Kirill Tkhai
Cc: David Miller, Hideaki YOSHIFUJI, avagin, Eric Dumazet,
Eric W. Biederman, Linux Kernel Network Developers
In-Reply-To: <153743469969.21312.2316847616468846087.stgit@localhost.localdomain>
On Thu, Sep 20, 2018 at 2:12 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> This function just initializes locks and defaults.
> Let register it before other pernet operation,
> since some of them potentially may relay on that.
>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
It adds no benefits but potential risks on error path
ordering, it is never late to bring this up again when any
future change needs it, until that:
Nacked-by: Cong Wang <xiyou.wangcong@gmail.com>
^ permalink raw reply
* [PATCH] net: lan78xx: Avoid unnecessary self assignment
From: Nathan Chancellor @ 2018-09-20 22:48 UTC (permalink / raw)
To: Woojung Huh, Microchip Linux Driver Support, David S. Miller
Cc: netdev, linux-usb, linux-kernel, Nathan Chancellor
Clang warns when a variable is assigned to itself.
drivers/net/usb/lan78xx.c:940:11: warning: explicitly assigning value of
variable of type 'u32' (aka 'unsigned int') to itself [-Wself-assign]
offset = offset;
~~~~~~ ^ ~~~~~~
1 warning generated.
Reorder the if statement to acheive the same result and avoid a self
assignment warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/129
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
drivers/net/usb/lan78xx.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 04f0a094d864..656441d9a955 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -936,11 +936,9 @@ static int lan78xx_read_otp(struct lan78xx_net *dev, u32 offset,
ret = lan78xx_read_raw_otp(dev, 0, 1, &sig);
if (ret == 0) {
- if (sig == OTP_INDICATOR_1)
- offset = offset;
- else if (sig == OTP_INDICATOR_2)
+ if (sig == OTP_INDICATOR_2)
offset += 0x100;
- else
+ else if (sig != OTP_INDICATOR_1)
ret = -EINVAL;
if (!ret)
ret = lan78xx_read_raw_otp(dev, offset, length, data);
--
2.19.0
^ permalink raw reply related
* Re: [Patch net-next] ipv4: initialize ra_mutex in inet_init_net()
From: Cong Wang @ 2018-09-20 17:05 UTC (permalink / raw)
To: Kirill Tkhai; +Cc: Linux Kernel Network Developers
In-Reply-To: <e425ef6d-f94c-731b-817a-6c743b777733@virtuozzo.com>
On Thu, Sep 20, 2018 at 2:04 AM Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>
> On 20.09.2018 0:28, Cong Wang wrote:
> You added me to CC, so you probably want to know my opinion about this.
> Since it's not a real problem fix, but just a refactoring, I say you
> my opinion, how this refactoring may be made better. If you don't want
> to know my opinion, you may consider not to CC me.
Sure, with respects to your opinions, I prefer to drop it happily.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 1/5] netlink: remove NLA_NESTED_COMPAT
From: Johannes Berg @ 2018-09-20 17:06 UTC (permalink / raw)
To: David Laight, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org
Cc: David Ahern
In-Reply-To: <1811E905-CD9B-4346-AF17-CE9DBEBBE4F6@sipsolutions.net>
On Thu, 2018-09-20 at 16:56 +0200, Johannes Berg wrote:
>
>
> > > @@ -172,7 +172,6 @@ enum {
> > > NLA_FLAG,
> > > NLA_MSECS,
> > > NLA_NESTED,
> > > - NLA_NESTED_COMPAT,
> > > NLA_NUL_STRING,
> > > NLA_BINARY,
> > > NLA_S8,
> >
> > ...
> >
> > Is it safe to remove an item from this emun ?
>
> I believe it is, since it's not part of uapi. At least as long as you
> recompile all netlink policies afterwards.
That came out confusing. It isn't UAPI, so the renumbering doesn't
matter, at least as long as you don't try to load a module compiled with
one version of the enum into a kernel compiled with the other, or
something strange like that.
johannes
^ permalink raw reply
* Re: [PATCH net] ixgbe: check return value of napi_complete_done()
From: Eric Dumazet @ 2018-09-20 23:22 UTC (permalink / raw)
To: Song Liu, Jeff Kirsher
Cc: Eric Dumazet, netdev, intel-wired-lan@lists.osuosl.org,
Kernel Team, stable@vger.kernel.org, Alexei Starovoitov
In-Reply-To: <0DAF1AF9-E98D-4D0F-BD68-F5936A0312C6@fb.com>
On 09/20/2018 03:42 PM, Song Liu wrote:
>
>
>> On Sep 20, 2018, at 2:01 PM, Jeff Kirsher <jeffrey.t.kirsher@intel.com> wrote:
>>
>> On Thu, 2018-09-20 at 13:35 -0700, Eric Dumazet wrote:
>>> On 09/20/2018 12:01 PM, Song Liu wrote:
>>>> The NIC driver should only enable interrupts when napi_complete_done()
>>>> returns true. This patch adds the check for ixgbe.
>>>>
>>>> Cc: stable@vger.kernel.org # 4.10+
>>>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>>> Suggested-by: Eric Dumazet <edumazet@google.com>
>>>> Signed-off-by: Song Liu <songliubraving@fb.com>
>>>> ---
>>>
>>>
>>> Well, unfortunately we do not know why this is needed,
>>> this is why I have not yet sent this patch formally.
>>>
>>> netpoll has correct synchronization :
>>>
>>> poll_napi() places into napi->poll_owner current cpu number before
>>> calling poll_one_napi()
>>>
>>> netpoll_poll_lock() does also use napi->poll_owner
>>>
>>> When netpoll calls ixgbe poll() method, it passed a budget of 0,
>>> meaning napi_complete_done() is not called.
>>>
>>> As long as we can not explain the problem properly in the changelog,
>>> we should investigate, otherwise we will probably see coming dozens of
>>> patches
>>> trying to fix a 'potential hazard'.
>>
>> Agreed, which is why I have our validation and developers looking into it,
>> while we test the current patch from Song.
>
> I figured out what is the issue here. And I have a proposal to fix it. I
> have verified that this fixes the issue in our tests. But Alexei suggests
> that it may not be the right way to fix.
>
> Here is what happened:
>
> netpoll tries to send skb with netpoll_start_xmit(). If that fails, it
> calls netpoll_poll_dev(), which calls ndo_poll_controller(). Then, in
> the driver, ndo_poll_controller() calls napi_schedule() for ALL NAPIs
> within the same NIC.
>
> This is problematic, because at the end napi_schedule() calls:
>
> ____napi_schedule(this_cpu_ptr(&softnet_data), n);
>
> which attached these NAPIs to softnet_data on THIS CPU. This is done
> via napi->poll_list.
>
> Then suddenly ksoftirqd on this CPU owns multiple NAPIs. And it will
> not give up the ownership until it calls napi_complete_done(). However,
> for a very busy server, we usually use 16 CPUs to poll NAPI, so this
> CPU can easily be overloaded. And as a result, each call of napi->poll()
> will hit budget (of 64), and it will not call napi_complete_done(),
> and the NAPI stays in the poll_list of this CPU.
>
> When this happens, the host usually cannot get out of this state until
> we throttle/stop client traffic.
>
>
> I am pretty confident this is what happened. Please let me know if
> anything above doesn't make sense.
>
>
> Here is my proposal to fix it: Instead of polling all NAPIs within one
> NIC, I would have netpoll to only poll the NAPI that will free space
> for netpoll_start_xmit(). I attached my two RFC patches to the end of
> this email.
>
> I chatted with Alexei about this. He think polling only one NAPI may
> not guarantee netpoll make progress with the TX queue we are aiming
> for. Also, the bigger problem may be the fact that NAPIs could get
> pinned to one CPU and cannot get released.
>
> At this point, I really don't know what is the best way to fix this.
>
> I will also work on a repro with netperf.
Thanks !
>
> Please let me know your suggestions.
>
Yeah, maybe that NICs using NAPI could not provide an ndo_poll_controller() method at all,
since it is very risky (potentially grab many NAPI, and end up in this locked situation)
poll_napi() could attempt to free skbs one napi at a time,
without the current cpu stealing all NAPI.
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 57557a6a950cc9cdff959391576a03381d328c1a..a992971d366090ba69d5c1af32eadd554d6880cf 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -205,13 +205,8 @@ static void netpoll_poll_dev(struct net_device *dev)
}
ops = dev->netdev_ops;
- if (!ops->ndo_poll_controller) {
- up(&ni->dev_lock);
- return;
- }
-
- /* Process pending work on NIC */
- ops->ndo_poll_controller(dev);
+ if (ops->ndo_poll_controller)
+ ops->ndo_poll_controller(dev);
poll_napi(dev);
^ permalink raw reply related
* Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
From: Marcelo Ricardo Leitner @ 2018-09-20 17:48 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev
In-Reply-To: <1537431250.3874.7.camel@sipsolutions.net>
On Thu, Sep 20, 2018 at 10:14:10AM +0200, Johannes Berg wrote:
> Anyway - we got into this discussion because of all the extra recursion
> stuff I was adding. With the change suggested by David we don't need
> that now at all, so I guess it'd be better to propose a patch if you (or
> perhaps I will see a need later) need such a facility for multiple
> messages or multiple message levels?
Yep!
Thanks,
Marcelo
^ permalink raw reply
* Re: [PATCH bpf-next] samples/bpf: fix compilation failure
From: Song Liu @ 2018-09-20 17:50 UTC (permalink / raw)
To: Prashant Bhole; +Cc: Alexei Starovoitov, Daniel Borkmann, Networking
In-Reply-To: <20180920075203.2464-1-bhole_prashant_q7@lab.ntt.co.jp>
On Thu, Sep 20, 2018 at 12:53 AM Prashant Bhole
<bhole_prashant_q7@lab.ntt.co.jp> wrote:
>
> following commit:
> commit d58e468b1112 ("flow_dissector: implements flow dissector BPF hook")
> added struct bpf_flow_keys which conflicts with the struct with
> same name in sockex2_kern.c and sockex3_kern.c
>
> similar to commit:
> commit 534e0e52bc23 ("samples/bpf: fix a compilation failure")
> we tried the rename it "flow_keys" but it also conflicted with struct
> having same name in include/net/flow_dissector.h. Hence renaming the
> struct to "flow_key_record". Also, this commit doesn't fix the
> compilation error completely because the similar struct is present in
> sockex3_kern.c. Hence renaming it in both files sockex3_user.c and
> sockex3_kern.c
>
> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> samples/bpf/sockex2_kern.c | 11 ++++++-----
> samples/bpf/sockex3_kern.c | 8 ++++----
> samples/bpf/sockex3_user.c | 4 ++--
> 3 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/samples/bpf/sockex2_kern.c b/samples/bpf/sockex2_kern.c
> index f58acfc92556..f2f9dbc021b0 100644
> --- a/samples/bpf/sockex2_kern.c
> +++ b/samples/bpf/sockex2_kern.c
> @@ -14,7 +14,7 @@ struct vlan_hdr {
> __be16 h_vlan_encapsulated_proto;
> };
>
> -struct bpf_flow_keys {
> +struct flow_key_record {
> __be32 src;
> __be32 dst;
> union {
> @@ -59,7 +59,7 @@ static inline __u32 ipv6_addr_hash(struct __sk_buff *ctx, __u64 off)
> }
>
> static inline __u64 parse_ip(struct __sk_buff *skb, __u64 nhoff, __u64 *ip_proto,
> - struct bpf_flow_keys *flow)
> + struct flow_key_record *flow)
> {
> __u64 verlen;
>
> @@ -83,7 +83,7 @@ static inline __u64 parse_ip(struct __sk_buff *skb, __u64 nhoff, __u64 *ip_proto
> }
>
> static inline __u64 parse_ipv6(struct __sk_buff *skb, __u64 nhoff, __u64 *ip_proto,
> - struct bpf_flow_keys *flow)
> + struct flow_key_record *flow)
> {
> *ip_proto = load_byte(skb,
> nhoff + offsetof(struct ipv6hdr, nexthdr));
> @@ -96,7 +96,8 @@ static inline __u64 parse_ipv6(struct __sk_buff *skb, __u64 nhoff, __u64 *ip_pro
> return nhoff;
> }
>
> -static inline bool flow_dissector(struct __sk_buff *skb, struct bpf_flow_keys *flow)
> +static inline bool flow_dissector(struct __sk_buff *skb,
> + struct flow_key_record *flow)
> {
> __u64 nhoff = ETH_HLEN;
> __u64 ip_proto;
> @@ -198,7 +199,7 @@ struct bpf_map_def SEC("maps") hash_map = {
> SEC("socket2")
> int bpf_prog2(struct __sk_buff *skb)
> {
> - struct bpf_flow_keys flow = {};
> + struct flow_key_record flow = {};
> struct pair *value;
> u32 key;
>
> diff --git a/samples/bpf/sockex3_kern.c b/samples/bpf/sockex3_kern.c
> index 95907f8d2b17..c527b57d3ec8 100644
> --- a/samples/bpf/sockex3_kern.c
> +++ b/samples/bpf/sockex3_kern.c
> @@ -61,7 +61,7 @@ struct vlan_hdr {
> __be16 h_vlan_encapsulated_proto;
> };
>
> -struct bpf_flow_keys {
> +struct flow_key_record {
> __be32 src;
> __be32 dst;
> union {
> @@ -88,7 +88,7 @@ static inline __u32 ipv6_addr_hash(struct __sk_buff *ctx, __u64 off)
> }
>
> struct globals {
> - struct bpf_flow_keys flow;
> + struct flow_key_record flow;
> };
>
> struct bpf_map_def SEC("maps") percpu_map = {
> @@ -114,14 +114,14 @@ struct pair {
>
> struct bpf_map_def SEC("maps") hash_map = {
> .type = BPF_MAP_TYPE_HASH,
> - .key_size = sizeof(struct bpf_flow_keys),
> + .key_size = sizeof(struct flow_key_record),
> .value_size = sizeof(struct pair),
> .max_entries = 1024,
> };
>
> static void update_stats(struct __sk_buff *skb, struct globals *g)
> {
> - struct bpf_flow_keys key = g->flow;
> + struct flow_key_record key = g->flow;
> struct pair *value;
>
> value = bpf_map_lookup_elem(&hash_map, &key);
> diff --git a/samples/bpf/sockex3_user.c b/samples/bpf/sockex3_user.c
> index 22f74d0e1493..9d02e0404719 100644
> --- a/samples/bpf/sockex3_user.c
> +++ b/samples/bpf/sockex3_user.c
> @@ -13,7 +13,7 @@
> #define PARSE_IP_PROG_FD (prog_fd[0])
> #define PROG_ARRAY_FD (map_fd[0])
>
> -struct flow_keys {
> +struct flow_key_record {
> __be32 src;
> __be32 dst;
> union {
> @@ -64,7 +64,7 @@ int main(int argc, char **argv)
> (void) f;
>
> for (i = 0; i < 5; i++) {
> - struct flow_keys key = {}, next_key;
> + struct flow_key_record key = {}, next_key;
> struct pair value;
>
> sleep(1);
> --
> 2.17.1
>
>
^ permalink raw reply
* Re: [PATCH net-next] net/tls: Add support for async encryption of records for performance
From: David Miller @ 2018-09-20 18:18 UTC (permalink / raw)
To: vakul.garg; +Cc: netdev, borisp, aviadye, davejwatson, doronrk
In-Reply-To: <20180919152135.20009-1-vakul.garg@nxp.com>
From: Vakul Garg <vakul.garg@nxp.com>
Date: Wed, 19 Sep 2018 20:51:35 +0530
> This patch enables encryption of multiple records in parallel when an
> async capable crypto accelerator is present in system.
This seems to be trading off zero copy with async support.
Async crypto device support is not the common case at all, and
synchronous crypto via cpu crypto acceleration instructions is
so much more likely.
Oh I see, the new logic is only triggered with ASYNC_CAPABLE is
set?
> +static inline bool is_tx_ready(struct tls_context *tls_ctx,
> + struct tls_sw_context_tx *ctx)
> +{
Two space between "inline" and "bool", please make it one.
> static void tls_write_space(struct sock *sk)
> {
> struct tls_context *ctx = tls_get_ctx(sk);
> + struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
Longest to shortest line (reverse christmas tree) ordering for
local variable declarations please.
>
> + list_for_each_prev(pos, &ctx->tx_ready_list) {
> + struct tls_rec *rec = (struct tls_rec *)pos;
> + u64 seq = be64_to_cpup((const __be64 *)&rec->aad_space);
Likewise.
> -static int tls_do_encryption(struct tls_context *tls_ctx,
> +int tls_tx_records(struct sock *sk, int flags)
> +{
> + struct tls_rec *rec, *tmp;
> + struct tls_context *tls_ctx = tls_get_ctx(sk);
> + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> + int rc = 0;
> + int tx_flags;
Likewise.
> +static void tls_encrypt_done(struct crypto_async_request *req, int err)
> +{
> + struct aead_request *aead_req = (struct aead_request *)req;
> + struct sock *sk = req->data;
> + struct tls_context *tls_ctx = tls_get_ctx(sk);
> + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> + struct tls_rec *rec;
> + int pending;
> + bool ready = false;
Likewise.
> +static int tls_do_encryption(struct sock *sk,
> + struct tls_context *tls_ctx,
> struct tls_sw_context_tx *ctx,
> struct aead_request *aead_req,
> size_t data_len)
> {
> int rc;
> + struct tls_rec *rec = ctx->open_rec;
Likewise.
> @@ -473,11 +630,12 @@ static int memcopy_from_iter(struct sock *sk, struct iov_iter *from,
> {
> struct tls_context *tls_ctx = tls_get_ctx(sk);
> struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> - struct scatterlist *sg = ctx->sg_plaintext_data;
> + struct tls_rec *rec = ctx->open_rec;
> + struct scatterlist *sg = rec->sg_plaintext_data;
> int copy, i, rc = 0;
Likewise.
> +struct tls_rec *get_rec(struct sock *sk)
> +{
> + int mem_size;
> + struct tls_context *tls_ctx = tls_get_ctx(sk);
> + struct tls_sw_context_tx *ctx = tls_sw_ctx_tx(tls_ctx);
> + struct tls_rec *rec;
Likewise.
> @@ -510,21 +707,33 @@ int tls_sw_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
> int record_room;
> bool full_record;
> int orig_size;
> + struct tls_rec *rec;
> bool is_kvec = msg->msg_iter.type & ITER_KVEC;
> + struct crypto_tfm *tfm = crypto_aead_tfm(ctx->aead_send);
> + bool async_capable = tfm->__crt_alg->cra_flags & CRYPTO_ALG_ASYNC;
> + int num_async = 0;
> + int num_zc = 0;
Likewise.
> @@ -661,6 +904,8 @@ int tls_sw_sendpage(struct sock *sk, struct page *page,
> struct scatterlist *sg;
> bool full_record;
> int record_room;
> + struct tls_rec *rec;
> + int num_async = 0;
Likewise.
^ permalink raw reply
* [PATCH net-next] net: phy: mdio-bcm-unimac: Allow configuring MDIO clock divider
From: Florian Fainelli @ 2018-09-21 0:05 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, David S. Miller, Rob Herring, Mark Rutland,
Andrew Lunn,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
Allow the configuration of the MDIO clock divider when the Device Tree
contains 'clock-frequency' property (similar to I2C and SPI buses).
Because the hardware may have lost its state during suspend/resume,
re-apply the MDIO clock divider upon resumption.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
.../bindings/net/brcm,unimac-mdio.txt | 3 +
drivers/net/phy/mdio-bcm-unimac.c | 83 ++++++++++++++++++-
2 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt b/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt
index 4648948f7c3b..e15589f47787 100644
--- a/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt
+++ b/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt
@@ -19,6 +19,9 @@ Optional properties:
- interrupt-names: must be "mdio_done_error" when there is a share interrupt fed
to this hardware block, or must be "mdio_done" for the first interrupt and
"mdio_error" for the second when there are separate interrupts
+- clocks: A reference to the clock supplying the MDIO bus controller
+- clock-frequency: the MDIO bus clock that must be output by the MDIO bus
+ hardware, if absent, the default hardware values are used
Child nodes of this MDIO bus controller node are standard Ethernet PHY device
nodes as described in Documentation/devicetree/bindings/net/phy.txt
diff --git a/drivers/net/phy/mdio-bcm-unimac.c b/drivers/net/phy/mdio-bcm-unimac.c
index 8d370667fa1b..80b9583eaa95 100644
--- a/drivers/net/phy/mdio-bcm-unimac.c
+++ b/drivers/net/phy/mdio-bcm-unimac.c
@@ -16,6 +16,7 @@
#include <linux/module.h>
#include <linux/io.h>
#include <linux/delay.h>
+#include <linux/clk.h>
#include <linux/of.h>
#include <linux/of_platform.h>
@@ -45,6 +46,8 @@ struct unimac_mdio_priv {
void __iomem *base;
int (*wait_func) (void *wait_func_data);
void *wait_func_data;
+ struct clk *clk;
+ u32 clk_freq;
};
static inline u32 unimac_mdio_readl(struct unimac_mdio_priv *priv, u32 offset)
@@ -189,6 +192,35 @@ static int unimac_mdio_reset(struct mii_bus *bus)
return 0;
}
+static void unimac_mdio_clk_set(struct unimac_mdio_priv *priv)
+{
+ unsigned long rate;
+ u32 reg, div;
+
+ /* Keep the hardware default values */
+ if (!priv->clk_freq)
+ return;
+
+ if (!priv->clk)
+ rate = 250000000;
+ else
+ rate = clk_get_rate(priv->clk);
+
+ div = (rate / (2 * priv->clk_freq)) - 1;
+ if (div & ~MDIO_CLK_DIV_MASK) {
+ pr_warn("Incorrect MDIO clock frequency, ignoring\n");
+ return;
+ }
+
+ /* The MDIO clock is the reference clock (typicaly 250Mhz) divided by
+ * 2 x (MDIO_CLK_DIV + 1)
+ */
+ reg = unimac_mdio_readl(priv, MDIO_CFG);
+ reg &= ~(MDIO_CLK_DIV_MASK << MDIO_CLK_DIV_SHIFT);
+ reg |= div << MDIO_CLK_DIV_SHIFT;
+ unimac_mdio_writel(priv, reg, MDIO_CFG);
+}
+
static int unimac_mdio_probe(struct platform_device *pdev)
{
struct unimac_mdio_pdata *pdata = pdev->dev.platform_data;
@@ -217,9 +249,26 @@ static int unimac_mdio_probe(struct platform_device *pdev)
return -ENOMEM;
}
+ priv->clk = devm_clk_get(&pdev->dev, NULL);
+ if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
+ return PTR_ERR(priv->clk);
+ else
+ priv->clk = NULL;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ if (of_property_read_u32(np, "clock-frequency", &priv->clk_freq))
+ priv->clk_freq = 0;
+
+ unimac_mdio_clk_set(priv);
+
priv->mii_bus = mdiobus_alloc();
- if (!priv->mii_bus)
- return -ENOMEM;
+ if (!priv->mii_bus) {
+ ret = -ENOMEM;
+ goto out_clk_disable;
+ }
bus = priv->mii_bus;
bus->priv = priv;
@@ -253,6 +302,8 @@ static int unimac_mdio_probe(struct platform_device *pdev)
out_mdio_free:
mdiobus_free(bus);
+out_clk_disable:
+ clk_disable_unprepare(priv->clk);
return ret;
}
@@ -262,10 +313,37 @@ static int unimac_mdio_remove(struct platform_device *pdev)
mdiobus_unregister(priv->mii_bus);
mdiobus_free(priv->mii_bus);
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static int unimac_mdio_suspend(struct device *d)
+{
+ struct unimac_mdio_priv *priv = dev_get_drvdata(d);
+
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static int unimac_mdio_resume(struct device *d)
+{
+ struct unimac_mdio_priv *priv = dev_get_drvdata(d);
+ int ret;
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ unimac_mdio_clk_set(priv);
return 0;
}
+static SIMPLE_DEV_PM_OPS(unimac_mdio_pm_ops,
+ unimac_mdio_suspend, unimac_mdio_resume);
+
static const struct of_device_id unimac_mdio_ids[] = {
{ .compatible = "brcm,genet-mdio-v5", },
{ .compatible = "brcm,genet-mdio-v4", },
@@ -281,6 +359,7 @@ static struct platform_driver unimac_mdio_driver = {
.driver = {
.name = UNIMAC_MDIO_DRV_NAME,
.of_match_table = unimac_mdio_ids,
+ .pm = &unimac_mdio_pm_ops,
},
.probe = unimac_mdio_probe,
.remove = unimac_mdio_remove,
--
2.17.1
^ permalink raw reply related
* [PATCH net-next 1/3] bnx2x: Add VF spoof-checking configuration
From: Shahed Shaikh @ 2018-09-20 18:22 UTC (permalink / raw)
To: davem; +Cc: netdev, Ariel.Elior, Dept-EngEverestLinuxL2
In-Reply-To: <20180920182252.13457-1-shahed.shaikh@cavium.com>
Add support for `ndo_set_vf_spoofchk' to allow PF control over
its VF spoof-checking configuration.
Signed-off-by: Shahed Shaikh <shahed.shaikh@cavium.com>
Signed-off-by: Ariel Elior <ariel.elior@cavium.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 1 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 80 ++++++++++++++++++++++-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h | 2 +
4 files changed, 82 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
index 0e508e5..142bc11 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h
@@ -494,6 +494,7 @@ int bnx2x_get_vf_config(struct net_device *dev, int vf,
int bnx2x_set_vf_mac(struct net_device *dev, int queue, u8 *mac);
int bnx2x_set_vf_vlan(struct net_device *netdev, int vf, u16 vlan, u8 qos,
__be16 vlan_proto);
+int bnx2x_set_vf_spoofchk(struct net_device *dev, int idx, bool val);
/* select_queue callback */
u16 bnx2x_select_queue(struct net_device *dev, struct sk_buff *skb,
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 71362b7..faf64ba 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -13121,6 +13121,7 @@ static int bnx2x_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
.ndo_set_vf_mac = bnx2x_set_vf_mac,
.ndo_set_vf_vlan = bnx2x_set_vf_vlan,
.ndo_get_vf_config = bnx2x_get_vf_config,
+ .ndo_set_vf_spoofchk = bnx2x_set_vf_spoofchk,
#endif
#ifdef NETDEV_FCOE_WWNN
.ndo_fcoe_get_wwn = bnx2x_fcoe_get_wwn,
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
index 62da465..2f76b37 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
@@ -209,7 +209,10 @@ void bnx2x_vfop_qctor_prep(struct bnx2x *bp,
*/
__set_bit(BNX2X_Q_FLG_TX_SWITCH, &setup_p->flags);
__set_bit(BNX2X_Q_FLG_TX_SEC, &setup_p->flags);
- __set_bit(BNX2X_Q_FLG_ANTI_SPOOF, &setup_p->flags);
+ if (vf->spoofchk)
+ __set_bit(BNX2X_Q_FLG_ANTI_SPOOF, &setup_p->flags);
+ else
+ __clear_bit(BNX2X_Q_FLG_ANTI_SPOOF, &setup_p->flags);
/* Setup-op rx parameters */
if (test_bit(BNX2X_Q_TYPE_HAS_RX, &q_type)) {
@@ -1269,6 +1272,8 @@ int bnx2x_iov_init_one(struct bnx2x *bp, int int_mode_param,
bnx2x_vf(bp, i, state) = VF_FREE;
mutex_init(&bnx2x_vf(bp, i, op_mutex));
bnx2x_vf(bp, i, op_current) = CHANNEL_TLV_NONE;
+ /* enable spoofchk by default */
+ bnx2x_vf(bp, i, spoofchk) = 1;
}
/* re-read the IGU CAM for VFs - index and abs_vfid must be set */
@@ -2632,7 +2637,7 @@ int bnx2x_get_vf_config(struct net_device *dev, int vfidx,
ivi->qos = 0;
ivi->max_tx_rate = 10000; /* always 10G. TBA take from link struct */
ivi->min_tx_rate = 0;
- ivi->spoofchk = 1; /*always enabled */
+ ivi->spoofchk = vf->spoofchk ? 1 : 0;
if (vf->state == VF_ENABLED) {
/* mac and vlan are in vlan_mac objects */
if (bnx2x_validate_vf_sp_objs(bp, vf, false)) {
@@ -2950,6 +2955,77 @@ int bnx2x_set_vf_vlan(struct net_device *dev, int vfidx, u16 vlan, u8 qos,
return rc;
}
+int bnx2x_set_vf_spoofchk(struct net_device *dev, int idx, bool val)
+{
+ struct bnx2x *bp = netdev_priv(dev);
+ struct bnx2x_virtf *vf;
+ int i, rc = 0;
+
+ vf = BP_VF(bp, idx);
+ if (!vf)
+ return -EINVAL;
+
+ /* nothing to do */
+ if (vf->spoofchk == val)
+ return 0;
+
+ vf->spoofchk = val ? 1 : 0;
+
+ DP(BNX2X_MSG_IOV, "%s spoofchk for VF %d\n",
+ val ? "enabling" : "disabling", idx);
+
+ /* is vf initialized and queue set up? */
+ if (vf->state != VF_ENABLED ||
+ bnx2x_get_q_logical_state(bp, &bnx2x_leading_vfq(vf, sp_obj)) !=
+ BNX2X_Q_LOGICAL_STATE_ACTIVE)
+ return rc;
+
+ /* User should be able to see error in system logs */
+ if (!bnx2x_validate_vf_sp_objs(bp, vf, true))
+ return -EINVAL;
+
+ /* send queue update ramrods to configure spoofchk */
+ for_each_vfq(vf, i) {
+ struct bnx2x_queue_state_params q_params = {NULL};
+ struct bnx2x_queue_update_params *update_params;
+
+ q_params.q_obj = &bnx2x_vfq(vf, i, sp_obj);
+
+ /* validate the Q is UP */
+ if (bnx2x_get_q_logical_state(bp, q_params.q_obj) !=
+ BNX2X_Q_LOGICAL_STATE_ACTIVE)
+ continue;
+
+ __set_bit(RAMROD_COMP_WAIT, &q_params.ramrod_flags);
+ q_params.cmd = BNX2X_Q_CMD_UPDATE;
+ update_params = &q_params.params.update;
+ __set_bit(BNX2X_Q_UPDATE_ANTI_SPOOF_CHNG,
+ &update_params->update_flags);
+ if (val) {
+ __set_bit(BNX2X_Q_UPDATE_ANTI_SPOOF,
+ &update_params->update_flags);
+ } else {
+ __clear_bit(BNX2X_Q_UPDATE_ANTI_SPOOF,
+ &update_params->update_flags);
+ }
+
+ /* Update the Queue state */
+ rc = bnx2x_queue_state_change(bp, &q_params);
+ if (rc) {
+ BNX2X_ERR("Failed to %s spoofchk on VF %d - vfq %d\n",
+ val ? "enable" : "disable", idx, i);
+ goto out;
+ }
+ }
+out:
+ if (!rc)
+ DP(BNX2X_MSG_IOV,
+ "%s spoofchk for VF[%d]\n", val ? "Enabled" : "Disabled",
+ idx);
+
+ return rc;
+}
+
/* crc is the first field in the bulletin board. Compute the crc over the
* entire bulletin board excluding the crc field itself. Use the length field
* as the Bulletin Board was posted by a PF with possibly a different version
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h
index eb814c6..b6ebd92 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h
@@ -142,6 +142,8 @@ struct bnx2x_virtf {
bool flr_clnup_stage; /* true during flr cleanup */
bool malicious; /* true if FW indicated so, until FLR */
+ /* 1(true) if spoof check is enabled */
+ u8 spoofchk;
/* dma */
dma_addr_t fw_stat_map;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 2/3] bnx2x: Ignore bandwidth attention in single function mode
From: Shahed Shaikh @ 2018-09-20 18:22 UTC (permalink / raw)
To: davem; +Cc: netdev, Ariel.Elior, Dept-EngEverestLinuxL2, Shahed Shaikh
In-Reply-To: <20180920182252.13457-1-shahed.shaikh@cavium.com>
From: Shahed Shaikh <Shahed.Shaikh@cavium.com>
This is a workaround for FW bug -
MFW generates bandwidth attention in single function mode, which
is only expected to be generated in multi function mode.
This undesired attention in SF mode results in incorrect HW
configuration and resulting into Tx timeout.
Signed-off-by: Shahed Shaikh <Shahed.Shaikh@cavium.com>
Signed-off-by: Ariel Elior <ariel.elior@cavium.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index faf64ba..16f64c6 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -3536,6 +3536,16 @@ static void bnx2x_drv_info_iscsi_stat(struct bnx2x *bp)
*/
static void bnx2x_config_mf_bw(struct bnx2x *bp)
{
+ /* Workaround for MFW bug.
+ * MFW is not supposed to generate BW attention in
+ * single function mode.
+ */
+ if (!IS_MF(bp)) {
+ DP(BNX2X_MSG_MCP,
+ "Ignoring MF BW config in single function mode\n");
+ return;
+ }
+
if (bp->link_vars.link_up) {
bnx2x_cmng_fns_init(bp, true, CMNG_FNS_MINMAX);
bnx2x_link_sync_notify(bp);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 3/3] bnx2x: Provide VF link status in ndo_get_vf_config
From: Shahed Shaikh @ 2018-09-20 18:22 UTC (permalink / raw)
To: davem; +Cc: netdev, Ariel.Elior, Dept-EngEverestLinuxL2, Shahed Shaikh
In-Reply-To: <20180920182252.13457-1-shahed.shaikh@cavium.com>
From: Shahed Shaikh <Shahed.Shaikh@cavium.com>
Provide current link status of VF in ndo_get_vf_config
handler.
Signed-off-by: Shahed Shaikh <Shahed.Shaikh@cavium.com>
Signed-off-by: Ariel Elior <ariel.elior@cavium.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
index 2f76b37..c835f6c 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
@@ -2638,6 +2638,7 @@ int bnx2x_get_vf_config(struct net_device *dev, int vfidx,
ivi->max_tx_rate = 10000; /* always 10G. TBA take from link struct */
ivi->min_tx_rate = 0;
ivi->spoofchk = vf->spoofchk ? 1 : 0;
+ ivi->linkstate = vf->link_cfg;
if (vf->state == VF_ENABLED) {
/* mac and vlan are in vlan_mac objects */
if (bnx2x_validate_vf_sp_objs(bp, vf, false)) {
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 0/3] bnx2x: enhancements
From: Shahed Shaikh @ 2018-09-20 18:22 UTC (permalink / raw)
To: davem; +Cc: netdev, Ariel.Elior, Dept-EngEverestLinuxL2, Shahed Shaikh
From: Shahed Shaikh <Shahed.Shaikh@cavium.com>
Hi Dave,
This series adds below changes -
- support for VF spoof-check configuration through .ndo_set_vf_spoofchk.
- workaround for MFW bug regarding unexpected bandwidth notifcation
in single function mode.
- supply VF link status as part of get VF config handling.
Please apply this to net-next tree.
Thanks,
Shahed
Shahed Shaikh (3):
bnx2x: Add VF spoof-checking configuration
bnx2x: Ignore bandwidth attention in single function mode
bnx2x: Provide VF link status in ndo_get_vf_config
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 1 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 11 +++
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 81 ++++++++++++++++++++++-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.h | 2 +
4 files changed, 93 insertions(+), 2 deletions(-)
--
1.8.3.1
^ 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