* RE: [PATCH net-next] bnx2x: ethtool -x full support
From: Mintz, Yuval @ 2016-10-20 12:27 UTC (permalink / raw)
To: Eric Dumazet, David Miller; +Cc: netdev, Ariel Elior
In-Reply-To: <1476818655.5650.69.camel@edumazet-glaptop3.roam.corp.google.com>
> > From: Eric Dumazet <edumazet@google.com>
> >
> > Implement ethtool -x full support, so that rss key can be fetched
> > instead of assuming it matches /proc/sys/net/core/netdev_rss_key
> > content.
>
> I'll send a V2, tested with CONFIG_BNX2X_SRIOV=y ;)
>
Hi Eric,
Thanks for this. Are you planning on sending v2 or do you want us to take over?
BTW, what's the benefit of changing the RSS keys? Giving the hash properties,
in which scenarios would we gain by switching keys?
Thanks,
Yuval
^ permalink raw reply
* Re: mlx4: panic during shutdown
From: Tariq Toukan @ 2016-10-20 9:07 UTC (permalink / raw)
To: Sebastian Ott, Yishai Hadas; +Cc: netdev, linux-kernel
In-Reply-To: <alpine.LFD.2.20.1610191617400.1730@schleppi>
Hi Sebastian,
Thanks for the report.
We've encountered this as well, and trying to find the correct way of
solving it.
On 19/10/2016 5:35 PM, Sebastian Ott wrote:
> Hi,
>
> After a userspace update (fedora 23->24) I reproducibly run into the
> following oops during shutdown (on s390):
>
> [ 71.054832] Unable to handle kernel pointer dereference in virtual kernel address space
> [ 71.054835] Failing address: 6b6b6b6b6b6b6000 TEID: 6b6b6b6b6b6b6803
> [ 71.054838] Fault in home space mode while using kernel ASCE.
> [ 71.054847] AS:0000000000f70007 R3:0000000000000024
> [ 71.054883] Oops: 0038 ilc:3 [#1] PREEMPT SMP
> [ 71.054887] Modules linked in: mlx4_ib ib_core mlx4_en ptp pps_core mlx4_core [...]
> [ 71.054912] CPU: 8 PID: 809 Comm: kworker/8:6 Not tainted 4.8.0-02896-g7137af2-dirty #6
> [ 71.054913] Hardware name: IBM 2964 N96 704 (LPAR)
> [ 71.054919] Workqueue: events linkwatch_event
> [ 71.054921] task: 00000000dbea0008 task.stack: 00000000dbea4000
> [ 71.054923] Krnl PSW : 0704e00180000000 000003ff8007a496 (mlx4_en_get_phys_port_id+0x66/0xb0 [mlx4_en])
> [ 71.054933] R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 RI:0 EA:3
> Krnl GPRS: 0000000000000080 0000000000000268 000000000000004e 00000000001c33e0
> [ 71.054937] 000003ff8007a486 0000000000882790 6b6b6b6b6b6b6b6b 0000000000000010
> [ 71.054939] 00000000dbea7b18 6b6b6b6b6b6b6b6b 00000000dbea7b18 00000000e72e0000
> [ 71.054941] 00000000f15ec900 0000000000000000 000003ff8007a486 00000000dbea79c8
> [ 71.054950] Krnl Code: 000003ff8007a486: e310b81c0d14 lgf %r1,55324(%r11)
> 000003ff8007a48c: a71b004b aghi %r1,75
> #000003ff8007a490: eb110003000d sllg %r1,%r1,3
> >000003ff8007a496: e31190000002 ltg %r1,0(%r1,%r9)
> 000003ff8007a49c: a7840015 brc 8,3ff8007a4c6
> 000003ff8007a4a0: 9208a020 mvi 32(%r10),8
> 000003ff8007a4a4: 4130a007 la %r3,7(%r10)
> 000003ff8007a4a8: a7290008 lghi %r2,8
> [ 71.054965] Call Trace:
> [ 71.054969] ([<000003ff8007a486>] mlx4_en_get_phys_port_id+0x56/0xb0 [mlx4_en])
> [ 71.054971] ([<0000000000760b94>] rtnl_fill_ifinfo+0x4ec/0xc90)
> [ 71.054974] ([<0000000000764fae>] rtmsg_ifinfo_build_skb+0x96/0xe8)
> [ 71.054976] ([<0000000000765038>] rtmsg_ifinfo+0x38/0x78)
> [ 71.054978] ([<000000000074150e>] netdev_state_change+0x5e/0x70)
> [ 71.054981] ([<0000000000765ca6>] linkwatch_do_dev+0x66/0xc8)
> [ 71.054983] ([<0000000000765fd6>] __linkwatch_run_queue+0x13e/0x190)
> [ 71.054985] ([<0000000000766070>] linkwatch_event+0x48/0x58)
> [ 71.054988] ([<0000000000162a2e>] process_one_work+0x3fe/0x820)
> [ 71.054990] ([<00000000001630e6>] worker_thread+0x296/0x460)
> [ 71.054992] ([<000000000016b41a>] kthread+0x112/0x120)
> [ 71.054996] ([<00000000008762b2>] kernel_thread_starter+0x6/0xc)
> [ 71.054998] ([<00000000008762ac>] kernel_thread_starter+0x0/0xc)
> [ 71.055000] INFO: lockdep is turned off.
> [ 71.055001] Last Breaking-Event-Address:
> [ 71.055004] [<0000000000294480>] printk+0xc8/0xd0
> [ 71.055006]
> [ 71.055008] Kernel panic - not syncing: Fatal exception: panic_on_oops
>
>
> This was observed with 4.8 but it's also reproducible on 4.9-rc1.
> In mlx4_en_get_phys_port_id (which looks like it's called from userspace
> via sysfs) the data behind mlx4_en_priv->mdev is already freed.
>
> The problem probably is that the lifetime of mlx4_en_priv->mdev seems to
> be shorter than that of struct net_device (and mlx4_en_get_phys_port_id
> can be called as long as struct net_device exists).
Right. This happens because we've already freed some resources.
One possible solution is to add a check of netif_device_present in
dev_get_phys_port_id.
Something like this:
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6601,6 +6601,8 @@ int dev_get_phys_port_id(struct net_device *dev,
if (!ops->ndo_get_phys_port_id)
return -EOPNOTSUPP;
+ if (!netif_device_present(dev))
+ return -ENODEV;
return ops->ndo_get_phys_port_id(dev, ppid);
}
EXPORT_SYMBOL(dev_get_phys_port_id);
However, this causes other issues when combining with MTU change.
In MTU change, netif_device_present returns false for a while, causing
an unexpected failure of dev_get_phys_port_id.
>
> Regards,
> Sebastian
>
Regards,
Tariq Toukan
^ permalink raw reply
* [PATCH net-next] net: phy: aquantia: add PHY ID of AQR106 and AQR107
From: shh.xie @ 2016-10-20 8:30 UTC (permalink / raw)
To: netdev, davem; +Cc: f.fainelli, Shaohui Xie
From: Shaohui Xie <Shaohui.Xie@nxp.com>
The AQR106 and AQR107 can use the existing driver.
Signed-off-by: Shaohui Xie <Shaohui.Xie@nxp.com>
---
drivers/net/phy/aquantia.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index 09b0b0a..e8ae50e 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c
@@ -21,6 +21,8 @@
#define PHY_ID_AQ1202 0x03a1b445
#define PHY_ID_AQ2104 0x03a1b460
#define PHY_ID_AQR105 0x03a1b4a2
+#define PHY_ID_AQR106 0x03a1b4d0
+#define PHY_ID_AQR107 0x03a1b4e0
#define PHY_ID_AQR405 0x03a1b4b0
#define PHY_AQUANTIA_FEATURES (SUPPORTED_10000baseT_Full | \
@@ -154,6 +156,30 @@ static struct phy_driver aquantia_driver[] = {
.read_status = aquantia_read_status,
},
{
+ .phy_id = PHY_ID_AQR106,
+ .phy_id_mask = 0xfffffff0,
+ .name = "Aquantia AQR106",
+ .features = PHY_AQUANTIA_FEATURES,
+ .flags = PHY_HAS_INTERRUPT,
+ .aneg_done = aquantia_aneg_done,
+ .config_aneg = aquantia_config_aneg,
+ .config_intr = aquantia_config_intr,
+ .ack_interrupt = aquantia_ack_interrupt,
+ .read_status = aquantia_read_status,
+},
+{
+ .phy_id = PHY_ID_AQR107,
+ .phy_id_mask = 0xfffffff0,
+ .name = "Aquantia AQR107",
+ .features = PHY_AQUANTIA_FEATURES,
+ .flags = PHY_HAS_INTERRUPT,
+ .aneg_done = aquantia_aneg_done,
+ .config_aneg = aquantia_config_aneg,
+ .config_intr = aquantia_config_intr,
+ .ack_interrupt = aquantia_ack_interrupt,
+ .read_status = aquantia_read_status,
+},
+{
.phy_id = PHY_ID_AQR405,
.phy_id_mask = 0xfffffff0,
.name = "Aquantia AQR405",
@@ -173,6 +199,8 @@ static struct mdio_device_id __maybe_unused aquantia_tbl[] = {
{ PHY_ID_AQ1202, 0xfffffff0 },
{ PHY_ID_AQ2104, 0xfffffff0 },
{ PHY_ID_AQR105, 0xfffffff0 },
+ { PHY_ID_AQR106, 0xfffffff0 },
+ { PHY_ID_AQR107, 0xfffffff0 },
{ PHY_ID_AQR405, 0xfffffff0 },
{ }
};
--
2.1.0.27.g96db324
^ permalink raw reply related
* Re: [Patch net v2] ipv6: fix a potential deadlock in do_ipv6_setsockopt()
From: Marcelo Ricardo Leitner @ 2016-10-20 11:12 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, eric.dumazet, sploving1
In-Reply-To: <1476945312-15572-1-git-send-email-xiyou.wangcong@gmail.com>
On Wed, Oct 19, 2016 at 11:35:12PM -0700, Cong Wang wrote:
> Baozeng reported this deadlock case:
>
> CPU0 CPU1
> ---- ----
> lock([ 165.136033] sk_lock-AF_INET6);
> lock([ 165.136033] rtnl_mutex);
> lock([ 165.136033] sk_lock-AF_INET6);
> lock([ 165.136033] rtnl_mutex);
>
> Similar to commit 87e9f0315952
> ("ipv4: fix a potential deadlock in mcast getsockopt() path")
> this is due to we still have a case, ipv6_sock_mc_close(),
> where we acquire sk_lock before rtnl_lock. Close this deadlock
> with the similar solution, that is always acquire rtnl lock first.
>
> Fixes: baf606d9c9b1 ("ipv4,ipv6: grab rtnl before locking the socket")
> Reported-by: Baozeng Ding <sploving1@gmail.com>
> Tested-by: Baozeng Ding <sploving1@gmail.com>
> Cc: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
> include/net/addrconf.h | 1 +
> net/ipv6/ipv6_sockglue.c | 3 ++-
> net/ipv6/mcast.c | 17 ++++++++++++-----
> 3 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
> index f2d0727..8f998af 100644
> --- a/include/net/addrconf.h
> +++ b/include/net/addrconf.h
> @@ -174,6 +174,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex,
> const struct in6_addr *addr);
> int ipv6_sock_mc_drop(struct sock *sk, int ifindex,
> const struct in6_addr *addr);
> +void __ipv6_sock_mc_close(struct sock *sk);
> void ipv6_sock_mc_close(struct sock *sk);
> bool inet6_mc_check(struct sock *sk, const struct in6_addr *mc_addr,
> const struct in6_addr *src_addr);
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 5330262..636ec56 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -120,6 +120,7 @@ struct ipv6_txoptions *ipv6_update_options(struct sock *sk,
> static bool setsockopt_needs_rtnl(int optname)
> {
> switch (optname) {
> + case IPV6_ADDRFORM:
> case IPV6_ADD_MEMBERSHIP:
> case IPV6_DROP_MEMBERSHIP:
> case IPV6_JOIN_ANYCAST:
> @@ -198,7 +199,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
> }
>
> fl6_free_socklist(sk);
> - ipv6_sock_mc_close(sk);
> + __ipv6_sock_mc_close(sk);
Considering we already took rtnl lock and the way __ipv6_sock_mc_close()
is written, we don't need that check
if (!rcu_access_pointer(np->ipv6_mc_list))
here too as the while() in there does it already.
LGTM, thanks
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>
> /*
> * Sock is moving from IPv6 to IPv4 (sk_prot), so
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index 75c1fc5..14a3903 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -276,16 +276,14 @@ static struct inet6_dev *ip6_mc_find_dev_rcu(struct net *net,
> return idev;
> }
>
> -void ipv6_sock_mc_close(struct sock *sk)
> +void __ipv6_sock_mc_close(struct sock *sk)
> {
> struct ipv6_pinfo *np = inet6_sk(sk);
> struct ipv6_mc_socklist *mc_lst;
> struct net *net = sock_net(sk);
>
> - if (!rcu_access_pointer(np->ipv6_mc_list))
> - return;
> + ASSERT_RTNL();
>
> - rtnl_lock();
> while ((mc_lst = rtnl_dereference(np->ipv6_mc_list)) != NULL) {
> struct net_device *dev;
>
> @@ -303,8 +301,17 @@ void ipv6_sock_mc_close(struct sock *sk)
>
> atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc);
> kfree_rcu(mc_lst, rcu);
> -
> }
> +}
> +
> +void ipv6_sock_mc_close(struct sock *sk)
> +{
> + struct ipv6_pinfo *np = inet6_sk(sk);
> +
> + if (!rcu_access_pointer(np->ipv6_mc_list))
> + return;
> + rtnl_lock();
> + __ipv6_sock_mc_close(sk);
> rtnl_unlock();
> }
>
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH] ipv6: properly prevent temp_prefered_lft sysctl race
From: Jiri Bohac @ 2016-10-20 10:29 UTC (permalink / raw)
To: David Miller
Cc: julia.lawall, kuznet, jmorris, yoshfuji, kaber, netdev,
kbuild-all
In-Reply-To: <20161019.145816.259194714319437266.davem@davemloft.net>
The check for an underflow of tmp_prefered_lft is always false
because tmp_prefered_lft is unsigned. The intention of the check
was to guard against racing with an update of the
temp_prefered_lft sysctl, potentially resulting in an underflow.
As suggested by David Miller, the best way to prevent the race is
by reading the sysctl variable using READ_ONCE.
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index cc7c26d..060dd99 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1185,6 +1185,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
u32 addr_flags;
unsigned long now = jiffies;
long max_desync_factor;
+ s32 cnf_temp_preferred_lft;
write_lock_bh(&idev->lock);
if (ift) {
@@ -1228,9 +1229,10 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
/* recalculate max_desync_factor each time and update
* idev->desync_factor if it's larger
*/
+ cnf_temp_preferred_lft = READ_ONCE(idev->cnf.temp_prefered_lft);
max_desync_factor = min_t(__u32,
idev->cnf.max_desync_factor,
- idev->cnf.temp_prefered_lft - regen_advance);
+ cnf_temp_preferred_lft - regen_advance);
if (unlikely(idev->desync_factor > max_desync_factor)) {
if (max_desync_factor > 0) {
@@ -1245,11 +1247,8 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
tmp_valid_lft = min_t(__u32,
ifp->valid_lft,
idev->cnf.temp_valid_lft + age);
- tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
+ tmp_prefered_lft = cnf_temp_preferred_lft + age -
idev->desync_factor;
- /* guard against underflow in case of concurrent updates to cnf */
- if (unlikely(tmp_prefered_lft < 0))
- tmp_prefered_lft = 0;
tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
tmp_plen = ifp->prefix_len;
tmp_tstamp = ifp->tstamp;
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
* Re: [PATCH net-next v4 2/3] udp: implement memory accounting helpers
From: Paolo Abeni @ 2016-10-20 10:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller, James Morris,
Trond Myklebust, Alexander Duyck, Daniel Borkmann, Eric Dumazet,
Tom Herbert, Hannes Frederic Sowa, Edward Cree,
linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1476938622.5650.111.camel-XN9IlZ5yJG9HTL0Zs8A6p+yfmBU6pStAUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Wed, 2016-10-19 at 21:43 -0700, Eric Dumazet wrote:
> On Wed, 2016-10-19 at 14:47 +0200, Paolo Abeni wrote:
> > Avoid using the generic helpers.
> > Adds a new spinlock_t to the udp_sock structure and use
> > it to protect the memory accounting operation, both on
> > enqueue and on dequeue.
> >
> > On dequeue perform partial memory reclaiming, trying to
> > leave a quantum of forward allocated memory.
> >
> > On enqueue use a custom helper, to allow some optimizations:
> > - use a plain spin_lock() variant instead of the slightly
> > costly spin_lock_irqsave(),
> > - avoid dst_force check, since the calling code has already
> > dropped the skb dst
> >
> > The above needs custom memory reclaiming on shutdown, provided
> > by the udp_destruct_sock().
> >
> > v3 -> v4:
> > - reworked memory accunting, simplifying the schema
> > - provide an helper for both memory scheduling and enqueuing
> >
> > v1 -> v2:
> > - use a udp specific destrctor to perform memory reclaiming
> > - remove a couple of helpers, unneeded after the above cleanup
> > - do not reclaim memory on dequeue if not under memory
> > pressure
> > - reworked the fwd accounting schema to avoid potential
> > integer overflow
> >
> > Acked-by: Hannes Frederic Sowa <hannes-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r@public.gmane.org>
> > Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > include/linux/udp.h | 1 +
> > include/net/udp.h | 4 ++
> > net/ipv4/udp.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 116 insertions(+)
> >
> > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > index d1fd8cd..8ea7f8b 100644
> > --- a/include/linux/udp.h
> > +++ b/include/linux/udp.h
> > @@ -66,6 +66,7 @@ struct udp_sock {
> > #define UDPLITE_RECV_CC 0x4 /* set via udplite setsocktopt */
> > __u8 pcflag; /* marks socket as UDP-Lite if > 0 */
> > __u8 unused[3];
> > + spinlock_t mem_lock; /* protects memory accounting */
> > /*
> > * For encapsulation sockets.
> > */
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > index ea53a87..18f1e6b 100644
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
> > @@ -246,6 +246,9 @@ static inline __be16 udp_flow_src_port(struct net *net, struct sk_buff *skb,
> > }
> >
> > /* net/ipv4/udp.c */
> > +void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len);
> > +int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb);
> > +
> > void udp_v4_early_demux(struct sk_buff *skb);
> > int udp_get_port(struct sock *sk, unsigned short snum,
> > int (*saddr_cmp)(const struct sock *,
> > @@ -258,6 +261,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
> > void udp4_hwcsum(struct sk_buff *skb, __be32 src, __be32 dst);
> > int udp_rcv(struct sk_buff *skb);
> > int udp_ioctl(struct sock *sk, int cmd, unsigned long arg);
> > +int udp_init_sock(struct sock *sk);
> > int udp_disconnect(struct sock *sk, int flags);
> > unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait);
> > struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 7d96dc2..7047bb3 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1172,6 +1172,117 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
> > return ret;
> > }
> >
> > +static void udp_rmem_release(struct sock *sk, int size, int partial)
> > +{
> > + struct udp_sock *up = udp_sk(sk);
> > + int amt;
> > +
> > + atomic_sub(size, &sk->sk_rmem_alloc);
> > +
> > + spin_lock_bh(&up->mem_lock);
> > + sk->sk_forward_alloc += size;
> > + amt = (sk->sk_forward_alloc - partial) & ~(SK_MEM_QUANTUM - 1);
> > + sk->sk_forward_alloc -= amt;
> > + spin_unlock_bh(&up->mem_lock);
> > +
> > + if (amt)
> > + __sk_mem_reduce_allocated(sk, amt >> SK_MEM_QUANTUM_SHIFT);
> > +}
> > +
> > +static void udp_rmem_free(struct sk_buff *skb)
> > +{
> > + udp_rmem_release(skb->sk, skb->truesize, 1);
> > +}
> > +
> > +int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
> > +{
> > + struct sk_buff_head *list = &sk->sk_receive_queue;
> > + int rmem, delta, amt, err = -ENOMEM;
> > + struct udp_sock *up = udp_sk(sk);
> > + int size = skb->truesize;
> > +
> > + /* try to avoid the costly atomic add/sub pair when the receive
> > + * queue is full
> > + */
> > + if (atomic_read(&sk->sk_rmem_alloc) + size > sk->sk_rcvbuf)
> > + goto drop;
> > +
> > + rmem = atomic_add_return(size, &sk->sk_rmem_alloc);
> > + if (rmem > sk->sk_rcvbuf)
> > + goto uncharge_drop;
>
> This is dangerous : We used to accept one packet in receive queue, even
> if sk_rcvbuf is too small. (Check __sock_queue_rcv_skb())
>
> With your code we might drop all packets even if receive queue is empty,
> this might add regressions for some apps.
>
> > +
> > + spin_lock(&up->mem_lock);
> > + if (size < sk->sk_forward_alloc)
> > + goto unlock;
>
> Strange label name, since you do "sk->sk_forward_alloc -= size;"before
> unlock.
>
> You could avoid the goto by :
>
> if (size >= sk->sk_forward_alloc) {
> amt = sk_mem_pages(size);
> ...
> }
> sk->sk_forward_alloc += delta;k_mem_schedule(
>
> > +
> > + amt = sk_mem_pages(size);
> > + delta = amt << SK_MEM_QUANTUM_SHIFT;
> > + if (!__sk_mem_raise_allocated(sk, delta, amt, SK_MEM_RECV)) {
> > + err = -ENOBUFS;
> > + spin_unlock(&up->mem_lock);
> > + goto uncharge_drop;
> > + }
> > +
> > + sk->sk_forward_alloc += delta;
> > +
> > +unlock:
> > + sk->sk_forward_alloc -= size;
> > + spin_unlock(&up->mem_lock);
> > +
> > + /* the skb owner in now the udp socket */
> > + skb_orphan(skb);
> > + skb->sk = sk;
> > + skb->destructor = udp_rmem_free;
>
> > +
> > + skb->dev = NULL;
> > + sock_skb_set_dropcount(sk, skb);
> > +
> > + spin_lock(&list->lock);
>
> Since we acquire this lock anyway to store the packet,
> why not protecting sk_forward_alloc as well with this spinlock,
> and get rid of up->mem_lock.
>
> Ideally a single spin_lock() should be enough.
I thought that would hit us back, since udp_recvmsg() needs to acquire
it twice and we are increasing the bh lock scope, but a quick test
showed otherwise!!!
> Then almost all other atomic ops could be transformed to non atomic ops.
I'm not sure which atomic operations you refer to ?!? AFAICS we still
need them to manipulate udp_memory_allocated, sk_rmem_alloc, and
sk_drops. Can you please hint which ones ?
Thank you,
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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: saving irq context for peernet2id()
From: Nicolas Dichtel @ 2016-10-20 10:17 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: sds, e, paul
In-Reply-To: <1476946352-15770-1-git-send-email-xiyou.wangcong@gmail.com>
Le 20/10/2016 à 08:52, Cong Wang a écrit :
> A kernel warning inside __local_bh_enable_ip() was reported by people
> running SELinux, this is caused due to some SELinux functions
> (indirectly) call peernet2id() with IRQ disabled in process context,
> when we re-enable BH with IRQ disabled kernel complains. Shut up this
> warning by saving IRQ context in peernet2id(), BH is still implicitly
> disabled.
Franckly, reverting the original patch seems better for me. The intention for
that patch was "it's not needed", see the commit log:
"We never read or change netns id in hardirq context,
the only place we read netns id in softirq context
is in vxlan_xmit(). So, it should be enough to just
disable BH."
Now, we see that "it's needed" and that the analysis was wrong. If a race is
introduced by this patch, it will be hard to detect and fix it.
Regards,
Nicolas
^ permalink raw reply
* Re: [PATCH 3/4] ptp_clock: allow for it to be optional
From: Thomas Gleixner @ 2016-10-20 9:50 UTC (permalink / raw)
To: Nicolas Pitre
Cc: John Stultz, Richard Cochran, Yann E MORIN, Josh Triplett, netdev,
linux-kbuild, linux-kernel
In-Reply-To: <1476920573-14384-4-git-send-email-nicolas.pitre@linaro.org>
On Wed, 19 Oct 2016, Nicolas Pitre wrote:
> The pch_gbe driver is a bit special as it relies on extra code in
> drivers/ptp/ptp_pch.c. Therefore we let the make process descend into
> drivers/ptp/ even if PTP_1588_CLOCK is unselected.
The above paragraph looks like a leftover of the previous patch set.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 0/4] make POSIX timers optional with some Kconfig help
From: Thomas Gleixner @ 2016-10-20 9:46 UTC (permalink / raw)
To: Nicolas Pitre
Cc: John Stultz, Richard Cochran, Yann E MORIN, Josh Triplett, netdev,
linux-kbuild, linux-kernel
In-Reply-To: <1476920573-14384-1-git-send-email-nicolas.pitre@linaro.org>
On Wed, 19 Oct 2016, Nicolas Pitre wrote:
> Therefore this series also includes kconfig changes to implement a new
> keyword to express some reverse dependencies like "select" does, named
> "imply", and still allowing for the target config symbol to be disabled
> if the user or a direct dependency says so.
That's really nice work! Thanks for doing that. It makes the whole thing
more palatable.
Thanks,
tglx
^ permalink raw reply
* Re: [RFC 6/6] qedi: Add support for data path.
From: Rangankar, Manish @ 2016-10-20 9:24 UTC (permalink / raw)
To: Hannes Reinecke, lduncan@suse.com, cleech@redhat.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <fc58b63a-ac67-59e4-7698-4ab742848080@suse.de>
On 19/10/16 3:54 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> This patch adds support for data path and TMF handling.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> drivers/scsi/qedi/qedi_fw.c | 1282
>>++++++++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_gbl.h | 6 +
>> drivers/scsi/qedi/qedi_iscsi.c | 6 +
>> drivers/scsi/qedi/qedi_main.c | 4 +
>> 4 files changed, 1298 insertions(+)
>>
>> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
>> index a820785..af1e14d 100644
>> --- a/drivers/scsi/qedi/qedi_fw.c
>> +++ b/drivers/scsi/qedi/qedi_fw.c
>> @@ -147,6 +147,114 @@ static void qedi_process_text_resp(struct
>>qedi_ctx *qedi,
>> spin_unlock(&session->back_lock);
>> }
--snipped--
>> +void qedi_trace_io(struct qedi_ctx *qedi, struct iscsi_task *task,
>> + u16 tid, int8_t direction)
>> +{
>> + struct qedi_io_log *io_log;
>> + struct iscsi_conn *conn = task->conn;
>> + struct qedi_conn *qedi_conn = conn->dd_data;
>> + struct scsi_cmnd *sc_cmd = task->sc;
>> + unsigned long flags;
>> + u8 op;
>> +
>> + spin_lock_irqsave(&qedi->io_trace_lock, flags);
>> +
>> + io_log = &qedi->io_trace_buf[qedi->io_trace_idx];
>> + io_log->direction = direction;
>> + io_log->task_id = tid;
>> + io_log->cid = qedi_conn->iscsi_conn_id;
>> + io_log->lun = sc_cmd->device->lun;
>> + io_log->op = sc_cmd->cmnd[0];
>> + op = sc_cmd->cmnd[0];
>> +
>> + if (op == READ_10 || op == WRITE_10) {
>> + io_log->lba[0] = sc_cmd->cmnd[2];
>> + io_log->lba[1] = sc_cmd->cmnd[3];
>> + io_log->lba[2] = sc_cmd->cmnd[4];
>> + io_log->lba[3] = sc_cmd->cmnd[5];
>> + } else {
>> + io_log->lba[0] = 0;
>> + io_log->lba[1] = 0;
>> + io_log->lba[2] = 0;
>> + io_log->lba[3] = 0;
>> + }
>Only for READ_10 and WRITE_10? What about the other read or write
>commands?
We will add support for other scsi commands in the next revision.
>
>> + io_log->bufflen = scsi_bufflen(sc_cmd);
>> + io_log->sg_count = scsi_sg_count(sc_cmd);
>> + io_log->fast_sgs = qedi->fast_sgls;
>> + io_log->cached_sgs = qedi->cached_sgls;
>> + io_log->slow_sgs = qedi->slow_sgls;
>> + io_log->cached_sge = qedi->use_cached_sge;
>> + io_log->slow_sge = qedi->use_slow_sge;
>> + io_log->fast_sge = qedi->use_fast_sge;
>> + io_log->result = sc_cmd->result;
>> + io_log->jiffies = jiffies;
>> + io_log->blk_req_cpu = smp_processor_id();
>> +
>> + if (direction == QEDI_IO_TRACE_REQ) {
>> + /* For requests we only care about the submission CPU */
>> + io_log->req_cpu = smp_processor_id() % qedi->num_queues;
>> + io_log->intr_cpu = 0;
>> + io_log->blk_rsp_cpu = 0;
>> + } else if (direction == QEDI_IO_TRACE_RSP) {
>> + io_log->req_cpu = smp_processor_id() % qedi->num_queues;
>> + io_log->intr_cpu = qedi->intr_cpu;
>> + io_log->blk_rsp_cpu = smp_processor_id();
>> + }
>> +
>> + qedi->io_trace_idx++;
>> + if (qedi->io_trace_idx == QEDI_IO_TRACE_SIZE)
>> + qedi->io_trace_idx = 0;
>> +
>> + qedi->use_cached_sge = false;
>> + qedi->use_slow_sge = false;
>> + qedi->use_fast_sge = false;
>> +
>> + spin_unlock_irqrestore(&qedi->io_trace_lock, flags);
>> +}
>> +
>> +int qedi_iscsi_send_ioreq(struct iscsi_task *task)
>> +{
>> + struct iscsi_conn *conn = task->conn;
>> + struct iscsi_session *session = conn->session;
>> + struct Scsi_Host *shost =
>>iscsi_session_to_shost(session->cls_session);
>> + struct qedi_ctx *qedi = iscsi_host_priv(shost);
>> + struct qedi_conn *qedi_conn = conn->dd_data;
>> + struct qedi_cmd *cmd = task->dd_data;
>> + struct scsi_cmnd *sc = task->sc;
>> + struct iscsi_task_context *fw_task_ctx;
>> + struct iscsi_cached_sge_ctx *cached_sge;
>> + struct iscsi_phys_sgl_ctx *phys_sgl;
>> + struct iscsi_virt_sgl_ctx *virt_sgl;
>> + struct ystorm_iscsi_task_st_ctx *yst_cxt;
>> + struct mstorm_iscsi_task_st_ctx *mst_cxt;
>> + struct iscsi_sgl *sgl_struct;
>> + struct iscsi_sge *single_sge;
>> + struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)task->hdr;
>> + struct iscsi_sge *bd = cmd->io_tbl.sge_tbl;
>> + enum iscsi_task_type task_type;
>> + struct iscsi_cmd_hdr *fw_cmd;
>> + u32 scsi_lun[2];
>> + u16 cq_idx = smp_processor_id() % qedi->num_queues;
>> + s16 ptu_invalidate = 0;
>> + s16 tid = 0;
>> + u8 num_fast_sgs;
>> +
>> + tid = qedi_get_task_idx(qedi);
>> + if (tid == -1)
>> + return -ENOMEM;
>> +
>> + qedi_iscsi_map_sg_list(cmd);
>> +
>> + int_to_scsilun(sc->device->lun, (struct scsi_lun *)scsi_lun);
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>> +
>> + memset(fw_task_ctx, 0, sizeof(struct iscsi_task_context));
>> + cmd->task_id = tid;
>> +
>> + /* Ystrom context */
>Ystrom or Ystorm?
Noted
>
>> + fw_cmd = &fw_task_ctx->ystorm_st_context.pdu_hdr.cmd;
>> + SET_FIELD(fw_cmd->flags_attr, ISCSI_CMD_HDR_ATTR, ISCSI_ATTR_SIMPLE);
>> +
>> + if (sc->sc_data_direction == DMA_TO_DEVICE) {
>> + if (conn->session->initial_r2t_en) {
>> + fw_task_ctx->ustorm_ag_context.exp_data_acked =
>> + min((conn->session->imm_data_en *
>> + conn->max_xmit_dlength),
>> + conn->session->first_burst);
>> + fw_task_ctx->ustorm_ag_context.exp_data_acked =
>> + min(fw_task_ctx->ustorm_ag_context.exp_data_acked,
>> + scsi_bufflen(sc));
>> + } else {
>> + fw_task_ctx->ustorm_ag_context.exp_data_acked =
>> + min(conn->session->first_burst, scsi_bufflen(sc));
>> + }
>> +
>> + SET_FIELD(fw_cmd->flags_attr, ISCSI_CMD_HDR_WRITE, 1);
>> + task_type = ISCSI_TASK_TYPE_INITIATOR_WRITE;
>> + } else {
>> + if (scsi_bufflen(sc))
>> + SET_FIELD(fw_cmd->flags_attr, ISCSI_CMD_HDR_READ, 1);
>> + task_type = ISCSI_TASK_TYPE_INITIATOR_READ;
>> + }
>> +
>> + fw_cmd->lun.lo = be32_to_cpu(scsi_lun[0]);
>> + fw_cmd->lun.hi = be32_to_cpu(scsi_lun[1]);
>> +
>> + qedi_update_itt_map(qedi, tid, task->itt);
>> + fw_cmd->itt = qedi_set_itt(tid, get_itt(task->itt));
>> + fw_cmd->expected_transfer_length = scsi_bufflen(sc);
>> + fw_cmd->cmd_sn = be32_to_cpu(hdr->cmdsn);
>> + fw_cmd->opcode = hdr->opcode;
>> + qedi_cpy_scsi_cdb(sc, (u32 *)fw_cmd->cdb);
>> +
>> + /* Mstorm context */
>> + fw_task_ctx->mstorm_st_context.sense_db.lo =
>>(u32)cmd->sense_buffer_dma;
>> + fw_task_ctx->mstorm_st_context.sense_db.hi =
>> + (u32)((u64)cmd->sense_buffer_dma >> 32);
>> + fw_task_ctx->mstorm_ag_context.task_cid = qedi_conn->iscsi_conn_id;
>> + fw_task_ctx->mstorm_st_context.task_type = task_type;
>> +
>> + if (qedi->tid_reuse_count[tid] == QEDI_MAX_TASK_NUM) {
>> + ptu_invalidate = 1;
>> + qedi->tid_reuse_count[tid] = 0;
>> + }
>> + fw_task_ctx->ystorm_st_context.state.reuse_count =
>> + qedi->tid_reuse_count[tid];
>> + fw_task_ctx->mstorm_st_context.reuse_count =
>> + qedi->tid_reuse_count[tid]++;
>> +
>> + /* Ustrorm context */
>Ustrorm?
Noted
Thanks,
Manish R.
^ permalink raw reply
* Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
From: Ying Xue @ 2016-10-20 9:30 UTC (permalink / raw)
To: Ben Hutchings, Jon Maloy; +Cc: netdev, Qian Zhang, Eric Dumazet
In-Reply-To: <20161019021654.GD2773@decadent.org.uk>
On 10/19/2016 10:16 AM, Ben Hutchings wrote:
> Qian Zhang (张谦) reported a potential socket buffer overflow in
> tipc_msg_build(). The minimum fragment length needs to be checked
> against the maximum packet size, which is based on the link MTU.
>
> Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This is untested, but I think it fixes the issue reported. Ideally
> tipc_l2_device_event() would also disable use of TIPC on devices with
> too small an MTU, like several other protocols do.
>
Yes, I think so. I will create a patch to disable TIPC sending process
when MTU size is too small.
> Ben.
>
> net/tipc/msg.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 17201aa8423d..b9124ac82c29 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
> goto error;
> }
>
> + /* Check that fragment and message header will fit */
> + if (INT_H_SIZE + mhsz > pktmax)
> + return -EMSGSIZE;
The "mhsz" represents the size of tipc packet header for current socket,
INT_H_SIZE indicates the size of tipc internal message header. So it
seems unreasonable to identify whether the sum of both header sizes is
bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to
compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should
return EMSGSIZE error code.
> +
> /* Prepare reusable fragment header */
> tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER,
> FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr));
>
^ permalink raw reply
* Re: [PATCH (net.git)] stmmac: display the descriptors if DES0 = 0
From: Alexandre Torgue @ 2016-10-20 9:29 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev
In-Reply-To: <1476950488-24923-1-git-send-email-peppe.cavallaro@st.com>
Hi Peppe,
On 10/20/2016 10:01 AM, Giuseppe Cavallaro wrote:
> It makes sense to display the descriptors even if
> DES0 is zero. This helps for example in case of it
> is needed to dump rx write-back descriptors to get
> timestamp status.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre TORGUE <alexandre.torgue@st.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 4ec7397..a1b17cd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -347,10 +347,9 @@ static void dwmac4_display_ring(void *head, unsigned int size, bool rx)
> pr_info("%s descriptor ring:\n", rx ? "RX" : "TX");
>
> for (i = 0; i < size; i++) {
> - if (p->des0)
> - pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
> - i, (unsigned int)virt_to_phys(p),
> - p->des0, p->des1, p->des2, p->des3);
> + pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
> + i, (unsigned int)virt_to_phys(p),
> + p->des0, p->des1, p->des2, p->des3);
> p++;
> }
> }
>
I agree. Acked-by: Alexandre Torgue <alexandre.torgue@st.com>
Thanks
Alex
^ permalink raw reply
* Re: [PATCH] bnx2x: Replace semaphore stats_lock with mutex
From: Arnd Bergmann @ 2016-10-20 9:16 UTC (permalink / raw)
To: Binoy Jayan; +Cc: Ariel Elior, netdev, linux-kernel
In-Reply-To: <1476953532-2019-1-git-send-email-binoy.jayan@linaro.org>
On Thursday, October 20, 2016 2:22:12 PM CEST Binoy Jayan wrote:
> @@ -1976,8 +1973,8 @@ int bnx2x_stats_safe_exec(struct bnx2x *bp,
> /* Wait for statistics to end [while blocking further requests],
> * then run supplied function 'safely'.
> */
> - rc = down_timeout(&bp->stats_lock, HZ / 10);
> - if (unlikely(rc)) {
> + rc = mutex_trylock(&bp->stats_lock);
> + if (unlikely(!rc)) {
> BNX2X_ERR("Failed to take statistics lock for safe execution\n");
> goto out_no_lock;
> }
This conversion looks suspicious, your changelog text does not mention
at all why would be to remove the timeout and fail immediately.
In fact, you don't seem to have any mutex_lock() call left, just
mutex_trylock(), and everything that tries to get the mutex fails
immediately if someone else holds it. The existing behavior of
the driver is not much better (it always gives up after 100ms),
but I think this needs some more thought put into it.
Arnd
^ permalink raw reply
* Re: [RFC 5/6] qedi: Add support for iSCSI session management.
From: Rangankar, Manish @ 2016-10-20 9:12 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: lduncan@suse.com, cleech@redhat.com, martin.petersen@oracle.com,
jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <20161019132801.eptbf2dpuoilplpd@linux-x5ow.site>
On 19/10/16 6:58 PM, "Johannes Thumshirn" <jthumshirn@suse.de> wrote:
>On Wed, Oct 19, 2016 at 01:01:12AM -0400, manish.rangankar@cavium.com
>wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> This patch adds support for iscsi_transport LLD Login,
>> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
>> and Firmware async event handling support.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>
>[...]
>
>> +void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
>> +{
>> + struct scsi_cmnd *sc = cmd->scsi_cmd;
>> +
>> + if (cmd->io_tbl.sge_valid && sc) {
>> + scsi_dma_unmap(sc);
>> + cmd->io_tbl.sge_valid = 0;
>> + }
>> +}
>
>Maybe set sge_valid to 0 and then call scsi_dma_unmap(). I don't know if
>it's
>really racy but it looks like it is.
>
>[...]
>
>> +static void qedi_process_text_resp(struct qedi_ctx *qedi,
>> + union iscsi_cqe *cqe,
>> + struct iscsi_task *task,
>> + struct qedi_conn *qedi_conn)
>> +{
>> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
>> + struct iscsi_session *session = conn->session;
>> + struct iscsi_task_context *task_ctx;
>> + struct iscsi_text_rsp *resp_hdr_ptr;
>> + struct iscsi_text_response_hdr *cqe_text_response;
>> + struct qedi_cmd *cmd;
>> + int pld_len;
>> + u32 *tmp;
>> +
>> + cmd = (struct qedi_cmd *)task->dd_data;
>> + task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(&qedi->tasks,
>> + cmd->task_id);
>
>No need to cast here, qedi_get_task_mem() returns void *.
>
>[...]
>
>> + cqe_login_response = &cqe->cqe_common.iscsi_hdr.login_response;
>> + task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(&qedi->tasks,
>> + cmd->task_id);
>
>Same here.
>
>[...]
>
>> + }
>> +
>> + pbl = (struct scsi_bd *)qedi->bdq_pbl;
>> + pbl += (qedi->bdq_prod_idx % qedi->rq_num_entries);
>> + pbl->address.hi =
>> + cpu_to_le32((u32)(((u64)(qedi->bdq[idx].buf_dma)) >> 32));
>> + pbl->address.lo =
>> + cpu_to_le32(((u32)(((u64)(qedi->bdq[idx].buf_dma)) &
>> + 0xffffffff)));
>
>Is this LISP or C?
>
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_CONN,
>> + "pbl [0x%p] pbl->address hi [0x%llx] lo [0x%llx] idx [%d]\n",
>> + pbl, pbl->address.hi, pbl->address.lo, idx);
>> + pbl->opaque.hi = cpu_to_le32((u32)(((u64)0) >> 32));
>
>Isn't this plain pbl->opaque.hi = 0; ?
>
>> + pbl->opaque.lo = cpu_to_le32(((u32)(((u64)idx) & 0xffffffff)));
>> +
>
>[...]
>
>> + switch (comp_type) {
>> + case ISCSI_CQE_TYPE_SOLICITED:
>> + case ISCSI_CQE_TYPE_SOLICITED_WITH_SENSE:
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>> + cqe->cqe_solicited.itid);
>
>Again, no cast needed.
>
>[...]
>
>> + writel(*(u32 *)&dbell, qedi_conn->ep->p_doorbell);
>> + /* Make sure fw idx is coherent */
>> + wmb();
>> + mmiowb();
>
>Isn't either wmb() or mmiowb() enough?
>
>[..]
>
>> +
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>
>Cast again.
>
>[...]
>
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>
>^^
>
>[...]
>
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks, tid);
>
>
>[...]
>
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>> +
>
>[...]
>
>> +
>> + qedi = (struct qedi_ctx *)iscsi_host_priv(shost);
>
>Same goes for iscsi_host_priv();
>
>[...]
>
>> + ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
>> + ((qedi_ep->state ==
>> + EP_STATE_OFLDCONN_FAILED) ||
>> + (qedi_ep->state ==
>> + EP_STATE_OFLDCONN_COMPL)),
>> + msecs_to_jiffies(timeout_ms));
>
>Maybe:
>#define QEDI_OLDCON_STATE(q) ((q)->state == EP_STATE_OFLDCONN_FAILED || \
> (q)->state == EP_STATE_OFLDCONN_COMPL)
>
>ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
> QEDI_OLDCON_STATE(qedi_ep),
> msec_to_jiffies(timeout_ms));
>
>But that could be just me hating linewraps.
>
>[...]
We will address all the above review comments in the next revision.
Thanks,
Manish R.
^ permalink raw reply
* Re: [RFC 5/6] qedi: Add support for iSCSI session management.
From: Rangankar, Manish @ 2016-10-20 9:09 UTC (permalink / raw)
To: Hannes Reinecke, lduncan@suse.com, cleech@redhat.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <6f79e92c-ea12-331b-5c20-e9a3587b1d80@suse.de>
On 19/10/16 1:33 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> This patch adds support for iscsi_transport LLD Login,
>> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
>> and Firmware async event handling support.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> drivers/scsi/qedi/qedi_fw.c | 1123 ++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_gbl.h | 67 ++
>> drivers/scsi/qedi/qedi_iscsi.c | 1604
>>++++++++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_iscsi.h | 228 ++++++
>> drivers/scsi/qedi/qedi_main.c | 164 ++++
>> 5 files changed, 3186 insertions(+)
>> create mode 100644 drivers/scsi/qedi/qedi_fw.c
>> create mode 100644 drivers/scsi/qedi/qedi_gbl.h
>> create mode 100644 drivers/scsi/qedi/qedi_iscsi.c
>> create mode 100644 drivers/scsi/qedi/qedi_iscsi.h
>>
--snipped--
>>
>> +static void qedi_process_async_mesg(struct qedi_ctx *qedi,
>> + union iscsi_cqe *cqe,
>> + struct iscsi_task *task,
>> + struct qedi_conn *qedi_conn,
>> + u16 que_idx)
>> +{
>> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
>> + struct iscsi_session *session = conn->session;
>> + struct iscsi_async_msg_hdr *cqe_async_msg;
>> + struct iscsi_async *resp_hdr;
>> + u32 scsi_lun[2];
>> + u32 pdu_len, num_bdqs;
>> + char bdq_data[QEDI_BDQ_BUF_SIZE];
>> + unsigned long flags;
>> +
>> + spin_lock_bh(&session->back_lock);
>> +
>> + cqe_async_msg = &cqe->cqe_common.iscsi_hdr.async_msg;
>> + pdu_len = cqe_async_msg->hdr_second_dword &
>> + ISCSI_ASYNC_MSG_HDR_DATA_SEG_LEN_MASK;
>> + num_bdqs = pdu_len / QEDI_BDQ_BUF_SIZE;
>> +
>> + if (cqe->cqe_common.cqe_type == ISCSI_CQE_TYPE_UNSOLICITED) {
>> + spin_lock_irqsave(&qedi->hba_lock, flags);
>> + qedi_unsol_pdu_adjust_bdq(qedi, &cqe->cqe_unsolicited,
>> + pdu_len, num_bdqs, bdq_data);
>> + spin_unlock_irqrestore(&qedi->hba_lock, flags);
>> + }
>> +
>> + resp_hdr = (struct iscsi_async *)&qedi_conn->gen_pdu.resp_hdr;
>> + memset(resp_hdr, 0, sizeof(struct iscsi_hdr));
>> + resp_hdr->opcode = cqe_async_msg->opcode;
>> + resp_hdr->flags = 0x80;
>> +
>> + scsi_lun[0] = cpu_to_be32(cqe_async_msg->lun.lo);
>> + scsi_lun[1] = cpu_to_be32(cqe_async_msg->lun.hi);
>I _think_ we have a SCSI LUN structure ...
Will do.
--snipped--
>> +void qedi_process_iscsi_error(struct qedi_endpoint *ep, struct
>>async_data *data)
>> +{
>> + struct qedi_conn *qedi_conn;
>> + struct qedi_ctx *qedi;
>> + char warn_notice[] = "iscsi_warning";
>> + char error_notice[] = "iscsi_error";
>> + char *message;
>> + int need_recovery = 0;
>> + u32 err_mask = 0;
>> + char msg[64];
>> +
>> + if (!ep)
>> + return;
>> +
>> + qedi_conn = ep->conn;
>> + if (!qedi_conn)
>> + return;
>> +
>> + qedi = ep->qedi;
>> +
>> + QEDI_ERR(&qedi->dbg_ctx, "async event iscsi error:0x%x\n",
>> + data->error_code);
>> +
>> + if (err_mask) {
>> + need_recovery = 0;
>> + message = warn_notice;
>> + } else {
>> + need_recovery = 1;
>> + message = error_notice;
>> + }
>> +
>> + switch (data->error_code) {
>> + case ISCSI_STATUS_NONE:
>> + strcpy(msg, "tcp_error none");
>> + break;
>> + case ISCSI_CONN_ERROR_TASK_CID_MISMATCH:
>> + strcpy(msg, "task cid mismatch");
>> + break;
>> + case ISCSI_CONN_ERROR_TASK_NOT_VALID:
>> + strcpy(msg, "invalid task");
>> + break;
>> + case ISCSI_CONN_ERROR_RQ_RING_IS_FULL:
>> + strcpy(msg, "rq ring full");
>> + break;
>> + case ISCSI_CONN_ERROR_CMDQ_RING_IS_FULL:
>> + strcpy(msg, "cmdq ring full");
>> + break;
>> + case ISCSI_CONN_ERROR_HQE_CACHING_FAILED:
>> + strcpy(msg, "sge caching failed");
>> + break;
>> + case ISCSI_CONN_ERROR_HEADER_DIGEST_ERROR:
>> + strcpy(msg, "hdr digest error");
>> + break;
>> + case ISCSI_CONN_ERROR_LOCAL_COMPLETION_ERROR:
>> + strcpy(msg, "local cmpl error");
>> + break;
>> + case ISCSI_CONN_ERROR_DATA_OVERRUN:
>> + strcpy(msg, "invalid task");
>> + break;
>> + case ISCSI_CONN_ERROR_OUT_OF_SGES_ERROR:
>> + strcpy(msg, "out of sge error");
>> + break;
>> + case ISCSI_CONN_ERROR_TCP_SEG_PROC_IP_OPTIONS_ERROR:
>> + strcpy(msg, "tcp seg ip options error");
>> + break;
>> + case ISCSI_CONN_ERROR_TCP_IP_FRAGMENT_ERROR:
>> + strcpy(msg, "tcp ip fragment error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_AHS_LEN:
>> + strcpy(msg, "AHS len protocol error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_ITT_OUT_OF_RANGE:
>> + strcpy(msg, "itt out of range error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_SEG_LEN_EXCEEDS_PDU_SIZE:
>> + strcpy(msg, "data seg more than pdu size");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_INVALID_OPCODE:
>> + strcpy(msg, "invalid opcode");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_INVALID_OPCODE_BEFORE_UPDATE:
>> + strcpy(msg, "invalid opcode before update");
>> + break;
>> + case ISCSI_CONN_ERROR_UNVALID_NOPIN_DSL:
>> + strcpy(msg, "unexpected opcode");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_CARRIES_NO_DATA:
>> + strcpy(msg, "r2t carries no data");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_SN:
>> + strcpy(msg, "data sn error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_IN_TTT:
>> + strcpy(msg, "data TTT error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_TTT:
>> + strcpy(msg, "r2t TTT error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_BUFFER_OFFSET:
>> + strcpy(msg, "buffer offset error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_BUFFER_OFFSET_OOO:
>> + strcpy(msg, "buffer offset ooo");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_SN:
>> + strcpy(msg, "data seg len 0");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DESIRED_DATA_TRNS_LEN_0:
>> + strcpy(msg, "data xer len error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DESIRED_DATA_TRNS_LEN_1:
>> + strcpy(msg, "data xer len1 error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DESIRED_DATA_TRNS_LEN_2:
>> + strcpy(msg, "data xer len2 error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_LUN:
>> + strcpy(msg, "protocol lun error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_F_BIT_ZERO:
>> + strcpy(msg, "f bit zero error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_F_BIT_ZERO_S_BIT_ONE:
>> + strcpy(msg, "f bit zero s bit one error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_EXP_STAT_SN:
>> + strcpy(msg, "exp stat sn error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DSL_NOT_ZERO:
>> + strcpy(msg, "dsl not zero error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_INVALID_DSL:
>> + strcpy(msg, "invalid dsl");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_SEG_LEN_TOO_BIG:
>> + strcpy(msg, "data seg len too big");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_OUTSTANDING_R2T_COUNT:
>> + strcpy(msg, "outstanding r2t count error");
>> + break;
>> + case ISCSI_CONN_ERROR_SENSE_DATA_LENGTH:
>> + strcpy(msg, "sense datalen error");
>> + break;
>Please use an array for mapping values onto strings.
Will add this change in next revision.
Thanks,
Manish R.
^ permalink raw reply
* Re: [RFC 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Rangankar, Manish @ 2016-10-20 8:27 UTC (permalink / raw)
To: Hannes Reinecke, lduncan@suse.com, cleech@redhat.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <50c821e3-28dc-78b1-8fa4-527a138f1709@suse.de>
Thanks Hannes for the review, please see my comments below,
On 19/10/16 1:15 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
>> for 41000 Series Converged Network Adapters by QLogic.
>>
>> This patch consists of following changes:
>> - MAINTAINERS Makefile and Kconfig changes for qedi,
>> - PCI driver registration,
>> - iSCSI host level initialization,
>> - Debugfs and log level infrastructure.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/net/ethernet/qlogic/Kconfig | 12 -
>> drivers/scsi/Kconfig | 1 +
>> drivers/scsi/Makefile | 1 +
>> drivers/scsi/qedi/Kconfig | 10 +
>> drivers/scsi/qedi/Makefile | 5 +
>> drivers/scsi/qedi/qedi.h | 286 +++++++
>> drivers/scsi/qedi/qedi_dbg.c | 143 ++++
>> drivers/scsi/qedi/qedi_dbg.h | 144 ++++
>> drivers/scsi/qedi/qedi_debugfs.c | 244 ++++++
>> drivers/scsi/qedi/qedi_hsi.h | 52 ++
>> drivers/scsi/qedi/qedi_main.c | 1550
>>+++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_sysfs.c | 52 ++
>> drivers/scsi/qedi/qedi_version.h | 14 +
>> 14 files changed, 2508 insertions(+), 12 deletions(-)
>> create mode 100644 drivers/scsi/qedi/Kconfig
>> create mode 100644 drivers/scsi/qedi/Makefile
>> create mode 100644 drivers/scsi/qedi/qedi.h
>> create mode 100644 drivers/scsi/qedi/qedi_dbg.c
>> create mode 100644 drivers/scsi/qedi/qedi_dbg.h
>> create mode 100644 drivers/scsi/qedi/qedi_debugfs.c
>> create mode 100644 drivers/scsi/qedi/qedi_hsi.h
>> create mode 100644 drivers/scsi/qedi/qedi_main.c
>> create mode 100644 drivers/scsi/qedi/qedi_sysfs.c
>> create mode 100644 drivers/scsi/qedi/qedi_version.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5e925a2..906d05f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9909,6 +9909,12 @@ F: drivers/net/ethernet/qlogic/qed/
>> F: include/linux/qed/
>> F: drivers/net/ethernet/qlogic/qede/
>>
>> +QLOGIC QL41xxx ISCSI DRIVER
>> +M: QLogic-Storage-Upstream@cavium.com
>> +L: linux-scsi@vger.kernel.org
>> +S: Supported
>> +F: drivers/scsi/qedi/
>> +
>> QNX4 FILESYSTEM
>> M: Anders Larsen <al@alarsen.net>
>> W: http://www.alarsen.net/linux/qnx4fs/
>> diff --git a/drivers/net/ethernet/qlogic/Kconfig
>>b/drivers/net/ethernet/qlogic/Kconfig
>> index bad4fae..28b4366 100644
>> --- a/drivers/net/ethernet/qlogic/Kconfig
>> +++ b/drivers/net/ethernet/qlogic/Kconfig
>> @@ -121,16 +121,4 @@ config INFINIBAND_QEDR
>> config QED_ISCSI
>> bool
>>
>> -config QEDI
>> - tristate "QLogic QED 25/40/100Gb iSCSI driver"
>> - depends on QED
>> - select QED_LL2
>> - select QED_ISCSI
>> - default n
>> - ---help---
>> - This provides a temporary node that allows the compilation
>> - and logical testing of the hardware offload iSCSI support
>> - for QLogic QED. This would be replaced by the 'real' option
>> - once the QEDI driver is added [+relocated].
>> -
>> endif # NET_VENDOR_QLOGIC
>Huh? You just introduce this one in patch 1/6.
>Please fold them together so that this can be omitted.
Yes, we will remove this in the next revision.
-- snipped --
>> @@ -0,0 +1,52 @@
>> +/*
>> + * QLogic iSCSI Offload Driver
>> + * Copyright (c) 2016 Cavium Inc.
>> + *
>> + * This software is available under the terms of the GNU General
>>Public License
>> + * (GPL) Version 2, available from the file COPYING in the main
>>directory of
>> + * this source tree.
>> + */
>> +#ifndef __QEDI_HSI__
>> +#define __QEDI_HSI__
>> +/********************************/
>> +/* Add include to common target */
>> +/********************************/
>> +#include <linux/qed/common_hsi.h>
>> +
>Please use kernel-doc style for comments
Will do.
--snipped--
>> +static void qedi_int_fp(struct qedi_ctx *qedi)
>> +{
>> + struct qedi_fastpath *fp;
>> + int id;
>> +
>> + memset((void *)qedi->fp_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->fp_array));
>> + memset((void *)qedi->sb_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->sb_array));
>> +
>> + for (id = 0; id < MIN_NUM_CPUS_MSIX(qedi); id++) {
>> + fp = &qedi->fp_array[id];
>> + fp->sb_info = &qedi->sb_array[id];
>> + fp->sb_id = id;
>> + fp->qedi = qedi;
>> + snprintf(fp->name, sizeof(fp->name), "%s-fp-%d",
>> + "qedi", id);
>> +
>> + /* fp_array[i] ---- irq cookie
>> + * So init data which is needed in int ctx
>> + */
>> + }
>> +}
>> +
>Please check if you cannot make use of Christophs irq rework.
Sure, we will explore this.
--snipped--
>> +static bool qedi_process_completions(struct qedi_fastpath *fp)
>> +{
>> + struct qedi_work *qedi_work = NULL;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + struct qed_sb_info *sb_info = fp->sb_info;
>> + struct status_block *sb = sb_info->sb_virt;
>> + struct qedi_percpu_s *p = NULL;
>> + struct global_queue *que;
>> + u16 prod_idx;
>> + unsigned long flags;
>> + union iscsi_cqe *cqe;
>> + int cpu;
>> +
>> + /* Get the current firmware producer index */
>> + prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
>> +
>> + if (prod_idx >= QEDI_CQ_SIZE)
>> + prod_idx = prod_idx % QEDI_CQ_SIZE;
>> +
>> + que = qedi->global_queues[fp->sb_id];
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_IO,
>> + "Before: global queue=%p prod_idx=%d cons_idx=%d, sb_id=%d\n",
>> + que, prod_idx, que->cq_cons_idx, fp->sb_id);
>> +
>> + qedi->intr_cpu = fp->sb_id;
>> + cpu = smp_processor_id();
>> + p = &per_cpu(qedi_percpu, cpu);
>> +
>> + if (unlikely(!p->iothread))
>> + WARN_ON(1);
>> +
>> + spin_lock_irqsave(&p->p_work_lock, flags);
>> + while (que->cq_cons_idx != prod_idx) {
>> + cqe = &que->cq[que->cq_cons_idx];
>> +
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_IO,
>> + "cqe=%p prod_idx=%d cons_idx=%d.\n",
>> + cqe, prod_idx, que->cq_cons_idx);
>> +
>> + /* Alloc and copy to the cqe */
>> + qedi_work = kzalloc(sizeof(*qedi_work), GFP_ATOMIC);
>> + if (qedi_work) {
>> + INIT_LIST_HEAD(&qedi_work->list);
>> + qedi_work->qedi = qedi;
>> + memcpy(&qedi_work->cqe, cqe, sizeof(union iscsi_cqe));
>> + qedi_work->que_idx = fp->sb_id;
>> + list_add_tail(&qedi_work->list, &p->work_list);
>> + } else {
>> + WARN_ON(1);
>> + continue;
>> + }
>> +
>Memory allocation in an interrupt routine?
>You must be kidding ...
We will revisit this code.
>
>> + que->cq_cons_idx++;
>> + if (que->cq_cons_idx == QEDI_CQ_SIZE)
>> + que->cq_cons_idx = 0;
>> + }
>> + wake_up_process(p->iothread);
>> + spin_unlock_irqrestore(&p->p_work_lock, flags);
>> +
>> + return true;
>> +}
>> +
>> +static bool qedi_fp_has_work(struct qedi_fastpath *fp)
>> +{
>> + struct qedi_ctx *qedi = fp->qedi;
>> + struct global_queue *que;
>> + struct qed_sb_info *sb_info = fp->sb_info;
>> + struct status_block *sb = sb_info->sb_virt;
>> + u16 prod_idx;
>> +
>> + barrier();
>> +
>> + /* Get the current firmware producer index */
>> + prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
>> +
>> + /* Get the pointer to the global CQ this completion is on */
>> + que = qedi->global_queues[fp->sb_id];
>> +
>> + /* prod idx wrap around uint16 */
>> + if (prod_idx >= QEDI_CQ_SIZE)
>> + prod_idx = prod_idx % QEDI_CQ_SIZE;
>> +
>> + return (que->cq_cons_idx != prod_idx);
>> +}
>> +
>> +/* MSI-X fastpath handler code */
>> +static irqreturn_t qedi_msix_handler(int irq, void *dev_id)
>> +{
>> + struct qedi_fastpath *fp = dev_id;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + bool wake_io_thread = true;
>> +
>> + qed_sb_ack(fp->sb_info, IGU_INT_DISABLE, 0);
>> +
>> +process_again:
>> + wake_io_thread = qedi_process_completions(fp);
>> + if (wake_io_thread) {
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_DISC,
>> + "process already running\n");
>> + }
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_update_sb_idx(fp->sb_info);
>> +
>> + /* Check for more work */
>> + rmb();
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_ack(fp->sb_info, IGU_INT_ENABLE, 1);
>> + else
>> + goto process_again;
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/* simd handler for MSI/INTa */
>> +static void qedi_simd_int_handler(void *cookie)
>> +{
>> + /* Cookie is qedi_ctx struct */
>> + struct qedi_ctx *qedi = (struct qedi_ctx *)cookie;
>> +
>> + QEDI_WARN(&qedi->dbg_ctx, "qedi=%p.\n", qedi);
>> +}
>> +
>> +#define QEDI_SIMD_HANDLER_NUM 0
>> +static void qedi_sync_free_irqs(struct qedi_ctx *qedi)
>> +{
>> + int i;
>> +
>> + if (qedi->int_info.msix_cnt) {
>> + for (i = 0; i < qedi->int_info.used_cnt; i++) {
>> + synchronize_irq(qedi->int_info.msix[i].vector);
>> + irq_set_affinity_hint(qedi->int_info.msix[i].vector,
>> + NULL);
>> + free_irq(qedi->int_info.msix[i].vector,
>> + &qedi->fp_array[i]);
>> + }
>> + } else {
>> + qedi_ops->common->simd_handler_clean(qedi->cdev,
>> + QEDI_SIMD_HANDLER_NUM);
>> + }
>> +
>> + qedi->int_info.used_cnt = 0;
>> + qedi_ops->common->set_fp_int(qedi->cdev, 0);
>> +}
>> +
>Again, consider using the interrupt affinity rework from Christoph Hellwig
Sure, we will explore this one also.
^ permalink raw reply
* RE: [PATCH] net: fec: drop check for clk==NULL before calling clk_*
From: Andy Duan @ 2016-10-20 8:56 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: kernel@pengutronix.de, netdev@vger.kernel.org
In-Reply-To: <20161020082827.598-1-u.kleine-koenig@pengutronix.de>
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Thursday, October 20, 2016 4:28 PM
> To: Andy Duan <fugang.duan@nxp.com>
> Cc: kernel@pengutronix.de; netdev@vger.kernel.org
> Subject: [PATCH] net: fec: drop check for clk==NULL before calling clk_*
>
> clk_prepare, clk_enable and their counterparts (at least the common clk
> ones, but also most others) do check for the clk being NULL anyhow (and
> return 0 then), so there is no gain when the caller checks, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
Acked-by: Fugang Duan <fugang.duan@nxp.com>
> drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 48a033e64423..1722ccac18c2 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1841,11 +1841,11 @@ static int fec_enet_clk_enable(struct net_device
> *ndev, bool enable)
> ret = clk_prepare_enable(fep->clk_ahb);
> if (ret)
> return ret;
> - if (fep->clk_enet_out) {
> - ret = clk_prepare_enable(fep->clk_enet_out);
> - if (ret)
> - goto failed_clk_enet_out;
> - }
> +
> + ret = clk_prepare_enable(fep->clk_enet_out);
> + if (ret)
> + goto failed_clk_enet_out;
> +
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> ret = clk_prepare_enable(fep->clk_ptp); @@ -
> 1857,23 +1857,20 @@ static int fec_enet_clk_enable(struct net_device
> *ndev, bool enable)
> }
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref) {
> - ret = clk_prepare_enable(fep->clk_ref);
> - if (ret)
> - goto failed_clk_ref;
> - }
> +
> + ret = clk_prepare_enable(fep->clk_ref);
> + if (ret)
> + goto failed_clk_ref;
> } else {
> clk_disable_unprepare(fep->clk_ahb);
> - if (fep->clk_enet_out)
> - clk_disable_unprepare(fep->clk_enet_out);
> + clk_disable_unprepare(fep->clk_enet_out);
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> clk_disable_unprepare(fep->clk_ptp);
> fep->ptp_clk_on = false;
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref)
> - clk_disable_unprepare(fep->clk_ref);
> + clk_disable_unprepare(fep->clk_ref);
> }
>
> return 0;
> --
> 2.9.3
^ permalink raw reply
* Re: [RFC 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Rangankar, Manish @ 2016-10-20 8:41 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: lduncan@suse.com, cleech@redhat.com, martin.petersen@oracle.com,
jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <20161019100253.vxqxp5fhoxrlt6ay@linux-x5ow.site>
Thanks Johannes for the review, please see comments below,
On 19/10/16 3:32 PM, "Johannes Thumshirn" <jthumshirn@suse.de> wrote:
>On Wed, Oct 19, 2016 at 01:01:10AM -0400, manish.rangankar@cavium.com
>wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
>> for 41000 Series Converged Network Adapters by QLogic.
>>
>> This patch consists of following changes:
>> - MAINTAINERS Makefile and Kconfig changes for qedi,
>> - PCI driver registration,
>> - iSCSI host level initialization,
>> - Debugfs and log level infrastructure.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>
>[...]
>
>> +static inline void *qedi_get_task_mem(struct qed_iscsi_tid *info, u32
>>tid)
>> +{
>> + return (void *)(info->blocks[tid / info->num_tids_per_block] +
>> + (tid % info->num_tids_per_block) * info->size);
>> +}
>
>Unnecessary cast here.
Noted
>
>
>[...]
>
>> +void
>> +qedi_dbg_err(struct qedi_dbg_ctx *qedi, const char *func, u32 line,
>> + const char *fmt, ...)
>> +{
>> + va_list va;
>> + struct va_format vaf;
>> + char nfunc[32];
>> +
>> + memset(nfunc, 0, sizeof(nfunc));
>> + memcpy(nfunc, func, sizeof(nfunc) - 1);
>> +
>> + va_start(va, fmt);
>> +
>> + vaf.fmt = fmt;
>> + vaf.va = &va;
>> +
>> + if (likely(qedi) && likely(qedi->pdev))
>> + pr_crit("[%s]:[%s:%d]:%d: %pV", dev_name(&qedi->pdev->dev),
>> + nfunc, line, qedi->host_no, &vaf);
>> + else
>> + pr_crit("[0000:00:00.0]:[%s:%d]: %pV", nfunc, line, &vaf);
>
>pr_crit, seriously?
We will change it to pr_err.
>
>[...]
>
>> +static void qedi_int_fp(struct qedi_ctx *qedi)
>> +{
>> + struct qedi_fastpath *fp;
>> + int id;
>> +
>> + memset((void *)qedi->fp_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->fp_array));
>> + memset((void *)qedi->sb_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->sb_array));
>
>I don't think the cast is necessary here.
Noted
>
>[...]
>
>> +static int qedi_setup_cid_que(struct qedi_ctx *qedi)
>> +{
>> + int i;
>> +
>> + qedi->cid_que.cid_que_base = kmalloc((qedi->max_active_conns *
>> + sizeof(u32)), GFP_KERNEL);
>> + if (!qedi->cid_que.cid_que_base)
>> + return -ENOMEM;
>> +
>> + qedi->cid_que.conn_cid_tbl = kmalloc((qedi->max_active_conns *
>> + sizeof(struct qedi_conn *)),
>> + GFP_KERNEL);
>
>Please use kmalloc_array() here.
Will do.
>
>[...]
>
>> +/* MSI-X fastpath handler code */
>> +static irqreturn_t qedi_msix_handler(int irq, void *dev_id)
>> +{
>> + struct qedi_fastpath *fp = dev_id;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + bool wake_io_thread = true;
>> +
>> + qed_sb_ack(fp->sb_info, IGU_INT_DISABLE, 0);
>> +
>> +process_again:
>> + wake_io_thread = qedi_process_completions(fp);
>> + if (wake_io_thread) {
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_DISC,
>> + "process already running\n");
>> + }
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_update_sb_idx(fp->sb_info);
>> +
>> + /* Check for more work */
>> + rmb();
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_ack(fp->sb_info, IGU_INT_ENABLE, 1);
>> + else
>> + goto process_again;
>> +
>> + return IRQ_HANDLED;
>> +}
>
>You might want to consider workqueues here.
We will revisit this code.
>
>[...]
>
>> +static int qedi_alloc_itt(struct qedi_ctx *qedi)
>> +{
>> + qedi->itt_map = kzalloc((sizeof(struct qedi_itt_map) *
>> + MAX_ISCSI_TASK_ENTRIES), GFP_KERNEL);
>
>that screams for kcalloc()
>
>> + if (!qedi->itt_map) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Unable to allocate itt map array memory\n");
>> + return -ENOMEM;
>> + }
>> + return 0;
>> +}
>> +
>> +static void qedi_free_itt(struct qedi_ctx *qedi)
>> +{
>> + kfree(qedi->itt_map);
>> +}
>> +
>> +static struct qed_ll2_cb_ops qedi_ll2_cb_ops = {
>> + .rx_cb = qedi_ll2_rx,
>> + .tx_cb = NULL,
>> +};
>> +
>> +static int qedi_percpu_io_thread(void *arg)
>> +{
>> + struct qedi_percpu_s *p = arg;
>> + struct qedi_work *work, *tmp;
>> + unsigned long flags;
>> + LIST_HEAD(work_list);
>> +
>> + set_user_nice(current, -20);
>> +
>> + while (!kthread_should_stop()) {
>> + spin_lock_irqsave(&p->p_work_lock, flags);
>> + while (!list_empty(&p->work_list)) {
>> + list_splice_init(&p->work_list, &work_list);
>> + spin_unlock_irqrestore(&p->p_work_lock, flags);
>> +
>> + list_for_each_entry_safe(work, tmp, &work_list, list) {
>> + list_del_init(&work->list);
>> + qedi_fp_process_cqes(work->qedi, &work->cqe,
>> + work->que_idx);
>> + kfree(work);
>> + }
>> + spin_lock_irqsave(&p->p_work_lock, flags);
>> + }
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + spin_unlock_irqrestore(&p->p_work_lock, flags);
>> + schedule();
>> + }
>> + __set_current_state(TASK_RUNNING);
>> +
>> + return 0;
>> +}
>
>A kthread with prio -20 IRQs turned off looping over a list, what could
>possibly go wrong here. I bet you your favorite beverage that this will
>cause Soft Lockups when running I/O stress tests BTDT.
Will remove this.
>
>[...]
>
>> + if (mode != QEDI_MODE_RECOVERY) {
>> + if (iscsi_host_add(qedi->shost, &pdev->dev)) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not add iscsi host\n");
>> + rc = -ENOMEM;
>> + goto remove_host;
>> + }
>> +
>> + /* Allocate uio buffers */
>> + rc = qedi_alloc_uio_rings(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "UIO alloc ring failed err=%d\n", rc);
>> + goto remove_host;
>> + }
>> +
>> + rc = qedi_init_uio(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "UIO init failed, err=%d\n", rc);
>> + goto free_uio;
>> + }
>> +
>> + /* host the array on iscsi_conn */
>> + rc = qedi_setup_cid_que(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not setup cid que\n");
>> + goto free_uio;
>> + }
>> +
>> + rc = qedi_cm_alloc_mem(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not alloc cm memory\n");
>> + goto free_cid_que;
>> + }
>> +
>> + rc = qedi_alloc_itt(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not alloc itt memory\n");
>> + goto free_cid_que;
>> + }
>> +
>> + sprintf(host_buf, "host_%d", qedi->shost->host_no);
>> + qedi->tmf_thread = create_singlethread_workqueue(host_buf);
>> + if (!qedi->tmf_thread) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Unable to start tmf thread!\n");
>> + rc = -ENODEV;
>> + goto free_cid_que;
>> + }
>> +
>> + sprintf(host_buf, "qedi_ofld%d", qedi->shost->host_no);
>> + qedi->offload_thread = create_workqueue(host_buf);
>> + if (!qedi->offload_thread) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Unable to start offload thread!\n");
>> + rc = -ENODEV;
>> + goto free_cid_que;
>> + }
>> +
>> + /* F/w needs 1st task context memory entry for performance */
>> + set_bit(QEDI_RESERVE_TASK_ID, qedi->task_idx_map);
>> + atomic_set(&qedi->num_offloads, 0);
>> + }
>> +
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_INFO,
>> + "QLogic FastLinQ iSCSI Module qedi %s, FW %d.%d.%d.%d\n",
>> + QEDI_MODULE_VERSION, FW_MAJOR_VERSION, FW_MINOR_VERSION,
>> + FW_REVISION_VERSION, FW_ENGINEERING_VERSION);
>> + return 0;
>
>Please put the QEDI_INFO() above the if and invert the condition.
Will do.
^ permalink raw reply
* [PATCH] bnx2x: Replace semaphore stats_lock with mutex
From: Binoy Jayan @ 2016-10-20 8:52 UTC (permalink / raw)
To: Ariel Elior; +Cc: Arnd Bergmann, netdev, linux-kernel, Binoy Jayan
stats_lock is used as a simple mutex, so replace it with a mutex.
Semaphores are going away in the future.
Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
---
They following is a patch which removes semaphores from bnx2x.
Its part of a bigger effort to eliminate all semaphores
from the linux kernel.
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 3 ++-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 +++---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 17 +++++++----------
3 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 7dd7490..fd5e5b8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -25,6 +25,7 @@
#include <linux/ptp_clock_kernel.h>
#include <linux/net_tstamp.h>
#include <linux/timecounter.h>
+#include <linux/mutex.h>
/* compilation time flags */
@@ -1698,7 +1699,7 @@ struct bnx2x {
int stats_state;
/* used for synchronization of concurrent threads statistics handling */
- struct semaphore stats_lock;
+ struct mutex stats_lock;
/* used by dmae command loader */
struct dmae_command stats_dmae;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 20fe6a8..b323d69 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12340,7 +12340,7 @@ static int bnx2x_init_bp(struct bnx2x *bp)
mutex_init(&bp->port.phy_mutex);
mutex_init(&bp->fw_mb_mutex);
mutex_init(&bp->drv_info_mutex);
- sema_init(&bp->stats_lock, 1);
+ mutex_init(&bp->stats_lock);
bp->drv_info_mng_owner = false;
INIT_LIST_HEAD(&bp->vlan_reg);
@@ -14205,9 +14205,9 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
cancel_delayed_work_sync(&bp->sp_task);
cancel_delayed_work_sync(&bp->period_task);
- if (!down_timeout(&bp->stats_lock, HZ / 10)) {
+ if (mutex_trylock(&bp->stats_lock)) {
bp->stats_state = STATS_STATE_DISABLED;
- up(&bp->stats_lock);
+ mutex_unlock(&bp->stats_lock);
}
bnx2x_save_statistics(bp);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
index 7e0919a..ee6ffd8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
@@ -1374,23 +1374,20 @@ void bnx2x_stats_handle(struct bnx2x *bp, enum bnx2x_stats_event event)
* that context in case someone is in the middle of a transition.
* For other events, wait a bit until lock is taken.
*/
- if (down_trylock(&bp->stats_lock)) {
+ if (!mutex_trylock(&bp->stats_lock)) {
if (event == STATS_EVENT_UPDATE)
return;
DP(BNX2X_MSG_STATS,
"Unlikely stats' lock contention [event %d]\n", event);
- if (unlikely(down_timeout(&bp->stats_lock, HZ / 10))) {
- BNX2X_ERR("Failed to take stats lock [event %d]\n",
- event);
- return;
- }
+ BNX2X_ERR("Failed to take stats lock [event %d]\n", event);
+ return;
}
bnx2x_stats_stm[state][event].action(bp);
bp->stats_state = bnx2x_stats_stm[state][event].next_state;
- up(&bp->stats_lock);
+ mutex_unlock(&bp->stats_lock);
if ((event != STATS_EVENT_UPDATE) || netif_msg_timer(bp))
DP(BNX2X_MSG_STATS, "state %d -> event %d -> state %d\n",
@@ -1976,8 +1973,8 @@ int bnx2x_stats_safe_exec(struct bnx2x *bp,
/* Wait for statistics to end [while blocking further requests],
* then run supplied function 'safely'.
*/
- rc = down_timeout(&bp->stats_lock, HZ / 10);
- if (unlikely(rc)) {
+ rc = mutex_trylock(&bp->stats_lock);
+ if (unlikely(!rc)) {
BNX2X_ERR("Failed to take statistics lock for safe execution\n");
goto out_no_lock;
}
@@ -1998,7 +1995,7 @@ int bnx2x_stats_safe_exec(struct bnx2x *bp,
/* No need to restart statistics - if they're enabled, the timer
* will restart the statistics.
*/
- up(&bp->stats_lock);
+ mutex_unlock(&bp->stats_lock);
out_no_lock:
return rc;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
From: Vitaly Kuznetsov @ 2016-10-20 8:51 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev@vger.kernel.org, Haiyang Zhang,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org
In-Reply-To: <BLUPR0301MB2098D6A07F0D465E63988D9FCCD50@BLUPR0301MB2098.namprd03.prod.outlook.com>
Stephen Hemminger <sthemmin@microsoft.com> writes:
> Do we need ACCESS_ONCE() here to avoid check/use issues?
>
I think we don't: this is the only place in the function where we read
the variable so we'll get normal read. We're not trying to syncronize
with netvsc_init_buf() as that would require locking, if we read stale
NULL value after it was already updated on a different CPU we're fine,
we'll just return -EAGAIN.
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, October 19, 2016 2:53 PM
> To: netdev@vger.kernel.org
> Cc: Stephen Hemminger <sthemmin@microsoft.com>; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>
> Subject: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
>
> Fix in commit 880988348270 ("hv_netvsc: set nvdev link after populating
> chn_table") turns out to be incomplete. A crash in
> netvsc_get_next_send_section() is observed on mtu change when the device is under load. The race I identified is: if we get to netvsc_send() after we set net_device_ctx->nvdev link in netvsc_device_add() but before we finish netvsc_connect_vsp()->netvsc_init_buf() send_section_map is not allocated and we crash. Unfortunately we can't set net_device_ctx->nvdev link after the netvsc_init_buf() call as during the negotiation we need to receive packets and on the receive path we check for it. It would probably be possible to split nvdev into a pair of nvdev_in and nvdev_out links and check them accordingly in get_outbound_net_device()/
> get_inbound_net_device() but this looks like an overkill.
>
> Check that send_section_map is allocated in netvsc_send().
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/net/hyperv/netvsc.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 720b5fa..e2bfaac 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -888,6 +888,13 @@ int netvsc_send(struct hv_device *device,
> if (!net_device)
> return -ENODEV;
>
> + /* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
> + * here before the negotiation with the host is finished and
> + * send_section_map may not be allocated yet.
> + */
> + if (!net_device->send_section_map)
> + return -EAGAIN;
> +
> out_channel = net_device->chn_table[q_idx];
>
> packet->send_buf_index = NETVSC_INVALID_INDEX;
> --
> 2.7.4
--
Vitaly
^ permalink raw reply
* Re: [PATCH net] conntrack: perform a full scan in gc
From: Nicolas Dichtel @ 2016-10-20 8:50 UTC (permalink / raw)
To: Florian Westphal; +Cc: pablo, davem, netdev, netfilter-devel
In-Reply-To: <34d607cc-5556-89c7-673d-be8cb0a4758a@6wind.com>
Le 18/10/2016 à 12:06, Nicolas Dichtel a écrit :
> Le 18/10/2016 à 10:47, Florian Westphal a écrit :
>> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>> After commit b87a2f9199ea ("netfilter: conntrack: add gc worker to remove
>>> timed-out entries"), netlink conntrack deletion events may be sent with a
>>> huge delay (5 minutes).
>>>
>>> There is two ways to evict conntrack:
>>> - during a conntrack lookup;
>>> - during a conntrack dump.
>>> Let's do a full scan of conntrack entries after a period of inactivity
>>> (no conntrack lookup).
>>>
>>> CC: Florian Westphal <fw@strlen.de>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>> ---
>>>
>>> Here is another proposal to try to fix the problem.
>>> Comments are welcomed,
>>> Nicolas
>>
>> Hmm, I don't think its good idea in practice.
>> If goal is to avoid starving arbitrary 'dead' ct for too long,
>> then simple ping will defeat the logic here, because...
>>
>>> net/netfilter/nf_conntrack_core.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>>> index ba6a1d421222..3dbb27bd9582 100644
>>> --- a/net/netfilter/nf_conntrack_core.c
>>> +++ b/net/netfilter/nf_conntrack_core.c
>>> @@ -87,6 +87,7 @@ static __read_mostly bool nf_conntrack_locks_all;
>>> #define GC_MAX_BUCKETS 8192u
>>> #define GC_INTERVAL (5 * HZ)
>>> #define GC_MAX_EVICTS 256u
>>> +static bool gc_full_scan = true;
>>>
>>> static struct conntrack_gc_work conntrack_gc_work;
>>>
>>> @@ -511,6 +512,7 @@ ____nf_conntrack_find(struct net *net, const struct nf_conntrack_zone *zone,
>>> unsigned int bucket, hsize;
>>>
>>> begin:
>>> + gc_full_scan = false;
>>
>> ... we do periodic lookup (but always in same slot), so no full scan is
>> triggered.
> Yes, I was wondering about that. My first idea was to have that bool per bucket
> and force a scan of the bucket instead of the whole table.
FYI, I'am off for about two weeks, but we really need to find a way to fix that
before the release goes out.
^ permalink raw reply
* RE: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
From: Stephen Hemminger @ 2016-10-20 8:36 UTC (permalink / raw)
To: Vitaly Kuznetsov, netdev@vger.kernel.org
Cc: devel@linuxdriverproject.org, Haiyang Zhang,
linux-kernel@vger.kernel.org
In-Reply-To: <1476885181-3456-1-git-send-email-vkuznets@redhat.com>
Do we need ACCESS_ONCE() here to avoid check/use issues?
-----Original Message-----
From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
Sent: Wednesday, October 19, 2016 2:53 PM
To: netdev@vger.kernel.org
Cc: Stephen Hemminger <sthemmin@microsoft.com>; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>
Subject: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
Fix in commit 880988348270 ("hv_netvsc: set nvdev link after populating
chn_table") turns out to be incomplete. A crash in
netvsc_get_next_send_section() is observed on mtu change when the device is under load. The race I identified is: if we get to netvsc_send() after we set net_device_ctx->nvdev link in netvsc_device_add() but before we finish netvsc_connect_vsp()->netvsc_init_buf() send_section_map is not allocated and we crash. Unfortunately we can't set net_device_ctx->nvdev link after the netvsc_init_buf() call as during the negotiation we need to receive packets and on the receive path we check for it. It would probably be possible to split nvdev into a pair of nvdev_in and nvdev_out links and check them accordingly in get_outbound_net_device()/
get_inbound_net_device() but this looks like an overkill.
Check that send_section_map is allocated in netvsc_send().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/net/hyperv/netvsc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 720b5fa..e2bfaac 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -888,6 +888,13 @@ int netvsc_send(struct hv_device *device,
if (!net_device)
return -ENODEV;
+ /* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
+ * here before the negotiation with the host is finished and
+ * send_section_map may not be allocated yet.
+ */
+ if (!net_device->send_section_map)
+ return -EAGAIN;
+
out_channel = net_device->chn_table[q_idx];
packet->send_buf_index = NETVSC_INVALID_INDEX;
--
2.7.4
^ permalink raw reply
* [PATCH] net: fec: drop check for clk==NULL before calling clk_*
From: Uwe Kleine-König @ 2016-10-20 8:28 UTC (permalink / raw)
To: Fugang Duan; +Cc: kernel, netdev
clk_prepare, clk_enable and their counterparts (at least the common clk
ones, but also most others) do check for the clk being NULL anyhow (and
return 0 then), so there is no gain when the caller checks, too.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 48a033e64423..1722ccac18c2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1841,11 +1841,11 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
ret = clk_prepare_enable(fep->clk_ahb);
if (ret)
return ret;
- if (fep->clk_enet_out) {
- ret = clk_prepare_enable(fep->clk_enet_out);
- if (ret)
- goto failed_clk_enet_out;
- }
+
+ ret = clk_prepare_enable(fep->clk_enet_out);
+ if (ret)
+ goto failed_clk_enet_out;
+
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
ret = clk_prepare_enable(fep->clk_ptp);
@@ -1857,23 +1857,20 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
}
mutex_unlock(&fep->ptp_clk_mutex);
}
- if (fep->clk_ref) {
- ret = clk_prepare_enable(fep->clk_ref);
- if (ret)
- goto failed_clk_ref;
- }
+
+ ret = clk_prepare_enable(fep->clk_ref);
+ if (ret)
+ goto failed_clk_ref;
} else {
clk_disable_unprepare(fep->clk_ahb);
- if (fep->clk_enet_out)
- clk_disable_unprepare(fep->clk_enet_out);
+ clk_disable_unprepare(fep->clk_enet_out);
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
clk_disable_unprepare(fep->clk_ptp);
fep->ptp_clk_on = false;
mutex_unlock(&fep->ptp_clk_mutex);
}
- if (fep->clk_ref)
- clk_disable_unprepare(fep->clk_ref);
+ clk_disable_unprepare(fep->clk_ref);
}
return 0;
--
2.9.3
^ permalink raw reply related
* [PATCH (net.git)] stmmac: display the descriptors if DES0 = 0
From: Giuseppe Cavallaro @ 2016-10-20 8:01 UTC (permalink / raw)
To: netdev; +Cc: Giuseppe Cavallaro, Alexandre TORGUE
It makes sense to display the descriptors even if
DES0 is zero. This helps for example in case of it
is needed to dump rx write-back descriptors to get
timestamp status.
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre TORGUE <alexandre.torgue@st.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
index 4ec7397..a1b17cd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
@@ -347,10 +347,9 @@ static void dwmac4_display_ring(void *head, unsigned int size, bool rx)
pr_info("%s descriptor ring:\n", rx ? "RX" : "TX");
for (i = 0; i < size; i++) {
- if (p->des0)
- pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
- i, (unsigned int)virt_to_phys(p),
- p->des0, p->des1, p->des2, p->des3);
+ pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
+ i, (unsigned int)virt_to_phys(p),
+ p->des0, p->des1, p->des2, p->des3);
p++;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next V2 3/9] liquidio CN23XX: Mailbox support
From: Raghu Vatsavayi @ 2016-10-20 5:40 UTC (permalink / raw)
To: davem
Cc: netdev, Raghu Vatsavayi, Raghu Vatsavayi, Derek Chickles,
Satanand Burla, Felix Manlunas
In-Reply-To: <1476942046-18789-1-git-send-email-rvatsavayi@caviumnetworks.com>
Adds support for mailbox communication between PF and VF.
Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@caviumnetworks.com>
Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
Signed-off-by: Satanand Burla <satananda.burla@caviumnetworks.com>
Signed-off-by: Felix Manlunas <felix.manlunas@caviumnetworks.com>
---
drivers/net/ethernet/cavium/liquidio/Makefile | 1 +
drivers/net/ethernet/cavium/liquidio/lio_core.c | 32 ++
.../net/ethernet/cavium/liquidio/liquidio_common.h | 6 +-
.../net/ethernet/cavium/liquidio/octeon_device.h | 4 +
.../net/ethernet/cavium/liquidio/octeon_mailbox.c | 321 +++++++++++++++++++++
.../net/ethernet/cavium/liquidio/octeon_mailbox.h | 115 ++++++++
drivers/net/ethernet/cavium/liquidio/octeon_main.h | 2 +-
7 files changed, 478 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
create mode 100644 drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
diff --git a/drivers/net/ethernet/cavium/liquidio/Makefile b/drivers/net/ethernet/cavium/liquidio/Makefile
index 5a27b2a..14958de 100644
--- a/drivers/net/ethernet/cavium/liquidio/Makefile
+++ b/drivers/net/ethernet/cavium/liquidio/Makefile
@@ -11,6 +11,7 @@ liquidio-$(CONFIG_LIQUIDIO) += lio_ethtool.o \
cn66xx_device.o \
cn68xx_device.o \
cn23xx_pf_device.o \
+ octeon_mailbox.o \
octeon_mem_ops.o \
octeon_droq.o \
octeon_nic.o
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_core.c b/drivers/net/ethernet/cavium/liquidio/lio_core.c
index 201eddb..e6026df 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_core.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_core.c
@@ -264,3 +264,35 @@ void liquidio_link_ctrl_cmd_completion(void *nctrl_ptr)
nctrl->ncmd.s.cmd);
}
}
+
+void octeon_pf_changed_vf_macaddr(struct octeon_device *oct, u8 *mac)
+{
+ bool macaddr_changed = false;
+ struct net_device *netdev;
+ struct lio *lio;
+
+ rtnl_lock();
+
+ netdev = oct->props[0].netdev;
+ lio = GET_LIO(netdev);
+
+ lio->linfo.macaddr_is_admin_asgnd = true;
+
+ if (!ether_addr_equal(netdev->dev_addr, mac)) {
+ macaddr_changed = true;
+ ether_addr_copy(netdev->dev_addr, mac);
+ ether_addr_copy(((u8 *)&lio->linfo.hw_addr) + 2, mac);
+ call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
+ }
+
+ rtnl_unlock();
+
+ if (macaddr_changed)
+ dev_info(&oct->pci_dev->dev,
+ "PF changed VF's MAC address to %02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx\n",
+ mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+
+ /* no need to notify the firmware of the macaddr change because
+ * the PF did that already
+ */
+}
diff --git a/drivers/net/ethernet/cavium/liquidio/liquidio_common.h b/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
index 0d990ac..caeff9a 100644
--- a/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
+++ b/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
@@ -731,13 +731,15 @@ struct oct_link_info {
#ifdef __BIG_ENDIAN_BITFIELD
u64 gmxport:16;
- u64 rsvd:32;
+ u64 macaddr_is_admin_asgnd:1;
+ u64 rsvd:31;
u64 num_txpciq:8;
u64 num_rxpciq:8;
#else
u64 num_rxpciq:8;
u64 num_txpciq:8;
- u64 rsvd:32;
+ u64 rsvd:31;
+ u64 macaddr_is_admin_asgnd:1;
u64 gmxport:16;
#endif
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.h b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
index cfd12ec..77a6eb7 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
@@ -492,6 +492,9 @@ struct octeon_device {
int msix_on;
+ /** Mail Box details of each octeon queue. */
+ struct octeon_mbox *mbox[MAX_POSSIBLE_VFS];
+
/** IOq information of it's corresponding MSI-X interrupt. */
struct octeon_ioq_vector *ioq_vector;
@@ -511,6 +514,7 @@ struct octeon_device {
#define OCTEON_CN6XXX(oct) ((oct->chip_id == OCTEON_CN66XX) || \
(oct->chip_id == OCTEON_CN68XX))
#define OCTEON_CN23XX_PF(oct) (oct->chip_id == OCTEON_CN23XX_PF_VID)
+#define OCTEON_CN23XX_VF(oct) ((oct)->chip_id == OCTEON_CN23XX_VF_VID)
#define CHIP_FIELD(oct, TYPE, field) \
(((struct octeon_ ## TYPE *)(oct->chip))->field)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
new file mode 100644
index 0000000..3a2f6c1
--- /dev/null
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
@@ -0,0 +1,321 @@
+/**********************************************************************
+ * Author: Cavium, Inc.
+ *
+ * Contact: support@cavium.com
+ * Please include "LiquidIO" in the subject.
+ *
+ * Copyright (c) 2003-2016 Cavium, Inc.
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful, but
+ * AS-IS and WITHOUT ANY WARRANTY; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, TITLE, or
+ * NONINFRINGEMENT. See the GNU General Public License for more details.
+ *
+ * This file may also be available under a different license from Cavium.
+ * Contact Cavium, Inc. for more information
+ ***********************************************************************/
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include "liquidio_common.h"
+#include "octeon_droq.h"
+#include "octeon_iq.h"
+#include "response_manager.h"
+#include "octeon_device.h"
+#include "octeon_main.h"
+#include "octeon_mailbox.h"
+
+/**
+ * octeon_mbox_read:
+ * @oct: Pointer mailbox
+ *
+ * Reads the 8-bytes of data from the mbox register
+ * Writes back the acknowldgement inidcating completion of read
+ */
+int octeon_mbox_read(struct octeon_mbox *mbox)
+{
+ union octeon_mbox_message msg;
+ int ret = 0;
+
+ spin_lock(&mbox->lock);
+
+ msg.u64 = readq(mbox->mbox_read_reg);
+
+ if ((msg.u64 == OCTEON_PFVFACK) || (msg.u64 == OCTEON_PFVFSIG)) {
+ spin_unlock(&mbox->lock);
+ return 0;
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVING) {
+ mbox->mbox_req.data[mbox->mbox_req.recv_len - 1] = msg.u64;
+ mbox->mbox_req.recv_len++;
+ } else {
+ if (mbox->state & OCTEON_MBOX_STATE_RESPONSE_RECEIVING) {
+ mbox->mbox_resp.data[mbox->mbox_resp.recv_len - 1] =
+ msg.u64;
+ mbox->mbox_resp.recv_len++;
+ } else {
+ if ((mbox->state & OCTEON_MBOX_STATE_IDLE) &&
+ (msg.s.type == OCTEON_MBOX_REQUEST)) {
+ mbox->state &= ~OCTEON_MBOX_STATE_IDLE;
+ mbox->state |=
+ OCTEON_MBOX_STATE_REQUEST_RECEIVING;
+ mbox->mbox_req.msg.u64 = msg.u64;
+ mbox->mbox_req.q_no = mbox->q_no;
+ mbox->mbox_req.recv_len = 1;
+ } else {
+ if ((mbox->state &
+ OCTEON_MBOX_STATE_RESPONSE_PENDING) &&
+ (msg.s.type == OCTEON_MBOX_RESPONSE)) {
+ mbox->state &=
+ ~OCTEON_MBOX_STATE_RESPONSE_PENDING;
+ mbox->state |=
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVING
+ ;
+ mbox->mbox_resp.msg.u64 = msg.u64;
+ mbox->mbox_resp.q_no = mbox->q_no;
+ mbox->mbox_resp.recv_len = 1;
+ } else {
+ writeq(OCTEON_PFVFERR,
+ mbox->mbox_read_reg);
+ mbox->state |= OCTEON_MBOX_STATE_ERROR;
+ spin_unlock(&mbox->lock);
+ return 1;
+ }
+ }
+ }
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVING) {
+ if (mbox->mbox_req.recv_len < msg.s.len) {
+ ret = 0;
+ } else {
+ mbox->state &= ~OCTEON_MBOX_STATE_REQUEST_RECEIVING;
+ mbox->state |= OCTEON_MBOX_STATE_REQUEST_RECEIVED;
+ ret = 1;
+ }
+ } else {
+ if (mbox->state & OCTEON_MBOX_STATE_RESPONSE_RECEIVING) {
+ if (mbox->mbox_resp.recv_len < msg.s.len) {
+ ret = 0;
+ } else {
+ mbox->state &=
+ ~OCTEON_MBOX_STATE_RESPONSE_RECEIVING;
+ mbox->state |=
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVED;
+ ret = 1;
+ }
+ } else {
+ WARN_ON(1);
+ }
+ }
+
+ writeq(OCTEON_PFVFACK, mbox->mbox_read_reg);
+
+ spin_unlock(&mbox->lock);
+
+ return ret;
+}
+
+/**
+ * octeon_mbox_write:
+ * @oct: Pointer Octeon Device
+ * @mbox_cmd: Cmd to send to mailbox.
+ *
+ * Populates the queue specific mbox structure
+ * with cmd information.
+ * Write the cmd to mbox register
+ */
+int octeon_mbox_write(struct octeon_device *oct,
+ struct octeon_mbox_cmd *mbox_cmd)
+{
+ struct octeon_mbox *mbox = oct->mbox[mbox_cmd->q_no];
+ u32 count, i, ret = OCTEON_MBOX_STATUS_SUCCESS;
+ unsigned long flags;
+
+ spin_lock_irqsave(&mbox->lock, flags);
+
+ if ((mbox_cmd->msg.s.type == OCTEON_MBOX_RESPONSE) &&
+ !(mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVED)) {
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ return OCTEON_MBOX_STATUS_FAILED;
+ }
+
+ if ((mbox_cmd->msg.s.type == OCTEON_MBOX_REQUEST) &&
+ !(mbox->state & OCTEON_MBOX_STATE_IDLE)) {
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ return OCTEON_MBOX_STATUS_BUSY;
+ }
+
+ if (mbox_cmd->msg.s.type == OCTEON_MBOX_REQUEST) {
+ memcpy(&mbox->mbox_resp, mbox_cmd,
+ sizeof(struct octeon_mbox_cmd));
+ mbox->state = OCTEON_MBOX_STATE_RESPONSE_PENDING;
+ }
+
+ spin_unlock_irqrestore(&mbox->lock, flags);
+
+ count = 0;
+
+ while (readq(mbox->mbox_write_reg) != OCTEON_PFVFSIG) {
+ schedule_timeout_uninterruptible(10);
+ if (count++ == 1000) {
+ ret = OCTEON_MBOX_STATUS_FAILED;
+ break;
+ }
+ }
+
+ if (ret == OCTEON_MBOX_STATUS_SUCCESS) {
+ writeq(mbox_cmd->msg.u64, mbox->mbox_write_reg);
+ for (i = 0; i < (u32)(mbox_cmd->msg.s.len - 1); i++) {
+ count = 0;
+ while (readq(mbox->mbox_write_reg) !=
+ OCTEON_PFVFACK) {
+ schedule_timeout_uninterruptible(10);
+ if (count++ == 1000) {
+ ret = OCTEON_MBOX_STATUS_FAILED;
+ break;
+ }
+ }
+ writeq(mbox_cmd->data[i], mbox->mbox_write_reg);
+ }
+ }
+
+ spin_lock_irqsave(&mbox->lock, flags);
+ if (mbox_cmd->msg.s.type == OCTEON_MBOX_RESPONSE) {
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ } else {
+ if ((!mbox_cmd->msg.s.resp_needed) ||
+ (ret == OCTEON_MBOX_STATUS_FAILED)) {
+ mbox->state &= ~OCTEON_MBOX_STATE_RESPONSE_PENDING;
+ if (!(mbox->state &
+ (OCTEON_MBOX_STATE_REQUEST_RECEIVING |
+ OCTEON_MBOX_STATE_REQUEST_RECEIVED)))
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ }
+ }
+ spin_unlock_irqrestore(&mbox->lock, flags);
+
+ return ret;
+}
+
+/**
+ * octeon_mbox_process_cmd:
+ * @mbox: Pointer mailbox
+ * @mbox_cmd: Pointer to command received
+ *
+ * Process the cmd received in mbox
+ */
+static int octeon_mbox_process_cmd(struct octeon_mbox *mbox,
+ struct octeon_mbox_cmd *mbox_cmd)
+{
+ struct octeon_device *oct = mbox->oct_dev;
+
+ switch (mbox_cmd->msg.s.cmd) {
+ case OCTEON_VF_ACTIVE:
+ dev_dbg(&oct->pci_dev->dev, "got vfactive sending data back\n");
+ mbox_cmd->msg.s.type = OCTEON_MBOX_RESPONSE;
+ mbox_cmd->msg.s.resp_needed = 1;
+ mbox_cmd->msg.s.len = 2;
+ mbox_cmd->data[0] = 0; /* VF version is in mbox_cmd->data[0] */
+ ((struct lio_version *)&mbox_cmd->data[0])->major =
+ LIQUIDIO_BASE_MAJOR_VERSION;
+ ((struct lio_version *)&mbox_cmd->data[0])->minor =
+ LIQUIDIO_BASE_MINOR_VERSION;
+ ((struct lio_version *)&mbox_cmd->data[0])->micro =
+ LIQUIDIO_BASE_MICRO_VERSION;
+ memcpy(mbox_cmd->msg.s.params, (uint8_t *)&oct->pfvf_hsword, 6);
+ /* Sending core cofig info to the corresponding active VF.*/
+ octeon_mbox_write(oct, mbox_cmd);
+ break;
+
+ case OCTEON_VF_FLR_REQUEST:
+ dev_info(&oct->pci_dev->dev,
+ "got a request for FLR from VF that owns DPI ring %u\n",
+ mbox->q_no);
+ pcie_capability_set_word(
+ oct->sriov_info.dpiring_to_vfpcidev_lut[mbox->q_no],
+ PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
+ break;
+
+ case OCTEON_PF_CHANGED_VF_MACADDR:
+ if (OCTEON_CN23XX_VF(oct))
+ octeon_pf_changed_vf_macaddr(oct,
+ mbox_cmd->msg.s.params);
+ break;
+
+ default:
+ break;
+ }
+ return 0;
+}
+
+/**
+ *octeon_mbox_process_message:
+ *
+ * Process the received mbox message.
+ */
+int octeon_mbox_process_message(struct octeon_mbox *mbox)
+{
+ struct octeon_mbox_cmd mbox_cmd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&mbox->lock, flags);
+
+ if (mbox->state & OCTEON_MBOX_STATE_ERROR) {
+ if (mbox->state & (OCTEON_MBOX_STATE_RESPONSE_PENDING |
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVING)) {
+ memcpy(&mbox_cmd, &mbox->mbox_resp,
+ sizeof(struct octeon_mbox_cmd));
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ mbox_cmd.recv_status = 1;
+ if (mbox_cmd.fn)
+ mbox_cmd.fn(mbox->oct_dev, &mbox_cmd,
+ mbox_cmd.fn_arg);
+ return 0;
+ }
+
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ return 0;
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_RESPONSE_RECEIVED) {
+ memcpy(&mbox_cmd, &mbox->mbox_resp,
+ sizeof(struct octeon_mbox_cmd));
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ mbox_cmd.recv_status = 0;
+ if (mbox_cmd.fn)
+ mbox_cmd.fn(mbox->oct_dev, &mbox_cmd, mbox_cmd.fn_arg);
+ return 0;
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVED) {
+ memcpy(&mbox_cmd, &mbox->mbox_req,
+ sizeof(struct octeon_mbox_cmd));
+ if (!mbox_cmd.msg.s.resp_needed) {
+ mbox->state &= ~OCTEON_MBOX_STATE_REQUEST_RECEIVED;
+ if (!(mbox->state &&
+ OCTEON_MBOX_STATE_RESPONSE_PENDING))
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ }
+
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ octeon_mbox_process_cmd(mbox, &mbox_cmd);
+ return 0;
+ }
+
+ WARN_ON(1);
+
+ return 0;
+}
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
new file mode 100644
index 0000000..377a43f
--- /dev/null
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
@@ -0,0 +1,115 @@
+/**********************************************************************
+ * Author: Cavium, Inc.
+ *
+ * Contact: support@cavium.com
+ * Please include "LiquidIO" in the subject.
+ *
+ * Copyright (c) 2003-2016 Cavium, Inc.
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful, but
+ * AS-IS and WITHOUT ANY WARRANTY; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, TITLE, or
+ * NONINFRINGEMENT. See the GNU General Public License for more details.
+ *
+ * This file may also be available under a different license from Cavium.
+ * Contact Cavium, Inc. for more information
+ ***********************************************************************/
+#ifndef __MAILBOX_H__
+#define __MAILBOX_H__
+
+/* Macros for Mail Box Communication */
+
+#define OCTEON_MBOX_DATA_MAX 32
+
+#define OCTEON_VF_ACTIVE 0x1
+#define OCTEON_VF_FLR_REQUEST 0x2
+#define OCTEON_PF_CHANGED_VF_MACADDR 0x4
+
+/*Macro for Read acknowldgement*/
+#define OCTEON_PFVFACK 0xffffffffffffffff
+#define OCTEON_PFVFSIG 0x1122334455667788
+#define OCTEON_PFVFERR 0xDEADDEADDEADDEAD
+
+enum octeon_mbox_cmd_status {
+ OCTEON_MBOX_STATUS_SUCCESS = 0,
+ OCTEON_MBOX_STATUS_FAILED = 1,
+ OCTEON_MBOX_STATUS_BUSY = 2
+};
+
+enum octeon_mbox_message_type {
+ OCTEON_MBOX_REQUEST = 0,
+ OCTEON_MBOX_RESPONSE = 1
+};
+
+union octeon_mbox_message {
+ u64 u64;
+ struct {
+ u16 type : 1;
+ u16 resp_needed : 1;
+ u16 cmd : 6;
+ u16 len : 8;
+ u8 params[6];
+ } s;
+};
+
+typedef void (*octeon_mbox_callback_t)(void *, void *, void *);
+
+struct octeon_mbox_cmd {
+ union octeon_mbox_message msg;
+ u64 data[OCTEON_MBOX_DATA_MAX];
+ u32 q_no;
+ u32 recv_len;
+ u32 recv_status;
+ octeon_mbox_callback_t fn;
+ void *fn_arg;
+};
+
+enum octeon_mbox_state {
+ OCTEON_MBOX_STATE_IDLE = 1,
+ OCTEON_MBOX_STATE_REQUEST_RECEIVING = 2,
+ OCTEON_MBOX_STATE_REQUEST_RECEIVED = 4,
+ OCTEON_MBOX_STATE_RESPONSE_PENDING = 8,
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVING = 16,
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVED = 16,
+ OCTEON_MBOX_STATE_ERROR = 32
+};
+
+struct octeon_mbox {
+ /** A spinlock to protect access to this q_mbox. */
+ spinlock_t lock;
+
+ struct octeon_device *oct_dev;
+
+ u32 q_no;
+
+ enum octeon_mbox_state state;
+
+ struct cavium_wk mbox_poll_wk;
+
+ /** SLI_MAC_PF_MBOX_INT for PF, SLI_PKT_MBOX_INT for VF. */
+ void *mbox_int_reg;
+
+ /** SLI_PKT_PF_VF_MBOX_SIG(0) for PF, SLI_PKT_PF_VF_MBOX_SIG(1) for VF.
+ */
+ void *mbox_write_reg;
+
+ /** SLI_PKT_PF_VF_MBOX_SIG(1) for PF, SLI_PKT_PF_VF_MBOX_SIG(0) for VF.
+ */
+ void *mbox_read_reg;
+
+ struct octeon_mbox_cmd mbox_req;
+
+ struct octeon_mbox_cmd mbox_resp;
+
+};
+
+int octeon_mbox_read(struct octeon_mbox *mbox);
+int octeon_mbox_write(struct octeon_device *oct,
+ struct octeon_mbox_cmd *mbox_cmd);
+int octeon_mbox_process_message(struct octeon_mbox *mbox);
+
+#endif
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_main.h b/drivers/net/ethernet/cavium/liquidio/octeon_main.h
index 366298f..3cddf28 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_main.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_main.h
@@ -66,7 +66,7 @@ void octeon_update_tx_completion_counters(void *buf, int reqtype,
unsigned int *bytes_compl);
void octeon_report_tx_completion_to_bql(void *txq, unsigned int pkts_compl,
unsigned int bytes_compl);
-
+void octeon_pf_changed_vf_macaddr(struct octeon_device *oct, u8 *mac);
/** Swap 8B blocks */
static inline void octeon_swap_8B_data(u64 *data, u32 blocks)
{
--
1.8.3.1
^ permalink raw reply related
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