* Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference
From: Steffen Klassert @ 2017-06-09 8:32 UTC (permalink / raw)
To: Hangbin Liu; +Cc: David Miller, Xin Long, network dev
In-Reply-To: <CAPwn2JSeKnspZpnWTXsze7sCSXJ9mbq2K3-mu-muuy8iT4A1qw@mail.gmail.com>
On Fri, Jun 09, 2017 at 04:23:01PM +0800, Hangbin Liu wrote:
> Hi Steffen,
>
> BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
> But If we put the check in flow_cache_percpu_empty(), we can prevent
> other functions set fc->percpu to NULL, although not much possible : )
>
> So I'm not quite sure whether we should put the check in
> flow_cache_percpu_empty() or in xfrm_policy_flush().
Can't we just call xfrm_policy_fini() first and then flow_cache_fini()?
^ permalink raw reply
* Re: [PATCH v2 7/8] net: mvmdio: add xmdio support
From: Antoine Tenart @ 2017-06-09 8:25 UTC (permalink / raw)
To: Florian Fainelli
Cc: Antoine Tenart, davem, jason, andrew, gregory.clement,
sebastian.hesselbarth, thomas.petazzoni, mw, linux, netdev,
linux-arm-kernel
In-Reply-To: <a60a0b26-62b1-7c9e-e846-215747dddb1f@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]
On Thu, Jun 08, 2017 at 09:42:21AM -0700, Florian Fainelli wrote:
> On 06/08/2017 02:26 AM, Antoine Tenart wrote:
> > This patch adds the xMDIO interface support in the mvmdio driver. This
> > interface is used in Ethernet controllers on Marvell 370, 7k and 8k (as
> > of now). The xSMI interface supported by this driver complies with the
> > IEEE 802.3 clause 45 (while the SMI interface complies with the clause
> > 22). The xSMI interface is used by 10GbE devices.
>
> In the previous version you were properly defining a new compatibles
> strings for xmdio, but now you don't and instead you runtime select the
> operations based on whether MII_ADDR_C45 is set in the register which is
> fine from a functional perspective.
>
> If I get this right, the xMDIO controller is actually a superset of the
> MDIO controller and has an extra MVMDIO_XSMI_ADDR_REG register to
> preform C45 accesses?
This is also a mistake. It's not a superset as the register offsets are
different. So we can't use the same smi operations in both cases. We
would need dedicated xmdio smi operations, but I don't think there is a
board where a c22 PHY is connected to the xMDIO interface.
I'll add smi and xsmi flags and return -ENOTSUPP based on the
MII_ADDR_C45 bit and flags combination. If the need to support smi
operations on the xMDIO bus arise it will be fairly easy to implement in
the future. I'll rename the ops functions as well to differentiate mdio
and xmdio operations.
Thanks,
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: David Laight @ 2017-06-09 8:23 UTC (permalink / raw)
To: 'Mintz, Yuval', 'David Miller'
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Kalderon, Michal
In-Reply-To: <BLUPR0701MB2004CA6F1F262EC4E815E6B58DCE0-v5ruerSQ/oj1oiTf6dR/AE5OhdzP3rhOnBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
From: Mintz, Yuval
> Sent: 09 June 2017 08:52
> > From: David Laight [mailto:David.Laight-ZS65k/vG3HxXrIkS9f7CXA@public.gmane.org]
> > Sent: Friday, June 09, 2017 10:28 AM
> > To: 'David Miller' <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>; Mintz, Yuval
> > <Yuval.Mintz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> > Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Kalderon, Michal
> > <Michal.Kalderon-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> > Subject: RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
> >
> > From: David Miller
> > > Sent: 09 June 2017 00:24
> > >
> > > From: Yuval Mintz <Yuval.Mintz-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> > > Date: Thu, 8 Jun 2017 19:13:16 +0300
> > >
> > > > @@ -67,6 +79,21 @@ struct qed_ll2_stats {
> > > > u64 sent_bcast_pkts;
> > > > };
> > > >
> > > > +struct qed_ll2_tx_pkt_info {
> > > > + u8 num_of_bds;
> > > > + u16 vlan;
> > > > + u8 bd_flags;
> > > > + u16 l4_hdr_offset_w; /* from start of packet */
> > > > + enum qed_ll2_tx_dest tx_dest;
> > > > + enum qed_ll2_roce_flavor_type qed_roce_flavor;
> > > > + dma_addr_t first_frag;
> > > > + u16 first_frag_len;
> > > > + bool enable_ip_cksum;
> > > > + bool enable_l4_cksum;
> > > > + bool calc_ip_len;
> > > > + void *cookie;
> > > > +};
> > > > +
> > >
> > > This layout is extremely inefficient, with lots of padding in between
> > > struct members.
> > >
> > > Group small u8 members and u16 members together so that they consume
> > > full 32-bit areas so you can eliminate all of the padding.
> >
> > I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8)
> > variables is likely to generate extra code (on non-x86).
> > You are using 32 bits for the 'enum' - I bet the values fit in 8 bits, so aren't
> > really worried about size.
> >
> > If size did matter you can easily get the above down to 32 bytes.
>
> You're right, and that's exactly the point - since this is not data-path critical
> I don't see why the size/efficiency should matter [greatly].
It is just good practise so that it happens automatically when it does matter.
Just swapping 'vlan' and 'bd_flags' would make it look much better.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net] net/flow: fix fc->percpu NULL pointer dereference
From: Hangbin Liu @ 2017-06-09 8:23 UTC (permalink / raw)
To: Steffen Klassert; +Cc: David Miller, Xin Long, Hangbin Liu, network dev
In-Reply-To: <1496996036-22077-1-git-send-email-liuhangbin@gmail.com>
Hi Steffen,
BTW, If we put the check in xfrm_policy_flush(), we can prevent it earlier.
But If we put the check in flow_cache_percpu_empty(), we can prevent
other functions set fc->percpu to NULL, although not much possible : )
So I'm not quite sure whether we should put the check in
flow_cache_percpu_empty() or in xfrm_policy_flush().
Do you have any suggestion?
Thanks
Hangbin
2017-06-09 16:13 GMT+08:00 Hangbin Liu <liuhangbin@gmail.com>:
> Now we will force to do garbage collection if any policy removed in
> xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini()
> first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini()
> -> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer
> dereference when check percpu_empty. The code path looks like:
>
> flow_cache_fini()
> - fc->percpu = NULL
> xfrm_policy_fini()
> - xfrm_policy_flush()
> - xfrm_garbage_collect()
> - flow_cache_flush()
> - flow_cache_percpu_empty()
> - fcp = per_cpu_ptr(fc->percpu, cpu)
>
> To reproduce, just add ipsec in netns and then remove the netns.
>
> Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
> net/core/flow.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/net/core/flow.c b/net/core/flow.c
> index f7f5d19..321fc53 100644
> --- a/net/core/flow.c
> +++ b/net/core/flow.c
> @@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache *fc, int cpu)
> struct flow_cache_percpu *fcp;
> unsigned int i;
>
> - fcp = per_cpu_ptr(fc->percpu, cpu);
> - for (i = 0; i < flow_cache_hash_size(fc); i++)
> - if (!hlist_empty(&fcp->hash_table[i]))
> - return 0;
> + if (fc->percpu) {
> + fcp = per_cpu_ptr(fc->percpu, cpu);
> + for (i = 0; i < flow_cache_hash_size(fc); i++)
> + if (!hlist_empty(&fcp->hash_table[i]))
> + return 0;
> + }
> +
> return 1;
> }
>
> --
> 2.5.5
>
^ permalink raw reply
* Re: [PATCH 1/5] net: mvpp2: remove mvpp2_bm_cookie_{build,pool_get}
From: kbuild test robot @ 2017-06-09 8:22 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: kbuild-all, David S. Miller, netdev, Marcin Wojtas,
Stefan Chulski, Nadav Haklai, Hanna Hawa, Yehuda Yitschak,
Antoine Tenart, Gregory Clement, Markus Elfring, Marc Zyngier,
Thomas Petazzoni
In-Reply-To: <1496935647-29670-2-git-send-email-thomas.petazzoni@free-electrons.com>
[-- Attachment #1: Type: text/plain, Size: 2483 bytes --]
Hi Thomas,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.12-rc4 next-20170608]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Thomas-Petazzoni/net-mvpp2-fixes-and-cleanups/20170609-083211
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
Note: the linux-review/Thomas-Petazzoni/net-mvpp2-fixes-and-cleanups/20170609-083211 HEAD 5a6bfd0022913d8ffcfd0eae9235b9bee844cf53 builds fine.
It only hurts bisectibility.
All errors (new ones prefixed by >>):
drivers/net/ethernet/marvell/mvpp2.c: In function 'mvpp2_bm_cookie_pool_set':
>> drivers/net/ethernet/marvell/mvpp2.c:3913:26: error: 'MVPP2_BM_COOKIE_POOL_OFFS' undeclared (first use in this function)
bm = cookie & ~(0xFF << MVPP2_BM_COOKIE_POOL_OFFS);
^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/ethernet/marvell/mvpp2.c:3913:26: note: each undeclared identifier is reported only once for each function it appears in
vim +/MVPP2_BM_COOKIE_POOL_OFFS +3913 drivers/net/ethernet/marvell/mvpp2.c
3f518509 Marcin Wojtas 2014-07-10 3907
3f518509 Marcin Wojtas 2014-07-10 3908 /* Set pool number in a BM cookie */
3f518509 Marcin Wojtas 2014-07-10 3909 static inline u32 mvpp2_bm_cookie_pool_set(u32 cookie, int pool)
3f518509 Marcin Wojtas 2014-07-10 3910 {
3f518509 Marcin Wojtas 2014-07-10 3911 u32 bm;
3f518509 Marcin Wojtas 2014-07-10 3912
3f518509 Marcin Wojtas 2014-07-10 @3913 bm = cookie & ~(0xFF << MVPP2_BM_COOKIE_POOL_OFFS);
3f518509 Marcin Wojtas 2014-07-10 3914 bm |= ((pool & 0xFF) << MVPP2_BM_COOKIE_POOL_OFFS);
3f518509 Marcin Wojtas 2014-07-10 3915
3f518509 Marcin Wojtas 2014-07-10 3916 return bm;
:::::: The code at line 3913 was first introduced by commit
:::::: 3f518509dedc99f0b755d2ce68d24f610e3a005a ethernet: Add new driver for Marvell Armada 375 network unit
:::::: TO: Marcin Wojtas <mw@semihalf.com>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35296 bytes --]
^ permalink raw reply
* [PATCH net] net/flow: fix fc->percpu NULL pointer dereference
From: Hangbin Liu @ 2017-06-09 8:13 UTC (permalink / raw)
To: netdev; +Cc: David Miller, Xin Long, Steffen Klassert, Hangbin Liu
Now we will force to do garbage collection if any policy removed in
xfrm_policy_flush(). But during xfrm_net_exit(). We call flow_cache_fini()
first and set set fc->percpu to NULL. Then after we call xfrm_policy_fini()
-> frxm_policy_flush() -> flow_cache_flush(), we will get NULL pointer
dereference when check percpu_empty. The code path looks like:
flow_cache_fini()
- fc->percpu = NULL
xfrm_policy_fini()
- xfrm_policy_flush()
- xfrm_garbage_collect()
- flow_cache_flush()
- flow_cache_percpu_empty()
- fcp = per_cpu_ptr(fc->percpu, cpu)
To reproduce, just add ipsec in netns and then remove the netns.
Fixes: 35db06912189 ("xfrm: do the garbage collection after flushing policy")
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
net/core/flow.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/core/flow.c b/net/core/flow.c
index f7f5d19..321fc53 100644
--- a/net/core/flow.c
+++ b/net/core/flow.c
@@ -332,10 +332,13 @@ static int flow_cache_percpu_empty(struct flow_cache *fc, int cpu)
struct flow_cache_percpu *fcp;
unsigned int i;
- fcp = per_cpu_ptr(fc->percpu, cpu);
- for (i = 0; i < flow_cache_hash_size(fc); i++)
- if (!hlist_empty(&fcp->hash_table[i]))
- return 0;
+ if (fc->percpu) {
+ fcp = per_cpu_ptr(fc->percpu, cpu);
+ for (i = 0; i < flow_cache_hash_size(fc); i++)
+ if (!hlist_empty(&fcp->hash_table[i]))
+ return 0;
+ }
+
return 1;
}
--
2.5.5
^ permalink raw reply related
* Re: [PATCH] wireless: wlcore: spi: remove unnecessary variable
From: Kalle Valo @ 2017-06-09 8:12 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20170609032430.GA7432@embeddedgus>
"Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> Remove unnecessary variable and refactor the code.
>
> Addresses-Coverity-ID: 1365000
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
I'll remove "wireless:" from the prefix.
--
Kalle Valo
^ permalink raw reply
* RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: Mintz, Yuval @ 2017-06-09 7:51 UTC (permalink / raw)
To: David Laight, 'David Miller'
Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
Kalderon, Michal
In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6DD001B851@AcuExch.aculab.com>
> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Friday, June 09, 2017 10:28 AM
> To: 'David Miller' <davem@davemloft.net>; Mintz, Yuval
> <Yuval.Mintz@cavium.com>
> Cc: netdev@vger.kernel.org; linux-rdma@vger.kernel.org; Kalderon, Michal
> <Michal.Kalderon@cavium.com>
> Subject: RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
>
> From: David Miller
> > Sent: 09 June 2017 00:24
> >
> > From: Yuval Mintz <Yuval.Mintz@cavium.com>
> > Date: Thu, 8 Jun 2017 19:13:16 +0300
> >
> > > @@ -67,6 +79,21 @@ struct qed_ll2_stats {
> > > u64 sent_bcast_pkts;
> > > };
> > >
> > > +struct qed_ll2_tx_pkt_info {
> > > + u8 num_of_bds;
> > > + u16 vlan;
> > > + u8 bd_flags;
> > > + u16 l4_hdr_offset_w; /* from start of packet */
> > > + enum qed_ll2_tx_dest tx_dest;
> > > + enum qed_ll2_roce_flavor_type qed_roce_flavor;
> > > + dma_addr_t first_frag;
> > > + u16 first_frag_len;
> > > + bool enable_ip_cksum;
> > > + bool enable_l4_cksum;
> > > + bool calc_ip_len;
> > > + void *cookie;
> > > +};
> > > +
> >
> > This layout is extremely inefficient, with lots of padding in between
> > struct members.
> >
> > Group small u8 members and u16 members together so that they consume
> > full 32-bit areas so you can eliminate all of the padding.
>
> I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8)
> variables is likely to generate extra code (on non-x86).
> You are using 32 bits for the 'enum' - I bet the values fit in 8 bits, so aren't
> really worried about size.
>
> If size did matter you can easily get the above down to 32 bytes.
You're right, and that's exactly the point - since this is not data-path critical
I don't see why the size/efficiency should matter [greatly].
^ permalink raw reply
* RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: David Laight @ 2017-06-09 7:28 UTC (permalink / raw)
To: 'David Miller', Yuval.Mintz@cavium.com
Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
Michal.Kalderon@cavium.com
In-Reply-To: <20170608.192358.1444190196602150329.davem@davemloft.net>
From: David Miller
> Sent: 09 June 2017 00:24
>
> From: Yuval Mintz <Yuval.Mintz@cavium.com>
> Date: Thu, 8 Jun 2017 19:13:16 +0300
>
> > @@ -67,6 +79,21 @@ struct qed_ll2_stats {
> > u64 sent_bcast_pkts;
> > };
> >
> > +struct qed_ll2_tx_pkt_info {
> > + u8 num_of_bds;
> > + u16 vlan;
> > + u8 bd_flags;
> > + u16 l4_hdr_offset_w; /* from start of packet */
> > + enum qed_ll2_tx_dest tx_dest;
> > + enum qed_ll2_roce_flavor_type qed_roce_flavor;
> > + dma_addr_t first_frag;
> > + u16 first_frag_len;
> > + bool enable_ip_cksum;
> > + bool enable_l4_cksum;
> > + bool calc_ip_len;
> > + void *cookie;
> > +};
> > +
>
> This layout is extremely inefficient, with lots of padding in between
> struct members.
>
> Group small u8 members and u16 members together so that they consume
> full 32-bit areas so you can eliminate all of the padding.
I'd also query the use of u16 sizes/lengths, any arithmetic on u16 (and u8)
variables is likely to generate extra code (on non-x86).
You are using 32 bits for the 'enum' - I bet the values fit in 8 bits,
so aren't really worried about size.
If size did matter you can easily get the above down to 32 bytes.
David
^ permalink raw reply
* RE: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks when fail to register_netdevice
From: 高峰 @ 2017-06-09 7:26 UTC (permalink / raw)
To: 'Sven Eckelmann', b.a.t.m.a.n
Cc: gfree.wind, mareklindner, sw, a, davem, netdev
In-Reply-To: <5895676.ebVRaD88im@bentobox>
> From: Sven Eckelmann [mailto:sven@narfation.org]
> Sent: Friday, June 9, 2017 3:23 PM
> Subject: Re: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible
memleaks
> when fail to register_netdevice
>
> On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
> > From: Gao Feng <fgao@ikuai8.com>
> >
> > Because the func batadv_softif_init_late allocate some resources and
> > it would be invoked in register_netdevice. So we need to invoke the
> > func batadv_softif_free instead of free_netdev to cleanup when fail to
> > register_netdevice.
> >
> > Signed-off-by: Gao Feng <fgao@ikuai8.com>
> > ---
> > net/batman-adv/soft-interface.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/batman-adv/soft-interface.c
> > b/net/batman-adv/soft-interface.c index d042c99..90bf990 100644
> > --- a/net/batman-adv/soft-interface.c
> > +++ b/net/batman-adv/soft-interface.c
> > @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net
> *net, const char *name)
> > if (ret < 0) {
> > pr_err("Unable to register the batman interface '%s': %i\n",
> > name, ret);
> > - free_netdev(soft_iface);
> > + batadv_softif_free(soft_iface);
> > return NULL;
> > }
>
> It looks to me like this change is invalid after David's change [1]. Can
you
> confirm that?
>
> Thanks,
> Sven
>
> [1]
>
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=cf1
> 24db566e6b036b8bcbe8decbed740bdfac8c6
[Gao Feng]
yes, this change is unnecessary.
Best Regards
Feng
^ permalink raw reply
* Re: [B.A.T.M.A.N.] [PATCH net] net: batman-adv: Fix possible memleaks when fail to register_netdevice
From: Sven Eckelmann @ 2017-06-09 7:23 UTC (permalink / raw)
To: b.a.t.m.a.n; +Cc: gfree.wind, mareklindner, sw, a, davem, netdev, Gao Feng
In-Reply-To: <1493121800-28066-1-git-send-email-gfree.wind@foxmail.com>
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
On Dienstag, 25. April 2017 20:03:20 CEST gfree.wind@foxmail.com wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> Because the func batadv_softif_init_late allocate some resources and
> it would be invoked in register_netdevice. So we need to invoke the
> func batadv_softif_free instead of free_netdev to cleanup when fail
> to register_netdevice.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
> net/batman-adv/soft-interface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/batman-adv/soft-interface.c b/net/batman-adv/soft-interface.c
> index d042c99..90bf990 100644
> --- a/net/batman-adv/soft-interface.c
> +++ b/net/batman-adv/soft-interface.c
> @@ -1011,7 +1011,7 @@ struct net_device *batadv_softif_create(struct net *net, const char *name)
> if (ret < 0) {
> pr_err("Unable to register the batman interface '%s': %i\n",
> name, ret);
> - free_netdev(soft_iface);
> + batadv_softif_free(soft_iface);
> return NULL;
> }
It looks to me like this change is invalid after David's change [1]. Can you
confirm that?
Thanks,
Sven
[1] https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/commit/?id=cf124db566e6b036b8bcbe8decbed740bdfac8c6
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
From: Belgazal, Netanel @ 2017-06-09 7:13 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, Woodhouse, David, Machulsky, Zorik,
Matushevsky, Alexander, BSHARA, Said, Wilson, Matt,
Liguori, Anthony, Bshara, Nafea, Schmeilin, Evgeny
In-Reply-To: <20170608.191712.1829919738916549276.davem@davemloft.net>
My apologies,
I was not aware.
Will make sure this won't happen again.
Regards,
Netanel
________________________________________
From: David Miller <davem@davemloft.net>
Sent: Friday, June 9, 2017 2:17 AM
To: Belgazal, Netanel
Cc: netdev@vger.kernel.org; Woodhouse, David; Machulsky, Zorik; Matushevsky, Alexander; BSHARA, Said; Wilson, Matt; Liguori, Anthony; Bshara, Nafea; Schmeilin, Evgeny
Subject: Re: [PATCH net-next 0/8] Bug fixes in ena ethernet driver
Two parallel patch series to the same driver and targetting the
same GIT tree is extremely undesirable, please don't do this.
Submit one series, and once applied submit the second series.
I'm deleting all of your patches from my queue, please resubmit
things properly.
Thank you.
^ permalink raw reply
* Re: [PATCH 2/2 net-next] net: stmmac: Improve documentation on AVB parameters
From: Giuseppe CAVALLARO @ 2017-06-09 7:10 UTC (permalink / raw)
To: Joao Pinto, davem; +Cc: netdev, alexandre.torgue, David Miller
In-Reply-To: <95bf7beef1abfb3c140ef56a8f59374bf6513496.1496944749.git.jpinto@synopsys.com>
Hi Joao
On 6/8/2017 8:02 PM, Joao Pinto wrote:
> This patch fixes the description of the DT AVB parameters and gives
> an accurate example. It was also included the base values that were
> used to get the example' CBS paremeter values.
>
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> ---
> Documentation/devicetree/bindings/net/stmmac.txt | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
> index c3a7be6..707426d 100644
> --- a/Documentation/devicetree/bindings/net/stmmac.txt
> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
> @@ -109,10 +109,10 @@ Optional properties:
> [Attention] Queue 0 is reserved for legacy traffic
> and so no AVB is available in this queue.
> - Configure Credit Base Shaper (if AVB Mode selected):
> - - snps,send_slope: enable Low Power Interface
> - - snps,idle_slope: unlock on WoL
> - - snps,high_credit: max write outstanding req. limit
> - - snps,low_credit: max read outstanding req. limit
> + - snps,send_slope: Send Slope Credit value
> + - snps,idle_slope: Idle Slope Credit value
> + - snps,high_credit: High Credit value
> + - snps,low_credit: Low Credit value
> - snps,priority: TX queue priority (Range: 0x0 to 0xF)
> Examples:
>
> @@ -143,10 +143,18 @@ Examples:
>
> queue1 {
> snps,avb-algorithm;
> - snps,send_slope = <0x1000>;
> - snps,idle_slope = <0x1000>;
> - snps,high_credit = <0x3E800>;
> - snps,low_credit = <0xFFC18000>;
> + /*
> + * Example AVB parameters based on:
> + * Allocated Bandwidth: 40%
> + * Maximum Frame size: 1000 bytes
> + * Maximum Interference size: 1500 bytes
> + * Port Transmit Rate: 8
> + * Scaling Factor: 1024
> + */
> + snps,idle_slope = <0xCCC>;
> + snps,send_slope = <0x1333>;
> + snps,high_credit = <0x4B0000>;
Thanks for having taken care about this changes, please, as required,
add a cover-letter
and give more information about these values that can be tuned by user
and, for example,
the snps,high_credit could be as default = 0xbe4000 that is a reasonable
value because
comes from 1522 * 8 * 1024 and LOW credit is the two complement.
^^^^^
frame size ---> maximum is 16
Regards
Peppe
> + snps,low_credit = <0xFFB50000>;
> snps,priority = <0x1>;
> };
> };
^ permalink raw reply
* RE: [PATCH net-next 1/8] qed: LL2 to use packed information for tx
From: Mintz, Yuval @ 2017-06-09 7:08 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
Kalderon, Michal
In-Reply-To: <20170608.192358.1444190196602150329.davem@davemloft.net>
> > +struct qed_ll2_tx_pkt_info {
> > + u8 num_of_bds;
> > + u16 vlan;
> > + u8 bd_flags;
> > + u16 l4_hdr_offset_w; /* from start of packet */
> > + enum qed_ll2_tx_dest tx_dest;
> > + enum qed_ll2_roce_flavor_type qed_roce_flavor;
> > + dma_addr_t first_frag;
> > + u16 first_frag_len;
> > + bool enable_ip_cksum;
> > + bool enable_l4_cksum;
> > + bool calc_ip_len;
> > + void *cookie;
> > +};
> > +
>
> This layout is extremely inefficient, with lots of padding in between struct
> members.
>
> Group small u8 members and u16 members together so that they consume
> full 32-bit areas so you can eliminate all of the padding.
While I can mend the holes, the total structure size doesn't really change:
struct qed_ll2_tx_pkt_info {
u8 num_of_bds; /* 0 1 */
/* XXX 1 byte hole, try to pack */
u16 vlan; /* 2 2 */
u8 bd_flags; /* 4 1 */
/* XXX 1 byte hole, try to pack */
u16 l4_hdr_offset_w; /* 6 2 */
enum qed_ll2_tx_dest tx_dest; /* 8 4 */
enum qed_ll2_roce_flavor_type qed_roce_flavor; /* 12 4 */
dma_addr_t first_frag; /* 16 8 */
u16 first_frag_len; /* 24 2 */
bool enable_ip_cksum; /* 26 1 */
bool enable_l4_cksum; /* 27 1 */
bool calc_ip_len; /* 28 1 */
/* XXX 3 bytes hole, try to pack */
void * cookie; /* 32 8 */
/* size: 40, cachelines: 1, members: 12 */
/* sum members: 35, holes: 3, sum holes: 5 */
/* last cacheline: 40 bytes */
};
Becomes:
struct qed_ll2_tx_pkt_info {
void * cookie; /* 0 8 */
dma_addr_t first_frag; /* 8 8 */
enum qed_ll2_tx_dest tx_dest; /* 16 4 */
enum qed_ll2_roce_flavor_type qed_roce_flavor; /* 20 4 */
u16 vlan; /* 24 2 */
u16 l4_hdr_offset_w; /* 26 2 */
u16 first_frag_len; /* 28 2 */
u8 num_of_bds; /* 30 1 */
u8 bd_flags; /* 31 1 */
bool enable_ip_cksum; /* 32 1 */
bool enable_l4_cksum; /* 33 1 */
bool calc_ip_len; /* 34 1 */
/* size: 40, cachelines: 1, members: 12 */
/* padding: 5 */
/* last cacheline: 40 bytes */
};
I'm going to send the changed version in V2 as as there's no harm to it,
[+ we *can* reduce the size of qed_ll2_comp_rx_data you commented
for patch #2 in series]
But one thing I thought of asking - do we consider layouts of relatively
insignificant structures to be some golden coding standard?
^ permalink raw reply
* [PATCH net-next 8/8] net: ena: bug fix in lost tx packets detection mechanism
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
check_for_missing_tx_completions() is called from a timer
task and looking for lost tx packets.
The old implementation accumulate all the lost tx packets
and did not check if those packets were retrieved on a later stage.
This cause to a situation where the driver reset
the device for no reason.
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 1 -
drivers/net/ethernet/amazon/ena/ena_netdev.c | 66 +++++++++++++++------------
drivers/net/ethernet/amazon/ena/ena_netdev.h | 14 +++++-
3 files changed, 50 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 533b2fb..3ee55e2 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -80,7 +80,6 @@ static const struct ena_stats ena_stats_tx_strings[] = {
ENA_STAT_TX_ENTRY(tx_poll),
ENA_STAT_TX_ENTRY(doorbells),
ENA_STAT_TX_ENTRY(prepare_ctx_err),
- ENA_STAT_TX_ENTRY(missing_tx_comp),
ENA_STAT_TX_ENTRY(bad_req_id),
};
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 3c366bf..4f16ed3 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1995,6 +1995,7 @@ static netdev_tx_t ena_start_xmit(struct sk_buff *skb, struct net_device *dev)
tx_info->tx_descs = nb_hw_desc;
tx_info->last_jiffies = jiffies;
+ tx_info->print_once = 0;
tx_ring->next_to_use = ENA_TX_RING_IDX_NEXT(next_to_use,
tx_ring->ring_size);
@@ -2564,13 +2565,44 @@ static void ena_fw_reset_device(struct work_struct *work)
"Reset attempt failed. Can not reset the device\n");
}
-static void check_for_missing_tx_completions(struct ena_adapter *adapter)
+static int check_missing_comp_in_queue(struct ena_adapter *adapter,
+ struct ena_ring *tx_ring)
{
struct ena_tx_buffer *tx_buf;
unsigned long last_jiffies;
+ u32 missed_tx = 0;
+ int i;
+
+ for (i = 0; i < tx_ring->ring_size; i++) {
+ tx_buf = &tx_ring->tx_buffer_info[i];
+ last_jiffies = tx_buf->last_jiffies;
+ if (unlikely(last_jiffies &&
+ time_is_before_jiffies(last_jiffies + TX_TIMEOUT))) {
+ if (!tx_buf->print_once)
+ netif_notice(adapter, tx_err, adapter->netdev,
+ "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
+ tx_ring->qid, i);
+
+ tx_buf->print_once = 1;
+ missed_tx++;
+
+ if (unlikely(missed_tx > MAX_NUM_OF_TIMEOUTED_PACKETS)) {
+ netif_err(adapter, tx_err, adapter->netdev,
+ "The number of lost tx completions is above the threshold (%d > %d). Reset the device\n",
+ missed_tx, MAX_NUM_OF_TIMEOUTED_PACKETS);
+ set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
+ return -EIO;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static void check_for_missing_tx_completions(struct ena_adapter *adapter)
+{
struct ena_ring *tx_ring;
- int i, j, budget;
- u32 missed_tx;
+ int i, budget, rc;
/* Make sure the driver doesn't turn the device in other process */
smp_rmb();
@@ -2586,31 +2618,9 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
for (i = adapter->last_monitored_tx_qid; i < adapter->num_queues; i++) {
tx_ring = &adapter->tx_ring[i];
- for (j = 0; j < tx_ring->ring_size; j++) {
- tx_buf = &tx_ring->tx_buffer_info[j];
- last_jiffies = tx_buf->last_jiffies;
- if (unlikely(last_jiffies && time_is_before_jiffies(last_jiffies + TX_TIMEOUT))) {
- netif_notice(adapter, tx_err, adapter->netdev,
- "Found a Tx that wasn't completed on time, qid %d, index %d.\n",
- tx_ring->qid, j);
-
- u64_stats_update_begin(&tx_ring->syncp);
- missed_tx = tx_ring->tx_stats.missing_tx_comp++;
- u64_stats_update_end(&tx_ring->syncp);
-
- /* Clear last jiffies so the lost buffer won't
- * be counted twice.
- */
- tx_buf->last_jiffies = 0;
-
- if (unlikely(missed_tx > MAX_NUM_OF_TIMEOUTED_PACKETS)) {
- netif_err(adapter, tx_err, adapter->netdev,
- "The number of lost tx completion is above the threshold (%d > %d). Reset the device\n",
- missed_tx, MAX_NUM_OF_TIMEOUTED_PACKETS);
- set_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags);
- }
- }
- }
+ rc = check_missing_comp_in_queue(adapter, tx_ring);
+ if (unlikely(rc))
+ return;
budget--;
if (!budget)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 8828f1d..88b5e56 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -146,7 +146,18 @@ struct ena_tx_buffer {
u32 tx_descs;
/* num of buffers used by this skb */
u32 num_of_bufs;
- /* Save the last jiffies to detect missing tx packets */
+
+ /* Used for detect missing tx packets to limit the number of prints */
+ u32 print_once;
+ /* Save the last jiffies to detect missing tx packets
+ *
+ * sets to non zero value on ena_start_xmit and set to zero on
+ * napi and timer_Service_routine.
+ *
+ * while this value is not protected by lock,
+ * a given packet is not expected to be handled by ena_start_xmit
+ * and by napi/timer_service at the same time.
+ */
unsigned long last_jiffies;
struct ena_com_buf bufs[ENA_PKT_MAX_BUFS];
} ____cacheline_aligned;
@@ -170,7 +181,6 @@ struct ena_stats_tx {
u64 napi_comp;
u64 tx_poll;
u64 doorbells;
- u64 missing_tx_comp;
u64 bad_req_id;
};
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 6/8] net: ena: fix theoretical Rx stuck on low memory systems
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
For the rare case where the device runs out of free rx buffer
descriptors (in case of pressure on kernel memory),
and the napi handler continuously fail to refill new Rx descriptors
until device rx queue totally runs out of all free rx buffers
to post incoming packet, leading to a deadlock:
* The device won't send interrupts since all the new
Rx packets will be dropped.
* The napi handler won't try to allocate new Rx descriptors
since allocation is part of NAPI that's not being invoked any more
The fix involves detecting this scenario and rescheduling NAPI
(to refill buffers) by the keepalive/watchdog task.
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 1 +
drivers/net/ethernet/amazon/ena/ena_netdev.c | 55 +++++++++++++++++++++++++++
drivers/net/ethernet/amazon/ena/ena_netdev.h | 2 +
3 files changed, 58 insertions(+)
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 67b2338f..533b2fb 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -94,6 +94,7 @@ static const struct ena_stats ena_stats_rx_strings[] = {
ENA_STAT_RX_ENTRY(dma_mapping_err),
ENA_STAT_RX_ENTRY(bad_desc_num),
ENA_STAT_RX_ENTRY(rx_copybreak_pkt),
+ ENA_STAT_RX_ENTRY(empty_rx_ring),
};
static const struct ena_stats ena_stats_ena_com_strings[] = {
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 4e9fbdd..3c366bf 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -190,6 +190,7 @@ static void ena_init_io_rings(struct ena_adapter *adapter)
rxr->sgl_size = adapter->max_rx_sgl_size;
rxr->smoothed_interval =
ena_com_get_nonadaptive_moderation_interval_rx(ena_dev);
+ rxr->empty_rx_queue = 0;
}
}
@@ -2619,6 +2620,58 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
adapter->last_monitored_tx_qid = i % adapter->num_queues;
}
+/* trigger napi schedule after 2 consecutive detections */
+#define EMPTY_RX_REFILL 2
+/* For the rare case where the device runs out of Rx descriptors and the
+ * napi handler failed to refill new Rx descriptors (due to a lack of memory
+ * for example).
+ * This case will lead to a deadlock:
+ * The device won't send interrupts since all the new Rx packets will be dropped
+ * The napi handler won't allocate new Rx descriptors so the device will be
+ * able to send new packets.
+ *
+ * This scenario can happen when the kernel's vm.min_free_kbytes is too small.
+ * It is recommended to have at least 512MB, with a minimum of 128MB for
+ * constrained environment).
+ *
+ * When such a situation is detected - Reschedule napi
+ */
+static void check_for_empty_rx_ring(struct ena_adapter *adapter)
+{
+ struct ena_ring *rx_ring;
+ int i, refill_required;
+
+ if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
+ return;
+
+ if (test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))
+ return;
+
+ for (i = 0; i < adapter->num_queues; i++) {
+ rx_ring = &adapter->rx_ring[i];
+
+ refill_required =
+ ena_com_sq_empty_space(rx_ring->ena_com_io_sq);
+ if (unlikely(refill_required == (rx_ring->ring_size - 1))) {
+ rx_ring->empty_rx_queue++;
+
+ if (rx_ring->empty_rx_queue >= EMPTY_RX_REFILL) {
+ u64_stats_update_begin(&rx_ring->syncp);
+ rx_ring->rx_stats.empty_rx_ring++;
+ u64_stats_update_end(&rx_ring->syncp);
+
+ netif_err(adapter, drv, adapter->netdev,
+ "trigger refill for ring %d\n", i);
+
+ napi_schedule(rx_ring->napi);
+ rx_ring->empty_rx_queue = 0;
+ }
+ } else {
+ rx_ring->empty_rx_queue = 0;
+ }
+ }
+}
+
/* Check for keep alive expiration */
static void check_for_missing_keep_alive(struct ena_adapter *adapter)
{
@@ -2673,6 +2726,8 @@ static void ena_timer_service(unsigned long data)
check_for_missing_tx_completions(adapter);
+ check_for_empty_rx_ring(adapter);
+
if (debug_area)
ena_dump_stats_to_buf(adapter, debug_area);
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 0e22bce..8828f1d 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -184,6 +184,7 @@ struct ena_stats_rx {
u64 dma_mapping_err;
u64 bad_desc_num;
u64 rx_copybreak_pkt;
+ u64 empty_rx_ring;
};
struct ena_ring {
@@ -231,6 +232,7 @@ struct ena_ring {
struct ena_stats_tx tx_stats;
struct ena_stats_rx rx_stats;
};
+ int empty_rx_queue;
} ____cacheline_aligned;
struct ena_stats_dev {
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 7/8] net: ena: disable admin msix while working in polling mode
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_com.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index ea60b9e..f5b237e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -61,6 +61,8 @@
#define ENA_MMIO_READ_TIMEOUT 0xFFFFFFFF
+#define ENA_REGS_ADMIN_INTR_MASK 1
+
/*****************************************************************************/
/*****************************************************************************/
/*****************************************************************************/
@@ -1454,6 +1456,12 @@ void ena_com_admin_destroy(struct ena_com_dev *ena_dev)
void ena_com_set_admin_polling_mode(struct ena_com_dev *ena_dev, bool polling)
{
+ u32 mask_value = 0;
+
+ if (polling)
+ mask_value = ENA_REGS_ADMIN_INTR_MASK;
+
+ writel(mask_value, ena_dev->reg_bar + ENA_REGS_INTR_MASK_OFF);
ena_dev->admin_queue.polling = polling;
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 6/8] net: ena: fix theoretical Rx hang on low memory systems
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
For the rare case where the device runs out of free rx buffer
descriptors (in case of pressure on kernel memory),
and the napi handler continuously fail to refill new Rx descriptors
until device rx queue totally runs out of all free rx buffers
to post incoming packet, leading to a deadlock:
* The device won't send interrupts since all the new
Rx packets will be dropped.
* The napi handler won't try to allocate new Rx descriptors
since allocation is part of NAPI that's not being invoked any more
The fix involves detecting this scenario and rescheduling NAPI
(to refill buffers) by the keepalive/watchdog task.
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 1 +
drivers/net/ethernet/amazon/ena/ena_netdev.c | 55 +++++++++++++++++++++++++++
drivers/net/ethernet/amazon/ena/ena_netdev.h | 2 +
3 files changed, 58 insertions(+)
diff --git a/drivers/net/ethernet/amazon/ena/ena_ethtool.c b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
index 67b2338f..533b2fb 100644
--- a/drivers/net/ethernet/amazon/ena/ena_ethtool.c
+++ b/drivers/net/ethernet/amazon/ena/ena_ethtool.c
@@ -94,6 +94,7 @@ static const struct ena_stats ena_stats_rx_strings[] = {
ENA_STAT_RX_ENTRY(dma_mapping_err),
ENA_STAT_RX_ENTRY(bad_desc_num),
ENA_STAT_RX_ENTRY(rx_copybreak_pkt),
+ ENA_STAT_RX_ENTRY(empty_rx_ring),
};
static const struct ena_stats ena_stats_ena_com_strings[] = {
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 4e9fbdd..3c366bf 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -190,6 +190,7 @@ static void ena_init_io_rings(struct ena_adapter *adapter)
rxr->sgl_size = adapter->max_rx_sgl_size;
rxr->smoothed_interval =
ena_com_get_nonadaptive_moderation_interval_rx(ena_dev);
+ rxr->empty_rx_queue = 0;
}
}
@@ -2619,6 +2620,58 @@ static void check_for_missing_tx_completions(struct ena_adapter *adapter)
adapter->last_monitored_tx_qid = i % adapter->num_queues;
}
+/* trigger napi schedule after 2 consecutive detections */
+#define EMPTY_RX_REFILL 2
+/* For the rare case where the device runs out of Rx descriptors and the
+ * napi handler failed to refill new Rx descriptors (due to a lack of memory
+ * for example).
+ * This case will lead to a deadlock:
+ * The device won't send interrupts since all the new Rx packets will be dropped
+ * The napi handler won't allocate new Rx descriptors so the device will be
+ * able to send new packets.
+ *
+ * This scenario can happen when the kernel's vm.min_free_kbytes is too small.
+ * It is recommended to have at least 512MB, with a minimum of 128MB for
+ * constrained environment).
+ *
+ * When such a situation is detected - Reschedule napi
+ */
+static void check_for_empty_rx_ring(struct ena_adapter *adapter)
+{
+ struct ena_ring *rx_ring;
+ int i, refill_required;
+
+ if (!test_bit(ENA_FLAG_DEV_UP, &adapter->flags))
+ return;
+
+ if (test_bit(ENA_FLAG_TRIGGER_RESET, &adapter->flags))
+ return;
+
+ for (i = 0; i < adapter->num_queues; i++) {
+ rx_ring = &adapter->rx_ring[i];
+
+ refill_required =
+ ena_com_sq_empty_space(rx_ring->ena_com_io_sq);
+ if (unlikely(refill_required == (rx_ring->ring_size - 1))) {
+ rx_ring->empty_rx_queue++;
+
+ if (rx_ring->empty_rx_queue >= EMPTY_RX_REFILL) {
+ u64_stats_update_begin(&rx_ring->syncp);
+ rx_ring->rx_stats.empty_rx_ring++;
+ u64_stats_update_end(&rx_ring->syncp);
+
+ netif_err(adapter, drv, adapter->netdev,
+ "trigger refill for ring %d\n", i);
+
+ napi_schedule(rx_ring->napi);
+ rx_ring->empty_rx_queue = 0;
+ }
+ } else {
+ rx_ring->empty_rx_queue = 0;
+ }
+ }
+}
+
/* Check for keep alive expiration */
static void check_for_missing_keep_alive(struct ena_adapter *adapter)
{
@@ -2673,6 +2726,8 @@ static void ena_timer_service(unsigned long data)
check_for_missing_tx_completions(adapter);
+ check_for_empty_rx_ring(adapter);
+
if (debug_area)
ena_dump_stats_to_buf(adapter, debug_area);
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.h b/drivers/net/ethernet/amazon/ena/ena_netdev.h
index 0e22bce..8828f1d 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.h
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.h
@@ -184,6 +184,7 @@ struct ena_stats_rx {
u64 dma_mapping_err;
u64 bad_desc_num;
u64 rx_copybreak_pkt;
+ u64 empty_rx_ring;
};
struct ena_ring {
@@ -231,6 +232,7 @@ struct ena_ring {
struct ena_stats_tx tx_stats;
struct ena_stats_rx rx_stats;
};
+ int empty_rx_queue;
} ____cacheline_aligned;
struct ena_stats_dev {
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 5/8] net: ena: add missing unmap bars on device removal
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
This patch also change the mapping functions to devm_ functions
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 1e71e89..4e9fbdd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -2853,6 +2853,11 @@ static void ena_release_bars(struct ena_com_dev *ena_dev, struct pci_dev *pdev)
{
int release_bars;
+ if (ena_dev->mem_bar)
+ devm_iounmap(&pdev->dev, ena_dev->mem_bar);
+
+ devm_iounmap(&pdev->dev, ena_dev->reg_bar);
+
release_bars = pci_select_bars(pdev, IORESOURCE_MEM) & ENA_BAR_MASK;
pci_release_selected_regions(pdev, release_bars);
}
@@ -2940,8 +2945,9 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_free_ena_dev;
}
- ena_dev->reg_bar = ioremap(pci_resource_start(pdev, ENA_REG_BAR),
- pci_resource_len(pdev, ENA_REG_BAR));
+ ena_dev->reg_bar = devm_ioremap(&pdev->dev,
+ pci_resource_start(pdev, ENA_REG_BAR),
+ pci_resource_len(pdev, ENA_REG_BAR));
if (!ena_dev->reg_bar) {
dev_err(&pdev->dev, "failed to remap regs bar\n");
rc = -EFAULT;
@@ -2961,8 +2967,9 @@ static int ena_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
ena_set_push_mode(pdev, ena_dev, &get_feat_ctx);
if (ena_dev->tx_mem_queue_type == ENA_ADMIN_PLACEMENT_POLICY_DEV) {
- ena_dev->mem_bar = ioremap_wc(pci_resource_start(pdev, ENA_MEM_BAR),
- pci_resource_len(pdev, ENA_MEM_BAR));
+ ena_dev->mem_bar = devm_ioremap_wc(&pdev->dev,
+ pci_resource_start(pdev, ENA_MEM_BAR),
+ pci_resource_len(pdev, ENA_MEM_BAR));
if (!ena_dev->mem_bar) {
rc = -EFAULT;
goto err_device_destroy;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 3/8] net: ena: add missing return when ena_com_get_io_handlers() fails
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 0e3c60c7..1e71e89 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1543,6 +1543,7 @@ static int ena_create_io_tx_queue(struct ena_adapter *adapter, int qid)
"Failed to get TX queue handlers. TX queue num %d rc: %d\n",
qid, rc);
ena_com_destroy_io_queue(ena_dev, ena_qid);
+ return rc;
}
ena_com_update_numa_node(tx_ring->ena_com_io_cq, ctx.numa_node);
@@ -1607,6 +1608,7 @@ static int ena_create_io_rx_queue(struct ena_adapter *adapter, int qid)
"Failed to get RX queue handlers. RX queue num %d rc: %d\n",
qid, rc);
ena_com_destroy_io_queue(ena_dev, ena_qid);
+ return rc;
}
ena_com_update_numa_node(rx_ring->ena_com_io_cq, ctx.numa_node);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 4/8] net: ena: fix race condition between submit and completion admin command
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
Bug:
"Completion context is occupied" error printout will be noticed in
dmesg.
This error will cause the admin command to fail, which will lead to
an ena_probe() failure or a watchdog reset (depends on which admin
command failed).
Root cause:
__ena_com_submit_admin_cmd() is the function that submits new entries to
the admin queue.
The function have a check that makes sure the queue is not full and the
function does not override any outstanding command.
It uses head and tail indexes for this check.
The head is increased by ena_com_handle_admin_completion() which runs
from interrupt context, and the tail index is increased by the submit
function (the function is running under ->q_lock, so there is no risk
of multithread increment).
Each command is associated with a completion context. This context
allocated before call to __ena_com_submit_admin_cmd() and freed by
ena_com_wait_and_process_admin_cq_interrupts(), right after the command
was completed.
This can lead to a state where the head was increased, the check passed,
but the completion context is still in use.
Solution:
Use the atomic variable ->outstanding_cmds instead of using the head and
the tail indexes.
This variable is safe for use since it is bumped in get_comp_ctx() in
__ena_com_submit_admin_cmd() and is freed by comp_ctxt_release()
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_com.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index e1c2fab..ea60b9e 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -232,11 +232,9 @@ static struct ena_comp_ctx *__ena_com_submit_admin_cmd(struct ena_com_admin_queu
tail_masked = admin_queue->sq.tail & queue_size_mask;
/* In case of queue FULL */
- cnt = admin_queue->sq.tail - admin_queue->sq.head;
+ cnt = atomic_read(&admin_queue->outstanding_cmds);
if (cnt >= admin_queue->q_depth) {
- pr_debug("admin queue is FULL (tail %d head %d depth: %d)\n",
- admin_queue->sq.tail, admin_queue->sq.head,
- admin_queue->q_depth);
+ pr_debug("admin queue is full.\n");
admin_queue->stats.out_of_space++;
return ERR_PTR(-ENOSPC);
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 1/8] net: ena: fix rare uncompleted admin command false alarm
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
The current flow to detect admin completion is:
while (command_not_completed) {
if (timeout)
error
check_for_completion()
sleep()
}
So in case the sleep took more than the timeout
(in case the thread/workqueue was not scheduled due to higher priority
task or prolonged VMexit), the driver can detect a stall even if
the completion is present.
The fix changes the order of this function to first check for
completion and only after that check if the timeout expired.
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_com.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index 08d11ce..e1c2fab 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -508,15 +508,20 @@ static int ena_com_comp_status_to_errno(u8 comp_status)
static int ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_ctx,
struct ena_com_admin_queue *admin_queue)
{
- unsigned long flags;
- u32 start_time;
+ unsigned long flags, timeout;
int ret;
- start_time = ((u32)jiffies_to_usecs(jiffies));
+ timeout = jiffies + ADMIN_CMD_TIMEOUT_US;
+
+ while (1) {
+ spin_lock_irqsave(&admin_queue->q_lock, flags);
+ ena_com_handle_admin_completion(admin_queue);
+ spin_unlock_irqrestore(&admin_queue->q_lock, flags);
- while (comp_ctx->status == ENA_CMD_SUBMITTED) {
- if ((((u32)jiffies_to_usecs(jiffies)) - start_time) >
- ADMIN_CMD_TIMEOUT_US) {
+ if (comp_ctx->status != ENA_CMD_SUBMITTED)
+ break;
+
+ if (time_is_before_jiffies(timeout)) {
pr_err("Wait for completion (polling) timeout\n");
/* ENA didn't have any completion */
spin_lock_irqsave(&admin_queue->q_lock, flags);
@@ -528,10 +533,6 @@ static int ena_com_wait_and_process_admin_cq_polling(struct ena_comp_ctx *comp_c
goto err;
}
- spin_lock_irqsave(&admin_queue->q_lock, flags);
- ena_com_handle_admin_completion(admin_queue);
- spin_unlock_irqrestore(&admin_queue->q_lock, flags);
-
msleep(100);
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 2/8] net: ena: fix bug that might cause hang after consecutive open/close interface.
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
In-Reply-To: <1496991325-551-1-git-send-email-netanel@amazon.com>
From: Netanel Belgazal <netanel@amazon.com>
Fixing a bug that the driver does not unmask the IO interrupts
in ndo_open():
occasionally, the MSI-X interrupt (for one or more IO queues)
can be masked when ndo_close() was called.
If that is followed by ndo open(),
then the MSI-X will be still masked so no interrupt
will be received by the driver.
Signed-off-by: Netanel Belgazal <netanel@amazon.com>
---
drivers/net/ethernet/amazon/ena/ena_netdev.c | 41 ++++++++++++++++++----------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 7c1214d..0e3c60c7 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1078,6 +1078,26 @@ inline void ena_adjust_intr_moderation(struct ena_ring *rx_ring,
rx_ring->per_napi_bytes = 0;
}
+static inline void ena_unmask_interrupt(struct ena_ring *tx_ring,
+ struct ena_ring *rx_ring)
+{
+ struct ena_eth_io_intr_reg intr_reg;
+
+ /* Update intr register: rx intr delay,
+ * tx intr delay and interrupt unmask
+ */
+ ena_com_update_intr_reg(&intr_reg,
+ rx_ring->smoothed_interval,
+ tx_ring->smoothed_interval,
+ true);
+
+ /* It is a shared MSI-X.
+ * Tx and Rx CQ have pointer to it.
+ * So we use one of them to reach the intr reg
+ */
+ ena_com_unmask_intr(rx_ring->ena_com_io_cq, &intr_reg);
+}
+
static inline void ena_update_ring_numa_node(struct ena_ring *tx_ring,
struct ena_ring *rx_ring)
{
@@ -1108,7 +1128,6 @@ static int ena_io_poll(struct napi_struct *napi, int budget)
{
struct ena_napi *ena_napi = container_of(napi, struct ena_napi, napi);
struct ena_ring *tx_ring, *rx_ring;
- struct ena_eth_io_intr_reg intr_reg;
u32 tx_work_done;
u32 rx_work_done;
@@ -1149,22 +1168,9 @@ static int ena_io_poll(struct napi_struct *napi, int budget)
if (ena_com_get_adaptive_moderation_enabled(rx_ring->ena_dev))
ena_adjust_intr_moderation(rx_ring, tx_ring);
- /* Update intr register: rx intr delay,
- * tx intr delay and interrupt unmask
- */
- ena_com_update_intr_reg(&intr_reg,
- rx_ring->smoothed_interval,
- tx_ring->smoothed_interval,
- true);
-
- /* It is a shared MSI-X.
- * Tx and Rx CQ have pointer to it.
- * So we use one of them to reach the intr reg
- */
- ena_com_unmask_intr(rx_ring->ena_com_io_cq, &intr_reg);
+ ena_unmask_interrupt(tx_ring, rx_ring);
}
-
ena_update_ring_numa_node(tx_ring, rx_ring);
ret = rx_work_done;
@@ -1485,6 +1491,11 @@ static int ena_up_complete(struct ena_adapter *adapter)
ena_napi_enable_all(adapter);
+ /* Enable completion queues interrupt */
+ for (i = 0; i < adapter->num_queues; i++)
+ ena_unmask_interrupt(&adapter->tx_ring[i],
+ &adapter->rx_ring[i]);
+
/* schedule napi in case we had pending packets
* from the last time we disable napi
*/
--
2.7.4
^ permalink raw reply related
* [PATCH net-next 0/8] Bug fixes in ena ethernet driver
From: netanel @ 2017-06-09 6:55 UTC (permalink / raw)
To: davem, netdev
Cc: Netanel Belgazal, dwmw, zorik, matua, saeedb, msw, aliguori,
nafea, evgenys
From: Netanel Belgazal <netanel@amazon.com>
This patchset contains fixes for the bugs that were discovered so far.
Netanel Belgazal (8):
net: ena: fix rare uncompleted admin command false alarm
net: ena: fix bug that might cause hang after consecutive open/close
interface.
net: ena: add missing return when ena_com_get_io_handlers() fails
net: ena: fix race condition between submit and completion admin
command
net: ena: add missing unmap bars on device removal
net: ena: fix theoretical Rx hang on low memory systems
net: ena: disable admin msix while working in polling mode
net: ena: bug fix in lost tx packets detection mechanism
drivers/net/ethernet/amazon/ena/ena_com.c | 35 +++--
drivers/net/ethernet/amazon/ena/ena_ethtool.c | 2 +-
drivers/net/ethernet/amazon/ena/ena_netdev.c | 179 +++++++++++++++++++-------
drivers/net/ethernet/amazon/ena/ena_netdev.h | 16 ++-
4 files changed, 168 insertions(+), 64 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH v2 7/8] net: mvmdio: add xmdio support
From: Antoine Tenart @ 2017-06-09 6:40 UTC (permalink / raw)
To: Andrew Lunn
Cc: Antoine Tenart, davem, jason, gregory.clement,
sebastian.hesselbarth, f.fainelli, thomas.petazzoni, mw, linux,
netdev, linux-arm-kernel
In-Reply-To: <20170608160331.GA20216@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 756 bytes --]
Hi Andrew,
On Thu, Jun 08, 2017 at 06:03:31PM +0200, Andrew Lunn wrote:
> On Thu, Jun 08, 2017 at 11:26:52AM +0200, Antoine Tenart wrote:
> > +#define MVMDIO_XSMI_MGNT_REG 0x0
> > +#define MVMDIO_XSMI_READ_VALID BIT(29)
> > +#define MVMDIO_XSMI_BUSY BIT(30)
> > +#define MVMDIO_XSMI_ADDR_REG 0x8
> > +#define MVMDIO_XSMI_PHYADDR_SHIFT 16
> > +#define MVMDIO_XSMI_DEVADDR_SHIFT 21
> > +#define MVMDIO_XSMI_READ_OPERATION (0x7 << 26)
> > +#define MVMDIO_XSMI_WRITE_OPERATION (0x5 << 27)
>
> These two operations seem odd. Generally ops have the same shift.
Indeed, this is odd. I'll have a look at this.
Thanks,
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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