Re: [RFC 5/6] qedi: Add support for iSCSI session management.

[Rangankar, Manish]

On 19/10/16 1:33 PM, "Hannes Reinecke"  wrote:

>On 10/19/2016 07:01 AM, manish.rangan...@cavium.com wrote:
>> From: Manish Rangankar 
>> 
>> This patch adds support for iscsi_transport LLD Login,
>> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
>> and Firmware async event handling support.
>> 
>> Signed-off-by: Nilesh Javali 
>> Signed-off-by: Adheer Chandravanshi 
>> Signed-off-by: Chad Dupuis 
>> Signed-off-by: Saurav Kashyap 
>> Signed-off-by: Arun Easi 
>> Signed-off-by: Manish Rangankar 
>> ---
>>  drivers/scsi/qedi/qedi_fw.c| 1123 
>>  drivers/scsi/qedi/qedi_gbl.h   |   67 ++
>>  drivers/scsi/qedi/qedi_iscsi.c | 1604
>>
>>  drivers/scsi/qedi/qedi_iscsi.h |  228 ++
>>  drivers/scsi/qedi/qedi_main.c  |  164 
>>  5 files changed, 3186 insertions(+)
>>  create mode 100644 drivers/scsi/qedi/qedi_fw.c
>>  create mode 100644 drivers/scsi/qedi/qedi_gbl.h
>>  create mode 100644 drivers/scsi/qedi/qedi_iscsi.c
>>  create mode 100644 drivers/scsi/qedi/qedi_iscsi.h
>> 

