public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] FCoE Sysfs
@ 2012-03-16 19:36 Robert Love
  2012-03-16 19:36 ` [PATCH v2 1/4] fcoe: Allocate fcoe_ctlr with fcoe_interface, not as a member Robert Love
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Robert Love @ 2012-03-16 19:36 UTC (permalink / raw)
  To: linux-scsi; +Cc: gregkh, giridhar.malavali, james.smart, bprakash

v2: Addressed Greg KH's review comments

* moved fcoe_ctlr_attrs and fcoe_fcf_attrs attribute
  helper macros from fcoe_sysfs.h to fcoe_sysfs.c so
  that they don't temp developers to use them. They're
  intended to be used in attribute show/store routine
  generating macros, but not by drivers.

* Removed unnecessary put_device calls on parent devices.
  Also removed unnecessary de-initialization of pointers
  to parent and removed unnecessary zero'ing of memory
  before freeing it.

* Added Documentation/ABI/testing/sysfs-class-fcoe
  document.

* Changed simple_strtoul usage to kstrtoul to avoid
  checkpatch.pl warning that I missed before. There's
  a bunch of noise in checkpatch.pl due to an odd macro
  usage, but I think any remaining checkpatch.pl warnings
  are OK.

---

This patch series adds a sysfs layer to libfcoe. It adds
a sysfs instance for FIP controllers (a SW entity) and
discovered Fibre Channel Forwarders (FCFs), which are
simply FCoE switches.

The new sysfs code is used by any driver that currently
uses libfcoe, namely fcoe.ko and bnx2fc_fcoe.ko. Any other
FCoE capable device that wishes to use the high-level APIs
defined in fcoe_syfs.h may, without having to use the
the protocol processing portions of libfcoe.

The code borrows heavily from the FC Transport, but is
less complicated because it does not need to interact
with the SCSI layer directly.

I think one thing to consider with this series is that
drivers, such as traditional HBAs, which wish to use
this infrastructure will now need to depend on libfcoe.ko.
It was either this or create a superfluous kernel module;
I think libfcoe is an appropriate place for this code.

This series was created against scsi-misc + 9 patches
mailed by me to linux-scsi on 03/09/12. (Note that 10
patches were mailed, but that patch 01/10 will be dropped)

I'm not sure if there's somewhere I should cross-post
this for general sysfs review. I don't think I'm doing
anything odd; I added Greg K-H to the CC list to try and
get some sysfs eyes on this code.

---

Robert Love (4):
      fcoe: Allocate fcoe_ctlr with fcoe_interface, not as a member
      bnx2fc: Allocate fcoe_ctlr with bnx2fc_interface, not as a member
      libfcoe: Add fcoe_sysfs
      fcoe, bnx2fc, libfcoe: SW FCoE and bnx2fc use FCoE Syfs


 Documentation/ABI/testing/sysfs-class-fcoe |   77 +++
 drivers/scsi/bnx2fc/bnx2fc.h               |    7 
 drivers/scsi/bnx2fc/bnx2fc_els.c           |    2 
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c          |  171 ++++--
 drivers/scsi/bnx2fc/bnx2fc_hwi.c           |   39 +
 drivers/scsi/fcoe/Makefile                 |    2 
 drivers/scsi/fcoe/fcoe.c                   |  196 +++++--
 drivers/scsi/fcoe/fcoe.h                   |    8 
 drivers/scsi/fcoe/fcoe_ctlr.c              |  159 +++++
 drivers/scsi/fcoe/fcoe_sysfs.c             |  840 ++++++++++++++++++++++++++++
 drivers/scsi/fcoe/fcoe_transport.c         |   13 
 include/scsi/fcoe_sysfs.h                  |  124 ++++
 include/scsi/libfcoe.h                     |   27 +
 13 files changed, 1532 insertions(+), 133 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fcoe
 create mode 100644 drivers/scsi/fcoe/fcoe_sysfs.c
 create mode 100644 include/scsi/fcoe_sysfs.h

-- 
Thanks, //Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/4] fcoe: Allocate fcoe_ctlr with fcoe_interface, not as a member
  2012-03-16 19:36 [PATCH v2 0/4] FCoE Sysfs Robert Love
@ 2012-03-16 19:36 ` Robert Love
  2012-03-16 19:36 ` [PATCH v2 2/4] bnx2fc: Allocate fcoe_ctlr with bnx2fc_interface, " Robert Love
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Robert Love @ 2012-03-16 19:36 UTC (permalink / raw)
  To: linux-scsi; +Cc: gregkh, giridhar.malavali, james.smart, bprakash

Currently the fcoe_ctlr associated with an interface is allocated
as a member of struct fcoe_interface. This causes problems when
attempting to use the new fcoe_sysfs APIs which allow us to allocate
the fcoe_interface as private data to the fcoe_ctlr_attrs instance.
The problem is that libfcoe wants to be able use pointer math to find a
fcoe_ctlr's fcoe_ctlr_attrs as well as finding a fcoe_ctlr_attrs'
assocated fcoe_ctlr. To do this we need to allocate the
fcoe_ctlr_attrs, with private data for the LLD. The private data
contains the fcoe_ctlr and it's private data is the fcoe_interface.

+-----------------+
| fcoe_ctlr_attrs |
+-----------------+
| fcoe_ctlr       |
+-----------------+
| fcoe_interface  |
+-----------------+

This prep work allows us to go from a fcoe_ctlr_attrs instance to its
fcoe_ctlr and we can also go from a fcoe_ctlr to its fcoe_ctlr_attrs
once the fcoe_sysfs API is in use (later patches in this series).

Signed-off-by: Robert Love <robert.w.love@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
---
 drivers/scsi/fcoe/fcoe.c |  132 ++++++++++++++++++++++++++++++----------------
 drivers/scsi/fcoe/fcoe.h |    8 ++-
 include/scsi/libfcoe.h   |    9 +++
 3 files changed, 99 insertions(+), 50 deletions(-)

diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 58c88b0..85ab965 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -282,7 +282,7 @@ static struct scsi_host_template fcoe_shost_template = {
 static int fcoe_interface_setup(struct fcoe_interface *fcoe,
 				struct net_device *netdev)
 {
-	struct fcoe_ctlr *fip = &fcoe->ctlr;
+	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
 	struct netdev_hw_addr *ha;
 	struct net_device *real_dev;
 	u8 flogi_maddr[ETH_ALEN];
@@ -366,7 +366,9 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
 static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
 						    enum fip_state fip_mode)
 {
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
+	int size;
 	int err;
 
 	if (!try_module_get(THIS_MODULE)) {
@@ -376,7 +378,9 @@ static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
 		goto out;
 	}
 
-	fcoe = kzalloc(sizeof(*fcoe), GFP_KERNEL);
+	size = sizeof(struct fcoe_ctlr) + sizeof(struct fcoe_interface);
+	ctlr = kzalloc(size, GFP_KERNEL);
+	fcoe = fcoe_ctlr_priv(ctlr);
 	if (!fcoe) {
 		FCOE_NETDEV_DBG(netdev, "Could not allocate fcoe structure\n");
 		fcoe = ERR_PTR(-ENOMEM);
@@ -388,15 +392,14 @@ static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
 	/*
 	 * Initialize FIP.
 	 */
-	fcoe_ctlr_init(&fcoe->ctlr, fip_mode);
-	fcoe->ctlr.send = fcoe_fip_send;
-	fcoe->ctlr.update_mac = fcoe_update_src_mac;
-	fcoe->ctlr.get_src_addr = fcoe_get_src_mac;
+	fcoe_ctlr_init(ctlr, fip_mode);
+	ctlr->send = fcoe_fip_send;
+	ctlr->update_mac = fcoe_update_src_mac;
+	ctlr->get_src_addr = fcoe_get_src_mac;
 
 	err = fcoe_interface_setup(fcoe, netdev);
 	if (err) {
-		fcoe_ctlr_destroy(&fcoe->ctlr);
-		kfree(fcoe);
+		fcoe_ctlr_destroy(ctlr);
 		dev_put(netdev);
 		fcoe = ERR_PTR(err);
 		goto out_putmod;
@@ -419,7 +422,7 @@ out:
 static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 {
 	struct net_device *netdev = fcoe->netdev;
-	struct fcoe_ctlr *fip = &fcoe->ctlr;
+	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
 	u8 flogi_maddr[ETH_ALEN];
 	const struct net_device_ops *ops;
 
@@ -459,7 +462,6 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	/* Release the self-reference taken during fcoe_interface_create() */
 	/* tear-down the FCoE controller */
 	fcoe_ctlr_destroy(fip);
-	kfree(fcoe);
 	dev_put(netdev);
 	module_put(THIS_MODULE);
 }
@@ -479,9 +481,11 @@ static int fcoe_fip_recv(struct sk_buff *skb, struct net_device *netdev,
 			 struct net_device *orig_dev)
 {
 	struct fcoe_interface *fcoe;
+	struct fcoe_ctlr *ctlr;
 
 	fcoe = container_of(ptype, struct fcoe_interface, fip_packet_type);
-	fcoe_ctlr_recv(&fcoe->ctlr, skb);
+	ctlr = fcoe_to_ctlr(fcoe);
+	fcoe_ctlr_recv(ctlr, skb);
 	return 0;
 }
 
@@ -633,11 +637,13 @@ static int fcoe_netdev_config(struct fc_lport *lport, struct net_device *netdev)
 	u32 mfs;
 	u64 wwnn, wwpn;
 	struct fcoe_interface *fcoe;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_port *port;
 
 	/* Setup lport private data to point to fcoe softc */
 	port = lport_priv(lport);
 	fcoe = port->priv;
+	ctlr = fcoe_to_ctlr(fcoe);
 
 	/*
 	 * Determine max frame size based on underlying device and optional
@@ -664,10 +670,10 @@ static int fcoe_netdev_config(struct fc_lport *lport, struct net_device *netdev)
 
 	if (!lport->vport) {
 		if (fcoe_get_wwn(netdev, &wwnn, NETDEV_FCOE_WWNN))
-			wwnn = fcoe_wwn_from_mac(fcoe->ctlr.ctl_src_addr, 1, 0);
+			wwnn = fcoe_wwn_from_mac(ctlr->ctl_src_addr, 1, 0);
 		fc_set_wwnn(lport, wwnn);
 		if (fcoe_get_wwn(netdev, &wwpn, NETDEV_FCOE_WWPN))
-			wwpn = fcoe_wwn_from_mac(fcoe->ctlr.ctl_src_addr,
+			wwpn = fcoe_wwn_from_mac(ctlr->ctl_src_addr,
 						 2, 0);
 		fc_set_wwpn(lport, wwpn);
 	}
@@ -1036,6 +1042,7 @@ static int fcoe_ddp_done(struct fc_lport *lport, u16 xid)
 static struct fc_lport *fcoe_if_create(struct fcoe_interface *fcoe,
 				       struct device *parent, int npiv)
 {
+	struct fcoe_ctlr *ctlr = fcoe_to_ctlr(fcoe);
 	struct net_device *netdev = fcoe->netdev;
 	struct fc_lport *lport, *n_port;
 	struct fcoe_port *port;
@@ -1099,7 +1106,7 @@ static struct fc_lport *fcoe_if_create(struct fcoe_interface *fcoe,
 	}
 
 	/* Initialize the library */
-	rc = fcoe_libfc_config(lport, &fcoe->ctlr, &fcoe_libfc_fcn_templ, 1);
+	rc = fcoe_libfc_config(lport, ctlr, &fcoe_libfc_fcn_templ, 1);
 	if (rc) {
 		FCOE_NETDEV_DBG(netdev, "Could not configure libfc for the "
 				"interface\n");
@@ -1366,6 +1373,7 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 {
 	struct fc_lport *lport;
 	struct fcoe_rcv_info *fr;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fc_frame_header *fh;
 	struct fcoe_percpu_s *fps;
@@ -1373,7 +1381,8 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 	unsigned int cpu;
 
 	fcoe = container_of(ptype, struct fcoe_interface, fcoe_packet_type);
-	lport = fcoe->ctlr.lp;
+	ctlr = fcoe_to_ctlr(fcoe);
+	lport = ctlr->lp;
 	if (unlikely(!lport)) {
 		FCOE_NETDEV_DBG(netdev, "Cannot find hba structure");
 		goto err2;
@@ -1389,8 +1398,8 @@ static int fcoe_rcv(struct sk_buff *skb, struct net_device *netdev,
 
 	eh = eth_hdr(skb);
 
-	if (is_fip_mode(&fcoe->ctlr) &&
-	    compare_ether_addr(eh->h_source, fcoe->ctlr.dest_addr)) {
+	if (is_fip_mode(ctlr) &&
+	    compare_ether_addr(eh->h_source, ctlr->dest_addr)) {
 		FCOE_NETDEV_DBG(netdev, "wrong source mac address:%pM\n",
 				eh->h_source);
 		goto err;
@@ -1524,6 +1533,7 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	unsigned int elen;		/* eth header, may include vlan */
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;
+	struct fcoe_ctlr *ctlr = fcoe_to_ctlr(fcoe);
 	u8 sof, eof;
 	struct fcoe_hdr *hp;
 
@@ -1539,7 +1549,7 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	}
 
 	if (unlikely(fh->fh_type == FC_TYPE_ELS) &&
-	    fcoe_ctlr_els_send(&fcoe->ctlr, lport, skb))
+	    fcoe_ctlr_els_send(ctlr, lport, skb))
 		return 0;
 
 	sof = fr_sof(fp);
@@ -1603,12 +1613,12 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	/* fill up mac and fcoe headers */
 	eh = eth_hdr(skb);
 	eh->h_proto = htons(ETH_P_FCOE);
-	memcpy(eh->h_dest, fcoe->ctlr.dest_addr, ETH_ALEN);
-	if (fcoe->ctlr.map_dest)
+	memcpy(eh->h_dest, ctlr->dest_addr, ETH_ALEN);
+	if (ctlr->map_dest)
 		memcpy(eh->h_dest + 3, fh->fh_d_id, 3);
 
-	if (unlikely(fcoe->ctlr.flogi_oxid != FC_XID_UNKNOWN))
-		memcpy(eh->h_source, fcoe->ctlr.ctl_src_addr, ETH_ALEN);
+	if (unlikely(ctlr->flogi_oxid != FC_XID_UNKNOWN))
+		memcpy(eh->h_source, ctlr->ctl_src_addr, ETH_ALEN);
 	else
 		memcpy(eh->h_source, port->data_src_addr, ETH_ALEN);
 
@@ -1657,6 +1667,7 @@ static void fcoe_percpu_flush_done(struct sk_buff *skb)
 static inline int fcoe_filter_frames(struct fc_lport *lport,
 				     struct fc_frame *fp)
 {
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fc_frame_header *fh;
 	struct sk_buff *skb = (struct sk_buff *)fp;
@@ -1678,7 +1689,8 @@ static inline int fcoe_filter_frames(struct fc_lport *lport,
 		return 0;
 
 	fcoe = ((struct fcoe_port *)lport_priv(lport))->priv;
-	if (is_fip_mode(&fcoe->ctlr) && fc_frame_payload_op(fp) == ELS_LOGO &&
+	ctlr = fcoe_to_ctlr(fcoe);
+	if (is_fip_mode(ctlr) && fc_frame_payload_op(fp) == ELS_LOGO &&
 	    ntoh24(fh->fh_s_id) == FC_FID_FLOGI) {
 		FCOE_DBG("fcoe: dropping FCoE lport LOGO in fip mode\n");
 		return -EINVAL;
@@ -1857,6 +1869,7 @@ static int fcoe_dcb_app_notification(struct notifier_block *notifier,
 				     ulong event, void *ptr)
 {
 	struct dcb_app_type *entry = ptr;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct net_device *netdev;
 	struct fcoe_port *port;
@@ -1874,6 +1887,8 @@ static int fcoe_dcb_app_notification(struct notifier_block *notifier,
 	if (!fcoe)
 		return NOTIFY_OK;
 
+	ctlr = fcoe_to_ctlr(fcoe);
+
 	if (entry->dcbx & DCB_CAP_DCBX_VER_CEE)
 		prio = ffs(entry->app.priority) - 1;
 	else
@@ -1884,10 +1899,10 @@ static int fcoe_dcb_app_notification(struct notifier_block *notifier,
 
 	if (entry->app.protocol == ETH_P_FIP ||
 	    entry->app.protocol == ETH_P_FCOE)
-		fcoe->ctlr.priority = prio;
+		ctlr->priority = prio;
 
 	if (entry->app.protocol == ETH_P_FCOE) {
-		port = lport_priv(fcoe->ctlr.lp);
+		port = lport_priv(ctlr->lp);
 		port->priority = prio;
 	}
 
@@ -1909,6 +1924,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 {
 	struct fc_lport *lport = NULL;
 	struct net_device *netdev = ptr;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fcoe_port *port;
 	struct fcoe_dev_stats *stats;
@@ -1918,7 +1934,8 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 
 	list_for_each_entry(fcoe, &fcoe_hostlist, list) {
 		if (fcoe->netdev == netdev) {
-			lport = fcoe->ctlr.lp;
+			ctlr = fcoe_to_ctlr(fcoe);
+			lport = ctlr->lp;
 			break;
 		}
 	}
@@ -1947,7 +1964,7 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 		break;
 	case NETDEV_UNREGISTER:
 		list_del(&fcoe->list);
-		port = lport_priv(fcoe->ctlr.lp);
+		port = lport_priv(ctlr->lp);
 		queue_work(fcoe_wq, &port->destroy_work);
 		goto out;
 		break;
@@ -1962,8 +1979,8 @@ static int fcoe_device_notification(struct notifier_block *notifier,
 	fcoe_link_speed_update(lport);
 
 	if (link_possible && !fcoe_link_ok(lport))
-		fcoe_ctlr_link_up(&fcoe->ctlr);
-	else if (fcoe_ctlr_link_down(&fcoe->ctlr)) {
+		fcoe_ctlr_link_up(ctlr);
+	else if (fcoe_ctlr_link_down(ctlr)) {
 		stats = per_cpu_ptr(lport->dev_stats, get_cpu());
 		stats->LinkFailureCount++;
 		put_cpu();
@@ -1983,6 +2000,7 @@ out:
  */
 static int fcoe_disable(struct net_device *netdev)
 {
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	int rc = 0;
 
@@ -1993,8 +2011,9 @@ static int fcoe_disable(struct net_device *netdev)
 	rtnl_unlock();
 
 	if (fcoe) {
-		fcoe_ctlr_link_down(&fcoe->ctlr);
-		fcoe_clean_pending_queue(fcoe->ctlr.lp);
+		ctlr = fcoe_to_ctlr(fcoe);
+		fcoe_ctlr_link_down(ctlr);
+		fcoe_clean_pending_queue(ctlr->lp);
 	} else
 		rc = -ENODEV;
 
@@ -2012,6 +2031,7 @@ static int fcoe_disable(struct net_device *netdev)
  */
 static int fcoe_enable(struct net_device *netdev)
 {
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	int rc = 0;
 
@@ -2020,11 +2040,17 @@ static int fcoe_enable(struct net_device *netdev)
 	fcoe = fcoe_hostlist_lookup_port(netdev);
 	rtnl_unlock();
 
-	if (!fcoe)
+	if (!fcoe) {
 		rc = -ENODEV;
-	else if (!fcoe_link_ok(fcoe->ctlr.lp))
-		fcoe_ctlr_link_up(&fcoe->ctlr);
+		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;
 }
@@ -2039,6 +2065,7 @@ static int fcoe_enable(struct net_device *netdev)
  */
 static int fcoe_destroy(struct net_device *netdev)
 {
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fc_lport *lport;
 	struct fcoe_port *port;
@@ -2051,7 +2078,8 @@ static int fcoe_destroy(struct net_device *netdev)
 		rc = -ENODEV;
 		goto out_nodev;
 	}
-	lport = fcoe->ctlr.lp;
+	ctlr = fcoe_to_ctlr(fcoe);
+	lport = ctlr->lp;
 	port = lport_priv(lport);
 	list_del(&fcoe->list);
 	queue_work(fcoe_wq, &port->destroy_work);
@@ -2106,7 +2134,8 @@ static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 	int dcbx;
 	u8 fup, up;
 	struct net_device *netdev = fcoe->realdev;
-	struct fcoe_port *port = lport_priv(fcoe->ctlr.lp);
+	struct fcoe_ctlr *ctlr = fcoe_to_ctlr(fcoe);
+	struct fcoe_port *port = lport_priv(ctlr->lp);
 	struct dcb_app app = {
 				.priority = 0,
 				.protocol = ETH_P_FCOE
@@ -2129,7 +2158,7 @@ static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 		}
 
 		port->priority = ffs(up) ? ffs(up) - 1 : 0;
-		fcoe->ctlr.priority = ffs(fup) ? ffs(fup) - 1 : port->priority;
+		ctlr->priority = ffs(fup) ? ffs(fup) - 1 : port->priority;
 	}
 #endif
 }
@@ -2146,6 +2175,7 @@ static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 {
 	int rc = 0;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fc_lport *lport;
 
@@ -2164,6 +2194,8 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 		goto out_nodev;
 	}
 
+	ctlr = fcoe_to_ctlr(fcoe);
+
 	lport = fcoe_if_create(fcoe, &netdev->dev, 0);
 	if (IS_ERR(lport)) {
 		printk(KERN_ERR "fcoe: Failed to create interface (%s)\n",
@@ -2175,7 +2207,7 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 	}
 
 	/* Make this the "master" N_Port */
-	fcoe->ctlr.lp = lport;
+	ctlr->lp = lport;
 
 	/* setup DCB priority attributes. */
 	fcoe_dcb_create(fcoe);
@@ -2187,7 +2219,7 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 	lport->boot_time = jiffies;
 	fc_fabric_login(lport);
 	if (!fcoe_link_ok(lport))
-		fcoe_ctlr_link_up(&fcoe->ctlr);
+		fcoe_ctlr_link_up(ctlr);
 
 out_nodev:
 	rtnl_unlock();
@@ -2297,11 +2329,12 @@ static int fcoe_reset(struct Scsi_Host *shost)
 	struct fc_lport *lport = shost_priv(shost);
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;
+	struct fcoe_ctlr *ctlr = fcoe_to_ctlr(fcoe);
 
-	fcoe_ctlr_link_down(&fcoe->ctlr);
-	fcoe_clean_pending_queue(fcoe->ctlr.lp);
-	if (!fcoe_link_ok(fcoe->ctlr.lp))
-		fcoe_ctlr_link_up(&fcoe->ctlr);
+	fcoe_ctlr_link_down(ctlr);
+	fcoe_clean_pending_queue(ctlr->lp);
+	if (!fcoe_link_ok(ctlr->lp))
+		fcoe_ctlr_link_up(ctlr);
 	return 0;
 }
 
@@ -2336,10 +2369,12 @@ fcoe_hostlist_lookup_port(const struct net_device *netdev)
  */
 static struct fc_lport *fcoe_hostlist_lookup(const struct net_device *netdev)
 {
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 
 	fcoe = fcoe_hostlist_lookup_port(netdev);
-	return (fcoe) ? fcoe->ctlr.lp : NULL;
+	ctlr = fcoe_to_ctlr(fcoe);
+	return (fcoe) ? ctlr->lp : NULL;
 }
 
 /**
@@ -2443,6 +2478,7 @@ module_init(fcoe_init);
 static void __exit fcoe_exit(void)
 {
 	struct fcoe_interface *fcoe, *tmp;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_port *port;
 	unsigned int cpu;
 
@@ -2454,7 +2490,8 @@ static void __exit fcoe_exit(void)
 	rtnl_lock();
 	list_for_each_entry_safe(fcoe, tmp, &fcoe_hostlist, list) {
 		list_del(&fcoe->list);
-		port = lport_priv(fcoe->ctlr.lp);
+		ctlr = fcoe_to_ctlr(fcoe);
+		port = lport_priv(ctlr->lp);
 		queue_work(fcoe_wq, &port->destroy_work);
 	}
 	rtnl_unlock();
@@ -2550,7 +2587,7 @@ static struct fc_seq *fcoe_elsct_send(struct fc_lport *lport, u32 did,
 {
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;
-	struct fcoe_ctlr *fip = &fcoe->ctlr;
+	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
 	struct fc_frame_header *fh = fc_frame_header_get(fp);
 
 	switch (op) {
@@ -2724,7 +2761,8 @@ static void fcoe_set_port_id(struct fc_lport *lport,
 {
 	struct fcoe_port *port = lport_priv(lport);
 	struct fcoe_interface *fcoe = port->priv;
+	struct fcoe_ctlr *ctlr = fcoe_to_ctlr(fcoe);
 
 	if (fp && fc_frame_payload_op(fp) == ELS_FLOGI)
-		fcoe_ctlr_recv_flogi(&fcoe->ctlr, lport, fp);
+		fcoe_ctlr_recv_flogi(ctlr, lport, fp);
 }
diff --git a/drivers/scsi/fcoe/fcoe.h b/drivers/scsi/fcoe/fcoe.h
index 3c2733a..55fdbdc 100644
--- a/drivers/scsi/fcoe/fcoe.h
+++ b/drivers/scsi/fcoe/fcoe.h
@@ -68,7 +68,6 @@ do {                                                            	\
  * @netdev:	      The associated net device
  * @fcoe_packet_type: FCoE packet type
  * @fip_packet_type:  FIP packet type
- * @ctlr:	      The FCoE controller (for FIP)
  * @oem:	      The offload exchange manager for all local port
  *		      instances associated with this port
  * This structure is 1:1 with a net devive.
@@ -79,11 +78,14 @@ struct fcoe_interface {
 	struct net_device  *realdev;
 	struct packet_type fcoe_packet_type;
 	struct packet_type fip_packet_type;
-	struct fcoe_ctlr   ctlr;
 	struct fc_exch_mgr *oem;
 };
 
-#define fcoe_from_ctlr(fip) container_of(fip, struct fcoe_interface, ctlr)
+#define fcoe_to_ctlr(x)						\
+	(struct fcoe_ctlr *)(((struct fcoe_ctlr *)(x)) - 1)
+
+#define fcoe_from_ctlr(x)			\
+	((struct fcoe_interface *)((x) + 1))
 
 /**
  * fcoe_netdev() - Return the net device associated with a local port
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index cfdb55f..69eca4b 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -159,6 +159,15 @@ struct fcoe_ctlr {
 };
 
 /**
+ * fcoe_ctlr_priv() - Return the private data from a fcoe_ctlr
+ * @cltr: The fcoe_ctlr whose private data will be returned
+ */
+static inline void *fcoe_ctlr_priv(const struct fcoe_ctlr *ctlr)
+{
+	return (void *)(ctlr + 1);
+}
+
+/**
  * struct fcoe_fcf - Fibre-Channel Forwarder
  * @list:	 list linkage
  * @time:	 system time (jiffies) when an advertisement was last received


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 2/4] bnx2fc: Allocate fcoe_ctlr with bnx2fc_interface, not as a member
  2012-03-16 19:36 [PATCH v2 0/4] FCoE Sysfs Robert Love
  2012-03-16 19:36 ` [PATCH v2 1/4] fcoe: Allocate fcoe_ctlr with fcoe_interface, not as a member Robert Love
