diff mbox

[v4,17/32] cxlflash: Remove dual port online dependency

Message ID 1443223018-9577-1-git-send-email-mrochs@linux.vnet.ibm.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Matthew R. Ochs Sept. 25, 2015, 11:16 p.m. UTC
At present, both ports must be online for the device to
configure properly. Remove this dependency and the unnecessary
internal LUN override logic as well. Additionally, as a refactoring
measure, change the return code variable name to match that used
throughout the driver.

Signed-off-by: Matthew R. Ochs <mrochs@linux.vnet.ibm.com>
Signed-off-by: Manoj N. Kumar <manoj@linux.vnet.ibm.com>
Reviewed-by: Brian King <brking@linux.vnet.ibm.com>
---
 drivers/scsi/cxlflash/main.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

Comments

Daniel Axtens Sept. 28, 2015, 11:37 p.m. UTC | #1
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Hi,

>  static int afu_set_wwpn(struct afu *afu, int port, u64 *fc_regs, u64 wwpn)
>  {
> -	int ret = 0;
> +	int rc = 0;
I realise it's nice to have things consistent, but making this change
now makes the rest of the patch quite difficult to follow.

>  
>  	set_port_offline(fc_regs);
>  
> @@ -1038,33 +1038,26 @@ static int afu_set_wwpn(struct afu *afu, int port, u64 *fc_regs, u64 wwpn)
>  			       FC_PORT_STATUS_RETRY_CNT)) {
>  		pr_debug("%s: wait on port %d to go offline timed out\n",
>  			 __func__, port);
> -		ret = -1; /* but continue on to leave the port back online */
> +		rc = -1; /* but continue on to leave the port back online */
>  	}
>  
> -	if (ret == 0)
> +	if (rc == 0)
>  		writeq_be(wwpn, &fc_regs[FC_PNAME / 8]);
>  
> +	/* Always return success after programming WWPN */
> +	rc = 0;
> +
>  	set_port_online(fc_regs);
>  
>  	if (!wait_port_online(fc_regs, FC_PORT_STATUS_RETRY_INTERVAL_US,
>  			      FC_PORT_STATUS_RETRY_CNT)) {
>  		pr_debug("%s: wait on port %d to go online timed out\n",
>  			 __func__, port);
> -		ret = -1;
> -
> -		/*
> -		 * Override for internal lun!!!
> -		 */
> -		if (afu->internal_lun) {
> -			pr_debug("%s: Overriding port %d online timeout!!!\n",
> -				 __func__, port);
> -			ret = 0;
> -		}
>  	}
>  
> -	pr_debug("%s: returning rc=%d\n", __func__, ret);
> +	pr_debug("%s: returning rc=%d\n", __func__, rc);
I'm not sure I fully understand the flow of this function, but it looks
like you set rc=0 regardless of how things actually go: is this ever
going to print a return value other than zero?

Regards,
Daniel
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: GPGTools - https://gpgtools.org

