| Message ID | 1443223018-9577-1-git-send-email-mrochs@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
-----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
> 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
(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
> 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 --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; } /**