@ 2012-03-16 19:36 ` Robert Love
  2012-03-16 19:36 ` [PATCH v2 3/4] libfcoe: Add fcoe_sysfs Robert Love
  2012-03-16 19:37 ` [PATCH v2 4/4] fcoe, bnx2fc, libfcoe: SW FCoE and bnx2fc use FCoE Syfs Robert Love
  3 siblings, 0 replies; 11+ messages in thread
From: Robert Love @ 2012-03-16 19:36 UTC (permalink / raw)
  To: linux-scsi; +Cc: gregkh, giridhar.malavali, james.smart, bprakash

Currently the fcoe_ctlr associated with an interface is allocated
    as a member of struct bnx2fc_interface. This causes problems when
    attempting to use the new fcoe_sysfs APIs which allow us to allocate
    the bnx2fc_interface as private data to the fcoe_ctlr_attrs instance.
    The problem is that libfcoe wants to be able use pointer math to find a
    fcoe_ctlr's fcoe_ctlr_attrs as well as finding a fcoe_ctlr_attrs'
    assocated fcoe_ctlr. To do this we need to allocate the
    fcoe_ctlr_attrs, with private data for the LLD. The private data
    contains the fcoe_ctlr and it's private data is the bnx2fc_interface.

    +------------------+
    | fcoe_ctlr_attrs  |
    +------------------+
    | fcoe_ctlr        |
    +------------------+
    | bnx2fc_interface |
    +------------------+

    This prep work allows us to go from a fcoe_ctlr_attrs instance to its
    fcoe_ctlr and we can also go from a fcoe_ctlr to its fcoe_ctlr_attrs
    once the fcoe_sysfs API is in use (later patches in this series).

Signed-off-by: Robert Love <robert.w.love@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
---
 drivers/scsi/bnx2fc/bnx2fc.h      |    7 ++
 drivers/scsi/bnx2fc/bnx2fc_els.c  |    2 -
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |  116 ++++++++++++++++++++++++-------------
 drivers/scsi/bnx2fc/bnx2fc_hwi.c  |   39 +++++++-----
 4 files changed, 101 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc.h b/drivers/scsi/bnx2fc/bnx2fc.h
index a4953ef..349bb14 100644
--- a/drivers/scsi/bnx2fc/bnx2fc.h
+++ b/drivers/scsi/bnx2fc/bnx2fc.h
@@ -228,13 +228,16 @@ struct bnx2fc_interface {
 	struct packet_type fip_packet_type;
 	struct workqueue_struct *timer_work_queue;
 	struct kref kref;
-	struct fcoe_ctlr ctlr;
 	u8 vlan_enabled;
 	int vlan_id;
 	bool enabled;
 };
 
-#define bnx2fc_from_ctlr(fip) container_of(fip, struct bnx2fc_interface, ctlr)
+#define bnx2fc_from_ctlr(x)			\
+	((struct bnx2fc_interface *)((x) + 1))
+
+#define bnx2fc_to_ctlr(x)					\
+	((struct fcoe_ctlr *)(((struct fcoe_ctlr *)(x)) - 1))
 
 struct bnx2fc_lport {
 	struct list_head list;
diff --git a/drivers/scsi/bnx2fc/bnx2fc_els.c b/drivers/scsi/bnx2fc/bnx2fc_els.c
index ce0ce3e..9a13912 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_els.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_els.c
@@ -910,7 +910,7 @@ struct fc_seq *bnx2fc_elsct_send(struct fc_lport *lport, u32 did,
 {
 	struct fcoe_port *port = lport_priv(lport);
 	struct bnx2fc_interface *interface = port->priv;
-	struct fcoe_ctlr *fip = &interface->ctlr;
+	struct fcoe_ctlr *fip = bnx2fc_to_ctlr(interface);
 	struct fc_frame_header *fh = fc_frame_header_get(fp);
 
 	switch (op) {
diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 1c34a44..90aaaf7 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -244,6 +244,7 @@ static int bnx2fc_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	struct sk_buff		*skb;
 	struct fc_frame_header	*fh;
 	struct bnx2fc_interface	*interface;
+	struct fcoe_ctlr        *ctlr;
 	struct bnx2fc_hba *hba;
 	struct fcoe_port	*port;
 	struct fcoe_hdr		*hp;
@@ -256,6 +257,7 @@ static int bnx2fc_xmit(struct fc_lport *lport, struct fc_frame *fp)
 
 	port = (struct fcoe_port *)lport_priv(lport);
 	interface = port->priv;
+	ctlr = bnx2fc_to_ctlr(interface);
 	hba = interface->hba;
 
 	fh = fc_frame_header_get(fp);
@@ -268,12 +270,12 @@ static int bnx2fc_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	}
 
 	if (unlikely(fh->fh_r_ctl == FC_RCTL_ELS_REQ)) {
-		if (!interface->ctlr.sel_fcf) {
+		if (!ctlr->sel_fcf) {
 			BNX2FC_HBA_DBG(lport, "FCF not selected yet!\n");
 			kfree_skb(skb);
 			return -EINVAL;
 		}
-		if (fcoe_ctlr_els_send(&interface->ctlr, lport, skb))
+		if (fcoe_ctlr_els_send(ctlr, lport, skb))
 			return 0;
 	}
 
@@ -347,14 +349,14 @@ static int bnx2fc_xmit(struct fc_lport *lport, struct fc_frame *fp)
 	/* fill up mac and fcoe headers */
 	eh = eth_hdr(skb);
 	eh->h_proto = htons(ETH_P_FCOE);
-	if (interface->ctlr.map_dest)
+	if (ctlr->map_dest)
 		fc_fcoe_set_mac(eh->h_dest, fh->fh_d_id);
 	else
 		/* insert GW address */
-		memcpy(eh->h_dest, interface->ctlr.dest_addr, ETH_ALEN);
+		memcpy(eh->h_dest, ctlr->dest_addr, ETH_ALEN);
 
-	if (unlikely(interface->ctlr.flogi_oxid != FC_XID_UNKNOWN))
-		memcpy(eh->h_source, interface->ctlr.ctl_src_addr, ETH_ALEN);
+	if (unlikely(ctlr->flogi_oxid != FC_XID_UNKNOWN))
+		memcpy(eh->h_source, ctlr->ctl_src_addr, ETH_ALEN);
 	else
 		memcpy(eh->h_source, port->data_src_addr, ETH_ALEN);
 
@@ -404,6 +406,7 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
 {
 	struct fc_lport *lport;
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	struct fc_frame_header *fh;
 	struct fcoe_rcv_info *fr;
 	struct fcoe_percpu_s *bg;
@@ -411,7 +414,8 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	interface = container_of(ptype, struct bnx2fc_interface,
 				 fcoe_packet_type);
-	lport = interface->ctlr.lp;
+	ctlr = bnx2fc_to_ctlr(interface);
+	lport = ctlr->lp;
 
 	if (unlikely(lport == NULL)) {
 		printk(KERN_ERR PFX "bnx2fc_rcv: lport is NULL\n");
@@ -759,11 +763,13 @@ static int bnx2fc_net_config(struct fc_lport *lport, struct net_device *netdev)
 {
 	struct bnx2fc_hba *hba;
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	struct fcoe_port *port;
 	u64 wwnn, wwpn;
 
 	port = lport_priv(lport);
 	interface = port->priv;
+	ctlr = bnx2fc_to_ctlr(interface);
 	hba = interface->hba;
 
 	/* require support for get_pauseparam ethtool op. */
@@ -782,13 +788,13 @@ static int bnx2fc_net_config(struct fc_lport *lport, struct net_device *netdev)
 
 	if (!lport->vport) {
 		if (fcoe_get_wwn(netdev, &wwnn, NETDEV_FCOE_WWNN))
-			wwnn = fcoe_wwn_from_mac(interface->ctlr.ctl_src_addr,
+			wwnn = fcoe_wwn_from_mac(ctlr->ctl_src_addr,
 						 1, 0);
 		BNX2FC_HBA_DBG(lport, "WWNN = 0x%llx\n", wwnn);
 		fc_set_wwnn(lport, wwnn);
 
 		if (fcoe_get_wwn(netdev, &wwpn, NETDEV_FCOE_WWPN))
-			wwpn = fcoe_wwn_from_mac(interface->ctlr.ctl_src_addr,
+			wwpn = fcoe_wwn_from_mac(ctlr->ctl_src_addr,
 						 2, 0);
 
 		BNX2FC_HBA_DBG(lport, "WWPN = 0x%llx\n", wwpn);
@@ -825,6 +831,7 @@ static void bnx2fc_indicate_netevent(void *context, unsigned long event,
 	struct fc_lport *lport;
 	struct fc_lport *vport;
 	struct bnx2fc_interface *interface, *tmp;
+	struct fcoe_ctlr *ctlr;
 	int wait_for_upload = 0;
 	u32 link_possible = 1;
 
@@ -875,7 +882,8 @@ static void bnx2fc_indicate_netevent(void *context, unsigned long event,
 		if (interface->hba != hba)
 			continue;
 
-		lport = interface->ctlr.lp;
+		ctlr = bnx2fc_to_ctlr(interface);
+		lport = ctlr->lp;
 		BNX2FC_HBA_DBG(lport, "netevent handler - event=%s %ld\n",
 				interface->netdev->name, event);
 
@@ -890,8 +898,8 @@ static void bnx2fc_indicate_netevent(void *context, unsigned long event,
 			 * on a stale vlan
 			 */
 			if (interface->enabled)
-				fcoe_ctlr_link_up(&interface->ctlr);
-		} else if (fcoe_ctlr_link_down(&interface->ctlr)) {
+				fcoe_ctlr_link_up(ctlr);
+		} else if (fcoe_ctlr_link_down(ctlr)) {
 			mutex_lock(&lport->lp_mutex);
 			list_for_each_entry(vport, &lport->vports, list)
 				fc_host_port_type(vport->host) =
@@ -996,9 +1004,11 @@ static int bnx2fc_fip_recv(struct sk_buff *skb, struct net_device *dev,
 			   struct net_device *orig_dev)
 {
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	interface = container_of(ptype, struct bnx2fc_interface,
 				 fip_packet_type);
-	fcoe_ctlr_recv(&interface->ctlr, skb);
+	ctlr = bnx2fc_to_ctlr(interface);
+	fcoe_ctlr_recv(ctlr, skb);
 	return 0;
 }
 
@@ -1156,6 +1166,7 @@ static int bnx2fc_interface_setup(struct bnx2fc_interface *interface)
 {
 	struct net_device *netdev = interface->netdev;
 	struct net_device *physdev = interface->hba->phys_dev;
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
 	struct netdev_hw_addr *ha;
 	int sel_san_mac = 0;
 
@@ -1170,7 +1181,7 @@ static int bnx2fc_interface_setup(struct bnx2fc_interface *interface)
 
 		if ((ha->type == NETDEV_HW_ADDR_T_SAN) &&
 		    (is_valid_ether_addr(ha->addr))) {
-			memcpy(interface->ctlr.ctl_src_addr, ha->addr,
+			memcpy(ctlr->ctl_src_addr, ha->addr,
 			       ETH_ALEN);
 			sel_san_mac = 1;
 			BNX2FC_MISC_DBG("Found SAN MAC\n");
@@ -1226,18 +1237,20 @@ static void bnx2fc_release_transport(void)
 static void bnx2fc_interface_release(struct kref *kref)
 {
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	struct net_device *netdev;
 
 	interface = container_of(kref, struct bnx2fc_interface, kref);
 	BNX2FC_MISC_DBG("Interface is being released\n");
 
+	ctlr = bnx2fc_to_ctlr(interface);
 	netdev = interface->netdev;
 
 	/* tear-down FIP controller */
 	if (test_and_clear_bit(BNX2FC_CTLR_INIT_DONE, &interface->if_flags))
-		fcoe_ctlr_destroy(&interface->ctlr);
+		fcoe_ctlr_destroy(ctlr);
 
-	kfree(interface);
+	kfree(ctlr);
 
 	dev_put(netdev);
 	module_put(THIS_MODULE);
@@ -1331,32 +1344,36 @@ struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
 				      enum fip_state fip_mode)
 {
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
+	int size;
 	int rc = 0;
 
-	interface = kzalloc(sizeof(*interface), GFP_KERNEL);
-	if (!interface) {
+	size = (sizeof(*interface) + sizeof(struct fcoe_ctlr));
+	ctlr = kzalloc(size, GFP_KERNEL);
+	if (!ctlr) {
 		printk(KERN_ERR PFX "Unable to allocate interface structure\n");
 		return NULL;
 	}
+	interface = fcoe_ctlr_priv(ctlr);
 	dev_hold(netdev);
 	kref_init(&interface->kref);
 	interface->hba = hba;
 	interface->netdev = netdev;
 
 	/* Initialize FIP */
-	fcoe_ctlr_init(&interface->ctlr, fip_mode);
-	interface->ctlr.send = bnx2fc_fip_send;
-	interface->ctlr.update_mac = bnx2fc_update_src_mac;
-	interface->ctlr.get_src_addr = bnx2fc_get_src_mac;
+	fcoe_ctlr_init(ctlr, fip_mode);
+	ctlr->send = bnx2fc_fip_send;
+	ctlr->update_mac = bnx2fc_update_src_mac;
+	ctlr->get_src_addr = bnx2fc_get_src_mac;
 	set_bit(BNX2FC_CTLR_INIT_DONE, &interface->if_flags);
 
 	rc = bnx2fc_interface_setup(interface);
 	if (!rc)
 		return interface;
 
-	fcoe_ctlr_destroy(&interface->ctlr);
+	fcoe_ctlr_destroy(ctlr);
 	dev_put(netdev);
-	kfree(interface);
+	kfree(ctlr);
 	return NULL;
 }
 
@@ -1374,6 +1391,7 @@ struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
 static struct fc_lport *bnx2fc_if_create(struct bnx2fc_interface *interface,
 				  struct device *parent, int npiv)
 {
+	struct fcoe_ctlr        *ctlr = bnx2fc_to_ctlr(interface);
 	struct fc_lport		*lport, *n_port;
 	struct fcoe_port	*port;
 	struct Scsi_Host	*shost;
@@ -1384,7 +1402,7 @@ static struct fc_lport *bnx2fc_if_create(struct bnx2fc_interface *interface,
 
 	blport = kzalloc(sizeof(struct bnx2fc_lport), GFP_KERNEL);
 	if (!blport) {
-		BNX2FC_HBA_DBG(interface->ctlr.lp, "Unable to alloc blport\n");
+		BNX2FC_HBA_DBG(ctlr->lp, "Unable to alloc blport\n");
 		return NULL;
 	}
 
@@ -1480,7 +1498,8 @@ static void bnx2fc_net_cleanup(struct bnx2fc_interface *interface)
 
 static void bnx2fc_interface_cleanup(struct bnx2fc_interface *interface)
 {
-	struct fc_lport *lport = interface->ctlr.lp;
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
+	struct fc_lport *lport = ctlr->lp;
 	struct fcoe_port *port = lport_priv(lport);
 	struct bnx2fc_hba *hba = interface->hba;
 
@@ -1520,7 +1539,8 @@ static void bnx2fc_if_destroy(struct fc_lport *lport)
 
 static void __bnx2fc_destroy(struct bnx2fc_interface *interface)
 {
-	struct fc_lport *lport = interface->ctlr.lp;
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
+	struct fc_lport *lport = ctlr->lp;
 	struct fcoe_port *port = lport_priv(lport);
 
 	bnx2fc_interface_cleanup(interface);
@@ -1544,13 +1564,15 @@ static int bnx2fc_destroy(struct net_device *netdev)
 {
 	struct bnx2fc_interface *interface = NULL;
 	struct workqueue_struct *timer_work_queue;
+	struct fcoe_ctlr *ctlr;
 	int rc = 0;
 
 	rtnl_lock();
 	mutex_lock(&bnx2fc_dev_lock);
 
 	interface = bnx2fc_interface_lookup(netdev);
-	if (!interface || !interface->ctlr.lp) {
+	ctlr = bnx2fc_to_ctlr(interface);
+	if (!interface || !ctlr->lp) {
 		rc = -ENODEV;
 		printk(KERN_ERR PFX "bnx2fc_destroy: interface or lport not found\n");
 		goto netdev_err;
@@ -1647,6 +1669,7 @@ static void bnx2fc_ulp_start(void *handle)
 {
 	struct bnx2fc_hba *hba = handle;
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	struct fc_lport *lport;
 
 	mutex_lock(&bnx2fc_dev_lock);
@@ -1658,7 +1681,8 @@ static void bnx2fc_ulp_start(void *handle)
 
 	list_for_each_entry(interface, &if_list, list) {
 		if (interface->hba == hba) {
-			lport = interface->ctlr.lp;
+			ctlr = bnx2fc_to_ctlr(interface);
+			lport = ctlr->lp;
 			/* Kick off Fabric discovery*/
 			printk(KERN_ERR PFX "ulp_init: start discovery\n");
 			lport->tt.frame_send = bnx2fc_xmit;
@@ -1678,13 +1702,14 @@ static void bnx2fc_port_shutdown(struct fc_lport *lport)
 
 static void bnx2fc_stop(struct bnx2fc_interface *interface)
 {
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
 	struct fc_lport *lport;
 	struct fc_lport *vport;
 
 	if (!test_bit(BNX2FC_FLAG_FW_INIT_DONE, &interface->hba->flags))
 		return;
 
-	lport = interface->ctlr.lp;
+	lport = ctlr->lp;
 	bnx2fc_port_shutdown(lport);
 
 	mutex_lock(&lport->lp_mutex);
@@ -1693,7 +1718,7 @@ static void bnx2fc_stop(struct bnx2fc_interface *interface)
 					FC_PORTTYPE_UNKNOWN;
 	mutex_unlock(&lport->lp_mutex);
 	fc_host_port_type(lport->host) = FC_PORTTYPE_UNKNOWN;
-	fcoe_ctlr_link_down(&interface->ctlr);
+	fcoe_ctlr_link_down(ctlr);
 	fcoe_clean_pending_queue(lport);
 }
 
@@ -1805,6 +1830,7 @@ exit:
 
 static void bnx2fc_start_disc(struct bnx2fc_interface *interface)
 {
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
 	struct fc_lport *lport;
 	int wait_cnt = 0;
 
@@ -1815,18 +1841,18 @@ static void bnx2fc_start_disc(struct bnx2fc_interface *interface)
 		return;
 	}
 
-	lport = interface->ctlr.lp;
+	lport = ctlr->lp;
 	BNX2FC_HBA_DBG(lport, "calling fc_fabric_login\n");
 
 	if (!bnx2fc_link_ok(lport) && interface->enabled) {
 		BNX2FC_HBA_DBG(lport, "ctlr_link_up\n");
-		fcoe_ctlr_link_up(&interface->ctlr);
+		fcoe_ctlr_link_up(ctlr);
 		fc_host_port_type(lport->host) = FC_PORTTYPE_NPORT;
 		set_bit(ADAPTER_STATE_READY, &interface->hba->adapter_state);
 	}
 
 	/* wait for the FCF to be selected before issuing FLOGI */
-	while (!interface->ctlr.sel_fcf) {
+	while (!ctlr->sel_fcf) {
 		msleep(250);
 		/* give up after 3 secs */
 		if (++wait_cnt > 12)
@@ -1890,19 +1916,21 @@ static void bnx2fc_ulp_init(struct cnic_dev *dev)
 static int bnx2fc_disable(struct net_device *netdev)
 {
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	int rc = 0;
 
 	rtnl_lock();
 	mutex_lock(&bnx2fc_dev_lock);
 
 	interface = bnx2fc_interface_lookup(netdev);
-	if (!interface || !interface->ctlr.lp) {
+	ctlr = bnx2fc_to_ctlr(interface);
+	if (!interface || !ctlr->lp) {
 		rc = -ENODEV;
 		printk(KERN_ERR PFX "bnx2fc_disable: interface or lport not found\n");
 	} else {
 		interface->enabled = false;
-		fcoe_ctlr_link_down(&interface->ctlr);
-		fcoe_clean_pending_queue(interface->ctlr.lp);
+		fcoe_ctlr_link_down(ctlr);
+		fcoe_clean_pending_queue(ctlr->lp);
 	}
 
 	mutex_unlock(&bnx2fc_dev_lock);
@@ -1914,17 +1942,19 @@ static int bnx2fc_disable(struct net_device *netdev)
 static int bnx2fc_enable(struct net_device *netdev)
 {
 	struct bnx2fc_interface *interface;
+	struct fcoe_ctlr *ctlr;
 	int rc = 0;
 
 	rtnl_lock();
 	mutex_lock(&bnx2fc_dev_lock);
 
 	interface = bnx2fc_interface_lookup(netdev);
-	if (!interface || !interface->ctlr.lp) {
+	ctlr = bnx2fc_to_ctlr(interface);
+	if (!interface || !ctlr->lp) {
 		rc = -ENODEV;
 		printk(KERN_ERR PFX "bnx2fc_enable: interface or lport not found\n");
-	} else if (!bnx2fc_link_ok(interface->ctlr.lp)) {
-		fcoe_ctlr_link_up(&interface->ctlr);
+	} else if (!bnx2fc_link_ok(ctlr->lp)) {
+		fcoe_ctlr_link_up(ctlr);
 		interface->enabled = true;
 	}
 
@@ -1945,6 +1975,7 @@ static int bnx2fc_enable(struct net_device *netdev)
  */
 static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 {
+	struct fcoe_ctlr *ctlr;
 	struct bnx2fc_interface *interface;
 	struct bnx2fc_hba *hba;
 	struct net_device *phys_dev;
@@ -2011,6 +2042,7 @@ static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 		goto ifput_err;
 	}
 
+	ctlr = bnx2fc_to_ctlr(interface);
 	interface->vlan_id = vlan_id;
 	interface->vlan_enabled = 1;
 
@@ -2036,10 +2068,10 @@ static int bnx2fc_create(struct net_device *netdev, enum fip_state fip_mode)
 	lport->boot_time = jiffies;
 
 	/* Make this master N_port */
-	interface->ctlr.lp = lport;
+	ctlr->lp = lport;
 
 	if (!bnx2fc_link_ok(lport)) {
-		fcoe_ctlr_link_up(&interface->ctlr);
+		fcoe_ctlr_link_up(ctlr);
 		fc_host_port_type(lport->host) = FC_PORTTYPE_NPORT;
 		set_bit(ADAPTER_STATE_READY, &interface->hba->adapter_state);
 	}
diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
index 1923a25..6aa6508 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
@@ -167,6 +167,7 @@ int bnx2fc_send_session_ofld_req(struct fcoe_port *port,
 {
 	struct fc_lport *lport = port->lport;
 	struct bnx2fc_interface *interface = port->priv;
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
 	struct bnx2fc_hba *hba = interface->hba;
 	struct kwqe *kwqe_arr[4];
 	struct fcoe_kwqe_conn_offload1 ofld_req1;
@@ -314,13 +315,13 @@ int bnx2fc_send_session_ofld_req(struct fcoe_port *port,
 	ofld_req4.src_mac_addr_mid[1] =  port->data_src_addr[2];
 	ofld_req4.src_mac_addr_hi[0] =  port->data_src_addr[1];
 	ofld_req4.src_mac_addr_hi[1] =  port->data_src_addr[0];
-	ofld_req4.dst_mac_addr_lo[0] =  interface->ctlr.dest_addr[5];
+	ofld_req4.dst_mac_addr_lo[0] =  ctlr->dest_addr[5];
 							/* fcf mac */
-	ofld_req4.dst_mac_addr_lo[1] =  interface->ctlr.dest_addr[4];
-	ofld_req4.dst_mac_addr_mid[0] =  interface->ctlr.dest_addr[3];
-	ofld_req4.dst_mac_addr_mid[1] =  interface->ctlr.dest_addr[2];
-	ofld_req4.dst_mac_addr_hi[0] =  interface->ctlr.dest_addr[1];
-	ofld_req4.dst_mac_addr_hi[1] =  interface->ctlr.dest_addr[0];
+	ofld_req4.dst_mac_addr_lo[1] = ctlr->dest_addr[4];
+	ofld_req4.dst_mac_addr_mid[0] = ctlr->dest_addr[3];
+	ofld_req4.dst_mac_addr_mid[1] = ctlr->dest_addr[2];
+	ofld_req4.dst_mac_addr_hi[0] = ctlr->dest_addr[1];
+	ofld_req4.dst_mac_addr_hi[1] = ctlr->dest_addr[0];
 
 	ofld_req4.lcq_addr_lo = (u32) tgt->lcq_dma;
 	ofld_req4.lcq_addr_hi = (u32)((u64) tgt->lcq_dma >> 32);
@@ -351,6 +352,7 @@ static int bnx2fc_send_session_enable_req(struct fcoe_port *port,
 {
 	struct kwqe *kwqe_arr[2];
 	struct bnx2fc_interface *interface = port->priv;
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
 	struct bnx2fc_hba *hba = interface->hba;
 	struct fcoe_kwqe_conn_enable_disable enbl_req;
 	struct fc_lport *lport = port->lport;
@@ -374,12 +376,12 @@ static int bnx2fc_send_session_enable_req(struct fcoe_port *port,
 	enbl_req.src_mac_addr_hi[1] =  port->data_src_addr[0];
 	memcpy(tgt->src_addr, port->data_src_addr, ETH_ALEN);
 
-	enbl_req.dst_mac_addr_lo[0] =  interface->ctlr.dest_addr[5];
-	enbl_req.dst_mac_addr_lo[1] =  interface->ctlr.dest_addr[4];
-	enbl_req.dst_mac_addr_mid[0] =  interface->ctlr.dest_addr[3];
-	enbl_req.dst_mac_addr_mid[1] =  interface->ctlr.dest_addr[2];
-	enbl_req.dst_mac_addr_hi[0] =  interface->ctlr.dest_addr[1];
-	enbl_req.dst_mac_addr_hi[1] =  interface->ctlr.dest_addr[0];
+	enbl_req.dst_mac_addr_lo[0] =  ctlr->dest_addr[5];
+	enbl_req.dst_mac_addr_lo[1] =  ctlr->dest_addr[4];
+	enbl_req.dst_mac_addr_mid[0] = ctlr->dest_addr[3];
+	enbl_req.dst_mac_addr_mid[1] = ctlr->dest_addr[2];
+	enbl_req.dst_mac_addr_hi[0] = ctlr->dest_addr[1];
+	enbl_req.dst_mac_addr_hi[1] = ctlr->dest_addr[0];
 
 	port_id = fc_host_port_id(lport->host);
 	if (port_id != tgt->sid) {
@@ -419,6 +421,7 @@ int bnx2fc_send_session_disable_req(struct fcoe_port *port,
 				    struct bnx2fc_rport *tgt)
 {
 	struct bnx2fc_interface *interface = port->priv;
+	struct fcoe_ctlr *ctlr = bnx2fc_to_ctlr(interface);
 	struct bnx2fc_hba *hba = interface->hba;
 	struct fcoe_kwqe_conn_enable_disable disable_req;
 	struct kwqe *kwqe_arr[2];
@@ -440,12 +443,12 @@ int bnx2fc_send_session_disable_req(struct fcoe_port *port,
 	disable_req.src_mac_addr_hi[0] =  tgt->src_addr[1];
 	disable_req.src_mac_addr_hi[1] =  tgt->src_addr[0];
 
-	disable_req.dst_mac_addr_lo[0] =  interface->ctlr.dest_addr[5];
-	disable_req.dst_mac_addr_lo[1] =  interface->ctlr.dest_addr[4];
-	disable_req.dst_mac_addr_mid[0] =  interface->ctlr.dest_addr[3];
-	disable_req.dst_mac_addr_mid[1] =  interface->ctlr.dest_addr[2];
-	disable_req.dst_mac_addr_hi[0] =  interface->ctlr.dest_addr[1];
-	disable_req.dst_mac_addr_hi[1] =  interface->ctlr.dest_addr[0];
+	disable_req.dst_mac_addr_lo[0] =  ctlr->dest_addr[5];
+	disable_req.dst_mac_addr_lo[1] =  ctlr->dest_addr[4];
+	disable_req.dst_mac_addr_mid[0] = ctlr->dest_addr[3];
+	disable_req.dst_mac_addr_mid[1] = ctlr->dest_addr[2];
+	disable_req.dst_mac_addr_hi[0] = ctlr->dest_addr[1];
+	disable_req.dst_mac_addr_hi[1] = ctlr->dest_addr[0];
 
 	port_id = tgt->sid;
 	disable_req.s_id[0] = (port_id & 0x000000FF);


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-16 19:36 [PATCH v2 0/4] FCoE Sysfs Robert Love
  2012-03-16 19:36 ` [PATCH v2 1/4] fcoe: Allocate fcoe_ctlr with fcoe_interface, not as a member Robert Love
  2012-03-16 19:36 ` [PATCH v2 2/4] bnx2fc: Allocate fcoe_ctlr with bnx2fc_interface, " Robert Love
@ 2012-03-16 19:36 ` Robert Love
  2012-03-17  0:25   ` Greg KH
  2012-03-16 19:37 ` [PATCH v2 4/4] fcoe, bnx2fc, libfcoe: SW FCoE and bnx2fc use FCoE Syfs Robert Love
  3 siblings, 1 reply; 11+ messages in thread
