[PATCH 0/3] cnic fixes.

[PATCH 0/3] cnic fixes.

[Michael Chan]

Fix 2 sleeping function from invalid context bugs and 1 missing iscsi netlink
message bug.

Michael Chan (3):
  cnic: Don't take rcu_read_lock in cnic_rcv_netevent()
  cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()
  cnic: Fix missing ISCSI_KEVENT_IF_DOWN message

 drivers/net/ethernet/broadcom/cnic.c |   29 ++++++++++++++++-------------
 1 files changed, 16 insertions(+), 13 deletions(-)

[PATCH 1/3] cnic: Don't take rcu_read_lock in cnic_rcv_netevent()

[Michael Chan]

Because the called function, such as bnx2fc_indicate_netevent(), can sleep,
we cannot take rcu_lock().  To prevent the rcu protected ulp_ops from going
away, we use the cnic_lock mutex and set the ULP_F_CALL_PENDING flag.
The code already waits for ULP_F_CALL_PENDING flag to clear in
cnic_unregister_device().

Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/cnic.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index 09f3fef..7f40a2c 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -5624,20 +5624,27 @@ static void cnic_rcv_netevent(struct cnic_local *cp, unsigned long event,
 {
 	int if_type;
 
-	rcu_read_lock();
 	for (if_type = 0; if_type < MAX_CNIC_ULP_TYPE; if_type++) {
 		struct cnic_ulp_ops *ulp_ops;
 		void *ctx;
 
-		ulp_ops = rcu_dereference(cp->ulp_ops[if_type]);
-		if (!ulp_ops || !ulp_ops->indicate_netevent)
+		mutex_lock(&cnic_lock);
+		ulp_ops = rcu_dereference_protected(cp->ulp_ops[if_type],
+						lockdep_is held(&cnic_lock));
+		if (!ulp_ops || !ulp_ops->indicate_netevent) {
+			mutex_unlock(&cnic_lock);
 			continue;
+		}
 
 		ctx = cp->ulp_handle[if_type];
 
+		set_bit(ULP_F_CALL_PENDING, &cp->ulp_flags[if_type]);
+		mutex_unlock(&cnic_lock);
+
 		ulp_ops->indicate_netevent(ctx, event, vlan_id);
+
+		clear_bit(ULP_F_CALL_PENDING, &cp->ulp_flags[if_type]);
 	}
-	rcu_read_unlock();
 }
 
 /* netdev event handler */
-- 
1.7.1

[PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Michael Chan]

We are allocating memory with GFP_KERNEL under spinlock.  Since this is
the only call manipulating the cnic_udev_list and it is always under
rtnl_lock, cnic_dev_lock can be safely removed.

Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/cnic.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index 7f40a2c..4ebab75 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
 	struct cnic_local *cp = dev->cnic_priv;
 	struct cnic_uio_dev *udev;
 
-	read_lock(&cnic_dev_lock);
 	list_for_each_entry(udev, &cnic_udev_list, list) {
 		if (udev->pdev == dev->pcidev) {
 			udev->dev = dev;
 			if (__cnic_alloc_uio_rings(udev, pages)) {
 				udev->dev = NULL;
-				read_unlock(&cnic_dev_lock);
 				return -ENOMEM;
 			}
 			cp->udev = udev;
-			read_unlock(&cnic_dev_lock);
 			return 0;
 		}
 	}
-	read_unlock(&cnic_dev_lock);
 
 	udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC);
 	if (!udev)
@@ -1067,9 +1063,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
 	if (__cnic_alloc_uio_rings(udev, pages))
 		goto err_udev;
 
-	write_lock(&cnic_dev_lock);
 	list_add(&udev->list, &cnic_udev_list);
-	write_unlock(&cnic_dev_lock);
 
 	pci_dev_get(udev->pdev);
 
-- 
1.7.1

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Benjamin Poirier]

On 2014/05/30 16:18, Michael Chan wrote:
> We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> the only call manipulating the cnic_udev_list and it is always under
> rtnl_lock, cnic_dev_lock can be safely removed.

