Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1255986 > unrolled thread

[PATCH v2 27/32] scsi: hisi_sas: add smp protocol support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support John Garry <john.garry@huawei.com> - 2015-10-26 15:10 +0100
    Re: [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support Arnd Bergmann <arnd@arndb.de> - 2015-10-30 15:00 +0100
      Re: [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support John Garry <john.garry@huawei.com> - 2015-10-30 17:30 +0100
        Re: [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support John Garry <john.garry@huawei.com> - 2015-11-02 18:10 +0100
          Re: [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support Arnd Bergmann <arnd@arndb.de> - 2015-11-02 21:40 +0100
            Re: [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support John Garry <john.garry@huawei.com> - 2015-11-03 12:50 +0100
              Re: [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support Arnd Bergmann <arnd@arndb.de> - 2015-11-03 13:40 +0100

#1255986 — [PATCH v2 27/32] scsi: hisi_sas: add smp protocol support

Add support for smp function, which allows devices
attached by expander to be controlled

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas.h       |  3 ++
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 10 +++-
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 93 ++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index e52f5d3..1dc71ee 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -36,6 +36,7 @@
 		(((sizeof(union hisi_sas_command_table)+3)/4)*4)
 
 #define HISI_SAS_MAX_SSP_RESP_SZ (sizeof(struct ssp_frame_hdr) + 1024)
+#define HISI_SAS_MAX_SMP_RESP_SZ 1028
 
 #define HISI_SAS_NAME_LEN 32
 #define HISI_SAS_CTRL_REG_CNT 5
@@ -135,6 +136,8 @@ struct hisi_sas_hw {
 	int (*prep_ssp)(struct hisi_hba *hisi_hba,
 			struct hisi_sas_slot *slot, int is_tmf,
 			struct hisi_sas_tmf_task *tmf);
+	int (*prep_smp)(struct hisi_hba *hisi_hba,
+			struct hisi_sas_slot *slot);
 	int (*slot_complete)(struct hisi_hba *hisi_hba,
 			     struct hisi_sas_slot *slot, int abort);
 	void (*free_device)(struct hisi_hba *hisi_hba,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 88bf72f..a1bfb47 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -110,6 +110,12 @@ void hisi_sas_slot_task_free(struct hisi_hba *hisi_hba, struct sas_task *task,
 }
 EXPORT_SYMBOL_GPL(hisi_sas_slot_task_free);
 
+static int hisi_sas_task_prep_smp(struct hisi_hba *hisi_hba,
+				  struct hisi_sas_slot *slot)
+{
+	return hisi_hba->hw->prep_smp(hisi_hba, slot);
+}
+
 static int hisi_sas_task_prep_ssp(struct hisi_hba *hisi_hba,
 				  struct hisi_sas_slot *slot, int is_tmf,
 				  struct hisi_sas_tmf_task *tmf)
@@ -228,10 +234,12 @@ static int hisi_sas_task_prep(struct sas_task *task, struct hisi_hba *hisi_hba,
 	memset(slot->cmd_hdr, 0, sizeof(struct hisi_sas_cmd_hdr));
 
 	switch (task->task_proto) {
+	case SAS_PROTOCOL_SMP:
+		rc = hisi_sas_task_prep_smp(hisi_hba, slot);
+		break;
 	case SAS_PROTOCOL_SSP:
 		rc = hisi_sas_task_prep_ssp(hisi_hba, slot, is_tmf, tmf);
 		break;
-	case SAS_PROTOCOL_SMP:
 	case SAS_PROTOCOL_SATA:
 	case SAS_PROTOCOL_STP:
 	case SAS_PROTOCOL_SATA | SAS_PROTOCOL_STP:
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index f8c53e7..0a2f97e 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -907,6 +907,79 @@ static int prep_prd_sge_v1_hw(struct hisi_hba *hisi_hba,
 	return 0;
 }
 
+static int prep_smp_v1_hw(struct hisi_hba *hisi_hba,
+			  struct hisi_sas_slot *slot)
+{
+	struct sas_task *task = slot->task;
+	struct hisi_sas_cmd_hdr *hdr = slot->cmd_hdr;
+	struct domain_device *device = task->dev;
+	struct device *dev = &hisi_hba->pdev->dev;
+	struct hisi_sas_port *port = slot->port;
+	struct scatterlist *sg_req, *sg_resp;
+	struct hisi_sas_device *sas_dev = device->lldd_dev;
+	dma_addr_t req_dma_addr;
+	unsigned int req_len, resp_len;
+	int elem, rc;
+
+	/*
+	* DMA-map SMP request, response buffers
+	*/
+	/* req */
+	sg_req = &task->smp_task.smp_req;
+	elem = dma_map_sg(dev, sg_req, 1, DMA_TO_DEVICE);
+	if (!elem)
+		return -ENOMEM;
+	req_len = sg_dma_len(sg_req);
+	req_dma_addr = sg_dma_address(sg_req);
+
+	/* resp */
+	sg_resp = &task->smp_task.smp_resp;
+	elem = dma_map_sg(dev, sg_resp, 1, DMA_FROM_DEVICE);
+	if (!elem) {
+		rc = -ENOMEM;
+		goto err_out_req;
+	}
+	resp_len = sg_dma_len(sg_resp);
+	if ((req_len & 0x3) || (resp_len & 0x3)) {
+		rc = -EINVAL;
+		goto err_out_resp;
+	}
+
+	/* create header */
+	/* dw0 */
+	hdr->dw0 = cpu_to_le32((port->id << CMD_HDR_PORT_OFF) |
+			       (1 << CMD_HDR_PRIORITY_OFF) | /* high pri */
+			       (1 << CMD_HDR_MODE_OFF) | /* ini mode */
+			       (2 << CMD_HDR_CMD_OFF)); /* smp */
+
+	/* map itct entry */
+	hdr->dw1 = cpu_to_le32(sas_dev->device_id << CMD_HDR_DEVICE_ID_OFF);
+
+	/* dw2 */
+	hdr->dw2 = cpu_to_le32((((req_len-4)/4) << CMD_HDR_CFL_OFF) |
+			       (HISI_SAS_MAX_SMP_RESP_SZ/4 <<
+			       CMD_HDR_MRFL_OFF));
+
+	hdr->transfer_tags = cpu_to_le32(slot->idx << CMD_HDR_IPTT_OFF);
+
+	hdr->cmd_table_addr_lo = cpu_to_le32(lower_32_bits(req_dma_addr));
+	hdr->cmd_table_addr_hi = cpu_to_le32(upper_32_bits(req_dma_addr));
+
+	hdr->sts_buffer_addr_lo =
+			cpu_to_le32(lower_32_bits(slot->status_buffer_dma));
+	hdr->sts_buffer_addr_hi =
+			cpu_to_le32(upper_32_bits(slot->status_buffer_dma));
+
+	return 0;
+
+err_out_resp:
+	dma_unmap_sg(dev, &slot->task->smp_task.smp_resp, 1,
+		     DMA_FROM_DEVICE);
+err_out_req:
+	dma_unmap_sg(dev, &slot->task->smp_task.smp_req, 1,
+		     DMA_TO_DEVICE);
+	return rc;
+}
 
 static int prep_ssp_v1_hw(struct hisi_hba *hisi_hba,
 			  struct hisi_sas_slot *slot, int is_tmf,
@@ -1240,6 +1313,25 @@ static int slot_complete_v1_hw(struct hisi_hba *hisi_hba,
 		sas_ssp_task_response(dev, task, iu);
 		break;
 	}
+	case SAS_PROTOCOL_SMP:
+	{
+		void *to;
+		struct scatterlist *sg_resp = &task->smp_task.smp_resp;
+
+		tstat->stat = SAM_STAT_GOOD;
+		to = kmap_atomic(sg_page(sg_resp));
+
+		dma_unmap_sg(dev, &task->smp_task.smp_resp, 1,
+			     DMA_FROM_DEVICE);
+		dma_unmap_sg(dev, &task->smp_task.smp_req, 1,
+			     DMA_TO_DEVICE);
+		memcpy(to + sg_resp->offset,
+		       slot->status_buffer +
+		       sizeof(struct hisi_sas_err_record),
+		       sg_dma_len(sg_resp));
+		kunmap_atomic(to);
+		break;
+	}
 	case SAS_PROTOCOL_SATA:
 	case SAS_PROTOCOL_STP:
 	case SAS_PROTOCOL_SATA | SAS_PROTOCOL_STP:
@@ -1616,6 +1708,7 @@ static const struct hisi_sas_hw hisi_sas_v1_hw = {
 	.setup_itct = setup_itct_v1_hw,
 	.sl_notify = sl_notify_v1_hw,
 	.free_device = free_device_v1_hw,
+	.prep_smp = prep_smp_v1_hw,
 	.prep_ssp = prep_ssp_v1_hw,
 	.get_free_slot = get_free_slot_v1_hw,
 	.start_delivery = start_delivery_v1_hw,
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1259504

On Monday 26 October 2015 22:14:58 John Garry wrote:

> +       /*
> +       * DMA-map SMP request, response buffers
> +       */
> +       /* req */
> +       sg_req = &task->smp_task.smp_req;
> +       elem = dma_map_sg(dev, sg_req, 1, DMA_TO_DEVICE);
> +       if (!elem)
> +               return -ENOMEM;
> +       req_len = sg_dma_len(sg_req);
> +       req_dma_addr = sg_dma_address(sg_req);

If you only use the first element, could you just use dma_map_single()?

> +       hdr->cmd_table_addr_lo = cpu_to_le32(lower_32_bits(req_dma_addr));
> +       hdr->cmd_table_addr_hi = cpu_to_le32(upper_32_bits(req_dma_addr));
> +
> +       hdr->sts_buffer_addr_lo =
> +                       cpu_to_le32(lower_32_bits(slot->status_buffer_dma));
> +       hdr->sts_buffer_addr_hi =
> +                       cpu_to_le32(upper_32_bits(slot->status_buffer_dma));
> +
> 

I see these a lot in your code. Could you replace this wit

	hdr->cmd_table_addr = cpu_to_le64(req_dma_addr);

and so on? That would be much more readable. Or are the two __le32 variables
swapped? If so, you could add a small helper function like

static inline __le64 cpu_to_le64_wordswapped(u64 val)
{
	return cpu_to_le64(val >> 32 | val << 32);
}

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1259609

On 30/10/2015 13:53, Arnd Bergmann wrote:
> On Monday 26 October 2015 22:14:58 John Garry wrote:
>
>> +       /*
>> +       * DMA-map SMP request, response buffers
>> +       */
>> +       /* req */
>> +       sg_req = &task->smp_task.smp_req;
>> +       elem = dma_map_sg(dev, sg_req, 1, DMA_TO_DEVICE);
>> +       if (!elem)
>> +               return -ENOMEM;
>> +       req_len = sg_dma_len(sg_req);
>> +       req_dma_addr = sg_dma_address(sg_req);
>
> If you only use the first element, could you just use dma_map_single()?
>

Can do. Actually sg_req seems only ever has one element:
expander.c, smp_execute_task()
sg_init_one(&task->smp_task.smp_req, req, req_size);

>> +       hdr->cmd_table_addr_lo = cpu_to_le32(lower_32_bits(req_dma_addr));
>> +       hdr->cmd_table_addr_hi = cpu_to_le32(upper_32_bits(req_dma_addr));
>> +
>> +       hdr->sts_buffer_addr_lo =
>> +                       cpu_to_le32(lower_32_bits(slot->status_buffer_dma));
>> +       hdr->sts_buffer_addr_hi =
>> +                       cpu_to_le32(upper_32_bits(slot->status_buffer_dma));
>> +
>>
>
> I see these a lot in your code. Could you replace this wit
>
> 	hdr->cmd_table_addr = cpu_to_le64(req_dma_addr);
>
This seems reasonable. They are not swapped.

> and so on? That would be much more readable. Or are the two __le32 variables
> swapped? If so, you could add a small helper function like
>
> static inline __le64 cpu_to_le64_wordswapped(u64 val)
> {
> 	return cpu_to_le64(val >> 32 | val << 32);
> }
>
> 	Arnd
>
> .
>

cheers


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260801

On 30/10/2015 16:22, John Garry wrote:
> On 30/10/2015 13:53, Arnd Bergmann wrote:
>> On Monday 26 October 2015 22:14:58 John Garry wrote:
>>
>>> +       /*
>>> +       * DMA-map SMP request, response buffers
>>> +       */
>>> +       /* req */
>>> +       sg_req = &task->smp_task.smp_req;
>>> +       elem = dma_map_sg(dev, sg_req, 1, DMA_TO_DEVICE);
>>> +       if (!elem)
>>> +               return -ENOMEM;
>>> +       req_len = sg_dma_len(sg_req);
>>> +       req_dma_addr = sg_dma_address(sg_req);
>>
>> If you only use the first element, could you just use dma_map_single()?
>>
>
> Can do. Actually sg_req seems only ever has one element:
> expander.c, smp_execute_task()
> sg_init_one(&task->smp_task.smp_req, req, req_size);
>
>
I tried replacing with dma_map_single, but I feel the code is not as 
clean as I need to manually set sg_dma_len() and sg_dma_address():
     req_len = sg_dma_len(sg_req) = sg_req->length;
     sg_dma_address(sg_req) = dma_map_single(dev, sg_virt(sg_req),
  					 req_len, DMA_TO_DEVICE);
     if (dma_mapping_error(dev, sg_dma_address(sg_req)))
          return -ENOMEM;
sg_dma_address(sg_req) is used in another function for unmap.

opinion?
>
>
> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>


cheers,
John

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1260945

On Monday 02 November 2015 17:03:58 John Garry wrote:
> >
> > Can do. Actually sg_req seems only ever has one element:
> > expander.c, smp_execute_task()
> > sg_init_one(&task->smp_task.smp_req, req, req_size);
> >
> >
> I tried replacing with dma_map_single, but I feel the code is not as 
> clean as I need to manually set sg_dma_len() and sg_dma_address():
>      req_len = sg_dma_len(sg_req) = sg_req->length;
>      sg_dma_address(sg_req) = dma_map_single(dev, sg_virt(sg_req),
>                                          req_len, DMA_TO_DEVICE);
>      if (dma_mapping_error(dev, sg_dma_address(sg_req)))
>           return -ENOMEM;
> sg_dma_address(sg_req) is used in another function for unmap.
> 
> opinion?
> 

What I meant was not using a struct scatterlist at all:
replace the 'sg_req' variable with a normal pointer, and then
do

	hdr->cmd_table_addr = cpu_to_le64(dma_map_single(dev, req, len, DMA_TO_DEVICE));

Any reason this won't work?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261434

On 02/11/2015 20:29, Arnd Bergmann wrote:
> On Monday 02 November 2015 17:03:58 John Garry wrote:
>>>
>>> Can do. Actually sg_req seems only ever has one element:
>>> expander.c, smp_execute_task()
>>> sg_init_one(&task->smp_task.smp_req, req, req_size);
>>>
>>>
>> I tried replacing with dma_map_single, but I feel the code is not as
>> clean as I need to manually set sg_dma_len() and sg_dma_address():
>>       req_len = sg_dma_len(sg_req) = sg_req->length;
>>       sg_dma_address(sg_req) = dma_map_single(dev, sg_virt(sg_req),
>>                                           req_len, DMA_TO_DEVICE);
>>       if (dma_mapping_error(dev, sg_dma_address(sg_req)))
>>            return -ENOMEM;
>> sg_dma_address(sg_req) is used in another function for unmap.
>>
>> opinion?
>>
>
> What I meant was not using a struct scatterlist at all:
> replace the 'sg_req' variable with a normal pointer, and then
> do
>
> 	hdr->cmd_table_addr = cpu_to_le64(dma_map_single(dev, req, len, DMA_TO_DEVICE));
>
> Any reason this won't work?
>
> 	Arnd
>
> .
>
There seems to be some misunderstanding. Are you suggesting I change 
sas_smp_task?
./include/scsi/libsas.h
struct sas_smp_task {
	struct scatterlist smp_req;
	struct scatterlist smp_resp;
};

thanks,
John

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261453

On Tuesday 03 November 2015 11:42:30 John Garry wrote:
> >
> There seems to be some misunderstanding. Are you suggesting I change 
> sas_smp_task?
> ./include/scsi/libsas.h
> struct sas_smp_task {
>         struct scatterlist smp_req;
>         struct scatterlist smp_resp;
> };
> 

Ah, no. I had not realized this comes from another file.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web