From: Robert Love @ 2012-03-16 19:36 UTC (permalink / raw)
  To: linux-scsi; +Cc: gregkh, giridhar.malavali, james.smart, bprakash

This patch adds infrastructure to libfcoe allow LLDs to
present FIP (FCoE Initialization Protocol) discovered
entities and their attributes to user space via sysfs.

This patch adds the following APIs-

fcoe_ctlr_attrs_add
fcoe_ctlr_attrs_delete
fcoe_fcf_attrs_add
fcoe_fcf_attrs_delete

They allow the LLD to expose the FCoE ENode Controller
and any discovered FCFs (Fibre Channel Forwarders, e.g.
FCoE switches) to the user. Each of these new devices
has their own class so that they are grouped together
for easy lookup from a user space application. Each
new class has an attribute_group to expose attributes
for any created instances. The attributes are-

fcoe_ctlr_attrs
* fcf_dev_loss_tmo
* lesb_link_fail
* lesb_vlink_fail
* lesb_miss_fka
* lesb_symb_err
* lesb_err_block
* lesb_fcs_error

fcoe_fcf_attrs
* fabric_name
* switch_name
* priority
* selected
* fc_map
* vfid
* mac
* fka_peroid
* fabric_state
* dev_loss_tmo

A device loss infrastructre similar to the FC Transport's
is also added by this patch. It is nice to have so that a
link flapping adapter doesn't continually advance the count
used to identify the discovered FCF. FCFs will exist in a
"Disconnected" state until either the timer expires or the
FCF is rediscovered and becomes "Connected."

This patch generates a few checkpatch.pl WARNINGS that
I'm not sure what to do about. They're macros modeled
around the FC Transport attribute building macros, which
have the same 'feature' where the caller can ommit a cast
in the argument list and no cast occurs in the code. I'm
not sure how to keep the code condensed while keeping the
macros. Any advice would be appreciated.

Signed-off-by: Robert Love <robert.w.love@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
---
 Documentation/ABI/testing/sysfs-class-fcoe |   77 +++
 drivers/scsi/fcoe/Makefile                 |    2 
 drivers/scsi/fcoe/fcoe_sysfs.c             |  840 ++++++++++++++++++++++++++++
 drivers/scsi/fcoe/fcoe_transport.c         |   13 
 include/scsi/fcoe_sysfs.h                  |  124 ++++
 include/scsi/libfcoe.h                     |    1 
 6 files changed, 1054 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fcoe
 create mode 100644 drivers/scsi/fcoe/fcoe_sysfs.c
 create mode 100644 include/scsi/fcoe_sysfs.h

diff --git a/Documentation/ABI/testing/sysfs-class-fcoe b/Documentation/ABI/testing/sysfs-class-fcoe
new file mode 100644
index 0000000..e9cd7e9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fcoe
@@ -0,0 +1,77 @@
+What:		/sys/class/fcoe_ctlr/ctlr_X
+Date:		March 2012
+KernelVersion:	TBD
+Contact:	Robert Love <robert.w.love@intel.com>, devel@open-fcoe.org
+Description:	A class for 'FCoE Controller' instances
+Attributes:
+
+	fcf_dev_loss_tmo: Device loss timeout peroid (see below). Changing
+			  this value will change the dev_loss_tmo for all
+			  FCFs discovered by this controller.
+
+	lesb_link_fail:   Link Error Status Block (LESB) link failure count.
+
+	lesb_vlink_fail:  Link Error Status Block (LESB) virtual link
+			  failure count.
+
+	lesb_miss_fka:    Link Error Status Block (LESB) missed FCoE
+			  Initialization Protocol (FIP) Keep-Alives (FKA).
+
+	lesb_symb_err:    Link Error Status Block (LESB) symbolic error count.
+
+	lesb_err_block:   Link Error Status Block (LESB) block error count.
+
+	lesb_fcs_error:   Link Error Status Block (LESB) Fibre Channel
+			  Serivces error count.
+
+Notes: ctlr_X (global increment starting at 0)
+
+What:		/sys/class/fcoe_fcf/fcf_X
+Date:		March 2012
+KernelVersion:	TBD
+Contact:	Robert Love <robert.w.love@intel.com>, devel@open-fcoe.org
+Description:	A class for 'FCoE FCF' instances. A FCF is a Fibre Channel
+		Forwarder, which is a FCoE switch that can accept FCoE
+		(Ethernet) packets, unpack them, and forward the embedded
+		Fibre Channel frames into a FC fabric. It can also take
+		outbound FC frames and pack them in Ethernet packets to
+		be sent to their destination on the Ethernet segment.
+Attributes:
+
+	fabric_name: Identifies the fabric that the FCF services.
+
+	switch_name: Identifies the FCF.
+
+	priority:    The switch's priority amongst other FCFs on the same
+		     fabric.
+
+	selected:    1 indicates that the switch has been selected for use;
+		     0 indicates that the swich will not be used.
+
+	fc_map:      The Fibre Channel MAP
+
+	vfid:	     The Virtual Fabric ID
+
+	mac:         The FCF's MAC address
+
+	fka_peroid:  The FIP Keep-Alive peroid
+
+	fabric_state: The internal kernel state
+		      "Unknown" - Initialization value
+		      "Disconnected" - No link to the FCF/fabric
+		      "Connected" - Host is connected to the FCF
+		      "Deleted" - FCF is being removed from the system
+
+	dev_loss_tmo: The device loss timeout peroid for this FCF.
+
+Notes: A device loss infrastructre similar to the FC Transport's
+       is present in fcoe_sysfs. It is nice to have so that a
+       link flapping adapter doesn't continually advance the count
+       used to identify the discovered FCF. FCFs will exist in a
+       "Disconnected" state until either the timer expires and the
+       FCF becomes "Deleted" or the FCF is rediscovered and becomes
+       "Connected."
+
+
+Users: The first user of this interface will be the fcoeadm application,
+       which is commonly packaged in the fcoe-utils package.
diff --git a/drivers/scsi/fcoe/Makefile b/drivers/scsi/fcoe/Makefile
index f6d37d0..aed0f5d 100644
--- a/drivers/scsi/fcoe/Makefile
+++ b/drivers/scsi/fcoe/Makefile
@@ -1,4 +1,4 @@
 obj-$(CONFIG_FCOE) += fcoe.o
 obj-$(CONFIG_LIBFCOE) += libfcoe.o
 
