Re: [PATCH 03/17] lpfc: NVME Initiator: Base modifications Part B

2017-01-19 Thread Johannes Thumshirn 
On Wed, Jan 18, 2017 at 06:45:46PM -0800, James Smart wrote:
> 
> 
> On 1/18/2017 2:11 AM, Johannes Thumshirn wrote:
> >On Tue, Jan 17, 2017 at 05:20:46PM -0800, James Smart wrote:
> >>NVME Initiator: Base modifications
> >>
> >>This is part B of parts A..F.
> >>
> >>Part B is limited to lpfc_attr.c: lpfc attribute modifications
> >>
> >>*
> >>
> >>Refer to Part A for a description of base modifications
> >>
> >>Signed-off-by: Dick Kennedy 
> >>Signed-off-by: James Smart 
> >>---
> >[...]
> >
> >>+   len += snprintf(buf + len, PAGE_SIZE - len,
> >>+   "%s%d WWPN x%llx WWNN x%llx DID x%06x %s\n",
> >>+   "NVME LPORT lpfc",
> >Is it the lack of coffee or should it be
> > "NVME LPORT lpfc%d WWPN x%llx WWNN x%llx DID x%06x %s\n"
> >
> >I think you're doing it to not hit the 80 chars limit, but then there are
> >way more offenders than that one, so...
> 
> The line split is certainly due to the 80 limit and have that issue a lot.
> 
> As for what the string name should be - I agree with you. something is
> confused.
> 
> >>+int
> >>+lpfc_emptyq_wait(struct lpfc_hba *phba, struct list_head *q, spinlock_t 
> >>*lock)
> >>+{
> >>+   int cnt = 0;
> >>+
> >>+   spin_lock_irq(lock);
> >>+   while (!list_empty(q)) {
> >>+   spin_unlock_irq(lock);
> >>+   msleep(20);
> >>+   if (cnt++ > 250) {  /* 5 secs */
> >>+   lpfc_printf_log(phba, KERN_WARNING, LOG_INIT,
> >>+   "0466 %s %s\n",
> >>+   "Outstanding IO when ",
> >>+   "bringing Adapter offline\n");
> >>+   return 0;
> >>+   }
> >>+   spin_lock_irq(lock);
> >>+   }
> >>+   spin_unlock_irq(lock);
> >>+   return 1;
> >>+}
> >>+
> >Aren't you using lpc_emptyq_wait() in patches prior to that already? This
> >breaks git bisect. Pleas test-build (ideally + checkpatch and sparse/smatch)
> >each patch in the series individually.
> 
> I called out - in patch2 - that Patches 2 through 7, known as parts A..F,
> area really one big patch. They will not follow the git bisect rules.  I
> could have sent them in one huge patch, but chose to break them up.
> Unfortunately, the mods accumulated over time with lots of reworks -
> creating a base that was too intertwined to put into small functional
> patches without spending oodles of time to carve them up.   I hope you can
> bear with me on this set and review the 7 pieces as one big patch.

For review one big patch clearly is a huge problem, I agree. But the final
merged version must not break bisects. So we can probably defer the discussion
to the end of the merging.

Byte,
Johannes
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/17] lpfc: NVME Initiator: Base modifications Part B

2017-01-18 Thread James Smart 



On 1/18/2017 2:11 AM, Johannes Thumshirn wrote:

On Tue, Jan 17, 2017 at 05:20:46PM -0800, James Smart wrote:

NVME Initiator: Base modifications

This is part B of parts A..F.

Part B is limited to lpfc_attr.c: lpfc attribute modifications

*

Refer to Part A for a description of base modifications

Signed-off-by: Dick Kennedy 
Signed-off-by: James Smart 
---

[...]


