[PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[James Smart]

Deprecate lpfc_soft_wwn parameter.
No longer allow override of hw-assigned wwns

Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
Signed-off-by: James Smart <james.smart@broadcom.com>
---
 drivers/scsi/lpfc/lpfc.h      |   4 -
 drivers/scsi/lpfc/lpfc_attr.c | 216 ------------------------------------------
 drivers/scsi/lpfc/lpfc_init.c |  16 +---
 3 files changed, 3 insertions(+), 233 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc.h b/drivers/scsi/lpfc/lpfc.h
index 8a20b4e..341b656e 100644
--- a/drivers/scsi/lpfc/lpfc.h
+++ b/drivers/scsi/lpfc/lpfc.h
@@ -729,8 +729,6 @@ struct lpfc_hba {
 	uint32_t cfg_sg_seg_cnt;
 	uint32_t cfg_prot_sg_seg_cnt;
 	uint32_t cfg_sg_dma_buf_size;
-	uint64_t cfg_soft_wwnn;
-	uint64_t cfg_soft_wwpn;
 	uint32_t cfg_hba_queue_depth;
 	uint32_t cfg_enable_hba_reset;
 	uint32_t cfg_enable_hba_heartbeat;
@@ -835,8 +833,6 @@ struct lpfc_hba {
 #define VPD_PORT            0x8         /* valid vpd port data */
 #define VPD_MASK            0xf         /* mask for any vpd data */
 
-	uint8_t soft_wwn_enable;
-
 	struct timer_list fcp_poll_timer;
 	struct timer_list eratt_poll;
 	uint32_t eratt_poll_interval;
diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index c847755..98c40a7 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -1987,7 +1987,6 @@ static DEVICE_ATTR(protocol, S_IRUGO, lpfc_sli4_protocol_show, NULL);
 static DEVICE_ATTR(lpfc_xlane_supported, S_IRUGO, lpfc_oas_supported_show,
 		   NULL);
 
-static char *lpfc_soft_wwn_key = "C99G71SL8032A";
 #define WWN_SZ 8
 /**
  * lpfc_wwn_set - Convert string to the 8 byte WWN value.
@@ -2031,216 +2030,6 @@ lpfc_wwn_set(const char *buf, size_t cnt, char wwn[])
 	}
 	return 0;
 }
-/**
- * lpfc_soft_wwn_enable_store - Allows setting of the wwn if the key is valid
- * @dev: class device that is converted into a Scsi_host.
- * @attr: device attribute, not used.
- * @buf: containing the string lpfc_soft_wwn_key.
- * @count: must be size of lpfc_soft_wwn_key.
- *
- * Returns:
- * -EINVAL if the buffer does not contain lpfc_soft_wwn_key
- * length of buf indicates success
- **/
-static ssize_t
-lpfc_soft_wwn_enable_store(struct device *dev, struct device_attribute *attr,
-			   const char *buf, size_t count)
-{
-	struct Scsi_Host  *shost = class_to_shost(dev);
-	struct lpfc_vport *vport = (struct lpfc_vport *) shost->hostdata;
-	struct lpfc_hba   *phba = vport->phba;
-	unsigned int cnt = count;
-
-	/*
-	 * We're doing a simple sanity check for soft_wwpn setting.
-	 * We require that the user write a specific key to enable
-	 * the soft_wwpn attribute to be settable. Once the attribute
-	 * is written, the enable key resets. If further updates are
-	 * desired, the key must be written again to re-enable the
-	 * attribute.
-	 *
-	 * The "key" is not secret - it is a hardcoded string shown
-	 * here. The intent is to protect against the random user or
-	 * application that is just writing attributes.
-	 */
-
-	/* count may include a LF at end of string */
-	if (buf[cnt-1] == '\n')
-		cnt--;
-
-	if ((cnt != strlen(lpfc_soft_wwn_key)) ||
-	    (strncmp(buf, lpfc_soft_wwn_key, strlen(lpfc_soft_wwn_key)) != 0))
-		return -EINVAL;
-
-	phba->soft_wwn_enable = 1;
-	return count;
-}
-static DEVICE_ATTR(lpfc_soft_wwn_enable, S_IWUSR, NULL,
-		   lpfc_soft_wwn_enable_store);
-
-/**
- * lpfc_soft_wwpn_show - Return the cfg soft ww port name of the adapter
- * @dev: class device that is converted into a Scsi_host.
- * @attr: device attribute, not used.
- * @buf: on return contains the wwpn in hexadecimal.
- *
- * Returns: size of formatted string.
- **/
-static ssize_t
-lpfc_soft_wwpn_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;
-
-	return snprintf(buf, PAGE_SIZE, "0x%llx\n",
-			(unsigned long long)phba->cfg_soft_wwpn);
-}
-
-/**
- * lpfc_soft_wwpn_store - Set the ww port name of the adapter
- * @dev class device that is converted into a Scsi_host.
- * @attr: device attribute, not used.
- * @buf: contains the wwpn in hexadecimal.
- * @count: number of wwpn bytes in buf
- *
- * Returns:
- * -EACCES hba reset not enabled, adapter over temp
- * -EINVAL soft wwn not enabled, count is invalid, invalid wwpn byte invalid
- * -EIO error taking adapter offline or online
- * value of count on success
- **/
-static ssize_t
-lpfc_soft_wwpn_store(struct device *dev, struct device_attribute *attr,
-		     const char *buf, size_t count)
-{
-	struct Scsi_Host  *shost = class_to_shost(dev);
-	struct lpfc_vport *vport = (struct lpfc_vport *) shost->hostdata;
-	struct lpfc_hba   *phba = vport->phba;
-	struct completion online_compl;
-	int stat1 = 0, stat2 = 0;
-	unsigned int cnt = count;
-	u8 wwpn[WWN_SZ];
-	int rc;
-
-	if (!phba->cfg_enable_hba_reset)
-		return -EACCES;
-	spin_lock_irq(&phba->hbalock);
-	if (phba->over_temp_state == HBA_OVER_TEMP) {
-		spin_unlock_irq(&phba->hbalock);
-		return -EACCES;
-	}
-	spin_unlock_irq(&phba->hbalock);
-	/* count may include a LF at end of string */
-	if (buf[cnt-1] == '\n')
-		cnt--;
-
-	if (!phba->soft_wwn_enable)
-		return -EINVAL;
-
-	/* lock setting wwpn, wwnn down */
-	phba->soft_wwn_enable = 0;
-
-	rc = lpfc_wwn_set(buf, cnt, wwpn);
-	if (!rc) {
-		/* not able to set wwpn, unlock it */
-		phba->soft_wwn_enable = 1;
-		return rc;
-	}
-
-	phba->cfg_soft_wwpn = wwn_to_u64(wwpn);
-	fc_host_port_name(shost) = phba->cfg_soft_wwpn;
-	if (phba->cfg_soft_wwnn)
-		fc_host_node_name(shost) = phba->cfg_soft_wwnn;
-
-	dev_printk(KERN_NOTICE, &phba->pcidev->dev,
-		   "lpfc%d: Reinitializing to use soft_wwpn\n", phba->brd_no);
-
-	stat1 = lpfc_do_offline(phba, LPFC_EVT_OFFLINE);
-	if (stat1)
-		lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
-				"0463 lpfc_soft_wwpn attribute set failed to "
-				"reinit adapter - %d\n", stat1);
-	init_completion(&online_compl);
-	rc = lpfc_workq_post_event(phba, &stat2, &online_compl,
-				   LPFC_EVT_ONLINE);
-	if (rc == 0)
-		return -ENOMEM;
-
-	wait_for_completion(&online_compl);
-	if (stat2)
-		lpfc_printf_log(phba, KERN_ERR, LOG_INIT,
-				"0464 lpfc_soft_wwpn attribute set failed to "
-				"reinit adapter - %d\n", stat2);
-	return (stat1 || stat2) ? -EIO : count;
-}
-static DEVICE_ATTR(lpfc_soft_wwpn, S_IRUGO | S_IWUSR,
-		   lpfc_soft_wwpn_show, lpfc_soft_wwpn_store);
-
-/**
- * lpfc_soft_wwnn_show - Return the cfg soft ww node name for the adapter
- * @dev: class device that is converted into a Scsi_host.
- * @attr: device attribute, not used.
- * @buf: on return contains the wwnn in hexadecimal.
- *
- * Returns: size of formatted string.
- **/
-static ssize_t
-lpfc_soft_wwnn_show(struct device *dev, struct device_attribute *attr,
-		    char *buf)
-{
-	struct Scsi_Host *shost = class_to_shost(dev);
-	struct lpfc_hba *phba = ((struct lpfc_vport *)shost->hostdata)->phba;
-	return snprintf(buf, PAGE_SIZE, "0x%llx\n",
-			(unsigned long long)phba->cfg_soft_wwnn);
-}
-
-/**
- * lpfc_soft_wwnn_store - sets the ww node name of the adapter
- * @cdev: class device that is converted into a Scsi_host.
- * @buf: contains the ww node name in hexadecimal.
- * @count: number of wwnn bytes in buf.
- *
- * Returns:
- * -EINVAL soft wwn not enabled, count is invalid, invalid wwnn byte invalid
- * value of count on success
- **/
-static ssize_t
-lpfc_soft_wwnn_store(struct device *dev, struct device_attribute *attr,
-		     const char *buf, size_t count)
-{
-	struct Scsi_Host *shost = class_to_shost(dev);
-	struct lpfc_hba *phba = ((struct lpfc_vport *)shost->hostdata)->phba;
-	unsigned int cnt = count;
-	u8 wwnn[WWN_SZ];
-	int rc;
-
-	/* count may include a LF at end of string */
-	if (buf[cnt-1] == '\n')
-		cnt--;
-
-	if (!phba->soft_wwn_enable)
-		return -EINVAL;
-
-	rc = lpfc_wwn_set(buf, cnt, wwnn);
-	if (!rc) {
-		/* Allow wwnn to be set many times, as long as the enable
-		 * is set. However, once the wwpn is set, everything locks.
-		 */
-		return rc;
-	}
-
-	phba->cfg_soft_wwnn = wwn_to_u64(wwnn);
-
-	dev_printk(KERN_NOTICE, &phba->pcidev->dev,
-		   "lpfc%d: soft_wwnn set. Value will take effect upon "
-		   "setting of the soft_wwpn\n", phba->brd_no);
-
-	return count;
-}
-static DEVICE_ATTR(lpfc_soft_wwnn, S_IRUGO | S_IWUSR,
-		   lpfc_soft_wwnn_show, lpfc_soft_wwnn_store);
 
 /**
  * lpfc_oas_tgt_show - Return wwpn of target whose luns maybe enabled for
@@ -4750,9 +4539,6 @@ struct device_attribute *lpfc_hba_attrs[] = {
 	&dev_attr_lpfc_fcp_cpu_map,
 	&dev_attr_lpfc_fcp_io_channel,
 	&dev_attr_lpfc_enable_bg,
-	&dev_attr_lpfc_soft_wwnn,
-	&dev_attr_lpfc_soft_wwpn,
-	&dev_attr_lpfc_soft_wwn_enable,
 	&dev_attr_lpfc_enable_hba_reset,
 	&dev_attr_lpfc_enable_hba_heartbeat,
 	&dev_attr_lpfc_EnableXLane,
@@ -5765,8 +5551,6 @@ lpfc_get_cfgparam(struct lpfc_hba *phba)
 	else
 		phba->cfg_poll = lpfc_poll;
 
-	phba->cfg_soft_wwnn = 0L;
-	phba->cfg_soft_wwpn = 0L;
 	lpfc_sg_seg_cnt_init(phba, lpfc_sg_seg_cnt);
 	lpfc_prot_sg_seg_cnt_init(phba, lpfc_prot_sg_seg_cnt);
 	lpfc_hba_queue_depth_init(phba, lpfc_hba_queue_depth);
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 4776fd8..43d2b06 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -319,36 +319,26 @@ lpfc_dump_wakeup_param_cmpl(struct lpfc_hba *phba, LPFC_MBOXQ_t *pmboxq)
 
 /**
  * lpfc_update_vport_wwn - Updates the fc_nodename, fc_portname,
- *	cfg_soft_wwnn, cfg_soft_wwpn
  * @vport: pointer to lpfc vport data structure.
  *
- *
  * Return codes
  *   None.
  **/
 void
 lpfc_update_vport_wwn(struct lpfc_vport *vport)
 {
-	/* If the soft name exists then update it using the service params */
-	if (vport->phba->cfg_soft_wwnn)
-		u64_to_wwn(vport->phba->cfg_soft_wwnn,
-			   vport->fc_sparam.nodeName.u.wwn);
-	if (vport->phba->cfg_soft_wwpn)
-		u64_to_wwn(vport->phba->cfg_soft_wwpn,
-			   vport->fc_sparam.portName.u.wwn);
-
 	/*
-	 * If the name is empty or there exists a soft name
+	 * If the name is empty
 	 * then copy the service params name, otherwise use the fc name
 	 */
-	if (vport->fc_nodename.u.wwn[0] == 0 || vport->phba->cfg_soft_wwnn)
+	if (vport->fc_nodename.u.wwn[0] == 0)
 		memcpy(&vport->fc_nodename, &vport->fc_sparam.nodeName,
 			sizeof(struct lpfc_name));
 	else
 		memcpy(&vport->fc_sparam.nodeName, &vport->fc_nodename,
 			sizeof(struct lpfc_name));
 
-	if (vport->fc_portname.u.wwn[0] == 0 || vport->phba->cfg_soft_wwpn)
+	if (vport->fc_portname.u.wwn[0] == 0)
 		memcpy(&vport->fc_portname, &vport->fc_sparam.portName,
 			sizeof(struct lpfc_name));
 	else
-- 
2.5.0

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[Hannes Reinecke]

On 12/20/2016 12:07 AM, James Smart wrote:
> 
> Deprecate lpfc_soft_wwn parameter.
> No longer allow override of hw-assigned wwns
> 
> Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
> Signed-off-by: James Smart <james.smart@broadcom.com>
> ---
>  drivers/scsi/lpfc/lpfc.h      |   4 -
>  drivers/scsi/lpfc/lpfc_attr.c | 216 ------------------------------------------
>  drivers/scsi/lpfc/lpfc_init.c |  16 +---
>  3 files changed, 3 insertions(+), 233 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@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)

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[Sebastian Herbszt]

James Smart wrote:
> Deprecate lpfc_soft_wwn parameter.
> No longer allow override of hw-assigned wwns
> 
> Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
> Signed-off-by: James Smart <james.smart@broadcom.com>

Any reason to remove this functionality?
I was actually using it.

Sebastian

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[James Smart]

I competes, without reasonable solutions to resolve which should be what 
at what time, with things like fabric-assigned wwn's as well as 
platform-oem name assignments via platform mechanisms.  Too many admin 
planes competing.

-- james


On 12/22/2016 2:00 PM, Sebastian Herbszt wrote:
> James Smart wrote:
>> Deprecate lpfc_soft_wwn parameter.
>> No longer allow override of hw-assigned wwns
>>
>> Signed-off-by: Dick Kennedy <dick.kennedy@broadcom.com>
>> Signed-off-by: James Smart <james.smart@broadcom.com>
> Any reason to remove this functionality?
> I was actually using it.
>
> Sebastian

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[Sebastian Herbszt]

James Smart wrote:
> I competes, without reasonable solutions to resolve which should be
> what at what time, with things like fabric-assigned wwn's as well as 
> platform-oem name assignments via platform mechanisms.  Too many
> admin planes competing.
> 
> -- james

FA-WWN seems to be fabric (and Brocade?) only and "platform mechanisms"
are likely not portable. Soft wwn is a simple and flexible solution
which works in fabric and loop configurations and is platform and
server vendor agnostic. It already works with existing HBAs on x86 and
should also work on PowerPC or SPARC systems.
It is useful for initiators (hardware independence/mobility) and
targets (transparent failover/floating wwn).
Therefore I like to ask you to revert this patch and not introduce this
functional regression.

Sebastian

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[James Smart]

Sebastian,

"not portable" isn't the right way to describe it. It's not a 
chip-architecture issue, but rather that some oem platforms have 
side-band management that overrides anything that could have been done 
in the os, and in ways that may not be easy to communicate back to the 
driver.   FAWN - true Brocade only - but that is the 70+% market share 
of the FC switches. The current implementation already doesn't work many 
situations - so there's no clear winner for having it work everywhere. 
Given the inability for it always to work, the decision was to deprecate 
the attribute.

-- james


On 12/28/2016 10:38 AM, Sebastian Herbszt wrote:
> James Smart wrote:
>> I competes, without reasonable solutions to resolve which should be
>> what at what time, with things like fabric-assigned wwn's as well as
>> platform-oem name assignments via platform mechanisms.  Too many
>> admin planes competing.
>>
>> -- james
> FA-WWN seems to be fabric (and Brocade?) only and "platform mechanisms"
> are likely not portable. Soft wwn is a simple and flexible solution
> which works in fabric and loop configurations and is platform and
> server vendor agnostic. It already works with existing HBAs on x86 and
> should also work on PowerPC or SPARC systems.
> It is useful for initiators (hardware independence/mobility) and
> targets (transparent failover/floating wwn).
> Therefore I like to ask you to revert this patch and not introduce this
> functional regression.
>
> Sebastian

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[Christoph Hellwig]

Hi James,

in Linux we have a pretty clear policy to avoid breaking existing real
life userspace.  Given that Sebastian (and probably other users as well)
make use of the soft WWN feature we can't just remove it.

Which btw is what this patch does - deprecating is telling people to
not use if, with the hopes of removing it in the future.  I think it
would be fine to deprecate it with your above rational and print a
warning to the kernel log, but we can't simply remove it.

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[James Smart]

ok.. I'll submit a patch to re-add the parameters, and add an 
appropriate "deprecation" warning

-- james


On 12/28/2016 11:41 PM, Christoph Hellwig wrote:
> Hi James,
>
> in Linux we have a pretty clear policy to avoid breaking existing real
> life userspace.  Given that Sebastian (and probably other users as well)
> make use of the soft WWN feature we can't just remove it.
>
> Which btw is what this patch does - deprecating is telling people to
> not use if, with the hopes of removing it in the future.  I think it
> would be fine to deprecate it with your above rational and print a
> warning to the kernel log, but we can't simply remove it.

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[Sebastian Herbszt]

James,

thank you for taking the time to answer me.

James Smart wrote:
> Sebastian,
> 
> "not portable" isn't the right way to describe it. It's not a 
> chip-architecture issue, but rather that some oem platforms have 
> side-band management that overrides anything that could have been
> done in the os, and in ways that may not be easy to communicate back
> to the driver.

"not portable" might not be the term I actually meant. Let's call it
server vendor dependent.
Can you please elaborate on the oem platforms and side-band management
you mean? I know of a solution called "ServerView Virtual-IO Manager"
by Fujitsu which might fit. AFAIK it is a piece of software which
communicates with the BMC and allows to change the MAC address and WWN.
Of course it only works with Fujitsu servers and supported cards.
I think I heard that for NICs it just changes the MAC address in the
eeprom. This is not done "online" but when the server is powered on.
Even though this might be a comparable case (external MAC address
configuration) I think the ability to change the MAC address by the
means of "ip link set dev ethX address" was not removed.

> FAWN - true Brocade only - but that is the 70+%
> market share of the FC switches. The current implementation already
> doesn't work many situations - so there's no clear winner for having
> it work everywhere. Given the inability for it always to work, the
> decision was to deprecate the attribute.
> 
> -- james

How do you choose the desired WWN between the possible options
(factory set, oem platform set, FA-WWN)?
And wouldn't soft wwn always win because it is applied at last?
However it might compete with FA-WWN. I wasn't yet able to
find details on how it works.

Sebastian

Re: [PATCH v2 03/11] lpfc: Deprecate lpfc_soft_wwn parameter

[James Smart]

On 12/29/2016 3:31 PM, Sebastian Herbszt wrote:
> "not portable" might not be the term I actually meant. Let's call it
> server vendor dependent.
> Can you please elaborate on the oem platforms and side-band management
> you mean? I know of a solution called "ServerView Virtual-IO Manager"
> by Fujitsu which might fit. AFAIK it is a piece of software which
> communicates with the BMC and allows to change the MAC address and WWN.
> Of course it only works with Fujitsu servers and supported cards.
> I think I heard that for NICs it just changes the MAC address in the
> eeprom. This is not done "online" but when the server is powered on.
> Even though this might be a comparable case (external MAC address
> configuration) I think the ability to change the MAC address by the
> means of "ip link set dev ethX address" was not removed.

Unfortunately, I can't elaborate on the platforms. Yes, there are things 
similar to what you describe above. Also realize that persisting the 
value across different cold/hot/flr resets affects boot and dump.

> How do you choose the desired WWN between the possible options
> (factory set, oem platform set, FA-WWN)?
> And wouldn't soft wwn always win because it is applied at last?
> However it might compete with FA-WWN. I wasn't yet able to
> find details on how it works.
>
> Sebastian

Unfortunately, I can't comment on that either.  It's not always last 
wins and that's one of the issues.

I've submitted a patch to reinstated the feature - it works as well as 
it did before.

-- james