iQIcBAEBCgAGBQJWCc87AAoJEPC3R3P2I92FQNcP/RF+M8MGZ2PJ8heh98D84rb5
Dx+Yq1czRJ+YZbK5tCfkyU66KspEzM7IIOiiXDLlBZ+AjcQtCUopTNMbL/UN+oVT
5lWrvPZlWRJqRN5bA/RA3i/DSBRucucmP8n4pmTKqsMMqKwzk/f3sE+Uo5oAzS+y
JaSywxm+Vd4dkW5T94kc6TXCeWcaD47tG0mgg0jHGwFtOioDEeWgf7Kie52+RV+o
I6z7GlQj9dgcKs2NmVr67AoY1dfRYl1ZvvJN7bYoLbHnEgiSw1d6XZK/2cqHzIpE
S1KEHOyuSZJh8Txwfg6oJ3sbpFZaurSIXDXfOhWuJ90OrOu4hgeODTPX/3o2CKae
K+WhsL6XOhrxyMhfq/VWplF6Hjo7VqLcT9e0sYZ4YNkUJrGAza3iPOqngK9zmdsM
80HLJdbsiZMkl+i55IOuisckCtvjUtVE+bDlzau6vwgBlgZ9DKByPPmqJGjS9I3L
vCEKsRZryaSvaYSnK46kpqXsukN/+QMefXL25IfTf4wQQaV4O+mSJxkkLXPAKqfd
cvCFg08MyAQS+YyNMBdFDJyj7tWVclGZhJkqlyjPjQ2YrFA5tQ7MoqY05NomxY9Q
xo0JuaceNccFetKPg1LMmTp5Ag/2DCcnGq/0Z3ioGVJTFIVil0BnWIFctlGbquya
n4Ylfe3h1T6hWJ7bjxwF
=cZRI
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew R. Ochs Sept. 29, 2015, 7:38 p.m. UTC | #2
> On Sep 28, 2015, at 6:37 PM, Daniel Axtens <dja@axtens.net> wrote:
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> Hi,
> 
>> static int afu_set_wwpn(struct afu *afu, int port, u64 *fc_regs, u64 wwpn)
>> {
>> -	int ret = 0;
>> +	int rc = 0;
> I realise it's nice to have things consistent, but making this change
> now makes the rest of the patch quite difficult to follow.

Next time I will try to separate out a change like this.

> 
>> 
>> 	set_port_offline(fc_regs);
>> 
>> @@ -1038,33 +1038,26 @@ static int afu_set_wwpn(struct afu *afu, int port, u64 *fc_regs, u64 wwpn)
>> 			       FC_PORT_STATUS_RETRY_CNT)) {
>> 		pr_debug("%s: wait on port %d to go offline timed out\n",
>> 			 __func__, port);
>> -		ret = -1; /* but continue on to leave the port back online */
>> +		rc = -1; /* but continue on to leave the port back online */
>> 	}
>> 
>> -	if (ret == 0)
>> +	if (rc == 0)
>> 		writeq_be(wwpn, &fc_regs[FC_PNAME / 8]);
>> 
>> +	/* Always return success after programming WWPN */
>> +	rc = 0;
>> +
>> 	set_port_online(fc_regs);
>> 
>> 	if (!wait_port_online(fc_regs, FC_PORT_STATUS_RETRY_INTERVAL_US,
>> 			      FC_PORT_STATUS_RETRY_CNT)) {
>> 		pr_debug("%s: wait on port %d to go online timed out\n",
>> 			 __func__, port);
>> -		ret = -1;
>> -
>> -		/*
>> -		 * Override for internal lun!!!
>> -		 */
>> -		if (afu->internal_lun) {
>> -			pr_debug("%s: Overriding port %d online timeout!!!\n",
>> -				 __func__, port);
>> -			ret = 0;
>> -		}
>> 	}
>> 
>> -	pr_debug("%s: returning rc=%d\n", __func__, ret);
>> +	pr_debug("%s: returning rc=%d\n", __func__, rc);
> I'm not sure I fully understand the flow of this function, but it looks
> like you set rc=0 regardless of how things actually go: is this ever
> going to print a return value other than zero?

Correct, this function behaves more like a void for the time being. The
overall goal of this is to allow a card to configure even when the link is
down. At some later point when the link is transitioned to 'up', a link state
change interrupt will trigger the port configuration. I left this with a return
code for right now in case we need to alter the behavior again (based
upon testing) and actually return a value other than 0.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Axtens Sept. 30, 2015, 11:50 p.m. UTC | #3
(resending to the list this time, apologies!)

>> I'm not sure I fully understand the flow of this function, but it looks
>> like you set rc=0 regardless of how things actually go: is this ever
>> going to print a return value other than zero?
>
> Correct, this function behaves more like a void for the time being. The
> overall goal of this is to allow a card to configure even when the link is
> down. At some later point when the link is transitioned to 'up', a link state
> change interrupt will trigger the port configuration. I left this with a return
> code for right now in case we need to alter the behavior again (based
> upon testing) and actually return a value other than 0.

OK. That makes more sense - it wasn't clear to me how it could be
correct to proceed if the links were down but now I understand how that
works. I think that explanation should go in the commit message.

