[RFC PATCH 0/5] Reorganize libfcoe control interfaces

[RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Robert Love]

The following series implements a move from using module parameters
as control interfaces to /sys/bus/fcoe based interfaces. A sysfs infrastructure
was added to the kernel a few cycles ago, this series builds on that work.

It moves the create, vn2vn_create, destroy, enable and disable interfaces
from /sys/module/libfcoe/parameters/ to various places under /sys/bus/fcoe/.
These interfaces simply are not module configurations- they are control
interfaces.

A second goal of this series is to change the initialization sequence for
a FCoE device. The result of this series is that interfaces created using
libfcoe.ko interfaces (i.e. fcoe.ko or bnx2fc.ko) will have the following
starting steps-

1) Create/alloc the port
   - Allocate kernel memory and create per-instance sysfs devices
   - No discovery or login

2) Configure the port
   - Change mode, set ddp_min, etc...

3) Start the port
   - Begins discovery and/or login (depending on mode)

4) Destroy the port
   - Logout and free all memory

I'm looking for feedback on using sysfs files as control interfaces that
the user (application) would write interface names to. I modeled this
series off of the bonding sysfs interface, but it was suggested to me that
it might not be a good example. I belive bonding uses two values per-file
a '+' or a '-" to add or delete and then the ifname apended. I am simply
writing the ifname to the ctlr_create or ctlr_destroy.

Series compiled and tested against v3.5. libfcoe.ko compile warning fixed
upstream after v3.5, anyone who compiles this can ignore section mismatch
warning. Also note that a modified fcoemon is needed to use the fcoe system
service against this kernel modification. I'd be happy to provide that
fcoemon code on request.

---

Robert Love (5):
      libfcoe, fcoe: Allow user to set a ctlr's mode
      libfcoe: Create new libfcoe control interfaces
      fcoe: Use new fcoe_sysfs control interface
      bnx2fc: Use new fcoe_sysfs control interface
      libfcoe, fcoe: Remove libfcoe module parameters


 Documentation/ABI/testing/sysfs-bus-fcoe |   51 +++++++
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c        |   98 ++++++++-----
 drivers/scsi/fcoe/fcoe.c                 |  229 +++++++++++++++---------------
 drivers/scsi/fcoe/fcoe.h                 |    9 +
 drivers/scsi/fcoe/fcoe_ctlr.c            |   24 +++
 drivers/scsi/fcoe/fcoe_sysfs.c           |  139 ++++++++++++++++++
 drivers/scsi/fcoe/fcoe_transport.c       |  174 ++++-------------------
 include/scsi/fcoe_sysfs.h                |    5 +
 include/scsi/libfcoe.h                   |   20 ++-
 9 files changed, 445 insertions(+), 304 deletions(-)

-- 

Thanks, //Rob

[RFC PATCH 1/5] libfcoe, fcoe: Allow user to set a ctlr's mode

[Robert Love]

This patch makes the 'mode' attribute of a
fcoe_ctlr_device writale. This allows the user
to store the mode with with the ctlr will be in.

Possible modes would be 'Fabric', or 'VN2VN'.

The default mode for a fcoe_ctlr{,_device} is 'Fabric'.
Drivers must implement the set_fcoe_ctlr_mode routine
to support this feature.

libfcoe offers an exported routine to set a fcoe_ctlr's
mode.

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-fcoe |    8 ++++
 drivers/scsi/fcoe/fcoe_ctlr.c            |   22 +++++++++++
 drivers/scsi/fcoe/fcoe_sysfs.c           |   61 +++++++++++++++++++++++++++++-
 include/scsi/fcoe_sysfs.h                |    1 
 include/scsi/libfcoe.h                   |    1 
 5 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-fcoe b/Documentation/ABI/testing/sysfs-bus-fcoe
index 469d09c..1b06ec1 100644
--- a/Documentation/ABI/testing/sysfs-bus-fcoe
+++ b/Documentation/ABI/testing/sysfs-bus-fcoe
@@ -9,6 +9,14 @@ Attributes:
 			  this value will change the dev_loss_tmo for all
 			  FCFs discovered by this controller.
 
+	mode:		  Display or change the FCoE Controller's mode. Possible
+			  modes are 'Fabric' and 'VN2VN'. If a FCoE Controller
+			  is started in 'Fabric' mode then FIP FCF discovery is
+			  initiated and ultimately a fabric login is attempted.
+			  If a FCoE Controller is started in 'VN2VN' mode then
+			  FIP VN2VN discovery and login is performed. A FCoE
+			  Controller only supports one mode at a time.
+
 	lesb_link_fail:   Link Error Status Block (LESB) link failure count.
 
 	lesb_vlink_fail:  Link Error Status Block (LESB) virtual link
diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index d68d572..bd899bf 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -2884,3 +2884,25 @@ void fcoe_ctlr_get_fip_mode(struct fcoe_ctlr_device *ctlr_dev)
 	mutex_unlock(&ctlr->ctlr_mutex);
 }
 EXPORT_SYMBOL(fcoe_ctlr_get_fip_mode);
+
+void fcoe_ctlr_set_fip_mode(struct fcoe_ctlr_device *ctlr_dev)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(ctlr_dev);
+
+	mutex_lock(&ctlr->ctlr_mutex);
+	switch (ctlr_dev->mode) {
+	case FIP_CONN_TYPE_VN2VN:
+		ctlr->mode = FIP_MODE_VN2VN;
+		break;
+	case FIP_CONN_TYPE_FABRIC:
+	default:
+		ctlr->mode = FIP_MODE_FABRIC;
+		break;
+	}
+
+	/* TODO: Probably need to restart the state machine */
+
+	mutex_unlock(&ctlr->ctlr_mutex);
+
+}
+EXPORT_SYMBOL(fcoe_ctlr_set_fip_mode);
diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index 2bc1631..3fbc556 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -21,6 +21,7 @@
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/etherdevice.h>
+#include <linux/ctype.h>
 
 #include <scsi/fcoe_sysfs.h>
 
@@ -40,6 +41,46 @@ MODULE_PARM_DESC(fcf_dev_loss_tmo,
 		 " insulate the loss of a fcf. Once this value is"
 		 " exceeded, the fcf is removed.");
 
+#define FCOE_MAX_MODENAME_LEN 20
+struct fcoe_ctlr_mode_table {
+	char *modename;
+	enum fip_conn_type mode;
+};
+
+const struct fcoe_ctlr_mode_table ctlr_mode_tbl[] = {
+	{       "fabric",           FIP_CONN_TYPE_FABRIC},
+	{       "vn2vn",            FIP_CONN_TYPE_VN2VN},
+	{       NULL,               -1},
+};
+
+static enum fip_conn_type fcoe_parse_mode(const char *buf,
+				  const struct fcoe_ctlr_mode_table *tbl)
+{
+	int modeint = -1, i, rv;
+	char *p, modestr[FCOE_MAX_MODENAME_LEN + 1] = { 0, };
+
+	for (p = (char *)buf; *p; p++)
+		if (!(isdigit(*p) || isspace(*p)))
+			break;
+
+	if (*p)
+		rv = sscanf(buf, "%20s", modestr);
+	else
+		rv = sscanf(buf, "%d", &modeint);
+
+	if (!rv)
+		return FIP_CONN_TYPE_UNKNOWN;
+
+	for (i = 0; tbl[i].modename; i++) {
+		if (modeint == tbl[i].mode)
+			return tbl[i].mode;
+		if (strcmp(modestr, tbl[i].modename) == 0)
+			return tbl[i].mode;
+	}
+
+	return FIP_CONN_TYPE_UNKNOWN;
+}
+
 /*
  * These are used by the fcoe_*_show_function routines, they
  * are intentionally placed in the .c file as they're not intended
@@ -273,8 +314,24 @@ static ssize_t show_ctlr_mode(struct device *dev,
 	return snprintf(buf, FCOE_CTLR_MODE_MAX_NAMELEN,
 			"%s\n", name);
 }
-static FCOE_DEVICE_ATTR(ctlr, mode, S_IRUGO,
-			show_ctlr_mode, NULL);
+
+static ssize_t store_ctlr_mode(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
+
+	if (!ctlr->f->set_fcoe_ctlr_mode)
+		return -EINVAL;
+
+	ctlr->mode = fcoe_parse_mode(buf, ctlr_mode_tbl);
+	ctlr->f->set_fcoe_ctlr_mode(ctlr);
+
+	return count;
+}
+
+static FCOE_DEVICE_ATTR(ctlr, mode, S_IRUGO | S_IWUSR,
+			show_ctlr_mode, store_ctlr_mode);
 
 static ssize_t
 store_private_fcoe_ctlr_fcf_dev_loss_tmo(struct device *dev,
diff --git a/include/scsi/fcoe_sysfs.h b/include/scsi/fcoe_sysfs.h
index 604cb9b..421ae67 100644
--- a/include/scsi/fcoe_sysfs.h
+++ b/include/scsi/fcoe_sysfs.h
@@ -35,6 +35,7 @@ struct fcoe_sysfs_function_template {
 	void (*get_fcoe_ctlr_err_block)(struct fcoe_ctlr_device *);
 	void (*get_fcoe_ctlr_fcs_error)(struct fcoe_ctlr_device *);
 	void (*get_fcoe_ctlr_mode)(struct fcoe_ctlr_device *);
+	void (*set_fcoe_ctlr_mode)(struct fcoe_ctlr_device *);
 	void (*get_fcoe_fcf_selected)(struct fcoe_fcf_device *);
 	void (*get_fcoe_fcf_vlan_id)(struct fcoe_fcf_device *);
 };
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 22b07cc..20533cc 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -359,6 +359,7 @@ int fcoe_get_paged_crc_eof(struct sk_buff *skb, int tlen,
 /* FCoE Sysfs helpers */
 void fcoe_fcf_get_selected(struct fcoe_fcf_device *);
 void fcoe_ctlr_get_fip_mode(struct fcoe_ctlr_device *);