--snipped--
>>
>> +static void qedi_process_async_mesg(struct qedi_ctx *qedi,
>> +union iscsi_cqe *cqe,
>> +struct iscsi_task *task,
>> +struct qedi_conn *qedi_conn,
>> +u16 que_idx)
>> +{
>> +struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
>> +struct iscsi_session *session = conn->session;
>> +struct iscsi_async_msg_hdr *cqe_async_msg;
>> +struct iscsi_async *resp_hdr;
>> +u32 scsi_lun[2];
>> +u32 pdu_len, num_bdqs;
>> +char bdq_data[QEDI_BDQ_BUF_SIZE];
>> +unsigned long flags;
>> +
>> +spin_lock_bh(>back_lock);
>> +
>> +cqe_async_msg = >cqe_common.iscsi_hdr.async_msg;
>> +pdu_len = cqe_async_msg->hdr_second_dword &
>> +ISCSI_ASYNC_MSG_HDR_DATA_SEG_LEN_MASK;
>> +num_bdqs = pdu_len / QEDI_BDQ_BUF_SIZE;
>> +
>> +if (cqe->cqe_common.cqe_type == ISCSI_CQE_TYPE_UNSOLICITED) {
>> +spin_lock_irqsave(>hba_lock, flags);
>> +qedi_unsol_pdu_adjust_bdq(qedi, >cqe_unsolicited,
>> +  pdu_len, num_bdqs, bdq_data);
>> +spin_unlock_irqrestore(>hba_lock, flags);
>> +}
>> +
>> +resp_hdr = (struct iscsi_async *)_conn->gen_pdu.resp_hdr;
>> +memset(resp_hdr, 0, sizeof(struct iscsi_hdr));
>> +resp_hdr->opcode = cqe_async_msg->opcode;
>> +resp_hdr->flags = 0x80;
>> +
>> +scsi_lun[0] = cpu_to_be32(cqe_async_msg->lun.lo);
>> +scsi_lun[1] = cpu_to_be32(cqe_async_msg->lun.hi);
>I _think_ we have a SCSI LUN structure ...

Will do.

--snipped--
>> +void qedi_process_iscsi_error(struct qedi_endpoint *ep, struct
>>async_data *data)
>> +{
>> +struct qedi_conn *qedi_conn;
>> +struct qedi_ctx *qedi;
>> +char warn_notice[] = "iscsi_warning";
>> +char error_notice[] = "iscsi_error";
>> +char *message;
>> +int need_recovery = 0;
>> +u32 err_mask = 0;
>> +char msg[64];
>> +
>> +if (!ep)
>> +return;
>> +
>> +qedi_conn = ep->conn;
>> +if (!qedi_conn)
>> +return;
>> +
>> +qedi = ep->qedi;
>> +
>> +QEDI_ERR(>dbg_ctx, "async event iscsi error:0x%x\n",
>> + data->error_code);
>> +
>> +if (err_mask) {
>> +need_recovery = 0;
>> +message = warn_notice;
>> +} else {
>> +need_recovery = 1;
>> +message = error_notice;
>> +}
>> +
>> +switch (data->error_code) {
>> +case ISCSI_STATUS_NONE:
>> +strcpy(msg, "tcp_error none");
>> +break;
>> +case ISCSI_CONN_ERROR_TASK_CID_MISMATCH:
>> +strcpy(msg, "task cid mismatch");
>> +break;
>> +case ISCSI_CONN_ERROR_TASK_NOT_VALID:
>> +strcpy(msg, "invalid task");
>> +break;
>> +case ISCSI_CONN_ERROR_RQ_RING_IS_FULL:
>> +strcpy(msg, "rq ring full");
>> +break;
>> +case ISCSI_CONN_ERROR_CMDQ_RING_IS_FULL:
>> +strcpy(msg, "cmdq ring full");
>> +break;
>> +case ISCSI_CONN_ERROR_HQE_CACHING_FAILED:
>> +strcpy(msg, "sge caching failed");
>> +break;
>> +case ISCSI_CONN_ERROR_HEADER_DIGEST_ERROR:
>> +strcpy(msg, "hdr digest error");
>> +break;
>> +case ISCSI_CONN_ERROR_LOCAL_COMPLETION_ERROR:
>> +strcpy(msg, "local cmpl error");
>> +break;
>> +case ISCSI_CONN_ERROR_DATA_OVERRUN:
>> +strcpy(msg, "invalid task");
>> +break;
>> +case ISCSI_CONN_ERROR_OUT_OF_SGES_ERROR:
>> +strcpy(msg, "out of sge error");
>> +break;
>> +case 

Re: [RFC 5/6] qedi: Add support for iSCSI session management.

[Rangankar, Manish]

On 19/10/16 6:58 PM, "Johannes Thumshirn"  wrote:

>On Wed, Oct 19, 2016 at 01:01:12AM -0400, manish.rangan...@cavium.com
>wrote:
>> From: Manish Rangankar 
>> 
>> This patch adds support for iscsi_transport LLD Login,
>> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
>> and Firmware async event handling support.
>> 
>> Signed-off-by: Nilesh Javali 
>> Signed-off-by: Adheer Chandravanshi 
>> Signed-off-by: Chad Dupuis 
>> Signed-off-by: Saurav Kashyap 
>> Signed-off-by: Arun Easi 
>> Signed-off-by: Manish Rangankar 
>> ---
>
>[...]
>
>> +void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
>> +{
>> +struct scsi_cmnd *sc = cmd->scsi_cmd;
>> +
>> +if (cmd->io_tbl.sge_valid && sc) {
>> +scsi_dma_unmap(sc);
>> +cmd->io_tbl.sge_valid = 0;
>> +}
>> +}
>
>Maybe set sge_valid to 0 and then call scsi_dma_unmap(). I don't know if
>it's
>really racy but it looks like it is.
>
>[...]
>
>> +static void qedi_process_text_resp(struct qedi_ctx *qedi,
>> +   union iscsi_cqe *cqe,
>> +   struct iscsi_task *task,
>> +   struct qedi_conn *qedi_conn)
>> +{
>> +struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
>> +struct iscsi_session *session = conn->session;
>> +struct iscsi_task_context *task_ctx;
>> +struct iscsi_text_rsp *resp_hdr_ptr;
>> +struct iscsi_text_response_hdr *cqe_text_response;
>> +struct qedi_cmd *cmd;
>> +int pld_len;
>> +u32 *tmp;
>> +
>> +cmd = (struct qedi_cmd *)task->dd_data;
>> +task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(>tasks,
>> +  cmd->task_id);
>
>No need to cast here, qedi_get_task_mem() returns void *.
>
>[...]
>
>> +cqe_login_response = >cqe_common.iscsi_hdr.login_response;
>> +task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(>tasks,
>> +  cmd->task_id);
>
>Same here.
>
>[...]
>
>> +}
>> +
>> +pbl = (struct scsi_bd *)qedi->bdq_pbl;
>> +pbl += (qedi->bdq_prod_idx % qedi->rq_num_entries);
>> +pbl->address.hi =
>> +  cpu_to_le32((u32)(((u64)(qedi->bdq[idx].buf_dma)) >> 32));
>> +pbl->address.lo =
>> +cpu_to_le32(((u32)(((u64)(qedi->bdq[idx].buf_dma)) &
>> +0x)));
>
>Is this LISP or C?
>
>> +QEDI_INFO(>dbg_ctx, QEDI_LOG_CONN,
>> +  "pbl [0x%p] pbl->address hi [0x%llx] lo [0x%llx] idx [%d]\n",
>> +  pbl, pbl->address.hi, pbl->address.lo, idx);
>> +pbl->opaque.hi = cpu_to_le32((u32)(((u64)0) >> 32));
>
>Isn't this plain pbl->opaque.hi = 0; ?
>
>> +pbl->opaque.lo = cpu_to_le32(((u32)(((u64)idx) & 0x)));
>> +
>
>[...]
>
>> +switch (comp_type) {
>> +case ISCSI_CQE_TYPE_SOLICITED:
>> +case ISCSI_CQE_TYPE_SOLICITED_WITH_SENSE:
>> +fw_task_ctx =
>> +  (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
>> +  cqe->cqe_solicited.itid);
>
>Again, no cast needed.
>
>[...]
>
>> +writel(*(u32 *), qedi_conn->ep->p_doorbell);
>> +/* Make sure fw idx is coherent */
>> +wmb();
>> +mmiowb();
>
>Isn't either wmb() or mmiowb() enough?
>
>[..]
>
>> +
>> +fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
>>tid);
>
>Cast again.
>
>[...]
>
>> +fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
>>tid);
>
>^^
>
>[...]
>
>> +fw_task_ctx =
>> +(struct iscsi_task_context *)qedi_get_task_mem(>tasks, tid);
>
>
>[...]
>
>> +fw_task_ctx =
>> +  (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
>>tid);
>> +
>
>[...]
>
>> +
>> +qedi = (struct qedi_ctx *)iscsi_host_priv(shost);
>
>Same goes for iscsi_host_priv();
>
>[...]
>
>> +ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
>> +   ((qedi_ep->state ==
>> +EP_STATE_OFLDCONN_FAILED) ||
>> +(qedi_ep->state ==
>> +EP_STATE_OFLDCONN_COMPL)),
>> +msecs_to_jiffies(timeout_ms));
>
>Maybe:
>#define QEDI_OLDCON_STATE(q) ((q)->state == EP_STATE_OFLDCONN_FAILED || \
>   (q)->state == EP_STATE_OFLDCONN_COMPL)
>
>ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
>   QEDI_OLDCON_STATE(qedi_ep),
>   msec_to_jiffies(timeout_ms));
>
>But that could be just me hating linewraps.
>
>[...]

