diff mbox series

[v4,2/5] scsi: hisi_sas: Relocate some code to reduce complexity

Message ID 1544103284-100497-3-git-send-email-john.garry@huawei.com (mailing list archive)
State Superseded
Headers show
Series hisi_sas: DIF support | expand

Commit Message

John Garry Dec. 6, 2018, 1:34 p.m. UTC 
From: Xiang Chen <chenxiang66@hisilicon.com>

Relocate the codes related to dma_map/unmap in hisi_sas_task_prep()
to reduce complexity, with a view to add DIF/DIX support.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 146 +++++++++++++++++++++-------------
 1 file changed, 90 insertions(+), 56 deletions(-)

Comments

Johannes Thumshirn Dec. 6, 2018, 2:17 p.m. UTC | #1 
On 06/12/2018 14:34, John Garry wrote:
[...]
> +static void hisi_sas_dma_unmap(struct hisi_hba *hisi_hba,
> +			       struct sas_task *task, int n_elem,
> +			       int n_elem_req, int n_elem_resp)
> +{
> +	struct device *dev = hisi_hba->dev;
> +
> +	if (!sas_protocol_ata(task->task_proto)) {
> +		if (task->num_scatter) {
> +			if (n_elem)
> +				dma_unmap_sg(dev, task->scatter,
> +					     task->num_scatter,
> +					     task->data_dir);
> +		} else if (task->task_proto & SAS_PROTOCOL_SMP) {
> +			if (n_elem_req)
> +				dma_unmap_sg(dev, &task->smp_task.smp_req,
> +					     1, DMA_TO_DEVICE);
> +			if (n_elem_resp)
> +				dma_unmap_sg(dev, &task->smp_task.smp_resp,
> +					     1, DMA_FROM_DEVICE);
> +		}
> +	}

	if (sas_protocol_ata(task->task_proto))
		return;

Would save you a level of indentation and make the above more readable.
John Garry Dec. 6, 2018, 3:37 p.m. UTC | #2 
On 06/12/2018 14:17, Johannes Thumshirn wrote:
> On 06/12/2018 14:34, John Garry wrote:
> [...]
>> +static void hisi_sas_dma_unmap(struct hisi_hba *hisi_hba,
>> +			       struct sas_task *task, int n_elem,
>> +			       int n_elem_req, int n_elem_resp)
>> +{
>> +	struct device *dev = hisi_hba->dev;
>> +
>> +	if (!sas_protocol_ata(task->task_proto)) {
>> +		if (task->num_scatter) {
>> +			if (n_elem)
>> +				dma_unmap_sg(dev, task->scatter,
>> +					     task->num_scatter,
>> +					     task->data_dir);
>> +		} else if (task->task_proto & SAS_PROTOCOL_SMP) {
>> +			if (n_elem_req)
>> +				dma_unmap_sg(dev, &task->smp_task.smp_req,
>> +					     1, DMA_TO_DEVICE);
>> +			if (n_elem_resp)
>> +				dma_unmap_sg(dev, &task->smp_task.smp_resp,
>> +					     1, DMA_FROM_DEVICE);
>> +		}
>> +	}
>
> 	if (sas_protocol_ata(task->task_proto))
> 		return;
>
> Would save you a level of indentation and make the above more readable.
>
>

Hi Johannes,

Whilst I agree with the idea, the current approach makes the function 
more symmetic with its mapping buddy, hisi_sas_dma_map():

static void hisi_sas_dma_unmap(struct hisi_hba *hisi_hba,
                    struct sas_task *task, int n_elem,
                    int n_elem_req, int n_elem_resp)
{
     struct device *dev = hisi_hba->dev;

     if (!sas_protocol_ata(task->task_proto)) {
         if (task->num_scatter) {
             if (n_elem)
                 dma_unmap_sg(dev, task->scatter,

	...
}

static int hisi_sas_dma_map(struct hisi_hba *hisi_hba,
                 struct sas_task *task, int *n_elem,
                 int *n_elem_req, int *n_elem_resp)
{
     struct device *dev = hisi_hba->dev;
     int rc;

     if (sas_protocol_ata(task->task_proto)) {
         *n_elem = task->num_scatter;
     } else {
         unsigned int req_len, resp_len;

         if (task->num_scatter) {
             *n_elem = dma_map_sg(dev, task->scatter,
                          task->num_scatter, task->data_dir);
    		...

}

which is important. Let me know if you disagree and I can change it.

Thanks,
John
Johannes Thumshirn Dec. 6, 2018, 4:20 p.m. UTC | #3 
On 06/12/2018 16:37, John Garry wrote:
> which is important. Let me know if you disagree and I can change it.

Sure, it's your driver. It was just because the patch is even titled
"Relocate some code to reduce complexity", so I thought of reducing the
complexity for readers even further (like you don't need the line wrap
at 80 chars, and so on).

Byte,
	Johannes
John Garry Dec. 7, 2018, 10:07 a.m. UTC | #4 
On 06/12/2018 16:20, Johannes Thumshirn wrote:
> On 06/12/2018 16:37, John Garry wrote:
>> which is important. Let me know if you disagree and I can change it.
>
> Sure, it's your driver. It was just because the patch is even titled
> "Relocate some code to reduce complexity", so I thought of reducing the
> complexity for readers even further (like you don't need the line wrap
> at 80 chars, and so on).
>
> Byte,
> 	Johannes
>

I would rather not change if you don't mind. When we say "reduce 
complexity", we are talking about moving the DMA mapping code from the 
task prep function, as, when we add the DIX-related DMA mapping code, 
leaving all the DMA mapping code in the task prep function would make it 
a monster.

Thanks,
John
Johannes Thumshirn Dec. 7, 2018, 10:53 a.m. UTC | #5 
On 07/12/2018 11:07, John Garry wrote:
> On 06/12/2018 16:20, Johannes Thumshirn wrote:
>> On 06/12/2018 16:37, John Garry wrote:
>>> which is important. Let me know if you disagree and I can change it.
>>
>> Sure, it's your driver. It was just because the patch is even titled
>> "Relocate some code to reduce complexity", so I thought of reducing the
>> complexity for readers even further (like you don't need the line wrap
>> at 80 chars, and so on).
>>
>> Byte,
>>     Johannes
>>
> 
> I would rather not change if you don't mind. When we say "reduce
> complexity", we are talking about moving the DMA mapping code from the
> task prep function, as, when we add the DIX-related DMA mapping code,
> leaving all the DMA mapping code in the task prep function would make it
> a monster.

Sure
diff mbox series

Patch

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index c39c91c..95350fd 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -296,6 +296,90 @@  static void hisi_sas_task_prep_abort(struct hisi_hba *hisi_hba,
 			device_id, abort_flag, tag_to_abort);
 }
 
+static void hisi_sas_dma_unmap(struct hisi_hba *hisi_hba,
+			       struct sas_task *task, int n_elem,
+			       int n_elem_req, int n_elem_resp)
+{
+	struct device *dev = hisi_hba->dev;
+
+	if (!sas_protocol_ata(task->task_proto)) {
+		if (task->num_scatter) {
+			if (n_elem)
+				dma_unmap_sg(dev, task->scatter,
+					     task->num_scatter,
+					     task->data_dir);
+		} else if (task->task_proto & SAS_PROTOCOL_SMP) {
+			if (n_elem_req)
+				dma_unmap_sg(dev, &task->smp_task.smp_req,
+					     1, DMA_TO_DEVICE);
+			if (n_elem_resp)
+				dma_unmap_sg(dev, &task->smp_task.smp_resp,
+					     1, DMA_FROM_DEVICE);
+		}
+	}
+}
+
+static int hisi_sas_dma_map(struct hisi_hba *hisi_hba,
+			    struct sas_task *task, int *n_elem,
+			    int *n_elem_req, int *n_elem_resp)
+{
+	struct device *dev = hisi_hba->dev;
+	int rc;
+
+	if (sas_protocol_ata(task->task_proto)) {
+		*n_elem = task->num_scatter;
+	} else {
+		unsigned int req_len, resp_len;
+
+		if (task->num_scatter) {
+			*n_elem = dma_map_sg(dev, task->scatter,
+					     task->num_scatter, task->data_dir);
+			if (!*n_elem) {
+				rc = -ENOMEM;
+				goto prep_out;
+			}
+		} else if (task->task_proto & SAS_PROTOCOL_SMP) {
+			*n_elem_req = dma_map_sg(dev, &task->smp_task.smp_req,
+						 1, DMA_TO_DEVICE);
+			if (!*n_elem_req) {
+				rc = -ENOMEM;
+				goto prep_out;
+			}
+			req_len = sg_dma_len(&task->smp_task.smp_req);
+			if (req_len & 0x3) {
+				rc = -EINVAL;
+				goto err_out_dma_unmap;
+			}
+			*n_elem_resp = dma_map_sg(dev, &task->smp_task.smp_resp,
+						  1, DMA_FROM_DEVICE);
+			if (!*n_elem_resp) {
+				rc = -ENOMEM;
+				goto err_out_dma_unmap;
+			}
+			resp_len = sg_dma_len(&task->smp_task.smp_resp);
+			if (resp_len & 0x3) {
+				rc = -EINVAL;
+				goto err_out_dma_unmap;
+			}
+		}
+	}
+
+	if (*n_elem > HISI_SAS_SGE_PAGE_CNT) {
+		dev_err(dev, "task prep: n_elem(%d) > HISI_SAS_SGE_PAGE_CNT",
+			*n_elem);
+		rc = -EINVAL;
+		goto err_out_dma_unmap;
+	}
+	return 0;
+
+err_out_dma_unmap:
+	/* It would be better to call dma_unmap_sg() here, but it's messy */
+	hisi_sas_dma_unmap(hisi_hba, task, *n_elem,
+			   *n_elem_req, *n_elem_resp);
+prep_out:
+	return rc;
+}
+
 static int hisi_sas_task_prep(struct sas_task *task,
 			      struct hisi_sas_dq **dq_pointer,
 			      bool is_tmf, struct hisi_sas_tmf_task *tmf,
@@ -338,49 +422,10 @@  static int hisi_sas_task_prep(struct sas_task *task,
 		return -ECOMM;
 	}
 
-	if (!sas_protocol_ata(task->task_proto)) {
-		unsigned int req_len, resp_len;
-
-		if (task->num_scatter) {
-			n_elem = dma_map_sg(dev, task->scatter,
-					    task->num_scatter, task->data_dir);
-			if (!n_elem) {
-				rc = -ENOMEM;
-				goto prep_out;
-			}
-		} else if (task->task_proto & SAS_PROTOCOL_SMP) {
-			n_elem_req = dma_map_sg(dev, &task->smp_task.smp_req,
-						1, DMA_TO_DEVICE);
-			if (!n_elem_req) {
-				rc = -ENOMEM;
-				goto prep_out;
-			}
-			req_len = sg_dma_len(&task->smp_task.smp_req);
-			if (req_len & 0x3) {
-				rc = -EINVAL;
-				goto err_out_dma_unmap;
-			}
-			n_elem_resp = dma_map_sg(dev, &task->smp_task.smp_resp,
-						 1, DMA_FROM_DEVICE);
-			if (!n_elem_resp) {
-				rc = -ENOMEM;
-				goto err_out_dma_unmap;
-			}
-			resp_len = sg_dma_len(&task->smp_task.smp_resp);
-			if (resp_len & 0x3) {
-				rc = -EINVAL;
-				goto err_out_dma_unmap;
-			}
-		}
-	} else
-		n_elem = task->num_scatter;
-
-	if (n_elem > HISI_SAS_SGE_PAGE_CNT) {
-		dev_err(dev, "task prep: n_elem(%d) > HISI_SAS_SGE_PAGE_CNT",
-			n_elem);
-		rc = -EINVAL;
-		goto err_out_dma_unmap;
-	}
+	rc = hisi_sas_dma_map(hisi_hba, task, &n_elem,
+			      &n_elem_req, &n_elem_resp);
+	if (rc < 0)
+		goto prep_out;
 
 	if (hisi_hba->hw->slot_index_alloc)
 		rc = hisi_hba->hw->slot_index_alloc(hisi_hba, device);
@@ -465,19 +510,8 @@  static int hisi_sas_task_prep(struct sas_task *task,
 err_out_tag:
 	hisi_sas_slot_index_free(hisi_hba, slot_idx);
 err_out_dma_unmap:
-	if (!sas_protocol_ata(task->task_proto)) {
-		if (task->num_scatter) {
-			dma_unmap_sg(dev, task->scatter, task->num_scatter,
-			     task->data_dir);
-		} else if (task->task_proto & SAS_PROTOCOL_SMP) {
-			if (n_elem_req)
-				dma_unmap_sg(dev, &task->smp_task.smp_req,
-					     1, DMA_TO_DEVICE);
-			if (n_elem_resp)
-				dma_unmap_sg(dev, &task->smp_task.smp_resp,
-					     1, DMA_FROM_DEVICE);
-		}
-	}
+	hisi_sas_dma_unmap(hisi_hba, task, n_elem,
+			   n_elem_req, n_elem_resp);
 prep_out:
 	dev_err(dev, "task prep: failed[%d]!\n", rc);
 	return rc;