Re: [PATCH 4/4] bnx2i: Add bnx2i iSCSI driver.

Previous thread: [PATCH 2/4] cnic: Add new CNIC driver. by Michael Chan on Friday, May 1, 2009 - 1:00 pm. (1 message)

Next thread: [PATCH 3/4] iscsi class, libiscsi: Add net config. by Michael Chan on Friday, May 1, 2009 - 1:00 pm. (12 messages)
From: Michael Chan
Date: Friday, May 1, 2009 - 1:00 pm

New iSCSI driver for BNX2 devices.  The driver interfaces with the
CNIC driver to access the hardware.

Signed-off-by: Anil Veerabhadrappa <anilgv@broadcom.com>
Signed-off-by: Michael Chan <mchan@broadcom.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
---
 drivers/scsi/Kconfig                      |    1 +
 drivers/scsi/Makefile                     |    1 +
 drivers/scsi/bnx2i/57xx_iscsi_constants.h |  155 ++
 drivers/scsi/bnx2i/57xx_iscsi_hsi.h       | 1509 ++++++++++++++++++
 drivers/scsi/bnx2i/Kconfig                |    6 +
 drivers/scsi/bnx2i/Makefile               |    3 +
 drivers/scsi/bnx2i/bnx2i.h                |  776 +++++++++
 drivers/scsi/bnx2i/bnx2i_hwi.c            | 2446 +++++++++++++++++++++++++++++
 drivers/scsi/bnx2i/bnx2i_init.c           |  434 +++++
 drivers/scsi/bnx2i/bnx2i_iscsi.c          | 2101 +++++++++++++++++++++++++
 drivers/scsi/bnx2i/bnx2i_sysfs.c          |  142 ++
 11 files changed, 7574 insertions(+), 0 deletions(-)
 create mode 100644 drivers/scsi/bnx2i/57xx_iscsi_constants.h
 create mode 100644 drivers/scsi/bnx2i/57xx_iscsi_hsi.h
 create mode 100644 drivers/scsi/bnx2i/Kconfig
 create mode 100644 drivers/scsi/bnx2i/Makefile
 create mode 100644 drivers/scsi/bnx2i/bnx2i.h
 create mode 100644 drivers/scsi/bnx2i/bnx2i_hwi.c
 create mode 100644 drivers/scsi/bnx2i/bnx2i_init.c
 create mode 100644 drivers/scsi/bnx2i/bnx2i_iscsi.c
 create mode 100644 drivers/scsi/bnx2i/bnx2i_sysfs.c

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 8ed2990..8427b77 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1374,6 +1374,7 @@ config SCSI_QLOGICPTI
 
 source "drivers/scsi/qla2xxx/Kconfig"
 source "drivers/scsi/qla4xxx/Kconfig"
+source "drivers/scsi/bnx2i/Kconfig"
 
 config SCSI_LPFC
 	tristate "Emulex LightPulse Fibre Channel Support"
diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index e7c861a..919157d 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -84,6 +84,7 @@ ...
From: Mike Christie
Date: Wednesday, May 6, 2009 - 9:48 am

I think I was wrong with one of the comments I gave you.

It seems like we have two iscsi net config models.

1. qla4xxx and Server Engines type of setup where the driver just tells 
the card to use some ip or do dhcp and some other net settings and it 
does all the net magic. The iscsi driver does not have to worry about 
anything like the dhcp process or arp. It only passes down the setup values.

2. cxgb3i and bnx2i type of model where kernel or userspace code is 
needed to execute many net operations.
- Right now, cxgb3i sort of cheated :) and only supports static IPs. It 
currently uses the iscsi set param interface to do this.

- bnx2i wants to add more complicated features and is going to do them 
in userspace. It us using the private messages that were added in the 
previous patch.


I think cxgb3i is one day going to want to support the same features 
bnx2i does. If that is right, then should we just make the NX2_UIO 
events common iscsi events, and hook cxb3i in? It would not use the 
iscsi set param interface at all and would work just like bnx2i. Is that 
possible? What about future drivers? Are done making iscsi cards and 
drivers. If so, thank goodness :)  If not then maybe we want to consider 
some future driver using the #2 module and possibly using this.

