| Message ID | 1445868903-183817-28-git-send-email-john.garry@huawei.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)