-libfcoe-objs := fcoe_ctlr.o fcoe_transport.o
+libfcoe-objs := fcoe_ctlr.o fcoe_transport.o fcoe_sysfs.o
diff --git a/drivers/scsi/fcoe/fcoe_sysfs.c b/drivers/scsi/fcoe/fcoe_sysfs.c
new file mode 100644
index 0000000..6c6e71f
--- /dev/null
+++ b/drivers/scsi/fcoe/fcoe_sysfs.c
@@ -0,0 +1,840 @@
+/*
+ * Copyright(c) 2011 - 2012 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Maintained at www.Open-FCoE.org
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/etherdevice.h>
+
+#include <scsi/fcoe_sysfs.h>
+
+static atomic_t ctlr_num;
+static atomic_t fcf_num;
+
+/*
+ * fcoe_fcf_dev_loss_tmo: the default number of seconds that fcoe sysfs
+ * should insulate the loss of a fcf.
+ */
+static unsigned int fcoe_fcf_dev_loss_tmo = 1800;  /* seconds */
+
+module_param_named(fcf_dev_loss_tmo, fcoe_fcf_dev_loss_tmo,
+		   uint, S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(fcf_dev_loss_tmo,
+		 "Maximum number of seconds that libfcoe should"
+		 " insulate the loss of a fcf. Once this value is"
+		 " exceeded, the fcf is removed.");
+
+
+/*
+ * These are used by the fcoe_*_show_function routines, they
+ * are intentionally placed in the .c file as they're not intended
+ * for use throughout the code.
+ */
+#define fcoe_ctlr_id(x)				\
+	((x)->id)
+#define fcoe_ctlr_work_q_name(x)		\
+	((x)->work_q_name)
+#define fcoe_ctlr_work_q(x)			\
+	((x)->work_q)
+#define fcoe_ctlr_devloss_work_q_name(x)	\
+	((x)->devloss_work_q_name)
+#define fcoe_ctlr_devloss_work_q(x)		\
+	((x)->devloss_work_q)
+#define fcoe_ctlr_mode(x)			\
+	((x)->mode)
+#define fcoe_ctlr_fcf_dev_loss_tmo(x)		\
+	((x)->fcf_dev_loss_tmo)
+#define fcoe_ctlr_link_fail(x)			\
+	((x)->lesb.lesb_link_fail)
+#define fcoe_ctlr_vlink_fail(x)			\
+	((x)->lesb.lesb_vlink_fail)
+#define fcoe_ctlr_miss_fka(x)			\
+	((x)->lesb.lesb_miss_fka)
+#define fcoe_ctlr_symb_err(x)			\
+	((x)->lesb.lesb_symb_err)
+#define fcoe_ctlr_err_block(x)			\
+	((x)->lesb.lesb_err_block)
+#define fcoe_ctlr_fcs_error(x)			\
+	((x)->lesb.lesb_fcs_error)
+#define fcoe_fcf_state(x)			\
+	((x)->state)
+#define fcoe_fcf_fabric_name(x)			\
+	((x)->fabric_name)
+#define fcoe_fcf_switch_name(x)			\
+	((x)->switch_name)
+#define fcoe_fcf_fc_map(x)			\
+	((x)->fc_map)
+#define fcoe_fcf_vfid(x)			\
+	((x)->vfid)
+#define fcoe_fcf_mac(x)				\
+	((x)->mac)
+#define fcoe_fcf_priority(x)			\
+	((x)->priority)
+#define fcoe_fcf_fka_period(x)			\
+	((x)->fka_period)
+#define fcoe_fcf_dev_loss_tmo(x)		\
+	((x)->dev_loss_tmo)
+#define fcoe_fcf_selected(x)			\
+	((x)->selected)
+#define fcoe_fcf_vlan_id(x)			\
+	((x)->vlan_id)
+
+/*
+ * dev_loss_tmo attribute
+ */
+static int fcoe_str_to_dev_loss(const char *buf, unsigned long *val)
+{
+	char *cp;
+	int ret;
+
+	ret = kstrtoul(buf, 0, val);
+	if (ret || *val < 0)
+		return -EINVAL;
+	/*
+	 * Check for overflow; dev_loss_tmo is u32
+	 */
+	if (*val > UINT_MAX)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int fcoe_fcf_set_dev_loss_tmo(struct fcoe_fcf_attrs *fcf,
+				     unsigned long val)
+{
+	if ((fcf->state == FCOE_FCF_STATE_UNKNOWN) ||
+	    (fcf->state == FCOE_FCF_STATE_DISCONNECTED) ||
+	    (fcf->state == FCOE_FCF_STATE_DELETED))
+		return -EBUSY;
+	/*
+	 * Check for overflow; dev_loss_tmo is u32
+	 */
+	if (val > UINT_MAX)
+		return -EINVAL;
+
+	fcoe_fcf_dev_loss_tmo(fcf) = val;
+	return 0;
+}
+
+#define FCOE_DEVICE_ATTR(_prefix, _name, _mode, _show, _store)	\
+struct device_attribute device_attr_fcoe_##_prefix##_##_name =	\
+	__ATTR(_name, _mode, _show, _store)
+
+#define fcoe_ctlr_show_function(field, format_string, sz, cast)	\
+static ssize_t show_fcoe_ctlr_attrs_##field(struct device *dev, \
+					    struct device_attribute *attr, \
+					    char *buf)			\
+{									\
+	struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);		\
+	if (ctlr->f->get_fcoe_ctlr_##field)				\
+		ctlr->f->get_fcoe_ctlr_##field(ctlr);			\
+	return snprintf(buf, sz, format_string,				\
+			cast fcoe_ctlr_##field(ctlr));			\
+}
+
+#define fcoe_fcf_show_function(field, format_string, sz, cast)	\
+static ssize_t show_fcoe_fcf_attrs_##field(struct device *dev,	\
+					   struct device_attribute *attr, \
+					   char *buf)			\
+{									\
+	struct fcoe_fcf_attrs *fcf = dev_to_fcf(dev);			\
+	struct fcoe_ctlr_attrs *ctlr = fcoe_fcf_to_ctlr(fcf);		\
+	if (ctlr->f->get_fcoe_fcf_##field)				\
+		ctlr->f->get_fcoe_fcf_##field(fcf);			\
+	return snprintf(buf, sz, format_string,				\
+			cast fcoe_fcf_##field(fcf));			\
+}
+
+#define fcoe_ctlr_private_show_function(field, format_string, sz, cast)	\
+static ssize_t show_fcoe_ctlr_attrs_##field(struct device *dev, \
+					    struct device_attribute *attr, \
+					    char *buf)			\
+{									\
+	struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);		\
+	return snprintf(buf, sz, format_string, cast fcoe_ctlr_##field(ctlr)); \
+}
+
+#define fcoe_fcf_private_show_function(field, format_string, sz, cast)	\
+static ssize_t show_fcoe_fcf_attrs_##field(struct device *dev,	\
+					   struct device_attribute *attr, \
+					   char *buf)			\
+{								\
+	struct fcoe_fcf_attrs *fcf = dev_to_fcf(dev);			\
+	return snprintf(buf, sz, format_string, cast fcoe_fcf_##field(fcf)); \
+}
+
+#define fcoe_ctlr_private_rd_attr(field, format_string, sz)		\
+	fcoe_ctlr_private_show_function(field, format_string, sz, )	\
+	static FCOE_DEVICE_ATTR(ctlr, field, S_IRUGO,			\
+				show_fcoe_ctlr_attrs_##field, NULL)
+
+#define fcoe_ctlr_rd_attr(field, format_string, sz)			\
+	fcoe_ctlr_show_function(field, format_string, sz, )		\
+	static FCOE_DEVICE_ATTR(ctlr, field, S_IRUGO,			\
+				show_fcoe_ctlr_attrs_##field, NULL)
+
+#define fcoe_fcf_rd_attr(field, format_string, sz)			\
+	fcoe_fcf_show_function(field, format_string, sz, )		\
+	static FCOE_DEVICE_ATTR(fcf, field, S_IRUGO,			\
+				show_fcoe_fcf_attrs_##field, NULL)
+
+#define fcoe_fcf_private_rd_attr(field, format_string, sz)		\
+	fcoe_fcf_private_show_function(field, format_string, sz, )	\
+	static FCOE_DEVICE_ATTR(fcf, field, S_IRUGO,			\
+				show_fcoe_fcf_attrs_##field, NULL)
+
+#define fcoe_ctlr_private_rd_attr_cast(field, format_string, sz, cast)	\
+	fcoe_ctlr_private_show_function(field, format_string, sz, (cast)) \
+	static FCOE_DEVICE_ATTR(ctlr, field, S_IRUGO,			\
+				show_fcoe_ctlr_attrs_##field, NULL)
+
+#define fcoe_fcf_private_rd_attr_cast(field, format_string, sz, cast)	\
+	fcoe_fcf_private_show_function(field, format_string, sz, (cast)) \
+	static FCOE_DEVICE_ATTR(fcf, field, S_IRUGO,			\
+				show_fcoe_fcf_attrs_##field, NULL)
+
+#define fcoe_enum_name_search(title, table_type, table)			\
+static const char *get_fcoe_##title##_name(enum table_type table_key)	\
+{									\
+	int i;								\
+	char *name = NULL;						\
+									\
+	for (i = 0; i < ARRAY_SIZE(table); i++) {			\
+		if (table[i].value == table_key) {			\
+			name = table[i].name;				\
+			break;						\
+		}							\
+	}								\
+	return name;							\
+}
+
+static struct {
+	enum fcf_state value;
+	char           *name;
+} fcf_state_names[] = {
+	{ FCOE_FCF_STATE_UNKNOWN,      "Unknown" },
+	{ FCOE_FCF_STATE_DISCONNECTED, "Disconnected" },
+	{ FCOE_FCF_STATE_CONNECTED,    "Connected" },
+};
+fcoe_enum_name_search(fcf_state, fcf_state, fcf_state_names)
+#define FCOE_FCF_STATE_MAX_NAMELEN 50
+
+static ssize_t show_fcf_state(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct fcoe_fcf_attrs *fcf = dev_to_fcf(dev);
+	const char *name;
+	name = get_fcoe_fcf_state_name(fcf->state);
+	if (!name)
+		return -EINVAL;
+	return snprintf(buf, FCOE_FCF_STATE_MAX_NAMELEN, "%s\n", name);
+}
+static FCOE_DEVICE_ATTR(fcf, state, S_IRUGO, show_fcf_state, NULL);
+
+static struct {
+	enum fip_conn_type value;
+	char               *name;
+} fip_conn_type_names[] = {
+	{ FIP_CONN_TYPE_UNKNOWN, "Unknown" },
+	{ FIP_CONN_TYPE_FABRIC, "Fabric" },
+	{ FIP_CONN_TYPE_VN2VN, "VN2VN" },
+};
+fcoe_enum_name_search(ctlr_mode, fip_conn_type, fip_conn_type_names)
+#define FCOE_CTLR_MODE_MAX_NAMELEN 50
+
+static ssize_t show_ctlr_mode(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);
+	const char *name;
+
+	if (ctlr->f->get_fcoe_ctlr_mode)
+		ctlr->f->get_fcoe_ctlr_mode(ctlr);
+
+	name = get_fcoe_ctlr_mode_name(ctlr->mode);
+	if (!name)
+		return -EINVAL;
+	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_private_fcoe_ctlr_fcf_dev_loss_tmo(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);
+	struct fcoe_fcf_attrs *fcf;
+	unsigned long val;
+	int rc;
+
+	rc = fcoe_str_to_dev_loss(buf, &val);
+	if (rc)
+		return rc;
+
+	fcoe_ctlr_fcf_dev_loss_tmo(ctlr) = val;
+	mutex_lock(&ctlr->lock);
+	list_for_each_entry(fcf, &ctlr->fcfs, peers)
+		fcoe_fcf_set_dev_loss_tmo(fcf, val);
+	mutex_unlock(&ctlr->lock);
+	return count;
+}
+fcoe_ctlr_private_show_function(fcf_dev_loss_tmo, "%d\n", 20, );
+static FCOE_DEVICE_ATTR(ctlr, fcf_dev_loss_tmo, S_IRUGO | S_IWUSR,
+			show_fcoe_ctlr_attrs_fcf_dev_loss_tmo,
+			store_private_fcoe_ctlr_fcf_dev_loss_tmo);
+
+/* Link Error Status Block (LESB) */
+fcoe_ctlr_rd_attr(link_fail, "%u\n", 20);
+fcoe_ctlr_rd_attr(vlink_fail, "%u\n", 20);
+fcoe_ctlr_rd_attr(miss_fka, "%u\n", 20);
+fcoe_ctlr_rd_attr(symb_err, "%u\n", 20);
+fcoe_ctlr_rd_attr(err_block, "%u\n", 20);
+fcoe_ctlr_rd_attr(fcs_error, "%u\n", 20);
+
+fcoe_fcf_private_rd_attr_cast(fabric_name, "0x%llx\n", 20, unsigned long long);
+fcoe_fcf_private_rd_attr_cast(switch_name, "0x%llx\n", 20, unsigned long long);
+fcoe_fcf_private_rd_attr(priority, "%u\n", 20);
+fcoe_fcf_private_rd_attr(fc_map, "0x%x\n", 20);
+fcoe_fcf_private_rd_attr(vfid, "%u\n", 20);
+fcoe_fcf_private_rd_attr(mac, "%pM\n", 20);
+fcoe_fcf_private_rd_attr(fka_period, "%u\n", 20);
+fcoe_fcf_rd_attr(selected, "%u\n", 20);
+fcoe_fcf_rd_attr(vlan_id, "%u\n", 20);
+
+fcoe_fcf_private_show_function(dev_loss_tmo, "%d\n", 20, )
+static ssize_t
+store_fcoe_fcf_dev_loss_tmo(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct fcoe_fcf_attrs *fcf = dev_to_fcf(dev);
+	unsigned long val;
+	int rc;
+
+	rc = fcoe_str_to_dev_loss(buf, &val);
+	if (rc)
+		return rc;
+
+	rc = fcoe_fcf_set_dev_loss_tmo(fcf, val);
+	if (rc)
+		return rc;
+	return count;
+}
+static FCOE_DEVICE_ATTR(fcf, dev_loss_tmo, S_IRUGO | S_IWUSR,
+			show_fcoe_fcf_attrs_dev_loss_tmo,
+			store_fcoe_fcf_dev_loss_tmo);
+
+static struct attribute *fcoe_ctlr_lesb_attributes[] = {
+	&device_attr_fcoe_ctlr_link_fail.attr,
+	&device_attr_fcoe_ctlr_vlink_fail.attr,
+	&device_attr_fcoe_ctlr_miss_fka.attr,
+	&device_attr_fcoe_ctlr_symb_err.attr,
+	&device_attr_fcoe_ctlr_err_block.attr,
+	&device_attr_fcoe_ctlr_fcs_error.attr,
+	NULL
+};
+
+static struct attribute_group fcoe_ctlr_lesb_attribute_group = {
+	.name = "lesb",
+	.attrs = fcoe_ctlr_lesb_attributes,
+};
+
+static struct attribute *fcoe_ctlr_attributes[] = {
+	&device_attr_fcoe_ctlr_fcf_dev_loss_tmo.attr,
+	&device_attr_fcoe_ctlr_mode.attr,
+	NULL
+};
+
+static struct attribute_group fcoe_ctlr_attribute_group = {
+	.attrs = fcoe_ctlr_attributes,
+};
+
+static struct attribute *fcoe_fcf_attributes[] = {
+	&device_attr_fcoe_fcf_fabric_name.attr,
+	&device_attr_fcoe_fcf_switch_name.attr,
+	&device_attr_fcoe_fcf_dev_loss_tmo.attr,
+	&device_attr_fcoe_fcf_fc_map.attr,
+	&device_attr_fcoe_fcf_vfid.attr,
+	&device_attr_fcoe_fcf_mac.attr,
+	&device_attr_fcoe_fcf_priority.attr,
+	&device_attr_fcoe_fcf_fka_period.attr,
+	&device_attr_fcoe_fcf_state.attr,
+	&device_attr_fcoe_fcf_selected.attr,
+	&device_attr_fcoe_fcf_vlan_id.attr,
+	NULL
+};
+
+static struct attribute_group fcoe_fcf_attribute_group = {
+	.attrs = fcoe_fcf_attributes,
+};
+
+/**
+ * fcoe_ctlr_attrs_flush_work() - Flush a FIP ctlr's workqueue
+ * @ctlr: Pointer to the FIP ctlr whose workqueue is to be flushed
+ */
+void fcoe_ctlr_attrs_flush_work(struct fcoe_ctlr_attrs *ctlr)
+{
+	if (!fcoe_ctlr_work_q(ctlr)) {
+		printk(KERN_ERR
+		       "ERROR: FIP Ctlr '%d' attempted to flush work, "
+		       "when no workqueue created.\n", ctlr->id);
+		dump_stack();
+		return;
+	}
+
+	flush_workqueue(fcoe_ctlr_work_q(ctlr));
+}
+
+/**
+ * fcoe_ctlr_attrs_queue_work() - Schedule work for a FIP ctlr's workqueue
+ * @ctlr: Pointer to the FIP ctlr who owns the devloss workqueue
+ * @work:   Work to queue for execution
+ *
+ * Return value:
+ *	1 on success / 0 already queued / < 0 for error
+ */
+int fcoe_ctlr_attrs_queue_work(struct fcoe_ctlr_attrs *ctlr,
+			       struct work_struct *work)
+{
+	if (unlikely(!fcoe_ctlr_work_q(ctlr))) {
+		printk(KERN_ERR
+		       "ERROR: FIP Ctlr '%d' attempted to queue work, "
+		       "when no workqueue created.\n", ctlr->id);
+		dump_stack();
+
+		return -EINVAL;
+	}
+
+	return queue_work(fcoe_ctlr_work_q(ctlr), work);
+}
+
+/**
+ * fcoe_ctlr_attrs_flush_devloss() - Flush a FIP ctlr's devloss workqueue
+ * @ctlr: Pointer to FIP ctlr whose workqueue is to be flushed
+ */
+void fcoe_ctlr_attrs_flush_devloss(struct fcoe_ctlr_attrs *ctlr)
+{
+	if (!fcoe_ctlr_devloss_work_q(ctlr)) {
+		printk(KERN_ERR
+		       "ERROR: FIP Ctlr '%d' attempted to flush work, "
+		       "when no workqueue created.\n", ctlr->id);
+		dump_stack();
+		return;
+	}
+
+	flush_workqueue(fcoe_ctlr_devloss_work_q(ctlr));
+}
+
+/**
+ * fcoe_ctlr_attrs_queue_devloss_work() - Schedule work for a FIP ctlr's devloss workqueue
+ * @ctlr: Pointer to the FIP ctlr who owns the devloss workqueue
+ * @work:   Work to queue for execution
+ * @delay:  jiffies to delay the work queuing
+ *
+ * Return value:
+ *	1 on success / 0 already queued / < 0 for error
+ */
+int fcoe_ctlr_attrs_queue_devloss_work(struct fcoe_ctlr_attrs *ctlr,
+				       struct delayed_work *work,
+				       unsigned long delay)
+{
+	if (unlikely(!fcoe_ctlr_devloss_work_q(ctlr))) {
+		printk(KERN_ERR
+		       "ERROR: FIP Ctlr '%d' attempted to queue work, "
+		       "when no workqueue created.\n", ctlr->id);
+		dump_stack();
+
+		return -EINVAL;
+	}
+
+	return queue_delayed_work(fcoe_ctlr_devloss_work_q(ctlr), work, delay);
+}
+
+/**
+ * fcoe_ctlr_attrs_release() - Release the FIP ctlr memory
+ * @dev: Pointer to the FIP ctlr's embedded device
+ *
+ * Called when the last FIP ctlr reference is released.
+ */
+static void fcoe_ctlr_attrs_release(struct device *dev)
+{
+	struct fcoe_ctlr_attrs *ctlr = dev_to_ctlr(dev);
+	kfree(ctlr);
+}
+
+struct class fcoe_ctlr_class = {
+	.name = "fcoe_ctlr",
+	.dev_release = fcoe_ctlr_attrs_release,
+};
+
+static int fcoe_fcf_attrs_match(struct fcoe_fcf_attrs *new,
+				struct fcoe_fcf_attrs *old)
+{
+	if (new->switch_name == old->switch_name &&
+	    new->fabric_name == old->fabric_name &&
+	    new->fc_map == old->fc_map &&
+	    compare_ether_addr(new->mac, old->mac) == 0)
+		return 1;
+	return 0;
+}
+
+/**
+ * fcoe_ctlr_attrs_add() - Add a FIP ctlr to sysfs
+ * @parent:    The parent device to which the fcoe_ctlr instance
+ *             should be attached
+ * @f:         The LLD's FCoE sysfs function template pointer
+ * @priv_size: Size to be allocated with the fcoe_ctlr_attrs for the LLD
+ *
+ * This routine allocates a FIP ctlr object with some additional memory
+ * for the LLD. The FIP ctlr is initialized, added to sysfs and then
+ * attributes are added to it.
+ */
+struct fcoe_ctlr_attrs *fcoe_ctlr_attrs_add(struct device *parent,
+				    struct fcoe_sysfs_function_template *f,
+				    int priv_size)
+{
+	struct fcoe_ctlr_attrs *ctlr;
+	int error = 0;
+
+	ctlr = kzalloc(sizeof(struct fcoe_ctlr_attrs) + priv_size,
+		       GFP_KERNEL);
+	if (!ctlr)
+		goto out;
+
+	ctlr->id = atomic_inc_return(&ctlr_num) - 1;
+	ctlr->f = f;
+	INIT_LIST_HEAD(&ctlr->fcfs);
+	mutex_init(&ctlr->lock);
+	device_initialize(&ctlr->dev);
+	ctlr->dev.parent = get_device(parent);
+	ctlr->dev.class = &fcoe_ctlr_class;
+
+	ctlr->fcf_dev_loss_tmo = fcoe_fcf_dev_loss_tmo;
+
+	snprintf(ctlr->work_q_name, sizeof(ctlr->work_q_name),
+		 "ctlr_wq_%d", ctlr->id);
+	ctlr->work_q = create_singlethread_workqueue(
+		ctlr->work_q_name);
+	if (!ctlr->work_q)
+		goto out_del;
+
+	snprintf(ctlr->devloss_work_q_name,
+		 sizeof(ctlr->devloss_work_q_name),
+		 "ctlr_dl_wq_%d", ctlr->id);
+	ctlr->devloss_work_q = create_singlethread_workqueue(
+		ctlr->devloss_work_q_name);
+	if (!ctlr->devloss_work_q)
+		goto out_del_q;
+
+	dev_set_name(&ctlr->dev, "ctlr_%d", ctlr->id);
+	error = device_add(&ctlr->dev);
+	if (error)
+		goto out_del_q2;
+
+	error = sysfs_create_group(&ctlr->dev.kobj,
+				   &fcoe_ctlr_attribute_group);
+	if (error)
+		goto out_del_dev;
+
+	error = sysfs_create_group(&ctlr->dev.kobj,
+				   &fcoe_ctlr_lesb_attribute_group);
+	if (error)
+		goto out_del_dev;
+
+	return ctlr;
+
+out_del_dev:
+	device_del(&ctlr->dev);
+out_del_q2:
+	destroy_workqueue(ctlr->devloss_work_q);
+	ctlr->devloss_work_q = NULL;
+out_del_q:
+	destroy_workqueue(ctlr->work_q);
+	ctlr->work_q = NULL;
+out_del:
+	put_device(parent);
+	kfree(ctlr);
+out:
+	return NULL;
+}
+EXPORT_SYMBOL(fcoe_ctlr_attrs_add);
+
+/**
+ * fcoe_ctlr_attrs_delete() - Delete a FIP ctlr and its subtree from sysfs
+ * @ctlr: A pointer to the ctlr to be deleted
+ *
+ * Deletes a FIP ctlr and any fcfs attached
+ * to it. Deleting fcfs will cause their childen
+ * to be deleted as well.
+ *
+ * The ctlr is detached from sysfs and it's resources
+ * are freed (work q), but the memory is not freed
+ * until its last reference is released.
+ *
+ * This routine expects no locks to be held before
+ * calling.
+ *
+ * TODO: Currently there are no callbacks to clean up LLD data
+ * for a fcoe_fcf_attrs. LLDs must keep this in mind as they need
+ * to clean up each of their LLD data for all fcoe_fcf_attrs before
+ * calling fcoe_ctlr_attrs_delete.
+ */
+void fcoe_ctlr_attrs_delete(struct fcoe_ctlr_attrs *ctlr)
+{
+	struct fcoe_fcf_attrs *fcf, *next;
+	/* Remove any attached fcfs */
+	mutex_lock(&ctlr->lock);
+	list_for_each_entry_safe(fcf, next,
+				 &ctlr->fcfs, peers) {
+		list_del(&fcf->peers);
+		fcf->state = FCOE_FCF_STATE_DELETED;
+		fcoe_ctlr_attrs_queue_work(ctlr, &fcf->delete_work);
+	}
+	mutex_unlock(&ctlr->lock);
+
+	fcoe_ctlr_attrs_flush_work(ctlr);
+
+	destroy_workqueue(ctlr->devloss_work_q);
+	ctlr->devloss_work_q = NULL;
+	destroy_workqueue(ctlr->work_q);
+	ctlr->work_q = NULL;
+
+	device_del(&ctlr->dev);
+	put_device(&ctlr->dev); /* self-reference */
+}
+EXPORT_SYMBOL(fcoe_ctlr_attrs_delete);
+
+/**
+ * fcoe_fcf_attrs_release() - Release the FIP fcf memory
+ * @dev: Pointer to the fcf's embedded device
+ *
+ * Called when the last FIP fcf reference is released.
+ */
+static void fcoe_fcf_attrs_release(struct device *dev)
+{
+	struct fcoe_fcf_attrs *fcf = dev_to_fcf(dev);
+	kfree(fcf);
+}
+
+/**
+ * fcoe_fcf_attrs_final_delete() - Final delete routine
+ * @work: The FIP fcf's embedded work struct
+ *
+ * It is expected that the fcf has been removed from
+ * the FIP ctlr's list before calling this routine.
+ */
+static void fcoe_fcf_attrs_final_delete(struct work_struct *work)
+{
+	struct fcoe_fcf_attrs *fcf =
+		container_of(work, struct fcoe_fcf_attrs, delete_work);
+	struct fcoe_ctlr_attrs *ctlr = fcoe_fcf_to_ctlr(fcf);
+
+	/*
+	 * Cancel any outstanding timers. These should really exist
+	 * only when rmmod'ing the LLDD and we're asking for
+	 * immediate termination of the rports
+	 */
+	if (!cancel_delayed_work(&fcf->dev_loss_work))
+		fcoe_ctlr_attrs_flush_devloss(ctlr);
+
+	device_del(&fcf->dev);
+	put_device(&fcf->dev); /* self-reference */
+}
+
+/**
+ * fip_timeout_deleted_fcf() - Delete a fcf when the devloss timer fires
+ * @work: The FIP fcf's embedded work struct
+ *
+ * Removes the fcf from the FIP ctlr's list of fcfs and
+ * queues the final deletion.
+ */
+static void fip_timeout_deleted_fcf(struct work_struct *work)
+{
+	struct fcoe_fcf_attrs *fcf =
+		container_of(work, struct fcoe_fcf_attrs, dev_loss_work.work);
+	struct fcoe_ctlr_attrs *ctlr = fcoe_fcf_to_ctlr(fcf);
+
+	mutex_lock(&ctlr->lock);
+
+	/*
+	 * If the fcf is deleted or reconnected before the timer
+	 * fires the devloss queue will be flushed, but the state will
+	 * either be CONNECTED or DELETED. If that is the case we
+	 * cancel deleting the fcf.
+	 */
+	if (fcf->state != FCOE_FCF_STATE_DISCONNECTED)
+		goto out;
+
+	dev_printk(KERN_ERR, &fcf->dev,
+		   "FIP fcf connection time out: removing fcf\n");
+
+	list_del(&fcf->peers);
+	fcf->state = FCOE_FCF_STATE_DELETED;
+	fcoe_ctlr_attrs_queue_work(ctlr, &fcf->delete_work);
+
+out:
+	mutex_unlock(&ctlr->lock);
+}
+
+/**
+ * fcoe_fcf_attrs_delete() - Delete a FIP fcf
+ * @fcf: Pointer to the fcf which is to be deleted
+ *
+ * Queues the FIP fcf on the devloss workqueue
+ *
+ * Expects the ctlr_attrs mutex to be held for fcf
+ * state change.
+ */
+void fcoe_fcf_attrs_delete(struct fcoe_fcf_attrs *fcf)
+{
+	struct fcoe_ctlr_attrs *ctlr = fcoe_fcf_to_ctlr(fcf);
+	int timeout = fcf->dev_loss_tmo;
+
+	if (fcf->state != FCOE_FCF_STATE_CONNECTED)
+		return;
+
+	fcf->state = FCOE_FCF_STATE_DISCONNECTED;
+
+	/*
+	 * FCF will only be re-connected by the LLD calling
+	 * fcoe_fcf_attrs_add, and it should be setting up
+	 * priv then.
+	 */
+	fcf->priv = NULL;
+
+	fcoe_ctlr_attrs_queue_devloss_work(ctlr, &fcf->dev_loss_work,
+					   timeout * HZ);
+}
+EXPORT_SYMBOL(fcoe_fcf_attrs_delete);
+
+struct class fcoe_fcf_class = {
+	.name = "fcoe_fcf",
+	.dev_release = fcoe_fcf_attrs_release,
+};
+
+
+/**
+ * fcoe_fcf_attrs_add() - Add a FCoE sysfs fcoe_fcf_attrs to the system
+ * @ctlr:    The fcoe_ctlr_attrs that will be the fcoe_fcf_attrs parent
+ * @new_fcf: A temporary FCF used for lookups on the current list of fcfs
+ *
+ * Expects to be called with the ctlr->lock held
+ */
+struct fcoe_fcf_attrs *fcoe_fcf_attrs_add(struct fcoe_ctlr_attrs *ctlr,
+					  struct fcoe_fcf_attrs *new_fcf)
+{
+	struct fcoe_fcf_attrs *fcf;
+	int error = 0;
+
+	list_for_each_entry(fcf, &ctlr->fcfs, peers) {
+		if (fcoe_fcf_attrs_match(new_fcf, fcf)) {
+			if (fcf->state == FCOE_FCF_STATE_CONNECTED)
+				return fcf;
+
+			fcf->state = FCOE_FCF_STATE_CONNECTED;
+
+			if (!cancel_delayed_work(&fcf->dev_loss_work))
+				fcoe_ctlr_attrs_flush_devloss(ctlr);
+
+			return fcf;
+		}
+	}
+
+	fcf = kzalloc(sizeof(struct fcoe_fcf_attrs), GFP_ATOMIC);
+	if (unlikely(!fcf))
+		goto out;
+
+	INIT_WORK(&fcf->delete_work, fcoe_fcf_attrs_final_delete);
+	INIT_DELAYED_WORK(&fcf->dev_loss_work, fip_timeout_deleted_fcf);
+
+	device_initialize(&fcf->dev);
+	fcf->dev.parent = get_device(&ctlr->dev);
+	fcf->dev.class = &fcoe_fcf_class;
+	fcf->id = atomic_inc_return(&fcf_num) - 1;
+	fcf->state = FCOE_FCF_STATE_UNKNOWN;
+
+	fcf->dev_loss_tmo = ctlr->fcf_dev_loss_tmo;
+
+	dev_set_name(&fcf->dev, "fcf_%d", fcf->id);
+
+	fcf->fabric_name = new_fcf->fabric_name;
+	fcf->switch_name = new_fcf->switch_name;
+	fcf->fc_map = new_fcf->fc_map;
+	fcf->vfid = new_fcf->vfid;
+	memcpy(fcf->mac, new_fcf->mac, ETH_ALEN);
+	fcf->priority = new_fcf->priority;
+	fcf->fka_period = new_fcf->fka_period;
+	fcf->selected = new_fcf->selected;
+
+	error = device_add(&fcf->dev);
+	if (error)
+		goto out_del;
+
+	error = sysfs_create_group(&fcf->dev.kobj,
+				   &fcoe_fcf_attribute_group);
+	if (error)
+		goto out_del_dev;
+
+	fcf->state = FCOE_FCF_STATE_CONNECTED;
+	list_add_tail(&fcf->peers, &ctlr->fcfs);
+
+	return fcf;
+
+out_del_dev:
+	device_del(&fcf->dev);
+	list_del(&fcf->peers);
+out_del:
+	put_device(&ctlr->dev);
+	kfree(fcf);
+out:
+	return NULL;
+}
+EXPORT_SYMBOL(fcoe_fcf_attrs_add);
+
+int __init fcoe_sysfs_setup(void)
+{
+	int error;
+
+	atomic_set(&ctlr_num, 0);
+	atomic_set(&fcf_num, 0);
+
+	error = class_register(&fcoe_ctlr_class);
+	if (error)
+		return error;
+
+	error = class_register(&fcoe_fcf_class);
+	if (error)
+		goto unreg_ctlr;
+
+	return 0;
+
+unreg_ctlr:
+	class_unregister(&fcoe_ctlr_class);
+	return error;
+}
+
+void __exit fcoe_sysfs_teardown(void)
+{
+	class_unregister(&fcoe_fcf_class);
+	class_unregister(&fcoe_ctlr_class);
+}
diff --git a/drivers/scsi/fcoe/fcoe_transport.c b/drivers/scsi/fcoe/fcoe_transport.c
index 0897be0..58b6753 100644
--- a/drivers/scsi/fcoe/fcoe_transport.c
+++ b/drivers/scsi/fcoe/fcoe_transport.c
@@ -816,9 +816,17 @@ out_nodev:
  */
 static int __init libfcoe_init(void)
 {
-	fcoe_transport_init();
+	int rc = 0;
 
-	return 0;
+	rc = fcoe_transport_init();
+	if (rc)
+		return rc;
+
+	rc = fcoe_sysfs_setup();
+	if (rc)
+		fcoe_transport_exit();
+
+	return rc;
 }
 module_init(libfcoe_init);
 
@@ -827,6 +835,7 @@ module_init(libfcoe_init);
  */
 static void __exit libfcoe_exit(void)
 {
+	fcoe_sysfs_teardown();
 	fcoe_transport_exit();
 }
 module_exit(libfcoe_exit);
diff --git a/include/scsi/fcoe_sysfs.h b/include/scsi/fcoe_sysfs.h
new file mode 100644
index 0000000..90eab43
--- /dev/null
+++ b/include/scsi/fcoe_sysfs.h
@@ -0,0 +1,124 @@
+/*
+ * Copyright (c) 2011-2012 Intel Corporation.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Maintained at www.Open-FCoE.org
+ */
+
+#ifndef FCOE_SYSFS
+#define FCOE_SYSFS
+
+#include <linux/if_ether.h>
+#include <linux/device.h>
+#include <scsi/fc/fc_fcoe.h>
+
+struct fcoe_ctlr_attrs;
+struct fcoe_fcf_attrs;
+
+struct fcoe_sysfs_function_template {
+	void (*get_fcoe_ctlr_link_fail)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_ctlr_vlink_fail)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_ctlr_miss_fka)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_ctlr_symb_err)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_ctlr_err_block)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_ctlr_fcs_error)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_ctlr_mode)(struct fcoe_ctlr_attrs *);
+	void (*get_fcoe_fcf_selected)(struct fcoe_fcf_attrs *);
+	void (*get_fcoe_fcf_vlan_id)(struct fcoe_fcf_attrs *);
+};
+
+#define dev_to_ctlr(d)					\
+	container_of((d), struct fcoe_ctlr_attrs, dev)
+
+enum fip_conn_type {
+	FIP_CONN_TYPE_UNKNOWN,
+	FIP_CONN_TYPE_FABRIC,
+	FIP_CONN_TYPE_VN2VN,
+};
+
+struct fcoe_ctlr_attrs {
+	u32				id;
+
+	struct device			dev;
+	struct fcoe_sysfs_function_template *f;
+
+	struct list_head		fcfs;
+	char				work_q_name[20];
+	struct workqueue_struct		*work_q;
+	char				devloss_work_q_name[20];
+	struct workqueue_struct		*devloss_work_q;
+	struct mutex			lock;
+
+	int                             fcf_dev_loss_tmo;
+	enum fip_conn_type              mode;
+
+	/* expected in host order for displaying */
+	struct fcoe_fc_els_lesb         lesb;
+};
+
+static inline void *fcoe_ctlr_attrs_priv(const struct fcoe_ctlr_attrs *ctlr)
+{
+	return (void *)(ctlr + 1);
+}
+
+/* fcf states */
+enum fcf_state {
+	FCOE_FCF_STATE_UNKNOWN,
+	FCOE_FCF_STATE_DISCONNECTED,
+	FCOE_FCF_STATE_CONNECTED,
+	FCOE_FCF_STATE_DELETED,
+};
+
+struct fcoe_fcf_attrs {
+	u32		    id;
+	struct device	    dev;
+	struct list_head    peers;
+	struct work_struct  delete_work;
+	struct delayed_work dev_loss_work;
+	u32		    dev_loss_tmo;
+	void                *priv;
+	enum fcf_state      state;
+
+	u64                 fabric_name;
+	u64                 switch_name;
+	u32                 fc_map;
+	u16                 vfid;
+	u8                  mac[ETH_ALEN];
+	u8                  priority;
+	u32                 fka_period;
+	u8                  selected;
+	u16                 vlan_id;
+};
+
+#define dev_to_fcf(d)					\
+	container_of((d), struct fcoe_fcf_attrs, dev)
+/* parentage should never be missing */
+#define fcoe_fcf_to_ctlr(x)			\
+	dev_to_ctlr((x)->dev.parent)
+#define fcoe_fcf_attrs_priv(x)			\
+	((x)->priv)
+
+struct fcoe_ctlr_attrs *fcoe_ctlr_attrs_add(struct device *parent,
+			    struct fcoe_sysfs_function_template *f,
+			    int priv_size);
+void fcoe_ctlr_attrs_delete(struct fcoe_ctlr_attrs *);
+struct fcoe_fcf_attrs *fcoe_fcf_attrs_add(struct fcoe_ctlr_attrs *,
+					  struct fcoe_fcf_attrs *);
+void fcoe_fcf_attrs_delete(struct fcoe_fcf_attrs *);
+
+int __init fcoe_sysfs_setup(void);
+void __exit fcoe_sysfs_teardown(void);
+
+#endif /* FCOE_SYSFS */
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 69eca4b..7b93f21 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -29,6 +29,7 @@
 #include <linux/random.h>
 #include <scsi/fc/fc_fcoe.h>
 #include <scsi/libfc.h>
+#include <scsi/fcoe_sysfs.h>
 
 #define FCOE_MAX_CMD_LEN	16	/* Supported CDB length */
 


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v2 4/4] fcoe, bnx2fc, libfcoe: SW FCoE and bnx2fc use FCoE Syfs
  2012-03-16 19:36 [PATCH v2 0/4] FCoE Sysfs Robert Love
                   ` (2 preceding siblings ...)
  2012-03-16 19:36 ` [PATCH v2 3/4] libfcoe: Add fcoe_sysfs Robert Love
