Re: [PATCH 0/9] ocrdma: Driver for Emulex OneConnect RDMA
Overall looks pretty good... some comments on individual patches coming. - R. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+#define ocrdma_err(format, arg...) printk(KERN_ERR format, ##arg)
I think you'd be better off using pr_err() rather than defining your own macro.
+struct ocrdma_cq {
+ struct ib_cq ibcq;
+ struct ocrdma_dev *dev;
+ struct ocrdma_cqe *va;
+ u32 phase;
+ u32 getp; /* pointer to pending wrs to
+ * return to stack, wrap arounds
+ * at max_hw_cqe
+ */
+ u32 max_hw_cqe;
+ bool phase_change;
+ bool armed, solicited;
+ bool arm_needed;
+
+ spinlock_t cq_lock cacheline_aligned; /* provide synchronization
+ * to cq polling
+ */
+ /* syncronizes cq completion handler invoked from multiple context */
+ spinlock_t comp_handler_lock cacheline_aligned;
You have quite a few of these alignment directives in the middle of structures.
Have you measured that leaving all these gaps gives a reall performance boost?
+ u16 id;
+ u16 eqn;
+
+ struct ocrdma_ucontext *ucontext;
+ dma_addr_t pa;
+ u32 len;
+ atomic_t use_cnt;
+
+ /* head of all qp's sq and rq for which cqes need to be flushed
+ * by the software.
+ */
+ struct list_head sq_head, rq_head;
+};
+#define OCRDMA_GET_NUM_POSTED_SHIFT_VAL(qp) \
+ (((qp-dev-nic_info.dev_family == OCRDMA_GEN2_FAMILY) \
+ (qp-id 64)) ? 24 : 16)
In general it's better to use inline functions when possible
instead of macros, which are less type-safe and harder to read.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Tue, Mar 20, 2012 at 3:39 PM, parav.pan...@emulex.com wrote:
From: Parav Pandit parav.pan...@emulex.com
- Header file for userspace library and kernel driver interface.
+struct ocrdma_alloc_ucontext_resp {
+ u32 dev_id;
+ u32 wqe_size;
+ u32 max_inline_data;
+ u32 dpp_wqe_size;
+ u64 ah_tbl_page;
+ u32 ah_tbl_len;
+ u32 rsvd;
+ u8 fw_ver[32];
+ u32 rqe_size;
+ u64 rsvd1;
+} __packed;
If I'm reading this correctly, you have the 8-byte rsvd1 member at an
offset only aligned to 4 bytes, because of the __packed directive. It
would be much better to have these structures laid out so they are
naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
__packed directive, which wrecks gcc code generation in some cases.
In this particular case, it seems you could just move rqe_size into the
slot where rsvd is, and get rid of rsvd1?
+/* user kernel communication data structures. */
+struct ocrdma_alloc_pd_ureq {
+ u64 rsvd1;
+} __packed;
Similar comment -- __packed is silly for a structure with one reserved
member (and which you don't seem to use anywhere)... why not just
delete this struct?
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+/* mailbox cmd response */
+struct ocrdma_mbx_rsp {
+ u32 subsys_op;
+ u32 status;
+ u32 rsp_len;
+ u32 add_rsp_len;
+} __packed;
...similar comments about only using __packed where you really need it...
+#define is_cqe_valid(cq, cqe) \
+ (((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_VALID)\
+ == cq-phase) ? 1 : 0)
+#define is_cqe_for_sq(cqe) \
+ ((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_QTYPE) ? 0 : 1)
+#define is_cqe_for_rq(cqe) \
+ ((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_QTYPE) ? 1 : 0)
+#define is_cqe_invalidated(cqe) \
+ ((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_INVALIDATE) ? \
+ 1 : 0)
+#define is_cqe_imm(cqe) \
+ ((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_IMM) ? 1 : 0)
+#define is_cqe_wr_imm(cqe) \
+ ((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_WRITE_IMM) ? 1 :
0)
...similar comment about using readable typesafe inline functions
instead of macros...
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
- Header file for userspace library and kernel driver interface.
+struct ocrdma_alloc_ucontext_resp {
+ u32 dev_id;
+ u32 wqe_size;
+ u32 max_inline_data;
+ u32 dpp_wqe_size;
+ u64 ah_tbl_page;
+ u32 ah_tbl_len;
+ u32 rsvd;
+ u8 fw_ver[32];
+ u32 rqe_size;
+ u64 rsvd1;
+} __packed;
If I'm reading this correctly, you have the 8-byte rsvd1 member at an
offset only aligned to 4 bytes, because of the __packed directive. It
would be much better to have these structures laid out so they are
naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
__packed directive, which wrecks gcc code generation in some cases.
gcc also supports defining types that have non-standard alignment
constraints that can be used to force the same alignment for
64bit fields between i386 and amd64.
Probably __attribute__((aligned,n)) or similar.
This can be used to force 32bit alignment in amd64 code in order
to match definitions in 32bit userspace.
For new things it would make sense to force 64bit alignment
of 64bit fields for 32bit code.
Adding __packed (rather than 32bit alignment) forces the compiler
to generate byte by byte accesses for all the fields on systems
that can't do misaligned accesses in hardware (eg sparc).
David
David
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+int ocrdma_qp_state_machine(struct ocrdma_qp *qp, enum ib_qp_state
new_ib_state,
+ enum ib_qp_state *old_ib_state)
+{
+ unsigned long flags;
+ int status = 0;
+ enum ocrdma_qp_state new_state;
+ new_state = get_ocrdma_qp_state(new_ib_state);
+
+ /* sync with wqe and rqe posting */
+ spin_lock_irqsave(qp-q_lock, flags);
+
+ if (old_ib_state)
+ *old_ib_state = get_ibqp_state(qp-state);
+ if (new_state == qp-state) {
+ spin_unlock_irqrestore(qp-q_lock, flags);
+ return 1;
+ }
+
+ switch (qp-state) {
+ case OCRDMA_QPS_RST:
+ switch (new_state) {
+ case OCRDMA_QPS_RST:
+ case OCRDMA_QPS_INIT:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_INIT:
+ /* qps: INIT-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_INIT:
+ case OCRDMA_QPS_RTR:
+ break;
+ case OCRDMA_QPS_ERR:
+ ocrdma_flush_qp(qp);
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_RTR:
+ /* qps: RTS-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_RTS:
+ break;
+ case OCRDMA_QPS_ERR:
+ ocrdma_flush_qp(qp);
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_RTS:
+ /* qps: RTS-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_SQD:
+ case OCRDMA_QPS_SQE:
+ break;
+ case OCRDMA_QPS_ERR:
+ ocrdma_flush_qp(qp);
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_SQD:
+ /* qps: SQD-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_RTS:
+ case OCRDMA_QPS_SQE:
+ case OCRDMA_QPS_ERR:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_SQE:
+ switch (new_state) {
+ case OCRDMA_QPS_RTS:
+ case OCRDMA_QPS_ERR:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_ERR:
+ /* qps: ERR-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_RST:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ if (!status)
+ qp-state = new_state;
+
+ spin_unlock_irqrestore(qp-q_lock, flags);
+ return status;
+}
The switch statement here seems to largely reimpliment ib_modify_qp_is_ok()
(which is exported from the rdma midlayer). Is there some reason that doesn't
work for your driver? I'd rather fix / generalize the core helper
function instead
of having something mostly duplicate in a hardware driver.
RE: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+#define is_cqe_wr_imm(cqe) \ + ((le32_to_cpu(cqe-flags_status_srcqpn) OCRDMA_CQE_WRITE_IMM) ? 1 : 0) ...similar comment about using readable typesafe inline functions instead of macros... and if you are using #defines, you need to enclose every reference to the parameters in (). David -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Tue, Mar 20, 2012 at 3:39 PM, parav.pan...@emulex.com wrote:
+struct ocrdma_queue_info {
+ void *va;
+ dma_addr_t dma;
+ u32 size;
+ u16 len;
+ u16 entry_size; /* Size of an element in the queue */
+ u16 id; /* qid, where to ring the doorbell. */
+ u16 head, tail;
+ bool created;
+ atomic_t used; /* Number of valid elements in the queue */
+};
Forgot to mention this before... the only place the used member is touched
that I can find is
+static inline void ocrdma_mq_inc_head(struct ocrdma_dev *dev)
+{
+ dev-mq.sq.head = (dev-mq.sq.head + 1) (OCRDMA_MQ_LEN - 1);
+ atomic_inc(dev-mq.sq.used);
+}
but I don't see anywhere that it gets read. Can used be deleted?
- R.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+struct ocrdma_cq {
+ struct ib_cq ibcq;
+ struct ocrdma_dev *dev;
nit: There are several structures where you store ocrdma_dev *. You can remove
these and use the struct ib_* to reach it as well.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+struct ocrdma_alloc_ucontext_resp {
+ u32 dev_id;
+ u32 wqe_size;
+ u32 max_inline_data;
+ u32 dpp_wqe_size;
+ u64 ah_tbl_page;
+ u32 ah_tbl_len;
+ u32 rsvd;
+ u8 fw_ver[32];
+ u32 rqe_size;
+ u64 rsvd1;
+} __packed;
Is there some reason to pack the structures in this file, rather than just
defining things to align on a 64-bit boundary? The structure above actually
ends up sized to a 4-byte boundary, rather than 8-byte, with rsvd1 field
misaligned.
+/* user kernel communication data structures. */
+struct ocrdma_alloc_pd_ureq {
+ u64 rsvd1;
+} __packed;
+
+struct ocrdma_alloc_pd_uresp {
+ u32 id;
+ u32 dpp_enabled;
+ u32 dpp_page_addr_hi;
+ u32 dpp_page_addr_lo;
+ u64 rsvd1;
+} __packed;
+
+struct ocrdma_create_cq_ureq {
+ u32 dpp_cq;
+ u32 rsvd;
+} __packed;
+
+#define MAX_CQ_PAGES 8
+struct ocrdma_create_cq_uresp {
+ u32 cq_id;
+ u32 page_size;
+ u32 num_pages;
+ u32 max_hw_cqe;
+ u64 page_addr[MAX_CQ_PAGES];
+ u64 db_page_addr;
+ u32 db_page_size;
+ u32 phase_change;
+ u64 rsvd1;
+ u64 rsvd2;
+} __packed;
Why the extra reserved space?
+
+#define MAX_QP_PAGES 8
+#define MAX_UD_AV_PAGES 8
+
+struct ocrdma_create_qp_ureq {
+ u8 enable_dpp_cq;
+ u8 rsvd;
+ u16 dpp_cq_id;
+ u32 rsvd1;
+};
+
+struct ocrdma_create_qp_uresp {
+ u16 qp_id;
+ u16 sq_dbid;
+ u16 rq_dbid;
+ u16 resv0;
+ u32 sq_page_size;
+ u32 rq_page_size;
+ u32 num_sq_pages;
+ u32 num_rq_pages;
+ u64 sq_page_addr[MAX_QP_PAGES];
+ u64 rq_page_addr[MAX_QP_PAGES];
+ u64 db_page_addr;
+ u32 db_page_size;
+ u32 dpp_credit;
+ u32 dpp_offset;
+ u32 rsvd1;
+ u32 num_wqe_allocated;
+ u32 num_rqe_allocated;
+ u32 free_wqe_delta;
+ u32 free_rqe_delta;
+ u32 db_sq_offset;
+ u32 db_rq_offset;
+ u32 db_shift;
+ u64 rsvd2;
+ u64 rsvd3;
+} __packed;
Why the extra reserved space? rsvd2 3 are also misaligned.
+
+struct ocrdma_create_srq_uresp {
+ u16 rq_dbid;
+ u16 resv0;
+ u32 resv1;
+
+ u32 rq_page_size;
+ u32 num_rq_pages;
+
+ u64 rq_page_addr[MAX_QP_PAGES];
+ u64 db_page_addr;
+
+ u32 db_page_size;
+ u32 num_rqe_allocated;
+ u32 db_rq_offset;
+ u32 db_shift;
+
+ u32 free_rqe_delta;
+ u32 rsvd2;
+ u64 rsvd3;
+} __packed;
Why the extra reserved space?
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
+struct ib_mr *ocrdma_get_dma_mr(struct ib_pd *ibpd, int acc)
+{
+ struct ocrdma_mr *mr;
+
+ mr = ocrdma_alloc_lkey(ibpd, acc, 0, OCRDMA_ADDR_CHECK_DISABLE);
+ if (!mr)
+ return ERR_PTR(-ENOMEM);
ocrdma_alloc_lkey does not return NULL on error.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Wed, Mar 21, 2012 at 10:42 AM, frank zago
fz...@systemfabricworks.com wrote:
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
+struct ib_mr *ocrdma_get_dma_mr(struct ib_pd *ibpd, int acc)
+{
+ struct ocrdma_mr *mr;
+
+ mr = ocrdma_alloc_lkey(ibpd, acc, 0, OCRDMA_ADDR_CHECK_DISABLE);
+ if (!mr)
+ return ERR_PTR(-ENOMEM);
ocrdma_alloc_lkey does not return NULL on error.
Good catch! Even more to the point, ocrdma_alloc_lkey() doesn't return a
struct ocrdma_mr* on success. So this function is totally broken.
- R.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
(Resent becasue the first one got lost)
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
+
+int ocrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
+ struct ib_send_wr **bad_wr)
+{
+ int status = 0;
+ struct ocrdma_qp *qp = get_ocrdma_qp(ibqp);
+ struct ocrdma_hdr_wqe *hdr;
+ unsigned long flags;
+
+ spin_lock_irqsave(qp-q_lock, flags);
+ if (qp-state != OCRDMA_QPS_RTS qp-state != OCRDMA_QPS_SQD) {
+ spin_unlock_irqrestore(qp-q_lock, flags);
+ return -EINVAL;
+ }
There, and in several places in this function, you return an error
without setting bad_wr.
Frank.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
From: Parav Pandit parav.pan...@emulex.com
+
+int ocrdma_mbx_dealloc_lkey(struct ocrdma_dev *dev, int fr_mr, u32 lkey)
+{
+ int status = -ENOMEM;
+ struct ocrdma_dealloc_lkey *cmd;
+
+ cmd = ocrdma_init_emb_mqe(OCRDMA_CMD_DEALLOC_LKEY, sizeof(*cmd));
+ if (!cmd)
+ return -ENOMEM;
+ cmd-lkey = lkey;
+ cmd-rsvd_frmr = fr_mr ? 1 : 0;
+ status = ocrdma_mbx_cmd(dev, (struct ocrdma_mqe *)cmd);
+ if (status)
+ goto mbx_err;
+mbx_err:
Missing return before the tag ? Or unnecessary goto.
+ kfree(cmd);
+ return status;
+}
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
+
+int ocrdma_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
+ struct ib_send_wr **bad_wr)
+{
+ int status = 0;
+ struct ocrdma_qp *qp = get_ocrdma_qp(ibqp);
+ struct ocrdma_hdr_wqe *hdr;
+ unsigned long flags;
+
+ spin_lock_irqsave(qp-q_lock, flags);
+ if (qp-state != OCRDMA_QPS_RTS qp-state != OCRDMA_QPS_SQD) {
+ spin_unlock_irqrestore(qp-q_lock, flags);
+ return -EINVAL;
+ }
There, and in several places in this function, you return an error
without setting bad_wr.
Frank.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
Inline.
-Original Message-
From: Roland Dreier [mailto:rol...@purestorage.com]
Sent: Wednesday, March 21, 2012 11:27 PM
To: frank zago
Cc: Pandit, Parav; linux-rdma@vger.kernel.org
Subject: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA
adapter
On Wed, Mar 21, 2012 at 10:42 AM, frank zago
fz...@systemfabricworks.com wrote:
On 03/20/2012 05:39 PM, parav.pan...@emulex.com wrote:
+struct ib_mr *ocrdma_get_dma_mr(struct ib_pd *ibpd, int acc) {
+ struct ocrdma_mr *mr;
+
+ mr = ocrdma_alloc_lkey(ibpd, acc, 0,
+ OCRDMA_ADDR_CHECK_DISABLE);
+ if (!mr)
+ return ERR_PTR(-ENOMEM);
ocrdma_alloc_lkey does not return NULL on error.
I'll fix this part.
Good catch! Even more to the point, ocrdma_alloc_lkey() doesn't return a
struct ocrdma_mr* on success. So this function is totally broken.
It does returns ocrdma_mr* on success. Why do you think it doesn't return?
Below is the snippet.
status = ocrdma_mbx_alloc_lkey(dev, mr-hwmr, pd-id, addr_check);
if (status) {
kfree(mr);
return ERR_PTR(-ENOMEM);
}
mr-pd = pd;
atomic_inc(pd-use_cnt);
mr-ibmr.lkey = mr-hwmr.lkey;
if (mr-hwmr.remote_wr || mr-hwmr.remote_rd)
mr-ibmr.rkey = mr-hwmr.lkey;
return mr;
- R.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
-Original Message-
From: Roland Dreier [mailto:rol...@purestorage.com]
Sent: Wednesday, March 21, 2012 10:04 PM
To: Pandit, Parav
Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
Subject: Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA
adapter
+int ocrdma_qp_state_machine(struct ocrdma_qp *qp, enum ib_qp_state
+new_ib_state,
+ enum ib_qp_state *old_ib_state) {
+ unsigned long flags;
+ int status = 0;
+ enum ocrdma_qp_state new_state;
+ new_state = get_ocrdma_qp_state(new_ib_state);
+
+ /* sync with wqe and rqe posting */
+ spin_lock_irqsave(qp-q_lock, flags);
+
+ if (old_ib_state)
+ *old_ib_state = get_ibqp_state(qp-state);
+ if (new_state == qp-state) {
+ spin_unlock_irqrestore(qp-q_lock, flags);
+ return 1;
+ }
+
+ switch (qp-state) {
+ case OCRDMA_QPS_RST:
+ switch (new_state) {
+ case OCRDMA_QPS_RST:
+ case OCRDMA_QPS_INIT:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_INIT:
+ /* qps: INIT-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_INIT:
+ case OCRDMA_QPS_RTR:
+ break;
+ case OCRDMA_QPS_ERR:
+ ocrdma_flush_qp(qp);
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_RTR:
+ /* qps: RTS-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_RTS:
+ break;
+ case OCRDMA_QPS_ERR:
+ ocrdma_flush_qp(qp);
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_RTS:
+ /* qps: RTS-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_SQD:
+ case OCRDMA_QPS_SQE:
+ break;
+ case OCRDMA_QPS_ERR:
+ ocrdma_flush_qp(qp);
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_SQD:
+ /* qps: SQD-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_RTS:
+ case OCRDMA_QPS_SQE:
+ case OCRDMA_QPS_ERR:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_SQE:
+ switch (new_state) {
+ case OCRDMA_QPS_RTS:
+ case OCRDMA_QPS_ERR:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ case OCRDMA_QPS_ERR:
+ /* qps: ERR-XXX */
+ switch (new_state) {
+ case OCRDMA_QPS_RST:
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ break;
+ default:
+ status = -EINVAL;
+ break;
+ };
+ if (!status)
+ qp-state = new_state;
+
+ spin_unlock_irqrestore(qp-q_lock, flags);
+ return status;
+}
The switch statement here seems to largely reimpliment
ib_modify_qp_is_ok() (which is exported from the rdma midlayer). Is there
some reason that doesn't work for your driver? I'd rather fix / generalize
the
core helper function instead of having something mostly duplicate in a
hardware driver.
Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be
returned during poll_cq() phase.
So state machine is implemented above.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
-Original Message-
From: Roland Dreier [mailto:rol...@purestorage.com]
Sent: Wednesday, March 21, 2012 10:13 PM
To: Pandit, Parav
Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
Subject: Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA
adapter
+struct ib_pd *ocrdma_alloc_pd(struct ib_device *ibdev,
+ struct ib_ucontext *context,
+ struct ib_udata *udata) {
+ struct ocrdma_dev *dev = get_ocrdma_dev(ibdev);
+ struct ocrdma_pd *pd;
+ int status;
+
+ pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return ERR_PTR(-ENOMEM);
+ pd-dev = dev;
+ if (udata context) {
+ pd-dpp_enabled = (dev-nic_info.dev_family ==
+ OCRDMA_GEN2_FAMILY) ? true :
+ false;
Writing
(bool expr) ? true : false
is pretty silly, since it's just an obfuscated way of writing
bool expr
IOW, you can just write
pd-dpp_enabled = (dev-nic_info.dev_family ==
OCRDMA_GEN2_FAMILY);
+int ocrdma_dealloc_pd(struct ib_pd *ibpd) {
+ struct ocrdma_pd *pd = get_ocrdma_pd(ibpd);
+ struct ocrdma_dev *dev = pd-dev;
+ int status;
+ u64 usr_db;
+
+ if (atomic_read(pd-use_cnt)) {
+ ocrdma_err(%s(%d) pd=0x%x is in use.\n,
+ __func__, dev-id, pd-id);
+ status = -EFAULT;
+ goto dealloc_err;
+ }
all of the use_cnt tracking in this driver seems to duplicate what the rdma
midlayer already does... is there any reason we need that in the low-level
hardware driver too, or can we just get rid of the various use_cnt members?
This use_cnt can be removed from low-level hardware driver. I'll remove it.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
It does returns ocrdma_mr* on success. Why do you think it doesn't return? Below is the snippet. Sorry, you're absoutely right. I was looking at ocrdma_mbx_alloc_lkey(). - R. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Wed, Mar 21, 2012 at 12:04 PM, parav.pan...@emulex.com wrote:
+/* mailbox cmd response */
+struct ocrdma_mbx_rsp {
+ u32 subsys_op;
+ u32 status;
+ u32 rsp_len;
+ u32 add_rsp_len;
+} __packed;
...similar comments about only using __packed where you really need it...
This pack is required as it is shared with hardware and need to be of 16
bytes for 32 and 64 bit architecture. Do not wanted to take risk of different
compiler versions. So keeping it packed.
I really think if you can't trust your compiler to lay this structure
out properly,
you have a lot of bigger problems. But whatever, it's not a big deal.
- R.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
-Original Message-
From: Roland Dreier [mailto:rol...@purestorage.com]
Sent: Thursday, March 22, 2012 1:02 AM
To: Pandit, Parav
Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
Subject: Re: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA
adapter
On Wed, Mar 21, 2012 at 12:09 PM, parav.pan...@emulex.com wrote:
Yes. Driver needs to put QP to flush state. So that appropriate CQEs can be
returned during poll_cq() phase.
So state machine is implemented above.
Couldn't you just write
if (ib_modify_qp_is_ok(...)) {
if (new_state == OCRDMA_QPS_ERR)
ocrdma_flush_qp(qp);
} else {
status = -EINVAL;
}
and save about 100 lines of code?
Yes, this can be done. This is one path.
Another path is async_event coming from adapter. So I still need
qp_state_machine function but as you suggested, I'll remove the states and will
have invoke flush_qp() on error.
- R.
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] please pull infiniband.git
On Mon, Mar 19, 2012 at 7:11 PM, Roland Dreier rol...@kernel.org wrote: InfiniBand/RDMA changes for the 3.4 merge window. Nothing big really stands out; by patch count lots of fixes to the mlx4 driver plus some cleanups and fixes to the core and other drivers. Hi Roland, So again, any reason not to merge the RAW QP patches for 3.4? they have been posted few months ago, its two kernel patches and we have Sean's reviewd-by Signature for the core patch (see http://marc.info/?l=linux-rdmam=132693668105264w=2) The cover-letter (patch #0) at http://marc.info/?l=linux-rdmam=132680041724452w=2 [PATCH 1/4] IB/core: add RAW Packet QP type http://marc.info/?l=linux-rdmam=132680053024484w=2 [PATCH 2/4] IB/mlx4: add Raw Packet QP support http://marc.info/?l=linux-rdmam=132680061024504w=2 patches 3 and 4 are for user-space, we can deal with them later. Or. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Wed, Mar 21, 2012 at 12:33:00PM -0700, Roland Dreier wrote:
On Wed, Mar 21, 2012 at 12:04 PM, parav.pan...@emulex.com wrote:
+/* mailbox cmd response */
+struct ocrdma_mbx_rsp {
+ ? ? ? u32 subsys_op;
+ ? ? ? u32 status;
+ ? ? ? u32 rsp_len;
+ ? ? ? u32 add_rsp_len;
+} __packed;
...similar comments about only using __packed where you really need it...
This pack is required as it is shared with hardware and need to be
of 16 bytes for 32 and 64 bit architecture. Do not wanted to take
risk of different compiler versions. So keeping it packed.
I really think if you can't trust your compiler to lay this
structure out properly, you have a lot of bigger problems. But
whatever, it's not a big deal.
Doesn't packed penalize all access to the structure on some
architectures, eg sparc?
A static assert is a better choice than packed...
BUILD_BUG_ON(sizeof(ocrdma_mbx_rsp) != 16);
Jason
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] please pull infiniband.git
On Wed, 21 Mar 2012, Or Gerlitz wrote: So again, any reason not to merge the RAW QP patches for 3.4? they have been posted few months ago, its two kernel patches and we have Sean's reviewd-by Signature for the core patch (see http://marc.info/?l=linux-rdmam=132693668105264w=2) Yes please. Can we can talk about this next week at the OFA conference? -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] please pull infiniband.git
On Wed, Mar 21, 2012 at 11:03 PM, Christoph Lameter c...@linux.com wrote: On Wed, 21 Mar 2012, Or Gerlitz wrote: So again, any reason not to merge the RAW QP patches for 3.4? they have been posted few months ago, its two kernel patches and we have Sean's reviewd-by Signature for the core patch (see http://marc.info/?l=linux-rdmam=132693668105264w=2) Yes please. Can we can talk about this next week at the OFA conference? Christoph, I will not attend, but will love to get any related feedback. Looking on the agenda (https://www.openfabrics.org/images/docs/PR/schedule.pdf) I see that on Wednesday noon there are two sessions dealing with exactly this - User Mode Ethernet Programming and Network Adapter Flow Steering by Tzahi Oved from Mellanox, so be there or be sqaure (e.g myself...) Or. -- To unsubscribe from this list: send the line unsubscribe linux-rdma in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+static inline void *ocrdma_get_eqe(struct ocrdma_eq *eq)
+{
+ return (u8 *)eq-q.va + (eq-q.tail * sizeof(struct ocrdma_eqe));
+}
casts from (void *) to (u8 *) are not needed. This occurs in multiple places.
+enum ib_qp_state get_ibqp_state(enum ocrdma_qp_state qps)
+{
+ switch (qps) {
+ case OCRDMA_QPS_RST:
+ return IB_QPS_RESET;
+ case OCRDMA_QPS_INIT:
+ return IB_QPS_INIT;
+ case OCRDMA_QPS_RTR:
+ return IB_QPS_RTR;
+ case OCRDMA_QPS_RTS:
+ return IB_QPS_RTS;
+ case OCRDMA_QPS_SQD:
+ case OCRDMA_QPS_SQ_DRAINING:
+ return IB_QPS_SQD;
+ case OCRDMA_QPS_SQE:
+ return IB_QPS_SQE;
+ case OCRDMA_QPS_ERR:
+ return IB_QPS_ERR;
+ };
+ return IB_QPS_ERR;
+}
+
+enum ocrdma_qp_state get_ocrdma_qp_state(enum ib_qp_state qps)
+{
+ switch (qps) {
+ case IB_QPS_RESET:
+ return OCRDMA_QPS_RST;
+ case IB_QPS_INIT:
+ return OCRDMA_QPS_INIT;
+ case IB_QPS_RTR:
+ return OCRDMA_QPS_RTR;
+ case IB_QPS_RTS:
+ return OCRDMA_QPS_RTS;
+ case IB_QPS_SQD:
+ return OCRDMA_QPS_SQD;
+ case IB_QPS_SQE:
+ return OCRDMA_QPS_SQE;
+ case IB_QPS_ERR:
+ return OCRDMA_QPS_ERR;
+ };
+ return OCRDMA_QPS_ERR;
+}
+
+static int ocrdma_get_mbx_errno(u32 status)
+{
+ int err_num = -EFAULT;
no need to initialize, since it's set in all cases
+ u8 mbox_status = (status OCRDMA_MBX_RSP_STATUS_MASK)
+ OCRDMA_MBX_RSP_STATUS_SHIFT;
+ u8 add_status = (status OCRDMA_MBX_RSP_ASTATUS_MASK)
+ OCRDMA_MBX_RSP_ASTATUS_SHIFT;
+
+ switch (mbox_status) {
+ case OCRDMA_MBX_STATUS_OOR:
+ case OCRDMA_MBX_STATUS_MAX_QP_EXCEEDS:
+ err_num = -EAGAIN;
+ break;
+
+ case OCRDMA_MBX_STATUS_INVALID_PD:
+ case OCRDMA_MBX_STATUS_INVALID_CQ:
+ case OCRDMA_MBX_STATUS_INVALID_SRQ_ID:
+ case OCRDMA_MBX_STATUS_INVALID_QP:
+ case OCRDMA_MBX_STATUS_INVALID_CHANGE:
+ case OCRDMA_MBX_STATUS_MTU_EXCEEDS:
+ case OCRDMA_MBX_STATUS_INVALID_RNR_NAK_TIMER:
+ case OCRDMA_MBX_STATUS_PKEY_INDEX_INVALID:
+ case OCRDMA_MBX_STATUS_PKEY_INDEX_EXCEEDS:
+ case OCRDMA_MBX_STATUS_ILLEGAL_FIELD:
+ case OCRDMA_MBX_STATUS_INVALID_PBL_ENTRY:
+ case OCRDMA_MBX_STATUS_INVALID_LKEY:
+ case OCRDMA_MBX_STATUS_INVALID_VA:
+ case OCRDMA_MBX_STATUS_INVALID_LENGTH:
+ case OCRDMA_MBX_STATUS_INVALID_FBO:
+ case OCRDMA_MBX_STATUS_INVALID_ACC_RIGHTS:
+ case OCRDMA_MBX_STATUS_INVALID_PBE_SIZE:
+ case OCRDMA_MBX_STATUS_ATOMIC_OPS_UNSUP:
+ case OCRDMA_MBX_STATUS_SRQ_ERROR:
+ case OCRDMA_MBX_STATUS_SRQ_SIZE_UNDERUNS:
+ err_num = -EINVAL;
+ break;
+
+ case OCRDMA_MBX_STATUS_PD_INUSE:
+ case OCRDMA_MBX_STATUS_QP_BOUND:
+ case OCRDMA_MBX_STATUS_MW_STILL_BOUND:
+ case OCRDMA_MBX_STATUS_MW_BOUND:
+ err_num = -EBUSY;
+ break;
+
+ case OCRDMA_MBX_STATUS_RECVQ_RQE_EXCEEDS:
+ case OCRDMA_MBX_STATUS_SGE_RECV_EXCEEDS:
+ case OCRDMA_MBX_STATUS_RQE_EXCEEDS:
+ case OCRDMA_MBX_STATUS_SRQ_LIMIT_EXCEEDS:
+ case OCRDMA_MBX_STATUS_ORD_EXCEEDS:
+ case OCRDMA_MBX_STATUS_IRD_EXCEEDS:
+ case OCRDMA_MBX_STATUS_SENDQ_WQE_EXCEEDS:
+ case OCRDMA_MBX_STATUS_SGE_SEND_EXCEEDS:
+ case OCRDMA_MBX_STATUS_SGE_WRITE_EXCEEDS:
+ err_num = -ENOBUFS;
+ break;
+
+ case OCRDMA_MBX_STATUS_FAILED:
+ switch (add_status) {
+ case OCRDMA_MBX_ADDI_STATUS_INSUFFICIENT_RESOURCES:
+ err_num = -EAGAIN;
+ break;
+ }
+ default:
+ err_num = -EFAULT;
+ }
+ return err_num;
+}
+
+static int ocrdma_get_mbx_cqe_errno(u16 cqe_status)
+{
+ int err_num = -EINVAL;
no need to initialize
+ switch (cqe_status) {
+ case OCRDMA_MBX_CQE_STATUS_INSUFFICIENT_PRIVILEDGES:
+ err_num = -EPERM;
+ break;
+ case OCRDMA_MBX_CQE_STATUS_INVALID_PARAMETER:
+ err_num = -EINVAL;
+ break;
+ case OCRDMA_MBX_CQE_STATUS_INSUFFICIENT_RESOURCES:
+ case OCRDMA_MBX_CQE_STATUS_QUEUE_FLUSHING:
+ err_num = -EAGAIN;
+ break;
+ case OCRDMA_MBX_CQE_STATUS_DMA_FAILED:
+ err_num = -EIO;
+ break;
+ }
+ return err_num;
+}
+static int ocrdma_mbx_create_eq(struct ocrdma_dev *dev, struct ocrdma_eq *eq)
+{
+ int status;
+ struct ocrdma_create_eq_req *cmd = dev-mbx_cmd;
+ struct ocrdma_create_eq_rsp *rsp = dev-mbx_cmd;
+
+ memset(cmd, 0, sizeof(*cmd));
+ ocrdma_init_mch(cmd-req, OCRDMA_CMD_CREATE_EQ, OCRDMA_SUBSYS_COMMON,
+
RE: [PATCH 5/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
+static int ocrdma_inet6addr_event(struct notifier_block *,
+ unsigned long, void *);
+
+static struct notifier_block ocrdma_inet6addr_notifier = {
+ .notifier_call = ocrdma_inet6addr_event
+};
+
+int ocrdma_get_instance(void)
+{
+ int instance = 0;
+
+ /* Assign an unused number */
+ if (!idr_pre_get(ocrdma_dev_id, GFP_KERNEL))
+ return -1;
+ if (idr_get_new(ocrdma_dev_id, NULL, instance))
+ return -1;
+ return instance;
+}
+
+void ocrdma_get_guid(struct ocrdma_dev *dev, u8 *guid)
+{
+ u8 mac_addr[6];
+
+ memcpy(mac_addr[0], dev-nic_info.mac_addr[0], ETH_ALEN);
+ guid[0] = mac_addr[0] ^ 2;
+ guid[1] = mac_addr[1];
+ guid[2] = mac_addr[2];
+ guid[3] = 0xff;
+ guid[4] = 0xfe;
+ guid[5] = mac_addr[3];
+ guid[6] = mac_addr[4];
+ guid[7] = mac_addr[5];
+}
+
+static void ocrdma_build_sgid_mac(union ib_gid *sgid, unsigned char
*mac_addr,
+ bool is_vlan, u16 vlan_id)
+{
+ sgid-global.subnet_prefix = cpu_to_be64(0xfe80LL);
+ sgid-raw[8] = mac_addr[0] ^ 2;
+ sgid-raw[9] = mac_addr[1];
+ sgid-raw[10] = mac_addr[2];
+ if (is_vlan) {
+ sgid-raw[11] = vlan_id 8;
+ sgid-raw[12] = vlan_id 0xff;
+ } else {
+ sgid-raw[11] = 0xff;
+ sgid-raw[12] = 0xfe;
+ }
+ sgid-raw[13] = mac_addr[3];
+ sgid-raw[14] = mac_addr[4];
+ sgid-raw[15] = mac_addr[5];
+}
+
+static void ocrdma_add_sgid(struct ocrdma_dev *dev, unsigned char *mac_addr,
+ bool is_vlan, u16 vlan_id)
+{
+ int i;
+ bool found = false;
+ union ib_gid new_sgid;
+ int free_idx = OCRDMA_MAX_SGID;
+ unsigned long flags;
+
+ memset(ocrdma_zero_sgid, 0, sizeof(union ib_gid));
ocrdma_zero_sgid should already be zeroed
Actually, can't we either use in6addr_any instead? I see that the mlx4 driver
also has a zero gid. So, if we don't want to overload the use of in6addr_any,
we can define gid_zero in ib_core.
+
+ ocrdma_build_sgid_mac(new_sgid, mac_addr, is_vlan, vlan_id);
+
+ spin_lock_irqsave(dev-sgid_lock, flags);
+ for (i = 0; i OCRDMA_MAX_SGID; i++) {
+ if (!memcmp(dev-sgid_tbl[i], ocrdma_zero_sgid,
+ sizeof(union ib_gid))) {
+ /* found free entry */
+ if (!found) {
+ free_idx = i;
+ found = true;
+ break;
+ }
+ } else if (!memcmp(dev-sgid_tbl[i], new_sgid,
+sizeof(union ib_gid))) {
+ /* entry already present, no addition is required. */
+ spin_unlock_irqrestore(dev-sgid_lock, flags);
+ return;
+ }
+ }
+ /* if entry doesn't exist and if table has some space, add entry */
+ if (found)
+ memcpy(dev-sgid_tbl[free_idx], new_sgid,
+sizeof(union ib_gid));
can move memcpy into for loop and remove found flag
+ spin_unlock_irqrestore(dev-sgid_lock, flags);
+}
--
To unsubscribe from this list: send the line unsubscribe linux-rdma in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