In that case, the many other instances of cnic_dev_lock throughout cnic
should also be removed, no?

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Michael Chan]

On Fri, 2014-05-30 at 15:33 -0700, Benjamin Poirier wrote: 
> On 2014/05/30 16:18, Michael Chan wrote:
> > We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> > the only call manipulating the cnic_udev_list and it is always under
> > rtnl_lock, cnic_dev_lock can be safely removed.
> 
> In that case, the many other instances of cnic_dev_lock throughout cnic
> should also be removed, no? 

I don't think so.  cnic_dev_list still needs to be protected using
cnic_dev_lock.  cnic_register_driver() for example is not called with
rtnl_lock().

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Benjamin Poirier]

On 2014/06/02 13:31, Michael Chan wrote:
> On Fri, 2014-05-30 at 15:33 -0700, Benjamin Poirier wrote: 
> > On 2014/05/30 16:18, Michael Chan wrote:
> > > We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> > > the only call manipulating the cnic_udev_list and it is always under
> > > rtnl_lock, cnic_dev_lock can be safely removed.
> > 
> > In that case, the many other instances of cnic_dev_lock throughout cnic
> > should also be removed, no? 
> 
> I don't think so.  cnic_dev_list still needs to be protected using
> cnic_dev_lock.  cnic_register_driver() for example is not called with
> rtnl_lock().
> 

Ah, that's right. I had not paid attention to the fact that the same
lock protected cnic_dev_list and cnic_udev_list. Thanks for pointing it
out.

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Benjamin Poirier]

On 2014/05/30 16:18, Michael Chan wrote:
> We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> the only call manipulating the cnic_udev_list and it is always under
> rtnl_lock, cnic_dev_lock can be safely removed.
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/cnic.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
> index 7f40a2c..4ebab75 100644
> --- a/drivers/net/ethernet/broadcom/cnic.c
> +++ b/drivers/net/ethernet/broadcom/cnic.c
> @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
>  	struct cnic_local *cp = dev->cnic_priv;
>  	struct cnic_uio_dev *udev;
>  
> -	read_lock(&cnic_dev_lock);
>  	list_for_each_entry(udev, &cnic_udev_list, list) {
>  		if (udev->pdev == dev->pcidev) {
>  			udev->dev = dev;
>  			if (__cnic_alloc_uio_rings(udev, pages)) {
>  				udev->dev = NULL;
> -				read_unlock(&cnic_dev_lock);
>  				return -ENOMEM;
>  			}
>  			cp->udev = udev;
> -			read_unlock(&cnic_dev_lock);
>  			return 0;
>  		}
>  	}
> -	read_unlock(&cnic_dev_lock);
>  
>  	udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC);

Oh, and you could also make the above GFP_KERNEL. Sorry for not
mentioning it in my previous mail.

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Michael Chan]

On Fri, 2014-05-30 at 15:50 -0700, Benjamin Poirier wrote: 
> On 2014/05/30 16:18, Michael Chan wrote:
> > We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> > the only call manipulating the cnic_udev_list and it is always under
> > rtnl_lock, cnic_dev_lock can be safely removed.
> > 
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > ---
> >  drivers/net/ethernet/broadcom/cnic.c |    6 ------
> >  1 files changed, 0 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
> > index 7f40a2c..4ebab75 100644
> > --- a/drivers/net/ethernet/broadcom/cnic.c
> > +++ b/drivers/net/ethernet/broadcom/cnic.c
> > @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
> >  	struct cnic_local *cp = dev->cnic_priv;
> >  	struct cnic_uio_dev *udev;
> >  
> > -	read_lock(&cnic_dev_lock);
> >  	list_for_each_entry(udev, &cnic_udev_list, list) {
> >  		if (udev->pdev == dev->pcidev) {
> >  			udev->dev = dev;
> >  			if (__cnic_alloc_uio_rings(udev, pages)) {
> >  				udev->dev = NULL;
> > -				read_unlock(&cnic_dev_lock);
> >  				return -ENOMEM;
> >  			}
> >  			cp->udev = udev;
> > -			read_unlock(&cnic_dev_lock);
> >  			return 0;
> >  		}
> >  	}
> > -	read_unlock(&cnic_dev_lock);
> >  
> >  	udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC);
> 
> Oh, and you could also make the above GFP_KERNEL. Sorry for not
> mentioning it in my previous mail.

