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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 20 Oct 2016 09:12:40 +0000
From:   "Rangankar, Manish" <Manish.Rangankar@...ium.com>
To:     Johannes Thumshirn <jthumshirn@...e.de>
CC:     "lduncan@...e.com" <lduncan@...e.com>,
        "cleech@...hat.com" <cleech@...hat.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "jejb@...ux.vnet.ibm.com" <jejb@...ux.vnet.ibm.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Mintz, Yuval" <Yuval.Mintz@...ium.com>,
        "Dept-Eng QLogic Storage Upstream" 
        <QLogic-Storage-Upstream@...ium.com>,
        "Javali, Nilesh" <Nilesh.Javali@...ium.com>,
        Adheer Chandravanshi <adheer.chandravanshi@...gic.com>,
        "Dupuis, Chad" <Chad.Dupuis@...ium.com>,
        "Kashyap, Saurav" <Saurav.Kashyap@...ium.com>,
        "Easi, Arun" <Arun.Easi@...ium.com>
Subject: Re: [RFC 5/6] qedi: Add support for iSCSI session management.



On 19/10/16 6:58 PM, "Johannes Thumshirn" <jthumshirn@...e.de> wrote:

>On Wed, Oct 19, 2016 at 01:01:12AM -0400, manish.rangankar@...ium.com
>wrote:
>> From: Manish Rangankar <manish.rangankar@...ium.com>
>> 
>> 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 <nilesh.javali@...ium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@...gic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@...ium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@...ium.com>
>> Signed-off-by: Arun Easi <arun.easi@...ium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@...ium.com>
>> ---
>
>[...]
>
>> +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(&qedi->tasks,
>> +								  cmd->task_id);
>
>No need to cast here, qedi_get_task_mem() returns void *.
>
>[...]
>
>> +	cqe_login_response = &cqe->cqe_common.iscsi_hdr.login_response;
>> +	task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(&qedi->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)) &
>> +					    0xffffffff)));
>
>Is this LISP or C?
>
>> +	QEDI_INFO(&qedi->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) & 0xffffffff)));
>> +
>
>[...]
>
>> +	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(&qedi->tasks,
>> +						      cqe->cqe_solicited.itid);
>
>Again, no cast needed.
>
>[...]
>
>> +	writel(*(u32 *)&dbell, 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(&qedi->tasks,
>>tid);
>
>Cast again.
>
>[...]
>
>> +	fw_task_ctx =
>> +	     (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>
>^^
>
>[...]
>
>> +	fw_task_ctx =
>> +	(struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks, tid);
>
>
>[...]
>
>> +	fw_task_ctx =
>> +	      (struct iscsi_task_context *)qedi_get_task_mem(&qedi->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 all the above review comments in the next revision.

Thanks,
Manish R.