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

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(-)

-----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-----

> 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.

(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

> 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.