+void fcoe_ctlr_set_fip_mode(struct fcoe_ctlr_device *);
 
 /**
  * struct netdev_list

Re: [RFC PATCH 1/5] libfcoe, fcoe: Allow user to set a ctlr's mode

[Greg KH]

On Mon, Sep 10, 2012 at 03:59:14PM -0700, Robert Love wrote:
> This patch makes the 'mode' attribute of a
> fcoe_ctlr_device writale. This allows the user
> to store the mode with with the ctlr will be in.
> 
> Possible modes would be 'Fabric', or 'VN2VN'.
> 
> The default mode for a fcoe_ctlr{,_device} is 'Fabric'.
> Drivers must implement the set_fcoe_ctlr_mode routine
> to support this feature.
> 
> libfcoe offers an exported routine to set a fcoe_ctlr's
> mode.
> 
> Signed-off-by: Robert Love <robert.w.love@intel.com>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [RFC PATCH 1/5] libfcoe, fcoe: Allow user to set a ctlr's mode

[Bart Van Assche]

On 09/11/12 00:59, Robert Love wrote:
> +static enum fip_conn_type fcoe_parse_mode(const char *buf,
> +				  const struct fcoe_ctlr_mode_table *tbl)
> +{
> +	int modeint = -1, i, rv;
> +	char *p, modestr[FCOE_MAX_MODENAME_LEN + 1] = { 0, };
> +
> +	for (p = (char *)buf; *p; p++)
> +		if (!(isdigit(*p) || isspace(*p)))
> +			break;

If you change the declaration of p from "char *p" into "const char *p"
you won't need a cast in the above for loop.

[ ... ]

> -static FCOE_DEVICE_ATTR(ctlr, mode, S_IRUGO,
> -			show_ctlr_mode, NULL);
> +
> +static ssize_t store_ctlr_mode(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
> +
> +	if (!ctlr->f->set_fcoe_ctlr_mode)
> +		return -EINVAL;
> +
> +	ctlr->mode = fcoe_parse_mode(buf, ctlr_mode_tbl);

As far as I know sysfs doesn't terminate buf with a '\0' before calling
a store method. Does that mean that you are passing a string that is not
'\0'-terminated to a function that expects a '\0'-terminated string ?

Bart.

Re: [RFC PATCH 1/5] libfcoe, fcoe: Allow user to set a ctlr's mode

[Love, Robert W]

On Mon 10 Sep 2012 10:51:41 PM PDT, Bart Van Assche wrote:
> On 09/11/12 00:59, Robert Love wrote:
>> +static enum fip_conn_type fcoe_parse_mode(const char *buf,
>> +				  const struct fcoe_ctlr_mode_table *tbl)
>> +{
>> +	int modeint = -1, i, rv;
>> +	char *p, modestr[FCOE_MAX_MODENAME_LEN + 1] = { 0, };
>> +
>> +	for (p = (char *)buf; *p; p++)
>> +		if (!(isdigit(*p) || isspace(*p)))
>> +			break;
>
> If you change the declaration of p from "char *p" into "const char *p"
> you won't need a cast in the above for loop.
>
> [ ... ]
>
>> -static FCOE_DEVICE_ATTR(ctlr, mode, S_IRUGO,
>> -			show_ctlr_mode, NULL);
>> +
>> +static ssize_t store_ctlr_mode(struct device *dev,
>> +			       struct device_attribute *attr,
>> +			       const char *buf, size_t count)
>> +{
>> +	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
>> +
>> +	if (!ctlr->f->set_fcoe_ctlr_mode)
>> +		return -EINVAL;
>> +
>> +	ctlr->mode = fcoe_parse_mode(buf, ctlr_mode_tbl);
>
> As far as I know sysfs doesn't terminate buf with a '\0' before calling
> a store method. Does that mean that you are passing a string that is not
> '\0'-terminated to a function that expects a '\0'-terminated string ?
>
> Bart.

Hey Bart. I just wanted to acknowledge your comments. I will make sure 
that they're addressed after figuring out if sysfs is the right place 
for the interfaces.

Thanks, //Rob

[RFC PATCH 2/5] libfcoe: Create new libfcoe control interfaces

[Robert Love]

This patch is the first in a series that will remove
libfcoe's create, destroy, enable and disable module
parameters and replace them with interface files in
the new /sys/bus/fcoe subsystem.

Old layout:

/sys/module/libfcoe/parameters/{create,destroy,enable,disable,vn2vn_create}

New layout:

/sys/bus/fcoe/ctlr_{create,destroy}
/sys/bus/fcoe/ctlr_X/{enable,disable,start}

This patch moves fcoe drivers to the following
initialization sequence-

1) create/alloc
2) configure
3) start

A control sysfs interface at /sys/bus/fcoe/ctlr_create
is added. Writing the interface name to this file
will allocate memory and create a sysfs entry for a
new fcoe_ctlr_device. The user may then tune the interface in
any desired way. After configuration the user will
echo any value into the /sys/bus/fcoe/devices/ctlr_X/start
interface to proceed with logging in.

VN2VN logins will still use the module parameters.
A follow up patch to this one will make the 'mode'
attribute of the fcoe_ctlr_device writable. Which will
allow a user to change the ctlr's mode to 'VN2VN'.

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-fcoe |   43 ++++++++++++
 drivers/scsi/fcoe/fcoe.h                 |    9 +++
 drivers/scsi/fcoe/fcoe_ctlr.c            |    2 -
 drivers/scsi/fcoe/fcoe_sysfs.c           |   78 ++++++++++++++++++++++
 drivers/scsi/fcoe/fcoe_transport.c       |  105 +++++++++++++++++++++++++++++-
 include/scsi/fcoe_sysfs.h                |    4 +
 include/scsi/libfcoe.h                   |   14 ++++
 7 files changed, 250 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-fcoe b/Documentation/ABI/testing/sysfs-bus-fcoe
index 1b06ec1..242d4a1 100644
--- a/Documentation/ABI/testing/sysfs-bus-fcoe
+++ b/Documentation/ABI/testing/sysfs-bus-fcoe
@@ -1,8 +1,37 @@
+What:		/sys/bus/fcoe/
+Date:		August 2012
+KernelVersion:	TBD
+Contact:	Robert Love <robert.w.love@intel.com>, devel@open-fcoe.org
+Description:	The FCoE bus. Attributes in this directory are control interfaces.
+Attributes:
+
+	ctlr_create: 'FCoE Controller' instance creation interface. Writing an
+		     <ifname> to this file will allocate and populate sysfs with a
+		     fcoe_ctlr_device (ctlr_X). The user can then configure any
+		     per-port settings and finally write to the fcoe_ctlr_device's
+		     'start' attribute to begin the kernel's discovery and login
+		     process.
+
+	ctlr_destroy: 'FCoE Controller' instance removal interface. Writing a
+		       fcoe_ctlr_device's sysfs name to this file will log the
+		       fcoe_ctlr_device out of the fabric or otherwise connected
+		       FCoE devices. It will also free all kernel memory allocated
+		       for this fcoe_ctlr_device and any structures associated
+		       with it, this includes the scsi_host.
+
 What:		/sys/bus/fcoe/ctlr_X
 Date:		March 2012
 KernelVersion:	TBD
 Contact:	Robert Love <robert.w.love@intel.com>, devel@open-fcoe.org
-Description:	'FCoE Controller' instances on the fcoe bus
+Description:	'FCoE Controller' instances on the fcoe bus.
+
+		The FCoE Controller now has a three stage creation process.
+		1) Write interface name to ctlr_create 2) Configure the FCoE
+		Controller (ctlr_X) 3) Write anything to the FCoE
+		Controller's 'start' file to begin discovery and login. The
+		FCoE Controller is destroyed by writing it's name, i.e. ctlr_X
+		to the ctlr_delete file.
+
 Attributes:
 
 	fcf_dev_loss_tmo: Device loss timeout peroid (see below). Changing
@@ -17,6 +46,18 @@ Attributes:
 			  FIP VN2VN discovery and login is performed. A FCoE
 			  Controller only supports one mode at a time.
 
+	start:		  Start the FCoE controller.
+
+	disable:	  Allow FCoE controller's that support disabling
+			  to be disabled when any value is written to this
+			  file. This attribute is only displayed if the
+			  driver supports it.
+
+	enable:           Allow FCoE controller's that support enabling
+			  to be enabled when any value is written to this
+			  file. This attribute is only displayed if the
+			  driver supports it.
+
 	lesb_link_fail:   Link Error Status Block (LESB) link failure count.
 
 	lesb_vlink_fail:  Link Error Status Block (LESB) virtual link
diff --git a/drivers/scsi/fcoe/fcoe.h b/drivers/scsi/fcoe/fcoe.h
index a624add..e984627 100644
--- a/drivers/scsi/fcoe/fcoe.h
+++ b/drivers/scsi/fcoe/fcoe.h
@@ -22,6 +22,7 @@
 
 #include <linux/skbuff.h>
 #include <linux/kthread.h>
+#include <scsi/libfc.h>
 
 #define FCOE_MAX_QUEUE_DEPTH	256
 #define FCOE_MIN_QUEUE_DEPTH	32
@@ -99,4 +100,12 @@ static inline struct net_device *fcoe_netdev(const struct fc_lport *lport)
 			((struct fcoe_port *)lport_priv(lport))->priv)->netdev;
 }
 
+struct net_device *fcoe_ctlr_to_netdev(
+	const struct fcoe_ctlr_device *ctlr_dev)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(ctlr_dev);
+	struct fc_lport *lport = ctlr->lp;
+	return fcoe_netdev(lport);
+}
+
 #endif /* _FCOE_H_ */
diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index bd899bf..ccb92323 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -147,7 +147,7 @@ static void fcoe_ctlr_map_dest(struct fcoe_ctlr *fip)
  */
 void fcoe_ctlr_init(struct fcoe_ctlr *fip, enum fip_state mode)
 {
-	fcoe_ctlr_set_state(fip, FIP_ST_LINK_WAIT);
+	fcoe_ctlr_set_state(fip, FIP_ST_DISABLED);
 	fip->mode = mode;
 	INIT_LIST_HEAD(&fip->fcfs);
 	mutex_init(&fip->ctlr_mutex);
diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
index 3fbc556..d2f8cf9 100644
--- a/drivers/scsi/fcoe/fcoe_sysfs.c
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -24,6 +24,7 @@
 #include <linux/ctype.h>
 
 #include <scsi/fcoe_sysfs.h>
+#include <scsi/libfcoe.h>
 
 static atomic_t ctlr_num;
 static atomic_t fcf_num;
@@ -41,6 +42,9 @@ MODULE_PARM_DESC(fcf_dev_loss_tmo,
 		 " insulate the loss of a fcf. Once this value is"
 		 " exceeded, the fcf is removed.");
 
+BUS_ATTR(ctlr_create, S_IWUSR, NULL, fcoe_ctlr_create_store);
+BUS_ATTR(ctlr_destroy, S_IWUSR, NULL, fcoe_ctlr_destroy_store);
+
 #define FCOE_MAX_MODENAME_LEN 20
 struct fcoe_ctlr_mode_table {
 	char *modename;
@@ -333,6 +337,65 @@ static ssize_t store_ctlr_mode(struct device *dev,
 static FCOE_DEVICE_ATTR(ctlr, mode, S_IRUGO | S_IWUSR,
 			show_ctlr_mode, store_ctlr_mode);
 
+static ssize_t store_ctlr_start(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
+	int rc;
+
+	if (ctlr->started == 1)
+		return -EINVAL;
+
+	rc = ctlr->f->set_fcoe_ctlr_start(ctlr);
+	if (rc)
+		return rc;
+
+	ctlr->started = 1;
+
+	return count;
+}
+static FCOE_DEVICE_ATTR(ctlr, start, S_IWUSR,
+			NULL, store_ctlr_start);
+
+static ssize_t store_ctlr_enable(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
+	int rc;
+
+	if (ctlr->started == 1)
+		return -EINVAL;
+
+	rc = ctlr->f->set_fcoe_ctlr_enable(ctlr);
+	if (rc)
+		return rc;
+
+	return count;
+}
+static FCOE_DEVICE_ATTR(ctlr, enable, S_IWUSR,
+			NULL, store_ctlr_enable);
+
+static ssize_t store_ctlr_disable(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf, size_t count)
+{
+	struct fcoe_ctlr_device *ctlr = dev_to_ctlr(dev);
+	int rc;
+
+	if (ctlr->started == 1)
+		return -EINVAL;
+
+	rc = ctlr->f->set_fcoe_ctlr_disable(ctlr);
+	if (rc)
+		return rc;
+
+	return count;
+}
+static FCOE_DEVICE_ATTR(ctlr, disable, S_IWUSR,
+			NULL, store_ctlr_disable);
+
 static ssize_t
 store_private_fcoe_ctlr_fcf_dev_loss_tmo(struct device *dev,
 					 struct device_attribute *attr,
@@ -416,6 +479,9 @@ static struct attribute_group fcoe_ctlr_lesb_attr_group = {
 
 static struct attribute *fcoe_ctlr_attrs[] = {
 	&device_attr_fcoe_ctlr_fcf_dev_loss_tmo.attr,
+	&device_attr_fcoe_ctlr_start.attr,
+	&device_attr_fcoe_ctlr_enable.attr,
+	&device_attr_fcoe_ctlr_disable.attr,
 	&device_attr_fcoe_ctlr_mode.attr,
 	NULL,
 };
@@ -880,6 +946,18 @@ int __init fcoe_sysfs_setup(void)
 	if (error)
 		return error;
 
+	error = bus_create_file(&fcoe_bus_type, &bus_attr_ctlr_create);
+	if (error) {
+		bus_unregister(&fcoe_bus_type);
+		return error;
+	}
+
+	error = bus_create_file(&fcoe_bus_type, &bus_attr_ctlr_destroy);
+	if (error) {
+		bus_unregister(&fcoe_bus_type);
+		return error;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/scsi/fcoe/fcoe_transport.c b/drivers/scsi/fcoe/fcoe_transport.c
index b46f43d..8e50d9a 100644
--- a/drivers/scsi/fcoe/fcoe_transport.c
+++ b/drivers/scsi/fcoe/fcoe_transport.c
@@ -515,9 +515,8 @@ static int __exit fcoe_transport_exit(void)
 	return 0;
 }
 
-
 static int fcoe_add_netdev_mapping(struct net_device *netdev,
-					struct fcoe_transport *ft)
+				   struct fcoe_transport *ft)
 {
 	struct fcoe_netdev_mapping *nm;
 
@@ -627,6 +626,108 @@ static int libfcoe_device_notification(struct notifier_block *notifier,
 	return NOTIFY_OK;
 }
 
+ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
+			       const char *buf, size_t count)
+{
+	struct net_device *netdev = NULL;
+	struct fcoe_transport *ft = NULL;
+	struct fcoe_ctlr_device *ctlr_dev = NULL;
+	int rc = -ENODEV;
+	int err;
+
+	mutex_lock(&ft_mutex);
+
+	netdev = fcoe_if_to_netdev(buf);
+	if (!netdev) {
+		LIBFCOE_TRANSPORT_DBG("Invalid device %s.\n", buf);
+		rc = -ENODEV;
+		goto out_nodev;
+	}
+
+	ft = fcoe_netdev_map_lookup(netdev);
+	if (ft) {
+		LIBFCOE_TRANSPORT_DBG("transport %s already has existing "
+				      "FCoE instance on %s.\n",
+				      ft->name, netdev->name);
+		rc = -EEXIST;
+		goto out_putdev;
+	}
+
+	ft = fcoe_transport_lookup(netdev);
+	if (!ft) {
+		LIBFCOE_TRANSPORT_DBG("no FCoE transport found for %s.\n",
+				      netdev->name);
+		rc = -ENODEV;
+		goto out_putdev;
+	}
+
+	/* pass to transport create */
+	err = ft->alloc ? ft->alloc(netdev) : -ENODEV;
+	if (err) {
+		fcoe_del_netdev_mapping(netdev);
+		rc = -ENOMEM;
+		goto out_putdev;
+	}
+
+	err = fcoe_add_netdev_mapping(netdev, ft);
+	if (err) {
+		LIBFCOE_TRANSPORT_DBG("failed to add new netdev mapping "
+				      "for FCoE transport %s for %s.\n",
+				      ft->name, netdev->name);
+		rc = -ENODEV;
+		goto out_putdev;
+	}
+
+	LIBFCOE_TRANSPORT_DBG("transport %s %s to create fcoe on %s.\n",
+			      ft->name, (ctlr_dev) ? "succeeded" : "failed",
+			      netdev->name);
+
+out_putdev:
+	dev_put(netdev);
+out_nodev:
+	mutex_unlock(&ft_mutex);
+	return rc;
+}
+
+ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
+				const char *buf, size_t count)
+{
+	int rc = -ENODEV;
+	struct net_device *netdev = NULL;
+	struct fcoe_transport *ft = NULL;
+
+	mutex_lock(&ft_mutex);
+
+	netdev = fcoe_if_to_netdev(buf);
+	if (!netdev) {
+		LIBFCOE_TRANSPORT_DBG("invalid device %s.\n", buf);
+		goto out_nodev;
+	}
+
+	ft = fcoe_netdev_map_lookup(netdev);
+	if (!ft) {
+		LIBFCOE_TRANSPORT_DBG("no FCoE transport found for %s.\n",
+				      netdev->name);
+		goto out_putdev;
+	}
+
+	/* pass to transport destroy */
+	rc = ft->destroy(netdev);
+	if (rc)
+		goto out_putdev;
+
+	fcoe_del_netdev_mapping(netdev);
+	LIBFCOE_TRANSPORT_DBG("transport %s %s to destroy fcoe on %s.\n",
+			      ft->name, (rc) ? "failed" : "succeeded",
+			      netdev->name);
+	rc = count; /* required for successful return */
+out_putdev:
+	dev_put(netdev);
+out_nodev:
+	mutex_unlock(&ft_mutex);
+	return rc;
+}
+EXPORT_SYMBOL(fcoe_ctlr_destroy_store);
 
 /**
  * fcoe_transport_create() - Create a fcoe interface
diff --git a/include/scsi/fcoe_sysfs.h b/include/scsi/fcoe_sysfs.h
index 421ae67..8c5ea70 100644
--- a/include/scsi/fcoe_sysfs.h
+++ b/include/scsi/fcoe_sysfs.h
@@ -36,6 +36,9 @@ struct fcoe_sysfs_function_template {
 	void (*get_fcoe_ctlr_fcs_error)(struct fcoe_ctlr_device *);
 	void (*get_fcoe_ctlr_mode)(struct fcoe_ctlr_device *);
 	void (*set_fcoe_ctlr_mode)(struct fcoe_ctlr_device *);
+	int  (*set_fcoe_ctlr_start)(struct fcoe_ctlr_device *);
+	int  (*set_fcoe_ctlr_enable)(struct fcoe_ctlr_device *);
+	int  (*set_fcoe_ctlr_disable)(struct fcoe_ctlr_device *);
 	void (*get_fcoe_fcf_selected)(struct fcoe_fcf_device *);
 	void (*get_fcoe_fcf_vlan_id)(struct fcoe_fcf_device *);
 };
@@ -64,6 +67,7 @@ struct fcoe_ctlr_device {
 
 	int                             fcf_dev_loss_tmo;
 	enum fip_conn_type              mode;
+	u8                              started:1;
 
 	/* expected in host order for displaying */
 	struct fcoe_fc_els_lesb         lesb;
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 20533cc..b19a489 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -289,8 +289,11 @@ static inline bool is_fip_mode(struct fcoe_ctlr *fip)
  * @attached:	whether this transport is already attached
  * @list:	list linkage to all attached transports
  * @match:	handler to allow the transport driver to match up a given netdev
+ * @alloc:      handler to allocate per-instance FCoE structures
+ *		(no discovery or login)
  * @create:	handler to sysfs entry of create for FCoE instances
- * @destroy:	handler to sysfs entry of destroy for FCoE instances
+ * @destroy:    handler to delete per-instance FCoE structures
+ *		(frees all memory)
  * @enable:	handler to sysfs entry of enable for FCoE instances
  * @disable:	handler to sysfs entry of disable for FCoE instances
  */
@@ -299,6 +302,7 @@ struct fcoe_transport {
 	bool attached;
 	struct list_head list;
 	bool (*match) (struct net_device *device);
+	int (*alloc) (struct net_device *device);
 	int (*create) (struct net_device *device, enum fip_state fip_mode);
 	int (*destroy) (struct net_device *device);
 	int (*enable) (struct net_device *device);
@@ -375,4 +379,12 @@ struct fcoe_netdev_mapping {
 int fcoe_transport_attach(struct fcoe_transport *ft);
 int fcoe_transport_detach(struct fcoe_transport *ft);
 
+/* sysfs store handler for ctrl_control interface */
+ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
+			       const char *buf, size_t count);
+ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
+				const char *buf, size_t count);
+
 #endif /* _LIBFCOE_H */
+
+

Re: [RFC PATCH 2/5] libfcoe: Create new libfcoe control interfaces

[Bhanu Prakash Gollapudi]

On 9/10/2012 3:59 PM, Robert Love wrote:
> This patch is the first in a series that will remove
> libfcoe's create, destroy, enable and disable module
> parameters and replace them with interface files in
> the new /sys/bus/fcoe subsystem.
>
> Old layout:
>
> /sys/module/libfcoe/parameters/{create,destroy,enable,disable,vn2vn_create}
>
> New layout:
>
> /sys/bus/fcoe/ctlr_{create,destroy}
> /sys/bus/fcoe/ctlr_X/{enable,disable,start}
>
> This patch moves fcoe drivers to the following
> initialization sequence-
>
> 1) create/alloc
> 2) configure
> 3) start
>
> A control sysfs interface at /sys/bus/fcoe/ctlr_create
> is added. Writing the interface name to this file
> will allocate memory and create a sysfs entry for a
> new fcoe_ctlr_device. The user may then tune the interface in
> any desired way. After configuration the user will
> echo any value into the /sys/bus/fcoe/devices/ctlr_X/start
> interface to proceed with logging in.
>
> VN2VN logins will still use the module parameters.
> A follow up patch to this one will make the 'mode'
> attribute of the fcoe_ctlr_device writable. Which will
> allow a user to change the ctlr's mode to 'VN2VN'.
>
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> ---
>   Documentation/ABI/testing/sysfs-bus-fcoe |   43 ++++++++++++
>   drivers/scsi/fcoe/fcoe.h                 |    9 +++
>   drivers/scsi/fcoe/fcoe_ctlr.c            |    2 -
>   drivers/scsi/fcoe/fcoe_sysfs.c           |   78 ++++++++++++++++++++++
>   drivers/scsi/fcoe/fcoe_transport.c       |  105 +++++++++++++++++++++++++++++-
>   include/scsi/fcoe_sysfs.h                |    4 +
>   include/scsi/libfcoe.h                   |   14 ++++
>   7 files changed, 250 insertions(+), 5 deletions(-)
>

<snip>

> diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
> index bd899bf..ccb92323 100644
> --- a/drivers/scsi/fcoe/fcoe_ctlr.c
> +++ b/drivers/scsi/fcoe/fcoe_ctlr.c
> @@ -147,7 +147,7 @@ static void fcoe_ctlr_map_dest(struct fcoe_ctlr *fip)
>    */
>   void fcoe_ctlr_init(struct fcoe_ctlr *fip, enum fip_state mode)
>   {
> -	fcoe_ctlr_set_state(fip, FIP_ST_LINK_WAIT);
> +	fcoe_ctlr_set_state(fip, FIP_ST_DISABLED);

Robert, what is the reason for initializing it to DISABLED? Unless the 
FIP state is FIP_ST_LINK_WAIT, fcoe_ctlr_link_up() doesnt set 
lport->link_up and hence does not allow any FIP/FCoE frames to be sent out.

>   	fip->mode = mode;
>   	INIT_LIST_HEAD(&fip->fcfs);
>   	mutex_init(&fip->ctlr_mutex);

<snip>

> @@ -627,6 +626,108 @@ static int libfcoe_device_notification(struct notifier_block *notifier,
>   	return NOTIFY_OK;
>   }
>
> +ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
> +			       const char *buf, size_t count)
> +{
> +	struct net_device *netdev = NULL;
> +	struct fcoe_transport *ft = NULL;
> +	struct fcoe_ctlr_device *ctlr_dev = NULL;
> +	int rc = -ENODEV;
> +	int err;
> +
> +	mutex_lock(&ft_mutex);
> +
> +	netdev = fcoe_if_to_netdev(buf);
> +	if (!netdev) {
> +		LIBFCOE_TRANSPORT_DBG("Invalid device %s.\n", buf);
> +		rc = -ENODEV;
> +		goto out_nodev;
> +	}
> +
> +	ft = fcoe_netdev_map_lookup(netdev);
> +	if (ft) {
> +		LIBFCOE_TRANSPORT_DBG("transport %s already has existing "
> +				      "FCoE instance on %s.\n",
> +				      ft->name, netdev->name);
> +		rc = -EEXIST;
> +		goto out_putdev;
> +	}
> +
> +	ft = fcoe_transport_lookup(netdev);
> +	if (!ft) {
> +		LIBFCOE_TRANSPORT_DBG("no FCoE transport found for %s.\n",
> +				      netdev->name);
> +		rc = -ENODEV;
> +		goto out_putdev;
> +	}
> +
> +	/* pass to transport create */
> +	err = ft->alloc ? ft->alloc(netdev) : -ENODEV;
> +	if (err) {
> +		fcoe_del_netdev_mapping(netdev);
> +		rc = -ENOMEM;
> +		goto out_putdev;
> +	}
> +
> +	err = fcoe_add_netdev_mapping(netdev, ft);
> +	if (err) {
> +		LIBFCOE_TRANSPORT_DBG("failed to add new netdev mapping "
> +				      "for FCoE transport %s for %s.\n",
> +				      ft->name, netdev->name);
> +		rc = -ENODEV;
> +		goto out_putdev;
> +	}
> +
> +	LIBFCOE_TRANSPORT_DBG("transport %s %s to create fcoe on %s.\n",
> +			      ft->name, (ctlr_dev) ? "succeeded" : "failed",
> +			      netdev->name);

Where is ctlr_dev updated? I guess you're intending to check return 
status of ft->alloc() here.

> +
> +out_putdev:
> +	dev_put(netdev);
> +out_nodev:
> +	mutex_unlock(&ft_mutex);
> +	return rc;
> +}
> +
> +ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
> +				const char *buf, size_t count)
> +{
> +	int rc = -ENODEV;
> +	struct net_device *netdev = NULL;
> +	struct fcoe_transport *ft = NULL;
> +
> +	mutex_lock(&ft_mutex);
> +
> +	netdev = fcoe_if_to_netdev(buf);
> +	if (!netdev) {
> +		LIBFCOE_TRANSPORT_DBG("invalid device %s.\n", buf);
> +		goto out_nodev;
> +	}
> +
> +	ft = fcoe_netdev_map_lookup(netdev);
> +	if (!ft) {
> +		LIBFCOE_TRANSPORT_DBG("no FCoE transport found for %s.\n",
> +				      netdev->name);
> +		goto out_putdev;
> +	}
> +
> +	/* pass to transport destroy */
> +	rc = ft->destroy(netdev);
> +	if (rc)
> +		goto out_putdev;
> +
> +	fcoe_del_netdev_mapping(netdev);
> +	LIBFCOE_TRANSPORT_DBG("transport %s %s to destroy fcoe on %s.\n",
> +			      ft->name, (rc) ? "failed" : "succeeded",
> +			      netdev->name);
> +	rc = count; /* required for successful return */
> +out_putdev:
> +	dev_put(netdev);
> +out_nodev:
> +	mutex_unlock(&ft_mutex);
> +	return rc;
> +}
> +EXPORT_SYMBOL(fcoe_ctlr_destroy_store);
>
>   /**
>    * fcoe_transport_create() - Create a fcoe interface
> diff --git a/include/scsi/fcoe_sysfs.h b/include/scsi/fcoe_sysfs.h
> index 421ae67..8c5ea70 100644
> --- a/include/scsi/fcoe_sysfs.h
> +++ b/include/scsi/fcoe_sysfs.h
> @@ -36,6 +36,9 @@ struct fcoe_sysfs_function_template {
>   	void (*get_fcoe_ctlr_fcs_error)(struct fcoe_ctlr_device *);
>   	void (*get_fcoe_ctlr_mode)(struct fcoe_ctlr_device *);
>   	void (*set_fcoe_ctlr_mode)(struct fcoe_ctlr_device *);
> +	int  (*set_fcoe_ctlr_start)(struct fcoe_ctlr_device *);
> +	int  (*set_fcoe_ctlr_enable)(struct fcoe_ctlr_device *);
> +	int  (*set_fcoe_ctlr_disable)(struct fcoe_ctlr_device *);
>   	void (*get_fcoe_fcf_selected)(struct fcoe_fcf_device *);
>   	void (*get_fcoe_fcf_vlan_id)(struct fcoe_fcf_device *);
>   };
> @@ -64,6 +67,7 @@ struct fcoe_ctlr_device {
>
>   	int                             fcf_dev_loss_tmo;
>   	enum fip_conn_type              mode;
> +	u8                              started:1;
>
>   	/* expected in host order for displaying */
>   	struct fcoe_fc_els_lesb         lesb;
> diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
> index 20533cc..b19a489 100644
> --- a/include/scsi/libfcoe.h
> +++ b/include/scsi/libfcoe.h
> @@ -289,8 +289,11 @@ static inline bool is_fip_mode(struct fcoe_ctlr *fip)
>    * @attached:	whether this transport is already attached
>    * @list:	list linkage to all attached transports
>    * @match:	handler to allow the transport driver to match up a given netdev
> + * @alloc:      handler to allocate per-instance FCoE structures
> + *		(no discovery or login)
>    * @create:	handler to sysfs entry of create for FCoE instances
> - * @destroy:	handler to sysfs entry of destroy for FCoE instances
> + * @destroy:    handler to delete per-instance FCoE structures
> + *		(frees all memory)
>    * @enable:	handler to sysfs entry of enable for FCoE instances
>    * @disable:	handler to sysfs entry of disable for FCoE instances
>    */
> @@ -299,6 +302,7 @@ struct fcoe_transport {
>   	bool attached;
>   	struct list_head list;
>   	bool (*match) (struct net_device *device);
> +	int (*alloc) (struct net_device *device);
>   	int (*create) (struct net_device *device, enum fip_state fip_mode);
>   	int (*destroy) (struct net_device *device);
>   	int (*enable) (struct net_device *device);
> @@ -375,4 +379,12 @@ struct fcoe_netdev_mapping {
>   int fcoe_transport_attach(struct fcoe_transport *ft);
>   int fcoe_transport_detach(struct fcoe_transport *ft);
>
> +/* sysfs store handler for ctrl_control interface */
> +ssize_t fcoe_ctlr_create_store(struct bus_type *bus,
> +			       const char *buf, size_t count);
> +ssize_t fcoe_ctlr_destroy_store(struct bus_type *bus,
> +				const char *buf, size_t count);
> +
>   #endif /* _LIBFCOE_H */
> +
> +
>
>

[RFC PATCH 3/5] fcoe: Use new fcoe_sysfs control interface

[Robert Love]

Convert fcoe to use the new fcoe_sysfs create, delete,
enable, disable, start and mode.

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |  229 +++++++++++++++++++++++-----------------------
 1 file changed, 115 insertions(+), 114 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index fe30b1b..fe9ce98 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -112,10 +112,10 @@ static int fcoe_dcb_app_notification(struct notifier_block *notifier,
 				     ulong event, void *ptr);
 
 static bool fcoe_match(struct net_device *netdev);
-static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode);
+static int fcoe_start(struct fcoe_ctlr_device *cdev);
 static int fcoe_destroy(struct net_device *netdev);
-static int fcoe_enable(struct net_device *netdev);
-static int fcoe_disable(struct net_device *netdev);
+static int fcoe_enable(struct fcoe_ctlr_device *cdev);
+static int fcoe_disable(struct fcoe_ctlr_device *cdev);
 
 static struct fc_seq *fcoe_elsct_send(struct fc_lport *,
 				      u32 did, struct fc_frame *,
@@ -155,6 +155,10 @@ static void fcoe_ctlr_get_lesb(struct fcoe_ctlr_device *);
 static void fcoe_fcf_get_vlan_id(struct fcoe_fcf_device *);
 
 static struct fcoe_sysfs_function_template fcoe_sysfs_templ = {
+	.set_fcoe_ctlr_mode = fcoe_ctlr_set_fip_mode,
+	.set_fcoe_ctlr_start = fcoe_start,
+	.set_fcoe_ctlr_enable = fcoe_enable,
+	.set_fcoe_ctlr_disable = fcoe_disable,
 	.get_fcoe_ctlr_mode = fcoe_ctlr_get_fip_mode,
 	.get_fcoe_ctlr_link_fail = fcoe_ctlr_get_lesb,
 	.get_fcoe_ctlr_vlink_fail = fcoe_ctlr_get_lesb,
@@ -2034,105 +2038,6 @@ out:
 }
 
 /**
- * fcoe_disable() - Disables a FCoE interface
- * @netdev  : The net_device object the Ethernet interface to create on
- *
- * Called from fcoe transport.
- *
- * Returns: 0 for success
- */
-static int fcoe_disable(struct net_device *netdev)
-{
-	struct fcoe_ctlr *ctlr;
-	struct fcoe_interface *fcoe;
-	int rc = 0;
-
-	mutex_lock(&fcoe_config_mutex);
-
-	rtnl_lock();
-	fcoe = fcoe_hostlist_lookup_port(netdev);
-	rtnl_unlock();
-
-	if (fcoe) {
-		ctlr = fcoe_to_ctlr(fcoe);
-		fcoe_ctlr_link_down(ctlr);
-		fcoe_clean_pending_queue(ctlr->lp);
-	} else
-		rc = -ENODEV;
-
-	mutex_unlock(&fcoe_config_mutex);
-	return rc;
-}
-
-/**
- * fcoe_enable() - Enables a FCoE interface
- * @netdev  : The net_device object the Ethernet interface to create on
- *
- * Called from fcoe transport.
- *
- * Returns: 0 for success
- */
-static int fcoe_enable(struct net_device *netdev)
-{
-	struct fcoe_ctlr *ctlr;
-	struct fcoe_interface *fcoe;
-	int rc = 0;
-
-	mutex_lock(&fcoe_config_mutex);
-	rtnl_lock();
-	fcoe = fcoe_hostlist_lookup_port(netdev);
-	rtnl_unlock();
-
-	if (!fcoe) {
-		rc = -ENODEV;
-		goto out;
-	}
-
-	ctlr = fcoe_to_ctlr(fcoe);
-
-	if (!fcoe_link_ok(ctlr->lp))
-		fcoe_ctlr_link_up(ctlr);
-
-out:
-	mutex_unlock(&fcoe_config_mutex);
-	return rc;
-}
-
-/**
- * fcoe_destroy() - Destroy a FCoE interface
- * @netdev  : The net_device object the Ethernet interface to create on
- *
- * Called from fcoe transport
- *
- * Returns: 0 for success
- */
-static int fcoe_destroy(struct net_device *netdev)
-{
-	struct fcoe_ctlr *ctlr;
-	struct fcoe_interface *fcoe;
-	struct fc_lport *lport;
-	struct fcoe_port *port;
-	int rc = 0;
-
-	mutex_lock(&fcoe_config_mutex);
-	rtnl_lock();
-	fcoe = fcoe_hostlist_lookup_port(netdev);
-	if (!fcoe) {
-		rc = -ENODEV;
-		goto out_nodev;
-	}
-	ctlr = fcoe_to_ctlr(fcoe);
-	lport = ctlr->lp;
-	port = lport_priv(lport);
-	list_del(&fcoe->list);
-	queue_work(fcoe_wq, &port->destroy_work);
-out_nodev:
-	rtnl_unlock();
-	mutex_unlock(&fcoe_config_mutex);
-	return rc;
-}
-
-/**
  * fcoe_destroy_work() - Destroy a FCoE port in a deferred work context
  * @work: Handle to the FCoE port to be destroyed
  */
@@ -2207,18 +2112,17 @@ static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 }
 
 /**
- * fcoe_create() - Create a fcoe interface
+ * fcoe_alloc() - Allocate memory for a fcoe interface
  * @netdev  : The net_device object the Ethernet interface to create on
- * @fip_mode: The FIP mode for this creation
  *
  * Called from fcoe transport
  *
  * Returns: 0 for success
  */
-static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
+static int fcoe_alloc(struct net_device *netdev)
 {
 	int rc = 0;
-	struct fcoe_ctlr_device *ctlr_dev;
+	struct fcoe_ctlr_device *ctlr_dev = NULL;
 	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fc_lport *lport;
@@ -2232,7 +2136,10 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 		goto out_nodev;
 	}
 
-	fcoe = fcoe_interface_create(netdev, fip_mode);
+	/*
+	 * Create the interface with the default 'Fabric' mode.
+	 */
+	fcoe = fcoe_interface_create(netdev, FIP_MODE_FABRIC);
 	if (IS_ERR(fcoe)) {
 		rc = PTR_ERR(fcoe);
 		goto out_nodev;
@@ -2250,14 +2157,36 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 		goto out_nortnl;
 	}
 
+	/* add to lports list */
+	fcoe_hostlist_add(lport);
+
 	/* Make this the "master" N_Port */
 	ctlr->lp = lport;
 
 	/* setup DCB priority attributes. */
 	fcoe_dcb_create(fcoe);
 
-	/* add to lports list */
-	fcoe_hostlist_add(lport);
+out_nodev:
+	rtnl_unlock();
+out_nortnl:
+	mutex_unlock(&fcoe_config_mutex);
+	return rc;
+}
+
+/**
+ * fcoe_start() - Start the controller on a fcoe interface
+ * @cdev  : The FCoE Controller Device to start
+ *
+ * Returns: 0 for success
+ */
+int fcoe_start(struct fcoe_ctlr_device *cdev)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
+	struct fc_lport *lport = ctlr->lp;
+	int rc = 0;
+
+	mutex_lock(&fcoe_config_mutex);
+	rtnl_lock();
 
 	/* start FIP Discovery and FLOGI */
 	lport->boot_time = jiffies;
@@ -2269,14 +2198,88 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 		return rc;
 	}
 
-out_nodev:
 	rtnl_unlock();
-out_nortnl:
 	mutex_unlock(&fcoe_config_mutex);