@ 2012-03-16 19:37 ` Robert Love
  3 siblings, 0 replies; 11+ messages in thread
From: Robert Love @ 2012-03-16 19:37 UTC (permalink / raw)
  To: linux-scsi; +Cc: gregkh, giridhar.malavali, james.smart, bprakash

This patch has the SW FCoE driver and the bnx2fc
driver make use of the new fcoe_sysfs API added
earlier in this patch series.

After this patch a fcoe_ctlr_attrs is allocated with
private data in this order.

+-----------------+   +------------------+
| fcoe_ctlr_attrs |   | fcoe_ctlr_attrs  |
+-----------------+   +------------------+
| fcoe_ctlr       |   | fcoe_ctlr        |
+-----------------+   +------------------+
| fcoe_interface  |   | bnx2fc_interface |
+-----------------+   +------------------+

libfcoe also takes part in this new model since it
discovers and manages fcoe_fcf instances. The memory
allocation is different for FCFs. I didn't want to
impact libfcoe's fcoe_fcf processing, so this patch
creates fcoe_fcf_attrs instances for each discovered
fcoe_fcf. The two are paired using a (void * priv)
member of the fcoe_ctlr_attrs. This allows libfcoe
to continue maintaining its list of fcoe_fcf instances
and simply attaches and detaches them from existing
or new fcoe_fcf_attrs instances.

Signed-off-by: Robert Love <robert.w.love@intel.com>
Tested-by: Ross Brattain <ross.b.brattain@intel.com>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c |   63 ++++++++++++++-
 drivers/scsi/fcoe/fcoe.c          |   70 +++++++++++++++-
 drivers/scsi/fcoe/fcoe_ctlr.c     |  159 ++++++++++++++++++++++++++++++++++---
 include/scsi/libfcoe.h            |   17 ++++
 4 files changed, 285 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 90aaaf7..89547f6 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -54,6 +54,7 @@ static struct cnic_ulp_ops bnx2fc_cnic_cb;
 static struct libfc_function_template bnx2fc_libfc_fcn_templ;
 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 void __bnx2fc_destroy(struct bnx2fc_interface *interface);
@@ -88,6 +89,7 @@ static void bnx2fc_port_shutdown(struct fc_lport *lport);
 static void bnx2fc_stop(struct bnx2fc_interface *interface);
 static int __init bnx2fc_mod_init(void);
 static void __exit bnx2fc_mod_exit(void);
+static void bnx2fc_ctlr_get_lesb(struct fcoe_ctlr_attrs *ctlr_attrs);
 
 unsigned int bnx2fc_debug_level;
 module_param_named(debug_logging, bnx2fc_debug_level, int, S_IRUGO|S_IWUSR);
@@ -118,6 +120,41 @@ static void bnx2fc_get_lesb(struct fc_lport *lport,
 	__fcoe_get_lesb(lport, fc_lesb, netdev);
 }
 
+static void bnx2fc_ctlr_get_lesb(struct fcoe_ctlr_attrs *ctlr_attrs)
+{
+	struct fcoe_ctlr *fip = fcoe_ctlr_attrs_priv(ctlr_attrs);
+	struct net_device *netdev = bnx2fc_netdev(fip->lp);
+	struct fcoe_fc_els_lesb *fcoe_lesb;
+	struct fc_els_lesb fc_lesb;
+
+	__fcoe_get_lesb(fip->lp, &fc_lesb, netdev);
+	fcoe_lesb = (struct fcoe_fc_els_lesb *)(&fc_lesb);
+
+	ctlr_attrs->lesb.lesb_link_fail =
+		ntohl(fcoe_lesb->lesb_link_fail);
+	ctlr_attrs->lesb.lesb_vlink_fail =
+		ntohl(fcoe_lesb->lesb_vlink_fail);
+	ctlr_attrs->lesb.lesb_miss_fka =
+		ntohl(fcoe_lesb->lesb_miss_fka);
+	ctlr_attrs->lesb.lesb_symb_err =
+		ntohl(fcoe_lesb->lesb_symb_err);
+	ctlr_attrs->lesb.lesb_err_block =
+		ntohl(fcoe_lesb->lesb_err_block);
+	ctlr_attrs->lesb.lesb_fcs_error =
+		ntohl(fcoe_lesb->lesb_fcs_error);
+}
+EXPORT_SYMBOL(bnx2fc_ctlr_get_lesb);
+
+static void bnx2fc_fcf_get_vlan_id(struct fcoe_fcf_attrs *fcf_attrs)
+{
+	struct fcoe_ctlr_attrs *ctlr_attrs =
+		fcoe_fcf_to_ctlr(fcf_attrs);
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_attrs_priv(ctlr_attrs);
+	struct bnx2fc_interface *fcoe = fcoe_ctlr_priv(ctlr);
+
+	fcf_attrs->vlan_id = fcoe->vlan_id;
+}
+
 static void bnx2fc_clean_rx_queue(struct fc_lport *lp)
 {
 	struct fcoe_percpu_s *bg;
@@ -1236,6 +1273,7 @@ static void bnx2fc_release_transport(void)
 
 static void bnx2fc_interface_release(struct kref *kref)
 {
+	struct fcoe_ctlr_attrs *ctlr_attrs;
 	struct bnx2fc_interface *interface;
 	struct fcoe_ctlr *ctlr;
 	struct net_device *netdev;
@@ -1244,13 +1282,14 @@ static void bnx2fc_interface_release(struct kref *kref)
 	BNX2FC_MISC_DBG("Interface is being released\n");
 
 	ctlr = bnx2fc_to_ctlr(interface);
+	ctlr_attrs = ctlr_to_ctlr_attrs(ctlr);
 	netdev = interface->netdev;
 
 	/* tear-down FIP controller */
 	if (test_and_clear_bit(BNX2FC_CTLR_INIT_DONE, &interface->if_flags))
 		fcoe_ctlr_destroy(ctlr);
 
-	kfree(ctlr);
+	fcoe_ctlr_attrs_delete(ctlr_attrs);
 
 	dev_put(netdev);
 	module_put(THIS_MODULE);
@@ -1343,17 +1382,20 @@ struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
 				      struct net_device *netdev,
 				      enum fip_state fip_mode)
 {
+	struct fcoe_ctlr_attrs *ctlr_attrs;
 	struct bnx2fc_interface *interface;
 	struct fcoe_ctlr *ctlr;
 	int size;
 	int rc = 0;
 
 	size = (sizeof(*interface) + sizeof(struct fcoe_ctlr));
-	ctlr = kzalloc(size, GFP_KERNEL);
-	if (!ctlr) {
+	ctlr_attrs = fcoe_ctlr_attrs_add(&netdev->dev, &bnx2fc_fcoe_sysfs_templ,
+					 size);
+	if (!ctlr_attrs) {
 		printk(KERN_ERR PFX "Unable to allocate interface structure\n");
 		return NULL;
 	}
+	ctlr = fcoe_ctlr_attrs_priv(ctlr_attrs);
 	interface = fcoe_ctlr_priv(ctlr);
 	dev_hold(netdev);
 	kref_init(&interface->kref);
@@ -1373,7 +1415,7 @@ struct bnx2fc_interface *bnx2fc_interface_create(struct bnx2fc_hba *hba,
 
 	fcoe_ctlr_destroy(ctlr);
 	dev_put(netdev);
-	kfree(ctlr);
+	fcoe_ctlr_attrs_delete(ctlr_attrs);
 	return NULL;
 }
 
@@ -2472,6 +2514,19 @@ static void __exit bnx2fc_mod_exit(void)
 module_init(bnx2fc_mod_init);
 module_exit(bnx2fc_mod_exit);
 
+static struct fcoe_sysfs_function_template bnx2fc_fcoe_sysfs_templ = {
+	.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,
+	.get_fcoe_ctlr_miss_fka = bnx2fc_ctlr_get_lesb,
+	.get_fcoe_ctlr_symb_err = bnx2fc_ctlr_get_lesb,
+	.get_fcoe_ctlr_err_block = bnx2fc_ctlr_get_lesb,
+	.get_fcoe_ctlr_fcs_error = bnx2fc_ctlr_get_lesb,
+
+	.get_fcoe_fcf_selected = fcoe_fcf_get_selected,
+	.get_fcoe_fcf_vlan_id = bnx2fc_fcf_get_vlan_id,
+};
+
 static struct fc_function_template bnx2fc_transport_function = {
 	.show_host_node_name = 1,
 	.show_host_port_name = 1,
diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
index 85ab965..bb32cab 100644
--- a/drivers/scsi/fcoe/fcoe.c
+++ b/drivers/scsi/fcoe/fcoe.c
@@ -41,6 +41,7 @@
 
 #include <scsi/fc/fc_encaps.h>
 #include <scsi/fc/fc_fip.h>
+#include <scsi/fc/fc_fcoe.h>
 
 #include <scsi/libfc.h>
 #include <scsi/fc_frame.h>
@@ -150,6 +151,21 @@ static int fcoe_vport_create(struct fc_vport *, bool disabled);
 static int fcoe_vport_disable(struct fc_vport *, bool disable);
 static void fcoe_set_vport_symbolic_name(struct fc_vport *);
 static void fcoe_set_port_id(struct fc_lport *, u32, struct fc_frame *);
+static void fcoe_ctlr_get_lesb(struct fcoe_ctlr_attrs *);
+static void fcoe_fcf_get_vlan_id(struct fcoe_fcf_attrs *);
+
+static struct fcoe_sysfs_function_template fcoe_sysfs_templ = {
+	.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,
+	.get_fcoe_ctlr_miss_fka = fcoe_ctlr_get_lesb,
+	.get_fcoe_ctlr_symb_err = fcoe_ctlr_get_lesb,
+	.get_fcoe_ctlr_err_block = fcoe_ctlr_get_lesb,
+	.get_fcoe_ctlr_fcs_error = fcoe_ctlr_get_lesb,
+
+	.get_fcoe_fcf_selected = fcoe_fcf_get_selected,
+	.get_fcoe_fcf_vlan_id = fcoe_fcf_get_vlan_id,
+};
 
 static struct libfc_function_template fcoe_libfc_fcn_templ = {
 	.frame_send = fcoe_xmit,
@@ -366,6 +382,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
 static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
 						    enum fip_state fip_mode)
 {
+	struct fcoe_ctlr_attrs *ctlr_attrs;
 	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	int size;
@@ -379,14 +396,17 @@ static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
 	}
 
 	size = sizeof(struct fcoe_ctlr) + sizeof(struct fcoe_interface);
-	ctlr = kzalloc(size, GFP_KERNEL);
-	fcoe = fcoe_ctlr_priv(ctlr);
-	if (!fcoe) {
-		FCOE_NETDEV_DBG(netdev, "Could not allocate fcoe structure\n");
+	ctlr_attrs = fcoe_ctlr_attrs_add(&netdev->dev, &fcoe_sysfs_templ,
+					 size);
+	if (!ctlr_attrs) {
+		FCOE_DBG("Failed to add fcoe_ctlr_attrs\n");
 		fcoe = ERR_PTR(-ENOMEM);
 		goto out_putmod;
 	}
 
+	ctlr = fcoe_ctlr_attrs_priv(ctlr_attrs);
+	fcoe = fcoe_ctlr_priv(ctlr);
+
 	dev_hold(netdev);
 
 	/*
@@ -400,6 +420,7 @@ static struct fcoe_interface *fcoe_interface_create(struct net_device *netdev,
 	err = fcoe_interface_setup(fcoe, netdev);
 	if (err) {
 		fcoe_ctlr_destroy(ctlr);
+		fcoe_ctlr_attrs_delete(ctlr_attrs);
 		dev_put(netdev);
 		fcoe = ERR_PTR(err);
 		goto out_putmod;
@@ -423,6 +444,7 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 {
 	struct net_device *netdev = fcoe->netdev;
 	struct fcoe_ctlr *fip = fcoe_to_ctlr(fcoe);
+	struct fcoe_ctlr_attrs *ctlr_attrs = ctlr_to_ctlr_attrs(fip);
 	u8 flogi_maddr[ETH_ALEN];
 	const struct net_device_ops *ops;
 
@@ -462,6 +484,7 @@ static void fcoe_interface_cleanup(struct fcoe_interface *fcoe)
 	/* Release the self-reference taken during fcoe_interface_create() */
 	/* tear-down the FCoE controller */
 	fcoe_ctlr_destroy(fip);
+	fcoe_ctlr_attrs_delete(ctlr_attrs);
 	dev_put(netdev);
 	module_put(THIS_MODULE);
 }
@@ -2175,6 +2198,7 @@ static void fcoe_dcb_create(struct fcoe_interface *fcoe)
 static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 {
 	int rc = 0;
+	struct fcoe_ctlr_attrs *ctlr_attrs;
 	struct fcoe_ctlr *ctlr;
 	struct fcoe_interface *fcoe;
 	struct fc_lport *lport;
@@ -2195,8 +2219,8 @@ static int fcoe_create(struct net_device *netdev, enum fip_state fip_mode)
 	}
 
 	ctlr = fcoe_to_ctlr(fcoe);
-
-	lport = fcoe_if_create(fcoe, &netdev->dev, 0);
+	ctlr_attrs = ctlr_to_ctlr_attrs(ctlr);
+	lport = fcoe_if_create(fcoe, &ctlr_attrs->dev, 0);
 	if (IS_ERR(lport)) {
 		printk(KERN_ERR "fcoe: Failed to create interface (%s)\n",
 		       netdev->name);
@@ -2744,6 +2768,40 @@ static void fcoe_get_lesb(struct fc_lport *lport,
 	__fcoe_get_lesb(lport, fc_lesb, netdev);
 }
 
+static void fcoe_ctlr_get_lesb(struct fcoe_ctlr_attrs *ctlr_attrs)
+{
+	struct fcoe_ctlr *fip = fcoe_ctlr_attrs_priv(ctlr_attrs);
+	struct net_device *netdev = fcoe_netdev(fip->lp);
+	struct fcoe_fc_els_lesb *fcoe_lesb;
+	struct fc_els_lesb fc_lesb;
+
+	__fcoe_get_lesb(fip->lp, &fc_lesb, netdev);
+	fcoe_lesb = (struct fcoe_fc_els_lesb *)(&fc_lesb);
+
+	ctlr_attrs->lesb.lesb_link_fail =
+		ntohl(fcoe_lesb->lesb_link_fail);
+	ctlr_attrs->lesb.lesb_vlink_fail =
+		ntohl(fcoe_lesb->lesb_vlink_fail);
+	ctlr_attrs->lesb.lesb_miss_fka =
+		ntohl(fcoe_lesb->lesb_miss_fka);
+	ctlr_attrs->lesb.lesb_symb_err =
+		ntohl(fcoe_lesb->lesb_symb_err);
+	ctlr_attrs->lesb.lesb_err_block =
+		ntohl(fcoe_lesb->lesb_err_block);
+	ctlr_attrs->lesb.lesb_fcs_error =
+		ntohl(fcoe_lesb->lesb_fcs_error);
+}
+
+static void fcoe_fcf_get_vlan_id(struct fcoe_fcf_attrs *fcf_attrs)
+{
+	struct fcoe_ctlr_attrs *ctlr_attrs =
+		fcoe_fcf_to_ctlr(fcf_attrs);
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_attrs_priv(ctlr_attrs);
+	struct fcoe_interface *fcoe = fcoe_ctlr_priv(ctlr);
+
+	fcf_attrs->vlan_id = vlan_dev_vlan_id(fcoe->netdev);
+}
+
 /**
  * fcoe_set_port_id() - Callback from libfc when Port_ID is set.
  * @lport: the local port
diff --git a/drivers/scsi/fcoe/fcoe_ctlr.c b/drivers/scsi/fcoe/fcoe_ctlr.c
index 249a106..00887cf 100644
--- a/drivers/scsi/fcoe/fcoe_ctlr.c
+++ b/drivers/scsi/fcoe/fcoe_ctlr.c
@@ -160,6 +160,76 @@ void fcoe_ctlr_init(struct fcoe_ctlr *fip, enum fip_state mode)
 }
 EXPORT_SYMBOL(fcoe_ctlr_init);
 
+static int fcoe_sysfs_fcf_add(struct fcoe_fcf *new)
+{
+	struct fcoe_ctlr *fip = new->fip;
+	struct fcoe_ctlr_attrs *ctlr_attrs = ctlr_to_ctlr_attrs(fip);
+	struct fcoe_fcf_attrs temp, *fcf_attrs;
+	int rc = 0;
+
+	LIBFCOE_FIP_DBG(fip, "New FCF fab %16.16llx mac %pM\n",
+			new->fabric_name, new->fcf_mac);
+
+	mutex_lock(&ctlr_attrs->lock);
+
+	temp.fabric_name = new->fabric_name;
+	temp.switch_name = new->switch_name;
+	temp.fc_map = new->fc_map;
+	temp.vfid = new->vfid;
+	memcpy(temp.mac, new->fcf_mac, ETH_ALEN);
+	temp.priority = new->pri;
+	temp.fka_period = new->fka_period;
+	temp.selected = 0; /* default to unselected */
+
+	fcf_attrs = fcoe_fcf_attrs_add(ctlr_attrs, &temp);
+	if (unlikely(!fcf_attrs)) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	/*
+	 * The fcoe_sysfs layer can return a CONNECTED fcf that
+	 * has a priv (fcf was never deleted) or a CONNECTED fcf
+	 * that doesn't have a priv (fcf was deleted). However,
+	 * libfcoe will always delete FCFs before trying to add
+	 * them. This is ensured because both recv_adv and
+	 * age_fcfs are protected by the the fcoe_ctlr's mutex.
+	 * This means that we should never get a FCF with a
+	 * non-NULL priv pointer.
+	 */
+	BUG_ON(fcf_attrs->priv);
+
+	fcf_attrs->priv = new;
+	new->fcf_attrs = fcf_attrs;
+
+	list_add(&new->list, &fip->fcfs);
+	fip->fcf_count++;
+
+out:
+	mutex_unlock(&ctlr_attrs->lock);
+	return rc;
+}
+
+static void fcoe_sysfs_fcf_del(struct fcoe_fcf *new)
+{
+	struct fcoe_ctlr *fip = new->fip;
+	struct fcoe_ctlr_attrs *ctlr_attrs = ctlr_to_ctlr_attrs(fip);
+	struct fcoe_fcf_attrs *fcf_attrs;
+
+	list_del(&new->list);
+	fip->fcf_count--;
+
+	mutex_lock(&ctlr_attrs->lock);
+
+	fcf_attrs = fcf_to_fcf_attrs(new);
+	WARN_ON(!fcf_attrs);
+	new->fcf_attrs = NULL;
+	fcoe_fcf_attrs_delete(fcf_attrs);
+	kfree(new);
+
+	mutex_unlock(&ctlr_attrs->lock);
+}
+
 /**
  * fcoe_ctlr_reset_fcfs() - Reset and free all FCFs for a controller
  * @fip: The FCoE controller whose FCFs are to be reset
@@ -173,10 +243,10 @@ static void fcoe_ctlr_reset_fcfs(struct fcoe_ctlr *fip)
 
 	fip->sel_fcf = NULL;
 	list_for_each_entry_safe(fcf, next, &fip->fcfs, list) {
-		list_del(&fcf->list);
-		kfree(fcf);
+		fcoe_sysfs_fcf_del(fcf);
 	}
-	fip->fcf_count = 0;
+	WARN_ON(fip->fcf_count);
+
 	fip->sel_time = 0;
 }
 
@@ -717,8 +787,11 @@ static unsigned long fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
 	unsigned long next_timer = jiffies + msecs_to_jiffies(FIP_VN_KA_PERIOD);
 	unsigned long deadline;
 	unsigned long sel_time = 0;
+	struct list_head del_list;
 	struct fcoe_dev_stats *stats;
 
+	INIT_LIST_HEAD(&del_list);
+
 	stats = per_cpu_ptr(fip->lp->dev_stats, get_cpu());
 
 	list_for_each_entry_safe(fcf, next, &fip->fcfs, list) {
@@ -739,10 +812,13 @@ static unsigned long fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
 		if (time_after_eq(jiffies, deadline)) {
 			if (fip->sel_fcf == fcf)
 				fip->sel_fcf = NULL;
+			/*
+			 * Move to delete list so we can call
+			 * fcoe_sysfs_fcf_del (which can sleep)
+			 * after the put_cpu().
+			 */
 			list_del(&fcf->list);
-			WARN_ON(!fip->fcf_count);
-			fip->fcf_count--;
-			kfree(fcf);
+			list_add(&fcf->list, &del_list);
 			stats->VLinkFailureCount++;
 		} else {
 			if (time_after(next_timer, deadline))
@@ -753,6 +829,12 @@ static unsigned long fcoe_ctlr_age_fcfs(struct fcoe_ctlr *fip)
 		}
 	}
 	put_cpu();
+
+	list_for_each_entry_safe(fcf, next, &del_list, list) {
+		/* Removes fcf from current list */
+		fcoe_sysfs_fcf_del(fcf);
+	}
+
 	if (sel_time && !fip->sel_fcf && !fip->sel_time) {
 		sel_time += msecs_to_jiffies(FCOE_CTLR_START_DELAY);
 		fip->sel_time = sel_time;
@@ -903,23 +985,23 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip, struct sk_buff *skb)
 {
 	struct fcoe_fcf *fcf;
 	struct fcoe_fcf new;
-	struct fcoe_fcf *found;
 	unsigned long sol_tov = msecs_to_jiffies(FCOE_CTRL_SOL_TOV);
 	int first = 0;
 	int mtu_valid;
+	int found = 0;
+	int rc = 0;
 
 	if (fcoe_ctlr_parse_adv(fip, skb, &new))
 		return;
 
 	mutex_lock(&fip->ctlr_mutex);
 	first = list_empty(&fip->fcfs);
-	found = NULL;
 	list_for_each_entry(fcf, &fip->fcfs, list) {
 		if (fcf->switch_name == new.switch_name &&
 		    fcf->fabric_name == new.fabric_name &&
 		    fcf->fc_map == new.fc_map &&
 		    compare_ether_addr(fcf->fcf_mac, new.fcf_mac) == 0) {
-			found = fcf;
+			found = 1;
 			break;
 		}
 	}
@@ -931,9 +1013,16 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip, struct sk_buff *skb)
 		if (!fcf)
 			goto out;
 
-		fip->fcf_count++;
 		memcpy(fcf, &new, sizeof(new));
-		list_add(&fcf->list, &fip->fcfs);
+		fcf->fip = fip;
+		rc = fcoe_sysfs_fcf_add(fcf);
+		if (rc) {
+			printk(KERN_ERR "Failed to allocate sysfs instance "
+			       "for FCF, fab %16.16llx mac %pM\n",
+			       new.fabric_name, new.fcf_mac);
+			kfree(fcf);
+			goto out;
+		}
 	} else {
 		/*
 		 * Update the FCF's keep-alive descriptor flags.
@@ -954,6 +1043,7 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip, struct sk_buff *skb)
 		fcf->fka_period = new.fka_period;
 		memcpy(fcf->fcf_mac, new.fcf_mac, ETH_ALEN);
 	}
+
 	mtu_valid = fcoe_ctlr_mtu_valid(fcf);
 	fcf->time = jiffies;
 	if (!found)
@@ -996,6 +1086,7 @@ static void fcoe_ctlr_recv_adv(struct fcoe_ctlr *fip, struct sk_buff *skb)
 		    time_before(fip->sel_time, fip->timer.expires))
 			mod_timer(&fip->timer, fip->sel_time);
 	}
+
 out:
 	mutex_unlock(&fip->ctlr_mutex);
 }
@@ -2712,9 +2803,9 @@ unlock:
 
 /**
  * fcoe_libfc_config() - Sets up libfc related properties for local port
- * @lp: The local port to configure libfc for
- * @fip: The FCoE controller in use by the local port
- * @tt: The libfc function template
+ * @lport:    The local port to configure libfc for
+ * @fip:      The FCoE controller in use by the local port
+ * @tt:       The libfc function template
  * @init_fcp: If non-zero, the FCP portion of libfc should be initialized
  *
  * Returns : 0 for success
@@ -2747,3 +2838,43 @@ int fcoe_libfc_config(struct fc_lport *lport, struct fcoe_ctlr *fip,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fcoe_libfc_config);
+
+void fcoe_fcf_get_selected(struct fcoe_fcf_attrs *fcf_attrs)
+{
+	struct fcoe_ctlr_attrs *ctlr_attrs = fcoe_fcf_to_ctlr(fcf_attrs);
+	struct fcoe_ctlr *fip = fcoe_ctlr_attrs_priv(ctlr_attrs);
+	struct fcoe_fcf *fcf;
+
+	mutex_lock(&fip->ctlr_mutex);
+	mutex_lock(&ctlr_attrs->lock);
+
+	fcf = fcoe_fcf_attrs_priv(fcf_attrs);
+	if (fcf)
+		fcf_attrs->selected = (fcf == fip->sel_fcf) ? 1 : 0;
+	else
+		fcf_attrs->selected = 0;
+
+	mutex_unlock(&ctlr_attrs->lock);
+	mutex_unlock(&fip->ctlr_mutex);
+}
+EXPORT_SYMBOL(fcoe_fcf_get_selected);
+
+void fcoe_ctlr_get_fip_mode(struct fcoe_ctlr_attrs *ctlr_attrs)
+{
+	struct fcoe_ctlr *ctlr = fcoe_ctlr_attrs_priv(ctlr_attrs);
+
+	mutex_lock(&ctlr->ctlr_mutex);
+	switch (ctlr->mode) {
+	case FIP_MODE_FABRIC:
+		ctlr_attrs->mode = FIP_CONN_TYPE_FABRIC;
+		break;
+	case FIP_MODE_VN2VN:
+		ctlr_attrs->mode = FIP_CONN_TYPE_VN2VN;
+		break;
+	default:
+		ctlr_attrs->mode = FIP_CONN_TYPE_UNKNOWN;
+		break;
+	}
+	mutex_unlock(&ctlr->ctlr_mutex);
+}
+EXPORT_SYMBOL(fcoe_ctlr_get_fip_mode);
diff --git a/include/scsi/libfcoe.h b/include/scsi/libfcoe.h
index 7b93f21..d46ac6e 100644
--- a/include/scsi/libfcoe.h
+++ b/include/scsi/libfcoe.h
@@ -168,9 +168,16 @@ static inline void *fcoe_ctlr_priv(const struct fcoe_ctlr *ctlr)
 	return (void *)(ctlr + 1);
 }
 
+#define ctlr_to_ctlr_attrs(x)						\
+	(struct fcoe_ctlr_attrs *)(((struct fcoe_ctlr_attrs *)(x)) - 1)
+
 /**
  * struct fcoe_fcf - Fibre-Channel Forwarder
  * @list:	 list linkage
+ * @event_work:  Work for FC Transport actions queue
+ * @event:       The event to be processed
+ * @fip:         The controller that the FCF was discovered on
+ * @fcf_attrs:   The associated fcoe_fcf_attrs instance
  * @time:	 system time (jiffies) when an advertisement was last received
  * @switch_name: WWN of switch from advertisement
  * @fabric_name: WWN of fabric from advertisement
@@ -192,6 +199,9 @@ static inline void *fcoe_ctlr_priv(const struct fcoe_ctlr *ctlr)
  */
 struct fcoe_fcf {
 	struct list_head list;
+	struct work_struct event_work;
+	struct fcoe_ctlr *fip;
+	struct fcoe_fcf_attrs *fcf_attrs;
 	unsigned long time;
 
 	u64 switch_name;
@@ -208,6 +218,9 @@ struct fcoe_fcf {
 	u8 fd_flags:1;
 };
 
+#define fcf_to_fcf_attrs(x)			\
+	((x)->fcf_attrs)
+
 /**
  * struct fcoe_rport - VN2VN remote port
  * @time:	time of create or last beacon packet received from node
@@ -343,6 +356,10 @@ void fcoe_queue_timer(ulong lport);
 int fcoe_get_paged_crc_eof(struct sk_buff *skb, int tlen,
 			   struct fcoe_percpu_s *fps);
 
+/* FCoE Sysfs helpers */
+void fcoe_fcf_get_selected(struct fcoe_fcf_attrs *);
+void fcoe_ctlr_get_fip_mode(struct fcoe_ctlr_attrs *);
+
 /**
  * struct netdev_list
  * A mapping from netdevice to fcoe_transport


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-16 19:36 ` [PATCH v2 3/4] libfcoe: Add fcoe_sysfs Robert Love
@ 2012-03-17  0:25   ` Greg KH
  2012-03-17  1:12     ` Love, Robert W
  2012-03-20  1:23     ` Love, Robert W
  0 siblings, 2 replies; 11+ messages in thread
From: Greg KH @ 2012-03-17  0:25 UTC (permalink / raw)
  To: Robert Love; +Cc: linux-scsi, giridhar.malavali, james.smart, bprakash

On Fri, Mar 16, 2012 at 12:36:56PM -0700, Robert Love wrote:
> This patch adds infrastructure to libfcoe allow LLDs to
> present FIP (FCoE Initialization Protocol) discovered
> entities and their attributes to user space via sysfs.
> 
> This patch adds the following APIs-
> 
> fcoe_ctlr_attrs_add
> fcoe_ctlr_attrs_delete
> fcoe_fcf_attrs_add
> fcoe_fcf_attrs_delete

Maybe I'm a bit confused, but "traditionally" attrs are sysfs attribute
files, right?  These need to be created _before_ the device is exposed
to userspace through a uevent call.  Having a separate function for
these is usually too late, unless your fcoe core delays the uevent call
until after these are called?

Who would make these function calls, individual drivers?  Why doesn't
the core do this for all fcoe devices?

> They allow the LLD to expose the FCoE ENode Controller
> and any discovered FCFs (Fibre Channel Forwarders, e.g.
> FCoE switches) to the user. Each of these new devices
> has their own class so that they are grouped together
> for easy lookup from a user space application. Each
> new class has an attribute_group to expose attributes
> for any created instances. The attributes are-
> 
> fcoe_ctlr_attrs
> * fcf_dev_loss_tmo
> * lesb_link_fail
> * lesb_vlink_fail
> * lesb_miss_fka
> * lesb_symb_err
> * lesb_err_block
> * lesb_fcs_error
> 
> fcoe_fcf_attrs
> * fabric_name
> * switch_name
> * priority
> * selected
> * fc_map
> * vfid
> * mac
> * fka_peroid
> * fabric_state
> * dev_loss_tmo
> 
> A device loss infrastructre similar to the FC Transport's
> is also added by this patch. It is nice to have so that a
> link flapping adapter doesn't continually advance the count
> used to identify the discovered FCF. FCFs will exist in a
> "Disconnected" state until either the timer expires or the
> FCF is rediscovered and becomes "Connected."
> 
> This patch generates a few checkpatch.pl WARNINGS that
> I'm not sure what to do about. They're macros modeled
> around the FC Transport attribute building macros, which
> have the same 'feature' where the caller can ommit a cast
> in the argument list and no cast occurs in the code. I'm
> not sure how to keep the code condensed while keeping the
> macros. Any advice would be appreciated.
> 
> Signed-off-by: Robert Love <robert.w.love@intel.com>
> Tested-by: Ross Brattain <ross.b.brattain@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-class-fcoe |   77 +++
>  drivers/scsi/fcoe/Makefile                 |    2 
>  drivers/scsi/fcoe/fcoe_sysfs.c             |  840 ++++++++++++++++++++++++++++
>  drivers/scsi/fcoe/fcoe_transport.c         |   13 
>  include/scsi/fcoe_sysfs.h                  |  124 ++++
>  include/scsi/libfcoe.h                     |    1 
>  6 files changed, 1054 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-fcoe
>  create mode 100644 drivers/scsi/fcoe/fcoe_sysfs.c
>  create mode 100644 include/scsi/fcoe_sysfs.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fcoe b/Documentation/ABI/testing/sysfs-class-fcoe
> new file mode 100644
> index 0000000..e9cd7e9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fcoe
> @@ -0,0 +1,77 @@
> +What:		/sys/class/fcoe_ctlr/ctlr_X

We are really trying to stay away from new classes being created.  Why
can't this be a bus and have the devices attach to that instead?  You
can have one bus with both types of "devices" attached to it, making
things a bit simpler in the end.

> +static atomic_t ctlr_num;
> +static atomic_t fcf_num;

I don't think you want to use an atomic variable here, see below for
why.

> +/**
> + * fcoe_ctlr_attrs_add() - Add a FIP ctlr to sysfs
> + * @parent:    The parent device to which the fcoe_ctlr instance
> + *             should be attached
> + * @f:         The LLD's FCoE sysfs function template pointer
> + * @priv_size: Size to be allocated with the fcoe_ctlr_attrs for the LLD
> + *
> + * This routine allocates a FIP ctlr object with some additional memory
> + * for the LLD. The FIP ctlr is initialized, added to sysfs and then
> + * attributes are added to it.
> + */
> +struct fcoe_ctlr_attrs *fcoe_ctlr_attrs_add(struct device *parent,
> +				    struct fcoe_sysfs_function_template *f,
> +				    int priv_size)
> +{
> +	struct fcoe_ctlr_attrs *ctlr;
> +	int error = 0;
> +
> +	ctlr = kzalloc(sizeof(struct fcoe_ctlr_attrs) + priv_size,
> +		       GFP_KERNEL);
> +	if (!ctlr)
> +		goto out;
> +
> +	ctlr->id = atomic_inc_return(&ctlr_num) - 1;

Does this work properly over the long run?  Shouldn't you use the idr
interface instead, to keep holes from showing up?

> +	ctlr->f = f;
> +	INIT_LIST_HEAD(&ctlr->fcfs);
> +	mutex_init(&ctlr->lock);
> +	device_initialize(&ctlr->dev);
> +	ctlr->dev.parent = get_device(parent);

I thought you dropped these get_device() calls on the parent?  The
driver core handles this for you, please don't do it here, it's not
needed.

> +	ctlr->dev.class = &fcoe_ctlr_class;
> +
> +	ctlr->fcf_dev_loss_tmo = fcoe_fcf_dev_loss_tmo;
> +
> +	snprintf(ctlr->work_q_name, sizeof(ctlr->work_q_name),
> +		 "ctlr_wq_%d", ctlr->id);
> +	ctlr->work_q = create_singlethread_workqueue(
> +		ctlr->work_q_name);
> +	if (!ctlr->work_q)
> +		goto out_del;
> +
> +	snprintf(ctlr->devloss_work_q_name,
> +		 sizeof(ctlr->devloss_work_q_name),
> +		 "ctlr_dl_wq_%d", ctlr->id);
> +	ctlr->devloss_work_q = create_singlethread_workqueue(
> +		ctlr->devloss_work_q_name);
> +	if (!ctlr->devloss_work_q)
> +		goto out_del_q;
> +
> +	dev_set_name(&ctlr->dev, "ctlr_%d", ctlr->id);
> +	error = device_add(&ctlr->dev);
> +	if (error)
> +		goto out_del_q2;
> +
> +	error = sysfs_create_group(&ctlr->dev.kobj,
> +				   &fcoe_ctlr_attribute_group);
> +	if (error)
> +		goto out_del_dev;
> +
> +	error = sysfs_create_group(&ctlr->dev.kobj,
> +				   &fcoe_ctlr_lesb_attribute_group);
> +	if (error)
> +		goto out_del_dev;

You just raced userspace by creating your attributes after device_add()
causing lots of problems in the longrun.  Why not make these default
attribute groups that the driver core automatically creates for you
properly?  That also makes your error path simpler, as well as your
cleanup path for when this device goes away.

> +	return ctlr;
> +
> +out_del_dev:
> +	device_del(&ctlr->dev);
> +out_del_q2:
> +	destroy_workqueue(ctlr->devloss_work_q);
> +	ctlr->devloss_work_q = NULL;
> +out_del_q:
> +	destroy_workqueue(ctlr->work_q);
> +	ctlr->work_q = NULL;
> +out_del:
> +	put_device(parent);
> +	kfree(ctlr);
> +out:
> +	return NULL;
> +}
> +EXPORT_SYMBOL(fcoe_ctlr_attrs_add);

EXPORT_SYMBOL_GPL() for this, and other exports?

> +/**
> + * fcoe_fcf_attrs_add() - Add a FCoE sysfs fcoe_fcf_attrs to the system
> + * @ctlr:    The fcoe_ctlr_attrs that will be the fcoe_fcf_attrs parent
> + * @new_fcf: A temporary FCF used for lookups on the current list of fcfs
> + *
> + * Expects to be called with the ctlr->lock held
> + */
> +struct fcoe_fcf_attrs *fcoe_fcf_attrs_add(struct fcoe_ctlr_attrs *ctlr,
> +					  struct fcoe_fcf_attrs *new_fcf)
> +{
> +	struct fcoe_fcf_attrs *fcf;
> +	int error = 0;
> +
> +	list_for_each_entry(fcf, &ctlr->fcfs, peers) {
> +		if (fcoe_fcf_attrs_match(new_fcf, fcf)) {
> +			if (fcf->state == FCOE_FCF_STATE_CONNECTED)
> +				return fcf;
> +
> +			fcf->state = FCOE_FCF_STATE_CONNECTED;
> +
> +			if (!cancel_delayed_work(&fcf->dev_loss_work))
> +				fcoe_ctlr_attrs_flush_devloss(ctlr);
> +
> +			return fcf;
> +		}
> +	}
> +
> +	fcf = kzalloc(sizeof(struct fcoe_fcf_attrs), GFP_ATOMIC);
> +	if (unlikely(!fcf))
> +		goto out;
> +
> +	INIT_WORK(&fcf->delete_work, fcoe_fcf_attrs_final_delete);
> +	INIT_DELAYED_WORK(&fcf->dev_loss_work, fip_timeout_deleted_fcf);
> +
> +	device_initialize(&fcf->dev);
> +	fcf->dev.parent = get_device(&ctlr->dev);

Same thing with the get_device() call here.

> +	fcf->dev.class = &fcoe_fcf_class;
> +	fcf->id = atomic_inc_return(&fcf_num) - 1;

And the id.

> +	fcf->state = FCOE_FCF_STATE_UNKNOWN;
> +
> +	fcf->dev_loss_tmo = ctlr->fcf_dev_loss_tmo;
> +
> +	dev_set_name(&fcf->dev, "fcf_%d", fcf->id);
> +
> +	fcf->fabric_name = new_fcf->fabric_name;
> +	fcf->switch_name = new_fcf->switch_name;
> +	fcf->fc_map = new_fcf->fc_map;
> +	fcf->vfid = new_fcf->vfid;
> +	memcpy(fcf->mac, new_fcf->mac, ETH_ALEN);
> +	fcf->priority = new_fcf->priority;
> +	fcf->fka_period = new_fcf->fka_period;
> +	fcf->selected = new_fcf->selected;
> +
> +	error = device_add(&fcf->dev);
> +	if (error)
> +		goto out_del;
> +
> +	error = sysfs_create_group(&fcf->dev.kobj,
> +				   &fcoe_fcf_attribute_group);
> +	if (error)
> +		goto out_del_dev;

Same thing here, you just raced userspace.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-17  0:25   ` Greg KH
@ 2012-03-17  1:12     ` Love, Robert W
  2012-03-17  9:07       ` James Bottomley
  2012-03-20 21:01       ` Greg KH
  2012-03-20  1:23     ` Love, Robert W
  1 sibling, 2 replies; 11+ messages in thread