+   len += snprintf(buf + len, PAGE_SIZE - len,
+   "%s%d WWPN x%llx WWNN x%llx DID x%06x %s\n",
+   "NVME LPORT lpfc",

Is it the lack of coffee or should it be
"NVME LPORT lpfc%d WWPN x%llx WWNN x%llx DID x%06x %s\n"

I think you're doing it to not hit the 80 chars limit, but then there are
way more offenders than that one, so...


The line split is certainly due to the 80 limit and have that issue a lot.

As for what the string name should be - I agree with you. something is 
confused.



+int
+lpfc_emptyq_wait(struct lpfc_hba *phba, struct list_head *q, spinlock_t *lock)
+{
+   int cnt = 0;
+
+   spin_lock_irq(lock);
+   while (!list_empty(q)) {
+   spin_unlock_irq(lock);
+   msleep(20);
+   if (cnt++ > 250) {  /* 5 secs */
+   lpfc_printf_log(phba, KERN_WARNING, LOG_INIT,
+   "0466 %s %s\n",
+   "Outstanding IO when ",
+   "bringing Adapter offline\n");
+   return 0;
+   }
+   spin_lock_irq(lock);
+   }
+   spin_unlock_irq(lock);
+   return 1;
+}
+

Aren't you using lpc_emptyq_wait() in patches prior to that already? This
breaks git bisect. Pleas test-build (ideally + checkpatch and sparse/smatch)
each patch in the series individually.


I called out - in patch2 - that Patches 2 through 7, known as parts 
A..F, area really one big patch. They will not follow the git bisect 
rules.  I could have sent them in one huge patch, but chose to break 
them up. Unfortunately, the mods accumulated over time with lots of 
reworks - creating a base that was too intertwined to put into small 
functional patches without spending oodles of time to carve them up.   I 
hope you can bear with me on this set and review the 7 pieces as one big 
patch.


-- james


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

Re: [PATCH 03/17] lpfc: NVME Initiator: Base modifications Part B

2017-01-18 Thread Johannes Thumshirn 
On Tue, Jan 17, 2017 at 05:20:46PM -0800, James Smart wrote:
> 
> NVME Initiator: Base modifications
> 
> This is part B of parts A..F.
> 
> Part B is limited to lpfc_attr.c: lpfc attribute modifications
> 
> *
> 
> Refer to Part A for a description of base modifications
> 
> Signed-off-by: Dick Kennedy 
> Signed-off-by: James Smart 
> ---

[...]

> + len += snprintf(buf + len, PAGE_SIZE - len,
> + "%s%d WWPN x%llx WWNN x%llx DID x%06x %s\n",
> + "NVME LPORT lpfc",

Is it the lack of coffee or should it be 
"NVME LPORT lpfc%d WWPN x%llx WWNN x%llx DID x%06x %s\n"

I think you're doing it to not hit the 80 chars limit, but then there are
way more offenders than that one, so...


> + phba->brd_no,
> + wwn_to_u64(vport->fc_portname.u.wwn),
> + wwn_to_u64(vport->fc_nodename.u.wwn),
> + localport->port_id, statep);

[...]

> +int
> +lpfc_emptyq_wait(struct lpfc_hba *phba, struct list_head *q, spinlock_t 
> *lock)
> +{
> + int cnt = 0;
> +
> + spin_lock_irq(lock);
> + while (!list_empty(q)) {
> + spin_unlock_irq(lock);
> + msleep(20);
> + if (cnt++ > 250) {  /* 5 secs */
> + lpfc_printf_log(phba, KERN_WARNING, LOG_INIT,
> + "0466 %s %s\n",
> + "Outstanding IO when ",
> + "bringing Adapter offline\n");
> + return 0;
> + }
> + spin_lock_irq(lock);
> + }
> + spin_unlock_irq(lock);
> + return 1;
> +}
> +

Aren't you using lpc_emptyq_wait() in patches prior to that already? This
breaks git bisect. Pleas test-build (ideally + checkpatch and sparse/smatch)
each patch in the series individually.

[...]

Thanks,
Johannes
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/17] lpfc: NVME Initiator: Base modifications Part B

2017-01-17 Thread Hannes Reinecke 
On 01/18/2017 02:20 AM, James Smart wrote:
> 
> NVME Initiator: Base modifications
> 
> This is part B of parts A..F.
> 
> Part B is limited to lpfc_attr.c: lpfc attribute modifications
> 
> *
> 
> Refer to Part A for a description of base modifications
> 
> Signed-off-by: Dick Kennedy 
> Signed-off-by: James Smart 
> ---
>  drivers/scsi/lpfc/lpfc_attr.c | 448 
> ++
>  1 file changed, 405 insertions(+), 43 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/17] lpfc: NVME Initiator: Base modifications Part B

2017-01-17 Thread James Smart 

NVME Initiator: Base modifications

This is part B of parts A..F.

Part B is limited to lpfc_attr.c: lpfc attribute modifications

*

Refer to Part A for a description of base modifications

Signed-off-by: Dick Kennedy 
Signed-off-by: James Smart 
---
 drivers/scsi/lpfc/lpfc_attr.c | 448 ++
 1 file changed, 405 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index 50cf402..72949f5 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -35,14 +35,17 @@
 #include 
 #include 
 
+#include 
+
 #include "lpfc_hw4.h"
 #include "lpfc_hw.h"
 #include "lpfc_sli.h"
 #include "lpfc_sli4.h"
 #include "lpfc_nl.h"
 #include "lpfc_disc.h"