>>> 	if (!wait_port_online(fc_regs, FC_PORT_STATUS_RETRY_INTERVAL_US,
>>> 			      FC_PORT_STATUS_RETRY_CNT)) {
>>> 		pr_debug("%s: wait on port %d to go online timed out\n",
>>> 			 __func__, port);

As an aside, should this be a bit noisier? It seems like something
a user would probably want to know - especially in the case where
something has actually gone wrong so there's no link state change
interrupt forthcoming regardless of how long you wait.

Regards,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matthew R. Ochs Oct. 1, 2015, 3 p.m. UTC | #4
> On Sep 30, 2015, at 6:50 PM, Daniel Axtens <dja@axtens.net> wrote:
> (resending to the list this time, apologies!)
> 
>>> I'm not sure I fully understand the flow of this function, but it looks
>>> like you set rc=0 regardless of how things actually go: is this ever
>>> going to print a return value other than zero?
>> 
>> Correct, this function behaves more like a void for the time being. The
>> overall goal of this is to allow a card to configure even when the link is
>> down. At some later point when the link is transitioned to 'up', a link state
>> change interrupt will trigger the port configuration. I left this with a return
>> code for right now in case we need to alter the behavior again (based
>> upon testing) and actually return a value other than 0.
> 
> OK. That makes more sense - it wasn't clear to me how it could be
> correct to proceed if the links were down but now I understand how that
> works. I think that explanation should go in the commit message.
> 
>>>> 	if (!wait_port_online(fc_regs, FC_PORT_STATUS_RETRY_INTERVAL_US,
>>>> 			      FC_PORT_STATUS_RETRY_CNT)) {
>>>> 		pr_debug("%s: wait on port %d to go online timed out\n",
>>>> 			 __func__, port);
> 
> As an aside, should this be a bit noisier? It seems like something
> a user would probably want to know - especially in the case where
> something has actually gone wrong so there's no link state change
> interrupt forthcoming regardless of how long you wait.

You bring up a good point. There is another place where we are noisier
with respect to the link being down, so we'll do the same here. I'll include
this in v5 along with an updated commit message as requested.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/cxlflash/main.c b/drivers/scsi/cxlflash/main.c
index ed9fd8c..d45388f 100644
--- a/drivers/scsi/cxlflash/main.c
+++ b/drivers/scsi/cxlflash/main.c
@@ -1030,7 +1030,7 @@  static int wait_port_offline(u64 *fc_regs, u32 delay_us, u32 nretry)
  */
 static int afu_set_wwpn(struct afu *afu, int port, u64 *fc_regs, u64 wwpn)
 {
-	int ret = 0;
+	int rc = 0;
 
 	set_port_offline(fc_regs);
 
@@ -1038,33 +1038,26 @@  static int afu_set_wwpn(struct afu *afu, int port, u64 *fc_regs, u64 wwpn)
 			       FC_PORT_STATUS_RETRY_CNT)) {
 		pr_debug("%s: wait on port %d to go offline timed out\n",
 			 __func__, port);
-		ret = -1; /* but continue on to leave the port back online */
+		rc = -1; /* but continue on to leave the port back online */
 	}
 
-	if (ret == 0)
+	if (rc == 0)
 		writeq_be(wwpn, &fc_regs[FC_PNAME / 8]);
 
+	/* Always return success after programming WWPN */
+	rc = 0;
+
 	set_port_online(fc_regs);
 
 	if (!wait_port_online(fc_regs, FC_PORT_STATUS_RETRY_INTERVAL_US,
 			      FC_PORT_STATUS_RETRY_CNT)) {
 		pr_debug("%s: wait on port %d to go online timed out\n",
 			 __func__, port);
-		ret = -1;
-
-		/*
-		 * Override for internal lun!!!
-		 */
-		if (afu->internal_lun) {
-			pr_debug("%s: Overriding port %d online timeout!!!\n",
-				 __func__, port);
-			ret = 0;
-		}
 	}
 
-	pr_debug("%s: returning rc=%d\n", __func__, ret);
+	pr_debug("%s: returning rc=%d\n", __func__, rc);
 
-	return ret;
+	return rc;
 }
 
 /**