From: Love, Robert W @ 2012-03-17  1:12 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-scsi@vger.kernel.org, giridhar.malavali@qlogic.com,
	james.smart@emulex.com, bprakash@broadcom.com

On 03/16/2012 05:25 PM, Greg KH wrote:
> On Fri, Mar 16, 2012 at 12:36:56PM -0700, Robert Love wrote:
>> This patch adds infrastructure to libfcoe allow LLDs to
>> present FIP (FCoE Initialization Protocol) discovered
>> entities and their attributes to user space via sysfs.
>>
>> This patch adds the following APIs-
>>
>> fcoe_ctlr_attrs_add
>> fcoe_ctlr_attrs_delete
>> fcoe_fcf_attrs_add
>> fcoe_fcf_attrs_delete
> Maybe I'm a bit confused, but "traditionally" attrs are sysfs attribute
> files, right?  These need to be created _before_ the device is exposed
> to userspace through a uevent call.  Having a separate function for
> these is usually too late, unless your fcoe core delays the uevent call
> until after these are called?
>
> Who would make these function calls, individual drivers?  Why doesn't
> the core do this for all fcoe devices?

I think my function names are misleading... These aren't explicitly 
adding attributes to a device, they're adding and removing the devices 
to the system. The fcoe core (libfcoe) already has 'struct fcoe_ctlr' 
and 'struct fcoe_fcf' for protocol processing, so the structures that I 
use (which are 1:1 with those protocol structures) are named 
fcoe_*_attrs. They're the structures that are only used to store the 
values for the sysfs attributes, so that they can be read when queried 
by the 'show' handlers.