-#include "lpfc_scsi.h"
 #include "lpfc.h"
+#include "lpfc_scsi.h"
+#include "lpfc_nvme.h"
 #include "lpfc_logmsg.h"
 #include "lpfc_version.h"
 #include "lpfc_compat.h"
@@ -50,9 +53,9 @@
 #include "lpfc_vport.h"
 #include "lpfc_attr.h"
 
-#define LPFC_DEF_DEVLOSS_TMO 30
-#define LPFC_MIN_DEVLOSS_TMO 1
-#define LPFC_MAX_DEVLOSS_TMO 255
+#define LPFC_DEF_DEVLOSS_TMO   30
+#define LPFC_MIN_DEVLOSS_TMO   1
+#define LPFC_MAX_DEVLOSS_TMO   255
 
 /*
  * Write key size should be multiple of 4. If write key is changed
@@ -130,6 +133,124 @@ lpfc_enable_fip_show(struct device *dev, struct 
device_attribute *attr,
 }
 
 static ssize_t
+lpfc_nvme_info_show(struct device *dev, struct device_attribute *attr,
+   char *buf)
+{
+   struct Scsi_Host *shost = class_to_shost(dev);
+   struct lpfc_vport *vport = (struct lpfc_vport *)shost->hostdata;
+   struct lpfc_hba   *phba = vport->phba;
+   struct nvme_fc_local_port *localport;
+   struct lpfc_nvme_lport *lport;
+   struct lpfc_nvme_rport *rport;
+   struct nvme_fc_remote_port *nrport;
+   char *statep;
+   int len = 0;
+
+   if (!(phba->cfg_enable_fc4_type & LPFC_ENABLE_NVME)) {
+   len += snprintf(buf, PAGE_SIZE, "NVME Disabled\n");
+   return len;
+   }
+
+   localport = vport->localport;
+   if (!localport) {
+   len = snprintf(buf, PAGE_SIZE,
+   "NVME Initiator x%llx is not allocated\n",
+   wwn_to_u64(vport->fc_portname.u.wwn));
+   return len;
+   }
+   len = snprintf(buf, PAGE_SIZE, "NVME Initiator Enabled\n");
+
+   spin_lock_irq(shost->host_lock);
+   lport = (struct lpfc_nvme_lport *)localport->private;
+
+   /* Port state is only one of two values for now. */
+   if (localport->port_id)
+   statep = "ONLINE";
+   else
+   statep = "UNKNOWN ";
+
+   len += snprintf(buf + len, PAGE_SIZE - len,
+   "%s%d WWPN x%llx WWNN x%llx DID x%06x %s\n",
+   "NVME LPORT lpfc",
+   phba->brd_no,
+   wwn_to_u64(vport->fc_portname.u.wwn),
+   wwn_to_u64(vport->fc_nodename.u.wwn),
+   localport->port_id, statep);
+
+   list_for_each_entry(rport, >rport_list, list) {
+   /* local short-hand pointer. */
+   nrport = rport->remoteport;
+
+   /* Port state is only one of two values for now. */
+   switch (nrport->port_state) {
+   case FC_OBJSTATE_ONLINE:
+   statep = "ONLINE";
+   break;
+   case FC_OBJSTATE_UNKNOWN:
+   statep = "UNKNOWN ";
+   break;
+   default:
+   statep = "UNSUPPORTED";
+   break;
+   }
+
+   /* Tab in to show lport ownership. */
+   len += snprintf(buf + len, PAGE_SIZE - len,
+   "NVME RPORT   ");
+   if (phba->brd_no >= 10)
+   len += snprintf(buf + len, PAGE_SIZE - len, " ");
+
+   len += snprintf(buf + len, PAGE_SIZE - len, "WWPN x%llx ",
+   nrport->port_name);
+   len += snprintf(buf + len, PAGE_SIZE - len, "WWNN x%llx ",
+   nrport->node_name);
+   len += snprintf(buf + len, PAGE_SIZE - len, "DID x%06x ",
+   nrport->port_id);
+
+   switch (nrport->port_role) {
+   case FC_PORT_ROLE_NVME_INITIATOR:
+   len +=  snprintf(buf + len, PAGE_SIZE - len,
+"INITIATOR ");
+   break;
+   case FC_PORT_ROLE_NVME_TARGET:
+   len +=  snprintf(buf + len, PAGE_SIZE - len,
+"TARGET ");
+   break;
+   case FC_PORT_ROLE_NVME_DISCOVERY:
+   len +=  snprintf(buf + len, PAGE_SIZE -