On 19/10/16 6:58 PM, "Johannes Thumshirn" <jthumshirn@xxxxxxx> wrote:
>On Wed, Oct 19, 2016 at 01:01:12AM -0400, manish.rangankar@xxxxxxxxxx
>wrote:
>> From: Manish Rangankar <manish.rangankar@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@xxxxxxxxxx>
>> Signed-off-by: Chad Dupuis <chad.dupuis@xxxxxxxxxx>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@xxxxxxxxxx>
>> Signed-off-by: Arun Easi <arun.easi@xxxxxxxxxx>
>> Signed-off-by: Manish Rangankar <manish.rangankar@xxxxxxxxxx>
>> ---
>
>[...]
>
>> +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