With this patch series this new API is only used by the fcoe core 
(libfcoe). Traditional HBAs that have the FCoE capability do not 
currently use libfcoe. Only the vendors with SW FCoE solutions use it 
(Intel, Broadcom, Cisco) and they get this feature simply because they 
use libfcoe for the protocol processing. libfcoe is the only user now, 
but I believe that my design would allow for HBA/CNA cards to use this 
new presentation layer without using the rest of libfcoe.

I debated whether I should just re-use the "protocol structures" or 
create these new "attribute structures." I decided to have new 
structures so that the HBA/CNAs that don't use libfcoe could just deal 
with structures for the presentation and didn't have to deal with 
structures that had members that libfcoe used internally. This is how 
the FC Transport (scsi_transport_fc.c) does things. It has its own 
'struct fc_rport' that is used by many drivers, but only for 
presentation. Each driver has it's own "protocol structure" for rports 
(remote ports) that will be 1:1 with the FC Transports "attribute 
structure" fc_rport.


>> They allow the LLD to expose the FCoE ENode Controller
>> and any discovered FCFs (Fibre Channel Forwarders, e.g.
>> FCoE switches) to the user. Each of these new devices
>> has their own class so that they are grouped together
>> for easy lookup from a user space application. Each
>> new class has an attribute_group to expose attributes
>> for any created instances. The attributes are-
>>
>> fcoe_ctlr_attrs
>> * fcf_dev_loss_tmo
>> * lesb_link_fail
>> * lesb_vlink_fail
>> * lesb_miss_fka
>> * lesb_symb_err
>> * lesb_err_block
>> * lesb_fcs_error
>>
>> fcoe_fcf_attrs
>> * fabric_name
>> * switch_name
>> * priority
>> * selected
>> * fc_map
>> * vfid
>> * mac
>> * fka_peroid
>> * fabric_state
>> * dev_loss_tmo
>>
>> A device loss infrastructre similar to the FC Transport's
>> is also added by this patch. It is nice to have so that a
>> link flapping adapter doesn't continually advance the count
>> used to identify the discovered FCF. FCFs will exist in a
>> "Disconnected" state until either the timer expires or the
>> FCF is rediscovered and becomes "Connected."
>>
>> This patch generates a few checkpatch.pl WARNINGS that
>> I'm not sure what to do about. They're macros modeled
>> around the FC Transport attribute building macros, which
>> have the same 'feature' where the caller can ommit a cast
>> in the argument list and no cast occurs in the code. I'm
>> not sure how to keep the code condensed while keeping the
>> macros. Any advice would be appreciated.
>>
>> Signed-off-by: Robert Love<robert.w.love@intel.com>
>> Tested-by: Ross Brattain<ross.b.brattain@intel.com>
>> ---
>>   Documentation/ABI/testing/sysfs-class-fcoe |   77 +++
>>   drivers/scsi/fcoe/Makefile                 |    2
>>   drivers/scsi/fcoe/fcoe_sysfs.c             |  840 ++++++++++++++++++++++++++++
>>   drivers/scsi/fcoe/fcoe_transport.c         |   13
>>   include/scsi/fcoe_sysfs.h                  |  124 ++++
>>   include/scsi/libfcoe.h                     |    1
>>   6 files changed, 1054 insertions(+), 3 deletions(-)
>>   create mode 100644 Documentation/ABI/testing/sysfs-class-fcoe
>>   create mode 100644 drivers/scsi/fcoe/fcoe_sysfs.c
>>   create mode 100644 include/scsi/fcoe_sysfs.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-fcoe b/Documentation/ABI/testing/sysfs-class-fcoe
>> new file mode 100644
>> index 0000000..e9cd7e9
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-fcoe
>> @@ -0,0 +1,77 @@
>> +What:		/sys/class/fcoe_ctlr/ctlr_X
> We are really trying to stay away from new classes being created.  Why
> can't this be a bus and have the devices attach to that instead?  You
> can have one bus with both types of "devices" attached to it, making
> things a bit simpler in the end.