If cxgb3i is really only going to support static ip setup and we think 
that bnx2i is going to be unique on how it sets up the network then I 
NX2_UIO private events are fine. Or is this a case of we are thinking 
that iscsi hardware people are creating crazy interfaces so there is no 
why to predict what they are going to do so there is no point in trying 
to design for them.
--

From: Michael Chan
Date: Thursday, May 7, 2009 - 10:03 am

If there is any possibility that cxgb3i will use something similar to
bnx2i, I think we can change the message to a standard one and make the
message structure somewhat more generic.  We'll probably still need a
private area in the message for hardware or vendor specific information.


--

From: Mike Christie
Date: Thursday, May 7, 2009 - 2:01 pm

Ok sounds good to me.
--

From: Michael Chan
Date: Monday, May 18, 2009 - 6:50 pm

Here are the more generic NETLINK_ISCSI messages and the iscsi transport
code to support them, please review.

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index d69a53a..60cb6cb 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -995,6 +995,39 @@ int iscsi_recv_pdu(struct iscsi_cls_conn *conn, struct iscsi_hdr *hdr,
 }
 EXPORT_SYMBOL_GPL(iscsi_recv_pdu);
 
+int iscsi_offload_mesg(struct Scsi_Host *shost,
+		       struct iscsi_transport *transport, uint32_t type,
+		       char *data, uint16_t data_size)
+{
+	struct nlmsghdr	*nlh;
+	struct sk_buff *skb;
+	struct iscsi_uevent *ev;
+	int len = NLMSG_SPACE(sizeof(*ev) + data_size);
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (!skb) {
+		printk(KERN_ERR "can not deliver iscsi offload message:OOM\n");
+		return -ENOMEM;
+	}
+
+	nlh = __nlmsg_put(skb, 0, 0, 0, (len - sizeof(*nlh)), 0);
+	ev = NLMSG_DATA(nlh);
+	memset(ev, 0, sizeof(*ev));
+	ev->type = type;
+	ev->transport_handle = iscsi_handle(transport);
+	switch (type) {
+	case ISCSI_KEVENT_PATH_REQ:
+		ev->r.req_path.host_no = shost->host_no;
+	case ISCSI_KEVENT_IF_DOWN:
+		ev->r.notify_if_down.host_no = shost->host_no;
+	}
+
+	memcpy((char*)ev + sizeof(*ev), data, data_size);
+
+	return iscsi_broadcast_skb(skb, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(iscsi_offload_mesg);
+
 void iscsi_conn_error_event(struct iscsi_cls_conn *conn, enum iscsi_err error)
 {
 	struct nlmsghdr	*nlh;
@@ -1393,6 +1426,30 @@ iscsi_set_host_param(struct iscsi_transport *transport,
 }
 
 static int
+iscsi_set_path(struct iscsi_transport *transport, struct iscsi_uevent *ev)
+{
+	struct Scsi_Host *shost;
+	struct iscsi_path *params;
+	int err;
+
+	if (!transport->set_path)
+		return -ENOSYS;
+
+	shost = scsi_host_lookup(ev->u.set_path.host_no);
+	if (!shost) {
+		printk(KERN_ERR "set path could not find host no %u\n",
+		       ev->u.set_path.host_no);
+		return -ENODEV;
+	}
+
+	params = ...
From: Mike Christie
Date: Tuesday, May 19, 2009 - 7:22 am

You can sync up what the gfp flag used here and for the alloc_skb call 
above. If you have process context, you probably want to use GFP_NOIO, 
because this could be called for reconnect for a disk in use.


Instead of using broadcast above and in some other places and then doing 
this check, could we just use multicast groups or something else? The 
events from iscsid could be in one group and then events for uip would 
be in another?

Or is it more common to do it like this or will it break compat with 
other tools if we change it?
--

From: Michael Chan
Date: Tuesday, May 19, 2009 - 1:47 pm

We have process context, but I think we should make it more general for

We need to do this check because we don't want the daemon_pid to be
overwritten with a pid that is not iscsid's.  If it was overwritten,
unicast NETLINK_ISCSI messages will not reach iscsid.

We can use multicast group 2 for the new messages if you prefer.  This
way, I think iscsid will not receive the new messages since it is only


--

From: Mike Christie
Date: Tuesday, May 19, 2009 - 2:58 pm

If you have process context just use GFP_NOIO. We can change it later 
when/if a driver needs it. I think we like to avoid GFP_ATOMIC if we 
can. Or just add a gfp_t argument to the function so the caller can do 

ok.
--

From: Michael Chan
Date: Wednesday, May 20, 2009 - 9:58 am

Here's the new patch.  The multicast change is from Mike Christie.

diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index d69a53a..9bd337a 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -37,7 +37,6 @@
 #define ISCSI_TRANSPORT_VERSION "2.0-870"
 
 struct iscsi_internal {
-	int daemon_pid;
 	struct scsi_transport_template t;
 	struct iscsi_transport *iscsi_transport;
 	struct list_head list;
@@ -938,23 +937,9 @@ iscsi_if_transport_lookup(struct iscsi_transport *tt)
 }
 
 static int
-iscsi_broadcast_skb(struct sk_buff *skb, gfp_t gfp)
+iscsi_multicast_skb(struct sk_buff *skb, u32 group, gfp_t gfp)
 {
-	return netlink_broadcast(nls, skb, 0, 1, gfp);
-}
-
-static int
-iscsi_unicast_skb(struct sk_buff *skb, int pid)
-{
-	int rc;
-
-	rc = netlink_unicast(nls, skb, pid, MSG_DONTWAIT);
-	if (rc < 0) {
-		printk(KERN_ERR "iscsi: can not unicast skb (%d)\n", rc);
-		return rc;
-	}
-
-	return 0;
+	return nlmsg_multicast(nls, skb, 0, group, gfp);
 }
 
 int iscsi_recv_pdu(struct iscsi_cls_conn *conn, struct iscsi_hdr *hdr,
@@ -980,7 +965,7 @@ int iscsi_recv_pdu(struct iscsi_cls_conn *conn, struct iscsi_hdr *hdr,
 		return -ENOMEM;
 	}
 
-	nlh = __nlmsg_put(skb, priv->daemon_pid, 0, 0, (len - sizeof(*nlh)), 0);
+	nlh = __nlmsg_put(skb, 0, 0, 0, (len - sizeof(*nlh)), 0);
 	ev = NLMSG_DATA(nlh);
 	memset(ev, 0, sizeof(*ev));
 	ev->transport_handle = iscsi_handle(conn->transport);
@@ -991,10 +976,45 @@ int iscsi_recv_pdu(struct iscsi_cls_conn *conn, struct iscsi_hdr *hdr,
 	memcpy(pdu, hdr, sizeof(struct iscsi_hdr));
 	memcpy(pdu + sizeof(struct iscsi_hdr), data, data_size);
 
-	return iscsi_unicast_skb(skb, priv->daemon_pid);
+	return iscsi_multicast_skb(skb, ISCSI_NL_GRP_ISCSID, GFP_ATOMIC);
 }
 EXPORT_SYMBOL_GPL(iscsi_recv_pdu);
 
+int iscsi_offload_mesg(struct Scsi_Host *shost,
+		       struct iscsi_transport *transport, uint32_t type,
+		       char *data, uint16_t ...
From: Mike Christie
Date: Wednesday, May 20, 2009 - 12:57 pm

I think we have to include in6.h and in.h in this file, right? iser 
would not compile with this patch without it. If that is right and it 
works for you now, I can just fix it when I merge the patch.
--

From: Michael Chan
Date: Wednesday, May 20, 2009 - 3:22 pm

Yes, either include these files here in iscsi_if.h or include them in
places where iscsi_if.h is included.  Which way do you prefer?


--

From: Mike Christie
Date: Wednesday, May 20, 2009 - 3:51 pm

Here is fine.
--

Previous thread: [PATCH 2/4] cnic: Add new CNIC driver. by Michael Chan on Friday, May 1, 2009 - 1:00 pm. (1 message)

Next thread: [PATCH 3/4] iscsi class, libiscsi: Add net config. by Michael Chan on Friday, May 1, 2009 - 1:00 pm. (12 messages)