+
 	return rc;
 }
 
 /**
+ * fcoe_destroy() - Destroy the controller on a fcoe interface
+ * @netdev: The netdevice associated with the controller to destroy
+ *
+ * Called from fcoe transport
+ *
+ * Returns: 0 for success
+ */
+int fcoe_destroy(struct net_device *netdev)
+{
+	struct fcoe_ctlr *ctlr;
+	struct fcoe_interface *fcoe;
+	struct fcoe_port *port;
+	int rc = 0;
+
+	mutex_lock(&fcoe_config_mutex);
+	rtnl_lock();
+
+	fcoe = fcoe_hostlist_lookup_port(netdev);
+	if (!fcoe) {
+		rc = -ENODEV;
+		goto out_nodev;
+	}
+
+	ctlr = fcoe_to_ctlr(fcoe);
+	port = lport_priv(ctlr->lp);
+	list_del(&fcoe->list);
+	queue_work(fcoe_wq, &port->destroy_work);
+
+out_nodev:
+	rtnl_unlock();
+	mutex_unlock(&fcoe_config_mutex);
+	return 0;
+}
+
+/**
+ * fcoe_enable() - Enable the controller on a fcoe interface
+ * @cdev  : The FCoE Controller Device to enable
+ *
+ * Returns: 0 for success
+ */
+int fcoe_enable(struct fcoe_ctlr_device *cdev)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
+
+	mutex_lock(&fcoe_config_mutex);
+
+	if (!fcoe_link_ok(ctlr->lp))
+		fcoe_ctlr_link_up(ctlr);
+
+	mutex_unlock(&fcoe_config_mutex);
+
+	return 0;
+}
+
+/**
+ * fcoe_disable() - Disable the controller on a fcoe interface
+ * @cdev  : The FCoE Controller Device to disable
+ *
+ * Returns: 0 for success
+ */
+int fcoe_disable(struct fcoe_ctlr_device *cdev)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
+
+	mutex_lock(&fcoe_config_mutex);
+
+	fcoe_ctlr_link_down(ctlr);
+	fcoe_clean_pending_queue(ctlr->lp);
+
+	mutex_unlock(&fcoe_config_mutex);
+
+	return 0;
+}
+
+/**
  * fcoe_link_speed_update() - Update the supported and actual link speeds
  * @lport: The local port to update speeds for
  *
@@ -2453,10 +2456,8 @@ static struct fcoe_transport fcoe_sw_transport = {
 	.attached = false,
 	.list = LIST_HEAD_INIT(fcoe_sw_transport.list),
 	.match = fcoe_match,
-	.create = fcoe_create,
+	.alloc = fcoe_alloc,
 	.destroy = fcoe_destroy,
-	.enable = fcoe_enable,
-	.disable = fcoe_disable,
 };
 
 /**

[RFC PATCH 4/5] bnx2fc: Use new fcoe_sysfs control interface

[Robert Love]

Convert	bnx2fc to use the new fcoe_sysfs create, delete,
enable, disable, start and mode.

bnx2fc doesn't support VN2VN. bnx2fc will not initialize
the set_fcoe_ctlr_mode routine and therefore its instances
will always be in FABRIC mode. There was previously an
explicit check for the ctlr's mode, but this is no longer
needed because not implementing set_fcoe_ctlr_mode implies
that the ctlr cannot change from the FABRIC mode.

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   98 +++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 38 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index f52f668f..560c8c8 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -56,11 +56,12 @@ static struct scsi_host_template bnx2fc_shost_template;
 static struct fc_function_template bnx2fc_transport_function;
 static struct fcoe_sysfs_function_template bnx2fc_fcoe_sysfs_templ;
 static struct fc_function_template bnx2fc_vport_xport_function;
-static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode);
+
+static int bnx2fc_start(struct fcoe_ctlr_device *);
+static int bnx2fc_enable(struct fcoe_ctlr_device *);
+static int bnx2fc_disable(struct fcoe_ctlr_device *);
+
 static void __bnx2fc_destroy(struct bnx2fc_interface *interface);
-static int bnx2fc_destroy(struct net_device *net_device);
-static int bnx2fc_enable(struct net_device *netdev);
-static int bnx2fc_disable(struct net_device *netdev);
 
 static void bnx2fc_recv_frame(struct sk_buff *skb);
 
@@ -1594,8 +1595,7 @@ static void __bnx2fc_destroy(struct bnx2fc_interface *interface)
 /**
  * bnx2fc_destroy - Destroy a bnx2fc FCoE interface
  *
- * @buffer: The name of the Ethernet interface to be destroyed
- * @kp:     The associated kernel parameter
+ * @netdev: The name of the Ethernet interface to be destroyed
  *
  * Called from sysfs.
  *
@@ -1954,10 +1954,11 @@ static void bnx2fc_ulp_init(struct cnic_dev *dev)
 }
 
 
-static int bnx2fc_disable(struct net_device *netdev)
+static int bnx2fc_disable(struct fcoe_ctlr_device *cdev)
 {
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
+	struct net_device *netdev = bnx2fc_netdev(ctlr->lp);
 	struct bnx2fc_interface *interface;
-	struct fcoe_ctlr *ctlr;
 	int rc = 0;
 
 	rtnl_lock();
@@ -1980,10 +1981,11 @@ static int bnx2fc_disable(struct net_device *netdev)
 }
 
 
-static int bnx2fc_enable(struct net_device *netdev)
+static int bnx2fc_enable(struct fcoe_ctlr_device *cdev)
 {
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
+	struct net_device *netdev = bnx2fc_netdev(ctlr->lp);
 	struct bnx2fc_interface *interface;
-	struct fcoe_ctlr *ctlr;
 	int rc = 0;
 
 	rtnl_lock();
@@ -2005,17 +2007,17 @@ static int bnx2fc_enable(struct net_device *netdev)
 }
 
 /**
- * bnx2fc_create - Create bnx2fc FCoE interface
+ * bnx2fc_alloc - Alocate a bnx2fc FCoE interface
  *
- * @buffer: The name of Ethernet interface to create on
- * @kp:     The associated kernel param
+ * @netdev: The name of Ethernet interface to allocate on
  *
  * Called from sysfs.
  *
  * Returns: 0 for success
  */
-static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
+static int bnx2fc_alloc(struct net_device *netdev)
 {
+	struct fcoe_ctlr_device *cdev;
 	struct fcoe_ctlr *ctlr;
 	struct bnx2fc_interface *interface;
 	struct bnx2fc_hba *hba;
@@ -2025,12 +2027,6 @@ static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 	int rc = 0;
 	int vlan_id;
 
-	BNX2FC_MISC_DBG("Entered bnx2fc_create\n");
-	if (fip_mode != FIP_MODE_FABRIC) {
-		printk(KERN_ERR "fip mode not FABRIC\n");
-		return -EIO;
-	}
-
 	rtnl_lock();
 
 	mutex_lock(&bnx2fc_dev_lock);
@@ -2077,13 +2073,17 @@ static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 		goto netdev_err;
 	}
 
-	interface = bnx2fc_interface_create(hba, netdev, fip_mode);
+	/*
+	 * Create the interface with the default 'Fabric' mode.
+	 */
+	interface = bnx2fc_interface_create(hba, netdev, FIP_MODE_FABRIC);
 	if (!interface) {
 		printk(KERN_ERR PFX "bnx2fc_interface_create failed\n");
 		goto ifput_err;
 	}
 
 	ctlr = bnx2fc_to_ctlr(interface);
+	cdev = fcoe_ctlr_to_ctlr_dev(ctlr);
 	interface->vlan_id = vlan_id;
 	interface->vlan_enabled = 1;
 
@@ -2106,20 +2106,6 @@ static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 	/* Add interface to if_list */
 	list_add_tail(&interface->list, &if_list);
 
-	lport->boot_time = jiffies;
-
-	/* Make this master N_port */
-	ctlr->lp = lport;
-
-	if (!bnx2fc_link_ok(lport)) {
-		fcoe_ctlr_link_up(ctlr);
-		fc_host_port_type(lport->host) = FC_PORTTYPE_NPORT;
-		set_bit(ADAPTER_STATE_READY, &interface->hba->adapter_state);
-	}
-
-	BNX2FC_HBA_DBG(lport, "create: START DISC\n");
-	bnx2fc_start_disc(interface);
-	interface->enabled = true;
 	/*
 	 * Release from kref_init in bnx2fc_interface_setup, on success
 	 * lport should be holding a reference taken in bnx2fc_if_create
@@ -2128,6 +2114,7 @@ static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 	/* put netdev that was held while calling dev_get_by_name */
 	mutex_unlock(&bnx2fc_dev_lock);
 	rtnl_unlock();
+
 	return 0;
 
 if_create_err:
