Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
On Tue, 2008-05-27 at 07:38 -0700, Roland Dreier wrote: 'bnx2id' is the user component in this solution. bnx2id daemon uses socket calls to bind tcp ports in high range and hands them over to driver. This is how iscsi driver tries to solve tcp port collision issue. User daemon communicates with the driver using sysfs and tcp port related functions are bnx2i_read_tcp_portd_*/bnx2i_write_tcp_portd_* (reference: bnx2i_sysfs.c) So you are creating sockets just to reserve TCP ports to avoid host stack clashes with your offload engine? Wasn't this approach strongly rejected (in the context of iWARP) in the past? We're doing it in userspace, so I don't if that makes it any better or any worse. Roland, what do you suggest? We can do it like cma_alloc_any_port() in cma.c. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
On Tue, 2008-05-27 at 12:52 -0700, David Miller wrote: From: Roland Dreier [EMAIL PROTECTED] Date: Tue, 27 May 2008 07:38:19 -0700 So you are creating sockets just to reserve TCP ports to avoid host stack clashes with your offload engine? Wasn't this approach strongly rejected (in the context of iWARP) in the past? Yes, it was, and likewise similar hacks in other areas will be rejected similarly. If we change the implementation to use a separate IP address and separate MAC address for iSCSI, will it be acceptable? The iSCSI IP/MAC addresses will be unknown to the Linux TCP stack and so no sharing of the 4-tuple space will be needed. The patches will be very similar, except that all inet calls and notifiers will be removed. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
On Fri, 2008-05-23 at 13:23 -0700, Roland Dreier wrote:
Hi Michael, I was reading over the driver to try and figure out how you
handle allocating source ports for the offloaded TCP connections you
make so that they don't collide with the main network stack. It looks
like you have:
+/**
+ * bnx2i_alloc_tcp_port - allocates a tcp port from the free list
+ *
+ * Assumes this function is called with 'bnx2i_resc_lock' held.
+ */
+static u16 bnx2i_alloc_tcp_port(void)
that has some failure code:
+ if (!tcp_port) {
+ printk(KERN_ERR bnx2i: run 'bnx2id' to alloc tcp ports\n);
but I don't know what bnx2id is?
'bnx2id' is the user component in this solution. bnx2id daemon uses
socket calls to bind tcp ports in high range and hands them over to
driver. This is how iscsi driver tries to solve tcp port collision
issue. User daemon communicates with the driver using sysfs and tcp port
related functions are bnx2i_read_tcp_portd_*/bnx2i_write_tcp_portd_*
(reference: bnx2i_sysfs.c)
and I didn't see anywhere that bnx2i_get_tcp_port_requirements() is
actually called, and it's not exported?
+/**
+ * bnx2i_get_tcp_port_requirements - returns num tcp ports to alloc/bind
+ *
+ * driver returns the number of TCP ports to be allocated/bound by
'bnx2id'
+ *daemon. Return value of '0' means driver has everything to
support
+ *max iscsi connections on enumerated NX2 devices
+ */
+int bnx2i_get_tcp_port_requirements(void)
Actually this logic was simplified by adding a new member,
'num_required' to 'tcp_port_mgmt' structure. This call is no more
required, will remove bnx2i_get_tcp_port_requirements() in the next
driver revision
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
On Thu, 2008-05-22 at 11:15 -0400, Konrad Rzeszutek wrote:
A cursory glance..
.. snip..
+struct bnx2i_cleanup_request {
+#if defined(__BIG_ENDIAN)
+ u8 op_code;
+ u8 reserved1;
+ u16 reserved0;
+#elif defined(__LITTLE_ENDIAN)
+ u16 reserved0;
+ u8 reserved1;
+ u8 op_code;
+#endif
+ u32 reserved2[3];
+#if defined(__BIG_ENDIAN)
+ u16 reserved3;
+ u16 itt;
+#define ISCSI_CLEANUP_REQUEST_INDEX (0x3FFF0)
+#define ISCSI_CLEANUP_REQUEST_INDEX_SHIFT 0
+#define ISCSI_CLEANUP_REQUEST_TYPE (0x314)
+#define ISCSI_CLEANUP_REQUEST_TYPE_SHIFT 14
+#elif defined(__LITTLE_ENDIAN)
+ u16 itt;
+#define ISCSI_CLEANUP_REQUEST_INDEX (0x3FFF0)
+#define ISCSI_CLEANUP_REQUEST_INDEX_SHIFT 0
+#define ISCSI_CLEANUP_REQUEST_TYPE (0x314)
+#define ISCSI_CLEANUP_REQUEST_TYPE_SHIFT 14
+ u16 reserved3;
+#endif
Why the duplication of the #define values? They look the same.
Sure we can take care of that by moving macros outside of #ifdef
.. snip ..
+/*
+ * mnc - lookup Jeff Garzack's rule about defining this
Has this TODO been completed?
+ * type of junk. Base on enum and make it less error prone.
+ * Anil - these are bit masks, won't enum be little ugly?
+ */
.. snip ..
+ * bnx2i_iscsi_license_error - displays iscsi license related error message
Doesn't look very license related. Just says 'not supported'.
+ * @hba: adapter instance pointer
+ * @error_code:error classification
+ *
+ * Puts out an error log when driver is unable to offload iscsi connection
+ * due to license restrictions
Maybe adding in some extra information to the error, such as: Due to
GPL restrictions, etc.. .. What does 'LOM' stand for?
This has nothing to do with code licensing but related to whether iSCSI
functionality is enabled on the device or not. iSCSI feature can be
controlled through NVRAM configuration and some OEM's do not enable
iscsi
+ */
+static void bnx2i_iscsi_license_error(struct bnx2i_hba *hba, u32
error_code)
+{
+ if (error_code == ISCSI_KCQE_COMPLETION_STATUS_ISCSI_NOT_SUPPORTED)
+ /* iSCSI offload not supported on this device */
+ printk(KERN_ERR bnx2i: iSCSI not supported, dev=%s\n,
+ hba-netdev-name);
+ if (error_code == ISCSI_KCQE_COMPLETION_STATUS_LOM_ISCSI_NOT_ENABLED)
+ /* iSCSI offload not supported on this LOM device */
+ printk(KERN_ERR bnx2i: LOM is not enable to
-enabled.
+ offload iSCSI connections, dev=%s\n,
+ hba-netdev-name);
+ set_bit(ADAPTER_STATE_INIT_FAILED, hba-adapter_state);
+}
+
+#ifdef _EVENT_COALESCE_
+#define CNIC_ARM_CQE 1
+#define CNIC_DISARM_CQE0
+extern unsigned int event_coal_div;
+
+/**
+ * bnx2i_arm_cq_event_coalescing - arms CQ to enable EQ notification
+ * @ep:endpoint (transport indentifier) structure
+ * @action:action, ARM or DISARM. For now only ARM_CQE is used
+ *
+ * Arm'ing CQ will enable chip to generate global EQ events inorder to
interrupt
+ * the driver. EQ event is generated CQ index is hit or at least 1 CQ is
+ * outstanding and on chip timer expires
+ */
+static void bnx2i_arm_cq_event_coalescing(struct bnx2i_endpoint *ep, u8
action)
+{
+ u16 cq_index;
+ if ((action == CNIC_ARM_CQE) ep-sess) {
+ cq_index = ep-qp.cqe_exp_seq_sn +
+ ep-sess-num_active_cmds / event_coal_div;
+ cq_index %= (ep-qp.cqe_size * 2 + 1);
+ if (!cq_index)
+ cq_index = 1;
+ writew(cq_index, ep-qp.ctx_base + CNIC_EVENT_COAL_INDEX);
+ }
+ writeb(action, ep-qp.ctx_base + CNIC_EVENT_CQ_ARM);
This code looks to be outside the function. Did you try to compile with the
_EVENT_COALESCE_ define?
function construction is ok, may be an additional blank line after
writeb() the reason for confusion
.. snip ..
+int bnx2i_send_iscsi_tmf(struct bnx2i_conn *bnx2i_conn,
+struct iscsi_task *mtask)
.. snip
+ tmfabort_wqe-op_attr = 0;
+ tmfabort_wqe-op_attr =
+ ISCSI_TMF_REQUEST_ALWAYS_ONE | ISCSI_TM_FUNC_ABORT_TASK;
Is the first assigment neccessary?
.. snip ..
+static int bnx2i_power_of2(u32 val)
There are no macros for this?
Did not find one. But there is is_power_of_2() to check if the given
integer is an exact power of 2
.. snip ..
+static void bnx2i_process_async_mesg(struct iscsi_session *session,
+struct bnx2i_conn *bnx2i_conn,
+struct cqe *cqe)
+{
+ struct bnx2i_async_msg *async_cqe;
+ struct iscsi_async *resp_hdr;
+ u8 async_event;
+
+ bnx2i_unsol_pdu_adjust_rq(bnx2i_conn);
+
+ async_cqe = (struct bnx2i_async_msg
Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
On Thu, 2008-05-22 at 17:15 -0400, Christoph Hellwig wrote:
+struct bnx2i_async_msg {
+#if defined(__BIG_ENDIAN)
+ u8 op_code;
+ u8 reserved1;
+ u16 reserved0;
+#elif defined(__LITTLE_ENDIAN)
+ u16 reserved0;
+ u8 reserved1;
+ u8 op_code;
+#endif
+ u32 reserved2;
+ u32 exp_cmd_sn;
+ u32 max_cmd_sn;
+ u32 reserved3[2];
Please don't do the ifdef big endian mess. Just read the whole
32bit word and do mask and shift operations to extract the actual value.
I agree with you that u32 is cleaner in some cases. We can just change
it to u32 op_code and we just need a OP_CODE_MASK of 0xff.
We'll go through this .h file and remove some of this big endian stuff
and also the duplicate constants pointed out earlier.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
Konrad Rzeszutek wrote: + * bnx2i_iscsi_license_error - displays iscsi license related error message Doesn't look very license related. Just says 'not supported'. Could be that some hardware strap pin is being used as a dongle. This doesn't seem entirely in the spirit of GPL, and would be rather easy to remove... + * @hba: adapter instance pointer + * @error_code:error classification + * + * Puts out an error log when driver is unable to offload iscsi connection + * due to license restrictions Maybe adding in some extra information to the error, such as: Due to GPL restrictions, etc.. .. What does 'LOM' stand for? LOM is a common abbreviation for LAN on motherboard as opposed to an expansion card. Ben. -- Ben Hutchings, Senior Software Engineer, Solarflare Communications Not speaking for my employer; that's the marketing department's job. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups open-iscsi group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [PATCH 3/3] bnx2i: Add bnx2i iSCSI driver.
+struct bnx2i_async_msg {
+#if defined(__BIG_ENDIAN)
+ u8 op_code;
+ u8 reserved1;
+ u16 reserved0;
+#elif defined(__LITTLE_ENDIAN)
+ u16 reserved0;
+ u8 reserved1;
+ u8 op_code;
+#endif
+ u32 reserved2;
+ u32 exp_cmd_sn;
+ u32 max_cmd_sn;
+ u32 reserved3[2];
Please don't do the ifdef big endian mess. Just read the whole
32bit word and do mask and shift operations to extract the actual value.
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---