We will address 

Re: [RFC 5/6] qedi: Add support for iSCSI session management.

[Johannes Thumshirn]

On Wed, Oct 19, 2016 at 01:01:12AM -0400, manish.rangan...@cavium.com wrote:
> From: Manish Rangankar 
> 
> This patch adds support for iscsi_transport LLD Login,
> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
> and Firmware async event handling support.
> 
> Signed-off-by: Nilesh Javali 
> Signed-off-by: Adheer Chandravanshi 
> Signed-off-by: Chad Dupuis 
> Signed-off-by: Saurav Kashyap 
> Signed-off-by: Arun Easi 
> Signed-off-by: Manish Rangankar 
> ---

[...]

> +void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
> +{
> + struct scsi_cmnd *sc = cmd->scsi_cmd;
> +
> + if (cmd->io_tbl.sge_valid && sc) {
> + scsi_dma_unmap(sc);
> + cmd->io_tbl.sge_valid = 0;
> + }
> +}

Maybe set sge_valid to 0 and then call scsi_dma_unmap(). I don't know if it's
really racy but it looks like it is.

[...]

> +static void qedi_process_text_resp(struct qedi_ctx *qedi,
> +union iscsi_cqe *cqe,
> +struct iscsi_task *task,
> +struct qedi_conn *qedi_conn)
> +{
> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
> + struct iscsi_session *session = conn->session;
> + struct iscsi_task_context *task_ctx;
> + struct iscsi_text_rsp *resp_hdr_ptr;
> + struct iscsi_text_response_hdr *cqe_text_response;
> + struct qedi_cmd *cmd;
> + int pld_len;
> + u32 *tmp;
> +
> + cmd = (struct qedi_cmd *)task->dd_data;
> + task_ctx = (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
> +   cmd->task_id);

No need to cast here, qedi_get_task_mem() returns void *.

[...]

> + cqe_login_response = >cqe_common.iscsi_hdr.login_response;
> + task_ctx = (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
> +   cmd->task_id);

Same here.

[...]

> + }
> +
> + pbl = (struct scsi_bd *)qedi->bdq_pbl;
> + pbl += (qedi->bdq_prod_idx % qedi->rq_num_entries);
> + pbl->address.hi =
> +   cpu_to_le32((u32)(((u64)(qedi->bdq[idx].buf_dma)) >> 32));
> + pbl->address.lo =
> + cpu_to_le32(((u32)(((u64)(qedi->bdq[idx].buf_dma)) &
> + 0x)));

Is this LISP or C?

> + QEDI_INFO(>dbg_ctx, QEDI_LOG_CONN,
> +   "pbl [0x%p] pbl->address hi [0x%llx] lo [0x%llx] idx [%d]\n",
> +   pbl, pbl->address.hi, pbl->address.lo, idx);
> + pbl->opaque.hi = cpu_to_le32((u32)(((u64)0) >> 32));

Isn't this plain pbl->opaque.hi = 0; ?

> + pbl->opaque.lo = cpu_to_le32(((u32)(((u64)idx) & 0x)));
> +

[...]

> + switch (comp_type) {
> + case ISCSI_CQE_TYPE_SOLICITED:
> + case ISCSI_CQE_TYPE_SOLICITED_WITH_SENSE:
> + fw_task_ctx =
> +   (struct iscsi_task_context *)qedi_get_task_mem(>tasks,
> +   cqe->cqe_solicited.itid);

Again, no cast needed.

[...]

> + writel(*(u32 *), qedi_conn->ep->p_doorbell);
> + /* Make sure fw idx is coherent */
> + wmb();
> + mmiowb();

Isn't either wmb() or mmiowb() enough?

[..]

> +
> + fw_task_ctx =
> +  (struct iscsi_task_context *)qedi_get_task_mem(>tasks, tid);

Cast again.

[...]

> + fw_task_ctx =
> +  (struct iscsi_task_context *)qedi_get_task_mem(>tasks, tid);

^^

[...]

> + fw_task_ctx =
> + (struct iscsi_task_context *)qedi_get_task_mem(>tasks, tid);


[...]

> + fw_task_ctx =
> +   (struct iscsi_task_context *)qedi_get_task_mem(>tasks, tid);
> +

[...]

> +
> + qedi = (struct qedi_ctx *)iscsi_host_priv(shost);

Same goes for iscsi_host_priv();

[...]

> + ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
> +((qedi_ep->state ==
> + EP_STATE_OFLDCONN_FAILED) ||
> + (qedi_ep->state ==
> + EP_STATE_OFLDCONN_COMPL)),
> + msecs_to_jiffies(timeout_ms));

Maybe:
#define QEDI_OLDCON_STATE(q) ((q)->state == EP_STATE_OFLDCONN_FAILED || \
(q)->state == EP_STATE_OFLDCONN_COMPL)

ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
QEDI_OLDCON_STATE(qedi_ep),
msec_to_jiffies(timeout_ms));

But that could be just me hating linewraps.

[...]

Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 

Re: [RFC 5/6] qedi: Add support for iSCSI session management.

[Hannes Reinecke]