Thanks for pointing this out.  We can do that in a separate patch.  It
is related but not part of this bug fix.

[PATCH 3/3] cnic: Fix missing ISCSI_KEVENT_IF_DOWN message

[Michael Chan]

The iSCSI netlink message needs to be sent before the ulp_ops is cleared
as it is sent through a function pointer in the ulp_ops.  This bug
causes iscsid to not get the message when the bnx2i driver is unloaded.

Signed-off-by: Michael Chan <mchan@broadcom.com>
---
 drivers/net/ethernet/broadcom/cnic.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index 4ebab75..565fb31 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -608,6 +608,10 @@ static int cnic_unregister_device(struct cnic_dev *dev, int ulp_type)
 		pr_err("%s: Bad type %d\n", __func__, ulp_type);
 		return -EINVAL;
 	}
+
+	if (ulp_type == CNIC_ULP_ISCSI)
+		cnic_send_nlmsg(cp, ISCSI_KEVENT_IF_DOWN, NULL);
+
 	mutex_lock(&cnic_lock);
 	if (rcu_dereference(cp->ulp_ops[ulp_type])) {
 		RCU_INIT_POINTER(cp->ulp_ops[ulp_type], NULL);
@@ -620,9 +624,7 @@ static int cnic_unregister_device(struct cnic_dev *dev, int ulp_type)
 	}
 	mutex_unlock(&cnic_lock);
 
-	if (ulp_type == CNIC_ULP_ISCSI)
-		cnic_send_nlmsg(cp, ISCSI_KEVENT_IF_DOWN, NULL);
-	else if (ulp_type == CNIC_ULP_FCOE)
+	if (ulp_type == CNIC_ULP_FCOE)
 		dev->fcoe_cap = NULL;
 
 	synchronize_rcu();
-- 
1.7.1

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Neil Horman]

On Fri, May 30, 2014 at 04:18:42PM -0700, Michael Chan wrote:
> We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> the only call manipulating the cnic_udev_list and it is always under
> rtnl_lock, cnic_dev_lock can be safely removed.
> 
 I don't think this is accurate.  cnic_alloc_uio_rings seems to protect the list
with cnic_dev_lock, but has several paths (those calling ->alloc_resc()), that
never hold the rtnl_lock.

Neil

> Signed-off-by: Michael Chan <mchan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/cnic.c |    6 ------
>  1 files changed, 0 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
> index 7f40a2c..4ebab75 100644
> --- a/drivers/net/ethernet/broadcom/cnic.c
> +++ b/drivers/net/ethernet/broadcom/cnic.c
> @@ -1039,21 +1039,17 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
>  	struct cnic_local *cp = dev->cnic_priv;
>  	struct cnic_uio_dev *udev;
>  
> -	read_lock(&cnic_dev_lock);
>  	list_for_each_entry(udev, &cnic_udev_list, list) {
>  		if (udev->pdev == dev->pcidev) {
>  			udev->dev = dev;
>  			if (__cnic_alloc_uio_rings(udev, pages)) {
>  				udev->dev = NULL;
> -				read_unlock(&cnic_dev_lock);
>  				return -ENOMEM;
>  			}
>  			cp->udev = udev;
> -			read_unlock(&cnic_dev_lock);
>  			return 0;
>  		}
>  	}
> -	read_unlock(&cnic_dev_lock);
>  
>  	udev = kzalloc(sizeof(struct cnic_uio_dev), GFP_ATOMIC);
>  	if (!udev)
> @@ -1067,9 +1063,7 @@ static int cnic_alloc_uio_rings(struct cnic_dev *dev, int pages)
>  	if (__cnic_alloc_uio_rings(udev, pages))
>  		goto err_udev;
>  
> -	write_lock(&cnic_dev_lock);
>  	list_add(&udev->list, &cnic_udev_list);
> -	write_unlock(&cnic_dev_lock);
>  
>  	pci_dev_get(udev->pdev);
>  
> -- 
> 1.7.1
> 
> 

