Messages in this thread | | | From | John Garry <> | Subject | Re: [PATCH v3 4/4] scsi: hisi_sas: Add support for DIF feature for v3 hw | Date | Tue, 4 Dec 2018 12:16:54 +0000 |
| |
On 04/12/2018 04:12, Martin K. Petersen wrote: > > John, >
Hi Martin,
Thanks for checking.
>> +static int hisi_sas_dif_dma_map(struct hisi_hba *hisi_hba, >> + int *n_elem_dif, struct sas_task *task) >> +{ >> + struct device *dev = hisi_hba->dev; >> + struct sas_ssp_task *ssp_task; >> + struct scsi_cmnd *scsi_cmnd; >> + int rc; >> + >> + if (task->num_scatter) { >> + ssp_task = &task->ssp_task; >> + scsi_cmnd = ssp_task->cmd; >> + >> + if (scsi_prot_sg_count(scsi_cmnd)) { >> + *n_elem_dif = dma_map_sg(dev, >> + scsi_prot_sglist(scsi_cmnd), >> + scsi_prot_sg_count(scsi_cmnd), >> + task->data_dir); >> + > > If you're only supporting DIF there is no DMA mapping or unmapping since > the PI is generated by or verified by the HBA. No additional data is > transferred to/from host memory. The protection scatterlist will be > NULL.
So I was bit rushed in dropping DIX support...
> >> + switch (prot_op) { >> + case SCSI_PROT_READ_INSERT: >> + prot->dw0 |= T10_INSRT_EN_MSK; >> + prot->lbrtgv = lbrt_chk_val; >> + break; >> + case SCSI_PROT_READ_STRIP: >> + prot->dw0 |= (T10_RMV_EN_MSK | T10_CHK_EN_MSK); >> + prot->lbrtcv = lbrt_chk_val; >> + if (prot_type == SCSI_PROT_DIF_TYPE1) >> + prot->dw4 |= (0xc << 16); >> + else if (prot_type == SCSI_PROT_DIF_TYPE3) >> + prot->dw4 |= (0xfc << 16); >> + break; >> + case SCSI_PROT_READ_PASS: >> + prot->dw0 |= T10_CHK_EN_MSK; >> + prot->lbrtcv = lbrt_chk_val; >> + if (prot_type == SCSI_PROT_DIF_TYPE1) >> + prot->dw4 |= (0xc << 16); >> + else if (prot_type == SCSI_PROT_DIF_TYPE3) >> + prot->dw4 |= (0xfc << 16); >> + break; >> + case SCSI_PROT_WRITE_INSERT: >> + prot->dw0 |= T10_INSRT_EN_MSK; >> + prot->lbrtgv = lbrt_chk_val; >> + break; >> + case SCSI_PROT_WRITE_STRIP: >> + prot->dw0 |= (T10_RMV_EN_MSK | T10_CHK_EN_MSK); >> + prot->lbrtcv = lbrt_chk_val; >> + break; >> + case SCSI_PROT_WRITE_PASS: >> + prot->dw0 |= T10_CHK_EN_MSK; >> + prot->lbrtcv = lbrt_chk_val; >> + if (prot_type == SCSI_PROT_DIF_TYPE1) >> + prot->dw4 |= (0xc << 16); >> + else if (prot_type == SCSI_PROT_DIF_TYPE3) >> + prot->dw4 |= (0xfc << 16); >> + break; >> + default: >> + WARN(1, "prot_op(0x%x) is not valid\n", prot_op); >> + break; >> + } > > DIF is WRITE_INSERT/READ_STRIP operations only. >
as above
>> + if ((prot_op == SCSI_PROT_READ_INSERT) || >> + (prot_op == SCSI_PROT_WRITE_INSERT) || >> + (prot_op == SCSI_PROT_WRITE_PASS) || >> + (prot_op == SCSI_PROT_READ_PASS)) { >> + unsigned int interval = scsi_prot_interval(scsi_cmnd); >> + unsigned int ilog2_interval = ilog2(interval); >> + >> + len = (task->total_xfer_len >> ilog2_interval) * 8; >> + } > > if (scmd->prot_flags & SCSI_PROT_TRANSFER_PI) { > >> + .sg_prot_tablesize = HISI_SAS_SGE_PAGE_CNT, > > Also not required if you're only doing DIF. There is no protection > scatterlist.
and again.
> > Anyway. Instead of throwing the DIX stuff away, let's figure out what > the problem is. >
So I just did not want to upstream support for a feature which seems to not be working.
Are you happy for us to make DIX support in this driver "hisi_sas DIX experimental" short term?
Regardless, we can discuss figuring out any SCSI MQ vs DIX issue in "DIF/DIX issue related to config CONFIG_SCSI_MQ_DEFAULT".
Cheers, John
| |