I did have a series about a year ago, when this series was overly 
grandiose, and I was using a bus. I don't have a reason for using 
classes vs. a bus other than the FC Transport, which this code is 
heavily modeled after, uses classes. If I remember correctly my 
fcoe_fcf_attrs and fcoe_ctlr_attrs would become the drivers for the bus 
and devices would become instances of that driver on the bus. I'm sure 
I'm butchering the terminology...

>> +static atomic_t ctlr_num;
>> +static atomic_t fcf_num;
> I don't think you want to use an atomic variable here, see below for
> why.
>
>> +/**
>> + * fcoe_ctlr_attrs_add() - Add a FIP ctlr to sysfs
>> + * @parent:    The parent device to which the fcoe_ctlr instance
>> + *             should be attached
>> + * @f:         The LLD's FCoE sysfs function template pointer
>> + * @priv_size: Size to be allocated with the fcoe_ctlr_attrs for the LLD
>> + *
>> + * This routine allocates a FIP ctlr object with some additional memory
>> + * for the LLD. The FIP ctlr is initialized, added to sysfs and then
>> + * attributes are added to it.
>> + */
>> +struct fcoe_ctlr_attrs *fcoe_ctlr_attrs_add(struct device *parent,
>> +				    struct fcoe_sysfs_function_template *f,
>> +				    int priv_size)
>> +{
>> +	struct fcoe_ctlr_attrs *ctlr;
>> +	int error = 0;
>> +
>> +	ctlr = kzalloc(sizeof(struct fcoe_ctlr_attrs) + priv_size,
>> +		       GFP_KERNEL);
>> +	if (!ctlr)
>> +		goto out;
>> +
>> +	ctlr->id = atomic_inc_return(&ctlr_num) - 1;
> Does this work properly over the long run?  Shouldn't you use the idr
> interface instead, to keep holes from showing up?

I'm not familiar wit the idr interface. I'll ask around and fix this.

>> +	ctlr->f = f;
>> +	INIT_LIST_HEAD(&ctlr->fcfs);
>> +	mutex_init(&ctlr->lock);
>> +	device_initialize(&ctlr->dev);
>> +	ctlr->dev.parent = get_device(parent);
> I thought you dropped these get_device() calls on the parent?  The
> driver core handles this for you, please don't do it here, it's not
> needed.

How would the driver core know what this device's parent is? This is the 
only place I do that assignment.

>> +	ctlr->dev.class =&fcoe_ctlr_class;
>> +
>> +	ctlr->fcf_dev_loss_tmo = fcoe_fcf_dev_loss_tmo;
>> +
>> +	snprintf(ctlr->work_q_name, sizeof(ctlr->work_q_name),
>> +		 "ctlr_wq_%d", ctlr->id);
>> +	ctlr->work_q = create_singlethread_workqueue(
>> +		ctlr->work_q_name);
>> +	if (!ctlr->work_q)
>> +		goto out_del;
>> +
>> +	snprintf(ctlr->devloss_work_q_name,
>> +		 sizeof(ctlr->devloss_work_q_name),
>> +		 "ctlr_dl_wq_%d", ctlr->id);
>> +	ctlr->devloss_work_q = create_singlethread_workqueue(
>> +		ctlr->devloss_work_q_name);
>> +	if (!ctlr->devloss_work_q)
>> +		goto out_del_q;
>> +
>> +	dev_set_name(&ctlr->dev, "ctlr_%d", ctlr->id);
>> +	error = device_add(&ctlr->dev);
>> +	if (error)
>> +		goto out_del_q2;
>> +
>> +	error = sysfs_create_group(&ctlr->dev.kobj,
>> +				&fcoe_ctlr_attribute_group);
>> +	if (error)
>> +		goto out_del_dev;
>> +
>> +	error = sysfs_create_group(&ctlr->dev.kobj,
>> +				&fcoe_ctlr_lesb_attribute_group);
>> +	if (error)
>> +		goto out_del_dev;
> You just raced userspace by creating your attributes after device_add()
> causing lots of problems in the longrun.  Why not make these default
> attribute groups that the driver core automatically creates for you
> properly?  That also makes your error path simpler, as well as your
> cleanup path for when this device goes away.
Got it, will fix and re-post.

>> +	return ctlr;
>> +
>> +out_del_dev:
>> +	device_del(&ctlr->dev);
>> +out_del_q2:
>> +	destroy_workqueue(ctlr->devloss_work_q);
>> +	ctlr->devloss_work_q = NULL;
>> +out_del_q:
>> +	destroy_workqueue(ctlr->work_q);
>> +	ctlr->work_q = NULL;
>> +out_del:
>> +	put_device(parent);
>> +	kfree(ctlr);
>> +out:
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL(fcoe_ctlr_attrs_add);
> EXPORT_SYMBOL_GPL() for this, and other exports?
>> +/**
>> + * fcoe_fcf_attrs_add() - Add a FCoE sysfs fcoe_fcf_attrs to the system
>> + * @ctlr:    The fcoe_ctlr_attrs that will be the fcoe_fcf_attrs parent
>> + * @new_fcf: A temporary FCF used for lookups on the current list of fcfs
>> + *
>> + * Expects to be called with the ctlr->lock held
>> + */
>> +struct fcoe_fcf_attrs *fcoe_fcf_attrs_add(struct fcoe_ctlr_attrs *ctlr,
>> +					  struct fcoe_fcf_attrs *new_fcf)
>> +{
>> +	struct fcoe_fcf_attrs *fcf;
>> +	int error = 0;
>> +
>> +	list_for_each_entry(fcf,&ctlr->fcfs, peers) {
>> +		if (fcoe_fcf_attrs_match(new_fcf, fcf)) {
>> +			if (fcf->state == FCOE_FCF_STATE_CONNECTED)
>> +				return fcf;
>> +
>> +			fcf->state = FCOE_FCF_STATE_CONNECTED;
>> +
>> +			if (!cancel_delayed_work(&fcf->dev_loss_work))
>> +				fcoe_ctlr_attrs_flush_devloss(ctlr);
>> +
>> +			return fcf;
>> +		}
>> +	}
>> +
>> +	fcf = kzalloc(sizeof(struct fcoe_fcf_attrs), GFP_ATOMIC);
>> +	if (unlikely(!fcf))
>> +		goto out;
>> +
>> +	INIT_WORK(&fcf->delete_work, fcoe_fcf_attrs_final_delete);
>> +	INIT_DELAYED_WORK(&fcf->dev_loss_work, fip_timeout_deleted_fcf);
>> +
>> +	device_initialize(&fcf->dev);
>> +	fcf->dev.parent = get_device(&ctlr->dev);
> Same thing with the get_device() call here.
>
>> +	fcf->dev.class =&fcoe_fcf_class;
>> +	fcf->id = atomic_inc_return(&fcf_num) - 1;
> And the id.
>
>> +	fcf->state = FCOE_FCF_STATE_UNKNOWN;
>> +
>> +	fcf->dev_loss_tmo = ctlr->fcf_dev_loss_tmo;
>> +
>> +	dev_set_name(&fcf->dev, "fcf_%d", fcf->id);
>> +
>> +	fcf->fabric_name = new_fcf->fabric_name;
>> +	fcf->switch_name = new_fcf->switch_name;
>> +	fcf->fc_map = new_fcf->fc_map;
>> +	fcf->vfid = new_fcf->vfid;
>> +	memcpy(fcf->mac, new_fcf->mac, ETH_ALEN);
>> +	fcf->priority = new_fcf->priority;
>> +	fcf->fka_period = new_fcf->fka_period;
>> +	fcf->selected = new_fcf->selected;
>> +
>> +	error = device_add(&fcf->dev);
>> +	if (error)
>> +		goto out_del;
>> +
>> +	error = sysfs_create_group(&fcf->dev.kobj,
>> +				&fcoe_fcf_attribute_group);
>> +	if (error)
>> +		goto out_del_dev;
> Same thing here, you just raced userspace.
>
> thanks,
>
> greg k-h

Thanks for reviewing this Greg! I'll address your comments and re-post.

//Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-17  1:12     ` Love, Robert W
@ 2012-03-17  9:07       ` James Bottomley
  2012-03-20 21:01       ` Greg KH
  1 sibling, 0 replies; 11+ messages in thread
From: James Bottomley @ 2012-03-17  9:07 UTC (permalink / raw)
  To: Love, Robert W
  Cc: Greg KH, linux-scsi@vger.kernel.org, giridhar.malavali@qlogic.com,
	james.smart@emulex.com, bprakash@broadcom.com

On Sat, 2012-03-17 at 01:12 +0000, Love, Robert W wrote:
> >> +    ctlr->id = atomic_inc_return(&ctlr_num) - 1;
> > Does this work properly over the long run?  Shouldn't you use the
> idr
> > interface instead, to keep holes from showing up?
> 
> I'm not familiar wit the idr interface. I'll ask around and fix this.
> 
The rule of thumb we've been using for idr in SCSI is that we don't
bother with it unless the name space is constrained.  So for sd<x> we
use it because we have a limited device space to fill (constrained by
minor numbers) for target<n> we just use an incrementing counter because
<n> is unconstrained.

James



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-17  0:25   ` Greg KH
  2012-03-17  1:12     ` Love, Robert W
@ 2012-03-20  1:23     ` Love, Robert W
  2012-03-20 21:05       ` Greg KH
  1 sibling, 1 reply; 11+ messages in thread
From: Love, Robert W @ 2012-03-20  1:23 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-scsi@vger.kernel.org, giridhar.malavali@qlogic.com,
	james.smart@emulex.com, bprakash@broadcom.com

On 03/16/2012 05:25 PM, Greg KH wrote:
> On Fri, Mar 16, 2012 at 12:36:56PM -0700, Robert Love wrote:

<snip>

> diff --git a/Documentation/ABI/testing/sysfs-class-fcoe b/Documentation/ABI/testing/sysfs-class-fcoe
> new file mode 100644
> index 0000000..e9cd7e9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fcoe
> @@ -0,0 +1,77 @@
> +What:		/sys/class/fcoe_ctlr/ctlr_X
> We are really trying to stay away from new classes being created.  Why
> can't this be a bus and have the devices attach to that instead?  You
> can have one bus with both types of "devices" attached to it, making
> things a bit simpler in the end.

<snip>

> +	dev_set_name(&ctlr->dev, "ctlr_%d", ctlr->id);
> +	error = device_add(&ctlr->dev);
> +	if (error)
> +		goto out_del_q2;
> +
> +	error = sysfs_create_group(&ctlr->dev.kobj,
> +				&fcoe_ctlr_attribute_group);
> +	if (error)
> +		goto out_del_dev;
> +
> +	error = sysfs_create_group(&ctlr->dev.kobj,
> +				&fcoe_ctlr_lesb_attribute_group);
> +	if (error)
> +		goto out_del_dev;
> You just raced userspace by creating your attributes after device_add()
> causing lots of problems in the longrun.  Why not make these default
> attribute groups that the driver core automatically creates for you
> properly?  That also makes your error path simpler, as well as your
> cleanup path for when this device goes away.
>

Hi Greg. I have a local series that addresses most of the comments 
you've made. I have a question about the above two requests.

I've converted my series to create a 'fcoe bus' and now instances of the 
'fcoe_ctlr' and 'fcoe_fcf' are on the 'fcoe bus.' I did not create any 
drivers for the bus; I'm simply adding devices with their device pointer 
set to my 'fcoe bus' (dev->bus = &fcoe_bus_type) so that these devices 
are grouped under /sys/bus/fcoe/. This change results in the following, 
which I think is what you want.

[root@bubba ~]# ls -l /sys/bus/fcoe/drivers
total 0
[root@bubba ~]# ls -l /sys/bus/fcoe/devices/
total 0
lrwxrwxrwx 1 root root 0 Mar 19 18:20 ctlr_0 -> 
../../../devices/virtual/net/eth4.172-fcoe/ctlr_0
lrwxrwxrwx 1 root root 0 Mar 19 18:20 fcf_0 -> 
../../../devices/virtual/net/eth4.172-fcoe/ctlr_0/fcf_0

Is there a way for me to make default attribute groups for each of these 
device types, as you suggest, without having a 'fcoe_ctlr' driver and a 
'fcoe_fcf' driver, or does your suggestion imply that I should be 
creating drivers for these two types?

I think I'm limited to having default attributes for any device on the 
'fcoe bus,' which is not what I want because my fcoe_fcf and fcoe_ctlr 
devices do not share attributes. Or to create a drivers which have 
default attribute groups and therefore the core will create attributes 
as devices are matched with their drivers. Since I'm dealing with a 
'subsystem bus' and not a real bus, I'm not sure if it's appropriate for 
me to be creating virtual subsystem drivers...

Thanks, //Rob

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-17  1:12     ` Love, Robert W
  2012-03-17  9:07       ` James Bottomley
@ 2012-03-20 21:01       ` Greg KH
  1 sibling, 0 replies; 11+ messages in thread
From: Greg KH @ 2012-03-20 21:01 UTC (permalink / raw)
  To: Love, Robert W
  Cc: linux-scsi@vger.kernel.org, giridhar.malavali@qlogic.com,
	james.smart@emulex.com, bprakash@broadcom.com

On Sat, Mar 17, 2012 at 01:12:10AM +0000, Love, Robert W wrote:
> >> +	ctlr->f = f;
> >> +	INIT_LIST_HEAD(&ctlr->fcfs);
> >> +	mutex_init(&ctlr->lock);
> >> +	device_initialize(&ctlr->dev);
> >> +	ctlr->dev.parent = get_device(parent);
> > I thought you dropped these get_device() calls on the parent?  The
> > driver core handles this for you, please don't do it here, it's not
> > needed.
> 
> How would the driver core know what this device's parent is? This is the 
> only place I do that assignment.

I'm not saying that assigning the parent is wrong, I'm saying you don't
need to grab the reference to the parent here.

The code should just be:
	ctrl->dev.parent = parent;

When you register the device, the parent will be properly reference
counted.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/4] libfcoe: Add fcoe_sysfs
  2012-03-20  1:23     ` Love, Robert W
@ 2012-03-20 21:05       ` Greg KH
  0 siblings, 0 replies; 11+ messages in thread
From: Greg KH @ 2012-03-20 21:05 UTC (permalink / raw)
  To: Love, Robert W
  Cc: linux-scsi@vger.kernel.org, giridhar.malavali@qlogic.com,
	james.smart@emulex.com, bprakash@broadcom.com

On Tue, Mar 20, 2012 at 01:23:15AM +0000, Love, Robert W wrote:
> On 03/16/2012 05:25 PM, Greg KH wrote:
> > On Fri, Mar 16, 2012 at 12:36:56PM -0700, Robert Love wrote:
> 
> <snip>
> 
> > diff --git a/Documentation/ABI/testing/sysfs-class-fcoe b/Documentation/ABI/testing/sysfs-class-fcoe
> > new file mode 100644
> > index 0000000..e9cd7e9
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-fcoe
> > @@ -0,0 +1,77 @@
> > +What:		/sys/class/fcoe_ctlr/ctlr_X
> > We are really trying to stay away from new classes being created.  Why
> > can't this be a bus and have the devices attach to that instead?  You
> > can have one bus with both types of "devices" attached to it, making
> > things a bit simpler in the end.
> 
> <snip>
> 
> > +	dev_set_name(&ctlr->dev, "ctlr_%d", ctlr->id);
> > +	error = device_add(&ctlr->dev);
> > +	if (error)
> > +		goto out_del_q2;
> > +
> > +	error = sysfs_create_group(&ctlr->dev.kobj,
> > +				&fcoe_ctlr_attribute_group);
> > +	if (error)
> > +		goto out_del_dev;
> > +
> > +	error = sysfs_create_group(&ctlr->dev.kobj,
> > +				&fcoe_ctlr_lesb_attribute_group);
> > +	if (error)
> > +		goto out_del_dev;
> > You just raced userspace by creating your attributes after device_add()
> > causing lots of problems in the longrun.  Why not make these default
> > attribute groups that the driver core automatically creates for you
> > properly?  That also makes your error path simpler, as well as your
> > cleanup path for when this device goes away.
> >
> 
> Hi Greg. I have a local series that addresses most of the comments 
> you've made. I have a question about the above two requests.
> 
> I've converted my series to create a 'fcoe bus' and now instances of the 
> 'fcoe_ctlr' and 'fcoe_fcf' are on the 'fcoe bus.' I did not create any 
> drivers for the bus; I'm simply adding devices with their device pointer 
> set to my 'fcoe bus' (dev->bus = &fcoe_bus_type) so that these devices 
> are grouped under /sys/bus/fcoe/. This change results in the following, 
> which I think is what you want.
> 
> [root@bubba ~]# ls -l /sys/bus/fcoe/drivers
> total 0
> [root@bubba ~]# ls -l /sys/bus/fcoe/devices/
> total 0
> lrwxrwxrwx 1 root root 0 Mar 19 18:20 ctlr_0 -> 
> ../../../devices/virtual/net/eth4.172-fcoe/ctlr_0
> lrwxrwxrwx 1 root root 0 Mar 19 18:20 fcf_0 -> 
> ../../../devices/virtual/net/eth4.172-fcoe/ctlr_0/fcf_0

Looks good to me.

> Is there a way for me to make default attribute groups for each of these 
> device types, as you suggest, without having a 'fcoe_ctlr' driver and a 
> 'fcoe_fcf' driver, or does your suggestion imply that I should be 
> creating drivers for these two types?

Yes you can do this.  I think you just need to set a different type for
the device, but I can't recall at the moment.  Look at the sysdev code
in 3.3 for examples of how to set up "interface" types, I think that's
what you want to do here.  It's what we do in the USB code.

> I think I'm limited to having default attributes for any device on the 
> 'fcoe bus,' which is not what I want because my fcoe_fcf and fcoe_ctlr 
> devices do not share attributes. Or to create a drivers which have 
> default attribute groups and therefore the core will create attributes 
> as devices are matched with their drivers. Since I'm dealing with a 
> 'subsystem bus' and not a real bus, I'm not sure if it's appropriate for 
> me to be creating virtual subsystem drivers...

You can always, at registration time, dynamically determine which
default attributes you enable or not, based on the type of the device.
The driver core will callback to the attributes themselves, if you set
it up properly.  Use the is_visible() callback in the attribute group
for this (search the kernel for lots of usages of this for examples.)

Hope this helps,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-03-20 21:05 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-16 19:36 [PATCH v2 0/4] FCoE Sysfs Robert Love
2012-03-16 19:36 ` [PATCH v2 1/4] fcoe: Allocate fcoe_ctlr with fcoe_interface, not as a member Robert Love
2012-03-16 19:36 ` [PATCH v2 2/4] bnx2fc: Allocate fcoe_ctlr with bnx2fc_interface, " Robert Love
2012-03-16 19:36 ` [PATCH v2 3/4] libfcoe: Add fcoe_sysfs Robert Love
2012-03-17  0:25   ` Greg KH
2012-03-17  1:12     ` Love, Robert W
2012-03-17  9:07       ` James Bottomley
2012-03-20 21:01       ` Greg KH
2012-03-20  1:23     ` Love, Robert W
2012-03-20 21:05       ` Greg KH
2012-03-16 19:37 ` [PATCH v2 4/4] fcoe, bnx2fc, libfcoe: SW FCoE and bnx2fc use FCoE Syfs Robert Love

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox