[PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[Anil Veerabhadrappa]

>From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17 00:00:00 2001
From: Anil Veerabhadrappa <anilgv@broadcom.com>
Date: Tue, 23 Jun 2009 13:56:29 -0700
Subject: [PATCH]     bnx2i - remove global variable bnx2i_reg_devices

    * Removed bnx2i_reg_devices as this counter is not really
      used in a meaningful way

    Cc: Mike Christie <michaelc@cs.wisc.edu>

Signed-off-by: Michael Chan <mchan@broadcom.com>
Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
---
 drivers/scsi/bnx2i/bnx2i_init.c |   20 +-------------------
 1 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_init.c b/drivers/scsi/bnx2i/bnx2i_init.c
index ae4b2d5..269192d 100644
--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -15,7 +15,6 @@
 
 static struct list_head adapter_list = LIST_HEAD_INIT(adapter_list);
 static u32 adapter_count;
-static int bnx2i_reg_device;
 
 #define DRV_MODULE_NAME		"bnx2i"
 #define DRV_MODULE_VERSION	"2.0.1d"
@@ -193,10 +192,6 @@ void bnx2i_register_device(struct bnx2i_hba *hba)
 
 	hba->cnic->register_device(hba->cnic, CNIC_ULP_ISCSI, hba);
 
-	spin_lock(&hba->lock);
-	bnx2i_reg_device++;
-	spin_unlock(&hba->lock);
-
 	set_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
 }
 
@@ -234,10 +229,6 @@ static void bnx2i_unreg_one_device(struct bnx2i_hba *hba)
 
 	hba->cnic->unregister_device(hba->cnic, CNIC_ULP_ISCSI);
 
-	spin_lock(&hba->lock);
-	bnx2i_reg_device--;
-	spin_unlock(&hba->lock);
-
 	/* ep_disconnect could come before NETDEV_DOWN, driver won't
 	 * see NETDEV_DOWN as it already unregistered itself.
 	 */
@@ -276,16 +267,12 @@ static int bnx2i_init_one(struct bnx2i_hba *hba, struct cnic_dev *cnic)
 	int rc;
 
 	read_lock(&bnx2i_dev_lock);
-	if (bnx2i_reg_device &&
-	    !test_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic)) {
+	if (!test_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic)) {
 		rc = cnic->register_device(cnic, CNIC_ULP_ISCSI, hba);
 		if (rc)		/* duplicate registration */
 			printk(KERN_ERR "bnx2i- dev reg failed\n");
 
-		spin_lock(&hba->lock);
-		bnx2i_reg_device++;
 		hba->age++;
-		spin_unlock(&hba->lock);
 
 		set_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
 	}
@@ -350,10 +337,6 @@ void bnx2i_ulp_exit(struct cnic_dev *dev)
 	if (test_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic)) {
 		hba->cnic->unregister_device(hba->cnic, CNIC_ULP_ISCSI);
 		clear_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
-
-		spin_lock(&hba->lock);
-		bnx2i_reg_device--;
-		spin_unlock(&hba->lock);
 	}
 	write_unlock(&bnx2i_dev_lock);
 
@@ -421,7 +404,6 @@ static void __exit bnx2i_mod_exit(void)
 		if (test_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic)) {
 			hba->cnic->unregister_device(hba->cnic, CNIC_ULP_ISCSI);
 			clear_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
-			bnx2i_reg_device--;
 		}
 
 		write_unlock(&bnx2i_dev_lock);
-- 
1.5.4.3

Re: [PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[Mike Christie]

On 06/23/2009 08:11 PM, Anil Veerabhadrappa wrote:
>> From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17 00:00:00 2001
> From: Anil Veerabhadrappa<anilgv@broadcom.com>
> Date: Tue, 23 Jun 2009 13:56:29 -0700
> Subject: [PATCH]     bnx2i - remove global variable bnx2i_reg_devices
>
>      * Removed bnx2i_reg_devices as this counter is not really
>        used in a meaningful way
>
>      Cc: Mike Christie<michaelc@cs.wisc.edu>
>
> Signed-off-by: Michael Chan<mchan@broadcom.com>
> Signed-off-by: Anil Veerabhadrappa<anilgv@broadcom.com>

Thanks for finishing up this cleanup from the review.

All patches

Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[James Bottomley]

On Tue, 2009-06-23 at 18:11 -0700, Anil Veerabhadrappa wrote:
> >From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17 00:00:00 2001
> From: Anil Veerabhadrappa <anilgv@broadcom.com>
> Date: Tue, 23 Jun 2009 13:56:29 -0700
> Subject: [PATCH]     bnx2i - remove global variable bnx2i_reg_devices
> 
>     * Removed bnx2i_reg_devices as this counter is not really
>       used in a meaningful way
>     Cc: Mike Christie <michaelc@cs.wisc.edu>
> 
> Signed-off-by: Michael Chan <mchan@broadcom.com>
> Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>

Tell me what these signoffs mean:  The first signoff by Michael Chan
implies it's his patch, but it has your authorship information.

James

Re: [PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[Michael Chan]

James Bottomley wrote:

> On Tue, 2009-06-23 at 18:11 -0700, Anil Veerabhadrappa wrote:
> > >From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17
> 00:00:00 2001
> > From: Anil Veerabhadrappa <anilgv@broadcom.com>
> > Date: Tue, 23 Jun 2009 13:56:29 -0700
> > Subject: [PATCH]     bnx2i - remove global variable
> bnx2i_reg_devices
> >
> >     * Removed bnx2i_reg_devices as this counter is not really
> >       used in a meaningful way
> >     Cc: Mike Christie <michaelc@cs.wisc.edu>
> >
> > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
>
> Tell me what these signoffs mean:  The first signoff by Michael Chan
> implies it's his patch, but it has your authorship information.
>

Sorry, Anil should have put his signoff first.  Do we need to re-spin
the pacthes?

Re: [PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[James Bottomley]

On Sun, 2009-06-28 at 08:46 -0700, Michael Chan wrote:
> James Bottomley wrote:
> 
> > On Tue, 2009-06-23 at 18:11 -0700, Anil Veerabhadrappa wrote:
> > > >From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17
> > 00:00:00 2001
> > > From: Anil Veerabhadrappa <anilgv@broadcom.com>
> > > Date: Tue, 23 Jun 2009 13:56:29 -0700
> > > Subject: [PATCH]     bnx2i - remove global variable
> > bnx2i_reg_devices
> > >
> > >     * Removed bnx2i_reg_devices as this counter is not really
> > >       used in a meaningful way
> > >     Cc: Mike Christie <michaelc@cs.wisc.edu>
> > >
> > > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > > Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
> >
> > Tell me what these signoffs mean:  The first signoff by Michael Chan
> > implies it's his patch, but it has your authorship information.
> >
> 
> Sorry, Anil should have put his signoff first.  Do we need to re-spin
> the pacthes?

So did you actually contribute to these?  In which case a note saying
something like 

[mchan: did X to the driver]

is useful ... otherwise if you're just confirming as maintainer it
should be acked-by (unless you send me the patches yourself).

James

Re: [PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[Michael Chan]

On Sun, 2009-06-28 at 10:25 -0700, James Bottomley wrote:
> On Sun, 2009-06-28 at 08:46 -0700, Michael Chan wrote:
> > James Bottomley wrote:
> > 
> > > On Tue, 2009-06-23 at 18:11 -0700, Anil Veerabhadrappa wrote:
> > > > >From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17
> > > 00:00:00 2001
> > > > From: Anil Veerabhadrappa <anilgv@broadcom.com>
> > > > Date: Tue, 23 Jun 2009 13:56:29 -0700
> > > > Subject: [PATCH]     bnx2i - remove global variable
> > > bnx2i_reg_devices
> > > >
> > > >     * Removed bnx2i_reg_devices as this counter is not really
> > > >       used in a meaningful way
> > > >     Cc: Mike Christie <michaelc@cs.wisc.edu>
> > > >
> > > > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > > > Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
> > >
> > > Tell me what these signoffs mean:  The first signoff by Michael Chan
> > > implies it's his patch, but it has your authorship information.
> > >
> > 
> > Sorry, Anil should have put his signoff first.  Do we need to re-spin
> > the pacthes?
> 
> So did you actually contribute to these?  In which case a note saying
> something like 
> 
> [mchan: did X to the driver]

Here's one example of what I did:

[mchan: bnx2i_init_one() - don't add hba to adater_list if
->register_device fails ]

In general, all our patchsets go through some internal review and
testing by others besides the author.  We use multiple signoffs to
include everyone who has made some contributions to the patchset.  We'll
try to be more specific about who did what in the future. 

Thanks.

> 
> is useful ... otherwise if you're just confirming as maintainer it
> should be acked-by (unless you send me the patches yourself).
> 

Re: [PATCH 1/3] bnx2i remove not so useful global variable bnx2i_reg_devices

[James Bottomley]

On Sun, 2009-06-28 at 12:59 -0700, Michael Chan wrote:
> On Sun, 2009-06-28 at 10:25 -0700, James Bottomley wrote:
> > On Sun, 2009-06-28 at 08:46 -0700, Michael Chan wrote:
> > > James Bottomley wrote:
> > > 
> > > > On Tue, 2009-06-23 at 18:11 -0700, Anil Veerabhadrappa wrote:
> > > > > >From a8267738e9ca5c73ad6c0d89f17cf864bfc908c8 Mon Sep 17
> > > > 00:00:00 2001
> > > > > From: Anil Veerabhadrappa <anilgv@broadcom.com>
> > > > > Date: Tue, 23 Jun 2009 13:56:29 -0700
> > > > > Subject: [PATCH]     bnx2i - remove global variable
> > > > bnx2i_reg_devices
> > > > >
> > > > >     * Removed bnx2i_reg_devices as this counter is not really
> > > > >       used in a meaningful way
> > > > >     Cc: Mike Christie <michaelc@cs.wisc.edu>
> > > > >
> > > > > Signed-off-by: Michael Chan <mchan@broadcom.com>
> > > > > Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
> > > >
> > > > Tell me what these signoffs mean:  The first signoff by Michael Chan
> > > > implies it's his patch, but it has your authorship information.
> > > >
> > > 
> > > Sorry, Anil should have put his signoff first.  Do we need to re-spin
> > > the pacthes?
> > 
> > So did you actually contribute to these?  In which case a note saying
> > something like 
> > 
> > [mchan: did X to the driver]
> 
> Here's one example of what I did:
> 
> [mchan: bnx2i_init_one() - don't add hba to adater_list if
> ->register_device fails ]
> 
> In general, all our patchsets go through some internal review and
> testing by others besides the author.  We use multiple signoffs to
> include everyone who has made some contributions to the patchset.  We'll
> try to be more specific about who did what in the future. 

That's fine ... and including what they actually did with square
brackets is a good way of reminding me that the usage is correct.

Many people use signed-off-by to award recognition, but that's not what
they're for: they're there because of SCO and the need to track patches
back to sources according to the Developer Certificate of Origin.  Under
the DCO a signed-off-by means the person in the signoff modified the
patch in some way (or retransmitted it).

James

[PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Anil Veerabhadrappa]

    * When using iface, bnx2i was unable to offload further connections after
      all active sessions are logged out. bnx2i will unregister the device from
      cnic when the last connection is torn down. Next call to ep_connect()
      will fail because the device is not registered. This issue is not seen
      if shost == NULL is passed to ep_connect() call because in that case bnx2i
      will registers all known devices with cnic before doing a route look-up.
      When shost != NULL, bnx2i knows the device on which to offload the
      connection and has to register this device before attempting to offload
      the connection

Signed-off-by: Anil Veerabhadrappa <anilgv-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
Reviewed-by: Michael Chan <mchan-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>
---
 drivers/scsi/bnx2i/bnx2i_init.c  |    7 +++++--
 drivers/scsi/bnx2i/bnx2i_iscsi.c |    7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_init.c b/drivers/scsi/bnx2i/bnx2i_init.c
index fd78540..0c4210d 100644
--- a/drivers/scsi/bnx2i/bnx2i_init.c
+++ b/drivers/scsi/bnx2i/bnx2i_init.c
@@ -185,14 +185,17 @@ void bnx2i_stop(void *handle)
  */
 void bnx2i_register_device(struct bnx2i_hba *hba)
 {
+	int rc;
+
 	if (test_bit(ADAPTER_STATE_GOING_DOWN, &hba->adapter_state) ||
 	    test_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic)) {
 		return;
 	}
 
-	hba->cnic->register_device(hba->cnic, CNIC_ULP_ISCSI, hba);
+	rc = hba->cnic->register_device(hba->cnic, CNIC_ULP_ISCSI, hba);
 
-	set_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
+	if (!rc)
+		set_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
 }
 
 
diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index f741219..98148f3 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1653,15 +1653,18 @@ static struct iscsi_endpoint *bnx2i_ep_connect(struct Scsi_Host *shost,
 	struct iscsi_endpoint *ep;
 	int rc = 0;
 
-	if (shost)
+	if (shost) {
 		/* driver is given scsi host to work with */
 		hba = iscsi_host_priv(shost);
-	else
+		/* Register the device with cnic if not already done so */
+		bnx2i_register_device(hba);
+	} else
 		/*
 		 * check if the given destination can be reached through
 		 * a iscsi capable NetXtreme2 device
 		 */
 		hba = bnx2i_check_route(dst_addr);
+
 	if (!hba) {
 		rc = -ENOMEM;
 		goto check_busy;
-- 
1.5.4.3

Re: [PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Mike Christie]

Anil Veerabhadrappa wrote:
>     * When using iface, bnx2i was unable to offload further connections after
>       all active sessions are logged out. bnx2i will unregister the device from
>       cnic when the last connection is torn down. Next call to ep_connect()
>       will fail because the device is not registered. This issue is not seen
>       if shost == NULL is passed to ep_connect() call because in that case bnx2i
>       will registers all known devices with cnic before doing a route look-up.
>       When shost != NULL, bnx2i knows the device on which to offload the
>       connection and has to register this device before attempting to offload
>       the connection
> 
> Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
> Reviewed-by: Michael Chan <mchan@broadcom.com>
> ---
>  drivers/scsi/bnx2i/bnx2i_init.c  |    7 +++++--
>  drivers/scsi/bnx2i/bnx2i_iscsi.c |    7 +++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/bnx2i/bnx2i_init.c b/drivers/scsi/bnx2i/bnx2i_init.c
> index fd78540..0c4210d 100644
> --- a/drivers/scsi/bnx2i/bnx2i_init.c
> +++ b/drivers/scsi/bnx2i/bnx2i_init.c
> @@ -185,14 +185,17 @@ void bnx2i_stop(void *handle)
>   */
>  void bnx2i_register_device(struct bnx2i_hba *hba)
>  {
> +	int rc;
> +
>  	if (test_bit(ADAPTER_STATE_GOING_DOWN, &hba->adapter_state) ||
>  	    test_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic)) {
>  		return;
>  	}
>  
> -	hba->cnic->register_device(hba->cnic, CNIC_ULP_ISCSI, hba);
> +	rc = hba->cnic->register_device(hba->cnic, CNIC_ULP_ISCSI, hba);
>  
> -	set_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
> +	if (!rc)
> +		set_bit(BNX2I_CNIC_REGISTERED, &hba->reg_with_cnic);
>  }
>  
>  
> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> index f741219..98148f3 100644
> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> @@ -1653,15 +1653,18 @@ static struct iscsi_endpoint *bnx2i_ep_connect(struct Scsi_Host *shost,
>  	struct iscsi_endpoint *ep;
>  	int rc = 0;
>  
> -	if (shost)
> +	if (shost) {
>  		/* driver is given scsi host to work with */
>  		hba = iscsi_host_priv(shost);
> -	else
> +		/* Register the device with cnic if not already done so */
> +		bnx2i_register_device(hba);
> +	} else
>  		/*
>  		 * check if the given destination can be reached through
>  		 * a iscsi capable NetXtreme2 device
>  		 */
>  		hba = bnx2i_check_route(dst_addr);
> +
>  	if (!hba) {
>  		rc = -ENOMEM;
>  		goto check_busy;

Do you want to do the test_bit(BNX2I_CNIC_REGISTERED, 
&hba->reg_with_cnic) test here instead of down below. If it failed then 
you might try to do a cm_create on a cnic that is not properly registered.

Re: [PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Michael Chan]

Mike Christie wrote:
> Anil Veerabhadrappa wrote:
> > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > index f741219..98148f3 100644
> > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > @@ -1653,15 +1653,18 @@ static struct iscsi_endpoint
> *bnx2i_ep_connect(struct Scsi_Host *shost,
> >     struct iscsi_endpoint *ep;
> >     int rc = 0;
> >
> > -   if (shost)
> > +   if (shost) {
> >             /* driver is given scsi host to work with */
> >             hba = iscsi_host_priv(shost);
> > -   else
> > +           /* Register the device with cnic if not already
> done so */
> > +           bnx2i_register_device(hba);
> > +   } else
> >             /*
> >              * check if the given destination can be reached through
> >              * a iscsi capable NetXtreme2 device
> >              */
> >             hba = bnx2i_check_route(dst_addr);
> > +
> >     if (!hba) {
> >             rc = -ENOMEM;
> >             goto check_busy;
>
> Do you want to do the test_bit(BNX2I_CNIC_REGISTERED,
> &hba->reg_with_cnic) test here instead of down below. If it
> failed then
> you might try to do a cm_create on a cnic that is not
> properly registered.
>

Good idea.  cm_create() will properly be ok because there is no
hardware interaction.  bnx2i_send_conn_ofld_req() will fail if
the cnic is not properly registered because there will be no call
backs from the hardware events.

Re: [PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Anil Veerabhadrappa]

On Thu, 2009-07-09 at 00:15 -0700, Michael Chan wrote:
> Mike Christie wrote:
> > Anil Veerabhadrappa wrote:
> > > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > > index f741219..98148f3 100644
> > > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> > > @@ -1653,15 +1653,18 @@ static struct iscsi_endpoint
> > *bnx2i_ep_connect(struct Scsi_Host *shost,
> > >     struct iscsi_endpoint *ep;
> > >     int rc = 0;
> > >
> > > -   if (shost)
> > > +   if (shost) {
> > >             /* driver is given scsi host to work with */
> > >             hba = iscsi_host_priv(shost);
> > > -   else
> > > +           /* Register the device with cnic if not already
> > done so */
> > > +           bnx2i_register_device(hba);
> > > +   } else
> > >             /*
> > >              * check if the given destination can be reached through
> > >              * a iscsi capable NetXtreme2 device
> > >              */
> > >             hba = bnx2i_check_route(dst_addr);
> > > +
> > >     if (!hba) {
> > >             rc = -ENOMEM;
> > >             goto check_busy;
> >
> > Do you want to do the test_bit(BNX2I_CNIC_REGISTERED,
> > &hba->reg_with_cnic) test here instead of down below. If it
> > failed then
> > you might try to do a cm_create on a cnic that is not
> > properly registered.
> >
> 
> Good idea.  cm_create() will properly be ok because there is no
> hardware interaction.  bnx2i_send_conn_ofld_req() will fail if
> the cnic is not properly registered because there will be no call
> backs from the hardware events.

following 2 conditions needs to be satisfied to offload an iscsi
connection,
  1) device has to be registered with cnic
  2) device has to support iscsi offload

  bnx2i_adapter_ready() will return success only if both conditions
(which is flagged by ADAPTER_STATE_UP bit in hba->adapter_state) are
met. bnx2i_ep_connect() will bailout if bnx2i_adapter_ready() does not
return correct status and this guarantees bnx2i will not make any cnic
calls on an unregistered device.

Re: [PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Mike Christie]

On 07/09/2009 09:39 AM, Anil Veerabhadrappa wrote:
> On Thu, 2009-07-09 at 00:15 -0700, Michael Chan wrote:
>> Mike Christie wrote:
>>> Anil Veerabhadrappa wrote:
>>>> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c
>>> b/drivers/scsi/bnx2i/bnx2i_iscsi.c
>>>> index f741219..98148f3 100644
>>>> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
>>>> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
>>>> @@ -1653,15 +1653,18 @@ static struct iscsi_endpoint
>>> *bnx2i_ep_connect(struct Scsi_Host *shost,
>>>>      struct iscsi_endpoint *ep;
>>>>      int rc = 0;
>>>>
>>>> -   if (shost)
>>>> +   if (shost) {
>>>>              /* driver is given scsi host to work with */
>>>>              hba = iscsi_host_priv(shost);
>>>> -   else
>>>> +           /* Register the device with cnic if not already
>>> done so */
>>>> +           bnx2i_register_device(hba);
>>>> +   } else
>>>>              /*
>>>>               * check if the given destination can be reached through
>>>>               * a iscsi capable NetXtreme2 device
>>>>               */
>>>>              hba = bnx2i_check_route(dst_addr);
>>>> +
>>>>      if (!hba) {
>>>>              rc = -ENOMEM;
>>>>              goto check_busy;
>>> Do you want to do the test_bit(BNX2I_CNIC_REGISTERED,
>>> &hba->reg_with_cnic) test here instead of down below. If it
>>> failed then
>>> you might try to do a cm_create on a cnic that is not
>>> properly registered.
>>>
>> Good idea.  cm_create() will properly be ok because there is no
>> hardware interaction.  bnx2i_send_conn_ofld_req() will fail if
>> the cnic is not properly registered because there will be no call
>> backs from the hardware events.
>
> following 2 conditions needs to be satisfied to offload an iscsi
> connection,
>    1) device has to be registered with cnic
>    2) device has to support iscsi offload
>
>    bnx2i_adapter_ready() will return success only if both conditions
> (which is flagged by ADAPTER_STATE_UP bit in hba->adapter_state) are
> met. bnx2i_ep_connect() will bailout if bnx2i_adapter_ready() does not
> return correct status and this guarantees bnx2i will not make any cnic
> calls on an unregistered device.
>

Ok then. It seems fine to me.

Reviewed-by Mike Christie <michaelc@cs.wisc.edu>

One other question though. Do you need the BNX2I_CNIC_REGISTERED tst in 
ep_connect then? If not then maybe in separate patch for the next 
feature window we can clean that up.

Re: [PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Anil Veerabhadrappa]

On Thu, 2009-07-09 at 10:43 -0700, Mike Christie wrote:
> On 07/09/2009 09:39 AM, Anil Veerabhadrappa wrote:
> > On Thu, 2009-07-09 at 00:15 -0700, Michael Chan wrote:
> >> Mike Christie wrote:
> >>> Anil Veerabhadrappa wrote:
> >>>> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>> b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>>> index f741219..98148f3 100644
> >>>> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>>> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>>> @@ -1653,15 +1653,18 @@ static struct iscsi_endpoint
> >>> *bnx2i_ep_connect(struct Scsi_Host *shost,
> >>>>      struct iscsi_endpoint *ep;
> >>>>      int rc = 0;
> >>>>
> >>>> -   if (shost)
> >>>> +   if (shost) {
> >>>>              /* driver is given scsi host to work with */
> >>>>              hba = iscsi_host_priv(shost);
> >>>> -   else
> >>>> +           /* Register the device with cnic if not already
> >>> done so */
> >>>> +           bnx2i_register_device(hba);
> >>>> +   } else
> >>>>              /*
> >>>>               * check if the given destination can be reached through
> >>>>               * a iscsi capable NetXtreme2 device
> >>>>               */
> >>>>              hba = bnx2i_check_route(dst_addr);
> >>>> +
> >>>>      if (!hba) {
> >>>>              rc = -ENOMEM;
> >>>>              goto check_busy;
> >>> Do you want to do the test_bit(BNX2I_CNIC_REGISTERED,
> >>> &hba->reg_with_cnic) test here instead of down below. If it
> >>> failed then
> >>> you might try to do a cm_create on a cnic that is not
> >>> properly registered.
> >>>
> >> Good idea.  cm_create() will properly be ok because there is no
> >> hardware interaction.  bnx2i_send_conn_ofld_req() will fail if
> >> the cnic is not properly registered because there will be no call
> >> backs from the hardware events.
> >
> > following 2 conditions needs to be satisfied to offload an iscsi
> > connection,
> >    1) device has to be registered with cnic
> >    2) device has to support iscsi offload
> >
> >    bnx2i_adapter_ready() will return success only if both conditions
> > (which is flagged by ADAPTER_STATE_UP bit in hba->adapter_state) are
> > met. bnx2i_ep_connect() will bailout if bnx2i_adapter_ready() does not
> > return correct status and this guarantees bnx2i will not make any cnic
> > calls on an unregistered device.
> >
> 
> Ok then. It seems fine to me.
> 
> Reviewed-by Mike Christie <michaelc@cs.wisc.edu>
> 
> One other question though. Do you need the BNX2I_CNIC_REGISTERED tst in 
> ep_connect then? If not then maybe in separate patch for the next 
> feature window we can clean that up.
> 
> 
It is not required to test for BNX2I_CNIC_REGISTERED because a
successful return from bnx2i_adapter_ready() implicitly means the device
is registered with cnic

Re: [PATCH] BNX2I: register given device with cnic if shost != NULL in ep_connect()

[Anil Veerabhadrappa]

On Thu, 2009-07-09 at 10:43 -0700, Mike Christie wrote:
> On 07/09/2009 09:39 AM, Anil Veerabhadrappa wrote:
> > On Thu, 2009-07-09 at 00:15 -0700, Michael Chan wrote:
> >> Mike Christie wrote:
> >>> Anil Veerabhadrappa wrote:
> >>>> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>> b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>>> index f741219..98148f3 100644
> >>>> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>>> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> >>>> @@ -1653,15 +1653,18 @@ static struct iscsi_endpoint
> >>> *bnx2i_ep_connect(struct Scsi_Host *shost,
> >>>>      struct iscsi_endpoint *ep;
> >>>>      int rc = 0;
> >>>>
> >>>> -   if (shost)
> >>>> +   if (shost) {
> >>>>              /* driver is given scsi host to work with */
> >>>>              hba = iscsi_host_priv(shost);
> >>>> -   else
> >>>> +           /* Register the device with cnic if not already
> >>> done so */
> >>>> +           bnx2i_register_device(hba);
> >>>> +   } else
> >>>>              /*
> >>>>               * check if the given destination can be reached through
> >>>>               * a iscsi capable NetXtreme2 device
> >>>>               */
> >>>>              hba = bnx2i_check_route(dst_addr);
> >>>> +
> >>>>      if (!hba) {
> >>>>              rc = -ENOMEM;
> >>>>              goto check_busy;
> >>> Do you want to do the test_bit(BNX2I_CNIC_REGISTERED,
> >>> &hba->reg_with_cnic) test here instead of down below. If it
> >>> failed then
> >>> you might try to do a cm_create on a cnic that is not
> >>> properly registered.
> >>>
> >> Good idea.  cm_create() will properly be ok because there is no
> >> hardware interaction.  bnx2i_send_conn_ofld_req() will fail if
> >> the cnic is not properly registered because there will be no call
> >> backs from the hardware events.
> >
> > following 2 conditions needs to be satisfied to offload an iscsi
> > connection,
> >    1) device has to be registered with cnic
> >    2) device has to support iscsi offload
> >
> >    bnx2i_adapter_ready() will return success only if both conditions
> > (which is flagged by ADAPTER_STATE_UP bit in hba->adapter_state) are
> > met. bnx2i_ep_connect() will bailout if bnx2i_adapter_ready() does not
> > return correct status and this guarantees bnx2i will not make any cnic
> > calls on an unregistered device.
> >
> 
> Ok then. It seems fine to me.
> 
> Reviewed-by Mike Christie <michaelc@cs.wisc.edu>
> 
> One other question though. Do you need the BNX2I_CNIC_REGISTERED tst in 
> ep_connect then? If not then maybe in separate patch for the next 
> feature window we can clean that up.
> 
> 
I agree there is no incentive to do the extra work if device
registration failed. We will take care of this in the next feature
window.

Thanks!!

[PATCH 1/2] bnx2i : Fix CMDSN jump issue seen during cable pull test

[Anil Veerabhadrappa]

    * Without the fix bnx2i would fail tt->xmit_task() when link is down
    * libiscsi would have already incremented session->cmdsn before calling
      bnx2i's xmit_task() entry point and will just return the command to
      SCSI-ML when xmit_task() fails. libiscsi does not retract the session->cmdsn
      as the command was never sent on wire
    * It is generally good idea for LLD, bnx2i to accept the scsi cmnd/nopout
      and let upper layer timeout and go though normal session recovery process
    * When link is down, unsolicited nopout will not be accepted by bnx2i and
      connection will never enter recovery state. This fix is required for MPIO
      to work corectly

Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
---
 drivers/scsi/bnx2i/bnx2i_iscsi.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
index f741219..9535bb6 100644
--- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
+++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
@@ -1160,9 +1160,6 @@ static int bnx2i_task_xmit(struct iscsi_task *task)
 	struct bnx2i_cmd *cmd = task->dd_data;
 	struct iscsi_cmd *hdr = (struct iscsi_cmd *) task->hdr;
 
-	if (test_bit(ADAPTER_STATE_LINK_DOWN, &hba->adapter_state))
-		return -ENOTCONN;
-
 	if (!bnx2i_conn->is_bound)
 		return -ENOTCONN;
 
-- 
1.5.4.3

Re: [PATCH 1/2] bnx2i : Fix CMDSN jump issue seen during cable pull test

[Mike Christie]

Anil Veerabhadrappa wrote:
> 
>     * Without the fix bnx2i would fail tt->xmit_task() when link is down
>     * libiscsi would have already incremented session->cmdsn before calling
>       bnx2i's xmit_task() entry point and will just return the command to
>       SCSI-ML when xmit_task() fails. libiscsi does not retract the session->cmdsn
>       as the command was never sent on wire

I fixed this in libiscsi.

>     * It is generally good idea for LLD, bnx2i to accept the scsi cmnd/nopout
>       and let upper layer timeout and go though normal session recovery process
>     * When link is down, unsolicited nopout will not be accepted by bnx2i and
>       connection will never enter recovery state. This fix is required for MPIO
>       to work corectly
> 
> Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
> ---
>  drivers/scsi/bnx2i/bnx2i_iscsi.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> index f741219..9535bb6 100644
> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c
> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c
> @@ -1160,9 +1160,6 @@ static int bnx2i_task_xmit(struct iscsi_task *task)
>  	struct bnx2i_cmd *cmd = task->dd_data;
>  	struct iscsi_cmd *hdr = (struct iscsi_cmd *) task->hdr;
>  
> -	if (test_bit(ADAPTER_STATE_LINK_DOWN, &hba->adapter_state))
> -		return -ENOTCONN;
> -
>  	if (!bnx2i_conn->is_bound)
>  		return -ENOTCONN;
>  

Looks ok.

Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

[PATCH 1/1] BNX2I - Fix context mapping issue for architectures with PAGE_SIZE != 4096

[Anil Veerabhadrappa]

    [BNX2I] Fix context mapping issue for architectures with PAGE_SIZE != 4096

    * 5706/5708/5709 devices allow driver/user to set page size. By default
      it is set to 4096
    * Current drivers do not program this register based on architecture type
      (e.g. x86 = 4K, IA64 = 16K) and by choice lets device use the defaults
    * So while mapping connection context memory (doorebll registers),
      driver has to match page size used by the device. Included change fixes
      the issue we uncovered during IA64 testing

Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
---
 drivers/scsi/bnx2i/bnx2i.h     |    2 ++
 drivers/scsi/bnx2i/bnx2i_hwi.c |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/bnx2i/bnx2i.h b/drivers/scsi/bnx2i/bnx2i.h
index d7576f2..5edde1a 100644
--- a/drivers/scsi/bnx2i/bnx2i.h
+++ b/drivers/scsi/bnx2i/bnx2i.h
@@ -100,6 +100,8 @@
 #define CTX_OFFSET 			0x10000
 #define MAX_CID_CNT			0x4000
 
+#define BNX2I_570X_PAGE_SIZE_DEFAULT	4096
+
 /* 5709 context registers */
 #define BNX2_MQ_CONFIG2			0x00003d00
 #define BNX2_MQ_CONFIG2_CONT_SZ		(0x7L<<4)
diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
index 906cef5..0afde71 100644
--- a/drivers/scsi/bnx2i/bnx2i_hwi.c
+++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
@@ -2386,7 +2386,7 @@ int bnx2i_map_ep_dbell_regs(struct bnx2i_endpoint *ep)
 		ctx_sz = (config2 & BNX2_MQ_CONFIG2_CONT_SZ) >> 3;
 		if (ctx_sz)
 			reg_off = CTX_OFFSET + MAX_CID_CNT * MB_KERNEL_CTX_SIZE
-				  + PAGE_SIZE *
+				  + BNX2I_570X_PAGE_SIZE_DEFAULT *
 				  (((cid_num - first_l4l5) / ctx_sz) + 256);
 		else
 			reg_off = CTX_OFFSET + (MB_KERNEL_CTX_SIZE * cid_num);
-- 
1.5.4.3