Re: [PATCH 2/3] cnic: Don't take cnic_dev_lock in cnic_alloc_uio_rings()

[Michael Chan]

On Sat, 2014-05-31 at 09:07 -0400, Neil Horman wrote: 
> On Fri, May 30, 2014 at 04:18:42PM -0700, Michael Chan wrote:
> > We are allocating memory with GFP_KERNEL under spinlock.  Since this is
> > the only call manipulating the cnic_udev_list and it is always under
> > rtnl_lock, cnic_dev_lock can be safely removed.
> > 
>  I don't think this is accurate.  cnic_alloc_uio_rings seems to protect the list
> with cnic_dev_lock, but has several paths (those calling ->alloc_resc()), that
> never hold the rtnl_lock.
> 
> 
->alloc_resc() is called by cnic_start_hw().  cnic_start_hw() is called
from 2 paths.  One is from netdev events which always hold rtnl_lock.
The other path is from bnx2/bnx2x with CNIC_CTL_START_CMD.  In bnx2,
this is called during bnx2_netif_start() which is always under
rtnl_lock.  In bnx2x, it is called during bnx2x_nic_load() which is also
under rtnl_lock().  Thanks.

Re: [PATCH 0/3] cnic fixes.

[David Miller]

From: Michael Chan <mchan@broadcom.com>
Date: Fri, 30 May 2014 16:18:40 -0700

> Fix 2 sleeping function from invalid context bugs and 1 missing iscsi netlink
> message bug.

Series applied, thanks Michael.

Re: [PATCH 0/3] cnic fixes.

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Mon, 02 Jun 2014 16:53:36 -0700 (PDT)

> From: Michael Chan <mchan@broadcom.com>
> Date: Fri, 30 May 2014 16:18:40 -0700
> 
>> Fix 2 sleeping function from invalid context bugs and 1 missing iscsi netlink
>> message bug.
> 
> Series applied, thanks Michael.

Michael, any chance you can compile test the changes you send to me?

drivers/net/ethernet/broadcom/cnic.c: In function ‘cnic_rcv_netevent’:
drivers/net/ethernet/broadcom/cnic.c:5628:13: error: ‘lockdep_is’ undeclared (first use in this function)
drivers/net/ethernet/broadcom/cnic.c:5628:13: note: each undeclared identifier is reported only once for each function it appears in
drivers/net/ethernet/broadcom/cnic.c:5628:13: error: expected ‘)’ before ‘held’

Re: [PATCH 0/3] cnic fixes.

[Michael Chan]

On Mon, 2014-06-02 at 16:57 -0700, David Miller wrote: 
> From: David Miller <davem@davemloft.net>
> Date: Mon, 02 Jun 2014 16:53:36 -0700 (PDT)
> 
> > From: Michael Chan <mchan@broadcom.com>
> > Date: Fri, 30 May 2014 16:18:40 -0700
> > 
> >> Fix 2 sleeping function from invalid context bugs and 1 missing iscsi netlink
> >> message bug.
> > 
> > Series applied, thanks Michael.
> 
> Michael, any chance you can compile test the changes you send to me?
> 
> drivers/net/ethernet/broadcom/cnic.c: In function ʽcnic_rcv_neteventʼ:
> drivers/net/ethernet/broadcom/cnic.c:5628:13: error: ʽlockdep_isʼ undeclared (first use in this function)
> drivers/net/ethernet/broadcom/cnic.c:5628:13: note: each undeclared identifier is reported only once for each function it appears in
> drivers/net/ethernet/broadcom/cnic.c:5628:13: error: expected ʽ)ʼ before ʽheldʼ

Sorry, I think LOCKDEP was not defined in my kernel.  I'll get it fixed
and resend the patches.