On 10/19/2016 07:01 AM, manish.rangan...@cavium.com wrote:
> From: Manish Rangankar 
> 
> This patch adds support for iscsi_transport LLD Login,
> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
> and Firmware async event handling support.
> 
> Signed-off-by: Nilesh Javali 
> Signed-off-by: Adheer Chandravanshi 
> Signed-off-by: Chad Dupuis 
> Signed-off-by: Saurav Kashyap 
> Signed-off-by: Arun Easi 
> Signed-off-by: Manish Rangankar 
> ---
>  drivers/scsi/qedi/qedi_fw.c| 1123 
>  drivers/scsi/qedi/qedi_gbl.h   |   67 ++
>  drivers/scsi/qedi/qedi_iscsi.c | 1604 
> 
>  drivers/scsi/qedi/qedi_iscsi.h |  228 ++
>  drivers/scsi/qedi/qedi_main.c  |  164 
>  5 files changed, 3186 insertions(+)
>  create mode 100644 drivers/scsi/qedi/qedi_fw.c
>  create mode 100644 drivers/scsi/qedi/qedi_gbl.h
>  create mode 100644 drivers/scsi/qedi/qedi_iscsi.c
>  create mode 100644 drivers/scsi/qedi/qedi_iscsi.h
> 
> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
> new file mode 100644
> index 000..a820785
> --- /dev/null
> +++ b/drivers/scsi/qedi/qedi_fw.c
> @@ -0,0 +1,1123 @@
> +/*
> + * QLogic iSCSI Offload Driver
> + * Copyright (c) 2016 Cavium Inc.
> + *
> + * This software is available under the terms of the GNU General Public 
> License
> + * (GPL) Version 2, available from the file COPYING in the main directory of
> + * this source tree.
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include "qedi.h"
> +#include "qedi_iscsi.h"
> +#include "qedi_gbl.h"
> +
> +static int qedi_send_iscsi_tmf(struct qedi_conn *qedi_conn,
> +struct iscsi_task *mtask);
> +
> +void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
> +{
> + struct scsi_cmnd *sc = cmd->scsi_cmd;
> +
> + if (cmd->io_tbl.sge_valid && sc) {
> + scsi_dma_unmap(sc);
> + cmd->io_tbl.sge_valid = 0;
> + }
> +}
> +
> +static void qedi_process_logout_resp(struct qedi_ctx *qedi,
> +  union iscsi_cqe *cqe,
> +  struct iscsi_task *task,
> +  struct qedi_conn *qedi_conn)
> +{
> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
> + struct iscsi_logout_rsp *resp_hdr;
> + struct iscsi_session *session = conn->session;
> + struct iscsi_logout_response_hdr *cqe_logout_response;
> + struct qedi_cmd *cmd;
> +
> + cmd = (struct qedi_cmd *)task->dd_data;
> + cqe_logout_response = >cqe_common.iscsi_hdr.logout_response;
> + spin_lock(>back_lock);
> + resp_hdr = (struct iscsi_logout_rsp *)_conn->gen_pdu.resp_hdr;
> + memset(resp_hdr, 0, sizeof(struct iscsi_hdr));
> + resp_hdr->opcode = cqe_logout_response->opcode;
> + resp_hdr->flags = cqe_logout_response->flags;
> + resp_hdr->hlength = 0;
> +
> + resp_hdr->itt = build_itt(cqe->cqe_solicited.itid, conn->session->age);
> + resp_hdr->statsn = cpu_to_be32(cqe_logout_response->stat_sn);
> + resp_hdr->exp_cmdsn = cpu_to_be32(cqe_logout_response->exp_cmd_sn);
> + resp_hdr->max_cmdsn = cpu_to_be32(cqe_logout_response->max_cmd_sn);
> +
> + resp_hdr->t2wait = cpu_to_be32(cqe_logout_response->time2wait);
> + resp_hdr->t2retain = cpu_to_be32(cqe_logout_response->time2retain);
> +
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_TID,
> +   "Freeing tid=0x%x for cid=0x%x\n",
> +   cmd->task_id, qedi_conn->iscsi_conn_id);
> +
> + if (likely(cmd->io_cmd_in_list)) {
> + cmd->io_cmd_in_list = false;
> + list_del_init(>io_cmd);
> + qedi_conn->active_cmd_count--;
> + } else {
> + QEDI_INFO(>dbg_ctx, QEDI_LOG_INFO,
> +   "Active cmd list node already deleted, tid=0x%x, 
> cid=0x%x, io_cmd_node=%p\n",
> +   cmd->task_id, qedi_conn->iscsi_conn_id,
> +   >io_cmd);
> + }
> +
> + cmd->state = RESPONSE_RECEIVED;
> + qedi_clear_task_idx(qedi, cmd->task_id);
> + __iscsi_complete_pdu(conn, (struct iscsi_hdr *)resp_hdr, NULL, 0);
> +
> + spin_unlock(>back_lock);
> +}
> +
> +static void qedi_process_text_resp(struct qedi_ctx *qedi,
> +union iscsi_cqe *cqe,
> +struct iscsi_task *task,
> +struct qedi_conn *qedi_conn)
> +{
> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
> + struct iscsi_session *session = conn->session;
> + struct iscsi_task_context *task_ctx;
> + struct iscsi_text_rsp *resp_hdr_ptr;
> + struct iscsi_text_response_hdr *cqe_text_response;
> + struct qedi_cmd *cmd;
> + int pld_len;
> + u32 *tmp;
> +
> + cmd = (struct