@@ -2145,6 +2132,40 @@ mod_err:
 }
 
 /**
+ * bnx2fc_alloc - Alocate a bnx2fc FCoE interface
+ *
+ * @cdev: The FCoE Controller Device to start
+ *
+ * Called from sysfs.
+ *
+ * Returns: 0 for success
+ */
+static int bnx2fc_start(struct fcoe_ctlr_device *cdev)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
+	struct fc_lport *lport = ctlr->lp;
+	struct fcoe_port *port = lport_priv(lport);
+	struct bnx2fc_interface *interface = port->priv;
+
+	lport->boot_time = jiffies;
+
+	/* Make this master N_port */
+	ctlr->lp = lport;
+
+	if (!bnx2fc_link_ok(lport)) {
+		fcoe_ctlr_link_up(ctlr);
+		fc_host_port_type(lport->host) = FC_PORTTYPE_NPORT;
+		set_bit(ADAPTER_STATE_READY, &interface->hba->adapter_state);
+	}
+
+	BNX2FC_HBA_DBG(lport, "create: START DISC\n");
+	bnx2fc_start_disc(interface);
+	interface->enabled = true;
+
+	return 0;
+}
+
+/**
  * bnx2fc_find_hba_for_cnic - maps cnic instance to bnx2fc hba instance
  *
  * @cnic:	Pointer to cnic device instance
@@ -2271,10 +2292,8 @@ static struct fcoe_transport bnx2fc_transport = {
 	.attached = false,
 	.list = LIST_HEAD_INIT(bnx2fc_transport.list),
 	.match = bnx2fc_match,
-	.create = bnx2fc_create,
+	.alloc = bnx2fc_alloc,
 	.destroy = bnx2fc_destroy,
-	.enable = bnx2fc_enable,
-	.disable = bnx2fc_disable,
 };
 
 /**
@@ -2514,6 +2533,9 @@ module_init(bnx2fc_mod_init);
 module_exit(bnx2fc_mod_exit);
 
 static struct fcoe_sysfs_function_template bnx2fc_fcoe_sysfs_templ = {
+	.set_fcoe_ctlr_start = bnx2fc_start,
+	.set_fcoe_ctlr_enable = bnx2fc_enable,
+	.set_fcoe_ctlr_disable = bnx2fc_disable,
 	.get_fcoe_ctlr_mode = fcoe_ctlr_get_fip_mode,
 	.get_fcoe_ctlr_link_fail = bnx2fc_ctlr_get_lesb,
 	.get_fcoe_ctlr_vlink_fail = bnx2fc_ctlr_get_lesb,

Re: [RFC PATCH 4/5] bnx2fc: Use new fcoe_sysfs control interface

[Bhanu Prakash Gollapudi]

On 09/10/2012 03:59 PM, Robert Love wrote:
> Convert	bnx2fc to use the new fcoe_sysfs create, delete,
> enable, disable, start and mode.
>
> bnx2fc doesn't support VN2VN. bnx2fc will not initialize
> the set_fcoe_ctlr_mode routine and therefore its instances
> will always be in FABRIC mode. There was previously an
> explicit check for the ctlr's mode, but this is no longer
> needed because not implementing set_fcoe_ctlr_mode implies
> that the ctlr cannot change from the FABRIC mode.
>
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> ---
>   drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   98 +++++++++++++++++++++++--------------
>   1 file changed, 60 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> index f52f668f..560c8c8 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
<snip>

>   /**
> + * bnx2fc_alloc - Alocate a bnx2fc FCoE interface
> + *
> + * @cdev: The FCoE Controller Device to start
> + *
> + * Called from sysfs.
> + *
> + * Returns: 0 for success
> + */
> +static int bnx2fc_start(struct fcoe_ctlr_device *cdev)
> +{
> +	struct fcoe_ctlr *ctlr = fcoe_ctlr_device_priv(cdev);
> +	struct fc_lport *lport = ctlr->lp;
> +	struct fcoe_port *port = lport_priv(lport);
> +	struct bnx2fc_interface *interface = port->priv;
> +
> +	lport->boot_time = jiffies;
> +
> +	/* Make this master N_port */
> +	ctlr->lp = lport;

ctlr->lp should be set in bnx2fc_alloc() as we access it here in the 
beginning of this function.

> +
> +	if (!bnx2fc_link_ok(lport)) {
> +		fcoe_ctlr_link_up(ctlr);
> +		fc_host_port_type(lport->host) = FC_PORTTYPE_NPORT;
> +		set_bit(ADAPTER_STATE_READY, &interface->hba->adapter_state);
> +	}
> +
> +	BNX2FC_HBA_DBG(lport, "create: START DISC\n");
> +	bnx2fc_start_disc(interface);
I think more changes are required for bnx2fc as fc_lport_init() is 
called just before calling fc_fabric_login() - whcih is called during 
'start'. Because of this, if we just call 'create' followed by 'destroy' 
without calling 'start', lport is not initialized and I expect to see 
some panics when destroy is called.

Let me try testing your patches and send you any fixes that are required.

> +	interface->enabled = true;
> +
> +	return 0;
> +}
> +
> +/**
>    * bnx2fc_find_hba_for_cnic - maps cnic instance to bnx2fc hba instance
>    *
>    * @cnic:	Pointer to cnic device instance
> @@ -2271,10 +2292,8 @@ static struct fcoe_transport bnx2fc_transport = {
>   	.attached = false,
>   	.list = LIST_HEAD_INIT(bnx2fc_transport.list),
>   	.match = bnx2fc_match,
> -	.create = bnx2fc_create,
> +	.alloc = bnx2fc_alloc,
>   	.destroy = bnx2fc_destroy,
> -	.enable = bnx2fc_enable,
> -	.disable = bnx2fc_disable,
>   };
>   
>   /**
> @@ -2514,6 +2533,9 @@ module_init(bnx2fc_mod_init);
>   module_exit(bnx2fc_mod_exit);
>   
>   static struct fcoe_sysfs_function_template bnx2fc_fcoe_sysfs_templ = {
> +	.set_fcoe_ctlr_start = bnx2fc_start,
> +	.set_fcoe_ctlr_enable = bnx2fc_enable,
> +	.set_fcoe_ctlr_disable = bnx2fc_disable,
>   	.get_fcoe_ctlr_mode = fcoe_ctlr_get_fip_mode,
>   	.get_fcoe_ctlr_link_fail = bnx2fc_ctlr_get_lesb,
>   	.get_fcoe_ctlr_vlink_fail = bnx2fc_ctlr_get_lesb,
>
>

[RFC PATCH 5/5] libfcoe, fcoe: Remove libfcoe module parameters

[Robert Love]

This patch removes the create, create_vn2vn, destroy,
enable and disable module parameters. Previous patches
have added these interfaces to the fcoe_sysfs layout
and these misplaced interfaces are no longer necessary.

Signed-off-by: Robert Love <robert.w.love@intel.com>
---
 drivers/scsi/fcoe/fcoe_transport.c |  211 ------------------------------------
 include/scsi/libfcoe.h             |    7 -
 2 files changed, 1 insertion(+), 217 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe_transport.c b/drivers/scsi/fcoe/fcoe_transport.c
index 8e50d9a..f9e6e67 100644
--- a/drivers/scsi/fcoe/fcoe_transport.c
+++ b/drivers/scsi/fcoe/fcoe_transport.c
@@ -32,15 +32,11 @@ MODULE_AUTHOR("Open-FCoE.org");
 MODULE_DESCRIPTION("FIP discovery protocol and FCoE transport for FCoE HBAs");
 MODULE_LICENSE("GPL v2");
 
-static int fcoe_transport_create(const char *, struct kernel_param *);
-static int fcoe_transport_destroy(const char *, struct kernel_param *);
 static int fcoe_transport_show(char *buffer, const struct kernel_param *kp);
 static struct fcoe_transport *fcoe_transport_lookup(struct net_device *device);
 static struct fcoe_transport *fcoe_netdev_map_lookup(struct net_device *device);
-static int fcoe_transport_enable(const char *, struct kernel_param *);
-static int fcoe_transport_disable(const char *, struct kernel_param *);
 static int libfcoe_device_notification(struct notifier_block *notifier,
-				    ulong event, void *ptr);
+				       ulong event, void *ptr);
 
 static LIST_HEAD(fcoe_transports);
 static DEFINE_MUTEX(ft_mutex);
@@ -55,29 +51,6 @@ module_param_call(show, NULL, fcoe_transport_show, NULL, S_IRUSR);
 __MODULE_PARM_TYPE(show, "string");
 MODULE_PARM_DESC(show, " Show attached FCoE transports");
 
-module_param_call(create, fcoe_transport_create, NULL,
-		  (void *)FIP_MODE_FABRIC, S_IWUSR);
-__MODULE_PARM_TYPE(create, "string");
-MODULE_PARM_DESC(create, " Creates fcoe instance on a ethernet interface");
-
-module_param_call(create_vn2vn, fcoe_transport_create, NULL,
-		  (void *)FIP_MODE_VN2VN, S_IWUSR);
-__MODULE_PARM_TYPE(create_vn2vn, "string");
-MODULE_PARM_DESC(create_vn2vn, " Creates a VN_node to VN_node FCoE instance "
-		 "on an Ethernet interface");
-
-module_param_call(destroy, fcoe_transport_destroy, NULL, NULL, S_IWUSR);
-__MODULE_PARM_TYPE(destroy, "string");
-MODULE_PARM_DESC(destroy, " Destroys fcoe instance on a ethernet interface");
-
-module_param_call(enable, fcoe_transport_enable, NULL, NULL, S_IWUSR);
-__MODULE_PARM_TYPE(enable, "string");
-MODULE_PARM_DESC(enable, " Enables fcoe on a ethernet interface.");
-
-module_param_call(disable, fcoe_transport_disable, NULL, NULL, S_IWUSR);
-__MODULE_PARM_TYPE(disable, "string");
-MODULE_PARM_DESC(disable, " Disables fcoe on a ethernet interface.");
-
 /* notification function for packets from net device */
 static struct notifier_block libfcoe_notifier = {
 	.notifier_call = libfcoe_device_notification,
@@ -730,188 +703,6 @@ out_nodev:
 EXPORT_SYMBOL(fcoe_ctlr_destroy_store);
 
 /**
- * fcoe_transport_create() - Create a fcoe interface
- * @buffer: The name of the Ethernet interface to create on
- * @kp:	    The associated kernel param
- *
- * Called from sysfs. This holds the ft_mutex while calling the
- * registered fcoe transport's create function.
- *
- * Returns: 0 for success
- */
-static int fcoe_transport_create(const char *buffer, struct kernel_param *kp)
-{
-	int rc = -ENODEV;
-	struct net_device *netdev = NULL;
-	struct fcoe_transport *ft = NULL;
-	enum fip_state fip_mode = (enum fip_state)(long)kp->arg;
-
-	mutex_lock(&ft_mutex);
-
-	netdev = fcoe_if_to_netdev(buffer);
-	if (!netdev) {
-		LIBFCOE_TRANSPORT_DBG("Invalid device %s.\n", buffer);
-		goto out_nodev;
-	}
-
-	ft = fcoe_netdev_map_lookup(netdev);
-	if (ft) {
-		LIBFCOE_TRANSPORT_DBG("transport %s already has existing "
-				      "FCoE instance on %s.\n",
-				      ft->name, netdev->name);
-		rc = -EEXIST;
-		goto out_putdev;
-	}
-
-	ft = fcoe_transport_lookup(netdev);
-	if (!ft) {
-		LIBFCOE_TRANSPORT_DBG("no FCoE transport found for %s.\n",
-				      netdev->name);
-		goto out_putdev;
-	}
-
-	rc = fcoe_add_netdev_mapping(netdev, ft);
-	if (rc) {
-		LIBFCOE_TRANSPORT_DBG("failed to add new netdev mapping "
-				      "for FCoE transport %s for %s.\n",
-				      ft->name, netdev->name);
-		goto out_putdev;
-	}
-
-	/* pass to transport create */
-	rc = ft->create ? ft->create(netdev, fip_mode) : -ENODEV;
-	if (rc)
-		fcoe_del_netdev_mapping(netdev);
-
-	LIBFCOE_TRANSPORT_DBG("transport %s %s to create fcoe on %s.\n",
-			      ft->name, (rc) ? "failed" : "succeeded",
-			      netdev->name);
-
-out_putdev:
-	dev_put(netdev);
-out_nodev:
-	mutex_unlock(&ft_mutex);
-	return rc;
-}
-
-/**
- * fcoe_transport_destroy() - Destroy a FCoE interface
- * @buffer: The name of the Ethernet interface to be destroyed
- * @kp:	    The associated kernel parameter
- *
- * Called from sysfs. This holds the ft_mutex while calling the
- * registered fcoe transport's destroy function.
- *
- * Returns: 0 for success
- */
-static int fcoe_transport_destroy(const char *buffer, struct kernel_param *kp)
-{
-	int rc = -ENODEV;
-	struct net_device *netdev = NULL;
-	struct fcoe_transport *ft = NULL;
-
-	mutex_lock(&ft_mutex);
-
-	netdev = fcoe_if_to_netdev(buffer);
-	if (!netdev) {
-		LIBFCOE_TRANSPORT_DBG("invalid device %s.\n", buffer);
-		goto out_nodev;
-	}
-
-	ft = fcoe_netdev_map_lookup(netdev);
-	if (!ft) {
-		LIBFCOE_TRANSPORT_DBG("no FCoE transport found for %s.\n",
-				      netdev->name);
-		goto out_putdev;
-	}
-
-	/* pass to transport destroy */
-	rc = ft->destroy ? ft->destroy(netdev) : -ENODEV;
-	fcoe_del_netdev_mapping(netdev);
-	LIBFCOE_TRANSPORT_DBG("transport %s %s to destroy fcoe on %s.\n",
-			      ft->name, (rc) ? "failed" : "succeeded",
-			      netdev->name);
-
-out_putdev:
-	dev_put(netdev);
-out_nodev:
-	mutex_unlock(&ft_mutex);
-	return rc;
-}
-
-/**
- * fcoe_transport_disable() - Disables a FCoE interface
- * @buffer: The name of the Ethernet interface to be disabled
- * @kp:	    The associated kernel parameter
- *
- * Called from sysfs.
- *
- * Returns: 0 for success
- */
-static int fcoe_transport_disable(const char *buffer, struct kernel_param *kp)
-{
-	int rc = -ENODEV;
-	struct net_device *netdev = NULL;
-	struct fcoe_transport *ft = NULL;
-
-	mutex_lock(&ft_mutex);
-
-	netdev = fcoe_if_to_netdev(buffer);
-	if (!netdev)
-		goto out_nodev;
-
-	ft = fcoe_netdev_map_lookup(netdev);
-	if (!ft)
-		goto out_putdev;
-
-	rc = ft->disable ? ft->disable(netdev) : -ENODEV;
-
-out_putdev:
-	dev_put(netdev);
-out_nodev:
-	mutex_unlock(&ft_mutex);
-
-	if (rc == -ERESTARTSYS)
-		return restart_syscall();
-	else
-		return rc;
-}
-
-/**
- * fcoe_transport_enable() - Enables a FCoE interface
- * @buffer: The name of the Ethernet interface to be enabled
- * @kp:     The associated kernel parameter
- *
- * Called from sysfs.
- *
- * Returns: 0 for success
- */
-static int fcoe_transport_enable(const char *buffer, struct kernel_param *kp)
-{
-	int rc = -ENODEV;
-	struct net_device *netdev = NULL;
-	struct fcoe_transport *ft = NULL;
-
-	mutex_lock(&ft_mutex);
-
-	netdev = fcoe_if_to_netdev(buffer);
-	if (!netdev)
-		goto out_nodev;
-
-	ft = fcoe_netdev_map_lookup(netdev);
-	if (!ft)
-		goto out_putdev;
-
-	rc = ft->enable ? ft->enable(netdev) : -ENODEV;
-
-out_putdev:
-	dev_put(netdev);
-out_nodev:
-	mutex_unlock(&ft_mutex);
-	return rc;
-}
-
-/**
  * libfcoe_init() - Initialization routine for libfcoe.ko
  */
 static int __init libfcoe_init(void)
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index b19a489..af2e444 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -291,11 +291,7 @@ static inline bool is_fip_mode(struct fcoe_ctlr *fip)
  * @match:	handler to allow the transport driver to match up a given netdev
  * @alloc:      handler to allocate per-instance FCoE structures
  *		(no discovery or login)
- * @create:	handler to sysfs entry of create for FCoE instances
  * @destroy:    handler to delete per-instance FCoE structures
- *		(frees all memory)
- * @enable:	handler to sysfs entry of enable for FCoE instances
- * @disable:	handler to sysfs entry of disable for FCoE instances
  */
 struct fcoe_transport {
 	char name[IFNAMSIZ];
@@ -303,10 +299,7 @@ struct fcoe_transport {
 	struct list_head list;
 	bool (*match) (struct net_device *device);
 	int (*alloc) (struct net_device *device);
-	int (*create) (struct net_device *device, enum fip_state fip_mode);
 	int (*destroy) (struct net_device *device);
-	int (*enable) (struct net_device *device);
-	int (*disable) (struct net_device *device);
 };
 
 /**

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Bhanu Prakash Gollapudi]

On 9/10/2012 3:59 PM, Robert Love wrote:
> The following series implements a move from using module parameters
> as control interfaces to /sys/bus/fcoe based interfaces. A sysfs infrastructure
> was added to the kernel a few cycles ago, this series builds on that work.
>
> It moves the create, vn2vn_create, destroy, enable and disable interfaces
> from /sys/module/libfcoe/parameters/ to various places under /sys/bus/fcoe/.
> These interfaces simply are not module configurations- they are control
> interfaces.
>
> A second goal of this series is to change the initialization sequence for
> a FCoE device. The result of this series is that interfaces created using
> libfcoe.ko interfaces (i.e. fcoe.ko or bnx2fc.ko) will have the following
> starting steps-
>
> 1) Create/alloc the port
>     - Allocate kernel memory and create per-instance sysfs devices
>     - No discovery or login
>
> 2) Configure the port
>     - Change mode, set ddp_min, etc...
>
> 3) Start the port
>     - Begins discovery and/or login (depending on mode)
>
> 4) Destroy the port
>     - Logout and free all memory

Robert, Can you please let me now what is the motivation for this change 
and what problem are we solving with this approach? Is this primarily to 
allow user to set the mode?

I'm concerned that we will be breaking user space compatibility with 
this change, as there should be a corresponding fcoemon/fipvlan change 
along with this, and existing utilities will not work.  Also the way we 
start fcoe will be completely different and the user may need to do the 
scripting changes, if any.

Thanks,
Bhanu

>
> I'm looking for feedback on using sysfs files as control interfaces that
> the user (application) would write interface names to. I modeled this
> series off of the bonding sysfs interface, but it was suggested to me that
> it might not be a good example. I belive bonding uses two values per-file
> a '+' or a '-" to add or delete and then the ifname apended. I am simply
> writing the ifname to the ctlr_create or ctlr_destroy.
>
> Series compiled and tested against v3.5. libfcoe.ko compile warning fixed
> upstream after v3.5, anyone who compiles this can ignore section mismatch
> warning. Also note that a modified fcoemon is needed to use the fcoe system
> service against this kernel modification. I'd be happy to provide that
> fcoemon code on request.
>
> ---
>
> Robert Love (5):
>        libfcoe, fcoe: Allow user to set a ctlr's mode
>        libfcoe: Create new libfcoe control interfaces
>        fcoe: Use new fcoe_sysfs control interface
>        bnx2fc: Use new fcoe_sysfs control interface
>        libfcoe, fcoe: Remove libfcoe module parameters
>
>
>   Documentation/ABI/testing/sysfs-bus-fcoe |   51 +++++++
>   drivers/scsi/bnx2fc/bnx2fc_fcoe.c        |   98 ++++++++-----
>   drivers/scsi/fcoe/fcoe.c                 |  229 +++++++++++++++---------------
>   drivers/scsi/fcoe/fcoe.h                 |    9 +
>   drivers/scsi/fcoe/fcoe_ctlr.c            |   24 +++
>   drivers/scsi/fcoe/fcoe_sysfs.c           |  139 ++++++++++++++++++
>   drivers/scsi/fcoe/fcoe_transport.c       |  174 ++++-------------------
>   include/scsi/fcoe_sysfs.h                |    5 +
>   include/scsi/libfcoe.h                   |   20 ++-
>   9 files changed, 445 insertions(+), 304 deletions(-)
>

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Love, Robert W]

On Mon 10 Sep 2012 05:05:20 PM PDT, Bhanu Prakash Gollapudi wrote:
> On 9/10/2012 3:59 PM, Robert Love wrote:
>> The following series implements a move from using module parameters
>> as control interfaces to /sys/bus/fcoe based interfaces. A sysfs
>> infrastructure
>> was added to the kernel a few cycles ago, this series builds on that
>> work.
>>
>> It moves the create, vn2vn_create, destroy, enable and disable
>> interfaces
>> from /sys/module/libfcoe/parameters/ to various places under
>> /sys/bus/fcoe/.
>> These interfaces simply are not module configurations- they are control
>> interfaces.
>>
>> A second goal of this series is to change the initialization sequence
>> for
>> a FCoE device. The result of this series is that interfaces created
>> using
>> libfcoe.ko interfaces (i.e. fcoe.ko or bnx2fc.ko) will have the
>> following
>> starting steps-
>>
>> 1) Create/alloc the port
>>     - Allocate kernel memory and create per-instance sysfs devices
>>     - No discovery or login
>>
>> 2) Configure the port
>>     - Change mode, set ddp_min, etc...
>>
>> 3) Start the port
>>     - Begins discovery and/or login (depending on mode)
>>
>> 4) Destroy the port
>>     - Logout and free all memory
>
> Robert, Can you please let me now what is the motivation for this
> change and what problem are we solving with this approach? Is this
> primarily to allow user to set the mode?
>

The main problem is that our control interfaces shouldn't be module 
parameters. I think of module parameters as things that globally alter 
the module.

I also think that moving to a create/configure/start model gives us 
more flexibility going forward. We don't have too many FC/FCoE knobs to 
tune right now, but if we wanted to add more we don't have a good way 
to do it without starting the whole discovery/login process and then 
making changes during the discovery/login.

I think the module parameter problem is the justification, but I'm 
trying to be comprehensive in coming up with a flexible interface that 
will allow us to evolve as well.

> I'm concerned that we will be breaking user space compatibility with
> this change, as there should be a corresponding fcoemon/fipvlan change
> along with this, and existing utilities will not work.  Also the way
> we start fcoe will be completely different and the user may need to do
> the scripting changes, if any.

See the last statement from my initial posting (it's below). I have 
patches to modify fcoemon to use these new interfaces. I'd be happy to 
share them, I just didn't want to spam this broad of a audience.

>
> Thanks,
> Bhanu
>
>>
>> I'm looking for feedback on using sysfs files as control interfaces that
>> the user (application) would write interface names to. I modeled this
>> series off of the bonding sysfs interface, but it was suggested to me
>> that
>> it might not be a good example. I belive bonding uses two values
>> per-file
>> a '+' or a '-" to add or delete and then the ifname apended. I am simply
>> writing the ifname to the ctlr_create or ctlr_destroy.
>>
>> Series compiled and tested against v3.5. libfcoe.ko compile warning
>> fixed
>> upstream after v3.5, anyone who compiles this can ignore section
>> mismatch
>> warning. Also note that a modified fcoemon is needed to use the fcoe
>> system
>> service against this kernel modification. I'd be happy to provide that
>> fcoemon code on request.

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Bhanu Prakash Gollapudi]

On 9/10/2012 6:41 PM, Love, Robert W wrote:
> On Mon 10 Sep 2012 05:05:20 PM PDT, Bhanu Prakash Gollapudi wrote:
>> On 9/10/2012 3:59 PM, Robert Love wrote:
>>> The following series implements a move from using module parameters
>>> as control interfaces to /sys/bus/fcoe based interfaces. A sysfs
>>> infrastructure
>>> was added to the kernel a few cycles ago, this series builds on that
>>> work.
>>>
>>> It moves the create, vn2vn_create, destroy, enable and disable
>>> interfaces
>>> from /sys/module/libfcoe/parameters/ to various places under
>>> /sys/bus/fcoe/.
>>> These interfaces simply are not module configurations- they are control
>>> interfaces.
>>>
>>> A second goal of this series is to change the initialization sequence
>>> for
>>> a FCoE device. The result of this series is that interfaces created
>>> using
>>> libfcoe.ko interfaces (i.e. fcoe.ko or bnx2fc.ko) will have the
>>> following
>>> starting steps-
>>>
>>> 1) Create/alloc the port
>>>      - Allocate kernel memory and create per-instance sysfs devices
>>>      - No discovery or login
>>>
>>> 2) Configure the port
>>>      - Change mode, set ddp_min, etc...
>>>
>>> 3) Start the port
>>>      - Begins discovery and/or login (depending on mode)
>>>
>>> 4) Destroy the port
>>>      - Logout and free all memory
>>
>> Robert, Can you please let me now what is the motivation for this
>> change and what problem are we solving with this approach? Is this
>> primarily to allow user to set the mode?
>>
>
> The main problem is that our control interfaces shouldn't be module
> parameters. I think of module parameters as things that globally alter
> the module.
>
> I also think that moving to a create/configure/start model gives us
> more flexibility going forward. We don't have too many FC/FCoE knobs to
> tune right now, but if we wanted to add more we don't have a good way
> to do it without starting the whole discovery/login process and then
> making changes during the discovery/login.
>
> I think the module parameter problem is the justification, but I'm
> trying to be comprehensive in coming up with a flexible interface that
> will allow us to evolve as well.
>
>> I'm concerned that we will be breaking user space compatibility with
>> this change, as there should be a corresponding fcoemon/fipvlan change
>> along with this, and existing utilities will not work.  Also the way
>> we start fcoe will be completely different and the user may need to do
>> the scripting changes, if any.
>
> See the last statement from my initial posting (it's below). I have
> patches to modify fcoemon to use these new interfaces. I'd be happy to
> share them, I just didn't want to spam this broad of a audience.
>
Thanks Robert for the explanation. Appreciate if you could share the 
fcoeutils patches also.

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Chris Leech]

On Mon, Sep 10, 2012 at 5:05 PM, Bhanu Prakash Gollapudi
<bprakash@broadcom.com> wrote:
>
> I'm concerned that we will be breaking user space compatibility with this
> change, as there should be a corresponding fcoemon/fipvlan change along with
> this, and existing utilities will not work.  Also the way we start fcoe will
> be completely different and the user may need to do the scripting changes,
> if any.
>

I agree with Bhanu on these concerns, even though I hope everyone's
using fcoeadm/fcoemon/fipvlan.  I think there needs to be more of a
transition plan than requiring everyone moving to a new kernel to
update the user-space tools at the same time.  Removing the module
parameters with the last patch might be rushed, they should probably
remain for a few kernel cycles with a warning.  When the tools are
updated for the new interface, they should probably maintain fallback
support for the module parameters for a while as well.

- Chris

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Love, Robert W]

On Tue 11 Sep 2012 10:12:09 AM PDT, Chris Leech wrote:
> On Mon, Sep 10, 2012 at 5:05 PM, Bhanu Prakash Gollapudi
> <bprakash@broadcom.com> wrote:
>>
>> I'm concerned that we will be breaking user space compatibility with this
>> change, as there should be a corresponding fcoemon/fipvlan change along with
>> this, and existing utilities will not work.  Also the way we start fcoe will
>> be completely different and the user may need to do the scripting changes,
>> if any.
>>
>
> I agree with Bhanu on these concerns, even though I hope everyone's
> using fcoeadm/fcoemon/fipvlan.  I think there needs to be more of a
> transition plan than requiring everyone moving to a new kernel to
> update the user-space tools at the same time.  Removing the module
> parameters with the last patch might be rushed, they should probably
> remain for a few kernel cycles with a warning.  When the tools are
> updated for the new interface, they should probably maintain fallback
> support for the module parameters for a while as well.
>

Deprecating the old interfaces instead of immediately removing them 
sounds like a good idea to me.

I hope everyone is using fcoeadm/fcoemon/fipvlan too. We do still carry 
the 'fcc.sh' script in fcoe-utils.git/contrib/. I don't use it, but I 
know others do. I did not update 'fcc.sh' with my user space change.

I'll post the fcoemon changes to devel@open-fcoe.org. I think I have a 
bunch of cleanup patches that the change depends on, so the series will 
probably be half cleanup and half feature.

 Thanks, //Rob

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Chris Leech]

On Mon, Sep 10, 2012 at 3:59 PM, Robert Love <robert.w.love@intel.com> wrote:
> The following series implements a move from using module parameters
> as control interfaces to /sys/bus/fcoe based interfaces. A sysfs infrastructure
> was added to the kernel a few cycles ago, this series builds on that work.
>
> It moves the create, vn2vn_create, destroy, enable and disable interfaces
> from /sys/module/libfcoe/parameters/ to various places under /sys/bus/fcoe/.
> These interfaces simply are not module configurations- they are control
> interfaces.
>
> A second goal of this series is to change the initialization sequence for
> a FCoE device. The result of this series is that interfaces created using
> libfcoe.ko interfaces (i.e. fcoe.ko or bnx2fc.ko) will have the following
> starting steps-
>
> 1) Create/alloc the port
>    - Allocate kernel memory and create per-instance sysfs devices
>    - No discovery or login
>
> 2) Configure the port
>    - Change mode, set ddp_min, etc...
>
> 3) Start the port
>    - Begins discovery and/or login (depending on mode)
>
> 4) Destroy the port
>    - Logout and free all memory
>
> I'm looking for feedback on using sysfs files as control interfaces that
> the user (application) would write interface names to. I modeled this
> series off of the bonding sysfs interface, but it was suggested to me that
> it might not be a good example. I belive bonding uses two values per-file
> a '+' or a '-" to add or delete and then the ifname apended. I am simply
> writing the ifname to the ctlr_create or ctlr_destroy.

Can you give an example session that goes through the 4 steps above
and what the sysfs hierarchy looks like at each step?  I mostly get it
from the patch descriptions, but I think it would help discussion of
your proposed interfaces to see an example of them in use.

This feels a little awkward with all the special control files.  Have
you thought about something designed for creating kernel objects, like
configfs?  Similarly the separate start, enable, disable files vs.
having some sort of status attribute that can take different values.
I feel like these need to be rethought as attributes instead of
triggers.  Is there a big difference between start and enable?  Can
you achieve the split between create and start by having it come up in
a disabled state by default?

That being said, I'm glad this is being reworked.  Do you have any
other functionality in mind that this is laying the groundwork for?

- Chris

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Love, Robert W]

On Tue 11 Sep 2012 10:06:29 AM PDT, Chris Leech wrote:
> On Mon, Sep 10, 2012 at 3:59 PM, Robert Love <robert.w.love@intel.com> wrote:

<snip>

>> 1) Create/alloc the port
>>     - Allocate kernel memory and create per-instance sysfs devices
>>     - No discovery or login
>>

# echo eth3.172-fcoe > /sys/bus/fcoe/ctlr_create

results in:

/sys/bus/fcoe/devices/ctlr_0/

>> 2) Configure the port
>>     - Change mode, set ddp_min, etc...
>>

# echo "Fabric" > /sys/bus/fcoe/devices/ctlr_0/mode

no visible change

>> 3) Start the port
>>     - Begins discovery and/or login (depending on mode)
>>

# echo 1 > /sys/bus/fcoe/devices/ctlr_0/start

Begins discovery and login. Assuming there are FCFs then results in:

/sys/bus/fcoe/devices/fcf_0

>> 4) Destroy the port
>>     - Logout and free all memory

# echo eth3.172-fcoe > /sys/bus/fcoe/ctlr_destroy

/sys/bus/fcoe/devices/ctlr_0 is removed.

>>
>> I'm looking for feedback on using sysfs files as control interfaces that
>> the user (application) would write interface names to. I modeled this
>> series off of the bonding sysfs interface, but it was suggested to me that
>> it might not be a good example. I belive bonding uses two values per-file
>> a '+' or a '-" to add or delete and then the ifname apended. I am simply
>> writing the ifname to the ctlr_create or ctlr_destroy.
>
> Can you give an example session that goes through the 4 steps above
> and what the sysfs hierarchy looks like at each step?  I mostly get it
> from the patch descriptions, but I think it would help discussion of
> your proposed interfaces to see an example of them in use.
>

See above. bash-style.

> This feels a little awkward with all the special control files.  Have
> you thought about something designed for creating kernel objects, like
> configfs?  Similarly the separate start, enable, disable files vs.

Let me do some more reading about configfs. I may not have given it 
enough thought.

> having some sort of status attribute that can take different values.
> I feel like these need to be rethought as attributes instead of
> triggers.  Is there a big difference between start and enable?  Can
> you achieve the split between create and start by having it come up in
> a disabled state by default?
>

It's a good idea. I'll look into it.

> That being said, I'm glad this is being reworked.  Do you have any
> other functionality in mind that this is laying the groundwork for?
>

I have one feature and a few ideas. I currently have a patch that adds 
a fabric selection feature. I add another RW attribute to the ctlr_X 
device. If the user writes fabric name to the file libfcoe uses it in 
it's FCF selection algorithm. Here's my commit message from that patch. 
I can share the patch if people would like to see it too. The current 
implementation also allows the user to force the login through a 
specific FCF.

     libfcoe, bnx2fc, fcoe: Add 'selection' attribute

    This patch adds a 'selection' attribute to the
    fcoe_ctlr_device. The user can write either a
    '0x' prefixed fabric name or a ':' separated
    MAC address to this file. If a fabric name is
    provided the fcoe ctlr will only consider FCFs
    with the fabric name when choosing a FCF to login
    to. If a MAC address is provided the initiator
    will only login to a FCF with the given Ethernet
    address. Only one selection is valid at a time.

    There are corresponding changes to fcoe-utils
    to take advantage of this kernel feature and
    to make it more accessible for the user.

To accompany this feature I created a new fipfcf application based on 
fipvlan that sends out a discovery solicitation and displays 
advertising FCFs.

I've also been talking with Mark Rustad about doing an 'auto' mode 
where Fabric discovery is attempted first and if it fails then it tries 
VN2VN discovery, but so for we've only had hallway conversations about 
it and nothing has been flushed out.

Thanks, //Rob

Re: [Open-FCoE] [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Love, Robert W]

On Tue 11 Sep 2012 10:36:35 AM PDT, Love, Robert W wrote:
> On Tue 11 Sep 2012 10:06:29 AM PDT, Chris Leech wrote:
>> On Mon, Sep 10, 2012 at 3:59 PM, Robert Love <robert.w.love@intel.com> wrote:

<snip>

>> That being said, I'm glad this is being reworked.  Do you have any
>> other functionality in mind that this is laying the groundwork for?
>>
>
> I have one feature and a few ideas. I currently have a patch that adds
> a fabric selection feature. I add another RW attribute to the ctlr_X
> device. If the user writes fabric name to the file libfcoe uses it in
> it's FCF selection algorithm. Here's my commit message from that patch.
> I can share the patch if people would like to see it too. The current
> implementation also allows the user to force the login through a
> specific FCF.
>

My fcoe-utils.git change here also allows the user to drive this fabric 
selection using a new variable in the /etc/fcoe/cfg-ethX files that the 
'fcoe service' uses.

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Bhanu Prakash Gollapudi]

On 9/11/2012 10:36 AM, Love, Robert W wrote:
> On Tue 11 Sep 2012 10:06:29 AM PDT, Chris Leech wrote:
>> On Mon, Sep 10, 2012 at 3:59 PM, Robert Love <robert.w.love@intel.com> wrote:
>
> <snip>
>
>>> 1) Create/alloc the port
>>>      - Allocate kernel memory and create per-instance sysfs devices
>>>      - No discovery or login
>>>
>
> # echo eth3.172-fcoe > /sys/bus/fcoe/ctlr_create

I'm assuming the existing functionality of automatically creating the 
vlan interface by fcoemon (using the cfg-ethX) continues to exist and 
the above is not a replacement for fcoeadm -c.

>
> results in:
>
> /sys/bus/fcoe/devices/ctlr_0/
>
>>> 2) Configure the port
>>>      - Change mode, set ddp_min, etc...
>>>
>
> # echo "Fabric" > /sys/bus/fcoe/devices/ctlr_0/mode
>
> no visible change
>
>>> 3) Start the port
>>>      - Begins discovery and/or login (depending on mode)
>>>
>
> # echo 1 > /sys/bus/fcoe/devices/ctlr_0/start
>
> Begins discovery and login. Assuming there are FCFs then results in:
>
> /sys/bus/fcoe/devices/fcf_0

I'm also assuming that the above three steps can be clubbed by 
fcoeutils, perhaps by adding 'mode' parameter into the cfg-ethX file. 
That way 'service fcoe start' will be no different with the proposed 
model, except that there will be multiple entry points into the driver 
(alloc, config, start) instead of just one (create).

>
>>> 4) Destroy the port
>>>      - Logout and free all memory
>
> # echo eth3.172-fcoe > /sys/bus/fcoe/ctlr_destroy
>
> /sys/bus/fcoe/devices/ctlr_0 is removed.
>
>>>
>>> I'm looking for feedback on using sysfs files as control interfaces that
>>> the user (application) would write interface names to. I modeled this
>>> series off of the bonding sysfs interface, but it was suggested to me that
>>> it might not be a good example. I belive bonding uses two values per-file
>>> a '+' or a '-" to add or delete and then the ifname apended. I am simply
>>> writing the ifname to the ctlr_create or ctlr_destroy.
>>
>> Can you give an example session that goes through the 4 steps above
>> and what the sysfs hierarchy looks like at each step?  I mostly get it
>> from the patch descriptions, but I think it would help discussion of
>> your proposed interfaces to see an example of them in use.
>>
>
> See above. bash-style.
>
>> This feels a little awkward with all the special control files.  Have
>> you thought about something designed for creating kernel objects, like
>> configfs?  Similarly the separate start, enable, disable files vs.
>
> Let me do some more reading about configfs. I may not have given it
> enough thought.
>
>> having some sort of status attribute that can take different values.
>> I feel like these need to be rethought as attributes instead of
>> triggers.  Is there a big difference between start and enable?  Can
>> you achieve the split between create and start by having it come up in
>> a disabled state by default?
>>
>
> It's a good idea. I'll look into it.
>
>> That being said, I'm glad this is being reworked.  Do you have any
>> other functionality in mind that this is laying the groundwork for?
>>
>
> I have one feature and a few ideas. I currently have a patch that adds
> a fabric selection feature. I add another RW attribute to the ctlr_X
> device. If the user writes fabric name to the file libfcoe uses it in
> it's FCF selection algorithm. Here's my commit message from that patch.
> I can share the patch if people would like to see it too. The current
> implementation also allows the user to force the login through a
> specific FCF.
>
>       libfcoe, bnx2fc, fcoe: Add 'selection' attribute
>
>      This patch adds a 'selection' attribute to the
>      fcoe_ctlr_device. The user can write either a
>      '0x' prefixed fabric name or a ':' separated
>      MAC address to this file. If a fabric name is
>      provided the fcoe ctlr will only consider FCFs
>      with the fabric name when choosing a FCF to login
>      to. If a MAC address is provided the initiator
>      will only login to a FCF with the given Ethernet
>      address. Only one selection is valid at a time.
>
>      There are corresponding changes to fcoe-utils
>      to take advantage of this kernel feature and
>      to make it more accessible for the user.
>
> To accompany this feature I created a new fipfcf application based on
> fipvlan that sends out a discovery solicitation and displays
> advertising FCFs.
>
> I've also been talking with Mark Rustad about doing an 'auto' mode
> where Fabric discovery is attempted first and if it fails then it tries
> VN2VN discovery, but so for we've only had hallway conversations about
> it and nothing has been flushed out.
>
> Thanks, //Rob\x04�{.n�+�������+%��lzwm��b�맲��r��zX��^[\x1c�(��\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[f
>


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

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Love, Robert W]

On Tue 11 Sep 2012 11:31:08 AM PDT, Bhanu Prakash Gollapudi wrote:
> On 9/11/2012 10:36 AM, Love, Robert W wrote:
>> On Tue 11 Sep 2012 10:06:29 AM PDT, Chris Leech wrote:
>>> On Mon, Sep 10, 2012 at 3:59 PM, Robert Love
>>> <robert.w.love@intel.com> wrote:
>>
>> <snip>
>>
>>>> 1) Create/alloc the port
>>>>      - Allocate kernel memory and create per-instance sysfs devices
>>>>      - No discovery or login
>>>>
>>
>> # echo eth3.172-fcoe > /sys/bus/fcoe/ctlr_create
>
> I'm assuming the existing functionality of automatically creating the
> vlan interface by fcoemon (using the cfg-ethX) continues to exist and
> the above is not a replacement for fcoeadm -c.
>

Yes, you're right. These examples are only if you were using the 
interfaces from BASH. It's all coded up for fcoemon and there shouldn't 
be any user experience change with the new code. I'll post that code to 
devel@open-fcoe.org once I get it rebased and quickly tested.

>>
>> results in:
>>
>> /sys/bus/fcoe/devices/ctlr_0/
>>
>>>> 2) Configure the port
>>>>      - Change mode, set ddp_min, etc...
>>>>
>>
>> # echo "Fabric" > /sys/bus/fcoe/devices/ctlr_0/mode
>>
>> no visible change
>>
>>>> 3) Start the port
>>>>      - Begins discovery and/or login (depending on mode)
>>>>
>>
>> # echo 1 > /sys/bus/fcoe/devices/ctlr_0/start
>>
>> Begins discovery and login. Assuming there are FCFs then results in:
>>
>> /sys/bus/fcoe/devices/fcf_0
>
> I'm also assuming that the above three steps can be clubbed by
> fcoeutils, perhaps by adding 'mode' parameter into the cfg-ethX file.
> That way 'service fcoe start' will be no different with the proposed
> model, except that there will be multiple entry points into the driver
> (alloc, config, start) instead of just one (create).
>

Yes, you're right here too. However, the default is 'Fabric' so fcoemon 
won't actually have to change the mode. However, I'd like to see VN2VN 
support added to fcoeadm/fcoemon. It's on the TODO list. Once that's 
implemented fcoemon would need to change the mode depending on the 
user's selection.

Re: [RFC PATCH 0/5] Reorganize libfcoe control interfaces

[Love, Robert W]

On 12-09-11 10:36 AM, Love, Robert W wrote:
> On Tue 11 Sep 2012 10:06:29 AM PDT, Chris Leech wrote:
>> On Mon, Sep 10, 2012 at 3:59 PM, Robert Love <robert.w.love-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

<snip>

> This feels a little awkward with all the special control files.  Have
> you thought about something designed for creating kernel objects, like
> configfs?  Similarly the separate start, enable, disable files vs.
> Let me do some more reading about configfs. I may not have given it
> enough thought.

I read though the configfs documentation and the fact that it's designed 
for user driven object creation does fit with what I'm trying to do. I 
have a couple concerns that I'm trying to sort through now-

1) This adds a new dependency for boot. I don't know if that is a 
problem or not. Are there any examples of configfs being mounted in an 
initrd?

2) Using configfs would likely mean that I'd be creating controller 
instances and possibly FCF instances in configfs, since they're already 
there in sysfs these objects would be duplicated. I'm not sure there's 
harm in that, but it certainly feels disjointed and messy. I suppose we 
could remove the /sys/bus/fcoe stuff in favor of configfs if it's the 
right thing to do.

Thanks, //Rob